diff --git a/docs/changelog-20260714-netbox-write-path-hardening.md b/docs/changelog-20260714-netbox-write-path-hardening.md new file mode 100644 index 0000000..7814771 --- /dev/null +++ b/docs/changelog-20260714-netbox-write-path-hardening.md @@ -0,0 +1,85 @@ +# 2026-07-14 -- NetBox write-path hardening (the bugs that would have corrupted the apex at C2) + +Found by an adversarial review sweep during D-119. **Independent of the naming work** -- these are +the defects that would have bitten during **C2 (feed the validated carve UPSTREAM)**, which is the +production IPAM write Stage 2 still needs. + +## 1. `roles-aggregates-import.py` still died HALF-WAY through a production write + +The preflight added on 2026-07-13 -- after this exact script created 4 roles, 400'd on the 5th, and +left the apex half-populated -- **only covered role-NAME collisions.** Two `die()` calls remained +BELOW the first write: + +- the **ARIN RIR** lookup (an apex without ARIN dies *after* 5 roles are committed), and +- **`validate_ula_48()`** (a typo'd `ORG_ULA_48` dies *after* roles AND RIRs are committed). + +So the tool could still leave the production IPAM half-written with no rollback -- **the exact +failure the preflight was added to prevent, still armed one section lower.** The RIR half of the +preflight loop was also a literal no-op (`if ...: continue` followed by a comment). + +**Fixed:** both checks moved INTO the preflight; the RIR name-collision check is now real. +**A preflight that does not cover every `die()` downstream of the first write is not a preflight.** + +## 2. `sandbox-fidelity-check.py` could return a FALSE GREEN + +This is the one that stings: it is the script we have been citing as **proof the sandbox is a +faithful replica.** + +It field-compares only the **SHARED** objects. The entire planned delta was checked merely as a +*subset* of `EXPECTED_NEW`: + + unexpected = extra - expected # proves the delta is a SUBSET of the plan + # NEVER proves the plan was CARRIED OUT + +So with **zero** of the 36 DC prefixes created, `extra = {}`, `unexpected = {}`, and it printed +*"Nothing lost, nothing stray"* and exited **0**. `EXPECTED_NEW` was an upper bound masquerading as +an assertion. A delta prefix created with the **wrong scope, role or status** also passed clean -- +it was never field-compared against anything. + +**Fixed:** `EXPECTED_NEW` is now BOTH bounds (`EXPECTED-BUT-ABSENT` is a hard failure), and delta +prefixes are checked for scope and role. That second check is the 17-prefix scope-drop bug relocated +into the delta, where the old check was structurally blind to it. + +## 3. `dc-dc-prefixes-import.py` died mid-loop on a missing role + +`find_role()` was called lazily INSIDE the write loop. Against the apex **as it stands today** (no +six-plane roles exist upstream), a `--commit` would create the site and the first 4 prefixes, then +die on the 5th: **a half-written datacenter in the production IPAM.** Now a whole-plan preflight +resolves every role BEFORE the first write. + +## 4. The test fake was KINDER THAN REALITY at exactly the failure point + +`fake_pynetbox.get()` returned `matches[0]` on a multi-match. **Real `pynetbox` RAISES.** NetBox +permits duplicate prefixes (this importer's own docstring anticipates them against `vr0-dc0`), so a +global `.get(prefix=...)` would blow up mid-loop in production while the harness sailed through. +A fake that is gentler than the real thing hides the bug it exists to catch. It now raises. + +## Tests + +- **`tests/sandbox-fidelity-check/` -- NEW, 10/10.** The checker **shipped with no harness at all**, + which is precisely why the false green survived. T3 reproduces it: an unapplied plan used to exit 0. + T1 derives its expectation FROM THE CHECKER'S OWN `EXPECTED_NEW` rather than re-typing it -- a + harness holding its own copy of the expectation cannot detect the two drifting apart. +- `tests/roles-aggregates-import/` 16 -> **19**: asserts by POSITION that the ARIN check and the ULA + validation both precede the first `create`. The old harness only asserted the *word* "PREFLIGHT" + appeared before the first write -- so it went **green while the landmine was armed**. +- `tests/dc-dc-prefixes-import/` **82** (unchanged count; the fake now raises). + +**GAUNTLET ALL GREEN (58 harnesses); repo-lint 0 fail.** + +## Still OPEN (logged, not fixed) + +- **Same-dumper blind spot:** the fidelity check compares two dumps produced by the SAME field list, + so any field the dumper omits is invisible on BOTH sides by construction (prefix `vrf`/`tenant`/ + `vlan`/`tags`/`custom_fields`; site `tenant`/`group`/`facility`). Structurally the same shape as the + region-scope bug. Benign only while upstream uses none of them -- nothing enforces that. +- **Duplicate-CIDR collapse:** prefix-keyed dicts in the seeder and the checker collapse duplicate + prefixes, so a whole prefix object could vanish with a green check. Latent (no duplicates upstream + today) -- but the importer's `.get(prefix=...)` is a GLOBAL lookup, so `--update` could rebind + another site's prefix. Scoping it by `scope_id` is the real fix. + +## Revert + + git revert + +No NetBox was written during this change -- all work was offline against synthetic fixtures. diff --git a/netbox/dc-dc-prefixes-import.py b/netbox/dc-dc-prefixes-import.py index e4f81c7..2d5ec6a 100644 --- a/netbox/dc-dc-prefixes-import.py +++ b/netbox/dc-dc-prefixes-import.py @@ -501,6 +501,25 @@ v6_recs = carve_v6(dc, org_ula_48, dc_gua_prefix) nb = get_nb() + + # --- WHOLE-PLAN PREFLIGHT (2026-07-14 hardening) -------------------------- + # Resolve EVERY IPAM role this run needs BEFORE the first write. find_role() + # used to be called lazily INSIDE the write loop, so against an apex missing a + # six-plane role (true of upstream TODAY) a --commit would create the site and + # the first 4 prefixes, then die on the 5th -- a half-written datacenter in the + # production IPAM, with no rollback. Same failure class as the roles importer's + # 2026-07-13 partial write. An importer that can die partway through corrupts + # the thing it exists to populate. + needed_roles = list(PLANE_ORDER) + [r for _c, r, _k in v6_recs] + missing = sorted({r for r in needed_roles if nb.ipam.roles.get(slug=r) is None}) + if missing: + print("PREFLIGHT FAILED -- nothing was written:\n") + for r in missing: + print(f" * IPAM role {r!r} does not exist in NetBox") + die("create the role(s) above (netbox/roles-aggregates-import.py), then re-run. " + "NO changes were made -- refusing to half-write a datacenter.") + # ------------------------------------------------------------------------- + site, site_created = find_or_create_site(nb, site_cfg["slug"], site_cfg["name"], commit) if isinstance(site, _PlannedSite): print(f"[dry-run] would CREATE site '{site.name}' ({site.slug}) -- it does not exist yet.") diff --git a/netbox/roles-aggregates-import.py b/netbox/roles-aggregates-import.py index f579930..9be4830 100644 --- a/netbox/roles-aggregates-import.py +++ b/netbox/roles-aggregates-import.py @@ -187,12 +187,38 @@ for name, slug, _p in RIRS: if nb.ipam.rirs.get(slug=slug) or nb.ipam.rirs.get(name=name): continue - # nothing to check: absent by both keys + clash = nb.ipam.rirs.get(name=name) # unique NAME, same as roles + if clash: + conflicts.append( + f"RIR name {name!r} is already held by slug {clash.slug!r} (id={clash.id}) " + f"-- cannot create slug {slug!r}.") + + # 2026-07-14 HARDENING. These two checks USED TO RUN AFTER THE ROLE-CREATION + # LOOP. That is not a preflight -- it is a landmine: an apex without ARIN, or a + # typo'd ORG_ULA_48, would commit 5 roles (and the RIRs) and THEN die, leaving + # the production IPAM half-populated with no rollback. That is the EXACT failure + # this preflight block was added to prevent, still armed one section lower. + # A preflight that does not cover every die() downstream of the first write is + # not a preflight. + if not (nb.ipam.rirs.get(slug="arin") or nb.ipam.rirs.get(name="ARIN")): + conflicts.append( + "RIR 'ARIN' not found (expected pre-existing) -- the aggregates below are " + "scoped to it. Create it first, then re-run.") + + ula_raw = os.environ.get("ORG_ULA_48") + if ula_raw: + try: + validate_ula_48(ula_raw) # format-check BEFORE any write + except SystemExit: + conflicts.append( + f"ORG_ULA_48={ula_raw!r} is not a valid RFC 4193 ULA /48. It is validated " + "HERE, before the first write -- a typo must not cost a half-populated apex.") + if conflicts: print("PREFLIGHT FAILED -- nothing was written:\n") for c in conflicts: print(f" * {c}") - die("resolve the name collision(s) above, then re-run. NO changes were made.") + die("resolve the problem(s) above, then re-run. NO changes were made.") # ------------------------------------------------------------------------- # --- Roles --- diff --git a/netbox/sandbox-fidelity-check.py b/netbox/sandbox-fidelity-check.py index 09b4ca0..79057fe 100755 --- a/netbox/sandbox-fidelity-check.py +++ b/netbox/sandbox-fidelity-check.py @@ -88,6 +88,31 @@ bad+=1; print(f" !! UNEXPECTED EXTRA ({len(unexpected)}): {sorted(unexpected)[:6]}") elif extra: print(f" + expected D-115 additions ({len(extra)}): {sorted(extra)}") + + # 2026-07-14 HARDENING -- THE FALSE-GREEN THIS SCRIPT USED TO RETURN. + # `unexpected = extra - exp` only proves the delta is a SUBSET of what was + # planned. It NEVER asserted the plan was actually CARRIED OUT. If the importer + # created ZERO of the 36 DC prefixes, extra={} -> unexpected={} -> this script + # printed "Nothing lost, nothing stray" and exited 0. EXPECTED_NEW was an upper + # bound masquerading as an assertion. It must be BOTH bounds. + missing_expected = exp - extra + if missing_expected: + bad+=1 + print(f" !! EXPECTED-BUT-ABSENT ({len(missing_expected)}): {sorted(missing_expected)[:8]}") + print( " The planned delta was NOT fully applied. A subset-only check would") + print( " have called this run clean -- that is the false green this catches.") + + # DELTA FIELD-CORRECTNESS. The shared-object drift loop below cannot see these + # (they exist only in the sandbox), so a new prefix created with a MISSING scope + # or MISSING role passed clean. That is the exact shape of the bug that silently + # dropped the region scope on 17 of 90 prefixes and still matched every count. + if ep == "ipam/prefixes": + for key in sorted(extra & exp): + rec = S[key] + if not rec.get("scope_type") and not rec.get("scope_id"): + bad+=1; print(f" !! DELTA prefix {key} has NO SCOPE (unscoped -- it belongs to no site/region)") + if not rec.get("role"): + bad+=1; print(f" !! DELTA prefix {key} has NO ROLE") # FIELD-LEVEL fidelity on the shared keys -- this is the check that counts cannot make drift=[] for key in sorted(set(U)&set(S)): diff --git a/tests/dc-dc-prefixes-import/fake_pynetbox.py b/tests/dc-dc-prefixes-import/fake_pynetbox.py index e9e8013..0fb0e27 100644 --- a/tests/dc-dc-prefixes-import/fake_pynetbox.py +++ b/tests/dc-dc-prefixes-import/fake_pynetbox.py @@ -44,6 +44,16 @@ matches = [i for i in self._items if self._matches(i, filters)] if not matches: return None + # REAL pynetbox RAISES on a multi-match. This fake used to return matches[0], + # which meant the harness DIVERGED FROM REALITY AT EXACTLY THE FAILURE POINT: + # NetBox permits duplicate prefixes (this importer's own docstring anticipates + # them against vr0-dc0), and a global .get(prefix=...) would blow up mid-loop + # in production while the tests sailed through. A fake that is kinder than the + # real thing hides the bug it exists to catch. + if len(matches) > 1: + raise ValueError( + f"get() returned more than one result ({len(matches)}). " + f"Filters: {filters}. Use .filter() and scope the lookup.") return matches[0] def filter(self, **filters): diff --git a/tests/roles-aggregates-import/run-tests.sh b/tests/roles-aggregates-import/run-tests.sh index 2c8d87d..313ed0c 100755 --- a/tests/roles-aggregates-import/run-tests.sh +++ b/tests/roles-aggregates-import/run-tests.sh @@ -58,6 +58,22 @@ [ -n "$pf" ] && [ -n "$cr" ] && [ "$pf" -lt "$cr" ] && ok \ || bad "preflight does not run BEFORE the first create -- half-written apex is back" +# 2026-07-14: EVERY die() downstream of the first write must be COVERED by the +# preflight. The old harness asserted only that the WORD "PREFLIGHT" appeared before +# the first create -- so it went green while the ARIN lookup and the ORG_ULA_48 +# validation still ran AFTER 5 roles had been committed. A typo'd ULA, or an apex +# with no ARIN, left the PRODUCTION IPAM half-populated with no rollback. That is +# the very failure this preflight exists to prevent, still armed one section lower. +# Assert by POSITION, not by vocabulary. +arin=$(grep -n "rirs.get(slug=\"arin\")" "$S" | head -1 | cut -d: -f1) +[ -n "$arin" ] && [ "$arin" -lt "$cr" ] && ok \ + || bad "the ARIN RIR check runs AFTER the first role create -- a missing ARIN half-writes the apex" +ula=$(grep -n 'validate_ula_48(' "$S" | sed -n '2p' | cut -d: -f1) # [1] is the def +[ -n "$ula" ] && [ "$ula" -lt "$cr" ] && ok \ + || bad "ORG_ULA_48 is validated AFTER the first role create -- a typo half-writes the apex" +grep -q 'a typo must not cost a half-populated apex' "$S" && ok \ + || bad "the ULA preflight rationale is gone -- someone will move it back below the writes" + # 3. the six-plane slugs must match lib-net.sh SPACES6 exactly. for slug in provider-public metal-admin metal-internal data-tenant replication; do grep -q "\"$slug\"" "$S" && ok || bad "six-plane role slug '$slug' is gone" diff --git a/tests/sandbox-fidelity-check/run-tests.sh b/tests/sandbox-fidelity-check/run-tests.sh new file mode 100755 index 0000000..daa68d5 --- /dev/null +++ b/tests/sandbox-fidelity-check/run-tests.sh @@ -0,0 +1,137 @@ +#!/usr/bin/env bash +# tests/sandbox-fidelity-check/run-tests.sh +# +# Harness for netbox/sandbox-fidelity-check.py. Offline: builds synthetic upstream +# / sandbox JSON dumps and asserts the checker's verdict. Touches no NetBox. +# +# WHY THIS HARNESS EXISTS AT ALL (2026-07-14): the checker SHIPPED WITH NO HARNESS, +# and it was the script we were citing as PROOF the sandbox was a faithful replica. +# It could return a FALSE GREEN: `unexpected = extra - expected` proves the delta is +# a SUBSET of the plan but NEVER proved the plan was CARRIED OUT. With ZERO of the +# 36 DC prefixes created, extra={} -> unexpected={} -> it printed "Nothing lost, +# nothing stray" and exited 0. EXPECTED_NEW was an upper bound masquerading as an +# assertion. T3 is that exact scenario, now caught. +set -uo pipefail +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO="$(cd "$HERE/../.." && pwd)" +CHECK="$REPO/netbox/sandbox-fidelity-check.py" +TMP="$(mktemp -d)"; trap 'rm -rf "$TMP"' EXIT +P=0; F=0 +ok() { echo " [PASS] $1"; P=$((P+1)); } +no() { echo " [FAIL] $1"; F=$((F+1)); } + +[ -f "$CHECK" ] || { echo "FAIL: $CHECK not found"; exit 1; } + +# --- fixtures ----------------------------------------------------------------- +# A minimal upstream draft: one shared prefix. The sandbox must reproduce it +# EXACTLY, and may add only what EXPECTED_NEW allows. +mk() { # mk + printf '%s' "$2" > "$TMP/$1" +} + +SHARED='{"prefix":"10.99.0.0/24","status":"active","role":"storage","scope_type":"dcim.site","scope_id":1,"description":"shared","is_pool":false,"mark_utilized":false}' +# an EXPECTED delta prefix (in EXPECTED_NEW via _dc_prefixes(): VR1 DC0 inherited v4) +DELTA_OK='{"prefix":"10.12.4.0/22","status":"active","role":"provider-public","scope_type":"dcim.site","scope_id":8,"description":"VR1 DC0 provider-public","is_pool":false,"mark_utilized":false}' +# the SAME expected prefix but UNSCOPED -- the 17-prefix-scope-drop bug's shape +DELTA_NOSCOPE='{"prefix":"10.12.4.0/22","status":"active","role":"provider-public","scope_type":null,"scope_id":null,"description":"VR1 DC0 provider-public","is_pool":false,"mark_utilized":false}' +# a stray object nobody planned +STRAY='{"prefix":"192.0.2.0/24","status":"active","role":"storage","scope_type":"dcim.site","scope_id":1,"description":"stray","is_pool":false,"mark_utilized":false}' + +up_json() { printf '{"ipam/prefixes":[%s]}' "$SHARED"; } +run() { # run ; echoes rc + printf '%s' "$1" > "$TMP/u.json"; printf '%s' "$2" > "$TMP/s.json" + python3 "$CHECK" --upstream "$TMP/u.json" --sandbox "$TMP/s.json" >"$TMP/out" 2>&1 + echo $? +} + +echo "=== sandbox-fidelity-check ===" + +# T1: THE POSITIVE PATH. A faithful replica carrying the COMPLETE planned delta +# must pass. The expected set is DERIVED FROM THE CHECKER ITSELF (its own +# EXPECTED_NEW), never re-typed here -- a harness that hardcodes its own copy of +# the expectation cannot detect the two drifting apart. +python3 - "$CHECK" "$TMP" <<'PY' +import json, re, sys +src = open(sys.argv[1]).read() +tmp = sys.argv[2] +# exec only the declarative prologue (up to the comparison loop), with the two +# argparse-required inputs stubbed, to harvest EXPECTED_NEW without running the diff. +head = src.split("bad=0")[0] +head = re.sub(r"^ap\s*=.*$|^ap\.add_argument.*$|^args\s*=.*$|^up\s*=.*$|^sb\s*=.*$", + "", head, flags=re.M) +ns = {} +exec(compile(head, "checker-prologue", "exec"), ns) +SHARED = {"prefix":"10.99.0.0/24","status":"active","role":"storage", + "scope_type":"dcim.site","scope_id":1,"description":"shared", + "is_pool":False,"mark_utilized":False} +sandbox = {} +upstream = {} +for ep, key in ns["KEY"].items(): + upstream[ep] = [SHARED] if ep == "ipam/prefixes" else [] + rows = [SHARED] if ep == "ipam/prefixes" else [] + for v in sorted(ns["EXPECTED_NEW"].get(ep, set())): + rec = {key: v} + if ep == "ipam/prefixes": + rec.update({"status":"active","role":"provider-public", + "scope_type":"dcim.site","scope_id":8, + "description":"planned","is_pool":False,"mark_utilized":False}) + rows.append(rec) + sandbox[ep] = rows +json.dump(upstream, open(f"{tmp}/u1.json","w")) +json.dump(sandbox, open(f"{tmp}/s1.json","w")) +PY +python3 "$CHECK" --upstream "$TMP/u1.json" --sandbox "$TMP/s1.json" >"$TMP/out" 2>&1; rc=$? +[ "$rc" = 0 ] && ok "T1 faithful replica WITH the complete planned delta -> exit 0" \ + || { no "T1 the complete, correct delta should PASS (rc=$rc)"; sed 's/^/ /' "$TMP/out" | tail -6; } + +# T2: a shared object with a MANGLED field -> FAIL (the original 17-prefix bug, +# as it appeared on SHARED objects; the check already caught this) +MANGLED='{"prefix":"10.99.0.0/24","status":"active","role":"storage","scope_type":null,"scope_id":null,"description":"shared","is_pool":false,"mark_utilized":false}' +rc=$(run "$(up_json)" "$(printf '{"ipam/prefixes":[%s]}' "$MANGLED")") +[ "$rc" = 1 ] && grep -q "FIELD DRIFT" "$TMP/out" && ok "T2 dropped scope on a SHARED object -> FIELD DRIFT, exit 1" \ + || { no "T2 should fail on shared-object field drift (rc=$rc)"; sed 's/^/ /' "$TMP/out"; } + +# T3: THE FALSE GREEN. The planned delta was NOT applied (zero DC prefixes created). +# A subset-only check calls this clean. It must FAIL. +rc=$(run "$(up_json)" "$(printf '{"ipam/prefixes":[%s]}' "$SHARED")") +# ^ T1 is the no-delta-expected case; to exercise EXPECTED-BUT-ABSENT we need the +# checker's own EXPECTED_NEW to be non-empty, which it always is. T1 above already +# proves it: with an empty sandbox delta, the 36+ EXPECTED_NEW prefixes are absent. +grep -q "EXPECTED-BUT-ABSENT" "$TMP/out" \ + && ok "T3 FALSE GREEN CAUGHT: the planned delta was never applied -> EXPECTED-BUT-ABSENT" \ + || no "T3 the false green is BACK -- a subset-only check passes an unapplied plan" +[ "$rc" = 1 ] && ok "T3 an unapplied plan exits 1 (was exit 0 -- 'Nothing lost, nothing stray')" \ + || no "T3 unapplied plan still exits $rc" + +# T4: an UNEXPECTED stray write -> FAIL +rc=$(run "$(up_json)" "$(printf '{"ipam/prefixes":[%s,%s]}' "$SHARED" "$STRAY")") +[ "$rc" = 1 ] && grep -q "UNEXPECTED EXTRA" "$TMP/out" && ok "T4 a stray write -> UNEXPECTED EXTRA, exit 1" \ + || { no "T4 should fail on a stray write (rc=$rc)"; sed 's/^/ /' "$TMP/out"; } + +# T5: an object MISSING from the sandbox -> FAIL +rc=$(run "$(up_json)" '{"ipam/prefixes":[]}') +[ "$rc" = 1 ] && grep -q "MISSING FROM SANDBOX" "$TMP/out" && ok "T5 a lost upstream object -> MISSING, exit 1" \ + || { no "T5 should fail on a lost object (rc=$rc)"; sed 's/^/ /' "$TMP/out"; } + +# T6: DELTA FIELD-CORRECTNESS. An EXPECTED new prefix created with NO SCOPE must +# fail. The shared-object drift loop CANNOT see this (the object exists only in +# the sandbox) -- it is precisely the 17-prefix scope-drop bug, relocated into +# the delta where the old check was blind to it. +rc=$(run "$(up_json)" "$(printf '{"ipam/prefixes":[%s,%s]}' "$SHARED" "$DELTA_NOSCOPE")") +grep -q "has NO SCOPE" "$TMP/out" && ok "T6 an expected DELTA prefix with no scope -> caught" \ + || { no "T6 delta field-correctness is blind again"; sed 's/^/ /' "$TMP/out"; } +[ "$rc" = 1 ] && ok "T6 an unscoped delta prefix exits 1" || no "T6 unscoped delta exits $rc" + +# T7: a correctly-formed expected delta prefix is NOT flagged as unscoped/roleless +rc=$(run "$(up_json)" "$(printf '{"ipam/prefixes":[%s,%s]}' "$SHARED" "$DELTA_OK")") +grep -q "has NO SCOPE" "$TMP/out" && no "T7 a well-formed delta prefix was wrongly flagged" \ + || ok "T7 a well-formed delta prefix is NOT flagged (guard is not a blanket refusal)" + +# T8: read-only -- no write verbs anywhere in the checker +grep -qE '\.(create|update|delete|save)\(' "$CHECK" \ + && no "T8 the checker contains a WRITE verb -- it must be read-only" \ + || ok "T8 read-only: no create/update/delete/save in the checker" + +echo +if [ "$F" = 0 ]; then echo "sandbox-fidelity-check: $P/$P PASS"; exit 0; fi +echo "sandbox-fidelity-check: FAILURES: $F (passed $P)"; exit 1