Loop over namespaces when listing reconciled objects #3251
Merged
Conversation
This file contains hidden or 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
makkes
added
bug
|
@timricese thank you for submitting this PR! 🎉 We'll review it as soon as we have some bandwidth. Some checks are failing. Could you please fix whatever can be fixed? (All that is not caused by some unpatched JS vulnerabilities or similar issues.) |
|
Thanks! The failing test (just linting whitespace) should work now. |
makkes
force-pushed
the
list-by-namespace
branch
from
13b2ba7 to
9a0e781
Compare
objects instead of getting from all namespaces
Adding tests for the different behaviour of the function based on user permissions required reworking how the gRPC server is setup for testing: Instead of having a single static user that's used for all requests to the server now each requests needs to provide the user to be impersonated in the context. This facilitates sophisticated permission tests uncovering potential issues such as the one fixed by this PR.
makkes
force-pushed
the
list-by-namespace
branch
from
38af8b7 to
3f9ddbb
Compare
|
Great stuff @timricese. I added a commit providing integration tests for the intended behaviour. Let me know what you think. |
makkes
approved these changes
Jan 12, 2023
Merged
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.
Fixes #3231
What changed?
When getting reconciled objects, instead of just looping over kinds and retrieving all objects cluster-wide, loop over only namespaces which the current user has access to, as well as kinds as before.
Why was this change made?
When an OIDC impersonated user does not have cluster-wide privileges, the weave-gitops UI will not show any resources that are reconciled as part of kustomize/helmrelease etc objects. This change means that only objects in namespaces which the user has permissions in will be listed.
How was this change implemented?
Simply providing another option to the client when calling List(), which specifies the namespace to list from, and looping over the namespaces which the user has access to. The list of user-accessible namespaces was already defined a few lines later in the code.
How did you validate the change?
Tested locally/in our production clusters.
We are expecting that existing tests should already cover this and dont need updating.
Release notes
Bugfix, nothing notable
Documentation Changes
None