⚠ Add manager option to enable caching of unstructured objects

[joelanford]

This PR adds a new manager/cluster option, called ClientCacheUnstructured that configures the ClientBuilder to build a client that caches unstructured objects.

It is a breaking change because it requires a new method on the cluster.ClientBuilder interface.

This is a follow-up based on #1332 (comment)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelanford

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [joelanford]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[joelanford]

/hold

For more discussion since this is a breaking change.

[joelanford]

/retest

[k8s-ci-robot]

@joelanford: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-controller-runtime-apidiff-master	6526b3a	link	/test pull-controller-runtime-apidiff-master

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[vincepri]

/lgtm

[shawn-hurley]

Could another approach be to make a new client.Option for this and that would not be a breaking change I think?

I am not saying we should just wondering if we looked at that approach.

[joelanford]

@shawn-hurley I'm not following your suggestion. I was also trying to think of a non-breaking change approach, but nothing is coming to me that would make this option available to arbitrary ClientBuilders.

IMO, it's a little bit odd that we would have cache-related methods on the ClientBuilder interface because it implies that all clients need to handle caching. We do pass a cache as a Build() parameter, but that could theoretically be ignored. Cache-specific methods on the interface for non-caching implementations would need to be no-ops.

I suppose we could make a new interface CachingClientBuilder like below and then do type assertions to see if we can call some extra methods:

type CachingClientBuilder interface {
	ClientBuilder
	WithUncached(objs ...client.Object) ClientBuilder
	CacheUnstructured(v bool) ClientBuilder
}

But even with that approach, @estroz made the astute observation that further configurations that affect the delegatingClient would require further breaking changes on this interface if we wanted to expose that configuration to users.

[shawn-hurley]

Admittedly, have not been involved enough over the last year to know if this makes sense so if it doesn't I am sorry :).

I was wondering if it could be a option here:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/client.go#L37

such that at ClientBuilder.Build it is passed in as an option for the client.

Maybe this doesn't make sense because the ClientBuilder already handles caching concerns and the Options don't so maybe this is a bad idea :)

[k8s-ci-robot]

@joelanford: PR needs rebase.

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.

[huozby]

I'm quite confused about manager option(filed ClientBuilder -> filed NewClient) in release v0.8.3 and v0.9.0.alpha1. Why the6. field fallback to release v0.6.5. Could someone explain it? Which version should I use when I write my own controller...

[joelanford]

@hzbcloud #1332 highlights the regression caused by the change to ClientBuilder and #1409 makes the change back to NewClientFunc. These PRs should explain why we needed to revert the ClientBuilder option.

Closing this since the problem this PR solves is no longer a problem with NewClientFunc.