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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

鉁 Add cluster.NewClientFunc with options #2054

Merged

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Nov 21, 2022

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2022
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 21, 2022
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (

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
Copy link
Contributor Author

erikgb commented Nov 22, 2022

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.

pkg/cluster/cluster.go Outdated Show resolved Hide resolved
pkg/cluster/cluster.go Outdated Show resolved Hide resolved
@erikgb erikgb force-pushed the feat/cache-unstructured-client branch from f0c054d to c7c2185 Compare November 25, 2022 20:40
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 25, 2022
@erikgb erikgb force-pushed the feat/cache-unstructured-client branch from c7c2185 to cca5160 Compare November 25, 2022 20:52
@erikgb erikgb changed the title 鉁 Add NewClientFunc to create client caching Unstructured 鉁 Add cluster.NewClientFunc with options Nov 25, 2022
@erikgb erikgb force-pushed the feat/cache-unstructured-client branch from cca5160 to 791d496 Compare November 25, 2022 21:07
Motivated by the ability to create a client caching Unstructured when configuring manager.
@erikgb erikgb force-pushed the feat/cache-unstructured-client branch from 791d496 to cdbd4b7 Compare November 25, 2022 22:23
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2022
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2022
@alvaroaleman
Copy link
Member

thanks!

@k8s-ci-robot k8s-ci-robot merged commit 007d240 into kubernetes-sigs:master Nov 26, 2022
9 of 10 checks passed
@erikgb
Copy link
Contributor Author

erikgb commented Nov 26, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants