Security fixes: Flip FORCE_FINALIZER_ON_TIMEOUT default from true to false

.claude/CHANGELOG.md

@@ -9,6 +9,76 @@ The format is based on the regulated environment requirements:
 
 ---
 
+## [2026-04-25 18:00] - Phase 2 of security audit: finalizer cleanup force-remove on PDB stall
+
+**Author:** Erick Bourgeois
+
+### Changed
+- `src/reconcilers/helpers.rs` (`handle_deletion`): refactor the timeout
+  branch from `tokio::time::timeout(...).await.map_err(...)??` (which
+  silently propagated the `TimeoutError` and **prevented** the finalizer
+  from being removed) into an explicit 3-arm match over a new
+  `CleanupOutcome` enum. The default mode now force-removes the
+  finalizer on timeout so namespace deletion is unblocked, surfaces a
+  `FinalizerCleanupTimedOut` Warning event on the SM, and increments
+  `fivespot_finalizer_cleanup_timeouts_total`. A real cleanup error (as
+  opposed to a timeout) still propagates and is retried — the finalizer
+  is kept in place so a transient API failure does not orphan
+  resources.
+- `src/reconcilers/helpers.rs`: new pure helpers `run_cleanup_with_timeout`
+  (returns `CleanupOutcome::{Completed, Failed(err), TimedOut}`) and
+  `build_finalizer_timeout_event`. Both are unit-testable without the
+  kube API; the timeout test runs in microseconds via
+  `#[tokio::test(start_paused = true)]`.
+- `src/reconcilers/scheduled_machine.rs` (`Context`): new
+  `force_finalizer_on_timeout: bool` field defaulting to `true`, plus a
+  `with_force_finalizer_on_timeout(bool)` builder for `main.rs` to
+  override from CLI/env. Strict-cleanup mode (`false`) keeps the
+  finalizer and propagates `TimeoutError` so reconciliation retries —
+  only safe when an external sweep garbage-collects stuck SMs.
+- `src/main.rs`: new `--force-finalizer-on-timeout` flag (env
+  `FORCE_FINALIZER_ON_TIMEOUT`, default `true`) wired through `Context`.
+- `src/metrics.rs`: new `FINALIZER_CLEANUP_TIMEOUTS_TOTAL` counter +
+  `record_finalizer_cleanup_timeout()` helper + label-less
+  `fallback_counter` constructor. Documented as an operator-alert
+  signal for orphan-resource detection.
+- `deploy/deployment/deployment.yaml`: added `FORCE_FINALIZER_ON_TIMEOUT`
+  env var with `value: "false"` (strict-cleanup mode) per operational
+  preference; the binary default remains `true`. Block-comment in the
+  manifest documents the trade-off and references the troubleshooting
+  runbook.
+- `docs/src/operations/troubleshooting.md`: new "Orphan resources after
+  finalizer timeout" section with the runbook (find orphan Machines via
+  ownerRef walk, inspect bootstrap/infra refs, cascading delete via
+  Machine, prevention guidance).
+- `src/reconcilers/helpers_tests.rs` / `src/metrics_tests.rs`: 10 new
+  tests covering `run_cleanup_with_timeout` (Completed/Failed/TimedOut),
+  `build_finalizer_timeout_event` (severity, reason, action,
+  note-content invariants), `Context.force_finalizer_on_timeout`
+  defaulting, and the metric-increments-on-call contract.
+
+### Why
+Finding F-007 from the 2026-04-25 adversarial security audit (filed in
+`~/dev/roadmaps/5spot-security-audit-2026-04-25.md`, Phase 2). A
+namespace-tenant with `create pods` + `create poddisruptionbudgets` —
+common tenant grants — could plant a `minAvailable: 999` PDB on a
+workload they own, schedule a machine on the same node, then delete
+the SM. Drain blocks indefinitely on the impossible eviction; with the
+old code the finalizer was never removed, the SM was stuck with
+`deletionTimestamp`, and namespace deletion stalled. The fix unblocks
+namespace deletion in the default mode while preserving
+strict-cleanup-or-stall semantics for operators with an external sweep.
+
+### Impact
+- [ ] Breaking change
+- [ ] Requires cluster rollout
+- [x] Config change only (new env var; behaviour change for clusters
+      that hit the timeout path — they'll now succeed at deletion
+      where they previously stalled)
+- [ ] Documentation only
+
+---
+
 ## [2026-04-25 06:30] - `gitleaks-install` now wires the local pre-commit hook
 
 **Author:** Erick Bourgeois

