Add nil check when applying variadic client options

[rajathagasthya]

All of the Create, List, Update and Delete client methods are variadic in that it takes any number of options. For example:

Create(ctx context.Context, obj runtime.Object, opts ...client.CreateOption) error

It's too easy to pass nil as the last argument for these methods and then it blows up with a NPE in ApplyOptions method. Example:

controller-runtime/pkg/client/options.go

Line 141 in 19e72eb

opt.ApplyToCreate(o)

There needs to be a nil check when options are applied.

[vincepri]

/kind cleanup
/milestone v0.5.x
/help
/good-first-issue
/priority backlog

[k8s-ci-robot]

@vincepri:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/kind cleanup
/milestone v0.5.x
/help
/good-first-issue
/priority backlog

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[boddumanohar]

@rajathagasthya its not possible to pass nil as a last argument to variadic function

package main

import (
	"fmt"
)

func printStrings(s ...string) {
	for _, s := range s {
		fmt.Println(s)
	}
}

func main() {
	printStrings("alice", "bob", nil)
}

https://play.golang.org/p/8iOacL4kOvX

this example will throw an error.

./prog.go:14:24: cannot use nil as type string in argument to printStrings

[rajathagasthya]

@boddumanohar CreateOption (and its variants) are interfaces, so it's possible to pass nil here.

https://play.golang.org/p/AZWZJZ7-ZAs

[boddumanohar]

created a PR for this.

[alvaroaleman]

Repeating my comment from the PR in #813 , IMHO we shouldn't be doing this:

/hold
Actually stop, I don't think we should be doing this, because methods of a nullpointer are still callable and we have a couple of examples where this works without issue, e.G. MatchingLabels, HasLabels, MatchingFields. Those examples here are a no-op if nil, but there could be examples out there where this is not the case. With this change, we silently stop applying them which is surprising at best.

IMHO a method that does not explicitly state that it may be called with nil args can expect the caller to make sure that this doesn't happen.

[alvaroaleman]

@rajathagasthya WDYT, does that make sense?

[rajathagasthya]

Discussed in #813. Closing based on preference not to add this check.

[rajathagasthya]

/close