fix(kubernetes): probe Keycloak CRDs by output in oidc-bootstrap cleanup - #3520
fix(kubernetes): probe Keycloak CRDs by output in oidc-bootstrap cleanup#3520mattia-eleuteri wants to merge 3 commits into
Conversation
`kubectl api-resources --api-group=<absent group>` exits 0 and prints an empty list, so guarding on the exit code always fell through. The delete then failed with `the server doesn't have a resource type "keycloakclient"` — `--ignore-not-found` covers a missing object, not an unknown type. Because the oidc-bootstrap Job is a post-install/post-upgrade Helm hook, that failure blocked the Helm upgrade of every `kubernetes-<cluster>` release on any cluster without the EDP Keycloak operator CRDs, which is the default configuration (`oidc.mode: None`). Capture the discovery output once and gate each delete on its own resource type, matching the probe idiom already used by the platform migrations and the ouroboros cleanup hook. Fixes cozystack#3516 Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
…ng OIDC bats Both suites skip when the EDP Keycloak CRDs are absent, but the probe tested the exit code of `kubectl api-resources`, which is 0 for an absent API group. The skip therefore never fired: on a runner without the Keycloak operator, monitoring-oidc-system spent its 60s timeout waiting for a KeycloakClient that can never exist, and monitoring-oidc-customconfig passed for the wrong reason (its negative assertions succeed on an unknown resource type). Test the discovery output instead, like the ValidatingAdmissionPolicy preflight in e2e-install-cozystack.bats already does. Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe OIDC cleanup job and monitoring tests now inspect OIDC resource discovery
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/kubernetes/templates/oidc-rbac-job.yaml`:
- Line 231: Preserve the kubectl api-resources exit status separately from
EDP_RESOURCES in the discovery logic of
packages/apps/kubernetes/templates/oidc-rbac-job.yaml, so cleanup retries or
fails on discovery errors and monitoring skips only when the successfully
discovered CRD list is empty. Update the corresponding assertions or setup in
hack/e2e-apps/monitoring-oidc-customconfig.bats lines 110-114 and
hack/e2e-apps/monitoring-oidc-system.bats lines 131-135 to cover the distinct
discovery-failure and empty-list cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c429de9-355b-485e-84fb-94fd4ec5b509
📒 Files selected for processing (4)
hack/e2e-apps/monitoring-oidc-customconfig.batshack/e2e-apps/monitoring-oidc-system.batspackages/apps/kubernetes/templates/oidc-rbac-job.yamlpackages/apps/kubernetes/tests/oidc_test.yaml
| # api-resources hits the discovery endpoint, which | ||
| # system:discovery grants to every authenticated principal, so | ||
| # the probe needs no cluster-scoped CRD read permission. | ||
| EDP_RESOURCES=$(kubectl api-resources --api-group=v1.edp.epam.com --output=name 2>/dev/null || true) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 2 'api-resources|EDP_RESOURCES|grep -q' \
packages/apps/kubernetes/templates/oidc-rbac-job.yaml \
hack/e2e-apps/monitoring-oidc-customconfig.bats \
hack/e2e-apps/monitoring-oidc-system.bats
failing_probe() { return 7; }
if ! failing_probe 2>/dev/null | grep -q '^keycloakclients\.'; then
echo "A failed probe enters the skip path."
fiRepository: cozystack/cozystack
Length of output: 6940
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== oidc-rbac-job relevant lines =="
sed -n '200,250p' packages/apps/kubernetes/templates/oidc-rbac-job.yaml
echo "== monitoring bats relevant lines =="
sed -n '126,138p' hack/e2e-apps/monitoring-oidc-system.bats
sed -n '106,118p' hack/e2e-apps/monitoring-oidc-customconfig.bats
echo "== behavioral probe mirrors current bash semantics for failed discovery piped to grep =="
failing_probe() { echo "not keycloak"; return 7; }
if ! failing_probe 2>/dev/null | grep -q '^keycloakclients\.'; then
echo "failed discovery currently treated as CRD absent"
fi
printf -- '\n== grep on failed discovery output vs empty output status and result ==\n'
tmp=$(mktemp)
printf 'empty' > "$tmp"
if kubectl api-resources --api-group=v1.edp.epam.com --output=name 2>/dev/null | grep -q '^keycloakclients\.'; then
echo "empty discovery output with kubectl presence: grep matched"
elif [ "${PIPESTATUS[0]:-?}" != "0" ]; then
echo "empty discovery output with kubectl presence: kubectl non-zero, no grep match"
else
echo "empty discovery output with kubectl presence: grep no match because no output matches"
fi
rm -f "$tmp"Repository: cozystack/cozystack
Length of output: 5273
Preserve discovery status before treating CRDs as absent.
A failed kubectl api-resources flows into the empty-output path here. Keep the discovery status separate from the probe output so cleanup can retry/fail on discovery errors, and monitoring skips only when the available CRD list is actually empty.
📍 Affects 3 files
packages/apps/kubernetes/templates/oidc-rbac-job.yaml#L231-L231(this comment)hack/e2e-apps/monitoring-oidc-customconfig.bats#L110-L114hack/e2e-apps/monitoring-oidc-system.bats#L131-L135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/kubernetes/templates/oidc-rbac-job.yaml` at line 231, Preserve
the kubectl api-resources exit status separately from EDP_RESOURCES in the
discovery logic of packages/apps/kubernetes/templates/oidc-rbac-job.yaml, so
cleanup retries or fails on discovery errors and monitoring skips only when the
successfully discovered CRD list is empty. Update the corresponding assertions
or setup in hack/e2e-apps/monitoring-oidc-customconfig.bats lines 110-114 and
hack/e2e-apps/monitoring-oidc-system.bats lines 131-135 to cover the distinct
discovery-failure and empty-list cases.
Note for reviewers: overlapping file with a sibling PR
|
|
The observation is accurate: line 231 uses This script is a 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 Correction to the paragraph above, and thanks for the prompt to look again. I wrote that the 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 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 |
Review follow-up on the skip guards these tests already carried. Testing the output rather than the exit code fixed the guard that never fired, but it also made a FAILED discovery call indistinguishable from an absent API group: both arrive as an empty list, and both skipped. A silent skip on a broken API server is a loss of signal, not a graceful degradation. The suite would report success for a cluster it never checked, which is the failure mode e2e tests exist to prevent. Keep the two apart: skip only on a genuinely empty list, and fail with the discovery error when the call itself fails. Unlike the Helm hook in this same PR, where conflating them is deliberate because any non-zero exit blocks the release upgrade, a test has no availability trade-off to make. Validated against a stub kubectl reproducing the three states: CRDs present continues, absent group (exit 0 with empty output) skips, and a failing call exits non-zero with the reason. bats was not available to run the suites themselves, and they need a live cluster besides. Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM, though the template fix itself is right and I want it in: people are hitting this in production today.
I verified the fix end to end rather than reading it. kubectl api-resources --api-group=<absent> --output=name exits 0 and prints nothing, while a present group prints <resource>.<group>, so the old exit-code guard could never skip and the ^keycloakclients\. anchor is the correct test. The mode=None path has no other unknown-type call. The new unit case fails all four assertions when I restore only the pre-fix oidc-rbac-job.yaml, so the test bites. release-1.6 still carries the broken guard, so the backport target is valid.
What blocks it sits in the second half of the diff and in the text around it.
Nothing executes the two files under hack/e2e-apps/. packages/core/testing/Makefile runs three named bats files plus chainsaw, the root Makefile builds its unit list from $(wildcard hack/*.bats) which is not recursive, and a grep for e2e-apps across .github/, hack/*.sh and both Makefiles returns one comment in cozytest.sh. That matters because the PR body describes their behaviour in CI ("burned its 60s timeout", "passed for the wrong reason") and commit 5810ac094 is titled "fail the OIDC e2e when CRD discovery itself fails". Neither can happen. With the backport label those statements travel to release-1.6, and a commit title is permanent. How they got there: the Chainsaw migration removed the runner on 2026-06-25 and these two files arrived on 2026-07-15, into a directory nothing had read for three weeks.
Second, the change makes an undefined command reachable in those same files. hack/cozytest.sh defines no skip, only an unrelated skip_next variable, and runs each body in a subshell under set -eu -x, so calling skip is exit 127 and fails the test. Before this diff the guard never fell true and the line was unreachable text; now it is reachable, which inverts what it was written to do. The same pattern sits at hack/e2e-install-cozystack.bats:593 in a file CI does run, so I filed that separately as #3827.
Both blockers close with one action: drop the two files, ideally in their own commit that says it removes dead code, take the CI claims out of the body, and reword 5810ac094 to describe the template fix it actually makes.
The red checks are not yours. Both die pushing to OCIR anonymously because this branch predates #3257: git show <head>:.github/workflows/pull-requests.yaml | grep -c OCI_EXPORT_DIR gives 0 here and a non-zero count on main, so the fork path that exports images instead of pushing them is simply absent from the branch. A rebase should clear them.
What this PR does
Fixes #3516.
packages/apps/kubernetes/templates/oidc-rbac-job.yaml: themode=Nonecleanup path of theoidc-bootstrapJob guarded its Keycloak deletes withif kubectl api-resources --api-group=v1.edp.epam.com >/dev/null 2>&1. That command exits 0 for an absent API group (it simply prints an empty list), so the guard always fell through and the delete failed witherror: the server doesn't have a resource type "keycloakclient"—--ignore-not-foundcovers a missing object, not an unknown type. Since the Job is apost-install,post-upgradeHelm hook withbackoffLimit: 6, this blocked the Helm upgrade of everykubernetes-<cluster>release on any cluster without the EDP Keycloak operator CRDs, i.e. at the chart defaultoidc.mode: None.The probe now captures the discovery output once and gates each delete on its own resource type being present, matching the idiom already used in
packages/core/platform/images/migrations/migrations/{34,38,39}andpackages/system/ouroboros/.../cleanup-hook.yaml.api-resourcesis kept (rather thanget crd) on purpose: it hits discovery, whichsystem:discoverygrants to every authenticated principal, so the namespaced Job Role needs no cluster-scoped CRD read.packages/apps/kubernetes/tests/oidc_test.yaml: new unit test asserting the rendered script probes by output and that the exit-code form does not come back. All four of its assertions fail against the pre-fix template.hack/e2e-apps/monitoring-oidc-{system,customconfig}.bats: the same ineffective pattern, used toskipwhen the CRDs are absent. The skip never fired, so on a runner without the Keycloak operatormonitoring-oidc-systemburned its 60s timeout waiting for a KeycloakClient that cannot exist, andmonitoring-oidc-customconfigpassed for the wrong reason. Fixed the same way (e2e-install-cozystack.batsalready tests the output for its VAP preflight).cmd/check-readinessand the platform migrations were checked too: they already test the output, not the exit code.Validation
helm unittest .inpackages/apps/kubernetes: 190 tests pass. Reverting only the template makes the new test fail (all 4 assertions), so it is a real regression guard.make generateinpackages/apps/kubernetes: no diff.mode=Nonescript and ransh -n/shellcheck -s sh— clean (only pre-existingSC3040forpipefailand one pre-existingSC2086).kubectlreproducing the two real behaviors (api-resourcesexits 0 with empty output for an absent group;delete <unknown type>exits 1 despite--ignore-not-found): the pre-fix script exits 1 withthe server doesn't have a resource type "keycloakclient", the fixed script exits 0 with the CRDs absent and still issues both deletes with the CRDs present.Downstream repositories
Release note
Summary by CodeRabbit
Bug Fixes
Tests