deploy/deployment/deployment.yaml

@@ -69,6 +69,28 @@ spec:
               value: "10"
             - name: LEASE_RETRY_PERIOD_SECONDS
               value: "2"
+            # Finalizer-cleanup timeout behaviour.
+            #
+            # When "true" (binary default): on cleanup timeout the
+            # controller force-removes the finalizer so namespace
+            # deletion is unblocked. May leave orphan CAPI Machine /
+            # bootstrap / infra resources — operators must reconcile
+            # via the runbook in
+            # docs/src/operations/troubleshooting.md
+            # ("Orphan resources after finalizer timeout").
+            #
+            # When "false" (set here): the SM stays stuck with its
+            # finalizer until cleanup succeeds. Namespace deletion will
+            # stall on a misconfigured PodDisruptionBudget — only safe
+            # when an external sweep is in place to garbage-collect
+            # stalled SMs.
+            #
+            # The fivespot_finalizer_cleanup_timeouts_total metric and
+            # the FinalizerCleanupTimedOut Warning event fire in BOTH
+            # modes; the only difference is whether the finalizer is
+            # removed on timeout.
+            - name: FORCE_FINALIZER_ON_TIMEOUT
+              value: "false"
           ports:
             - name: metrics
               containerPort: 8080

docs/src/operations/troubleshooting.md

