ROX-33511: Allow scope selection by cluster ID#19505
ROX-33511: Allow scope selection by cluster ID#19505rhybrillou wants to merge 3 commits intomaster-yann/ROX-33511/rework-scope-selectionfrom
Conversation
|
Skipping CI for Draft Pull Request. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the GraphQL schema,
includedClusterIds: [String!]!is declared non-null but the resolver returnsresolver.data.GetIncludedClusterIds()directly, which may benil; consider normalizing to an empty slice before returning to conform to the non-null list contract and avoid runtime GraphQL errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the GraphQL schema, `includedClusterIds: [String!]!` is declared non-null but the resolver returns `resolver.data.GetIncludedClusterIds()` directly, which may be `nil`; consider normalizing to an empty slice before returning to conform to the non-null list contract and avoid runtime GraphQL errors.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at 7877581. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master-yann/ROX-33511/rework-scope-selection #19505 +/- ##
=============================================================================
Coverage 49.72% 49.72%
=============================================================================
Files 2701 2701
Lines 203475 203499 +24
=============================================================================
+ Hits 101175 101195 +20
- Misses 94777 94781 +4
Partials 7523 7523
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:
|
| // Next ID: 4 | ||
| } | ||
|
|
||
| repeated string included_cluster_ids = 5; |
There was a problem hiding this comment.
Summary of discussion in #19351
The selection behaviour in case data is present in both included_cluster_ids and included_clusters should be clarified: process sets exclusively with precedence rule or union of the sets.
The behaviour is the one described by the comment at the start of the proto message:
// Each element of any repeated field is an individual rule. Rules are
// joined by logical OR: if there exists a rule allowing resource `x`,
// `x` is in the access scope.
This new set of selection rules ends up extending the ruleset, with the result of the selection being the union of all matching rules.
| // The namespace field and at least one cluster field must be set. | ||
| // The cluster ID takes precedence over the cluster name when a cluster with that ID exists. | ||
| string cluster_id = 3; |
There was a problem hiding this comment.
Comment from #19351
nit: There was an effort couple years ago (probably controversial) that ordered these things by number such that it was easier to see what the next number is to reduce issues in working with protos. My personal preference is numerical order BUT that is a preference. Not something I'm going to feel strongly about but I will mention it when I see it as I'm doing here. You can take that and choose the path of your preference.
My preference is logical grouping (in that case cluster identification). Messages were enriched with comments providing the next available number.
rhybrillou
force-pushed
the
master-yann/ROX-33511/scope-selection-by-cluster-id
branch
3 times, most recently
from
9c74804 to
38cb56c
Compare
rhybrillou
force-pushed
the
master-yann/ROX-33511/scope-selection-by-cluster-id
branch
from
38cb56c to
4b7af99
Compare
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
convertRulesToSelectors, namespace rules favorClusterIdoverClusterNamewhen both are set; consider updating the function comment (and/or the proto comment forSimpleAccessScope_Rules_Namespace) to explicitly document this precedence so callers and future maintainers don’t have to infer it from the implementation and tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `convertRulesToSelectors`, namespace rules favor `ClusterId` over `ClusterName` when both are set; consider updating the function comment (and/or the proto comment for `SimpleAccessScope_Rules_Namespace`) to explicitly document this precedence so callers and future maintainers don’t have to infer it from the implementation and tests.
## Individual Comments
### Comment 1
<location path="pkg/sac/effectiveaccessscope/conversion.go" line_range="116-117" />
<code_context>
includedClusterNames := scopeRules.GetIncludedClusters()
output.clustersByName = set.NewStringSet(includedClusterNames...)
+ includedClusterIDs := scopeRules.GetIncludedClusterIds()
+ output.clustersByID = set.NewStringSet(includedClusterIDs...)
+
// Convert each selector to labels.Selector.
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify behavior when both cluster ID and cluster name are specified for the same namespace/cluster
Since IDs are now preferred over names (non-empty namespace `clusterID` short-circuits, and cluster IDs are checked before names), a conflicting `cluster_id`/`cluster_name` pair will silently ignore the name. Please either validate that both, if provided, match, or clearly document that `cluster_id` always takes precedence so callers aren’t surprised when `cluster_name` is ignored.
</issue_to_address>
### Comment 2
<location path="central/graphql/resolvers/generated.go" line_range="1404-1405" />
<code_context>
"includedNamespaces: [SimpleAccessScope_Rules_Namespace]!",
"namespaceLabelSelectors: [SetBasedLabelSelector]!",
}))
utils.Must(builder.AddType("SimpleAccessScope_Rules_Namespace", []string{
+ "clusterId: String!",
"clusterName: String!",
"namespaceName: String!",
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Non-null GraphQL `clusterId` may not align with validation semantics allowing name-only namespaces
The GraphQL field is `String!`, but the underlying validation only requires one of `cluster_id` or `cluster_name`. Because the proto defaults `cluster_id` to `""`, the resolver will always return a non-null value that may actually be semantically unset. This can mislead clients into treating an empty string as a real ID. Consider making this field nullable (`String`) or updating the contract so that empty strings are not used to represent “unset”.
Suggested implementation:
```golang
utils.Must(builder.AddType("SimpleAccessScope_Rules_Namespace", []string{
"clusterId: String",
"clusterName: String!",
"namespaceName: String!",
}))
```
To fully align the GraphQL contract with the validation semantics (where `cluster_id` may be unset):
1. Update the resolver / model for `SimpleAccessScope_Rules_Namespace.clusterId` so that it can return `null` instead of an empty string when the underlying `cluster_id` is logically unset. This may require:
- Changing the generated/resolver type for `clusterId` from `string` to `*string`, and
- Mapping empty proto values (`""`) to `nil` before returning to GraphQL.
2. Ensure any tests that assert on the shape of the `SimpleAccessScope_Rules_Namespace` GraphQL type or sample responses are updated to allow `clusterId` to be nullable and, where appropriate, `null` instead of `""`.
</issue_to_address>
### Comment 3
<location path="pkg/sac/effectiveaccessscope/effective_access_scope_test.go" line_range="469-467" />
<code_context>
+ {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering a namespace rule that uses a non-existent cluster ID
To fully exercise this behavior, please also add a test where `IncludedNamespaces` uses a `ClusterId` that is not present in `clusterIDs`/known clusters, and assert the expected outcome for that "unknown" ID (e.g., nothing included vs. mapped to a "Not Found" cluster). This will clarify the intended semantics and guard against regressions in how unknown cluster IDs are handled.
Suggested implementation:
```golang
desc: "namespace included by name does not include anything else",
```
You’ll want to add a new test case that mirrors the existing table-driven tests for `ComputeEffectiveAccessScope` (or whatever function is under test in this file), specifically exercising a namespace rule with a **ClusterId that is not in the known cluster IDs** used by the test.
Assuming the existing pattern is something like:
```go
tests := []struct {
desc string
scopeDesc string
scopeStr string
scopeJSON string
scope *storage.SimpleAccessScope
detail v1.ComputeEffectiveAccessScopeRequest_Detail
hasError bool
}{
{
desc: "namespace included by name does not include anything else",
scopeDesc: `namespace: "Arrakis::Atreides" => { "Arrakis::Atreides" }`,
scopeStr: "Arrakis::Atreides",
scopeJSON: `{"Arrakis":["Atreides"]}`,
scope: &storage.SimpleAccessScope{
Id: accessScopeID,
Name: accessScopeName,
Rules: &storage.SimpleAccessScope_Rules{
IncludedNamespaces: []*storage.SimpleAccessScope_Rules_Namespace{
{
ClusterId: "known-cluster-id",
NamespaceName: "Atreides",
},
},
},
},
detail: v1.ComputeEffectiveAccessScopeRequest_HIGH,
hasError: false,
},
// ...
}
```
add a **new test case** entry right after it that uses an **unknown** cluster ID:
```go
{
desc: "namespace rule with unknown cluster ID is ignored",
scopeDesc: `namespace: "unknown-cluster-id::Atreides" => {}`,
scopeStr: "unknown-cluster-id::Atreides",
scopeJSON: `{"unknown-cluster-id":["Atreides"]}`,
scope: &storage.SimpleAccessScope{
Id: accessScopeID,
Name: accessScopeName,
Rules: &storage.SimpleAccessScope_Rules{
IncludedNamespaces: []*storage.SimpleAccessScope_Rules_Namespace{
{
ClusterId: "unknown-cluster-id",
NamespaceName: "Atreides",
},
},
},
},
detail: v1.ComputeEffectiveAccessScopeRequest_HIGH,
hasError: false,
},
```
Key points to adapt to your actual code:
1. **Use the real namespace rule fields**: if `SimpleAccessScope_Rules_Namespace` uses `ClusterName` instead of `ClusterId`, adjust accordingly (and update the test description/comment to match).
2. **Match the existing expectations**:
* If the intended semantics are “unknown cluster IDs result in *no* namespaces included”, ensure the test assertions expect an **empty** effective access scope for that cluster ID.
* If the code instead aggregates them under some “unknown/not found” cluster bucket, assert that behavior explicitly in the test.
3. **Reuse existing helpers**: however the other table entries validate the effective access scope (e.g., by comparing stringified descriptions, JSON, or using a helper like `assertEffectiveScopeEquals`), follow the exact same pattern for this new test case.
4. Ensure the new entry is included in the same `tests` slice (or table) that is iterated over in the test function so it actually runs.
Because I only saw a small portion of the file, you’ll need to:
- Insert the new struct literal into the existing test table right next to the “namespace included by name does not include anything else” case.
- Use the appropriate `detail` value and validation mechanism that the rest of the tests in this file already use.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| includedClusterIDs := scopeRules.GetIncludedClusterIds() | ||
| output.clustersByID = set.NewStringSet(includedClusterIDs...) |
There was a problem hiding this comment.
issue (bug_risk): Clarify behavior when both cluster ID and cluster name are specified for the same namespace/cluster
Since IDs are now preferred over names (non-empty namespace clusterID short-circuits, and cluster IDs are checked before names), a conflicting cluster_id/cluster_name pair will silently ignore the name. Please either validate that both, if provided, match, or clearly document that cluster_id always takes precedence so callers aren’t surprised when cluster_name is ignored.
| @@ -466,6 +466,61 @@ func TestComputeEffectiveAccessScope(t *testing.T) { | |||
| detail: v1.ComputeEffectiveAccessScopeRequest_HIGH, | |||
| hasError: false, | |||
There was a problem hiding this comment.
suggestion (testing): Add a test covering a namespace rule that uses a non-existent cluster ID
To fully exercise this behavior, please also add a test where IncludedNamespaces uses a ClusterId that is not present in clusterIDs/known clusters, and assert the expected outcome for that "unknown" ID (e.g., nothing included vs. mapped to a "Not Found" cluster). This will clarify the intended semantics and guard against regressions in how unknown cluster IDs are handled.
Suggested implementation:
desc: "namespace included by name does not include anything else",You’ll want to add a new test case that mirrors the existing table-driven tests for ComputeEffectiveAccessScope (or whatever function is under test in this file), specifically exercising a namespace rule with a ClusterId that is not in the known cluster IDs used by the test.
Assuming the existing pattern is something like:
tests := []struct {
desc string
scopeDesc string
scopeStr string
scopeJSON string
scope *storage.SimpleAccessScope
detail v1.ComputeEffectiveAccessScopeRequest_Detail
hasError bool
}{
{
desc: "namespace included by name does not include anything else",
scopeDesc: `namespace: "Arrakis::Atreides" => { "Arrakis::Atreides" }`,
scopeStr: "Arrakis::Atreides",
scopeJSON: `{"Arrakis":["Atreides"]}`,
scope: &storage.SimpleAccessScope{
Id: accessScopeID,
Name: accessScopeName,
Rules: &storage.SimpleAccessScope_Rules{
IncludedNamespaces: []*storage.SimpleAccessScope_Rules_Namespace{
{
ClusterId: "known-cluster-id",
NamespaceName: "Atreides",
},
},
},
},
detail: v1.ComputeEffectiveAccessScopeRequest_HIGH,
hasError: false,
},
// ...
}add a new test case entry right after it that uses an unknown cluster ID:
{
desc: "namespace rule with unknown cluster ID is ignored",
scopeDesc: `namespace: "unknown-cluster-id::Atreides" => {}`,
scopeStr: "unknown-cluster-id::Atreides",
scopeJSON: `{"unknown-cluster-id":["Atreides"]}`,
scope: &storage.SimpleAccessScope{
Id: accessScopeID,
Name: accessScopeName,
Rules: &storage.SimpleAccessScope_Rules{
IncludedNamespaces: []*storage.SimpleAccessScope_Rules_Namespace{
{
ClusterId: "unknown-cluster-id",
NamespaceName: "Atreides",
},
},
},
},
detail: v1.ComputeEffectiveAccessScopeRequest_HIGH,
hasError: false,
},Key points to adapt to your actual code:
- Use the real namespace rule fields: if
SimpleAccessScope_Rules_NamespaceusesClusterNameinstead ofClusterId, adjust accordingly (and update the test description/comment to match). - Match the existing expectations:
- If the intended semantics are “unknown cluster IDs result in no namespaces included”, ensure the test assertions expect an empty effective access scope for that cluster ID.
- If the code instead aggregates them under some “unknown/not found” cluster bucket, assert that behavior explicitly in the test.
- Reuse existing helpers: however the other table entries validate the effective access scope (e.g., by comparing stringified descriptions, JSON, or using a helper like
assertEffectiveScopeEquals), follow the exact same pattern for this new test case. - Ensure the new entry is included in the same
testsslice (or table) that is iterated over in the test function so it actually runs.
Because I only saw a small portion of the file, you’ll need to:
- Insert the new struct literal into the existing test table right next to the “namespace included by name does not include anything else” case.
- Use the appropriate
detailvalue and validation mechanism that the rest of the tests in this file already use.
…r part of the selection rules
|
@rhybrillou: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Description
This PR is part of the split of #19351
The split results in the following stack of PRs:
The current PR introduces the concept of selection by cluster ID. This kind of selection becomes more relevant in the scope of the OCP console security plugin: access tokens are requested from the cluster console and used later on to filter ACS data related to the cluster the console user is logged in on. If in the lifetime of such a token, the cluster is removed and another cluster is added with the same name, this could lead to data leaks (security data from another cluster being displayed).
The cluster identification by UUID instead of name should reduce the chances of data leak (as cluster identifiers are more likely to be programatically generated than cluster names).
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.
Currently, the creation of simple access scopes identifying clusters by ID is only possible through direct API calls.
A manual test plan will be added.