Fix List() calls that didn't fill in revision #9599
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fasaxc
added
cherry-pick-candidate
docs-not-required
marvin-tigera
added
the
release-note-required
caseydavenport
approved these changes
Dec 13, 2024
fasaxc
force-pushed
the
fix-missing-rev
branch
from
27fb6ea to
69aec51
Compare
caseydavenport
previously approved these changes
Dec 13, 2024
The watchercache requires the KVList to contain the revision for correctness. Without it, it can miss updates from the server. Fix the resources that were missing the revision and panic if the revision is missing to surface the problem in future.
fasaxc
force-pushed
the
fix-missing-rev
branch
from
69aec51 to
3130b5f
Compare
Replace Gets() with lists that filter on name.
fasaxc
force-pushed
the
fix-missing-rev
branch
from
aceb176 to
dda0f92
Compare
fasaxc
force-pushed
the
fix-missing-rev
branch
from
e193b8c to
3384f89
Compare
Looks like k8s might return that in some cases (customK8sResourceClient checks for it).
fasaxc
force-pushed
the
fix-missing-rev
branch
from
3384f89 to
0995bc6
Compare
CNI plugin relies on its List() being turned into a Get() for a single Pod. It doesn't have RBAC permissions for list and it only uses List() because one pod can result in >1 WEP.
fasaxc
dismissed
caseydavenport’s stale review
Big changes: refactored to use list with fieldSelector
| // We know that, in KDD, even though there may be >1 endpoint, we're only | ||
| // looking up one backing Pod. Send it a hint that we really want it to | ||
| // do a Get instead of a list. CNI plugin only has RBAC permissions to do | ||
| // a get on Pod resources. The default used to be to do a Get if possible, |
There was a problem hiding this comment.
CNI plugin only has RBAC permissions
Any reason to not just switch to a List() with the field selector specified, and give the CNI plugin the necessary permissions?
There was a problem hiding this comment.
Mainly just thinking that it was quite a big loosening to give CNI plugin list permissions when it could make do with get before. That, and the fact that I'd need to adjust manifests and operator to loosen permissions, which felt like extra work to go in the wrong direction.
There was a problem hiding this comment.
I suppose I just don't see a huge security distinction between "Get any pod in the cluster" vs "List any pod in the cluster"
But I'm happy to leave it as is.
caseydavenport
approved these changes
Jan 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The watchercache requires the KVList to contain the collection resource version for correctness. Without it, it can miss updates from the datastore.
List()calls intoGet()calls when only one resource was needed. This code was incorrect becauseGet()doesn't return the "collection resource version", which is what aList()call should return (and must return for a subsequentWatch()to function correctly).Get()use aList()with aFieldSelectorthat matches on the name of the resource. This returns the same resource but it wraps it in a List object that carries the needed collection version.WorkloadEndpointbehind an opt-inContextflag (with a nice long name to make users of the hacky flag feel bad :-P). The CNI plugin relies on that logic to avoid needinglistRBAC permissions for Pods.Related issues/PRs
CORE-10904
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.