ACM-39327: Fix non-admin SSE OOM under large inventory - #6638
Conversation
Avoid O(N) SelfSubjectAccessReviews and inflate-before-filter on the /events stream so restricted users no longer OOM the console backend under large inventory. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ginxo 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 |
📝 WalkthroughWalkthroughChangesThe event pipeline now preserves resource metadata and filters before inflation. RBAC authorization now uses hashed, bounded caches, Event delivery and metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SSEEventSource
participant EventResourceMeta
participant EventFilter
participant SubjectRulesReviewCache
participant KubernetesAuthorizationAPI
participant SSARCache
SSEEventSource->>EventResourceMeta: Resolve resource identity
EventResourceMeta->>EventFilter: Provide resource metadata
EventFilter->>SubjectRulesReviewCache: Resolve kind access
SubjectRulesReviewCache->>KubernetesAuthorizationAPI: Submit SelfSubjectRulesReview
KubernetesAuthorizationAPI-->>SubjectRulesReviewCache: Return access classification
EventFilter->>SSARCache: Evaluate incomplete rules
SSARCache->>KubernetesAuthorizationAPI: Submit SSAR request
KubernetesAuthorizationAPI-->>SSARCache: Return access decision
EventFilter->>EventFilter: Inflate permitted event
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
backend/test/lib/server-side-events.test.ts (2)
58-66: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDispose the keep-alive interval after the suite.
ServerSideEvents.reset()re-createsintervalTimerthroughthis.intervalTimer ??= setInterval(...), as shown at Lines 125-127 ofbackend/src/lib/server-side-events.ts. This suite never clears it. Jest can then report an open handle or fail to exit. Callawait ServerSideEvents.dispose()in anafterAllhook.🧹 Proposed cleanup
afterEach(() => { ServerSideEvents.eventFilter = undefined as unknown as typeof ServerSideEvents.eventFilter ServerSideEvents.reset() }) + + afterAll(async () => { + await ServerSideEvents.dispose() + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/lib/server-side-events.test.ts` around lines 58 - 66, Add an afterAll hook in the server-side-events test suite that awaits ServerSideEvents.dispose() to clear the intervalTimer created by ServerSideEvents.reset(). Keep the existing beforeEach and afterEach cleanup unchanged.
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 129 exceeds the 120-character print width.
Wrap the object literal so Prettier formatting stays stable.
💅 Proposed formatting
- object: { kind: 'ManagedCluster', apiVersion: 'v1', metadata: { name: 'cluster-1', namespace: '', resourceVersion: '1' } }, + object: { + kind: 'ManagedCluster', + apiVersion: 'v1', + metadata: { name: 'cluster-1', namespace: '', resourceVersion: '1' }, + },As per coding guidelines: "Use the project's Prettier configuration: 120-character width".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/lib/server-side-events.test.ts` at line 129, Reformat the object literal in the server-side events test around the ManagedCluster fixture so no line exceeds the project’s 120-character Prettier width, while preserving all existing property values and test behavior.Source: Coding guidelines
backend/src/routes/events.ts (1)
222-230: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
enforceAccessCacheEntryCapruns on every cache insert.
canAccesscalls this function on each new SSAR entry at Line 1215. Each call allocates a fullObject.keys()array. With the cap at 2000 entries per token and a high SSE event rate, this repeats a 2000-element allocation per authorization miss. Once the cache exceeds the cap, every insert also sorts the key array.Consider enforcing the cap only when the size crosses the limit, or tracking insertion order so eviction is O(1). The periodic
cleanupAccessCachealready enforces the cap at Line 254, so the per-insert call is a safety net rather than the primary mechanism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/events.ts` around lines 222 - 230, Optimize enforceAccessCacheEntryCap so canAccess does not allocate and sort the full tokenCache key set on every insert. Track the entry count or otherwise check the cache size before creating keys, and only perform eviction when the cap is exceeded; preserve cleanupAccessCache as the periodic enforcement path and retain eviction of the oldest entries when the safety-net runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/lib/server-side-events.ts`:
- Around line 343-346: Update the compression metrics in the event-send flow
around the values initialized from this.events and the uncompressed calculation
near sendEvent so the logged compression field is no longer derived from two
deflated payload sizes. Prefer removing the misleading ratio, or compute it
using a genuinely inflated source while preserving the existing event delivery
behavior.
In `@backend/src/routes/events.ts`:
- Around line 259-268: Update cleanupAccessCache so subjectRulesCache and
kindGetAccessCache are also bounded by ACCESS_CACHE_MAX_TOKENS, in addition to
their existing TTL cleanup. Apply the same token-based eviction strategy used
for accessCache, preserving the per-entry data and ensuring both caches cannot
grow beyond the configured cap.
- Around line 1095-1098: Update the SelfSubjectRulesReview failure handling
around the catch block and evaluateKindGetAccess to distinguish unavailable
reviews from genuinely empty rule sets. Mark fetch failures explicitly, classify
that state before the empty resourceRules case so applyKindGetAccess invokes
onIncomplete and falls back to per-object SSAR, and skip caching failed results
in the ACCESS_CACHE_TTL path.
In `@backend/test/lib/server-side-events.test.ts`:
- Around line 104-138: Update the assertion in the “inflates events only after
the filter allows them” test to verify that inflateEvent was called with the
supplied MODIFIED event, rather than merely checking that it was called.
Distinguish the MODIFIED event from the additional LOADED event emitted by
ServerSideEvents.pushEvent while preserving the existing filter and setup.
In `@backend/test/routes/events.test.ts`:
- Around line 1694-1699: Capture the Nock scope returned by the
selfsubjectaccessreviews mock in the test surrounding canGetResource, then
assert ssarScope.isDone() after the access-result assertion to verify the
fallback request was consumed.
---
Nitpick comments:
In `@backend/src/routes/events.ts`:
- Around line 222-230: Optimize enforceAccessCacheEntryCap so canAccess does not
allocate and sort the full tokenCache key set on every insert. Track the entry
count or otherwise check the cache size before creating keys, and only perform
eviction when the cap is exceeded; preserve cleanupAccessCache as the periodic
enforcement path and retain eviction of the oldest entries when the safety-net
runs.
In `@backend/test/lib/server-side-events.test.ts`:
- Around line 58-66: Add an afterAll hook in the server-side-events test suite
that awaits ServerSideEvents.dispose() to clear the intervalTimer created by
ServerSideEvents.reset(). Keep the existing beforeEach and afterEach cleanup
unchanged.
- Line 129: Reformat the object literal in the server-side events test around
the ManagedCluster fixture so no line exceeds the project’s 120-character
Prettier width, while preserving all existing property values and test behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56eab48d-a1e6-4ef7-848c-068f26c6d421
📒 Files selected for processing (5)
backend/src/lib/compression.tsbackend/src/lib/server-side-events.tsbackend/src/routes/events.tsbackend/test/lib/server-side-events.test.tsbackend/test/routes/events.test.ts
| // Classify using meta / inflated object identity — do not inflate the whole cache up front. | ||
| const values = Object.values(this.events) | ||
| const compressed = sizeOf(values) | ||
| let parts = await batchPromiseAll(values, (event) => inflateEvent(event)) | ||
| let parts: ServerSideEvent[] = [...values] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The logged compression ratio is no longer meaningful.
compressed at Line 345 measures the cached events while their object fields are still deflated buffers. Because inflation now happens inside sendEvent, uncompressed = sizeOf(sending) at Line 463 measures those same compressed objects. Both sides of the ratio at Line 465 now measure compressed payloads, so the reported percentage collapses toward zero and no longer reports compression effectiveness.
Either drop the field or compute it from a source that is still inflated.
🔧 Proposed fix: report byte counts instead of a misleading ratio
- logger.info({ msg: 'event stream start', events: sentCount, compression: 100 - (compressed / uncompressed) * 100 })
+ logger.info({ msg: 'event stream start', events: sentCount, cachedBytes: compressed, sentBytes: uncompressed })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/lib/server-side-events.ts` around lines 343 - 346, Update the
compression metrics in the event-send flow around the values initialized from
this.events and the uncompressed calculation near sendEvent so the logged
compression field is no longer derived from two deflated payload sizes. Prefer
removing the misleading ratio, or compute it using a genuinely inflated source
while preserving the existing event delivery behavior.
| for (const key in subjectRulesCache) { | ||
| if (subjectRulesCache[key].time < cutoffTime) { | ||
| delete subjectRulesCache[key] | ||
| } | ||
| } | ||
| for (const key in kindGetAccessCache) { | ||
| if (kindGetAccessCache[key].time < cutoffTime) { | ||
| delete kindGetAccessCache[key] | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
The two new caches are not bounded by ACCESS_CACHE_MAX_TOKENS.
cleanupAccessCache caps accessCache at ACCESS_CACHE_MAX_TOKENS tokens at Lines 270-277, and caps entries per token at Line 254. It applies only TTL expiry to subjectRulesCache and kindGetAccessCache. Those two caches therefore grow with token churn until the next cleanup run.
The exposure is bounded but larger than the capped path. ACCESS_CACHE_CLEANUP_INTERVAL is 90 seconds and ACCESS_CACHE_TTL is 60 seconds, so an entry can live about 150 seconds. kindGetAccessCache is keyed by hash:kind:apiVersion, so each distinct token contributes one entry per watched kind. allow-names entries also retain a Set of resource names. Under high token churn this escapes the token cap that this PR adds.
Apply a size cap to both caches, consistent with the accessCache treatment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/events.ts` around lines 259 - 268, Update
cleanupAccessCache so subjectRulesCache and kindGetAccessCache are also bounded
by ACCESS_CACHE_MAX_TOKENS, in addition to their existing TTL cleanup. Apply the
same token-based eviction strategy used for accessCache, preserving the
per-entry data and ensuring both caches cannot grow beyond the configured cap.
| .catch((err: unknown) => { | ||
| logger.warn({ msg: 'SelfSubjectRulesReview failed; falling back to per-object SSAR', error: err }) | ||
| return { incomplete: true, resourceRules: [] as SubjectRulesStatus['resourceRules'] } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A failed SelfSubjectRulesReview denies all events instead of falling back.
The catch handler returns { incomplete: true, resourceRules: [] }. The log message states that the code falls back to per-object SSAR, but it does not.
evaluateKindGetAccess evaluates case rules.resourceRules.length === 0 at Line 1137 before case rules.incomplete at Line 1140. An empty rule list therefore produces { type: 'deny-all' }, and applyKindGetAccess returns false at Line 1054 without calling onIncomplete. The failed result is also cached at Line 1100 for ACCESS_CACHE_TTL.
Consequence: if the selfsubjectrulesreviews request fails, every ADDED and MODIFIED event is denied for that token for 60 seconds. Non-admin users see a silently empty console. Admins are unaffected because the cluster-scoped list fast path at Line 991 succeeds first.
Distinguish a fetch failure from a genuinely empty rule set, and do not cache the failure.
🐛 Proposed fix: mark the failure explicitly and skip caching it
Add a discriminator to the status type:
interface SubjectRulesStatus {
incomplete: boolean
+ /** True when the SelfSubjectRulesReview request itself failed. */
+ unavailable?: boolean
resourceRules: Array<{Then classify and cache accordingly:
.catch((err: unknown) => {
logger.warn({ msg: 'SelfSubjectRulesReview failed; falling back to per-object SSAR', error: err })
- return { incomplete: true, resourceRules: [] as SubjectRulesStatus['resourceRules'] }
+ delete subjectRulesCache[cacheKey]
+ return { incomplete: true, unavailable: true, resourceRules: [] as SubjectRulesStatus['resourceRules'] }
})And order the classification so an unavailable review falls back:
switch (true) {
case allowAll:
return { type: 'allow-all' }
case names.size > 0:
return { type: 'allow-names', names }
+ // The review request failed; defer to the per-object SSAR fallback.
+ case rules.unavailable === true:
+ return { type: 'incomplete' }
// OpenShift often sets incomplete=true even when the user has no bindings and resourceRules
// is empty. Treat empty rules as deny-all so we do not fall back to O(N) namespaced SSARs.
case rules.resourceRules.length === 0:
return { type: 'deny-all' }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/events.ts` around lines 1095 - 1098, Update the
SelfSubjectRulesReview failure handling around the catch block and
evaluateKindGetAccess to distinguish unavailable reviews from genuinely empty
rule sets. Mark fetch failures explicitly, classify that state before the empty
resourceRules case so applyKindGetAccess invokes onIncomplete and falls back to
per-object SSAR, and skip caching failed results in the ACCESS_CACHE_TTL path.
| nock(process.env.CLUSTER_API_URL || '') | ||
| .post('/apis/authorization.k8s.io/v1/selfsubjectaccessreviews') | ||
| .reply(200, { status: { allowed: true } }) | ||
|
|
||
| expect(await canGetResource(managedCluster('cluster-1'), 'incomplete-user-token')).toBe(true) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)events\.test\.ts$|backend/test/routes/events\.test\.ts'
echo "== outline around file =="
wc -l backend/test/routes/events.test.ts
ast-grep outline backend/test/routes/events.test.ts --view compact 2>/dev/null | sed -n '1,220p' || true
echo "== relevant lines =="
sed -n '1660,1705p' backend/test/routes/events.test.ts
echo "== nock usage in test file =="
rg -n "nock\\(|\\.post\\(|isDone\\(|afterEach|after\\(|beforeEach|before\\(" backend/test/routes/events.test.ts | sed -n '1,240p'Repository: stolostron/console
Length of output: 4984
Assert that the SSAR fallback mock is consumed.
This test only checks the access result. The configured /selfsubjectaccessreviews fallback request can be ignored while the test still passes. Store the Nock scope and assert ssarScope.isDone() after the result assertion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/test/routes/events.test.ts` around lines 1694 - 1699, Capture the
Nock scope returned by the selfsubjectaccessreviews mock in the test surrounding
canGetResource, then assert ssarScope.isDone() after the access-result assertion
to verify the fallback request was consumed.
Source: Coding guidelines
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/lib/compression.ts`:
- Around line 248-256: Update the object-bearing branch around watchEvent so it
preserves all existing event fields, including optional meta, while replacing
only object with its inflated value when needed. Avoid reconstructing data with
only type and object; retain the existing event structure for already-inflated
objects. Add a regression test covering an event containing meta.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d5a92168-62fb-477a-9074-0c34a6170bae
📒 Files selected for processing (5)
backend/src/lib/compression.tsbackend/src/lib/server-side-events.tsbackend/src/routes/events.tsbackend/test/lib/server-side-events.test.tsbackend/test/routes/events.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/test/lib/server-side-events.test.ts
- backend/src/lib/server-side-events.ts
- backend/test/routes/events.test.ts
- backend/src/routes/events.ts
| const watchEvent = data as WatchEvent & { meta?: unknown } | ||
| const { type, object } = watchEvent | ||
| return !object | ||
| ? event | ||
| : { id, data: { type, object: Buffer.isBuffer(object) ? await inflateResource(object, getEventDict()) : object } } | ||
| : { | ||
| id, | ||
| name, | ||
| namespace, | ||
| data: { type, object: Buffer.isBuffer(object) ? await inflateResource(object, getEventDict()) : object }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the complete watch event during inflation.
WatchEvent declares optional meta, but Line [256] rebuilds data with only type and object. This drops meta from every object-bearing event, including events whose object is already inflated. Preserve the existing event and replace only object.
Proposed fix
- const { id, name, namespace, data } = event
+ const { data } = event
...
- id,
- name,
- namespace,
- data: { type, object: Buffer.isBuffer(object) ? await inflateResource(object, getEventDict()) : object },
+ ...event,
+ data: {
+ ...watchEvent,
+ object: Buffer.isBuffer(object) ? await inflateResource(object, getEventDict()) : object,
+ },Add a regression test for an event containing meta.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const watchEvent = data as WatchEvent & { meta?: unknown } | |
| const { type, object } = watchEvent | |
| return !object | |
| ? event | |
| : { id, data: { type, object: Buffer.isBuffer(object) ? await inflateResource(object, getEventDict()) : object } } | |
| : { | |
| id, | |
| name, | |
| namespace, | |
| data: { type, object: Buffer.isBuffer(object) ? await inflateResource(object, getEventDict()) : object }, | |
| const { data } = event | |
| const watchEvent = data as WatchEvent & { meta?: unknown } | |
| const { type, object } = watchEvent | |
| return !object | |
| ? event | |
| : { | |
| ...event, | |
| data: { | |
| ...watchEvent, | |
| object: Buffer.isBuffer(object) ? await inflateResource(object, getEventDict()) : object, | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/lib/compression.ts` around lines 248 - 256, Update the
object-bearing branch around watchEvent so it preserves all existing event
fields, including optional meta, while replacing only object with its inflated
value when needed. Avoid reconstructing data with only type and object; retain
the existing event structure for already-inflated objects. Add a regression test
covering an event containing meta.
|
|
@Ginxo: The following tests 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. |
1 participant
Summary
Fixes ACM-39327:
console-mce-console*/ local backend OOM and CPU saturation for non-admin users when the SSE/eventsstream filters a large inventory (reproduced withMOCK_CLUSTERS=1000).Restricted users do not short-circuit on cluster-scoped
list. The previous path fell through to namespacedlistand per-objectgetSelfSubjectAccessReviews (O(N)), while also inflating every cached resource before the access check. Admins were largely unaffected because cluster-scopedlistsucceeds once per kind.Approach
Compression + lightweight metadata
cacheResource/ delete attachmeta: { kind, apiVersion, name, namespace }so RBAC and kind classification do not require inflate.getEventResourceMeta()readsmeta, or falls back to an already-inflated object.inflateEventstripsmetafrom the wire payload (clients still receive{ type, object }only).Filter before inflate (
server-side-events)sendEventrunseventFilterbeforeinflateEvent.metainstead of bulk-inflating the entire cache up front.Access cache (
resetAccessCache/cleanupAccessCache/ per-token keys)verb:kind:namespace:name.resetAccessCache/cleanupAccessCachealso clear SelfSubjectRulesReview / kind-access caches and enforce TTLs.resolveKindGetAccess+applyKindGetAccessvscanAccesslistis denied, do not immediately run O(namespaces) namespacedlist+ O(N)getSSARs.SelfSubjectRulesReviewper token (namespacedefault; ClusterRole bindings are included) →resolveKindGetAccessper kind.applyKindGetAccessmaps the result:deny-all/allow-all/allow-names→ local decision (no per-object SSAR)resourceRules(including OpenShiftincomplete: truewith empty rules) →deny-all(typicalnoneuser)incomplete→ fallback to namespaced list /canAccessgetcanAccessremains the SSAR primitive for list checks and incomplete fallback only.Performance comparison (
MOCK_CLUSTERS=1000, local backend)RSS from
ps(KiB → MiB). Same Node backend process; Inventory exercises the full SSE filter path./welcome/inventorynone)/welcomenone)/inventoryTakeaway: admin inventory cost stays in the same ballpark. Non-admin inventory no longer diverges into multi‑GiB RSS and sustained high CPU; restricted-user filtering is bounded by rules review + cheap denies instead of O(N) SSARs and inflate-before-filter.
Test plan
getEventResourceMeta+ filter-before-inflate (no inflate on deny)MOCK_CLUSTERS=1000, kubeadmin vsnoneon/multicloud/infrastructure/environments(Inventory); confirm RSS/CPU stay boundednonecompletes SSE load (empty inventory expected) without backend hangMade with Cursor
Summary by CodeRabbit
New Features
Performance & Security
Bug Fixes