diff --git a/spinifex/services/vpcd/mock_ovn.go b/spinifex/services/vpcd/mock_ovn.go index 8a427ab78..7c0c3a104 100644 --- a/spinifex/services/vpcd/mock_ovn.go +++ b/spinifex/services/vpcd/mock_ovn.go @@ -504,6 +504,26 @@ func (m *MockOVNClient) AddStaticRoute(_ context.Context, routerName string, rou return nil } +func (m *MockOVNClient) FindStaticRoute(_ context.Context, routerName, ipPrefix string) (*nbdb.LogicalRouterStaticRoute, error) { + m.mu.Lock() + defer m.mu.Unlock() + lr, exists := m.routers[routerName] + if !exists { + return nil, fmt.Errorf("logical router %q not found", routerName) + } + for _, uuid := range lr.StaticRoutes { + r, ok := m.staticRoutes[uuid] + if !ok { + continue + } + if r.IPPrefix == ipPrefix { + result := *r + return &result, nil + } + } + return nil, nil +} + func (m *MockOVNClient) DeleteStaticRoute(_ context.Context, routerName string, ipPrefix string) error { m.mu.Lock() defer m.mu.Unlock() diff --git a/spinifex/services/vpcd/ovn.go b/spinifex/services/vpcd/ovn.go index ca529351b..5857830c2 100644 --- a/spinifex/services/vpcd/ovn.go +++ b/spinifex/services/vpcd/ovn.go @@ -96,6 +96,7 @@ type OVNClient interface { // Static routes AddStaticRoute(ctx context.Context, routerName string, route *nbdb.LogicalRouterStaticRoute) error DeleteStaticRoute(ctx context.Context, routerName string, ipPrefix string) error + FindStaticRoute(ctx context.Context, routerName, ipPrefix string) (*nbdb.LogicalRouterStaticRoute, error) // Port Groups (security group enforcement) CreatePortGroup(ctx context.Context, name string, ports []string) error @@ -921,6 +922,36 @@ func (c *LiveOVNClient) AddStaticRoute(ctx context.Context, routerName string, r return nil } +// FindStaticRoute returns the first static route on routerName whose IPPrefix +// matches. (nil, nil) when no row matches — caller compares Nexthop and +// OutputPort to detect operator-overridden entries. AddStaticRoute is +// non-idempotent (every retry leaves a fresh duplicate row), so callers that +// run on every reconcile pass must dedupe via this helper first. +func (c *LiveOVNClient) FindStaticRoute(ctx context.Context, routerName, ipPrefix string) (*nbdb.LogicalRouterStaticRoute, error) { + lr, err := c.GetLogicalRouter(ctx, routerName) + if err != nil { + return nil, fmt.Errorf("get logical router for static route lookup: %w", err) + } + owned := make(map[string]struct{}, len(lr.StaticRoutes)) + for _, u := range lr.StaticRoutes { + owned[u] = struct{}{} + } + var routes []nbdb.LogicalRouterStaticRoute + if err := c.client.WhereCache(func(r *nbdb.LogicalRouterStaticRoute) bool { + if r.IPPrefix != ipPrefix { + return false + } + _, ok := owned[r.UUID] + return ok + }).List(ctx, &routes); err != nil { + return nil, fmt.Errorf("list static routes: %w", err) + } + if len(routes) == 0 { + return nil, nil + } + return &routes[0], nil +} + func (c *LiveOVNClient) DeleteStaticRoute(ctx context.Context, routerName string, ipPrefix string) error { // Find the route by IP prefix var routes []nbdb.LogicalRouterStaticRoute diff --git a/spinifex/services/vpcd/reconcile.go b/spinifex/services/vpcd/reconcile.go index cbe65bca2..b1618cde1 100644 --- a/spinifex/services/vpcd/reconcile.go +++ b/spinifex/services/vpcd/reconcile.go @@ -85,10 +85,15 @@ func AcquireReconcileLeader(nc *nats.Conn, holder string) (func(), bool) { } // ReconcileResult tracks what was created during reconciliation. +// +// IGWsReconciled has "called" semantics rather than "created" because +// reconcileIGW is self-healing and idempotent — every successful invocation +// increments, regardless of whether new entities were added. Gauging "what +// changed" requires inspecting OVN directly. type ReconcileResult struct { RoutersCreated int SwitchesCreated int - IGWsCreated int + IGWsReconciled int PortsCreated int NATsReconciled int } @@ -141,23 +146,20 @@ func Reconcile(ctx context.Context, topo *TopologyHandler, bootstrap *BootstrapV } } - // 3. Ensure IGW topology exists (external switch, SNAT, gateway chassis) - extSwitchName := "ext-" + bootstrap.VpcId - if _, err := topo.ovn.GetLogicalSwitch(ctx, extSwitchName); err != nil { - slog.Info("vpcd reconcile: creating IGW topology", "switch", extSwitchName) - if err := topo.reconcileIGW(ctx, bootstrap.VpcId, bootstrap.IgwId); err != nil { - slog.Error("vpcd reconcile: failed to create IGW topology", "err", err) - } else { - result.IGWsCreated++ - } + // 3. Ensure IGW topology. reconcileIGW is ensure-then-skip per child, so + // call it unconditionally — gating on "ext switch missing" trapped + // partial topologies (switch + LRP present, no localnet LSP) in an + // unhealable state across vpcd restarts. + if err := topo.reconcileIGW(ctx, bootstrap.VpcId, bootstrap.IgwId); err != nil { + slog.Error("vpcd reconcile: failed to ensure IGW topology", "err", err) } else { - slog.Debug("vpcd reconcile: IGW topology exists", "switch", extSwitchName) + result.IGWsReconciled++ } slog.Info("vpcd reconcile: complete", "routers_created", result.RoutersCreated, "switches_created", result.SwitchesCreated, - "igws_created", result.IGWsCreated, + "igws_reconciled", result.IGWsReconciled, ) return result @@ -294,14 +296,10 @@ func ReconcileFromKV(ctx context.Context, nc *nats.Conn, topo *TopologyHandler, continue } - extSwitchName := "ext-" + rec.VpcId - if _, err := topo.ovn.GetLogicalSwitch(ctx, extSwitchName); err != nil { - slog.Info("vpcd reconcile-kv: creating IGW topology", "switch", extSwitchName, "igw_id", rec.InternetGatewayId) - if err := topo.reconcileIGW(ctx, rec.VpcId, rec.InternetGatewayId); err != nil { - slog.Error("vpcd reconcile-kv: failed to create IGW topology", "err", err) - } else { - result.IGWsCreated++ - } + if err := topo.reconcileIGW(ctx, rec.VpcId, rec.InternetGatewayId); err != nil { + slog.Error("vpcd reconcile-kv: failed to ensure IGW topology", "err", err, "igw_id", rec.InternetGatewayId) + } else { + result.IGWsReconciled++ } } } @@ -444,7 +442,7 @@ func ReconcileFromKV(ctx context.Context, nc *nats.Conn, topo *TopologyHandler, slog.Info("vpcd reconcile-kv: complete", "routers_created", result.RoutersCreated, "switches_created", result.SwitchesCreated, - "igws_created", result.IGWsCreated, + "igws_reconciled", result.IGWsReconciled, "ports_created", result.PortsCreated, "nats_reconciled", result.NATsReconciled, ) diff --git a/spinifex/services/vpcd/reconcile_test.go b/spinifex/services/vpcd/reconcile_test.go index cbbe619e2..e72d18a2e 100644 --- a/spinifex/services/vpcd/reconcile_test.go +++ b/spinifex/services/vpcd/reconcile_test.go @@ -131,7 +131,7 @@ func TestReconcile_NoBootstrap(t *testing.T) { result := Reconcile(context.Background(), topo, nil) assert.Equal(t, 0, result.RoutersCreated) assert.Equal(t, 0, result.SwitchesCreated) - assert.Equal(t, 0, result.IGWsCreated) + assert.Equal(t, 0, result.IGWsReconciled) } func TestReconcile_EmptyBootstrap(t *testing.T) { @@ -169,7 +169,7 @@ func TestReconcile_CreatesBootstrapTopology(t *testing.T) { result := Reconcile(context.Background(), topo, bootstrap) assert.Equal(t, 1, result.RoutersCreated) assert.Equal(t, 1, result.SwitchesCreated) - assert.Equal(t, 1, result.IGWsCreated) + assert.Equal(t, 1, result.IGWsReconciled) // Verify OVN objects exist ctx := context.Background() @@ -221,13 +221,20 @@ func TestReconcile_Idempotent(t *testing.T) { r1 := Reconcile(context.Background(), topo, bootstrap) assert.Equal(t, 1, r1.RoutersCreated) assert.Equal(t, 1, r1.SwitchesCreated) - assert.Equal(t, 1, r1.IGWsCreated) + assert.Equal(t, 1, r1.IGWsReconciled) - // Second run should skip everything (already exists) + // Second run should skip router/switch creation (already exist). + // IGWsReconciled has "called" semantics — every successful pass counts, + // even when it's a no-op self-heal walk. r2 := Reconcile(context.Background(), topo, bootstrap) assert.Equal(t, 0, r2.RoutersCreated) assert.Equal(t, 0, r2.SwitchesCreated) - assert.Equal(t, 0, r2.IGWsCreated) + assert.Equal(t, 1, r2.IGWsReconciled) + + // Pass must not have duplicated the default route. + lr, err := ovn.GetLogicalRouter(context.Background(), "vpc-vpc-idem") + require.NoError(t, err) + assert.Len(t, lr.StaticRoutes, 1, "default route must not duplicate on retry") } func TestReconcile_PartialState(t *testing.T) { @@ -260,7 +267,7 @@ func TestReconcile_PartialState(t *testing.T) { result := Reconcile(ctx, topo, bootstrap) assert.Equal(t, 0, result.RoutersCreated) assert.Equal(t, 1, result.SwitchesCreated) - assert.Equal(t, 1, result.IGWsCreated) + assert.Equal(t, 1, result.IGWsReconciled) } // --- ReconcileFromKV tests --- @@ -345,7 +352,7 @@ func TestReconcileFromKV_CreatesFullTopology(t *testing.T) { result := ReconcileFromKV(ctx, nc, topo, nil) assert.Equal(t, 1, result.RoutersCreated) assert.Equal(t, 1, result.SwitchesCreated) - assert.Equal(t, 1, result.IGWsCreated) + assert.Equal(t, 1, result.IGWsReconciled) assert.Equal(t, 1, result.PortsCreated) // Verify OVN objects @@ -425,7 +432,7 @@ func TestReconcileFromKV_SkipsDetachedIGW(t *testing.T) { result := ReconcileFromKV(ctx, nc, topo, nil) assert.Equal(t, 1, result.RoutersCreated) // VPC router created - assert.Equal(t, 0, result.IGWsCreated) // Detached IGW skipped + assert.Equal(t, 0, result.IGWsReconciled) // Detached IGW skipped _, err := ovn.GetLogicalSwitch(ctx, "ext-vpc-det") assert.Error(t, err) // External switch should NOT exist @@ -442,7 +449,7 @@ func TestReconcileFromKV_NoBuckets(t *testing.T) { result := ReconcileFromKV(context.Background(), nc, topo, nil) assert.Equal(t, 0, result.RoutersCreated) assert.Equal(t, 0, result.SwitchesCreated) - assert.Equal(t, 0, result.IGWsCreated) + assert.Equal(t, 0, result.IGWsReconciled) assert.Equal(t, 0, result.PortsCreated) } @@ -532,7 +539,7 @@ func TestReconcileFromKV_VersionKeysAndBadJSON(t *testing.T) { // Valid records should still be reconciled despite bad records assert.Equal(t, 1, result.RoutersCreated) assert.Equal(t, 1, result.SwitchesCreated) - assert.Equal(t, 1, result.IGWsCreated) + assert.Equal(t, 1, result.IGWsReconciled) assert.Equal(t, 1, result.PortsCreated) // Verify OVN objects created from valid records @@ -570,7 +577,7 @@ func TestReconcileFromKV_EmptyBuckets(t *testing.T) { result := ReconcileFromKV(context.Background(), nc, topo, nil) assert.Equal(t, 0, result.RoutersCreated) assert.Equal(t, 0, result.SwitchesCreated) - assert.Equal(t, 0, result.IGWsCreated) + assert.Equal(t, 0, result.IGWsReconciled) assert.Equal(t, 0, result.PortsCreated) } diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index 4ad7daa90..cb1139d7d 100644 --- a/spinifex/services/vpcd/topology.go +++ b/spinifex/services/vpcd/topology.go @@ -1300,8 +1300,11 @@ func (h *TopologyHandler) reconcileSubnet(ctx context.Context, subnetId, vpcId, return nil } -// reconcileIGW creates the OVN external switch, gateway router port, SNAT rule, -// default route, and gateway chassis for a VPC's internet gateway. +// reconcileIGW ensures the OVN external switch, localnet port, gateway router +// port, router-bridging LSP, default route, and gateway chassis bindings exist +// for a VPC's internet gateway. Every step is Get-then-Create idempotent, with +// no cascade rollback on failure: partial topology is safe — the next +// reconcile pass heals whatever is missing. func (h *TopologyHandler) reconcileIGW(ctx context.Context, vpcId, igwId string) error { routerName := "vpc-" + vpcId extSwitchName := "ext-" + vpcId @@ -1309,15 +1312,62 @@ func (h *TopologyHandler) reconcileIGW(ctx context.Context, vpcId, igwId string) gwPortName := "gw-" + vpcId switchGWPortName := "gw-port-" + vpcId - // Direct mode: link-local LRP. Centralized mode: allocate from - // pool.GwLrpRange so the LRP can ARP on the WAN subnet (mulga-siv-36). pool := h.findExternalPool("", "") - gatewayNetwork := gatewayPortNetwork wanGateway := "169.254.0.2" - gwLrpIP := "" if pool != nil { wanGateway = pool.Gateway + } + + // 1. External logical switch + if _, err := h.ovn.GetLogicalSwitch(ctx, extSwitchName); err != nil { + extIDs := map[string]string{ + "spinifex:vpc_id": vpcId, + "spinifex:role": "external", + } + if igwId != "" { + extIDs["spinifex:igw_id"] = igwId + } + if err := h.ovn.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{ + Name: extSwitchName, + ExternalIDs: extIDs, + }); err != nil { + return fmt.Errorf("create external switch %s: %w", extSwitchName, err) + } + } + + // 2. Localnet port. ensureLocalnetOptions runs unconditionally — it is a + // read-before-write no-op when the options are already correct, and the + // recovery path (port exists with stale/missing options) needs it. + if _, err := h.ovn.GetLogicalSwitchPort(ctx, extPortName); err != nil { + portExtIDs := map[string]string{"spinifex:vpc_id": vpcId} + if igwId != "" { + portExtIDs["spinifex:igw_id"] = igwId + } + opts := map[string]string{"network_name": "external"} if h.useCentralizedNAT() { + opts["nat-addresses"] = "router" + } + if err := h.ovn.CreateLogicalSwitchPort(ctx, extSwitchName, &nbdb.LogicalSwitchPort{ + Name: extPortName, + Type: "localnet", + Addresses: []string{"unknown"}, + Options: opts, + ExternalIDs: portExtIDs, + }); err != nil { + return fmt.Errorf("create localnet port %s: %w", extPortName, err) + } + } + if err := h.ensureLocalnetOptions(ctx, extPortName); err != nil { + return fmt.Errorf("retrofit localnet options %s: %w", extPortName, err) + } + + // 3. Gateway router port. expectedGatewayPortNetwork is only consulted + // when we actually create the LRP — RetrofitAllGatewayPortNetworks + // handles drift on pre-existing ports separately. + if _, err := h.ovn.GetLogicalRouterPort(ctx, gwPortName); err != nil { + gatewayNetwork := gatewayPortNetwork + gwLrpIP := "" + if pool != nil && h.useCentralizedNAT() { network, ip, allocErr := h.expectedGatewayPortNetwork(ctx, vpcId) if allocErr != nil { return fmt.Errorf("allocate gw LRP IP for %s: %w", vpcId, allocErr) @@ -1325,113 +1375,71 @@ func (h *TopologyHandler) reconcileIGW(ctx context.Context, vpcId, igwId string) gwLrpIP = ip gatewayNetwork = network } - } - - // Build external IDs with optional IGW ID - extIDs := map[string]string{ - "spinifex:vpc_id": vpcId, - "spinifex:role": "external", - } - if igwId != "" { - extIDs["spinifex:igw_id"] = igwId - } - - // 1. Create external logical switch - extSwitch := &nbdb.LogicalSwitch{ - Name: extSwitchName, - ExternalIDs: extIDs, - } - if err := h.ovn.CreateLogicalSwitch(ctx, extSwitch); err != nil { - return fmt.Errorf("create external switch %s: %w", extSwitchName, err) - } - - // 2. Create localnet port - portExtIDs := map[string]string{"spinifex:vpc_id": vpcId} - if igwId != "" { - portExtIDs["spinifex:igw_id"] = igwId - } - reconcileLocalnetOpts := map[string]string{ - "network_name": "external", - } - if h.useCentralizedNAT() { - reconcileLocalnetOpts["nat-addresses"] = "router" - } - localnetPort := &nbdb.LogicalSwitchPort{ - Name: extPortName, - Type: "localnet", - Addresses: []string{"unknown"}, - Options: reconcileLocalnetOpts, - ExternalIDs: portExtIDs, - } - if err := h.ovn.CreateLogicalSwitchPort(ctx, extSwitchName, localnetPort); err != nil { - _ = h.ovn.DeleteLogicalSwitch(ctx, extSwitchName) - return fmt.Errorf("create localnet port %s: %w", extPortName, err) - } - // Retrofit options on pre-existing ports whose mode no longer matches - // (create above is a no-op when the port exists — seeds options only - // on first creation; see ensureLocalnetOptions). - if err := h.ensureLocalnetOptions(ctx, extPortName); err != nil { - return fmt.Errorf("retrofit localnet options %s: %w", extPortName, err) - } - - // 3. Create gateway router port - gwMAC := generateMAC("gw-" + vpcId) - reconcileLrpExtIDs := map[string]string{ - "spinifex:vpc_id": vpcId, - "spinifex:role": "gateway", - } - if gwLrpIP != "" { - reconcileLrpExtIDs[gatewayIPExtID] = gwLrpIP - } - lrp := &nbdb.LogicalRouterPort{ - Name: gwPortName, - MAC: gwMAC, - Networks: []string{gatewayNetwork}, - ExternalIDs: reconcileLrpExtIDs, - } - if err := h.ovn.CreateLogicalRouterPort(ctx, routerName, lrp); err != nil { - _ = h.ovn.DeleteLogicalSwitch(ctx, extSwitchName) - return fmt.Errorf("create gateway router port %s: %w", gwPortName, err) - } - // Stale-Networks self-heal lives in RetrofitAllGatewayPortNetworks at - // startup — Reconcile gates this whole function on "ext switch missing", - // so by the time we reach this line the LRP we just created cannot be - // stale (mulga-siv-26 D8). - - // 4. Create switch port connecting external switch to router - switchGWPort := &nbdb.LogicalSwitchPort{ - Name: switchGWPortName, - Type: "router", - Addresses: []string{"router"}, - Options: map[string]string{"router-port": gwPortName}, - ExternalIDs: map[string]string{ + lrpExtIDs := map[string]string{ "spinifex:vpc_id": vpcId, - }, + "spinifex:role": "gateway", + } + if gwLrpIP != "" { + lrpExtIDs[gatewayIPExtID] = gwLrpIP + } + if err := h.ovn.CreateLogicalRouterPort(ctx, routerName, &nbdb.LogicalRouterPort{ + Name: gwPortName, + MAC: generateMAC("gw-" + vpcId), + Networks: []string{gatewayNetwork}, + ExternalIDs: lrpExtIDs, + }); err != nil { + return fmt.Errorf("create gateway router port %s: %w", gwPortName, err) + } } - if err := h.ovn.CreateLogicalSwitchPort(ctx, extSwitchName, switchGWPort); err != nil { - _ = h.ovn.DeleteLogicalRouterPort(ctx, routerName, gwPortName) - _ = h.ovn.DeleteLogicalSwitch(ctx, extSwitchName) - return fmt.Errorf("create switch gateway port %s: %w", switchGWPortName, err) + + // 4. Switch port connecting external switch to router + if _, err := h.ovn.GetLogicalSwitchPort(ctx, switchGWPortName); err != nil { + if err := h.ovn.CreateLogicalSwitchPort(ctx, extSwitchName, &nbdb.LogicalSwitchPort{ + Name: switchGWPortName, + Type: "router", + Addresses: []string{"router"}, + Options: map[string]string{"router-port": gwPortName}, + ExternalIDs: map[string]string{"spinifex:vpc_id": vpcId}, + }); err != nil { + return fmt.Errorf("create switch gateway port %s: %w", switchGWPortName, err) + } } // 5. No blanket SNAT — per-VM dnat_and_snat rules handle public instances. // See handleIGWAttach comment for rationale (AWS parity). - // 6. Add default route (OutputPort required because the LRP uses link-local - // 169.254.0.1/30, off-subnet from the WAN nexthop) - defaultRoute := &nbdb.LogicalRouterStaticRoute{ - IPPrefix: "0.0.0.0/0", - Nexthop: wanGateway, - OutputPort: &gwPortName, - ExternalIDs: map[string]string{ - "spinifex:vpc_id": vpcId, - }, + // 6. Default route. AddStaticRoute is non-idempotent (every retry leaves + // a fresh duplicate row), so look up first via FindStaticRoute. An + // existing row with mismatched nexthop/output is treated as an operator + // override and left alone. + existing, err := h.ovn.FindStaticRoute(ctx, routerName, "0.0.0.0/0") + if err != nil { + slog.Warn("vpcd reconcile: failed to query default route", "router", routerName, "err", err) } - if err := h.ovn.AddStaticRoute(ctx, routerName, defaultRoute); err != nil { - slog.Warn("vpcd reconcile: failed to add default route", "err", err) + switch { + case existing == nil && err == nil: + if addErr := h.ovn.AddStaticRoute(ctx, routerName, &nbdb.LogicalRouterStaticRoute{ + IPPrefix: "0.0.0.0/0", + Nexthop: wanGateway, + OutputPort: &gwPortName, + ExternalIDs: map[string]string{"spinifex:vpc_id": vpcId}, + }); addErr != nil { + slog.Warn("vpcd reconcile: failed to add default route", "err", addErr) + } + case existing != nil: + existingPort := "" + if existing.OutputPort != nil { + existingPort = *existing.OutputPort + } + if existing.Nexthop != wanGateway || existingPort != gwPortName { + slog.Warn("vpcd reconcile: default route differs from expected, leaving existing entry in place", + "router", routerName, + "existing_nexthop", existing.Nexthop, "want_nexthop", wanGateway, + "existing_output_port", existingPort, "want_output_port", gwPortName) + } } - // 7. Schedule gateway chassis + // 7. Schedule gateway chassis (already idempotent via SetGatewayChassis). for i, chassis := range h.chassisNames { priority := max(20-(i*5), 1) if err := h.ovn.SetGatewayChassis(ctx, gwPortName, chassis, priority); err != nil { @@ -1439,9 +1447,8 @@ func (h *TopologyHandler) reconcileIGW(ctx context.Context, vpcId, igwId string) } } - slog.Info("vpcd reconcile: created IGW topology", - "ext_switch", extSwitchName, "gw_port", gwPortName, - "lrp_network", gatewayNetwork, "wan_gateway", wanGateway) + slog.Info("vpcd reconcile: ensured IGW topology", + "ext_switch", extSwitchName, "gw_port", gwPortName, "wan_gateway", wanGateway) return nil } diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index 0627fb5f9..43812661e 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/spinifex/services/vpcd/topology_test.go @@ -3086,42 +3086,246 @@ func TestEnsureLocalnetOptions_ErrorOnMissingPort(t *testing.T) { } } -// 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) { +// reconcileIGW is now ensure-then-skip per child with no rollback paths, so +// running against an empty NB DB must produce a fully-formed IGW topology in +// one pass. +func TestReconcileIGW_FullCreate(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) + topo := NewTopologyHandler(mock, + WithBridgeMode(BridgeModeDirect), + WithChassisNames([]string{"chassis-A"}), + ) + if err := mock.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-vpc-fc"}); err != nil { + t.Fatalf("seed router: %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") + if err := topo.reconcileIGW(ctx, "vpc-fc", "igw-fc"); err != nil { + t.Fatalf("reconcileIGW: %v", err) + } + + if _, err := mock.GetLogicalSwitch(ctx, "ext-vpc-fc"); err != nil { + t.Errorf("ext switch missing: %v", err) + } + lsp, err := mock.GetLogicalSwitchPort(ctx, "ext-port-vpc-fc") + if err != nil { + t.Fatalf("localnet port missing: %v", err) + } + if lsp.Type != "localnet" || lsp.Options["network_name"] != "external" { + t.Errorf("localnet port misconfigured: type=%q options=%v", lsp.Type, lsp.Options) + } + if _, err := mock.GetLogicalRouterPort(ctx, "gw-vpc-fc"); err != nil { + t.Errorf("gateway LRP missing: %v", err) + } + if _, err := mock.GetLogicalSwitchPort(ctx, "gw-port-vpc-fc"); err != nil { + t.Errorf("router-LSP missing: %v", err) + } + route, err := mock.FindStaticRoute(ctx, "vpc-vpc-fc", "0.0.0.0/0") + if err != nil { + t.Fatalf("FindStaticRoute: %v", err) + } + if route == nil { + t.Errorf("default route missing") + } + gc, err := mock.GetGatewayChassisByName(ctx, "gw-vpc-fc-chassis-A") + if err != nil { + t.Fatalf("GetGatewayChassisByName: %v", err) + } + if gc == nil { + t.Errorf("gateway chassis binding missing") + } +} + +// Partial state: switch + LRP + router-LSP exist but localnet LSP is missing +// (the cell-10 nightly's bootstrap topology shape). reconcileIGW must add the +// missing localnet without touching the rest. +func TestReconcileIGW_PartialState_MissingLocalnet(t *testing.T) { + mock := NewMockOVNClient() + _ = mock.Connect(context.Background()) + ctx := context.Background() + topo := NewTopologyHandler(mock, + WithBridgeMode(BridgeModeDirect), + WithChassisNames([]string{"chassis-A"}), + ) + + if err := mock.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-vpc-pml"}); err != nil { + t.Fatalf("seed router: %v", err) + } + if err := mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{ + Name: "ext-vpc-pml", + ExternalIDs: map[string]string{ + "spinifex:vpc_id": "vpc-pml", + "spinifex:role": "external", + }, + }); err != nil { + t.Fatalf("seed ext switch: %v", err) + } + if err := mock.CreateLogicalRouterPort(ctx, "vpc-vpc-pml", &nbdb.LogicalRouterPort{ + Name: "gw-vpc-pml", + MAC: "02:00:00:aa:bb:01", + Networks: []string{"169.254.0.1/30"}, + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-pml", "spinifex:role": "gateway"}, + }); err != nil { + t.Fatalf("seed LRP: %v", err) + } + if err := mock.CreateLogicalSwitchPort(ctx, "ext-vpc-pml", &nbdb.LogicalSwitchPort{ + Name: "gw-port-vpc-pml", + Type: "router", + Addresses: []string{"router"}, + Options: map[string]string{"router-port": "gw-vpc-pml"}, + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-pml"}, + }); err != nil { + t.Fatalf("seed router-LSP: %v", err) + } + + if err := topo.reconcileIGW(ctx, "vpc-pml", "igw-pml"); err != nil { + t.Fatalf("reconcileIGW: %v", err) + } + + lsp, err := mock.GetLogicalSwitchPort(ctx, "ext-port-vpc-pml") + if err != nil { + t.Fatalf("localnet port still missing after self-heal: %v", err) + } + if lsp.Type != "localnet" { + t.Errorf("expected type=localnet, got %q", lsp.Type) + } + if lsp.Options["network_name"] != "external" { + t.Errorf("expected network_name=external, got %q", lsp.Options["network_name"]) + } +} + +// Partial state: switch + localnet exist but the gateway LRP and router-LSP +// are missing. reconcileIGW must finish what a previously-failed pass started. +func TestReconcileIGW_PartialState_MissingLRP(t *testing.T) { + mock := NewMockOVNClient() + _ = mock.Connect(context.Background()) + ctx := context.Background() + topo := NewTopologyHandler(mock, + WithBridgeMode(BridgeModeDirect), + WithChassisNames([]string{"chassis-A"}), + ) + + if err := mock.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-vpc-pmr"}); err != nil { + t.Fatalf("seed router: %v", err) + } + if err := mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{ + Name: "ext-vpc-pmr", + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-pmr", "spinifex:role": "external"}, + }); err != nil { + t.Fatalf("seed ext switch: %v", err) + } + if err := mock.CreateLogicalSwitchPort(ctx, "ext-vpc-pmr", &nbdb.LogicalSwitchPort{ + Name: "ext-port-vpc-pmr", + Type: "localnet", + Addresses: []string{"unknown"}, + Options: map[string]string{"network_name": "external"}, + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-pmr"}, + }); err != nil { + t.Fatalf("seed localnet: %v", err) + } + + if err := topo.reconcileIGW(ctx, "vpc-pmr", "igw-pmr"); err != nil { + t.Fatalf("reconcileIGW: %v", err) + } + + if _, err := mock.GetLogicalRouterPort(ctx, "gw-vpc-pmr"); err != nil { + t.Errorf("gateway LRP not created: %v", err) + } + if _, err := mock.GetLogicalSwitchPort(ctx, "gw-port-vpc-pmr"); err != nil { + t.Errorf("router-LSP not created: %v", err) + } + gc, err := mock.GetGatewayChassisByName(ctx, "gw-vpc-pmr-chassis-A") + if err != nil { + t.Fatalf("GetGatewayChassisByName: %v", err) + } + if gc == nil { + t.Errorf("gateway chassis binding missing after heal") + } +} + +// Full pre-existing topology: reconcileIGW must be a no-op (no errors, no new +// entities, no duplicate default route). This is the steady-state self-heal +// path that runs on every vpcd start. +func TestReconcileIGW_FullExisting(t *testing.T) { + mock := NewMockOVNClient() + _ = mock.Connect(context.Background()) + ctx := context.Background() + topo := NewTopologyHandler(mock, + WithBridgeMode(BridgeModeDirect), + WithChassisNames([]string{"chassis-A"}), + ) + if err := mock.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-vpc-fe"}); err != nil { + t.Fatalf("seed router: %v", err) + } + + // First pass creates the topology end-to-end. + if err := topo.reconcileIGW(ctx, "vpc-fe", "igw-fe"); err != nil { + t.Fatalf("first reconcileIGW: %v", err) + } + extBefore, err := mock.GetLogicalSwitch(ctx, "ext-vpc-fe") + if err != nil { + t.Fatalf("ext switch missing after first pass: %v", err) + } + lrBefore, err := mock.GetLogicalRouter(ctx, "vpc-vpc-fe") + if err != nil { + t.Fatalf("router fetch: %v", err) + } + if got := len(lrBefore.StaticRoutes); got != 1 { + t.Fatalf("first pass: expected 1 default route, got %d", got) + } + + // Second pass against the now-complete topology. + if err := topo.reconcileIGW(ctx, "vpc-fe", "igw-fe"); err != nil { + t.Fatalf("second reconcileIGW: %v", err) + } + extAfter, err := mock.GetLogicalSwitch(ctx, "ext-vpc-fe") + if err != nil { + t.Fatalf("ext switch fetch: %v", err) + } + if extAfter.UUID != extBefore.UUID { + t.Errorf("ext switch UUID changed (was %q, now %q) — second pass must not recreate", extBefore.UUID, extAfter.UUID) + } + lrAfter, err := mock.GetLogicalRouter(ctx, "vpc-vpc-fe") + if err != nil { + t.Fatalf("router fetch: %v", err) + } + if got := len(lrAfter.StaticRoutes); got != 1 { + t.Errorf("second pass: expected 1 default route, got %d", got) + } +} + +// AddStaticRoute is non-idempotent on its own — without FindStaticRoute, every +// retry leaves a fresh duplicate 0.0.0.0/0 row. Pre-seed a complete topology +// (exercising the path where reconcileIGW finds an existing route) and verify +// no duplicate is added. +func TestReconcileIGW_DuplicateRoute(t *testing.T) { + mock := NewMockOVNClient() + _ = mock.Connect(context.Background()) + ctx := context.Background() + topo := NewTopologyHandler(mock, + WithBridgeMode(BridgeModeDirect), + WithChassisNames([]string{"chassis-A"}), + ) + if err := mock.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-vpc-dr"}); err != nil { + t.Fatalf("seed router: %v", err) + } + if err := topo.reconcileIGW(ctx, "vpc-dr", "igw-dr"); err != nil { + t.Fatalf("first reconcileIGW: %v", err) + } + + // Multiple subsequent passes must keep the route count at exactly 1. + for i := range 3 { + if err := topo.reconcileIGW(ctx, "vpc-dr", "igw-dr"); err != nil { + t.Fatalf("reconcileIGW pass %d: %v", i+2, err) + } + lr, err := mock.GetLogicalRouter(ctx, "vpc-vpc-dr") + if err != nil { + t.Fatalf("router fetch pass %d: %v", i+2, err) + } + if got := len(lr.StaticRoutes); got != 1 { + t.Fatalf("pass %d: expected 1 default route, got %d", i+2, got) + } } }