frontend: api/v2: Include class identity in kube object query keys#5716
frontend: api/v2: Include class identity in kube object query keys#5716WasThatRudy wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: WasThatRudy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR updates frontend Kubernetes API v2 React Query cache keys so KubeObject subclasses targeting the same Kubernetes endpoint do not unintentionally share cached objects, addressing wrong _class()/details route behavior for plugin-defined resource classes.
Changes:
- Adds a class-name discriminator to list and single-object query keys.
- Threads the discriminator through object-link cache prepopulation.
- Updates list-query tests and adds cache-key coverage for distinct classes.
CI status and PR commit history were not available in this review context.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
frontend/src/lib/k8s/api/v2/useKubeObjectList.ts |
Adds class-name slot to list query keys. |
frontend/src/lib/k8s/api/v2/useKubeObjectList.test.tsx |
Updates expected list keys and adds key identity tests. |
frontend/src/lib/k8s/api/v2/hooks.ts |
Adds optional class-name slot to single-object query keys. |
frontend/src/components/common/Link.tsx |
Uses the object class name when prepopulating object cache entries. |
Comments suppressed due to low confidence (1)
frontend/src/lib/k8s/api/v2/hooks.ts:157
- The memoized key now depends on
kubeObjectClass.name, but the dependency list was not updated. If a component re-rendersuseKubeObjectwith a different class for the same endpoint/namespace/name, React will keep using the previous class-specific query key, so this change will not separate the object cache for that render path.
kubeObjectQueryKey({
cluster,
name,
namespace,
endpoint,
queryParams: cleanedUpQueryParams,
kubeObjectClassName: kubeObjectClass.name,
}),
// eslint-disable-next-line react-hooks/exhaustive-deps
[endpoint, namespace, name]
24044ef to
4749d06
Compare
|
Pushed Replaced Added a regression test ( |
4749d06 to
a9e9b64
Compare
|
both follow-ups are addressed in a9e9b64:
cc @illume — small follow-up push, no behavior change to the list-side fix. |
a9e9b64 to
798f740
Compare
798f740 to
d466e74
Compare
d466e74 to
ea87ee3
Compare
ea87ee3 to
aafefa6
Compare
aafefa6 to
aaed0dc
Compare
64eabb9 to
bfab3d9
Compare
bfab3d9 to
6e2748c
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
The GitHub CI test job has snapshot failures. Run cd frontend && npm run test -- -u to regenerate the snapshots.
How to update snapshots
Run cd frontend && npm run test -- -u to regenerate all snapshots. Review the diff to make sure the visual changes are intentional, then commit the updated snapshot files.
The query keys used by `useKubeObjectList` and `useKubeObject` are derived from `apiVersion` + `apiName` + cluster + namespace + name + queryParams. They omit the class itself, so two distinct KubeObject subclasses that target the same API endpoint share a cache entry. Concretely: a plugin defines `class MyPod extends KubeObject` with `apiVersion = 'v1'` and `apiName = 'pods'` to render its own detail route. Headlamp's built-in Pod uses the same `apiVersion` and `apiName`. The two `useList()` calls produce the same query key, so react-query runs only one queryFn and serves both call sites from the first class's cached items. `_class()`, `detailsRoute`, and `getDetailsLink()` then resolve to whichever class loaded first rather than the class the caller actually used. Adds `kubeObjectClass.name` to the list query key in `kubeObjectListQuery` and threads a new `kubeObjectClassName` field through `kubeObjectQueryKey` for the single-object hook. Updates the two callers of `kubeObjectQueryKey` (`useKubeObject` and `Link`) to pass the class name. The hardcoded query-key arrays in `useKubeObjectList.test.tsx` are updated to match the new shape. Adds a `kubeObjectListQuery query key` describe block covering two cases: two distinct classes targeting the same endpoint produce distinct keys, and the same class with the same arguments produces identical keys. Note: this fix relies on plugin-authored classes carrying a JS class name that differs from the built-in's (the reported repro uses `MyPod` vs `Pod`). Plugins that define a custom class with the same JS name as a built-in will still collide; a follow-up could expose an explicit static cache-key field on `KubeObject` for that edge case. Bug report: kubernetes-sigs#4780. Signed-off-by: Rudraksha Singh Sengar <rudraksharss@gmail.com>
6e2748c to
b3598e0
Compare
There was a problem hiding this comment.
this change currently breaks all the snapshot tests, rendering things like No data to be shown.
please make sure that snapshots stay the same
I'm also not sure if this is a good direction for us to take, we should avoid situations where same resource is represented by different class implementations
b3598e0 to
847373c
Compare
|
Hi @illume @sniok, two updates: Snapshots: the breakage sniok saw was self-inflicted by an earlier commit of mine that regenerated snapshots on Node 24 locally. Node 24's stricter Architectural concern @sniok raised: today the cache silently swaps a plugin's class for the built-in one whenever they share an endpoint (the #4780 repro: If the longer-term direction is "plugins shouldn't be allowed to define a second class against an endpoint that already has one", that's an additive guard (warn or reject at registration time) and worth doing separately, but it doesn't subsume this fix, because users who already shipped such plugins still hit the wrong-class bug today. Happy to follow up with a registration-time guard in a separate PR if that's the direction you want. Let me know your thoughts. |
No plugins have hit this issue. Redefining built-in classes doesn't really make sense although technically possible today |
|
@sniok you're right, sorry for pushing back. I leaned on a user impact claim I didn't actually have. The component level approach covers what someone would reach for a subclass to do, without the collision in the first place: use the built in Plan: close this PR. If it's useful I can follow up with a small change that warns or rejects at registration time when a plugin tries to register a second class against an endpoint that already has one, plus a docs note pointing plugin authors at the component pattern. Or just close and skip the follow up if that's noise. Let me know which you prefer. |
Summary
useKubeObjectListanduseKubeObjectbuild their react-query cache keys fromapiVersion+apiName+ cluster + namespace + name + queryParams. The keys do not include the class itself, so two distinct KubeObject subclasses that target the same API endpoint share a cache entry. Whichever class'suseList()resolves first wins, and the second class reads back items wrapped as the wrong class._class(),detailsRoute, andgetDetailsLink()then resolve incorrectly for the second caller.The reported repro: a plugin defines
class MyPod extends KubeObjectwithapiVersion = 'v1'andapiName = 'pods'and its own detail route. Visiting the built-in Pods list first, then the plugin'sMyPodslist, makes the plugin's row links point at the built-in's detail page.Related Issue
Fixes #4780.
Changes
frontend/src/lib/k8s/api/v2/queryKeys.ts(new): houseskubeObjectQueryKey,getKubeObjectClassCacheKey, andgetWebsocketMultiplexerEnabledso non-hook callers don't have to import from hook-centric modules. The discriminator is identity-based: aWeakMap<KubeObjectClass, string>stored onglobalThisunderSymbol.for('headlamp.kubeObjectClassCacheKey')maps each class constructor to acls-<uuid>-<displayName>string.Symbol.forkeys the registry in the global Symbol registry, so the plugin SDK's bundled copy offrontend/src/lib/**reads and writes the sameWeakMapas the app.crypto.randomUUID()is used when available, with aDate.now() + Math.random()fallback for environments that lack it.frontend/src/lib/k8s/api/v2/useKubeObjectList.ts: threadsgetKubeObjectClassCacheKey(kubeObjectClass)into thequeryKeyproduced bykubeObjectListQuery. Two distinct constructors that happen to share a JS.nameget distinct discriminators.frontend/src/lib/k8s/api/v2/hooks.ts: extendskubeObjectQueryKeywith an optionalkubeObjectClassCacheKeyfield;useKubeObjectpassesgetKubeObjectClassCacheKey(kubeObjectClass). ThequeryKeyuseMemo lists[cluster, endpoint, namespace, name, kubeObjectClass, cleanedUpQueryParams]as deps.cleanedUpQueryParamsis built by a newuseStableCleanedQueryParamshelper that proxies through a stringified key derived from cleaned + sorted entries, so structurally equal queryParams (different ref / same content; key-order differences; filtered-out empty values) don't churn the downstream legacy WebSocket. The legacyconnectionsRequestsmemo lists every value it captures so the watch callback can't drift away from the active key.frontend/src/components/common/Link.tsx: updated the existingkubeObjectQueryKeycall site to passgetKubeObjectClassCacheKey(kubeObject._class())when prepopulating the cache from a known instance, so the prepopulated entry lives at the same key the correspondinguseKubeObjecthook reads from.frontend/src/lib/k8s/api/v2/useKubeObjectList.test.tsx: refactored the four hardcodedsetQueryDataarray literals to call the exportedkubeObjectListQuery(...)so the test no longer hard-codes the key shape. Added adescribe('kubeObjectListQuery query key', ...)block: two distinct classes that share a JS class name targeting the same endpoint produce distinct keys, and the same class with the same arguments produces identical keys.frontend/src/lib/k8s/api/v2/hooks.test.tsx: added a regression test for the single-object path — twoclass Podconstructors with the same.nameand the same v1/pods endpoint produce distinct'object'cache entries, and eachuseKubeObject'sdatais aninstanceofthe class that requested it (not the other). Added avi.resetModules-based test that the discriminator agrees across separate module instances ofqueryKeys(surrogate for the plugin SDK bundle duplication). Added four tests foruseStableCleanedQueryParamscovering: same reference when content unchanged, key-order independence, undefined/empty-value equivalence, and reference change on real content change.Steps to Test
cd frontend && npm installnpm run lint: passes.npm run tsc: passes.npx vitest run src/lib/k8s/api/v2/useKubeObjectList.test.tsx src/lib/k8s/api/v2/hooks.test.tsx: all tests pass (existing + new regressions)./c/<cluster>/podsfirst, then the plugin's/my-plugin/pods. Row name links in the plugin's table point at/c/<cluster>/pods/...(the built-in detail route).MyPod.detailsRoute.Screenshots
N/A. The fix is in the cache layer; the user-visible effect is correct detail-route links.
Notes for the Reviewer
globalThisunderSymbol.for(...)rather than module-local state so that the plugin SDK's bundled copy offrontend/src/lib/**shares the same class→id mapping as the app — otherwise each bundle would get its own counter/map and two distinct constructors in different bundles could collide on the same discriminator string.