refactor(sac): keep highest scope state for duplicate clusters and ns#19506
Draft
rhybrillou wants to merge 1 commit intomaster-yann/ROX-33511/scope-selection-by-cluster-idfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
populateStateForCluster, when a cluster with an existing name but a different ID is processed,clusterIDToNameis no longer updated (due to the early return), which changes previous behavior where the last-seen cluster ID for that name was recorded; consider whether this regression is acceptable or if all IDs for a given name should be tracked. - The new logic in
populateStateForNamespaceonly updates the state of an existing namespace entry but never refreshes its attributes, unlike the cluster-level logic; if attributes are expected to reflect the latest selection/detail context, you may want to update them there as well for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `populateStateForCluster`, when a cluster with an existing name but a different ID is processed, `clusterIDToName` is no longer updated (due to the early return), which changes previous behavior where the last-seen cluster ID for that name was recorded; consider whether this regression is acceptable or if all IDs for a given name should be tracked.
- The new logic in `populateStateForNamespace` only updates the state of an existing namespace entry but never refreshes its attributes, unlike the cluster-level logic; if attributes are expected to reflect the latest selection/detail context, you may want to update them there as well for consistency.
## Individual Comments
### Comment 1
<location path="pkg/sac/effectiveaccessscope/effective_access_scope.go" line_range="321-327" />
<code_context>
// Match the cluster.
clusterState := ruleSelectors.matchCluster(cluster)
+ // Set the cluster state to the pre-existing state.
+ if clusterSubTree := root.Clusters[clusterName]; clusterSubTree != nil {
+ if clusterSubTree.State < clusterState {
+ clusterSubTree.State = clusterState
+ clusterSubTree.Attributes = nodeAttributesForCluster(cluster, detail)
+ }
+ return
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Reusing existing cluster subtree skips clusterIDToName updates, which may break lookups for additional cluster IDs with the same name.
With this early return, later clusters that share `clusterName` but have lower/equal `clusterState` will skip `root.clusterIDToName[clusterID]` updates. Previously, even when reusing/overwriting the subtree, the new `clusterID` was still mapped to `clusterName`. If callers expect all cluster IDs to resolve via `clusterIDToName`, those lookups can now fail. You can avoid this by updating `root.clusterIDToName[clusterID]` before returning, regardless of whether the state changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+321
to
+327
| // Set the cluster state to the pre-existing state. | ||
| if clusterSubTree := root.Clusters[clusterName]; clusterSubTree != nil { | ||
| if clusterSubTree.State < clusterState { | ||
| clusterSubTree.State = clusterState | ||
| clusterSubTree.Attributes = nodeAttributesForCluster(cluster, detail) | ||
| } | ||
| return |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Reusing existing cluster subtree skips clusterIDToName updates, which may break lookups for additional cluster IDs with the same name.
With this early return, later clusters that share clusterName but have lower/equal clusterState will skip root.clusterIDToName[clusterID] updates. Previously, even when reusing/overwriting the subtree, the new clusterID was still mapped to clusterName. If callers expect all cluster IDs to resolve via clusterIDToName, those lookups can now fail. You can avoid this by updating root.clusterIDToName[clusterID] before returning, regardless of whether the state changes.
Contributor
|
Images are ready for the commit at 100b9a2. To use with deploy scripts, first |
This was referenced Mar 19, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master-yann/ROX-33511/scope-selection-by-cluster-id #19506 +/- ##
=======================================================================================
+ Coverage 49.69% 49.72% +0.03%
=======================================================================================
Files 2701 2701
Lines 203514 203524 +10
=======================================================================================
+ Hits 101136 101211 +75
+ Misses 94845 94788 -57
+ Partials 7533 7525 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5 tasks
5 tasks
ff9a4f5 to
8abc271
Compare
6773139 to
9602a55
Compare
8abc271 to
9c74804
Compare
9602a55 to
cbc8794
Compare
9c74804 to
38cb56c
Compare
cbc8794 to
100b9a2
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
This PR is part of the split of #19351
The split results in the following stack of PRs:
The code changed here is unlikely to be actually called in production, as in that case, the list of clusters and namespaces are provided by the database, which has unicity constraints on the cluster name, and relays kubernetes namespace information from the clusters (the latter ones should enforce namespace unicity too).
Nevertheless, if some duplicate entries for a given node of the access scope tree (root -> cluster -> namespace) were provided in input (that is database/cache lookup at the start of processing of a service call), then the highest selection result (excluded < partial < included) is kept for that node instead of the last seen one.
User-facing documentation
is updated ORupdate is not neededis created and is linked above ORis not neededTesting and quality
Automated testing
How I validated my change
Manual CI run.
There should be no duplicate cluster or namespace in the access scope computation for actual service calls. E2e validation of this change might be tricky. However, there should be no change in behaviour for the nominal case, and the current SAC e2e tests should catch changes in behaviour.