fix: loud detectBridgeMode + verifyBridgeMode sanity check (mulga-998.b Fix 2)

Make bridge-mode detection loud and fail-start when runtime state does
not match config. Closes D4/D12/D18 of dhcp-vpcd-issues.md.

Changes:
- detectBridgeMode: slog.Info at each decision, slog.Warn on fall-through
  (was silent Debug — let the veth persistence bug hide for weeks).
- Extract resolveBridgeConfig so mode/bind-bridge resolution is unit
  testable. Empty mode now stays empty so verifyBridgeMode rejects it
  with an explicit list of supported values.
- Add verifyBridgeMode: refuses to start when external iface missing
  (direct), when external iface not on the OVS bridge (direct), when
  veth-wan-ovs missing or attached to the wrong OVS bridge, or when
  veth-wan-br missing or on the wrong Linux bridge. References
  OvnExternalBridge (= "br-ext") so the sanity check tracks the contract,
  not a hardcoded string.
- Wire verifyBridgeMode into launchService. Error is loud and includes
  operator guidance ("re-run scripts/setup-ovn.sh").
- Injectable probes (ifaceIsMacvlan, ifaceExists, portToBr,
  readLinkMaster) so tests can exercise every branch without touching
  real interfaces.
- Tests: 16 new unit tests covering detectBridgeMode, resolveBridgeConfig,
  and all verifyBridgeMode branches.

spinifex/services/vpcd/topology.go

@@ -84,6 +84,12 @@ const (
 	// OVS bridge (br-ext). Requires centralized NAT like macvlan because the
 	// Linux bridge intermediary breaks distributed NAT hairpin routing.
 	BridgeModeVeth = "veth"
+	// OvnExternalBridge is the OVS bridge that ovn-bridge-mappings targets
+	// for the "external" localnet. Owned by setup-ovn.sh's ovn-bridge-mappings
+	// setup and independent of dhcp_bind_bridge (which is Linux-side in veth
+	// mode). Named as a constant so the vpcd sanity check refers to the
+	// contract, not a hardcoded string (D18).
+	OvnExternalBridge = "br-ext"
 )
 
 // TopologyHandler translates VPC lifecycle NATS events into OVN NB DB operations.

spinifex/services/vpcd/topology_test.go

@@ -3,6 +3,7 @@ package vpcd
 import (
 	"context"
 	"encoding/json"
+	"strings"
 	"testing"
 
 	"github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb"
@@ -2404,3 +2405,103 @@ func TestEnsureLocalnetOptions_NoOpWhenAlreadyCorrect(t *testing.T) {
 			mock.UpdateLogicalSwitchPortCalls-startUpdates)
 	}
 }
+
+func TestEnsureLocalnetOptions_NoOpInDirectModeWhenAbsent(t *testing.T) {
+	mock := NewMockOVNClient()
+	_ = mock.Connect(context.Background())
+	ctx := context.Background()
+
+	_ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "ext-vpc-d"})
+	_ = mock.CreateLogicalSwitchPort(ctx, "ext-vpc-d", &nbdb.LogicalSwitchPort{
+		Name:      "ext-port-vpc-d",
+		Type:      "localnet",
+		Addresses: []string{"unknown"},
+		Options:   map[string]string{"network_name": "external"},
+	})
+	start := mock.UpdateLogicalSwitchPortCalls
+
+	topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeDirect))
+	if err := topo.ensureLocalnetOptions(ctx, "ext-port-vpc-d"); err != nil {
+		t.Fatalf("ensureLocalnetOptions: %v", err)
+	}
+	if mock.UpdateLogicalSwitchPortCalls != start {
+		t.Errorf("direct mode + no nat-addresses should be a no-op; got %d new calls",
+			mock.UpdateLogicalSwitchPortCalls-start)
+	}
+}
+
+func TestEnsureLocalnetOptions_HandlesNilOptionsMap(t *testing.T) {
+	mock := NewMockOVNClient()
+	_ = mock.Connect(context.Background())
+	ctx := context.Background()
+
+	_ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "ext-vpc-n"})
+	_ = mock.CreateLogicalSwitchPort(ctx, "ext-vpc-n", &nbdb.LogicalSwitchPort{
+		Name:      "ext-port-vpc-n",
+		Type:      "localnet",
+		Addresses: []string{"unknown"},
+	})
+
+	topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeVeth))
+	if err := topo.ensureLocalnetOptions(ctx, "ext-port-vpc-n"); err != nil {
+		t.Fatalf("ensureLocalnetOptions: %v", err)
+	}
+	port, _ := mock.GetLogicalSwitchPort(ctx, "ext-port-vpc-n")
+	if port.Options["nat-addresses"] != "router" {
+		t.Errorf("expected nat-addresses=router after nil-map init, got %q", port.Options["nat-addresses"])
+	}
+}
+
+func TestEnsureLocalnetOptions_ErrorOnMissingPort(t *testing.T) {
+	mock := NewMockOVNClient()
+	_ = mock.Connect(context.Background())
+	ctx := context.Background()
+
+	topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeVeth))
+	err := topo.ensureLocalnetOptions(ctx, "ext-port-missing")
+	if err == nil {
+		t.Fatal("expected error when port missing")
+	}
+	if !strings.Contains(err.Error(), "ext-port-missing") {
+		t.Errorf("error should name the port; got: %v", err)
+	}
+}
+
+// reconcileIGW must surface the CreateLogicalSwitchPort error and roll back
+// the external switch when the localnet port name collides (e.g. a stale
+// pre-existing port from a prior attach that leaked).
+func TestTopologyHandler_ReconcileIGW_LocalnetPortCreateError(t *testing.T) {
+	mock := NewMockOVNClient()
+	_ = mock.Connect(context.Background())
+	ctx := context.Background()
+
+	topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeVeth))
+
+	// Pre-create VPC router.
+	_ = mock.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{
+		Name:        "vpc-vpc-clash",
+		ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-clash", "spinifex:cidr": "10.0.0.0/16"},
+	})
+
+	// Pre-seed a logical switch port named "ext-port-vpc-clash" on an
+	// unrelated switch so reconcileIGW's CreateLogicalSwitchPort call
+	// collides with the existing port.
+	_ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "stale-switch"})
+	_ = mock.CreateLogicalSwitchPort(ctx, "stale-switch", &nbdb.LogicalSwitchPort{
+		Name: "ext-port-vpc-clash",
+		Type: "localnet",
+	})
+
+	err := topo.reconcileIGW(ctx, "vpc-clash", "igw-clash")
+	if err == nil {
+		t.Fatal("expected error from reconcileIGW when localnet port collides")
+	}
+	if !strings.Contains(err.Error(), "create localnet port") {
+		t.Errorf("expected 'create localnet port' in error; got: %v", err)
+	}
+
+	// The external switch must have been rolled back.
+	if _, err := mock.GetLogicalSwitch(ctx, "ext-vpc-clash"); err == nil {
+		t.Error("expected external switch to be rolled back after port-create failure")
+	}
+}

spinifex/services/vpcd/vpcd.go

