cache crd objects to speed up VpaTargetSelectorFetcher and ControllerFetcher

[chenchun]

Fixes #3266
Without this patch, it takes 1m41s (from I0806 07:50:08.676461 to I0806 07:51:49.018659) to get all selectors for each of 3111 vpas (mostly crd workload targets).

With this patch, it takes 1s (from I0806 08:02:49.137257 to I0806 08:02:50.266574).

We also save 3111*3/2/60 qps of scale http calls to kube-apiserver and speed up vpa admission.

[krzysied]

Wow! The performance improvement looks amazing.
Can I review this?

[bskiba]

/assign @krzysied
Krzysztof please do take a look.
My main concern with the approach is that we need get, list and watch permissions on all objects in the cluster.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign krzysied
You can assign the PR to them by writing /assign @krzysied in a comment when ready.

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

Needs approval from an approver in each of these files:

• vertical-pod-autoscaler/OWNERS

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

[krzysied]

My main concern with the approach is that we need get, list and watch permissions on all objects in the cluster.

I'm not sure if we can do it differently. The best solution would be having watch on scale subresource, but k8s doesn't prove such option.
These permissions give a lot of access, but it's only read access.

[krzysied]

stop channel should provided via function params.

[chenchun]

I don't know what you mean. I deleted stop chan param and follow the rules in other package to create stop chan in crdcache.

[krzysied]

I was thinking about having stop channel as an input parameter. This would be helpful if cluster feeder was recreated many times during recommender life.
However, I looked at the recommender code and it didn't seem like a use case. Moreover other informers are also not propagating stop channels.
Deleting stop channel seems ok.

[krzysied]

/hold
I talked with security folks and having rbac that allows to list the whole universe doesn't seems to be a great idea.
I will look for some alternatives/ways to restrict the access.

[bskiba]

From investigation by @krzysied, the current understanding is that giving VPA read access to all objects is a no-go from security perspective (gives access to secrets) and there is no way to express "give access to everything but secrets". Unfortunately, I don't see how we can make this PR work without that so I'm closing it, but feel free to reopen if you see an alternative.
/close

[k8s-ci-robot]

@bskiba: Closed this PR.

In response to this:

Form investigation by @krzysied, the current understanding is that giving VPA read access to all objects is a no-go from security perspective (gives access to secrets) and there is no way to express "give access to everything but secrets". Unfortunately, I don't see how we can make this PR work without that so I'm closing it, but feel free to reopen if you see an alternative.
/close

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.

[chenchun]

there is no way to express "give access to everything but secrets"

I find kubernetes/kubernetes#85963 talks about this.
@bskiba Do you think it's worth adding a flag to make this feature turned off by default and can be turned on for specific clusters by adding permissions for certain workloads instead of all objects to RBAC ?

[bskiba]

Sadly, looks like kubernetes/kubernetes#85963 is rejected. We're looking currently at a different way of tackling this, I'm hoping #3589 will help.