Conversation
Port SelfSubjectRulesReview short-circuit and access-cache hardening from main (stolostron#6638) so restricted users no longer trigger O(N) SSARs on /events. Omit compression/meta/filter-before-inflate pieces not present on 2.13. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Cover hashed access-cache behavior and SelfSubjectRulesReview short-circuit paths, including SSRR HTTP failure fallback to SSAR. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: stolostron/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
/hold |
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
/unhold |
KevinFCormier
left a comment
There was a problem hiding this comment.
There are a couple issues with the logic, so fixing them an maintaining the performance gain may be tricky.
- Checking permissions in the
defaultnamespace is used a proxy for checking cluster-scoped resource access, but many resources are not cluster-scoped, and there is no code guaranteeing that this check is used only for cluster-scoped resources. I granted useruser1permission to get secrets in thedefaultnamespace, but then the user is able to see all Credentials. This is major security hole. - I granted
user1cluster-admin access for theacm39327-mc-01cluster. With the old code, it shows up in the cluster list. With your new code, it does not.
Other areas for improvement - caching keys include the API version, but permissions checking is only done against groups. I think you could drop the version part earlier.
- Cache SelfSubjectRulesReview per token+namespace instead of a single `default`-namespace review, so namespaced permissions are evaluated in the resource's own namespace. - For cluster-scoped resources, confirm unrestricted SSRR grants with SSAR to prevent RoleBindings in `default` from impersonating cluster-scoped access. - Update unit tests to cover per-namespace caching and namespace isolation. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Split `events.ts` into `eventsAccess.ts` for RBAC evaluation and `eventsCache.ts` for access cache management, and add unit tests for both new modules. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
622f6b3 to
3d7a488
Compare
What was wrong
What changed
|
- Maintain an explicit allowlist of cluster-scoped kinds watched by the console and route their SSRR probes through the default namespace. - Require SSAR confirmation for any non-deny cluster-scoped SSRR result, closing the gap where RoleBindings in default could impersonate cluster-scoped grants. - Surface and handle SSRR evaluationError as incomplete rule enumeration, falling back to SSAR when rule lists may be partial. - Expand unit tests for cluster-scoped allow-names, deny paths, and evaluationError behavior. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
/test Red Hat Konflux / console-acm-213-on-pull-request |
|
/test images |
|
/hold |
KevinFCormier
left a comment
There was a problem hiding this comment.
I discovered a new issue while reviewing the main PR. Please see inline comments.
Mark cluster-scoped watches on IWatchOptions, include API group in SSAR cache keys, and retry SelfSubjectRulesReview after unavailable results so namespaced kinds cannot impersonate cluster access on release-2.13. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
a5d189b to
f0afd2f
Compare
|
@KevinFCormier your cluster scope feedback is in on both PRs. eventsCache.ts / eventsAccess.ts are the same on main and release-2.13. Only events.ts differs by branch (main keeps compression/meta/SSE optimizations). Changes:
|
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
| configureClusterScopedKinds( | ||
| definitions.filter((definition) => definition.clusterScoped).map((definition) => definition.kind) | ||
| ) | ||
|
|
There was a problem hiding this comment.
This seems overly complicated. I don't follow why in eventsAccess.ts you have the clusterScopedKinds defined as a let, and then you are depending on the side effect of loading this file to call the configureClusterScopedKinds function. And in the tests, you are testing with a subset of cluster-scoped kinds. Wouldn't it be more straightforward to do something like this?
| export const CLUSTER_SCOPED_KINDS = new Set(definitions.filter((definition) => definition.clusterScoped).map((definition) => definition.kind)) | |
| ) |
If you're trying to avoid a circular dependency between events.ts and eventsAccess.ts, maybe we just need to move the definitions to their own file?
There was a problem hiding this comment.
what do you think about 0a66bdb
as soon as we agree on this I will forward/backport it to the rest of versions (2.14-5.1)
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ginxo, KevinFCormier The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |



Summary
release-2.13(fast-forwards tobackplane-2.8).listis denied, use oneSelfSubjectRulesReviewper token/kind instead of O(namespaces) namespaced list + O(N) per-object SSARs, which OOMs/hangs restricted users under large inventory (MOCK_CLUSTERS/ real fleets).unavailableand fall back to SSAR (not cached as deny-all).Not ported (not present on 2.13): event-body compression,
metaon cached events, and filter-before-inflate inserver-side-events.ts. Those half of #6638 do not apply here; the O(N) SSAR path does.Related to #6638
Test plan
backendunit tests for access cache + SSRR short-circuit (npm test -- --testPathPattern=test/routes/events.test.ts)npm run checkin backendMOCK_CLUSTERS=1000, kubeadmin vs restricted/noneon Inventory; confirm backend RSS/CPU stay boundednonecompletes SSE load without backend hangTest support
I normally use
for checking the CPU/MEM consumption on the local environment
Test results
I can confirm the situation is even worse for 2.13 and the memory grows up out of control for non-admin users. This PR is totally mandatory for 2.13
Test commands
Image generation
Deployment
and the output should be something like
quay.io/emingora/console-mce:acm39327-6648-20260818-105951Test execution
/tmpwe should see something like
otherwise execute
Execute the resource watcher in a terminal
and then start the test execution on another terminal