fix(kubernetes): probe Keycloak CRDs by output in oidc-bootstrap cleanup - #3520

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

Merge Risk: ⚪ Minimal · up to 06400

The cleanup change is ready to merge after normal checks. No actionable merge-blocking risk remains.

Architecture Summary

Architecture risk: 🔵 Low · up to 06400

The change affects 1 system.

Changed systems: packages/apps

Architecture concerns
No architecture-level concerns identified.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Note for reviewers: overlapping file with a sibling PR

packages/apps/kubernetes/templates/oidc-rbac-job.yaml is also touched by #3521, which raises that Job's
bootstrap container memory limit from 256Mi to 512Mi. This PR only rewrites the Keycloak cleanup guard's shell
logic, so the two changes are independent, but a textual conflict is likely for whichever lands second. Happy to
rebase.

The observation is accurate: line 231 uses 2>/dev/null || true, so a discovery failure and an absent API group both arrive as an empty EDP_RESOURCES. That conflation is deliberate, and I would rather not remove it, because the suggested change reintroduces the class of bug this PR fixes.

This script is a post-install,post-upgrade Helm hook with backoffLimit: 6. Anything that makes it exit non-zero blocks the Helm upgrade of the whole kubernetes-<cluster> release. That is precisely the reported failure: the previous exit-code guard let the deletes run on clusters without the Keycloak operator, they failed on an unknown resource type, and every upgrade at the chart default oidc.mode: None was stuck. Making cleanup "retry or fail on discovery errors" would trade a permanent failure on absent CRDs for a failure on a transient discovery hiccup, which is the same outage with a rarer trigger.

The failure mode of keeping it as-is is also much milder, and self-healing: if discovery genuinely fails, the cleanup skips, the hook succeeds, and the deletes are attempted again on the next upgrade of that release. The objects it would have removed are chart-owned, so nothing else claims them in the meantime. A stuck upgrade, by contrast, needs a human.

Your point about the two .bats files stands on its own, though, and I would separate it: there a conflated status means the test skips where it should fail loudly, and a silent skip in CI is a real loss of signal, with none of the availability trade-off that applies to the hook. I have left it out of this PR to keep the fix reviewable as one change, and I am happy to follow up on the e2e side if a maintainer prefers it in scope here.

Correction to the paragraph above, and thanks for the prompt to look again. I wrote that the .bats change was left out of this PR. That was wrong: both monitoring-oidc-system.bats and monitoring-oidc-customconfig.bats are in the diff already, converted from the exit-code guard to the output probe. What was missing is the narrower thing you actually pointed at, distinguishing a failed discovery call from an empty list, and that is now fixed in 5810ac0.

The guards now skip only on a genuinely empty list and fail with the discovery error when the call itself fails. Validated against a stub kubectl reproducing the three states: CRDs present continues, an absent group (exit 0 with empty output) skips, and a failing call exits non-zero with the reason. I could not run the suites themselves, which need a live cluster.

My position on the Helm hook is unchanged, and the contrast is the point: there any non-zero exit blocks the release upgrade, so conflating the two states is a deliberate availability trade-off. A test has no such trade-off to make, so your finding was right for the .bats files and wrong for the hook.

mattia-eleuteri Where this stands: the template fix and its test are right and verified, and the only thing holding this is the second half of the diff, the two files under hack/e2e-apps that nothing runs, plus the CI wording in the body and the title of 5810ac0. The two red checks are the fork path missing #3257, a rebase clears them. Can you drop the two files and reword those two texts? If you would rather not touch it further, say so and I will open a separate PR for the template fix with credit to you.

Done, and I would rather finish it than hand it over.

The branch is now two commits on top of main:

• fix(kubernetes): probe Keycloak CRDs by output in oidc-bootstrap cleanup — the template and its unit test, unchanged.
• chore(tests): remove the two dead monitoring OIDC bats suites — git rm on the pair, in its own commit, as you asked.

5810ac094 and 5c57db5fa touched only those two files, so once the files go both commits are empty and the rebase drops them. There was nothing left to reword, which is a better outcome than a reworded title: the permanent record no longer contains the claim at all.

I checked your three findings rather than taking them on trust, and all three hold. BATS_UNIT_FILES := $(filter-out hack/e2e-%.bats,$(wildcard hack/*.bats)) at Makefile:161 is non-recursive and filters e2e names, hack/select-e2e.sh matches hack/[^/]+\.bats$, and packages/core/testing/Makefile names three files one by one. hack/cozytest.sh has no skip, only the unrelated skip_next at line 31 — so the branch my change made reachable was the one that would have exited 127, which is the inversion you described. The CI claims are out of the body, and it now says plainly that the probe fix in those two files was unobservable.

Two things I did beyond the ask, both to keep the deletion honest:

Four comments asserted what the directory currently holds and the deletion falsifies them — hack/cozytest.sh:164, hack/select-e2e.sh:358, hack/cozytest-capture-gate.bats:178, hack/bats-no-exit-trap.bats:574 ("hack/e2e-apps/ already holds two"). They now speak of a suite placed there instead. The machinery is untouched on purpose, for the reason your own comment in select-e2e.sh gives: marking it inert would bake the orphan status into the rule. bats-no-exit-trap.bats 22/22, cozytest-capture-gate.bats 5/5 and select-e2e_test.bats 50/50 still pass through hack/cozytest.sh.

And #3786 says the choice needs someone who knows whether the coverage is still wanted. It is, so I opened #4051 with what the pair actually asserted in each mode, plus the two traps a port should not repeat — probe the output, and keep a failed discovery distinct from an empty list, which a test has no reason to conflate. Closes #3638 and Closes #3786 are on the deletion commit; the port stays a feature request, which is the disposition you proposed in #3638.

My position on the hook itself is unchanged and now stands alone: there, conflating a failed discovery with an absent group is deliberate, because any non-zero exit blocks the release upgrade. That trade-off was never a reason to conflate them in a test — it just no longer has a test to argue with.

Rebased for #3257, so the OCIR checks should clear. make bats-unit-tests does not complete on my machine — hack/migration-54-redis-adopt.bats wants a Docker daemon — so that lane is on CI rather than on my word.

Aleksei Sviridkin (@lexfrei) Rebased. 06817c1e3 is dropped and the branch is now the single fix commit on current main (06400ac03), with no conflicts.

The trailer is rewritten to Assisted-by: LLM, and hack/check-commit-trailers.sh upstream/main..HEAD passes. helm unittest packages/apps/kubernetes passes 317/317, and putting main's template back fails only the new case.

The PR body no longer mentions the suite deletion, the four reworded comments or Closes #3638. I also removed the stale e2e line from the CodeRabbit summary, since the body becomes the merge commit message.

Successfully created backport PR for release-1.6:

• [Backport release-1.6] fix(kubernetes): probe Keycloak CRDs by output in oidc-bootstrap cleanup #4571