✨ Add cluster.NewClientFunc with options

[erikgb]

This adds a new cluster.ClientBuilderWithOptions alongside the cluster.DefaultNewClient, which will allow users to easily get a client that caches Unstructured - which the default client intentionally doesn't, ref #615 (comment).

Almost all the operators I work on seem to benefit from enabling caching of Unstructured. While not "that" common, I have also seen users asking for this on Slack. 😉 Adding this new func to controller-runtime will avoid having to implement the workaround explained in this blog.

[varshaprasad96]

I understand the use-case of caching unstructured objects especially since most of them are CRs in controllers. But going through the discussion in the issue, we would still be hitting the same issue of caching all the arbitrary objects accidentally. The evolution of uncached client was that we initially used to hit the API server to read all the unstructured objects (https://github.com/kubernetes-sigs/controller-runtime/blob/release-0.5/pkg/client/split.go#L45-L61) which then turned out to be expensive. Which is why the option of specifying uncached objects was introduced so that we could hit the api server only when needed (

controller-runtime/pkg/client/split.go

Line 115 in accd262

if !d.cacheUnstructured {

). IMO I would still not want to cache all the unstructured objects by default (accidentally caching un-needed objects) or turn that option "on" for users who aren't aware of what happens inside the hood. From a users perspective, I feel its easier to figure out and understand that there is throttling happening in API server's end (and fix it) than to figure out it's a caching problem while debugging performance issues.

[erikgb]

But why are more concerned about this for Unstructured than for any other type? I've seen many questions on Slack asking why a controller needs cluster-wide RBAC to resources just because the cache/watch is auto-created for any read of in-tree resource types like secrets. I have no problem with the current default (not caching Unstructured), but IMO it should be easier for users to enable caching of Unstructured, and would like it to surface somehow. It's considered best practice to grant a controller/operator least-privilege permissions, so caching all resources in the cluster is unlikely IMO.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, erikgb

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 [alvaroaleman]

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

[alvaroaleman]

thanks!

[erikgb]

Thanks for the review @alvaroaleman. I really like your suggestion, which makes this change extensible for future improvements! 👍