-
Notifications
You must be signed in to change notification settings - Fork 125
ACM-44885: Fix non-admin SSE OOM under large inventory #6638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6a69242
c35ff3f
b42bb19
2baff1b
6f6bd31
9c5efd3
2ced7d5
cd7932b
2f1da6e
3c9f829
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ import { constants } from 'node:http2' | |
| import type { Transform } from 'node:stream' | ||
| import { clearInterval } from 'node:timers' | ||
| import type { Zlib } from 'node:zlib' | ||
| import { batchPromiseAll } from './batch-promise-all' | ||
| import { getEncodeStream, inflateEvent } from './compression' | ||
| import { setCookie } from './cookies' | ||
| import { logger } from './logger' | ||
|
|
@@ -35,6 +34,15 @@ export interface ServerSideEvent<DataT = unknown> { | |
| namespace?: string | ||
| data?: DataT | ||
| } | ||
|
|
||
| /** Lightweight resource identity for RBAC filtering without inflating compressed objects. */ | ||
| export interface EventResourceMeta { | ||
| kind: string | ||
| apiVersion: string | ||
| name?: string | ||
| namespace?: string | ||
| } | ||
|
|
||
| export interface WatchEvent { | ||
| type: 'ADDED' | 'DELETED' | 'MODIFIED' | 'EOP' | ||
| object: { | ||
|
|
@@ -46,6 +54,24 @@ export interface WatchEvent { | |
| resourceVersion: string | ||
| } | ||
| } | ||
| meta?: EventResourceMeta | ||
| } | ||
|
|
||
| /** Resolve kind/apiVersion/name/namespace from meta or an already-inflated object. */ | ||
| export function getEventResourceMeta(event: ServerSideEvent): EventResourceMeta | undefined { | ||
| const data = event.data as (WatchEvent & { type?: string }) | undefined | ||
| if (!data || typeof data !== 'object') return undefined | ||
| if (data.meta?.kind) return data.meta | ||
| const object = data.object as WatchEvent['object'] | Buffer | undefined | ||
| if (object && !Buffer.isBuffer(object) && typeof object === 'object' && object.kind) { | ||
| return { | ||
| kind: object.kind, | ||
| apiVersion: object.apiVersion, | ||
| name: object.metadata?.name, | ||
| namespace: object.metadata?.namespace, | ||
| } | ||
| } | ||
| return undefined | ||
| } | ||
|
|
||
| export interface ServerSideEventClient { | ||
|
|
@@ -125,22 +151,26 @@ export class ServerSideEvents { | |
| } | ||
| } | ||
|
|
||
| private static async sendEvent(clientID: string, event: ServerSideEvent): Promise<void> { | ||
| private static sendEvent(clientID: string, event: ServerSideEvent): Promise<void> { | ||
| const client = this.clients[clientID] | ||
| if (!client) return | ||
| if (client.events && !client.events[event.name]) return | ||
| if (client.namespaces && !client.namespaces[event.namespace]) return | ||
| event = await inflateEvent(event) | ||
| if (!client) return Promise.resolve() | ||
| if (client.events && !client.events[event.name]) return Promise.resolve() | ||
| if (client.namespaces && !client.namespaces[event.namespace]) return Promise.resolve() | ||
| // Filter before inflate so denied events never materialize full resource JSON in memory. | ||
| if (this.eventFilter) { | ||
| client.eventQueue.push( | ||
| this.eventFilter(client.token, event) | ||
| .then((shouldSendEvent) => (shouldSendEvent ? event : undefined)) | ||
| .then((shouldSendEvent) => { | ||
| if (!shouldSendEvent) return undefined | ||
| return inflateEvent(event) | ||
| }) | ||
| .catch((): undefined => undefined) | ||
| ) | ||
| } else { | ||
| client.eventQueue.push(Promise.resolve(event)) | ||
| client.eventQueue.push(inflateEvent(event)) | ||
| } | ||
| void this.processClient(clientID) | ||
| return Promise.resolve() | ||
| } | ||
|
|
||
| private static async processClient(clientID: string): Promise<void> { | ||
|
|
@@ -311,10 +341,10 @@ export class ServerSideEvents { | |
|
|
||
| // SORT EVENTS INTO SMALLER PACKETS | ||
| // SO THAT BROWSER PAGE LOADS QUICKER | ||
| // uncompress and split events into packets | ||
| // 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] | ||
|
Comment on lines
+344
to
+347
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win The logged
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 |
||
|
|
||
| // mock a large environment | ||
| if (process.env.MOCK_CLUSTERS) { | ||
|
|
@@ -343,9 +373,9 @@ export class ServerSideEvents { | |
| const other: ServerSideEvent<unknown>[] = [] | ||
| const remainder: ServerSideEvent<unknown>[] = [] | ||
| parts.forEach((event) => { | ||
| const data = event.data as WatchEvent | ||
| const meta = getEventResourceMeta(event) | ||
| // see frontend/src/components/LoadPluginData.tsx for what pages are fast loaded | ||
| switch (data.object.kind) { | ||
| switch (meta?.kind) { | ||
| case 'ManagedCluster': | ||
| case 'HostedCluster': | ||
| case 'ClusterDeployment': | ||
|
|
@@ -384,9 +414,9 @@ export class ServerSideEvents { | |
| // sort events alphabetically so that browser list fills from top to bottom | ||
| const compareFn = | ||
| (propName: 'name' | 'namespace') => (a: ServerSideEvent<unknown>, b: ServerSideEvent<unknown>) => { | ||
| const adata = a.data as WatchEvent | ||
| const bdata = b.data as WatchEvent | ||
| return adata.object.metadata[propName].localeCompare(bdata.object.metadata[propName]) | ||
| const aVal = getEventResourceMeta(a)?.[propName] ?? '' | ||
| const bVal = getEventResourceMeta(b)?.[propName] ?? '' | ||
| return aVal.localeCompare(bVal) | ||
| } | ||
| clusters.sort(compareFn('name')) | ||
| infos.sort(compareFn('namespace')) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the complete watch event during inflation.
WatchEventdeclares optionalmeta, but Line [256] rebuildsdatawith onlytypeandobject. This dropsmetafrom every object-bearing event, including events whose object is already inflated. Preserve the existing event and replace onlyobject.Proposed fix
Add a regression test for an event containing
meta.📝 Committable suggestion
🤖 Prompt for AI Agents