fix(e2e): keep failure snapshots complete and gate tenant patch on aggregated-API auth - #3444
Conversation
📝 WalkthroughWalkthroughChangesE2E reliability updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Two independent e2e-harness fixes, both mechanically sound and confined to hack/ (no packages/, no chart/RBAC/migration surface); the third-party crust-gather flag semantics were verified against upstream source and the tenant-gate matches the file's existing readiness idiom, so nothing blocks — the notes below are diagnostic/verification caveats only.
Claim mismatches
None. The load-bearing third-party claim was independently verified:
--disable-additional-logsis a realcrust-gather collectflag (upstreamsrc/cli.rs:738-739). It setsConfig.disable_additional_logs, which flows intoHostLogs.disabled(src/scanners/host_logs.rs:121), andHostLogs::representations()returns early onself.disabled(host_logs.rs:143-146) BEFORE creating the per-node debug pod (host_logs.rs:151get_template_pod/get_or_create). That debug pod is documented as privileged (cli.rs:745-746). So the PR-body/comment claim "the host-log leg spawns a privileged debug pod per node; the flag skips that leg" is accurate, not narrated.
Caveats
- Phase 5b (upgrade) / 5c (config-matrix) are not applicable and were not run: the PR touches only
hack/cozytest.sh,hack/e2e-chainsaw/.chainsaw.yaml,hack/e2e-chainsaw/_lib/run-kubernetes.sh,hack/e2e-install-cozystack.bats— CI e2e harness only, zero files underpackages/, so there is no fresh-install / N-1-upgrade / chart-render surface to regress. - Phase 5d: these are test-harness fixes, so there is no isolated unit/regression test guarding them and none is expected — validation is the e2e run itself. Two efficacy claims are runtime-only and are NOT reproducible in this hermetic static review (no live cluster): (a) that the sandbox actually 403s the debug pod at all three collect sites via baseline PodSecurity, and (b) that the added authenticated
kubectl get tenants.apps.cozystack.io rootgate (hack/e2e-install-cozystack.bats:228) eliminates thesystem:anonymous403 race before the patch. Both mechanisms are coherent and the gate follows the file's own precedent (e2e-install-cozystack.bats:211,216use the sametimeout N sh -ec 'until kubectl get …'idiom, and it polls the downstream authenticated effect rather than trusting theAvailablecondition at:216-217). --disable-additional-logsis applied UNCONDITIONALLY at all three sites (cozytest.sh:151,.chainsaw.yaml:173,run-kubernetes.sh:228). Upstream, the samedisabledflag also gates node kubelet-log collection (it runs through the sameHostLogsdebug-pod path, cf.host_logs.rskubelet-log-path), so any target cluster whose debug-pod namespace (defaultdefault,host_logs.rs:253) would have PERMITTED the privileged pod now loses node-level host+kubelet logs from failure snapshots too. In the sandbox the PR's premise is that the pod is rejected everywhere (so nothing is lost); I could not verify the PSS posture of the three target clusters offline. Diagnostic-only, failure-path — non-blocking.
Recommended follow-ups
- Shell portability is clean for this diff (the added
sh -ec 'until … done'ate2e-install-cozystack.bats:228is POSIX/dash-safe; the crust-gather additions add no bashisms to the#!/bin/shcozytest.sh). No action needed — noted so it is not re-raised. - The PR body names out-of-scope "broader convergence flakiness"; if it has a tracking issue, link it so this fix's
run 30004030331root-cause is not lost.
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — two harness-only fixes, both sound. The crust-gather flag is correct and the tenant-auth gate is a real improvement; one note below on how far the gate actually closes the race.
Business context: crust-gather's host-log leg is rejected by baseline PodSecurity (exit 1, so the snapshot verdict printed FAILED on every red run even though the namespaced state was captured), and the root-tenant patch fired before cozystack-api had loaded its requestheader client-CA, so it was answered as system:anonymous (403).
--disable-additional-logs is valid for the pinned crust-gather v0.15.2 and disables only the privileged host-log leg (the one PodSecurity blocks here); object collection is unaffected. No concerns on that fix.
Non-blocking follow-ups
-
The auth gate can still lose the race with two API replicas (
hack/e2e-install-cozystack.bats:226).cozystack-apirunsreplicas: 2, the Deployment carries no readiness/liveness/startup probe, and its Service load-balances across both endpoints — so a freshly-started replica joins the endpoints before it has loaded the requestheader CA. The gate returns as soon as one GET authenticates against some replica, but the followingkubectl patchcan be routed to the other, still-anonymous replica and hit the same 403. The fix is a strict improvement over no gate and does not regress anything, and the durable product-side fix (not reporting ready before the CA is loaded) is already scoped out — so this does not block. But "no longer flakes" in the release note is stronger than what the gate guarantees. A minimal test-side close would be to retry the patch itself on failure (it is an idempotent merge patch), or to poll until every endpoint authenticates rather than just one. -
On a genuine 120s wedge the gate's
>/dev/null 2>&1leaves only exit 124, hiding whether the cause was the expected 403, a missing Tenant, or an unreachable API. One un-redirectedkubectl geton the failure path would keep it self-explanatory. Minor — the exit-trap snapshot still captures cluster state.
The diagnostic snapshot captured on a failed e2e run spawns a privileged debug pod per node for its host-log leg. Baseline PodSecurity rejects every such pod with a 403, and crust-gather's retry loop keeps trying until it burns the entire --duration budget, then exits 1 with a deadline error. The snapshot verdict therefore printed FAILED on every failure capture even though the namespaced object state had been collected, and the host-log leg itself gathered nothing usable here. Pass --disable-additional-logs at all three collect sites: the host snapshot from the BATS runner, the host snapshot from the Chainsaw error.catch, and the nested tenant snapshot. The object collection now finishes cleanly, the happy path reports complete, and FAILED once again means a genuine collection failure. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The Configure Tenant test issued its first kubectl patch of tenants/root as soon as the previous test saw the aggregated APIService report Available. That condition tracks endpoint availability, not the serving pod's auth stack: a freshly rolled cozystack-api pod can answer while it has not yet loaded the requestheader client-CA, so it drops the front-proxy identity and rejects the request as system:anonymous with a 403, failing the test 0.3s in on its very first command. Add an authenticated GET against the same resource, bounded to 120s, before the patch. It proves the aggregated API both serves and authenticates a request against the actual resource before the patch's own initial GET runs, closing the inter-test race without a blind retry; if the aggregation layer is genuinely wedged past two minutes the gate fails loud, surfacing a real regression. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
ce8c58c to
3733ebd
Compare
What this PR does
Two unrelated defects made the sandbox install job's failure handling unreliable. This fixes both.
The first hit every red run. When a test fails, the diagnostic snapshot runs
crust-gather collect, and its host-log leg spawns a privileged debug pod per node. The sandbox runs baseline PodSecurity, which rejects each of those pods with a 403, so crust-gather keeps retrying until it burns its whole collection budget and exits 1. The snapshot verdict then printedFAILEDon every failed run even though the namespaced object state was already captured, so the line that says whether the uploaded snapshot is trustworthy was always wrong. The host-log leg collects nothing usable here anyway, so this passes--disable-additional-logsat all three collect sites: the host snapshot from the BATS runner, the host snapshot from the Chainsaw error handler, and the nested tenant snapshot. Object collection now finishes and the verdict again separates a real collection failure from a complete snapshot.The second is a race in the install test. The "Configure Tenant" step issues its first
kubectl patchof the root tenant as soon as an earlier test sees the aggregated APIService goAvailable. That condition only tracks endpoint availability, not the serving pod's auth state. A freshly rolled cozystack-api pod can start answering before it has loaded the requestheader client-CA, and until it does it drops the front-proxy identity and answers assystem:anonymous, so the patch is rejected with a 403 on the test's very first command. This adds a short authenticated GET against the same resource before the patch, so the step proceeds only once the aggregated API both serves and authenticates the request. If the aggregation layer is actually wedged past two minutes the gate fails loud instead of hiding a real regression.Both were traced from a recent sandbox failure (run 30004030331, during PR #3441's CI).
Out of scope: the broader convergence flakiness of the install step is not touched here. That covers the long reconcile windows for the storage and platform releases and the app-level races in the Chainsaw suite. The durable product-side fix for the second defect, where cozystack-api should not report ready before the requestheader client-CA is loaded, is a separate follow-up.
This partially addresses #3368, its crust-gather diagnostics item, and does not close it.
Screenshots
No UI changes.
Downstream repositories
The diff touches only the e2e test harness (
hack/cozytest.sh,hack/e2e-chainsaw/.chainsaw.yaml,hack/e2e-chainsaw/_lib/run-kubernetes.sh,hack/e2e-install-cozystack.bats). The trigger map'shack/couplings arehack/e2e-prepare-cluster.bats(node prerequisites for ansible-cozystack and talm) andhack/package.mk,hack/common-envs.mk,hack/update-crd.shplus the package layout (ccp, external-apps-example, cozyhr). None of those are moved, renamed, or edited here, and nothing else in the map matches.Release note
Summary by CodeRabbit
Bug Fixes
Tests