@@ -202,6 +202,74 @@ kubectl logs -n 5spot-system -l app=5spot-controller -f | \
   kubectl annotate scheduledmachine <name> 5spot.finos.org/force-reconcile="$(date -u +%s)" --overwrite
   ```
 
+### Orphan resources after finalizer timeout
+
+**Symptom:**
+- `fivespot_finalizer_cleanup_timeouts_total` increments above zero.
+- A `kubectl describe scheduledmachine <name>` shows a Warning event with reason `FinalizerCleanupTimedOut`.
+- The ScheduledMachine has been deleted (gone from `kubectl get`) but a CAPI Machine, bootstrap resource, or infrastructure resource may still exist in the namespace.
+
+**Root cause.**
+`handle_deletion` wraps CAPI cleanup in a hard timeout
+(`FINALIZER_CLEANUP_TIMEOUT_SECS`, default 600s / 10 minutes) so a hung
+eviction cannot stall namespace deletion. By default
+(`--force-finalizer-on-timeout=true`, env `FORCE_FINALIZER_ON_TIMEOUT=true`)
+the controller force-removes its finalizer when the timeout fires —
+unblocking namespace deletion at the cost of potentially leaving CAPI
+resources without a managing ScheduledMachine. The most common trigger
+is a **misconfigured Pod Disruption Budget** (e.g. `minAvailable: 999`)
+on a workload the controller is trying to evict during node drain.
+
+**Runbook.**
+
+1. Find the orphaned CAPI Machine:
+   ```bash
+   # Machines from this namespace whose owning ScheduledMachine no longer exists.
+   kubectl get machines.cluster.x-k8s.io -n <ns> -o json \
+     | jq -r '.items[] | select(.metadata.ownerReferences[]?.kind == "ScheduledMachine")
+              | .metadata.name'
+   for m in $(kubectl get machines.cluster.x-k8s.io -n <ns> -o name); do
+     owner=$(kubectl get $m -n <ns> -o jsonpath='{.metadata.ownerReferences[?(@.kind=="ScheduledMachine")].name}')
+     if [ -n "$owner" ] && ! kubectl get scheduledmachine "$owner" -n <ns> >/dev/null 2>&1; then
+       echo "ORPHAN: $m (was owned by $owner)"
+     fi
+   done
+   ```
+
+2. Identify the bootstrap and infrastructure resources the orphan Machine references:
+   ```bash
+   kubectl get machine.cluster.x-k8s.io <orphan-name> -n <ns> -o jsonpath='{.spec.bootstrap.configRef}'
+   kubectl get machine.cluster.x-k8s.io <orphan-name> -n <ns> -o jsonpath='{.spec.infrastructureRef}'
+   ```
+
+3. Delete the orphan Machine first; CAPI cascades into the bootstrap / infra refs via ownerReferences:
+   ```bash
+   kubectl delete machine.cluster.x-k8s.io <orphan-name> -n <ns>
+   ```
+   If the Machine itself is stuck terminating (drain still blocked),
+   inspect Pods + PDBs on the underlying Node and remove the offending
+   PDB before retrying.
+
+4. Verify nothing is left behind:
+   ```bash
+   kubectl get machines.cluster.x-k8s.io,k0sworkerconfigs.k0smotron.io,remotemachines.k0smotron.io -n <ns>
+   ```
+
+**Prevention.**
+
+- Alert on `rate(fivespot_finalizer_cleanup_timeouts_total[5m]) > 0`.
+- Validate Pod Disruption Budgets at admission (CEL
+  `ValidatingAdmissionPolicy`) — reject `minAvailable` values that exceed
+  the workload's replica count.
+- For environments where stalled SMs are preferable to potential
+  orphans (e.g. a sweep job is in place), set
+  `--force-finalizer-on-timeout=false` (env
+  `FORCE_FINALIZER_ON_TIMEOUT=false`). The metric and Warning event
+  fire in both modes; the only difference is whether the finalizer is
+  removed on timeout. **Strict mode requires an external sweep** to
+  garbage-collect SMs whose drain is permanently blocked, otherwise
+  namespace deletion stalls indefinitely.
+
 ## Emergency Reclaim (Kill Switch)
 
 See [Emergency Reclaim](../concepts/emergency-reclaim.md) for the full lifecycle.

src/main.rs

@@ -104,6 +104,15 @@ struct Cli {
     /// Retry period in seconds — documented for ops parity; not a direct `LeaseManager` parameter
     #[clap(long, env = "LEASE_RETRY_PERIOD_SECONDS", default_value_t = DEFAULT_LEASE_RETRY_PERIOD_SECS)]
     _lease_retry_period_secs: u64,
+
+    /// On finalizer-cleanup timeout, force-remove the finalizer so namespace
+    /// deletion is unblocked (default). Set to `false` for strict-cleanup
+    /// mode where the SM stays stuck until cleanup succeeds — operators
+    /// then need an external sweep to garbage-collect stalled SMs. The
+    /// `fivespot_finalizer_cleanup_timeouts_total` metric and the
+    /// `FinalizerCleanupTimedOut` Warning event fire in both modes.
+    #[clap(long, env = "FORCE_FINALIZER_ON_TIMEOUT", default_value_t = true)]
+    force_finalizer_on_timeout: bool,
 }
 
 /// Async entry point.
@@ -167,11 +176,10 @@ async fn main() -> Result<()> {
     health_state.set_k8s_connected(true);
 
     // Create shared context
-    let context = Arc::new(Context::new(
-        client.clone(),
-        cli.instance_id,
-        cli.instance_count,
-    ));
+    let context = Arc::new(
+        Context::new(client.clone(), cli.instance_id, cli.instance_count)
+            .with_force_finalizer_on_timeout(cli.force_finalizer_on_timeout),
+    );
 
     // Leader election (Basel III HA — P2-4)
     //

src/metrics.rs

@@ -30,8 +30,8 @@
 use std::sync::LazyLock;
 
 use prometheus::{
-    register_counter_vec, register_gauge, register_gauge_vec, register_histogram_vec, CounterVec,
-    Gauge, GaugeVec, HistogramVec, Opts,
+    register_counter, register_counter_vec, register_gauge, register_gauge_vec,
+    register_histogram_vec, Counter, CounterVec, Gauge, GaugeVec, HistogramVec, Opts,
 };
 
 // ============================================================================
@@ -69,6 +69,13 @@ fn fallback_gauge(name: &str, help: &str) -> Gauge {
         .unwrap_or_else(|e| panic!("{FALLBACK_METRIC_BUG_MSG}: name={name:?} err={e}"))
 }
 
+/// Create an *unregistered* `Counter` (label-less) used as a no-op fallback
+/// when `register_counter!` fails.
+fn fallback_counter(name: &str, help: &str) -> Counter {
+    Counter::new(name, help)
+        .unwrap_or_else(|e| panic!("{FALLBACK_METRIC_BUG_MSG}: name={name:?} err={e}"))
+}
+
 /// Create an *unregistered* `GaugeVec` used as a no-op fallback when
 /// `register_gauge_vec!` fails.
 fn fallback_gauge_vec(name: &str, help: &str, labels: &[&str]) -> GaugeVec {
@@ -280,6 +287,33 @@ pub static POD_EVICTIONS_TOTAL: LazyLock<CounterVec> = LazyLock::new(|| {
     })
 });
 
+/// Finalizer cleanup timeouts during deletion handling.
+///
+/// Incremented every time `handle_deletion` exceeds
+/// [`crate::constants::FINALIZER_CLEANUP_TIMEOUT_SECS`] while removing a
+/// machine from its cluster. A non-zero value typically indicates a
+/// misconfigured Pod Disruption Budget on a workload that the controller
+/// is trying to evict — the controller force-removes the finalizer to
+/// unblock namespace deletion, but this metric tells operators an
+/// orphaned CAPI Machine + bootstrap/infrastructure resources may need
+/// manual cleanup.
+///
+/// Alert when the rate is non-zero. See
+/// `docs/src/operations/troubleshooting.md` for the orphan-cleanup runbook.
+pub static FINALIZER_CLEANUP_TIMEOUTS_TOTAL: LazyLock<Counter> = LazyLock::new(|| {
+    register_counter!(
+        "fivespot_finalizer_cleanup_timeouts_total",
+        "Total number of finalizer cleanup timeouts (force-removed; possible orphan resources)"
+    )
+    .unwrap_or_else(|e| {
+        eprintln!("WARN: Failed to register fivespot_finalizer_cleanup_timeouts_total: {e}");
+        fallback_counter(
+            "fivespot_finalizer_cleanup_timeouts_total",
+            "Total number of finalizer cleanup timeouts (force-removed; possible orphan resources)",
+        )
+    })
+});
+
 /// Record a successful reconciliation
 pub fn record_reconciliation_success(phase: &str, duration_secs: f64) {
     RECONCILIATIONS_TOTAL
@@ -330,6 +364,15 @@ pub fn record_pod_eviction(success: bool) {
     POD_EVICTIONS_TOTAL.with_label_values(&[result]).inc();
 }
 
+/// Record a finalizer-cleanup timeout (force-remove path).
+///
+/// Operators should treat any non-zero rate as a signal that orphan CAPI
+/// Machine / bootstrap / infrastructure resources may exist and need
+/// manual reconciliation.
+pub fn record_finalizer_cleanup_timeout() {
+    FINALIZER_CLEANUP_TIMEOUTS_TOTAL.inc();
+}
+
 /// Initialize controller info metric
 pub fn init_controller_info(version: &str, instance_id: u32) {
     CONTROLLER_INFO

src/metrics_tests.rs

@@ -46,6 +46,17 @@ fn test_record_pod_eviction() {
     record_pod_eviction(false);
 }
 
+#[test]
+fn test_record_finalizer_cleanup_timeout_increments_counter() {
+    let before = FINALIZER_CLEANUP_TIMEOUTS_TOTAL.get();
+    record_finalizer_cleanup_timeout();
+    let after = FINALIZER_CLEANUP_TIMEOUTS_TOTAL.get();
+    assert!(
+        after > before,
+        "FINALIZER_CLEANUP_TIMEOUTS_TOTAL must increment on call: before={before} after={after}"
+    );
+}
+
 #[test]
 fn test_init_controller_info() {
     init_controller_info("0.1.0", 0);