Skip to content

Commit bdf23e6

Browse files
lexfreilexfrei
authored
fix(kubernetes): close admin-kubeconfig race on tenant cluster bootstrap (#2413)
## What this PR does Closes #2412. On a cold tenant-Kubernetes bootstrap, the parent HelmRelease raced the admin-kubeconfig Secret that Kamaji provisions asynchronously. Three CP-side Deployments (cluster-autoscaler, kccm, kcsi-controller) mounted that Secret as a hard volume, flux helm-controller's default wait budget was too short for Kamaji cold start, and `install.remediation { retries: -1 }` then uninstalled the Cluster CR and restarted the cycle forever. Implements a defense-in-depth fix: - `optional: true` on the admin-kubeconfig Secret volume in all three Deployments so kubelet no longer FailedMounts while Kamaji is still bootstrapping. - A shared `wait-for-kubeconfig` init container (in `templates/_helpers.tpl`) that polls for `super-admin.svc` with a 10m deadline, strictly below the HelmRelease Install.Timeout so a broken tenant falls into CrashLoopBackOff visibly instead of hanging forever. - Per-Application HelmRelease Install/Upgrade timeout, driven by a new `release.cozystack.io/helm-install-timeout` annotation on ApplicationDefinition. Kubernetes-rd sets it to `15m`; other kinds leave it unset and keep flux defaults, so their failed installs remediate on the normal cadence. Parser rejects ns/us/µs (accepted by `time.ParseDuration`, rejected by Flux's CRD pattern) at startup. - Soft-skip when `_namespace.etcd` is empty: the CP-side Deployments, the Cluster/KamajiControlPlane/KubevirtCluster/WorkloadMonitor CRs, and every child HelmRelease that references admin-kubeconfig now render only when an etcd DataStore exists for this tenant. An `awaiting-etcd` ConfigMap is emitted as a user-visible status beacon so `helm install` still succeeds and flux retries on its 5m interval until the Tenant chart catches up. - e2e remediation guard built on `.status.history[].status` (the Snapshot shape), not on `.status.installFailures` - `ClearFailures()` zeroes the latter on every successful reconciliation, which made the previous guard vacuous. Tests: - Go unit tests for the annotation parser (accepted/rejected units) and the HR builder (table-driven across kinds). - helm unittest for the per-template structure (optional volume, init container, dataStoreName, awaiting-etcd beacon). - bats unit tests for the shell guard (every combination of empty/zero/positive history entries, plus pinned HR v2 shape). - Chart-wide bats invariants: every Deployment mounting admin-kubeconfig has the guards; zero such Deployments and zero HelmReleases render when etcd is empty. All wired into the existing `make unit-tests` target (`go-unit-tests` added alongside `helm-unit-tests` and `bats-unit-tests`). Option 2 from the ticket (separate HelmRelease with `dependsOn`) was intentionally not taken: the combination above closes the same race without restructuring the chart's HelmRelease topology. ### Release note ```release-note fix(kubernetes): close admin-kubeconfig race on tenant Kubernetes bootstrap. The parent HelmRelease no longer enters an uninstall/retry cycle when Kamaji control-plane cold start exceeds flux's default wait budget. A Kubernetes tenant created before the parent Tenant application has etcd enabled now renders only an awaiting-etcd beacon ConfigMap and waits quietly for the DataStore to appear, instead of producing half-installed Deployments that CrashLoopBackOff forever. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Per-application Helm install/upgrade timeout via metadata annotation. * Init-container guards that wait for admin kubeconfig before workloads start. * Chart resources now render conditionally based on etcd presence. * **Tests** * Helm-template tests for admin-kubeconfig invariants and remediation-cycle detection. * New Go unit tests and CI Helm/unittest coverage plus test value files. * **Chores** * Added BusyBox image pin and new Makefile test targets (including Go unit-tests). <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents bc1eca1 + fb1ef59 commit bdf23e6

42 files changed

Lines changed: 1059 additions & 48 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎Makefile‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,19 @@ test:
8282
make -C packages/core/testing apply
8383
make -C packages/core/testing test
8484

85-
unit-tests: helm-unit-tests bats-unit-tests
85+
unit-tests: helm-unit-tests bats-unit-tests go-unit-tests
8686

8787
helm-unit-tests:
8888
hack/helm-unit-tests.sh
8989

90+
# Scoped go test over the cozystack-api surface that this repo owns. Kept
91+
# narrow intentionally - running `go test ./...` pulls in generated code
92+
# round-trip suites whose behavior depends on tool versions outside this
93+
# repo's control (kubebuilder, openapi-gen, etc.) and is better exercised
94+
# from their generator workflows.
95+
go-unit-tests:
96+
go test ./pkg/registry/... ./pkg/config/... ./pkg/cmd/server/...
97+
9098
# Discover every hack/*.bats file that is NOT an e2e test and run it
9199
# through cozytest.sh. Drop a new *.bats file in hack/ and it is picked
92100
# up automatically on the next `make unit-tests` run.

‎api/apps/v1alpha1/kubernetes/types.go‎

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎hack/admin-kubeconfig-invariant.bats‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
#!/usr/bin/env bats
2+
# -----------------------------------------------------------------------------
3+
# Chart-wide invariant for packages/apps/kubernetes:
4+
#
5+
# Every Deployment in this chart that mounts <release>-admin-kubeconfig as a
6+
# Secret volume MUST:
7+
# - declare that volume optional: true (so kubelet does not FailedMount
8+
# while Kamaji is still provisioning the Secret), AND
9+
# - include the wait-for-kubeconfig init container (so the pod becomes
10+
# Ready only after Kamaji publishes the Secret).
11+
#
12+
# The per-template unittests in packages/apps/kubernetes/tests/ lock in
13+
# today's three Deployments (cluster-autoscaler, kccm, csi controller) by
14+
# name. This invariant is stricter: any future Deployment added to this
15+
# chart that mounts the same Secret but forgets the guard will fail here.
16+
#
17+
# Requires: helm, yq (mikefarah v4+), jq. All three are available on the
18+
# project's CI runners and on the maintainer workstation.
19+
# -----------------------------------------------------------------------------
20+
21+
@test "every Deployment mounting admin-kubeconfig has optional:true and wait-for-kubeconfig init" {
22+
values_file="packages/apps/kubernetes/tests/values-ci.yaml"
23+
[ -f "$values_file" ]
24+
25+
tmp=$(mktemp -d)
26+
trap 'rm -rf "$tmp"' EXIT
27+
28+
helm template invariant packages/apps/kubernetes \
29+
--namespace tenant-root \
30+
--values "$values_file" \
31+
2>/dev/null > "$tmp/rendered.yaml"
32+
33+
# yq streams one JSON object per input document. jq -s slurps the stream
34+
# into an array so we can treat all Deployments as a single collection.
35+
yq --output-format=json eval-all '.' "$tmp/rendered.yaml" \
36+
| jq -s --raw-output '
37+
map(select(.kind == "Deployment")) |
38+
map({
39+
name: .metadata.name,
40+
volumes: (.spec.template.spec.volumes // []),
41+
initNames: ((.spec.template.spec.initContainers // []) | map(.name)),
42+
}) |
43+
map(
44+
.name as $n |
45+
.initNames as $ins |
46+
(.volumes[] | select(.secret.secretName | test("-admin-kubeconfig$")?))
47+
| {
48+
name: $n,
49+
optional: (.secret.optional == true),
50+
hasInit: ($ins | index("wait-for-kubeconfig") != null),
51+
}
52+
)
53+
' > "$tmp/summary.json"
54+
55+
# At least one Deployment must match; if a refactor removes every
56+
# admin-kubeconfig volume from this chart, the test must be updated
57+
# deliberately rather than silently passing.
58+
matched=$(jq 'length' "$tmp/summary.json")
59+
[ "$matched" -ge 1 ]
60+
61+
offenders=$(jq --raw-output '.[] | select(.optional != true or .hasInit != true) | .name' "$tmp/summary.json")
62+
63+
if [ -n "$offenders" ]; then
64+
echo "Deployments mounting *-admin-kubeconfig without optional:true + wait-for-kubeconfig init:" >&2
65+
echo "$offenders" >&2
66+
echo "Full summary:" >&2
67+
cat "$tmp/summary.json" >&2
68+
exit 1
69+
fi
70+
71+
echo "Invariant holds for $matched Deployment(s)"
72+
}
73+
74+
@test "chart emits zero admin-kubeconfig Deployments when tenant has no etcd DataStore" {
75+
# Without a DataStore (parent Tenant has not populated _namespace.etcd yet)
76+
# the control-plane-side Deployments must NOT render at all. If they did,
77+
# the wait-for-kubeconfig init would CrashLoopBackOff indefinitely - there
78+
# would be no KamajiControlPlane to provision the Secret - consuming the
79+
# HelmRelease wait budget and triggering exactly the remediation cycle the
80+
# rest of this chart tries to avoid. This test renders the whole chart
81+
# with etcd empty and asserts no Deployment references the admin-kubeconfig
82+
# Secret.
83+
tmp=$(mktemp -d)
84+
trap 'rm -rf "$tmp"' EXIT
85+
86+
helm template invariant packages/apps/kubernetes \
87+
--namespace tenant-root \
88+
--values packages/apps/kubernetes/tests/values-ci-no-etcd.yaml \
89+
2>/dev/null > "$tmp/rendered.yaml"
90+
91+
matched=$(
92+
yq --output-format=json eval-all '.' "$tmp/rendered.yaml" \
93+
| jq -s '
94+
map(select(.kind == "Deployment")) |
95+
map(select(
96+
(.spec.template.spec.volumes // [])
97+
| any(.secret.secretName | test("-admin-kubeconfig$")?)
98+
)) |
99+
length
100+
'
101+
)
102+
103+
if [ "$matched" -ne 0 ]; then
104+
echo "Expected zero Deployments mounting *-admin-kubeconfig when etcd is empty, got $matched:" >&2
105+
yq --output-format=json eval-all '.' "$tmp/rendered.yaml" \
106+
| jq -s 'map(select(.kind == "Deployment") | .metadata.name)' >&2
107+
exit 1
108+
fi
109+
110+
echo "No admin-kubeconfig Deployments rendered for empty etcd (as expected)"
111+
}
112+
113+
@test "chart emits zero admin-kubeconfig HelmReleases when tenant has no etcd DataStore" {
114+
# Same principle as the Deployment variant above, extended to every child
115+
# HelmRelease (cilium, coredns, csi, cert-manager, ...). They reference
116+
# *-admin-kubeconfig via kubeConfig.secretRef and would otherwise sit in
117+
# NotReady forever on an etcd-less tenant, polluting the HelmRelease list
118+
# the operator sees and contradicting the "awaiting-etcd beacon only"
119+
# contract of the soft-skip path.
120+
tmp=$(mktemp -d)
121+
trap 'rm -rf "$tmp"' EXIT
122+
123+
helm template invariant packages/apps/kubernetes \
124+
--namespace tenant-root \
125+
--values packages/apps/kubernetes/tests/values-ci-no-etcd.yaml \
126+
2>/dev/null > "$tmp/rendered.yaml"
127+
128+
matched=$(
129+
yq --output-format=json eval-all '.' "$tmp/rendered.yaml" \
130+
| jq -s '
131+
map(select(.kind == "HelmRelease")) |
132+
map(select(.spec.kubeConfig.secretRef.name | test("-admin-kubeconfig$")?)) |
133+
length
134+
'
135+
)
136+
137+
if [ "$matched" -ne 0 ]; then
138+
echo "Expected zero HelmReleases referencing *-admin-kubeconfig when etcd is empty, got $matched:" >&2
139+
yq --output-format=json eval-all '.' "$tmp/rendered.yaml" \
140+
| jq -s 'map(select(.kind == "HelmRelease") | .metadata.name)' >&2
141+
exit 1
142+
fi
143+
144+
echo "No admin-kubeconfig HelmReleases rendered for empty etcd (as expected)"
145+
}

‎hack/e2e-apps/remediation-guard.sh‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
# Helpers for asserting that a Flux HelmRelease did not fall into an
2+
# install/upgrade remediation cycle during an e2e run.
3+
#
4+
# Background: Flux helm-controller's ClearFailures() zeroes
5+
# .status.installFailures / .status.upgradeFailures on every successful
6+
# reconciliation (see the upstream ClearFailures method on
7+
# HelmReleaseStatus). That makes those counters useless for a guard that
8+
# runs after the HelmRelease has reached Ready - the values are always 0.
9+
#
10+
# What survives a successful reconciliation is .status.history, a bounded
11+
# list of release Snapshots. Each Snapshot carries a status field that
12+
# tracks the Helm release state: deployed, superseded, failed, uninstalled,
13+
# and so on. A remediation cycle leaves the footprint behind: a snapshot
14+
# with status "uninstalled" (from install/upgrade remediation) or "failed"
15+
# (Helm release failure that remediation then uninstalled). Those stay in
16+
# history even after a subsequent successful reinstall.
17+
#
18+
# helmrelease_has_remediation_cycle takes a newline-delimited list of
19+
# snapshot statuses (whatever the caller extracted via kubectl -o jsonpath
20+
# or equivalent) and returns 0 (detected) when any entry is "failed" or
21+
# "uninstalled", 1 otherwise. Empty input is treated as "no history yet,
22+
# no cycle observed".
23+
24+
helmrelease_has_remediation_cycle() {
25+
statuses="$1"
26+
if [ -z "${statuses}" ]; then
27+
return 1
28+
fi
29+
# printf + grep over the pipe, rather than a heredoc plus while read.
30+
# printf %s treats the status string as a literal payload, so any stray
31+
# $ in a future caller's input does not trigger shell expansion. grep
32+
# returns 0 iff at least one line matches the allowlist, which is
33+
# exactly the contract the caller wants, so we can return its exit
34+
# status directly.
35+
if printf '%s\n' "${statuses}" | grep --extended-regexp --quiet '^(failed|uninstalled)$'; then
36+
return 0
37+
fi
38+
return 1
39+
}

‎hack/e2e-apps/run-kubernetes.sh‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
. hack/e2e-apps/remediation-guard.sh
2+
13
run_kubernetes_test() {
24
local version_expr="$1"
35
local test_name="$2"
@@ -320,6 +322,35 @@ EOF
320322
done
321323
kubectl wait hr kubernetes-${test_name}-ingress-nginx -n tenant-test --timeout=5m --for=condition=ready
322324

325+
# Guard: parent HelmRelease must not have entered an install/upgrade remediation cycle.
326+
# A non-zero installFailures/upgradeFailures indicates the helm-wait budget expired while
327+
# admin-kubeconfig was still being provisioned, which would trigger uninstall remediation
328+
# and churn the Cluster CR.
329+
# Flux helm-controller v2 retains per-revision release Snapshots in
330+
# .status.history; each Snapshot's .status reflects the Helm release
331+
# state (deployed/superseded/failed/uninstalled). A remediation cycle
332+
# leaves a "failed" or "uninstalled" entry behind that survives a later
333+
# successful reinstall, unlike the installFailures/upgradeFailures
334+
# counters (which ClearFailures zeroes on every successful reconcile).
335+
# The shape is pinned by hack/remediation-guard.bats; the upstream
336+
# types are github.com/fluxcd/helm-controller/api v2 Snapshot.
337+
history_statuses=$(kubectl get hr -n tenant-test "kubernetes-${test_name}" \
338+
-ojsonpath='{range .status.history[*]}{.status}{"\n"}{end}')
339+
# Always emit the raw value so a silent future-Flux field rename shows
340+
# up as "empty history on a Ready HR" in CI logs rather than vanishing.
341+
echo "Parent HelmRelease history statuses:"
342+
printf '%s\n' "${history_statuses:-<empty>}"
343+
if [ -z "${history_statuses}" ]; then
344+
echo "Unexpected empty .status.history on a Ready HelmRelease - Flux API shape may have changed." >&2
345+
kubectl -n tenant-test describe hr "kubernetes-${test_name}" >&2
346+
exit 1
347+
fi
348+
if helmrelease_has_remediation_cycle "${history_statuses}"; then
349+
echo "Parent HelmRelease entered remediation cycle." >&2
350+
kubectl -n tenant-test describe hr "kubernetes-${test_name}" >&2
351+
exit 1
352+
fi
353+
323354
# Clean up
324355
pkill -f "port-forward.*${port}:" 2>/dev/null || true
325356
rm -f "tenantkubeconfig-${test_name}"

‎hack/remediation-guard.bats‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
#!/usr/bin/env bats
2+
# -----------------------------------------------------------------------------
3+
# Unit tests for hack/e2e-apps/remediation-guard.sh
4+
#
5+
# helmrelease_has_remediation_cycle takes a newline-delimited list of
6+
# HelmRelease history snapshot status values (deployed/superseded/failed/
7+
# uninstalled/...) and returns 0 when any entry is "failed" or "uninstalled"
8+
# (meaning flux helm-controller performed install/upgrade remediation).
9+
#
10+
# This is used by the e2e script after the HelmRelease reaches Ready. The
11+
# failure/upgrade counters (.status.installFailures / .status.upgradeFailures)
12+
# are useless there because flux's ClearFailures zeroes them on successful
13+
# reconciliation; .status.history retains the snapshot trail.
14+
#
15+
# cozytest.sh's awk parser recognizes only @test blocks and a bare `}` on
16+
# its own line; there is no bats `run` or `$status`. Assertions are
17+
# expressed as direct shell tests that exit non-zero on failure.
18+
#
19+
# Run with: hack/cozytest.sh hack/remediation-guard.bats
20+
# -----------------------------------------------------------------------------
21+
22+
@test "empty history returns not-detected" {
23+
. hack/e2e-apps/remediation-guard.sh
24+
if helmrelease_has_remediation_cycle ""; then
25+
echo "expected not-detected for empty history" >&2
26+
exit 1
27+
fi
28+
}
29+
30+
@test "single deployed snapshot returns not-detected" {
31+
. hack/e2e-apps/remediation-guard.sh
32+
if helmrelease_has_remediation_cycle "deployed"; then
33+
echo "expected not-detected for deployed-only history" >&2
34+
exit 1
35+
fi
36+
}
37+
38+
@test "deployed then superseded returns not-detected" {
39+
. hack/e2e-apps/remediation-guard.sh
40+
statuses=$(printf 'deployed\nsuperseded\n')
41+
if helmrelease_has_remediation_cycle "${statuses}"; then
42+
echo "expected not-detected for deployed+superseded history" >&2
43+
exit 1
44+
fi
45+
}
46+
47+
@test "single failed snapshot returns detected" {
48+
. hack/e2e-apps/remediation-guard.sh
49+
if ! helmrelease_has_remediation_cycle "failed"; then
50+
echo "expected detected when history contains failed snapshot" >&2
51+
exit 1
52+
fi
53+
}
54+
55+
@test "single uninstalled snapshot returns detected" {
56+
# The exact signature of the install-remediation race: the first install
57+
# exceeded flux's wait budget, remediation uninstalled, the next retry
58+
# eventually succeeded. History still carries the uninstalled snapshot.
59+
. hack/e2e-apps/remediation-guard.sh
60+
if ! helmrelease_has_remediation_cycle "uninstalled"; then
61+
echo "expected detected when history contains uninstalled snapshot" >&2
62+
exit 1
63+
fi
64+
}
65+
66+
@test "uninstalled then deployed still returns detected" {
67+
. hack/e2e-apps/remediation-guard.sh
68+
statuses=$(printf 'uninstalled\ndeployed\n')
69+
if ! helmrelease_has_remediation_cycle "${statuses}"; then
70+
echo "expected detected despite later successful deploy" >&2
71+
exit 1
72+
fi
73+
}
74+
75+
@test "deployed then failed still returns detected" {
76+
. hack/e2e-apps/remediation-guard.sh
77+
statuses=$(printf 'deployed\nfailed\n')
78+
if ! helmrelease_has_remediation_cycle "${statuses}"; then
79+
echo "expected detected when any entry is failed" >&2
80+
exit 1
81+
fi
82+
}
83+
84+
@test "status.history extraction pins HR v2 status.history shape" {
85+
# Pins the Flux HelmRelease v2 .status.history[].status shape that
86+
# run-kubernetes.sh relies on. If a future flux release renames the
87+
# field, the jsonpath returns nothing, the guard reports no cycle,
88+
# and real remediation loops slip past the e2e assertion. This test
89+
# uses yq to read the exact path used in the e2e script; the upstream
90+
# Snapshot type lives at
91+
# github.com/fluxcd/helm-controller/api/v2.Snapshot (via go.mod).
92+
tmp=$(mktemp -d)
93+
trap 'rm -rf "$tmp"' EXIT
94+
95+
cat > "$tmp/hr.yaml" <<'YAML'
96+
apiVersion: helm.toolkit.fluxcd.io/v2
97+
kind: HelmRelease
98+
metadata:
99+
name: kubernetes-test
100+
namespace: tenant-test
101+
status:
102+
history:
103+
- name: kubernetes-test
104+
namespace: tenant-test
105+
version: 1
106+
status: uninstalled
107+
- name: kubernetes-test
108+
namespace: tenant-test
109+
version: 2
110+
status: deployed
111+
YAML
112+
113+
# Default yq output is yaml scalar format, which for string values emits
114+
# bare unquoted tokens - matching what kubectl -o jsonpath produces in
115+
# e2e. Do not switch to JSON output here; that would quote the values
116+
# and break the loop in helmrelease_has_remediation_cycle.
117+
statuses=$(yq '.status.history[].status' "$tmp/hr.yaml")
118+
119+
[ -n "$statuses" ]
120+
echo "$statuses" | grep --quiet '^uninstalled$'
121+
122+
. hack/e2e-apps/remediation-guard.sh
123+
if ! helmrelease_has_remediation_cycle "$statuses"; then
124+
echo "expected detected for pinned HR snippet with uninstalled + deployed history" >&2
125+
exit 1
126+
fi
127+
}

0 commit comments

Comments
 (0)