From a4d409ea572fcfa04bf9d6ab3f47636c5e4f2789 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Thu, 21 May 2026 16:48:20 +1000 Subject: [PATCH] feat(network/reconcile): L6 intent-actual reconciler replaces 5-pass startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build spinifex/spinifex/network/reconcile/ package per Phase 2.2 of the network layer redesign. Reads desired state from NATS KV filtered by vpc.AZ (with legacy empty-AZ matches-all rule), scans actual state from OVN NB DB, diffs and applies with VPC → Subnet → Port → SG → IGW → EIP/NATGW topological ordering. Atomic port-group join via CreateLogicalSwitchPortInGroups closes the racy gap the legacy scanENIPortMembership pass used to revive 30s later. A 5-minute drift goroutine (leader-gated via JetStream KV CAS) replaces the old 30s ReconcileSGsLoop. Adds AZ field to VPCRecord stamped at create time. Empty AZ on pre-Phase 2.2 records matches every local AZ, so no migration pass is required. Replaces and removes the 5-pass startup sequence in services/vpcd: - Reconcile + ReconcileFromKV - ReconcileSGsOnce + ReconcileSGsLoop - RetrofitAllExternalLocalnetOptions - RetrofitAllGatewayPortNetworks IGW fresh-create continues to flow through the legacy NATS handler path (handleIGWAttach) so the dhcpManager wiring stays intact until Phase 2.3; the reconciler only rebinds gateway-chassis on existing IGW topology. Closes mulga-siv-125.3.2. --- cmd/spinifex/cmd/service.go | 1 + spinifex/handlers/ec2/vpc/service_impl.go | 19 +- spinifex/network/reconcile/actual.go | 94 ++ spinifex/network/reconcile/apply.go | 350 ++++++++ spinifex/network/reconcile/apply_test.go | 303 +++++++ spinifex/network/reconcile/drift.go | 66 ++ spinifex/network/reconcile/intent.go | 431 +++++++++ spinifex/network/reconcile/intent_test.go | 181 ++++ spinifex/network/reconcile/lock.go | 74 ++ spinifex/network/reconcile/reconcile.go | 143 +++ spinifex/services/vpcd/reconcile.go | 451 ---------- spinifex/services/vpcd/reconcile_sgs.go | 256 ------ spinifex/services/vpcd/reconcile_sgs_test.go | 368 -------- spinifex/services/vpcd/reconcile_test.go | 892 ------------------- spinifex/services/vpcd/topology.go | 170 ---- spinifex/services/vpcd/topology_test.go | 2 +- spinifex/services/vpcd/vpcd.go | 126 ++- 17 files changed, 1756 insertions(+), 2171 deletions(-) create mode 100644 spinifex/network/reconcile/actual.go create mode 100644 spinifex/network/reconcile/apply.go create mode 100644 spinifex/network/reconcile/apply_test.go create mode 100644 spinifex/network/reconcile/drift.go create mode 100644 spinifex/network/reconcile/intent.go create mode 100644 spinifex/network/reconcile/intent_test.go create mode 100644 spinifex/network/reconcile/lock.go create mode 100644 spinifex/network/reconcile/reconcile.go delete mode 100644 spinifex/services/vpcd/reconcile.go delete mode 100644 spinifex/services/vpcd/reconcile_sgs.go delete mode 100644 spinifex/services/vpcd/reconcile_sgs_test.go delete mode 100644 spinifex/services/vpcd/reconcile_test.go diff --git a/cmd/spinifex/cmd/service.go b/cmd/spinifex/cmd/service.go index 0ac6b9fbd..103d8314b 100644 --- a/cmd/spinifex/cmd/service.go +++ b/cmd/spinifex/cmd/service.go @@ -780,6 +780,7 @@ var vpcdStartCmd = &cobra.Command{ ExternalInterface: nodeConfig.VPCD.ExternalInterface, DhcpBindBridge: nodeConfig.VPCD.DhcpBindBridge, BridgeMode: nodeConfig.VPCD.BridgeMode, + AZ: nodeConfig.AZ, }) if err != nil { fmt.Println("Error starting vpcd service:", err) diff --git a/spinifex/handlers/ec2/vpc/service_impl.go b/spinifex/handlers/ec2/vpc/service_impl.go index 4790a0df5..b09d683e9 100644 --- a/spinifex/handlers/ec2/vpc/service_impl.go +++ b/spinifex/handlers/ec2/vpc/service_impl.go @@ -35,13 +35,18 @@ const ( KVBucketVNICounterVersion = 1 ) -// VPCRecord represents a stored VPC +// VPCRecord represents a stored VPC. AZ stamps the local availability zone +// at create time; the network/reconcile package filters its intent set by +// matching `vpc.AZ == localAZ` (with the legacy rule that empty matches +// every AZ for pre-Phase-2.2 records). New records always populate AZ from +// the daemon's config.AZ. type VPCRecord struct { VpcId string `json:"vpc_id"` CidrBlock string `json:"cidr_block"` State string `json:"state"` IsDefault bool `json:"is_default"` VNI int64 `json:"vni"` + AZ string `json:"az,omitempty"` EnableDnsHostnames bool `json:"enable_dns_hostnames"` EnableDnsSupport bool `json:"enable_dns_support"` EnableNetworkAddressUsageMetrics bool `json:"enable_network_address_usage_metrics"` @@ -86,6 +91,16 @@ func (s *VPCServiceImpl) SetExternalIPAM(ipam *ExternalIPAM, eipKV nats.KeyValue s.eipKV = eipKV } +// localAZ returns the node's local availability zone, sourced from +// s.config.AZ. Returns "" when no config is wired (mostly test paths), +// which the reconciler treats as a legacy record matching every AZ. +func (s *VPCServiceImpl) localAZ() string { + if s.config == nil { + return "" + } + return s.config.AZ +} + // NewVPCServiceImplWithNATS creates a VPC service with NATS JetStream for persistence func NewVPCServiceImplWithNATS(cfg *config.Config, natsConn *nats.Conn) (*VPCServiceImpl, error) { js, err := natsConn.JetStream() @@ -231,6 +246,7 @@ func (s *VPCServiceImpl) CreateVpc(input *ec2.CreateVpcInput, accountID string) State: "available", IsDefault: false, VNI: vni, + AZ: s.localAZ(), EnableDnsSupport: true, // AWS default EnableDnsHostnames: false, // AWS default Tags: utils.ExtractTags(input.TagSpecifications, "vpc"), @@ -1026,6 +1042,7 @@ func (s *VPCServiceImpl) EnsureDefaultVPC(accountID string, bootstrap ...Bootstr State: "available", IsDefault: true, VNI: vni, + AZ: s.localAZ(), EnableDnsSupport: true, // AWS default EnableDnsHostnames: true, // AWS default for default VPC Tags: map[string]string{"Name": "default"}, diff --git a/spinifex/network/reconcile/actual.go b/spinifex/network/reconcile/actual.go new file mode 100644 index 000000000..bf35dc5d5 --- /dev/null +++ b/spinifex/network/reconcile/actual.go @@ -0,0 +1,94 @@ +package reconcile + +import ( + "context" + "fmt" + "strings" + + "github.com/mulgadc/spinifex/spinifex/network/ovn" +) + +// ActualState is the live OVN NB DB snapshot the reconciler diffs IntentState +// against. Membership in each set is presence-by-name — the apply stage +// drives a Get-then-mutate against the live client when it needs the +// per-row payload. +// +// Orphan deletion is restricted to security group port groups (sg_*) to +// match the legacy ReconcileSGsOnce contract; other resource types are +// create-or-skip only. Tightening this is a Phase 4 cleanup item. +type ActualState struct { + Routers map[string]struct{} // logical router name → present + Switches map[string]struct{} // logical switch name → present + Ports map[string]struct{} // logical switch port name → present + RouterPorts map[string]struct{} // logical router port name → present + PortGroups map[string]struct{} // port group name → present + ExternalSwch map[string]struct{} // vpcID → external switch present (spinifex:role=external) +} + +func newActualState() ActualState { + return ActualState{ + Routers: make(map[string]struct{}), + Switches: make(map[string]struct{}), + Ports: make(map[string]struct{}), + RouterPorts: make(map[string]struct{}), + PortGroups: make(map[string]struct{}), + ExternalSwch: make(map[string]struct{}), + } +} + +// scanActual walks the OVN NB DB once and returns a presence map per +// resource type. Any List failure is fatal because a partial scan produces +// unsafe diff decisions (a missing entry would look like a create +// candidate). Callers must surface the error and skip the reconcile cycle. +func scanActual(ctx context.Context, client ovn.Client) (ActualState, error) { + actual := newActualState() + + routers, err := client.ListLogicalRouters(ctx) + if err != nil { + return ActualState{}, fmt.Errorf("list logical routers: %w", err) + } + for _, r := range routers { + actual.Routers[r.Name] = struct{}{} + } + + switches, err := client.ListLogicalSwitches(ctx) + if err != nil { + return ActualState{}, fmt.Errorf("list logical switches: %w", err) + } + for _, s := range switches { + actual.Switches[s.Name] = struct{}{} + if s.ExternalIDs["spinifex:role"] == "external" { + if vpcID := s.ExternalIDs["spinifex:vpc_id"]; vpcID != "" { + actual.ExternalSwch[vpcID] = struct{}{} + } + } + for _, port := range s.Ports { + actual.Ports[port] = struct{}{} + } + } + + routerPorts, err := client.ListLogicalRouterPorts(ctx) + if err != nil { + return ActualState{}, fmt.Errorf("list logical router ports: %w", err) + } + for _, rp := range routerPorts { + actual.RouterPorts[rp.Name] = struct{}{} + } + + portGroups, err := client.ListPortGroups(ctx) + if err != nil { + return ActualState{}, fmt.Errorf("list port groups: %w", err) + } + for _, pg := range portGroups { + actual.PortGroups[pg.Name] = struct{}{} + } + + return actual, nil +} + +// portGroupIsManaged reports whether a port group name is one we own +// (sg_*) and is therefore in scope for orphan deletion. Non-managed +// port groups (third-party usage) are left alone. +func portGroupIsManaged(name string) bool { + return strings.HasPrefix(name, "sg_") +} diff --git a/spinifex/network/reconcile/apply.go b/spinifex/network/reconcile/apply.go new file mode 100644 index 000000000..fc918a8f2 --- /dev/null +++ b/spinifex/network/reconcile/apply.go @@ -0,0 +1,350 @@ +package reconcile + +import ( + "context" + "fmt" + "log/slog" + "net/netip" + + "github.com/mulgadc/spinifex/spinifex/network/topology" + "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" + "github.com/mulgadc/spinifex/spinifex/utils" +) + +// applyVPCs ensures every intent VPC has a LogicalRouter. OVN-only routers +// are left alone (manual ops territory; tightening this is a Phase 4 +// cleanup item). +func (r *reconciler) applyVPCs(ctx context.Context, intent IntentState, actual ActualState) { + for vpcID, spec := range intent.VPCs { + routerName := topology.VPCRouter(vpcID) + if _, ok := actual.Routers[routerName]; ok { + continue + } + lr := &nbdb.LogicalRouter{ + Name: routerName, + ExternalIDs: map[string]string{ + "spinifex:vpc_id": vpcID, + "spinifex:cidr": spec.CIDR.String(), + }, + } + if err := r.ovn.CreateLogicalRouter(ctx, lr); err != nil { + slog.Error("reconcile/apply: create VPC router failed", "vpc_id", vpcID, "err", err) + continue + } + actual.Routers[routerName] = struct{}{} + slog.Info("reconcile/apply: created VPC router", "vpc_id", vpcID, "router", routerName) + } +} + +// applySubnets ensures every intent Subnet has a LogicalSwitch, the matching +// LRP on the parent router, the router-side LSP on the switch, and a +// DHCPOptions row. Each of the four steps is independently idempotent so a +// partial existing topology heals without needing rollback. +func (r *reconciler) applySubnets(ctx context.Context, intent IntentState, actual ActualState) { + for subnetID, spec := range intent.Subnets { + switchName := topology.SubnetSwitch(subnetID) + routerName := topology.VPCRouter(spec.VPCID) + routerPortName := topology.SubnetRouterPort(subnetID) + switchRouterPortName := topology.SubnetSwitchRouterPort(subnetID) + + gwIP, prefixBits, err := subnetGatewayCIDR(spec.CIDR) + if err != nil { + slog.Error("reconcile/apply: subnet gateway calc failed", "subnet_id", subnetID, "err", err) + continue + } + gwCIDRString := fmt.Sprintf("%s/%d", gwIP, prefixBits) + routerMAC := utils.HashMAC(subnetID) + + if _, ok := actual.Switches[switchName]; !ok { + ls := &nbdb.LogicalSwitch{ + Name: switchName, + ExternalIDs: map[string]string{ + "spinifex:subnet_id": subnetID, + "spinifex:vpc_id": spec.VPCID, + }, + } + if err := r.ovn.CreateLogicalSwitch(ctx, ls); err != nil { + slog.Error("reconcile/apply: create subnet switch failed", "subnet_id", subnetID, "err", err) + continue + } + actual.Switches[switchName] = struct{}{} + } + + if _, ok := actual.RouterPorts[routerPortName]; !ok { + lrp := &nbdb.LogicalRouterPort{ + Name: routerPortName, + MAC: routerMAC, + Networks: []string{gwCIDRString}, + ExternalIDs: map[string]string{ + "spinifex:subnet_id": subnetID, + "spinifex:vpc_id": spec.VPCID, + }, + } + if err := r.ovn.CreateLogicalRouterPort(ctx, routerName, lrp); err != nil { + slog.Error("reconcile/apply: create subnet LRP failed", "subnet_id", subnetID, "err", err) + continue + } + actual.RouterPorts[routerPortName] = struct{}{} + } + + if _, err := r.ovn.GetLogicalSwitchPort(ctx, switchRouterPortName); err != nil { + lsp := &nbdb.LogicalSwitchPort{ + Name: switchRouterPortName, + Type: "router", + Addresses: []string{"router"}, + Options: map[string]string{"router-port": routerPortName}, + ExternalIDs: map[string]string{ + "spinifex:subnet_id": subnetID, + "spinifex:vpc_id": spec.VPCID, + }, + } + if err := r.ovn.CreateLogicalSwitchPort(ctx, switchName, lsp); err != nil { + slog.Error("reconcile/apply: create subnet router-LSP failed", "subnet_id", subnetID, "err", err) + continue + } + } + + if existing, err := r.ovn.FindDHCPOptionsByCIDR(ctx, spec.CIDR.String()); err != nil || existing == nil { + opts := &nbdb.DHCPOptions{ + CIDR: spec.CIDR.String(), + Options: map[string]string{ + "server_id": gwIP, + "server_mac": routerMAC, + "lease_time": "3600", + "router": gwIP, + "dns_server": "8.8.8.8", + "mtu": "1442", + }, + ExternalIDs: map[string]string{ + "spinifex:subnet_id": subnetID, + "spinifex:vpc_id": spec.VPCID, + }, + } + if _, dErr := r.ovn.CreateDHCPOptions(ctx, opts); dErr != nil { + slog.Warn("reconcile/apply: create DHCP options failed (non-fatal)", "subnet_id", subnetID, "err", dErr) + } + } + + slog.Info("reconcile/apply: ensured subnet topology", + "subnet_id", subnetID, "switch", switchName, "router_port", routerPortName) + } +} + +// applySGs runs the SG stage in two halves: first ensure every intent SG's +// port group exists in OVN, then push the ACL set via the policy manager. +// Orphan port groups (sg_* with no matching intent SG) are torn down. +// Port group lifecycle remains the reconciler's responsibility until +// topology.Manager grows EnsureSecurityGroupPortGroup / DeleteSGPortGroup +// (parent plan §2.1 deferred to a follow-on bead). +func (r *reconciler) applySGs(ctx context.Context, intent IntentState, actual ActualState) { + for groupID, spec := range intent.SGs { + pgName := topology.SecurityGroupPortGroup(groupID) + if _, ok := actual.PortGroups[pgName]; !ok { + if err := r.ovn.CreatePortGroup(ctx, pgName, nil); err != nil { + slog.Error("reconcile/apply: create SG port group failed", "sg", groupID, "pg", pgName, "err", err) + continue + } + actual.PortGroups[pgName] = struct{}{} + } + if err := r.sg.EnsureSG(ctx, spec); err != nil { + slog.Error("reconcile/apply: EnsureSG failed", "sg", groupID, "err", err) + } + } + + wantPGs := make(map[string]struct{}, len(intent.SGs)) + for groupID := range intent.SGs { + wantPGs[topology.SecurityGroupPortGroup(groupID)] = struct{}{} + } + for pgName := range actual.PortGroups { + if !portGroupIsManaged(pgName) { + continue + } + if _, ok := wantPGs[pgName]; ok { + continue + } + if err := r.ovn.ClearACLs(ctx, pgName); err != nil { + slog.Warn("reconcile/apply: orphan ClearACLs failed", "pg", pgName, "err", err) + continue + } + if err := r.ovn.DeletePortGroup(ctx, pgName); err != nil { + slog.Warn("reconcile/apply: orphan DeletePortGroup failed", "pg", pgName, "err", err) + continue + } + delete(actual.PortGroups, pgName) + slog.Info("reconcile/apply: removed orphan port group", "pg", pgName) + } +} + +// applyPorts ensures every intent ENI port has a LogicalSwitchPort with +// atomic port-group memberships matching its SGIDs. Existing ports get a +// diff-based UpdatePortGroupMemberships call so a 5-SG → different-5-SG +// modify never exposes an intermediate state (OVN default = unrestricted +// would apply for the gap). The atomic create-with-groups primitive is the +// reason this stage sits below applySGs in the topo order: the port groups +// must exist first. +func (r *reconciler) applyPorts(ctx context.Context, intent IntentState, actual ActualState) { + for portID, spec := range intent.Ports { + portName := topology.Port(portID) + switchName := topology.SubnetSwitch(spec.SubnetID) + desiredPGs := make([]string, 0, len(spec.SGIDs)) + for _, sgID := range spec.SGIDs { + pgName := topology.SecurityGroupPortGroup(sgID) + if _, ok := actual.PortGroups[pgName]; !ok { + slog.Warn("reconcile/apply: skipping port SG membership — port group missing in OVN", + "port", portName, "sg", sgID, "pg", pgName) + continue + } + desiredPGs = append(desiredPGs, pgName) + } + + if _, err := r.ovn.GetLogicalSwitchPort(ctx, portName); err != nil { + addrStr := spec.MAC.String() + " " + spec.PrivateIP.String() + lsp := &nbdb.LogicalSwitchPort{ + Name: portName, + Addresses: []string{addrStr}, + PortSecurity: []string{addrStr}, + ExternalIDs: map[string]string{ + "spinifex:eni_id": portID, + "spinifex:subnet_id": spec.SubnetID, + "spinifex:vpc_id": spec.VPCID, + }, + } + if dhcpOpts, derr := r.ovn.FindDHCPOptionsByExternalID(ctx, "spinifex:subnet_id", spec.SubnetID); derr == nil && dhcpOpts != nil { + lsp.DHCPv4Options = &dhcpOpts.UUID + } + if err := r.ovn.CreateLogicalSwitchPortInGroups(ctx, switchName, lsp, desiredPGs); err != nil { + slog.Error("reconcile/apply: create ENI port failed", "port", portName, "err", err) + } + continue + } + + currentPGs, err := r.ovn.ListPortGroupsForPort(ctx, portName) + if err != nil { + slog.Warn("reconcile/apply: list port groups for port failed", "port", portName, "err", err) + continue + } + addPGs, removePGs := diffSets(desiredPGs, currentPGs) + if len(addPGs) == 0 && len(removePGs) == 0 { + continue + } + if err := r.ovn.UpdatePortGroupMemberships(ctx, portName, addPGs, removePGs); err != nil { + slog.Warn("reconcile/apply: update port group memberships failed", "port", portName, "err", err) + } + } +} + +// applyIGWs rebinds Gateway_Chassis on every intent IGW whose OVN topology +// already exists. Fresh-IGW creation flows through the NATS handler path +// (handlers/ec2/igw → vpc.igw-attach event → TopologyHandler.reconcileIGW) +// for the duration of Phase 2 — that path retains DHCP-uplink awareness +// via dhcpManager, which network/external.NewIGWManager does not yet have +// (bead mulga-siv-125.3.3 will remove the DHCP manager entirely; Phase 4 +// then migrates the handler onto external.IGWManager). +// +// Chassis-rebind drift handling stays in the reconciler because it's a +// pure NB-DB-only fix-up: a system-id change across reboot leaves +// Gateway_Chassis rows pointing at a chassis no ovn-controller owns +// (mulga-999), and SetGatewayChassis is idempotent. +// +// The igw manager field stays wired (config requires it) so this stage +// can grow into a full AttachIGW path once DHCP awareness lands in +// network/external. +func (r *reconciler) applyIGWs(ctx context.Context, intent IntentState, actual ActualState) { + for vpcID := range intent.IGWs { + if _, ok := actual.ExternalSwch[vpcID]; !ok { + slog.Debug("reconcile/apply: IGW topology missing in OVN, deferring to NATS handler path", "vpc_id", vpcID) + continue + } + r.rebindGatewayChassis(ctx, vpcID) + } +} + +// rebindGatewayChassis is the chassis-drift correction step for an +// already-existing IGW gateway LRP. It calls SetGatewayChassis for every +// configured chassis; the OVN client's idempotency layer treats unchanged +// (chassis_name, priority) tuples as no-ops. Missing chassis are skipped +// at the caller level — we don't enumerate stale Gateway_Chassis rows +// here because IGWManager.AttachIGW owns that for fresh creates and the +// legacy reconcileGatewayChassis pass is going away in 2.4 when +// topology.go is split. +func (r *reconciler) rebindGatewayChassis(ctx context.Context, vpcID string) { + if len(r.chassis) == 0 { + return + } + gwPortName := topology.GatewayRouterPort(vpcID) + if _, err := r.ovn.GetLogicalRouterPort(ctx, gwPortName); err != nil { + return + } + for i, chassis := range r.chassis { + priority := max(20-(i*5), 1) + if err := r.ovn.SetGatewayChassis(ctx, gwPortName, chassis, priority); err != nil { + slog.Warn("reconcile/apply: SetGatewayChassis failed", "vpc_id", vpcID, "chassis", chassis, "err", err) + } + } +} + +// applyEIPs runs every intent EIP through NATManager.AddEIP. AddEIP is +// idempotent (stale dnat_and_snat rules on any router referencing the +// external IP are cleaned before the new rule is added), so re-running on +// a hot reconciler boot just re-asserts the existing OVN state. +func (r *reconciler) applyEIPs(ctx context.Context, intent IntentState, _ ActualState) { + for _, spec := range intent.EIPs { + if err := r.nat.AddEIP(ctx, spec); err != nil { + slog.Error("reconcile/apply: AddEIP failed", "external_ip", spec.ExternalIP, "logical_ip", spec.LogicalIP, "err", err) + } + } +} + +// applyNATGWs runs every intent NAT gateway through NATManager.AddNATGateway. +// AddNATGateway rejects duplicate (snat, SubnetCIDR) tuples; on a re-run we +// rely on the underlying OVN client's existence check rather than a Get-first +// query here. +func (r *reconciler) applyNATGWs(ctx context.Context, intent IntentState, _ ActualState) { + for _, spec := range intent.NATGWs { + if err := r.nat.AddNATGateway(ctx, spec); err != nil { + slog.Warn("reconcile/apply: AddNATGateway failed (likely already exists)", + "natgw_id", spec.NATGatewayID, "subnet_cidr", spec.SubnetCIDR, "err", err) + } + } +} + +// subnetGatewayCIDR returns the .1 host of the subnet's CIDR and the +// prefix bit count. Matches the legacy services/vpcd/topology.go's +// subnetGateway, kept local to the reconcile package so the cross-package +// dependency on services/vpcd can be dropped. +func subnetGatewayCIDR(prefix netip.Prefix) (string, int, error) { + if !prefix.IsValid() { + return "", 0, fmt.Errorf("invalid prefix") + } + addr := prefix.Masked().Addr() + if !addr.Is4() { + return "", 0, fmt.Errorf("only IPv4 supported: %s", prefix) + } + bytes := addr.As4() + bytes[3]++ + return netip.AddrFrom4(bytes).String(), prefix.Bits(), nil +} + +// diffSets returns the (desired - current) and (current - desired) sets. +// Used by applyPorts to compute the minimal port-group membership delta. +func diffSets(desired, current []string) (add, remove []string) { + desiredSet := make(map[string]struct{}, len(desired)) + for _, s := range desired { + desiredSet[s] = struct{}{} + } + currentSet := make(map[string]struct{}, len(current)) + for _, s := range current { + currentSet[s] = struct{}{} + } + for s := range desiredSet { + if _, ok := currentSet[s]; !ok { + add = append(add, s) + } + } + for s := range currentSet { + if _, ok := desiredSet[s]; !ok { + remove = append(remove, s) + } + } + return add, remove +} diff --git a/spinifex/network/reconcile/apply_test.go b/spinifex/network/reconcile/apply_test.go new file mode 100644 index 000000000..8d15677a1 --- /dev/null +++ b/spinifex/network/reconcile/apply_test.go @@ -0,0 +1,303 @@ +package reconcile + +import ( + "context" + "net" + "net/netip" + "slices" + "testing" + + "github.com/mulgadc/spinifex/spinifex/network/external" + "github.com/mulgadc/spinifex/spinifex/network/ovn/mock" + "github.com/mulgadc/spinifex/spinifex/network/policy" + "github.com/mulgadc/spinifex/spinifex/network/topology" +) + +// newTestReconciler builds a reconciler around the in-memory OVN mock. NAT +// mode defaults to distributed (physical uplink) so EIP rules carry +// LogicalPort/MAC. +func newTestReconciler(t *testing.T) (*reconciler, *mock.Client) { + t.Helper() + m := mock.New() + sg := policy.NewSecurityGroupManager(m) + nat, err := policy.NewNATManager(m, policy.NATModeDistributed) + if err != nil { + t.Fatalf("NewNATManager: %v", err) + } + routes := policy.NewRouteManager(m) + igw, err := external.NewIGWManager(external.IGWManagerConfig{ + OVN: m, + Routes: routes, + NAT: nat, + Allocator: external.LinkLocalAllocator{}, + NATMode: policy.NATModeDistributed, + }) + if err != nil { + t.Fatalf("NewIGWManager: %v", err) + } + rec, err := New(Config{ + OVN: m, SG: sg, NAT: nat, Routes: routes, IGW: igw, + LocalAZ: "us-east-1a", NodeHostname: "test-host", + }) + if err != nil { + t.Fatalf("New: %v", err) + } + r, ok := rec.(*reconciler) + if !ok { + t.Fatalf("New returned %T, want *reconciler", rec) + } + return r, m +} + +func TestReconcile_TopoOrder_VPCThenSubnetThenPort(t *testing.T) { + rec, m := newTestReconciler(t) + ctx := context.Background() + + intent := freshIntent(t) + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + if _, ok := m.Routers["vpc-"+intent.VPCs["vpc-a"].VPCID]; !ok { + t.Errorf("VPC router not created") + } + if _, ok := m.Switches["subnet-"+intent.Subnets["subnet-a"].SubnetID]; !ok { + t.Errorf("subnet switch not created") + } + if _, ok := m.Ports["port-"+intent.Ports["eni-a"].PortID]; !ok { + t.Errorf("ENI port not created") + } + if _, ok := m.PortGroups[topology.SecurityGroupPortGroup("sg-a")]; !ok { + t.Errorf("SG port group not created") + } +} + +func TestReconcile_PortJoinsPortGroupAtomically(t *testing.T) { + rec, m := newTestReconciler(t) + ctx := context.Background() + + intent := freshIntent(t) + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + pg := m.PortGroups[topology.SecurityGroupPortGroup("sg-a")] + if pg == nil { + t.Fatal("port group not present") + } + port := m.Ports["port-"+intent.Ports["eni-a"].PortID] + if port == nil { + t.Fatal("ENI port not present") + } + if !slices.Contains(pg.Ports, port.UUID) { + t.Errorf("ENI port not joined to SG port group atomically — racy gap revived") + } +} + +func TestReconcile_Idempotent(t *testing.T) { + rec, m := newTestReconciler(t) + ctx := context.Background() + + intent := freshIntent(t) + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile #1: %v", err) + } + routerCountBefore := len(m.Routers) + switchCountBefore := len(m.Switches) + portCountBefore := len(m.Ports) + pgCountBefore := len(m.PortGroups) + addACLsBefore := m.AddACLCalls + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile #2: %v", err) + } + + if len(m.Routers) != routerCountBefore { + t.Errorf("second reconcile created duplicate routers: %d → %d", routerCountBefore, len(m.Routers)) + } + if len(m.Switches) != switchCountBefore { + t.Errorf("second reconcile created duplicate switches: %d → %d", switchCountBefore, len(m.Switches)) + } + if len(m.Ports) != portCountBefore { + t.Errorf("second reconcile created duplicate ports: %d → %d", portCountBefore, len(m.Ports)) + } + if len(m.PortGroups) != pgCountBefore { + t.Errorf("second reconcile created duplicate port groups: %d → %d", pgCountBefore, len(m.PortGroups)) + } + // EnsureSG re-applies ACLs every call (replace-all semantics); both + // passes add the same 4 infra + 0 tenant ACLs, so total adds doubles. + if m.AddACLCalls < addACLsBefore { + t.Errorf("second reconcile fewer AddACL calls than first — replace-all semantics regressed") + } +} + +func TestReconcile_OrphanPortGroupRemoved(t *testing.T) { + rec, m := newTestReconciler(t) + ctx := context.Background() + + if err := m.CreatePortGroup(ctx, "sg_orphan", nil); err != nil { + t.Fatalf("seed orphan port group: %v", err) + } + + intent := IntentState{ + VPCs: map[string]topology.VPCSpec{}, + Subnets: map[string]topology.SubnetSpec{}, + Ports: map[string]topology.PortSpec{}, + SGs: map[string]policy.SGSpec{}, + IGWs: map[string]external.IGWSpec{}, + EIPs: map[string]policy.EIPSpec{}, + NATGWs: map[string]policy.NATGWSpec{}, + } + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + if _, ok := m.PortGroups["sg_orphan"]; ok { + t.Errorf("orphan port group not removed") + } +} + +func TestReconcile_ChassisRebindOnExistingIGW(t *testing.T) { + m := mock.New() + sg := policy.NewSecurityGroupManager(m) + nat, _ := policy.NewNATManager(m, policy.NATModeDistributed) + routes := policy.NewRouteManager(m) + igw, _ := external.NewIGWManager(external.IGWManagerConfig{ + OVN: m, Routes: routes, NAT: nat, + Allocator: external.LinkLocalAllocator{}, + NATMode: policy.NATModeDistributed, + }) + rec, err := New(Config{ + OVN: m, SG: sg, NAT: nat, Routes: routes, IGW: igw, + LocalAZ: "us-east-1a", NodeHostname: "test-host", + Chassis: []string{"chassis-1", "chassis-2"}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + ctx := context.Background() + + // AttachIGW creates a gateway LRP on the VPC router, so seed the VPC + // router via a VPC-only reconcile first. + seedIntent := IntentState{ + VPCs: map[string]topology.VPCSpec{ + "vpc-a": {VPCID: "vpc-a", CIDR: netip.MustParsePrefix("10.0.0.0/16"), VNI: 100}, + }, + Subnets: map[string]topology.SubnetSpec{}, + Ports: map[string]topology.PortSpec{}, + SGs: map[string]policy.SGSpec{}, + IGWs: map[string]external.IGWSpec{}, + EIPs: map[string]policy.EIPSpec{}, + NATGWs: map[string]policy.NATGWSpec{}, + } + if err := rec.Reconcile(ctx, seedIntent); err != nil { + t.Fatalf("seed VPC reconcile: %v", err) + } + // Seed an external switch + gateway LRP so the apply path takes the + // rebind branch rather than AttachIGW. + if err := igw.AttachIGW(ctx, external.IGWSpec{VPCID: "vpc-a", InternetGatewayID: "igw-a"}); err != nil { + t.Fatalf("seed AttachIGW: %v", err) + } + setCallsBefore := m.SetGatewayChassisCalls + + intent := IntentState{ + VPCs: map[string]topology.VPCSpec{}, + Subnets: map[string]topology.SubnetSpec{}, + Ports: map[string]topology.PortSpec{}, + SGs: map[string]policy.SGSpec{}, + IGWs: map[string]external.IGWSpec{"vpc-a": {VPCID: "vpc-a", InternetGatewayID: "igw-a"}}, + EIPs: map[string]policy.EIPSpec{}, + NATGWs: map[string]policy.NATGWSpec{}, + } + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + if m.SetGatewayChassisCalls <= setCallsBefore { + t.Errorf("expected SetGatewayChassis to fire on existing IGW for chassis rebind; calls before=%d after=%d", + setCallsBefore, m.SetGatewayChassisCalls) + } +} + +func TestReconcile_PortMembershipDriftCorrected(t *testing.T) { + rec, m := newTestReconciler(t) + ctx := context.Background() + + intent := freshIntent(t) + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile #1: %v", err) + } + + // Drift: add a second SG to the intent, expect membership update. + intent.SGs["sg-b"] = policy.SGSpec{GroupID: "sg-b", VPCID: "vpc-a"} + port := intent.Ports["eni-a"] + port.SGIDs = append(port.SGIDs, "sg-b") + intent.Ports["eni-a"] = port + + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile #2: %v", err) + } + + pgB := m.PortGroups[topology.SecurityGroupPortGroup("sg-b")] + if pgB == nil { + t.Fatal("new SG port group not created") + } + storedPort := m.Ports["port-"+port.PortID] + if !slices.Contains(pgB.Ports, storedPort.UUID) { + t.Errorf("ENI port not joined to new SG port group on drift") + } +} + +func TestDiffSets(t *testing.T) { + add, remove := diffSets([]string{"a", "b", "c"}, []string{"b", "c", "d"}) + if !slices.Equal(sortedCopy(add), []string{"a"}) { + t.Errorf("add = %v, want [a]", add) + } + if !slices.Equal(sortedCopy(remove), []string{"d"}) { + t.Errorf("remove = %v, want [d]", remove) + } +} + +func TestSubnetGatewayCIDR(t *testing.T) { + prefix := netip.MustParsePrefix("10.0.1.0/24") + gw, bits, err := subnetGatewayCIDR(prefix) + if err != nil { + t.Fatalf("subnetGatewayCIDR: %v", err) + } + if gw != "10.0.1.1" || bits != 24 { + t.Errorf("got (%q, %d), want (10.0.1.1, 24)", gw, bits) + } +} + +func freshIntent(t *testing.T) IntentState { + t.Helper() + mac, _ := net.ParseMAC("02:00:00:00:00:01") + return IntentState{ + VPCs: map[string]topology.VPCSpec{ + "vpc-a": {VPCID: "vpc-a", CIDR: netip.MustParsePrefix("10.0.0.0/16"), VNI: 100}, + }, + Subnets: map[string]topology.SubnetSpec{ + "subnet-a": {SubnetID: "subnet-a", VPCID: "vpc-a", CIDR: netip.MustParsePrefix("10.0.1.0/24")}, + }, + Ports: map[string]topology.PortSpec{ + "eni-a": {PortID: "eni-a", SubnetID: "subnet-a", VPCID: "vpc-a", + PrivateIP: netip.MustParseAddr("10.0.1.10"), MAC: mac, SGIDs: []string{"sg-a"}}, + }, + SGs: map[string]policy.SGSpec{ + "sg-a": {GroupID: "sg-a", VPCID: "vpc-a"}, + }, + IGWs: map[string]external.IGWSpec{}, + EIPs: map[string]policy.EIPSpec{}, + NATGWs: map[string]policy.NATGWSpec{}, + } +} + +func sortedCopy(in []string) []string { + out := append([]string(nil), in...) + slices.Sort(out) + return out +} diff --git a/spinifex/network/reconcile/drift.go b/spinifex/network/reconcile/drift.go new file mode 100644 index 000000000..891316fcc --- /dev/null +++ b/spinifex/network/reconcile/drift.go @@ -0,0 +1,66 @@ +package reconcile + +import ( + "context" + "log/slog" + "time" + + "github.com/nats-io/nats.go" +) + +// DriftInterval is the gap between drift-detection passes. Five minutes is +// loose enough that two minutes of NATS KV silence after a successful boot +// reconcile is observable as "no drift" before the next tick; tight enough +// that a missed NATS event surfaces within the same support call. +// +// Var (not const) so integration tests can shrink it. +var DriftInterval = 5 * time.Minute + +// DriftLoop runs Reconcile every DriftInterval, gated on AcquireLeader so +// only one vpcd in the cluster scans at a time. Returns when ctx is +// cancelled. +// +// On each tick: acquire leader → load intent → reconcile → release. A +// missed acquire (another vpcd is leader, or JetStream is unreachable) is +// logged at Debug and the tick is skipped — the next tick retries. +// +// Replaces services/vpcd.ReconcileSGsLoop (30s, SG-only). The 5-minute +// interval matches parent plan §11.4. +func DriftLoop(ctx context.Context, rec Reconciler, nc *nats.Conn, localAZ, holder string) { + js, err := nc.JetStream() + if err != nil { + slog.Error("reconcile/drift: JetStream context unavailable, drift loop disabled", "err", err) + return + } + + ticker := time.NewTicker(DriftInterval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + runDriftCycle(ctx, rec, nc, js, localAZ, holder) + } + } +} + +// runDriftCycle is one tick body — split out so tests can drive it +// directly without spinning the goroutine. +func runDriftCycle(ctx context.Context, rec Reconciler, nc *nats.Conn, js nats.JetStreamContext, localAZ, holder string) { + release, elected := AcquireLeader(nc, holder) + if !elected { + return + } + defer release() + + intent, err := LoadIntentFromKV(ctx, js, localAZ) + if err != nil { + slog.Warn("reconcile/drift: load intent failed", "err", err) + return + } + if err := rec.Reconcile(ctx, intent); err != nil { + slog.Warn("reconcile/drift: reconcile failed", "err", err) + } +} diff --git a/spinifex/network/reconcile/intent.go b/spinifex/network/reconcile/intent.go new file mode 100644 index 000000000..2f45f32b8 --- /dev/null +++ b/spinifex/network/reconcile/intent.go @@ -0,0 +1,431 @@ +package reconcile + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "net" + "net/netip" + "strings" + + handlers_ec2_eip "github.com/mulgadc/spinifex/spinifex/handlers/ec2/eip" + handlers_ec2_igw "github.com/mulgadc/spinifex/spinifex/handlers/ec2/igw" + handlers_ec2_natgw "github.com/mulgadc/spinifex/spinifex/handlers/ec2/natgw" + handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" + "github.com/mulgadc/spinifex/spinifex/network/external" + "github.com/mulgadc/spinifex/spinifex/network/policy" + "github.com/mulgadc/spinifex/spinifex/network/topology" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/nats-io/nats.go" +) + +// IntentState is the desired OVN state derived from the NATS KV snapshot, +// scoped to the local AZ. Empty maps are valid and represent "no objects of +// this kind should exist locally". +type IntentState struct { + VPCs map[string]topology.VPCSpec // vpcID → spec + Subnets map[string]topology.SubnetSpec // subnetID → spec + Ports map[string]topology.PortSpec // portID (ENI ID) → spec + SGs map[string]policy.SGSpec // groupID → spec + IGWs map[string]external.IGWSpec // vpcID → spec (attached only) + EIPs map[string]policy.EIPSpec // logicalIP → spec (associated only) + NATGWs map[string]policy.NATGWSpec // natGwID → spec +} + +// LoadIntentFromKV walks every relevant KV bucket and assembles an +// IntentState scoped to localAZ. Buckets that don't yet exist (first boot, +// fresh cluster) are treated as empty rather than fatal — the legacy boot +// path tolerated this and the reconciler must as well to keep cold-start +// semantics intact. +// +// AZ filter: a VPC record matches when `vpc.AZ == "" || vpc.AZ == localAZ`. +// Subnets, ports, SGs, IGWs, EIPs, and NAT gateways inherit the filter +// transitively via their parent VPC — a child whose parent is not in scope +// is silently dropped. +func LoadIntentFromKV(ctx context.Context, js nats.JetStreamContext, localAZ string) (IntentState, error) { + intent := IntentState{ + VPCs: make(map[string]topology.VPCSpec), + Subnets: make(map[string]topology.SubnetSpec), + Ports: make(map[string]topology.PortSpec), + SGs: make(map[string]policy.SGSpec), + IGWs: make(map[string]external.IGWSpec), + EIPs: make(map[string]policy.EIPSpec), + NATGWs: make(map[string]policy.NATGWSpec), + } + + localVPCs, err := loadVPCs(js, localAZ, intent.VPCs) + if err != nil { + return IntentState{}, err + } + if err := loadSubnets(js, localVPCs, intent.Subnets); err != nil { + return IntentState{}, err + } + if err := loadSGs(js, localVPCs, intent.SGs); err != nil { + return IntentState{}, err + } + if err := loadPorts(js, localVPCs, intent.Ports); err != nil { + return IntentState{}, err + } + if err := loadIGWs(js, localVPCs, intent.IGWs); err != nil { + return IntentState{}, err + } + if err := loadEIPs(js, localVPCs, intent.EIPs); err != nil { + return IntentState{}, err + } + if err := loadNATGWs(js, localVPCs, intent.Subnets, intent.NATGWs); err != nil { + return IntentState{}, err + } + + _ = ctx // reserved for future cancellation in long KV walks + return intent, nil +} + +// matchesLocalAZ implements the §11.1 backwards-compat rule: empty AZ on a +// VPC record is treated as local (legacy pre-Phase-2.2 records). Hoisted +// out so tests can assert the rule directly. +func matchesLocalAZ(vpcAZ, localAZ string) bool { + return vpcAZ == "" || vpcAZ == localAZ +} + +// keyIsVersion skips the per-bucket version-marker key. Mirrors what every +// other KV consumer in the tree does (services/vpcd/reconcile.go etc). +func keyIsVersion(key string) bool { return key == utils.VersionKey } + +func loadVPCs(js nats.JetStreamContext, localAZ string, out map[string]topology.VPCSpec) (map[string]struct{}, error) { + localVPCs := make(map[string]struct{}) + + kv, err := js.KeyValue(handlers_ec2_vpc.KVBucketVPCs) + if err != nil { + slog.Debug("reconcile/intent: VPC bucket not available, skipping", "err", err) + return localVPCs, nil + } + keys, err := kv.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return localVPCs, nil + } + return nil, fmt.Errorf("list VPC keys: %w", err) + } + for _, key := range keys { + if keyIsVersion(key) { + continue + } + entry, err := kv.Get(key) + if err != nil { + slog.Warn("reconcile/intent: VPC read failed", "key", key, "err", err) + continue + } + var rec handlers_ec2_vpc.VPCRecord + if err := json.Unmarshal(entry.Value(), &rec); err != nil { + slog.Warn("reconcile/intent: VPC unmarshal failed", "key", key, "err", err) + continue + } + if !matchesLocalAZ(rec.AZ, localAZ) { + continue + } + prefix, err := netip.ParsePrefix(rec.CidrBlock) + if err != nil { + slog.Warn("reconcile/intent: VPC CIDR parse failed", "vpc_id", rec.VpcId, "cidr", rec.CidrBlock, "err", err) + continue + } + out[rec.VpcId] = topology.VPCSpec{ + VPCID: rec.VpcId, + CIDR: prefix, + VNI: rec.VNI, + } + localVPCs[rec.VpcId] = struct{}{} + } + return localVPCs, nil +} + +func loadSubnets(js nats.JetStreamContext, localVPCs map[string]struct{}, out map[string]topology.SubnetSpec) error { + kv, err := js.KeyValue(handlers_ec2_vpc.KVBucketSubnets) + if err != nil { + slog.Debug("reconcile/intent: subnet bucket not available, skipping", "err", err) + return nil + } + keys, err := kv.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return nil + } + return fmt.Errorf("list subnet keys: %w", err) + } + for _, key := range keys { + if keyIsVersion(key) { + continue + } + entry, err := kv.Get(key) + if err != nil { + slog.Warn("reconcile/intent: subnet read failed", "key", key, "err", err) + continue + } + var rec handlers_ec2_vpc.SubnetRecord + if err := json.Unmarshal(entry.Value(), &rec); err != nil { + slog.Warn("reconcile/intent: subnet unmarshal failed", "key", key, "err", err) + continue + } + if _, ok := localVPCs[rec.VpcId]; !ok { + continue + } + prefix, err := netip.ParsePrefix(rec.CidrBlock) + if err != nil { + slog.Warn("reconcile/intent: subnet CIDR parse failed", "subnet_id", rec.SubnetId, "cidr", rec.CidrBlock, "err", err) + continue + } + out[rec.SubnetId] = topology.SubnetSpec{ + SubnetID: rec.SubnetId, + VPCID: rec.VpcId, + CIDR: prefix, + } + } + return nil +} + +func loadSGs(js nats.JetStreamContext, localVPCs map[string]struct{}, out map[string]policy.SGSpec) error { + kv, err := js.KeyValue(handlers_ec2_vpc.KVBucketSecurityGroups) + if err != nil { + slog.Debug("reconcile/intent: SG bucket not available, skipping", "err", err) + return nil + } + keys, err := kv.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return nil + } + return fmt.Errorf("list SG keys: %w", err) + } + for _, key := range keys { + if keyIsVersion(key) { + continue + } + entry, err := kv.Get(key) + if err != nil { + slog.Warn("reconcile/intent: SG read failed", "key", key, "err", err) + continue + } + var rec handlers_ec2_vpc.SecurityGroupRecord + if err := json.Unmarshal(entry.Value(), &rec); err != nil { + slog.Warn("reconcile/intent: SG unmarshal failed", "key", key, "err", err) + continue + } + if _, ok := localVPCs[rec.VpcId]; !ok { + continue + } + out[rec.GroupId] = policy.SGSpec{ + GroupID: rec.GroupId, + VPCID: rec.VpcId, + IngressRules: sgRulesToPolicyRules(rec.IngressRules), + EgressRules: sgRulesToPolicyRules(rec.EgressRules), + } + } + return nil +} + +func loadPorts(js nats.JetStreamContext, localVPCs map[string]struct{}, out map[string]topology.PortSpec) error { + kv, err := js.KeyValue(handlers_ec2_vpc.KVBucketENIs) + if err != nil { + slog.Debug("reconcile/intent: ENI bucket not available, skipping", "err", err) + return nil + } + keys, err := kv.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return nil + } + return fmt.Errorf("list ENI keys: %w", err) + } + for _, key := range keys { + if keyIsVersion(key) { + continue + } + entry, err := kv.Get(key) + if err != nil { + slog.Warn("reconcile/intent: ENI read failed", "key", key, "err", err) + continue + } + var rec handlers_ec2_vpc.ENIRecord + if err := json.Unmarshal(entry.Value(), &rec); err != nil { + slog.Warn("reconcile/intent: ENI unmarshal failed", "key", key, "err", err) + continue + } + if _, ok := localVPCs[rec.VpcId]; !ok { + continue + } + addr, err := netip.ParseAddr(rec.PrivateIpAddress) + if err != nil { + slog.Warn("reconcile/intent: ENI IP parse failed", "eni", rec.NetworkInterfaceId, "ip", rec.PrivateIpAddress, "err", err) + continue + } + mac, err := net.ParseMAC(rec.MacAddress) + if err != nil { + slog.Warn("reconcile/intent: ENI MAC parse failed", "eni", rec.NetworkInterfaceId, "mac", rec.MacAddress, "err", err) + continue + } + out[rec.NetworkInterfaceId] = topology.PortSpec{ + PortID: rec.NetworkInterfaceId, + SubnetID: rec.SubnetId, + VPCID: rec.VpcId, + PrivateIP: addr, + MAC: mac, + SGIDs: append([]string(nil), rec.SecurityGroupIds...), + } + } + return nil +} + +func loadIGWs(js nats.JetStreamContext, localVPCs map[string]struct{}, out map[string]external.IGWSpec) error { + kv, err := js.KeyValue(handlers_ec2_igw.KVBucketIGW) + if err != nil { + slog.Debug("reconcile/intent: IGW bucket not available, skipping", "err", err) + return nil + } + keys, err := kv.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return nil + } + return fmt.Errorf("list IGW keys: %w", err) + } + for _, key := range keys { + if keyIsVersion(key) { + continue + } + entry, err := kv.Get(key) + if err != nil { + slog.Warn("reconcile/intent: IGW read failed", "key", key, "err", err) + continue + } + var rec handlers_ec2_igw.IGWRecord + if err := json.Unmarshal(entry.Value(), &rec); err != nil { + slog.Warn("reconcile/intent: IGW unmarshal failed", "key", key, "err", err) + continue + } + if rec.VpcId == "" || !strings.EqualFold(rec.State, "attached") { + continue + } + if _, ok := localVPCs[rec.VpcId]; !ok { + continue + } + out[rec.VpcId] = external.IGWSpec{ + VPCID: rec.VpcId, + InternetGatewayID: rec.InternetGatewayId, + } + } + return nil +} + +func loadEIPs(js nats.JetStreamContext, localVPCs map[string]struct{}, out map[string]policy.EIPSpec) error { + kv, err := js.KeyValue(handlers_ec2_eip.KVBucketEIPs) + if err != nil { + slog.Debug("reconcile/intent: EIP bucket not available, skipping", "err", err) + return nil + } + keys, err := kv.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return nil + } + return fmt.Errorf("list EIP keys: %w", err) + } + for _, key := range keys { + if keyIsVersion(key) { + continue + } + entry, err := kv.Get(key) + if err != nil { + slog.Warn("reconcile/intent: EIP read failed", "key", key, "err", err) + continue + } + var rec handlers_ec2_eip.EIPRecord + if err := json.Unmarshal(entry.Value(), &rec); err != nil { + slog.Warn("reconcile/intent: EIP unmarshal failed", "key", key, "err", err) + continue + } + if !strings.EqualFold(rec.State, "associated") || rec.VpcId == "" || rec.PublicIp == "" || rec.PrivateIp == "" { + continue + } + if _, ok := localVPCs[rec.VpcId]; !ok { + continue + } + spec := policy.EIPSpec{ + VPCID: rec.VpcId, + ExternalIP: rec.PublicIp, + LogicalIP: rec.PrivateIp, + } + if rec.ENIId != "" { + spec.PortName = topology.Port(rec.ENIId) + } + out[rec.PrivateIp] = spec + } + return nil +} + +func loadNATGWs(js nats.JetStreamContext, localVPCs map[string]struct{}, subnets map[string]topology.SubnetSpec, out map[string]policy.NATGWSpec) error { + kv, err := js.KeyValue(handlers_ec2_natgw.KVBucketNatGateways) + if err != nil { + slog.Debug("reconcile/intent: NAT GW bucket not available, skipping", "err", err) + return nil + } + keys, err := kv.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return nil + } + return fmt.Errorf("list NAT GW keys: %w", err) + } + for _, key := range keys { + if keyIsVersion(key) { + continue + } + entry, err := kv.Get(key) + if err != nil { + slog.Warn("reconcile/intent: NAT GW read failed", "key", key, "err", err) + continue + } + var rec handlers_ec2_natgw.NatGatewayRecord + if err := json.Unmarshal(entry.Value(), &rec); err != nil { + slog.Warn("reconcile/intent: NAT GW unmarshal failed", "key", key, "err", err) + continue + } + if !strings.EqualFold(rec.State, "available") || rec.VpcId == "" || rec.PublicIp == "" || rec.SubnetId == "" { + continue + } + if _, ok := localVPCs[rec.VpcId]; !ok { + continue + } + subnet, ok := subnets[rec.SubnetId] + if !ok { + slog.Warn("reconcile/intent: NAT GW subnet not in intent, skipping", "natgw_id", rec.NatGatewayId, "subnet_id", rec.SubnetId) + continue + } + out[rec.NatGatewayId] = policy.NATGWSpec{ + VPCID: rec.VpcId, + NATGatewayID: rec.NatGatewayId, + PublicIP: rec.PublicIp, + SubnetCIDR: subnet.CIDR.String(), + } + } + return nil +} + +// sgRulesToPolicyRules adapts the handler-side SGRule slice (IpProtocol, +// CidrIp) to the policy-layer Rule shape (IPProtocol, CIDR). +func sgRulesToPolicyRules(in []handlers_ec2_vpc.SGRule) []policy.Rule { + if len(in) == 0 { + return nil + } + out := make([]policy.Rule, len(in)) + for i, r := range in { + out[i] = policy.Rule{ + IPProtocol: r.IpProtocol, + FromPort: r.FromPort, + ToPort: r.ToPort, + CIDR: r.CidrIp, + SourceSG: r.SourceSG, + } + } + return out +} diff --git a/spinifex/network/reconcile/intent_test.go b/spinifex/network/reconcile/intent_test.go new file mode 100644 index 000000000..f9acae49e --- /dev/null +++ b/spinifex/network/reconcile/intent_test.go @@ -0,0 +1,181 @@ +package reconcile + +import ( + "context" + "encoding/json" + "testing" + "time" + + handlers_ec2_eip "github.com/mulgadc/spinifex/spinifex/handlers/ec2/eip" + handlers_ec2_igw "github.com/mulgadc/spinifex/spinifex/handlers/ec2/igw" + handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" + "github.com/mulgadc/spinifex/spinifex/testutil" +) + +func TestMatchesLocalAZ(t *testing.T) { + cases := []struct { + vpcAZ, localAZ string + want bool + }{ + {"", "us-east-1a", true}, // legacy record matches every AZ + {"us-east-1a", "us-east-1a", true}, + {"us-east-1b", "us-east-1a", false}, + {"us-east-1a", "", false}, // non-legacy vpc AZ does not match empty local AZ + } + for _, c := range cases { + if got := matchesLocalAZ(c.vpcAZ, c.localAZ); got != c.want { + t.Errorf("matchesLocalAZ(%q, %q) = %v, want %v", c.vpcAZ, c.localAZ, got, c.want) + } + } +} + +func TestLoadIntentFromKV_AZFilter(t *testing.T) { + _, _, js := testutil.StartTestJetStream(t) + + localVPC := handlers_ec2_vpc.VPCRecord{ + VpcId: "vpc-local", CidrBlock: "10.0.0.0/16", State: "available", VNI: 100, AZ: "us-east-1a", CreatedAt: time.Now(), + } + foreignVPC := handlers_ec2_vpc.VPCRecord{ + VpcId: "vpc-foreign", CidrBlock: "10.1.0.0/16", State: "available", VNI: 101, AZ: "us-east-1b", CreatedAt: time.Now(), + } + legacyVPC := handlers_ec2_vpc.VPCRecord{ + VpcId: "vpc-legacy", CidrBlock: "10.2.0.0/16", State: "available", VNI: 102, CreatedAt: time.Now(), + } + testutil.SeedKV(t, js, handlers_ec2_vpc.KVBucketVPCs, map[string][]byte{ + "acct/" + localVPC.VpcId: mustJSON(t, localVPC), + "acct/" + foreignVPC.VpcId: mustJSON(t, foreignVPC), + "acct/" + legacyVPC.VpcId: mustJSON(t, legacyVPC), + }) + + intent, err := LoadIntentFromKV(context.Background(), js, "us-east-1a") + if err != nil { + t.Fatalf("LoadIntentFromKV: %v", err) + } + + if _, ok := intent.VPCs["vpc-local"]; !ok { + t.Errorf("local VPC missing from intent") + } + if _, ok := intent.VPCs["vpc-legacy"]; !ok { + t.Errorf("legacy (empty-AZ) VPC missing from intent — backwards-compat rule broken") + } + if _, ok := intent.VPCs["vpc-foreign"]; ok { + t.Errorf("foreign-AZ VPC leaked into intent") + } +} + +func TestLoadIntentFromKV_TransitiveSubnetFilter(t *testing.T) { + _, _, js := testutil.StartTestJetStream(t) + + testutil.SeedKV(t, js, handlers_ec2_vpc.KVBucketVPCs, map[string][]byte{ + "acct/vpc-local": mustJSON(t, handlers_ec2_vpc.VPCRecord{ + VpcId: "vpc-local", CidrBlock: "10.0.0.0/16", AZ: "us-east-1a", CreatedAt: time.Now(), + }), + "acct/vpc-foreign": mustJSON(t, handlers_ec2_vpc.VPCRecord{ + VpcId: "vpc-foreign", CidrBlock: "10.1.0.0/16", AZ: "us-east-1b", CreatedAt: time.Now(), + }), + }) + + testutil.SeedKV(t, js, handlers_ec2_vpc.KVBucketSubnets, map[string][]byte{ + "acct/subnet-local": mustJSON(t, handlers_ec2_vpc.SubnetRecord{ + SubnetId: "subnet-local", VpcId: "vpc-local", CidrBlock: "10.0.1.0/24", + }), + "acct/subnet-foreign": mustJSON(t, handlers_ec2_vpc.SubnetRecord{ + SubnetId: "subnet-foreign", VpcId: "vpc-foreign", CidrBlock: "10.1.1.0/24", + }), + }) + + intent, err := LoadIntentFromKV(context.Background(), js, "us-east-1a") + if err != nil { + t.Fatalf("LoadIntentFromKV: %v", err) + } + + if _, ok := intent.Subnets["subnet-local"]; !ok { + t.Errorf("local subnet missing from intent") + } + if _, ok := intent.Subnets["subnet-foreign"]; ok { + t.Errorf("foreign-AZ subnet leaked into intent — transitive filter broken") + } +} + +func TestLoadIntentFromKV_EIPStateFilter(t *testing.T) { + _, _, js := testutil.StartTestJetStream(t) + + testutil.SeedKV(t, js, handlers_ec2_vpc.KVBucketVPCs, map[string][]byte{ + "acct/vpc-local": mustJSON(t, handlers_ec2_vpc.VPCRecord{ + VpcId: "vpc-local", CidrBlock: "10.0.0.0/16", AZ: "us-east-1a", + }), + }) + testutil.SeedKV(t, js, handlers_ec2_eip.KVBucketEIPs, map[string][]byte{ + "acct/eipassoc-a": mustJSON(t, handlers_ec2_eip.EIPRecord{ + AllocationId: "eipalloc-a", PublicIp: "203.0.113.10", PrivateIp: "10.0.1.5", + VpcId: "vpc-local", State: "associated", + }), + "acct/eipassoc-b": mustJSON(t, handlers_ec2_eip.EIPRecord{ + AllocationId: "eipalloc-b", PublicIp: "203.0.113.11", PrivateIp: "10.0.1.6", + VpcId: "vpc-local", State: "allocated", // not associated → excluded + }), + }) + + intent, err := LoadIntentFromKV(context.Background(), js, "us-east-1a") + if err != nil { + t.Fatalf("LoadIntentFromKV: %v", err) + } + + if _, ok := intent.EIPs["10.0.1.5"]; !ok { + t.Errorf("associated EIP missing from intent") + } + if _, ok := intent.EIPs["10.0.1.6"]; ok { + t.Errorf("non-associated EIP leaked into intent") + } +} + +func TestLoadIntentFromKV_IGWAttachedFilter(t *testing.T) { + _, _, js := testutil.StartTestJetStream(t) + + testutil.SeedKV(t, js, handlers_ec2_vpc.KVBucketVPCs, map[string][]byte{ + "acct/vpc-local": mustJSON(t, handlers_ec2_vpc.VPCRecord{ + VpcId: "vpc-local", CidrBlock: "10.0.0.0/16", AZ: "us-east-1a", + }), + }) + testutil.SeedKV(t, js, handlers_ec2_igw.KVBucketIGW, map[string][]byte{ + "acct/igw-attached": mustJSON(t, handlers_ec2_igw.IGWRecord{ + InternetGatewayId: "igw-attached", VpcId: "vpc-local", State: "attached", + }), + "acct/igw-detached": mustJSON(t, handlers_ec2_igw.IGWRecord{ + InternetGatewayId: "igw-detached", VpcId: "", State: "available", + }), + }) + + intent, err := LoadIntentFromKV(context.Background(), js, "us-east-1a") + if err != nil { + t.Fatalf("LoadIntentFromKV: %v", err) + } + + if _, ok := intent.IGWs["vpc-local"]; !ok { + t.Errorf("attached IGW missing from intent") + } + if len(intent.IGWs) != 1 { + t.Errorf("got %d IGWs, want 1 (detached should be excluded)", len(intent.IGWs)) + } +} + +func TestLoadIntentFromKV_NoBucketsTolerated(t *testing.T) { + _, _, js := testutil.StartTestJetStream(t) + + intent, err := LoadIntentFromKV(context.Background(), js, "us-east-1a") + if err != nil { + t.Fatalf("LoadIntentFromKV on empty cluster: %v", err) + } + if len(intent.VPCs)+len(intent.Subnets)+len(intent.Ports)+len(intent.SGs)+len(intent.IGWs)+len(intent.EIPs)+len(intent.NATGWs) != 0 { + t.Errorf("expected empty intent on fresh cluster, got %#v", intent) + } +} + +func mustJSON(t *testing.T, v any) []byte { + t.Helper() + b, err := json.Marshal(v) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return b +} diff --git a/spinifex/network/reconcile/lock.go b/spinifex/network/reconcile/lock.go new file mode 100644 index 000000000..1e12413fd --- /dev/null +++ b/spinifex/network/reconcile/lock.go @@ -0,0 +1,74 @@ +package reconcile + +import ( + "log/slog" + "time" + + "github.com/nats-io/nats.go" +) + +// reconcileLeaderBucket holds a single CAS-elected leader key. MaxAge bounds +// recovery time when an elected vpcd crashes before releasing the lock. +const ( + reconcileLeaderBucket = "spinifex-vpcd-reconcile" + reconcileLeaderKey = "leader" + reconcileLeaderTTL = 60 * time.Second +) + +// Bounded wait for JetStream to come up at vpcd startup. On a cold multi-node +// start NATS clustering takes a few seconds and js.CreateKeyValue returns +// "context deadline exceeded" until quorum forms. Retrying makes the election +// deterministic; without it every vpcd falls through and races the same OVN +// NB transactions (mulga-js-72). Vars (not consts) so tests can shrink them. +var ( + leaderRetryFor = 60 * time.Second + leaderRetryStep = 1 * time.Second +) + +// AcquireLeader returns release+true exactly once across all vpcds in a +// cluster, gating the startup Reconcile pass. Other vpcds get release=nil, +// elected=false and skip reconcile. Bounded retry on JetStream-not-ready +// keeps cold-multi-node-boot deterministic; on exhaustion we return +// elected=false so a follow-up retry (systemd Restart=on-failure) can run. +// +// Exported with the same contract as the legacy +// services/vpcd.AcquireReconcileLeader — call sites in services/vpcd/vpcd.go +// switch to this package by changing the import path only. +func AcquireLeader(nc *nats.Conn, holder string) (func(), bool) { + js, _ := nc.JetStream() + + var ( + kv nats.KeyValue + err error + ) + deadline := time.Now().Add(leaderRetryFor) + for { + kv, err = js.CreateKeyValue(&nats.KeyValueConfig{ + Bucket: reconcileLeaderBucket, + History: 1, + TTL: reconcileLeaderTTL, + }) + if err == nil { + break + } + if time.Now().After(deadline) { + slog.Error("reconcile/lock: JetStream KV unreachable after retry, skipping reconcile", + "holder", holder, "waited", leaderRetryFor, "err", err) + return nil, false + } + slog.Debug("reconcile/lock: JetStream KV not ready, retrying", "holder", holder, "err", err) + time.Sleep(leaderRetryStep) + } + + if _, err := kv.Create(reconcileLeaderKey, []byte(holder)); err != nil { + slog.Info("reconcile/lock: another vpcd is leader, skipping reconcile", "holder", holder, "err", err) + return nil, false + } + + slog.Info("reconcile/lock: elected", "holder", holder) + return func() { + if err := kv.Delete(reconcileLeaderKey); err != nil { + slog.Warn("reconcile/lock: failed to release lock (TTL will reap)", "holder", holder, "err", err) + } + }, true +} diff --git a/spinifex/network/reconcile/reconcile.go b/spinifex/network/reconcile/reconcile.go new file mode 100644 index 000000000..9498bd19b --- /dev/null +++ b/spinifex/network/reconcile/reconcile.go @@ -0,0 +1,143 @@ +// Package reconcile is the spinifex network stack's intent-actual +// reconciliation layer. It reads the canonical desired state from the NATS +// JetStream KV snapshot, scopes it to the local availability zone, and +// applies the diff against OVN NB DB in a single topologically-sorted pass +// (VPC → Subnet → Port → SG ACLs → IGW → EIP/NATGW). Deletes run in reverse +// order. +// +// The reconciler replaces the legacy five-pass startup sequence +// (`Reconcile` + `ReconcileFromKV` + `ReconcileSGsOnce` + +// `RetrofitAllExternalLocalnetOptions` + `RetrofitAllGatewayPortNetworks`) +// in `services/vpcd`. A 5-minute drift goroutine drives the same call +// periodically, replacing the 30-second `ReconcileSGsLoop`. +// +// Multi-AZ scoping: `IntentState` is built by `LoadIntentFromKV(ctx, js, +// localAZ)`. The filter rule is `vpc.AZ == "" || vpc.AZ == localAZ` — +// records without an `AZ` field are legacy (pre-Phase 2.2) and are treated +// as local. New VPCs are stamped with the local AZ at create time by the +// `handlers/ec2/vpc.CreateVpc` path. +// +// See docs/development/feature/spinifex-network-redesign.md §11 and +// docs/development/feature/spinifex-network-redesign-phase2.md §2.2 for the +// authoritative contract. +package reconcile + +import ( + "context" + "errors" + "fmt" + "log/slog" + + "github.com/mulgadc/spinifex/spinifex/network/external" + "github.com/mulgadc/spinifex/spinifex/network/ovn" + "github.com/mulgadc/spinifex/spinifex/network/policy" +) + +// Reconciler converges OVN NB DB to a declared IntentState. Implementations +// are idempotent: a second call with the same IntentState is a no-op. +type Reconciler interface { + Reconcile(ctx context.Context, intent IntentState) error +} + +// Config is the construction-time bag for the reconciler. Every field +// except Chassis is required. +type Config struct { + OVN ovn.Client + SG policy.SecurityGroupManager + NAT policy.NATManager + Routes policy.RouteManager + IGW external.IGWManager + LocalAZ string + // NodeHostname is the holder identity stamped on the leader-election + // CAS row. Set to os.Hostname() in production. + NodeHostname string + // Chassis is the SBDB-discovered chassis list, used for gateway LRP + // chassis rebinding during drift. Same value passed to TopologyHandler + // in the legacy boot path. + Chassis []string +} + +type reconciler struct { + ovn ovn.Client + sg policy.SecurityGroupManager + nat policy.NATManager + routes policy.RouteManager + igw external.IGWManager + localAZ string + host string + chassis []string +} + +var _ Reconciler = (*reconciler)(nil) + +// New constructs a Reconciler from cfg. Returns an error when any required +// field is missing. +func New(cfg Config) (Reconciler, error) { + switch { + case cfg.OVN == nil: + return nil, errors.New("reconcile: OVN client required") + case cfg.SG == nil: + return nil, errors.New("reconcile: SecurityGroupManager required") + case cfg.NAT == nil: + return nil, errors.New("reconcile: NATManager required") + case cfg.Routes == nil: + return nil, errors.New("reconcile: RouteManager required") + case cfg.IGW == nil: + return nil, errors.New("reconcile: IGWManager required") + case cfg.NodeHostname == "": + return nil, errors.New("reconcile: NodeHostname required") + } + return &reconciler{ + ovn: cfg.OVN, + sg: cfg.SG, + nat: cfg.NAT, + routes: cfg.Routes, + igw: cfg.IGW, + localAZ: cfg.LocalAZ, + host: cfg.NodeHostname, + chassis: cfg.Chassis, + }, nil +} + +// Reconcile snapshots OVN actual state, computes the (intent - actual) and +// (actual - intent) deltas per resource type, and applies them in +// topologically-sorted order: VPC → Subnet → SG (port-group + ACLs) → +// Port → IGW → EIP → NATGW. Deletes run reverse: NATGW → EIP → IGW → Port +// → SG → Subnet → VPC. +// +// SG comes before Port so that ENI LSPs can be created with their port-group +// memberships atomically (CreateLogicalSwitchPortInGroups) — the racy +// create-then-join used by the legacy 5-pass startup is the exact gap this +// reconciler kills. +// +// A per-stage error is logged and the stage continues with the next item; +// the function only returns an error when the actual-state scan fails. This +// matches the legacy 5-pass behaviour: partial progress beats no progress. +func (r *reconciler) Reconcile(ctx context.Context, intent IntentState) error { + actual, err := scanActual(ctx, r.ovn) + if err != nil { + return fmt.Errorf("scan actual OVN state: %w", err) + } + + slog.Info("reconcile: starting", + "local_az", r.localAZ, + "intent_vpcs", len(intent.VPCs), + "intent_subnets", len(intent.Subnets), + "intent_ports", len(intent.Ports), + "intent_sgs", len(intent.SGs), + "intent_igws", len(intent.IGWs), + "intent_eips", len(intent.EIPs), + "intent_natgws", len(intent.NATGWs), + ) + + r.applyVPCs(ctx, intent, actual) + r.applySubnets(ctx, intent, actual) + r.applySGs(ctx, intent, actual) + r.applyPorts(ctx, intent, actual) + r.applyIGWs(ctx, intent, actual) + r.applyEIPs(ctx, intent, actual) + r.applyNATGWs(ctx, intent, actual) + + slog.Info("reconcile: complete") + return nil +} diff --git a/spinifex/services/vpcd/reconcile.go b/spinifex/services/vpcd/reconcile.go deleted file mode 100644 index b1618cde1..000000000 --- a/spinifex/services/vpcd/reconcile.go +++ /dev/null @@ -1,451 +0,0 @@ -package vpcd - -import ( - "context" - "encoding/json" - "errors" - "log/slog" - "time" - - handlers_ec2_eip "github.com/mulgadc/spinifex/spinifex/handlers/ec2/eip" - handlers_ec2_igw "github.com/mulgadc/spinifex/spinifex/handlers/ec2/igw" - handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" - "github.com/mulgadc/spinifex/spinifex/utils" - "github.com/nats-io/nats.go" -) - -// reconcileLeaderBucket holds a single CAS-elected leader key. MaxAge bounds -// recovery time when an elected vpcd crashes before releasing the lock. -const ( - reconcileLeaderBucket = "spinifex-vpcd-reconcile" - reconcileLeaderKey = "leader" - reconcileLeaderTTL = 60 * time.Second -) - -// Bounded wait for JetStream to come up at vpcd startup. On a cold multi-node -// start NATS clustering takes a few seconds and js.CreateKeyValue returns -// "context deadline exceeded" until quorum forms. Retrying makes the election -// deterministic; without it every vpcd falls through and races the same OVN -// NB transactions (mulga-js-72). Vars (not consts) so tests can shrink them. -var ( - reconcileLeaderRetryFor = 60 * time.Second - reconcileLeaderRetryStep = 1 * time.Second -) - -// AcquireReconcileLeader returns release+true exactly once across all vpcds in -// a cluster, gating the startup Reconcile/ReconcileFromKV passes. Other vpcds -// get release=nil, elected=false and skip reconcile (runtime VPC events use the -// vpcd-workers queue group, so they remain handled cluster-wide). -// -// Bounded-retry contract: when JetStream KV is not yet reachable (cold -// multi-node start, NATS quorum still forming) we retry for -// reconcileLeaderRetryFor before giving up. On exhaustion we return -// elected=false rather than running unguarded — concurrent reconciles produce -// silent OVN NB damage (mulga-js-72). Operators can restart vpcd to retry -// reconcile once NATS recovers. -func AcquireReconcileLeader(nc *nats.Conn, holder string) (func(), bool) { - js, _ := nc.JetStream() - - var ( - kv nats.KeyValue - err error - ) - deadline := time.Now().Add(reconcileLeaderRetryFor) - for { - kv, err = js.CreateKeyValue(&nats.KeyValueConfig{ - Bucket: reconcileLeaderBucket, - History: 1, - TTL: reconcileLeaderTTL, - }) - if err == nil { - break - } - if time.Now().After(deadline) { - slog.Error("vpcd reconcile-leader: JetStream KV unreachable after retry, skipping reconcile", - "holder", holder, "waited", reconcileLeaderRetryFor, "err", err) - return nil, false - } - slog.Debug("vpcd reconcile-leader: JetStream KV not ready, retrying", - "holder", holder, "err", err) - time.Sleep(reconcileLeaderRetryStep) - } - - if _, err := kv.Create(reconcileLeaderKey, []byte(holder)); err != nil { - slog.Info("vpcd reconcile-leader: another vpcd is leader, skipping reconcile", "holder", holder, "err", err) - return nil, false - } - - slog.Info("vpcd reconcile-leader: elected", "holder", holder) - return func() { - if err := kv.Delete(reconcileLeaderKey); err != nil { - slog.Warn("vpcd reconcile-leader: failed to release lock (TTL will reap)", "holder", holder, "err", err) - } - }, true -} - -// 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 - IGWsReconciled int - PortsCreated int - NATsReconciled int -} - -// Reconcile ensures OVN topology matches the expected state from the bootstrap config. -// This runs on vpcd startup before subscribing to NATS topics. -// -// Pass 1 (bootstrap): Uses [bootstrap] from spinifex.toml to create the default VPC -// topology. This covers first-install where admin init ran before services started. -// -// All operations are idempotent — safe to call on every startup. -func Reconcile(ctx context.Context, topo *TopologyHandler, bootstrap *BootstrapVPC) ReconcileResult { - var result ReconcileResult - - if bootstrap == nil || bootstrap.VpcId == "" { - slog.Debug("vpcd reconcile: no bootstrap config, skipping") - return result - } - - slog.Info("vpcd reconcile: checking bootstrap VPC topology", - "vpc_id", bootstrap.VpcId, - "subnet_id", bootstrap.SubnetId, - ) - - // 1. Ensure VPC router exists - routerName := "vpc-" + bootstrap.VpcId - if _, err := topo.ovn.GetLogicalRouter(ctx, routerName); err != nil { - slog.Info("vpcd reconcile: creating VPC router", "router", routerName) - if err := topo.reconcileVPC(ctx, bootstrap.VpcId, bootstrap.Cidr); err != nil { - slog.Error("vpcd reconcile: failed to create VPC router", "err", err) - } else { - result.RoutersCreated++ - } - } else { - slog.Debug("vpcd reconcile: VPC router exists", "router", routerName) - } - - // 2. Ensure subnet switch + router port + DHCP exists - if bootstrap.SubnetId != "" { - switchName := "subnet-" + bootstrap.SubnetId - if _, err := topo.ovn.GetLogicalSwitch(ctx, switchName); err != nil { - slog.Info("vpcd reconcile: creating subnet topology", "switch", switchName) - if err := topo.reconcileSubnet(ctx, bootstrap.SubnetId, bootstrap.VpcId, bootstrap.SubnetCidr); err != nil { - slog.Error("vpcd reconcile: failed to create subnet topology", "err", err) - } else { - result.SwitchesCreated++ - } - } else { - slog.Debug("vpcd reconcile: subnet switch exists", "switch", switchName) - } - } - - // 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 { - result.IGWsReconciled++ - } - - slog.Info("vpcd reconcile: complete", - "routers_created", result.RoutersCreated, - "switches_created", result.SwitchesCreated, - "igws_reconciled", result.IGWsReconciled, - ) - - return result -} - -// ReconcileFromKV reads all VPCs, subnets, IGW attachments, and ENIs from NATS KV -// and ensures OVN topology matches. This is Pass 2 from the Phase 12 plan — it -// handles reboots, OVN DB loss, and any missed events. All operations are idempotent. -// -// chassisNames is the set of OVN chassis names discovered from the SBDB at -// startup; reconcileGatewayChassis uses it to delete stale Gateway_Chassis -// rows (e.g. left over from a chassis_name change across reboot) and rebind -// every gateway LRP against the live chassis (mulga-999). -func ReconcileFromKV(ctx context.Context, nc *nats.Conn, topo *TopologyHandler, chassisNames []string) ReconcileResult { - var result ReconcileResult - - // 0. Reconcile gateway_chassis bindings before any per-VPC work. State - // drift here (a gateway_chassis row pointing at a chassis that no longer - // exists) leaves the cr-gw* port unbound — centralized NAT silently - // breaks. Single SB-derived list, applied once. - if err := topo.reconcileGatewayChassis(ctx, chassisNames); err != nil { - slog.Warn("vpcd reconcile-kv: gateway_chassis reconcile failed", "err", err) - } - - js, err := nc.JetStream() - if err != nil { - slog.Warn("vpcd reconcile-kv: failed to get JetStream context", "err", err) - return result - } - - // 1. Reconcile VPCs: ensure each VPC has a logical router - vpcKV, err := js.KeyValue(handlers_ec2_vpc.KVBucketVPCs) - if err != nil { - slog.Debug("vpcd reconcile-kv: VPC KV bucket not available (first boot before daemon?)", "err", err) - return result - } - - // Build a map of VPC ID → CIDR for subnet/IGW reconciliation - type vpcInfo struct { - VpcId string - CidrBlock string - } - vpcMap := make(map[string]vpcInfo) - - vpcKeys, err := vpcKV.Keys() - if err != nil && !errors.Is(err, nats.ErrNoKeysFound) { - slog.Warn("vpcd reconcile-kv: failed to list VPC keys", "err", err) - } - for _, key := range vpcKeys { - if key == utils.VersionKey { - continue - } - entry, err := vpcKV.Get(key) - if err != nil { - continue - } - var rec handlers_ec2_vpc.VPCRecord - if err := json.Unmarshal(entry.Value(), &rec); err != nil { - slog.Warn("vpcd reconcile-kv: failed to unmarshal VPC record", "key", key, "err", err) - continue - } - vpcMap[rec.VpcId] = vpcInfo{VpcId: rec.VpcId, CidrBlock: rec.CidrBlock} - - routerName := "vpc-" + rec.VpcId - if _, err := topo.ovn.GetLogicalRouter(ctx, routerName); err != nil { - slog.Info("vpcd reconcile-kv: creating VPC router", "router", routerName) - if err := topo.reconcileVPC(ctx, rec.VpcId, rec.CidrBlock); err != nil { - slog.Error("vpcd reconcile-kv: failed to create VPC router", "err", err) - } else { - result.RoutersCreated++ - } - } - } - - // 2. Reconcile subnets: ensure each subnet has a logical switch + router port + DHCP - subnetKV, err := js.KeyValue(handlers_ec2_vpc.KVBucketSubnets) - if err != nil { - slog.Debug("vpcd reconcile-kv: subnet KV bucket not available", "err", err) - } else { - subnetKeys, err := subnetKV.Keys() - if err != nil && !errors.Is(err, nats.ErrNoKeysFound) { - slog.Warn("vpcd reconcile-kv: failed to list subnet keys", "err", err) - } - for _, key := range subnetKeys { - if key == utils.VersionKey { - continue - } - entry, err := subnetKV.Get(key) - if err != nil { - continue - } - var rec handlers_ec2_vpc.SubnetRecord - if err := json.Unmarshal(entry.Value(), &rec); err != nil { - slog.Warn("vpcd reconcile-kv: failed to unmarshal subnet record", "key", key, "err", err) - continue - } - - switchName := "subnet-" + rec.SubnetId - if _, err := topo.ovn.GetLogicalSwitch(ctx, switchName); err != nil { - slog.Info("vpcd reconcile-kv: creating subnet topology", "switch", switchName) - if err := topo.reconcileSubnet(ctx, rec.SubnetId, rec.VpcId, rec.CidrBlock); err != nil { - slog.Error("vpcd reconcile-kv: failed to create subnet topology", "err", err) - } else { - result.SwitchesCreated++ - } - } - } - } - - // 3. Reconcile IGW attachments: ensure attached IGWs have external switch + SNAT - igwKV, err := js.KeyValue(handlers_ec2_igw.KVBucketIGW) - if err != nil { - slog.Debug("vpcd reconcile-kv: IGW KV bucket not available", "err", err) - } else { - igwKeys, err := igwKV.Keys() - if err != nil && !errors.Is(err, nats.ErrNoKeysFound) { - slog.Warn("vpcd reconcile-kv: failed to list IGW keys", "err", err) - } - for _, key := range igwKeys { - if key == utils.VersionKey { - continue - } - entry, err := igwKV.Get(key) - if err != nil { - continue - } - var rec handlers_ec2_igw.IGWRecord - if err := json.Unmarshal(entry.Value(), &rec); err != nil { - slog.Warn("vpcd reconcile-kv: failed to unmarshal IGW record", "key", key, "err", err) - continue - } - // Only reconcile attached IGWs - if rec.VpcId == "" || rec.State != "attached" { - continue - } - - 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++ - } - } - } - - // 4. Reconcile ENI ports: ensure each ENI has a logical switch port. - // eniMAC is populated here and consumed by Step 5 to build distributed NAT - // rules (direct bridge mode only; centralized mode doesn't need the MAC). - eniMAC := make(map[string]string) // eniID → MAC - eniKV, err := js.KeyValue(handlers_ec2_vpc.KVBucketENIs) - if err != nil { - slog.Debug("vpcd reconcile-kv: ENI KV bucket not available", "err", err) - } else { - eniKeys, err := eniKV.Keys() - if err != nil && !errors.Is(err, nats.ErrNoKeysFound) { - slog.Warn("vpcd reconcile-kv: failed to list ENI keys", "err", err) - } - for _, key := range eniKeys { - if key == utils.VersionKey { - continue - } - entry, err := eniKV.Get(key) - if err != nil { - continue - } - var rec handlers_ec2_vpc.ENIRecord - if err := json.Unmarshal(entry.Value(), &rec); err != nil { - slog.Warn("vpcd reconcile-kv: failed to unmarshal ENI record", "key", key, "err", err) - continue - } - - eniMAC[rec.NetworkInterfaceId] = rec.MacAddress - - portName := "port-" + rec.NetworkInterfaceId - if _, err := topo.ovn.GetLogicalSwitchPort(ctx, portName); err != nil { - switchName := "subnet-" + rec.SubnetId - addrStr := rec.MacAddress + " " + rec.PrivateIpAddress - lsp := &nbdb.LogicalSwitchPort{ - Name: portName, - Addresses: []string{addrStr}, - PortSecurity: []string{addrStr}, - ExternalIDs: map[string]string{ - "spinifex:eni_id": rec.NetworkInterfaceId, - "spinifex:subnet_id": rec.SubnetId, - "spinifex:vpc_id": rec.VpcId, - }, - } - - // Attach DHCP options if available - dhcpOpts, dhcpErr := topo.ovn.FindDHCPOptionsByExternalID(ctx, "spinifex:subnet_id", rec.SubnetId) - if dhcpErr == nil { - lsp.DHCPv4Options = &dhcpOpts.UUID - } - - slog.Info("vpcd reconcile-kv: creating ENI port", "port", portName, "switch", switchName) - if err := topo.ovn.CreateLogicalSwitchPort(ctx, switchName, lsp); err != nil { - slog.Error("vpcd reconcile-kv: failed to create ENI port", "port", portName, "err", err) - } else { - result.PortsCreated++ - } - } - } - } - - // 5. Reconcile EIP NAT rules: ensure each associated EIP has a dnat_and_snat - // rule in OVN NB DB. NAT rules are only created by AssociateAddress events - // and are never otherwise reconciled — an OVN NB DB wipe or any state drift - // silently breaks inbound EIP traffic. This step is the safety net. - eipKV, err := js.KeyValue(handlers_ec2_eip.KVBucketEIPs) - if err != nil { - slog.Debug("vpcd reconcile-kv: EIP KV bucket not available", "err", err) - } else { - eipKeys, err := eipKV.Keys() - if err != nil && !errors.Is(err, nats.ErrNoKeysFound) { - slog.Warn("vpcd reconcile-kv: failed to list EIP keys", "err", err) - } - for _, key := range eipKeys { - if key == utils.VersionKey { - continue - } - entry, err := eipKV.Get(key) - if err != nil { - continue - } - var rec handlers_ec2_eip.EIPRecord - if err := json.Unmarshal(entry.Value(), &rec); err != nil { - slog.Warn("vpcd reconcile-kv: failed to unmarshal EIP record", "key", key, "err", err) - continue - } - if rec.State != "associated" || rec.VpcId == "" || rec.PublicIp == "" || rec.PrivateIp == "" { - continue - } - - existing, err := topo.ovn.FindNATByExternalIP(ctx, "dnat_and_snat", rec.PublicIp) - if err != nil { - slog.Warn("vpcd reconcile-kv: failed to query NAT rule", "external_ip", rec.PublicIp, "err", err) - continue - } - if existing != nil && existing.LogicalIP == rec.PrivateIp { - slog.Debug("vpcd reconcile-kv: NAT rule correct, skipping", "external_ip", rec.PublicIp) - continue - } - - routerName := "vpc-" + rec.VpcId - natRule := &nbdb.NAT{ - Type: "dnat_and_snat", - ExternalIP: rec.PublicIp, - LogicalIP: rec.PrivateIp, - ExternalIDs: map[string]string{ - "spinifex:vpc_id": rec.VpcId, - "spinifex:public_ip": rec.PublicIp, - }, - } - if !topo.useCentralizedNAT() && rec.ENIId != "" { - portName := "port-" + rec.ENIId - natRule.LogicalPort = &portName - if mac, ok := eniMAC[rec.ENIId]; ok && mac != "" { - natRule.ExternalMAC = &mac - } - } - - if existing != nil { - if removed, err := topo.ovn.DeleteAllNATsByExternalIP(ctx, "dnat_and_snat", rec.PublicIp); err != nil { - slog.Warn("vpcd reconcile-kv: failed to remove stale NAT rule", "external_ip", rec.PublicIp, "err", err) - continue - } else if removed > 0 { - slog.Info("vpcd reconcile-kv: removed stale NAT rule before re-add", "external_ip", rec.PublicIp, "removed", removed) - } - } - - if err := topo.ovn.AddNAT(ctx, routerName, natRule); err != nil { - slog.Error("vpcd reconcile-kv: failed to add NAT rule", "external_ip", rec.PublicIp, "logical_ip", rec.PrivateIp, "err", err) - continue - } - slog.Info("vpcd reconcile-kv: reconciled NAT rule", - "external_ip", rec.PublicIp, "logical_ip", rec.PrivateIp, "router", routerName) - result.NATsReconciled++ - } - } - - slog.Info("vpcd reconcile-kv: complete", - "routers_created", result.RoutersCreated, - "switches_created", result.SwitchesCreated, - "igws_reconciled", result.IGWsReconciled, - "ports_created", result.PortsCreated, - "nats_reconciled", result.NATsReconciled, - ) - - return result -} diff --git a/spinifex/services/vpcd/reconcile_sgs.go b/spinifex/services/vpcd/reconcile_sgs.go deleted file mode 100644 index 3566faabf..000000000 --- a/spinifex/services/vpcd/reconcile_sgs.go +++ /dev/null @@ -1,256 +0,0 @@ -package vpcd - -import ( - "context" - "encoding/json" - "errors" - "log/slog" - "os" - "strings" - "time" - - handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" - "github.com/mulgadc/spinifex/spinifex/utils" - "github.com/nats-io/nats.go" -) - -// reconcileSGsLoopInterval is how often the reconciler walks KV + OVN to -// converge drift. Chosen so a vpcd that wins AcquireReconcileLeader (60s TTL) -// typically holds the bucket key across at least one cycle. -const reconcileSGsLoopInterval = 30 * time.Second - -// SGReconcileResult tracks what each periodic pass converged. -type SGReconcileResult struct { - PortGroupsRecreated int - PortMembershipsSynced int - OrphanPortGroupsRemoved int -} - -// ReconcileSGsLoop runs the SG/ENI convergence scans every 30s, gated on -// AcquireReconcileLeader so only one vpcd in the cluster runs them at a time. -// Returns when ctx is cancelled. -func ReconcileSGsLoop(ctx context.Context, nc *nats.Conn, topo *TopologyHandler) { - holder, _ := os.Hostname() - ticker := time.NewTicker(reconcileSGsLoopInterval) - defer ticker.Stop() - - for { - release, elected := AcquireReconcileLeader(nc, holder) - if elected { - result := ReconcileSGsOnce(ctx, nc, topo) - if result.PortGroupsRecreated+result.PortMembershipsSynced+result.OrphanPortGroupsRemoved > 0 { - slog.Info("vpcd reconcile-sgs: converged drift", - "port_groups_recreated", result.PortGroupsRecreated, - "port_memberships_synced", result.PortMembershipsSynced, - "orphan_port_groups_removed", result.OrphanPortGroupsRemoved, - ) - } - release() - } - - select { - case <-ctx.Done(): - return - case <-ticker.C: - } - } -} - -// ReconcileSGsOnce runs a single pass. Exported so tests can drive it directly. -func ReconcileSGsOnce(ctx context.Context, nc *nats.Conn, topo *TopologyHandler) SGReconcileResult { - var result SGReconcileResult - if topo == nil || topo.ovn == nil { - slog.Warn("vpcd reconcile-sgs: skipped — topology handler not connected") - return result - } - - js, err := nc.JetStream() - if err != nil { - slog.Warn("vpcd reconcile-sgs: failed to get JetStream context", "err", err) - return result - } - - sgKV, err := js.KeyValue(handlers_ec2_vpc.KVBucketSecurityGroups) - if err != nil { - slog.Debug("vpcd reconcile-sgs: SG bucket not available", "err", err) - sgKV = nil - } - eniKV, err := js.KeyValue(handlers_ec2_vpc.KVBucketENIs) - if err != nil { - slog.Debug("vpcd reconcile-sgs: ENI bucket not available", "err", err) - eniKV = nil - } - - var sgs []handlers_ec2_vpc.SecurityGroupRecord - if sgKV != nil { - sgs = listSGRecords(sgKV) - } - - // SG records without an OVN port group → recreate. - if sgKV != nil { - result.PortGroupsRecreated = scanMissingPortGroups(ctx, topo, sgs) - } - - // OVN port groups with no matching SG record → tear down. Re-snapshot KV - // right before the orphan check: scanMissingPortGroups may have taken - // seconds on a large cluster, and any CreateSecurityGroup that landed in - // the meantime would otherwise look like an orphan against the stale - // snapshot and get silently deleted. - if sgKV != nil { - sgs = listSGRecords(sgKV) - } - result.OrphanPortGroupsRemoved = scanOrphanPortGroups(ctx, topo, sgs) - - // ENIs with SecurityGroupIds → reconcile membership. - if eniKV != nil { - result.PortMembershipsSynced = scanENIPortMembership(ctx, topo, eniKV) - } - - return result -} - -// listSGRecords reads every SG record from the bucket. Logs and skips -// malformed entries; the orphan-SG case can't occur on a healthy cluster. -func listSGRecords(sgKV nats.KeyValue) []handlers_ec2_vpc.SecurityGroupRecord { - keys, err := sgKV.Keys() - if err != nil && !errors.Is(err, nats.ErrNoKeysFound) { - slog.Warn("vpcd reconcile-sgs: failed to list SG keys", "err", err) - return nil - } - var out []handlers_ec2_vpc.SecurityGroupRecord - for _, key := range keys { - if key == utils.VersionKey { - continue - } - entry, err := sgKV.Get(key) - if err != nil { - slog.Warn("vpcd reconcile-sgs: SG read failed", "key", key, "err", err) - continue - } - var rec handlers_ec2_vpc.SecurityGroupRecord - if err := json.Unmarshal(entry.Value(), &rec); err != nil { - slog.Warn("vpcd reconcile-sgs: SG unmarshal failed", "key", key, "err", err) - continue - } - out = append(out, rec) - } - return out -} - -// scanMissingPortGroups recreates the OVN port group + ACLs for any SG record -// whose port group has gone missing in OVN NB. Returns the count recreated. -func scanMissingPortGroups(ctx context.Context, topo *TopologyHandler, sgs []handlers_ec2_vpc.SecurityGroupRecord) int { - recreated := 0 - for _, sg := range sgs { - pgName := portGroupName(sg.GroupId) - if _, err := topo.ovn.GetPortGroup(ctx, pgName); err == nil { - continue - } - if err := topo.provisionSG(ctx, sg.GroupId, sgRulesToACLRules(sg.IngressRules), sgRulesToACLRules(sg.EgressRules)); err != nil { - slog.Warn("vpcd reconcile-sgs: failed to recreate port group for SG", "sg", sg.GroupId, "err", err) - continue - } - slog.Info("vpcd reconcile-sgs: recreated missing port group", "sg", sg.GroupId, "pg", pgName) - recreated++ - } - return recreated -} - -// scanENIPortMembership runs reconcilePortSGs for every ENI with a non-empty -// SecurityGroupIds list. Idempotent. Counts only ENIs whose membership -// actually changed — a converged ENI scanned with no drift is a no-op and -// must not appear in the converged-drift log line. -func scanENIPortMembership(ctx context.Context, topo *TopologyHandler, eniKV nats.KeyValue) int { - keys, err := eniKV.Keys() - if err != nil && !errors.Is(err, nats.ErrNoKeysFound) { - slog.Warn("vpcd reconcile-sgs: failed to list ENI keys", "err", err) - return 0 - } - synced := 0 - for _, key := range keys { - if key == utils.VersionKey { - continue - } - entry, err := eniKV.Get(key) - if err != nil { - slog.Warn("vpcd reconcile-sgs: ENI read failed", "key", key, "err", err) - continue - } - var rec handlers_ec2_vpc.ENIRecord - if err := json.Unmarshal(entry.Value(), &rec); err != nil { - slog.Warn("vpcd reconcile-sgs: ENI unmarshal failed", "key", key, "err", err) - continue - } - if len(rec.SecurityGroupIds) == 0 { - continue - } - portName := "port-" + rec.NetworkInterfaceId - changed, err := topo.reconcilePortSGs(ctx, portName, rec.SecurityGroupIds) - if err != nil { - // Most often: the LSP itself doesn't exist yet. Warn-and-continue. - slog.Warn("vpcd reconcile-sgs: port SG reconcile failed", "eni", rec.NetworkInterfaceId, "err", err) - continue - } - if changed { - synced++ - } - } - return synced -} - -// scanOrphanPortGroups removes any spinifex-managed port group (`sg_*`) in OVN -// that has no matching SG record in KV. Mirrors handleDeleteSG's teardown -// (ClearACLs → DeletePortGroup). The matching `_ip4` / `_ip6` -// Address_Set rows in SB are auto-derived from port group membership by -// ovn-northd, so removing the port group also removes its address sets. -// Non-`sg_*` port groups (e.g. third-party usage) are left alone. Returns -// the count removed. -func scanOrphanPortGroups(ctx context.Context, topo *TopologyHandler, sgs []handlers_ec2_vpc.SecurityGroupRecord) int { - pgs, err := topo.ovn.ListPortGroups(ctx) - if err != nil { - slog.Warn("vpcd reconcile-sgs: list port groups failed", "err", err) - return 0 - } - - expected := make(map[string]struct{}, len(sgs)) - for _, sg := range sgs { - expected[portGroupName(sg.GroupId)] = struct{}{} - } - - removed := 0 - for _, pg := range pgs { - if !strings.HasPrefix(pg.Name, "sg_") { - continue - } - if _, ok := expected[pg.Name]; ok { - continue - } - if err := topo.ovn.ClearACLs(ctx, pg.Name); err != nil { - slog.Warn("vpcd reconcile-sgs: orphan ClearACLs failed", "pg", pg.Name, "err", err) - continue - } - if err := topo.ovn.DeletePortGroup(ctx, pg.Name); err != nil { - slog.Warn("vpcd reconcile-sgs: orphan DeletePortGroup failed", "pg", pg.Name, "err", err) - continue - } - slog.Info("vpcd reconcile-sgs: removed orphan port group", "pg", pg.Name) - removed++ - } - return removed -} - -// sgRulesToACLRules converts handler-side SGRules to vpcd-side SGRuleForACL. -// The duplication keeps vpcd from importing the handler package. -func sgRulesToACLRules(rules []handlers_ec2_vpc.SGRule) []SGRuleForACL { - out := make([]SGRuleForACL, len(rules)) - for i, r := range rules { - out[i] = SGRuleForACL{ - IpProtocol: r.IpProtocol, - FromPort: r.FromPort, - ToPort: r.ToPort, - CidrIp: r.CidrIp, - SourceSG: r.SourceSG, - } - } - return out -} diff --git a/spinifex/services/vpcd/reconcile_sgs_test.go b/spinifex/services/vpcd/reconcile_sgs_test.go deleted file mode 100644 index b80fd5340..000000000 --- a/spinifex/services/vpcd/reconcile_sgs_test.go +++ /dev/null @@ -1,368 +0,0 @@ -package vpcd - -import ( - "context" - "encoding/json" - "testing" - "time" - - handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" - "github.com/mulgadc/spinifex/spinifex/utils" - "github.com/nats-io/nats.go" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -const sgReconcileAccountID = "000000000001" - -func seedSGBucket(t *testing.T, nc *nats.Conn, sgs []handlers_ec2_vpc.SecurityGroupRecord) nats.KeyValue { - t.Helper() - js, err := nc.JetStream() - require.NoError(t, err) - kv, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketSecurityGroups, History: 1}) - require.NoError(t, err) - for _, sg := range sgs { - data, _ := json.Marshal(sg) - _, err := kv.Put(utils.AccountKey(sgReconcileAccountID, sg.GroupId), data) - require.NoError(t, err) - } - return kv -} - -func seedENIBucket(t *testing.T, nc *nats.Conn, enis []handlers_ec2_vpc.ENIRecord) nats.KeyValue { - t.Helper() - js, err := nc.JetStream() - require.NoError(t, err) - kv, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketENIs, History: 1}) - require.NoError(t, err) - for _, e := range enis { - data, _ := json.Marshal(e) - _, err := kv.Put(utils.AccountKey(sgReconcileAccountID, e.NetworkInterfaceId), data) - require.NoError(t, err) - } - return kv -} - -// TestReconcileSGsOnce_RecreatesMissingPortGroup: an SG record exists in KV -// but the corresponding OVN port group is missing (e.g. OVN NB was wiped). -func TestReconcileSGsOnce_RecreatesMissingPortGroup(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - - seedSGBucket(t, nc, []handlers_ec2_vpc.SecurityGroupRecord{{ - GroupId: "sg-missing0000000", - GroupName: "web", - VpcId: "vpc-x", - IngressRules: []handlers_ec2_vpc.SGRule{ - {IpProtocol: "tcp", FromPort: 80, ToPort: 80, CidrIp: "0.0.0.0/0"}, - }, - EgressRules: []handlers_ec2_vpc.SGRule{ - {IpProtocol: "-1", FromPort: 0, ToPort: 0, CidrIp: "0.0.0.0/0"}, - }, - CreatedAt: time.Now(), - }}) - seedENIBucket(t, nc, nil) - - result := ReconcileSGsOnce(context.Background(), nc, topo) - - assert.Equal(t, 1, result.PortGroupsRecreated) - pg, err := ovn.GetPortGroup(context.Background(), portGroupName("sg-missing0000000")) - require.NoError(t, err) - assert.Len(t, pg.ACLs, 6, "deny-ingress + deny-egress + dhcp-egress + dhcp-ingress + 1 allow ingress + 1 allow egress") -} - -// TestReconcileSGsOnce_RecreatesPortGroupIdempotent: a second pass on a fresh -// SG must not double-recreate when the port group already exists. -func TestReconcileSGsOnce_RecreatesPortGroupIdempotent(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - - seedSGBucket(t, nc, []handlers_ec2_vpc.SecurityGroupRecord{{ - GroupId: "sg-idempotent0000", VpcId: "vpc-x", CreatedAt: time.Now(), - }}) - seedENIBucket(t, nc, nil) - - r1 := ReconcileSGsOnce(context.Background(), nc, topo) - assert.Equal(t, 1, r1.PortGroupsRecreated) - r2 := ReconcileSGsOnce(context.Background(), nc, topo) - assert.Equal(t, 0, r2.PortGroupsRecreated, "second pass must be a no-op") -} - -// TestReconcileSGsOnce_SyncsENIMembership: an ENI's KV record lists SGs but -// the LSP is in the wrong port groups (or none). -func TestReconcileSGsOnce_SyncsENIMembership(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - ctx := context.Background() - - require.NoError(t, ovn.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "subnet-x"})) - subs, err := topo.Subscribe(nc) - require.NoError(t, err) - defer func() { - for _, s := range subs { - _ = s.Unsubscribe() - } - }() - - sgEvt := SGEvent{GroupId: "sg-eni0000000000000", VpcId: "vpc-x"} - sgData, _ := json.Marshal(sgEvt) - resp, err := nc.Request(TopicCreateSG, sgData, 2*time.Second) - require.NoError(t, err) - assertSuccess(t, resp, "create SG") - - // Create the LSP without joining any port group — that's the drift. - require.NoError(t, ovn.CreateLogicalSwitchPortInGroups(ctx, "subnet-x", - &nbdb.LogicalSwitchPort{Name: "port-eni-sync1", Addresses: []string{"02:00:00:aa:bb:01 10.0.0.5"}}, - nil)) - - seedSGBucket(t, nc, []handlers_ec2_vpc.SecurityGroupRecord{{ - GroupId: "sg-eni0000000000000", VpcId: "vpc-x", CreatedAt: time.Now(), - }}) - seedENIBucket(t, nc, []handlers_ec2_vpc.ENIRecord{{ - NetworkInterfaceId: "eni-sync1", - PrivateIpAddress: "10.0.0.5", - MacAddress: "02:00:00:aa:bb:01", - SecurityGroupIds: []string{"sg-eni0000000000000"}, - CreatedAt: time.Now(), - }}) - - result := ReconcileSGsOnce(ctx, nc, topo) - - assert.Equal(t, 1, result.PortMembershipsSynced) - groups, err := ovn.ListPortGroupsForPort(ctx, "port-eni-sync1") - require.NoError(t, err) - assert.Contains(t, groups, portGroupName("sg-eni0000000000000"), - "LSP must end up in the SG's port group after reconcile") -} - -// TestReconcileSGsOnce_AllScansClean: a fully-converged cluster sees zero work. -func TestReconcileSGsOnce_AllScansClean(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - - seedSGBucket(t, nc, nil) - seedENIBucket(t, nc, nil) - - result := ReconcileSGsOnce(context.Background(), nc, topo) - - assert.Equal(t, SGReconcileResult{}, result, "no drift means no work") -} - -// TestReconcileSGsOnce_NoBucketsHandledGracefully: first-boot path where no -// KV buckets exist yet. -func TestReconcileSGsOnce_NoBucketsHandledGracefully(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - - result := ReconcileSGsOnce(context.Background(), nc, topo) - assert.Equal(t, SGReconcileResult{}, result) -} - -// TestReconcileSGsOnce_NoDriftZeroSync: a fully converged ENI (LSP already in -// the right port group, IP already in the address set) must not be counted as -// synced. Otherwise the steady-state cluster logs phantom drift every cycle -// (Plan I.8 — "Constant churn means the reconciler is misclassifying steady -// state as drift"). -func TestReconcileSGsOnce_NoDriftZeroSync(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - ctx := context.Background() - - require.NoError(t, ovn.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "subnet-c"})) - subs, err := topo.Subscribe(nc) - require.NoError(t, err) - defer func() { - for _, s := range subs { - _ = s.Unsubscribe() - } - }() - - sgEvt := SGEvent{GroupId: "sg-converged000000", VpcId: "vpc-x"} - sgData, _ := json.Marshal(sgEvt) - resp, err := nc.Request(TopicCreateSG, sgData, 2*time.Second) - require.NoError(t, err) - assertSuccess(t, resp, "create SG") - - // LSP already joined the port group — matches what handleCreateLSP / - // handleUpdatePortSGs leave behind on the happy path. The per-port-group - // `_ip4` Address_Set is auto-derived by ovn-northd in production. - pgName := portGroupName("sg-converged000000") - require.NoError(t, ovn.CreateLogicalSwitchPortInGroups(ctx, "subnet-c", - &nbdb.LogicalSwitchPort{Name: "port-eni-converged", Addresses: []string{"02:00:00:aa:bb:02 10.0.0.6"}}, - []string{pgName})) - - seedSGBucket(t, nc, []handlers_ec2_vpc.SecurityGroupRecord{{ - GroupId: "sg-converged000000", VpcId: "vpc-x", CreatedAt: time.Now(), - }}) - seedENIBucket(t, nc, []handlers_ec2_vpc.ENIRecord{{ - NetworkInterfaceId: "eni-converged", - PrivateIpAddress: "10.0.0.6", - MacAddress: "02:00:00:aa:bb:02", - SecurityGroupIds: []string{"sg-converged000000"}, - CreatedAt: time.Now(), - }}) - - result := ReconcileSGsOnce(ctx, nc, topo) - - assert.Equal(t, 0, result.PortMembershipsSynced, "fully converged ENI must not count as synced") - assert.Equal(t, 0, result.PortGroupsRecreated, "existing PG must not be recreated") - assert.Equal(t, 0, result.OrphanPortGroupsRemoved, "PG with matching SG record must not be torn down") -} - -// TestReconcileSGsOnce_DeletesOrphanPortGroup: an `sg_*` port group with no -// matching SG record must be torn down (PG + ACLs) within one pass. Section -// G2 of the manual test plan. The matching `_ip4` Address_Set is -// auto-derived in SB by ovn-northd and goes away with the port group. -func TestReconcileSGsOnce_DeletesOrphanPortGroup(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - ctx := context.Background() - - const orphanPG = "sg_orphan0000000000" - require.NoError(t, ovn.CreatePortGroup(ctx, orphanPG, nil)) - - // Pre-existing ACL on the orphan PG to confirm the scan clears it before - // deleting (matches handleDeleteSG's order). - require.NoError(t, ovn.AddACLs(ctx, orphanPG, []ACLSpec{{ - Direction: "to-lport", Priority: 900, - Match: "outport == @" + orphanPG + " && ip4", Action: "drop", - }})) - - seedSGBucket(t, nc, nil) - seedENIBucket(t, nc, nil) - - result := ReconcileSGsOnce(ctx, nc, topo) - - assert.Equal(t, 1, result.OrphanPortGroupsRemoved) - _, err := ovn.GetPortGroup(ctx, orphanPG) - assert.Error(t, err, "orphan port group must be gone") - pgs, err := ovn.ListPortGroups(ctx) - require.NoError(t, err) - for _, pg := range pgs { - assert.NotEqual(t, orphanPG, pg.Name) - } -} - -// TestReconcileSGsOnce_LeavesNonSpinifexPortGroupsAlone: only `sg_*` port -// groups are spinifex-managed. A PG named differently (e.g. one a third -// party put in OVN) must not be touched. -func TestReconcileSGsOnce_LeavesNonSpinifexPortGroupsAlone(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - ctx := context.Background() - - require.NoError(t, ovn.CreatePortGroup(ctx, "third-party-pg", nil)) - - seedSGBucket(t, nc, nil) - seedENIBucket(t, nc, nil) - - result := ReconcileSGsOnce(ctx, nc, topo) - assert.Equal(t, 0, result.OrphanPortGroupsRemoved) - _, err := ovn.GetPortGroup(ctx, "third-party-pg") - assert.NoError(t, err, "non-`sg_*` port group must be left untouched") -} - -// TestReconcileSGsOnce_ENIWithoutLSPLogs: an ENI's KV record references SGs -// but no OVN LSP exists yet. The scan should not count this ENI as synced. -func TestReconcileSGsOnce_ENIWithoutLSPLogs(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - - seedSGBucket(t, nc, nil) - seedENIBucket(t, nc, []handlers_ec2_vpc.ENIRecord{{ - NetworkInterfaceId: "eni-no-lsp", - PrivateIpAddress: "10.0.0.1", - SecurityGroupIds: []string{"sg-something00000"}, - CreatedAt: time.Now(), - }}) - - result := ReconcileSGsOnce(context.Background(), nc, topo) - assert.Equal(t, 0, result.PortMembershipsSynced, "ENI with no LSP must not count as synced") -} - -// TestReconcileSGsOnce_NilTopologyHandler: defensive guard — a partial vpcd -// startup that fails before TopologyHandler.ovn is wired must not panic when -// the leader-elected reconciler tick fires. -func TestReconcileSGsOnce_NilTopologyHandler(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - - assert.Equal(t, SGReconcileResult{}, ReconcileSGsOnce(context.Background(), nc, nil), - "nil topo must return zero result, not panic") - assert.Equal(t, SGReconcileResult{}, ReconcileSGsOnce(context.Background(), nc, NewTopologyHandler(nil)), - "topo with nil ovn must return zero result, not panic") -} - -// TestReconcileSGsOnce_SkipsMalformedSGEntry: a corrupt SG record (non-JSON -// bytes) must be logged-and-skipped, not crash the reconciler. Otherwise a -// single bad write to the SG bucket would freeze convergence cluster-wide. -func TestReconcileSGsOnce_SkipsMalformedSGEntry(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - - // Seed with one valid + one malformed entry. The malformed entry must not - // abort the scan — the valid SG must still get its port group created. - js, err := nc.JetStream() - require.NoError(t, err) - kv, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketSecurityGroups, History: 1}) - require.NoError(t, err) - _, err = kv.Put(utils.AccountKey(sgReconcileAccountID, "sg-malformed00000"), []byte("not valid json")) - require.NoError(t, err) - good, _ := json.Marshal(handlers_ec2_vpc.SecurityGroupRecord{ - GroupId: "sg-good0000000000", VpcId: "vpc-x", CreatedAt: time.Now(), - }) - _, err = kv.Put(utils.AccountKey(sgReconcileAccountID, "sg-good0000000000"), good) - require.NoError(t, err) - - result := ReconcileSGsOnce(context.Background(), nc, topo) - assert.Equal(t, 1, result.PortGroupsRecreated, "valid SG must still be processed") - _, err = ovn.GetPortGroup(context.Background(), portGroupName("sg-good0000000000")) - assert.NoError(t, err) - _, err = ovn.GetPortGroup(context.Background(), portGroupName("sg-malformed00000")) - assert.Error(t, err, "malformed entry must not produce a port group") -} - -// TestReconcileSGsLoop_ExitsOnContextCancel: the periodic loop must stop when -// its context is cancelled. Otherwise vpcd shutdown leaks a goroutine that -// keeps holding the reconcile-leader lock until TTL expiry — blocking other -// vpcds from converging during the shutdown window. -func TestReconcileSGsLoop_ExitsOnContextCancel(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - require.NoError(t, ovn.Connect(context.Background())) - topo := NewTopologyHandler(ovn) - - ctx, cancel := context.WithCancel(context.Background()) - done := make(chan struct{}) - go func() { - ReconcileSGsLoop(ctx, nc, topo) - close(done) - }() - - cancel() - select { - case <-done: - case <-time.After(2 * time.Second): - t.Fatal("ReconcileSGsLoop did not return within 2s of ctx cancel") - } -} diff --git a/spinifex/services/vpcd/reconcile_test.go b/spinifex/services/vpcd/reconcile_test.go deleted file mode 100644 index e72d18a2e..000000000 --- a/spinifex/services/vpcd/reconcile_test.go +++ /dev/null @@ -1,892 +0,0 @@ -package vpcd - -import ( - "context" - "encoding/json" - "sync" - "sync/atomic" - "testing" - "time" - - handlers_ec2_eip "github.com/mulgadc/spinifex/spinifex/handlers/ec2/eip" - handlers_ec2_igw "github.com/mulgadc/spinifex/spinifex/handlers/ec2/igw" - handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" - "github.com/mulgadc/spinifex/spinifex/testutil" - "github.com/mulgadc/spinifex/spinifex/utils" - "github.com/nats-io/nats.go" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestAcquireReconcileLeader_OnlyOneWinner(t *testing.T) { - _, nc, _ := testutil.StartTestJetStream(t) - - releaseA, electedA := AcquireReconcileLeader(nc, "node-a") - require.True(t, electedA, "first caller must win") - require.NotNil(t, releaseA) - - releaseB, electedB := AcquireReconcileLeader(nc, "node-b") - assert.False(t, electedB, "second caller must lose while A holds the lock") - assert.Nil(t, releaseB) - - releaseA() - - // After release the next caller can claim it. - releaseC, electedC := AcquireReconcileLeader(nc, "node-c") - require.True(t, electedC, "next caller wins after release") - releaseC() -} - -// withReconcileLeaderRetry shrinks the bounded-retry window for tests so they -// run sub-second instead of waiting the production 60s deadline. -func withReconcileLeaderRetry(t *testing.T, retryFor, retryStep time.Duration) { - t.Helper() - prevFor, prevStep := reconcileLeaderRetryFor, reconcileLeaderRetryStep - reconcileLeaderRetryFor = retryFor - reconcileLeaderRetryStep = retryStep - t.Cleanup(func() { - reconcileLeaderRetryFor = prevFor - reconcileLeaderRetryStep = prevStep - }) -} - -func TestAcquireReconcileLeader_WaitsForJetStream(t *testing.T) { - // Cold-start scenario: NATS is up but JetStream is not yet enabled. - // AcquireReconcileLeader must retry until JetStream comes online and - // then win the election, instead of failing immediately. - ns, nc := testutil.StartTestNATS(t) - withReconcileLeaderRetry(t, 3*time.Second, 50*time.Millisecond) - - enableAt := time.Now().Add(300 * time.Millisecond) - go func() { - time.Sleep(time.Until(enableAt)) - _ = ns.EnableJetStream(nil) - }() - - start := time.Now() - release, elected := AcquireReconcileLeader(nc, "node-a") - elapsed := time.Since(start) - - require.True(t, elected, "must win election after JetStream comes up") - require.NotNil(t, release) - defer release() - assert.GreaterOrEqual(t, elapsed, 300*time.Millisecond, - "must not return before JetStream is enabled") - assert.Less(t, elapsed, 3*time.Second, "must return within retry deadline") -} - -func TestAcquireReconcileLeader_TimeoutReturnsNotElected(t *testing.T) { - // JetStream never enabled on this server — bounded retry must exhaust - // and return elected=false rather than fall through and run unguarded - // (mulga-js-72: concurrent reconciles produce silent OVN NB damage). - _, nc := testutil.StartTestNATS(t) - withReconcileLeaderRetry(t, 200*time.Millisecond, 20*time.Millisecond) - - start := time.Now() - release, elected := AcquireReconcileLeader(nc, "node-a") - elapsed := time.Since(start) - - assert.False(t, elected, "must skip reconcile when JetStream KV unreachable") - assert.Nil(t, release, "no release closure when not elected") - assert.GreaterOrEqual(t, elapsed, 200*time.Millisecond, "must wait full deadline") -} - -func TestAcquireReconcileLeader_ConcurrentExactlyOneElected(t *testing.T) { - _, nc, _ := testutil.StartTestJetStream(t) - - const goroutines = 5 - var ( - wg sync.WaitGroup - electedCt atomic.Int32 - releases = make(chan func(), goroutines) - ) - wg.Add(goroutines) - for i := range goroutines { - go func(id int) { - defer wg.Done() - release, elected := AcquireReconcileLeader(nc, "node-"+string(rune('a'+id))) - if elected { - electedCt.Add(1) - releases <- release - } else { - assert.Nil(t, release, "non-leader must return nil release") - } - }(i) - } - wg.Wait() - close(releases) - - assert.Equal(t, int32(1), electedCt.Load(), "exactly one caller must win") - for r := range releases { - r() - } -} - -func TestReconcile_NoBootstrap(t *testing.T) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - topo := NewTopologyHandler(ovn) - - result := Reconcile(context.Background(), topo, nil) - assert.Equal(t, 0, result.RoutersCreated) - assert.Equal(t, 0, result.SwitchesCreated) - assert.Equal(t, 0, result.IGWsReconciled) -} - -func TestReconcile_EmptyBootstrap(t *testing.T) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - topo := NewTopologyHandler(ovn) - - result := Reconcile(context.Background(), topo, &BootstrapVPC{}) - assert.Equal(t, 0, result.RoutersCreated) -} - -func TestReconcile_CreatesBootstrapTopology(t *testing.T) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", - RangeStart: "192.168.1.200", - RangeEnd: "192.168.1.250", - Gateway: "192.168.1.1", - GatewayIP: "192.168.1.200", - PrefixLen: 23, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - bootstrap := &BootstrapVPC{ - AccountID: "000000000001", - VpcId: "vpc-test123", - SubnetId: "subnet-test456", - Cidr: "172.31.0.0/16", - SubnetCidr: "172.31.0.0/20", - } - - result := Reconcile(context.Background(), topo, bootstrap) - assert.Equal(t, 1, result.RoutersCreated) - assert.Equal(t, 1, result.SwitchesCreated) - assert.Equal(t, 1, result.IGWsReconciled) - - // Verify OVN objects exist - ctx := context.Background() - - _, err := ovn.GetLogicalRouter(ctx, "vpc-vpc-test123") - require.NoError(t, err) - - _, err = ovn.GetLogicalSwitch(ctx, "subnet-subnet-test456") - require.NoError(t, err) - - _, err = ovn.GetLogicalSwitch(ctx, "ext-vpc-test123") - require.NoError(t, err) - - // Router port for subnet - _, err = ovn.GetLogicalRouterPort(ctx, "rtr-subnet-test456") - require.NoError(t, err) - - // Gateway router port - _, err = ovn.GetLogicalRouterPort(ctx, "gw-vpc-test123") - require.NoError(t, err) - - // DHCP options should exist - _, err = ovn.FindDHCPOptionsByExternalID(ctx, "spinifex:subnet_id", "subnet-test456") - require.NoError(t, err) -} - -func TestReconcile_Idempotent(t *testing.T) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", - Gateway: "192.168.1.1", - GatewayIP: "192.168.1.200", - PrefixLen: 23, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - bootstrap := &BootstrapVPC{ - AccountID: "000000000001", - VpcId: "vpc-idem", - SubnetId: "subnet-idem", - Cidr: "172.31.0.0/16", - SubnetCidr: "172.31.0.0/20", - } - - // First run creates everything - r1 := Reconcile(context.Background(), topo, bootstrap) - assert.Equal(t, 1, r1.RoutersCreated) - assert.Equal(t, 1, r1.SwitchesCreated) - assert.Equal(t, 1, r1.IGWsReconciled) - - // 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, 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) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", - Gateway: "192.168.1.1", - GatewayIP: "192.168.1.200", - PrefixLen: 23, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - bootstrap := &BootstrapVPC{ - AccountID: "000000000001", - VpcId: "vpc-partial", - SubnetId: "subnet-partial", - Cidr: "172.31.0.0/16", - SubnetCidr: "172.31.0.0/20", - } - - // Pre-create just the router (simulating partial OVN state) - ctx := context.Background() - _ = topo.reconcileVPC(ctx, "vpc-partial", "172.31.0.0/16") - // IGW ID not needed for pre-creating just the router - - // Reconcile should skip router but create subnet + IGW - result := Reconcile(ctx, topo, bootstrap) - assert.Equal(t, 0, result.RoutersCreated) - assert.Equal(t, 1, result.SwitchesCreated) - assert.Equal(t, 1, result.IGWsReconciled) -} - -// --- ReconcileFromKV tests --- - -// seedKVBuckets creates NATS KV buckets and populates them with test data. -func seedKVBuckets(t *testing.T, nc *nats.Conn, vpcs []handlers_ec2_vpc.VPCRecord, subnets []handlers_ec2_vpc.SubnetRecord, igws []handlers_ec2_igw.IGWRecord, enis []handlers_ec2_vpc.ENIRecord) { - t.Helper() - js, err := nc.JetStream() - require.NoError(t, err) - - vpcKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketVPCs, History: 1}) - require.NoError(t, err) - for _, v := range vpcs { - data, _ := json.Marshal(v) - _, err := vpcKV.Put(utils.AccountKey("000000000001", v.VpcId), data) - require.NoError(t, err) - } - - subnetKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketSubnets, History: 1}) - require.NoError(t, err) - for _, s := range subnets { - data, _ := json.Marshal(s) - _, err := subnetKV.Put(utils.AccountKey("000000000001", s.SubnetId), data) - require.NoError(t, err) - } - - igwKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_igw.KVBucketIGW, History: 1}) - require.NoError(t, err) - for _, i := range igws { - data, _ := json.Marshal(i) - _, err := igwKV.Put(utils.AccountKey("000000000001", i.InternetGatewayId), data) - require.NoError(t, err) - } - - eniKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketENIs, History: 1}) - require.NoError(t, err) - for _, e := range enis { - data, _ := json.Marshal(e) - _, err := eniKV.Put(utils.AccountKey("000000000001", e.NetworkInterfaceId), data) - require.NoError(t, err) - } -} - -func TestReconcileFromKV_CreatesFullTopology(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - ctx := context.Background() - - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", - RangeStart: "10.0.0.200", - RangeEnd: "10.0.0.250", - Gateway: "10.0.0.1", - GatewayIP: "10.0.0.200", - PrefixLen: 24, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - seedKVBuckets(t, nc, - []handlers_ec2_vpc.VPCRecord{{ - VpcId: "vpc-kv1", CidrBlock: "10.100.0.0/16", State: "available", - CreatedAt: time.Now(), - }}, - []handlers_ec2_vpc.SubnetRecord{{ - SubnetId: "subnet-kv1", VpcId: "vpc-kv1", CidrBlock: "10.100.1.0/24", - State: "available", CreatedAt: time.Now(), - }}, - []handlers_ec2_igw.IGWRecord{{ - InternetGatewayId: "igw-kv1", VpcId: "vpc-kv1", State: "attached", - CreatedAt: time.Now(), - }}, - []handlers_ec2_vpc.ENIRecord{{ - NetworkInterfaceId: "eni-kv1", SubnetId: "subnet-kv1", VpcId: "vpc-kv1", - PrivateIpAddress: "10.100.1.10", MacAddress: "02:00:00:aa:bb:cc", - Status: "in-use", CreatedAt: time.Now(), - }}, - ) - - result := ReconcileFromKV(ctx, nc, topo, nil) - assert.Equal(t, 1, result.RoutersCreated) - assert.Equal(t, 1, result.SwitchesCreated) - assert.Equal(t, 1, result.IGWsReconciled) - assert.Equal(t, 1, result.PortsCreated) - - // Verify OVN objects - _, err := ovn.GetLogicalRouter(ctx, "vpc-vpc-kv1") - require.NoError(t, err) - - _, err = ovn.GetLogicalSwitch(ctx, "subnet-subnet-kv1") - require.NoError(t, err) - - _, err = ovn.GetLogicalSwitch(ctx, "ext-vpc-kv1") - require.NoError(t, err) - - lsp, err := ovn.GetLogicalSwitchPort(ctx, "port-eni-kv1") - require.NoError(t, err) - assert.Contains(t, lsp.Addresses[0], "10.100.1.10") -} - -func TestReconcileFromKV_Idempotent(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - ctx := context.Background() - - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", Gateway: "10.0.0.1", GatewayIP: "10.0.0.200", PrefixLen: 24, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - seedKVBuckets(t, nc, - []handlers_ec2_vpc.VPCRecord{{ - VpcId: "vpc-idem2", CidrBlock: "10.200.0.0/16", State: "available", - CreatedAt: time.Now(), - }}, - []handlers_ec2_vpc.SubnetRecord{{ - SubnetId: "subnet-idem2", VpcId: "vpc-idem2", CidrBlock: "10.200.1.0/24", - State: "available", CreatedAt: time.Now(), - }}, - nil, nil, - ) - - r1 := ReconcileFromKV(ctx, nc, topo, nil) - assert.Equal(t, 1, r1.RoutersCreated) - assert.Equal(t, 1, r1.SwitchesCreated) - - // Second run: everything exists - r2 := ReconcileFromKV(ctx, nc, topo, nil) - assert.Equal(t, 0, r2.RoutersCreated) - assert.Equal(t, 0, r2.SwitchesCreated) -} - -func TestReconcileFromKV_SkipsDetachedIGW(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - ctx := context.Background() - - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", Gateway: "10.0.0.1", GatewayIP: "10.0.0.200", PrefixLen: 24, - }}), - ) - - seedKVBuckets(t, nc, - []handlers_ec2_vpc.VPCRecord{{ - VpcId: "vpc-det", CidrBlock: "10.50.0.0/16", State: "available", - CreatedAt: time.Now(), - }}, - nil, - []handlers_ec2_igw.IGWRecord{{ - InternetGatewayId: "igw-det", VpcId: "", State: "available", - CreatedAt: time.Now(), - }}, - nil, - ) - - result := ReconcileFromKV(ctx, nc, topo, nil) - assert.Equal(t, 1, result.RoutersCreated) // VPC router created - 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 -} - -func TestReconcileFromKV_NoBuckets(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - - topo := NewTopologyHandler(ovn) - - // No KV buckets seeded — should handle gracefully - result := ReconcileFromKV(context.Background(), nc, topo, nil) - assert.Equal(t, 0, result.RoutersCreated) - assert.Equal(t, 0, result.SwitchesCreated) - assert.Equal(t, 0, result.IGWsReconciled) - assert.Equal(t, 0, result.PortsCreated) -} - -func TestReconcileFromKV_VersionKeysAndBadJSON(t *testing.T) { - // Tests that _version keys are skipped and malformed JSON records are handled gracefully. - // This covers the "continue" branches for version key filtering and unmarshal errors. - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - ctx := context.Background() - - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", - RangeStart: "10.0.0.200", - RangeEnd: "10.0.0.250", - Gateway: "10.0.0.1", - GatewayIP: "10.0.0.200", - PrefixLen: 24, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - js, err := nc.JetStream() - require.NoError(t, err) - - // Create VPC bucket with _version key and one bad JSON record - vpcKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketVPCs, History: 1}) - require.NoError(t, err) - _, err = vpcKV.PutString(utils.VersionKey, "1") - require.NoError(t, err) - _, err = vpcKV.Put("bad-vpc-record", []byte("not-json")) - require.NoError(t, err) - // Also add a valid VPC record - vpcData, _ := json.Marshal(handlers_ec2_vpc.VPCRecord{ - VpcId: "vpc-ver1", CidrBlock: "10.100.0.0/16", State: "available", - CreatedAt: time.Now(), - }) - _, err = vpcKV.Put(utils.AccountKey("000000000001", "vpc-ver1"), vpcData) - require.NoError(t, err) - - // Create subnet bucket with _version key and one bad JSON record - subnetKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketSubnets, History: 1}) - require.NoError(t, err) - _, err = subnetKV.PutString(utils.VersionKey, "1") - require.NoError(t, err) - _, err = subnetKV.Put("bad-subnet", []byte("{invalid")) - require.NoError(t, err) - subnetData, _ := json.Marshal(handlers_ec2_vpc.SubnetRecord{ - SubnetId: "subnet-ver1", VpcId: "vpc-ver1", CidrBlock: "10.100.1.0/24", - State: "available", CreatedAt: time.Now(), - }) - _, err = subnetKV.Put(utils.AccountKey("000000000001", "subnet-ver1"), subnetData) - require.NoError(t, err) - - // Create IGW bucket with _version key and one bad JSON record - igwKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_igw.KVBucketIGW, History: 1}) - require.NoError(t, err) - _, err = igwKV.PutString(utils.VersionKey, "1") - require.NoError(t, err) - _, err = igwKV.Put("bad-igw", []byte("???")) - require.NoError(t, err) - igwData, _ := json.Marshal(handlers_ec2_igw.IGWRecord{ - InternetGatewayId: "igw-ver1", VpcId: "vpc-ver1", State: "attached", - CreatedAt: time.Now(), - }) - _, err = igwKV.Put(utils.AccountKey("000000000001", "igw-ver1"), igwData) - require.NoError(t, err) - - // Create ENI bucket with _version key and one bad JSON record - eniKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketENIs, History: 1}) - require.NoError(t, err) - _, err = eniKV.PutString(utils.VersionKey, "1") - require.NoError(t, err) - _, err = eniKV.Put("bad-eni", []byte("nope")) - require.NoError(t, err) - eniData, _ := json.Marshal(handlers_ec2_vpc.ENIRecord{ - NetworkInterfaceId: "eni-ver1", SubnetId: "subnet-ver1", VpcId: "vpc-ver1", - PrivateIpAddress: "10.100.1.10", MacAddress: "02:00:00:aa:bb:01", - Status: "in-use", CreatedAt: time.Now(), - }) - _, err = eniKV.Put(utils.AccountKey("000000000001", "eni-ver1"), eniData) - require.NoError(t, err) - - result := ReconcileFromKV(ctx, nc, topo, nil) - - // 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.IGWsReconciled) - assert.Equal(t, 1, result.PortsCreated) - - // Verify OVN objects created from valid records - _, err = ovn.GetLogicalRouter(ctx, "vpc-vpc-ver1") - require.NoError(t, err) - _, err = ovn.GetLogicalSwitch(ctx, "subnet-subnet-ver1") - require.NoError(t, err) - _, err = ovn.GetLogicalSwitch(ctx, "ext-vpc-ver1") - require.NoError(t, err) - _, err = ovn.GetLogicalSwitchPort(ctx, "port-eni-ver1") - require.NoError(t, err) -} - -func TestReconcileFromKV_EmptyBuckets(t *testing.T) { - // Tests the nats.ErrNoKeysFound branch when KV buckets exist but are empty. - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - - topo := NewTopologyHandler(ovn) - - js, err := nc.JetStream() - require.NoError(t, err) - - // Create all KV buckets but leave them empty (no keys at all) - _, err = js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketVPCs, History: 1}) - require.NoError(t, err) - _, err = js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketSubnets, History: 1}) - require.NoError(t, err) - _, err = js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_igw.KVBucketIGW, History: 1}) - require.NoError(t, err) - _, err = js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_vpc.KVBucketENIs, History: 1}) - require.NoError(t, err) - - result := ReconcileFromKV(context.Background(), nc, topo, nil) - assert.Equal(t, 0, result.RoutersCreated) - assert.Equal(t, 0, result.SwitchesCreated) - assert.Equal(t, 0, result.IGWsReconciled) - assert.Equal(t, 0, result.PortsCreated) -} - -// --- reconcileGatewayChassis tests (mulga-999) --- - -// seedGatewayLRP creates a router + LRP tagged spinifex:role=gateway so -// reconcileGatewayChassis's rebind step has something to bind. -func seedGatewayLRP(t *testing.T, ovn *MockOVNClient, routerName, lrpName string) { - t.Helper() - ctx := context.Background() - require.NoError(t, ovn.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: routerName})) - require.NoError(t, ovn.CreateLogicalRouterPort(ctx, routerName, &nbdb.LogicalRouterPort{ - Name: lrpName, - ExternalIDs: map[string]string{"spinifex:role": "gateway"}, - })) -} - -func TestReconcileGatewayChassis_RemovesStaleRows(t *testing.T) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - topo := NewTopologyHandler(ovn) - ctx := context.Background() - - seedGatewayLRP(t, ovn, "vpc-A", "gw-A") - // One stale (chassis-old not in valid set), one live (UUID-A is). - ovn.SeedGatewayChassis("gw-A", &nbdb.GatewayChassis{ - Name: "gw-A-chassis-old", - ChassisName: "chassis-old", - Priority: 20, - }) - ovn.SeedGatewayChassis("gw-A", &nbdb.GatewayChassis{ - Name: "gw-A-UUID-A", - ChassisName: "UUID-A", - Priority: 20, - }) - - require.NoError(t, topo.reconcileGatewayChassis(ctx, []string{"UUID-A"})) - - rows, err := ovn.ListGatewayChassis(ctx) - require.NoError(t, err) - require.Len(t, rows, 1, "stale row should be deleted, live row should remain") - assert.Equal(t, "UUID-A", rows[0].ChassisName) - assert.Equal(t, 1, ovn.DeleteGatewayChassisCalls) -} - -func TestReconcileGatewayChassis_RebindsAllGatewayLRPs(t *testing.T) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - topo := NewTopologyHandler(ovn) - ctx := context.Background() - - seedGatewayLRP(t, ovn, "vpc-A", "gw-A") - seedGatewayLRP(t, ovn, "vpc-B", "gw-B") - // Untagged LRP must NOT be rebinded. - require.NoError(t, ovn.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-C"})) - require.NoError(t, ovn.CreateLogicalRouterPort(ctx, "vpc-C", &nbdb.LogicalRouterPort{ - Name: "rtr-subnet-C", - ExternalIDs: map[string]string{"spinifex:role": "internal"}, - })) - - require.NoError(t, topo.reconcileGatewayChassis(ctx, []string{"chassis-A", "chassis-B"})) - - // 2 gateway LRPs × 2 chassis = 4 SetGatewayChassis create calls. The - // internal LRP must contribute zero. - assert.Equal(t, 4, ovn.SetGatewayChassisCalls) - rows, err := ovn.ListGatewayChassis(ctx) - require.NoError(t, err) - assert.Len(t, rows, 4) - - // Verify priorities — first chassis gets 20, second 15. - prioByGC := make(map[string]int, len(rows)) - for _, gc := range rows { - prioByGC[gc.Name] = gc.Priority - } - assert.Equal(t, 20, prioByGC["gw-A-chassis-A"]) - assert.Equal(t, 15, prioByGC["gw-A-chassis-B"]) - assert.Equal(t, 20, prioByGC["gw-B-chassis-A"]) - assert.Equal(t, 15, prioByGC["gw-B-chassis-B"]) -} - -// TestReconcileIGW_RewritesStaleGatewayPortNetworks guards the mulga-siv-26 -// D8 self-heal path. Pre-seed an IGW topology with a stale pool-IP on the -// gateway LRP (the buggy state shipped to env20); the startup retrofit must -// rewrite every such LRP in place to link-local via UpdateLogicalRouterPort. -// Internal-role LRPs and LRPs already correct must not be touched. -func TestReconcileIGW_RewritesStaleGatewayPortNetworks(t *testing.T) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - ctx := context.Background() - topo := NewTopologyHandler(ovn) - - // Stale gateway LRP on VPC A — the buggy pool-IP CIDR. - require.NoError(t, ovn.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-A"})) - require.NoError(t, ovn.CreateLogicalRouterPort(ctx, "vpc-A", &nbdb.LogicalRouterPort{ - Name: "gw-A", - MAC: "02:00:00:74:e8:d2", - Networks: []string{"192.168.0.160/24"}, - ExternalIDs: map[string]string{"spinifex:role": "gateway"}, - })) - // Already-correct gateway LRP on VPC B — must not be touched. - require.NoError(t, ovn.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-B"})) - require.NoError(t, ovn.CreateLogicalRouterPort(ctx, "vpc-B", &nbdb.LogicalRouterPort{ - Name: "gw-B", - MAC: "02:00:00:bb:6d:14", - Networks: []string{"169.254.0.1/30"}, - ExternalIDs: map[string]string{"spinifex:role": "gateway"}, - })) - // Internal LRP on VPC C — must not be touched even though Networks differ. - require.NoError(t, ovn.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-C"})) - require.NoError(t, ovn.CreateLogicalRouterPort(ctx, "vpc-C", &nbdb.LogicalRouterPort{ - Name: "rtr-subnet-C", - Networks: []string{"10.0.1.1/24"}, - ExternalIDs: map[string]string{"spinifex:role": "internal"}, - })) - - topo.RetrofitAllGatewayPortNetworks(ctx) - - gwA, err := ovn.GetLogicalRouterPort(ctx, "gw-A") - require.NoError(t, err) - assert.Equal(t, []string{"169.254.0.1/30"}, gwA.Networks, - "stale gateway Networks must be rewritten in place") - - gwB, err := ovn.GetLogicalRouterPort(ctx, "gw-B") - require.NoError(t, err) - assert.Equal(t, []string{"169.254.0.1/30"}, gwB.Networks) - - internal, err := ovn.GetLogicalRouterPort(ctx, "rtr-subnet-C") - require.NoError(t, err) - assert.Equal(t, []string{"10.0.1.1/24"}, internal.Networks, - "internal LRP must be untouched") - - assert.Equal(t, 1, ovn.UpdateLogicalRouterPortCalls, - "only the stale gateway LRP should trigger Update") - - // Idempotent second pass — all gateway LRPs now correct, no Update. - ovn.UpdateLogicalRouterPortCalls = 0 - topo.RetrofitAllGatewayPortNetworks(ctx) - assert.Equal(t, 0, ovn.UpdateLogicalRouterPortCalls, - "no Update expected when every gateway LRP already link-local") -} - -func TestReconcileGatewayChassis_NoOpWhenAlreadyCorrect(t *testing.T) { - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - topo := NewTopologyHandler(ovn) - ctx := context.Background() - - seedGatewayLRP(t, ovn, "vpc-A", "gw-A") - require.NoError(t, topo.reconcileGatewayChassis(ctx, []string{"chassis-A"})) - // Reset counters from the first (creating) pass. - ovn.SetGatewayChassisCalls = 0 - ovn.DeleteGatewayChassisCalls = 0 - ovn.UpdateGatewayChassisPriorityCalls = 0 - - // Second pass: state is already correct; idempotent path must take no - // destructive or mutative action. - require.NoError(t, topo.reconcileGatewayChassis(ctx, []string{"chassis-A"})) - - assert.Equal(t, 0, ovn.SetGatewayChassisCalls, "no new creates expected") - assert.Equal(t, 0, ovn.DeleteGatewayChassisCalls, "no deletes expected") - assert.Equal(t, 0, ovn.UpdateGatewayChassisPriorityCalls, "no priority updates expected") -} - -// seedEIPKVBucket creates the EIP KV bucket and populates it with test records. -func seedEIPKVBucket(t *testing.T, nc *nats.Conn, eips []handlers_ec2_eip.EIPRecord) { - t.Helper() - js, err := nc.JetStream() - require.NoError(t, err) - eipKV, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: handlers_ec2_eip.KVBucketEIPs, History: 1}) - require.NoError(t, err) - for _, e := range eips { - data, _ := json.Marshal(e) - _, err := eipKV.Put(utils.AccountKey("000000000001", e.AllocationId), data) - require.NoError(t, err) - } -} - -func TestReconcileFromKV_ReconcilesEIPNATRules(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - ctx := context.Background() - - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", Gateway: "10.0.0.1", GatewayIP: "10.0.0.200", PrefixLen: 24, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - // Seed VPC + IGW so the router exists for AddNAT. - seedKVBuckets(t, nc, - []handlers_ec2_vpc.VPCRecord{{ - VpcId: "vpc-nat1", CidrBlock: "10.10.0.0/16", State: "available", CreatedAt: time.Now(), - }}, - nil, - []handlers_ec2_igw.IGWRecord{{ - InternetGatewayId: "igw-nat1", VpcId: "vpc-nat1", State: "attached", CreatedAt: time.Now(), - }}, - nil, - ) - - // One associated EIP (missing NAT rule), one unassociated EIP (must be skipped). - seedEIPKVBucket(t, nc, []handlers_ec2_eip.EIPRecord{ - { - AllocationId: "eipalloc-1", PublicIp: "10.0.0.210", PoolName: "wan", - State: "associated", VpcId: "vpc-nat1", PrivateIp: "10.10.1.5", ENIId: "eni-abc123", - CreatedAt: time.Now(), - }, - { - AllocationId: "eipalloc-2", PublicIp: "10.0.0.211", PoolName: "wan", - State: "allocated", - CreatedAt: time.Now(), - }, - }) - - result := ReconcileFromKV(ctx, nc, topo, nil) - assert.Equal(t, 1, result.NATsReconciled, "one missing NAT rule must be created") - - nat, err := ovn.FindNATByExternalIP(ctx, "dnat_and_snat", "10.0.0.210") - require.NoError(t, err) - require.NotNil(t, nat, "dnat_and_snat rule must exist after reconcile") - assert.Equal(t, "10.10.1.5", nat.LogicalIP) - assert.Equal(t, "dnat_and_snat", nat.Type) - - // Unassociated EIP must produce no NAT rule. - nat2, err := ovn.FindNATByExternalIP(ctx, "dnat_and_snat", "10.0.0.211") - require.NoError(t, err) - assert.Nil(t, nat2, "unassociated EIP must not produce a NAT rule") -} - -func TestReconcileFromKV_EIPNATIdempotent(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - ctx := context.Background() - - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", Gateway: "10.0.0.1", GatewayIP: "10.0.0.200", PrefixLen: 24, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - seedKVBuckets(t, nc, - []handlers_ec2_vpc.VPCRecord{{ - VpcId: "vpc-nat2", CidrBlock: "10.20.0.0/16", State: "available", CreatedAt: time.Now(), - }}, - nil, - []handlers_ec2_igw.IGWRecord{{ - InternetGatewayId: "igw-nat2", VpcId: "vpc-nat2", State: "attached", CreatedAt: time.Now(), - }}, - nil, - ) - seedEIPKVBucket(t, nc, []handlers_ec2_eip.EIPRecord{{ - AllocationId: "eipalloc-3", PublicIp: "10.0.0.220", PoolName: "wan", - State: "associated", VpcId: "vpc-nat2", PrivateIp: "10.20.1.7", ENIId: "eni-def456", - CreatedAt: time.Now(), - }}) - - // First reconcile: creates the NAT rule. - r1 := ReconcileFromKV(ctx, nc, topo, nil) - assert.Equal(t, 1, r1.NATsReconciled) - - // Second reconcile: rule already correct, must skip. - r2 := ReconcileFromKV(ctx, nc, topo, nil) - assert.Equal(t, 0, r2.NATsReconciled, "correct NAT rule must not be re-created") -} - -func TestReconcileFromKV_EIPNATStaleRuleReplaced(t *testing.T) { - _, nc := startTestJetStreamNATS(t) - ovn := NewMockOVNClient() - _ = ovn.Connect(context.Background()) - ctx := context.Background() - - topo := NewTopologyHandler(ovn, - WithExternalNetwork("pool", []ExternalPoolConfig{{ - Name: "wan", Gateway: "10.0.0.1", GatewayIP: "10.0.0.200", PrefixLen: 24, - }}), - WithChassisNames([]string{"chassis-node1"}), - ) - - seedKVBuckets(t, nc, - []handlers_ec2_vpc.VPCRecord{{ - VpcId: "vpc-nat3", CidrBlock: "10.30.0.0/16", State: "available", CreatedAt: time.Now(), - }}, - nil, - []handlers_ec2_igw.IGWRecord{{ - InternetGatewayId: "igw-nat3", VpcId: "vpc-nat3", State: "attached", CreatedAt: time.Now(), - }}, - nil, - ) - seedEIPKVBucket(t, nc, []handlers_ec2_eip.EIPRecord{{ - AllocationId: "eipalloc-4", PublicIp: "10.0.0.230", PoolName: "wan", - State: "associated", VpcId: "vpc-nat3", PrivateIp: "10.30.1.9", ENIId: "eni-ghi789", - CreatedAt: time.Now(), - }}) - - // Create the router so we can pre-seed a stale NAT rule against it. - require.NoError(t, ovn.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-vpc-nat3"})) - require.NoError(t, ovn.AddNAT(ctx, "vpc-vpc-nat3", &nbdb.NAT{ - Type: "dnat_and_snat", ExternalIP: "10.0.0.230", LogicalIP: "10.30.1.99", - })) - - result := ReconcileFromKV(ctx, nc, topo, nil) - assert.Equal(t, 1, result.NATsReconciled, "stale NAT rule must be replaced") - - nat, err := ovn.FindNATByExternalIP(ctx, "dnat_and_snat", "10.0.0.230") - require.NoError(t, err) - require.NotNil(t, nat) - assert.Equal(t, "10.30.1.9", nat.LogicalIP, "logical IP must be updated to match KV") -} diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index d49430d6d..0a16b64d5 100644 --- a/spinifex/services/vpcd/topology.go +++ b/spinifex/services/vpcd/topology.go @@ -1441,108 +1441,6 @@ func (h *TopologyHandler) handleDeleteNAT(msg *nats.Msg) { // --- Reconciliation (called on startup, not via NATS) --- -// reconcileVPC creates the OVN logical router for a VPC if it doesn't exist. -func (h *TopologyHandler) reconcileVPC(ctx context.Context, vpcId, cidr string) error { - routerName := "vpc-" + vpcId - - lr := &nbdb.LogicalRouter{ - Name: routerName, - ExternalIDs: map[string]string{ - "spinifex:vpc_id": vpcId, - "spinifex:cidr": cidr, - }, - } - if err := h.ovn.CreateLogicalRouter(ctx, lr); err != nil { - return fmt.Errorf("create router %s: %w", routerName, err) - } - - slog.Info("vpcd reconcile: created VPC router", "router", routerName, "vpc_id", vpcId) - return nil -} - -// reconcileSubnet creates the OVN logical switch, router port, and DHCP options for a subnet. -func (h *TopologyHandler) reconcileSubnet(ctx context.Context, subnetId, vpcId, cidr string) error { - switchName := "subnet-" + subnetId - routerName := "vpc-" + vpcId - routerPortName := "rtr-" + subnetId - switchRouterPortName := "rtr-port-" + subnetId - - gwIP, mask, err := subnetGateway(cidr) - if err != nil { - return fmt.Errorf("compute gateway for %s: %w", cidr, err) - } - gwCIDR := fmt.Sprintf("%s/%d", gwIP, mask) - routerMAC := generateMAC(subnetId) - - // 1. Create LogicalSwitch - ls := &nbdb.LogicalSwitch{ - Name: switchName, - ExternalIDs: map[string]string{ - "spinifex:subnet_id": subnetId, - "spinifex:vpc_id": vpcId, - }, - } - if err := h.ovn.CreateLogicalSwitch(ctx, ls); err != nil { - return fmt.Errorf("create switch %s: %w", switchName, err) - } - - // 2. Create LogicalRouterPort - lrp := &nbdb.LogicalRouterPort{ - Name: routerPortName, - MAC: routerMAC, - Networks: []string{gwCIDR}, - ExternalIDs: map[string]string{ - "spinifex:subnet_id": subnetId, - "spinifex:vpc_id": vpcId, - }, - } - if err := h.ovn.CreateLogicalRouterPort(ctx, routerName, lrp); err != nil { - _ = h.ovn.DeleteLogicalSwitch(ctx, switchName) - return fmt.Errorf("create router port %s: %w", routerPortName, err) - } - - // 3. Create LogicalSwitchPort (type=router) - lsp := &nbdb.LogicalSwitchPort{ - Name: switchRouterPortName, - Type: "router", - Addresses: []string{"router"}, - Options: map[string]string{"router-port": routerPortName}, - ExternalIDs: map[string]string{ - "spinifex:subnet_id": subnetId, - "spinifex:vpc_id": vpcId, - }, - } - if err := h.ovn.CreateLogicalSwitchPort(ctx, switchName, lsp); err != nil { - _ = h.ovn.DeleteLogicalRouterPort(ctx, routerName, routerPortName) - _ = h.ovn.DeleteLogicalSwitch(ctx, switchName) - return fmt.Errorf("create switch router port %s: %w", switchRouterPortName, err) - } - - // 4. Create DHCP options - dhcpOpts := &nbdb.DHCPOptions{ - CIDR: cidr, - Options: map[string]string{ - "server_id": gwIP, - "server_mac": routerMAC, - "lease_time": "3600", - "router": gwIP, - "dns_server": h.dnsServer(), - "mtu": "1442", - }, - ExternalIDs: map[string]string{ - "spinifex:subnet_id": subnetId, - "spinifex:vpc_id": vpcId, - }, - } - if _, err := h.ovn.CreateDHCPOptions(ctx, dhcpOpts); err != nil { - slog.Warn("vpcd reconcile: failed to create DHCP options (non-fatal)", "cidr", cidr, "err", err) - } - - slog.Info("vpcd reconcile: created subnet topology", - "switch", switchName, "router_port", routerPortName, "gateway", gwCIDR) - return nil -} - // 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 @@ -1695,74 +1593,6 @@ func (h *TopologyHandler) reconcileIGW(ctx context.Context, vpcId, igwId string) return nil } -// reconcileGatewayChassis brings every gateway LRP's HA-scheduling state into -// agreement with the live SBDB chassis list. Two-phase: -// -// 1. Delete Gateway_Chassis rows whose chassis_name no longer matches any -// entry in validNames. These rows are inevitable when the OVS system-id -// changes across a reboot (mulga-999): NB still references the old -// chassis-* name, but the SB Chassis row is now under a UUID, so OVN -// can't resolve the binding and cr-gw* port-bindings sit empty. -// -// 2. Re-bind every LRP tagged spinifex:role=gateway against validNames via -// the idempotent SetGatewayChassis. Existing rows with matching priority -// are no-ops; mismatched priorities are mutated; missing rows are -// created. -// -// Called as the first step of ReconcileFromKV so chassis state is correct -// before any reconcileVPC / reconcileIGW work. Failures are logged and -// returned but do not abort the rest of the reconcile loop — partial -// progress is better than none. -func (h *TopologyHandler) reconcileGatewayChassis(ctx context.Context, validNames []string) error { - valid := make(map[string]struct{}, len(validNames)) - for _, n := range validNames { - valid[n] = struct{}{} - } - - rows, err := h.ovn.ListGatewayChassis(ctx) - if err != nil { - return fmt.Errorf("list gateway_chassis: %w", err) - } - for _, gc := range rows { - if _, ok := valid[gc.ChassisName]; ok { - continue - } - // Gateway_Chassis.Name is "lrpName-chassisName" (see SetGatewayChassis). - // The chassis suffix is unique because chassis names are globally - // unique; trimming it leaves the owning LRP name. - lrpName := strings.TrimSuffix(gc.Name, "-"+gc.ChassisName) - if err := h.ovn.DeleteGatewayChassis(ctx, lrpName, gc.UUID); err != nil { - slog.Warn("vpcd: failed to delete stale gateway_chassis", - "row", gc.Name, "chassis_name", gc.ChassisName, "err", err) - continue - } - slog.Warn("vpcd: deleted stale gateway_chassis", - "row", gc.Name, "chassis_name", gc.ChassisName, "lrp", lrpName) - } - - if len(validNames) == 0 { - return nil - } - - lrps, err := h.ovn.ListLogicalRouterPorts(ctx) - if err != nil { - return fmt.Errorf("list logical_router_port: %w", err) - } - for _, lrp := range lrps { - if lrp.ExternalIDs["spinifex:role"] != "gateway" { - continue - } - for i, name := range validNames { - priority := max(20-(i*5), 1) - if err := h.ovn.SetGatewayChassis(ctx, lrp.Name, name, priority); err != nil { - slog.Warn("vpcd: failed to set gateway chassis on rebind", - "lrp", lrp.Name, "chassis", name, "priority", priority, "err", err) - } - } - } - return nil -} - // --- Helpers --- // gwLrpRange returns the per-VPC gateway LRP IP range for a pool. Priority: diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index 9cb8803fa..0576a12c6 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/spinifex/services/vpcd/topology_test.go @@ -1054,7 +1054,7 @@ func TestTopologyHandler_AddNAT_Idempotent(t *testing.T) { t.Errorf("second add-nat ran flow barrier: waitForFlowsHV called %d times, want 1", barrier) } - nat := mock.nats[router.NAT[0]] + nat := mock.NATs[router.NAT[0]] if nat.LogicalIP != evt.LogicalIP { t.Errorf("NAT logical_ip=%q want %q", nat.LogicalIP, evt.LogicalIP) } diff --git a/spinifex/services/vpcd/vpcd.go b/spinifex/services/vpcd/vpcd.go index 3a524164c..9fbdf3f65 100644 --- a/spinifex/services/vpcd/vpcd.go +++ b/spinifex/services/vpcd/vpcd.go @@ -15,6 +15,10 @@ import ( "time" "github.com/mulgadc/spinifex/spinifex/admin" + "github.com/mulgadc/spinifex/spinifex/network/external" + "github.com/mulgadc/spinifex/spinifex/network/host" + "github.com/mulgadc/spinifex/spinifex/network/policy" + "github.com/mulgadc/spinifex/spinifex/network/reconcile" "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/nats-io/nats.go" @@ -78,6 +82,10 @@ type Config struct { // directly to the OVS bridge; veth uses a veth pair to link a Linux bridge // to OVS. When empty, auto-detected at startup. BridgeMode string + // AZ is the local availability zone identifier. The reconciler uses it + // to scope its IntentState scan to local-AZ KV records; new VPC records + // are stamped with this value at create time. + AZ string } // ExternalPoolConfig mirrors config.ExternalPool for vpcd's internal use. @@ -455,7 +463,7 @@ func launchService(cfg *Config) error { // Runtime VPC events still fan out via the vpcd-workers queue group, so // non-leaders remain functional after Subscribe below. holder, _ := os.Hostname() - releaseLeader, isLeader := AcquireReconcileLeader(nc, holder) + releaseLeader, isLeader := reconcile.AcquireLeader(nc, holder) subs, err := topo.Subscribe(nc) if err != nil { @@ -514,49 +522,81 @@ func launchService(cfg *Config) error { } }() - // Pass 1: Bootstrap-config reconcile. Creates the bootstrap VPC / IGW - // topology if missing. Leader-only. - if isLeader { - Reconcile(ctx, topo, cfg.Bootstrap) + // Construct the network/reconcile manager stack. The reconciler replaces + // the five-pass startup sequence (Reconcile + ReconcileFromKV + + // ReconcileSGsOnce + RetrofitAllExternalLocalnetOptions + + // RetrofitAllGatewayPortNetworks) with a single intent-driven pass that + // also runs on a 5-minute drift ticker (replaces the 30s ReconcileSGsLoop). + uplinkMode := host.UplinkModePhysical + if bridgeMode == BridgeModeVeth { + uplinkMode = host.UplinkModeVeth + } + natMode := policy.NATModeFromUplinkMode(uplinkMode) + + sgMgr := policy.NewSecurityGroupManager(liveClient) + natMgr, err := policy.NewNATManager(liveClient, natMode) + if err != nil { + return fmt.Errorf("construct NAT manager: %w", err) + } + routeMgr := policy.NewRouteManager(liveClient) + + var igwPool *external.ExternalPoolConfig + if cfg.ExternalMode != "" && len(cfg.ExternalPools) > 0 { + p := externalPoolConfigToShared(cfg.ExternalPools[0]) + igwPool = &p + } + igwMgr, err := external.NewIGWManager(external.IGWManagerConfig{ + OVN: liveClient, + Routes: routeMgr, + NAT: natMgr, + Pool: igwPool, + Allocator: external.NewStaticRangeAllocator(liveClient), + Chassis: chassisNames, + NATMode: natMode, + FlowsBarrier: waitForFlowsHV, + }) + if err != nil { + return fmt.Errorf("construct IGW manager: %w", err) } - // Pass 2: Reconcile from NATS KV (handles reboots, OVN DB loss, missed events). - // Runs after subscribing so new events are not missed during reconciliation. - // Leader-gated (mulga-siv-29) to avoid concurrent Create races against OVN NB. - // - // ReconcileFromKV creates ENI LSPs with the non-atomic primitive (no SG - // port-group joins). Calling ReconcileSGsOnce immediately after — under the - // same leader lock — recreates any missing port groups and joins the - // fresh LSPs to them, closing the enforcement gap before the periodic - // loop's first 30s tick. - if isLeader { - ReconcileFromKV(ctx, nc, topo, chassisNames) - ReconcileSGsOnce(ctx, nc, topo) + rec, err := reconcile.New(reconcile.Config{ + OVN: liveClient, + SG: sgMgr, + NAT: natMgr, + Routes: routeMgr, + IGW: igwMgr, + LocalAZ: cfg.AZ, + NodeHostname: holder, + Chassis: chassisNames, + }) + if err != nil { + return fmt.Errorf("construct reconciler: %w", err) } - // 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. - // Pass 4: Retrofit gateway-port Networks. Reconcile gates IGW work on - // "ext switch missing", so a cluster shipped with the pool-IP bug - // (mulga-siv-26) never re-enters reconcileIGW. Walk every gateway LRP - // directly and rewrite stale Networks to link-local in place. - // Both leader-gated (mulga-siv-29) — these read-modify-write OVN NB and - // would race against the leader's Reconcile/ReconcileFromKV otherwise. + // Startup reconcile: leader-gated read of NATS KV intent state, applied + // once against OVN NB DB. The drift loop below handles the recovery case + // (KV not yet populated when this fires — daemon's EnsureDefaultVPC may + // race with vpcd's startup). Subscribers above keep handling per-event + // NATS messages via the legacy TopologyHandler path; phase 4 migrates + // those onto the new managers. if isLeader { - topo.RetrofitAllExternalLocalnetOptions(ctx) - topo.RetrofitAllGatewayPortNetworks(ctx) + intent, intentErr := reconcile.LoadIntentFromKV(ctx, js, cfg.AZ) + if intentErr != nil { + slog.Warn("vpcd: startup intent load failed", "err", intentErr) + } else if err := rec.Reconcile(ctx, intent); err != nil { + slog.Warn("vpcd: startup reconcile failed", "err", err) + } releaseLeader() } - // Periodic SG/port-group/address-set drift convergence. Leader-gated on the - // shared reconcile bucket so only one vpcd in the cluster runs the scans at - // a time; cancelled when the parent ctx is. + // Periodic drift detection. Leader-gated on the shared reconcile bucket + // so only one vpcd in the cluster scans at a time; cancelled when the + // parent ctx is. loopCtx, loopCancel := context.WithCancel(ctx) defer loopCancel() loopDone := make(chan struct{}) go func() { - ReconcileSGsLoop(loopCtx, nc, topo) + reconcile.DriftLoop(loopCtx, rec, nc, cfg.AZ, holder) close(loopDone) }() @@ -616,6 +656,28 @@ func newDHCPManagerWithRetry(ctx context.Context, nc *nats.Conn, js nats.JetStre } } +// externalPoolConfigToShared translates the vpcd-local ExternalPoolConfig +// into the network/external package's shared shape. The two will merge +// once topology.go is split (bead mulga-siv-125.3.4 — see external.go's +// type doc); until then this is the seam. +func externalPoolConfigToShared(p ExternalPoolConfig) external.ExternalPoolConfig { + return external.ExternalPoolConfig{ + Name: p.Name, + Source: p.Source, + RangeStart: p.RangeStart, + RangeEnd: p.RangeEnd, + Gateway: p.Gateway, + GatewayIP: p.GatewayIP, + PrefixLen: p.PrefixLen, + DNSServers: p.DNSServers, + Region: p.Region, + AZ: p.AZ, + DhcpBindBridge: p.DhcpBindBridge, + GwLrpRangeStart: p.GwLrpRangeStart, + GwLrpRangeEnd: p.GwLrpRangeEnd, + } +} + // 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