From 56dd6c2e413db2ede4e46f76ff384776d2002e0e Mon Sep 17 00:00:00 2001 From: Alexey Artamonov Date: Tue, 13 Oct 2026 14:06:57 +0300 Subject: [PATCH] fix(fluxcd): refuse a flux-plunger drain deadline that leaves no time A drain deadline of zero or less is already spent when the drain starts, so flux-plunger would exit holding every uninstall fence without listing one. The chart could produce one: its 40-second floor was a literal, so raising the manager's stop bound or the margin before SIGKILL, which the drain subtraction is already tied to, would leave the floor rendering a non-positive deadline. The binary now refuses such a flag at startup, before it reaches for a cluster, and the chart's floor and the timings the README and the values comment state are held to the same constants. A whole number of a million seconds or more printed in exponent form and was refused as a fraction; the range is now checked first, so it gets the range message. Assisted-by: LLM Signed-off-by: Alexey Artamonov (cherry picked from commit 8bcb709ce80a38ad936b44a307da8a1f38d61061) --- cmd/flux-plunger/main.go | 5 ++ internal/controller/fluxplunger/drain.go | 10 +++ .../fluxplunger/hook_contract_test.go | 75 +++++++++++++++++++ .../controller/fluxplunger/shutdown_test.go | 15 ++++ .../flux-plunger/templates/deployment.yaml | 6 +- .../flux-plunger/tests/deployment_test.yaml | 9 +++ 6 files changed, 117 insertions(+), 3 deletions(-) diff --git a/cmd/flux-plunger/main.go b/cmd/flux-plunger/main.go index a45ef71f3..72fd89ecb 100644 --- a/cmd/flux-plunger/main.go +++ b/cmd/flux-plunger/main.go @@ -82,6 +82,11 @@ func main() { ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) + if err := fluxplunger.ValidateShutdownDrainTimeout(shutdownDrainTimeout); err != nil { + setupLog.Error(err, "invalid flag") + os.Exit(1) + } + // if the enable-http2 flag is false (the default), http/2 should be disabled // due to its vulnerabilities. More specifically, disabling http/2 will // prevent from being vulnerable to the HTTP/2 Stream Cancellation and diff --git a/internal/controller/fluxplunger/drain.go b/internal/controller/fluxplunger/drain.go index 53eb93e5f..4c8acec07 100644 --- a/internal/controller/fluxplunger/drain.go +++ b/internal/controller/fluxplunger/drain.go @@ -27,6 +27,16 @@ const ( ShutdownDrainTimeoutFlag = "shutdown-drain-timeout" ) +// ValidateShutdownDrainTimeout refuses a drain deadline that is already spent +// when the drain starts: the drain would leave every fence behind without +// listing one. +func ValidateShutdownDrainTimeout(d time.Duration) error { + if d <= 0 { + return fmt.Errorf("--%s must be positive, got %v", ShutdownDrainTimeoutFlag, d) + } + return nil +} + // 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 diff --git a/internal/controller/fluxplunger/hook_contract_test.go b/internal/controller/fluxplunger/hook_contract_test.go index 1c9b0559f..7fc4c5732 100644 --- a/internal/controller/fluxplunger/hook_contract_test.go +++ b/internal/controller/fluxplunger/hook_contract_test.go @@ -2,6 +2,7 @@ package fluxplunger import ( "os" + "os/exec" "regexp" "strconv" "strings" @@ -68,6 +69,80 @@ func TestDeploymentTemplate_SubtractsTheStopAndTheMarginTheBinaryUses(t *testing } } +// The binary must refuse a drain deadline it could not drain under, whatever +// the chart or a hand-written Deployment passes, and before it reaches for a +// cluster: the run has no kubeconfig, so a binary that skipped the check fails +// on that instead, with another message. +func TestMain_RefusesADrainTimeoutThatLeavesNoTime(t *testing.T) { + bin := t.TempDir() + "/flux-plunger" + if out, err := exec.Command("go", "build", "-o", bin, "../../../cmd/flux-plunger").CombinedOutput(); err != nil { + t.Fatalf("build flux-plunger: %v\n%s", err, out) + } + cmd := exec.Command(bin, "--"+ShutdownDrainTimeoutFlag+"=0s") + cmd.Env = append(os.Environ(), "KUBECONFIG=/dev/null", "KUBERNETES_SERVICE_HOST=") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatalf("flux-plunger started with a drain deadline of 0s:\n%s", out) + } + if want := "--" + ShutdownDrainTimeoutFlag + " must be positive"; !strings.Contains(string(out), want) { + t.Fatalf("flux-plunger did not refuse the deadline itself, want %q in:\n%s", want, out) + } +} + +// The chart's lowest accepted grace period has to leave the drain time after the +// manager's stop and the margin before SIGKILL, or raising either constant makes +// the chart render a drain deadline of zero or less at its own minimum. +func TestDeploymentTemplate_TheLowestGracePeriodLeavesTheDrainTime(t *testing.T) { + raw, err := os.ReadFile(deploymentTemplate) + if err != nil { + t.Fatalf("read %s: %v", deploymentTemplate, err) + } + m := regexp.MustCompile(`\(lt \$grace (\d+)\)`).FindStringSubmatch(string(raw)) + if m == nil { + t.Fatal("deployment has no lower bound on shutdownGracePeriodSeconds") + } + floor, err := strconv.Atoi(m[1]) + if err != nil { + t.Fatalf("lower bound %q: %v", m[1], err) + } + if drain := time.Duration(floor)*time.Second - managerStopTimeout - killMargin; drain <= 0 { + t.Fatalf("at the lowest grace period %ds the drain gets %v (manager stop %v, margin %v)", floor, drain, managerStopTimeout, killMargin) + } + if want := strconv.Itoa(int(managerStopTimeout/time.Second)) + "s stop"; !strings.Contains(string(raw), want) { + t.Errorf("the range message does not name the manager's %q", want) + } +} + +// The README and the values comment restate the drain's timings for operators; +// they must name the numbers the binary and the chart use, or a change to either +// leaves the prose stating the old ones. +func TestDocs_StateTheTimingsTheBinaryAndTheChartUse(t *testing.T) { + raw, err := os.ReadFile(deploymentTemplate) + if err != nil { + t.Fatalf("read %s: %v", deploymentTemplate, err) + } + m := regexp.MustCompile(`\(lt \$grace (\d+)\) \(gt \$grace (\d+)\)`).FindStringSubmatch(string(raw)) + if m == nil { + t.Fatal("deployment has no range on shutdownGracePeriodSeconds") + } + want := []string{ + "between " + m[1] + " and " + m[2], + strconv.Itoa(int(managerStopTimeout/time.Second)) + " seconds", + strconv.Itoa(int((managerStopTimeout+killMargin)/time.Second)) + " seconds", + } + for _, doc := range []string{"../../../packages/system/flux-plunger/README.md", "../../../packages/system/flux-plunger/values.yaml"} { + text, err := os.ReadFile(doc) + if err != nil { + t.Fatalf("read %s: %v", doc, err) + } + for _, w := range want { + if !strings.Contains(string(text), w) { + t.Errorf("%s does not state %q", doc, w) + } + } + } +} + // 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) { diff --git a/internal/controller/fluxplunger/shutdown_test.go b/internal/controller/fluxplunger/shutdown_test.go index 9ed044892..13cf68c91 100644 --- a/internal/controller/fluxplunger/shutdown_test.go +++ b/internal/controller/fluxplunger/shutdown_test.go @@ -177,6 +177,21 @@ func TestApplyShutdownTimings_TheLeaseOutlastsTheStopAndTheDrain(t *testing.T) { } } +// A drain deadline that is not positive is already spent when the drain starts, +// so every fence would be left behind without one listing. +func TestValidateShutdownDrainTimeout_RefusesADeadlineThatLeavesNoTime(t *testing.T) { + for _, d := range []time.Duration{0, -5 * time.Second} { + if err := ValidateShutdownDrainTimeout(d); err == nil { + t.Errorf("drain timeout %v accepted", d) + } + } + for _, d := range []time.Duration{time.Second, 65 * time.Second} { + if err := ValidateShutdownDrainTimeout(d); err != nil { + t.Errorf("drain timeout %v refused: %v", d, err) + } + } +} + // 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. diff --git a/packages/system/flux-plunger/templates/deployment.yaml b/packages/system/flux-plunger/templates/deployment.yaml index fd856d251..8f4b35a27 100644 --- a/packages/system/flux-plunger/templates/deployment.yaml +++ b/packages/system/flux-plunger/templates/deployment.yaml @@ -22,12 +22,12 @@ spec: 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 }} + {{- if ne (toString .Values.shutdownGracePeriodSeconds) (toString $grace) }} + {{- fail "shutdownGracePeriodSeconds must be a whole number of seconds: terminationGracePeriodSeconds takes an integer" }} + {{- end }} replicas: 1 strategy: type: RollingUpdate diff --git a/packages/system/flux-plunger/tests/deployment_test.yaml b/packages/system/flux-plunger/tests/deployment_test.yaml index a351bef20..23dfdf2c8 100644 --- a/packages/system/flux-plunger/tests/deployment_test.yaml +++ b/packages/system/flux-plunger/tests/deployment_test.yaml @@ -56,3 +56,12 @@ tests: asserts: - failedTemplate: errorPattern: shutdownGracePeriodSeconds must be between 40 and 140 + + # A whole number of a million or more prints in exponent form, which the + # whole-number check would misread as a fraction. + - it: names the range, not the fraction, for a whole number far above it + set: + shutdownGracePeriodSeconds: 1000000 + asserts: + - failedTemplate: + errorPattern: shutdownGracePeriodSeconds must be between 40 and 140