From b01ea94a22865896e656f45ce47df80a8e1c89a8 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Mon, 25 May 2026 16:07:01 +1000 Subject: [PATCH] test(network): pin ADR-0006 invariants to executable suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add static-analysis + behavioural tests that bind every safety and liveness clause in ADR-0006 (S1-S9, L1-L3) to a named Go test. Failure output quotes the ADR clause verbatim and points at the offending file:line so future debugging sessions can't quietly bypass a layer contract to make a test green. Static-analysis suite (spinifex/network/invariants/): - S1 layers_test.go: parses `go list -json`; rejects upward layer imports and layers pulling from cross-cutter packages - S2 host_containment_test.go: AST-detects ovs-vsctl / ovn-nbctl / ovs-ofctl shell-out outside network/host/ - S3 nat_mode_test.go: rejects .UplinkMode() / NATModeFromUplinkMode() calls outside the init-time resolver locations - S4 names_test.go: AST-detects OVN object name construction (concat, fmt.Sprintf, strings.Join) outside topology/names.go - S5 policy_no_create_test.go: rejects CreateLogical{Switch,Router,...} and DeleteLogical{...} calls inside network/policy/ - S6 federation_isolation_test.go: rejects TransitSwitch / TransitRouterPort references and ovn-ic literals outside network/federation/ (pre-protects the layer before it lands) - S8 ipsec_test.go: rejects strongswan / xfrm / IKEv2 / libreswan / openswan imports anywhere in network/ - S9 reconciler_singleton_test.go: rejects Reconcile…Loop and Reconcile…Pass function names anywhere in network/ - S7 nats_subjects_test.go: scans for `vpc.` subjects missing the {azID} segment; calls t.Skip with the gap report so every test run prints the work-to-do without breaking CI. Flip to t.Fatalf when the cluster-wide subject rename ships. Behavioural suite (spinifex/network/reconcile/liveness_test.go): - L1 reconciler converges: scaled-intent (N=1,3,8) double-Reconcile property test; asserts second pass is a no-op - L2 drift bound: structural check that DriftInterval is positive and ≤30 min support window; full fake-clock behavioural test deferred - L3 federation re-enables: t.Skip stub pointing at the missing network/federation/ package Fixes the one production S4 violation the new walker found: network/external/igw.go was constructing "gw-port-"+vpcID inline. Adds topology.GatewaySwitchPort(vpcID) helper and retargets both AttachIGW and DetachIGW callsites through L2. Plan + per-clause mechanic + phasing in docs/development/improvements/network-invariants.md. Refs: mulga-siv-140 --- spinifex/network/external/igw.go | 4 +- spinifex/network/invariants/doc.go | 15 + .../invariants/federation_isolation_test.go | 166 ++++++++++ .../invariants/host_containment_test.go | 138 +++++++++ spinifex/network/invariants/ipsec_test.go | 93 ++++++ .../invariants/layers_classify_test.go | 27 ++ spinifex/network/invariants/layers_test.go | 293 ++++++++++++++++++ spinifex/network/invariants/names_test.go | 271 ++++++++++++++++ .../network/invariants/names_unit_test.go | 30 ++ spinifex/network/invariants/nat_mode_test.go | 160 ++++++++++ .../network/invariants/nats_subjects_test.go | 147 +++++++++ .../invariants/policy_no_create_test.go | 131 ++++++++ .../invariants/reconciler_singleton_test.go | 120 +++++++ spinifex/network/reconcile/liveness_test.go | 147 +++++++++ spinifex/network/topology/names.go | 4 + spinifex/network/topology/names_test.go | 1 + 16 files changed, 1745 insertions(+), 2 deletions(-) create mode 100644 spinifex/network/invariants/doc.go create mode 100644 spinifex/network/invariants/federation_isolation_test.go create mode 100644 spinifex/network/invariants/host_containment_test.go create mode 100644 spinifex/network/invariants/ipsec_test.go create mode 100644 spinifex/network/invariants/layers_classify_test.go create mode 100644 spinifex/network/invariants/layers_test.go create mode 100644 spinifex/network/invariants/names_test.go create mode 100644 spinifex/network/invariants/names_unit_test.go create mode 100644 spinifex/network/invariants/nat_mode_test.go create mode 100644 spinifex/network/invariants/nats_subjects_test.go create mode 100644 spinifex/network/invariants/policy_no_create_test.go create mode 100644 spinifex/network/invariants/reconciler_singleton_test.go create mode 100644 spinifex/network/reconcile/liveness_test.go diff --git a/spinifex/network/external/igw.go b/spinifex/network/external/igw.go index 6f9486ec8..e77aa50a4 100644 --- a/spinifex/network/external/igw.go +++ b/spinifex/network/external/igw.go @@ -115,7 +115,7 @@ func (m *igwManager) AttachIGW(ctx context.Context, spec IGWSpec) error { extSwitchName := topology.ExternalSwitch(spec.VPCID) extPortName := topology.ExternalLocalnetPort(spec.VPCID) gwPortName := topology.GatewayRouterPort(spec.VPCID) - switchGWPortName := "gw-port-" + spec.VPCID + switchGWPortName := topology.GatewaySwitchPort(spec.VPCID) routerName := topology.VPCRouter(spec.VPCID) if _, err := m.ovn.GetLogicalSwitch(ctx, extSwitchName); err == nil { @@ -235,7 +235,7 @@ func (m *igwManager) DetachIGW(ctx context.Context, vpcID string) error { extSwitchName := topology.ExternalSwitch(vpcID) extPortName := topology.ExternalLocalnetPort(vpcID) gwPortName := topology.GatewayRouterPort(vpcID) - switchGWPortName := "gw-port-" + vpcID + switchGWPortName := topology.GatewaySwitchPort(vpcID) routerName := topology.VPCRouter(vpcID) if err := m.routes.DeleteDefaultRoute(ctx, vpcID); err != nil { diff --git a/spinifex/network/invariants/doc.go b/spinifex/network/invariants/doc.go new file mode 100644 index 000000000..64ece91f3 --- /dev/null +++ b/spinifex/network/invariants/doc.go @@ -0,0 +1,15 @@ +// Package invariants holds executable tests that pin the spinifex network +// layer tree to the safety and liveness invariants declared in ADR-0006 +// (docs/development/proposals/az-local-architecture/0006-spinifex-network-layer-contract.md). +// +// Each test name encodes its governing ADR clause ID (TestS1_…, TestS4_…, +// TestL1_…). On failure the test quotes the ADR clause verbatim so the fix +// path leads back to the contract, not to the test author. +// +// The package contains no production code; it exists solely so `go test +// ./spinifex/network/invariants/...` runs the suite without polluting any +// layer's own package. +// +// See docs/development/improvements/network-invariants.md for the +// per-clause mechanic and phasing. +package invariants diff --git a/spinifex/network/invariants/federation_isolation_test.go b/spinifex/network/invariants/federation_isolation_test.go new file mode 100644 index 000000000..89f2a2b2a --- /dev/null +++ b/spinifex/network/invariants/federation_isolation_test.go @@ -0,0 +1,166 @@ +package invariants + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "sort" + "strings" + "testing" +) + +// TestS6_FederationIsolated enforces ADR-0006 clause S6. +// +// "L4 is the only layer aware of other AZs. No layer below L4 references +// remote AZ identifiers, OVN-IC transit switches, or inter-AZ link state. +// Cross-AZ federation is always mediated by L4." +// +// Mechanic: scan prod .go files under spinifex/network/ for symbols that +// only the federation layer is allowed to use: +// - identifiers: TransitSwitch, TransitRouterPort (the L2 name helpers +// are exempt — L2 owns names; L4 will call into them when it lands) +// - string literals containing "ovn-ic", "ovn-icnbctl", "ovn-icsbctl" +// +// Exempt locations: +// - network/topology/names.go (defines the names per ADR table) +// - network/federation/ (when it exists — currently empty; the fence +// pre-protects the package) +// - tests +func TestS6_FederationIsolated(t *testing.T) { + const clause = `ADR-0006 S6: "L4 is the only layer aware of other AZs. ` + + `No layer below L4 references remote AZ identifiers, OVN-IC transit ` + + `switches, or inter-AZ link state. Cross-AZ federation is always ` + + `mediated by L4."` + + forbiddenIdents := map[string]struct{}{ + "TransitSwitch": {}, + "TransitRouterPort": {}, + } + forbiddenLiterals := []string{ + "ovn-ic", + "ovn-icnbctl", + "ovn-icsbctl", + } + + root := filepath.Join(repoRoot(t), "spinifex", "network") + type hit struct { + file string + line int + sym string + } + var hits []hit + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if isVendoredOrGenerated(d.Name()) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + if strings.HasSuffix(path, "_test.go") { + return nil + } + if isS6Exempt(path) { + return nil + } + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + t.Fatalf("parse %s: %v", path, perr) + } + ast.Inspect(file, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.SelectorExpr: + if _, bad := forbiddenIdents[x.Sel.Name]; bad { + pos := fset.Position(x.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, x.Sel.Name}) + } + case *ast.Ident: + if _, bad := forbiddenIdents[x.Name]; bad { + pos := fset.Position(x.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, x.Name}) + } + case *ast.BasicLit: + if x.Kind != token.STRING { + return true + } + lit, ok := stringLit(x) + if !ok { + return true + } + for _, sub := range forbiddenLiterals { + if strings.Contains(lit, sub) { + pos := fset.Position(x.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, "\"" + lit + "\""}) + break + } + } + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walk: %v", err) + } + + if len(hits) == 0 { + return + } + + sort.Slice(hits, func(i, j int) bool { + if hits[i].file != hits[j].file { + return hits[i].file < hits[j].file + } + return hits[i].line < hits[j].line + }) + + rootForRel := repoRoot(t) + var b strings.Builder + b.WriteString(clause) + b.WriteString("\n") + limit := 5 + for i, h := range hits { + if i >= limit { + b.WriteString(" …\n") + break + } + b.WriteString(" ") + b.WriteString(relTo(h.file, rootForRel)) + b.WriteString(":") + b.WriteString(itoa(h.line)) + b.WriteString(": federation symbol ") + b.WriteString(h.sym) + b.WriteString(" outside L4\n") + } + b.WriteString(" Fix: route the inter-AZ operation through network/federation/. ") + b.WriteString("Only L4 may reference remote-AZ identifiers, OVN-IC objects, ") + b.WriteString("or inter-AZ link state.\n") + if len(hits) > limit { + b.WriteString(" ") + b.WriteString(itoa(len(hits) - limit)) + b.WriteString(" further violations suppressed.\n") + } + t.Fatalf("%s", b.String()) +} + +func isS6Exempt(path string) bool { + clean := filepath.ToSlash(path) + switch { + case strings.HasSuffix(clean, "/network/topology/names.go"): + return true + case strings.Contains(clean, "/network/federation/"): + return true + case strings.Contains(clean, "/network/invariants/"): + return true + } + return false +} diff --git a/spinifex/network/invariants/host_containment_test.go b/spinifex/network/invariants/host_containment_test.go new file mode 100644 index 000000000..c77989710 --- /dev/null +++ b/spinifex/network/invariants/host_containment_test.go @@ -0,0 +1,138 @@ +package invariants + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "sort" + "strings" + "testing" +) + +// TestS2_BridgeModeContainedInL0 enforces ADR-0006 clause S2. +// +// "Bridge mode is contained in L0. No layer above L0 reads uplink type, +// bridge mode, or physical NIC state directly. L0's UplinkMode() is the +// only resolver; all layers above receive a typed enum at init time." +// +// Mechanic: scan every prod .go file under spinifex/network/ for calls to +// host-level external tools (ovs-vsctl, ovn-nbctl, ovs-ofctl, ip) outside +// network/host/. These represent direct reads of host bridge / NIC state +// that S2 forbids above L0. Comments are ignored — only argument literals +// in exec.Command / sudoCommand / Run-style calls are checked. +// +// Scope: network/ only. vpcd/ is the entrypoint and lives outside the +// layer model; its startup detection shell-outs are tracked separately. +func TestS2_BridgeModeContainedInL0(t *testing.T) { + const clause = `ADR-0006 S2: "Bridge mode is contained in L0. No layer ` + + `above L0 reads uplink type, bridge mode, or physical NIC state ` + + `directly. L0's UplinkMode() is the only resolver; all layers ` + + `above receive a typed enum at init time."` + + hostTools := map[string]struct{}{ + "ovs-vsctl": {}, + "ovs-ofctl": {}, + "ovn-nbctl": {}, + "ovn-sbctl": {}, + } + + root := filepath.Join(repoRoot(t), "spinifex", "network") + type hit struct { + file string + line int + tool string + } + var hits []hit + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if isVendoredOrGenerated(d.Name()) { + return filepath.SkipDir + } + // network/host/ is the only layer that may shell out to + // host tools. Skip it entirely. + if strings.HasSuffix(filepath.ToSlash(path), "/network/host") { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + if strings.HasSuffix(path, "_test.go") { + return nil + } + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + t.Fatalf("parse %s: %v", path, perr) + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if len(call.Args) < 1 { + return true + } + lit, ok := stringLit(call.Args[0]) + if !ok { + return true + } + if _, banned := hostTools[lit]; !banned { + return true + } + pos := fset.Position(call.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, lit}) + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walk: %v", err) + } + + if len(hits) == 0 { + return + } + + sort.Slice(hits, func(i, j int) bool { + if hits[i].file != hits[j].file { + return hits[i].file < hits[j].file + } + return hits[i].line < hits[j].line + }) + + rootForRel := repoRoot(t) + var b strings.Builder + b.WriteString(clause) + b.WriteString("\n") + limit := 5 + for i, h := range hits { + if i >= limit { + b.WriteString(" …\n") + break + } + b.WriteString(" ") + b.WriteString(relTo(h.file, rootForRel)) + b.WriteString(":") + b.WriteString(itoa(h.line)) + b.WriteString(": invokes ") + b.WriteString(h.tool) + b.WriteString(" outside network/host/\n") + } + b.WriteString(" Fix: move the bridge/NIC operation into network/host/ ") + b.WriteString("and expose a typed method on host.Wiring. Layers above ") + b.WriteString("L0 may not shell out to host tools directly.\n") + if len(hits) > limit { + b.WriteString(" ") + b.WriteString(itoa(len(hits) - limit)) + b.WriteString(" further violations suppressed.\n") + } + t.Fatalf("%s", b.String()) +} diff --git a/spinifex/network/invariants/ipsec_test.go b/spinifex/network/invariants/ipsec_test.go new file mode 100644 index 000000000..8af7de227 --- /dev/null +++ b/spinifex/network/invariants/ipsec_test.go @@ -0,0 +1,93 @@ +package invariants + +import ( + "encoding/json" + "os/exec" + "sort" + "strings" + "testing" +) + +// TestS8_IPSecOVNNativeOnly enforces ADR-0006 clause S8. +// +// "IPSec is OVN-native only. No layer implements custom IKEv2, XFRM +// rules, or direct strongSwan management. IPSec SA lifecycle is +// delegated entirely to OVN native IPSec and is invisible above L0." +// +// Mechanic: read every package under spinifex/network/ via `go list -json` +// and reject imports that match a denylist of strongSwan, XFRM, IKEv2, +// or custom-IPSec libraries. The denylist is conservative; new entries +// are added as new IPSec libraries become available in Go. +func TestS8_IPSecOVNNativeOnly(t *testing.T) { + const clause = `ADR-0006 S8: "IPSec is OVN-native only. No layer ` + + `implements custom IKEv2, XFRM rules, or direct strongSwan ` + + `management. IPSec SA lifecycle is delegated entirely to OVN ` + + `native IPSec and is invisible above L0."` + + deniedSubstrings := []string{ + "strongswan", + "strongSwan", + "/xfrm", + "netlink/nl/xfrm", + "ikev2", + "IKEv2", + "libreswan", + "openswan", + } + + cmd := exec.Command("go", "list", "-json", "./...") + cmd.Dir = repoRoot(t) + out, err := cmd.Output() + if err != nil { + t.Fatalf("go list: %v", err) + } + + type hit struct { + pkg string + imp string + } + var hits []hit + dec := json.NewDecoder(strings.NewReader(string(out))) + for dec.More() { + var p goListPackage + if err := dec.Decode(&p); err != nil { + t.Fatalf("decode go list: %v", err) + } + if !strings.HasPrefix(p.ImportPath, networkRoot) { + continue + } + for _, imp := range p.Imports { + for _, sub := range deniedSubstrings { + if strings.Contains(imp, sub) { + hits = append(hits, hit{p.ImportPath, imp}) + } + } + } + } + + if len(hits) == 0 { + return + } + + sort.Slice(hits, func(i, j int) bool { + if hits[i].pkg != hits[j].pkg { + return hits[i].pkg < hits[j].pkg + } + return hits[i].imp < hits[j].imp + }) + + var b strings.Builder + b.WriteString(clause) + b.WriteString("\n") + for _, h := range hits { + b.WriteString(" ") + b.WriteString(shortPkg(h.pkg)) + b.WriteString(" imports ") + b.WriteString(h.imp) + b.WriteString("\n") + } + b.WriteString(" Fix: remove the import. IPSec SA lifecycle is handled ") + b.WriteString("by OVN native IPSec (strongSwan-as-OVN-internal); do not ") + b.WriteString("manage IKEv2 / XFRM / strongSwan directly from Go.\n") + t.Fatalf("%s", b.String()) +} diff --git a/spinifex/network/invariants/layers_classify_test.go b/spinifex/network/invariants/layers_classify_test.go new file mode 100644 index 000000000..dadf55f89 --- /dev/null +++ b/spinifex/network/invariants/layers_classify_test.go @@ -0,0 +1,27 @@ +package invariants + +import "testing" + +func TestS1_ClassifyTable(t *testing.T) { + cases := []struct { + name string + from, to packageKind + wantBad bool + }{ + {"L3 imports L0 (downward, permitted)", kindL3Policy, kindL0Host, false}, + {"L5 imports L1 (downward skip, permitted per ADR L5 description)", kindL5External, kindL1OVN, false}, + {"L0 imports L1 (upward, forbidden)", kindL0Host, kindL1OVN, true}, + {"L2 imports L3 (upward, forbidden)", kindL2Topology, kindL3Policy, true}, + {"reconcile imports L1 (orchestrator, permitted)", kindCrossCutter, kindL1OVN, false}, + {"L2 imports reconcile (layer pulling orchestrator, forbidden)", kindL2Topology, kindCrossCutter, true}, + {"L1 imports L1 (same layer, permitted)", kindL1OVN, kindL1OVN, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := classify(tc.from, tc.to) + if (got != "") != tc.wantBad { + t.Fatalf("classify(%v, %v) = %q; wantBad=%v", tc.from, tc.to, got, tc.wantBad) + } + }) + } +} diff --git a/spinifex/network/invariants/layers_test.go b/spinifex/network/invariants/layers_test.go new file mode 100644 index 000000000..66c1be291 --- /dev/null +++ b/spinifex/network/invariants/layers_test.go @@ -0,0 +1,293 @@ +package invariants + +import ( + "encoding/json" + "os/exec" + "sort" + "strings" + "testing" +) + +// TestS1_LayerSkipProhibited enforces ADR-0006 clause S1. +// +// "No code above layer Lk calls through to a layer below Lk's immediate +// interface. Every cross-layer call passes through the typed interface +// of the immediate lower neighbor." +// +// Mechanic: derive each network/* package's layer from layerOf(). Reject +// any import where the importer's layer is strictly lower than the +// importee's layer (i.e. an upward edge in the layer graph). Cross-cutters +// (reconcile, subscribers) may import any layer, but no layer may import +// them — that direction would re-introduce the tangling the redesign +// removed. +func TestS1_LayerSkipProhibited(t *testing.T) { + const clause = `ADR-0006 S1: "No code above layer Lk calls through to a ` + + `layer below Lk's immediate interface. Every cross-layer call ` + + `passes through the typed interface of the immediate lower neighbor."` + + pkgs := listNetworkPackages(t) + + type violation struct { + from string + fromKind packageKind + to string + toKind packageKind + reason string + } + var bad []violation + + for _, p := range pkgs { + fromKind := layerOf(p.ImportPath) + for _, imp := range p.Imports { + if !strings.HasPrefix(imp, networkRoot) { + continue + } + toKind := layerOf(imp) + reason := classify(fromKind, toKind) + if reason == "" { + continue + } + bad = append(bad, violation{ + from: p.ImportPath, fromKind: fromKind, + to: imp, toKind: toKind, + reason: reason, + }) + } + } + + if len(bad) == 0 { + return + } + + sort.Slice(bad, func(i, j int) bool { + if bad[i].from != bad[j].from { + return bad[i].from < bad[j].from + } + return bad[i].to < bad[j].to + }) + + var b strings.Builder + b.WriteString(clause) + b.WriteString("\n") + limit := 5 + for i, v := range bad { + if i >= limit { + b.WriteString(" …\n") + break + } + b.WriteString(" ") + b.WriteString(shortPkg(v.from)) + b.WriteString(" (") + b.WriteString(v.fromKind.String()) + b.WriteString(") -> ") + b.WriteString(shortPkg(v.to)) + b.WriteString(" (") + b.WriteString(v.toKind.String()) + b.WriteString("): ") + b.WriteString(v.reason) + b.WriteString("\n") + } + b.WriteString(" Fix: route the call through the immediate lower ") + b.WriteString("layer's typed interface, or move the offending code to ") + b.WriteString("an orchestrator (reconcile / subscribers).\n") + if len(bad) > limit { + b.WriteString(" ") + b.WriteString(itoa(len(bad) - limit)) + b.WriteString(" further violations suppressed.\n") + } + t.Fatalf("%s", b.String()) +} + +// packageKind tags a package by its position in the ADR-0006 layer model. +type packageKind int + +const ( + kindUnknown packageKind = iota + kindL0Host + kindL1OVN + kindL2Topology + kindL3Policy + kindL4Federation + kindL5External + kindCrossCutter // reconcile, subscribers — orchestrators, not layers + kindInvariants // this package +) + +func (k packageKind) String() string { + switch k { + case kindL0Host: + return "L0/host" + case kindL1OVN: + return "L1/ovn" + case kindL2Topology: + return "L2/topology" + case kindL3Policy: + return "L3/policy" + case kindL4Federation: + return "L4/federation" + case kindL5External: + return "L5/external" + case kindCrossCutter: + return "cross-cutter" + case kindInvariants: + return "invariants" + default: + return "unknown" + } +} + +// rank returns the ADR layer number for ordered layers. Non-layered +// packages return -1 and are handled separately in classify(). +func (k packageKind) rank() int { + switch k { + case kindL0Host: + return 0 + case kindL1OVN: + return 1 + case kindL2Topology: + return 2 + case kindL3Policy: + return 3 + case kindL4Federation: + return 4 + case kindL5External: + return 5 + } + return -1 +} + +const networkRoot = "github.com/mulgadc/spinifex/spinifex/network" + +func layerOf(importPath string) packageKind { + suffix := strings.TrimPrefix(importPath, networkRoot) + suffix = strings.TrimPrefix(suffix, "/") + switch { + case suffix == "host" || strings.HasPrefix(suffix, "host/"): + return kindL0Host + case suffix == "ovn" || strings.HasPrefix(suffix, "ovn/"): + return kindL1OVN + case suffix == "topology" || strings.HasPrefix(suffix, "topology/"): + return kindL2Topology + case suffix == "policy" || strings.HasPrefix(suffix, "policy/"): + return kindL3Policy + case suffix == "federation" || strings.HasPrefix(suffix, "federation/"): + return kindL4Federation + case suffix == "external" || strings.HasPrefix(suffix, "external/"): + return kindL5External + case suffix == "reconcile" || strings.HasPrefix(suffix, "reconcile/"): + return kindCrossCutter + case suffix == "subscribers" || strings.HasPrefix(suffix, "subscribers/"): + return kindCrossCutter + case suffix == "invariants" || strings.HasPrefix(suffix, "invariants/"): + return kindInvariants + } + return kindUnknown +} + +// classify returns a non-empty reason iff the edge from→to violates S1. +// Empty string means the edge is permitted. +func classify(from, to packageKind) string { + if from == to { + return "" + } + if from == kindInvariants || to == kindInvariants { + return "" + } + // No layer may import a cross-cutter — that would re-tangle the tree. + if to == kindCrossCutter && from != kindCrossCutter { + return "layer importing a cross-cutter (reconcile/subscribers)" + } + // Cross-cutters may import any layer. + if from == kindCrossCutter { + return "" + } + fr, tr := from.rank(), to.rank() + if fr < 0 || tr < 0 { + return "" + } + if tr > fr { + return "upward layer import (L" + itoa(fr) + " -> L" + itoa(tr) + ")" + } + return "" +} + +// goListPackage mirrors the subset of `go list -json` fields the test needs. +type goListPackage struct { + ImportPath string `json:"ImportPath"` + Imports []string `json:"Imports"` +} + +func listNetworkPackages(t *testing.T) []goListPackage { + t.Helper() + cmd := exec.Command("go", "list", "-json", "./...") + cmd.Dir = repoRoot(t) + out, err := cmd.Output() + if err != nil { + t.Fatalf("go list: %v", err) + } + var pkgs []goListPackage + dec := json.NewDecoder(strings.NewReader(string(out))) + for dec.More() { + var p goListPackage + if err := dec.Decode(&p); err != nil { + t.Fatalf("decode go list output: %v", err) + } + if !strings.HasPrefix(p.ImportPath, networkRoot) { + continue + } + // Skip the invariants package itself. + if layerOf(p.ImportPath) == kindInvariants { + continue + } + pkgs = append(pkgs, p) + } + if len(pkgs) == 0 { + t.Fatalf("go list returned no packages under %s", networkRoot) + } + return pkgs +} + +// repoRoot walks up from the test binary's working directory to find the +// spinifex go.mod, so `go list` runs in the right module regardless of +// where the test runner is invoked from. +func repoRoot(t *testing.T) string { + t.Helper() + cmd := exec.Command("go", "env", "GOMOD") + out, err := cmd.Output() + if err != nil { + t.Fatalf("go env GOMOD: %v", err) + } + gomod := strings.TrimSpace(string(out)) + if gomod == "" || gomod == "/dev/null" { + t.Fatalf("not in a go module (GOMOD=%q)", gomod) + } + return strings.TrimSuffix(gomod, "/go.mod") +} + +func shortPkg(p string) string { + return strings.TrimPrefix(p, networkRoot+"/") +} + +// itoa avoids pulling strconv into the failure-format hot path; pure +// stdlib readability win, no allocation concern. +func itoa(n int) string { + if n == 0 { + return "0" + } + neg := n < 0 + if neg { + n = -n + } + var buf [20]byte + i := len(buf) + for n > 0 { + i-- + buf[i] = byte('0' + n%10) + n /= 10 + } + if neg { + i-- + buf[i] = '-' + } + return string(buf[i:]) +} diff --git a/spinifex/network/invariants/names_test.go b/spinifex/network/invariants/names_test.go new file mode 100644 index 000000000..2c3f6ea5f --- /dev/null +++ b/spinifex/network/invariants/names_test.go @@ -0,0 +1,271 @@ +package invariants + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" +) + +// TestS4_OVNNamesL2Owned enforces ADR-0006 clause S4. +// +// "All OVN NB DB object names are constructed exclusively by L2 using +// the naming contract table. No layer above or below L2 constructs OVN +// object names independently." +// +// Mechanic: AST walk of every .go file under spinifex/network/ and +// spinifex/vpcd/. Flag any string literal that starts with an OVN-name +// prefix from the ADR contract table and is composed with an identifier +// (concatenation, fmt.Sprintf format string, or strings.Join element). +// The only legal residence for these literals is topology/names.go (the +// contract) and its test. +// +// Scope is intentionally limited to network/ + vpcd/. The AWS handler tree +// uses overlapping prefixes (e.g. `vpc-cidr-assoc-`) that are AWS resource +// IDs, not OVN object names, and are out of S4's scope. +func TestS4_OVNNamesL2Owned(t *testing.T) { + const clause = `ADR-0006 S4: "All OVN NB DB object names are constructed ` + + `exclusively by L2 using the naming contract table. No layer above ` + + `or below L2 constructs OVN object names independently."` + + roots := s4ScanRoots(t) + type hit struct { + file string + line int + col int + lit string + via string // "concat", "sprintf", "join" + } + var hits []hit + + for _, root := range roots { + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if isVendoredOrGenerated(d.Name()) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + if strings.HasSuffix(path, "_test.go") { + // Tests legitimately construct synthetic OVN-name-shaped + // strings as fixtures. The fence is for production code. + return nil + } + if isS4Exempt(path) { + return nil + } + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + t.Fatalf("parse %s: %v", path, perr) + } + ast.Inspect(file, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.BinaryExpr: + if x.Op != token.ADD { + return true + } + lit, ok := stringLit(x.X) + if !ok { + return true + } + if !hasOVNPrefix(lit) { + return true + } + pos := fset.Position(x.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, pos.Column, lit, "concat"}) + case *ast.CallExpr: + name := calleeName(x.Fun) + switch name { + case "fmt.Sprintf": + if len(x.Args) < 1 { + return true + } + lit, ok := stringLit(x.Args[0]) + if !ok { + return true + } + if !hasOVNPrefix(lit) { + return true + } + pos := fset.Position(x.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, pos.Column, lit, "sprintf"}) + case "strings.Join": + if len(x.Args) < 1 { + return true + } + comp, ok := x.Args[0].(*ast.CompositeLit) + if !ok { + return true + } + for _, e := range comp.Elts { + lit, ok := stringLit(e) + if !ok { + continue + } + if !hasOVNPrefix(lit) { + continue + } + pos := fset.Position(e.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, pos.Column, lit, "join"}) + } + } + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + } + + if len(hits) == 0 { + return + } + + sort.Slice(hits, func(i, j int) bool { + if hits[i].file != hits[j].file { + return hits[i].file < hits[j].file + } + return hits[i].line < hits[j].line + }) + + rootForRel := repoRoot(t) + var b strings.Builder + b.WriteString(clause) + b.WriteString("\n") + limit := 5 + for i, h := range hits { + if i >= limit { + b.WriteString(" …\n") + break + } + b.WriteString(" ") + b.WriteString(relTo(h.file, rootForRel)) + b.WriteString(":") + b.WriteString(itoa(h.line)) + b.WriteString(":") + b.WriteString(itoa(h.col)) + b.WriteString(": ") + b.WriteString(h.via) + b.WriteString(" of ") + b.WriteString(strconv.Quote(h.lit)) + b.WriteString("\n") + } + b.WriteString(" Fix: add a helper to spinifex/network/topology/names.go ") + b.WriteString("and call it from the offending site. Do not construct OVN ") + b.WriteString("object names outside L2.\n") + if len(hits) > limit { + b.WriteString(" ") + b.WriteString(itoa(len(hits) - limit)) + b.WriteString(" further violations suppressed.\n") + } + t.Fatalf("%s", b.String()) +} + +// ovnNamePrefixes is the ADR-0006 contract table, plus the in-tree +// transitional names (rtr-, rtr-port-, ext-port-) the §7.3 migration has +// not yet renamed. Keep in lockstep with topology/names.go. +var ovnNamePrefixes = []string{ + "vpc-", + "subnet-", + "port-", + "rtr-", + "rtr-port-", + "gw-", + "gw-port-", + "ext-", + "ext-port-", + "ts-", + "trp-", +} + +func hasOVNPrefix(s string) bool { + for _, p := range ovnNamePrefixes { + if len(s) > len(p) && strings.HasPrefix(s, p) { + return true + } + } + return false +} + +func stringLit(e ast.Expr) (string, bool) { + bl, ok := e.(*ast.BasicLit) + if !ok || bl.Kind != token.STRING { + return "", false + } + v, err := strconv.Unquote(bl.Value) + if err != nil { + return "", false + } + return v, true +} + +func calleeName(fun ast.Expr) string { + switch f := fun.(type) { + case *ast.SelectorExpr: + if id, ok := f.X.(*ast.Ident); ok { + return id.Name + "." + f.Sel.Name + } + case *ast.Ident: + return f.Name + } + return "" +} + +// isS4Exempt returns true for files that legitimately contain OVN-name +// prefix literals: the L2 names contract itself and the invariants suite +// (which references prefixes for testing). +func isS4Exempt(path string) bool { + clean := filepath.ToSlash(path) + switch { + case strings.HasSuffix(clean, "/network/topology/names.go"): + return true + case strings.HasSuffix(clean, "/network/topology/names_test.go"): + return true + case strings.Contains(clean, "/network/invariants/"): + return true + } + return false +} + +func isVendoredOrGenerated(dir string) bool { + switch dir { + case "vendor", "testdata", "node_modules", ".git": + return true + } + return false +} + +// s4ScanRoots returns the directories S4 enforces over: the network layer +// tree and vpcd. Anything else (handlers, daemon, services) is out of +// scope — those packages legitimately deal in AWS resource ID prefixes +// like `vpc-cidr-assoc-` that share leading bytes with OVN object names. +func s4ScanRoots(t *testing.T) []string { + t.Helper() + root := repoRoot(t) + return []string{ + filepath.Join(root, "spinifex", "network"), + filepath.Join(root, "spinifex", "vpcd"), + } +} + +func relTo(p, root string) string { + r, err := filepath.Rel(root, p) + if err != nil { + return p + } + return filepath.ToSlash(r) +} diff --git a/spinifex/network/invariants/names_unit_test.go b/spinifex/network/invariants/names_unit_test.go new file mode 100644 index 000000000..3373693a1 --- /dev/null +++ b/spinifex/network/invariants/names_unit_test.go @@ -0,0 +1,30 @@ +package invariants + +import "testing" + +func TestS4_HasOVNPrefix(t *testing.T) { + cases := []struct { + in string + want bool + }{ + {"vpc-abc", true}, + {"subnet-1", true}, + {"gw-port-x", true}, + {"ts-az1-vpc1", true}, + {"vpc-", false}, // bare prefix, no ID + {"vpc", false}, // not prefixed + {"hello-vpc-x", false}, + {"port-to-br", true}, // matches "port-" — detected by string-prefix + // note: port-to-br is an ovs-vsctl subcommand, not an OVN object + // name. The S4 walker exempts network/host/ ovs-vsctl callsites + // indirectly because they aren't in ` + ` or + // fmt.Sprintf format-string position. This test confirms the + // prefix matcher is intentionally broad; precision comes from + // the AST context, not the matcher. + } + for _, tc := range cases { + if got := hasOVNPrefix(tc.in); got != tc.want { + t.Errorf("hasOVNPrefix(%q) = %v; want %v", tc.in, got, tc.want) + } + } +} diff --git a/spinifex/network/invariants/nat_mode_test.go b/spinifex/network/invariants/nat_mode_test.go new file mode 100644 index 000000000..7d1e4b869 --- /dev/null +++ b/spinifex/network/invariants/nat_mode_test.go @@ -0,0 +1,160 @@ +package invariants + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "sort" + "strings" + "testing" +) + +// TestS3_NATModeInitTimeConstant enforces ADR-0006 clause S3. +// +// "NAT distribution mode is an init-time constant. Centralised-vs- +// distributed NAT is determined once at startup from L0's UplinkMode() +// and never re-evaluated at runtime. No layer above L0 branches on NAT +// mode dynamically." +// +// Mechanic: forbid calls to `.UplinkMode()` and `NATModeFromUplinkMode(...)` +// outside the legitimate resolver locations: +// - network/host/ (defines UplinkMode()) +// - network/policy/policy.go (defines NATModeFromUplinkMode) +// - vpcd/vpcd.go (single entrypoint resolves once at startup) +// - any *_test.go (tests legitimately exercise the resolver) +// +// Any additional callsite would be a re-resolution and a S3 violation. +func TestS3_NATModeInitTimeConstant(t *testing.T) { + const clause = `ADR-0006 S3: "NAT distribution mode is an init-time ` + + `constant. Centralised-vs-distributed NAT is determined once at ` + + `startup from L0's UplinkMode() and never re-evaluated at runtime. ` + + `No layer above L0 branches on NAT mode dynamically."` + + type hit struct { + file string + line int + sym string + } + var hits []hit + + roots := []string{ + filepath.Join(repoRoot(t), "spinifex", "network"), + filepath.Join(repoRoot(t), "spinifex", "vpcd"), + } + for _, root := range roots { + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if isVendoredOrGenerated(d.Name()) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + if strings.HasSuffix(path, "_test.go") { + return nil + } + if isS3Exempt(path) { + return nil + } + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + t.Fatalf("parse %s: %v", path, perr) + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + name := calleeName(call.Fun) + selName := selectorTail(call.Fun) + switch { + case selName == "UplinkMode": + pos := fset.Position(call.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, selName + "()"}) + case strings.HasSuffix(name, "NATModeFromUplinkMode"): + pos := fset.Position(call.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, "NATModeFromUplinkMode"}) + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + } + + if len(hits) == 0 { + return + } + + sort.Slice(hits, func(i, j int) bool { + if hits[i].file != hits[j].file { + return hits[i].file < hits[j].file + } + return hits[i].line < hits[j].line + }) + + rootForRel := repoRoot(t) + var b strings.Builder + b.WriteString(clause) + b.WriteString("\n") + limit := 5 + for i, h := range hits { + if i >= limit { + b.WriteString(" …\n") + break + } + b.WriteString(" ") + b.WriteString(relTo(h.file, rootForRel)) + b.WriteString(":") + b.WriteString(itoa(h.line)) + b.WriteString(": ") + b.WriteString(h.sym) + b.WriteString(" called outside the init-time resolver location\n") + } + b.WriteString(" Fix: resolve NAT mode once at startup (vpcd entrypoint) ") + b.WriteString("and pass the typed enum to constructors. Store as an ") + b.WriteString("immutable field; do not re-resolve.\n") + if len(hits) > limit { + b.WriteString(" ") + b.WriteString(itoa(len(hits) - limit)) + b.WriteString(" further violations suppressed.\n") + } + t.Fatalf("%s", b.String()) +} + +func isS3Exempt(path string) bool { + clean := filepath.ToSlash(path) + switch { + case strings.HasSuffix(clean, "/network/host"): + return true + case strings.Contains(clean, "/network/host/"): + return true + case strings.HasSuffix(clean, "/network/policy/policy.go"): + return true + case strings.HasSuffix(clean, "/vpcd/vpcd.go"): + return true + case strings.Contains(clean, "/network/invariants/"): + return true + } + return false +} + +// selectorTail returns the trailing selector name (`.Foo`) of a call +// expression, or "" if the call is not a selector. Used to detect method +// calls regardless of the receiver type. +func selectorTail(fun ast.Expr) string { + if sel, ok := fun.(*ast.SelectorExpr); ok { + return sel.Sel.Name + } + return "" +} diff --git a/spinifex/network/invariants/nats_subjects_test.go b/spinifex/network/invariants/nats_subjects_test.go new file mode 100644 index 000000000..12592cc42 --- /dev/null +++ b/spinifex/network/invariants/nats_subjects_test.go @@ -0,0 +1,147 @@ +package invariants + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" +) + +// TestS7_NATSSubjectsCarryAZPrefix enforces ADR-0006 clause S7. +// +// "Every vpcd-originated NATS publication uses a subject prefixed +// vpc.{azID}. A subscription never processes a message whose subject +// AZ prefix does not match the local node's AZ identifier." +// +// Status: aspirational. As of mulga-siv-125.* the subject scheme is still +// the legacy `vpc.` form (`vpc.create`, `vpc.delete`, …) with no +// `{azID}` segment. ADR-0006 §"AZ identity and NATS subject routing" +// describes the planned rename; until the cluster-wide cutover lands, +// renaming subjects would break rolling upgrades. +// +// To keep the gap visible without breaking CI, the test runs the audit and +// — when violations are found — calls t.Skip with the gap report. The +// `go test` summary prints the SKIP reason on every run so the work +// remains discoverable. +// +// Once the rename ships, change t.Skip to t.Fatalf and the invariant +// becomes a true fence. +func TestS7_NATSSubjectsCarryAZPrefix(t *testing.T) { + const clause = `ADR-0006 S7: "Every vpcd-originated NATS publication ` + + `uses a subject prefixed vpc.{azID}. A subscription never processes ` + + `a message whose subject AZ prefix does not match the local node's ` + + `AZ identifier."` + + // Subject literals beginning with `vpc.` followed immediately by a + // non-`{` token violate S7. The compliant form is `vpc.{azID}.` + // (or a template fragment containing `{azID}`). + compliant := regexp.MustCompile(`^vpc\.\{`) + subjectLike := regexp.MustCompile(`^vpc\.[a-zA-Z]`) + + roots := []string{ + filepath.Join(repoRoot(t), "spinifex", "network"), + filepath.Join(repoRoot(t), "spinifex", "vpcd"), + } + + type hit struct { + file string + line int + subject string + } + var hits []hit + + for _, root := range roots { + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if isVendoredOrGenerated(d.Name()) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + if strings.HasSuffix(path, "_test.go") { + return nil + } + if strings.Contains(filepath.ToSlash(path), "/network/invariants/") { + return nil + } + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + t.Fatalf("parse %s: %v", path, perr) + } + ast.Inspect(file, func(n ast.Node) bool { + bl, ok := n.(*ast.BasicLit) + if !ok || bl.Kind != token.STRING { + return true + } + lit, ok := stringLit(bl) + if !ok { + return true + } + if !subjectLike.MatchString(lit) { + return true + } + if compliant.MatchString(lit) { + return true + } + pos := fset.Position(bl.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, lit}) + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + } + + if len(hits) == 0 { + return + } + + sort.Slice(hits, func(i, j int) bool { + if hits[i].file != hits[j].file { + return hits[i].file < hits[j].file + } + return hits[i].line < hits[j].line + }) + + rootForRel := repoRoot(t) + var b strings.Builder + b.WriteString("S7 gap (aspirational): subject rename pending. ") + b.WriteString("Once the cluster-wide cutover lands, switch t.Skip to ") + b.WriteString("t.Fatalf in nats_subjects_test.go.\n") + b.WriteString(clause) + b.WriteString("\n") + limit := 5 + for i, h := range hits { + if i >= limit { + b.WriteString(" …\n") + break + } + b.WriteString(" ") + b.WriteString(relTo(h.file, rootForRel)) + b.WriteString(":") + b.WriteString(itoa(h.line)) + b.WriteString(": subject ") + b.WriteString(h.subject) + b.WriteString(" lacks {azID} segment\n") + } + if len(hits) > limit { + b.WriteString(" ") + b.WriteString(itoa(len(hits) - limit)) + b.WriteString(" further subjects suppressed.\n") + } + t.Skip(b.String()) +} diff --git a/spinifex/network/invariants/policy_no_create_test.go b/spinifex/network/invariants/policy_no_create_test.go new file mode 100644 index 000000000..b8d37f802 --- /dev/null +++ b/spinifex/network/invariants/policy_no_create_test.go @@ -0,0 +1,131 @@ +package invariants + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "sort" + "strings" + "testing" +) + +// TestS5_PolicyNeverCreatesOrDeletes enforces ADR-0006 clause S5. +// +// "L3 never creates or deletes logical objects. L3 only attaches policy +// (ACLs, NAT rules, static routes) to objects that L2 already owns. L3 +// never calls create or delete on logical switches, routers, or ports." +// +// Mechanic: AST walk prod .go files under spinifex/network/policy/. Flag +// any call whose method tail matches the forbidden set: +// +// Create{LogicalSwitch,LogicalRouter,LogicalSwitchPort,LogicalRouterPort} +// Delete{LogicalSwitch,LogicalRouter,LogicalSwitchPort,LogicalRouterPort} +// +// Test files are exempt — fixtures legitimately build temporary objects +// to exercise the policy under test. +func TestS5_PolicyNeverCreatesOrDeletes(t *testing.T) { + const clause = `ADR-0006 S5: "L3 never creates or deletes logical ` + + `objects. L3 only attaches policy (ACLs, NAT rules, static routes) ` + + `to objects that L2 already owns. L3 never calls create or delete ` + + `on logical switches, routers, or ports."` + + forbidden := map[string]struct{}{ + "CreateLogicalSwitch": {}, + "CreateLogicalRouter": {}, + "CreateLogicalSwitchPort": {}, + "CreateLogicalRouterPort": {}, + "DeleteLogicalSwitch": {}, + "DeleteLogicalRouter": {}, + "DeleteLogicalSwitchPort": {}, + "DeleteLogicalRouterPort": {}, + } + + root := filepath.Join(repoRoot(t), "spinifex", "network", "policy") + type hit struct { + file string + line int + sym string + } + var hits []hit + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if isVendoredOrGenerated(d.Name()) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + if strings.HasSuffix(path, "_test.go") { + return nil + } + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + t.Fatalf("parse %s: %v", path, perr) + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + tail := selectorTail(call.Fun) + if _, bad := forbidden[tail]; !bad { + return true + } + pos := fset.Position(call.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, tail}) + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walk: %v", err) + } + + if len(hits) == 0 { + return + } + + sort.Slice(hits, func(i, j int) bool { + if hits[i].file != hits[j].file { + return hits[i].file < hits[j].file + } + return hits[i].line < hits[j].line + }) + + rootForRel := repoRoot(t) + var b strings.Builder + b.WriteString(clause) + b.WriteString("\n") + limit := 5 + for i, h := range hits { + if i >= limit { + b.WriteString(" …\n") + break + } + b.WriteString(" ") + b.WriteString(relTo(h.file, rootForRel)) + b.WriteString(":") + b.WriteString(itoa(h.line)) + b.WriteString(": L3 calls ") + b.WriteString(h.sym) + b.WriteString("\n") + } + b.WriteString(" Fix: move the create/delete to L2 (topology) and have ") + b.WriteString("L3 attach policy only. L3 must never mutate logical ") + b.WriteString("switch/router/port lifecycle.\n") + if len(hits) > limit { + b.WriteString(" ") + b.WriteString(itoa(len(hits) - limit)) + b.WriteString(" further violations suppressed.\n") + } + t.Fatalf("%s", b.String()) +} diff --git a/spinifex/network/invariants/reconciler_singleton_test.go b/spinifex/network/invariants/reconciler_singleton_test.go new file mode 100644 index 000000000..04c934c29 --- /dev/null +++ b/spinifex/network/invariants/reconciler_singleton_test.go @@ -0,0 +1,120 @@ +package invariants + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" +) + +// TestS9_SingleReconcilerNoRetrofitPasses enforces ADR-0006 clause S9. +// +// "On each leader-gated startup, exactly one intent-actual reconciliation +// pass runs against the NATS KV snapshot and OVN NB DB actual state. No +// additional serial retrofit passes exist." +// +// Mechanic: scan exported funcs and methods under spinifex/network/ whose +// names match the retrofit-pass shape — `Reconcile…Loop` or +// `Reconcile…Pass`. The canonical single entrypoint is the +// reconciler in network/reconcile/. Anything matching the pattern outside +// that package (or any *Pass\d+ name regardless of location) is a regression +// toward the pre-redesign serial-pass startup the ADR explicitly retired. +// +// The periodic drift detector is a different name (`Drift…`) and is +// allowed; it runs the same reconciler entrypoint on a timer. +func TestS9_SingleReconcilerNoRetrofitPasses(t *testing.T) { + const clause = `ADR-0006 S9: "On each leader-gated startup, exactly one ` + + `intent-actual reconciliation pass runs against the NATS KV snapshot ` + + `and OVN NB DB actual state. No additional serial retrofit passes exist."` + + loopRE := regexp.MustCompile(`^Reconcile[A-Za-z]*Loop$`) + passRE := regexp.MustCompile(`^Reconcile[A-Za-z]*Pass\d+$`) + + root := filepath.Join(repoRoot(t), "spinifex", "network") + type hit struct { + file string + line int + sym string + why string + } + var hits []hit + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if isVendoredOrGenerated(d.Name()) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + if strings.HasSuffix(path, "_test.go") { + return nil + } + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + t.Fatalf("parse %s: %v", path, perr) + } + ast.Inspect(file, func(n ast.Node) bool { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Name == nil { + return true + } + name := fn.Name.Name + switch { + case loopRE.MatchString(name): + pos := fset.Position(fn.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, name, "Reconcile…Loop (retrofit pass)"}) + case passRE.MatchString(name): + pos := fset.Position(fn.Pos()) + hits = append(hits, hit{pos.Filename, pos.Line, name, "Reconcile…Pass (serial retrofit)"}) + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walk: %v", err) + } + + if len(hits) == 0 { + return + } + + sort.Slice(hits, func(i, j int) bool { + if hits[i].file != hits[j].file { + return hits[i].file < hits[j].file + } + return hits[i].line < hits[j].line + }) + + rootForRel := repoRoot(t) + var b strings.Builder + b.WriteString(clause) + b.WriteString("\n") + for _, h := range hits { + b.WriteString(" ") + b.WriteString(relTo(h.file, rootForRel)) + b.WriteString(":") + b.WriteString(itoa(h.line)) + b.WriteString(": ") + b.WriteString(h.sym) + b.WriteString(" — ") + b.WriteString(h.why) + b.WriteString("\n") + } + b.WriteString(" Fix: collapse this function into the single intent-actual ") + b.WriteString("reconciler in network/reconcile/. Periodic re-runs schedule ") + b.WriteString("the same entrypoint; they do not warrant a new function.\n") + t.Fatalf("%s", b.String()) +} diff --git a/spinifex/network/reconcile/liveness_test.go b/spinifex/network/reconcile/liveness_test.go new file mode 100644 index 000000000..582a137bc --- /dev/null +++ b/spinifex/network/reconcile/liveness_test.go @@ -0,0 +1,147 @@ +package reconcile + +import ( + "context" + "fmt" + "net" + "net/netip" + "testing" + "time" + + "github.com/mulgadc/spinifex/spinifex/network/external" + "github.com/mulgadc/spinifex/spinifex/network/policy" + "github.com/mulgadc/spinifex/spinifex/network/topology" +) + +// TestL1_ReconcilerConverges enforces ADR-0006 liveness clause L1. +// +// "While both the NATS KV and OVN NB DB are reachable, the reconciler +// eventually drives the delta to zero. Idempotent EnsureX methods make +// repeated runs safe." +// +// Mechanic: synthesise IntentStates of varying size (1..N VPCs, each with +// subnets, ports, SGs). For each: Reconcile, snapshot mock OVN state, +// Reconcile again, assert mock state unchanged across the second pass +// (delta = 0). Convergence in one extra pass demonstrates idempotency at +// every scale within the parameter range. +// +// TestReconcile_Idempotent already covers the N=1 case; this test exists +// to bind the ADR clause to a multi-size property check so future changes +// that break convergence at scale fail under the L1 clause name. +func TestL1_ReconcilerConverges(t *testing.T) { + for _, n := range []int{1, 3, 8} { + t.Run(fmt.Sprintf("N=%d", n), func(t *testing.T) { + rec, m := newTestReconciler(t) + ctx := context.Background() + intent := scaledIntent(t, n) + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile #1: %v", err) + } + routers1 := len(m.Routers) + switches1 := len(m.Switches) + ports1 := len(m.Ports) + pgs1 := len(m.PortGroups) + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile #2: %v", err) + } + if len(m.Routers) != routers1 { + t.Errorf("ADR-0006 L1: convergence violated — routers %d → %d on second pass", routers1, len(m.Routers)) + } + if len(m.Switches) != switches1 { + t.Errorf("ADR-0006 L1: convergence violated — switches %d → %d on second pass", switches1, len(m.Switches)) + } + if len(m.Ports) != ports1 { + t.Errorf("ADR-0006 L1: convergence violated — ports %d → %d on second pass", ports1, len(m.Ports)) + } + if len(m.PortGroups) != pgs1 { + t.Errorf("ADR-0006 L1: convergence violated — port groups %d → %d on second pass", pgs1, len(m.PortGroups)) + } + }) + } +} + +// TestL2_DriftIntervalBounded enforces a structural piece of ADR-0006 +// liveness clause L2. +// +// "While the leader is active, the periodic drift-detection loop +// eventually re-runs and re-applies any divergence introduced since +// the last reconciler run." +// +// Mechanic: assert that DriftInterval is a strictly positive duration and +// upper-bounded to a sensible support-window value. A misconfigured zero +// or negative interval would silently disable drift detection at runtime. +// +// A full behavioural test requires injecting a fake clock + fake NATS +// JetStream context into DriftLoop, which is beyond this suite's scope +// (the loop reads time.NewTicker and a real *nats.Conn). Marked TODO so +// the gap stays visible. +func TestL2_DriftIntervalBounded(t *testing.T) { + if DriftInterval <= 0 { + t.Fatalf("ADR-0006 L2: DriftInterval must be > 0; got %v", DriftInterval) + } + if DriftInterval > 30*time.Minute { + t.Fatalf("ADR-0006 L2: DriftInterval %v exceeds 30m support-window upper bound", DriftInterval) + } + // TODO: inject a fake clock + fake JetStream into DriftLoop and assert + // runDriftCycle is invoked at least twice within ctx.Deadline(). + // Blocked on a clock-injectable DriftLoop signature. +} + +// TestL3_FederationReEnablesOnLinkRecovery is a placeholder for ADR-0006 +// liveness clause L3. +// +// "Once an inter-AZ link reaches ≥ Degraded class (ADR-0003), L4 +// eventually calls BringUpLink and establishes the OVN-IC tunnel to +// the peer AZ." +// +// network/federation/ does not yet exist. The test skips with the ADR +// clause text so the gap appears in every test report. +func TestL3_FederationReEnablesOnLinkRecovery(t *testing.T) { + t.Skip(`ADR-0006 L3 deferred: network/federation/ has not yet been built. ` + + `Once it lands, replace this stub with a test that drives a fake ` + + `LinkObserver from Down → Degraded and asserts BringUpLink fires.`) +} + +// scaledIntent produces an IntentState with n VPCs, each holding one +// subnet, one ENI port, and one SG. Determinism over a fixed seed keeps +// the test reproducible while still varying topology size. +func scaledIntent(t *testing.T, n int) IntentState { + t.Helper() + intent := IntentState{ + VPCs: make(map[string]topology.VPCSpec, n), + Subnets: make(map[string]topology.SubnetSpec, n), + Ports: make(map[string]topology.PortSpec, n), + SGs: make(map[string]policy.SGSpec, n), + IGWs: map[string]external.IGWSpec{}, + EIPs: map[string]policy.EIPSpec{}, + NATGWs: map[string]policy.NATGWSpec{}, + } + for i := range n { + vpcID := fmt.Sprintf("vpc-l1-%d", i) + subnetID := fmt.Sprintf("subnet-l1-%d", i) + eniID := fmt.Sprintf("eni-l1-%d", i) + sgID := fmt.Sprintf("sg-l1-%d", i) + + base := byte(i) + mac, _ := net.ParseMAC(fmt.Sprintf("02:00:00:00:00:%02x", base+1)) + + intent.VPCs[vpcID] = topology.VPCSpec{ + VPCID: vpcID, + CIDR: netip.MustParsePrefix(fmt.Sprintf("10.%d.0.0/16", base)), + VNI: int64(100 + i), + } + intent.Subnets[subnetID] = topology.SubnetSpec{ + SubnetID: subnetID, VPCID: vpcID, + CIDR: netip.MustParsePrefix(fmt.Sprintf("10.%d.1.0/24", base)), + } + intent.Ports[eniID] = topology.PortSpec{ + PortID: eniID, SubnetID: subnetID, VPCID: vpcID, + PrivateIP: netip.MustParseAddr(fmt.Sprintf("10.%d.1.10", base)), + MAC: mac, SGIDs: []string{sgID}, + } + intent.SGs[sgID] = policy.SGSpec{GroupID: sgID, VPCID: vpcID} + } + return intent +} diff --git a/spinifex/network/topology/names.go b/spinifex/network/topology/names.go index 8c7731553..895d77f55 100644 --- a/spinifex/network/topology/names.go +++ b/spinifex/network/topology/names.go @@ -30,6 +30,10 @@ func Port(portID string) string { return "port-" + portID } // GatewayRouterPort is the OVN LRP name for the VPC's uplink/gateway port. func GatewayRouterPort(vpcID string) string { return "gw-" + vpcID } +// GatewaySwitchPort is the OVN LSP (type=router) on the external switch +// peered with GatewayRouterPort. +func GatewaySwitchPort(vpcID string) string { return "gw-port-" + vpcID } + // ExternalSwitch is the OVN logical switch name for the per-VPC external // switch bridging the gateway LRP to the localnet. func ExternalSwitch(vpcID string) string { return "ext-" + vpcID } diff --git a/spinifex/network/topology/names_test.go b/spinifex/network/topology/names_test.go index 746c83191..f6df0fedb 100644 --- a/spinifex/network/topology/names_test.go +++ b/spinifex/network/topology/names_test.go @@ -14,6 +14,7 @@ func TestNameHelpers(t *testing.T) { {"SubnetSwitchRouterPort", SubnetSwitchRouterPort("subnet-1"), "rtr-port-subnet-1"}, {"Port", Port("eni-1"), "port-eni-1"}, {"GatewayRouterPort", GatewayRouterPort("vpc-abc"), "gw-vpc-abc"}, + {"GatewaySwitchPort", GatewaySwitchPort("vpc-abc"), "gw-port-vpc-abc"}, {"ExternalSwitch", ExternalSwitch("vpc-abc"), "ext-vpc-abc"}, {"ExternalLocalnetPort", ExternalLocalnetPort("vpc-abc"), "ext-port-vpc-abc"}, {"SecurityGroupPortGroup", SecurityGroupPortGroup("sg-abc-123"), "sg_abc_123"},