From 853da4cc2acdfab026517cb91d9a602cfd9ddad0 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Fri, 24 Apr 2026 13:08:04 +1000 Subject: [PATCH] fix: loud detectBridgeMode + verifyBridgeMode sanity check (mulga-998.b Fix 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 6 + spinifex/services/vpcd/topology_test.go | 101 ++++++++++++++ spinifex/services/vpcd/vpcd.go | 145 ++++++++++++++++--- spinifex/services/vpcd/vpcd_test.go | 177 ++++++++++++++++++++++++ 4 files changed, 408 insertions(+), 21 deletions(-) diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index 16c59acab..7025f2382 100644 --- a/spinifex/services/vpcd/topology.go +++ b/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. diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index 3c50b126f..3d91b40f6 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/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") + } +} diff --git a/spinifex/services/vpcd/vpcd.go b/spinifex/services/vpcd/vpcd.go index fe8a4a9f1..799b8f9e3 100644 --- a/spinifex/services/vpcd/vpcd.go +++ b/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//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 { diff --git a/spinifex/services/vpcd/vpcd_test.go b/spinifex/services/vpcd/vpcd_test.go index c3f4bbf5a..6ec571365 100644 --- a/spinifex/services/vpcd/vpcd_test.go +++ b/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) + } +}