pkg/types: return error instead of crashing when code supplies an incompatible type to types.LoadArgs()

[aaithal]

Summary

types.LoadArgs() does an unsafe typecast for env args key value pairs.
This can cause a panic if the config type fields do not implement
the encoding.TextUnmarshaler interface. This commit modifies it to
do a safe type cast and return an error in such scenarios.

Testing

• Added a new unit test that would panic without this code-change, which passes now
• ./test.sh succeeds as well

Reference

1. Test Panic without the code change

$ go test ./pkg/types/
Running Suite: Types Suite
==========================
Random Seed: 1495483401
Will run 12 of 12 specs

•••••
------------------------------
•! Panic [0.000 seconds]
LoadArgs
/home/ec2-user/workspace/src/github.com/containernetworking/cni/pkg/types/args_test.go:134
  When known arguments are passed and cannot be unmarshalled
  /home/ec2-user/workspace/src/github.com/containernetworking/cni/pkg/types/args_test.go:133
    LoadArgs should fail [It]
    /home/ec2-user/workspace/src/github.com/containernetworking/cni/pkg/types/args_test.go:132

    Test Panicked
    interface conversion: *types.IPNet is not encoding.TextUnmarshaler: missing method UnmarshalText
    /home/ec2-user/go16/go/src/runtime/panic.go:443

    Full Stack Trace
        /home/ec2-user/go16/go/src/runtime/panic.go:443 +0x4e9
    github.com/containernetworking/cni/pkg/types.LoadArgs(0x87cbd0, 0xe, 0x7387a0, 0xc8202522d0, 0x0, 0x0)
        /home/ec2-user/workspace/src/github.com/containernetworking/cni/pkg/types/args.go:89 +0x784
    github.com/containernetworking/cni/pkg/types_test.glob.func4.6.1()
        /home/ec2-user/workspace/src/github.com/containernetworking/cni/pkg/types/args_test.go:128 +0x81
    github.com/containernetworking/cni/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync(0xc820063860, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /home/ec2-user/workspace/src/github.com/containernetworking/cni/pkg/types/types_suite_test.go:26 +0x7c
    testing.tRunner(0xc8201b4ea0, 0xa8b390)
        /home/ec2-user/go16/go/src/testing/testing.go:473 +0x98
    created by testing.RunTests
        /home/ec2-user/go16/go/src/testing/testing.go:582 +0x892

------------------------------
••••••

Summarizing 1 Failure:

[Panic!] LoadArgs When known arguments are passed and cannot be unmarshalled [It] LoadArgs should fail
/home/ec2-user/go16/go/src/runtime/panic.go:443

Ran 12 of 12 Specs in 0.001 seconds
FAIL! -- 11 Passed | 1 Failed | 0 Pending | 0 Skipped --- FAIL: TestTypes (0.00s)
FAIL
FAIL    github.com/containernetworking/cni/pkg/types    0.005s

2. Test succeeds with the code change:

$ go test ./pkg/types/
ok      github.com/containernetworking/cni/pkg/types    0.005s

[bboreham]

What is the use-case where panic is an unsuitable response?

[aaithal]

@bboreham as a plugin developer, it'd be a lot easier to deal with errors than panics when the config parsing logic in the plugin does not handle unmarshalling of textual form of the object. It provides an avenue to plugin custom logic when such errors occur. For example, the plugin might choose to apply defaults or handle it more gracefully with a more meaningful error message to the invoker rather than just emitting a stack trace of the panic. It plays nicely with the existing logic since the return type includes an error already.

[bboreham]

when the config parsing logic in the plugin does not handle unmarshalling of textual form of the object

But this is not what you changed. You changed the case where a plugin developer passes an object into LoadArgs that has fields that do not match the expected interface.

It provides an avenue to plugin custom logic when such errors occur.

@aaithal if you wanted to do that then you'd need a custom error type, not fmt.Errorf.

But I do not buy this at all. You are trying to provide a run-time solution to a logic error that occurred before compile-time.

[aaithal]

You changed the case where a plugin developer passes an object into LoadArgs that has fields that do not match the expected interface.

Since these arguments are set via the CNI_ARGS env var, the plugin developer doesn't have much control over it, right? The user of the plugin would be different than the developer and there can easily be a scenario where a config has a field of type IPNet and the user passes IPNet=10.0.0.2, causing a panic.

You are trying to provide a run-time solution to a logic error that occurred before compile-time.

Since the arguments are passed by the user using CNI_ARGS, it's not a compile-time error IIUC

[bboreham]

In your example, what values can be passed in CNI_ARGS that do not cause an error?

[aaithal]

In your example, what values can be passed in CNI_ARGS that do not cause an error?

If the type is IPNet, it'll always cause a panic because it doesn't implement the Unmarshaler. However, if the type is IPNetWrapper, where the wrapper implements the interface, it won't.

Taking a step back, my thought was that if there's an error that you know can be caused and you can detect it, it's better to detect and return the same rather than a panic as it can be gracefully handled (For example, may be the plugin can cleanup resources if it has created any). If that's not what we want to do here and the intention is to make it panic rather than return an error, I'm fine with that too (just means that I misunderstood the intent).

[bboreham]

You can write code to handle a panic in Go, if you have essential cleanup to do.
Changing one particular line where you saw a panic does not make this any less necessary.

I argue that panic is fine for this case because it is a logic error that will be caught by the developer's testing, not something that would be expected at runtime.

[aaithal]

I'm not arguing that you can't handle panics in go. My point is that error handling is preferred over handling panic's in go.

I argue that panic is fine for this case because it is a logic error that will be caught by the developer's testing, not something that would be expected at runtime.

If your point is that the onus is on the developer to ensure that all of their types implement the text unmarshaller, I agree. However, this only becomes apparent if you take a look at the LoadArgs code. As a plugin developer, you are not aware of that until you, and more importantly your users hit this codepath. At a higher level, the CNI_ARGS hook for field=value seems like an unsafe hook to provide to override config args using type and field names that are specific to the plugin config.

I can make this a typed error instead of fmt.Errorf if you're willing to accept the PR. Otherwise, you can close this PR.

[bboreham]

@aaithal I would be happy with a comment or other documentation making clear the requirements on LoadArgs.

Not happy with complicating the code for developers who don't test all their arguments.

If you can explain the case where a developer can test the expected behaviour and yet get caught by this panic, I'm listening.

[bboreham]

@sosans1357 your messages seem disruptive. Please explain what you are doing or stop.

[squeed]

Given that the json package returns an error, let's do the same thing. However, the error message is a bit more descriptive: see https://play.golang.org/p/ab1Ppkx2C-. Can you make the returned error something like

ARGS: cannot unmarshal in to field IP - type net.IPNet does not implement encoding.TextUnmarshaler

[aaithal]

@squeed thanks for taking a look and for the comment. Do you also want this to be a named error or can I stop after making the error message more descriptive?

[squeed]

I personally don't care if it's an explicit error type, but @bboreham is welcome to disagree :-).

[aaithal]

@squeed I've updated the PR with a more descriptive error message. Can you please take another look? Thanks!

[bboreham]

I did not advocate for a custom error type; I observed that this was one mis-match between the claimed benefit and the code proposed.

Can we adjust the title and the claimed benefit to match whatever is the aim now? Is it just to shorten the plugin developer's debugging process?