mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-21 19:56:33 +00:00
fix(vpcd): self-heal partial IGW topology on reconcile
reconcileIGW was non-idempotent and gated on "external switch missing", so once a prior pass left the ext switch + LRP in place without the localnet LSP, every subsequent vpcd start silently skipped the function and the partial topology persisted forever (br-ext never got its patch port; external traffic failed). Convert each step to Get-then-Create idempotent and drop the cascade rollbacks — partial state is now safe to leave for the next pass to heal. Add FindStaticRoute so the default route is deduped instead of duplicated on every retry. Drop the ext-switch gate in Reconcile and ReconcileFromKV; rename IGWsCreated to IGWsReconciled to reflect the new "called" semantics.
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
+113
-106
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user