diff --git a/.github/workflows/pull-requests.yaml b/.github/workflows/pull-requests.yaml index 97f7cdfe9..b8b06fedf 100644 --- a/.github/workflows/pull-requests.yaml +++ b/.github/workflows/pull-requests.yaml @@ -353,6 +353,17 @@ jobs: sudo "$bats_source/install.sh" /usr/local fi bats --version | grep -qx "Bats $BATS_VERSION" + # setup-envtest and the kube-apiserver and etcd it fetches, so a pull + # request downloads them only when the versions pinned in the Makefile + # change, and an outage at their sources cannot fail unrelated changes. + # The envtest target runs the controller suites a fake client cannot + # stand in for. + - name: Cache envtest binaries + if: needs.plan.outputs.code == 'true' + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: _out/envtest + key: envtest-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('Makefile') }} # -k is required, not cosmetic: unit-tests and test-controllers are two # independent goals on the same command line, and without -k make stops # handing out new work the moment any unit-tests prerequisite fails — @@ -361,7 +372,7 @@ jobs: # goals keep running; the job still fails overall on any red target. - name: Run unit and controller tests if: needs.plan.outputs.code == 'true' - run: make unit-tests test-controllers -j4 -k --output-sync=target + run: make unit-tests test-controllers test-controllers-envtest -j4 -k --output-sync=target - name: Run Bats unit tests for docs-only changes if: needs.plan.outputs.code == 'false' run: make bats-unit-tests diff --git a/Makefile b/Makefile index 412568384..99de4c38e 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: manifests assets prepare-env prepare-env-container unit-tests helm-unit-tests bats-unit-tests bats-unit-files-check bats-posix-compat-tests print-bats-unit-files print-bats-posix-compat-files print-bats-jobs rd-presets-check migrations-target-check test test-controllers test-backport-audit preflight +.PHONY: manifests assets prepare-env prepare-env-container unit-tests helm-unit-tests bats-unit-tests bats-unit-files-check bats-posix-compat-tests print-bats-unit-files print-bats-posix-compat-files print-bats-jobs rd-presets-check migrations-target-check test test-controllers test-controllers-envtest test-backport-audit preflight include hack/common-envs.mk @@ -163,6 +163,29 @@ go-module-tests: test-controllers: go test ./internal/... -count=1 +# Controller suites that need a real apiserver (build tag `envtest`): server-side +# apply ownership, apply preconditions and finalizer handling, which a fake client +# does not implement. setup-envtest fetches the kube-apiserver and etcd release +# pinned by ENVTEST_K8S_VERSION into _out/envtest; the HelmRelease CRD comes from +# the in-tree Flux manifests (envtest skips their non-CRD objects). CI runs it +# next to test-controllers. Only the packages listed below are compiled, so a +# package that gains an envtest suite has to be added here. +# +# setup-envtest itself is built into _out/envtest too, and binaries already there +# are used without asking the release index, so a warm _out/envtest (CI caches +# it) needs no network at all. +ENVTEST_K8S_VERSION ?= 1.35.0 +SETUP_ENVTEST_VERSION ?= v0.0.0-20260305142021-f9589b9f2b9d +ENVTEST_DIR = $(CURDIR)/_out/envtest +SETUP_ENVTEST = $(ENVTEST_DIR)/bin/setup-envtest-$(SETUP_ENVTEST_VERSION) +$(SETUP_ENVTEST): + GOBIN=$(ENVTEST_DIR)/bin go install sigs.k8s.io/controller-runtime/tools/setup-envtest@$(SETUP_ENVTEST_VERSION) + mv $(ENVTEST_DIR)/bin/setup-envtest $@ +test-controllers-envtest: $(SETUP_ENVTEST) + KUBEBUILDER_ASSETS="$$($(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --installed-only --bin-dir $(ENVTEST_DIR) --print path 2>/dev/null || $(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(ENVTEST_DIR) --print path)" \ + HELMRELEASE_CRD_DIR=$(CURDIR)/internal/fluxinstall/manifests \ + go test -tags envtest -run '^TestEnvtest' ./internal/controller/fluxplunger/... -count=1 + # Black-box golden test for cmd/check-readiness. Builds the binary and runs it # against a mock kubectl (fixtures under test/check-readiness/testdata), # diffing stdout/stderr/exit against golden files. Regenerate goldens with: diff --git a/cmd/flux-plunger/main.go b/cmd/flux-plunger/main.go index 13fe88cd3..a45ef71f3 100644 --- a/cmd/flux-plunger/main.go +++ b/cmd/flux-plunger/main.go @@ -20,6 +20,7 @@ import ( "crypto/tls" "flag" "os" + "time" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. @@ -56,6 +57,7 @@ func main() { var probeAddr string var secureMetrics bool var enableHTTP2 bool + var shutdownDrainTimeout time.Duration var tlsOpts []func(*tls.Config) flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ @@ -68,6 +70,9 @@ func main() { "If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.") flag.BoolVar(&enableHTTP2, "enable-http2", false, "If set, HTTP/2 will be enabled for the metrics server") + flag.DurationVar(&shutdownDrainTimeout, fluxplunger.ShutdownDrainTimeoutFlag, 75*time.Second, + "How long to spend giving up the uninstall fences flux-plunger holds before it exits. "+ + "Keep it inside the pod's termination grace period.") opts := zap.Options{ Development: false, @@ -102,31 +107,25 @@ func main() { TLSOpts: tlsOpts, } - mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ + mgrOpts := ctrl.Options{ Scheme: scheme, Metrics: metricsServerOptions, HealthProbeBindAddress: probeAddr, LeaderElection: enableLeaderElection, LeaderElectionID: "flux-plunger.cozystack.io", - // LeaderElectionReleaseOnCancel defines if the leader should step down voluntarily - // when the Manager ends. This requires the binary to immediately end when the - // Manager is stopped, otherwise, this setting is unsafe. Setting this significantly - // speeds up voluntary leader transitions as the new leader don't have to wait - // LeaseDuration time first. - // - // In the default scaffold provided, the program ends immediately after - // the manager stops, so would be fine to enable this option. However, - // if you are doing or is intended to do any operation such as perform cleanups - // after the manager stops then its usage might be unsafe. - // LeaderElectionReleaseOnCancel: true, - }) + } + // The Lease is held, not released, through the drain after the manager stops. + fluxplunger.ApplyShutdownTimings(&mgrOpts, shutdownDrainTimeout) + mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), mgrOpts) if err != nil { setupLog.Error(err, "unable to create manager") os.Exit(1) } if err = (&fluxplunger.FluxPlunger{ - Client: mgr.GetClient(), + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Recorder: mgr.GetEventRecorderFor("flux-plunger"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "FluxPlunger") os.Exit(1) @@ -144,7 +143,7 @@ func main() { } setupLog.Info("starting manager") - if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { + if err := fluxplunger.RunManager(ctrl.SetupSignalHandler(), mgr, shutdownDrainTimeout); err != nil { setupLog.Error(err, "problem running manager") os.Exit(1) } diff --git a/hack/bats-runner-contract.bats b/hack/bats-runner-contract.bats index 22426edb2..a50ed0ba5 100644 --- a/hack/bats-runner-contract.bats +++ b/hack/bats-runner-contract.bats @@ -75,7 +75,7 @@ brc_write_parallel_stub() { code_command=$(yq -r '.jobs.checks.steps[] | select(.name == "Run unit and controller tests") | .run' "$BRC_WORKFLOW") docs_command=$(yq -r '.jobs.checks.steps[] | select(.name == "Run Bats unit tests for docs-only changes") | .run' "$BRC_WORKFLOW") [ -z "$lint_skip" ] - [ "$code_command" = 'make unit-tests test-controllers -j4 -k --output-sync=target' ] + [ "$code_command" = 'make unit-tests test-controllers test-controllers-envtest -j4 -k --output-sync=target' ] [ "$docs_command" = 'make bats-unit-tests' ] } diff --git a/hack/downstream-trigger-map.bats b/hack/downstream-trigger-map.bats index b26996755..c5ebf6af3 100644 --- a/hack/downstream-trigger-map.bats +++ b/hack/downstream-trigger-map.bats @@ -119,7 +119,7 @@ map_section() { [ "$code_if" = "needs.plan.outputs.code == 'true'" ] [ "$docs_if" = "needs.plan.outputs.code == 'false'" ] [ "$docs_run" = 'make bats-unit-tests' ] - [ "$code_run" = 'make unit-tests test-controllers -j4 -k --output-sync=target' ] + [ "$code_run" = 'make unit-tests test-controllers test-controllers-envtest -j4 -k --output-sync=target' ] } @test "the trigger map and the PR-template checklist list the same repositories" { diff --git a/hack/flux-plunger-fence-hook_test.bats b/hack/flux-plunger-fence-hook_test.bats new file mode 100644 index 000000000..14cd7f835 --- /dev/null +++ b/hack/flux-plunger-fence-hook_test.bats @@ -0,0 +1,728 @@ +#!/usr/bin/env bats +# Behavioural test for flux-plunger's pre-delete hook +# (packages/system/flux-plunger/templates/uninstall-fence-cleanup.yaml), the +# backstop that releases what flux-plunger's own shutdown drain could not. +# +# The script is extracted from the rendered chart and RUN against a fake kubectl +# that keeps each HelmRelease's finalizers, suspension owner, resourceVersion, +# generation and observed generation in files, and a fake clock. The fake answers +# the hook's templates only when they ask the right question (the exact fence +# literal, the f:suspend field of flux-plunger's Apply entry, the exact +# "helm-controller has not handled it" predicate), lists a release once per fence +# entry as the real template does unless the template collapses them, bumps a +# release's generation when its suspension is lifted as the apiserver does, lifts +# a suspension only as a JSON patch preconditioned on the resourceVersion and uid +# it read and never one another manager also holds, refuses kubectl apply, and +# applies a finalizer removal only when it is the guarded test-then-remove at an +# index that holds the fence. The helm-unittest +# suites in packages/system/flux-plunger/tests match the manifest text instead. +# +# CI runs this file under Bats through `make bats-unit-tests`. It also remains +# compatible with the legacy `hack/cozytest.sh` translator, so each @test builds +# its own fixture and removes it at the end of the body instead of using +# setup()/teardown(). No EXIT trap: docs/agents/e2e-testing.md bans them in +# hack/*.bats. +# +# Run with: bats hack/flux-plunger-fence-hook_test.bats + +load test_helper + +CHART=packages/system/flux-plunger +JOB=flux-plunger-uninstall-fence-cleanup +FENCE=flux-plunger.cozystack.io/uninstall-fence +HELMFIN=finalizers.fluxcd.io + +# render_hook [helm args] +# Extract the hook Job's shell script and activeDeadlineSeconds, the script's pod +# wait, retry window, selector and Deployment name, and the Deployment's grace +# period, name and pod labels, and the hook Role's resourceNames. The non-empty +# checks are early `return 1`s: hack/cozytest.sh rewrites every line matching ^}$ +# into `return 0` plus `}`, helpers included. +render_hook() { + out=$1 + shift + helm template flux-plunger "$CHART" --namespace cozy-fluxcd "$@" > "$out.all" + yq eval "select(.kind == \"Job\" and .metadata.name == \"$JOB\") | .spec.template.spec.containers[0].command[2]" "$out.all" > "$out" + [ -s "$out" ] || return 1 + DEADLINE=$(yq eval "select(.kind == \"Job\" and .metadata.name == \"$JOB\") | .spec.activeDeadlineSeconds" "$out.all") + GRACE=$(yq eval 'select(.kind == "Deployment") | .spec.template.spec.terminationGracePeriodSeconds' "$out.all") + DEPLOY_NAME=$(yq eval 'select(.kind == "Deployment") | .metadata.name' "$out.all") + ROLE_DEPLOY=$(yq eval "select(.kind == \"Role\" and .metadata.name == \"$JOB\") | .rules[0].resourceNames[0]" "$out.all") + POD_LABELS=$(yq eval 'select(.kind == "Deployment") | .spec.template.metadata.labels | to_entries | map(.key + "=" + .value) | join(",")' "$out.all") + POD_WAIT=$(sed -n 's/^ *pod_wait=\([0-9][0-9]*\)$/\1/p' "$out") + WINDOW=$(sed -n 's/.*retry_until=\$(( \$(date +%s) + \([0-9][0-9]*\) )).*/\1/p' "$out") + SCRIPT_DEPLOY=$(sed -n 's/^ *deploy=\([a-z0-9-]*\)$/\1/p' "$out") + POD_SELECTOR=$(sed -n 's/.*--selector \([^ ]*\).*/\1/p' "$out" | sort -u) + for v in "$DEADLINE" "$GRACE" "$DEPLOY_NAME" "$ROLE_DEPLOY" "$POD_LABELS" "$POD_WAIT" "$WINDOW" "$SCRIPT_DEPLOY" "$POD_SELECTOR"; do + [ -n "$v" ] || return 1 + done +} + +# add_release ... +# One HelmRelease, its finalizers in order, one per line, at generation 1 and +# observed, resourceVersion 1, suspended. +add_release() { + st=$1 ns=$2 name=$3 + shift 3 + : > "$st/$ns@$name.fin" + for f in "$@"; do + echo "$f" >> "$st/$ns@$name.fin" + done + echo 1 > "$st/$ns@$name.gen" + echo 1 > "$st/$ns@$name.obs" + echo 1 > "$st/$ns@$name.rv" + touch "$st/$ns@$name.suspended" +} + +# fences_left : how many fence entries remain across all releases. +fences_left() { + cat "$1"/*.fin 2>/dev/null | grep --count --line-regexp "$FENCE" +} + +# write_fakes +# A fake kubectl, sleep and date sharing one simulated clock in $CLOCK (seconds +# since the hook started): sleep advances it, date +%s reads it. kubectl logs +# every call to $KLOG and works on the release state in $STATE: +# * @.fin finalizers, in order; +# * @.gen/.obs metadata.generation and status.observedGeneration; +# lifting a suspension bumps the generation; +# * @.rv resourceVersion, bumped by every write; +# * @.suspended spec.suspend is true; +# * @.deleting the release is being deleted; +# * @.owned flux-plunger's Apply entry owns spec.suspend, +# holding what a correct ownership template prints; +# * @.coowned another manager also owns spec.suspend at true; +# flux-plunger's own path leaves such a suspension; +# * @.applyonly flux-plunger has an Apply entry that does not own +# spec.suspend; +# * @.updateowned flux-plunger owns spec.suspend through an Update +# entry, as an earlier flux-plunger's merge patch left +# it; the controller does not take that one as its own; +# * @.vanish the release is deleted the first time it is read +# after the listing; reading it then is NotFound +# unless --ignore-not-found is given; +# * @.readd something puts the fence back after every removal, +# as a flux-plunger still running would; +# * @.latefence the release gains the fence right after the first +# listing, as one a running flux-plunger fences would; +# * deploy.absent the flux-plunger Deployment does not exist; +# * pods.stuck flux-plunger's pod never goes: the wait runs to its +# --timeout on the clock (a negative one, as kubectl +# reads it, for a week) and fails, and the pod lists; +# * scale.slow seconds a successful scale-down takes; +# * .fail a count of calls to fail first, for the Deployment +# read (deploy), the scale-down (scale), the pod wait +# for a reason other than pods (wait), the listing +# (list), a listing after the first (relist), the +# pod listing (pods), a release's ownership read +# (@.ownedread), unsuspend +# (@.apply), unsuspend racing another +# write (@.applyconflict) or finalizer patch +# (@.patch); +# * list.partial.fail a count of listings that print what they have, then +# fail, as kubectl does on a partial error. +# Releases whose suspension was lifted are appended to $STATE/unsuspended. +# No line below is a bare column-0 closing brace, so cozytest.sh leaves it intact. +write_fakes() { + cat > "$1/kubectl" <<'KEOF' +#!/bin/sh +echo "$*" >> "$KLOG" +FENCE=flux-plunger.cozystack.io/uninstall-fence +HELMFIN=finalizers.fluxcd.io +# The one predicate that tells a release helm-controller has not yet handled in +# its unsuspended form: being deleted, no longer held by its finalizer, and its +# latest generation not yet observed. +UNHANDLED='{{if .metadata.deletionTimestamp}}{{$held := false}}{{range .metadata.finalizers}}{{if eq . "finalizers.fluxcd.io"}}{{$held = true}}{{end}}{{end}}{{if and (not $held) (ne (printf "%v" .status.observedGeneration) (printf "%v" .metadata.generation))}}' +# In the hook's pod kubectl finds the API server only through the in-cluster +# config, which client-go stops falling back to once a client override such as +# --request-timeout is set: the call then goes to localhost:8080 and fails. +case " $* " in + *" --request-timeout"*|*" --server="*|*" --server "*|*" --token"*) + echo "The connection to the server localhost:8080 was refused" >&2 + exit 1 ;; +esac +take_fail() { + [ -s "$1.fail" ] || return 1 + n=$(cat "$1.fail") + [ "$n" -gt 0 ] || return 1 + echo $((n - 1)) > "$1.fail" + return 0 + } +bump_rv() { + echo $(( $(cat "$1.rv") + 1 )) > "$1.rv" + } +ns="" name="" payload="" tmpl="" type="" fm="" all=no kind="" verb="" ignore=no timeout="" for="" ssa=no file="" +while [ $# -gt 0 ]; do + case "$1" in + --namespace) ns=$2; shift ;; + --all-namespaces) all=yes ;; + --output) tmpl=$2; shift ;; + --type) type=$2; shift ;; + --patch) payload=$2; shift ;; + --field-manager) fm=$2; shift ;; + --filename) file=$2; shift ;; + --selector) shift ;; + --ignore-not-found) ignore=yes ;; + --server-side) ssa=yes ;; + --timeout=*) timeout=${1#--timeout=}; timeout=${timeout%s} ;; + --for=*) for=${1#--for=} ;; + --*) ;; + get|patch|scale|wait|apply) [ -n "$verb" ] || verb=$1 ;; + helmreleases.helm.toolkit.fluxcd.io) kind=hr ;; + deployment) kind=deployment ;; + pod) kind=pod ;; + *) if [ -n "$kind" ] && [ -z "$name" ]; then name=$1; fi ;; + esac + shift +done +case "$verb" in + scale) + if take_fail "$STATE/scale"; then echo "fake: the scale-down was rejected" >&2; exit 1; fi + if [ -s "$STATE/scale.slow" ]; then echo $(( $(cat "$CLOCK") + $(cat "$STATE/scale.slow") )) > "$CLOCK"; fi + echo "deployment.apps/$name scaled" + exit 0 ;; + wait) + if [ "$for" != delete ]; then echo "fake: waiting on '$for', not on the pods being gone" >&2; exit 64; fi + if take_fail "$STATE/wait"; then echo "error: watch closed before the condition was met" >&2; exit 1; fi + if [ -e "$STATE/pods.stuck" ]; then + now=$(cat "$CLOCK") + # kubectl wait: zero checks once, a negative timeout waits for a week. + [ "$timeout" -ge 0 ] || timeout=604800 + echo $((now + timeout)) > "$CLOCK" + echo "error: timed out waiting for the condition on pods/flux-plunger-0" >&2 + exit 1 + fi + exit 0 ;; +esac +if [ "$kind" = pod ] && [ "$verb" = get ]; then + if take_fail "$STATE/pods"; then echo "fake: the pod listing timed out" >&2; exit 1; fi + [ -e "$STATE/pods.stuck" ] && echo "pod/flux-plunger-0" + exit 0 +fi +if [ "$kind" = deployment ] && [ "$verb" = get ]; then + if take_fail "$STATE/deploy"; then echo "fake: the Deployment read timed out" >&2; exit 1; fi + [ -e "$STATE/deploy.absent" ] || echo "deployment.apps/$name" + exit 0 +fi +if [ "$verb" = apply ]; then + # kubectl apply --server-side migrates every field kubectl-client-side-apply + # owns to the applying manager and then re-applies, so a minimal manifest strips + # those fields from a release created with a client-side kubectl apply. + echo "fake: kubectl apply takes over and strips client-side-apply fields" >&2 + exit 64 +fi +rel="$STATE/$ns@$name" +if [ "$verb" = get ] && [ -z "$name" ]; then + if take_fail "$STATE/list"; then echo "fake: the listing was throttled" >&2; exit 1; fi + if [ -e "$STATE/listed" ] && take_fail "$STATE/relist"; then echo "fake: the second listing was throttled" >&2; exit 1; fi + partial=no + if take_fail "$STATE/list.partial"; then partial=yes; fi + case "$tmpl" in *"eq . \"$FENCE\""*) ;; *) exit 0 ;; esac + for f in "$STATE"/*@*.fin; do + [ -e "$f" ] || continue + r=$(basename "$f" .fin) + rns=${r%@*} + if [ "$all" = yes ] || [ "$rns" = "$ns" ]; then + case "$tmpl" in + *'{{if $fenced}}'*) grep -q -x "$FENCE" "$f" && printf '%s/%s\n' "$rns" "${r#*@}" ;; + *) grep -x "$FENCE" "$f" | while read -r _; do printf '%s/%s\n' "$rns" "${r#*@}"; done ;; + esac + fi + done + for l in "$STATE"/*@*.latefence; do + [ -e "$l" ] || continue + echo "$FENCE" >> "${l%.latefence}.fin" + rm -f "$l" + done + if [ "$partial" = yes ]; then echo "fake: an error occurred partway through the listing" >&2; exit 1; fi + touch "$STATE/listed" + exit 0 +fi +if [ "$verb" = get ]; then + if [ -e "$rel.vanish" ]; then + rm -f "$rel.vanish" "$rel.fin" + fi + if [ ! -e "$rel.fin" ]; then + [ "$ignore" = yes ] && exit 0 + echo "Error from server (NotFound): helmreleases.helm.toolkit.fluxcd.io \"$name\" not found" >&2 + exit 1 + fi + case "$tmpl" in + *managedFields*) + if take_fail "$rel.ownedread"; then + echo "Error executing template: an error blob on stdout" + exit 1 + fi + case "$tmpl" in *'.metadata.resourceVersion}} {{.metadata.uid}}'*) printf '%s uid-%s ' "$(cat "$rel.rv")" "$name" ;; esac + case "$tmpl" in *'{{if .spec.suspend}}'*) [ -e "$rel.suspended" ] || exit 0 ;; esac + case "$tmpl" in *'eq .manager "flux-plunger"'*) ;; *) exit 0 ;; esac + [ -e "$rel.owned" ] && cat "$rel.owned" + case "$tmpl" in *'(eq .operation "Apply")'*) ;; *) [ -e "$rel.updateowned" ] && echo yes ;; esac + case "$tmpl" in *'"f:suspend"'*) ;; *) [ -e "$rel.applyonly" ] && echo yes ;; esac + # Another manager's entry also owning spec.suspend. + case "$tmpl" in *'ne .manager "flux-plunger"'*) [ -e "$rel.coowned" ] && echo other ;; esac + exit 0 ;; + *deletionTimestamp*) + case "$tmpl" in *"$UNHANDLED"*) ;; *) exit 0 ;; esac + [ -e "$rel.deleting" ] || exit 0 + grep -q -x "$HELMFIN" "$rel.fin" && exit 0 + [ "$(cat "$rel.gen")" = "$(cat "$rel.obs")" ] && exit 0 + printf 'unhandled' + exit 0 ;; + *'range $i, $f'*) + case "$tmpl" in + *"eq \$f \"$FENCE\""*) grep -n -x "$FENCE" "$rel.fin" | cut -d: -f1 | awk '{ printf "%d ", $1 - 1 }' ;; + esac + exit 0 ;; + esac +fi +if [ "$verb" = patch ]; then + [ -e "$rel.fin" ] || { echo "fake: $ns/$name not found" >&2; exit 1; } + case "$type" in + json) + case "$payload" in + *'"path":"/spec/suspend"'*) + if take_fail "$rel.apply"; then echo "fake: the unsuspend was rejected" >&2; exit 1; fi + if take_fail "$rel.applyconflict"; then bump_rv "$rel"; fi + if [ "$fm" != flux-plunger ]; then echo "fake: unsuspend not written as flux-plunger" >&2; exit 64; fi + want="[{\"op\":\"test\",\"path\":\"/metadata/resourceVersion\",\"value\":\"$(cat "$rel.rv")\"},{\"op\":\"test\",\"path\":\"/metadata/uid\",\"value\":\"uid-$name\"},{\"op\":\"replace\",\"path\":\"/spec/suspend\",\"value\":false}]" + if [ "$payload" != "$want" ]; then echo "fake: testing value /metadata/resourceVersion failed or not a preconditioned unsuspend: $payload" >&2; exit 1; fi + if [ -e "$rel.coowned" ]; then echo "fake: cleared a suspension another manager also holds" >&2; exit 64; fi + rm -f "$rel.owned" "$rel.applyonly" "$rel.suspended" + echo $(( $(cat "$rel.gen") + 1 )) > "$rel.gen" + echo "$ns/$name" >> "$STATE/unsuspended" + bump_rv "$rel" + exit 0 ;; + esac + if take_fail "$rel.patch"; then echo "fake: the patch was rejected" >&2; exit 1; fi + i=$(printf '%s' "$payload" | sed -n 's#^\[{"op":"test","path":"/metadata/finalizers/\([0-9][0-9]*\)","value":"flux-plunger\.cozystack\.io/uninstall-fence"},{"op":"remove","path":"/metadata/finalizers/\1"}\]$#\1#p') + [ -n "$i" ] || { echo "fake: not a guarded fence removal: $payload" >&2; exit 1; } + [ "$(sed -n "$((i + 1))p" "$rel.fin")" = "$FENCE" ] || { echo "fake: testing value /metadata/finalizers/$i failed" >&2; exit 1; } + sed -i.bak "$((i + 1))d" "$rel.fin" + [ -e "$rel.readd" ] && echo "$FENCE" >> "$rel.fin" + bump_rv "$rel" + now=$(cat "$CLOCK") + echo $((now + 1)) > "$CLOCK" + exit 0 ;; + esac + echo "fake: a $type patch; spec.suspend is lifted only by the guarded JSON patch" >&2 + exit 64 +fi +echo "fake kubectl: unexpected call" >&2 +exit 64 +KEOF + cat > "$1/sleep" <<'SEOF' +#!/bin/sh +now=$(cat "$CLOCK") +echo $((now + $1)) > "$CLOCK" +SEOF + cat > "$1/date" <<'DEOF' +#!/bin/sh +cat "$CLOCK" +DEOF + chmod 0755 "$1/kubectl" "$1/sleep" "$1/date" +} + +# prepare : fakes, script, empty state, clock at zero. +prepare() { + write_fakes "$1" + render_hook "$1/hook.sh" || return 1 + mkdir "$1/state" + export KLOG="$1/calls" CLOCK="$1/clock" STATE="$1/state" NAMESPACE=cozy-fluxcd + echo 0 > "$CLOCK" + : > "$KLOG" +} + +# run_hook : run the script, its output in /out, its status in $status. +run_hook() { + status=0 + PATH="$1:$PATH" sh "$1/hook.sh" > "$1/out" 2>&1 || status=$? + cat "$1/out" + echo "status=$status elapsed=$(cat "$CLOCK")s" +} + +@test "the hook waits for flux-plunger's own drain, fits under helm's bound, and stops the Deployment the chart deploys" { + tmp=$(mktemp -d) + prepare "$tmp" + echo "deadline=${DEADLINE}s grace=${GRACE}s pod-wait=${POD_WAIT}s window=${WINDOW}s deploy=$SCRIPT_DEPLOY/$DEPLOY_NAME/$ROLE_DEPLOY selector=$POD_SELECTOR labels=$POD_LABELS" + # helm-controller runs the pre-delete hook under the release's uninstall + # timeout, 300s by default, and the operator sets none; that clock starts + # when the hook is created, so the Job's own deadline has to end first. + [ "$DEADLINE" -lt 300 ] + # Retries stop a minute before the Job is killed, the pod wait covers the + # pod's grace period, and fits inside the retry window. + [ $((DEADLINE - WINDOW)) -ge 60 ] + [ "$POD_WAIT" -gt "$GRACE" ] + [ "$POD_WAIT" -lt "$WINDOW" ] + # The wait must find flux-plunger's pod: kubectl wait --for=delete succeeds at + # once on a selector that matches nothing. + [ "$POD_SELECTOR" = "$POD_LABELS" ] + # A name the chart does not deploy reads as a Deployment already gone. + [ "$SCRIPT_DEPLOY" = "$DEPLOY_NAME" ] + [ "$ROLE_DEPLOY" = "$DEPLOY_NAME" ] + rm -rf "$tmp" +} + +@test "at the largest grace period the chart accepts the hook still fits under helm's bound" { + tmp=$(mktemp -d) + render_hook "$tmp/hook.sh" --set shutdownGracePeriodSeconds=140 + echo "deadline=$DEADLINE pod-wait=$POD_WAIT window=$WINDOW" + [ "$DEADLINE" -lt 300 ] + [ $((DEADLINE - WINDOW)) -ge 60 ] + [ "$POD_WAIT" -gt 140 ] + [ "$POD_WAIT" -lt "$WINDOW" ] + rm -rf "$tmp" +} + +@test "the hook lifts only a suspension flux-plunger alone holds and removes only the fence, at the index it holds" { + tmp=$(mktemp -d) + prepare "$tmp" + add_release "$STATE" tenant-a app-a example.com/keep "$FENCE" "$HELMFIN" + echo yes > "$STATE/tenant-a@app-a.owned" + add_release "$STATE" tenant-b app-b "$FENCE" + # Two managedFields entries would print the ownership mark twice. + add_release "$STATE" tenant-c app-c example.com/one example.com/two "$FENCE" + echo yesyes > "$STATE/tenant-c@app-c.owned" + # Someone else removes this fence between the listing and the hook's next read. + add_release "$STATE" tenant-d app-d "$FENCE" example.com/keep + touch "$STATE/tenant-d@app-d.vanish" + # flux-plunger applied to this release, but its entry does not own spec.suspend. + add_release "$STATE" tenant-e app-e "$FENCE" + touch "$STATE/tenant-e@app-e.applyonly" + # A successful read that prints something other than the ownership mark. + add_release "$STATE" tenant-f app-f "$FENCE" + echo unrelated > "$STATE/tenant-f@app-f.owned" + # The apiserver accepts a finalizer listed twice; both copies go, and the + # release is counted once. + add_release "$STATE" tenant-g app-g "$FENCE" example.com/keep "$FENCE" + # Another manager also holds spec.suspend at true: its suspension stands, as + # the controller's relinquish leaves it. + add_release "$STATE" tenant-h app-h "$FENCE" + echo yes > "$STATE/tenant-h@app-h.owned" + touch "$STATE/tenant-h@app-h.coowned" + # flux-plunger owns spec.suspend at false, as after its own unsuspend: there is + # nothing to lift, and no write. + add_release "$STATE" tenant-i app-i "$FENCE" + echo yes > "$STATE/tenant-i@app-i.owned" + rm "$STATE/tenant-i@app-i.suspended" + # A suspension an earlier flux-plunger left through a merge patch is not one + # the controller takes as its own, so the hook leaves it too. + add_release "$STATE" tenant-j app-j "$FENCE" + touch "$STATE/tenant-j@app-j.updateowned" + + run_hook "$tmp" + + echo "unsuspended=$(tr '\n' ' ' < "$STATE/unsuspended" 2>/dev/null)" + [ "$status" -eq 0 ] + [ "$(fences_left "$STATE")" -eq 0 ] + [ "$(sort "$STATE/unsuspended" | tr '\n' ' ')" = "tenant-a/app-a tenant-c/app-c " ] + [ -e "$STATE/tenant-h@app-h.suspended" ] + [ -e "$STATE/tenant-j@app-j.suspended" ] + [ "$(tr '\n' ' ' < "$STATE/tenant-a@app-a.fin")" = "example.com/keep $HELMFIN " ] + [ "$(tr '\n' ' ' < "$STATE/tenant-c@app-c.fin")" = "example.com/one example.com/two " ] + [ "$(tr '\n' ' ' < "$STATE/tenant-g@app-g.fin")" = "example.com/keep " ] + grep -q -x 'released 10 of 10 fenced HelmReleases, 0 without an uninstall' "$tmp/out" + # Nothing the fake refused, and the pod wait asked the question it answers. + if grep -q -e '^fake:' -e 'the wait for flux-plunger pods to go failed' "$tmp/out"; then + echo "FAIL: the hook made a call the fake refused" + false + fi + rm -rf "$tmp" +} + +@test "a release deleted before helm-controller handled it is released, and named as deleted without an uninstall" { + tmp=$(mktemp -d) + prepare "$tmp" + # helm-controller dropped its finalizer on the suspended delete; the fence is + # all that holds it, and flux-plunger's drain did not get it through. Lifting + # the suspension bumps the generation, which helm-controller has not observed. + add_release "$STATE" tenant-a app-a "$FENCE" + touch "$STATE/tenant-a@app-a.deleting" + echo yes > "$STATE/tenant-a@app-a.owned" + # Being deleted, but helm-controller's finalizer still holds it: helm-controller + # will run the uninstall once the fence is gone. + add_release "$STATE" tenant-b app-b "$HELMFIN" "$FENCE" + touch "$STATE/tenant-b@app-b.deleting" + echo yes > "$STATE/tenant-b@app-b.owned" + # Being deleted, helm-controller has handled its current generation. + add_release "$STATE" tenant-c app-c "$FENCE" + touch "$STATE/tenant-c@app-c.deleting" + # Its uninstall completes mid-sweep, and the release is gone when read. + add_release "$STATE" tenant-d app-d "$FENCE" + touch "$STATE/tenant-d@app-d.vanish" + + run_hook "$tmp" + + [ "$status" -eq 0 ] + [ "$(fences_left "$STATE")" -eq 0 ] + grep -q -x "released $FENCE from tenant-a/app-a without an uninstall: helm-controller had not handled it" "$tmp/out" + grep -q -x "released $FENCE from tenant-b/app-b" "$tmp/out" + grep -q -x "released $FENCE from tenant-c/app-c" "$tmp/out" + grep -q -x "released $FENCE from tenant-d/app-d" "$tmp/out" + grep -q -x 'released 4 of 4 fenced HelmReleases, 1 without an uninstall' "$tmp/out" + rm -rf "$tmp" +} + +@test "transient API failures are retried, and a failed attempt's output is not taken as an answer" { + tmp=$(mktemp -d) + prepare "$tmp" + echo 2 > "$STATE/deploy.fail" + echo 2 > "$STATE/scale.fail" + echo 2 > "$STATE/list.fail" + # Two listings that print what they have and then fail: their output must + # not be added to the one that succeeds. + echo 2 > "$STATE/list.partial.fail" + add_release "$STATE" tenant-a app-a "$FENCE" + echo 2 > "$STATE/tenant-a@app-a.patch.fail" + # Not flux-plunger's suspension; a failed ownership read prints an error blob + # to stdout first. + add_release "$STATE" tenant-b app-b "$FENCE" + echo 2 > "$STATE/tenant-b@app-b.ownedread.fail" + add_release "$STATE" tenant-d app-d "$FENCE" + echo yes > "$STATE/tenant-d@app-d.owned" + echo 2 > "$STATE/tenant-d@app-d.apply.fail" + # A write lands between the ownership read and the unsuspend: the patch is + # rejected on its resourceVersion, and the decision is read again. + add_release "$STATE" tenant-e app-e "$FENCE" + echo yes > "$STATE/tenant-e@app-e.owned" + echo 1 > "$STATE/tenant-e@app-e.applyconflict.fail" + + run_hook "$tmp" + + [ "$status" -eq 0 ] + [ "$(fences_left "$STATE")" -eq 0 ] + [ "$(sort "$STATE/unsuspended" | tr '\n' ' ')" = "tenant-d/app-d tenant-e/app-e " ] + [ "$(grep --count "released $FENCE from" "$tmp/out")" -eq 4 ] + grep -q -x 'released 4 of 4 fenced HelmReleases, 0 without an uninstall' "$tmp/out" + rm -rf "$tmp" +} + +@test "a release that cannot be read, unsuspended or unfenced fails the hook, keeps its fence and is named" { + tmp=$(mktemp -d) + prepare "$tmp" + add_release "$STATE" tenant-a app-a "$FENCE" + echo 9999 > "$STATE/tenant-a@app-a.patch.fail" + add_release "$STATE" tenant-b app-b "$FENCE" + echo 9999 > "$STATE/tenant-b@app-b.ownedread.fail" + # Its suspension is flux-plunger's and cannot be lifted: without the fence a + # deleted release that is still suspended is removed without an uninstall. + add_release "$STATE" tenant-c app-c "$FENCE" + echo yes > "$STATE/tenant-c@app-c.owned" + echo 9999 > "$STATE/tenant-c@app-c.apply.fail" + add_release "$STATE" tenant-d app-d "$FENCE" + + run_hook "$tmp" + + [ "$status" -ne 0 ] + grep -q '^NOT released tenant-a/app-a' "$tmp/out" + grep -q '^NOT released tenant-b/app-b' "$tmp/out" + grep -q '^NOT released tenant-c/app-c' "$tmp/out" + if grep -q "released $FENCE from tenant-[abc]/" "$tmp/out"; then + echo "FAIL: a release left fenced was reported released" + false + fi + grep -q -x 'released 1 of 4 fenced HelmReleases, 0 without an uninstall' "$tmp/out" + [ "$(fences_left "$STATE")" -eq 3 ] + [ -e "$STATE/tenant-c@app-c.owned" ] + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" +} + +@test "a fence that keeps coming back fails the hook inside its deadline" { + tmp=$(mktemp -d) + prepare "$tmp" + add_release "$STATE" tenant-a app-a "$FENCE" + touch "$STATE/tenant-a@app-a.readd" + + run_hook "$tmp" + + [ "$status" -ne 0 ] + grep -q '^NOT released tenant-a/app-a' "$tmp/out" + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" +} + +@test "a listing that never succeeds fails the hook instead of reading as no fences" { + tmp=$(mktemp -d) + prepare "$tmp" + echo 9999 > "$STATE/list.fail" + add_release "$STATE" tenant-a app-a "$FENCE" + + run_hook "$tmp" + + [ "$status" -ne 0 ] + grep -q 'could not list' "$tmp/out" + [ "$(fences_left "$STATE")" -eq 1 ] + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" +} + +@test "a Deployment that cannot be read or scaled down fails the hook before any fence is touched" { + for knob in deploy scale; do + tmp=$(mktemp -d) + prepare "$tmp" + # flux-plunger may still be running and would fence behind a sweep, and a + # sweep that ran anyway would leave the retried uninstall nothing to + # converge on. + echo 9999 > "$STATE/$knob.fail" + add_release "$STATE" tenant-a app-a "$FENCE" + echo yes > "$STATE/tenant-a@app-a.owned" + + run_hook "$tmp" + + [ "$status" -ne 0 ] + grep -q 'no fence was touched' "$tmp/out" + [ "$(fences_left "$STATE")" -eq 1 ] + [ -e "$STATE/tenant-a@app-a.owned" ] + if grep -q 'helmreleases' "$KLOG"; then + echo "FAIL: the sweep ran with flux-plunger possibly still running" + false + fi + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" + done +} + +@test "a flux-plunger Deployment that is already gone has nothing to stop, and says so" { + tmp=$(mktemp -d) + prepare "$tmp" + touch "$STATE/deploy.absent" + echo 9999 > "$STATE/scale.fail" + add_release "$STATE" tenant-a app-a "$FENCE" + + run_hook "$tmp" + + [ "$status" -eq 0 ] + grep -q 'no flux-plunger Deployment in cozy-fluxcd; nothing to stop' "$tmp/out" + [ "$(fences_left "$STATE")" -eq 0 ] + if grep -q ' scale ' "$KLOG"; then + echo "FAIL: scaled a Deployment that is not there" + false + fi + rm -rf "$tmp" +} + +@test "a flux-plunger pod that does not go is named, and the sweep goes on" { + tmp=$(mktemp -d) + prepare "$tmp" + # A pod stuck Terminating on a NotReady node stays until the node returns or + # someone force-deletes it; failing on it would fail every retried uninstall. + touch "$STATE/pods.stuck" + add_release "$STATE" tenant-a app-a "$FENCE" + echo 2 > "$STATE/tenant-a@app-a.patch.fail" + + run_hook "$tmp" + + [ "$status" -eq 0 ] + grep -q 'pods are still running, sweeping anyway: pod/flux-plunger-0' "$tmp/out" + [ "$(fences_left "$STATE")" -eq 0 ] + grep -q -x 'released 1 of 1 fenced HelmReleases, 0 without an uninstall' "$tmp/out" + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" +} + +@test "a release the still-running flux-plunger fences after the listing fails the hook and is named" { + tmp=$(mktemp -d) + prepare "$tmp" + # The stuck pod's container is alive: it never got SIGTERM, still holds the + # Lease, and fences one more release once the listing has returned. With the + # chart gone nothing would ever remove that fence. + touch "$STATE/pods.stuck" + add_release "$STATE" tenant-a app-a "$FENCE" + add_release "$STATE" tenant-z app-z example.com/keep + touch "$STATE/tenant-z@app-z.latefence" + + run_hook "$tmp" + + [ "$status" -ne 0 ] + grep -q '^NOT released tenant-z/app-z: fenced after the sweep' "$tmp/out" + [ "$(fences_left "$STATE")" -eq 1 ] + rm -rf "$tmp" +} + +@test "a pod wait that fails for another reason is told apart from a pod that stays" { + tmp=$(mktemp -d) + prepare "$tmp" + echo 1 > "$STATE/wait.fail" + add_release "$STATE" tenant-a app-a "$FENCE" + + run_hook "$tmp" + + [ "$status" -eq 0 ] + grep -q 'the wait for flux-plunger pods to go failed, but none is left' "$tmp/out" + [ "$(fences_left "$STATE")" -eq 0 ] + # With the pods gone, nothing can have fenced behind the sweep. + [ "$(grep --count 'helmreleases.helm.toolkit.fluxcd.io --all-namespaces' "$KLOG")" -eq 1 ] + rm -rf "$tmp" +} + +@test "a pod wait after a slow scale-down is cut to what is left of the retry window" { + tmp=$(mktemp -d) + prepare "$tmp" + # The scale-down is retried for most of the window, and the pod never goes: + # the wait must not run past the window, or the Job is killed before the + # sweep and its report. + echo 80 > "$STATE/scale.fail" + touch "$STATE/pods.stuck" + add_release "$STATE" tenant-a app-a "$FENCE" + + run_hook "$tmp" + + [ "$status" -eq 0 ] + grep -q -x 'released 1 of 1 fenced HelmReleases, 0 without an uninstall' "$tmp/out" + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" +} + +@test "a pod listing that cannot be read after a failed wait is treated as a pod still running" { + tmp=$(mktemp -d) + prepare "$tmp" + echo 1 > "$STATE/wait.fail" + echo 9999 > "$STATE/pods.fail" + add_release "$STATE" tenant-a app-a "$FENCE" + add_release "$STATE" tenant-z app-z example.com/keep + touch "$STATE/tenant-z@app-z.latefence" + + run_hook "$tmp" + + [ "$status" -ne 0 ] + grep -q 'could not tell whether flux-plunger pods are still running; sweeping anyway' "$tmp/out" + grep -q '^NOT released tenant-z/app-z: fenced after the sweep' "$tmp/out" + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" +} + +@test "a listing after the sweep that never succeeds fails the hook" { + tmp=$(mktemp -d) + prepare "$tmp" + # The pod is still there, so whether it fenced anything behind the sweep is + # unknown until a listing answers. + touch "$STATE/pods.stuck" + echo 9999 > "$STATE/relist.fail" + add_release "$STATE" tenant-a app-a "$FENCE" + + run_hook "$tmp" + + [ "$status" -ne 0 ] + grep -q 'could not list fenced HelmReleases again after the sweep' "$tmp/out" + [ "$(fences_left "$STATE")" -eq 0 ] + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" +} + +@test "a scale-down that returns after the retry window still bounds the pod wait" { + tmp=$(mktemp -d) + prepare "$tmp" + # What is left of the window is then negative, which kubectl wait reads as a + # week: the Job would be killed before the sweep and its report. + echo $((WINDOW + 5)) > "$STATE/scale.slow" + touch "$STATE/pods.stuck" + add_release "$STATE" tenant-a app-a "$FENCE" + + run_hook "$tmp" + + [ "$status" -eq 0 ] + grep -q -x 'released 1 of 1 fenced HelmReleases, 0 without an uninstall' "$tmp/out" + [ "$(cat "$CLOCK")" -lt "$DEADLINE" ] + rm -rf "$tmp" +} diff --git a/hack/unit-test-parallelism.bats b/hack/unit-test-parallelism.bats index c7cdf0d52..532dbf4f2 100644 --- a/hack/unit-test-parallelism.bats +++ b/hack/unit-test-parallelism.bats @@ -25,7 +25,7 @@ load test_helper @test "PR workflow schedules unit and controller targets with four Make jobs and -k" { # -k is what lets test-controllers start when a unit-tests prerequisite # fails first; without it a red Bats file hides the Go controller suite. - grep -qF 'run: make unit-tests test-controllers -j4 -k --output-sync=target' \ + grep -qF 'run: make unit-tests test-controllers test-controllers-envtest -j4 -k --output-sync=target' \ .github/workflows/pull-requests.yaml || { echo "PR checks do not use the bounded four-slot make invocation" >&2 exit 1 diff --git a/internal/controller/fluxplunger/drain.go b/internal/controller/fluxplunger/drain.go new file mode 100644 index 000000000..53eb93e5f --- /dev/null +++ b/internal/controller/fluxplunger/drain.go @@ -0,0 +1,239 @@ +// SPDX-License-Identifier: Apache-2.0 + +package fluxplunger + +import ( + "context" + "errors" + "fmt" + "time" + + helmv2 "github.com/fluxcd/helm-controller/api/v2" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/log" +) + +const ( + // drainPollInterval is how often a drain re-reads the releases it still holds. + drainPollInterval = time.Second + // ShutdownDrainTimeoutFlag names the flag that bounds the drain; the chart + // passes it, derived from the pod's termination grace period. + ShutdownDrainTimeoutFlag = "shutdown-drain-timeout" +) + +// DrainFences gives up every fence flux-plunger holds, for a flux-plunger that is +// stopping. A live release has its recovery aborted, the suspension and the fence +// lifted together, which leaves it as recovery found it for the next flux-plunger +// to pick up. A release being deleted goes through reconcileTerminating until the +// fence is gone: flux-plunger's suspension is lifted, the fence is kept until +// helm-controller has handled the unsuspended generation, and only then lifted. +// When ctx ends first, the fences still held are left in place and counted: the +// next flux-plunger carries on with them, and when flux-plunger is being removed +// the chart's pre-delete hook releases them. Reads go to the apiserver, since the +// informer cache has stopped with the manager by now. +func (r *FluxPlunger) DrainFences(ctx context.Context) (int, error) { + logger := log.FromContext(ctx) + var reader client.Reader = r.Client + if r.APIReader != nil { + reader = r.APIReader + } + // held is what the last listing that succeeded found; -1 until one has. + held := -1 + for { + list := &helmv2.HelmReleaseList{} + if err := reader.List(ctx, list); err != nil { + // A failed listing is retried like any other step: on the uninstall + // path ending here would leave the pre-delete hook to let go of a + // release the drain could still have carried through its uninstall. + logger.Info("Could not list HelmReleases yet, retrying", "error", err.Error()) + select { + case <-ctx.Done(): + return held, ctx.Err() + case <-time.After(drainPollInterval): + } + continue + } + held = 0 + for i := range list.Items { + hr := &list.Items[i] + if !controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + continue + } + held++ + var err error + if hr.DeletionTimestamp.IsZero() { + // The relinquishing apply drops every field flux-plunger's manager owns, + // so a suspension of its own goes with the fence, and one it does not own + // is left alone. + err = r.releaseFence(ctx, hr) + } else { + _, err = r.reconcileTerminating(ctx, hr) + } + if err != nil { + logger.Info("Could not give up a fence yet, retrying", "helmrelease", client.ObjectKeyFromObject(hr).String(), "error", err.Error()) + } + } + if held == 0 { + return 0, nil + } + select { + case <-ctx.Done(): + return held, ctx.Err() + case <-time.After(drainPollInterval): + } + } +} + +const ( + // managerStopTimeout bounds how long the manager may take to stop its + // controllers before the drain starts. + managerStopTimeout = 10 * time.Second + // killMargin is left between the end of the drain and the kubelet's SIGKILL. + // The chart's --shutdown-drain-timeout is the grace period less this and + // managerStopTimeout, which TestDeploymentTemplate_SubtractsTheStopAndTheMarginTheBinaryUses + // holds. + killMargin = 15 * time.Second + // leaseRetryPeriod is how often the leader renews its Lease. + leaseRetryPeriod = 5 * time.Second + // leaseRenewDeadline is how long a leader whose renewals fail goes on acting + // before it gives up; it is part of what the Lease has to outlast. + leaseRenewDeadline = 15 * time.Second + // leaseMargin is added on top of everything a stop can take. + leaseMargin = 20 * time.Second +) + +// ApplyShutdownTimings sizes the leader Lease so the next flux-plunger cannot +// lead while this one drains. A rollout or an eviction starts the next pod while +// the old one is still terminating, whatever the Deployment strategy, so it is +// the Lease, not the rollout, that keeps a single writer. The Lease is not given +// up on stop, and it outlasts its last renewal, the renew deadline, the +// manager's bounded stop and the drain; the price is that a new pod waits for it +// to run out before it leads. +func ApplyShutdownTimings(opts *ctrl.Options, drain time.Duration) { + stop := managerStopTimeout + renew := leaseRenewDeadline + lease := leaseRetryPeriod + renew + stop + drain + leaseMargin + retry := leaseRetryPeriod + opts.GracefulShutdownTimeout = &stop + opts.LeaseDuration = &lease + opts.RenewDeadline = &renew + opts.RetryPeriod = &retry + opts.LeaderElectionReleaseOnCancel = false +} + +// drainingManager is the part of a ctrl.Manager RunManager uses. +type drainingManager interface { + Start(context.Context) error + GetClient() client.Client + GetAPIReader() client.Reader + Elected() <-chan struct{} +} + +// RunManager runs the manager, then drains flux-plunger's fences. The drain +// reads through the manager's API reader: once the manager has stopped, its +// informer cache is frozen and would show fences already gone. Only a +// flux-plunger that led drains: one that never did stops while the leader goes +// on, and the fences are the leader's. Without leader election Elected is closed +// from the start. +func RunManager(ctx context.Context, mgr drainingManager, drain time.Duration) error { + drainer := &FluxPlunger{ + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Recorder: apiEventRecorder{c: mgr.GetClient()}, + } + return RunThenDrain(ctx, mgr.Start, func(dctx context.Context) (int, error) { + select { + case <-mgr.Elected(): + return drainer.DrainFences(dctx) + default: + log.FromContext(dctx).Info("Never became leader; leaving the fences to the leader") + return 0, nil + } + }, drain) +} + +// RunThenDrain runs the manager until it stops, then drains under a deadline of +// its own: the context run was given is cancelled by the stop signal, so the drain +// cannot be bounded by it. A manager that failed is not drained: a lost Lease +// means another flux-plunger leads now. A stop that overran the manager's grace +// period is drained anyway, since a reconcile slow to notice its cancelled +// context must not cost the fences their drain, and is still reported. A drain +// that leaves fences behind is logged rather than turned into a failed exit, +// since the next flux-plunger or the pre-delete hook takes them over. +func RunThenDrain(ctx context.Context, run func(context.Context) error, drain func(context.Context) (int, error), timeout time.Duration) error { + runErr := run(ctx) + if runErr != nil && !errors.Is(runErr, context.DeadlineExceeded) { + return runErr + } + dctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + logger := log.FromContext(ctx) + if left, err := drain(dctx); err != nil { + logger.Info("Stopped with uninstall fences still held; the next flux-plunger or the chart's pre-delete hook releases them", "held", left, "error", err.Error()) + } else { + logger.Info("Released every uninstall fence before stopping") + } + return runErr +} + +// apiEventRecorder writes each Event straight to the apiserver. The drain cannot +// use the manager's recorder: the manager stops its recorder provider before +// Start returns, after which that recorder delivers nothing. +type apiEventRecorder struct { + c client.Client +} + +func (r apiEventRecorder) Event(obj runtime.Object, eventtype, reason, message string) { + r.write(obj, eventtype, reason, message) +} + +func (r apiEventRecorder) Eventf(obj runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) { + r.write(obj, eventtype, reason, fmt.Sprintf(messageFmt, args...)) +} + +func (r apiEventRecorder) AnnotatedEventf(obj runtime.Object, _ map[string]string, eventtype, reason, messageFmt string, args ...interface{}) { + r.write(obj, eventtype, reason, fmt.Sprintf(messageFmt, args...)) +} + +func (r apiEventRecorder) write(obj runtime.Object, eventtype, reason, message string) { + logger := log.Log.WithName("drain-events") + o, ok := obj.(client.Object) + if !ok { + logger.Info("Cannot record an event for this object", "type", fmt.Sprintf("%T", obj), "reason", reason) + return + } + gvk, err := apiutil.GVKForObject(o, r.c.Scheme()) + if err != nil { + logger.Info("Cannot record an event", "reason", reason, "error", err.Error()) + return + } + now := metav1.Now() + ev := &corev1.Event{ + ObjectMeta: metav1.ObjectMeta{GenerateName: o.GetName() + ".", Namespace: o.GetNamespace()}, + InvolvedObject: corev1.ObjectReference{ + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, + Namespace: o.GetNamespace(), + Name: o.GetName(), + UID: o.GetUID(), + }, + Reason: reason, + Message: message, + Type: eventtype, + Source: corev1.EventSource{Component: suspendFieldManager}, + FirstTimestamp: now, + LastTimestamp: now, + Count: 1, + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := r.c.Create(ctx, ev); err != nil { + logger.Info("Could not record an event", "reason", reason, "error", err.Error()) + } +} diff --git a/internal/controller/fluxplunger/drain_envtest_test.go b/internal/controller/fluxplunger/drain_envtest_test.go new file mode 100644 index 000000000..7a0f6b5d1 --- /dev/null +++ b/internal/controller/fluxplunger/drain_envtest_test.go @@ -0,0 +1,323 @@ +// SPDX-License-Identifier: Apache-2.0 +//go:build envtest + +package fluxplunger + +import ( + "context" + "os" + "testing" + "time" + + helmv2 "github.com/fluxcd/helm-controller/api/v2" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" +) + +// fenceAndSuspend gives a release flux-plunger's recovery state: its own +// suspension and the fence, applied together as a recovery does. +func fenceAndSuspend(t *testing.T, ctx context.Context, cl client.Client, key types.NamespacedName) { + t.Helper() + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + r := &FluxPlunger{Client: cl, APIReader: cl} + if acquired, err := r.suspendHelmRelease(ctx, hr); err != nil || !acquired { + t.Fatalf("acquire: acquired=%v err=%v", acquired, err) + } +} + +func TestEnvtest_DrainAbortsTheRecoveryOfALiveRelease(t *testing.T) { + ctx, cl, newRelease, stop := startFenceEnv(t) + defer stop() + key := newRelease(testName + "-drain-live") + fenceAndSuspend(t, ctx, cl, key) + + r := &FluxPlunger{Client: cl, APIReader: cl} + dctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + left, err := r.DrainFences(dctx) + if err != nil || left != 0 { + t.Fatalf("drain: left=%d err=%v", left, err) + } + + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + if hr.Spec.Suspend || ownsSuspensionManagedFields(hr) { + t.Fatalf("drain must lift flux-plunger's suspension: suspend=%v owned=%v", hr.Spec.Suspend, ownsSuspensionManagedFields(hr)) + } + if controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + t.Fatalf("drain must lift the fence with the suspension, finalizers %v", hr.Finalizers) + } + if !controllerutil.ContainsFinalizer(hr, helmv2.HelmReleaseFinalizer) { + t.Fatalf("drain must leave helm-controller's finalizer, finalizers %v", hr.Finalizers) + } +} + +// A live release left with the fence alone (a recovery interrupted between its +// steps) is released at once, not held until the drain's deadline. +func TestEnvtest_DrainReleasesALiveFenceWithoutASuspensionAtOnce(t *testing.T) { + ctx, cl, newRelease, stop := startFenceEnv(t) + defer stop() + key := newRelease(testName + "-drain-fence-only") + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + r := &FluxPlunger{Client: cl, APIReader: cl} + if err := r.applyRecoveryState(ctx, hr, nil, true, false); err != nil { + t.Fatalf("fence: %v", err) + } + + dctx, cancel := context.WithTimeout(ctx, 20*time.Second) + defer cancel() + start := time.Now() + if left, err := r.DrainFences(dctx); err != nil || left != 0 { + t.Fatalf("drain: left=%d err=%v", left, err) + } + if took := time.Since(start); took > 5*time.Second { + t.Fatalf("a live fence must go at once, the drain took %v", took) + } + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + if controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + t.Fatalf("the fence must go, finalizers %v", hr.Finalizers) + } +} + +func TestEnvtest_DrainLeavesAnotherManagersSuspensionAlone(t *testing.T) { + ctx, cl, newRelease, stop := startFenceEnv(t) + defer stop() + key := newRelease(testName + "-drain-foreign") + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + r := &FluxPlunger{Client: cl, APIReader: cl} + if err := r.applyRecoveryState(ctx, hr, nil, true, false); err != nil { + t.Fatalf("fence: %v", err) + } + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + hr.Spec.Suspend = true + if err := cl.Update(ctx, hr, client.FieldOwner(fmOper)); err != nil { + t.Fatalf("operator suspend: %v", err) + } + + dctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + if left, err := r.DrainFences(dctx); err != nil || left != 0 { + t.Fatalf("drain: left=%d err=%v", left, err) + } + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + if controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + t.Fatalf("the fence must go, finalizers %v", hr.Finalizers) + } + if !hr.Spec.Suspend { + t.Fatal("an operator's suspension must survive the drain") + } +} + +// A rollout or an eviction starts the next flux-plunger while the old one is +// still terminating, so two of them overlap whatever the Deployment strategy. The +// old one must still be the only writer while it drains: it keeps the leader +// Lease, which it does not give up, and the next one becomes leader only once +// the Lease has run out, after the drain. +func TestEnvtest_TheNextFluxPlungerLeadsOnlyAfterTheDrain(t *testing.T) { + crdDir := os.Getenv("HELMRELEASE_CRD_DIR") + if crdDir == "" { + t.Skip("set HELMRELEASE_CRD_DIR to the dir with the HelmRelease CRD") + } + env := newEnvtestEnvironment(crdDir) + cfg, err := env.Start() + if err != nil { + t.Fatalf("start envtest: %v", err) + } + defer func() { _ = env.Stop() }() + cl, err := client.New(cfg, client.Options{Scheme: clientgoscheme.Scheme}) + if err != nil { + t.Fatalf("client: %v", err) + } + if err := cl.Create(context.Background(), &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNS}}); err != nil { + t.Fatalf("create ns: %v", err) + } + + // Longer than the Lease retry period, so a Lease given up on stop would be + // taken by the next manager while the drain still runs. + const drainFor = 3 * leaseRetryPeriod + newManager := func() ctrl.Manager { + opts := ctrl.Options{ + Scheme: clientgoscheme.Scheme, + Metrics: metricsserver.Options{BindAddress: "0"}, + HealthProbeBindAddress: "0", + LeaderElection: true, + LeaderElectionID: "flux-plunger.cozystack.io", + LeaderElectionNamespace: testNS, + } + ApplyShutdownTimings(&opts, drainFor) + m, err := ctrl.NewManager(cfg, opts) + if err != nil { + t.Fatalf("manager: %v", err) + } + return m + } + + old := newManager() + oldCtx, stopOld := context.WithCancel(context.Background()) + var drainEnded time.Time + oldDone := make(chan error, 1) + go func() { + oldDone <- RunThenDrain(oldCtx, old.Start, func(ctx context.Context) (int, error) { + time.Sleep(drainFor) + drainEnded = time.Now() + return 0, nil + }, drainFor) + }() + select { + case <-old.Elected(): + case <-time.After(30 * time.Second): + t.Fatal("the first flux-plunger never became leader") + } + + next := newManager() + nextCtx, stopNext := context.WithCancel(context.Background()) + defer stopNext() + go func() { _ = next.Start(nextCtx) }() + + stopOld() + if err := <-oldDone; err != nil { + t.Fatalf("a clean stop must drain, not fail: %v", err) + } + if drainEnded.IsZero() { + t.Fatal("the stopping flux-plunger did not drain") + } + select { + case <-next.Elected(): + t.Fatal("the next flux-plunger became leader while the old one was still draining") + default: + } + + select { + case <-next.Elected(): + if !time.Now().After(drainEnded) { + t.Fatal("the next flux-plunger led before the drain ended") + } + case <-time.After(2 * time.Minute): + t.Fatal("the next flux-plunger never became leader once the Lease ran out") + } +} + +// A delete that helm-controller handled while flux-plunger held the suspension +// (no uninstall, its finalizer dropped) leaves the fence as the only thing +// keeping the release. Draining must lift the suspension and keep the fence +// until helm-controller has handled the unsuspended generation, so the uninstall +// runs before the release goes. +func TestEnvtest_DrainCarriesADeleteThroughTheUninstall(t *testing.T) { + ctx, cl, newRelease, stop := startFenceEnv(t) + defer stop() + key := newRelease(testName + "-drain-delete") + fenceAndSuspend(t, ctx, cl, key) + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + if err := cl.Delete(ctx, hr); err != nil { + t.Fatalf("delete: %v", err) + } + if simulateHelmControllerDelete(t, ctx, cl, key) { + t.Fatal("helm-controller must skip the uninstall of a suspended release") + } + + r := &FluxPlunger{Client: cl, APIReader: cl} + dctx, cancel := context.WithTimeout(ctx, 20*time.Second) + defer cancel() + type result struct { + left int + err error + } + done := make(chan result, 1) + go func() { + left, err := r.DrainFences(dctx) + done <- result{left, err} + }() + + // helm-controller reconciles again once the suspension is lifted. Until it + // has, the fence must keep the release, through any number of drain passes. + deadline := time.Now().Add(10 * time.Second) + for { + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("the fence must keep the release until helm-controller has handled it, got %v", err) + } + if !hr.Spec.Suspend { + break + } + if time.Now().After(deadline) { + t.Fatal("drain did not lift flux-plunger's suspension") + } + time.Sleep(100 * time.Millisecond) + } + time.Sleep(3 * drainPollInterval) + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("the fence must hold while helm-controller has not observed the unsuspended generation, got %v", err) + } + if !simulateHelmControllerDelete(t, ctx, cl, key) { + t.Fatal("with the suspension lifted, helm-controller must run the uninstall") + } + + res := <-done + if res.err != nil || res.left != 0 { + t.Fatalf("drain: left=%d err=%v", res.left, res.err) + } + if err := cl.Get(ctx, key, hr); !apierrors.IsNotFound(err) { + t.Fatalf("after the uninstall the drain must lift the fence so the release is deleted, got %v", err) + } +} + +// A helm-controller that never handles the unsuspended release must not make the +// drain drop the fence: the next flux-plunger, or the chart's pre-delete hook when +// flux-plunger is being removed, takes it from there. +func TestEnvtest_DrainKeepsTheFenceWhenHelmControllerNeverAnswers(t *testing.T) { + ctx, cl, newRelease, stop := startFenceEnv(t) + defer stop() + key := newRelease(testName + "-drain-stalled") + fenceAndSuspend(t, ctx, cl, key) + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + if err := cl.Delete(ctx, hr); err != nil { + t.Fatalf("delete: %v", err) + } + simulateHelmControllerDelete(t, ctx, cl, key) + + r := &FluxPlunger{Client: cl, APIReader: cl} + dctx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + left, err := r.DrainFences(dctx) + if err == nil || left != 1 { + t.Fatalf("a drain cut short must report the fence it left: left=%d err=%v", left, err) + } + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("the fence must keep the release, got %v", err) + } + if hr.Spec.Suspend { + t.Fatal("the drain must still have lifted flux-plunger's suspension") + } + if !controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + t.Fatalf("the fence must stay, finalizers %v", hr.Finalizers) + } +} diff --git a/internal/controller/fluxplunger/flux_plunger.go b/internal/controller/fluxplunger/flux_plunger.go index 44886a2cb..fc57b405c 100644 --- a/internal/controller/fluxplunger/flux_plunger.go +++ b/internal/controller/fluxplunger/flux_plunger.go @@ -2,17 +2,24 @@ package fluxplunger import ( "context" + "encoding/json" + "errors" "fmt" "sort" "strconv" "strings" + "time" helmv2 "github.com/fluxcd/helm-controller/api/v2" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -20,74 +27,197 @@ import ( const ( annotationLastProcessedVersion = "flux-plunger.cozystack.io/last-processed-version" errorMessageNoDeployedReleases = "has no deployed releases" - fieldManager = "flux-client-side-apply" + // fieldManager owns the client-side patch of the processed-version annotation. + fieldManager = "flux-client-side-apply" + // suspendFieldManager is the server-side-apply field manager flux-plunger uses + // to own spec.suspend. Ownership of a suspension is decided by whether this + // manager owns the field in metadata.managedFields, not by an annotation: the + // apiserver transfers the field away the moment another writer changes + // spec.suspend (a resume, then a re-suspend), so a later operator suspension is + // never misread as flux-plunger's, while an unrelated re-apply of other fields + // leaves the ownership untouched (no self-disown). A write of the value the field + // already holds does not transfer it. + suspendFieldManager = "flux-plunger" + // fenceFinalizer holds a HelmRelease while flux-plunger's recovery has it + // suspended. On a delete, helm-controller v1.5.x drops its own finalizer without + // running the Helm uninstall if the release is suspended, and it wakes on the same + // event flux-plunger does, so lifting the suspension first cannot be guaranteed. + // This finalizer keeps the object until flux-plunger has lifted the suspension + // and helm-controller has reconciled the unsuspended release, which runs the + // uninstall. It is applied in the same server-side apply as the suspension and + // removed with it once a recovery completes; on a delete it outlives the + // suspension and is removed only once helm-controller has handled the + // unsuspended release (uninstallReached). + fenceFinalizer = "flux-plunger.cozystack.io/uninstall-fence" + + // fenceRecheckInterval is how often a delete held by the fence is re-checked + // without waiting for an event, so a stalled helm-controller is noticed. + fenceRecheckInterval = time.Minute + // fenceHeldWarnAfter is how long a delete may be held by the fence, waiting for + // helm-controller to handle the unsuspended release, before it is raised as a + // Warning: an uninstall normally lands within seconds. + fenceHeldWarnAfter = 10 * time.Minute ) // FluxPlunger watches HelmRelease resources and fixes "has no deployed releases" errors type FluxPlunger struct { client.Client + + // APIReader reads directly from the apiserver, bypassing the informer cache. + // The re-fetch guards below (already-suspended, terminating) must not read the + // cache: it lags a write or delete the apiserver has already accepted, which + // would defeat the pre-delete termination re-check whose whole purpose is to + // catch a just-arrived delete. Production wires mgr.GetAPIReader(); when nil + // (tests), refetch falls back to the cached client. + APIReader client.Reader + + // Recorder emits Events so an operator can see a release flux-plunger declined + // to act on (an externally-owned suspension) rather than reading it out of the + // log. Production wires mgr.GetEventRecorderFor; nil is tolerated (tests). + Recorder record.EventRecorder + + // owns overrides the suspension-ownership check. Production leaves it nil and + // uses ownsSuspensionManagedFields (the managedFields-based check, which the + // controller-runtime fake client cannot represent). Tests in this package + // inject a stand-in so the branch, ordering and requeue behaviour can be + // exercised on the fake client, while the managedFields check itself is pinned + // by a unit test on hand-crafted managedFields and by the envtest suite. + owns func(*helmv2.HelmRelease) bool + + // now overrides the clock for tests; production leaves it nil. + now func() time.Time +} + +func (r *FluxPlunger) clock() time.Time { + if r.now != nil { + return r.now() + } + return time.Now() +} + +// recordEvent emits an Event when a Recorder is wired. The message is static so +// the recorder's event correlator folds repeats into a single counted Event rather +// than a flood. +func (r *FluxPlunger) recordEvent(hr *helmv2.HelmRelease, eventType, reason, message string) { + if r.Recorder != nil { + r.Recorder.Event(hr, eventType, reason, message) + } +} + +// recordUnsuspendFailed raises a Warning when flux-plunger holds a suspension and +// could not clear it: the state in which a delete makes helm-controller skip the +// uninstall and orphan the release, which an operator can see in +// `kubectl describe helmrelease` and must be able to alert on. A conflict is not +// such a failure: the apply is preconditioned on what was read, so any write in +// between (helm-controller's status update included) rejects it, and the requeue +// retries it against the new state. Nor is a stop cancelling the reconcile: the +// drain that follows lifts the suspension. +func (r *FluxPlunger) recordUnsuspendFailed(hr *helmv2.HelmRelease, err error) { + if apierrors.IsConflict(err) || errors.Is(err, context.Canceled) { + return + } + r.recordEvent(hr, corev1.EventTypeWarning, "UnsuspendFailed", + "Could not clear a suspension flux-plunger set; retrying. While it stays suspended, deleting the release skips the Helm uninstall and orphans its resources") +} + +// uninstallReached reports whether helm-controller has handled a terminating +// release in its current, unsuspended form: its finalizer is gone and it has +// observed the latest generation, which lifting the suspension bumped. Checking the +// finalizer alone is not enough, because helm-controller drops it without an +// uninstall while the release is still suspended. +func uninstallReached(hr *helmv2.HelmRelease) bool { + return !controllerutil.ContainsFinalizer(hr, helmv2.HelmReleaseFinalizer) && + hr.Status.ObservedGeneration == hr.Generation +} + +// ownsSuspension reports whether flux-plunger owns the current suspension, through +// the injected check in tests or the managedFields check in production. +func (r *FluxPlunger) ownsSuspension(hr *helmv2.HelmRelease) bool { + if r.owns != nil { + return r.owns(hr) + } + return ownsSuspensionManagedFields(hr) +} + +// refetch reads the freshest state of a HelmRelease. It uses the uncached +// APIReader when wired (production) so an already-suspended or termination +// re-check sees a write or delete the apiserver has just accepted; it falls back +// to the cached client only when no APIReader is set (tests). +func (r *FluxPlunger) refetch(ctx context.Context, key types.NamespacedName, hr *helmv2.HelmRelease) error { + if r.APIReader != nil { + return r.APIReader.Get(ctx, key, hr) + } + return r.Get(ctx, key, hr) } // +kubebuilder:rbac:groups=helm.toolkit.fluxcd.io,resources=helmreleases,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;delete +// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // Reconcile handles HelmRelease resources with "has no deployed releases" error func (r *FluxPlunger) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) - // Get the HelmRelease + // Read the HelmRelease from the apiserver, not the informer cache: the + // already-processed guard below compares its processed-version annotation with a + // Secret list that is itself read from the apiserver, and a cache that has not yet + // seen the annotation written by the previous recovery would fail that guard and + // delete the next revision down. hr := &helmv2.HelmRelease{} - if err := r.Get(ctx, req.NamespacedName, hr); err != nil { + if err := r.refetch(ctx, req.NamespacedName, hr); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } + if !hr.DeletionTimestamp.IsZero() { + return r.reconcileTerminating(ctx, hr) + } + // Check if HelmRelease is suspended if hr.Spec.Suspend { - logger.Info("HelmRelease is suspended, checking if we need to unsuspend") + // Ownership is decided by which field manager owns spec.suspend (see + // ownsSuspension). A suspension we own but see in a fresh reconcile means + // recovery crashed mid-flight rather than another instance running now: only + // the holder of the leader Lease reconciles, a stopping instance keeps that + // Lease through its drain (ApplyShutdownTimings), and controller-runtime + // serialises reconciles per key within the one process, so there is no + // concurrent owner to race. Releasing it un-sticks the release and lets + // error-recovery start over. A suspension flux-plunger does not own belongs to + // an operator or another controller and must be left untouched. + if !r.ownsSuspension(hr) { + logger.Info("HelmRelease is suspended by external process, skipping") + // On a release flux-plunger recovered before, the suspension is most + // likely one an earlier flux-plunger left mid-recovery under the shared + // flux-client-side-apply manager, which no one will lift unless told. + if _, recovered := hr.Annotations[annotationLastProcessedVersion]; recovered { + r.recordEvent(hr, corev1.EventTypeWarning, "SuspensionNotOwned", + "Left a suspension flux-plunger does not own untouched on a release it has recovered before; if an earlier flux-plunger left it, resume the release (see the flux-plunger chart README)") + } else { + r.recordEvent(hr, corev1.EventTypeNormal, "SuspensionNotOwned", + "Left an externally-owned suspension untouched; flux-plunger only clears suspensions it set") + } + // Our fence only guards a suspension flux-plunger holds; with the + // suspension taken over, do not keep the release from being deleted. + return ctrl.Result{}, r.releaseFence(ctx, hr) + } - // Get the list of Helm release secrets - secrets, err := r.listHelmReleaseSecrets(ctx, hr.Namespace, hr.Name) - if err != nil { - logger.Error(err, "Failed to list Helm release secrets") + logger.Info("Removing suspension owned by flux-plunger") + if err := r.unsuspendHelmRelease(ctx, hr); err != nil { + // Requeue with backoff so a stuck owned suspension is not left until the + // next informer resync, symmetric with the deletion and recovery paths. + logger.Info("Could not unsuspend HelmRelease, requeuing", "error", err.Error()) + r.recordUnsuspendFailed(hr, err) return ctrl.Result{}, err } - - // If no secrets, treat latest version as 0 - latestVersion := 0 - if len(secrets) > 0 { - latestSecret := getLatestSecret(secrets) - latestVersion = extractVersionNumber(latestSecret.Name) - } else { - logger.Info("No Helm release secrets found while suspended, treating as version 0") - } - - // Check if version is previous to just processed (latestVersion+1 == processedVersion) - // This is the ONLY condition when we unsuspend - shouldUnsuspend := false - if hr.Annotations != nil { - if processedVersionStr, exists := hr.Annotations[annotationLastProcessedVersion]; exists { - processedVersion, err := strconv.Atoi(processedVersionStr) - if err == nil && latestVersion+1 == processedVersion { - shouldUnsuspend = true - } - } - } - - if shouldUnsuspend { - // Unsuspend the HelmRelease - logger.Info("Secret was already deleted in previous run, removing suspend", "latest", latestVersion, "processed", latestVersion+1) - if err := r.unsuspendHelmRelease(ctx, hr); err != nil { - logger.Info("Could not unsuspend HelmRelease, will retry on next reconcile", "error", err.Error()) - return ctrl.Result{}, nil - } - return ctrl.Result{}, nil - } - - // If not previous to processed, skip all actions - logger.Info("HelmRelease is suspended by external process, skipping", "latest", latestVersion) return ctrl.Result{}, nil } + // A fence without a suspension is left when another writer (an operator, + // `cozyhr resume`) set spec.suspend=false and so took the field from + // flux-plunger; it guards nothing any more. + if controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + return ctrl.Result{}, r.releaseFence(ctx, hr) + } + // Check if HelmRelease has the specific error if !hasNoDeployedReleasesError(hr) { logger.V(1).Info("HelmRelease does not have 'has no deployed releases' error, skipping") @@ -132,10 +262,94 @@ func (r *FluxPlunger) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Res // Suspend the HelmRelease logger.Info("Suspending HelmRelease") - if err := r.suspendHelmRelease(ctx, hr); err != nil { - // Optimistic lock conflicts are normal - FluxCD also updates HelmRelease - // Don't return error, just log and let controller-runtime requeue on next update - logger.Info("Could not suspend HelmRelease, will retry on next reconcile", "error", err.Error()) + acquired, err := r.suspendHelmRelease(ctx, hr) + if err != nil { + // Requeue with backoff on any error, symmetric with every step below. + // suspendHelmRelease reports every conflict, on either apply, as not acquired + // (handled below), so an error reaching here is a non-conflict failure (an + // admission webhook rejecting the apply, or an API error) that produces no + // follow-up event. Swallowing it would strand a stuck release until the next + // informer resync. + logger.Info("Could not suspend HelmRelease, requeuing", "error", err.Error()) + return ctrl.Result{}, err + } + if !acquired { + // The release was suspended by someone else, started terminating, or was + // written again between our read and our write, so flux-plunger does not own + // it. Do not delete its storage Secret; the write that got in between is an + // update event, and the next reconcile handles the release as it now is. + logger.Info("HelmRelease was not acquired for recovery, deferring to next reconcile") + r.recordEvent(hr, corev1.EventTypeNormal, "RecoveryDeferred", + "Deferred recovery: spec.suspend was taken by another manager or the release began terminating between read and write; will retry on the next reconcile") + return ctrl.Result{}, nil + } + + // From here flux-plunger owns the suspension (acquired above). Guarantee it is + // released before returning from any path below: a step that fails must never + // leave the HelmRelease suspended, because a delete arriving on a suspended + // release makes helm-controller skip the uninstall and orphan the release's + // resources. The unsuspend is idempotent, so the happy path running it + // explicitly and this deferred fallback are safe together. + completed := false + defer func() { + if completed { + return + } + if err := r.unsuspendHelmRelease(ctx, hr); err != nil { + logger.Info("Could not clear suspension after failed recovery step, will retry on next reconcile", "error", err.Error()) + r.recordUnsuspendFailed(hr, err) + } + }() + + // Record the version we are about to reclaim BEFORE deleting its storage + // Secret. The "already processed" guard above keys on this annotation, so + // writing it first means a failed write can never authorise deleting the NEXT + // revision on the following requeue: on failure we return here, the deferred + // unsuspend clears the suspension, and the retry re-deletes the SAME revision + // (idempotent) instead of walking down the release history one Secret per + // requeue until none is left, which would itself orphan the release. That covers + // a failed write or delete only: the guard compares revision numbers, so it cannot + // tell them from a revision Helm re-creates under the same number after the + // delete, and such a revision is reclaimed again. Requeue with backoff on failure, + // symmetric with every other step. + logger.Info("Updating annotation with processed version", "version", latestVersion) + if err := r.updateProcessedVersionAnnotation(ctx, hr, latestVersion); err != nil { + logger.Info("Could not update annotation, requeuing", "error", err.Error()) + return ctrl.Result{}, err + } + + // A delete can land between acquiring the suspension and here. Deleting the + // storage Secret of a terminating release removes the only revision + // helm-controller could uninstall; it then treats the release as already gone, + // drops its finalizer, and orphans the resources — the exact failure this + // recovery exists to prevent, reached by a narrower race than the suspended one. + // The top-of-Reconcile check and suspendHelmRelease's re-fetch only see a delete + // visible earlier, so re-check immediately before the delete. On termination, + // leave the Secret intact and return: the deferred unsuspend clears our + // suspension so helm-controller runs the uninstall against the intact revision. + // This narrows the window to the Get-then-Delete gap but cannot fully close it — + // there is no HelmRelease+Secret transaction — so a delete landing in that gap is + // an accepted residual, the tightest a two-object check-then-act allows. + terminating, err := r.releaseIsTerminating(ctx, hr) + if err != nil { + logger.Info("Could not re-check deletion before deleting the storage Secret, requeuing", "error", err.Error()) + return ctrl.Result{}, err + } + if terminating { + // Clear our suspension explicitly and requeue on failure. Leaving it to the + // log-only deferred fallback would let a failed unsuspend strand an owned + // suspension on a terminating release with nothing retrying it — the exact + // state that makes helm-controller skip the uninstall and orphan the + // resources. The Secret is left intact so the uninstall has a revision. + logger.Info("HelmRelease started terminating during recovery, clearing suspension so helm-controller can uninstall") + if err := r.unsuspendHelmRelease(ctx, hr); err != nil { + logger.Info("Could not clear suspension on terminating release, requeuing", "error", err.Error()) + r.recordUnsuspendFailed(hr, err) + // The returned error requeues and retries; skip the redundant deferred attempt. + completed = true + return ctrl.Result{}, err + } + completed = true return ctrl.Result{}, nil } @@ -146,22 +360,141 @@ func (r *FluxPlunger) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Res return ctrl.Result{}, err } - // Update annotation with processed version - logger.Info("Updating annotation with processed version", "version", latestVersion) - if err := r.updateProcessedVersionAnnotation(ctx, hr, latestVersion); err != nil { - logger.Info("Could not update annotation, will retry on next reconcile", "error", err.Error()) - return ctrl.Result{}, nil - } - - // Unsuspend the HelmRelease + // Unsuspend the HelmRelease. Force a backoff requeue on failure, symmetric with + // the deletion path: a persistently failing unsuspend (e.g. an admission webhook + // rejecting the patch) would otherwise leave the release suspended-by-plunger + // until the informer's periodic resync, far longer than necessary. logger.Info("Unsuspending HelmRelease") if err := r.unsuspendHelmRelease(ctx, hr); err != nil { - logger.Info("Could not unsuspend HelmRelease, will retry on next reconcile", "error", err.Error()) + logger.Info("Could not unsuspend HelmRelease, requeuing", "error", err.Error()) + r.recordUnsuspendFailed(hr, err) + // The returned error requeues and retries; skip the redundant deferred attempt. + completed = true + return ctrl.Result{}, err + } + + completed = true + logger.Info("Successfully processed HelmRelease", "version", latestVersion) + return ctrl.Result{}, nil +} + +// reconcileTerminating makes a delete of a release flux-plunger is recovering reach +// the Helm uninstall. helm-controller v1.5.x skips the uninstall and drops its +// finalizer for a suspended HelmRelease +// (https://github.com/fluxcd/helm-controller/blob/v1.5.0/internal/controller/helmrelease_controller.go#L447-L475), +// and it handles the delete on the same event flux-plunger does. The fence +// finalizer keeps the object whichever handles it first; flux-plunger lifts its own +// suspension, which bumps the generation and makes helm-controller reconcile the +// unsuspended release and uninstall it, and only then lifts the fence. A terminating +// release never begins recovery. +func (r *FluxPlunger) reconcileTerminating(ctx context.Context, hr *helmv2.HelmRelease) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + if hr.Spec.Suspend && r.ownsSuspension(hr) { + logger.Info("HelmRelease is being deleted while suspended by flux-plunger, clearing suspension so uninstall can proceed") + if err := r.unsuspendHelmRelease(ctx, hr); err != nil { + logger.Info("Could not clear suspension on deleted HelmRelease, requeuing", "error", err.Error()) + r.recordUnsuspendFailed(hr, err) + return ctrl.Result{}, err + } + // helm-controller's reconcile of the new generation updates the status, + // which wakes this controller again to lift the fence. return ctrl.Result{}, nil } - logger.Info("Successfully processed HelmRelease", "version", latestVersion) - return ctrl.Result{}, nil + if hr.Spec.Suspend { + // Another manager's suspension: helm-controller skips the uninstall for it + // and leaves the release's resources in place, which a migration that + // suspends then deletes relies on. That is not flux-plunger's decision to + // override, and not a release its fence should hold. A release that + // carries flux-plunger's processed-version annotation was recovered before, + // so the suspension that skips its uninstall was not a migration's choice (an + // operator's hand, or one an earlier flux-plunger release left behind): that + // is the orphaning this controller exists to prevent, and a Warning. + logger.Info("HelmRelease is being deleted while suspended by another manager; helm-controller skips the uninstall") + if _, recovered := hr.Annotations[annotationLastProcessedVersion]; recovered { + r.recordEvent(hr, corev1.EventTypeWarning, "TerminatingWhileSuspended", + "HelmRelease is being deleted while suspended by another manager; helm-controller skips the Helm uninstall for a suspended release, so its resources are left behind") + } else { + r.recordEvent(hr, corev1.EventTypeNormal, "TerminatingWhileSuspended", + "HelmRelease is being deleted while suspended by another manager; helm-controller skips the Helm uninstall for a suspended release and leaves its resources in place") + } + return ctrl.Result{}, r.releaseFence(ctx, hr) + } + + if !controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + return ctrl.Result{}, nil + } + if uninstallReached(hr) { + logger.Info("helm-controller has handled the unsuspended release, lifting the uninstall fence") + return ctrl.Result{}, r.releaseFence(ctx, hr) + } + // Still waiting for helm-controller to handle the unsuspended release. Re-check + // on a timer rather than only on its next status write, which never comes if it + // is not running, and say so once the delete has been held past the deadline. + if held := r.clock().Sub(hr.DeletionTimestamp.Time); held >= fenceHeldWarnAfter { + logger.Info("HelmRelease delete is still held by the uninstall fence, waiting for helm-controller", "held", held.String()) + r.recordEvent(hr, corev1.EventTypeWarning, "UninstallFenceHeld", + "HelmRelease delete is held by flux-plunger's uninstall fence until helm-controller handles the unsuspended release; check that helm-controller is running and reconciling it") + } + return ctrl.Result{RequeueAfter: fenceRecheckInterval}, nil +} + +// releaseFence removes flux-plunger's fence finalizer by relinquishing it. Callers +// hold no suspension of their own at this point: the relinquishing apply drops +// every field flux-plunger's manager owns. +func (r *FluxPlunger) releaseFence(ctx context.Context, hr *helmv2.HelmRelease) error { + if !controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + return nil + } + return r.applyRecoveryState(ctx, hr, nil, false, false) +} + +// ownsSuspensionManagedFields reports whether flux-plunger owns the spec.suspend +// field via its server-side-apply field manager, recorded by the apiserver in +// metadata.managedFields. This is the sole ownership signal, and it is exact in +// the ways a bare annotation marker was not: +// +// - An operator (or `cozyhr resume`, or the backup controller) that resumes and +// later re-suspends the release writes spec.suspend under its own manager, so +// the apiserver transfers the field away from flux-plunger and this returns +// false — the later suspension is never misread as ours and cleared out from +// under them, the one boundary this controller must never cross. +// - An unrelated re-apply of other spec fields (the routine reconcile of the +// cozystack operator or helm-controller) leaves spec.suspend's owner untouched, +// so flux-plunger keeps ownership of a suspension it is mid-recovery on — no +// self-disown, unlike keying ownership on metadata.generation. +func ownsSuspensionManagedFields(hr *helmv2.HelmRelease) bool { + for _, e := range hr.ManagedFields { + if e.Manager == suspendFieldManager && + e.Operation == metav1.ManagedFieldsOperationApply && + fieldsManageSuspend(e.FieldsV1) { + return true + } + } + return false +} + +// fieldsManageSuspend reports whether a managed-fields set includes spec.suspend, +// encoded by the apiserver as {"f:spec":{"f:suspend":{}}}. +func fieldsManageSuspend(fs *metav1.FieldsV1) bool { + if fs == nil || len(fs.Raw) == 0 { + return false + } + var top map[string]json.RawMessage + if err := json.Unmarshal(fs.Raw, &top); err != nil { + return false + } + specRaw, ok := top["f:spec"] + if !ok { + return false + } + var spec map[string]json.RawMessage + if err := json.Unmarshal(specRaw, &spec); err != nil { + return false + } + _, ok = spec["f:suspend"] + return ok } // hasNoDeployedReleasesError checks if the HelmRelease has the specific error @@ -176,7 +509,21 @@ func hasNoDeployedReleasesError(hr *helmv2.HelmRelease) bool { return false } -// listHelmReleaseSecrets lists all Helm release secrets for a specific release +// listHelmReleaseSecrets lists all Helm release secrets for a specific release. +// +// It reads through the uncached APIReader (when wired). This is not only a race +// guard but a correctness fix: the controller's cached client (mgr.GetClient()) +// would serve this List from an informer, and the ClusterRole grants secrets only +// get/list, NOT watch, which an informer needs to stay synced — so a cached List +// cannot reliably reflect the cluster (stale at best, unsynced at worst, with a +// steady stream of watch-forbidden errors). Reading directly from the apiserver via +// the APIReader needs only get/list and sidesteps the informer entirely. +// getLatestSecret over the result also chooses which storage Secret is deleted and +// which version the processed annotation records, and this List runs before the +// release is suspended, exactly when helm-controller may still be creating a +// revision Secret — the same race the other re-reads guard against — so the direct +// read is doubly warranted. flux-plunger deliberately does NOT cache Secrets (no +// watch RBAC, and it would mean watching every Secret in the cluster). func (r *FluxPlunger) listHelmReleaseSecrets(ctx context.Context, namespace, releaseName string) ([]corev1.Secret, error) { secretList := &corev1.SecretList{} listOpts := []client.ListOption{ @@ -187,7 +534,11 @@ func (r *FluxPlunger) listHelmReleaseSecrets(ctx context.Context, namespace, rel }, } - if err := r.List(ctx, secretList, listOpts...); err != nil { + lister := client.Reader(r.Client) + if r.APIReader != nil { + lister = r.APIReader + } + if err := lister.List(ctx, secretList, listOpts...); err != nil { return nil, fmt.Errorf("failed to list secrets: %w", err) } @@ -240,44 +591,177 @@ func extractVersionNumber(secretName string) int { return num } -// suspendHelmRelease sets suspend to true on the HelmRelease -func (r *FluxPlunger) suspendHelmRelease(ctx context.Context, hr *helmv2.HelmRelease) error { - // Re-fetch the HelmRelease to get the latest state - key := types.NamespacedName{Namespace: hr.Namespace, Name: hr.Name} - latestHR := &helmv2.HelmRelease{} - if err := r.Get(ctx, key, latestHR); err != nil { - return fmt.Errorf("failed to get latest HelmRelease: %w", err) +// applyRecoveryState server-side-applies the fields flux-plunger's field manager +// owns during a recovery: spec.suspend and the fence finalizer, and nothing else. +// Whatever the body leaves out is relinquished. value == nil omits spec.suspend +// (when flux-plunger is its sole manager the field reverts to the unsuspended +// default); value != nil sets it to *value. fence declares the fence finalizer; +// metadata.finalizers is a set, so this adds or drops only flux-plunger's entry and +// leaves helm-controller's alone. force appends client.ForceOwnership. +// +// The apply body is an unstructured object carrying just metadata and these +// fields, NOT a typed HelmReleaseSpec: marshaling a typed spec would also emit +// spec.interval ("0s", it is a required field without omitempty), and applying +// that over a release whose interval another manager owns conflicts on +// spec.interval and never suspends, while forcing would reset the release's +// reconcile interval. +func (r *FluxPlunger) applyRecoveryState(ctx context.Context, hr *helmv2.HelmRelease, value *bool, fence, force bool) error { + apply := &unstructured.Unstructured{} + apply.SetGroupVersionKind(helmv2.GroupVersion.WithKind("HelmRelease")) + apply.SetNamespace(hr.Namespace) + apply.SetName(hr.Name) + // Precondition every apply on the object the caller read, so a write or delete + // that lands after that read is rejected with a conflict and the decision is + // taken again on the new state. The resourceVersion covers a change: unforced, + // the apiserver would otherwise accept true over a foreign true as shared + // ownership; forced, take the field over; and a relinquish decided on a live + // release would remove the fence after helm-controller had already dropped its + // finalizer on a delete, deleting the release without an uninstall. The uid + // covers a release that is gone: server-side apply creates what does not exist, + // and only a uid mismatch stops it resurrecting an empty HelmRelease. + apply.SetResourceVersion(hr.ResourceVersion) + apply.SetUID(hr.UID) + if fence { + apply.SetFinalizers([]string{fenceFinalizer}) } - - // If already suspended, nothing to do - if latestHR.Spec.Suspend { - return nil + if value != nil { + if err := unstructured.SetNestedField(apply.Object, *value, "spec", "suspend"); err != nil { + return err + } } - - patch := client.MergeFromWithOptions(latestHR.DeepCopy(), client.MergeFromWithOptimisticLock{}) - latestHR.Spec.Suspend = true - - return r.Patch(ctx, latestHR, patch, client.FieldOwner(fieldManager)) + opts := []client.PatchOption{client.FieldOwner(suspendFieldManager)} + if force { + opts = append(opts, client.ForceOwnership) + } + return r.Patch(ctx, apply, client.Apply, opts...) } -// unsuspendHelmRelease sets suspend to false on the HelmRelease -func (r *FluxPlunger) unsuspendHelmRelease(ctx context.Context, hr *helmv2.HelmRelease) error { +// suspendHelmRelease takes ownership of a fresh suspension via server-side apply. +// It reports whether it acquired one: false means the release was already +// suspended by someone else, or it started terminating after the top-of-Reconcile +// check, so the caller must not proceed as if it owns it. +// +// Both applies set the fence finalizer together with the suspension, so there is +// no moment at which flux-plunger holds a suspension a delete could slip past, and +// both are preconditioned on the resourceVersion of the read that decided on them +// (see applyRecoveryState), so any write landing after that read makes the apply +// fail with a conflict rather than be absorbed. +// +// The apply is attempted WITHOUT force first. A fresh, unowned spec.suspend applies +// cleanly and flux-plunger owns it. A conflict means the release changed since the +// read, or another manager already owns the field: this repo's backup controllers +// set spec.suspend via a dynamic-client Update (setCNPGRestoreHRSuspended / +// setEtcdRestoreHRSuspended / velero's suspendHelmRelease), and earlier flux-plunger +// releases owned it via a merge Patch, so a non-forced apply would 409 forever and +// flux-plunger could never recover such a release. On conflict we re-fetch and force +// ONLY if spec.suspend is still false: a manager owning the field at false is stale +// ownership safe to take over, while a value of true is an ACTIVE suspension (an +// operator's, or a backup mid-restore) that flux-plunger must never clear. A +// conflict on the forced retry means yet another write landed after the re-read, +// the same race as a foreign suspension seen on the re-read, so it is reported the +// same way: not acquired, no error. +func (r *FluxPlunger) suspendHelmRelease(ctx context.Context, hr *helmv2.HelmRelease) (bool, error) { // Re-fetch the HelmRelease to get the latest state key := types.NamespacedName{Namespace: hr.Namespace, Name: hr.Name} latestHR := &helmv2.HelmRelease{} - if err := r.Get(ctx, key, latestHR); err != nil { + if err := r.refetch(ctx, key, latestHR); err != nil { + return false, fmt.Errorf("failed to get latest HelmRelease: %w", err) + } + + // Never suspend a terminating release. A delete can arrive between the + // top-of-Reconcile deletion check and this re-fetch, and suspending a release + // helm-controller is about to uninstall is exactly what makes it skip the + // uninstall and orphan the release's resources. + if !latestHR.DeletionTimestamp.IsZero() { + return false, nil + } + + // If already suspended, do not take ownership of a suspension we did not set. + if latestHR.Spec.Suspend { + return false, nil + } + + suspendTrue := true + err := r.applyRecoveryState(ctx, latestHR, &suspendTrue, true, false) + if err == nil { + return true, nil + } + if !apierrors.IsConflict(err) { + return false, err + } + + // Conflict: another manager owns spec.suspend. Re-read its value and force only + // if it is still unsuspended (stale ownership); leave an active suspension alone. + // Decode into a FRESH object: client-go's Get unmarshals into the receiver without + // zeroing it, so a field absent from the second response (spec.suspend is + // omitempty) would otherwise retain the first read's value. + rechecked := &helmv2.HelmRelease{} + if err := r.refetch(ctx, key, rechecked); err != nil { + return false, fmt.Errorf("failed to re-fetch HelmRelease after suspend conflict: %w", err) + } + if !rechecked.DeletionTimestamp.IsZero() { + return false, nil + } + if rechecked.Spec.Suspend { + return false, nil + } + if err := r.applyRecoveryState(ctx, rechecked, &suspendTrue, true, true); err != nil { + if apierrors.IsConflict(err) { + return false, nil + } + return false, err + } + return true, nil +} + +// releaseIsTerminating re-fetches the HelmRelease and reports whether it has begun +// terminating. Recovery calls this immediately before deleting the storage Secret +// so a delete that landed after the earlier checks cannot make it remove the only +// revision helm-controller could uninstall. A release that is already gone counts +// as terminating. +func (r *FluxPlunger) releaseIsTerminating(ctx context.Context, hr *helmv2.HelmRelease) (bool, error) { + key := types.NamespacedName{Namespace: hr.Namespace, Name: hr.Name} + latestHR := &helmv2.HelmRelease{} + if err := r.refetch(ctx, key, latestHR); err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } + return false, fmt.Errorf("failed to get latest HelmRelease: %w", err) + } + return !latestHR.DeletionTimestamp.IsZero(), nil +} + +// unsuspendHelmRelease clears a suspension flux-plunger owns by relinquishing the +// spec.suspend field: a server-side apply that no longer declares it, which reverts +// the field to unsuspended (flux-plunger being its only manager) and drops +// flux-plunger's ownership. On a live release the fence goes with it. On a +// terminating one the fence stays: helm-controller may already have dropped its +// finalizer without an uninstall, and the fence is what keeps the object until it +// reconciles the unsuspended release (reconcileTerminating lifts it then). A +// release that is already gone has nothing left to clear. +// +// Callers already gate on ownership; the re-fetched ownership check here is +// defense-in-depth against a future caller that forgets to, so an operator's +// suspension, for which the apiserver has already transferred the field away from +// flux-plunger, is never cleared by accident. +func (r *FluxPlunger) unsuspendHelmRelease(ctx context.Context, hr *helmv2.HelmRelease) error { + key := types.NamespacedName{Namespace: hr.Namespace, Name: hr.Name} + latestHR := &helmv2.HelmRelease{} + if err := r.refetch(ctx, key, latestHR); err != nil { + if apierrors.IsNotFound(err) { + return nil + } return fmt.Errorf("failed to get latest HelmRelease: %w", err) } - // If already unsuspended, nothing to do - if !latestHR.Spec.Suspend { + // Nothing to do if flux-plunger does not own the suspension: either it is + // already unsuspended, or the field belongs to another manager now. + if !r.ownsSuspension(latestHR) { return nil } - patch := client.MergeFromWithOptions(latestHR.DeepCopy(), client.MergeFromWithOptimisticLock{}) - latestHR.Spec.Suspend = false - - return r.Patch(ctx, latestHR, patch, client.FieldOwner(fieldManager)) + keepFence := !latestHR.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(latestHR, fenceFinalizer) + return r.applyRecoveryState(ctx, latestHR, nil, keepFence, false) } // updateProcessedVersionAnnotation updates the annotation with the processed version @@ -285,7 +769,7 @@ func (r *FluxPlunger) updateProcessedVersionAnnotation(ctx context.Context, hr * // Re-fetch the HelmRelease to get the latest state key := types.NamespacedName{Namespace: hr.Namespace, Name: hr.Name} latestHR := &helmv2.HelmRelease{} - if err := r.Get(ctx, key, latestHR); err != nil { + if err := r.refetch(ctx, key, latestHR); err != nil { return fmt.Errorf("failed to get latest HelmRelease: %w", err) } @@ -299,35 +783,41 @@ func (r *FluxPlunger) updateProcessedVersionAnnotation(ctx context.Context, hr * return r.Patch(ctx, latestHR, patch, client.FieldOwner(fieldManager)) } +// watchPredicate selects the HelmReleases flux-plunger reconciles: those carrying +// the specific error, a suspension flux-plunger owns (crash recovery, and a delete +// arriving mid-recovery, both need to wake a reconcile), flux-plunger's fence, or +// any suspended release that is being deleted. Kept a named method so a test can +// pin it. +func (r *FluxPlunger) watchPredicate(obj client.Object) bool { + hr, ok := obj.(*helmv2.HelmRelease) + if !ok { + return false + } + if hasNoDeployedReleasesError(hr) { + return true + } + if hr.Spec.Suspend && r.ownsSuspension(hr) { + return true + } + // The fence is lifted in response to helm-controller's status update after the + // uninstall, which carries neither the error nor a flux-plunger suspension. + if controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + return true + } + // A suspended release being deleted skips the Helm uninstall even when the + // suspension is someone else's and the release is healthy: Reconcile has to see it + // to record TerminatingWhileSuspended. + if hr.Spec.Suspend && !hr.DeletionTimestamp.IsZero() { + return true + } + return false +} + // SetupWithManager sets up the controller with the Manager func (r *FluxPlunger) SetupWithManager(mgr ctrl.Manager) error { - // Watch HelmReleases that either: - // 1. Have the specific error, OR - // 2. Are suspended with our annotation (to handle crash recovery) - pred := predicate.NewPredicateFuncs(func(obj client.Object) bool { - hr, ok := obj.(*helmv2.HelmRelease) - if !ok { - return false - } - - // Always process if has error - if hasNoDeployedReleasesError(hr) { - return true - } - - // Also process suspended HelmReleases with our annotation (crash recovery) - if hr.Spec.Suspend && hr.Annotations != nil { - if _, exists := hr.Annotations[annotationLastProcessedVersion]; exists { - return true - } - } - - return false - }) - return ctrl.NewControllerManagedBy(mgr). Named("fluxplunger"). For(&helmv2.HelmRelease{}). - WithEventFilter(pred). + WithEventFilter(predicate.NewPredicateFuncs(r.watchPredicate)). Complete(r) } diff --git a/internal/controller/fluxplunger/flux_plunger_test.go b/internal/controller/fluxplunger/flux_plunger_test.go new file mode 100644 index 000000000..91dbba869 --- /dev/null +++ b/internal/controller/fluxplunger/flux_plunger_test.go @@ -0,0 +1,2081 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 The Cozystack Authors. + +package fluxplunger + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + helmv2 "github.com/fluxcd/helm-controller/api/v2" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +const ( + testNS = "tenant-root" + testName = "vm-disk-0" + // helm-controller's finalizer; present on every reconciled HelmRelease and + // required by the fake client for any object carrying a deletionTimestamp. + helmFinalizer = "finalizers.fluxcd.io" +) + +func testScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(s); err != nil { + t.Fatalf("add client-go scheme: %v", err) + } + if err := helmv2.AddToScheme(s); err != nil { + t.Fatalf("add helm scheme: %v", err) + } + return s +} + +// The controller-runtime fake client does not persist metadata.managedFields, so +// the production ownership check (ownsSuspensionManagedFields, verified by +// TestOwnsSuspensionManagedFields and the envtest suite) cannot be exercised +// through it. Behavioural tests inject FluxPlunger.owns with testOwns, a stand-in +// that reads a plain annotation the fake client DOES persist, and ssaModelApply +// maintains that annotation with the same dynamics the apiserver gives +// managedFields: set on flux-plunger's suspend apply, dropped on its relinquish. +const testOwnedKey = "test.fluxplunger.cozystack.io/owned" + +func testOwns(hr *helmv2.HelmRelease) bool { + return hr.Annotations[testOwnedKey] == "true" +} + +// ownedFixture marks a HelmRelease as suspended and owned by flux-plunger. +func ownedFixture(hr *helmv2.HelmRelease) *helmv2.HelmRelease { + hr.Spec.Suspend = true + if hr.Annotations == nil { + hr.Annotations = map[string]string{} + } + hr.Annotations[testOwnedKey] = "true" + return hr +} + +// newPlunger builds the reconciler with the injected test ownership check. +func newPlunger(c client.Client) *FluxPlunger { + return &FluxPlunger{Client: c, owns: testOwns} +} + +// suspendManagedFields builds a managedFields entry for the pure-unit test of the +// real ownership check. +func suspendManagedFields(manager string, op metav1.ManagedFieldsOperationType) []metav1.ManagedFieldsEntry { + return []metav1.ManagedFieldsEntry{{ + Manager: manager, + Operation: op, + APIVersion: helmv2.GroupVersion.String(), + FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:suspend":{}}}`)}, + }} +} + +// noDeployedReleasesHR builds a HelmRelease stuck with the "has no deployed +// releases" error that flux-plunger is meant to recover. +func noDeployedReleasesHR() *helmv2.HelmRelease { + return &helmv2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNS, + Name: testName, + Finalizers: []string{helmFinalizer}, + }, + Status: helmv2.HelmReleaseStatus{ + Conditions: []metav1.Condition{{ + Type: "Ready", + Status: metav1.ConditionFalse, + Reason: "InstallFailed", + Message: "Helm install failed: " + errorMessageNoDeployedReleases, + }}, + }, + } +} + +func helmReleaseSecret(version string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNS, + Name: "sh.helm.release.v1." + testName + "." + version, + Labels: map[string]string{ + "name": testName, + "owner": "helm", + }, + }, + Type: "helm.sh/release.v1", + } +} + +func getHR(t *testing.T, c client.Client) *helmv2.HelmRelease { + t.Helper() + hr := &helmv2.HelmRelease{} + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: testName}, hr); err != nil { + t.Fatalf("get HelmRelease: %v", err) + } + return hr +} + +// expectWarning drains the recorded events and fails unless one is a Warning with +// the given reason. +func expectWarning(t *testing.T, rec *record.FakeRecorder, reason string) { + t.Helper() + var seen []string + for { + select { + case e := <-rec.Events: + if strings.HasPrefix(e, corev1.EventTypeWarning+" "+reason+" ") { + return + } + seen = append(seen, e) + default: + t.Fatalf("expected a Warning %s event, got %q", reason, seen) + } + } +} + +func reconcile(t *testing.T, r *FluxPlunger) { + t.Helper() + _, _ = r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}, + }) +} + +// ssaModelApply models, on the fake client, what a real apiserver does with a +// flux-plunger server-side apply of spec.suspend, recorded via the test ownership +// annotation instead of managedFields (which the fake client drops): apply +// suspend=true and set the owned annotation; on the relinquish apply (suspend +// omitted), revert suspend to false and drop the annotation. This is exactly the +// dynamics the build-tagged envtest suite (ssa_ownership_envtest_test.go) proves +// against a real apiserver: an apply of true takes ownership, and a sole-owner +// relinquish reverts to false and drops the owner. Any other patch passes through +// unchanged. +// +// LIMITATION: this models NET STATE only. It treats every flux-plunger apply that +// carries suspend=false OR omits the field as "clear the suspension", so it cannot +// distinguish a relinquish (omit) from a forced set-false — on a real apiserver a +// relinquish leaves a value another manager owns unchanged, while a forced apply +// moves it. A test that depends on that distinction must assert the request SHAPE +// via plungerApplyDetail (present/value/force), not the resulting object; a test +// that checks only the object state is blind to it and can pass against broken code. +func ssaModelApply(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + po := &client.PatchOptions{} + po.ApplyOptions(opts) + suspend, isPlunger := plungerApplySuspend(obj, patch, po) + if !isPlunger { + return cl.Patch(ctx, obj, patch, opts...) + } + // Model the net effect with a plain Get+Update rather than the real SSA apply: + // the fake client's Apply strips unmanaged fields (it would drop a finalizer off + // a deleting object and garbage-collect it), which is a fake-client artefact, not + // apiserver behaviour. Get+Update preserves finalizers and status while applying + // exactly the suspend/ownership transition. + cur := &helmv2.HelmRelease{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(obj), cur); err != nil { + return err + } + if cur.Annotations == nil { + cur.Annotations = map[string]string{} + } + switch { + case suspend: + cur.Spec.Suspend = true + cur.Annotations[testOwnedKey] = "true" + case cur.Annotations[testOwnedKey] == "true": + // Relinquishing a suspension flux-plunger owns reverts it; one another + // manager owns keeps its value. + cur.Spec.Suspend = false + delete(cur.Annotations, testOwnedKey) + } + // The fence finalizer follows the body, and, like the apiserver, a new + // finalizer cannot be added to an object that is being deleted. + wantFence := false + for _, f := range obj.GetFinalizers() { + if f == fenceFinalizer { + wantFence = true + } + } + hasFence := controllerutil.ContainsFinalizer(cur, fenceFinalizer) + if wantFence && !hasFence { + if !cur.DeletionTimestamp.IsZero() { + return apierrors.NewForbidden(schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + cur.Name, errors.New("no new finalizers can be added if the object is being deleted")) + } + controllerutil.AddFinalizer(cur, fenceFinalizer) + } + if !wantFence && hasFence { + controllerutil.RemoveFinalizer(cur, fenceFinalizer) + } + return cl.Update(ctx, cur) +} + +// plungerApplyDetail reports the shape of a flux-plunger spec.suspend apply so a +// test can tell whether it forces ownership: acquire attempts an unforced apply +// (present, value true, force false) first and forces (force true) only over a +// field re-read as false. plungerApplySuspend does not expose the force flag; this +// does. +func plungerApplyDetail(obj client.Object, patch client.Patch, opts []client.PatchOption) (present, value, force, ok bool) { + po := &client.PatchOptions{} + po.ApplyOptions(opts) + u, isU := obj.(*unstructured.Unstructured) + if !isU || patch.Type() != types.ApplyPatchType || po.FieldManager != suspendFieldManager { + return false, false, false, false + } + val, found, _ := unstructured.NestedBool(u.Object, "spec", "suspend") + return found, val, po.Force != nil && *po.Force, true +} + +// plungerApplySuspend reports whether a patch is flux-plunger's server-side apply +// of spec.suspend (the production applySuspend sends an unstructured body carrying +// only spec.suspend) and, if so, the applied value (false when the field is +// omitted — the relinquish). The failure-injection tests use it to fail exactly +// the suspend or the unsuspend step. +func plungerApplySuspend(obj client.Object, patch client.Patch, po *client.PatchOptions) (suspend bool, ok bool) { + u, isU := obj.(*unstructured.Unstructured) + if !isU || patch.Type() != types.ApplyPatchType || po.FieldManager != suspendFieldManager { + return false, false + } + val, found, _ := unstructured.NestedBool(u.Object, "spec", "suspend") + return found && val, true +} + +// isPlungerApply is the boolean form used where the test only cares about a +// specific applied value. +func isPlungerApply(obj client.Object, patch client.Patch, opts []client.PatchOption, suspend bool) bool { + po := &client.PatchOptions{} + po.ApplyOptions(opts) + got, ok := plungerApplySuspend(obj, patch, po) + return ok && got == suspend +} + +// TestOwnsSuspensionManagedFields pins the production ownership predicate directly +// on hand-crafted managedFields: only flux-plunger's own Apply of spec.suspend +// counts, not an operator Update of the same field, nor an apply that manages +// other fields. This is the check the behavioural tests inject a stand-in for. +func TestOwnsSuspensionManagedFields(t *testing.T) { + cases := []struct { + name string + mf []metav1.ManagedFieldsEntry + want bool + }{ + {"plunger apply owns", suspendManagedFields(suspendFieldManager, metav1.ManagedFieldsOperationApply), true}, + // Isolates the operation conjunct: same manager as the owning case but an + // Update, which only the Operation==Apply term rejects. + {"plunger update does not count", suspendManagedFields(suspendFieldManager, metav1.ManagedFieldsOperationUpdate), false}, + {"operator update does not count", suspendManagedFields("kubectl", metav1.ManagedFieldsOperationUpdate), false}, + {"another manager apply does not count", suspendManagedFields("helm-controller", metav1.ManagedFieldsOperationApply), false}, + // fieldManager (flux-client-side-apply) is a SHARED client-side manager: the + // earlier flux-plunger releases, platform migrations and the tenant delete hook all + // set spec.suspend under it, so it does NOT identify flux-plunger — its + // ownership must never be read as flux-plunger's. + {"shared client-side manager does not count", suspendManagedFields(fieldManager, metav1.ManagedFieldsOperationUpdate), false}, + {"no managed fields", nil, false}, + {"plunger apply of other fields", []metav1.ManagedFieldsEntry{{ + Manager: suspendFieldManager, Operation: metav1.ManagedFieldsOperationApply, + FieldsType: "FieldsV1", FieldsV1: &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:chart":{}}}`)}, + }}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hr := &helmv2.HelmRelease{ObjectMeta: metav1.ObjectMeta{ManagedFields: tc.mf}} + if got := ownsSuspensionManagedFields(hr); got != tc.want { + t.Fatalf("ownsSuspensionManagedFields=%v want %v", got, tc.want) + } + }) + } +} + +// The normal recovery of a stuck release must delete the latest storage Secret, +// record the processed version and leave the HelmRelease unsuspended and no +// longer owned by flux-plunger. +func TestReconcile_HappyPathRecovery(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v3") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("recovery must end unsuspended, got suspend=true") + } + if testOwns(got) { + t.Fatalf("recovery must relinquish ownership of the suspension") + } + if got.Annotations[annotationLastProcessedVersion] != "3" { + t.Fatalf("recovery must record the processed version, got %q", got.Annotations[annotationLastProcessedVersion]) + } + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); !apierrors.IsNotFound(err) { + t.Fatalf("recovery must delete the latest Helm storage Secret, got err=%v", err) + } +} + +// If the release is suspended by someone else between flux-plunger's initial +// read and its own suspend write, recovery must abort without deleting the +// storage Secret: flux-plunger does not own that suspension. +func TestReconcile_AbortsWhenSuspendedConcurrently(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + hrGets := 0 + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := cl.Get(ctx, key, obj, opts...); err != nil { + return err + } + // The first Get is the top-of-reconcile read (must see suspend=false + // so recovery starts). The second Get is suspendHelmRelease's re-fetch: + // simulate an external actor having suspended it meanwhile (no plunger + // ownership). + if h, ok := obj.(*helmv2.HelmRelease); ok { + hrGets++ + if hrGets == 2 { + h.Spec.Suspend = true + } + } + return nil + }, + Patch: ssaModelApply, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(err) { + t.Fatalf("recovery must not delete the storage Secret when it did not acquire the suspension") + } +} + +// If the release starts terminating after recovery already acquired the +// suspension (the delete lands at the pre-delete re-check), recovery must leave +// the storage Secret intact and clear its own suspension so helm-controller can +// uninstall the release cleanly. +func TestReconcile_TerminatingDuringRecoveryKeepsSecretAndClearsSuspension(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + hrGets := 0 + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := cl.Get(ctx, key, obj, opts...); err != nil { + return err + } + // Gets 1-3 are the top read, suspendHelmRelease's re-fetch and the + // annotation re-fetch (all must see a live release so recovery starts and + // acquires). The 4th Get is releaseIsTerminating's pre-delete re-check: + // simulate the delete having landed by then. + if h, ok := obj.(*helmv2.HelmRelease); ok { + hrGets++ + if hrGets >= 4 { + now := metav1.Now() + h.DeletionTimestamp = &now + h.Finalizers = []string{helmFinalizer} + } + } + return nil + }, + Patch: ssaModelApply, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(err) { + t.Fatalf("terminating recovery must leave the storage Secret for helm-controller to uninstall") + } + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("terminating recovery must clear the suspension so helm-controller can uninstall, got suspend=true") + } +} + +// If the release starts terminating during recovery and clearing the suspension +// then fails, recovery must requeue with an error rather than return success: +// leaving an owned suspension on a terminating release is what makes +// helm-controller skip the uninstall and orphan the resources. +func TestReconcile_TerminatingDuringRecoveryRequeuesOnUnsuspendFailure(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + hrGets := 0 + unsuspendApplies := 0 + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := cl.Get(ctx, key, obj, opts...); err != nil { + return err + } + if h, ok := obj.(*helmv2.HelmRelease); ok { + hrGets++ + if hrGets >= 4 { + now := metav1.Now() + h.DeletionTimestamp = &now + h.Finalizers = []string{helmFinalizer} + } + } + return nil + }, + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, false) { + unsuspendApplies++ + return errors.New("simulated unsuspend apply failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := newPlunger(c) + r.Recorder = rec + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a failed unsuspend on a terminating release must requeue with an error, got nil") + } + // The returned error requeues the retry, so the deferred fallback must not make a + // second, redundant attempt (symmetric with the final-unsuspend path). + if unsuspendApplies != 1 { + t.Fatalf("want exactly 1 unsuspend attempt on the terminating path, got %d", unsuspendApplies) + } + expectWarning(t, rec, "UnsuspendFailed") +} + +// If a delete arrives between the top-of-Reconcile deletion check and the +// re-fetch inside suspendHelmRelease, recovery must abort without suspending the +// now-terminating release or deleting its storage Secret. +func TestReconcile_AbortsWhenDeletedConcurrently(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + hrGets := 0 + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := cl.Get(ctx, key, obj, opts...); err != nil { + return err + } + if h, ok := obj.(*helmv2.HelmRelease); ok { + hrGets++ + if hrGets == 2 { + now := metav1.Now() + h.DeletionTimestamp = &now + h.Finalizers = []string{helmFinalizer} + } + } + return nil + }, + Patch: ssaModelApply, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("recovery must not suspend a release that started terminating, got suspend=true") + } + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(err) { + t.Fatalf("recovery must not delete the storage Secret of a release that started terminating") + } +} + +// A HelmRelease deleted while flux-plunger owns its suspension must have that +// suspension cleared, otherwise helm-controller v1.5.x skips the uninstall. +func TestReconcile_DeletionClearsPlungerSuspension(t *testing.T) { + hr := noDeployedReleasesHR() + ownedFixture(hr) + now := metav1.Now() + hr.DeletionTimestamp = &now + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("suspension must be cleared on a deleted HelmRelease so uninstall can proceed, got suspend=true") + } +} + +// flux-plunger must never begin recovery (suspend) on a HelmRelease that is +// already terminating. +func TestReconcile_DoesNotSuspendTerminatingHelmRelease(t *testing.T) { + hr := noDeployedReleasesHR() + now := metav1.Now() + hr.DeletionTimestamp = &now + + secret := helmReleaseSecret("v1") + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("flux-plunger must not suspend a terminating HelmRelease, got suspend=true") + } + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); err != nil { + t.Fatalf("flux-plunger must not touch the Helm storage Secret of a terminating release: %v", err) + } +} + +// A recovery step that fails after flux-plunger has suspended the release must +// not leave it suspended, or a later delete would skip the uninstall. +func TestReconcile_FailedStepDoesNotLeaveSuspended(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: ssaModelApply, + Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return errors.New("simulated secret delete failure") + } + return cl.Delete(ctx, obj, opts...) + }, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("a failed recovery step must clear the suspension flux-plunger owns, got suspend=true") + } +} + +// When a recovery step fails and the deferred unsuspend fails as well, flux-plunger +// is left holding a suspension it cannot clear: the state in which a delete orphans +// the release. That must surface as a Warning, not only a log line. +func TestReconcile_DeferredUnsuspendFailureEmitsWarning(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, false) { + return errors.New("simulated unsuspend apply failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return errors.New("simulated secret delete failure") + } + return cl.Delete(ctx, obj, opts...) + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := newPlunger(c) + r.Recorder = rec + + reconcile(t, r) + + expectWarning(t, rec, "UnsuspendFailed") +} + +// A suspension flux-plunger owns but never recorded progress for (recovery +// crashed right after suspending) must be released on a later reconcile. +func TestReconcile_ReleasesOwnedSuspensionWithoutProgress(t *testing.T) { + hr := noDeployedReleasesHR() + ownedFixture(hr) + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("a suspension flux-plunger owns with no recorded progress must be released, got suspend=true") + } +} + +// A suspension flux-plunger does not own must be left untouched even when a stale +// processed-version annotation happens to match the latest revision: the +// annotation is never an ownership signal, only the managedFields owner is. +func TestReconcile_LeavesExternalSuspensionWithStaleProcessedVersion(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Spec.Suspend = true // suspended by an operator: not flux-plunger-owned + hr.Annotations = map[string]string{annotationLastProcessedVersion: "5"} + secret := helmReleaseSecret("v4") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if !got.Spec.Suspend { + t.Fatalf("a stale processed-version match must not be treated as ownership; operator suspension was cleared") + } +} + +// Recovery must delete the LATEST (highest-version) storage Secret and record its +// version, even when several historical revision Secrets are present. +func TestReconcile_DeletesLatestOfMultipleSecrets(t *testing.T) { + hr := noDeployedReleasesHR() + oldest := helmReleaseSecret("v1") + middle := helmReleaseSecret("v2") + latest := helmReleaseSecret("v5") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, oldest, middle, latest). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: latest.Name}, &corev1.Secret{}); !apierrors.IsNotFound(err) { + t.Fatalf("recovery must delete the latest storage Secret (v5), err=%v", err) + } + for _, s := range []*corev1.Secret{oldest, middle} { + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: s.Name}, &corev1.Secret{}); err != nil { + t.Fatalf("recovery must not delete a non-latest Secret (%s): %v", s.Name, err) + } + } + got := getHR(t, c) + if got.Annotations[annotationLastProcessedVersion] != "5" { + t.Fatalf("recovery must record the latest processed version 5, got %q", got.Annotations[annotationLastProcessedVersion]) + } +} + +// A HelmRelease that does not carry the "has no deployed releases" error must be +// left entirely alone. +func TestReconcile_SkipsReleaseWithoutTargetError(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Status.Conditions[0].Message = "Helm upgrade failed: some unrelated error" + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("a release without the target error must not be suspended") + } + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(err) { + t.Fatalf("a release without the target error must not have its storage Secret deleted") + } +} + +// A HelmRelease with the target error but no Helm storage Secrets must be skipped +// cleanly (pins the empty-list guard, without which getLatestSecret would panic). +func TestReconcile_SkipsWhenNoStorageSecrets(t *testing.T) { + hr := noDeployedReleasesHR() + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) // must not panic + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("a release with no storage Secrets must not be suspended") + } +} + +// The "already processed" skip-guard: once a pass reclaimed a revision and +// recorded the processed version, the next reconcile (latestVersion+1 == +// processedVersion) must NOT suspend or delete another Secret. +func TestReconcile_SkipsWhenAlreadyProcessed(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Annotations = map[string]string{annotationLastProcessedVersion: "5"} + secret := helmReleaseSecret("v4") // 4+1 == 5 (already processed) + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("an already-processed release must not be suspended again, got suspend=true") + } + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(err) { + t.Fatalf("an already-processed release must not have another storage Secret deleted") + } +} + +// The converse of the skip-guard: a recorded version NOT exactly one above the +// latest surviving revision means recovery must proceed (pins the off-by-one). +func TestReconcile_ProcessesWhenProcessedVersionDoesNotMatch(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Annotations = map[string]string{annotationLastProcessedVersion: "5"} + secret := helmReleaseSecret("v5") // 5+1 == 6 != 5, so NOT already processed + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); !apierrors.IsNotFound(err) { + t.Fatalf("a non-matching processed version must not skip recovery; the latest Secret should have been deleted, err=%v", err) + } + got := getHR(t, c) + if got.Annotations[annotationLastProcessedVersion] != "5" { + t.Fatalf("recovery must record the processed version 5, got %q", got.Annotations[annotationLastProcessedVersion]) + } +} + +// Deleting a release an operator suspended (flux-plunger does not own it) must +// leave the suspension in place: the single most safety-relevant boundary, +// enforced by the caller gate and the ownership check inside unsuspendHelmRelease. +func TestReconcile_LeavesExternalSuspensionOnDeletion(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Spec.Suspend = true // operator-owned: not flux-plunger + now := metav1.Now() + hr.DeletionTimestamp = &now + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if !got.Spec.Suspend { + t.Fatalf("flux-plunger must not clear an operator's suspension on deletion, got suspend=false") + } +} + +// Releasing a stuck owned suspension (crash-recovery branch) must requeue on +// failure, uniform with the other unsuspend call sites. +func TestReconcile_SuspendedBranchRequeuesOnUnsuspendFailure(t *testing.T) { + hr := noDeployedReleasesHR() + ownedFixture(hr) + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, false) { + return errors.New("simulated unsuspend apply failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := newPlunger(c) + r.Recorder = rec + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a failed unsuspend in the crash-recovery branch must requeue with an error, got nil") + } + expectWarning(t, rec, "UnsuspendFailed") +} + +// A failing processed-version annotation write must requeue with an error. +func TestReconcile_RecoveryRequeuesOnAnnotationFailure(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + // Fail only the annotation patch (a merge patch carrying the + // processed-version annotation), not the suspend/unsuspend applies. + if h, ok := obj.(*helmv2.HelmRelease); ok && patch.Type() != types.ApplyPatchType && h.Annotations[annotationLastProcessedVersion] != "" { + return errors.New("simulated annotation patch failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a failed processed-version annotation write must requeue with an error, got nil") + } +} + +// A failing final unsuspend on the recovery path must requeue with an error. +func TestReconcile_RecoveryRequeuesOnUnsuspendFailure(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, false) { + return errors.New("simulated unsuspend apply failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := newPlunger(c) + r.Recorder = rec + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a failed final unsuspend on the recovery path must requeue with an error, got nil") + } + expectWarning(t, rec, "UnsuspendFailed") +} + +// On a terminating release owned by flux-plunger, a failed unsuspend must requeue. +func TestReconcile_DeletionRequeuesOnUnsuspendFailure(t *testing.T) { + hr := noDeployedReleasesHR() + ownedFixture(hr) + now := metav1.Now() + hr.DeletionTimestamp = &now + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, false) { + return errors.New("simulated unsuspend apply failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := newPlunger(c) + r.Recorder = rec + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a failed unsuspend on a terminating release must requeue with an error, got nil") + } + expectWarning(t, rec, "UnsuspendFailed") +} + +// A suspension flux-plunger does not own must be left untouched — including one an +// operator owns via a client-side Update of spec.suspend (managedFields shows the +// operator, not flux-plunger). This is the misattribution boundary, closed by the +// apiserver transferring the field away from flux-plunger (proven end to end in +// the envtest suite, V2/V3). +func TestReconcile_LeavesExternalSuspensionUntouched(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Spec.Suspend = true // operator-owned: no test ownership marker + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got := getHR(t, c) + if !got.Spec.Suspend { + t.Fatalf("flux-plunger must not clear a suspension it does not own, got suspend=false") + } +} + +// A delete that lands after flux-plunger acquires the suspension but before it +// deletes the storage Secret must abort the delete and clear the suspension. +func TestReconcile_AbortsWhenDeletedBeforeSecretDelete(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := cl.Get(ctx, key, obj, opts...); err != nil { + return err + } + // Inject a delete once the processed-version annotation this recovery + // just wrote is visible: that is the releaseIsTerminating re-fetch, right + // before the storage Secret delete. + if h, ok := obj.(*helmv2.HelmRelease); ok && + h.Annotations[annotationLastProcessedVersion] != "" && h.DeletionTimestamp == nil { + now := metav1.Now() + h.DeletionTimestamp = &now + h.Finalizers = []string{helmFinalizer} + } + return nil + }, + Patch: ssaModelApply, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(err) { + t.Fatalf("recovery must not delete the storage Secret of a release that started terminating before the delete") + } + got := getHR(t, c) + if got.Spec.Suspend { + t.Fatalf("the suspension flux-plunger acquired must be cleared once the release starts terminating, got suspend=true") + } +} + +// The processed-version annotation is recorded BEFORE the storage Secret is +// deleted, so a failed annotation write must leave the Secret intact. +func TestReconcile_AnnotationFailureLeavesSecretIntact(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if h, ok := obj.(*helmv2.HelmRelease); ok && patch.Type() != types.ApplyPatchType && h.Annotations[annotationLastProcessedVersion] != "" { + return errors.New("simulated annotation patch failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a failed processed-version write must requeue with an error, got nil") + } + if getErr := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(getErr) { + t.Fatalf("a failed annotation write must not delete the storage Secret (annotation is recorded before the delete)") + } +} + +// unsuspendHelmRelease's own ownership re-check (defense-in-depth for a future +// caller that forgets the caller-side gate): with suspend=true and no flux-plunger +// ownership, a direct call sends no apply at all and leaves the suspension. The +// apply count is what pins the guard; a relinquish could not clear another +// manager's value anyway, so the resulting object alone would not. +func TestUnsuspendHelmRelease_LeavesUnownedSuspension(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Spec.Suspend = true // operator-owned: no flux-plunger managedFields + + applies := 0 + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, _, _, ok := plungerApplyDetail(obj, patch, opts); ok { + applies++ + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + if err := r.unsuspendHelmRelease(context.Background(), hr); err != nil { + t.Fatalf("unsuspendHelmRelease must not error on an unowned suspension: %v", err) + } + + if applies != 0 { + t.Fatalf("unsuspendHelmRelease must not apply on a suspension flux-plunger does not own, sent %d", applies) + } + if got := getHR(t, c); !got.Spec.Suspend { + t.Fatalf("unsuspendHelmRelease must not clear a suspension flux-plunger does not own (callee-side guard)") + } +} + +// The watch predicate must wake a reconcile for the error state and for a +// flux-plunger-owned suspension, and must NOT wake for an operator-owned +// suspension or an unrelated release. This pins both terms of the predicate, +// which nothing exercised before (SetupWithManager was never called in a test). +func TestWatchPredicate(t *testing.T) { + r := &FluxPlunger{owns: testOwns} + withError := noDeployedReleasesHR() + ownedSuspended := ownedFixture(noDeployedReleasesHR()) + ownedSuspended.Status.Conditions = nil // no error, only an owned suspension + externalSuspended := noDeployedReleasesHR() + externalSuspended.Status.Conditions = nil + externalSuspended.Spec.Suspend = true // suspended, not owned + quiet := noDeployedReleasesHR() + quiet.Status.Conditions = nil // no error, not suspended + // Owned marker but NOT suspended and no error: isolates the hr.Spec.Suspend + // conjunct. Dropping it from the predicate would wake on ownsSuspension alone and + // this case would (wrongly) match; with it, a release that is not suspended never + // matches on the suspension arm. + ownedNotSuspended := noDeployedReleasesHR() + ownedNotSuspended.Status.Conditions = nil + ownedNotSuspended.Annotations = map[string]string{testOwnedKey: "true"} + // A healthy release an operator or a migration suspended, now being deleted: + // neither the error arm nor the owned arm matches, yet this is the orphan state the + // TerminatingWhileSuspended Warning exists for, so it must wake a reconcile. + now := metav1.Now() + terminatingExternal := noDeployedReleasesHR() + terminatingExternal.Status.Conditions = nil + terminatingExternal.Spec.Suspend = true + terminatingExternal.DeletionTimestamp = &now + terminatingUnsuspended := noDeployedReleasesHR() + terminatingUnsuspended.Status.Conditions = nil + terminatingUnsuspended.DeletionTimestamp = &now + // Only the fence: the status write helm-controller makes after the uninstall + // carries no other signal, and it is what lets flux-plunger lift the fence. + fencedOnly := fencedFixture(noDeployedReleasesHR()) + fencedOnly.Status.Conditions = nil + + cases := []struct { + name string + obj client.Object + want bool + }{ + {"has target error", withError, true}, + {"owned suspension, no error", ownedSuspended, true}, + {"external suspension is ignored", externalSuspended, false}, + {"no error and not suspended", quiet, false}, + {"owned marker but not suspended", ownedNotSuspended, false}, + {"terminating under an external suspension", terminatingExternal, true}, + {"terminating and not suspended", terminatingUnsuspended, false}, + {"fenced by flux-plunger", fencedOnly, true}, + {"non-HelmRelease object", &corev1.Secret{}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := r.watchPredicate(tc.obj); got != tc.want { + t.Fatalf("watchPredicate=%v want %v", got, tc.want) + } + }) + } +} + +// On the final-unsuspend-failure path recovery sets completed=true before +// returning the error, so the deferred fallback does NOT run a second, redundant +// unsuspend (the requeue covers the retry). Pin that: exactly one unsuspend apply +// is attempted. Removing the completed=true makes the deferred fallback fire and +// this count become 2. +func TestReconcile_FinalUnsuspendFailureDoesNotRetryDeferred(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + unsuspendApplies := 0 + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, false) { + unsuspendApplies++ + return errors.New("simulated unsuspend apply failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + _, _ = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if unsuspendApplies != 1 { + t.Fatalf("a failed final unsuspend must not trigger the deferred retry; want 1 unsuspend attempt, got %d", unsuspendApplies) + } +} + +// The pre-delete termination re-check must read the uncached APIReader, not the +// informer cache: the cache can lag a delete the apiserver already accepted, and a +// termination check that misses that delete would delete the storage Secret of a +// terminating release and orphan it. Here the cached client sees a live release +// while the APIReader sees the delete; releaseIsTerminating must report terminating. +func TestReleaseIsTerminating_ReadsAPIReaderNotCache(t *testing.T) { + cached := noDeployedReleasesHR() // cache: not terminating + fresh := noDeployedReleasesHR() + now := metav1.Now() + fresh.DeletionTimestamp = &now // apiserver: delete already accepted + fresh.Finalizers = []string{helmFinalizer} + + cachedClient := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(cached).Build() + apiReader := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(fresh).Build() + r := &FluxPlunger{Client: cachedClient, APIReader: apiReader, owns: testOwns} + + terminating, err := r.releaseIsTerminating(context.Background(), cached) + if err != nil { + t.Fatalf("releaseIsTerminating: %v", err) + } + if !terminating { + t.Fatalf("releaseIsTerminating must read the uncached APIReader (delete already accepted), not the stale cache") + } +} + +// The already-processed guard compares the processed-version annotation against the +// latest storage Secret, and the Secret list is read from the apiserver. If the +// annotation came from a lagging cache instead, a reconcile right after a finished +// recovery would see the fresh "latest" (the revision below the one just deleted) +// but the old annotation, fail the guard, and delete that revision too — walking the +// release history down one Secret per reconcile. Here the cache has no annotation +// while the apiserver records processed=5 over the surviving v4: recovery must not +// start, so no suspend apply may be sent. +func TestReconcile_ProcessedGuardReadsAPIReaderNotCache(t *testing.T) { + cached := noDeployedReleasesHR() // cache: annotation write not yet observed + fresh := noDeployedReleasesHR() + fresh.Annotations = map[string]string{annotationLastProcessedVersion: "5"} + secret := helmReleaseSecret("v4") + + applies := 0 + cachedClient := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(cached, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, true) { + applies++ + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + apiReader := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(fresh, secret).Build() + r := &FluxPlunger{Client: cachedClient, APIReader: apiReader, owns: testOwns} + + reconcile(t, r) + + if applies != 0 { + t.Fatalf("the processed guard must read the apiserver: v4 is already processed (annotation 5), yet %d suspend apply was sent", applies) + } +} + +// The storage-Secret List chooses which revision is deleted and which version the +// processed annotation records, so it must read the uncached APIReader like every +// other race-sensitive read: a cache lagging a just-created revision Secret would +// pick a stale "latest" and delete the wrong revision. Here the cache is empty while +// the APIReader sees the Secret; listHelmReleaseSecrets must return it. +func TestListHelmReleaseSecrets_ReadsAPIReaderNotCache(t *testing.T) { + secret := helmReleaseSecret("v1") + cachedClient := fake.NewClientBuilder().WithScheme(testScheme(t)).Build() // cache: no secret + apiReader := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(secret).Build() + r := &FluxPlunger{Client: cachedClient, APIReader: apiReader, owns: testOwns} + + got, err := r.listHelmReleaseSecrets(context.Background(), testNS, testName) + if err != nil { + t.Fatalf("listHelmReleaseSecrets: %v", err) + } + if len(got) != 1 { + t.Fatalf("listHelmReleaseSecrets must read the uncached APIReader (cache is empty); got %d secrets want 1", len(got)) + } +} + +// When flux-plunger declines to touch an externally-owned suspension it must emit +// an Event, so an operator can see and alert on it instead of grepping logs. +func TestReconcile_EmitsEventOnExternalSuspension(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Spec.Suspend = true // suspended by an operator: not flux-plunger-owned + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + rec := record.NewFakeRecorder(8) + r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec} + + reconcile(t, r) + + select { + case e := <-rec.Events: + if !strings.Contains(e, "SuspensionNotOwned") { + t.Fatalf("expected a SuspensionNotOwned event, got %q", e) + } + default: + t.Fatalf("declining an externally-owned suspension must emit an Event, got none") + } +} + +// A suspension flux-plunger does not own on a release flux-plunger has recovered +// before is most likely one an earlier flux-plunger left mid-recovery under the +// shared field manager: it stays suspended for good unless someone acts, so it +// is a Warning, while one on a release flux-plunger never touched stays Normal. +func TestReconcile_ExternalSuspensionOnARecoveredReleaseWarns(t *testing.T) { + for _, tc := range []struct { + name string + recovered bool + want string + }{ + {"recovered before", true, corev1.EventTypeWarning}, + {"never recovered", false, corev1.EventTypeNormal}, + } { + t.Run(tc.name, func(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Spec.Suspend = true + if tc.recovered { + hr.Annotations = map[string]string{annotationLastProcessedVersion: "3"} + } + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + rec := record.NewFakeRecorder(8) + r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec} + + reconcile(t, r) + + select { + case e := <-rec.Events: + if !strings.HasPrefix(e, tc.want+" SuspensionNotOwned") { + t.Fatalf("want a %s SuspensionNotOwned event, got %q", tc.want, e) + } + default: + t.Fatal("no event") + } + }) + } +} + +// A non-conflict error while acquiring the suspension must requeue with an error. +func TestReconcile_SuspendFailureRequeuesOnNonConflictError(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, true) { + return errors.New("simulated transient suspend failure") + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a non-conflict suspend failure must requeue with an error, got nil") + } +} + +// A conflict on the forced retry as well means another manager wrote the release +// again after the re-read: the same race as a foreign suspension caught on the +// unforced attempt, so it gets the same treatment: recovery is deferred with a +// RecoveryDeferred Event and no error, instead of a rate-limited backoff whose +// choice depends only on which of the two applies the write collided with. That +// write is itself an update event, so the next reconcile follows. Nothing is +// acquired, so the storage Secret stays intact. +func TestReconcile_ForcedAcquireConflictDefers(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)). + WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + // Conflict on every acquire apply, forced included: a foreign write lands + // before each attempt. + if isPlungerApply(obj, patch, opts, true) { + return apierrors.NewConflict( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("the object has been modified")) + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec} + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err != nil { + t.Fatalf("a conflict on the forced acquire must defer recovery, not requeue with an error: %v", err) + } + select { + case e := <-rec.Events: + if !strings.Contains(e, "RecoveryDeferred") { + t.Fatalf("expected a RecoveryDeferred event, got %q", e) + } + default: + t.Fatalf("a conflict on the forced acquire must emit RecoveryDeferred, got no event") + } + if getErr := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(getErr) { + t.Fatalf("a conflicted suspend acquires nothing, so it must not delete the storage Secret") + } +} + +// A non-conflict failure of the forced acquire (an admission webhook rejecting it, +// or an API error) produces no follow-up event, so it must requeue with an error +// rather than be treated as a deferral. +func TestReconcile_ForcedAcquireNonConflictErrorRequeues(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + conflicted := false + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + present, value, force, ok := plungerApplyDetail(obj, patch, opts) + if ok && present && value && !force && !conflicted { + conflicted = true + return apierrors.NewConflict( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("stale ownership at false")) + } + if ok && present && value && force { + return apierrors.NewForbidden( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("admission webhook denied the request")) + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a non-conflict failure of the forced acquire must requeue with an error, got nil") + } +} + +// Acquire re-fetches on conflict and forces ONLY over a field still read as false. +// Here the conflicting write suspended the release (an operator, or a backup +// mid-restore): the re-fetch sees suspend=true, so acquire must abort WITHOUT a +// forced apply and must not delete the storage Secret. +func TestReconcile_AcquireDoesNotForceActiveForeignSuspension(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + var sawForce bool + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + present, value, force, ok := plungerApplyDetail(obj, patch, opts) + if ok && present && value && force { + sawForce = true + } + // The unforced acquire loses the race: a foreign manager suspends the + // release, then our apply conflicts. Model both: write suspend=true to the + // store so the post-conflict re-fetch sees it, then return the conflict. + if ok && present && value && !force { + cur := &helmv2.HelmRelease{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(obj), cur); err != nil { + return err + } + cur.Spec.Suspend = true + if err := cl.Update(ctx, cur); err != nil { + return err + } + return apierrors.NewConflict( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("foreign suspend won the race")) + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if sawForce { + t.Fatalf("acquire must not force over a suspension it re-read as active (true)") + } + if getErr := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(getErr) { + t.Fatalf("an aborted acquire must not delete the storage Secret") + } +} + +// The complementary case: the conflict came from a manager owning spec.suspend at +// FALSE (a backup controller that resumed, or an earlier flux-plunger release). The re-fetch +// sees suspend=false, so acquire forces over the stale ownership and recovery +// proceeds — the storage Secret is deleted. +func TestReconcile_AcquireForcesStaleForeignSuspension(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + var sawForce bool + conflicted := false + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + present, value, force, ok := plungerApplyDetail(obj, patch, opts) + // The unforced acquire conflicts once (foreign manager owns the field at + // false); the value is left false, so the re-fetch reads false. + if ok && present && value && !force && !conflicted { + conflicted = true + return apierrors.NewConflict( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("stale ownership at false")) + } + if ok && present && value && force { + sawForce = true + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if !sawForce { + t.Fatalf("acquire must force over stale ownership re-read as false; no forced apply seen") + } + if getErr := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); !apierrors.IsNotFound(getErr) { + t.Fatalf("after forcing acquisition recovery must delete the storage Secret") + } +} + +// Both acquire applies, the unforced first attempt and the forced retry, must carry +// the resourceVersion of the read that decided spec.suspend was false, so the +// apiserver rejects either with a conflict if another manager wrote the release in +// between. Without it the unforced apply of true over a foreign true is accepted as +// shared ownership and flux-plunger proceeds as if it had acquired a suspension a +// restore is holding. Pin that each is preconditioned on the stored resourceVersion. +func TestReconcile_AcquireAppliesCarryResourceVersion(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + conflicted := false + var sawUnforced, sawForce bool + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + present, value, force, ok := plungerApplyDetail(obj, patch, opts) + if ok && present && value { + cur := &helmv2.HelmRelease{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(obj), cur); err != nil { + return err + } + if got := obj.GetResourceVersion(); got == "" || got != cur.ResourceVersion { + t.Fatalf("acquire apply (force=%v) must be preconditioned on the read resourceVersion %q, got %q", force, cur.ResourceVersion, got) + } + if !controllerutil.ContainsFinalizer(obj, fenceFinalizer) { + t.Fatalf("acquire apply (force=%v) must carry the fence finalizer, got %v", force, obj.GetFinalizers()) + } + } + if ok && present && value && !force && !conflicted { + sawUnforced = true + conflicted = true + return apierrors.NewConflict( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("stale ownership at false")) + } + if ok && present && value && force { + sawForce = true + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if !sawUnforced || !sawForce { + t.Fatalf("expected an unforced then a forced acquire; unforced=%v forced=%v", sawUnforced, sawForce) + } +} + +// A delete that lands while the unforced acquire is conflicting must stop the +// forced retry: suspending a terminating release is what makes helm-controller skip +// the uninstall. The re-read after the conflict sees the deletionTimestamp, so no +// forced apply may be sent and the storage Secret stays intact. +func TestReconcile_AcquireAbortsWhenTerminatingAfterConflict(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + var sawForce bool + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + present, value, force, ok := plungerApplyDetail(obj, patch, opts) + if ok && present && value && force { + sawForce = true + } + if ok && present && value && !force { + // The release is deleted (it keeps helm-controller's finalizer, so it + // becomes terminating) and the unforced acquire conflicts. + cur := &helmv2.HelmRelease{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(obj), cur); err != nil { + return err + } + if err := cl.Delete(ctx, cur); err != nil { + return err + } + return apierrors.NewConflict( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("stale ownership at false")) + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if sawForce { + t.Fatalf("acquire must not force a suspension onto a release that began terminating during the conflict") + } + if getErr := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(getErr) { + t.Fatalf("an aborted acquire must not delete the storage Secret") + } +} + +// A release terminating while suspended by a manager flux-plunger does NOT own is +// the orphan state itself; flux-plunger cannot clear it but must surface it with a +// Warning Event, not only a log line, so an operator can intervene. +func TestReconcile_EmitsEventOnTerminatingExternalSuspension(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Spec.Suspend = true // suspended by an external manager: not flux-plunger-owned + now := metav1.Now() + hr.DeletionTimestamp = &now + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + rec := record.NewFakeRecorder(8) + r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec} + + reconcile(t, r) + + select { + case e := <-rec.Events: + if !strings.Contains(e, "TerminatingWhileSuspended") { + t.Fatalf("expected a TerminatingWhileSuspended event, got %q", e) + } + default: + t.Fatalf("a terminating release under an external suspension must emit an Event, got none") + } +} + +// When recovery is deferred because the suspension was taken by another manager +// between read and write, flux-plunger must emit an Event, symmetric with the +// externally-owned-suspension branch, not only log. +func TestReconcile_EmitsEventOnDeferredRecovery(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + // The unforced acquire loses the race to a foreign suspension. + if present, value, force, ok := plungerApplyDetail(obj, patch, opts); ok && present && value && !force { + cur := &helmv2.HelmRelease{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(obj), cur); err != nil { + return err + } + cur.Spec.Suspend = true + if err := cl.Update(ctx, cur); err != nil { + return err + } + return apierrors.NewConflict( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("foreign suspend won the race")) + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec} + + reconcile(t, r) + + select { + case e := <-rec.Events: + if !strings.Contains(e, "RecoveryDeferred") { + t.Fatalf("expected a RecoveryDeferred event, got %q", e) + } + default: + t.Fatalf("a deferred recovery must emit an Event, got none") + } +} + +// fencedFixture marks a HelmRelease as held by flux-plunger's fence finalizer. +func fencedFixture(hr *helmv2.HelmRelease) *helmv2.HelmRelease { + controllerutil.AddFinalizer(hr, fenceFinalizer) + return hr +} + +// terminatingFixture marks a HelmRelease as deleted; the fake client keeps it only +// while a finalizer remains. +func terminatingFixture(hr *helmv2.HelmRelease) *helmv2.HelmRelease { + now := metav1.Now() + hr.DeletionTimestamp = &now + return hr +} + +// hrExists reports whether the test HelmRelease is still stored. +func hrExists(t *testing.T, c client.Client) (*helmv2.HelmRelease, bool) { + t.Helper() + hr := &helmv2.HelmRelease{} + err := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: testName}, hr) + if apierrors.IsNotFound(err) { + return nil, false + } + if err != nil { + t.Fatalf("get HelmRelease: %v", err) + } + return hr, true +} + +// The acquire must set the fence in the same apply as the suspension, on both +// attempts, so there is no moment at which flux-plunger holds a suspension a delete +// could slip past. A completed recovery lifts both together. +func TestReconcile_AcquireSetsFenceWithSuspension(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + acquires := 0 + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if present, value, _, ok := plungerApplyDetail(obj, patch, opts); ok && present && value { + acquires++ + if !controllerutil.ContainsFinalizer(obj, fenceFinalizer) { + t.Fatalf("the acquire apply must carry the fence finalizer, got finalizers %v", obj.GetFinalizers()) + } + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if acquires == 0 { + t.Fatalf("expected an acquire apply") + } + got, ok := hrExists(t, c) + if !ok { + t.Fatalf("a completed recovery must leave the release in place") + } + if got.Spec.Suspend || controllerutil.ContainsFinalizer(got, fenceFinalizer) { + t.Fatalf("a completed recovery must lift both the suspension and the fence; suspend=%v finalizers=%v", got.Spec.Suspend, got.Finalizers) + } +} + +// A delete on a release flux-plunger holds suspended must lift the suspension but +// keep the fence: helm-controller may already have dropped its own finalizer +// without an uninstall, and the fence is what keeps the object until it reconciles +// the unsuspended release. +func TestReconcile_TerminatingOwnedSuspensionKeepsFence(t *testing.T) { + hr := terminatingFixture(fencedFixture(ownedFixture(noDeployedReleasesHR()))) + // The state observed on a live helm-controller v1.5.0 right after it handled the + // delete of the suspended release: its finalizer is gone and it has observed the + // current generation, yet nothing was uninstalled. The fence must not be lifted + // on that; only the suspension is. + hr.Finalizers = []string{fenceFinalizer} + hr.Generation = 3 + hr.Status.ObservedGeneration = 3 + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got, ok := hrExists(t, c) + if !ok { + t.Fatalf("the fence must keep a terminating release until the uninstall runs") + } + if got.Spec.Suspend { + t.Fatalf("the suspension must be lifted so helm-controller uninstalls") + } + if !controllerutil.ContainsFinalizer(got, fenceFinalizer) { + t.Fatalf("the fence must stay until helm-controller has handled the unsuspended release") + } +} + +// uninstallReached is the condition for lifting the fence: helm-controller's +// finalizer gone AND the latest generation observed. Each conjunct on its own is +// not enough: the finalizer is also dropped, without an uninstall, while the +// release is suspended. +func TestUninstallReached(t *testing.T) { + mk := func(helmFinalizer bool, gen, observed int64) *helmv2.HelmRelease { + hr := noDeployedReleasesHR() + hr.Finalizers = nil + if helmFinalizer { + hr.Finalizers = []string{helmv2.HelmReleaseFinalizer} + } + hr.Generation = gen + hr.Status.ObservedGeneration = observed + return hr + } + cases := []struct { + name string + hr *helmv2.HelmRelease + want bool + }{ + {"finalizer gone and generation observed", mk(false, 4, 4), true}, + {"finalizer gone, unsuspended generation not yet observed", mk(false, 4, 3), false}, + {"generation observed, finalizer still held", mk(true, 4, 4), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := uninstallReached(tc.hr); got != tc.want { + t.Fatalf("uninstallReached=%v want %v", got, tc.want) + } + }) + } +} + +// Once helm-controller has handled the unsuspended release the fence is lifted and +// the release goes; until then it stays. +func TestReconcile_LiftsFenceOnlyOnceUninstallReached(t *testing.T) { + cases := []struct { + name string + observed int64 + wantGone bool + }{ + {"uninstall reached", 4, true}, + {"unsuspended generation not yet observed", 3, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hr := terminatingFixture(noDeployedReleasesHR()) + hr.Finalizers = []string{fenceFinalizer} + hr.Generation = 4 + hr.Status.ObservedGeneration = tc.observed + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + if _, exists := hrExists(t, c); exists == tc.wantGone { + t.Fatalf("release exists=%v, want gone=%v", exists, tc.wantGone) + } + }) + } +} + +// A release being deleted under another manager's suspension is that manager's +// decision (a migration suspends then deletes to leave the resources in place): +// flux-plunger must not keep the object with its fence, must leave the suspension +// alone, and records it. +func TestReconcile_TerminatingForeignSuspensionReleasesFence(t *testing.T) { + hr := terminatingFixture(fencedFixture(noDeployedReleasesHR())) + hr.Spec.Suspend = true // not flux-plunger's + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + rec := record.NewFakeRecorder(8) + r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec} + + reconcile(t, r) + + got, ok := hrExists(t, c) + if !ok { + t.Fatalf("helm-controller's finalizer still holds the release in this fixture") + } + if controllerutil.ContainsFinalizer(got, fenceFinalizer) { + t.Fatalf("the fence must not hold a release under another manager's suspension") + } + if !got.Spec.Suspend { + t.Fatalf("another manager's suspension must be left alone") + } + select { + case e := <-rec.Events: + if !strings.HasPrefix(e, corev1.EventTypeNormal+" TerminatingWhileSuspended ") { + t.Fatalf("expected a Normal TerminatingWhileSuspended event, got %q", e) + } + default: + t.Fatalf("expected a TerminatingWhileSuspended event, got none") + } +} + +// A fenced, terminating, unsuspended release that helm-controller has not handled +// yet is held by flux-plunger's fence. That hold must not be silent: the reconcile +// requeues itself so it is re-checked without waiting for an event, and once the +// delete has been held past the deadline it raises a Warning an operator can find +// before a `kubectl delete` that never returns. +func TestReconcile_HeldFenceRequeuesAndWarnsPastTheDeadline(t *testing.T) { + deletedAt := time.Date(2026, 9, 25, 10, 0, 0, 0, time.UTC) + cases := []struct { + name string + since time.Duration + wantWarn bool + }{ + {"within the deadline", time.Minute, false}, + {"past the deadline", fenceHeldWarnAfter + time.Minute, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hr := noDeployedReleasesHR() + hr.Finalizers = []string{fenceFinalizer} + hr.DeletionTimestamp = &metav1.Time{Time: deletedAt} + hr.Generation = 4 + hr.Status.ObservedGeneration = 3 // the unsuspended generation not yet handled + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + rec := record.NewFakeRecorder(8) + r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec, + now: func() time.Time { return deletedAt.Add(tc.since) }} + + res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if res.RequeueAfter <= 0 { + t.Fatalf("a held fence must requeue itself, got %+v", res) + } + warned := false + for drained := false; !drained; { + select { + case e := <-rec.Events: + if strings.HasPrefix(e, corev1.EventTypeWarning+" UninstallFenceHeld ") { + warned = true + } + default: + drained = true + } + } + if warned != tc.wantWarn { + t.Fatalf("UninstallFenceHeld Warning=%v, want %v", warned, tc.wantWarn) + } + if _, ok := hrExists(t, c); !ok { + t.Fatalf("the fence must keep holding the release; warning is not lifting it") + } + }) + } +} + +// The same terminating-under-a-foreign-suspension state is two different things. +// A migration that suspends then deletes chose to keep the resources, so it is +// Normal. A release flux-plunger has recovered before (it carries the +// processed-version annotation: an operator suspended it by hand and deleted it, +// or an earlier flux-plunger release left it suspended) is losing its uninstall by +// accident, the failure this controller exists to prevent, so it is a Warning. +func TestReconcile_TerminatingForeignSuspensionWarnsOnlyForAPreviouslyRecoveredRelease(t *testing.T) { + cases := []struct { + name string + annotated bool + wantType string + }{ + {"never recovered by flux-plunger (migration)", false, corev1.EventTypeNormal}, + {"recovered by flux-plunger before", true, corev1.EventTypeWarning}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hr := terminatingFixture(noDeployedReleasesHR()) + hr.Spec.Suspend = true // not flux-plunger's + if tc.annotated { + hr.Annotations = map[string]string{annotationLastProcessedVersion: "3"} + } + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + rec := record.NewFakeRecorder(8) + r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec} + + reconcile(t, r) + + select { + case e := <-rec.Events: + if !strings.HasPrefix(e, tc.wantType+" TerminatingWhileSuspended ") { + t.Fatalf("expected a %s TerminatingWhileSuspended event, got %q", tc.wantType, e) + } + default: + t.Fatalf("expected a TerminatingWhileSuspended event, got none") + } + }) + } +} + +// A fence left without a suspension on a live release (another writer set +// spec.suspend=false and took the field), or a fence whose suspension another +// manager took over, guards nothing and must be lifted, without starting a recovery. +func TestReconcile_ReleasesUnguardingFence(t *testing.T) { + cases := []struct { + name string + suspend bool + }{ + {"fence without a suspension", false}, + {"fence under another manager's suspension", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hr := fencedFixture(noDeployedReleasesHR()) + hr.Spec.Suspend = tc.suspend + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + r := newPlunger(c) + + reconcile(t, r) + + got, _ := hrExists(t, c) + if controllerutil.ContainsFinalizer(got, fenceFinalizer) { + t.Fatalf("a fence that guards nothing must be lifted") + } + if got.Spec.Suspend != tc.suspend { + t.Fatalf("lifting the fence must not change spec.suspend; got %v want %v", got.Spec.Suspend, tc.suspend) + } + if getErr := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(getErr) { + t.Fatalf("lifting a stale fence must not also run a recovery") + } + }) + } +} + +// Every apply flux-plunger sends, the relinquish included, must carry the uid and +// resourceVersion of the object it was decided on: the resourceVersion so a write in +// between is rejected rather than absorbed, the uid so an apply to a release that +// has gone cannot create an empty one. +func TestReconcile_EveryApplyCarriesPreconditions(t *testing.T) { + hr := noDeployedReleasesHR() + hr.UID = "release-uid" + secret := helmReleaseSecret("v1") + + applies := 0 + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, _, _, ok := plungerApplyDetail(obj, patch, opts); ok { + applies++ + cur := &helmv2.HelmRelease{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(obj), cur); err != nil { + return err + } + if obj.GetUID() != "release-uid" { + t.Fatalf("apply must carry the release uid, got %q", obj.GetUID()) + } + if obj.GetResourceVersion() == "" || obj.GetResourceVersion() != cur.ResourceVersion { + t.Fatalf("apply must carry the read resourceVersion %q, got %q", cur.ResourceVersion, obj.GetResourceVersion()) + } + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + r := newPlunger(c) + + reconcile(t, r) + + if applies < 2 { + t.Fatalf("expected the acquire and the relinquish applies, got %d", applies) + } +} + +// Every flux-plunger apply is preconditioned on the object it was decided on, so a +// routine write in between (helm-controller updating the status) makes the final +// unsuspend conflict. That is retried through the requeue, not a failure to clear +// the suspension: it must requeue, but must not raise the UnsuspendFailed Warning. +func TestReconcile_UnsuspendConflictRequeuesWithoutWarning(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if isPlungerApply(obj, patch, opts, false) { + return apierrors.NewConflict( + schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"}, + testName, errors.New("the object has been modified")) + } + return ssaModelApply(ctx, cl, obj, patch, opts...) + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := newPlunger(c) + r.Recorder = rec + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err == nil { + t.Fatalf("a conflicted unsuspend must requeue with an error") + } + for drained := false; !drained; { + select { + case e := <-rec.Events: + if strings.Contains(e, "UnsuspendFailed") { + t.Fatalf("a conflicted unsuspend is retried, not a failure to clear; got %q", e) + } + default: + drained = true + } + } +} + +// A release that is gone by the pre-delete termination re-check counts as +// terminating: its storage Secret must be left alone and the reconcile must not +// requeue with an error for an object no reconcile will follow. +func TestReconcile_ReleaseGoneBeforeSecretDeleteKeepsSecret(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if err := ssaModelApply(ctx, cl, obj, patch, opts...); err != nil { + return err + } + // Right after the processed-version annotation is written, the release + // is removed outright. + if h, ok := obj.(*helmv2.HelmRelease); ok && patch.Type() != types.ApplyPatchType && h.Annotations[annotationLastProcessedVersion] != "" { + gone := &helmv2.HelmRelease{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(obj), gone); err != nil { + return err + } + gone.Finalizers = nil + if err := cl.Update(ctx, gone); err != nil { + return err + } + return client.IgnoreNotFound(cl.Delete(ctx, gone)) + } + return nil + }, + }).Build() + r := newPlunger(c) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err != nil { + t.Fatalf("a release gone before the Secret delete must not requeue with an error: %v", err) + } + if getErr := c.Get(context.Background(), types.NamespacedName{Namespace: testNS, Name: secret.Name}, &corev1.Secret{}); apierrors.IsNotFound(getErr) { + t.Fatalf("a release gone before the Secret delete must keep its storage Secret") + } +} + +// A release that disappears in the middle of a recovery has nothing left to clear: +// the unsuspend must not report a failure it will "retry", since no reconcile will +// follow for an object that is gone. +func TestReconcile_ReleaseGoneMidRecoveryIsNotAnUnsuspendFailure(t *testing.T) { + hr := noDeployedReleasesHR() + secret := helmReleaseSecret("v1") + + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr, secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: ssaModelApply, + Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if err := cl.Delete(ctx, obj, opts...); err != nil { + return err + } + if _, ok := obj.(*corev1.Secret); ok { + // The release is removed outright right after its storage Secret. + gone := &helmv2.HelmRelease{} + if err := cl.Get(ctx, types.NamespacedName{Namespace: testNS, Name: testName}, gone); err != nil { + return err + } + gone.Finalizers = nil + if err := cl.Update(ctx, gone); err != nil { + return err + } + return client.IgnoreNotFound(cl.Delete(ctx, gone)) + } + return nil + }, + }).Build() + rec := record.NewFakeRecorder(8) + r := newPlunger(c) + r.Recorder = rec + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNS, Name: testName}}) + if err != nil { + t.Fatalf("a release gone mid-recovery must not requeue with an error: %v", err) + } + for drained := false; !drained; { + select { + case e := <-rec.Events: + if strings.Contains(e, "UnsuspendFailed") { + t.Fatalf("a release gone mid-recovery is not an unsuspend failure, got %q", e) + } + default: + drained = true + } + } +} diff --git a/internal/controller/fluxplunger/hook_contract_test.go b/internal/controller/fluxplunger/hook_contract_test.go new file mode 100644 index 000000000..1c9b0559f --- /dev/null +++ b/internal/controller/fluxplunger/hook_contract_test.go @@ -0,0 +1,111 @@ +package fluxplunger + +import ( + "os" + "regexp" + "strconv" + "strings" + "testing" + "time" +) + +// The chart's pre-delete hook restates two of this package's names in shell and +// go-template, where nothing ties them to the Go constants: the field manager +// whose Apply entry owns a suspension, and the fence finalizer. A rename on +// either side alone would leave the hook removing fences while keeping +// suspensions, or looking for a fence nobody sets. These tests guard the +// spellings only; what the hook's predicates mean is held by +// hack/flux-plunger-fence-hook_test.bats. +const ( + hookTemplate = "../../../packages/system/flux-plunger/templates/uninstall-fence-cleanup.yaml" + deploymentTemplate = "../../../packages/system/flux-plunger/templates/deployment.yaml" +) + +// The binary must run the manager through RunManager, so the drain runs on stop, +// and size the Lease with ApplyShutdownTimings, so the next flux-plunger cannot +// lead during it; dropping either from main.go leaves every other test green. +func TestMain_RunsTheDrainUnderTheLease(t *testing.T) { + raw, err := os.ReadFile("../../../cmd/flux-plunger/main.go") + if err != nil { + t.Fatalf("read main.go: %v", err) + } + for _, want := range []string{"fluxplunger.RunManager(", "fluxplunger.ApplyShutdownTimings(", "mgr.GetAPIReader()"} { + if !strings.Contains(string(raw), want) { + t.Errorf("main.go does not call %q", want) + } + } + if strings.Contains(string(raw), "mgr.Start(") { + t.Error("main.go starts the manager directly, skipping the drain") + } +} + +// The chart must turn leader election on: without it nothing keeps the next +// flux-plunger from writing while the old one drains. +func TestDeploymentTemplate_EnablesLeaderElection(t *testing.T) { + raw, err := os.ReadFile(deploymentTemplate) + if err != nil { + t.Fatalf("read %s: %v", deploymentTemplate, err) + } + if !strings.Contains(string(raw), "- --leader-elect\n") && !strings.Contains(string(raw), "- --leader-elect=true") { + t.Error("deployment does not pass --leader-elect") + } +} + +// The chart sizes the drain as the grace period less what the manager's stop and +// the margin before SIGKILL take; both live here, so the subtraction must match. +func TestDeploymentTemplate_SubtractsTheStopAndTheMarginTheBinaryUses(t *testing.T) { + raw, err := os.ReadFile(deploymentTemplate) + if err != nil { + t.Fatalf("read %s: %v", deploymentTemplate, err) + } + m := regexp.MustCompile(`--` + ShutdownDrainTimeoutFlag + `=\{\{ sub \.Values\.shutdownGracePeriodSeconds (\d+) \}\}s`).FindStringSubmatch(string(raw)) + if m == nil { + t.Fatal("deployment does not derive the drain timeout from shutdownGracePeriodSeconds") + } + want := int((managerStopTimeout + killMargin) / time.Second) + if m[1] != strconv.Itoa(want) { + t.Fatalf("deployment subtracts %s, the binary needs %d (manager stop %v + margin %v)", m[1], want, managerStopTimeout, killMargin) + } +} + +// The chart passes the drain deadline as a flag; a name the binary does not +// define makes flux-plunger exit on start. +func TestDeploymentTemplate_PassesTheDrainFlagTheBinaryDefines(t *testing.T) { + raw, err := os.ReadFile(deploymentTemplate) + if err != nil { + t.Fatalf("read %s: %v", deploymentTemplate, err) + } + if want := "- --" + ShutdownDrainTimeoutFlag + "="; !strings.Contains(string(raw), want) { + t.Errorf("deployment does not pass %q", want) + } +} + +func TestHookTemplate_UsesTheControllersNames(t *testing.T) { + raw, err := os.ReadFile(hookTemplate) + if err != nil { + t.Fatalf("read %s: %v", hookTemplate, err) + } + hook := string(raw) + + for _, want := range []string{ + `(eq .manager "` + suspendFieldManager + `") (eq .operation "Apply")`, + `--field-manager ` + suspendFieldManager, + `fence=` + fenceFinalizer, + } { + if !strings.Contains(hook, want) { + t.Errorf("hook does not contain %q", want) + } + } + + // Every flux-plunger finalizer literal in the templates is the controller's + // fence, whole: matched by the domain, so a misspelt name is caught too. + literals := regexp.MustCompile(`"[^"\s]*cozystack\.io/[^"\s]*"`).FindAllString(hook, -1) + if len(literals) == 0 { + t.Fatal("hook names no fence finalizer in a template") + } + for _, l := range literals { + if l != `"`+fenceFinalizer+`"` { + t.Errorf("hook template names fence %s, controller sets %q", l, fenceFinalizer) + } + } +} diff --git a/internal/controller/fluxplunger/shutdown_test.go b/internal/controller/fluxplunger/shutdown_test.go new file mode 100644 index 000000000..9ed044892 --- /dev/null +++ b/internal/controller/fluxplunger/shutdown_test.go @@ -0,0 +1,265 @@ +// SPDX-License-Identifier: Apache-2.0 + +package fluxplunger + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + helmv2 "github.com/fluxcd/helm-controller/api/v2" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +// countingReader counts List calls and fails the first failLists of them. +type countingReader struct { + client.Reader + lists, failLists int +} + +func (r *countingReader) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + r.lists++ + if r.lists <= r.failLists { + return errors.New("etcdserver: leader changed") + } + return r.Reader.List(ctx, list, opts...) +} + +type stubManager struct { + client client.Client + reader client.Reader + elected chan struct{} +} + +func (m stubManager) Start(context.Context) error { return nil } +func (m stubManager) GetClient() client.Client { return m.client } +func (m stubManager) GetAPIReader() client.Reader { return m.reader } +func (m stubManager) Elected() <-chan struct{} { return m.elected } + +// electedChan is the Elected channel of a manager that became leader. +func electedChan() chan struct{} { + c := make(chan struct{}) + close(c) + return c +} + +// A flux-plunger that never became leader stops while the leader goes on; its +// drain would write next to the leader. +func TestRunManager_DoesNotDrainWithoutHavingLed(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(fencedLiveRelease()). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + reader := &countingReader{Reader: c} + if err := RunManager(context.Background(), stubManager{client: c, reader: reader, elected: make(chan struct{})}, 10*time.Second); err != nil { + t.Fatalf("RunManager: %v", err) + } + if reader.lists != 0 { + t.Fatal("a flux-plunger that never led must not drain") + } + hr := &helmv2.HelmRelease{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(fencedLiveRelease()), hr); err != nil { + t.Fatalf("get: %v", err) + } + if !controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + t.Fatal("the leader's fence must be left to the leader") + } +} + +func fencedLiveRelease() *helmv2.HelmRelease { + return &helmv2.HelmRelease{ObjectMeta: metav1.ObjectMeta{ + Namespace: testNS, Name: testName, Finalizers: []string{fenceFinalizer}, + }} +} + +// After the manager stops its informer cache is frozen, so the drain must read +// through the manager's API reader; a drain reading the cache would act on +// fences already gone. +func TestRunManager_DrainsThroughTheManagersAPIReader(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(fencedLiveRelease()). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + reader := &countingReader{Reader: c} + if err := RunManager(context.Background(), stubManager{client: c, reader: reader, elected: electedChan()}, 10*time.Second); err != nil { + t.Fatalf("RunManager: %v", err) + } + if reader.lists == 0 { + t.Fatal("the drain did not read through the API reader") + } + hr := &helmv2.HelmRelease{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(fencedLiveRelease()), hr); err != nil { + t.Fatalf("get: %v", err) + } + if controllerutil.ContainsFinalizer(hr, fenceFinalizer) { + t.Fatalf("the drain must release the fence, finalizers %v", hr.Finalizers) + } +} + +func TestDrainFences_RetriesAFailedListing(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(fencedLiveRelease()). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + reader := &countingReader{Reader: c, failLists: 2} + r := &FluxPlunger{Client: c, APIReader: reader} + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + left, err := r.DrainFences(ctx) + if err != nil || left != 0 { + t.Fatalf("a listing that fails and then succeeds must not end the drain: left=%d err=%v lists=%d", left, err, reader.lists) + } + if reader.lists < 3 { + t.Fatalf("want the listing retried, lists=%d", reader.lists) + } +} + +func TestRunThenDrain_DrainsAfterTheManagerStopsWithItsOwnDeadline(t *testing.T) { + stopped, drained := false, false + signal, cancel := context.WithCancel(context.Background()) + run := func(ctx context.Context) error { + cancel() // the stop signal + <-ctx.Done() + stopped = true + return nil + } + drain := func(ctx context.Context) (int, error) { + drained = true + if !stopped { + t.Error("drain ran before the manager stopped") + } + // The signal context is cancelled by now; the drain must get time of its own. + if err := ctx.Err(); err != nil { + t.Errorf("drain context was already done: %v", err) + } + dl, ok := ctx.Deadline() + if !ok || time.Until(dl) > time.Minute || time.Until(dl) < 50*time.Second { + t.Errorf("drain deadline %v, want about a minute from now", dl) + } + return 0, nil + } + + if err := RunThenDrain(signal, run, drain, time.Minute); err != nil { + t.Fatalf("RunThenDrain: %v", err) + } + if !drained { + t.Fatal("drain did not run") + } +} + +// The next flux-plunger may become leader only once the old one has stopped its +// manager and finished draining, counted from the old one's last Lease renewal. +func TestApplyShutdownTimings_TheLeaseOutlastsTheStopAndTheDrain(t *testing.T) { + for _, drain := range []time.Duration{2 * time.Second, 60 * time.Second, 110 * time.Second} { + opts := ctrl.Options{} + ApplyShutdownTimings(&opts, drain) + if opts.LeaderElectionReleaseOnCancel { + t.Fatalf("drain %v: a Lease released on stop lets the next flux-plunger lead during the drain", drain) + } + if opts.GracefulShutdownTimeout == nil || opts.LeaseDuration == nil || opts.RenewDeadline == nil || opts.RetryPeriod == nil { + t.Fatalf("drain %v: timings left unset: %+v", drain, opts) + } + stop, lease, renew, retry := *opts.GracefulShutdownTimeout, *opts.LeaseDuration, *opts.RenewDeadline, *opts.RetryPeriod + if stop <= 0 { + t.Fatalf("drain %v: the manager's stop must be bounded, got %v", drain, stop) + } + // A leader whose renewals fail keeps acting until RenewDeadline has + // passed since its last renewal, so that counts too. + if lease <= retry+renew+stop+drain { + t.Fatalf("drain %v: lease %v does not outlast a renewal %v ago, a renew deadline of %v, a stop of %v and the drain", drain, lease, retry, renew, stop) + } + if !(renew < lease && time.Duration(float64(retry)*1.2) < renew) { + t.Fatalf("drain %v: client-go needs lease > renew > 1.2 * retry, got %v %v %v", drain, lease, renew, retry) + } + } +} + +// controller-runtime reports a stop that overran GracefulShutdownTimeout as an +// error from Start. A reconcile slow to notice its cancelled context must not +// cost the drain the fence exists for; the exit stays non-zero. +func TestRunThenDrain_DrainsAfterAStopThatOverranItsGracePeriod(t *testing.T) { + stopErr := fmt.Errorf("failed waiting for all runnables to end within grace period of 10s: %w", context.DeadlineExceeded) + drained := false + err := RunThenDrain(context.Background(), + func(context.Context) error { return stopErr }, + func(context.Context) (int, error) { drained = true; return 0, nil }, + time.Minute) + if !drained { + t.Fatal("a stop that overran its grace period must still drain") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("the overrun must still be reported, got %v", err) + } +} + +// The drain's Events cannot go through the manager's recorder, which is stopped +// before Start returns; they are written directly. +func TestRunManager_DrainEventsReachTheAPI(t *testing.T) { + now := metav1.Now() + hr := fencedLiveRelease() + hr.Spec.Suspend = true + hr.DeletionTimestamp = &now + hr.Finalizers = append(hr.Finalizers, "example.com/keep") + c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr). + WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build() + if err := RunManager(context.Background(), stubManager{client: c, reader: c, elected: electedChan()}, 5*time.Second); err != nil { + t.Fatalf("RunManager: %v", err) + } + events := &corev1.EventList{} + if err := c.List(context.Background(), events); err != nil { + t.Fatalf("list events: %v", err) + } + for _, e := range events.Items { + if e.Reason == "TerminatingWhileSuspended" && e.InvolvedObject.Name == hr.Name && e.InvolvedObject.Namespace == hr.Namespace { + return + } + } + t.Fatalf("the drain's event did not reach the API: %+v", events.Items) +} + +func TestRunThenDrain_DoesNotDrainWhenTheManagerFails(t *testing.T) { + boom := errors.New("boom") + drained := false + err := RunThenDrain(context.Background(), + func(context.Context) error { return boom }, + func(context.Context) (int, error) { drained = true; return 0, nil }, + time.Minute) + if !errors.Is(err, boom) { + t.Fatalf("want the manager's error, got %v", err) + } + if drained { + t.Fatal("a manager that failed must not be followed by a drain") + } +} + +func TestRunThenDrain_AnIncompleteDrainIsNotAFailure(t *testing.T) { + err := RunThenDrain(context.Background(), + func(context.Context) error { return nil }, + func(context.Context) (int, error) { return 2, context.DeadlineExceeded }, + time.Minute) + if err != nil { + t.Fatalf("fences left for the next flux-plunger or the pre-delete hook are not an error: %v", err) + } +} + +// A stop cancels every reconcile, so an unsuspend cut short by it is not the +// failure the Warning reports: the drain lifts that suspension next. +func TestRecordUnsuspendFailed_IgnoresACancelledStop(t *testing.T) { + rec := record.NewFakeRecorder(4) + r := &FluxPlunger{Recorder: rec} + r.recordUnsuspendFailed(fencedLiveRelease(), fmt.Errorf("apply: %w", context.Canceled)) + select { + case e := <-rec.Events: + t.Fatalf("a cancelled stop must not raise %q", e) + default: + } + r.recordUnsuspendFailed(fencedLiveRelease(), errors.New("forbidden")) + select { + case <-rec.Events: + default: + t.Fatal("a real failure must still raise the Warning") + } +} diff --git a/internal/controller/fluxplunger/ssa_ownership_envtest_test.go b/internal/controller/fluxplunger/ssa_ownership_envtest_test.go new file mode 100644 index 000000000..0d39c5df9 --- /dev/null +++ b/internal/controller/fluxplunger/ssa_ownership_envtest_test.go @@ -0,0 +1,722 @@ +// SPDX-License-Identifier: Apache-2.0 +//go:build envtest + +// Source of truth for what the suspension fix relies on from a REAL apiserver +// (envtest), where the controller-runtime fake client cannot stand in: it does not +// track metadata.managedFields, and it does not enforce resourceVersion or uid +// preconditions on server-side apply. It drives the PRODUCTION code against a +// realistic release, one an operator owns with a non-zero spec.interval. +// +// Ownership: acquire without conflict and without touching spec.interval, the +// operator transfer (misattribution boundary), no self-disown, relinquish, never +// claiming a suspension set under the shared client-side field manager +// (migrations, delete hook), and never acquiring an actively foreign-owned +// suspension; ownsSuspensionManagedFields is asserted directly against +// apiserver-produced managedFields. +// +// Uninstall fence (issue #4060): a delete landing during recovery reaches the Helm +// uninstall whichever of helm-controller and flux-plunger handles it first, with +// helm-controller v1.5.0's delete handling reproduced step for step; a delete +// racing the unsuspend of a live release is survived; and no apply resurrects a +// release that has gone. +// +// Built only under the `envtest` tag; `make test-controllers-envtest` fetches the +// apiserver and runs it, and CI runs that target on every pull request. +package fluxplunger + +import ( + "context" + "os" + "testing" + "time" + + helmv2 "github.com/fluxcd/helm-controller/api/v2" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/dynamic" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" +) + +const ( + fmHelm = "cozystack-operator" + fmOper = "kubectl-patch" +) + +// envtestStartTimeout replaces envtest's 20s default: CI runs these suites beside +// three other make goals on a four-CPU runner, where kube-apiserver can take +// longer than that to come up. +const envtestStartTimeout = 2 * time.Minute + +func newEnvtestEnvironment(crdDir string) *envtest.Environment { + return &envtest.Environment{ + CRDDirectoryPaths: []string{crdDir}, + ErrorIfCRDPathMissing: true, + ControlPlaneStartTimeout: envtestStartTimeout, + } +} + +func TestEnvtest_EnvironmentToleratesASlowControlPlaneStart(t *testing.T) { + if got := newEnvtestEnvironment("crds").ControlPlaneStartTimeout; got < time.Minute { + t.Fatalf("ControlPlaneStartTimeout = %v, want at least a minute", got) + } +} + +func TestEnvtest_SuspendOwnership(t *testing.T) { + crdDir := os.Getenv("HELMRELEASE_CRD_DIR") + if crdDir == "" { + t.Skip("set HELMRELEASE_CRD_DIR to the dir with the HelmRelease CRD") + } + env := newEnvtestEnvironment(crdDir) + cfg, err := env.Start() + if err != nil { + t.Fatalf("start envtest: %v", err) + } + defer func() { _ = env.Stop() }() + + if err := helmv2.AddToScheme(clientgoscheme.Scheme); err != nil { + t.Fatalf("add helm scheme: %v", err) + } + cl, err := client.New(cfg, client.Options{Scheme: clientgoscheme.Scheme}) + if err != nil { + t.Fatalf("client: %v", err) + } + ctx := context.Background() + r := &FluxPlunger{Client: cl} + key := types.NamespacedName{Namespace: testNS, Name: testName} + get := func() *helmv2.HelmRelease { + h := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, h); err != nil { + t.Fatalf("get: %v", err) + } + return h + } + + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNS}} + if err := cl.Create(ctx, ns); err != nil { + t.Fatalf("create ns: %v", err) + } + + // The operator establishes a realistic release via a full-object apply that owns + // a NON-ZERO spec.interval and does NOT set spec.suspend. This is the shape a + // typed-struct suspend apply conflicts with (it would emit spec.interval "0s"). + const operatorInterval = 5 * time.Minute + base := &helmv2.HelmRelease{ + TypeMeta: metav1.TypeMeta{APIVersion: helmv2.GroupVersion.String(), Kind: "HelmRelease"}, + ObjectMeta: metav1.ObjectMeta{Namespace: testNS, Name: testName}, + Spec: helmv2.HelmReleaseSpec{ + Interval: metav1.Duration{Duration: operatorInterval}, + Chart: &helmv2.HelmChartTemplate{Spec: helmv2.HelmChartTemplateSpec{Chart: "x", SourceRef: helmv2.CrossNamespaceObjectReference{Kind: "HelmRepository", Name: "r"}}}, + }, + } + if err := cl.Patch(ctx, base, client.Apply, client.FieldOwner(fmHelm)); err != nil { + t.Fatalf("operator base apply: %v", err) + } + + // Acquire: production suspendHelmRelease must acquire WITHOUT conflicting on + // spec.interval, and must leave the operator's interval untouched. + acquired, err := r.suspendHelmRelease(ctx, get()) + if err != nil { + t.Fatalf("acquire: suspendHelmRelease errored (interval conflict regression?): %v", err) + } + if !acquired { + t.Fatalf("acquire: suspendHelmRelease did not acquire the suspension") + } + h := get() + if !h.Spec.Suspend || !ownsSuspensionManagedFields(h) { + t.Fatalf("acquire: expected suspend=true owned by flux-plunger; suspend=%v owns=%v", h.Spec.Suspend, ownsSuspensionManagedFields(h)) + } + if h.Spec.Interval.Duration != operatorInterval { + t.Fatalf("acquire: suspend must not touch the operator's spec.interval; got %v want %v", h.Spec.Interval.Duration, operatorInterval) + } + t.Log("acquire: acquired suspend without conflict, operator interval preserved") + + // Unrelated re-apply: operator re-applies the whole object (unrelated fields), NOT touching + // suspend. flux-plunger must retain ownership (no self-disown). + base2 := &helmv2.HelmRelease{ + TypeMeta: metav1.TypeMeta{APIVersion: helmv2.GroupVersion.String(), Kind: "HelmRelease"}, + ObjectMeta: metav1.ObjectMeta{Namespace: testNS, Name: testName}, + Spec: helmv2.HelmReleaseSpec{ + Interval: metav1.Duration{Duration: operatorInterval}, + Chart: &helmv2.HelmChartTemplate{Spec: helmv2.HelmChartTemplateSpec{Chart: "x2", SourceRef: helmv2.CrossNamespaceObjectReference{Kind: "HelmRepository", Name: "r"}}}, + }, + } + if err := cl.Patch(ctx, base2, client.Apply, client.FieldOwner(fmHelm)); err != nil { + t.Fatalf("unrelated re-apply: operator re-apply: %v", err) + } + if h = get(); !h.Spec.Suspend || !ownsSuspensionManagedFields(h) { + t.Fatalf("unrelated re-apply (self-disown): after unrelated operator re-apply suspend=%v owns=%v", h.Spec.Suspend, ownsSuspensionManagedFields(h)) + } + t.Log("unrelated re-apply: unrelated operator re-apply did NOT disown flux-plunger's suspend") + + // Operator transfer: operator resumes then re-suspends via client-side patches. The field + // transfers to the operator, so flux-plunger no longer owns it. + for _, want := range []bool{false, true} { + cur := get() + patch := client.MergeFrom(cur.DeepCopy()) + cur.Spec.Suspend = want + if err := cl.Patch(ctx, cur, patch, client.FieldOwner(fmOper)); err != nil { + t.Fatalf("operator set suspend=%v: %v", want, err) + } + } + if h = get(); ownsSuspensionManagedFields(h) { + t.Fatalf("operator transfer (misattribution): flux-plunger still owns an operator-set suspension") + } + t.Log("operator transfer: after operator resume+resuspend, flux-plunger does NOT own the suspension") + + // unsuspendHelmRelease must now be a no-op (not owned): the operator's + // suspension must survive. + if err := r.unsuspendHelmRelease(ctx, get()); err != nil { + t.Fatalf("unsuspend on unowned suspension errored: %v", err) + } + if h = get(); !h.Spec.Suspend { + t.Fatalf("boundary FAIL: flux-plunger cleared an operator's suspension") + } + + // Relinquish: reset to a flux-plunger-owned suspension (operator resumes so the field is + // free), acquire, then unsuspendHelmRelease must relinquish → suspend=false, + // interval untouched, ownership dropped. + cur := get() + patch := client.MergeFrom(cur.DeepCopy()) + cur.Spec.Suspend = false + if err := cl.Patch(ctx, cur, patch, client.FieldOwner(fmOper)); err != nil { + t.Fatalf("relinquish: operator resume: %v", err) + } + if acquired, err := r.suspendHelmRelease(ctx, get()); err != nil || !acquired { + t.Fatalf("relinquish: re-acquire: acquired=%v err=%v", acquired, err) + } + if err := r.unsuspendHelmRelease(ctx, get()); err != nil { + t.Fatalf("relinquish: unsuspend: %v", err) + } + h = get() + if h.Spec.Suspend { + t.Fatalf("relinquish: unsuspend did not clear the suspension") + } + if ownsSuspensionManagedFields(h) { + t.Fatalf("relinquish: flux-plunger still owns suspend after unsuspend") + } + if h.Spec.Interval.Duration != operatorInterval { + t.Fatalf("relinquish: unsuspend must not touch the operator's spec.interval; got %v", h.Spec.Interval.Duration) + } + t.Log("relinquish: unsuspend relinquished ownership, reverted suspend to false, interval preserved") + + // Shared manager: a suspension set under the SHARED client-side field manager + // (flux-client-side-apply) must NOT be read as flux-plunger's, and + // unsuspendHelmRelease must leave it untouched. That manager is used by the + // platform migrations (cozy-proxy, cert-manager) and the tenant delete hook — and + // historically by earlier flux-plunger releases, which is exactly why the field + // manager cannot identify flux-plunger. Recognising it would let flux-plunger + // clear a migration's suspension (unsuspending cozy-proxy deletes + // cozystack-operator), so the boundary is pinned here on a real apiserver. + curShared := get() + sharedPatch := client.MergeFrom(curShared.DeepCopy()) + curShared.Spec.Suspend = true + if err := cl.Patch(ctx, curShared, sharedPatch, client.FieldOwner(fieldManager)); err != nil { + t.Fatalf("shared manager: shared-manager suspend: %v", err) + } + if h = get(); ownsSuspensionManagedFields(h) { + t.Fatalf("shared manager (misattribution risk): a suspension under the shared client-side manager must NOT be read as flux-plunger's") + } + if err := r.unsuspendHelmRelease(ctx, get()); err != nil { + t.Fatalf("shared manager: unsuspend: %v", err) + } + if h = get(); !h.Spec.Suspend { + t.Fatalf("shared manager (misattribution): flux-plunger cleared a suspension set under the shared client-side manager") + } + t.Log("shared manager: a shared-client-side-manager suspension is left untouched") + + // Active foreign suspension: an operator actively owns suspend=TRUE. suspendHelmRelease must NOT acquire + // or force it away — an active foreign suspension is never taken. The early + // already-suspended guard covers the common case; the re-fetch-after-conflict + // guard covers the race the fake-client tests pin. + curActive := get() + activePatch := client.MergeFrom(curActive.DeepCopy()) + curActive.Spec.Suspend = true + if err := cl.Patch(ctx, curActive, activePatch, client.FieldOwner(fmOper)); err != nil { + t.Fatalf("active foreign suspension: operator suspend: %v", err) + } + acquired, err = r.suspendHelmRelease(ctx, get()) + if err != nil { + t.Fatalf("active foreign suspension: suspend: %v", err) + } + if acquired { + t.Fatalf("active foreign suspension: acquired an actively foreign-suspended release") + } + if h = get(); !h.Spec.Suspend { + t.Fatalf("active foreign suspension: cleared an active foreign suspension") + } + if ownsSuspensionManagedFields(h) { + t.Fatalf("active foreign suspension: took ownership of an active foreign suspension") + } + t.Log("active foreign suspension: did not acquire or clear an actively foreign-owned suspension") +} + +// TestEnvtest_AcquiresOverBackupOwnedSuspend pins acquiring a +// suspension a backup controller left owned. A backup/restore controller in this repo sets +// spec.suspend via a dynamic-client Update, taking ownership of the field. A +// non-forced apply of spec.suspend then 409s forever; production suspendHelmRelease +// re-fetches after the conflict, sees the field still false, and forces on acquire, +// which must succeed without touching the operator's interval. Reverting the +// conditional force in suspendHelmRelease turns this RED with a .spec.suspend +// conflict. +func TestEnvtest_AcquiresOverBackupOwnedSuspend(t *testing.T) { + crdDir := os.Getenv("HELMRELEASE_CRD_DIR") + if crdDir == "" { + t.Skip("set HELMRELEASE_CRD_DIR to the dir with the HelmRelease CRD") + } + env := newEnvtestEnvironment(crdDir) + cfg, err := env.Start() + if err != nil { + t.Fatalf("start envtest: %v", err) + } + defer func() { _ = env.Stop() }() + + if err := helmv2.AddToScheme(clientgoscheme.Scheme); err != nil { + t.Fatalf("add helm scheme: %v", err) + } + cl, err := client.New(cfg, client.Options{Scheme: clientgoscheme.Scheme}) + if err != nil { + t.Fatalf("client: %v", err) + } + dyn, err := dynamic.NewForConfig(cfg) + if err != nil { + t.Fatalf("dynamic client: %v", err) + } + gvr := schema.GroupVersionResource{Group: "helm.toolkit.fluxcd.io", Version: "v2", Resource: "helmreleases"} + ctx := context.Background() + r := &FluxPlunger{Client: cl} + key := types.NamespacedName{Namespace: testNS, Name: testName} + if err := cl.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNS}}); err != nil { + t.Fatalf("create ns: %v", err) + } + + const operatorInterval = 5 * time.Minute + base := &helmv2.HelmRelease{ + TypeMeta: metav1.TypeMeta{APIVersion: helmv2.GroupVersion.String(), Kind: "HelmRelease"}, + ObjectMeta: metav1.ObjectMeta{Namespace: testNS, Name: testName}, + Spec: helmv2.HelmReleaseSpec{ + Interval: metav1.Duration{Duration: operatorInterval}, + Chart: &helmv2.HelmChartTemplate{Spec: helmv2.HelmChartTemplateSpec{Chart: "x", SourceRef: helmv2.CrossNamespaceObjectReference{Kind: "HelmRepository", Name: "r"}}}, + }, + } + if err := cl.Patch(ctx, base, client.Apply, client.FieldOwner("cozystack-operator")); err != nil { + t.Fatalf("operator base apply: %v", err) + } + + // A backup/restore controller suspends then resumes via a dynamic-client Update, + // leaving spec.suspend=false but OWNED by it (operation=Update). This is the + // state that 409s a non-forced apply. + for _, v := range []bool{true, false} { + u, err := dyn.Resource(gvr).Namespace(testNS).Get(ctx, testName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("backup get: %v", err) + } + _ = unstructured.SetNestedField(u.Object, v, "spec", "suspend") + if _, err := dyn.Resource(gvr).Namespace(testNS).Update(ctx, u, metav1.UpdateOptions{}); err != nil { + t.Fatalf("backup Update suspend=%v: %v", v, err) + } + } + + cur := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, cur); err != nil { + t.Fatalf("get: %v", err) + } + acquired, err := r.suspendHelmRelease(ctx, cur) + if err != nil { + t.Fatalf("backup-owned acquire: suspendHelmRelease could not acquire a backup-controller-owned spec.suspend (force missing?): %v", err) + } + if !acquired { + t.Fatalf("backup-owned acquire: suspendHelmRelease did not acquire") + } + got := &helmv2.HelmRelease{} + _ = cl.Get(ctx, key, got) + if !got.Spec.Suspend || !ownsSuspensionManagedFields(got) { + t.Fatalf("backup-owned acquire: expected suspend=true owned by flux-plunger; suspend=%v owns=%v", got.Spec.Suspend, ownsSuspensionManagedFields(got)) + } + if got.Spec.Interval.Duration != operatorInterval { + t.Fatalf("backup-owned acquire: acquire must not touch the operator's interval; got %v", got.Spec.Interval.Duration) + } + t.Log("backup-owned acquire: force-on-acquire took spec.suspend from a dynamic-Update owner, interval preserved") + + // Concurrent write: the backup controller holds the field at false and then + // suspends the release for real in the gap after one of acquire's reads. Both + // applies are preconditioned on the resourceVersion of the read that decided on + // them, so neither the unforced first attempt (which would otherwise record + // flux-plunger as a co-owner of the backup's true) nor the forced retry may take + // the suspension: acquire reports not acquired, without an error, and the + // backup's suspension keeps its owner. + setBackupSuspend := func(v bool) { + u, err := dyn.Resource(gvr).Namespace(testNS).Get(ctx, testName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("concurrent write: backup get: %v", err) + } + _ = unstructured.SetNestedField(u.Object, v, "spec", "suspend") + if _, err := dyn.Resource(gvr).Namespace(testNS).Update(ctx, u, metav1.UpdateOptions{}); err != nil { + t.Fatalf("concurrent write: backup Update suspend=%v: %v", v, err) + } + } + for _, tc := range []struct { + name string + after int // the backup writes right after this Get of acquire + }{ + {"before the unforced apply", 1}, + {"before the forced apply", 2}, + } { + setBackupSuspend(false) + racing := &FluxPlunger{Client: cl, APIReader: &writeAfterNthGet{Reader: cl, n: tc.after, write: func() { setBackupSuspend(true) }}} + start := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, start); err != nil { + t.Fatalf("concurrent write %s: get: %v", tc.name, err) + } + acquired, err := racing.suspendHelmRelease(ctx, start) + if err != nil || acquired { + t.Fatalf("concurrent write %s: acquire must report not acquired without an error; acquired=%v err=%v", tc.name, acquired, err) + } + after := &helmv2.HelmRelease{} + _ = cl.Get(ctx, key, after) + if !after.Spec.Suspend || ownsSuspensionManagedFields(after) { + t.Fatalf("concurrent write %s: the backup's suspension must survive unowned by flux-plunger; suspend=%v owns=%v", tc.name, after.Spec.Suspend, ownsSuspensionManagedFields(after)) + } + t.Logf("concurrent write %s: acquire declined, backup's suspension kept", tc.name) + } +} + +// writeAfterNthGet runs write right after its n-th Get returns, standing in for +// another controller writing the object in the gap between a read and the apply +// decided on it. +type writeAfterNthGet struct { + client.Reader + n, calls int + write func() +} + +func (w *writeAfterNthGet) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + err := w.Reader.Get(ctx, key, obj, opts...) + w.calls++ + if w.calls == w.n { + w.write() + } + return err +} + +// simulateHelmControllerDelete stands in for helm-controller v1.5.0's +// reconcileDelete (internal/controller/helmrelease_controller.go, the +// deletionTimestamp branch): on a terminating HelmRelease it runs the Helm +// uninstall only when spec.suspend is false, then removes its own finalizer either +// way and records the observed generation. It reports whether it uninstalled. It is +// the behaviour issue #4060 is about: a delete handled while the release is +// suspended drops the finalizer without an uninstall. +func simulateHelmControllerDelete(t *testing.T, ctx context.Context, cl client.Client, key types.NamespacedName) (uninstalled bool) { + t.Helper() + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("helm-controller: get: %v", err) + } + if hr.DeletionTimestamp.IsZero() { + t.Fatalf("helm-controller: release is not terminating") + } + uninstalled = !hr.Spec.Suspend + hr.Status.ObservedGeneration = hr.Generation + if err := cl.Status().Update(ctx, hr); err != nil { + t.Fatalf("helm-controller: status update: %v", err) + } + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("helm-controller: re-get: %v", err) + } + var kept []string + for _, f := range hr.Finalizers { + if f != helmv2.HelmReleaseFinalizer { + kept = append(kept, f) + } + } + if len(kept) != len(hr.Finalizers) { + hr.Finalizers = kept + if err := cl.Update(ctx, hr); err != nil { + t.Fatalf("helm-controller: remove finalizer: %v", err) + } + } + return uninstalled +} + +// TestEnvtest_DeleteDuringRecoveryReachesUninstall is the regression for issue +// #4060 itself: a delete arriving while flux-plunger holds its recovery suspension +// must still reach the Helm uninstall. helm-controller and flux-plunger wake on the +// same delete event, so both orders are pinned: helm-controller handling it first +// (it sees the suspension, skips the uninstall and drops its finalizer; only +// flux-plunger's fence finalizer keeps the object alive until the suspension is +// lifted and the uninstall runs), and flux-plunger handling it first. +func TestEnvtest_DeleteDuringRecoveryReachesUninstall(t *testing.T) { + crdDir := os.Getenv("HELMRELEASE_CRD_DIR") + if crdDir == "" { + t.Skip("set HELMRELEASE_CRD_DIR to the dir with the HelmRelease CRD") + } + env := newEnvtestEnvironment(crdDir) + cfg, err := env.Start() + if err != nil { + t.Fatalf("start envtest: %v", err) + } + defer func() { _ = env.Stop() }() + if err := helmv2.AddToScheme(clientgoscheme.Scheme); err != nil { + t.Fatalf("add helm scheme: %v", err) + } + cl, err := client.New(cfg, client.Options{Scheme: clientgoscheme.Scheme}) + if err != nil { + t.Fatalf("client: %v", err) + } + ctx := context.Background() + if err := cl.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNS}}); err != nil { + t.Fatalf("create ns: %v", err) + } + + for _, tc := range []struct { + name string + helmFirst bool + }{ + {"helm-controller handles the delete first", true}, + {"flux-plunger handles the delete first", false}, + } { + t.Run(tc.name, func(t *testing.T) { + name := testName + map[bool]string{true: "-hc", false: "-fp"}[tc.helmFirst] + key := types.NamespacedName{Namespace: testNS, Name: name} + base := &helmv2.HelmRelease{ + TypeMeta: metav1.TypeMeta{APIVersion: helmv2.GroupVersion.String(), Kind: "HelmRelease"}, + ObjectMeta: metav1.ObjectMeta{Namespace: testNS, Name: name}, + Spec: helmv2.HelmReleaseSpec{ + Interval: metav1.Duration{Duration: 5 * time.Minute}, + Chart: &helmv2.HelmChartTemplate{Spec: helmv2.HelmChartTemplateSpec{Chart: "x", SourceRef: helmv2.CrossNamespaceObjectReference{Kind: "HelmRepository", Name: "r"}}}, + }, + } + if err := cl.Patch(ctx, base, client.Apply, client.FieldOwner(fmHelm)); err != nil { + t.Fatalf("operator base apply: %v", err) + } + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + hr.Finalizers = append(hr.Finalizers, helmv2.HelmReleaseFinalizer) + if err := cl.Update(ctx, hr); err != nil { + t.Fatalf("add helm-controller finalizer: %v", err) + } + + r := &FluxPlunger{Client: cl, APIReader: cl} + req := ctrl.Request{NamespacedName: key} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + if acquired, err := r.suspendHelmRelease(ctx, hr); err != nil || !acquired { + t.Fatalf("acquire: acquired=%v err=%v", acquired, err) + } + + // The delete lands inside the recovery window. + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + if err := cl.Delete(ctx, hr); err != nil { + t.Fatalf("delete: %v", err) + } + + uninstalled := false + if tc.helmFirst { + uninstalled = simulateHelmControllerDelete(t, ctx, cl, key) || uninstalled + if err := cl.Get(ctx, key, &helmv2.HelmRelease{}); err != nil { + t.Fatalf("the release must survive helm-controller dropping its finalizer while suspended (fence), got %v", err) + } + } + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("flux-plunger reconcile of the terminating release: %v", err) + } + // Lifting the suspension is itself an update event, so flux-plunger can + // reconcile again before helm-controller has seen the new generation. When + // helm-controller handled the delete first its finalizer is already gone; + // only the observed generation still lagging tells flux-plunger the + // uninstall has not run, so the fence must hold through this pass. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("flux-plunger reconcile racing ahead of helm-controller: %v", err) + } + if err := cl.Get(ctx, key, &helmv2.HelmRelease{}); err != nil { + t.Fatalf("the fence must hold until helm-controller has observed the unsuspended generation, got %v", err) + } + if !tc.helmFirst { + uninstalled = simulateHelmControllerDelete(t, ctx, cl, key) || uninstalled + } else { + // With the suspension lifted, helm-controller reconciles again and now + // runs the uninstall. + uninstalled = simulateHelmControllerDelete(t, ctx, cl, key) || uninstalled + } + if !uninstalled { + t.Fatalf("a delete arriving during recovery must reach the Helm uninstall") + } + // With the uninstall done, flux-plunger lifts its fence and the release is + // gone. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("flux-plunger reconcile after the uninstall: %v", err) + } + if err := cl.Get(ctx, key, &helmv2.HelmRelease{}); !apierrors.IsNotFound(err) { + t.Fatalf("after the uninstall flux-plunger must lift its fence so the release is deleted, got %v", err) + } + t.Logf("%s: delete during recovery reached the uninstall, release gone", tc.name) + }) + } +} + +// startFenceEnv starts envtest and returns a client plus a helper that creates a +// release under helm-controller's finalizer, as helm-controller leaves every release +// it manages. +func startFenceEnv(t *testing.T) (context.Context, client.Client, func(name string) types.NamespacedName, func()) { + t.Helper() + crdDir := os.Getenv("HELMRELEASE_CRD_DIR") + if crdDir == "" { + t.Skip("set HELMRELEASE_CRD_DIR to the dir with the HelmRelease CRD") + } + env := newEnvtestEnvironment(crdDir) + cfg, err := env.Start() + if err != nil { + t.Fatalf("start envtest: %v", err) + } + if err := helmv2.AddToScheme(clientgoscheme.Scheme); err != nil { + t.Fatalf("add helm scheme: %v", err) + } + cl, err := client.New(cfg, client.Options{Scheme: clientgoscheme.Scheme}) + if err != nil { + t.Fatalf("client: %v", err) + } + ctx := context.Background() + if err := cl.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNS}}); err != nil { + t.Fatalf("create ns: %v", err) + } + newRelease := func(name string) types.NamespacedName { + key := types.NamespacedName{Namespace: testNS, Name: name} + base := &helmv2.HelmRelease{ + TypeMeta: metav1.TypeMeta{APIVersion: helmv2.GroupVersion.String(), Kind: "HelmRelease"}, + ObjectMeta: metav1.ObjectMeta{Namespace: testNS, Name: name}, + Spec: helmv2.HelmReleaseSpec{ + Interval: metav1.Duration{Duration: 5 * time.Minute}, + Chart: &helmv2.HelmChartTemplate{Spec: helmv2.HelmChartTemplateSpec{Chart: "x", SourceRef: helmv2.CrossNamespaceObjectReference{Kind: "HelmRepository", Name: "r"}}}, + }, + } + if err := cl.Patch(ctx, base, client.Apply, client.FieldOwner(fmHelm)); err != nil { + t.Fatalf("operator base apply: %v", err) + } + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("get: %v", err) + } + hr.Finalizers = append(hr.Finalizers, helmv2.HelmReleaseFinalizer) + if err := cl.Update(ctx, hr); err != nil { + t.Fatalf("add helm-controller finalizer: %v", err) + } + return key + } + return ctx, cl, newRelease, func() { _ = env.Stop() } +} + +// TestEnvtest_DeleteRacingUnsuspendOfLiveReleaseKeepsFence pins the window between +// unsuspendHelmRelease's read of a still-live release and its apply, which lifts the +// suspension and the fence together. A delete landing there that helm-controller +// handles first (suspended, so no uninstall and its finalizer dropped) would leave +// the fence as the last finalizer, and an unpreconditioned apply removing it would +// delete the release without an uninstall. The apply must fail instead, and the +// next reconcile must carry the delete through to the uninstall. +func TestEnvtest_DeleteRacingUnsuspendOfLiveReleaseKeepsFence(t *testing.T) { + ctx, cl, newRelease, stop := startFenceEnv(t) + defer stop() + key := newRelease(testName + "-live") + + start := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, start); err != nil { + t.Fatalf("get: %v", err) + } + if acquired, err := (&FluxPlunger{Client: cl, APIReader: cl}).suspendHelmRelease(ctx, start); err != nil || !acquired { + t.Fatalf("acquire: acquired=%v err=%v", acquired, err) + } + + // The delete lands right after unsuspendHelmRelease reads the live release, and + // helm-controller handles it while the release is still suspended. + uninstalled := false + racing := &FluxPlunger{Client: cl, APIReader: &writeAfterNthGet{Reader: cl, n: 1, write: func() { + hr := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, hr); err != nil { + t.Fatalf("race: get: %v", err) + } + if err := cl.Delete(ctx, hr); err != nil { + t.Fatalf("race: delete: %v", err) + } + uninstalled = simulateHelmControllerDelete(t, ctx, cl, key) || uninstalled + }}} + _ = racing.unsuspendHelmRelease(ctx, start) + + if err := cl.Get(ctx, key, &helmv2.HelmRelease{}); err != nil { + t.Fatalf("the release must survive a delete racing the live unsuspend (fence), got %v", err) + } + + r := &FluxPlunger{Client: cl, APIReader: cl} + req := ctrl.Request{NamespacedName: key} + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("reconcile of the terminating release: %v", err) + } + uninstalled = simulateHelmControllerDelete(t, ctx, cl, key) || uninstalled + if !uninstalled { + t.Fatalf("the delete must reach the Helm uninstall") + } + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("reconcile after the uninstall: %v", err) + } + if err := cl.Get(ctx, key, &helmv2.HelmRelease{}); !apierrors.IsNotFound(err) { + t.Fatalf("after the uninstall the fence must be lifted and the release gone, got %v", err) + } +} + +// TestEnvtest_ApplyNeverRecreatesAVanishedRelease pins that flux-plunger's apply +// cannot resurrect a release. Server-side apply creates what does not exist, so an +// apply decided on a read of a release that has since gone (its fence removed by +// hand, then deleted) would otherwise create an empty HelmRelease carrying the fence. +func TestEnvtest_ApplyNeverRecreatesAVanishedRelease(t *testing.T) { + ctx, cl, newRelease, stop := startFenceEnv(t) + defer stop() + key := newRelease(testName + "-ghost") + + start := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, start); err != nil { + t.Fatalf("get: %v", err) + } + r := &FluxPlunger{Client: cl, APIReader: cl} + if acquired, err := r.suspendHelmRelease(ctx, start); err != nil || !acquired { + t.Fatalf("acquire: acquired=%v err=%v", acquired, err) + } + stale := &helmv2.HelmRelease{} + if err := cl.Get(ctx, key, stale); err != nil { + t.Fatalf("get: %v", err) + } + + // The release goes for good: its finalizers are cleared by hand and it is deleted. + gone := stale.DeepCopy() + gone.Finalizers = nil + if err := cl.Update(ctx, gone); err != nil { + t.Fatalf("clear finalizers: %v", err) + } + if err := cl.Delete(ctx, gone); err != nil { + t.Fatalf("delete: %v", err) + } + if err := cl.Get(ctx, key, &helmv2.HelmRelease{}); !apierrors.IsNotFound(err) { + t.Fatalf("setup: release must be gone, got %v", err) + } + + for _, apply := range []struct { + name string + run func() error + }{ + {"relinquish", func() error { return r.applyRecoveryState(ctx, stale, nil, false, false) }}, + {"keep the fence", func() error { return r.applyRecoveryState(ctx, stale, nil, true, false) }}, + } { + _ = apply.run() + if err := cl.Get(ctx, key, &helmv2.HelmRelease{}); !apierrors.IsNotFound(err) { + t.Fatalf("%s apply on a vanished release must not recreate it, got %v", apply.name, err) + } + } +} diff --git a/packages/system/flux-plunger/Makefile b/packages/system/flux-plunger/Makefile index d88b840bd..1ed805bc7 100644 --- a/packages/system/flux-plunger/Makefile +++ b/packages/system/flux-plunger/Makefile @@ -4,6 +4,10 @@ export NAMESPACE=cozy-fluxcd include ../../../hack/common-envs.mk include ../../../hack/package.mk +.PHONY: test +test: + helm unittest . + image: docker buildx build -f images/flux-plunger/Dockerfile ../../../ \ $(call image-tags,flux-plunger,v$(COZYSTACK_VERSION)) \ diff --git a/packages/system/flux-plunger/README.md b/packages/system/flux-plunger/README.md new file mode 100644 index 000000000..93361f568 --- /dev/null +++ b/packages/system/flux-plunger/README.md @@ -0,0 +1,31 @@ +# flux-plunger + +Recovers a HelmRelease stuck with `has no deployed releases`: it suspends the release, deletes the stale Helm storage Secret, records the processed revision in the `flux-plunger.cozystack.io/last-processed-version` annotation, and unsuspends. + +## The uninstall fence + +While a release is suspended for recovery it also carries the finalizer `flux-plunger.cozystack.io/uninstall-fence`. helm-controller skips the Helm uninstall of a suspended HelmRelease that is deleted, so without the fence a delete arriving during recovery would leave the release's resources behind. With it, flux-plunger lifts its suspension, helm-controller runs the uninstall, and flux-plunger then removes the fence. A delete held this way for more than ten minutes raises an `UninstallFenceHeld` Warning on the HelmRelease. + +When flux-plunger stops it gives up the fences it holds itself, within its termination grace period (`shutdownGracePeriodSeconds`, 90 by default, accepted between 40 and 140): its manager is given 10 seconds to stop, and the drain 25 seconds less than the grace period. The next flux-plunger starts while the old one is still terminating, on a rollout as on an eviction or a node drain, so a single writer is kept by leader election: the old pod holds its Lease through the drain without giving it up, and the Lease outlasts the drain, so a new pod waits for it to run out, a little under two minutes at the default, before it acts. A recovery in progress is aborted: the suspension and the fence go together, and the next flux-plunger starts the recovery again. A release being deleted has flux-plunger's suspension lifted and keeps the fence until helm-controller has run its uninstall, which the drain waits for; one helm-controller has not handled when the grace period runs out keeps its fence. Uninstalling this chart then releases whatever is left: a pre-delete hook stops flux-plunger, waits out its grace period, and lifts any remaining suspension and fence without waiting again. It lifts a suspension only where flux-plunger is the sole manager holding it, which is the outcome flux-plunger's own relinquish has, so a suspension another manager also holds stays; it does so with a patch of `spec.suspend` alone rather than a `kubectl apply`, which on a release last written by a client-side apply would take over and strip the fields that apply set. A release it unfences that was being deleted, that helm-controller's own finalizer no longer held and whose latest generation helm-controller had not observed, is deleted without an uninstall and is named as such. The Job's 240-second deadline sits under the 300 seconds helm-controller allows a pre-delete hook by default. A Deployment that cannot be read or scaled down (flux-plunger would keep running and could fence behind the sweep) ends the hook before any fence is touched, a listing that never succeeds ends it before any fence is touched too, and a fence the hook cannot remove ends it after the sweep; either way the hook exits non-zero, which fails the uninstall and makes helm-controller retry it, hook included. A flux-plunger Deployment that is already gone has nothing to stop, and a flux-plunger pod that is still there after its grace period, typically one stuck `Terminating` on a `NotReady` node, is named and the sweep goes on, since failing on it would fail every retry until the node returns or the pod is force-deleted; because such a pod may still be fencing, the fences are listed again after the sweep, and any found fails the hook. Its report, `released N of M fenced HelmReleases, K without an uninstall` and a line per release, stays readable for ten minutes after the Job finishes, success included: `kubectl logs --namespace cozy-fluxcd job/flux-plunger-uninstall-fence-cleanup`. + +If flux-plunger is merely stopped (scaled to zero) rather than uninstalled, or killed without a graceful stop, a fenced release that is deleted and that helm-controller has not uninstalled stays `Terminating` until flux-plunger runs again. To release one by hand, after confirming its uninstall has run or is not wanted, remove the finalizer at the position `INDEX` it has in `metadata.finalizers`; the `test` operation makes the patch fail rather than remove another finalizer if the list has changed: + +```bash +kubectl patch helmrelease NAME --namespace NAMESPACE --type json --patch '[{"op":"test","path":"/metadata/finalizers/INDEX","value":"flux-plunger.cozystack.io/uninstall-fence"},{"op":"remove","path":"/metadata/finalizers/INDEX"}]' +``` + +Fenced releases can be listed with: + +```bash +kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output go-template='{{range .items}}{{$ns := .metadata.namespace}}{{$name := .metadata.name}}{{range .metadata.finalizers}}{{if eq . "flux-plunger.cozystack.io/uninstall-fence"}}{{$ns}}/{{$name}}{{"\n"}}{{end}}{{end}}{{end}}' +``` + +## Suspensions left by an earlier flux-plunger + +Earlier flux-plunger releases set `spec.suspend` through the shared `flux-client-side-apply` manager, so a suspension one of them left behind cannot be told apart from a migration's or an operator's and is treated as external: it is not cleared automatically. Such a leftover exists only if the old pod stopped in the middle of a recovery, and flux-plunger raises a `SuspensionNotOwned` Warning on it. To list candidates, suspended releases carrying the flux-plunger annotation whose `spec.suspend` is not owned by flux-plunger's apply: + +```bash +kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output json | jq --raw-output '.items[] | select(.spec.suspend == true) | select(.metadata.annotations["flux-plunger.cozystack.io/last-processed-version"] != null) | select([.metadata.managedFields[]? | select(.manager == "flux-plunger" and .operation == "Apply" and .fieldsV1["f:spec"]["f:suspend"] != null)] | length == 0) | "\(.metadata.namespace)/\(.metadata.name)"' +``` + +Clearing one is an operator action: confirm nothing else (a migration, a restore) is holding it, then resume it with `flux resume helmrelease NAME --namespace NAMESPACE`. diff --git a/packages/system/flux-plunger/templates/deployment.yaml b/packages/system/flux-plunger/templates/deployment.yaml index 7ce6bb13c..fd856d251 100644 --- a/packages/system/flux-plunger/templates/deployment.yaml +++ b/packages/system/flux-plunger/templates/deployment.yaml @@ -6,7 +6,34 @@ metadata: app.kubernetes.io/name: flux-plunger app.kubernetes.io/instance: {{ .Release.Name }} spec: + {{- /* + flux-plunger treats a suspension its own field manager owns, seen in a fresh + reconcile, as its own interrupted run, so exactly one process may act on a + HelmRelease at a time. No rollout strategy gives that: the next pod starts as + soon as the old one is terminating, and an eviction or a node drain replaces a + pod the same way, while the old one stops its manager and drains its fences + (cmd/flux-plunger/main.go). What keeps a single writer is the leader Lease + (--leader-elect), which the old pod holds through the drain and which outlasts + it, so the next pod waits for it before it acts. maxSurge 0 only keeps a rollout + from starting the next pod before the old one is told to stop. This stays + RollingUpdate rather than Recreate because existing installs carry the + apiserver-defaulted rollingUpdate block (the chart shipped without a strategy), + and helm-controller applies server-side, where type Recreate over that block is + rejected on upgrade. + */}} + {{- $grace := int .Values.shutdownGracePeriodSeconds }} + {{- if ne (toString .Values.shutdownGracePeriodSeconds) (toString $grace) }} + {{- fail "shutdownGracePeriodSeconds must be a whole number of seconds: terminationGracePeriodSeconds takes an integer" }} + {{- end }} + {{- if or (lt $grace 40) (gt $grace 140) }} + {{- fail "shutdownGracePeriodSeconds must be between 40 and 140: below, the drain gets no time after the manager's 10s stop; above, the pre-delete hook cannot wait the pod out inside its deadline" }} + {{- end }} replicas: 1 + strategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 0 + maxUnavailable: 1 selector: matchLabels: app.kubernetes.io/name: flux-plunger @@ -18,6 +45,7 @@ spec: app.kubernetes.io/instance: {{ .Release.Name }} spec: serviceAccountName: flux-plunger + terminationGracePeriodSeconds: {{ .Values.shutdownGracePeriodSeconds }} containers: - name: flux-plunger image: "{{ .Values.image }}" @@ -29,6 +57,9 @@ spec: {{- end }} - --metrics-bind-address=:8080 - --metrics-secure=false + - --leader-elect + # The manager's stop takes up to 10s; the rest leaves 15s before SIGKILL. + - --shutdown-drain-timeout={{ sub .Values.shutdownGracePeriodSeconds 25 }}s ports: - name: metrics containerPort: 8080 diff --git a/packages/system/flux-plunger/templates/rbac.yaml b/packages/system/flux-plunger/templates/rbac.yaml index 97d4eac7e..9f47ac21c 100644 --- a/packages/system/flux-plunger/templates/rbac.yaml +++ b/packages/system/flux-plunger/templates/rbac.yaml @@ -22,6 +22,13 @@ rules: - get - list - delete +- apiGroups: + - "" + resources: + - events + verbs: + - create + - patch --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding @@ -35,3 +42,28 @@ subjects: - kind: ServiceAccount name: flux-plunger namespace: {{ .Release.Namespace }} +--- +# The leader Lease (--leader-elect) lives in the release namespace. +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: flux-plunger-leader-election + namespace: {{ .Release.Namespace }} +rules: +- apiGroups: ["coordination.k8s.io"] + resources: ["leases"] + verbs: ["get", "list", "watch", "create", "update", "patch"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: flux-plunger-leader-election + namespace: {{ .Release.Namespace }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: flux-plunger-leader-election +subjects: +- kind: ServiceAccount + name: flux-plunger + namespace: {{ .Release.Namespace }} diff --git a/packages/system/flux-plunger/templates/uninstall-fence-cleanup.yaml b/packages/system/flux-plunger/templates/uninstall-fence-cleanup.yaml new file mode 100644 index 000000000..669d0f804 --- /dev/null +++ b/packages/system/flux-plunger/templates/uninstall-fence-cleanup.yaml @@ -0,0 +1,391 @@ +{{- /* + flux-plunger sets a finalizer, flux-plunger.cozystack.io/uninstall-fence, on a + HelmRelease it is recovering, and only flux-plunger removes it. When it stops it + gives them all up itself (DrainFences): it lifts its suspensions and, within its + termination grace period, waits for helm-controller to uninstall each release + being deleted before lifting that fence. This hook is the backstop for what the + drain could not finish, so that removing the chart never leaves a release that + tenant teardown waits on forever: it stops flux-plunger, waits out the drain, + then lifts any suspension and fence that remain, without waiting again. + + Two bounds apply. helm-controller runs a pre-delete hook under the release's + uninstall timeout, 300s by default (the operator sets none), counted from when + helm creates the hook; the Job's activeDeadlineSeconds is counted from the Job's + start, so it has to end first, with room for the pod to be scheduled and pulled. + The pod wait covers flux-plunger's grace period, and the sweep has the rest. + + A scale-down or a listing that never succeeds ends the hook before any fence is + touched, and a fence that cannot be removed ends it once the sweep is done. + Either way it exits non-zero: the failed hook fails the uninstall before helm + records anything, and helm-controller retries it, running this hook again from a + state it can still converge from; exiting 0 would let the chart go and leave the + fence with nothing to remove it. A pod still there after its grace period is + named and the sweep goes on; since that pod may still be fencing, the fences are + listed again after the sweep, and any found fails the hook the same way. +*/ -}} +{{- $img := printf "%s:%s" .Values.kubectlImage.repository .Values.kubectlImage.tag }} +{{- with .Values.kubectlImage.digest }}{{- $img = printf "%s@%s" $img . }}{{- end }} +{{- $deadline := 240 }} +{{- $podWait := add .Values.shutdownGracePeriodSeconds 10 }} +{{- $retryWindow := sub $deadline 60 }} +apiVersion: v1 +kind: ServiceAccount +metadata: + name: flux-plunger-uninstall-fence-cleanup + namespace: {{ .Release.Namespace }} + annotations: + helm.sh/hook: pre-delete + helm.sh/hook-weight: "-10" + helm.sh/hook-delete-policy: hook-succeeded,hook-failed,before-hook-creation +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: flux-plunger-uninstall-fence-cleanup + annotations: + helm.sh/hook: pre-delete + helm.sh/hook-weight: "-10" + helm.sh/hook-delete-policy: hook-succeeded,hook-failed,before-hook-creation +rules: + - apiGroups: ["helm.toolkit.fluxcd.io"] + resources: ["helmreleases"] + verbs: ["get", "list", "patch"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: flux-plunger-uninstall-fence-cleanup + annotations: + helm.sh/hook: pre-delete + helm.sh/hook-weight: "-10" + helm.sh/hook-delete-policy: hook-succeeded,hook-failed,before-hook-creation +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: flux-plunger-uninstall-fence-cleanup +subjects: + - kind: ServiceAccount + name: flux-plunger-uninstall-fence-cleanup + namespace: {{ .Release.Namespace }} +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: flux-plunger-uninstall-fence-cleanup + namespace: {{ .Release.Namespace }} + annotations: + helm.sh/hook: pre-delete + helm.sh/hook-weight: "-10" + helm.sh/hook-delete-policy: hook-succeeded,hook-failed,before-hook-creation +rules: + # kubectl scale reads the Deployment, then patches its scale subresource. + - apiGroups: ["apps"] + resources: ["deployments"] + resourceNames: ["flux-plunger"] + verbs: ["get"] + - apiGroups: ["apps"] + resources: ["deployments/scale"] + resourceNames: ["flux-plunger"] + verbs: ["patch"] + - apiGroups: [""] + resources: ["pods"] + verbs: ["get", "list", "watch"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: flux-plunger-uninstall-fence-cleanup + namespace: {{ .Release.Namespace }} + annotations: + helm.sh/hook: pre-delete + helm.sh/hook-weight: "-10" + helm.sh/hook-delete-policy: hook-succeeded,hook-failed,before-hook-creation +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: flux-plunger-uninstall-fence-cleanup +subjects: + - kind: ServiceAccount + name: flux-plunger-uninstall-fence-cleanup + namespace: {{ .Release.Namespace }} +--- +apiVersion: batch/v1 +kind: Job +metadata: + name: flux-plunger-uninstall-fence-cleanup + namespace: {{ .Release.Namespace }} + annotations: + helm.sh/hook: pre-delete + helm.sh/hook-weight: "0" + # No hook-succeeded: a succeeded Job would go with its pod and the report + # in its log. helm.sh/hook-output-log-policy cannot stand in for that, since + # helm-controller calls a nil output writer for it + # (https://github.com/fluxcd/helm-controller/issues/1562). + helm.sh/hook-delete-policy: before-hook-creation +spec: + # Under helm's own bound for the hook; see the header. + activeDeadlineSeconds: {{ $deadline }} + # One run fills the deadline; helm-controller retries the whole uninstall, + # with a fresh budget, instead of a second pod racing it out. + backoffLimit: 0 + # The finished Job, and the report in its pod's log, outlive the chart this long. + ttlSecondsAfterFinished: 600 + template: + spec: + serviceAccountName: flux-plunger-uninstall-fence-cleanup + restartPolicy: Never + securityContext: + runAsNonRoot: true + runAsUser: 65532 + seccompProfile: + type: RuntimeDefault + # kubectl keeps its discovery cache under $HOME; under a read-only root + # filesystem the emptyDir is the one place it can write it. + volumes: + - name: kube-cache + emptyDir: {} + containers: + - name: cleanup + image: {{ $img }} + imagePullPolicy: {{ .Values.kubectlImage.pullPolicy }} + # Same small fixed footprint as the other hooks that run this image, + # so the pod is scheduled promptly inside the deadline. + resources: + requests: + cpu: 10m + memory: 32Mi + limits: + cpu: 100m + memory: 64Mi + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] + env: + - name: NAMESPACE + value: {{ .Release.Namespace }} + - name: HOME + value: /tmp + volumeMounts: + - name: kube-cache + mountPath: /tmp + command: + - sh + - -c + - | + set -u + fence=flux-plunger.cozystack.io/uninstall-fence + deploy=flux-plunger + hr=helmreleases.helm.toolkit.fluxcd.io + # HR_SCOPE narrows the sweep so the script can be exercised against + # one namespace; the hook itself always covers the cluster. + scope="${HR_SCOPE:---all-namespaces}" + # The pod is given its grace period, and a little more, to drain. + pod_wait={{ $podWait }} + # Retries stop here, a minute short of activeDeadlineSeconds, which + # counts from the Job's start, so the report is still written when + # scheduling and the image pull took part of it. The pod wait is cut + # to what is left of this window too. + retry_until=$(( $(date +%s) + {{ $retryWindow }} )) + # Each fenced release once, however many copies of the fence it + # carries: the apiserver accepts a finalizer listed twice. + listing='{{`{{range .items}}{{$fenced := false}}{{range .metadata.finalizers}}{{if eq . "flux-plunger.cozystack.io/uninstall-fence"}}{{$fenced = true}}{{end}}{{end}}{{if $fenced}}{{.metadata.namespace}}/{{.metadata.name}}{{"\n"}}{{end}}{{end}}`}}' + + # No per-call --request-timeout: any client override makes kubectl + # skip the in-cluster config and dial localhost:8080. A wedged call + # is bounded by activeDeadlineSeconds instead, and a Job killed by + # it fails the uninstall, which helm-controller then retries. + + # retry ...: run until it succeeds or the retry window closes, + # at least once. Only the successful attempt's output is printed: + # kubectl prints what it has before it fails, and that must not be + # read as part of the answer. Each failure's stderr stays in the log. + retry() { + while :; do + if out=$("$@"); then + printf '%s\n' "$out" + return 0 + fi + [ "$(date +%s)" -lt "$retry_until" ] || return 1 + sleep 2 + done + } + + # lift_suspension : lift the spec.suspend flux-plunger's + # apply owns, with the same outcome as the controller's relinquish: a + # suspension another manager also holds is left standing. Not with + # kubectl apply --server-side: on a release created by a client-side + # kubectl apply it hands every field that apply set to flux-plunger + # and then strips them. The patch is preconditioned on the + # resourceVersion and uid of the read that decided on it, and a write + # landing in between makes it read again. Returns 2 when the release + # cannot be read, 1 when the suspension cannot be lifted. + lift_suspension() { + ls_ns=$1 + ls_name=$2 + while :; do + if ! read_out=$(retry kubectl --namespace "$ls_ns" get "$hr" "$ls_name" --ignore-not-found --output go-template='{{`{{.metadata.resourceVersion}} {{.metadata.uid}} {{if .spec.suspend}}{{range .metadata.managedFields}}{{$owns := false}}{{range $k, $v := .fieldsV1}}{{if eq $k "f:spec"}}{{range $k2, $v2 := $v}}{{if eq $k2 "f:suspend"}}{{$owns = true}}{{end}}{{end}}{{end}}{{end}}{{if $owns}}{{if and (eq .manager "flux-plunger") (eq .operation "Apply")}} yes{{end}}{{if ne .manager "flux-plunger"}} other{{end}}{{end}}{{end}}{{end}}`}}'); then + return 2 + fi + # shellcheck disable=SC2086 # split into resourceVersion, uid, marks + set -- $read_out + [ "$#" -ge 2 ] || return 0 + ls_rv=$1 + ls_uid=$2 + shift 2 + case "$*" in + *other*) return 0 ;; + *yes*) ;; + *) return 0 ;; + esac + if kubectl --namespace "$ls_ns" patch "$hr" "$ls_name" --type json --field-manager flux-plunger \ + --patch "[{\"op\":\"test\",\"path\":\"/metadata/resourceVersion\",\"value\":\"$ls_rv\"},{\"op\":\"test\",\"path\":\"/metadata/uid\",\"value\":\"$ls_uid\"},{\"op\":\"replace\",\"path\":\"/spec/suspend\",\"value\":false}]"; then + return 0 + fi + [ "$(date +%s)" -lt "$retry_until" ] || return 1 + sleep 2 + done + } + + # release_fence : remove the fence at the index it holds + # now, and again if the apiserver, which accepts a finalizer listed + # twice, has it twice. A failed patch is retried with a fresh read, + # because helm-controller keeps adding and dropping its own finalizer + # ahead of the fence while this runs. A release or fence already + # gone counts as released; one whose fence keeps coming back, as + # under a flux-plunger still running, gives up after a few copies. + release_fence() { + removed=0 + while :; do + if idx=$(kubectl --namespace "$1" get "$hr" "$2" --ignore-not-found --output go-template='{{`{{range $i, $f := .metadata.finalizers}}{{if eq $f "flux-plunger.cozystack.io/uninstall-fence"}}{{$i}} {{end}}{{end}}`}}'); then + idx=${idx%% *} + [ -n "$idx" ] || return 0 + [ "$removed" -lt 8 ] || return 1 + case "$idx" in + *[!0-9]*) echo "unexpected finalizer index '$idx' on $1/$2" >&2 ;; + *) + if kubectl --namespace "$1" patch "$hr" "$2" --type json \ + --patch "[{\"op\":\"test\",\"path\":\"/metadata/finalizers/$idx\",\"value\":\"$fence\"},{\"op\":\"remove\",\"path\":\"/metadata/finalizers/$idx\"}]"; then + removed=$((removed + 1)) + continue + fi ;; + esac + fi + [ "$(date +%s)" -lt "$retry_until" ] || return 1 + sleep 2 + done + } + + # flux-plunger gives up its fences itself when it stops: it lifts its + # suspensions and waits, within its grace period, for helm-controller + # to uninstall each release being deleted. So stop it and wait for + # the pod to go before sweeping. A scale-down that fails exits before + # any fence is touched, since a flux-plunger still running could set a + # fence behind the sweep and the retried uninstall should start from + # the same state; a Deployment already gone has nothing to stop. + if ! present=$(retry kubectl --namespace "$NAMESPACE" get deployment "$deploy" --ignore-not-found --output name); then + echo "could not read the $deploy Deployment; no fence was touched" >&2 + exit 1 + fi + if [ -z "$present" ]; then + echo "no $deploy Deployment in $NAMESPACE; nothing to stop" + elif ! retry kubectl --namespace "$NAMESPACE" scale deployment "$deploy" --replicas=0; then + echo "could not scale $deploy down; no fence was touched" >&2 + exit 1 + fi + # A pod still there after its grace period is named, not failed on: + # the usual one is stuck Terminating on a NotReady node until the node + # returns or the pod is force-deleted, and failing would fail every + # retried uninstall. A wait that failed for another reason is told + # apart by listing the pods. + wait_for=$(( retry_until - $(date +%s) )) + [ "$wait_for" -le "$pod_wait" ] || wait_for=$pod_wait + [ "$wait_for" -ge 1 ] || wait_for=1 + pods_gone=yes + if ! kubectl --namespace "$NAMESPACE" wait --for=delete pod \ + --selector app.kubernetes.io/name=flux-plunger,app.kubernetes.io/instance={{ .Release.Name }} \ + --timeout="${wait_for}s"; then + if ! left_pods=$(retry kubectl --namespace "$NAMESPACE" get pod \ + --selector app.kubernetes.io/name=flux-plunger,app.kubernetes.io/instance={{ .Release.Name }} \ + --output name); then + echo "could not tell whether $deploy pods are still running; sweeping anyway" >&2 + pods_gone=no + elif [ -n "$left_pods" ]; then + # shellcheck disable=SC2086 # one line for the names + echo "$deploy pods are still running, sweeping anyway:" $left_pods >&2 + pods_gone=no + else + echo "the wait for $deploy pods to go failed, but none is left" >&2 + fi + fi + + # An unanswered listing is not an empty one. + # shellcheck disable=SC2086 # scope is one or two words on purpose + if ! fenced=$(retry kubectl get "$hr" $scope --output "go-template=$listing"); then + echo "could not list fenced HelmReleases; no fence was released" >&2 + exit 1 + fi + + # What is left is what the drain could not finish: flux-plunger did + # not stop gracefully, or helm-controller did not uninstall a release + # being deleted within the grace period. Each loses flux-plunger's + # suspension and the fence without waiting any longer. One being + # deleted that helm-controller's own finalizer no longer holds, and + # whose latest generation it has not observed, is then deleted + # without an uninstall, and is named as such. + total=0 + released=0 + unhandled=0 + for ref in $fenced; do + total=$((total + 1)) + hns=${ref%%/*} + name=${ref#*/} + lift_suspension "$hns" "$name" + case $? in + 2) + echo "NOT released $ref: it could not be read" >&2 + continue ;; + 1) + # Without the fence a deleted release that is still + # suspended is removed without an uninstall. + echo "NOT released $ref: flux-plunger's suspension could not be lifted" >&2 + continue ;; + esac + state=$(kubectl --namespace "$hns" get "$hr" "$name" --ignore-not-found --output go-template='{{`{{if .metadata.deletionTimestamp}}{{$held := false}}{{range .metadata.finalizers}}{{if eq . "finalizers.fluxcd.io"}}{{$held = true}}{{end}}{{end}}{{if and (not $held) (ne (printf "%v" .status.observedGeneration) (printf "%v" .metadata.generation))}}unhandled{{end}}{{end}}`}}') || state=unknown + if ! release_fence "$hns" "$name"; then + echo "NOT released $ref: the fence could not be removed" >&2 + continue + fi + released=$((released + 1)) + case "$state" in + unhandled) + unhandled=$((unhandled + 1)) + echo "released $fence from $ref without an uninstall: helm-controller had not handled it" ;; + unknown) + echo "released $fence from $ref; whether helm-controller had handled it could not be read" ;; + *) + echo "released $fence from $ref" ;; + esac + done + + echo "released $released of $total fenced HelmReleases, $unhandled without an uninstall" + # A flux-plunger still running could have fenced a release after the + # listing, and with the chart gone nothing would remove that fence. + if [ "$pods_gone" = no ]; then + # shellcheck disable=SC2086 # scope is one or two words on purpose + if ! again=$(retry kubectl get "$hr" $scope --output "go-template=$listing"); then + echo "could not list fenced HelmReleases again after the sweep" >&2 + exit 1 + fi + if [ -n "$again" ]; then + for ref in $again; do + echo "NOT released $ref: fenced after the sweep; a flux-plunger is still running" >&2 + done + exit 1 + fi + fi + if [ "$released" -eq "$total" ]; then + exit 0 + fi + exit 1 diff --git a/packages/system/flux-plunger/tests/deployment_test.yaml b/packages/system/flux-plunger/tests/deployment_test.yaml new file mode 100644 index 000000000..a351bef20 --- /dev/null +++ b/packages/system/flux-plunger/tests/deployment_test.yaml @@ -0,0 +1,58 @@ +suite: flux-plunger Deployment +# On SIGTERM flux-plunger stops its manager (up to 10s) and then gives up the +# uninstall fences it holds (DrainFences). The kubelet kills it once +# terminationGracePeriodSeconds is spent, so stop and drain together have to end +# inside that grace period. The next flux-plunger starts while this one is still +# terminating, so it is leader election that keeps a single writer meanwhile. +release: + name: flux-plunger + namespace: cozy-fluxcd +templates: + - templates/deployment.yaml +tests: + - it: gives the shutdown drain a deadline inside the termination grace period + asserts: + - equal: + path: spec.template.spec.terminationGracePeriodSeconds + value: 90 + - contains: + path: spec.template.spec.containers[0].args + content: --shutdown-drain-timeout=65s + + - it: derives both from one value + set: + shutdownGracePeriodSeconds: 120 + asserts: + - equal: + path: spec.template.spec.terminationGracePeriodSeconds + value: 120 + - contains: + path: spec.template.spec.containers[0].args + content: --shutdown-drain-timeout=95s + + - it: runs under leader election, the one thing that keeps the next pod from writing during the drain + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: --leader-elect + + - it: refuses a grace period that leaves the drain no time + set: + shutdownGracePeriodSeconds: 39 + asserts: + - failedTemplate: + errorPattern: shutdownGracePeriodSeconds must be between 40 and 140 + + - it: refuses a grace period that is not a whole number of seconds + set: + shutdownGracePeriodSeconds: 90.7 + asserts: + - failedTemplate: + errorPattern: shutdownGracePeriodSeconds must be a whole number of seconds + + - it: refuses a grace period the pre-delete hook cannot wait out inside its deadline + set: + shutdownGracePeriodSeconds: 141 + asserts: + - failedTemplate: + errorPattern: shutdownGracePeriodSeconds must be between 40 and 140 diff --git a/packages/system/flux-plunger/tests/rbac_test.yaml b/packages/system/flux-plunger/tests/rbac_test.yaml new file mode 100644 index 000000000..aa00815c2 --- /dev/null +++ b/packages/system/flux-plunger/tests/rbac_test.yaml @@ -0,0 +1,21 @@ +suite: flux-plunger RBAC +release: + name: flux-plunger + namespace: cozy-fluxcd +templates: + - templates/rbac.yaml +tests: + - it: may hold the leader Lease the Deployment's leader election uses, in its own namespace only + documentSelector: + path: kind + value: Role + asserts: + - equal: + path: metadata.namespace + value: cozy-fluxcd + - equal: + path: rules + value: + - apiGroups: ["coordination.k8s.io"] + resources: ["leases"] + verbs: ["get", "list", "watch", "create", "update", "patch"] diff --git a/packages/system/flux-plunger/tests/uninstall-fence-cleanup_test.yaml b/packages/system/flux-plunger/tests/uninstall-fence-cleanup_test.yaml new file mode 100644 index 000000000..1502174f2 --- /dev/null +++ b/packages/system/flux-plunger/tests/uninstall-fence-cleanup_test.yaml @@ -0,0 +1,132 @@ +suite: flux-plunger pre-delete uninstall-fence cleanup +# flux-plunger holds a finalizer (flux-plunger.cozystack.io/uninstall-fence) on a +# HelmRelease it is recovering. Nothing else in the tree knows that finalizer, so +# uninstalling this chart must release it, or a release deleted meanwhile stays +# Terminating forever. +release: + name: flux-plunger + namespace: cozy-fluxcd +templates: + - templates/uninstall-fence-cleanup.yaml +tests: + - it: runs before the release is deleted, bounded, and cleans itself up afterwards + documentSelector: + path: kind + value: Job + asserts: + - equal: + path: metadata.annotations["helm.sh/hook"] + value: pre-delete + # Kept after it finishes, success included, so its report can be read + # until ttlSecondsAfterFinished; the next run replaces it. + - equal: + path: metadata.annotations["helm.sh/hook-delete-policy"] + value: before-hook-creation + - equal: + path: spec.ttlSecondsAfterFinished + value: 600 + # helm-controller never sets Helm's hook output writer, and a hook with + # this annotation makes it call the nil func + # (https://github.com/fluxcd/helm-controller/issues/1562). + - notExists: + path: metadata.annotations["helm.sh/hook-output-log-policy"] + # Under helm-controller's default 300s uninstall timeout, which bounds the + # hook; hack/flux-plunger-fence-hook_test.bats holds the relation. + - equal: + path: spec.activeDeadlineSeconds + value: 240 + # One run fills the deadline, so a second pod would end the Job + # DeadlineExceeded and lose the first one's report; helm-controller + # retries the whole uninstall, with a fresh budget, instead. + - equal: + path: spec.backoffLimit + value: 0 + # Same pull policy and fixed footprint as the other hooks that run this + # image, so the pod is scheduled and started promptly inside the deadline. + - equal: + path: spec.template.spec.containers[0].imagePullPolicy + value: IfNotPresent + - equal: + path: spec.template.spec.containers[0].resources + value: + requests: + cpu: 10m + memory: 32Mi + limits: + cpu: 100m + memory: 64Mi + - equal: + path: spec.template.spec.serviceAccountName + value: flux-plunger-uninstall-fence-cleanup + # Digest-pinned so an upstream retag does not change what runs. + - equal: + path: spec.template.spec.containers[0].image + value: docker.io/clastix/kubectl:v1.32@sha256:b9ef7d8dbe65bcc81a46c09b8dc7543103055021c4f43287bf59e92a8f4fe05c + + - it: strips the fence by name and stops flux-plunger first + documentSelector: + path: kind + value: Job + asserts: + - matchRegex: + path: spec.template.spec.containers[0].command[2] + pattern: 'fence=flux-plunger\.cozystack\.io/uninstall-fence' + - matchRegex: + path: spec.template.spec.containers[0].command[2] + pattern: 'scale deployment "\$deploy" --replicas=0' + - equal: + path: spec.template.spec.containers[0].env[0].name + value: NAMESPACE + - equal: + path: spec.template.spec.containers[0].env[0].value + value: cozy-fluxcd + + - it: may read and patch HelmReleases cluster-wide, and nothing more there + documentSelector: + path: kind + value: ClusterRole + asserts: + - equal: + path: rules + value: + - apiGroups: ["helm.toolkit.fluxcd.io"] + resources: ["helmreleases"] + verbs: ["get", "list", "patch"] + + - it: may scale down only its own Deployment and watch its pods go + documentSelector: + path: kind + value: Role + asserts: + - equal: + path: metadata.namespace + value: cozy-fluxcd + - equal: + path: rules + value: + - apiGroups: ["apps"] + resources: ["deployments"] + resourceNames: ["flux-plunger"] + verbs: ["get"] + - apiGroups: ["apps"] + resources: ["deployments/scale"] + resourceNames: ["flux-plunger"] + verbs: ["patch"] + - apiGroups: [""] + resources: ["pods"] + verbs: ["get", "list", "watch"] + + - it: runs under the restricted pod security standard + documentSelector: + path: kind + value: Job + asserts: + - equal: + path: spec.template.spec.securityContext.runAsNonRoot + value: true + - equal: + path: spec.template.spec.containers[0].securityContext.allowPrivilegeEscalation + value: false + - equal: + path: spec.template.spec.containers[0].securityContext.readOnlyRootFilesystem + value: true diff --git a/packages/system/flux-plunger/values.yaml b/packages/system/flux-plunger/values.yaml index 1da126104..69e9f1465 100644 --- a/packages/system/flux-plunger/values.yaml +++ b/packages/system/flux-plunger/values.yaml @@ -1 +1,23 @@ image: "ghcr.io/cozystack/cozystack/flux-plunger:latest@sha256:6a6ec938973e6583c5e1573f7d25beddc1852a6699a089116b2407e29a564a72" + +# How long flux-plunger is given to stop, between 40 and 140. On SIGTERM it stops +# its manager (up to 10 seconds), then gives up the uninstall fences it holds, +# waiting for helm-controller to uninstall a release being deleted, under a +# deadline 25 seconds shorter than this. Its leader Lease outlasts both, so the +# next flux-plunger waits a little under two minutes at the default before it +# acts. The pre-delete hook waits for the pod a little longer than this before it +# releases what is left. +shutdownGracePeriodSeconds: 90 + +# Image for the pre-delete uninstall-fence cleanup hook Job (see +# templates/uninstall-fence-cleanup.yaml). Only needs a kubectl binary. The tag +# is digest-pinned so an upstream retag does not change what runs across the +# fleet; refresh by resolving the current manifest-list digest +# (`docker manifest inspect docker.io/clastix/kubectl:v1.32`) and updating +# `digest` below. Same image the etcd-operator and postgres-operator hooks pin. +# renovate: datasource=docker depName=docker.io/clastix/kubectl +kubectlImage: + repository: docker.io/clastix/kubectl + tag: v1.32 + digest: sha256:b9ef7d8dbe65bcc81a46c09b8dc7543103055021c4f43287bf59e92a8f4fe05c + pullPolicy: IfNotPresent