diff --git a/.github/workflows/chart.yml b/.github/workflows/chart.yml index ec78e4f5..75021d54 100644 --- a/.github/workflows/chart.yml +++ b/.github/workflows/chart.yml @@ -231,6 +231,33 @@ jobs: echo "engine auto-wired advisory despite feedSync.enabled=false"; exit 1 fi echo "ok" + - name: "Assert liveness/readiness probes tolerate a sweep-CPU burst (JEF-560)" + run: | + # JEF-560: a protector container was crashlooping (Ready never latching, restart + # count climbing, clean exit 0) because the default 1s probe timeout / 3-strike + # (30s) budget was too tight for the CPU the per-pass signing/provenance sweep + # legitimately bursts under the chart's CPU limit — kubelet SIGTERM'd a healthy, + # working engine. Both probes must carry an explicit, more forgiving timeout + + # failure budget so a busy-but-alive process isn't mistaken for a dead one. + render=$(helm template protector charts/protector --namespace protector) + live=$(echo "$render" | awk '/livenessProbe:/,/readinessProbe:/') + echo "$live" | grep -q 'timeoutSeconds: 5' \ + || { echo "livenessProbe missing the widened timeoutSeconds"; exit 1; } + echo "$live" | grep -q 'failureThreshold: 6' \ + || { echo "livenessProbe missing the widened failureThreshold"; exit 1; } + # readinessProbe through the container's own resources block (bounded by the + # NEXT section, `env:`) — `resources:` alone isn't unique enough to scope on + # (RBAC rules and the feed-fetcher sidecar both render their own). + ready=$(echo "$render" | awk '/readinessProbe:/,/^ env:/') + echo "$ready" | grep -q 'timeoutSeconds: 5' \ + || { echo "readinessProbe missing the widened timeoutSeconds"; exit 1; } + echo "$ready" | grep -q 'failureThreshold: 3' \ + || { echo "readinessProbe missing an explicit failureThreshold"; exit 1; } + # The CPU limit that produced the throttling-induced probe misses also needs + # headroom (the RAM-tight request is unchanged; only the burst ceiling moves). + echo "$ready" | grep -q 'cpu: 500m' \ + || { echo "cpu limit not raised past the JEF-560 250m ceiling"; exit 1; } + echo "ok" - name: Assert the feed-fetcher sidecar is unprivileged and has NO apiserver access run: | # JEF-238 egress boundary: the sidecar is the ONLY container with egress. It must diff --git a/charts/protector/README.md b/charts/protector/README.md index d36c7830..ece94d0a 100644 --- a/charts/protector/README.md +++ b/charts/protector/README.md @@ -273,7 +273,7 @@ Requires the `protector-agent` image and probes load-tested on your kernel (see | `feedSync.epssUrl` | FIRST.org EPSS scores CSV (gzipped) | EPSS source (gzipped CSV, gunzipped in place). See feeds section. | | `feedSync.interval` | `"12h"` | Re-fetch interval for the sidecar (a `sleep` arg, e.g. `6h`, `30m`). | | `webhook.enforcedFailurePolicy` | `Fail` | The fail-closed enforcing webhook's policy (its scope is derived from `enforceScope`). | -| `resources` | 10m/64Mi → 250m/256Mi | RAM-tight, arm64-friendly. | +| `resources` | 10m/64Mi → 500m/256Mi | RAM-tight, arm64-friendly; the CPU limit has headroom for signing-sweep bursts (JEF-560). | See [`values.yaml`](values.yaml) for the fully commented set. diff --git a/charts/protector/values.yaml b/charts/protector/values.yaml index 846d55a7..f18d05ed 100644 --- a/charts/protector/values.yaml +++ b/charts/protector/values.yaml @@ -118,25 +118,42 @@ service: # RAM-tight, arm64-friendly: a webhook that mostly idles with occasional signature # verification (network I/O to the registry + Rekor) plus the async engine loop. +# The 250m limit (JEF-560) proved too tight: a per-pass signing/provenance sweep across a +# fleet's worth of images does a burst of TLS handshakes + JSON/crypto work on the SAME cgroup +# the probe HTTP server shares, and the CFS quota can throttle the whole container — including +# the probe response — long enough to look dead. 500m gives that burst headroom without +# meaningfully changing steady-state idle usage (still requests only 10m). resources: requests: cpu: 10m memory: 64Mi limits: - cpu: 250m + cpu: 500m memory: 256Mi -# Probes hit the HTTPS server; kubelet doesn't verify the cert for probes. +# Probes hit the HTTPS server; kubelet doesn't verify the cert for probes. `/healthz` and +# `/readyz` are both served from process start (no engine-sweep dependency), so a probe +# failure here means the process is either genuinely wedged or — the JEF-560 incident — was +# merely CPU-throttled long enough to miss the default 1s timeout / 3-strike (30s) budget +# during a legitimate sweep burst. Widened timeout + failure budget so a busy-but-alive engine +# isn't mistaken for a dead one and SIGTERM'd into a restart loop; liveness (which restarts the +# container) gets the most slack, readiness (which only pulls it from admission routing) less. livenessProbe: httpGet: path: /healthz port: https scheme: HTTPS + timeoutSeconds: 5 + periodSeconds: 15 + failureThreshold: 6 readinessProbe: httpGet: path: /readyz port: https scheme: HTTPS + timeoutSeconds: 5 + periodSeconds: 10 + failureThreshold: 3 nodeSelector: {} tolerations: [] diff --git a/engine/src/engine/run_loop.rs b/engine/src/engine/run_loop.rs index 36511c17..25cd9296 100644 --- a/engine/src/engine/run_loop.rs +++ b/engine/src/engine/run_loop.rs @@ -299,6 +299,33 @@ fn build_adjudicator( } } +/// Block until the driving loop has something to do, then drain any already-queued burst so +/// it coalesces into one pass. Wakes on either a cluster change or a behavioral/audit report +/// — the behavioral/audit channels only fire when the ingest actually changed the evidence +/// store (a new observation, not a repeat), so mundane churn never reaches here. +/// +/// Returns `true` to keep looping, `false` only once `change_rx` has PERMANENTLY closed — +/// i.e. every clone of its `Sender` (each reflector task's, and the loop's own retained one) +/// has been dropped, which happens only on total shutdown, never after a single pass (JEF-560: +/// pinned by [`tests::wake_channel_survives_many_passes_and_only_closes_when_every_sender_drops`], +/// a regression test for an operational incident where the engine container was suspected — +/// wrongly, per that test — of running to completion instead of looping). +async fn wait_for_wake( + change_rx: &mut tokio::sync::mpsc::Receiver<()>, + runtime_rx: &mut tokio::sync::mpsc::Receiver<()>, + audit_rx: &mut tokio::sync::mpsc::Receiver<()>, +) -> bool { + tokio::select! { + next = change_rx.recv() => if next.is_none() { return false }, + _ = runtime_rx.recv() => {}, + _ = audit_rx.recv() => {}, + } + while change_rx.try_recv().is_ok() {} + while runtime_rx.try_recv().is_ok() {} + while audit_rx.try_recv().is_ok() {} + true +} + /// Event-driven observer: the default. Reflectors keep an in-memory store of each /// watched resource current via `list`-then-`watch` (the periodic relist is the /// resync floor ADR-0004 calls for). The engine reacts to *events* — it sits quiet @@ -691,21 +718,9 @@ pub async fn run_watch( tracing::info!("engine: watching cluster (event-driven)"); loop { - // Wake on either a cluster change or a behavioral report. The behavioral channel - // only fires when the ingest actually changed the evidence store (a new - // observation, not a repeat) — see `ingest_behavior`. So a report that tells us - // nothing new never reaches here, and we don't burn a graph rebuild + CRD lists - // for it; mundane churn (the same connections, again) is dropped at ingest. - tokio::select! { - next = change_rx.recv() => if next.is_none() { break }, - _ = runtime_rx.recv() => {}, - _ = audit_rx.recv() => {}, + if !wait_for_wake(&mut change_rx, &mut runtime_rx, &mut audit_rx).await { + break; } - // Coalesce an already-queued burst (a Deployment rollout, or several material - // reports) into one pass. - while change_rx.try_recv().is_ok() {} - while runtime_rx.try_recv().is_ok() {} - while audit_rx.try_recv().is_ok() {} let (linkerd_servers_now, linkerd_policies_now, linkerd_mtls_now) = observe::list_linkerd_authz(&client).await; diff --git a/engine/src/engine/run_loop/tests.rs b/engine/src/engine/run_loop/tests.rs index 30783d68..56ae851d 100644 --- a/engine/src/engine/run_loop/tests.rs +++ b/engine/src/engine/run_loop/tests.rs @@ -118,6 +118,77 @@ fn tier_grants_config_fails_loud_but_serves_on_valid_or_absent() { clear(); } +// JEF-560: an operational incident reported the protector container exiting cleanly +// (exit 0) and restart-looping, never reaching Ready — read from the tail of its logs as +// "the engine is doing real work, then the process just stops" and hypothesized as a +// run-to-completion bug (the driving loop returning after a single pass instead of +// blocking for the next one). These tests exercise `wait_for_wake`, the actual primitive +// `run_watch`'s `loop {}` calls each iteration, in isolation (no `kube::Client` needed) and +// disprove that hypothesis: the loop only ever stops when the `change` channel's Sender has +// been dropped EVERYWHERE it was cloned to (every reflector task, plus the loop's own +// retained handle) — never merely because one pass finished. + +/// Many passes in a row: `wait_for_wake` must keep returning `true` — i.e. the driving loop +/// keeps running — for as long as ANY clone of `change_tx` (mirroring a live reflector task) +/// is still held, exactly like `run_watch` itself holds one for its entire body. A regression +/// to "process the first pass and return" would fail this on the second iteration. +#[tokio::test(flavor = "multi_thread")] +async fn wake_channel_survives_many_passes_and_only_closes_when_every_sender_drops() { + let (change_tx, mut change_rx) = tokio::sync::mpsc::channel::<()>(64); + let (_runtime_tx, mut runtime_rx) = tokio::sync::mpsc::channel::<()>(64); + let (_audit_tx, mut audit_rx) = tokio::sync::mpsc::channel::<()>(64); + + // Mirror `run_watch`'s reflector tasks: each holds its OWN clone, independent of the + // driving loop's retained `change_tx`, and sends one tick per simulated cluster change. + let reflector_tx = change_tx.clone(); + + const PASSES: usize = 5; + for pass in 0..PASSES { + reflector_tx + .send(()) + .await + .expect("reflector clone can still send"); + assert!( + super::wait_for_wake(&mut change_rx, &mut runtime_rx, &mut audit_rx).await, + "pass {pass}: the loop must keep running while a Sender clone is alive — a \ + run-to-completion regression would return false after the very first pass" + ); + } + + // Drop every clone (the "reflector" one, then the loop's own retained original) — only + // now, with the Sender fully gone, may `wait_for_wake` report shutdown. + drop(reflector_tx); + drop(change_tx); + assert!( + !super::wait_for_wake(&mut change_rx, &mut runtime_rx, &mut audit_rx).await, + "once every Sender clone is dropped, the loop must be told to stop" + ); +} + +/// A burst of several already-queued ticks on the SAME pass coalesces into one wake, exactly +/// as the comment on `wait_for_wake` documents (a Deployment rollout's several rapid changes +/// must not fan out into a rebuild per tick). +#[tokio::test(flavor = "multi_thread")] +async fn queued_burst_coalesces_into_one_wake() { + let (change_tx, mut change_rx) = tokio::sync::mpsc::channel::<()>(64); + let (_runtime_tx, mut runtime_rx) = tokio::sync::mpsc::channel::<()>(64); + let (_audit_tx, mut audit_rx) = tokio::sync::mpsc::channel::<()>(64); + + for _ in 0..4 { + change_tx.send(()).await.expect("send queues fine"); + } + assert!( + super::wait_for_wake(&mut change_rx, &mut runtime_rx, &mut audit_rx).await, + "a queued burst still wakes the loop" + ); + // The burst was fully drained by that one call — nothing left queued for a second wake + // without a fresh send. + assert!( + change_rx.try_recv().is_err(), + "wait_for_wake must drain the whole burst, not leave a residual tick queued" + ); +} + /// The reflected element type asks the apiserver for metadata only. `metadata_api()` /// is what drives both `watcher(Api::>, _)` and /// `Api::::list_metadata` to issue `.../secrets` requests that return