@@ -296,20 +296,12 @@ func launchService(cfg *Config) error {
 	defer liveClient.Close()
 	slog.Info("Connected to OVN NB DB", "endpoint", cfg.OVNNBAddr)
 
-	// Detect bridge mode: if not explicitly configured, auto-detect by checking
-	// whether the WAN bridge has a macvlan port or a physical NIC.
-	bridgeMode := cfg.BridgeMode
-	if bridgeMode == "" && cfg.ExternalInterface != "" {
-		bridgeMode = detectBridgeMode(cfg.ExternalInterface)
-	}
-	if bridgeMode == "" {
-		bridgeMode = BridgeModeMacvlan // default for backward compatibility
-	}
-	dhcpBindBridge := cfg.DhcpBindBridge
-	if dhcpBindBridge == "" {
-		dhcpBindBridge = "br-wan"
-	}
+	bridgeMode, dhcpBindBridge := resolveBridgeConfig(cfg.BridgeMode, cfg.ExternalInterface, cfg.DhcpBindBridge)
 	slog.Info("External bridge mode", "mode", bridgeMode, "dhcp_bind_bridge", dhcpBindBridge)
+	if err := verifyBridgeMode(bridgeMode, cfg.ExternalInterface, dhcpBindBridge); err != nil {
+		slog.Error("vpcd: bridge mode sanity check failed", "err", err)
+		return err
+	}
 
 	// Reconcile OVN topology from bootstrap config before subscribing.
 	// This ensures the default VPC topology exists even if admin init ran
@@ -377,27 +369,138 @@ func launchService(cfg *Config) error {
 	return nil
 }
 
+// resolveBridgeConfig picks the bridge mode and DHCP-bind-bridge to use,
+// auto-detecting mode when unset. Empty mode stays empty — verifyBridgeMode
+// rejects it with a list of supported values (D12). Empty bind bridge
+// defaults to "br-wan", the consumer-router convention.
+func resolveBridgeConfig(cfgBridgeMode, externalIface, cfgDhcpBindBridge string) (string, string) {
+	bridgeMode := cfgBridgeMode
+	if bridgeMode == "" && externalIface != "" {
+		bridgeMode = detectBridgeMode(externalIface)
+	}
+	dhcpBindBridge := cfgDhcpBindBridge
+	if dhcpBindBridge == "" {
+		dhcpBindBridge = "br-wan"
+	}
+	return bridgeMode, dhcpBindBridge
+}
+
+// ifaceIsMacvlan returns true when the given interface exists and is a
+// macvlan sub-interface. Injected as a var so detectBridgeMode tests can stub
+// it without launching ip(8).
+var ifaceIsMacvlan = func(name string) bool {
+	out, err := exec.Command("ip", "-d", "link", "show", name).CombinedOutput()
+	return err == nil && strings.Contains(string(out), "macvlan")
+}
+
+// ifaceExists returns true when the kernel reports the named link.
+var ifaceExists = func(name string) bool {
+	return exec.Command("ip", "link", "show", name).Run() == nil
+}
+
 // detectBridgeMode checks how the WAN bridge is wired:
 //   - macvlan: spx-ext-{iface} macvlan sub-interface exists
 //   - veth: veth-wan-ovs interface exists (Linux bridge linked to OVS via veth pair)
 //   - direct: physical NIC is added directly to the OVS bridge
+//
+// Each decision point logs at Info so `journalctl -u spinifex-vpcd | grep
+// bridge` surfaces the full trail. The fall-through case logs at Warn — the
+// silent Debug fall-through is what let the veth-persistence bug hide for
+// weeks (mulga-998.b Fix 2).
 func detectBridgeMode(externalIface string) string {
 	macvlanName := "spx-ext-" + externalIface
-	out, err := exec.Command("ip", "-d", "link", "show", macvlanName).CombinedOutput()
-	if err == nil && strings.Contains(string(out), "macvlan") {
-		slog.Debug("vpcd: detected macvlan interface on WAN bridge", "iface", macvlanName)
+	if ifaceIsMacvlan(macvlanName) {
+		slog.Info("vpcd: detected macvlan interface on WAN bridge", "iface", macvlanName, "mode", BridgeModeMacvlan)
 		return BridgeModeMacvlan
 	}
-	// Check for veth pair linking a Linux bridge to OVS (setup-ovn.sh creates
-	// veth-wan-br ↔ veth-wan-ovs when the default route is on a Linux bridge).
-	if _, vethErr := exec.Command("ip", "link", "show", "veth-wan-ovs").CombinedOutput(); vethErr == nil {
-		slog.Debug("vpcd: detected veth pair linking Linux bridge to OVS")
+	if ifaceExists("veth-wan-ovs") {
+		slog.Info("vpcd: detected veth pair linking Linux bridge to OVS", "mode", BridgeModeVeth)
 		return BridgeModeVeth
 	}
-	slog.Debug("vpcd: no macvlan or veth found, assuming direct bridge mode", "checked", macvlanName)
+	slog.Warn("vpcd: no macvlan or veth interface found, assuming direct bridge mode",
+		"external_iface", externalIface, "checked_macvlan", macvlanName, "checked_veth", "veth-wan-ovs",
+		"mode", BridgeModeDirect)
 	return BridgeModeDirect
 }
 
+// portToBr returns the OVS bridge that owns `port`. Returns "" when the port
+// is not in OVSDB. Used by the post-detect sanity checks.
+var portToBr = func(port string) (string, error) {
+	out, err := sudoCommand("ovs-vsctl", "port-to-br", port).CombinedOutput()
+	if err != nil {
+		return "", fmt.Errorf("ovs-vsctl port-to-br %s: %s: %w", port, strings.TrimSpace(string(out)), err)
+	}
+	return strings.TrimSpace(string(out)), nil
+}
+
+// readLinkMaster returns the master of a kernel link by reading
+// /sys/class/net/<iface>/master. Returns "" if the link has no master.
+var readLinkMaster = func(iface string) (string, error) {
+	target, err := os.Readlink(filepath.Join("/sys/class/net", iface, "master"))
+	if err != nil {
+		return "", err
+	}
+	return filepath.Base(target), nil
+}
+
+// verifyBridgeMode is the post-detect sanity check. It refuses to start vpcd
+// when the chosen bridge mode does not match the host plumbing (D4+D18):
+//
+//   - direct: ExternalInterface must be an OVS port on DhcpBindBridge. That is
+//     the whole contract of direct mode.
+//   - veth: (a) veth-wan-ovs must be an OVS port on OvnExternalBridge — the
+//     OVN side, owned by setup-ovn.sh's ovn-bridge-mappings. (b) veth-wan-br
+//     must be enslaved to DhcpBindBridge — the Linux side, where the DHCP
+//     client sees LAN frames.
+//   - empty / unknown: fail with the list of supported values.
+//
+// Fail-start, not soft-degrade — the distributed-NAT-on-veth-host footgun is
+// exactly what this plan set out to kill.
+func verifyBridgeMode(mode, externalIface, dhcpBindBridge string) error {
+	switch mode {
+	case BridgeModeDirect:
+		if externalIface == "" {
+			return fmt.Errorf("vpcd: direct bridge mode requires external_interface (the WAN NIC name)")
+		}
+		if dhcpBindBridge == "" {
+			return fmt.Errorf("vpcd: direct bridge mode requires dhcp_bind_bridge (the OVS bridge holding the WAN NIC)")
+		}
+		br, err := portToBr(externalIface)
+		if err != nil {
+			return fmt.Errorf("vpcd: direct bridge mode: %w", err)
+		}
+		if br != dhcpBindBridge {
+			return fmt.Errorf("vpcd: direct bridge mode: %q is on OVS bridge %q, expected %q (dhcp_bind_bridge)",
+				externalIface, br, dhcpBindBridge)
+		}
+		return nil
+	case BridgeModeVeth:
+		if dhcpBindBridge == "" {
+			return fmt.Errorf("vpcd: veth bridge mode requires dhcp_bind_bridge (the Linux bridge holding the WAN NIC)")
+		}
+		br, err := portToBr("veth-wan-ovs")
+		if err != nil {
+			return fmt.Errorf("vpcd: veth bridge mode: veth-wan-ovs not on OVS — is setup-ovn.sh's veth branch installed and systemd-networkd up? %w", err)
+		}
+		if br != OvnExternalBridge {
+			return fmt.Errorf("vpcd: veth bridge mode: veth-wan-ovs is on OVS bridge %q, expected %q",
+				br, OvnExternalBridge)
+		}
+		master, err := readLinkMaster("veth-wan-br")
+		if err != nil {
+			return fmt.Errorf("vpcd: veth bridge mode: veth-wan-br missing or has no master — systemd-networkd drop-in not applied? %w", err)
+		}
+		if master != dhcpBindBridge {
+			return fmt.Errorf("vpcd: veth bridge mode: veth-wan-br master is %q, expected %q (dhcp_bind_bridge)",
+				master, dhcpBindBridge)
+		}
+		return nil
+	default:
+		return fmt.Errorf("vpcd: unknown bridge_mode %q — supported values: %q, %q",
+			mode, BridgeModeDirect, BridgeModeVeth)
+	}
+}
+
 // setMacvlanMAC sets the MAC address on a macvlan interface. The interface is
 // brought down, MAC changed, and brought back up. Requires sudo/NET_ADMIN.
 func setMacvlanMAC(iface, mac string) error {

spinifex/services/vpcd/vpcd_test.go

@@ -219,3 +219,180 @@ func TestPreflightOVN_BothFail_ReportsFirst(t *testing.T) {
 		t.Errorf("expected first error to mention br-int, got: %v", err)
 	}
 }
+
+// verifyBridgeMode is the post-detect sanity check — mulga-998.b Fix 2.
+// portToBr and readLinkMaster are injected for tests.
+
+func stubBridgeProbes(t *testing.T, ovsPorts map[string]string, links map[string]string) {
+	t.Helper()
+	origPort := portToBr
+	origLink := readLinkMaster
+	t.Cleanup(func() {
+		portToBr = origPort
+		readLinkMaster = origLink
+	})
+	portToBr = func(port string) (string, error) {
+		br, ok := ovsPorts[port]
+		if !ok {
+			return "", fmt.Errorf("no port named %q", port)
+		}
+		return br, nil
+	}
+	readLinkMaster = func(iface string) (string, error) {
+		m, ok := links[iface]
+		if !ok {
+			return "", fmt.Errorf("no link named %q", iface)
+		}
+		return m, nil
+	}
+}
+
+func TestVerifyBridgeMode_DirectOK(t *testing.T) {
+	stubBridgeProbes(t, map[string]string{"enp0s3": "br-wan"}, nil)
+	if err := verifyBridgeMode(BridgeModeDirect, "enp0s3", "br-wan"); err != nil {
+		t.Fatalf("unexpected: %v", err)
+	}
+}
+
+func TestVerifyBridgeMode_DirectMismatch(t *testing.T) {
+	stubBridgeProbes(t, map[string]string{"enp0s3": "br-wan"}, nil)
+	err := verifyBridgeMode(BridgeModeDirect, "enp0s3", "br-ext")
+	if err == nil || !strings.Contains(err.Error(), "br-ext") {
+		t.Fatalf("expected mismatch error, got: %v", err)
+	}
+}
+
+func TestVerifyBridgeMode_DirectMissingNIC(t *testing.T) {
+	stubBridgeProbes(t, map[string]string{}, nil)
+	err := verifyBridgeMode(BridgeModeDirect, "enp0s3", "br-wan")
+	if err == nil {
+		t.Fatal("expected error when WAN NIC not in OVSDB")
+	}
+}
+
+func TestVerifyBridgeMode_VethOK(t *testing.T) {
+	stubBridgeProbes(t,
+		map[string]string{"veth-wan-ovs": OvnExternalBridge},
+		map[string]string{"veth-wan-br": "br-wan"})
+	if err := verifyBridgeMode(BridgeModeVeth, "", "br-wan"); err != nil {
+		t.Fatalf("unexpected: %v", err)
+	}
+}
+
+func TestVerifyBridgeMode_VethMissingOvsPort(t *testing.T) {
+	stubBridgeProbes(t, map[string]string{}, map[string]string{"veth-wan-br": "br-wan"})
+	err := verifyBridgeMode(BridgeModeVeth, "", "br-wan")
+	if err == nil || !strings.Contains(err.Error(), "veth-wan-ovs") {
+		t.Fatalf("expected veth-wan-ovs error, got: %v", err)
+	}
+}
+
+func TestVerifyBridgeMode_VethWrongOvsBridge(t *testing.T) {
+	stubBridgeProbes(t,
+		map[string]string{"veth-wan-ovs": "br-wan"},
+		map[string]string{"veth-wan-br": "br-wan"})
+	err := verifyBridgeMode(BridgeModeVeth, "", "br-wan")
+	if err == nil || !strings.Contains(err.Error(), OvnExternalBridge) {
+		t.Fatalf("expected br-ext mismatch, got: %v", err)
+	}
+}
+
+func TestVerifyBridgeMode_VethWrongLinuxMaster(t *testing.T) {
+	stubBridgeProbes(t,
+		map[string]string{"veth-wan-ovs": OvnExternalBridge},
+		map[string]string{"veth-wan-br": "br-other"})
+	err := verifyBridgeMode(BridgeModeVeth, "", "br-wan")
+	if err == nil || !strings.Contains(err.Error(), "br-other") {
+		t.Fatalf("expected master mismatch, got: %v", err)
+	}
+}
+
+func TestVerifyBridgeMode_UnknownModeLists(t *testing.T) {
+	err := verifyBridgeMode("macvlan", "enp0s3", "br-wan")
+	if err == nil {
+		t.Fatal("expected error for unsupported mode")
+	}
+	if !strings.Contains(err.Error(), BridgeModeDirect) || !strings.Contains(err.Error(), BridgeModeVeth) {
+		t.Errorf("expected error to list supported values, got: %v", err)
+	}
+}
+
+func TestVerifyBridgeMode_EmptyModeRejected(t *testing.T) {
+	err := verifyBridgeMode("", "", "")
+	if err == nil {
+		t.Fatal("expected error for empty mode (D12)")
+	}
+}
+
+// detectBridgeMode — mulga-998.b Fix 2.
+
+func stubDetectProbes(t *testing.T, macvlans []string, links []string) {
+	t.Helper()
+	origMac := ifaceIsMacvlan
+	origExists := ifaceExists
+	t.Cleanup(func() {
+		ifaceIsMacvlan = origMac
+		ifaceExists = origExists
+	})
+	macSet := map[string]bool{}
+	for _, m := range macvlans {
+		macSet[m] = true
+	}
+	linkSet := map[string]bool{}
+	for _, l := range links {
+		linkSet[l] = true
+	}
+	ifaceIsMacvlan = func(name string) bool { return macSet[name] }
+	ifaceExists = func(name string) bool { return linkSet[name] }
+}
+
+func TestDetectBridgeMode_MacvlanWins(t *testing.T) {
+	stubDetectProbes(t, []string{"spx-ext-enp0s3"}, []string{"veth-wan-ovs"})
+	if got := detectBridgeMode("enp0s3"); got != BridgeModeMacvlan {
+		t.Errorf("want %q, got %q", BridgeModeMacvlan, got)
+	}
+}
+
+func TestDetectBridgeMode_VethWhenNoMacvlan(t *testing.T) {
+	stubDetectProbes(t, nil, []string{"veth-wan-ovs"})
+	if got := detectBridgeMode("enp0s3"); got != BridgeModeVeth {
+		t.Errorf("want %q, got %q", BridgeModeVeth, got)
+	}
+}
+
+func TestDetectBridgeMode_FallthroughDirect(t *testing.T) {
+	stubDetectProbes(t, nil, nil)
+	if got := detectBridgeMode("enp0s3"); got != BridgeModeDirect {
+		t.Errorf("want %q, got %q", BridgeModeDirect, got)
+	}
+}
+
+func TestResolveBridgeConfig_UsesExplicitMode(t *testing.T) {
+	stubDetectProbes(t, nil, nil)
+	mode, br := resolveBridgeConfig(BridgeModeVeth, "enp0s3", "br-wan")
+	if mode != BridgeModeVeth || br != "br-wan" {
+		t.Errorf("got (%q,%q), want (%q,br-wan)", mode, br, BridgeModeVeth)
+	}
+}
+
+func TestResolveBridgeConfig_AutoDetects(t *testing.T) {
+	stubDetectProbes(t, nil, []string{"veth-wan-ovs"})
+	mode, _ := resolveBridgeConfig("", "enp0s3", "br-wan")
+	if mode != BridgeModeVeth {
+		t.Errorf("want auto-detect veth, got %q", mode)
+	}
+}
+
+func TestResolveBridgeConfig_EmptyStaysEmptyWithNoIface(t *testing.T) {
+	mode, _ := resolveBridgeConfig("", "", "")
+	if mode != "" {
+		t.Errorf("empty mode + no iface should stay empty (D12); got %q", mode)
+	}
+}
+
+func TestResolveBridgeConfig_DefaultsBindBridge(t *testing.T) {
+	_, br := resolveBridgeConfig(BridgeModeDirect, "enp0s3", "")
+	if br != "br-wan" {
+		t.Errorf("empty dhcp_bind_bridge should default to br-wan, got %q", br)
+	}
+}