frontend: crd: Guard against incomplete CRD spec in makeCRClass#5717
frontend: crd: Guard against incomplete CRD spec in makeCRClass#5717WasThatRudy 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 |
3895192 to
882f9fc
Compare
|
Pushed Refactored |
There was a problem hiding this comment.
Pull request overview
This PR hardens CRD handling in the frontend by making CustomResourceDefinition.makeCRClass() and related helpers resilient to transient/incomplete CRD specs (e.g., partial watch updates), preventing dashboard crashes like Cannot read properties of undefined (reading 'names'|'versions').
Changes:
- Made
CustomResourceDefinition.makeCRClass()returnnull(with a warning) instead of throwing when required CRD spec fields are missing, and added safer defaults togetMainAPIGroup(),plural, andisNamespacedScope. - Updated CRD consumers (CRD details/list/resource map) to handle a nullable CR class by skipping incomplete CRDs or showing a loader.
- Adjusted typing at call sites to enforce null-handling via TypeScript.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/lib/k8s/crd.ts | Adds defensive guards/defaults and makes makeCRClass() nullable to avoid crashes on partial CRD specs. |
| frontend/src/components/crd/CustomResourceDetails.tsx | Shows a loader when makeCRClass() returns null and avoids calling useGet without a valid class. |
| frontend/src/components/crd/CustomResourceInstancesList.tsx | Skips CRDs with incomplete specs when building per-CRD list queries. |
| frontend/src/components/crd/CustomResourceList.tsx | Widens CRClass type to `... |
| frontend/src/components/resourceMap/sources/definitions/sources.tsx | Skips creating graph sources for CRDs whose makeCRClass() returns null. |
| frontend/src/components/resourceMap/sources/definitions/relations.tsx | Drops owner relations for CRDs that can’t build a class due to incomplete spec. |
Comments suppressed due to low confidence (2)
frontend/src/components/crd/CustomResourceInstancesList.tsx:38
- Switching from
maptoflatMaphere breaks positional alignment betweenqueries/dataClassCrdsand the originalcrdsarray. Later code indexescrds[i]/crds[index]when iterating overqueries, which will now refer to the wrong CRD (or the wrong name in the warning) whenever any CRD is skipped. Use the filtereddataClassCrds[i].crdinstead ofcrds[i], or avoid filtering before building these index-based mappings.
const dataClassCrds = crds.flatMap(crd => {
const crdClass = crd.makeCRClass();
if (!crdClass) {
// CRD with incomplete spec; skip rather than crash on useList (#4824).
return [];
}
const data = crdClass.useList({ cluster: crd.cluster, namespace: namespaces });
return [{ data, crdClass, crd }];
});
frontend/src/components/crd/CustomResourceInstancesList.tsx:38
- There are existing component tests for this file, but none cover the new “skip incomplete CRD” behavior introduced by returning
[]whenmakeCRClass()returnsnull. Adding a test case with one CRD returningnullwould validate both the skip behavior and that the warning/error mapping still refers to the correct CRD.
const dataClassCrds = crds.flatMap(crd => {
const crdClass = crd.makeCRClass();
if (!crdClass) {
// CRD with incomplete spec; skip rather than crash on useList (#4824).
return [];
}
const data = crdClass.useList({ cluster: crd.cluster, namespace: namespaces });
return [{ data, crdClass, crd }];
});
882f9fc to
121f373
Compare
|
Pushed
|
121f373 to
89b7a2e
Compare
|
all five follow-ups are addressed in 89b7a2e:
cc @illume — pushed as an amend; lint, tsc, and the relevant suites ( |
89b7a2e to
8d8c313
Compare
8d8c313 to
4a43711
Compare
f5bebd0 to
6f008f8
Compare
646d118 to
81263ed
Compare
81263ed to
9972482
Compare
9972482 to
45d533d
Compare
45d533d to
8ba2394
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for these changes.
It looks like this PR has git conflicts. Can you please fix them?
How to resolve conflicts
Rebase or merge the latest main into your branch, resolve the conflicts, and push the updated branch.
`CustomResourceDefinition.makeCRClass`, `getMainAPIGroup`, the
`plural` getter, and `isNamespacedScope` all assumed `this.spec`,
`this.spec.versions`, and `this.spec.names` were fully populated.
When a CRD reaches one of these methods with a partial spec (a
transient watch update or in-flight refetch can leave the field
undefined for a moment) the nested access throws and the whole
dashboard view crashes with `Cannot read properties of undefined
(reading 'names')` or `(reading 'versions')`.
Adds defensive guards to all four:
- `makeCRClass()` now returns `typeof KubeObject<KubeCRD> | null`.
It logs a `console.warn` and returns null when spec is missing
or has no versions/names, instead of crashing on the map.
- `getMainAPIGroup()` returns `['', '', '']` when spec is
undefined.
- `get plural()` returns `''` when spec.names is missing.
- `get isNamespacedScope()` returns false when spec is missing.
All five production call sites of `makeCRClass()` now handle the
nullable return:
- `CustomResourceDetails.tsx` skips `useGet` and renders the
existing Loader while the spec is incomplete.
- `CustomResourceInstancesList.tsx` flat-maps to drop CRDs that
haven't fully loaded, so the list view continues to render the
healthy CRDs.
- `CustomResourceList.tsx` widens the local type annotation; the
`if (!CRClass)` branch already in the file renders an empty
state.
- `resourceMap/sources/definitions/sources.tsx` skips the CRD
rather than creating a broken graph source.
- `resourceMap/sources/definitions/relations.tsx` flat-maps to
drop incomplete CRDs.
A unit test for `makeCRClass()` would have been ideal but is blocked
by a pre-existing circular-import issue in `lib/k8s/`: `crd.ts`
imports `ResourceClasses` from `./index`, which transitively loads
`deployment.ts` and `replicaSet.ts` in a cycle that vitest cannot
resolve. The existing component tests in `crd/` mock the crd module
entirely; they continue to pass on this branch (7/7).
This is a defensive fix at the consumer boundary. The deeper question
of why a CRD reaches these methods with a partial spec (websocket
ADDED with incomplete payload, query cache trimming, etc.) is left
for a follow-up that needs reproduction or maintainer guidance.
Bug report: kubernetes-sigs#4824.
Signed-off-by: Rudraksha Singh Sengar <rudraksharss@gmail.com>
|
@illume done. i have rebased the PR, it does not have conflicts anymore. |
Summary
CustomResourceDefinition.makeCRClass,getMainAPIGroup, thepluralgetter, andisNamespacedScopeall assumedthis.spec,this.spec.versions, andthis.spec.nameswere fully populated. When a CRD reaches one of these methods with a partial spec (a transient watch update or in-flight refetch can leave the field undefined for a moment), the nested access throws and the whole dashboard view crashes withTypeError: Cannot read properties of undefined (reading 'names')or(reading 'versions').Related Issue
Fixes #4824.
Changes
frontend/src/lib/k8s/crdSpec.ts(new)Pure-helper module extracted so the validation and version-selection logic can be unit tested without instantiating
CustomResourceDefinition(which transitively triggers a circular import inlib/k8s/index.ts).CRDSpecLike,CRDVersionLike,UsableCRDVersioninterfaces describing the subset of the CRD schema the helpers depend on.validateCRDSpec(spec)returns{ ok, missing, usableVersions }.ok=falselists the missing required fields.usableVersionsis the subset filtered toname && served === true.selectMainAPIGroup(spec)returns[group, version, plural]ornull. Prefers the storage version, falls back to the first served version, honours the v1beta1spec.versionsingle-version shape only whenspec.versions[]is empty or whenspec.versionmatches a served entry.Both helpers are pure: no logging, no module-level state, safe to call from render paths.
frontend/src/lib/k8s/crd.tsmakeCRClass()keeps its original non-nullable signature (typeof KubeObject<KubeCRD>). Throws with a clear error listing the missing fields when the spec is incomplete, so plugin/library consumers see the same kind of failure as before Crash Report: Cannot read properties of undefined (reading 'names') #4824 with a more diagnostic message and a pointer to the new API.makeCRClassOrNull()(new) returnstypeof KubeObject<KubeCRD> | nullfor callers that want to render a non-error UI state for partially-loaded or malformed CRDs. Pure: no console side effects, safe insideuseMemo.names.singularis intentionally not required (Kubernetes server defaults it fromkind); when absent we fall back tospec.names.kind.toLowerCase().getMainAPIGroup()keeps its original non-nullable signature, returning['', '', '']for incomplete specs to preserve backward compatibility.getMainAPIGroupOrNull()(new) returns the same tuple ornullfor callers that need to distinguish "incomplete CRD" from genuinely-empty fields.get plural()uses optional chaining and returns''when the spec is incomplete; return type staysstringfor backward compatibility. Callers needing an explicit signal should usegetMainAPIGroupOrNull().get isNamespacedScope()uses optional chaining onspec.scope.makeCustomResourceClass.getBaseObject()consumesgetMainAPIGroupOrNull()and falls back toapiInfoArgs[0]when the spec is incomplete.Call sites updated to use the new nullable variants
frontend/src/components/crd/CustomResourceDetails.tsxEmptywith "incomplete spec" message) + inner that takes a non-nullCRClassprop.getExtraColumnsguardsspec?.versions?.find.frontend/src/components/crd/CustomResourceInstancesList.tsxCrInstanceList) into aclassifiedlist, sorts by${cluster}/${uid||name}so the child's per-entryuseListhook order is stable across refetches.remountKeyis a content-addressed fingerprint (two independent 32-bit hashes concatenated, no BigInt literals). When all CRDs are unusable, renders a non-loadingEmptyso users aren't stuck on an indefinite spinner.frontend/src/components/crd/CustomResourceList.tsxEmptywith "incomplete spec" message) + inner that takes them as non-null props.titlefalls back tocrd.metadata.namewhenkindis missing.frontend/src/components/resourceMap/sources/definitions/sources.tsxcontinuewhenspec?.names?.kindis missing; null guardsgetMainAPIGroupOrNull()andmakeCRClassOrNull().frontend/src/components/resourceMap/sources/definitions/relations.tsxflatMapdrops CRDs whosemakeCRClassOrNull()is null.frontend/src/components/resourceMap/sources/GraphSources.tsxmakeKubeObjectNodenull guardsgetMainAPIGroupOrNull()before destructuring.TypeScript enforces the null handling at compile time on the
*OrNullpaths;tscfails if any consumer of the new APIs forgets to handlenull.Tests
frontend/src/lib/k8s/crd.test.ts(new): 16 unit tests coveringselectMainAPIGroup(storage-vs-served precedence, served filtering, v1beta1spec.versionfallback, mismatchedspec.version, all-empty inputs) andvalidateCRDSpec(complete vs incomplete spec, missing-field reporting, usable-version filter, v1beta1 shape, undefined input).frontend/src/components/crd/CustomResourceDetails.test.tsx: asserts the empty-state message renders whenmakeCRClassOrNull()returns null.frontend/src/components/crd/CustomResourceInstancesList.test.tsx: asserts CRDs with nullmakeCRClassOrNull()are filtered out without crashing.Translations
The two new user-facing strings ("This CustomResourceDefinition has an incomplete spec." and "No CustomResourceDefinitions with usable specs were found.") are translated in all 15 non-English locales (ar, de, es, fr, he, hi, it, ja, ko, pt, ru, ta, ur, zh, zh-tw).
CustomResourceDefinitionis left untranslated since it's a Kubernetes API proper noun.Steps to Test
cd frontend && npm installnpm run lint: passes.npm run tsc: passes.npx vitest run src/components/crd src/lib/k8s/crd.test.ts: 25 tests pass.Cannot read properties of undefined (reading 'names')or(reading 'versions')and the whole view goes white.Screenshots
N/A. The fix prevents a crash; visible effect is the absence of one.
Notes for the Reviewer
makeCRClass()andgetMainAPIGroup()keep their pre-existing non-nullable signatures so existing plugin consumers compile unchanged. New*OrNull()variants are the recommended path for code that needs to handle the incomplete-spec state without throwing.ADDEDevent delivering an incomplete payload? an early useMemo evaluation? a query-cache eviction?) needs reproduction and is left as a follow-up.