Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Check types of values passed to native functions, prevent panics. #728

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rudo-thomas
Copy link

Previously, the native functions would panic if the type of the value
wasn't right. Now, there is a bit of reflection to detect incorrect
types and also to do the conversions to the types the function needs.

@CLAassistant
Copy link

CLAassistant commented Jul 7, 2022

CLA assistant check
All committers have signed the CLA.

@rudo-thomas
Copy link
Author

Maybe @sh0rez could take a look? Thanks.

@rudo-thomas
Copy link
Author

Rebased against v0.25.0.

Previously, the native functions would panic if the type of the value
wasn't right. Now, there is a bit of reflection to detect incorrect
types and also to do the conversions to the types the function needs.
@rudo-thomas
Copy link
Author

rudo-thomas commented Sep 15, 2023

Rebased against current HEAD (7345b9d) and improved a bit.

Edit: The diff is now best viewed with changes in whitespace hidden :)

@rudo-thomas
Copy link
Author

Just to elaborate a bit on what this PR tries to achieve.

  1. Cleaner implementation of native functions: There are no type assertions and hence no possible panics on unexpected code from the user. Everything is checked at runtime, using reflection.

  2. Better error messages. (Though, arguably, the difference is not as stark as it was against older versions of Tanka; newer versions use newer go-jsonnet, which no longer prints a stack trace when a native function panicks -- thanks to my PR :-D)

With native-boom.jsonnet having the following content:

// to be processed by "tk eval"
std.native('parseJson')(4)  // explodes, 4 is not a string

You'd get...

With 0.26.0:

$ tk-0.26.0 eval native-boom.jsonnet 
Error: evaluating jsonnet: RUNTIME ERROR: native function "parseJson" panicked: interface conversion: interface {} is float64, not string
	/home/r/git/inframon-k8s/native-boom.jsonnet:2:1-27	$
	During evaluation	

With this PR:

$ tk-fixed eval native-boom.jsonnet 
Error: evaluating jsonnet: RUNTIME ERROR: parseJson(): argument "json" has unexpected type
	/home/r/git/inframon-k8s/native-boom.jsonnet:2:1-27	$
	During evaluation	

A stacktrace in 0.23.1 (which was around the time when I filed this PR):

$ tk-0.23.1 eval native-boom.jsonnet
Error: evaluating jsonnet: INTERNAL ERROR: (CRASH) interface conversion: interface {} is float64, not string
goroutine 1 [running]:
runtime/debug.Stack()
	/usr/local/go/src/runtime/debug/stack.go:24 +0x65
github.com/google/go-jsonnet.(*VM).Evaluate.func1()
	/go/pkg/mod/github.com/google/go-jsonnet@v0.18.0/vm.go:182 +0x45
panic({0x8b3d40, 0xc00022bce0})
	/usr/local/go/src/runtime/panic.go:884 +0x212
github.com/grafana/tanka/pkg/jsonnet/native.parseJSON.func1({0xc000233600, 0x1, 0xc0002d1210?})
	/drone/src/pkg/jsonnet/native/funcs.go:46 +0xa7
...

@Duologic
Copy link
Member

Could this be a good fit for upstream go-jsonnet? Looks like a potential improvement for jsonnet.NativeFunction.

@rudo-thomas
Copy link
Author

That's a good point. It'd have to be a bit more generic on the types side I guess.

I'll prepare something and ask the go-jsonnet people...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants