fix: retrofit localnet options on every vpcd startup (mulga-998.b Fix 3)

ensureLocalnetOptions was only invoked inside reconcileIGW, which is gated on the external logical switch being absent — so a port whose options were cleared or drifted from the current bridge mode would never be corrected on restart. Drill 2 (stale-options recovery) exposed this: clearing ext-port options and restarting vpcd left the port with options={}. Changes: - Extend ensureLocalnetOptions to also restore network_name=external (not just nat-addresses). A manual `ovn-nbctl clear` wipes both, and the port is non-functional without network_name. - Add RetrofitAllExternalLocalnetOptions: walks every OVN logical switch tagged spinifex:role=external and calls ensureLocalnetOptions on its ext-port-<vpcId>. OVN is the source of truth; NATS KV records may be missing or stale. - Call RetrofitAllExternalLocalnetOptions after ReconcileFromKV during vpcd startup. - Tests: network_name restoration in both centralized and direct modes; RetrofitAll across multiple switches with mixed stale / correct / non- external state.
2026-10-19 19:01:42 +00:00 · 2026-04-24 15:31:42 +10:00
parent 6b75470b0f
commit a00550a200
3 changed files with 168 additions and 14 deletions
@@ -151,13 +151,15 @@ func (h *TopologyHandler) useCentralizedNAT() bool {
 	return h.bridgeMode != BridgeModeDirect
 }

-// ensureLocalnetOptions aligns options:nat-addresses on an existing localnet
-// port with the current bridge mode. CreateLogicalSwitchPort only seeds the
-// options at first creation; if vpcd came up in the wrong mode once (e.g.
-// because setup-ovn.sh had not recreated the veth yet — pre-Fix 1), the
-// localnet port carries stale options forever. This read-before-write is a
-// no-op when already correct, so it is safe to call on every reconcile.
+// ensureLocalnetOptions aligns options on an existing localnet port with the
+// current bridge mode. CreateLogicalSwitchPort only seeds options at first
+// creation; once created, stale options persist forever (e.g. vpcd came up in
+// the wrong mode once — pre-Fix 1 — or an operator cleared options manually).
+// This read-before-write is a no-op when already correct, so it is safe to
+// call on every reconcile.
 //
+// network_name=external is required regardless of bridge mode — it binds the
+// port to the ovn-bridge-mappings entry for the external OVS bridge.
 // Centralized mode (veth/macvlan): options:nat-addresses=router must be set.
 // Distributed mode (direct): options:nat-addresses must be absent.
 func (h *TopologyHandler) ensureLocalnetOptions(ctx context.Context, extPortName string) error {
@@ -165,30 +167,61 @@ func (h *TopologyHandler) ensureLocalnetOptions(ctx context.Context, extPortName
 	if err != nil {
 		return fmt.Errorf("get localnet port %s: %w", extPortName, err)
 	}
-	want := h.useCentralizedNAT()
-	current, hasNat := lsp.Options["nat-addresses"]
-	if want && hasNat && current == "router" {
-		return nil
-	}
-	if !want && !hasNat {
+	wantNat := h.useCentralizedNAT()
+	currentNat, hasNat := lsp.Options["nat-addresses"]
+	currentNetName, hasNetName := lsp.Options["network_name"]
+	natOK := (wantNat && hasNat && currentNat == "router") || (!wantNat && !hasNat)
+	netNameOK := hasNetName && currentNetName == "external"
+	if natOK && netNameOK {
 		return nil
 	}
 	if lsp.Options == nil {
 		lsp.Options = map[string]string{}
 	}
-	if want {
+	lsp.Options["network_name"] = "external"
+	if wantNat {
 		lsp.Options["nat-addresses"] = "router"
 	} else {
 		delete(lsp.Options, "nat-addresses")
 	}
 	slog.Info("vpcd: retrofitting localnet options",
-		"port", extPortName, "bridge_mode", h.bridgeMode, "nat-addresses", lsp.Options["nat-addresses"])
+		"port", extPortName, "bridge_mode", h.bridgeMode,
+		"nat-addresses", lsp.Options["nat-addresses"],
+		"network_name", lsp.Options["network_name"])
 	if err := h.ovn.UpdateLogicalSwitchPort(ctx, lsp); err != nil {
 		return fmt.Errorf("update localnet port %s options: %w", extPortName, err)
 	}
 	return nil
 }

+// RetrofitAllExternalLocalnetOptions walks every OVN logical switch tagged
+// spinifex:role=external and calls ensureLocalnetOptions on its localnet
+// port. OVN is the source of truth — NATS KV records for IGWs may be absent
+// or stale (external switch was created via a live event whose KV record
+// later expired, or an operator cleared options manually). Walking OVN
+// directly catches all of these on every vpcd startup. Idempotent.
+func (h *TopologyHandler) RetrofitAllExternalLocalnetOptions(ctx context.Context) {
+	switches, err := h.ovn.ListLogicalSwitches(ctx)
+	if err != nil {
+		slog.Warn("vpcd: retrofit skipped — list logical switches failed", "err", err)
+		return
+	}
+	for i := range switches {
+		ls := &switches[i]
+		if ls.ExternalIDs["spinifex:role"] != "external" {
+			continue
+		}
+		vpcID := ls.ExternalIDs["spinifex:vpc_id"]
+		if vpcID == "" {
+			continue
+		}
+		extPortName := "ext-port-" + vpcID
+		if err := h.ensureLocalnetOptions(ctx, extPortName); err != nil {
+			slog.Error("vpcd: retrofit localnet options failed", "port", extPortName, "err", err)
+		}
+	}
+}
+
 // dnsServer returns the DNS server string for OVN DHCP options.
 // Uses dns_servers from the external pool config (auto-detected by admin init).
 // Falls back to 8.8.8.8 and 1.1.1.1 if none configured.
@@ -2452,6 +2452,122 @@ func TestEnsureLocalnetOptions_HandlesNilOptionsMap(t *testing.T) {
 	}
 }

+func TestEnsureLocalnetOptions_RestoresNetworkName(t *testing.T) {
+	// Operator (or test drill) cleared options entirely — retrofit must
+	// restore both network_name=external and nat-addresses=router.
+	mock := NewMockOVNClient()
+	_ = mock.Connect(context.Background())
+	ctx := context.Background()
+
+	_ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "ext-vpc-r"})
+	_ = mock.CreateLogicalSwitchPort(ctx, "ext-vpc-r", &nbdb.LogicalSwitchPort{
+		Name:      "ext-port-vpc-r",
+		Type:      "localnet",
+		Addresses: []string{"unknown"},
+		Options:   map[string]string{},
+	})
+
+	topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeVeth))
+	if err := topo.ensureLocalnetOptions(ctx, "ext-port-vpc-r"); err != nil {
+		t.Fatalf("ensureLocalnetOptions: %v", err)
+	}
+	port, _ := mock.GetLogicalSwitchPort(ctx, "ext-port-vpc-r")
+	if port.Options["network_name"] != "external" {
+		t.Errorf("expected network_name=external after retrofit, got %q", port.Options["network_name"])
+	}
+	if port.Options["nat-addresses"] != "router" {
+		t.Errorf("expected nat-addresses=router after retrofit, got %q", port.Options["nat-addresses"])
+	}
+}
+
+func TestEnsureLocalnetOptions_RestoresNetworkNameDirectMode(t *testing.T) {
+	// Direct mode: network_name still required, nat-addresses still absent.
+	mock := NewMockOVNClient()
+	_ = mock.Connect(context.Background())
+	ctx := context.Background()
+
+	_ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "ext-vpc-rd"})
+	_ = mock.CreateLogicalSwitchPort(ctx, "ext-vpc-rd", &nbdb.LogicalSwitchPort{
+		Name:      "ext-port-vpc-rd",
+		Type:      "localnet",
+		Addresses: []string{"unknown"},
+		Options:   map[string]string{},
+	})
+
+	topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeDirect))
+	if err := topo.ensureLocalnetOptions(ctx, "ext-port-vpc-rd"); err != nil {
+		t.Fatalf("ensureLocalnetOptions: %v", err)
+	}
+	port, _ := mock.GetLogicalSwitchPort(ctx, "ext-port-vpc-rd")
+	if port.Options["network_name"] != "external" {
+		t.Errorf("expected network_name=external after retrofit, got %q", port.Options["network_name"])
+	}
+	if _, ok := port.Options["nat-addresses"]; ok {
+		t.Errorf("direct mode should not set nat-addresses, got %q", port.Options["nat-addresses"])
+	}
+}
+
+func TestRetrofitAllExternalLocalnetOptions_FixesClearedOptions(t *testing.T) {
+	// Walks every ext-* switch in OVN (authoritative) and retrofits options
+	// on its localnet port. Covers VPCs whose IGW KV record is missing or
+	// whose options were cleared manually (mulga-998.b Drill 2).
+	mock := NewMockOVNClient()
+	_ = mock.Connect(context.Background())
+	ctx := context.Background()
+
+	// Two external switches with cleared options, one subnet switch (must be
+	// skipped because role != external), one external switch already correct.
+	for _, vpcID := range []string{"vpc-a", "vpc-b"} {
+		_ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{
+			Name:        "ext-" + vpcID,
+			ExternalIDs: map[string]string{"spinifex:role": "external", "spinifex:vpc_id": vpcID},
+		})
+		_ = mock.CreateLogicalSwitchPort(ctx, "ext-"+vpcID, &nbdb.LogicalSwitchPort{
+			Name:      "ext-port-" + vpcID,
+			Type:      "localnet",
+			Addresses: []string{"unknown"},
+			Options:   map[string]string{},
+		})
+	}
+	// A subnet switch that must NOT be retrofitted.
+	_ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{
+		Name:        "subnet-foo",
+		ExternalIDs: map[string]string{"spinifex:subnet_id": "subnet-foo"},
+	})
+	// An ext switch already correct — must remain a no-op.
+	_ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{
+		Name:        "ext-vpc-ok",
+		ExternalIDs: map[string]string{"spinifex:role": "external", "spinifex:vpc_id": "vpc-ok"},
+	})
+	_ = mock.CreateLogicalSwitchPort(ctx, "ext-vpc-ok", &nbdb.LogicalSwitchPort{
+		Name:      "ext-port-vpc-ok",
+		Type:      "localnet",
+		Addresses: []string{"unknown"},
+		Options:   map[string]string{"network_name": "external", "nat-addresses": "router"},
+	})
+	startUpdates := mock.UpdateLogicalSwitchPortCalls
+
+	topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeVeth))
+	topo.RetrofitAllExternalLocalnetOptions(ctx)
+
+	for _, vpcID := range []string{"vpc-a", "vpc-b"} {
+		port, err := mock.GetLogicalSwitchPort(ctx, "ext-port-"+vpcID)
+		if err != nil {
+			t.Fatalf("get port: %v", err)
+		}
+		if port.Options["network_name"] != "external" {
+			t.Errorf("%s: expected network_name=external, got %q", vpcID, port.Options["network_name"])
+		}
+		if port.Options["nat-addresses"] != "router" {
+			t.Errorf("%s: expected nat-addresses=router, got %q", vpcID, port.Options["nat-addresses"])
+		}
+	}
+	// Expect exactly 2 updates (vpc-a + vpc-b); vpc-ok must be idempotent no-op.
+	if got := mock.UpdateLogicalSwitchPortCalls - startUpdates; got != 2 {
+		t.Errorf("expected 2 UpdateLogicalSwitchPort calls (one per stale ext port); got %d", got)
+	}
+}
+
 func TestEnsureLocalnetOptions_ErrorOnMissingPort(t *testing.T) {
 	mock := NewMockOVNClient()
 	_ = mock.Connect(context.Background())
@@ -358,6 +358,11 @@ func launchService(cfg *Config) error {
 	// Runs after subscribing so new events are not missed during reconciliation.
 	ReconcileFromKV(ctx, nc, topo)

+	// Pass 3: Retrofit localnet options on every external switch. Walks OVN
+	// directly so stale/missing KV records can't hide a stale nat-addresses
+	// or cleared network_name. Idempotent; silent when all correct.
+	topo.RetrofitAllExternalLocalnetOptions(ctx)
+
 	slog.Info("vpcd service started, waiting for VPC lifecycle events", "subscriptions", len(subs))

 	// Wait for shutdown signal