cert-manager, keda, volcano: Only report not installed on a 404 (#972) by prabindersinghh · Pull Request #1206 · headlamp-k8s/plugins

prabindersinghh · 2026-08-18T00:43:55Z

These three plugins decide whether their operator is installed by asking for the
API group and treating any failure as absence:

try {
  await ApiProxy.request('/apis/cert-manager.io/v1', { method: 'GET' });
  return true;
} catch (error) {
  return false;
}

A transient failure therefore renders the "not installed" banner on a cluster
that does have the operator. The SDK synthesises status: 502 when fetch
throws and 408 on timeout (lib/k8s/api/v1/clusterRequests.js), and both land
in that catch. The user gets told to install something that is already there,
and the underlying error is never shown.

The probes now return one of three results instead of a boolean:

export type InstallProbeResult = 'installed' | 'absent' | 'unreachable';

Only a 404 counts as absent. Per the measurements @Ralthos posted on #972, API
group discovery is not RBAC gated: Kubernetes binds the system:discovery
ClusterRole to system:authenticated, so any authenticated user gets a real
answer from /apis/<group>/<version>, and a missing group still returns
NotFound. That makes 404 a dependable signal for these three, and it means the
RBAC wrinkle that complicates the CRD probe plugins does not arise here.

When the probe cannot complete, the views render as normal and the actual list
or detail request reports the real error. That seemed better than guessing,
since the request that follows will fail with a more useful message anyway.

Volcano

Volcano probes more than one group, and the old code wrapped a single
try/catch around Promise.all, so the first rejection threw away the results of
every other probe. Each group now catches its own failure and the results get
combined afterwards. If any group is unreachable the whole check is unreachable,
since not being able to reach a group is not evidence that the feature is
missing.

Cluster switching

The hooks ran their probe once on mount with an empty dependency array, so the
answer from the first cluster stuck around after switching. They now re-probe
when the cluster changes and ignore a result that arrives after unmount.

Scope

This is the discovery probe half of what came up in #972. #984 covers flux,
which probes CRDs and has the RBAC case to deal with. Kubeflow uses the same
discovery pattern in kubeflow/src/checks/isKubeflowInstalled.ts and still has
the old behaviour. I left it out to keep this reviewable, but happy to send a
follow-up or fold it in here if you would rather have it in one go.

Worth flagging that #1075 adds a knative test asserting the probe returns false
on failure, which codifies the behaviour this PR moves away from.

Testing

New unit tests for each probe cover 404 to absent, and 403, 502, 408 and a
rejection with no status to unreachable. The volcano tests also cover the
multi group combinations, including one group absent while another is
unreachable.

Ran per plugin against the local toolchain:

plugin	tsc	test	lint	build
cert-manager	pass	6 passed, 1 skipped	pass	pass
keda	pass	6 passed, 1 skipped	pass	pass
volcano	pass	26 passed, 1 skipped	pass	pass

The skipped one in each is the existing storyshots test.

I could not exercise the unreachable path against a live cluster, so that side is
covered by unit tests only.

The install check treated any failed discovery request as proof that cert-manager is missing. A 403, a dropped connection or a timeout all rendered the "not installed" banner, which sends people off to install something that is already on the cluster. The probe now reports installed, absent or unreachable, and only a 404 counts as absent. Discovery is not RBAC gated, so an authenticated user always gets a definitive answer when the request actually completes. When it does not complete we render the normal views and let the real request report the real error. The hook also re-runs when the cluster changes and drops results that arrive after unmount, so switching clusters no longer keeps the answer from the previous one. Signed-off-by: Prabinder Singh <prabindersinghh@gmail.com>

The install check treated any failed discovery request as proof that KEDA is missing, so a 403, a dropped connection or a timeout all rendered the "not installed" banner on a cluster that has KEDA. The probe now reports installed, absent or unreachable, and only a 404 counts as absent. When the probe cannot complete, KedaInstallCheck renders its children and lets the real request report the real error. The hook also re-runs when the cluster changes and drops results that arrive after unmount. Signed-off-by: Prabinder Singh <prabindersinghh@gmail.com>

The install checks treated any failed discovery request as proof that Volcano is missing, so a 403, a dropped connection or a timeout all rendered the "not installed" banner on a cluster that has Volcano. Each API group is now probed on its own and reports installed, absent or unreachable, with only a 404 counting as absent. The group probes also catch their own failures instead of sharing one catch around Promise.all, so one rejection no longer throws away what the other probes already established. When the groups disagree, an unreachable result wins over an absent one: if we could not reach a group we do not know the feature is missing, and saying so is the misreport this check is meant to avoid. The hooks also re-run when the cluster changes and drop results that arrive after unmount. Signed-off-by: Prabinder Singh <prabindersinghh@gmail.com>

Ralthos

This is the right fix, and the three-value result is the shape it needed. unreachable renders
the content and lets the real request report the failure, so nobody gets told to install what
they already have.

The tests are doing real work too. Covering 403, 502, 408 and a rejection carrying no status
at all pins the exact behaviour the old boolean threw away, and mocking ApiProxy at the module
boundary means they exercise the shipped function, not a copy of it.

Two things.

The precedence in probeApiGroups discards a result the code has already proven.

if (results.includes('unreachable')) {
  return 'unreachable';
}
return results.includes('absent') ? 'absent' : 'installed';

I can see from prefers unreachable over absent when a probe was inconclusive that this is
deliberate, so I am asking about the semantics here.

The groups passed to probeVolcanoCoreInstalled are a conjunction: the views need scheduling
and job. If scheduling returns 404 and job returns 502, the feature is definitively not
installed. One required group is provably missing, and no outcome for the other group can change
that. The 404 settles the question on its own, and the 502 currently overrides it.

That inverts the bug this PR fixes. On a cluster genuinely without Volcano, one flaky probe
turns a correct "not installed" banner into the resource views, which then fail on their own.
Less harmful than the false banner, and still a case where the code holds the answer and does
not use it.

I think absent should win when any required group returns 404, leaving unreachable for when
nothing definitive came back at all:

if (results.includes('absent')) {
  return 'absent';
}
return results.includes('unreachable') ? 'unreachable' : 'installed';

That keeps the property you are protecting, since a probe which never completed still cannot
produce absent by itself. If the current order is deliberate for a reason I have missed, it is
worth a line in the doc comment: the comment argues the general principle without noting that a
conjunctive requirement inverts it.

isInstalled has no caller, and it is a trap.

All three hooks return isInstalled, notInstalled and isLoading, and every call site gates
on !notInstalled && !isLoading or notInstalled || isLoading. Nothing reads isInstalled.

That is fine today, and it is how unreachable ends up rendering the content. The problem is
the next person to touch this. if (isInstalled) looks like the obvious way to use the hook,
and it silently restores the old behaviour: unreachable stops rendering the content and the
banner comes back. Either drop it, or say in a comment that gating on it reintroduces the bug
this PR removes.

One follow-up, and not something I am asking you to change here. The probe, the type and the doc
comment are now duplicated across cert-manager, keda and volcano. That is three copies of a
decision it took a measurement to reach, and the failure mode is one of them drifting later.
#1083 was about exactly this. Happy to do that consolidation once this lands, since the
behaviour is what matters first.

On the RBAC line in the doc comments: that matches what I measured on #972. A ServiceAccount
denied every resource in the group still got 200 from discovery, and the CRD read returned 403.
One detail worth having is that the bound subject is system:authenticated, so an anonymous
request is not covered and can see a 403 on discovery. That lands in unreachable, which is the
correct answer for it.

Ralthos · 2026-08-18T06:27:44Z

One more thing, about the way these tests are written.

All three test files reach for the same escape hatch:

const { request } = vi.hoisted(() => ({ request: vi.fn() }));
vi.mock('@kinvolk/headlamp-plugin/lib', () => ({ ApiProxy: { request } }));

That works, and for a probe whose only dependency is ApiProxy.request it is arguably the right
call. But it is the second distinct workaround this repo has grown for the same underlying
problem, and the two are worth comparing while they are both in one thread.

On #1079 I hit the other side of it. findAuthenticationEdges lived in mapView.tsx, which
imports the detail components, which import the resource classes, which import
@kinvolk/headlamp-plugin/lib/k8s/cluster. The module could not be loaded under test at all, so
I moved the logic into its own file instead of mocking the SDK. Mocking would have meant
stubbing a surface I did not use.

Both exist because of kubernetes-sigs/headlamp#7036: plugin modules that import SDK
values cannot be unit tested because the path does not resolve on disk. 86 files are affected,
and it looks like the reason most plugins ship with no tests at all.

The two workarounds have different costs. Mocking the module is cheap and local, and it goes
stale silently when the SDK changes shape, since the test keeps passing against a fake that no
longer resembles the real thing. Extracting is more invasive and survives that, but it only
works when the logic is separable, which a probe calling one SDK function largely is not.

Nothing here is a change request for this PR. I raise it because a reader of this diff will
copy the mock into the next plugin, and it would be better if that were a decision the repo made
once. If there is appetite, the shared install-probe helper I mentioned above is a natural place
to settle it: one module, one mock, tested in one location, and the nine plugins in #1083 stop
each inventing their own answer.

The groups a feature needs are a conjunction, so a single 404 answers the question on its own: one required group is provably missing and no result for the other groups can change that. Checking unreachable first let a flaky probe override a 404 that had already settled it, which on a cluster genuinely without Volcano turned a correct banner into resource views that then fail by themselves. Absent is now checked first. A probe that never completed still cannot produce absent on its own, so the property this is protecting is intact. Also notes in the probe doc that the system:discovery binding covers system:authenticated, so an anonymous request can still see a 403 there. That lands in unreachable, which is the right answer for it. Signed-off-by: Prabinder Singh <prabindersinghh@gmail.com>

The system:discovery ClusterRole is bound to system:authenticated, so the "discovery is not RBAC gated" claim holds for authenticated users only. An anonymous request can be refused with a 403, which lands in unreachable. Signed-off-by: Prabinder Singh <prabindersinghh@gmail.com>

Nothing read it. Every call site gates on notInstalled and isLoading, which is what lets an unreachable probe fall through to the content. Leaving it there was a trap: gating on isInstalled reads like the obvious way to use the hook and quietly restores the old behaviour, sending the unreachable case back to the banner. The doc comment now says why there is no positive flag. Signed-off-by: Prabinder Singh <prabindersinghh@gmail.com>

prabindersinghh · 2026-08-18T13:11:45Z

Thanks, the precedence point is right and I had it backwards.

I was applying the general principle, that an inconclusive probe is not evidence of
absence, without noticing that a conjunction inverts it. If scheduling returns 404
then Volcano's core cannot be there, and nothing the job probe returns changes that.
Flipped in c2ea975:

if (results.includes('absent')) {
  return 'absent';
}
return results.includes('unreachable') ? 'unreachable' : 'installed';

The test that pinned the old order now asserts absent, and I added one for the
property you were checking I had not lost: two inconclusive probes with no 404
anywhere still give unreachable, so a probe that never completed cannot produce
absent by itself. The doc comment says why a single 404 is conclusive here, since
the old one argued the general rule and skipped the exception.

On isInstalled, I dropped it. Nothing read it, and I would rather it not exist than
ship a comment asking people not to use it. The hooks return notInstalled and
isLoading, and the doc now says why there is no positive flag.

I also took the system:authenticated detail into the probe docs. An anonymous
request is outside that binding and can get a 403 from discovery, and unreachable
is the right place for that to land.

On the shared helper and the mocking question, agreed on both, and both look like
they want to be one decision rather than three. Happy to do the consolidation as a
follow-up once this lands, and if the repo wants to settle the test approach in the
same place that seems like the right home for it. I have no strong preference on
which way it goes, only that the nine plugins in #1083 should not each answer it
differently.

Ralthos · 2026-08-19T07:40:59Z

Checked the new commit. The conjunction case returns absent now, never reports absent from inconclusive probes alone pins the property in the other direction so the flip cannot quietly
go too far, and the doc comment says why one 404 settles it. That all reads right.

Dropping isInstalled is better than what I suggested. I proposed a comment warning people off
it; removing it means there is nothing to warn about.

On whether the testing approach should be settled in the same place: I would keep the two apart,
and I think the consolidation makes that easier rather than harder.

Consolidating the probe answers the mocking question for install detection largely by dissolving
it. One module, one dependency on ApiProxy.request, mocked in one file, tested in one file, and
the plugins import a function instead of each standing up their own fake. That is worth doing for
its own sake.

What it does not settle is the general case, which is kubernetes-sigs/headlamp#7036: any plugin
module importing SDK values cannot be loaded under test, and install detection is one instance.
A consolidation PR is the wrong room for repo-wide test policy, because the people who would
have to live with the decision are not reading that diff. Better to let the consolidation show
the pattern working and argue the general question on #7036, where it stands on its own.

I will take the consolidation once this lands. I will keep it to the behaviour you have arrived
at here, so it is a move and not a redesign, and open it against the plugins in #1083 that this
PR does not touch. Converting cert-manager, keda and volcano can come last, so nothing here has
to move.

illume

Thanks for working on this.

The commit messages could use some tidying up to match our contribution guidelines. We use Linux kernel style — the contributing guide has the details, and git log shows good examples.

Commits that need attention

cert-manager, keda: Note the anonymous case in the probe docs — Missing area: description prefix — e.g. frontend: HomeButton: Fix so it navigates to home or backend: config: Add enable-dynamic-clusters flag.
cert-manager, keda, volcano: Drop the unused isInstalled flag — Missing area: description prefix — e.g. frontend: HomeButton: Fix so it navigates to home or backend: config: Add enable-dynamic-clusters flag.

Commit guidelines

Use atomic commits focused on a single change.
Use the title format <area>: <Description of changes> — description must start with a capital letter.
Keep the title under 72 characters (soft requirement).
Explain the intention and why the change is needed.
Make commit titles meaningful and describe what changed.
Do not add code that a later commit rewrites; squash or reorder commits instead.
Do not include Fixes #NN in commit messages.

Good examples:

frontend: HomeButton: Fix so it navigates to home
backend: config: Add enable-dynamic-clusters flag

Copilot

Copilot review overview

🟡 Changes recommended

Cluster changes are not reactively observed, and Volcano’s mixed-result precedence conflicts with the documented contract.

Review effort: Balanced
Findings: 4

Open (4)

Use reactive useCluster hook to detect route changes · New
Use reactive useCluster hook to detect route changes · New
Use reactive useCluster hook to detect route changes · New
Make unreachable take precedence over absent for mixed results · New

What changed in this PR

Updates cert-manager, KEDA, and Volcano installation detection so only 404 responses indicate absence.

Changes:

Introduces tri-state installation probes.
Re-probes after cluster changes.
Adds probe unit tests and updates installation guards.

File	Description
`volcano/src/volcanoInstallChecks.ts`	Adds tri-state, multi-group probing.
`volcano/src/volcanoInstallChecks.test.ts`	Tests probe outcomes and combinations.
`volcano/src/hooks/useVolcanoInstallChecks.tsx`	Updates Volcano probe hooks.
`volcano/src/components/common/CommonComponents.tsx`	Uses new hook results.
`keda/src/isKedaInstalled.ts`	Adds tri-state KEDA probe.
`keda/src/isKedaInstalled.test.ts`	Tests KEDA probe outcomes.
`keda/src/hooks/useKedaInstalled.tsx`	Updates KEDA installation hook.
`keda/src/components/common/CommonComponents.tsx`	Updates KEDA installation guard.
`cert-manager/src/isCertManagerInstalled.ts`	Adds tri-state cert-manager probe.
`cert-manager/src/isCertManagerInstalled.test.ts`	Tests cert-manager probe outcomes.
`cert-manager/src/hooks/useCertManagerInstalled.ts`	Updates cert-manager installation hook.
`cert-manager/src/components/orders/List.tsx`	Uses new installation state.
`cert-manager/src/components/orders/Detail.tsx`	Uses new installation state.
`cert-manager/src/components/issuers/List.tsx`	Uses new installation state.
`cert-manager/src/components/issuers/Detail.tsx`	Uses new installation state.
`cert-manager/src/components/clusterIssuers/List.tsx`	Uses new installation state.
`cert-manager/src/components/clusterIssuers/Detail.tsx`	Uses new installation state.
`cert-manager/src/components/challenges/List.tsx`	Uses new installation state.
`cert-manager/src/components/challenges/Detail.tsx`	Uses new installation state.
`cert-manager/src/components/certificates/List.tsx`	Uses new installation state.
`cert-manager/src/components/certificates/Detail.tsx`	Uses new installation state.
`cert-manager/src/components/certificateRequests/List.tsx`	Uses new installation state.
`cert-manager/src/components/certificateRequests/Detail.tsx`	Uses new installation state.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ */
 export function useCertManagerInstalled() {
-  const [isManagerInstalled, setIsManagerInstalled] = useState<boolean | null>(null);
+  const cluster = Utils.getCluster() ?? '';


+ */
 export function useKedaInstalled() {
-  const [isKedaInstalled, setIsKedaInstalled] = useState<boolean | null>(null);
+  const cluster = Utils.getCluster() ?? '';


+import { Utils } from '@kinvolk/headlamp-plugin/lib';
 import { useEffect, useState } from 'react';
 import {


+  if (results.includes('absent')) {
+    return 'absent';
+  }
+
+  return results.includes('unreachable') ? 'unreachable' : 'installed';


prabindersinghh added 3 commits August 18, 2026 06:10

prabindersinghh requested review from ashu8912, illume, joaquimrocha, skoeva, sniok, vyncent-t and yolossn as code owners August 18, 2026 00:43

Ralthos reviewed Aug 18, 2026

View reviewed changes

prabindersinghh added 3 commits August 18, 2026 18:40

This was referenced Aug 19, 2026

fix(kueue): resolve namespace-scoped RBAC blocks for Workloads and L… #1200

Open

kmesh: waypoints/nodeinfo: Add related-resource links to detail pages #1218

Open

illume requested a balanced review from Copilot September 29, 2026 06:40

illume requested changes Sep 29, 2026

View reviewed changes

Copilot started reviewing on behalf of illume September 29, 2026 06:40 View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

Navigation Menu

Repository navigation

cert-manager, keda, volcano: Only report not installed on a 404 (#972) - #1206

cert-manager, keda, volcano: Only report not installed on a 404 (#972)#1206
prabindersinghh wants to merge 6 commits into
headlamp-k8s:mainfrom
prabindersinghh:install-detection/probe-errors-972

prabindersinghh commented Aug 18, 2026

Uh oh!

Ralthos left a comment

Uh oh!

Ralthos commented Aug 18, 2026

Uh oh!

prabindersinghh commented Aug 18, 2026

Uh oh!

Ralthos commented Aug 19, 2026

Uh oh!

illume left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

prabindersinghh commented Aug 18, 2026

Volcano

Cluster switching

Scope

Testing

Uh oh!

Ralthos left a comment

Choose a reason for hiding this comment

Uh oh!

Ralthos commented Aug 18, 2026

Uh oh!

prabindersinghh commented Aug 18, 2026

Uh oh!

Ralthos commented Aug 19, 2026

Uh oh!

illume left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants