fix(engine): wire ContainNode into break-glass + ledger self-revert (ADR-0040 §5) - #323
Merged
thejefflarson merged 1 commit intoAug 3, 2026
Conversation
…ADR-0040 §5) Break-glass and the standard ledger self-revert (ADR-0017/ADR-0036) previously only knew the NetworkPolicy cut shape: routing a standing ContainNode reversion through the generic network actuator would silently no-op (wrong object kind/name) and leave the node cordoned. Adds a NodeContainmentRevert seam so Engine::process's self-revert loop calls the real cordon-lift + co-resident-deny-lift path instead, gated on an attached actuator and an observed NodeFact (skips rather than fabricates when either is missing, matching node_containment's existing "no data" discipline). The ownership self-gate (revert_decision, from #322) is exercised end-to-end. Adds engine-level acceptance tests exercising the real proof/menu-resolution pipeline (boundary_break trigger (c)) with the standing cut synthesized directly into the action log, since neither the node arming rung nor node observation exist yet (a separate ticket per ADR-0040 §6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VtjoJttCvBY4dzCoE4f9vP
thejefflarson
deleted the
thejefflarson/jef-733-verify-break-glass-ledger-self-revert-actually-uncordon
branch
August 3, 2026 06:29
thejefflarson
added a commit
that referenced
this pull request
Aug 3, 2026
…-closed (ADR-0040 §5) Rework of the node-class arming PR after review found ADR-0040 §5 violations: 1. CRITICAL — removed the auto-cordon path entirely. `evaluate_apply`/`ApplyOutcome` (which called `NodeContainmentActuator::apply` once armed+in-scope+rails-clean) are replaced by `evaluate_proposal`/`ProposalOutcome`, which only ever decides whether to SURFACE a proposal. ADR-0040 §5 is explicit that a node cut is propose-first by construction (alive collateral always present) — the deterministic rails are a bound on the human-approval gate, never a replacement for it. There is now no code path in Engine that calls `NodeContainmentActuator::apply` at all; only the REVERT half (`with_node_containment_actuator`, from #323) is wired. The `applied` metric event is gone; `proposed` now also fires (level-triggered) whenever the rails are rail-clean and armed, alongside the pre-existing edge-triggered ledger proposal event. 2. HIGH — fixed the enforceScope bypass. `ActuationScope::in_scope` resolves a namespace per endpoint and returns vacuously true for ContainNode's host self-reference (which has none), so `enforceScope` confined nothing for this class. `contain_node_in_scope` confines it instead through the co-resident labelled workload set (which structurally includes the boundary-broken trigger), reusing `co_resident_denies` + the existing per-mitigation `in_scope` check on each candidate — no scope bypass on the private internals, and a host with no in-scope co-resident pod is never proposed. 3. MEDIUM — control-plane detection now fails closed: recognizes the legacy `node-role.kubernetes.io/master` label and a control-plane-shaped taint (either canonical or legacy key, any effect) as additional, independent control-plane signals, union'd with the canonical label. A node with none of the three signals remains the ordinary cordon-eligible worker default (the correct reading for vanilla/kubeadm clusters, where workers carry no role marker at all). 4. MEDIUM — rewrote the chart/README language describing `enforceRung: node` and the engine's cut-severity ladder doc to state plainly that ContainNode is eligible to PROPOSE, never to auto-apply, at any rung. 5. LOW — documented (rather than implemented) that the two-worker floor counts schedulable nodes, not Ready ones; `Node.status` stays unread per the observation adapter's metadata-only discipline. Rebased onto main (#323's NodeContainmentRevert/`with_node_fact` seam) and composed with it: `Engine::process` now refreshes `node_facts` every pass from the real observation adapter (falling back to test-seeded facts when `Snapshot::nodes` is empty), and the real `NodeContainmentActuator` is wired as the revert actuator in `run_loop.rs`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VtjoJttCvBY4dzCoE4f9vP
thejefflarson
added a commit
that referenced
this pull request
Aug 3, 2026
…fined, CP fail-closed (#324) * feat(engine): arm the node class — ArmingRung::Node, node observation, live rail wiring (ADR-0040) Rung 3 `node` (strictly above `quarantine`) arms ContainNode on the arming ladder; the chart's ClusterRole gains an always-on metadata-only `nodes` read (fleet observation for the rails) and a `nodes` patch write gated on `mode: enforce` + `enforceRung: node`, mirroring the existing posture-derived RBAC pattern. A new metadata-only Node watch (`observe::adapter::node_fact`) feeds the deterministic rails a real NodeFact fleet each pass. Engine::process now evaluates the rails (`node_containment::evaluate_apply`) against that fleet for every active ContainNode mitigation and fires the `applied`/ `rail_refused` metric events, failing closed on a host absent from the fleet (RailRefusal::UnknownNode) rather than fabricating a passing rail. Startup (engine + chart) refuses `enforceRung: node` without `mode: enforce` + a non-empty enforceScope, mirroring the existing empty-scope refusal. The generic decide()/AutoApply path still forbids ContainNode unconditionally (is_additive_live() == false); this ships ContainNode's own apply-side gate instead, armed by the rung and safety-checked by the deterministic rails rather than the generic blast-radius check. The break-glass/self-revert uncordon path is out of scope here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VtjoJttCvBY4dzCoE4f9vP * fix(engine): make ContainNode propose-only, fix scope bypass, CP fail-closed (ADR-0040 §5) Rework of the node-class arming PR after review found ADR-0040 §5 violations: 1. CRITICAL — removed the auto-cordon path entirely. `evaluate_apply`/`ApplyOutcome` (which called `NodeContainmentActuator::apply` once armed+in-scope+rails-clean) are replaced by `evaluate_proposal`/`ProposalOutcome`, which only ever decides whether to SURFACE a proposal. ADR-0040 §5 is explicit that a node cut is propose-first by construction (alive collateral always present) — the deterministic rails are a bound on the human-approval gate, never a replacement for it. There is now no code path in Engine that calls `NodeContainmentActuator::apply` at all; only the REVERT half (`with_node_containment_actuator`, from #323) is wired. The `applied` metric event is gone; `proposed` now also fires (level-triggered) whenever the rails are rail-clean and armed, alongside the pre-existing edge-triggered ledger proposal event. 2. HIGH — fixed the enforceScope bypass. `ActuationScope::in_scope` resolves a namespace per endpoint and returns vacuously true for ContainNode's host self-reference (which has none), so `enforceScope` confined nothing for this class. `contain_node_in_scope` confines it instead through the co-resident labelled workload set (which structurally includes the boundary-broken trigger), reusing `co_resident_denies` + the existing per-mitigation `in_scope` check on each candidate — no scope bypass on the private internals, and a host with no in-scope co-resident pod is never proposed. 3. MEDIUM — control-plane detection now fails closed: recognizes the legacy `node-role.kubernetes.io/master` label and a control-plane-shaped taint (either canonical or legacy key, any effect) as additional, independent control-plane signals, union'd with the canonical label. A node with none of the three signals remains the ordinary cordon-eligible worker default (the correct reading for vanilla/kubeadm clusters, where workers carry no role marker at all). 4. MEDIUM — rewrote the chart/README language describing `enforceRung: node` and the engine's cut-severity ladder doc to state plainly that ContainNode is eligible to PROPOSE, never to auto-apply, at any rung. 5. LOW — documented (rather than implemented) that the two-worker floor counts schedulable nodes, not Ready ones; `Node.status` stays unread per the observation adapter's metadata-only discipline. Rebased onto main (#323's NodeContainmentRevert/`with_node_fact` seam) and composed with it: `Engine::process` now refreshes `node_facts` every pass from the real observation adapter (falling back to test-seeded facts when `Snapshot::nodes` is empty), and the real `NodeContainmentActuator` is wired as the revert actuator in `run_loop.rs`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VtjoJttCvBY4dzCoE4f9vP * ci(e2e): grant the engine's Node watch RBAC in the e2e ClusterRole (ADR-0040) The engine now runs an unconditional, metadata-only Node watch (observe::adapter::node_fact::observe_node_facts, ADR-0040 §3/§6). The Helm chart's ClusterRole was updated to grant nodes:[get,list,watch] always-on, but the e2e harness deploys from its own inline ClusterRole in scripts/e2e.sh, which was not. Without the grant the Node watch 403-floods the run loop every pass and starves the deterministic proof passes, so the structural web->session-key chain never proves within 300s and e2e fails. Mirror the chart's always-on metadata-only read here. No `patch` grant: e2e never arms enforceRung: node (ContainNode is propose-only at every rung, ADR-0040 §5), so the posture-derived write surface stays absent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VtjoJttCvBY4dzCoE4f9vP --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ADR-0040 §5 requires
ContainNodeto join the break-glass armed-set revert trigger(ADR-0036) and the standard ledger self-revert lifecycle (ADR-0017) — but the existing
break-glass suite (
engine/src/engine/break_glass_tests.rs) only knew the NetworkPolicycut shape (delete an engine-owned additive object). A cordon is a shared-field mutation,
not an additive object, and
NodeContainmentActuator's revert doesn't fit the genericsingle-mitigation
Actuatortrait (oneContainNodemitigation maps to many clusterobjects: the cordon plus one deny per co-resident pod). Routing a standing
ContainNodereversion through the generic network actuator would silently no-op (wrong object
kind/name) and leave the node cordoned after an operator hit break-glass — exactly the
gap this ticket verifies and closes.
What changed:
NodeContainmentRevert, a narrow trait mirroringNodeContainmentActuator::revert's signature, soEngine's self-revert loop can holdeither the live cluster actuator or a test double (mirroring how
Actuatoralready letsKubeActuatorand a recording double stand in for each other).Enginegained two dormant-by-default fields/builders(
with_node_containment_actuator,with_node_fact) and arevert_contain_nodehelper(extracted to
node_containment_revert.rsto stay under the 1,000-line file cap). Astanding
ContainNodereversion now routes through this path instead of the genericactuator, computing the co-resident deny set fresh from the current graph.attached actuator, or no observed
NodeFactfor the host — the same "no data ⇒ no pass"discipline
node_containment's own module doc already applies to the cordon rails. Whenboth are present, the actuator's own ownership self-gate (
revert_decision, alreadylanded in feat(engine): node containment actuator — cordon + co-resident default-deny (ADR-0040) #322) is what decides whether the uncordon actually happens.
node_containment.rs's module doc to reflect that the revert side is now wiredwhile the apply side (the
nodearming rung, node observation, RBAC) remains afollow-up — explicitly out of this ticket's lane.
Deliberately does not touch:
arming_ladder.rs,values.yaml,charts/, or add anyRBAC — that's the sibling ticket's lane (arming rung + node observation adapter +
apply-side rail wiring). The new tests synthesize a standing
ContainNodemitigationdirectly into the action log rather than relying on that rung existing.
Design decisions (recorded here since no ADR captures this exact wiring choice)
NodeContainmentReverttrait, not anActuatorextension. ExtendingActuator's signature would touch every existing implementor (DryRunActuator,KubeActuator,IsolationActuator, test doubles) for a shape onlyContainNodeneeds.node_containment's own doc already establishes thatNodeContainmentActuatordeliberately isn't
Actuator-shaped (one mitigation → many objects) — the new traitkeeps that boundary rather than blurring it.
NodeFactfleet as an Engine-attached map, not aSnapshotfield. Real nodeobservation (watching Kubernetes
Nodeobjects) is explicitly the sibling ticket's lane.Adding it to
Snapshot/observe::Snapshot::observewould collide with that work, so thisticket exposes a narrow seam (
Engine::with_node_fact) a future observation adapter canpopulate, and leaves it empty (safe no-op) until that lands.
Test plan
engine/src/engine/break_glass_node_tests.rs(5 tests), driving the realproof →
boundary_break→ menu-resolution → ledger pipeline (a live kernel-tamperruntime signal on an internet-exposed entry scheduled on a node with a co-resident
labelled pod), with
EnabledActions::enable(ProposedAction::ContainNode)used directly(bypassing the not-yet-built arming rung) and the "already applied" step synthesized
into the action log:
workload as a boundary-broken target (no break-glass involved)
counters, exercising the real
revert_decisionrail)NodeFactis observed for the host (the "no fabricated no-datapass" discipline)
having engaged it (adapted from
clearing_break_glass_restores_the_configured_posture_byte_identical—
ContainNodenever auto-applies, so the honest mirror is that the ledger'sproposal itself is unperturbed by disarm)
cargo fmt --check— cleancargo clippy --lib --tests -- -D warnings— cleancargo test --lib— 1184 passed, 0 failed, 2 ignored (pre-existing, unrelated)cargo build --workspace— greenno new external input/auth/secrets surface)
/simplify(self-run, single-pass — Agent tool unavailable in this context) — foundand fixed one duplication (a test fixture rebuilding
tests::exposed_snapshot's shapeby hand instead of reusing it)
Closes JEF-733