From 42a27567cee2bd013ba04331b43fb48199c4b148 Mon Sep 17 00:00:00 2001 From: Alexey Artamonov Date: Tue, 15 Sep 2026 23:34:52 +0300 Subject: [PATCH] fix(fluxcd): keep Helm uninstall working during flux-plunger recovery flux-plunger recovers a "has no deployed releases" HelmRelease by briefly suspending it, deleting the stale storage Secret and unsuspending. helm-controller v1.5.x skips the uninstall and drops its finalizer when it handles a delete on a suspended HelmRelease, and it wakes on the same delete event flux-plunger does, so a delete inside that window orphaned the release's DataVolume, PVCs, RBAC and storage Secret. Lifting the suspension on the delete event cannot win that race reliably. Hold the release with a flux-plunger finalizer while it is suspended. The finalizer is applied together with the suspension in one server-side apply. On a delete flux-plunger lifts its suspension and keeps the finalizer, helm-controller reconciles the unsuspended generation and runs the uninstall, and flux-plunger removes the finalizer once helm-controller's own is gone and that generation is observed. This holds whichever controller handles the delete first. A completed recovery lifts the suspension and the finalizer together, and a terminating release never begins recovery. Decide ownership of a suspension by which field manager owns spec.suspend in metadata.managedFields rather than by an annotation. The apiserver transfers the field away the moment an operator, cozyhr resume or a backup controller writes a different value to it, so a later suspension is never misread as flux-plunger's, and one set through the shared flux-client-side-apply manager (platform migrations, the tenant delete hook, a manual kubectl apply) is never claimed. A release deleted under another manager's suspension is left to that manager's decision. Acquire applies without forcing first and, on a conflict, re-fetches and forces only if spec.suspend is still false: a backup controller owns the field at false through a dynamic-client Update, which a non-forced apply can never take, while true is an active suspension to leave alone. Every apply is preconditioned on the resourceVersion and uid of the read that decided on it. Without the resourceVersion, the unforced apply of true over a foreign true is accepted as shared ownership, a window the optimistic-locked merge patch this replaces had closed, and lifting the suspension and the finalizer from a release read as live would delete it without an uninstall if helm-controller had meanwhile handled a delete. Without the uid, an apply to a release that has gone would create an empty one. The apply carries only spec.suspend and the finalizer, so it cannot leak the required spec.interval. Read the release, every re-check and the storage Secret list from the apiserver rather than the informer cache. A lagging cache would let the processed-version guard delete the revision below the one just reclaimed, and the ClusterRole grants secrets get and list but not the watch an informer needs. Emit a Warning when flux-plunger holds a suspension it failed to clear, and Normal events when it leaves an external suspension alone or defers a recovery. An external suspension on a release flux-plunger has recovered before is a Warning instead: it is most likely one an earlier flux-plunger left under the shared manager, which nothing lifts unless an operator does. A release deleted under another manager's suspension is a Normal event when that was a migration's choice, and a Warning when the release carries flux-plunger's processed-version annotation, since then the skipped uninstall is an accident. Keep a single writer with leader election. A second process able to act under the same field manager could clear a suspension the first is still recovering, and no rollout strategy prevents one: 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. The old pod keeps its Lease, unreleased, through its stop and drain, and the Lease outlasts both, so the next pod acts only after the old one is done; the price is that a new pod waits for the Lease to run out. The strategy sets maxSurge 0 with maxUnavailable 1 and stays RollingUpdate because existing installs carry the apiserver-defaulted rollingUpdate block and helm-controller applies server-side, where type Recreate over it is rejected on upgrade. Give the finalizer a way out when flux-plunger itself goes. Once the manager has stopped, within a bounded time, and with it every reconcile, flux-plunger drains its finalizers under a deadline that, with the stop, fits inside the pod's termination grace period. A stop that overran its bound is drained too and still reported, and only a pod that led drains. The drain writes its events straight to the apiserver, since the manager's recorder stops with the manager. A recovery in progress is aborted, the suspension and the finalizer lifted together for the next flux-plunger to start again, and a release being deleted has the suspension lifted and keeps the finalizer until helm-controller has run the uninstall. A release helm-controller does not handle in time keeps it. The waiting lives in the controller, where the envtest suites hold it against a real apiserver's generations and finalizers, rather than in a shell script against a model of them. A pre-delete hook on the chart is the backstop for what the drain could not finish, so uninstalling the component never leaves a release that tenant teardown waits on forever. It stops flux-plunger, waits out its grace period, then lifts any remaining suspension and finalizer without waiting again, naming each release it lets go of before helm-controller uninstalled it. It lifts a suspension only where flux-plunger alone holds it, with a JSON patch preconditioned on the resourceVersion and uid it read: kubectl apply --server-side on a release last written by a client-side apply moves that apply's fields to the applying manager and strips every one the body leaves out. The Job's deadline sits under the 300s helm-controller gives a pre-delete hook by default. A Deployment that cannot be stopped or a listing that never succeeds ends the hook before any finalizer is touched, and a finalizer it cannot remove ends it after the sweep; either way it exits non-zero: a failed pre-delete hook leaves the release untouched and helm-controller retries the uninstall, hook included, from a state it can still converge from, where exiting 0 would remove the component and strand the finalizer. A flux-plunger pod still present after its grace period, typically one stuck Terminating on a NotReady node, is only reported, since failing on it would fail every retry until the node returns; because it may still be setting finalizers, they are listed again after the sweep and any found fails the hook. The Job runs once, since helm-controller's retry is the one that starts afresh, and is kept after success so that its report stays readable; helm.sh/hook-output-log-policy would make helm-controller call a nil output writer (fluxcd/helm-controller#1562). A delete still held after ten minutes raises a Warning, and the held state is re-checked on a timer instead of only on helm-controller's next status write. The chart README documents the finalizer and how to clear it. Run the envtest suites on every pull request through a new make target that fetches a kube-apiserver and etcd with a pinned setup-envtest and takes the HelmRelease CRD from the in-tree Flux manifests: the unit tests drive a fake client, and everything the finalizer rests on (apply preconditions, the finalizer set-merge, real managedFields) only holds against a real apiserver. CI caches those binaries, and the target uses them without asking the release index, so a pull request does not depend on their sources being reachable; the control plane gets a longer start timeout because it starts beside the other test goals. Assisted-by: LLM Signed-off-by: Alexey Artamonov --- .github/workflows/pull-requests.yaml | 13 +- Makefile | 25 +- cmd/flux-plunger/main.go | 29 +- hack/bats-runner-contract.bats | 2 +- hack/downstream-trigger-map.bats | 2 +- hack/flux-plunger-fence-hook_test.bats | 728 ++++++ hack/unit-test-parallelism.bats | 2 +- internal/controller/fluxplunger/drain.go | 239 ++ .../fluxplunger/drain_envtest_test.go | 323 +++ .../controller/fluxplunger/flux_plunger.go | 710 +++++- .../fluxplunger/flux_plunger_test.go | 2081 +++++++++++++++++ .../fluxplunger/hook_contract_test.go | 111 + .../controller/fluxplunger/shutdown_test.go | 265 +++ .../fluxplunger/ssa_ownership_envtest_test.go | 722 ++++++ packages/system/flux-plunger/Makefile | 4 + packages/system/flux-plunger/README.md | 31 + .../flux-plunger/templates/deployment.yaml | 31 + .../system/flux-plunger/templates/rbac.yaml | 32 + .../templates/uninstall-fence-cleanup.yaml | 391 ++++ .../flux-plunger/tests/deployment_test.yaml | 58 + .../system/flux-plunger/tests/rbac_test.yaml | 21 + .../tests/uninstall-fence-cleanup_test.yaml | 132 ++ packages/system/flux-plunger/values.yaml | 22 + 23 files changed, 5844 insertions(+), 130 deletions(-) create mode 100644 hack/flux-plunger-fence-hook_test.bats create mode 100644 internal/controller/fluxplunger/drain.go create mode 100644 internal/controller/fluxplunger/drain_envtest_test.go create mode 100644 internal/controller/fluxplunger/flux_plunger_test.go create mode 100644 internal/controller/fluxplunger/hook_contract_test.go create mode 100644 internal/controller/fluxplunger/shutdown_test.go create mode 100644 internal/controller/fluxplunger/ssa_ownership_envtest_test.go create mode 100644 packages/system/flux-plunger/README.md create mode 100644 packages/system/flux-plunger/templates/uninstall-fence-cleanup.yaml create mode 100644 packages/system/flux-plunger/tests/deployment_test.yaml create mode 100644 packages/system/flux-plunger/tests/rbac_test.yaml create mode 100644 packages/system/flux-plunger/tests/uninstall-fence-cleanup_test.yaml 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