fix(dataplane): validate fallible phases before the first host mutation - #6894
Open
psaab wants to merge 26 commits into
Open
fix(dataplane): validate fallible phases before the first host mutation#6894psaab wants to merge 26 commits into
psaab wants to merge 26 commits into
Conversation
CompileConfig runs at APPLY time, after the commit has already succeeded. `pkg/configstore` has ZERO imports of `pkg/dataplane`, so `commit check` validates only the pure `pkg/config.CompileConfig` and never reaches this one -- the two share a name, which is much of why this path reads as safer than it is. Phase 2 `compileZones` then performs the first and only destructive host netlink mutation in the function: `ensureVLANSubInterface` creates and links up VLAN devices, `reconcileInterfaceAddresses` deletes and adds addresses. Every later phase is a bare `return nil, err` with no undo path. The codebase documents the reachable input class itself at userspace/flow.go:139 -- a malformed application-set reference or an app_id-space overflow (#3438 H4) makes compileApplications, Phase 4, "hard-error and abort the apply". So an ordinary commit can leave VLANs created and addresses reconciled on the live host, the control plane reporting failure, and the dataplane still on the old snapshot. validateBeforeMutate runs the eleven fallible HOST-PURE phases against a discardingDataPlane before compileZones touches anything, so those failures return with the host untouched. WHY ADDITIVE RATHER THAN A REORDER. Moving the real phases ahead of compileZones looks equivalent and is strictly worse. compileFirewallFilters and compilePortMirroring resolve VLAN SUB-INTERFACE names that exist only because Phase 2 created them, and a miss there is `slog.Warn(...); continue` -- so a reorder would leave firewall filters silently unassigned on EVERY apply, trading a loud post-mutation abort for a quiet fail-open on a security surface. The precondition Phase 2 supplies for free is the VLAN device existing, and a soft skip makes that precondition unobservable at the call site. Every existing call keeps its position; the only new behaviour is an early abort. COVERAGE, and the gap is deliberate. Eleven of thirteen fallible post-zones phases are validated. The two excluded are exactly the two above: running them pre-mutation would evaluate a different world than the real pass -- every sub-interface lookup would miss and soft-skip -- so the pre-pass would emit misleading warnings on every successful commit and still not predict the real run. A hard failure inside those two therefore still occurs post-mutation. That residual is named in the code rather than papered over; closing it needs the soft skips addressed first (#6893). The soft skips are also a live fail-open independent of this change: ensureVLANSubInterface failing is `slog.Warn; return nil`, so compileZones SUCCEEDS with the device absent and the later filter lookup then misses and continues -- commit reports success, filter is not applied. Filed as #6893, not fixed here. It is one root cause with two reachability paths, which is the argument for why this fix must be additive: a reorder would turn an occasional fail-open into a universal one. CompileConfig's inline CompileResult initialisation is factored into newValidationResult(), shared with the pre-pass. If the two initialised separately they could drift, and the pre-pass would validate a differently-shaped result than the real pass programs from -- the exact divergence this change exists to avoid. CORRECTION TO PRIOR ART. `research/4960-apply-txn` §4.4 states destructive host mutation is in "exactly two phases -- compileZones and compilePortMirroring (compiler.go:1704, netlink at :1758/:1811)". Verified wrong at 86927d2: compilePortMirroring spans 1704-1748 and contains no `netlink.` reference at all, only dp.ClearMirrorConfigs / dp.SetMirrorConfig map writes; :1758 and :1816 are `netlink.LinkByName` READS inside getPermAddr and getOriginalKernelName. Line numbers have not drifted (that plan's own :1704 is still exact), so this was wrong when written. It is ONE phase -- which is why a single pre-pass covers all host mutation with nothing left over. §4.4's middle-phase purity claim does agree with the enumeration re-derived here independently. SCOPE. This is NOT the apply-transaction redesign. Making the Go and Rust planes re-converge on a dataplane NACK is the other half of #4960, tracked on research/4960-apply-txn (twice PLAN-NEEDS-MAJOR). That work is about what happens AFTER the dataplane is asked; this is about not wrecking the host BEFORE it is asked, and needs no fence and no daemon disposition changes. Note also that "no mutation" here means HOST netlink mutation; every phase writes dataplane MAP state, which is re-derivable by the next successful compile and is what the fence work addresses. Validation: * TestNoHostMutationWhenALaterPhaseFails_4960 -- a config that passes config-compile and fails compileApplications leaves zero SetZoneConfig and zero SetVlanIfaceInfo calls. SetZoneConfig is the probe: programZoneMaps calls it per zone BEFORE iterating that zone's interfaces, and mapZoneInterface (the only caller of the netlink mutators) runs inside that iteration, so zero calls proves compileZones never started. * TestValidConfigStillReachesZoneCompile_4960 -- control. Without it the property test is satisfied by a pre-pass that rejects everything. * TestPrePassDoesNotPerturbIDAssignment_4960 -- the pre-pass compiles the config twice and discards one result, which is only free if no ID assignment reads state outliving a CompileResult. Two passes assign byte-identical ZoneIDs, ScreenIDs, AddrIDs, AppIDs, PoolIDs, NATCounterIDs and implicitSets, including the #5099 NAT-counter family. Proven sensitive: injecting drift AFTER finalizeNATCounterIDs re-derives the map REDs it (injecting before is overwritten by the re-derive and proves nothing). * Revert cell: removing the validateBeforeMutate call REDs the property test with "compileZones RAN before the failing phase was caught (2 SetZoneConfig calls)". * `go test ./...` rc=0. Advances #4960. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Gate fold r1 on #6894. Five items, none a fail-open or regression. F1 -- the coverage claim was asserted by nothing. The doc said "eleven of the thirteen fallible post-zones phases", and dropping ten of the eleven rows left `go test ./pkg/dataplane/...` fully green: the single binding fixture used an unresolvable APPLICATION name, so it pinned exactly one row of eleven. The call site was bound in both directions; the table's CONTENTS were free to shrink to nothing. The table is now a named `validationPhases()` rather than a literal inside validateBeforeMutate, so a test can assert over it: TestValidationPhaseTableMatchesDocumentedCoverage_4960 pins the length and the ordered name set against the doc comment, and TestNoHostMutationWhenNATPhaseFails_4960 adds a second binding fixture from the opposite end of the table (an unresolvable SNAT pool). Dropping rows now REDs both. The reusable shape: a guard can bind perfectly at its wiring and still leave the thing it is scoped over unbound. The mutation that finds it is at the EDGE of the claim -- not "does the pre-pass run" but "does it still cover what the sentence says". F2 -- validateFilterProtocols is now covered, taking this to TWELVE of thirteen. It is the first statement of Phase 10 (compiler_filter.go:26) and is purely a function of cfg: no result, no dp, no logging, so validating it early cannot read anything a later phase set up. It is ADDED, not moved -- the in-place call is untouched, so a config where compileFirewallFilters succeeds behaves bit-identically and simply runs a pure check twice. It was the SOLE config-shape hard error still reachable after the mutation point; reachable via the tolerant load paths (Store.Load boot, Store.SyncApply HA peer-sync), which downgrade the strict rejection to a warning and then reach CompileConfig. The residual is now narrowed to the interface-binding tail of the two excluded phases. F3 -- every covered phase logged twice (15 -> 22 INFO lines per compile) and one line printed a value that was false for the run that matters: compileFlowConfig logs lo0_filter_v4 from a pass where compileFirewallFilters has not run, so the 65535 sentinel was emitted immediately before the real pass logged the armed id. An operator asking "did my lo0 filter arm?" read NO then YES for a single apply. This is the same defect class the code's own exclusion rationale cites, applied to the phases that were kept. isValidationPass(dp) gates the 8 duplicated sites. The marker rides on the DATAPLANE, not CompileResult, because compileDefaultPolicy and compileFlowTimeouts take no result and could not otherwise be gated without a signature change. F4 -- the test fixture used ge-0-0-0.50 / ge-0-0-1.0. config.LinuxIfName only maps "/" to "-", so those are byte-identical to real interfaces on the standalone VM and on loss cluster node 0, and this is the first test in pkg/dataplane to drive compileZones to completion -- as root, once cachedInterfaceByName succeeded it would have run LinkAdd/LinkSetUp, AddrDel/AddrAdd, ethtool -K/-G and os.WriteFile to /proc/sys. Renamed to xpft4960a/b, plus a belt that skips when running as root with a live link of that name. F5 -- three citations corrected. The parenthetical in compiler.go understated the mutation set (it omits netlink.LinkDel / LinkSetDown on unmanaged interfaces via stripUnmanagedInterfaces, and the ethtool and /proc/sys writes); the "first and only" claim itself was correct. The shim was cited as "the LegacyDataPlaneAdapter idiom" when it is the INVERSION of it -- that adapter embeds a NON-nil dataplane and delegates, this one leaves the interface nil so an un-overridden method panics. The comment now says so, states plainly that nothing enforces completeness at compile time, and names TestPrePassShimCoversTheCalledSurface_4960 as what does enforce it. The ID probe's "same order as CompileConfig" was not literally true (it includes finalizeNATCounterIDs, which the pre-pass omits). Validation: * drop 10 of 12 rows -> TestValidationPhaseTableMatchesDocumentedCoverage REDs ("the pre-pass covers 2 phases but the documented coverage is 12") AND TestNoHostMutationWhenNATPhaseFails REDs ("host was mutated before the NAT phase failed: 2 SetZoneConfig") * go build ./... rc=0 * go vet ./... rc=0 * go test ./... -count=1 rc=0 * go test ./pkg/refactoraudit/ rc=0 -- 6 top-level tests, 0 SKIPs Advances #4960. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
psaab
pushed a commit
that referenced
this pull request
Aug 6, 2026
Fold round 1 on the observe-only arm-coverage proof. Seven findings from an independent hostile gate; the three runtime ones each made the measurement wrong in a way that would have read as clean. Attach mode is now GROUND TRUTH. `SurfaceCoverage.Generic` came from a per-compile record of the native->generic fallback. That record is correct exactly once: `attachUserspaceShimXDP` falls back on compile #1, and the `m.xdpLinks` entry it leaves behind SURVIVES every later compile — `syncInterfaceAttachments` detaches only ifindexes outside the allowed ingress set, and the userspace pin sweep deletes link PINS, not the map — so every subsequent commit, HA config sync and deferred-MAC reapply short-circuits on "already attached" against a FRESH, empty record. The proof therefore logged `direct/native` for a box that never left skb-mode, on precisely the population (iavf SR-IOV VFs, no native XDP at all) whose size this phase exists to measure. `xdpLinkModeGeneric` now reads `XDP_ATTACHED_SKB` from the kernel per surface, which describes the mode the link has NOW rather than what one compile did to it. `CompileResult.fallbackGenericIfindexes`, its initialiser and the loader recording are deleted; the old source supplied nothing the new one does not (it also folded in `tunnelIfindexes`, and a tunnel is attached with forceGeneric so the kernel reports it too), and nothing else read it. A surface the compiler DECLINES to arm is no longer invisible. Three soft skips in compiler_iface.go — interface not found, VLAN child create failed, administratively disabled — drop a configured attach point from `pendingXDP` behind nothing but a slog line, while the compile SUCCEEDS, so the proof reported `Uncovered=0` for surfaces it never looked at. Each now records an `UnarmedSurface` and is reported as a distinct `skipped` branch. The sharp variant is promoted further: `set interfaces <if> disable` whose `netlink.LinkSetDown` fails is a WARN and nothing else — the netdev stays UP, is still address-reconciled, is still in a zone, is still forwarded through, and carries no XDP — so that reads as `uncovered` and would-gate, which is the policy-free router #5275 is about. `LogArmCoverage` also emits unconditionally now, with a `ran` field: this file argues at DidGate that "an absence is indistinguishable from a proof that never ran" and then suppressed its own line nine lines from the end. Delegation resolves the delegate's COVERAGE, not merely its link. The doc asserted twice that the parent must itself be directly covered; the code only checked that a link was tracked for it. On an enabled->disabled commit the previous parent link is still tracked while the parent is admin-DOWN and about to be torn down — and compiler_iface.go appends the VLAN child unconditionally (the VLAN block precedes the isDisabled check) without appending the parent, so the child would report covered by a surface nothing proves armed. Classification is now two-pass: every direct surface first, then children resolved against those results. "Observe-only" is now literally true. `coverSurface` called `cachedLinkByIndex`, which memoises into two maps on the CompileResult it is proving; it uses the non-memoising `peekLinkByIndex` instead. The `m.lastCompile = result` hoist is gone — it existed only to feed `attachedInstance`, and it published a half-observed CompileResult through the exported `LastCompileResult()` before the proof wrote into it. That also closes the hand-built-CompileResult panic, since the exported entry point no longer writes to a map it did not allocate. Smaller: the apply generation is folded into the stage label, because the RETH deferred-MAC path issues a second ApplyConfig in the same apply and two records with an identical label cannot be told apart; the delegated token carries the delegate's attach mode, so a VLAN child behind an skb-mode parent is distinguishable from one behind a native parent; plan §10 gains an explicit PR0 measurement phase (PR1 there is a gate, not a diagnostic) and §5 states that this proof is the PRELIMINARY stage, so the divergence rate it emits is a lower bound on the gate's. Removing `fallbackGenericIfindexes` reverts the CompileResult literal to its master form byte-for-byte, so the gofmt realignment disappears from the diff and the collision with #6894's `newValidationResult()` shrinks to a struct-field addition. Validation. `go build ./...`, `go vet ./...`, `go test ./... -count=1` and `go test ./pkg/refactoraudit/` all exit 0; all 26 retirement-boundary canary tests pass and none skip. Eleven mutations, each snapshotted and restored byte-for-byte with sha256 verification, every one ASSERTION-RED with `go vet` exit 0 (one first produced an undefined-symbol build break and was rewritten to thread the lookup through, so the failure is a real assertion): - kernel mode source -> per-compile record: the second-compile test - declined surfaces dropped from the report: both skip tests - still-forwarding not promoted to uncovered: the sharp-variant test only — a benign decline stays `skipped` - a recordUnarmedSurface site deleted: the soft-skip canary - empty-report log suppression restored: the enumerated-nothing test - the CompileUserspaceShim call site deleted, and a constant seq argument: the call-site canary, distinctly - peek -> cached: the no-memoise test - delegation resolving the link: the required-delegate test only — legitimate parent-covered delegation stayed green - identical stage label: the distinct-label test - delegated token without the mode: both summary tests Over-reach guards that stayed GREEN throughout: a driver-mode attach is not reported generic, an untracked ifindex is not manufactured into coverage, generic still counts as armed, and a VLAN child behind a required, directly-covered parent is still delegated. Advances #5275.
The round-1 fold added a twelfth row to the validation phase table and left the header saying "Eleven of the thirteen fallible post-zones phases". That is the third time this campaign a fold has grown a list and left its count behind, and the second on this exact class of comment. Correcting the numeral would have set up the fourth. So the count is gone: the prose now defers to the table as the authority and points at `TestValidationPhaseTableMatchesDocumentedCoverage_4960`, which pins both its length and its name order, so a row added or removed without touching the comment reds. A number in a comment is a coverage claim and it rots. A pointer to an asserted table does not. The same sentence was wrong a second way that a numeral swap would not have caught: the exclusion is not "two phases". It is `compilePortMirroring` entirely, plus all of `compileFirewallFilters` EXCEPT its cfg-pure prefix `validateFilterProtocols`, which round 1 hoisted into the table as its own row. Also separated the two kinds of binding, which the gate measured and this file did not distinguish. The table test binds the SET -- no row can vanish silently. Behavioural fixtures bind that a given row actually rejects before the mutation, and there are exactly two of those (`applications` via an unresolvable application name, `nat` via an unresolvable pool). Every other row is set-bound only. That is a deliberate trade -- a per-row failure fixture for all twelve would triple the file for little added signal once deletion is impossible -- and it is now written down rather than left to be inferred from which tests exist. Advances #4960. Comment-only: the production diff filtered to non-comment lines is empty. `go build ./...` rc=0; `go test ./pkg/dataplane/ -count=1` rc=0; `gofmt -l` clean.
… count The Codex gate returned MERGE-NEEDS-MINOR with no runtime findings and four refuted claims. All four are comment or log text; the Go diff has zero non-comment changed lines. The load-bearing one: `isValidationPass(dp)` was described as having suppressed the pre-pass log duplication. It suppresses the sites it is wired into. A large inventory inside the covered phases still logs unconditionally and still emits twice -- the application compiler, the NAT/DNAT/static-NAT/NPTv6/NAT64 families, and the flow-timeouts record. Measured on a composite config: 25 INFO/WARN records, of which eleven distinct covered-phase records appeared twice. The doc comment now states the scope limit, names the ungated families, and points at #6903 so a reader cannot take it as proof that a duplicate they are looking at is impossible. The "measured 15 -> 22 lines" figure is deleted rather than re-measured. The fixture that produced it was never recorded, so it is unreproducible; the likely candidate emits 12 today. A count assertion has to ship with the fixture that produces it, which is what #6903 says to do if a count is wanted at all. "validateFilterProtocols is the FIRST statement of Phase 10" is literally false -- a local map init precedes it. Corrected to "first FALLIBLE statement", which is the property the hoist actually depends on. And `dp.BumpFIBGeneration()` sat outside both the phase table and the exclusion prose while being able to return an error; it is now named there as out-of-scope by construction, since the caller discards it under the fire-and-forget contract and it cannot produce a returned CompileConfig error. Documented, not changed: hoisting the filter row alters operator-visible diagnostic precedence. A config carrying both an unknown screen-profile reference and `from protocol bogus-proto` now reports the filter error where it previously reported `compile zones: screen profile ... not found`. Both are hard errors aborting the same apply, so nothing reaches the dataplane either way and only the message changes. The comment warns against "fixing" this by moving the row later -- the ordering is what keeps the mutation point clean. Validation: `go build ./...` rc 0, `go test ./pkg/dataplane/ -count=1` ok, `gofmt -l` clean, and `git diff -U0 -- '*.go'` confirmed comment-only. Advances #4960. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
# Conflicts: # _Log.md
Owner
Author
|
Hostile gate at F1 (MAJOR, test-only, does not touch the shipped binary). Measured with identical instrumentation on both revisions: The codebase already names this hazard verbatim:
F2 (MINOR, text). The scope line is "not wrecking the host BEFORE the dataplane is asked", but three fallible steps sit after the mutation and before publication and none is named: F3 (MINOR). "No row can vanish silently" over-claims — the table test asserts only Confirmed sound, and worth recording: the validator and the doer are ONE code path run twice, not two implementations — the drift failure mode that usually kills this refactor shape is structurally excluded. Sequencing with #6864 resolved: no conflict, either order, no rebase. |
TestValidConfigStillReachesZoneCompile_4960 is the negative control for the
validate-before-mutate ordering assertions, so by design it lets CompileConfig
reach compileZones. compileZones then runs to its tail, and that tail is
stripUnmanagedInterfaces: it enumerates the REAL host with net.Interfaces()
and, for every link that is not `lo`, not in the config and not daemon-owned,
issues netlink.AddrDel followed by netlink.LinkSetDown -- plus netlink.LinkDel
outright for a bond. Confirmed firsthand with an instrumented panic at
compiler_iface.go:1255: it fires on `eno2` via compiler_validate_4960_test.go
-> CompileConfig -> compileZones, with 50+ further links behind it.
Unprivileged those calls fail with EPERM, so the test looked harmless; run as
root it black-holes the host's networking. The shipped binary is unaffected.
The fix is the tripwire idiom the codebase already uses for exactly this
hazard (errCompileStopBeforeReconcile, compiler_rxvlan_failclosed_5268_test).
recordingDP.SetZoneConfig now counts the call and then returns
errStopBeforeHostReconcile. programZoneMaps calls SetZoneConfig once per zone
BEFORE iterating that zone's interfaces, so failing the first call halts the
compile after the probe has recorded "compileZones was entered" and before
mapZoneInterface -- therefore before ensureVLANSubInterface,
reconcileInterfaceAddresses, the ethtool/procfs writes and the unmanaged
strip. The control stays live rather than being skipped: errors.Is on that
sentinel is reachable only once the pre-pass has PASSED the config, so a
pre-pass that rejected good configs still reds here. Two counters
(SetVlanIfaceInfo, SetZone/AddTxPort -- all reached only from inside
mapZoneInterface) turn the safety property into a runtime assertion instead of
a structural argument.
skipIfCouldMutateAHost is removed rather than widened. It skipped as root only
if one of the two FIXTURE interface names resolved to a live link, but once
compileZones runs at all the strip takes down every unmanaged NIC on the box,
so those two names were never the exposure. The tripwire covers it at any
euid, for any host interface inventory, and is asserted.
Two smaller corrections in the same file. The scope paragraph drew its line at
"not wrecking the host BEFORE the dataplane is asked" while three fallible
steps sit after the mutation and before the snapshot is published; all three
are now named -- preflightCheckIfindexCaps (structurally unhoistable, it
consumes result.pendingXDP which only compileZones populates),
attachUserspaceShimXDP (the reachable one: an XDP attach failure on a driver
rejecting a generic attach leaves VLANs created and addresses reconciled while
the apply reports failure), and
buildSnapshotWithSchedulerStateAndNATCounters. And the coverage claim that "no
row can vanish silently" was stronger than its guard:
TestValidationPhaseTableMatchesDocumentedCoverage_4960 asserts only
got[i].name, so replacing ten row BODIES with `func() error { return nil }`
left the package green. TestEachValidationPhaseRowRunsItsOwnCompiler_4960
drives every row against an input its own compiler rejects and requires the
error back prefixed `validate <that row's name>: `. Five rows -- nptv6, screen
profiles, default policy, flow timeouts, flow config -- have no config-shaped
hard error at all (every bad input is slog.Warn/continue; the only return err
is the dataplane call), so validateBeforeMutate gains a
validateBeforeMutateWith(dp, cfg) seam letting a test fail exactly one
dataplane method. validationPhases was already dp-parameterised for the same
reason.
Validation, all at uid 1000: go build ./... exit 0; go test
./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1 -race exit 0; go test
./pkg/refactoraudit/ -count=1 exit 0. With the panic probe armed in
stripUnmanagedInterfaces the whole pkg/dataplane package passes; neutralising
the tripwire to `return nil` with the probe still armed re-fires it on `eno2`,
which is the positive control. Moving validateBeforeMutate after compileZones
reds both no-mutation tests as assertions ("compileZones RAN before the
failing phase was caught (1 SetZoneConfig calls)") and the revert path is now
itself host-safe. Gutting the ten unbound row bodies reds exactly those ten
subtests while applications, nat and the name-order test stay green; swapping
the static-nat and nat64 bodies with names intact reds both on the prefix.
Advances #4960.
Owner
Author
|
Fold r2 at F1 (the MAJOR). Proven two-sided with an instrumented panic probe at
The NIT is closed properly rather than dropped: F3 found something worth recording. Five rows — Mutation (b) is the one that matters: swapping the F2 names all three post-mutation steps, with Handed to the gate as its top risks: whether the |
added 2 commits
August 6, 2026 03:31
The validate-before-mutate pre-pass runs against discardingDataPlane, which embeds a NIL DataPlane so any un-overridden method nil-panics rather than silently succeeding against a real dataplane. Nothing enforces that override set at compile time; TestPrePassShimCoversTheCalledSurface_4960 is the only thing that does, by driving the pre-pass over a rich config so a new call surfaces as a test panic. Its fixture reached every PHASE but not every phase's WRITE SURFACE. Instrumenting all 40 overrides with a name recorder measured 28/40, and nine of the misses -- SetDNATEntry, SetDNATEntryV6, SetNAT64Config, SetNATPoolIPV6, SetNPTv6Rule, SetSNATEgressIP, SetSNATRuleV6, SetStaticNATEntryV4, SetStaticNATEntryV6 -- were reached by no test in the package at all. Deleting any one of them left the whole pkg/dataplane suite green while an ordinary `security nat destination` stanza would nil-panic the daemon on apply, with no recover() anywhere on that path. idProbeConfig now carries destination NAT (v4 and v6), static NAT (v4 and v6), an NPTv6 rule, a NAT64 rule-set, an IPv6 source pool with its v6 SNAT rule, an interface-SNAT rule and a security policy, taking the reached set to 39. The 40th, IsLoaded, is called by CompileConfig itself above the pre-pass and is bound by the CompileConfig-driving tests instead. The fixture's zone interface names move off the vSRX scheme: the interface list is load-bearing here for the first time, and ge-0-0-1 is a real link on the standalone test VM and on loss cluster node 0. SetSNATEgressIP is the one covered write that is not a pure function of cfg -- compileNAT resolves the egress zone's member through cachedInterfaceByName and soft-skips on a miss. validateBeforeMutateWithResult makes the CompileResult injectable so the test can seed a SYNTHETIC ifCache entry. Naming a real link instead would work on any host and is exactly what must not be done: a resolving fixture name is one CompileConfig call away from reconciling a live interface's addresses. Two smaller corrections. The r2 claim that vlanIfaceInfoCall == 0 and ifaceSetupCalls == 0 "turn the safety property into a runtime assertion instead of a structural argument" is wrong, and this is the second time that assertion has been mis-described. Both counters read 0 with AND without the tripwire, because mapZoneInterface soft-skips this fixture's interface names before reaching either. zoneConfigCalls is what distinguishes (1 vs 2, measured). The comments now say so; the counters stay as defence in depth and must not be made live with real interface names. And the note that any dp passed to the pre-pass MUST embed discardingDataPlane is now an isValidationPass guard ahead of the phase loop, not a request. pkg/dataplane/README.md listed the compile phases without the pre-pass, which can reject a config on its own and deliberately changes which error an operator sees when a config carries both a Phase-2 fault and a bad firewall filter protocol. Validated at uid 1000: `go build ./...`, `go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1 -race` and `go test ./pkg/refactoraudit/ -count=1` all exit 0. Mutation matrix, each mutation grepped back out and the file restored byte-identical after every cell: deleting any one of the nine overrides at the previous head leaves the whole package GREEN, 9 for 9; at this head all nine RED with a nil-pointer panic at their own compiler_nat.go call site. Deleting the F4 guard reds the new test on an assertion, and moving it after the phase loop reds it on a different assertion -- it wrote 6 address-book entries before rejecting. Advances #4960.
# Conflicts: # _Log.md
added 11 commits
August 6, 2026 05:05
r3 widened idProbeConfig so the pre-pass shim's whole write surface is exercised. The ID DRIVER did not widen with it, and three of the dimensions TestPrePassDoesNotPerturbIDAssignment_4960 compares were left structurally unable to differ. compileIDsOnce ran address book -> applications -> NAT -> finalize; the real pre-pass runs policies, nat, static nat and nat64. An empty map DeepEquals an empty map, so each omission passed silently. Measured before this change: implicitSets was empty, PoolIDs held only pool-a/pool-b/pool-v6 with pool-nat64 absent, and NATCounterIDs held five keys with no static/ entry at all. The test presented itself as the regression binding for double-compilation ID stability while unable to fire on three of its own columns. The driver now runs CompileConfig Phases 3, 4, 5, 6, 6.5, the finalization and 6.6 in production order. compileNPTv6 (6.7) stays out deliberately: it takes no *CompileResult and assigns no IDs. implicitSets needed a second, fixture-side half. resolveAddrList filters "any" out entirely and returns a lone surviving name through the direct-ID branch, so the old policy's ["any"] / ["servers"] lists built no implicit set even once compilePolicies ran. The fixture's p-multi policy now carries genuinely multi-valued address lists; p-single is kept alongside as the negative control for the two shapes that build nothing. NextPoolID joins the compared set — a scalar, because an off-by-one in the NAT64 auto-assign branch would leave every map identical while the next pool allocated collides with an existing one. The non-vacuity assertions now name the specific entries expected (one counter key per NAT type including both static ones, both implicit-set cache keys, pool-nat64, and NextPoolID past its id) rather than only checking a map is non-empty, which a partial regression would still satisfy. The finalize POSITION is asserted as a precondition rather than through the output, because the output cannot see it: finalization only changes an id under a base-hash collision, and this fixture has none, so moving the call above compileStaticNAT was measured to leave the whole test green. What the position buys is that the sorted re-derivation sees every NAT type's keys, so the driver asserts exactly that, with the expectation derived from the config and compared against the compiler's map. Validated at uid 1000: `go build ./...`, `go vet ./pkg/dataplane/`, `go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1 -race` and `go test ./pkg/refactoraudit/ -count=1` all exit 0. Before -> after: implicitSets 0 -> 2, PoolIDs 3 -> 4 with NextPoolID 4, NATCounterIDs 5 -> 7. Perturbation proof, injecting a process-global drift into each of the three ID-assignment sites in turn — the exact state class a discarded double-compile would expose, each grepped back in and every file restored byte-identical: at the previous head all three leave the whole package GREEN; at this head all three RED on their own dimension. Non-vacuity proof, one knockout per cell, all RED with their own assertion message, including reverting p-multi to the single/"any" shapes. The 40-cell delete-one-override matrix was re-run against the whole package: 40 RED / 0 GREEN, so widening the ID driver did not weaken the shim-coverage test. Test-only round: no production file is touched and no compile behaviour changes, so the README pre-pass entry added in r3 still describes the shipped behaviour exactly. Advances #4960.
# Conflicts: # _Log.md
The #4960 ID-stability probe compares ID columns across two compile passes. r4 fixed the columns that were vacuous because the driver never ran the phase populating them; ScreenIDs failed one level below that. The driver did run Phase 1.5 — but by RE-IMPLEMENTING it, so the column compared compileIDsOnce's own loop against itself and could not observe a production drift at all. Three verbatim copies of the screen-ID prelude existed: CompileConfig, validateBeforeMutateWithResult, and the probe's driver. They were token-identical modulo identifier names — same uint16(1) seed, same cfg.Security.Screen source, same sort.Strings, same 1-based increment, each running against a newValidationResult()-allocated map behind its own nil-cfg guard — so collapsing them is a cleanup, not a behaviour change. Extract assignScreenIDs next to assignZoneIDs and call it from all three sites; its doc comment records why it must stay a single site. compiler_validate_4960.go's "sort" import had exactly one use, the prelude, and is dropped with it. Add two per-column non-emptiness floors so neither column can decay back into an empty-vs-empty DeepEqual. ScreenIDs names alpha, mid and zeta. AddrIDs had been protected only BY ACCIDENT: the implicitSets assertion demands "db,web" and "dns,servers", which cannot exist without their AddrIDs entries — but that covers only one of the map's two writers. compileNAT's resolveSNATMatchAddr synthesizes a _snat_match_<cidr> entry per SNAT match CIDR and nothing pinned it, so the floor names entries from both writers. The full key set was measured before the floor was written, not assumed. Validated with a mutation grid over the whole pkg/dataplane package, every mutation confined to a throwaway git-archive extract (the worktree tree hash was verified identical before and after), each cell run at BOTH the base commit and this head so the claims are measured flips rather than one-sided reds: - Seeding the screen-ID assignment from a package-level counter so pass 1 and pass 2 diverge was GREEN at base against EITHER inline production copy — both sites could drift with the suite passing — and is RED here: "ScreenIDs differs between pass 1 and pass 2". - Dropping the fixture's three screen profiles was GREEN at base ({} DeepEquals {}) and is RED here on the new ScreenIDs floor. The DeepEqual stays quiet in that cell, which is why the floor exists. - Drifting assignZoneIDs, a dimension this change does not touch, stays RED — the harness can red on its own. - Deleting resolveSNATMatchAddr's AddrIDs store was GREEN at base, the whole package passing with that writer contributing nothing, and is RED here on the new AddrIDs floor. go build, go vet ./pkg/dataplane, go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1 -race, go test ./pkg/refactoraudit/, and -run 4960 -count=10 -race all pass. No live module doc describes the prelude or its call sites, and compile behaviour is unchanged, so the rationale lives in the two doc comments instead. Advances #4960.
Comment-only. No production or test behaviour changes. TestPrePassDoesNotPerturbIDAssignment_4960 compares two passes of the same phases over the same config. What that detects is perturbation ACROSS passes — state outliving a CompileResult, such as a process-global, a counter on the DataPlane, or an interned table — which is precisely the #4960 question, since the validate pre-pass compiles twice and throws the first result away. It does not detect incorrect assignment WITHIN a pass: anything applied identically to both passes, a wrong seed or sort key or id formula, produces two identical maps and stays green by construction. The wording added when the three screen-ID preludes were collapsed said that column "could not observe a production drift". That claims the stronger property. It could not observe a CROSS-PASS drift, which is what the extraction actually restored, and the mutation that demonstrated it was seeding the assignment from a package-level counter rather than any drift at all. Reword assignScreenIDs' doc and compileIDsOnce' doc to say cross-pass and to name the measured mutation precisely, then state the general form once in compileIDsOnce — scoped to EVERY column, not to ScreenIDs. A reader who takes a green here as "the ids are right" is misreading all of them, so the limit belongs where it covers all of them. The same paragraph separates this test's three mechanisms, which had been easy to conflate: the DeepEqual binds cross-pass stability, the phase list binds which dimensions exist at all, and the non-vacuity floors only keep a column from being an empty-vs-empty comparison. Proven comment-only by stripping blank lines and whole-line comments from both revisions of each file and comparing what remains: compiler.go 1377 non-comment lines identical, compiler_idprobe_4960_test.go 330 identical. The floors, the driver's phase list and its order, assignScreenIDs' body, and all three call sites are untouched, so the mutation grid from the previous round still holds without re-running — no line it exercises changed. go build, go vet ./pkg/dataplane, go test ./pkg/dataplane/... -count=1, and gofmt -l all pass. Advances #4960.
…tation The defect #4960 exists to close was still reachable through the phase this PR leaves as the mutation point. programZoneMaps ranges cfg.Security.Zones — a Go MAP — and per zone calls buildZoneConfig, which resolves that zone's `screen-profile` reference, then SetZoneConfig, then iterates the zone's interfaces into mapZoneInterface where the netlink and /proc/sys writes live. An unknown reference on a zone the runtime visits second or later therefore aborts the compile AFTER every earlier zone has been programmed. With a live interface on one of those zones the abort lands after real host mutation, which is the half-reconfigured host this change claims to prevent, produced by the mechanism it claims to have closed. Because the iteration order is randomised, the blast radius differs run to run on the same config. The source said so and denied it in the same breath: compiler_validate_4960.go called filter protocols "the ONE config-shape hard error still reachable after the mutation point", then acknowledged the unknown screen reference and asserted "nothing reaches the dataplane either way". Both could not be true. That self-contradiction is how this survived four review rounds, and the comment now says which half was false rather than carrying a bare count. The fix is a pure upfront sweep, validateZoneScreenReferences, called at the TOP of compileZones so no zone can mutate before every reference has been checked whatever the caller did first, and registered as the "zone screen references" pre-pass row so the pre-pass reports it with the same precedence as its siblings. It is deliberately NOT just another phase- table row: compileScreenProfiles is already in that table and did not catch this, because what is unresolvable is the zone's reference TO a profile rather than the profile set itself. The generalising question, answered rather than assumed. There are TWO config-shaped aborts in that loop and hoisting only the first would still abort mid-loop: buildZoneConfig resolves against result.ScreenIDs (the compiled id map) and mapZoneInterface resolves the same reference against cfg.Security.Screen (the config map) on the VLAN sub-interface path, after several host mutations. The sweep checks both sources. Every other error return in the loop — set vlan_iface_info, set zone, add tx port, the address-reconcile tail — is a netlink/dataplane I/O failure, not a config-shape one, so it cannot be pre-validated against a discarding shim and is a different class. The revert probe is multi-trial on purpose. Map order is randomised per range, so with the fix reverted the offending zone is visited first about 1/N of the time and the compile aborts before mutating — indistinguish- able from a pass. A single-run probe would report green on broken code a fraction of the time. The fixture uses eight valid interface-carrying zones plus one offender over 24 trials, so a reverted build slips through with probability 8^-24. Measured with BOTH halves of the fix removed: `go vet ./pkg/dataplane` rc=0 — an assertion failure, not a build break — and the test rc=1, failing at "compileZones programmed 1 zone(s) before rejecting the stale screen reference". The three over-reach guards (a valid reference still reaches compileZones; a zone with no screen profile and a nil zone slot are both tolerated, matching what the loop tolerates) stayed green under that revert. Both production files were restored and cmp-verified against a pristine copy. Three text corrections, all to claims that overstated what was measured. The ID-stability comment offered "a counter on the DataPlane" as state the driver could observe; compileIDsOnce constructs a fresh stateless idProbeDP{} per invocation, so per-instance dataplane state is not observable there at all — replaced with "a package-level sequence". The r5 log entry called the three screen-ID preludes "verbatim ... token-identical" when the driver's copy used the locals `names`/`n` against production's `screenNames`/`name`; they were alpha-equivalent, so the conclusion stands and the wording did not. The AddrIDs floor comment named the synthetic writer as the "interface-SNAT branch" when the pinned keys come from named-pool rules. Validation: go build ./... rc=0; go vet ./pkg/dataplane rc=0; go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1 -race rc=0; go test ./pkg/refactoraudit/ -count=1 rc=0; gofmt clean on every touched file. Advances #4960.
The fold that added the `zone screen references` row inverted the precedence example and left it stated in two shipping artifacts. The pre-pass returns on the FIRST failing row, and the screen sweep now sits earlier in validationPhases than `firewall filter protocols` -- so a config carrying both an unknown screen-profile reference and a bad filter protocol reports the SCREEN error. Both artifacts asserted the opposite: compiler_validate_4960.go's PRECEDENCE paragraph said the filter error "now" wins where the screen error used to. Both halves became false the moment the screen sweep joined the table ahead of it -- and that paragraph is the one the fold edited, to correct a different false claim inside it. pkg/dataplane/README.md said the filter error is reported "ahead of a zones-phase fault such as an unknown screen-profile reference". Doubly stale: the screen reference is no longer a zones-phase fault at all. This is the defect class the fold exists to close, so it is worth the words even though runtime is correct and only the description was wrong. Both are now stated as an ORDER rather than as row numbers. Row indices shift whenever a row is inserted, and two readers of this table already disagreed on whether to count from 0 or 1 -- a number would have been the next thing to go stale. The comment says to re-derive the example by reading the table, and the README says to read the order off validationPhases rather than trusting the sentence. Comment/doc-only: the Go diff contains no non-comment line. No phase, row, predicate or test changed. Advances #4960. Validation: gofmt clean, go build ./... (0), go test ./pkg/dataplane/... -count=1 (0, four packages ok).
…elity Round 5 review of the #4960 validate-before-mutate pre-pass. Two of the six findings invert on measurement; this records why rather than quietly not acting on them. The blocking finding does not reproduce. It held that discardingDataPlane.SetAddressBookEntry returns nil while "the real backend" calls net.ParseCIDR, so an empty address-book prefix could clear the pre-pass, let compileZones mutate the host, and then fail with the address maps already cleared. That ParseCIDR belongs to (*dataplane.Manager) — the retired eBPF backend, which NewDataPlane and NewRuntimeDataPlane both refuse with ErrEBPFBackendRetired. Production compiles against userspaceShimCompileDataplane, via userspace.Manager.ApplyConfig -> .Compile -> CompileUserspaceShim. Measured across the whole surface rather than spot-checked: all 55 shim methods are a bare `return nil`, and all 38 shared with the fake are byte-identical. compileAddressBook therefore cannot fail from an empty value on the only supported runtime path. Teaching the fake to parse would make the pre-pass STRICTER than production and reject at commit what the runtime accepts — an empty `value` is a deliberate warning (#2229), and this gate has already swung through over-rejection twice. What the finding is right about is that the equivalence is a property of the shim, not a law. TestPrePassFakeIsNoMorePermissiveThanProduction asserts every userspaceShimCompileDataplane method is still a no-op, with a floor on the method count so a rename cannot make the scan vacuous. Adding a validating body to the shim reds it, with go vet exiting 0 so the failure is an assertion rather than a build break. The ordering defect is real and was mine. validationPhases claimed to list phases "in the same order CompileConfig runs them" while `zone screen references` sat eighth; production reaches it FIRST, since validateZoneScreenReferences is the first statement in compileZones and compileZones is the first phase. A config with both a stale screen reference and a broken address-book set was reported by the pre-pass as "address book" while production aborts on the screen reference, so the operator was pointed at the wrong stanza. The row moves to first, which makes the documented claim true instead of weakening it. The existing coverage test could not have caught that: it compares the table against another hand-written list, so both were wrong together. TestPrePassReportsTheSamePhaseProductionWould drives a config tripping both phases and asserts the pre-pass names the one production would. Moving the row back while keeping the index list consistent reds the new test and leaves the coverage test green — the discrimination that was missing. The probabilistic revert guard's arithmetic is replaced with measured numbers. The comment called a single run "a coin flip" (0.5) and derived 8^-24 from uniformity over the eight valid zones. The map holds nine keys and Go's range is not uniform over which lands first: reverting both halves and running 200,000 single-trial compiles gives 12632/200000 = 0.0632 per trial, so 24 trials leave ~1e-29. Corrected in the comment and in the earlier _Log.md entry that repeated it. The same test had a vacuity hole — nothing tied the trial count to the fixture, so trimming the valid-zone loop to zero leaves only the offender, which is then visited first every time and makes the probe pass 24/24 on broken code. A fixture floor now asserts eight valid zones plus the offender, all eight carrying interfaces, and shrinking the loop reds it. Also corrected: the row count ("five of the twelve", "ten of the twelve" -> thirteen) and four rotting line references — compiler.go:182 -> :268 for the IsLoaded check across three files, and compiler.go:245-254 -> :330 for the NAT finalize. Two findings are NOT addressed and are owed: widening the ID driver to the PolicySetID / RuleID / PolicyNames / scheduler slots, and the success logs emitted for no-op validation-pass operations. Both are coverage and log noise rather than runtime defects; neither is started. go build ./..., go vet ./pkg/dataplane, and go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1 all pass. Advances #4960.
Follow-up to the round-5 fold. Acting on the ask to enumerate the shim's method set dynamically rather than from a hardcoded list turned up a divergence the previous round's claim had missed, so this corrects that claim as well as hardening the guard. Round 5 argued the fake and userspaceShimCompileDataplane are equivalent because all 38 methods they share are byte-identical. That is true and incomplete. The shim EMBEDS *Manager, so any DataPlane method it does not override is promoted to the retired-eBPF implementation. DataPlane has 130 methods and the shim declares 55, so roughly 75 promote. Comparing the pre-pass call surface — the 41 methods discardingDataPlane declares — against the shim's 55 leaves three the shim does not override, and one is live on the compile path. GetPersistentNAT is called twice inside compileNAT. The fake returns a typed-nil table; the shim promotes to (*Manager).GetPersistentNAT and returns the live one. Both sites nil-guard, so the pre-pass SKIPS ClearPoolConfigs and SetPoolConfig while production runs them. Neither returns an error, so nothing escapes the pre-pass into the post-mutation window and the earlier refutation still holds — but "the pre-pass sees what the real compile sees" was not true, and nothing pinned it. IsLoaded diverges too, harmlessly: CompileConfig checks it above every mutation. Both are now allowlisted with that argument rather than left invisible. The guard is rebuilt around the invariant that actually matters. It is hybrid by necessity: reflection enumerates the DataPlane method set so a method added later is automatically in scope, and AST classifies bodies and declared-versus-promoted, because reflection cannot tell those apart — Go synthesizes a wrapper on the outer type for an embedded pointer, so comparing func pointers reports zero promoted out of 130. Four checks: every shim-declared method is a no-op; every pre-pass call-surface method is overridden by the shim or allowlisted with a reason; the allowlist is minimal so a stale exemption cannot wave through a later divergence; and the shim satisfies every DataPlane method, with a floor so the reflection half cannot go vacuous. Proven to fire, every cell in a throwaway extract with go vet exiting 0 so each red is an assertion rather than a build break. Giving the shim a validating SetAddressBookEntry reds on the non-no-op body. Deleting the GetPersistentNAT allowlist entry reds on the promotion. Deleting the shim's SetAddressBookEntry override so it promotes to the validating eBPF Manager also reds — that cell is the originally reported chain made real, and the guard catches it. As a negative control, deleting an override for a method the pre-pass never calls stays green, which is correct: it cannot cause a divergence. The redundancy argument for the two zone-to-screen resolution sites rested on assignScreenIDs populating ScreenIDs by ranging cfg.Security.Screen, which was implicit. TestScreenIDsKeySetMirrorsConfig asserts the key sets agree in both directions across empty, single and multi-profile fixtures; adding one ScreenIDs key no config declares reds it. Without that, the comment claiming independent binding is structurally impossible would quietly become false the day ScreenIDs gains a second source. go build ./..., go vet ./pkg/dataplane, go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1, and gofmt -l all pass. Advances #4960.
Master advanced while the #4960 fold was in flight. Only `_Log.md` conflicted, across three hunks; it was union-resolved keeping both sides verbatim and dropping only the conflict markers. Verified structurally rather than by eye: ancestor 1485 entries, this branch added 6, master added 16, merged file carries 1507 -- the exact union -- and both sides' newest entries were then located by content. `pkg/dataplane/README.md` auto-merged. `go build ./...` clean at the merge.
Round-6 fold of the three minors from the MERGE-NEEDS-MINOR re-gate. No blocking findings and no production behaviour change. The first is a gap in the pin added last round, and the review is right about it. TestScreenIDsKeySetMirrorsConfig calls assignScreenIDs and compares its output against cfg.Security.Screen, so it fires when the divergence is introduced INSIDE that function — which is where the previous round's mutation put it. A realistic second writer, a synced peer or a cached snapshot, is a SEPARATE SITE writing after assignScreenIDs has returned. Reproduced: inserting a ScreenIDs key at both production call sites left the whole package green, so the comment claiming "this test is what fails at that moment" was false for the shape it named. TestAssignScreenIDsIsTheSoleWriter scans every non-test file in the package for index assignments and deletes on .ScreenIDs and asserts a single enclosing function, assignScreenIDs. That is the structural property the zone-to-screen redundancy argument actually rests on, and it is satisfiable today because compiler.go carries the only write. Floors on the file count and the writer count keep a rename from making it vacuous. It reds on the previously-green mutation, naming all three writers with file and line. The second is the hoist's own claim. compileZones opens with validateZoneScreenReferences and says that makes it structurally impossible for a zone to mutate before every reference is checked whatever the caller did first. Deleting the hoist while keeping the pre-pass row left the package green, because compileZones has one production caller and the pre-pass rejects ahead of it — so every test reaches the phase through a path that already refused. TestCompileZonesRejectsStaleScreenRefWhateverTheCallerDid calls compileZones directly, the idiom the rx-vlan fail-closed test already uses, and reds on that deletion with one zone programmed. That count of one also corrects the PR body, which claimed two. The third adds PolicyNames, PolicySets and AppNames to the ID driver, each with a floor; compilePolicies and compileApplications were already in the driver, so these were omitted rather than unreachable. PolicyScheduleRuleSlots is deliberately left out: the fixture declares no scheduler, so the slice is empty and the column would compare empty against empty — the vacuity an earlier round removed from three other columns. The test records that, and what to add first. Also corrected: a README sentence that said an unknown screen reference is "no longer a zones-phase fault at all", which described only the pre-pass half and read as though the zones-phase check had been removed; it has not, this work hoisted it there. And twelve rotted file:line citations, now written as symbol references rather than re-pinned numbers, because they keep rotting inside this PR — one of them had rotted since last round because this branch's own hoist insertion shifted it. go build ./..., go vet ./pkg/dataplane, go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1, and gofmt -l all pass. Advances #4960.
…list Round-7 fold of the MERGE-NEEDS-MAJOR verdict. Two of the findings are guards that did not bind what their claims asserted; the rest are text against code. No production behaviour changes. The ordering claim was true at this head and unbound, which supersedes an earlier reading that it was settled. validationPhases says it lists phases in the same order CompileConfig runs them, and neither guard observed CompileConfig. The structural test compares the table against a hand-written want list — both authored together, so both can be wrong together, which is exactly what happened when the zone-screen row sat eighth. The behavioural test calls only validateBeforeMutate, so moving the table reds it while swapping the production calls does not. A reviewer mutated the thing the claim is about and it stayed green. TestPrePassRowOrderMatchesCompileConfig derives the order from CompileConfig's source by AST and compares it to the table, mapping each row to the production symbol whose position defines its precedence — zone screen references to compileZones, because the sweep is hoisted to its top, and firewall filter protocols to compileFirewallFilters, because that row runs a pure check production performs inside the phase. Swapping compileNAT and compileStaticNAT in CompileConfig reds it and nothing else, which is the point. Allowlist minimality was much weaker than it looked. The stale check required both types to declare the name, so a nonexistent method, an empty rationale, or a declaration moved out of either scanned file all passed silently — an arbitrary bogus exemption left the test green. A name can therefore be pre-added and a later stub plus compile call silently excused while production promotes the fallible Manager implementation. The check is now a positive test of legitimacy: on the pre-pass call surface, not overridden by the shim, and carrying a rationale. xpfValidationPass is scoped out of the allowlist entirely; it is the unexported marker, absent from the DataPlane interface, and never was a fake-versus-production divergence. The persistent-NAT exemption vouched for two methods while compileNAT calls three — ClearPoolConfigs, SetPoolConfig, and RegisterNATIP twice — and said "neither returns an error", a two-item word for a three-item set. The conclusion held, but a reviewer re-checking the premise would have verified two of three. Naming three decays the same way one call later, so TestPrePassPersistentNATCallsCannotFail derives the set from compileNAT's pnat calls and asserts each is declared with an empty result list. That also discharges an obligation which previously had no consumer at all: both call sites are bare statements, so a new result would compile silently at both and nothing would observe it. Giving RegisterNATIP an error return reds it — the method the prose never named. The exemption now records why the typed nil is load-bearing in the safe direction, because the obvious improvement is the harmful one. Under master compileNAT ran once against the real dp; with the pre-pass it is skipped and then run once by the real pass, so the live table sees an identical sequence. Had the fake modelled a real table, the pre-pass would have cleared and repopulated the LIVE table from a compile whose result is discarded. Two text corrections. validateBeforeMutateWith claimed embedding discardingDataPlane "is enforced"; what is checked is the unexported marker, and an in-package type could return true from it while delegating writes to a live dataplane. Production is safe because the sole caller constructs the fake directly — a property of the call site, not an invariant, so the claim now says what is checked. And the ID-probe header said the phases run twice against the same dp while compileIDsOnce builds a fresh one per invocation, which would reset a per-instance counter; it now states what the probe actually detects. go build ./..., go vet ./pkg/dataplane, go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1, and gofmt -l all pass. Advances #4960.
Owner
Author
Exhaustive classification of
|
| method | real impl | rejection |
|---|---|---|
SetAddressBookEntry |
(*Manager), maps_policy.go |
net.ParseCIDR(cidr) |
This is the only override whose real counterpart can refuse an input the fake accepts. It is the retired eBPF Manager — NewDataPlane returns ErrEBPFBackendRetired (pkg/dataplane/dataplane.go), so it is not constructible as a runtime dataplane.
Class B — returns an error, but only on dataplane STATE (26)
ClearAddressBookV4 · ClearAddressBookV6 · ClearAddressMembership · ClearSNATEgressIPs · SetAddressMembership · SetApplication · SetAppRange · SetDefaultPolicy · SetDNATEntry · SetDNATEntryV6 · SetFlowConfig · SetFlowTimeout · SetNAT64Config · SetNAT64Count · SetNATPoolConfig · SetNATPoolIPV4 · SetNATPoolIPV6 · SetNPTv6Rule · SetPolicyRule · SetScreenConfig · SetSNATEgressIP · SetSNATRule · SetSNATRuleV6 · SetStaticNATEntryV4 · SetStaticNATEntryV6 · SetZonePairPolicy
Every error in this class is "<map> not found" or an ebpf.Map.Update failure. Both are properties of a loaded eBPF dataplane, not of the config. A pre-pass has no maps and cannot reproduce them — and should not: a fake that failed here would reject every config on a host where the pre-pass legitimately has no dataplane state. Their being no-ops in the fake is correct, not an oversight.
Class C — void or non-error (13)
DeleteStaleApplications · DeleteStaleDNATStatic · DeleteStaleDNATStaticV6 · DeleteStaleNAT64 · DeleteStaleNPTv6 · DeleteStaleSNATRules · DeleteStaleSNATRulesV6 · DeleteStaleStaticNAT · DeleteStaleZonePairPolicies · ZeroStaleNATPoolConfigs · ZeroStaleScreenConfigs · IsLoaded · GetPersistentNAT
Nothing to diverge on — no error channel. GetPersistentNAT returns a typed nil from the fake against a live table from the shim, and both of its consumers (compiler_nat.go) nil-check before use, so the pre-pass skips ClearPoolConfigs/SetPoolConfig while production runs them. Neither returns an error, so the skip cannot change the compile's outcome.
Why the class-A count of 1 is load-bearing: the two real backends disagree with each other
This is the part that makes "hardcode the parse into the fake" the wrong fix rather than merely an unnecessary one.
(*Manager).SetAddressBookEntry(maps_policy.go) — validates vianet.ParseCIDR.userspaceShimCompileDataplane.SetAddressBookEntry(loader.go, the dataplaneManager.CompileUserspaceShimactually passes toCompileConfig) — a barereturn nil.
There is no single rule the fake can hardcode that is correct for both. Matching the eBPF Manager makes the pre-pass stricter than the live backend: compileAddressBook renders an address with no usable prefix as the literal "/32", and that shape is deliberately a WARNING and never a hard reject (pkg/config/compiler_validate_warn.go — "rejecting it would brick existing configs"). So a parsing fake would fail configs at commit that the runtime accepts today.
Matching the live shim — a bare return nil — is what the fake already does. The divergence is therefore confined to one method on a backend that cannot be constructed, which is exactly what the canary in compiler_prepass_fidelity_4960_test.go pins: it reds if userspaceShimCompileDataplane ever gains a non-no-op method, i.e. the moment the premise stops holding.
Method sets and the class split were re-derived mechanically at e42fa044c rather than read off the earlier round.
Owner
Author
The multi-trial revert probe's escape rate depends on INSERTION ORDER, not on the element countThree independent measurements of It has been described throughout as a function of the element count — "the offender is visited first roughly 1/N of the time". That framing is incorrect, and not only because Go's map iteration is non-uniform. MeasuredSame nine keys (
A factor of five, from insertion order alone, with N and the key set held constant. The distribution is strongly skewed either way: in the real fixture, Reproduce by building the map both ways and sampling the first key of a Why it belongs next to
|
Round-8 fold of the scoped re-gate. The blocking item is mine. Round 6 added PolicyNames, PolicySets and AppNames to the ID driver's return map, with a comment claiming an order-dependent id would show there and nowhere else. The DeepEqual loop still listed eight keys and none of the three, so nothing read them: a package-level counter perturbing result.PolicyNames across passes left the test passing, while the same mutation shape on a compared column reds. Three columns were asked for, three columns appeared, and the property stayed unmeasured — a prescribed test form satisfied with the defect intact. The acceptance check for adding a column to a comparison-driven test is a mutation in that column, never the column's presence, and that is now recorded at the loop. Each of the three is proven separately; the drift has to be injected after the phase allocates the map, because PolicyNames and AppNames are not built by newValidationResult and an injection above that panics rather than reds. The persistent-NAT assertion floored at "more than zero" while matching the literal receiver name, and compileNAT has two such blocks — so renaming one dropped coverage from three methods to one with the floor still satisfied, leaving a fallible RegisterNATIP unnoticed. The receiver identifiers are now derived from the GetPersistentNAT assignments, which makes a rename harmless rather than invisible, and the count is exact, matching the discipline the ordering guard already used. Deleting a whole pnat block now fails with the counts named. Both halves of the fidelity guard read their files by name, so moving a shim method into a sibling file with a rejecting body left it passing — and the phase in question runs after compileZones has mutated the host, which is the shape this whole change exists to prevent. The method-count floor only catches a wholesale move; with 55 methods, fifteen can migrate first. It now scans the package directory, as the sole-writer canary already did, and matches the receiver name exactly rather than by substring. The sole-writer canary's header claimed it caught a second writer anywhere. It binds two syntactic forms; a local alias and a helper taking the map as a parameter both evade it. Those escapes are inherent to a canary of this kind and are not chased — the header now says so, and says which regression it does cover. Also: the pnat declaration lookup was keyed by bare method name across every receiver in the file, which is last-declaration-wins the moment a second appears; a dead strings import kept alive by a blank assignment; a single-argument filepath.Join; and a hand-rolled itoa. go build ./..., go vet ./pkg/dataplane, go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1, and gofmt -l all pass. Advances #4960.
claiming success Cherry-pick candidate on top of fdc19ca. It carries only what that head does not already have, or has less well. F1, F2 and the F5 ordering treatment from the parallel r5 lane are deliberately NOT here; _Log.md records why for each. F4, stated accurately: the BLOCKING half was fixed at fdc19ca while this was being written. That commit appended PolicyNames / PolicySets / AppNames to the comparison list, which settles it. What is left is one level up. The failure mode was a SECOND list falling out of sync with the returned map, and a list that must be edited whenever a column is added can fall out of sync again -- it just did, once. This derives the key set from the map, so a new column is compared the moment it exists. Measured on this tree with one mutation, seeding policySetID from a package-level sequence: the old eight-key list is GREEN, the eleven-key list and the derived set are both RED on "PolicyNames differs between pass 1 and pass 2". So grade this as recurrence prevention, not as a defect fix. The floors are not made redundant by it and stay: a column that is present but empty compares clean forever, which the loop cannot see. Adding a column needs both -- membership, and a floor that fails if the fixture stops producing it. PolicyScheduleRuleSlots joins the comparison, and the r8 refusal of it was CORRECT on its own terms: compiler.go appends only under `if pol.SchedulerName != ""`, neither probe policy set it, so the column would have been nil-vs-nil. This removes the premise rather than overriding the conclusion -- p-single now carries a SchedulerName, which reaches the sole writer, and only then is the column added. Adding it without widening the fixture would have been precisely the vacuous comparison r8 declined. It is also the column that DISCRIMINATES: under the seeded-id mutation it and PolicyNames both move while PolicySets, a count, does not, and that asymmetry is only visible if the columns are genuinely consumed. F6. Eleven INFO records inside covered phases reported completed work unconditionally. Every dataplane write in the pre-pass is a no-op, so a failed apply's journal read "static NAT compilation complete" and then the failure, for a compile whose result was thrown away. All are now behind !isValidationPass(dp), with a two-way grid: removing one gate REDs the negative test, over-widening it to suppress BOTH passes REDs the over-reach guard. WARN duplication in the same phases is left alone and stays on #6903 -- noise, not a false success claim. F3 is one measurement, not a rewrite. This head already corrected the "coin flip" and uniformity errors and measured 0.0632. What it does not say is that the rate is a property of the fixture's INSERTION ORDER and not of the element count: measured over 200,000 constructions, 8-valid-then-offender gives 0.0622 while offender-first gives 0.31 -- same nine keys, same value type, a factor of five. So hoisting the offender's assignment above the builder loop is a refactor that changes no test, no N and no constant, and silently moves the 24-trial bound from ~1e-29 to ~1e-12. Recorded next to the constant. F5 is two stale comments. The r7 reorder moved every row index but no row name, and three comments still read "row 2 / row 3 / row 5 / row 7". Replaced with row NAMES so they cannot rot on the next insertion. Validation at this head: go build ./... rc=0, go vet ./pkg/dataplane/... rc=0, go test ./... rc=0. Every mutation cell re-run on this tree rather than inherited from the branch it was written on. _Log.md union-resolved against the r8 entry and verified structurally -- the upstream file is an exact prefix of the result, after a first attempt silently dropped 66 lines of it. Advances #4960. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
939405d is a clean descendant of fdc19ca, so there is no union to resolve. What this adds is the verification the merging lane owes rather than the incoming lane's self-report. The _Log.md structural check: 70812 to 70883 lines, and the older file is an exact prefix of the newer, so nothing of the previous entry was dropped. Growth alone would not have shown that — a union that both adds and silently drops still grows — which is how the other lane lost 66 lines before catching it. The producibility check on the fixture widening, which is the merging lane's to run. PolicyScheduleRuleSlots was declined earlier because the writer appends only under a non-empty SchedulerName and no probe policy set one, making the column nil against nil. The widening removes that premise, and the column now yields one real slot with its PolicySetID, RuleIndex, RuleID and scheduler name populated — confirmed by running it, not by reading the fixture. The six gated INFO sites are the runtime regression this round exists for, and they were introduced by this PR's own pre-pass: every commit double-logged them, and a config the pre-pass then rejected still logged persistent-NAT and static-NAT success for work that never happened. Both arms of the guard are kept — without the over-reach arm, a gate widened to suppress both passes would satisfy the first. Both ID-driver treatments are kept. The derived key set is what stops the defect recurring, since a parallel list fell out of sync exactly once already; the per-column mutation proof is what shows it currently binds. Re-run on the merged result, all four columns red independently. The two lanes ran different experiments rather than disagreeing: a single mutation can only move the columns it touches. go build ./..., go vet ./pkg/dataplane, go test ./pkg/dataplane/... ./pkg/config ./pkg/daemon -count=1, and gofmt -l all pass. Advances #4960.
Owner
Author
Independent Codex leg at
|
| file | sites |
|---|---|
compiler.go |
7 = 4 Info + 3 Warn |
compiler_nat.go |
36 = 13 Info + 23 Warn |
compiler_iface.go |
1 Info |
| total | 44 = 18 Info + 26 Warn |
Gated: 19 (all 18 Info + 1 Warn). Ungated: 25 Warn. Two sites cannot emit on the pre-pass at all (persistent-NAT success — the fake GetPersistentNAT() returns nil; failed-SNAT-egress — its fake setter cannot fail), so runtime-emittable is 42.
The gate itself is complete and narrow — no reachable success Info escapes, and the real apply path does not carry the validation marker. What is incomplete is the binding:
- 13/19 — removal RED, suppress-both RED
- 1/19 — removal RED, suppress-both GREEN (
screen profile compiled) - 5/19 — both mutations GREEN (default-deny, source-NAT-off, SNAT-egress-success, no-address SNAT warning, persistent-NAT registration)
For Info alone the negative arm covers 14/18 and the over-reach arm 13/18. Widening the screen gate to suppress both passes left go test ./pkg/dataplane -count=1 fully green — the over-reach control is unbound for a reachable real-pass record. The persistent-NAT gate is the sharper case: it cannot fire on the pre-pass path it ostensibly protects.
MINOR — derived key set works; vacuity and aliasing do not
Adding a divergent column with no comparison-loop edit correctly REDs TestPrePassDoesNotPerturbIDAssignment_4960. The opposite control fails: an always-empty column compares {} vs {} with no automatic floor. Several floors also contradict the "specific entry, not merely non-empty" claim — ZoneIDs/AppIDs/AppNames are non-empty only, PolicySets nonzero only, PoolIDs pins only the NAT64 pool, NextPoolID only "greater than". Exact producer checks do exist for ScreenIDs, AddrIDs, NATCounterIDs, implicitSets, PolicyNames, PolicyScheduleRuleSlots.
Aliasing hole: first retains live map/slice references, the second compile runs, and only then does reflect.DeepEqual compare (compiler_idprobe_4960_test.go:443). A regression using process-global or interned storage would mutate both observations at once, leaving the comparison and the post-second-pass floors green — the precise failure class this test claims to cover. first must be deeply snapshotted before the second compile.
Confirmed sound
- The scheduler fixture does reach the writer:
p-singlecarriesSchedulerName: "sched-6894"and exercises the zone-policy append branch (compiler.go:993); the one-slot assertion pinsPolicySetID=0,RuleIndex=2,RuleID=2, policy name and scheduler name. Not nil-vs-nil. - Apart from NPTv6, the pass does not reorder, duplicate or change real mutations. Pool IDs, NAT counter IDs and
NextPoolIDlive in freshCompileResultinstances. No off-by-one, no reuse-after-reject. - A pre-pass rejection holds no lock and leaves only the discarded local result/caches;
compileZoneshas not run.
Caveat recorded by the reviewer, and I am carrying it rather than burying it: its sandbox was read-only, so it could not create fresh mutations or a Go build cache, and it did not score those failures as REDs. The mutation conclusions above come from preserved exact-head executions with explicitly non-zero test matches, plus source verification.
Verdict: MERGE-BLOCK. The NPTv6 case blocks on its own; the other two are the same family as the defects already fixed this round (a guard whose evidence does not reach the property it names).
The #4960 validate-before-mutate pre-pass exists to reject a config that will fail the apply BEFORE compileZones performs the first destructive host netlink mutation. An independent review found the pre-pass violating its own property, from inside a row it explicitly covers. Match = "2001:db8:9::/48" Then = "not-a-prefix" Lenient validation retains that rule with a warning (#1960 no-brick). compileNPTv6 warned, `continue`d and returned nil. buildNptv6Snapshots copied both strings through verbatim. Rust's Nptv6State::try_from_snapshots then rejected the WHOLE snapshot, and that rejection lands in publishSnapshotFailClosedLocked -- after VLANs were created and addresses reconciled, with no rollback. The `nptv6` row is in validationPhases, so the file's downstream-failure carve-out did not cover it. compileNPTv6 now returns an error for the parse class the helper refuses: unparseable prefix, mismatched lengths, an unsupported length, a non-IPv6 prefix, and -- new here -- host bits set beyond the prefix length, which the Go compiler silently masked by truncating to the prefix bytes while parse_prefix fails CLOSED on it (#4519). That moves an ALREADY-CERTAIN apply failure ahead of the mutation point; it creates no new one. Two properties keep it from over-rejecting. It fires only for rules that reach the helper: a rule the snapshot builder drops for an unsupported match scope (#5818) keeps warn-and-skip, because today's apply succeeds without it. config.NPTv6ScopeUnsupported is the new single source of truth both the builder and the compiler read, so the two cannot drift apart. And it cannot brick a boot or an HA peer sync -- configstore's Load and SyncApply compile through pkg/config.compileTreeLenient and never reach CompileConfig, so the config still loads with the lenient warning and only the dataplane apply, which already failed at publish, fails. Making the Go builder DROP the malformed rule instead was rejected: that is the fail-open #2240 closed. DeleteStaleNPTv6 runs over only the valid subset, so a rule edited into an invalid one would have its working translation torn down with no replacement, and validateNPTv6Strict's own lenient warning promises the opposite ("the helper rejects the whole NPTv6 snapshot and the previous state is kept"). Residual, named in the code: the helper also rejects overlapping prefixes (#2241) partitioned by zone scope (#5176). That partitioning is not replicated -- reusing the commit-time overlap check, which does not partition, would reject configs the helper accepts. An overlap still lands post-mutation. Validation. Reverting the compileNPTv6 change reds the three rejection tests inside assertNoHostMutation with "compileZones RAN before the failing phase was caught (1 SetZoneConfig calls)", while the five scope-excluded sub-cases and the valid-NPTv6 control stay green. A builder that re-inlines the scope predicate without one dimension reds the cross-package parity test. The Rust half was run directly: three try_from_snapshots rejection tests pass against the same fixture strings. Also folded from the same review: The log gate's over-reach arm named 13 of 19 gated records, so six gates could be widened to suppress the REAL pass with the package green -- measured, including the screen-profile record. The pre-pass's reachable set is now measured rather than reasoned about: 17 of 19 sites are reachable and bound, the two that are not (SNAT-egress needs a populated ifCache, persistent-NAT a live table) are stated at the test, and the gate count is derived by AST so a new gate cannot be added unaccounted. The ID-determinism probe compared live map references only after the second compile, so a regression using process-global storage would mutate both observations at once and leave it green -- the exact class it names. It now renders the first pass to strings before the second runs; the same simulated regression is RED with the snapshot and GREEN without it. A mechanical floor rejects any always-empty column, and four floors that checked only non-emptiness were tightened or derived: PolicySets is exact, NextPoolID is max+1, AppNames agrees with AppIDs. Advances #4960. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Comment-only. No production behaviour, no test and no fixture changed;
the only non-comment edit in this commit is the _Log.md entry.
The pre-pass's no-brick argument was written as a call trace: configstore's
Load and SyncApply "compile through pkg/config.compileTreeLenient and never
reach CompileConfig". That is true, but it is a statement about today's
call sites. One new call site makes it silently false, and false in the
direction that matters, because the next reader trusts the comment and
skips the check.
The durable form is structural: pkg/dataplane is not in pkg/configstore's
dependency CLOSURE at all -- not merely un-imported directly, but
unreachable through any intermediate package -- so no tolerant load or HA
peer sync can enter the pre-pass, and the path cannot be created without a
visible new dependency edge. A call trace answers "is this reached today";
an import closure answers "is this expressible at all", and a safety claim
needs the second.
The comment now carries the re-verification commands, including the -test
variant, since a _test.go import would be a real edge for this purpose:
go list -deps ./pkg/configstore | grep -c psaab/xpf/pkg/dataplane
go list -deps -test ./pkg/configstore | grep -c psaab/xpf/pkg/dataplane
and, alongside them, the positive control that makes those zeros mean
something:
go list -deps ./pkg/daemon | grep -c psaab/xpf/pkg/dataplane
A bare count of 0 is indistinguishable from a broken query -- a wrong
module path, a wrong flag or a swallowed build failure all print 0 too.
The pkg/daemon leg must be NON-ZERO, which proves the query can find a
real edge and converts the zeros from an absence of output into an absence
of dependency. Its exact count is deliberately not written down: this file
already carries one lesson about a number stated in a comment going stale
(the "ONE such error" note in validationPhases). Non-zero is what makes
the query trustworthy; the value is not.
Measured at ba3aa2c: both configstore legs 0, the daemon control
non-zero over four real edges (pkg/dataplane, .../runtime, .../userspace,
.../userspace/format).
Validation: gofmt clean; go vet ./pkg/dataplane/... rc=0; go test
./pkg/dataplane/... ./pkg/config/... -count=1 rc=0. The diff was checked
mechanically to be comment-only -- zero non-comment lines added and zero
removed in the .go file.
Advances #4960.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
# Conflicts: # _Log.md
# Conflicts: # _Log.md
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
1 participant
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.
Advances #4960 (bounded increment — see Scope).
The defect
CompileConfigruns at apply time, after the commit already succeeded.pkg/configstorehas zero imports ofpkg/dataplane, socommit checkvalidates only the purepkg/config.CompileConfigand never reaches this one — the two share a name, which is much of why this path reads as safer than it is.Phase 2
compileZonesthen performs the first destructive host netlink mutation:ensureVLANSubInterfacecreates and links up VLAN devices,reconcileInterfaceAddressesdeletes and adds addresses. Every later phase is a barereturn nil, errwith no undo path.The reachable input class is documented by the codebase itself at
userspace/flow.go:139— a malformed application-set reference or an app_id-space overflow (#3438 H4) makescompileApplications(Phase 4) "hard-error and abort the apply". So an ordinary commit can leave VLANs created and addresses reconciled on the live host, control plane reporting failure, dataplane still on the old snapshot.The fix
validateBeforeMutateruns the eleven fallible host-pure phases against adiscardingDataPlanebeforecompileZonestouches anything.Why additive, not a reorder — this is the load-bearing design point
Moving the real phases ahead of
compileZoneslooks equivalent and is strictly worse.compileFirewallFiltersandcompilePortMirroringresolve VLAN sub-interface names that exist only because Phase 2 created them, and a miss there isslog.Warn(...); continue. A reorder would leave firewall filters silently unassigned on every apply — trading a loud post-mutation abort for a quiet fail-open on a security surface.The precondition Phase 2 supplies for free is the VLAN device existing, and a soft skip makes that precondition unobservable at the call site.
Coverage, and a deliberate gap
Thirteen of thirteen fallible post-zones phases are validated. The two excluded are exactly the two above: running them pre-mutation would evaluate a different world than the real pass — every sub-interface lookup missing and soft-skipping — so the pre-pass would emit misleading warnings on every successful commit and still not predict the real run. A hard failure inside those two still occurs post-mutation. That residual is named in the code, not papered over.
Those soft skips are also a live fail-open independent of this PR:
ensureVLANSubInterfacefailing isslog.Warn; return nil, socompileZonessucceeds with the device absent, the later filter lookup misses and continues, and the commit reports success with the filter unapplied. Filed as #6893. One root cause, two reachability paths — which is precisely the argument for keeping this fix additive: a reorder would turn an occasional fail-open into a universal one.Correction to prior art
research/4960-apply-txn§4.4 states destructive host mutation is in "exactly two phases —compileZonesandcompilePortMirroring(compiler.go:1704, netlink at:1758/:1811)". Verified wrong at 86927d2:compilePortMirroringspans 1704–1748 and contains nonetlink.reference at all — onlydp.ClearMirrorConfigs/dp.SetMirrorConfigmap writes.:1758and:1816arenetlink.LinkByName— reads — insidegetPermAddrandgetOriginalKernelName.Line numbers have not drifted (that plan's own
:1704is still exact), so this was wrong when written. It is one phase, which is why a single pre-pass covers all host mutation with nothing left over. §4.4's middle-phase purity claim does agree with the enumeration re-derived independently here.Scope — this is not the apply-transaction redesign
Making the Go and Rust planes re-converge on a dataplane NACK is the other half of #4960, tracked on
research/4960-apply-txn(twice PLAN-NEEDS-MAJOR). That work is about what happens after the dataplane is asked; this is about not wrecking the host before it is asked, and needs no fence and no daemon disposition changes.Note "no mutation" here means host netlink mutation. Every phase writes dataplane map state, which is re-derivable by the next successful compile and is what the fence work addresses.
Validation
TestNoHostMutationWhenALaterPhaseFails_4960compileApplicationsleaves zeroSetZoneConfigand zeroSetVlanIfaceInfocallsTestValidConfigStillReachesZoneCompile_4960compileZones; without it the property test is satisfied by a pre-pass that rejects everythingTestPrePassDoesNotPerturbIDAssignment_4960Why
SetZoneConfigis a sound probe:programZoneMapscalls it once per zone before iterating that zone's interfaces, andmapZoneInterface— the only caller of the netlink mutators — runs inside that iteration. Zero calls provescompileZonesnever started.Revert cell: removing the
validateBeforeMutatecall REDs the property test — "compileZones RAN before the failing phase was caught (1 SetZoneConfig call)".ID-determinism probe proven sensitive: injecting process-global drift after
finalizeNATCounterIDsre-derives the map REDs it. Injecting before is overwritten by the re-derive and proves nothing — a negative control placed upstream of a normalizer tests nothing.go test ./...rc=0.🤖 Generated with Claude Code
https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi