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 <aleksei.artamonov@aenix.io>

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

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

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) {

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.

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

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