fix(capi): use exact API-group matching in CAPI renderer guards - #1618
changyonggang wants to merge 1 commit into
Conversation
Three CAPI guards in ResourceRendererDispatch matched the API group with
`apiVersion?.includes("cluster.x-k8s.io")`. That also matches a foreign
group ending in the same suffix (e.g. `extension.cluster.x-k8s.io`), so
a generic CRD would render through a CAPI renderer, get a fabricated
CAPI status, or receive the "Topology-controlled" ClusterClass warning.
Replace the four remaining substring checks with `isApiGroup(..., "cluster.x-k8s.io")`,
matching the exact-group pattern already used for CAPI `clusters` and the
CNPG/Velero collision handling.
`machines` and `machinesets` are in KNOWN_KINDS, so the strict guard on
its own would render a blank drawer for a foreign CRD. Add a
`capiCollisionFallthrough` that routes such kinds to GenericRenderer,
mirroring the existing `groupGatedFallthrough` for `clusters` etc.
Regression coverage in ResourceRendererDispatch.test.tsx uses a foreign
`extension.cluster.x-k8s.io/v1` CRD and asserts, at all three boundaries,
that a substring guard would have failed:
- renderer dispatch for `clusters`, `machines`, `machinesets`
- `getResourceStatus` for `clusters` and `machines`
- the topology-controlled warning banner
plus a positive check that a real `cluster.x-k8s.io` resource still gets
the warning.
PR Summary by QodoUse exact CAPI API-group matching in renderer dispatch
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. machinesets status lacks group guard
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| {kind === 'machines' && isApiGroup(data?.apiVersion, 'cluster.x-k8s.io') && <CAPIMachineRenderer data={data} onNavigate={onNavigate} />} | ||
| {kind === 'machinedeployments' && <CAPIMachineDeploymentRenderer data={data} onNavigate={onNavigate} />} | ||
| {kind === 'machinesets' && data?.apiVersion?.includes('cluster.x-k8s.io') && <CAPIMachineSetRenderer data={data} onNavigate={onNavigate} />} | ||
| {kind === 'machinesets' && isApiGroup(data?.apiVersion, 'cluster.x-k8s.io') && <CAPIMachineSetRenderer data={data} onNavigate={onNavigate} />} |
There was a problem hiding this comment.
1. machinesets status lacks group guard 📘 Rule violation ≡ Correctness
The renderer now routes foreign machinesets CRDs to GenericRenderer, but getResourceStatus() still sends every resource with that plural through getMachineSetStatus(). This inconsistent collision handling causes the drawer header and Diagnose health to interpret unrelated CRD phases and conditions using CAPI semantics, potentially fabricating CAPI status.
Agent Prompt
## Issue description
Foreign CRDs using the `machinesets` plural are rendered generically, but their status is still calculated by the CAPI-specific `getMachineSetStatus()` function.
## Issue Context
The renderer, status calculation, and warning banner are independent dispatch boundaries. Apply the same exact `cluster.x-k8s.io` group guard used by the renderer and the `machines` status branch to the MachineSet status branch, then add regression coverage proving that a foreign `machinesets` resource receives generic status while a real CAPI MachineSet retains CAPI status.
## Fix Focus Areas
- packages/k8s-ui/src/components/shared/ResourceRendererDispatch.tsx[1198-1200]
- packages/k8s-ui/src/components/shared/ResourceRendererDispatch.test.tsx[888-908]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // A foreign group ending in `cluster.x-k8s.io` (e.g. `extension.cluster.x-k8s.io`) | ||
| // used to pass a substring guard and inherit CAPI rendering, status, or the |
There was a problem hiding this comment.
2. Comment records previous behavior 📘 Rule violation ⚙ Maintainability
The added test comment says the foreign group used to pass the old substring guard, explicitly recording change history in source code. The rationale can be retained without describing the previous implementation.
Agent Prompt
## Issue description
The added comment records previous implementation behavior with the phrase `used to pass`.
## Issue Context
Keep the collision invariant and reason for exact-group matching, but phrase it in present-tense terms without referring to code history.
## Fix Focus Areas
- packages/k8s-ui/src/components/shared/ResourceRendererDispatch.test.tsx[849-853]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| apiVersion: FOREIGN_GROUP, | ||
| status: { phase: 'Provisioned' }, | ||
| }) | ||
| // The generic getter surfaces the raw phase text. |
There was a problem hiding this comment.
3. Comment restates status assertion 📘 Rule violation ⚙ Maintainability
The comment immediately before expect(s?.text).toBe('Provisioned') only restates that the generic
getter returns the raw phase. It adds no rationale beyond the preceding explanation and assertion.
Agent Prompt
## Issue description
The comment directly narrates the immediately following status assertion without adding intent, constraints, or non-obvious context.
## Issue Context
The preceding comment already explains the distinction between generic and CAPI status handling, so this additional comment can be removed.
## Fix Focus Areas
- packages/k8s-ui/src/components/shared/ResourceRendererDispatch.test.tsx[865-872]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1db0a91. Configure here.
| {kind === 'poolers' && isApiGroup(data?.apiVersion, CNPG_GROUP) && <CNPGPoolerRenderer data={data} onNavigate={onNavigate} />} | ||
| {/* Cluster API (CAPI) */} | ||
| {'topology.cluster.x-k8s.io/owned' in (data?.metadata?.labels ?? {}) && data?.apiVersion?.includes('cluster.x-k8s.io') && ( | ||
| {'topology.cluster.x-k8s.io/owned' in (data?.metadata?.labels ?? {}) && isApiGroup(data?.apiVersion, 'cluster.x-k8s.io') && ( |
There was a problem hiding this comment.
Topology banner misses CAPI groups
Medium Severity
The ClusterClass warning now requires an exact cluster.x-k8s.io match, so topology-owned resources in official sibling groups such as controlplane.cluster.x-k8s.io and infrastructure.cluster.x-k8s.io no longer show it. Those groups previously matched the substring check and still receive topology.cluster.x-k8s.io/owned.
Reviewed by Cursor Bugbot for commit 1db0a91. Configure here.
|
Thanks for the fix. We are closing this one. Issue #1582 drew two independent fixes. #1587 was opened a day earlier and covers the same three dispatch points, so we took that one. Both patches change the same lines, so we could not take both. Sorry for the wasted effort. Two notes on your patch, for next time.
You found a real bug here. We have other open issues if you want to pick one up. |
2 participants
Description
Three CAPI renderer guards in
packages/k8s-ui/src/components/shared/ResourceRendererDispatch.tsxmatched the API group withapiVersion?.includes(\"cluster.x-k8s.io\"). That substring test also matches a foreign group whose name ends in the same suffix (e.g.extension.cluster.x-k8s.io), so a generic CRD would render through a CAPI renderer, get a fabricated CAPI status, or receive the topology-controlled ClusterClass warning banner.Replaced the four remaining substring checks with the existing
isApiGroup(..., 'cluster.x-k8s.io')helper, following the exact-group pattern already used for CAPIclusterson line 861 and the CNPG/Velero collision handling.machinesandmachinesetsare inKNOWN_KINDS, which suppresses the generic renderer. With the strict guard, a foreign CRD sharing those plurals would match neither the CAPI render line nor the generic fall-through and would render a blank drawer — the exact trap the Crossplane block documents. Added acapiCollisionFallthroughthat routes such kinds toGenericRenderer, mirroring the existinggroupGatedFallthroughforclusters,backups, etc.Read
docs/INTEGRATION_GUIDE.mdbefore reviewing renderer-dispatch changes.Type of change
How has this been tested?
Regression coverage in
ResourceRendererDispatch.test.tsxuses a foreignextension.cluster.x-k8s.io/v1CRD and asserts, at all three boundaries the issue calls out, that a substring guard would have failed:clusters,machines,machinesets— foreign CRD renders throughGenericRenderer, no CAPI-specific textgetResourceStatusforclustersandmachines— foreign CRD gets the generic phase-echo, not the CAPI badgetopology.cluster.x-k8s.io/ownedlabel on a foreign group does not trigger the ClusterClass warningcluster.x-k8s.io/v1beta1resource still shows the warningTest results:
Checklist
Related issues
Fixes #1582
Note
Low Risk
UI-only dispatch and status logic for resource detail views; tightens matching so fewer foreign CRDs are mislabeled, with no auth or data-path changes.
Overview
Fixes mis-routing of CRDs whose API group merely contains
cluster.x-k8s.io(e.g.extension.cluster.x-k8s.io) through Cluster API UI paths.ResourceRendererDispatch now gates the topology-controlled ClusterClass banner,
machines/machinesetsCAPI renderers, and CAPI machine status onisApiGroup(..., 'cluster.x-k8s.io')instead of substringincludes. Foreignmachines/machinesetsthat share those plurals but are not CAPI are sent toGenericRenderervia newcapiCollisionFallthrough, avoiding blank drawers after the stricter guards.Regression tests cover renderer dispatch, status badges, and the topology banner for
extension.cluster.x-k8s.io, plus a positive check for real CAPI resources.Reviewed by Cursor Bugbot for commit 1db0a91. Bugbot is set up for automated code reviews on this repo. Configure here.