-
Notifications
You must be signed in to change notification settings - Fork 65
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the MultiScopedCache #90
Conversation
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: Nvm, apparently there are k8s reasons this is all moot, ignore.
I'd not really thought much about caching in trust-manager yet, so thank you for this thought provoking PR!
Looking at it, this makes sense and seems to be an improvement in the case that we want caching. But I'm not actually sure caching is worth it right now.
As far as I can see, the absolute worst case scenario in which our cache would help is in the case where we have n different Bundle
resources, all of which refer to the same Secret
.
That would mean that without a cache an update to the Secret
would trigger n API calls, whereas a cache would reduce that to 1 API call in the best case.
That's an improvement of course but I see some issues:
- Given that Secrets probably aren't the best type of source for a bundle (since trusted CA data isn't a "secret") the question is 'what about ConfigMaps'. We don't have caching for ConfigMaps (I think?) and they're likely to be more common.
- Large values of n seem really unlikely to me. I can imagine a multi-tenanted situation where a single trusted CA is shared by many bundles. But for n to grow large enough that we cause performance problems seems like it would need an extremely large cluster - and I'd imagine that in such a large cluster, the kube API server would be pretty beefy and capable of handling a decent amount of load.
- If we were to add
Secret
s as targets in the future, having to work around a cache would introduce complications. Nothing we couldn't manage, but it'd be something we'd have to think about.
Caches are complicated things and introduce a lot of possibilities for bugs. I'm thinking it might be easier to just avoid all of those bugs by explicitly skipping caching entirely and only adding it later if someone actually comes along with a concrete example of a performance issue (in which case, we'd have to look at ConfigMaps too).
As a bonus, skipping caching will slightly reduce runtime memory usage and eliminate the possibility of us caching any Secrets we definitely don't want to cache, reducing our attack surface (slightly).
@SgtCoDFish it is not entirely clear to me what this PR is trying to fix however the reason why we have this "multi-namespace cache" is that we want to inform on Secrets for only a single Namespace (the trust Namespace). Other resources we want to inform at the cluster level; either because they are cluster scoped resources (
We can't get away from using caches as they are a requirement for the informer. This should ideally be hidden away from us with the controller-runtime SDK, but at least at the time of writing this code, multi-scope namespacing per gvk wasn't possible. |
As far as I can tell, only one of these can be true. |
@SgtCoDFish This is the point of spinning up our own Namespaced cache which is wrapped. If you disable the
|
@JoshVanL that is what this PR fixes, now controller-runtime does not try to spin up a cache at the cluster level, instead it uses the MultiScopedCache to limit the scope of the cache. It was not working before because the *corev1.Secret objects don't have a GVK value set, so MultiScopedCache was just passing the requests to the cluster scoped cache. |
@inteon could you please add a test showing what is being fixed here? Still struggling to see what we are improving. Is it the fact that ClientDisableCacheFor is now nil? |
b61ccfe
to
a57220e
Compare
/retest |
561de08
to
b5d28ed
Compare
/retest |
/lgtm |
@JoshVanL can you approve the PR too? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon, JoshVanL 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 |
This PR remoes
ClientDisableCacheFor
and fixes theMultiScopedCache
by populating GVK values when missing based on the type of the providedclient.Object
.Currently,
ClientDisableCacheFor
disables caching for Secret resources; I reenabled caching for these resources. I also fixed theMultiScopedCache
, now it can handle *corev1.Secret resources that don't have GVK values set.