diff --git a/.github/scripts/capture-net-diag.sh b/.github/scripts/capture-net-diag.sh index 43e9123fe..11a1c1dc7 100755 --- a/.github/scripts/capture-net-diag.sh +++ b/.github/scripts/capture-net-diag.sh @@ -42,6 +42,25 @@ run ovn.sb_show sudo ovn-sbctl show run ovn.nb_show sudo ovn-nbctl show run ovn.sb_chassis sudo ovn-sbctl list chassis +run_per_lr() { + local label="$1"; shift + local subcmd="$1"; shift + { + echo "# ovn-nbctl $subcmd " + local lrs + lrs="$(sudo ovn-nbctl --bare --columns=name list Logical_Router 2>/dev/null)" + for lr in $lrs; do + echo + echo "=== $lr ===" + sudo ovn-nbctl "$subcmd" "$lr" 2>&1 + done + } > "$OUT/$label" || true +} + +run_per_lr ovn.lr_routes lr-route-list +run_per_lr ovn.lr_policies lr-policy-list +run_per_lr ovn.lr_nat lr-nat-list + run ovs.show sudo ovs-vsctl show run ovs.flows_br_int sudo ovs-ofctl dump-flows br-int run ovs.tunnels sudo ovs-appctl ofproto/list-tunnels br-int @@ -49,7 +68,21 @@ run ovs.tunnels sudo ovs-appctl ofproto/list-tunnels br- run net.links ip -s link show run net.routes ip route show table all run net.addrs ip -br addr show +run net.neigh ip neigh show +run net.bridge_fdb bridge fdb show +run net.conntrack sudo conntrack -L run systemd.failed_units sudo systemctl list-units --state=failed --no-pager +# Background pcap (started by start-net-capture.sh, if present). +if [ -f /tmp/spinifex-e2e-tcpdump.pid ]; then + PID="$(cat /tmp/spinifex-e2e-tcpdump.pid 2>/dev/null)" + sudo kill -INT "$PID" 2>/dev/null || true + sleep 1 + if [ -f /tmp/spinifex-e2e-wan.pcap ]; then + sudo cp /tmp/spinifex-e2e-wan.pcap "$OUT/wan.pcap" 2>/dev/null || true + sudo chmod 0644 "$OUT/wan.pcap" 2>/dev/null || true + fi +fi + exit 0 diff --git a/.github/scripts/start-net-capture.sh b/.github/scripts/start-net-capture.sh new file mode 100755 index 000000000..05a6d2a23 --- /dev/null +++ b/.github/scripts/start-net-capture.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# start-net-capture.sh — kick off background tcpdump on br-wan. +# Pair: capture-net-diag.sh stops it and copies the pcap into net-diag/. +# +# Filter: anything on br-wan involving the EIP pool (192.168.0.0/23) plus +# ARP for the same range. Single ring buffer (-W 1 -G 0) → grows until stop; +# capped at 64 MiB via -C 64. Snaplen 200 bytes (headers + a bit) to keep +# size sane while preserving L2/L3/L4 + first payload bytes. +set +e + +PCAP=/tmp/spinifex-e2e-wan.pcap +PIDFILE=/tmp/spinifex-e2e-tcpdump.pid + +if [ -f "$PIDFILE" ] && kill -0 "$(cat "$PIDFILE" 2>/dev/null)" 2>/dev/null; then + echo "[start-net-capture] tcpdump already running pid=$(cat "$PIDFILE")" + exit 0 +fi + +sudo rm -f "$PCAP" "$PIDFILE" + +if ! command -v tcpdump >/dev/null 2>&1; then + echo "[start-net-capture] tcpdump not installed — skipping" >&2 + exit 0 +fi +if ! ip link show br-wan >/dev/null 2>&1; then + echo "[start-net-capture] br-wan absent — skipping" >&2 + exit 0 +fi + +sudo nohup tcpdump -i br-wan -s 200 -C 64 -W 1 -U \ + -w "$PCAP" \ + '(net 192.168.0.0/23) or (arp and (arp[24:4]&0xfffffe00=0xc0a80000))' \ + >/tmp/spinifex-e2e-tcpdump.log 2>&1 & +PID=$! +echo "$PID" | sudo tee "$PIDFILE" >/dev/null +disown + +sleep 1 +if ! kill -0 "$PID" 2>/dev/null; then + echo "[start-net-capture] tcpdump died — see /tmp/spinifex-e2e-tcpdump.log" >&2 + cat /tmp/spinifex-e2e-tcpdump.log >&2 || true + sudo rm -f "$PIDFILE" + exit 1 +fi + +echo "[start-net-capture] tcpdump pid=$PID pcap=$PCAP" +exit 0 diff --git a/.github/workflows/e2e-go.yml b/.github/workflows/e2e-go.yml index 1258a66cd..9236d7382 100644 --- a/.github/workflows/e2e-go.yml +++ b/.github/workflows/e2e-go.yml @@ -215,6 +215,16 @@ jobs: tar -C spinifex/tests/e2e/_bin -cf - . | ssh "${SSH_OPTS[@]}" "$VM" "tar -xf - -C \$HOME/spinifex-e2e-bin" ok "shipped binaries to $VM" + - name: Start packet capture on VM br-wan + run: | + set +e + SSH_OPTS=(-o StrictHostKeyChecking=no -i "${{ steps.vm.outputs.ssh_key }}") + VM=tf-user@${{ steps.vm.outputs.wan_ip }} + scp "${SSH_OPTS[@]}" \ + "${GITHUB_WORKSPACE}/spinifex/.github/scripts/start-net-capture.sh" \ + "$VM":/tmp/start-net-capture.sh + ssh "${SSH_OPTS[@]}" "$VM" "bash /tmp/start-net-capture.sh" || true + - name: Run Go E2E suites id: e2e run: | @@ -581,6 +591,19 @@ jobs: tar -C spinifex/tests/e2e/_bin -cf - . | ssh "${SSH_OPTS[@]}" "$VM" "tar -xf - -C \$HOME/spinifex-e2e-bin" ok "shipped binaries to $VM" + - name: Start packet capture on all nodes br-wan + run: | + set +e + SSH_OPTS=(-o StrictHostKeyChecking=no -i "${{ steps.cluster.outputs.ssh_key }}") + IFS=',' read -ra NODE_LIST <<< "${{ steps.cluster.outputs.nodes_csv }}" + for ip in "${NODE_LIST[@]}"; do + [ -z "$ip" ] && continue + scp "${SSH_OPTS[@]}" \ + "${GITHUB_WORKSPACE}/spinifex/.github/scripts/start-net-capture.sh" \ + tf-user@"$ip":/tmp/start-net-capture.sh + ssh "${SSH_OPTS[@]}" tf-user@"$ip" "bash /tmp/start-net-capture.sh" || true + done + - name: Pre-flight diagnostics on node1 # Captures cluster + tf-user state directly via SSH so the post-mortem # in run 26163994815-style spx-NATS failures has structured data diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 50d8e3156..8689cccda 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -106,6 +106,10 @@ type ResourceManager struct { nodeID string // node identifier for node-specific topic subscriptions } +// Compile-time guarantee that the RouteTable service satisfies the IGW +// handler's GatePublisher hook — the two services are wired together below. +var _ handlers_ec2_igw.GatePublisher = (*handlers_ec2_routetable.RouteTableServiceImpl)(nil) + // Daemon represents the main daemon service type Daemon struct { node string @@ -1137,6 +1141,9 @@ func (d *Daemon) startCluster() error { return fmt.Errorf("failed to initialize RouteTable service: %w", err) } + // Wire IGW attach/detach to RT-aware per-subnet egress gate fan-out. + d.igwService.SetGatePublisher(d.routeTableService) + d.natGatewayService, err = initServiceWithRetry("NatGateway service", func() (*handlers_ec2_natgw.NatGatewayServiceImpl, error) { return handlers_ec2_natgw.NewNatGatewayServiceImplWithNATS(d.natsConn) }) diff --git a/spinifex/handlers/ec2/igw/service_impl.go b/spinifex/handlers/ec2/igw/service_impl.go index fcb0d7493..5493503e3 100644 --- a/spinifex/handlers/ec2/igw/service_impl.go +++ b/spinifex/handlers/ec2/igw/service_impl.go @@ -37,12 +37,29 @@ type IGWRecord struct { CreatedAt time.Time `json:"created_at"` } +// GatePublisher recomputes the per-subnet egress gate/ungate decision for +// every subnet in a VPC. Wired by the daemon after IGW + RouteTable services +// are constructed so AttachInternetGateway / DetachInternetGateway can fan +// out gate decisions immediately instead of waiting up to 5 minutes for the +// reconciler's drift tick to install OVN DROP policies on unrouted subnets. +// Nil-safe: when unset the handler falls back to publish-only behaviour. +type GatePublisher interface { + PublishGateDecisionsForVPC(accountID, vpcID, destCidr string) +} + // IGWServiceImpl implements Internet Gateway operations with NATS JetStream persistence type IGWServiceImpl struct { - config *config.Config - igwKV nats.KeyValue - vpcKV nats.KeyValue - natsConn *nats.Conn + config *config.Config + igwKV nats.KeyValue + vpcKV nats.KeyValue + natsConn *nats.Conn + gatePublisher GatePublisher +} + +// SetGatePublisher installs the cross-handler fan-out hook. Called by the +// daemon after the RouteTable service is constructed. +func (s *IGWServiceImpl) SetGatePublisher(p GatePublisher) { + s.gatePublisher = p } // NewIGWServiceImplWithNATS creates an Internet Gateway service with NATS JetStream for persistence @@ -315,6 +332,14 @@ func (s *IGWServiceImpl) AttachInternetGateway(input *ec2.AttachInternetGatewayI } } + // Gate fan-out intentionally skipped on attach. The default-VPC + // bootstrap path attaches the IGW and immediately publishes a + // 0.0.0.0/0 → IGW CreateRoute event; the two race across distinct + // NATS subjects (gate vs ungate) and ungate-then-gate leaves the + // subnet DROPped. Subnets without a default route stay protected by + // the reconciler's drift pass (loadSubnetDropGates). Detach is + // race-free because no follow-up CreateRoute fires. + slog.Info("AttachInternetGateway completed", "internetGatewayId", igwID, "vpcId", vpcID, "accountID", accountID) return &ec2.AttachInternetGatewayOutput{}, nil @@ -373,6 +398,14 @@ func (s *IGWServiceImpl) DetachInternetGateway(input *ec2.DetachInternetGatewayI } } + // After detach the VPC's LR external gateway and router-wide default + // route are removed; any subnet whose effective RT still points at the + // (now-blackholed) IGW must surface the ungate→gate transition so the + // reconciler drops the previous ungate policy in favour of a DROP. + if s.gatePublisher != nil { + s.gatePublisher.PublishGateDecisionsForVPC(accountID, vpcID, "0.0.0.0/0") + } + slog.Info("DetachInternetGateway completed", "internetGatewayId", igwID, "vpcId", vpcID, "accountID", accountID) return &ec2.DetachInternetGatewayOutput{}, nil diff --git a/spinifex/handlers/ec2/igw/service_impl_test.go b/spinifex/handlers/ec2/igw/service_impl_test.go index b01e55887..6fefe0cef 100644 --- a/spinifex/handlers/ec2/igw/service_impl_test.go +++ b/spinifex/handlers/ec2/igw/service_impl_test.go @@ -1,6 +1,7 @@ package handlers_ec2_igw import ( + "sync" "testing" "time" @@ -14,6 +15,35 @@ import ( "github.com/stretchr/testify/require" ) +// fakeGatePublisher records every PublishGateDecisionsForVPC invocation so +// tests can assert the IGW handler fans out at attach / detach time. +type fakeGatePublisher struct { + mu sync.Mutex + calls []gateCall +} + +type gateCall struct { + AccountID string + VpcID string + DestCidr string +} + +func (p *fakeGatePublisher) PublishGateDecisionsForVPC(accountID, vpcID, destCidr string) { + p.mu.Lock() + defer p.mu.Unlock() + p.calls = append(p.calls, gateCall{accountID, vpcID, destCidr}) +} + +func (p *fakeGatePublisher) snapshot() []gateCall { + p.mu.Lock() + defer p.mu.Unlock() + out := make([]gateCall, len(p.calls)) + copy(out, p.calls) + return out +} + +var _ GatePublisher = (*fakeGatePublisher)(nil) + func setupTestIGWService(t *testing.T) (*IGWServiceImpl, *nats.Conn) { t.Helper() _, nc, js := testutil.StartTestJetStream(t) @@ -668,3 +698,62 @@ func TestDeleteInternetGateway_PublishesNoEvent(t *testing.T) { // Expected — no event } } + +// TestAttachInternetGateway_NoGateFanOut documents that attach intentionally +// skips the gate fan-out to avoid racing the default-VPC bootstrap path, +// which attaches the IGW and immediately adds a 0.0.0.0/0 → IGW route. The +// two events would race across the gate/ungate NATS subjects and leave the +// default subnet DROPped when ungate landed first. +func TestAttachInternetGateway_NoGateFanOut(t *testing.T) { + svc, _ := setupTestIGWService(t) + pub := &fakeGatePublisher{} + svc.SetGatePublisher(pub) + igwID := createTestIGW(t, svc) + + _, err := svc.AttachInternetGateway(&ec2.AttachInternetGatewayInput{ + InternetGatewayId: aws.String(igwID), + VpcId: aws.String("vpc-test123"), + }, testAccountID) + require.NoError(t, err) + + assert.Empty(t, pub.snapshot(), "attach must not fan out gate decisions") +} + +func TestDetachInternetGateway_FanOutsGateDecisionsForVPC(t *testing.T) { + svc, _ := setupTestIGWService(t) + igwID := createTestIGW(t, svc) + + _, err := svc.AttachInternetGateway(&ec2.AttachInternetGatewayInput{ + InternetGatewayId: aws.String(igwID), + VpcId: aws.String("vpc-test123"), + }, testAccountID) + require.NoError(t, err) + + // Wire publisher AFTER attach so detach is the only fan-out we observe. + pub := &fakeGatePublisher{} + svc.SetGatePublisher(pub) + + _, err = svc.DetachInternetGateway(&ec2.DetachInternetGatewayInput{ + InternetGatewayId: aws.String(igwID), + VpcId: aws.String("vpc-test123"), + }, testAccountID) + require.NoError(t, err) + + calls := pub.snapshot() + require.Len(t, calls, 1, "expected one gate fan-out call on detach") + assert.Equal(t, testAccountID, calls[0].AccountID) + assert.Equal(t, "vpc-test123", calls[0].VpcID) + assert.Equal(t, "0.0.0.0/0", calls[0].DestCidr) +} + +func TestAttachInternetGateway_NoGatePublisher_NoOp(t *testing.T) { + svc, _ := setupTestIGWService(t) + igwID := createTestIGW(t, svc) + + // Default state — gatePublisher nil. Must not panic. + _, err := svc.AttachInternetGateway(&ec2.AttachInternetGatewayInput{ + InternetGatewayId: aws.String(igwID), + VpcId: aws.String("vpc-test123"), + }, testAccountID) + require.NoError(t, err) +} diff --git a/spinifex/handlers/ec2/routetable/service_impl.go b/spinifex/handlers/ec2/routetable/service_impl.go index b41a3e40c..902bcba09 100644 --- a/spinifex/handlers/ec2/routetable/service_impl.go +++ b/spinifex/handlers/ec2/routetable/service_impl.go @@ -128,6 +128,289 @@ func (s *RouteTableServiceImpl) getVPCCidr(accountID, vpcID string) (string, err return vpcRecord.CidrBlock, nil } +// mainRouteTable returns the VPC's main route table or (nil, nil) if none +// exists. Used to enumerate effective routes for subnets that have no +// explicit RT association (AWS implicit-main semantics). +// +// Deterministic across duplicate IsMain=true records: see preferMain. A +// duplicate is a bootstrap-race symptom — the canonical fix lives in +// VPCServiceImpl.createMainRouteTable, but the picker stays defensive so a +// stale dup (e.g. cross-AZ restore, manual KV edit) can't break gate +// recomputation by selecting the orphan non-deterministically. +func (s *RouteTableServiceImpl) mainRouteTable(accountID, vpcID string) (*RouteTableRecord, error) { + rts, err := s.allRouteTablesForVPC(accountID, vpcID) + if err != nil { + return nil, err + } + var main *RouteTableRecord + for i := range rts { + if rts[i].IsMain && preferMain(main, &rts[i]) { + main = &rts[i] + } + } + return main, nil +} + +// subnetsImplicitlyOnMainRT returns SubnetIds in vpcID that have no explicit +// non-main association on any RT — they inherit the main RT's routes per AWS +// semantics, so main-RT route events must fan out to them. +func (s *RouteTableServiceImpl) subnetsImplicitlyOnMainRT(accountID, vpcID string) ([]string, error) { + rts, err := s.allRouteTablesForVPC(accountID, vpcID) + if err != nil { + return nil, err + } + explicit := map[string]bool{} + for _, rt := range rts { + for _, assoc := range rt.Associations { + if assoc.SubnetId == "" || assoc.Main { + continue + } + explicit[assoc.SubnetId] = true + } + } + + prefix := accountID + "." + keys, err := s.subnetKV.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return nil, nil + } + return nil, fmt.Errorf("list subnet keys: %w", err) + } + var implicit []string + for _, key := range keys { + if key == utils.VersionKey || !strings.HasPrefix(key, prefix) { + continue + } + entry, err := s.subnetKV.Get(key) + if err != nil { + if errors.Is(err, nats.ErrKeyNotFound) { + continue + } + return nil, fmt.Errorf("read subnet %s: %w", key, err) + } + var subnet handlers_ec2_vpc.SubnetRecord + if err := json.Unmarshal(entry.Value(), &subnet); err != nil { + slog.Warn("subnetsImplicitlyOnMainRT: corrupt subnet record", "key", key, "err", err) + continue + } + if subnet.VpcId != vpcID || subnet.SubnetId == "" { + continue + } + if explicit[subnet.SubnetId] { + continue + } + implicit = append(implicit, subnet.SubnetId) + } + return implicit, nil +} + +// effectiveRouteTable returns the route table whose routes apply to subnetID +// per AWS semantics: explicit non-main association if one exists, otherwise +// the VPC's main RT. Returns (nil, nil) when neither is present. +// +// Main-RT selection is deterministic across IsMain=true duplicates via +// preferMain — see mainRouteTable. +func (s *RouteTableServiceImpl) effectiveRouteTable(accountID, vpcID, subnetID string) (*RouteTableRecord, error) { + rts, err := s.allRouteTablesForVPC(accountID, vpcID) + if err != nil { + return nil, err + } + var main *RouteTableRecord + for i := range rts { + for _, assoc := range rts[i].Associations { + if assoc.SubnetId == subnetID && !assoc.Main { + return &rts[i], nil + } + } + if rts[i].IsMain && preferMain(main, &rts[i]) { + main = &rts[i] + } + } + return main, nil +} + +// preferMain reports whether candidate should replace current as the chosen +// main RT. Ordered tiebreakers when two IsMain=true records survive for one +// VPC: (1) more routes — user-added 0.0.0.0/0 etc. flag the "real" RT vs an +// orphan that only carries the implicit local route; (2) oldest CreatedAt — +// the original wins over a late dup; (3) smaller RouteTableId — total order +// so the same input always produces the same answer across nodes. +func preferMain(current, candidate *RouteTableRecord) bool { + if current == nil { + return true + } + if len(candidate.Routes) != len(current.Routes) { + return len(candidate.Routes) > len(current.Routes) + } + if !candidate.CreatedAt.Equal(current.CreatedAt) { + return candidate.CreatedAt.Before(current.CreatedAt) + } + return candidate.RouteTableId < current.RouteTableId +} + +// subnetHasDefaultEgress reports whether subnetID's effective RT carries a +// destCidr route pointing at any internet-bound target (IGW or NAT GW). If +// false, the subnet must be gated with a DROP policy because the VPC LR's +// router-wide default static route would otherwise let packets egress. +func (s *RouteTableServiceImpl) subnetHasDefaultEgress(accountID, vpcID, subnetID, destCidr string) (bool, error) { + rt, err := s.effectiveRouteTable(accountID, vpcID, subnetID) + if err != nil { + return false, err + } + if rt == nil { + return false, nil + } + for _, r := range rt.Routes { + if r.DestinationCidrBlock != destCidr { + continue + } + if r.GatewayId != "" && strings.HasPrefix(r.GatewayId, "igw-") { + return true, nil + } + if r.NatGatewayId != "" { + return true, nil + } + } + return false, nil +} + +// publishSubnetEgressGateDecision recomputes the gate/ungate decision for one +// subnet+destCidr pair and emits either vpc.gate-subnet-egress (when the +// effective RT lacks an internet-bound route) or vpc.ungate-subnet-egress +// (when one is present). Called after every per-subnet IGW/NATGW event so +// the drop policy state always matches the AWS RT view. Side-effect only; +// errors are logged. +func (s *RouteTableServiceImpl) publishSubnetEgressGateDecision(accountID, vpcID, subnetID, destCidr string) { + if s.natsConn == nil || subnetID == "" || vpcID == "" { + return + } + if destCidr == "" { + destCidr = "0.0.0.0/0" + } + hasEgress, err := s.subnetHasDefaultEgress(accountID, vpcID, subnetID, destCidr) + if err != nil { + slog.Warn("subnet egress gate: effective RT lookup failed", + "vpcId", vpcID, "subnetId", subnetID, "err", err) + return + } + topic := "vpc.gate-subnet-egress" + if hasEgress { + topic = "vpc.ungate-subnet-egress" + } + evt := struct { + VpcId string `json:"vpc_id"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` + }{ + VpcId: vpcID, + SubnetId: subnetID, + DestinationCidr: destCidr, + } + data, err := json.Marshal(evt) + if err != nil { + slog.Warn("subnet egress gate: marshal failed", "topic", topic, "err", err) + return + } + if err := s.natsConn.Publish(topic, data); err != nil { + slog.Warn("subnet egress gate: publish failed", "topic", topic, "subnetId", subnetID, "err", err) + return + } + slog.Info("subnet egress gate decision published", + "topic", topic, "vpcId", vpcID, "subnetId", subnetID, "destinationCidr", destCidr) +} + +// PublishGateDecisionsForVPC recomputes gate/ungate for every subnet in +// vpcID against destCidr by enumerating all subnets in the VPC and resolving +// each one's effective RT. Called from the IGW handler on Attach / Detach so +// per-subnet OVN DROP policies are reinstated immediately instead of waiting +// for the reconciler's drift tick. destCidr defaults to 0.0.0.0/0. +func (s *RouteTableServiceImpl) PublishGateDecisionsForVPC(accountID, vpcID, destCidr string) { + if s.natsConn == nil || vpcID == "" { + return + } + if destCidr == "" { + destCidr = "0.0.0.0/0" + } + subnets, err := s.allSubnetsForVPC(accountID, vpcID) + if err != nil { + slog.Warn("PublishGateDecisionsForVPC: enumerate subnets failed", + "vpcId", vpcID, "err", err) + return + } + for _, subnetID := range subnets { + s.publishSubnetEgressGateDecision(accountID, vpcID, subnetID, destCidr) + } +} + +// allSubnetsForVPC returns every SubnetId in vpcID by scanning the subnet KV. +// Order is unspecified. +func (s *RouteTableServiceImpl) allSubnetsForVPC(accountID, vpcID string) ([]string, error) { + prefix := accountID + "." + keys, err := s.subnetKV.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return nil, nil + } + return nil, fmt.Errorf("list subnet keys: %w", err) + } + var subnets []string + for _, key := range keys { + if key == utils.VersionKey || !strings.HasPrefix(key, prefix) { + continue + } + entry, err := s.subnetKV.Get(key) + if err != nil { + if errors.Is(err, nats.ErrKeyNotFound) { + continue + } + return nil, fmt.Errorf("read subnet %s: %w", key, err) + } + var subnet handlers_ec2_vpc.SubnetRecord + if err := json.Unmarshal(entry.Value(), &subnet); err != nil { + slog.Warn("allSubnetsForVPC: corrupt subnet record", "key", key, "err", err) + continue + } + if subnet.VpcId != vpcID || subnet.SubnetId == "" { + continue + } + subnets = append(subnets, subnet.SubnetId) + } + return subnets, nil +} + +// publishSubnetEgressGateDecisionForRT recomputes gate/ungate per subnet in a +// route table (explicit associations + implicit-main subnets if the RT is +// main). destCidr is the route prefix whose presence/absence drove the call. +func (s *RouteTableServiceImpl) publishSubnetEgressGateDecisionForRT(accountID string, record *RouteTableRecord, destCidr string) { + if s.natsConn == nil || record == nil { + return + } + seen := map[string]bool{} + for _, assoc := range record.Associations { + if assoc.SubnetId == "" || assoc.Main || seen[assoc.SubnetId] { + continue + } + seen[assoc.SubnetId] = true + s.publishSubnetEgressGateDecision(accountID, record.VpcId, assoc.SubnetId, destCidr) + } + if !record.IsMain { + return + } + implicit, err := s.subnetsImplicitlyOnMainRT(accountID, record.VpcId) + if err != nil { + slog.Warn("subnet egress gate: enumerate implicit main-RT subnets failed", + "vpcId", record.VpcId, "err", err) + return + } + for _, subnetID := range implicit { + if seen[subnetID] { + continue + } + s.publishSubnetEgressGateDecision(accountID, record.VpcId, subnetID, destCidr) + } +} + // allRouteTablesForVPC returns all route tables belonging to a VPC func (s *RouteTableServiceImpl) allRouteTablesForVPC(accountID, vpcID string) ([]RouteTableRecord, error) { prefix := accountID + "." @@ -388,6 +671,10 @@ func (s *RouteTableServiceImpl) CreateRoute(input *ec2.CreateRouteInput, account Origin: "CreateRoute", } + // Publish vpc.add-igw-route events for each subnet associated with this + // route table so the network subscriber installs per-subnet egress policies. + s.publishIGWRouteEvents(accountID, "vpc.add-igw-route", record, igwRecord.VpcId, igwID, destCidr) + case input.NatGatewayId != nil && *input.NatGatewayId != "": natgwID := *input.NatGatewayId // Verify NAT GW exists and belongs to the same VPC @@ -414,7 +701,7 @@ func (s *RouteTableServiceImpl) CreateRoute(input *ec2.CreateRouteInput, account } // Publish vpc.add-nat-gateway events for each subnet associated with this route table - s.publishNatGatewayEvents(accountID, record, natgwRecord.VpcId, natgwID, natgwRecord.PublicIp) + s.publishNatGatewayEvents(accountID, record, natgwRecord.VpcId, natgwID, natgwRecord.PublicIp, destCidr) default: return nil, errors.New(awserrors.ErrorInvalidParameterValue) @@ -426,6 +713,12 @@ func (s *RouteTableServiceImpl) CreateRoute(input *ec2.CreateRouteInput, account return nil, err } + // Re-evaluate per-subnet gate decision now that the new route is in KV. + // publishSubnetEgressGateDecisionForRT skips when natsConn is nil. + if destCidr == "0.0.0.0/0" { + s.publishSubnetEgressGateDecisionForRT(accountID, record, destCidr) + } + slog.Info("CreateRoute completed", "routeTableId", rtbID, "destination", destCidr, "accountID", accountID) return &ec2.CreateRouteOutput{ @@ -461,8 +754,10 @@ func (s *RouteTableServiceImpl) DeleteRoute(input *ec2.DeleteRouteInput, account return nil, errors.New(awserrors.ErrorInvalidRouteNotFound) } + departing := record.Routes[idx] + // Cannot delete local route - if record.Routes[idx].GatewayId == "local" { + if departing.GatewayId == "local" { return nil, errors.New(awserrors.ErrorInvalidParameterValue) } @@ -472,6 +767,21 @@ func (s *RouteTableServiceImpl) DeleteRoute(input *ec2.DeleteRouteInput, account return nil, err } + if departing.GatewayId != "" && strings.HasPrefix(departing.GatewayId, "igw-") { + s.publishIGWRouteEvents(accountID, "vpc.delete-igw-route", record, record.VpcId, departing.GatewayId, departing.DestinationCidrBlock) + } + + if departing.NatGatewayId != "" { + s.publishNatGatewayDeleteEvents(accountID, record, departing.NatGatewayId, departing.DestinationCidrBlock) + } + + // Re-evaluate gate decision: if the deleted route was 0.0.0.0/0 and no + // other egress target remains in the effective RT, subnets that were + // allowed now need a drop policy installed. + if departing.DestinationCidrBlock == "0.0.0.0/0" { + s.publishSubnetEgressGateDecisionForRT(accountID, record, departing.DestinationCidrBlock) + } + slog.Info("DeleteRoute completed", "routeTableId", rtbID, "destination", destCidr, "accountID", accountID) return &ec2.DeleteRouteOutput{}, nil @@ -596,10 +906,27 @@ func (s *RouteTableServiceImpl) AssociateRouteTable(input *ec2.AssociateRouteTab return nil, err } + // Subnet leaves implicit main-RT membership: tear down per-subnet rules + // that came from the main RT's routes before the explicit RT's rules + // take effect. Skip if the explicit RT IS the main RT (can't happen via + // public API but DescribeRouteTables permits the query). + if !record.IsMain { + if mainRT, err := s.mainRouteTable(accountID, record.VpcId); err != nil { + slog.Warn("AssociateRouteTable: main RT lookup failed", "vpcId", record.VpcId, "err", err) + } else if mainRT != nil { + s.publishNatGatewayEventsForAssociation(accountID, "vpc.delete-nat-gateway", mainRT, subnetID) + s.publishIGWRouteEventsForAssociation(accountID, "vpc.delete-igw-route", mainRT, subnetID) + } + } + // Terraform commonly creates the route table + NAT GW route before associating // subnets. CreateRoute runs against a table with zero associations so no SNAT // events fire, so we must emit them here once the subnet joins. s.publishNatGatewayEventsForAssociation(accountID, "vpc.add-nat-gateway", record, subnetID) + s.publishIGWRouteEventsForAssociation(accountID, "vpc.add-igw-route", record, subnetID) + + // Subnet's effective RT just changed — recompute gate decision. + s.publishSubnetEgressGateDecision(accountID, record.VpcId, subnetID, "0.0.0.0/0") slog.Info("AssociateRouteTable completed", "routeTableId", rtbID, "subnetId", subnetID, "associationId", assocID, "accountID", accountID) @@ -663,6 +990,21 @@ func (s *RouteTableServiceImpl) DisassociateRouteTable(input *ec2.DisassociateRo // Tear down per-subnet SNAT rules for any NAT GW routes on this table. s.publishNatGatewayEventsForAssociation(accountID, "vpc.delete-nat-gateway", &record, departingSubnetID) + s.publishIGWRouteEventsForAssociation(accountID, "vpc.delete-igw-route", &record, departingSubnetID) + + // Subnet falls back to implicit main-RT membership: re-install + // per-subnet rules for the main RT's routes (no-op if departing + // table WAS the main, which can't happen since DisassociateMain + // is rejected above). + if mainRT, err := s.mainRouteTable(accountID, record.VpcId); err != nil { + slog.Warn("DisassociateRouteTable: main RT lookup failed", "vpcId", record.VpcId, "err", err) + } else if mainRT != nil && mainRT.RouteTableId != record.RouteTableId { + s.publishNatGatewayEventsForAssociation(accountID, "vpc.add-nat-gateway", mainRT, departingSubnetID) + s.publishIGWRouteEventsForAssociation(accountID, "vpc.add-igw-route", mainRT, departingSubnetID) + } + + // Subnet's effective RT just changed — recompute gate decision. + s.publishSubnetEgressGateDecision(accountID, record.VpcId, departingSubnetID, "0.0.0.0/0") slog.Info("DisassociateRouteTable completed", "associationId", assocID, "routeTableId", record.RouteTableId, "accountID", accountID) return &ec2.DisassociateRouteTableOutput{}, nil @@ -744,6 +1086,7 @@ func (s *RouteTableServiceImpl) ReplaceRouteTableAssociation(input *ec2.ReplaceR // subnet is leaving so its per-CIDR rules must be removed before // the new table's rules take effect. s.publishNatGatewayEventsForAssociation(accountID, "vpc.delete-nat-gateway", &oldRecord, assoc.SubnetId) + s.publishIGWRouteEventsForAssociation(accountID, "vpc.delete-igw-route", &oldRecord, assoc.SubnetId) // Add to new table with new ID newAssocID := utils.GenerateResourceID("rtbassoc") @@ -765,6 +1108,10 @@ func (s *RouteTableServiceImpl) ReplaceRouteTableAssociation(input *ec2.ReplaceR // Install SNAT for any NAT GW routes on the new table. s.publishNatGatewayEventsForAssociation(accountID, "vpc.add-nat-gateway", newRecord, assoc.SubnetId) + s.publishIGWRouteEventsForAssociation(accountID, "vpc.add-igw-route", newRecord, assoc.SubnetId) + + // Subnet's effective RT just changed — recompute gate decision. + s.publishSubnetEgressGateDecision(accountID, newRecord.VpcId, assoc.SubnetId, "0.0.0.0/0") slog.Info("ReplaceRouteTableAssociation completed", "oldAssociationId", assocID, "newAssociationId", newAssocID, @@ -901,15 +1248,76 @@ func rtbMatchesFilters(record *RouteTableRecord, filters map[string][]string) bo // associated with this route table, so vpcd creates the SNAT rules. Called by // CreateRoute when a NAT GW route is added to a table that may already have // subnet associations. -func (s *RouteTableServiceImpl) publishNatGatewayEvents(accountID string, record *RouteTableRecord, vpcID, natgwID, publicIp string) { +func (s *RouteTableServiceImpl) publishNatGatewayEvents(accountID string, record *RouteTableRecord, vpcID, natgwID, publicIp, destCidr string) { if s.natsConn == nil { return } + seen := map[string]bool{} for _, assoc := range record.Associations { - if assoc.SubnetId == "" || assoc.Main { + if assoc.SubnetId == "" || assoc.Main || seen[assoc.SubnetId] { continue } - s.publishNatGatewayEventForSubnet(accountID, "vpc.add-nat-gateway", assoc.SubnetId, vpcID, natgwID, publicIp) + seen[assoc.SubnetId] = true + s.publishNatGatewayEventForSubnet(accountID, "vpc.add-nat-gateway", assoc.SubnetId, vpcID, natgwID, publicIp, destCidr) + } + if !record.IsMain { + return + } + implicit, err := s.subnetsImplicitlyOnMainRT(accountID, vpcID) + if err != nil { + slog.Warn("NAT GW event: enumerate implicit main-RT subnets failed", "topic", "vpc.add-nat-gateway", "vpcId", vpcID, "err", err) + return + } + for _, subnetID := range implicit { + if seen[subnetID] { + continue + } + s.publishNatGatewayEventForSubnet(accountID, "vpc.add-nat-gateway", subnetID, vpcID, natgwID, publicIp, destCidr) + } +} + +// publishNatGatewayDeleteEvents fans vpc.delete-nat-gateway out to every +// associated subnet so each per-subnet SNAT rule + egress policy is removed +// when CreateRoute's NATGW route is deleted (mirror of publishNatGatewayEvents). +func (s *RouteTableServiceImpl) publishNatGatewayDeleteEvents(accountID string, record *RouteTableRecord, natgwID, destCidr string) { + if s.natsConn == nil { + return + } + natgwEntry, err := s.natgwKV.Get(utils.AccountKey(accountID, natgwID)) + if err != nil { + slog.Warn("NAT GW event: natgw lookup failed", "topic", "vpc.delete-nat-gateway", "natGatewayId", natgwID, "err", err) + return + } + var natgw struct { + NatGatewayId string `json:"nat_gateway_id"` + VpcId string `json:"vpc_id"` + PublicIp string `json:"public_ip"` + } + if err := json.Unmarshal(natgwEntry.Value(), &natgw); err != nil { + slog.Warn("NAT GW event: natgw unmarshal failed", "topic", "vpc.delete-nat-gateway", "natGatewayId", natgwID, "err", err) + return + } + seen := map[string]bool{} + for _, assoc := range record.Associations { + if assoc.SubnetId == "" || assoc.Main || seen[assoc.SubnetId] { + continue + } + seen[assoc.SubnetId] = true + s.publishNatGatewayEventForSubnet(accountID, "vpc.delete-nat-gateway", assoc.SubnetId, natgw.VpcId, natgw.NatGatewayId, natgw.PublicIp, destCidr) + } + if !record.IsMain { + return + } + implicit, err := s.subnetsImplicitlyOnMainRT(accountID, natgw.VpcId) + if err != nil { + slog.Warn("NAT GW event: enumerate implicit main-RT subnets failed", "topic", "vpc.delete-nat-gateway", "vpcId", natgw.VpcId, "err", err) + return + } + for _, subnetID := range implicit { + if seen[subnetID] { + continue + } + s.publishNatGatewayEventForSubnet(accountID, "vpc.delete-nat-gateway", subnetID, natgw.VpcId, natgw.NatGatewayId, natgw.PublicIp, destCidr) } } @@ -939,14 +1347,14 @@ func (s *RouteTableServiceImpl) publishNatGatewayEventsForAssociation(accountID, slog.Warn("NAT GW event: natgw unmarshal failed", "topic", topic, "natGatewayId", r.NatGatewayId, "err", err) continue } - s.publishNatGatewayEventForSubnet(accountID, topic, subnetID, natgw.VpcId, natgw.NatGatewayId, natgw.PublicIp) + s.publishNatGatewayEventForSubnet(accountID, topic, subnetID, natgw.VpcId, natgw.NatGatewayId, natgw.PublicIp, r.DestinationCidrBlock) } } // publishNatGatewayEventForSubnet publishes a single vpc.{add,delete}-nat-gateway // event for the given subnet. Side-effect only — logs and swallows errors so a // missing subnet record doesn't fail the caller's API response. -func (s *RouteTableServiceImpl) publishNatGatewayEventForSubnet(accountID, topic, subnetID, vpcID, natgwID, publicIp string) { +func (s *RouteTableServiceImpl) publishNatGatewayEventForSubnet(accountID, topic, subnetID, vpcID, natgwID, publicIp, destCidr string) { if s.natsConn == nil { return } @@ -961,15 +1369,19 @@ func (s *RouteTableServiceImpl) publishNatGatewayEventForSubnet(accountID, topic return } evt := struct { - VpcId string `json:"vpc_id"` - NatGatewayId string `json:"nat_gateway_id"` - PublicIp string `json:"public_ip"` - SubnetCidr string `json:"subnet_cidr"` + VpcId string `json:"vpc_id"` + NatGatewayId string `json:"nat_gateway_id"` + PublicIp string `json:"public_ip"` + SubnetCidr string `json:"subnet_cidr"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` }{ - VpcId: vpcID, - NatGatewayId: natgwID, - PublicIp: publicIp, - SubnetCidr: subnet.CidrBlock, + VpcId: vpcID, + NatGatewayId: natgwID, + PublicIp: publicIp, + SubnetCidr: subnet.CidrBlock, + SubnetId: subnetID, + DestinationCidr: destCidr, } data, err := json.Marshal(evt) if err != nil { @@ -980,7 +1392,86 @@ func (s *RouteTableServiceImpl) publishNatGatewayEventForSubnet(accountID, topic slog.Warn("NAT GW event: publish failed", "topic", topic, "subnetId", subnetID, "err", err) return } - slog.Info("NAT GW event published", "topic", topic, "subnetCidr", subnet.CidrBlock, "publicIp", publicIp) + slog.Info("NAT GW event published", "topic", topic, "subnetCidr", subnet.CidrBlock, "publicIp", publicIp, "subnetId", subnetID, "destinationCidr", destCidr) +} + +// publishIGWRouteEvents emits one vpc.{add,delete}-igw-route event per subnet +// associated with the route table. Called by CreateRoute/DeleteRoute when an +// IGW route is added or removed against a table that may already carry +// subnet associations. +func (s *RouteTableServiceImpl) publishIGWRouteEvents(accountID, topic string, record *RouteTableRecord, vpcID, igwID, destCidr string) { + if s.natsConn == nil { + return + } + seen := map[string]bool{} + for _, assoc := range record.Associations { + if assoc.SubnetId == "" || assoc.Main || seen[assoc.SubnetId] { + continue + } + seen[assoc.SubnetId] = true + s.publishIGWRouteEventForSubnet(topic, assoc.SubnetId, vpcID, igwID, destCidr) + } + if !record.IsMain { + return + } + implicit, err := s.subnetsImplicitlyOnMainRT(accountID, vpcID) + if err != nil { + slog.Warn("IGW route event: enumerate implicit main-RT subnets failed", "topic", topic, "vpcId", vpcID, "err", err) + return + } + for _, subnetID := range implicit { + if seen[subnetID] { + continue + } + s.publishIGWRouteEventForSubnet(topic, subnetID, vpcID, igwID, destCidr) + } +} + +// publishIGWRouteEventsForAssociation emits one IGW route event per IGW route +// on the table, scoped to a single subnet. Called when a subnet joins or leaves +// a route table that already has IGW routes so OVN policy state tracks +// association lifecycle (terraform creates the route first, then associates). +func (s *RouteTableServiceImpl) publishIGWRouteEventsForAssociation(accountID, topic string, record *RouteTableRecord, subnetID string) { + if s.natsConn == nil || subnetID == "" { + return + } + for _, r := range record.Routes { + if r.GatewayId == "" || !strings.HasPrefix(r.GatewayId, "igw-") { + continue + } + s.publishIGWRouteEventForSubnet(topic, subnetID, record.VpcId, r.GatewayId, r.DestinationCidrBlock) + } +} + +// publishIGWRouteEventForSubnet publishes a single vpc.{add,delete}-igw-route +// event. Side-effect only — logs and swallows errors so a publish failure +// doesn't fail the caller's API response. The subscriber (network/subscribers) +// resolves the gateway nexthop and installs the OVN Logical_Router_Policy. +func (s *RouteTableServiceImpl) publishIGWRouteEventForSubnet(topic, subnetID, vpcID, igwID, destCidr string) { + if s.natsConn == nil { + return + } + evt := struct { + VpcId string `json:"vpc_id"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` + InternetGatewayId string `json:"internet_gateway_id"` + }{ + VpcId: vpcID, + SubnetId: subnetID, + DestinationCidr: destCidr, + InternetGatewayId: igwID, + } + data, err := json.Marshal(evt) + if err != nil { + slog.Warn("IGW route event: marshal failed", "topic", topic, "err", err) + return + } + if err := s.natsConn.Publish(topic, data); err != nil { + slog.Warn("IGW route event: publish failed", "topic", topic, "subnetId", subnetID, "err", err) + return + } + slog.Info("IGW route event published", "topic", topic, "subnetId", subnetID, "destinationCidr", destCidr, "igwId", igwID) } // recordToEC2 converts an internal record to an AWS SDK RouteTable struct diff --git a/spinifex/handlers/ec2/routetable/service_impl_test.go b/spinifex/handlers/ec2/routetable/service_impl_test.go index f8ae2c3ff..792829666 100644 --- a/spinifex/handlers/ec2/routetable/service_impl_test.go +++ b/spinifex/handlers/ec2/routetable/service_impl_test.go @@ -477,13 +477,17 @@ func receiveNatGWEvent(t *testing.T, sub *nats.Subscription, wantCidr string) (n msg, err := sub.NextMsg(2 * time.Second) require.NoError(t, err, "expected NAT GW event on %s", sub.Subject) var evt struct { - VpcId string `json:"vpc_id"` - NatGatewayId string `json:"nat_gateway_id"` - PublicIp string `json:"public_ip"` - SubnetCidr string `json:"subnet_cidr"` + VpcId string `json:"vpc_id"` + NatGatewayId string `json:"nat_gateway_id"` + PublicIp string `json:"public_ip"` + SubnetCidr string `json:"subnet_cidr"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` } require.NoError(t, json.Unmarshal(msg.Data, &evt)) assert.Equal(t, wantCidr, evt.SubnetCidr) + assert.NotEmpty(t, evt.SubnetId, "subnet_id required so subscriber can install per-subnet egress policy") + assert.NotEmpty(t, evt.DestinationCidr, "destination_cidr required so subscriber can install per-subnet egress policy") return evt.NatGatewayId, evt.PublicIp } @@ -573,6 +577,45 @@ func TestDisassociateRouteTable_PublishesNatGatewayDeleteEvent(t *testing.T) { assert.Equal(t, "nat-test1", natgwID) } +// TestDeleteRoute_NATGW_PublishesDeleteForAssociatedSubnets covers DeleteRoute +// on a NATGW route: every subnet associated with the table receives a +// vpc.delete-nat-gateway event so the subscriber tears down both the SNAT rule +// and the per-subnet egress policy (mirror of the IGW DeleteRoute path). +func TestDeleteRoute_NATGW_PublishesDeleteForAssociatedSubnets(t *testing.T) { + svc := setupTestService(t) + addSub, err := svc.natsConn.SubscribeSync("vpc.add-nat-gateway") + require.NoError(t, err) + defer func() { _ = addSub.Unsubscribe() }() + delSub, err := svc.natsConn.SubscribeSync("vpc.delete-nat-gateway") + require.NoError(t, err) + defer func() { _ = delSub.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + NatGatewayId: aws.String("nat-test1"), + }, testAccountID) + require.NoError(t, err) + // Drain the add event from CreateRoute. + natgwID, _ := receiveNatGWEvent(t, addSub, "10.0.11.0/24") + require.Equal(t, "nat-test1", natgwID) + + _, err = svc.DeleteRoute(&ec2.DeleteRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + }, testAccountID) + require.NoError(t, err) + + delNatgwID, _ := receiveNatGWEvent(t, delSub, "10.0.11.0/24") + assert.Equal(t, "nat-test1", delNatgwID) +} + // TestReplaceRouteTableAssociation_PublishesNatGatewayEvents covers the move // path: subnet leaves a NAT-routed table for a NAT-free table. We expect a // delete event against the old table's NAT GW route and no add event. @@ -617,3 +660,693 @@ func TestReplaceRouteTableAssociation_PublishesNatGatewayEvents(t *testing.T) { _, err = addSub.NextMsg(100 * time.Millisecond) assert.Error(t, err, "Replace must not publish add when new table has no NAT GW routes") } + +func receiveIGWRouteEvent(t *testing.T, sub *nats.Subscription, wantCidr string) (subnetID, igwID, destCidr string) { + t.Helper() + msg, err := sub.NextMsg(time.Second) + require.NoError(t, err) + var evt struct { + VpcId string `json:"vpc_id"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` + InternetGatewayId string `json:"internet_gateway_id"` + } + require.NoError(t, json.Unmarshal(msg.Data, &evt)) + assert.Equal(t, wantCidr, evt.DestinationCidr) + return evt.SubnetId, evt.InternetGatewayId, evt.DestinationCidr +} + +// TestCreateRoute_IGW_PublishesAddIGWRouteForAssociatedSubnets covers the +// CreateRoute(IGW) flow: emits one vpc.add-igw-route event per associated +// subnet so the network subscriber installs per-subnet egress policies. +func TestCreateRoute_IGW_PublishesAddIGWRouteForAssociatedSubnets(t *testing.T) { + svc := setupTestService(t) + sub, err := svc.natsConn.SubscribeSync("vpc.add-igw-route") + require.NoError(t, err) + defer func() { _ = sub.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + + // Associate subnet FIRST, then CreateRoute — exercises the "route after + // association" leg. + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + subnetID, igwID, _ := receiveIGWRouteEvent(t, sub, "0.0.0.0/0") + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "igw-test1", igwID) +} + +// TestAssociateRouteTable_PublishesAddIGWRouteForExistingRoute covers the +// terraform ordering: CreateRoute(IGW) → AssociateRouteTable. CreateRoute runs +// against an empty-association table so no event fires; Associate must emit +// the event so the subscriber installs the policy for the joining subnet. +func TestAssociateRouteTable_PublishesAddIGWRouteForExistingRoute(t *testing.T) { + svc := setupTestService(t) + sub, err := svc.natsConn.SubscribeSync("vpc.add-igw-route") + require.NoError(t, err) + defer func() { _ = sub.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + // No subnet yet → no event from CreateRoute. + _, err = sub.NextMsg(100 * time.Millisecond) + assert.Error(t, err, "CreateRoute(IGW) must not publish when table has no associations") + + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + subnetID, igwID, _ := receiveIGWRouteEvent(t, sub, "0.0.0.0/0") + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "igw-test1", igwID) +} + +// TestDeleteRoute_IGW_PublishesDeleteIGWRoute verifies policy teardown when +// the IGW route is removed from a table with associations. +func TestDeleteRoute_IGW_PublishesDeleteIGWRoute(t *testing.T) { + svc := setupTestService(t) + delSub, err := svc.natsConn.SubscribeSync("vpc.delete-igw-route") + require.NoError(t, err) + defer func() { _ = delSub.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + _, err = svc.DeleteRoute(&ec2.DeleteRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + }, testAccountID) + require.NoError(t, err) + + subnetID, igwID, _ := receiveIGWRouteEvent(t, delSub, "0.0.0.0/0") + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "igw-test1", igwID) +} + +// TestDisassociateRouteTable_PublishesDeleteIGWRoute verifies the egress policy +// is torn down when a subnet leaves a table that carries an IGW route. +func TestDisassociateRouteTable_PublishesDeleteIGWRoute(t *testing.T) { + svc := setupTestService(t) + delSub, err := svc.natsConn.SubscribeSync("vpc.delete-igw-route") + require.NoError(t, err) + defer func() { _ = delSub.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + assocOut, err := svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + _, err = svc.DisassociateRouteTable(&ec2.DisassociateRouteTableInput{ + AssociationId: assocOut.AssociationId, + }, testAccountID) + require.NoError(t, err) + + subnetID, igwID, _ := receiveIGWRouteEvent(t, delSub, "0.0.0.0/0") + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "igw-test1", igwID) +} + +// drainIGWSubnets reads up to `count` events from sub and returns the unique +// SubnetIds. Used to assert fan-out shape regardless of NATS message order. +func drainIGWSubnets(t *testing.T, sub *nats.Subscription, count int) map[string]bool { + t.Helper() + got := map[string]bool{} + for range count { + msg, err := sub.NextMsg(500 * time.Millisecond) + if err != nil { + break + } + var evt struct { + SubnetId string `json:"subnet_id"` + } + require.NoError(t, json.Unmarshal(msg.Data, &evt)) + got[evt.SubnetId] = true + } + return got +} + +// TestCreateRoute_IGW_OnMainRT_FansOutToImplicitSubnets covers the primary +// publish-gap fixed in this change: CreateRoute(IGW) on a VPC's main RT must +// emit one vpc.add-igw-route event per subnet that has no explicit non-main +// RT association (AWS implicit-main semantics). +func TestCreateRoute_IGW_OnMainRT_FansOutToImplicitSubnets(t *testing.T) { + svc := setupTestService(t) + sub, err := svc.natsConn.SubscribeSync("vpc.add-igw-route") + require.NoError(t, err) + defer func() { _ = sub.Unsubscribe() }() + + mainRT, err := svc.CreateRouteTableForVPC("vpc-test1", "10.0.0.0/16", testAccountID, true, "") + require.NoError(t, err) + + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(mainRT.RouteTableId), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + got := drainIGWSubnets(t, sub, 4) + assert.True(t, got["subnet-test1"], "implicit main-RT subnet must receive add event") + assert.True(t, got["subnet-priv1"], "implicit main-RT subnet must receive add event") + assert.Len(t, got, 2) +} + +// TestCreateRoute_IGW_OnMainRT_SkipsExplicitlyAssociatedSubnets ensures a +// subnet with an explicit non-main RT association does NOT receive the main +// RT's IGW route event — its effective route table is the explicit one. +func TestCreateRoute_IGW_OnMainRT_SkipsExplicitlyAssociatedSubnets(t *testing.T) { + svc := setupTestService(t) + sub, err := svc.natsConn.SubscribeSync("vpc.add-igw-route") + require.NoError(t, err) + defer func() { _ = sub.Unsubscribe() }() + + mainRT, err := svc.CreateRouteTableForVPC("vpc-test1", "10.0.0.0/16", testAccountID, true, "") + require.NoError(t, err) + + customRtbID := createTestRtb(t, svc) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(customRtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(mainRT.RouteTableId), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + got := drainIGWSubnets(t, sub, 4) + assert.True(t, got["subnet-test1"], "implicit subnet must receive add event") + assert.False(t, got["subnet-priv1"], "explicitly-associated subnet must NOT receive main-RT event") +} + +// TestAssociateRouteTable_RemovesMainRTRoutesForJoiningSubnet covers the +// symmetric fix: when a subnet leaves implicit main-RT membership for an +// explicit non-main RT, the main RT's per-subnet rules must be torn down so +// state matches AWS effective-route semantics. +func TestAssociateRouteTable_RemovesMainRTRoutesForJoiningSubnet(t *testing.T) { + svc := setupTestService(t) + delSub, err := svc.natsConn.SubscribeSync("vpc.delete-igw-route") + require.NoError(t, err) + defer func() { _ = delSub.Unsubscribe() }() + + mainRT, err := svc.CreateRouteTableForVPC("vpc-test1", "10.0.0.0/16", testAccountID, true, "") + require.NoError(t, err) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(mainRT.RouteTableId), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + customRtbID := createTestRtb(t, svc) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(customRtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + got := drainIGWSubnets(t, delSub, 2) + assert.True(t, got["subnet-priv1"], "joining subnet must receive delete event for main-RT routes") + assert.Len(t, got, 1) +} + +// TestDisassociateRouteTable_RestoresMainRTRoutesForDepartingSubnet covers +// the symmetric add-back: when a subnet leaves an explicit RT and falls back +// to implicit main-RT membership, the main RT's per-subnet rules must be +// re-installed. +// receiveGateEvent reads one message from sub and returns the (subnet, +// destCidr) it carried. Used for both vpc.gate-subnet-egress and +// vpc.ungate-subnet-egress since the payload shape is identical. +func receiveGateEvent(t *testing.T, sub *nats.Subscription) (subnetID, destCidr string) { + t.Helper() + msg, err := sub.NextMsg(time.Second) + require.NoError(t, err) + var evt struct { + VpcId string `json:"vpc_id"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` + } + require.NoError(t, json.Unmarshal(msg.Data, &evt)) + return evt.SubnetId, evt.DestinationCidr +} + +func drainGateSubnets(t *testing.T, sub *nats.Subscription, limit int) map[string]string { + t.Helper() + got := map[string]string{} + for range limit { + msg, err := sub.NextMsg(200 * time.Millisecond) + if err != nil { + break + } + var evt struct { + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` + } + require.NoError(t, json.Unmarshal(msg.Data, &evt)) + got[evt.SubnetId] = evt.DestinationCidr + } + return got +} + +// TestCreateRoute_IGW_PublishesUngateForAssociatedSubnets: when a subnet is +// already associated to a route table and an IGW default route is added, +// the gate decision flips to "egress allowed" so the subscriber must remove +// any previously-installed drop policy. +func TestCreateRoute_IGW_PublishesUngateForAssociatedSubnets(t *testing.T) { + svc := setupTestService(t) + ungate, err := svc.natsConn.SubscribeSync("vpc.ungate-subnet-egress") + require.NoError(t, err) + defer func() { _ = ungate.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + // Drain Associate-time gate event (gate, no egress yet). + for { + if _, err := ungate.NextMsg(100 * time.Millisecond); err != nil { + break + } + } + + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + subnetID, dest := receiveGateEvent(t, ungate) + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "0.0.0.0/0", dest) +} + +// TestDeleteRoute_IGW_PublishesGateForAssociatedSubnets: removing the IGW +// default route from a table flips associated subnets back to "no egress" +// so the subscriber must reinstall the drop policy. +func TestDeleteRoute_IGW_PublishesGateForAssociatedSubnets(t *testing.T) { + svc := setupTestService(t) + gate, err := svc.natsConn.SubscribeSync("vpc.gate-subnet-egress") + require.NoError(t, err) + defer func() { _ = gate.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + for { + if _, err := gate.NextMsg(100 * time.Millisecond); err != nil { + break + } + } + + _, err = svc.DeleteRoute(&ec2.DeleteRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + }, testAccountID) + require.NoError(t, err) + + subnetID, dest := receiveGateEvent(t, gate) + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "0.0.0.0/0", dest) +} + +// TestAssociateRouteTable_NoDefaultRoute_PublishesGate: joining a table that +// has no 0.0.0.0/0 route must publish a gate event so the subnet is dropped. +func TestAssociateRouteTable_NoDefaultRoute_PublishesGate(t *testing.T) { + svc := setupTestService(t) + gate, err := svc.natsConn.SubscribeSync("vpc.gate-subnet-egress") + require.NoError(t, err) + defer func() { _ = gate.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + subnetID, dest := receiveGateEvent(t, gate) + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "0.0.0.0/0", dest) +} + +// TestAssociateRouteTable_HasIGWRoute_PublishesUngate: joining a table that +// already carries 0.0.0.0/0 -> IGW must publish ungate so any pre-existing +// drop policy is removed. +func TestAssociateRouteTable_HasIGWRoute_PublishesUngate(t *testing.T) { + svc := setupTestService(t) + ungate, err := svc.natsConn.SubscribeSync("vpc.ungate-subnet-egress") + require.NoError(t, err) + defer func() { _ = ungate.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + subnetID, dest := receiveGateEvent(t, ungate) + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "0.0.0.0/0", dest) +} + +// TestDisassociateRouteTable_MainHasNoEgress_PublishesGate: leaving an +// egress-carrying table for a main RT without 0.0.0.0/0 must gate. +func TestDisassociateRouteTable_MainHasNoEgress_PublishesGate(t *testing.T) { + svc := setupTestService(t) + gate, err := svc.natsConn.SubscribeSync("vpc.gate-subnet-egress") + require.NoError(t, err) + defer func() { _ = gate.Unsubscribe() }() + + // Main RT with no 0.0.0.0/0. + _, err = svc.CreateRouteTableForVPC("vpc-test1", "10.0.0.0/16", testAccountID, true, "") + require.NoError(t, err) + customRtbID := createTestRtb(t, svc) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(customRtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + assocOut, err := svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(customRtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + for { + if _, err := gate.NextMsg(100 * time.Millisecond); err != nil { + break + } + } + + _, err = svc.DisassociateRouteTable(&ec2.DisassociateRouteTableInput{ + AssociationId: assocOut.AssociationId, + }, testAccountID) + require.NoError(t, err) + + subnetID, dest := receiveGateEvent(t, gate) + assert.Equal(t, "subnet-priv1", subnetID) + assert.Equal(t, "0.0.0.0/0", dest) +} + +// TestCreateRoute_NonDefaultPrefix_NoGateEvent: only 0.0.0.0/0 routes drive +// gate decisions today (subnet-level scoping is what AWS RTs gate on). +func TestCreateRoute_NonDefaultPrefix_NoGateEvent(t *testing.T) { + svc := setupTestService(t) + gate, err := svc.natsConn.SubscribeSync("vpc.gate-subnet-egress") + require.NoError(t, err) + defer func() { _ = gate.Unsubscribe() }() + ungate, err := svc.natsConn.SubscribeSync("vpc.ungate-subnet-egress") + require.NoError(t, err) + defer func() { _ = ungate.Unsubscribe() }() + + rtbID := createTestRtb(t, svc) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + // Drain the Associate-time gate event. + for { + if _, err := gate.NextMsg(100 * time.Millisecond); err != nil { + break + } + } + + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("8.8.8.8/32"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + _, err = gate.NextMsg(200 * time.Millisecond) + assert.Error(t, err, "non-default prefix must not emit a gate event") + _, err = ungate.NextMsg(200 * time.Millisecond) + assert.Error(t, err, "non-default prefix must not emit an ungate event") +} + +// TestCreateRoute_IGW_OnMainRT_FansOutGateToImplicitSubnets: when the main +// RT acquires an IGW default route, implicit-main subnets receive ungate. +func TestCreateRoute_IGW_OnMainRT_FansOutUngateToImplicitSubnets(t *testing.T) { + svc := setupTestService(t) + ungate, err := svc.natsConn.SubscribeSync("vpc.ungate-subnet-egress") + require.NoError(t, err) + defer func() { _ = ungate.Unsubscribe() }() + + mainRT, err := svc.CreateRouteTableForVPC("vpc-test1", "10.0.0.0/16", testAccountID, true, "") + require.NoError(t, err) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(mainRT.RouteTableId), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + got := drainGateSubnets(t, ungate, 3) + assert.Equal(t, "0.0.0.0/0", got["subnet-test1"]) + assert.Equal(t, "0.0.0.0/0", got["subnet-priv1"]) +} + +func TestDisassociateRouteTable_RestoresMainRTRoutesForDepartingSubnet(t *testing.T) { + svc := setupTestService(t) + addSub, err := svc.natsConn.SubscribeSync("vpc.add-igw-route") + require.NoError(t, err) + defer func() { _ = addSub.Unsubscribe() }() + + mainRT, err := svc.CreateRouteTableForVPC("vpc-test1", "10.0.0.0/16", testAccountID, true, "") + require.NoError(t, err) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(mainRT.RouteTableId), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + + customRtbID := createTestRtb(t, svc) + assocOut, err := svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(customRtbID), + SubnetId: aws.String("subnet-priv1"), + }, testAccountID) + require.NoError(t, err) + + // Drain everything published during setup so the Disassociate assertions + // only see post-disassociate events. + for { + if _, err := addSub.NextMsg(100 * time.Millisecond); err != nil { + break + } + } + + _, err = svc.DisassociateRouteTable(&ec2.DisassociateRouteTableInput{ + AssociationId: assocOut.AssociationId, + }, testAccountID) + require.NoError(t, err) + + got := drainIGWSubnets(t, addSub, 2) + assert.True(t, got["subnet-priv1"], "departing subnet must receive add event for main-RT routes") + assert.Len(t, got, 1) +} + +// TestPublishGateDecisionsForVPC_GatesAllSubnetsWhenNoEgress: with a main RT +// carrying only the local route, every subnet in the VPC must receive a gate +// event. Exercises the IGW attach/detach fan-out hook. +func TestPublishGateDecisionsForVPC_GatesAllSubnetsWhenNoEgress(t *testing.T) { + svc := setupTestService(t) + _, err := svc.CreateRouteTableForVPC("vpc-test1", "10.0.0.0/16", testAccountID, true, "") + require.NoError(t, err) + + gate, err := svc.natsConn.SubscribeSync("vpc.gate-subnet-egress") + require.NoError(t, err) + defer func() { _ = gate.Unsubscribe() }() + + svc.PublishGateDecisionsForVPC(testAccountID, "vpc-test1", "0.0.0.0/0") + + got := drainGateSubnets(t, gate, 4) + assert.Equal(t, "0.0.0.0/0", got["subnet-test1"]) + assert.Equal(t, "0.0.0.0/0", got["subnet-priv1"]) + assert.Len(t, got, 2) +} + +// TestPublishGateDecisionsForVPC_UngatesSubnetWithIGWRoute: a subnet whose +// effective RT carries 0.0.0.0/0 -> IGW must receive ungate; the other +// (implicit-main, no egress) subnet still receives gate. +func TestPublishGateDecisionsForVPC_UngatesSubnetWithIGWRoute(t *testing.T) { + svc := setupTestService(t) + _, err := svc.CreateRouteTableForVPC("vpc-test1", "10.0.0.0/16", testAccountID, true, "") + require.NoError(t, err) + + // Custom RT with 0.0.0.0/0 -> IGW, associated to subnet-test1. + rtbID := createTestRtb(t, svc) + _, err = svc.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + require.NoError(t, err) + _, err = svc.AssociateRouteTable(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String(rtbID), + SubnetId: aws.String("subnet-test1"), + }, testAccountID) + require.NoError(t, err) + + gate, err := svc.natsConn.SubscribeSync("vpc.gate-subnet-egress") + require.NoError(t, err) + defer func() { _ = gate.Unsubscribe() }() + ungate, err := svc.natsConn.SubscribeSync("vpc.ungate-subnet-egress") + require.NoError(t, err) + defer func() { _ = ungate.Unsubscribe() }() + + svc.PublishGateDecisionsForVPC(testAccountID, "vpc-test1", "0.0.0.0/0") + + gateGot := drainGateSubnets(t, gate, 4) + ungateGot := drainGateSubnets(t, ungate, 4) + assert.Equal(t, "0.0.0.0/0", ungateGot["subnet-test1"], "subnet-test1 has IGW route → ungate") + assert.Equal(t, "0.0.0.0/0", gateGot["subnet-priv1"], "subnet-priv1 implicit-main no egress → gate") + assert.Len(t, ungateGot, 1) + assert.Len(t, gateGot, 1) +} + +// TestEffectiveRouteTable_DeterministicWithDuplicateMain seeds two +// IsMain=true RTs for the same VPC (the bootstrap-race symptom) and asserts +// effectiveRouteTable picks the one with more routes — the "real" RT carries +// any user-added 0.0.0.0/0 entry, while the orphan only has the implicit +// local route. Non-deterministic selection produced the run 26699045536 +// guest-SSH timeout: CreateRoute's post-write gate recompute happened to +// resolve to the orphan and published vpc.gate-subnet-egress. +func TestEffectiveRouteTable_DeterministicWithDuplicateMain(t *testing.T) { + svc := setupTestService(t) + now := time.Now() + + orphan := RouteTableRecord{ + RouteTableId: "rtb-zzzorphan", + VpcId: "vpc-test1", + AccountID: testAccountID, + IsMain: true, + Routes: []RouteRecord{{DestinationCidrBlock: "10.0.0.0/16", GatewayId: "local", State: "active", Origin: "CreateRouteTable"}}, + CreatedAt: now.Add(time.Second), // later, would lose tiebreak even with same route count + } + real_ := RouteTableRecord{ + RouteTableId: "rtb-aaareal", + VpcId: "vpc-test1", + AccountID: testAccountID, + IsMain: true, + Routes: []RouteRecord{ + {DestinationCidrBlock: "10.0.0.0/16", GatewayId: "local", State: "active", Origin: "CreateRouteTable"}, + {DestinationCidrBlock: "0.0.0.0/0", GatewayId: "igw-test1", State: "active", Origin: "CreateRoute"}, + }, + CreatedAt: now, + } + require.NoError(t, svc.putRouteTable(testAccountID, &orphan)) + require.NoError(t, svc.putRouteTable(testAccountID, &real_)) + + rt, err := svc.effectiveRouteTable(testAccountID, "vpc-test1", "subnet-priv1") + require.NoError(t, err) + require.NotNil(t, rt) + assert.Equal(t, "rtb-aaareal", rt.RouteTableId, "must prefer main RT with more routes") + + main, err := svc.mainRouteTable(testAccountID, "vpc-test1") + require.NoError(t, err) + require.NotNil(t, main) + assert.Equal(t, "rtb-aaareal", main.RouteTableId, "mainRouteTable must use same tiebreak") +} + +// TestPreferMain_RouteCountWinsOverCreatedAt confirms route-count is the +// primary tiebreaker — a later-created RT with more routes beats an older +// orphan with only the implicit local route. +func TestPreferMain_RouteCountWinsOverCreatedAt(t *testing.T) { + older := &RouteTableRecord{RouteTableId: "rtb-a", CreatedAt: time.Unix(100, 0), Routes: []RouteRecord{{}}} + newer := &RouteTableRecord{RouteTableId: "rtb-b", CreatedAt: time.Unix(200, 0), Routes: []RouteRecord{{}, {}}} + assert.True(t, preferMain(older, newer), "more routes wins regardless of CreatedAt") + assert.False(t, preferMain(newer, older), "asymmetric — older with fewer routes loses") +} + +// TestPreferMain_CreatedAtTiebreak: equal route counts → oldest CreatedAt +// wins (original record beats the late dup). +func TestPreferMain_CreatedAtTiebreak(t *testing.T) { + older := &RouteTableRecord{RouteTableId: "rtb-b", CreatedAt: time.Unix(100, 0), Routes: []RouteRecord{{}}} + newer := &RouteTableRecord{RouteTableId: "rtb-a", CreatedAt: time.Unix(200, 0), Routes: []RouteRecord{{}}} + assert.True(t, preferMain(newer, older), "older CreatedAt wins on equal route count") + assert.False(t, preferMain(older, newer), "asymmetric — newer loses") +} + +// TestPublishGateDecisionsForVPC_EmptyVPCNoOp: VPC with zero subnets must +// silently emit no events. +func TestPublishGateDecisionsForVPC_EmptyVPCNoOp(t *testing.T) { + svc := setupTestService(t) + gate, err := svc.natsConn.SubscribeSync("vpc.gate-subnet-egress") + require.NoError(t, err) + defer func() { _ = gate.Unsubscribe() }() + ungate, err := svc.natsConn.SubscribeSync("vpc.ungate-subnet-egress") + require.NoError(t, err) + defer func() { _ = ungate.Unsubscribe() }() + + svc.PublishGateDecisionsForVPC(testAccountID, "vpc-empty", "0.0.0.0/0") + + _, err = gate.NextMsg(150 * time.Millisecond) + assert.Error(t, err, "expected no gate event") + _, err = ungate.NextMsg(150 * time.Millisecond) + assert.Error(t, err, "expected no ungate event") +} diff --git a/spinifex/handlers/ec2/vpc/service_impl.go b/spinifex/handlers/ec2/vpc/service_impl.go index b09d683e9..6d6b5a9cd 100644 --- a/spinifex/handlers/ec2/vpc/service_impl.go +++ b/spinifex/handlers/ec2/vpc/service_impl.go @@ -1126,7 +1126,22 @@ func (s *VPCServiceImpl) EnsureDefaultVPC(accountID string, bootstrap ...Bootstr // createMainRouteTable writes a main route table record directly to the route table KV bucket. // This avoids a circular import (routetable package imports vpc package for validation). +// +// Idempotency: when concurrent daemons race EnsureDefaultVPC for the same +// bootstrap-pinned VPC, both would otherwise mint a fresh random rtb-ID +// and Put it, leaving the KV with two IsMain=true records for one VPC. +// effectiveRouteTable then resolves the subnet's RT non-deterministically, +// and CreateRoute's post-write gate recompute can publish +// vpc.gate-subnet-egress against the orphan (route-less) main RT, installing +// a Priority-1100 DROP that kills egress on the default subnet. func (s *VPCServiceImpl) createMainRouteTable(accountID, vpcID, vpcCidr string) error { + if existing, err := s.findMainRouteTableID(accountID, vpcID); err != nil { + return fmt.Errorf("check existing main route table: %w", err) + } else if existing != "" { + slog.Info("Main route table already exists for VPC, skipping creation", + "routeTableId", existing, "vpcId", vpcID, "accountID", accountID) + return nil + } type rtbRecord struct { RouteTableId string `json:"route_table_id"` VpcId string `json:"vpc_id"` @@ -1182,6 +1197,44 @@ func (s *VPCServiceImpl) createMainRouteTable(accountID, vpcID, vpcCidr string) return nil } +// findMainRouteTableID returns the rtb-ID of the main route table for vpcID +// in accountID, or "" if none exists. Partial unmarshal — only the fields +// needed to identify a main RT, so this stays cheap on hot KV scans. +func (s *VPCServiceImpl) findMainRouteTableID(accountID, vpcID string) (string, error) { + prefix := accountID + "." + keys, err := s.rtbKV.Keys() + if err != nil { + if errors.Is(err, nats.ErrNoKeysFound) { + return "", nil + } + return "", fmt.Errorf("list rtb keys: %w", err) + } + for _, key := range keys { + if key == utils.VersionKey || !strings.HasPrefix(key, prefix) { + continue + } + entry, err := s.rtbKV.Get(key) + if err != nil { + if errors.Is(err, nats.ErrKeyNotFound) { + continue + } + return "", fmt.Errorf("read rtb %s: %w", key, err) + } + var rt struct { + RouteTableId string `json:"route_table_id"` + VpcId string `json:"vpc_id"` + IsMain bool `json:"is_main"` + } + if err := json.Unmarshal(entry.Value(), &rt); err != nil { + continue + } + if rt.VpcId == vpcID && rt.IsMain { + return rt.RouteTableId, nil + } + } + return "", nil +} + // GetDefaultSubnet returns the default subnet for RunInstances when no SubnetId is specified. func (s *VPCServiceImpl) GetDefaultSubnet(accountID string) (*SubnetRecord, error) { prefix := accountID + "." diff --git a/spinifex/handlers/ec2/vpc/service_impl_test.go b/spinifex/handlers/ec2/vpc/service_impl_test.go index b40e008c5..a556887ab 100644 --- a/spinifex/handlers/ec2/vpc/service_impl_test.go +++ b/spinifex/handlers/ec2/vpc/service_impl_test.go @@ -596,6 +596,58 @@ func TestEnsureDefaultVPC_SkipsWhenDefaultExists(t *testing.T) { assert.Equal(t, 1, defaultCount) } +// TestCreateMainRouteTable_Idempotent asserts a second createMainRouteTable +// call for the same VPC is a no-op. Without this guard, concurrent multi-node +// EnsureDefaultVPC calls (admin account, bootstrap-pinned VpcId) each Put a +// random rtb-ID with IsMain=true, and the resulting duplicate causes +// CreateRoute's post-write gate recompute to resolve effective RT to the +// orphan (route-less) record and publish vpc.gate-subnet-egress. +func TestCreateMainRouteTable_Idempotent(t *testing.T) { + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.99.0.0/16") // CreateVpc auto-calls createMainRouteTable + + firstID, err := svc.findMainRouteTableID(testAccountID, vpcID) + require.NoError(t, err) + require.NotEmpty(t, firstID, "CreateVpc should have created a main route table") + + require.NoError(t, svc.createMainRouteTable(testAccountID, vpcID, "10.99.0.0/16")) + + secondID, err := svc.findMainRouteTableID(testAccountID, vpcID) + require.NoError(t, err) + assert.Equal(t, firstID, secondID, "second call must be a no-op") + + mains := countMainRouteTablesForVPC(t, svc, vpcID) + assert.Equal(t, 1, mains, "exactly one IsMain=true record after duplicate call") +} + +// countMainRouteTablesForVPC scans rtbKV directly to surface duplicate-main +// state in tests. Returns the number of IsMain=true records for vpcID. +func countMainRouteTablesForVPC(t *testing.T, svc *VPCServiceImpl, vpcID string) int { + t.Helper() + keys, err := svc.rtbKV.Keys() + if err != nil { + return 0 + } + n := 0 + for _, key := range keys { + entry, err := svc.rtbKV.Get(key) + if err != nil { + continue + } + var rt struct { + VpcId string `json:"vpc_id"` + IsMain bool `json:"is_main"` + } + if err := json.Unmarshal(entry.Value(), &rt); err != nil { + continue + } + if rt.VpcId == vpcID && rt.IsMain { + n++ + } + } + return n +} + // TestEnsureDefaultVPC_NoVpcdResponder simulates the daemon-startup race where // EnsureDefaultVPC runs before vpcd has subscribed to vpc.create-sg. Without // the fix, the synchronous SG round-trip errors out and EnsureDefaultVPC diff --git a/spinifex/network/external/igw.go b/spinifex/network/external/igw.go index e75157627..7c9375ae9 100644 --- a/spinifex/network/external/igw.go +++ b/spinifex/network/external/igw.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "net/netip" "github.com/mulgadc/spinifex/spinifex/network/ovn" "github.com/mulgadc/spinifex/spinifex/network/ovn/nbdb" @@ -21,6 +22,31 @@ type FlowsBarrier func() error type IGWManager interface { AttachIGW(ctx context.Context, spec IGWSpec) error DetachIGW(ctx context.Context, vpcID string) error + // EnsureSubnetEgress installs an OVN Logical_Router_Policy on the VPC + // router so traffic sourced from subnetID and destined for prefix + // reroutes via the IGW's gateway port. Called from the route-table + // subscriber when CreateRoute / AssociateRouteTable wires a subnet to a + // route table carrying a 0.0.0.0/0 -> igw-X route. Idempotent. + EnsureSubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error + // RemoveSubnetEgress is the inverse. Idempotent. + RemoveSubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error + // EnsureNATGatewaySubnetEgress installs the per-subnet egress policy that + // gives private-subnet packets a default route to leave the LR after + // NATGW SNAT. Reuses the IGW's gateway port + wan nexthop (NATGW SNAT + // happens on the same VPC router); priority SubnetEgressPriorityNATGW + // (lower than IGW so an IGW route on the same subnet would win). + // Requires AttachIGW to have run first (NATGW depends on IGW per AWS). + EnsureNATGatewaySubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error + // RemoveNATGatewaySubnetEgress is the inverse. Idempotent. + RemoveNATGatewaySubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error + // EnsureSubnetEgressDrop installs a DROP policy at + // SubnetEgressPriorityDrop for subnets whose effective route table lacks + // a 0.0.0.0/0 entry. The drop fires after lr_in_ip_routing has matched + // the VPC LR's router-wide default static route, killing the packet + // before egress. Idempotent. + EnsureSubnetEgressDrop(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error + // RemoveSubnetEgressDrop is the inverse. Idempotent. + RemoveSubnetEgressDrop(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error } // IGWManagerConfig is the construction-time bag for igwManager. @@ -249,6 +275,128 @@ func (m *igwManager) DetachIGW(ctx context.Context, vpcID string) error { return nil } +// EnsureSubnetEgress installs a per-subnet egress policy on the VPC router +// rerouting (inport == "rtr-" && ip4.dst == prefix) via the IGW +// nexthop out the gateway LRP. Idempotent (drift-replace via the underlying +// policy.RouteManager). +func (m *igwManager) EnsureSubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error { + return m.ensureSubnetEgressAtPriority(ctx, vpcID, subnetID, prefix, policy.SubnetEgressPriorityIGW, "EnsureSubnetEgress") +} + +// RemoveSubnetEgress deletes the policy installed by EnsureSubnetEgress. +// Idempotent. +func (m *igwManager) RemoveSubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error { + if vpcID == "" || subnetID == "" { + return errors.New("RemoveSubnetEgress: vpcID and subnetID required") + } + return m.routes.DeleteSubnetEgress(ctx, vpcID, subnetID, prefix, m.vpcExcludeCIDRs(ctx, vpcID)) +} + +// EnsureNATGatewaySubnetEgress is the NATGW priority sibling of +// EnsureSubnetEgress. Same nexthop and output port; lower priority so an +// IGW route on the same subnet (if ever both present) wins. +func (m *igwManager) EnsureNATGatewaySubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error { + return m.ensureSubnetEgressAtPriority(ctx, vpcID, subnetID, prefix, policy.SubnetEgressPriorityNATGW, "EnsureNATGatewaySubnetEgress") +} + +// RemoveNATGatewaySubnetEgress deletes the per-subnet egress policy at any +// priority for (subnetID, prefix). policy.RouteManager.DeleteSubnetEgress +// already deletes both IGW and NATGW priorities defensively. +func (m *igwManager) RemoveNATGatewaySubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error { + if vpcID == "" || subnetID == "" { + return errors.New("RemoveNATGatewaySubnetEgress: vpcID and subnetID required") + } + return m.routes.DeleteSubnetEgress(ctx, vpcID, subnetID, prefix, m.vpcExcludeCIDRs(ctx, vpcID)) +} + +// EnsureSubnetEgressDrop installs a drop policy at SubnetEgressPriorityDrop +// for a subnet whose effective route table lacks a 0.0.0.0/0 entry. The drop +// fires in lr_in_policy AFTER the VPC LR's router-wide default static route +// has already matched in lr_in_ip_routing, killing the packet before egress. +// Excludes the VPC CIDR (intra-VPC traffic untouched), 169.254.0.0/16 (DHCP +// / link-local / metadata), and 224.0.0.0/4 (multicast). +func (m *igwManager) EnsureSubnetEgressDrop(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error { + if vpcID == "" || subnetID == "" { + return errors.New("EnsureSubnetEgressDrop: vpcID and subnetID required") + } + return m.routes.AddSubnetEgressDrop(ctx, vpcID, policy.SubnetEgressDropSpec{ + SubnetID: subnetID, + Prefix: prefix, + ExcludeCIDRs: m.dropExcludeCIDRs(ctx, vpcID), + }) +} + +// RemoveSubnetEgressDrop deletes the policy installed by +// EnsureSubnetEgressDrop. Idempotent. +func (m *igwManager) RemoveSubnetEgressDrop(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix) error { + if vpcID == "" || subnetID == "" { + return errors.New("RemoveSubnetEgressDrop: vpcID and subnetID required") + } + return m.routes.DeleteSubnetEgressDrop(ctx, vpcID, subnetID, prefix, m.dropExcludeCIDRs(ctx, vpcID)) +} + +func (m *igwManager) ensureSubnetEgressAtPriority(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix, priority int, opName string) error { + if vpcID == "" || subnetID == "" { + return fmt.Errorf("%s: vpcID and subnetID required", opName) + } + _, nexthop, _, err := m.resolveGatewayNetwork(ctx, vpcID) + if err != nil { + return fmt.Errorf("resolve gateway network for %s: %w", vpcID, err) + } + if nexthop == "" { + return fmt.Errorf("%s: no gateway nexthop available for %s (IGW not attached?)", opName, vpcID) + } + return m.routes.AddSubnetEgress(ctx, vpcID, policy.SubnetEgressSpec{ + SubnetID: subnetID, + Prefix: prefix, + Nexthop: nexthop, + OutputPort: topology.GatewayRouterPort(vpcID), + Priority: priority, + ExcludeCIDRs: m.vpcExcludeCIDRs(ctx, vpcID), + }) +} + +// linkLocalCIDR / multicastCIDR are appended to drop-policy excludes so +// link-local (DHCP, metadata, 169.254.169.254) and multicast destinations +// are not killed by the per-subnet gate. +var ( + linkLocalCIDR = netip.MustParsePrefix("169.254.0.0/16") + multicastCIDR = netip.MustParsePrefix("224.0.0.0/4") +) + +// dropExcludeCIDRs returns the exclusion list for a per-subnet drop policy: +// VPC CIDR (if discoverable) plus link-local and multicast. Reroute policies +// only need the VPC CIDR exclusion because their match scope is narrower; the +// drop policy is a default-deny within the subnet's egress so it must spare +// any class the platform legitimately uses outside the VPC. +func (m *igwManager) dropExcludeCIDRs(ctx context.Context, vpcID string) []netip.Prefix { + excludes := []netip.Prefix{linkLocalCIDR, multicastCIDR} + if vpc := m.vpcExcludeCIDRs(ctx, vpcID); len(vpc) > 0 { + excludes = append(vpc, excludes...) + } + return excludes +} + +// vpcExcludeCIDRs fetches the VPC's primary CIDR from the LR ExternalIDs +// label so per-subnet egress reroute policies skip in-VPC destinations. +// Returns nil on lookup failure or unset label — caller installs a policy +// without exclusions, preserving prior behaviour. +func (m *igwManager) vpcExcludeCIDRs(ctx context.Context, vpcID string) []netip.Prefix { + router, err := m.ovn.GetLogicalRouter(ctx, topology.VPCRouter(vpcID)) + if err != nil || router == nil { + return nil + } + cidr := router.ExternalIDs["spinifex:cidr"] + if cidr == "" { + return nil + } + prefix, err := netip.ParsePrefix(cidr) + if err != nil { + return nil + } + return []netip.Prefix{prefix} +} + // resolveGatewayNetwork picks LRP Networks/nexthop/IP. Distributed: link-local. // Centralised: WAN-subnet IP (LRP is on-wire). func (m *igwManager) resolveGatewayNetwork(ctx context.Context, vpcID string) (network, nexthop, gwIP string, err error) { diff --git a/spinifex/network/external/igw_test.go b/spinifex/network/external/igw_test.go index 5873feaaf..1a6802ded 100644 --- a/spinifex/network/external/igw_test.go +++ b/spinifex/network/external/igw_test.go @@ -3,6 +3,7 @@ package external import ( "context" "errors" + "net/netip" "testing" "github.com/stretchr/testify/assert" @@ -85,13 +86,27 @@ func TestAttachIGW_Distributed_LinkLocalLRP(t *testing.T) { _, hasGwIP := lrp.ExternalIDs[gatewayIPExtIDKey] assert.False(t, hasGwIP, "link-local LRP must not record a gateway IP") - // Default route points at pool.Gateway, OutputPort pinned. + // OVN pipeline runs lr_in_ip_routing (table 15) before lr_in_policy + // (table 17) — without a router-wide default static route every external + // destination drops in routing before EnsureSubnetEgress's reroute policy + // gets a chance. AttachIGW installs `0.0.0.0/0 -> pool.Gateway` on the + // VPC router so policy lookup can refine per-subnet egress on top. route, err := m.FindStaticRoute(ctx, topology.VPCRouter("vpc-1"), "0.0.0.0/0") require.NoError(t, err) - require.NotNil(t, route) - assert.Equal(t, pool.Gateway, route.Nexthop) + require.NotNil(t, route, "AttachIGW must install the default static route so routing forwards to gw-port") require.NotNil(t, route.OutputPort) assert.Equal(t, topology.GatewayRouterPort("vpc-1"), *route.OutputPort) + assert.Equal(t, pool.Gateway, route.Nexthop) + + // EnsureSubnetEgress installs per-subnet policy with the expected nexthop. + require.NoError(t, mgr.EnsureSubnetEgress(ctx, "vpc-1", "subnet-pub", netip.MustParsePrefix("0.0.0.0/0"))) + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + require.NotNil(t, policies[0].Nexthop) + assert.Equal(t, pool.Gateway, *policies[0].Nexthop) + assert.Equal(t, topology.GatewayRouterPort("vpc-1"), policies[0].ExternalIDs["spinifex:output_port"]) + assert.Contains(t, policies[0].Match, topology.SubnetRouterPort("subnet-pub")) // Gateway chassis set once per chassis. assert.Equal(t, 2, m.SetGatewayChassisCalls) @@ -153,8 +168,17 @@ func TestAttachIGW_NoPoolNoChassis_UsesLinkLocalFallbacks(t *testing.T) { route, err := m.FindStaticRoute(ctx, topology.VPCRouter("vpc-1"), "0.0.0.0/0") require.NoError(t, err) - require.NotNil(t, route) + require.NotNil(t, route, "AttachIGW must install the default static route so routing forwards to gw-port") + require.NotNil(t, route.OutputPort) + assert.Equal(t, topology.GatewayRouterPort("vpc-1"), *route.OutputPort) assert.Equal(t, linkLocalGatewayNexthop, route.Nexthop) + + require.NoError(t, mgr.EnsureSubnetEgress(ctx, "vpc-1", "subnet-pub", netip.MustParsePrefix("0.0.0.0/0"))) + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + require.NotNil(t, policies[0].Nexthop) + assert.Equal(t, linkLocalGatewayNexthop, *policies[0].Nexthop) assert.Equal(t, 0, m.SetGatewayChassisCalls) } @@ -227,10 +251,12 @@ func TestAttachIGW_Centralized_AllocatorNexthopOverridesPool(t *testing.T) { require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) - route, err := m.FindStaticRoute(ctx, topology.VPCRouter("vpc-1"), "0.0.0.0/0") + require.NoError(t, mgr.EnsureSubnetEgress(ctx, "vpc-1", "subnet-pub", netip.MustParsePrefix("0.0.0.0/0"))) + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) require.NoError(t, err) - require.NotNil(t, route) - assert.Equal(t, "192.168.1.254", route.Nexthop, "allocator-supplied nexthop must override pool/link-local fallback") + require.Len(t, policies, 1) + require.NotNil(t, policies[0].Nexthop) + assert.Equal(t, "192.168.1.254", *policies[0].Nexthop, "allocator-supplied nexthop must override pool/link-local fallback") } func TestAttachIGW_AllocatorFailureUnwindsExtSwitch(t *testing.T) { @@ -245,3 +271,128 @@ func TestAttachIGW_AllocatorFailureUnwindsExtSwitch(t *testing.T) { _, err := m.GetLogicalSwitch(ctx, topology.ExternalSwitch("vpc-1")) assert.Error(t, err, "external switch must be unwound on allocator failure") } + +// TestEnsureNATGatewaySubnetEgress installs an LR policy at the NATGW +// priority (lower than IGW) reusing the IGW's gateway port + nexthop. +// Without this policy, NATGW SNAT rewrites the source IP but the packet +// has no route to leave the LR — private subnets cannot reach the internet. +func TestEnsureNATGatewaySubnetEgress(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + pool := &ExternalPoolConfig{Name: "p", Gateway: "192.168.1.1", PrefixLen: 24} + mgr, _ := newTestIGWManager(t, m, policy.NATModeDistributed, pool, LinkLocalAllocator{}, []string{"chassis-a"}) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + require.NoError(t, mgr.EnsureNATGatewaySubnetEgress(ctx, "vpc-1", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Equal(t, policy.SubnetEgressPriorityNATGW, policies[0].Priority) + require.NotNil(t, policies[0].Nexthop) + assert.Equal(t, pool.Gateway, *policies[0].Nexthop) + assert.Equal(t, topology.GatewayRouterPort("vpc-1"), policies[0].ExternalIDs["spinifex:output_port"]) + assert.Contains(t, policies[0].Match, topology.SubnetRouterPort("subnet-priv")) +} + +// TestEnsureNATGatewaySubnetEgress_IsIdempotent re-runs the install and +// asserts the row count stays at 1 (no drift, no duplicate). +func TestEnsureNATGatewaySubnetEgress_IsIdempotent(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + pool := &ExternalPoolConfig{Name: "p", Gateway: "192.168.1.1", PrefixLen: 24} + mgr, _ := newTestIGWManager(t, m, policy.NATModeDistributed, pool, LinkLocalAllocator{}, nil) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + require.NoError(t, mgr.EnsureNATGatewaySubnetEgress(ctx, "vpc-1", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) + require.NoError(t, mgr.EnsureNATGatewaySubnetEgress(ctx, "vpc-1", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Len(t, policies, 1) +} + +// TestRemoveNATGatewaySubnetEgress removes the policy installed by +// EnsureNATGatewaySubnetEgress and is idempotent when absent. +func TestRemoveNATGatewaySubnetEgress(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + pool := &ExternalPoolConfig{Name: "p", Gateway: "192.168.1.1", PrefixLen: 24} + mgr, _ := newTestIGWManager(t, m, policy.NATModeDistributed, pool, LinkLocalAllocator{}, nil) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + require.NoError(t, mgr.EnsureNATGatewaySubnetEgress(ctx, "vpc-1", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) + require.NoError(t, mgr.RemoveNATGatewaySubnetEgress(ctx, "vpc-1", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Empty(t, policies) + + // Idempotent: second remove is a no-op. + require.NoError(t, mgr.RemoveNATGatewaySubnetEgress(ctx, "vpc-1", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) +} + +// TestNATGatewaySubnetEgress_RejectsEmptyArgs guards the helper validation +// for callers that hand in zero values from malformed events. +func TestNATGatewaySubnetEgress_RejectsEmptyArgs(t *testing.T) { + ctx := context.Background() + m := mock.New() + mgr, _ := newTestIGWManager(t, m, policy.NATModeDistributed, nil, LinkLocalAllocator{}, nil) + + require.Error(t, mgr.EnsureNATGatewaySubnetEgress(ctx, "", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) + require.Error(t, mgr.EnsureNATGatewaySubnetEgress(ctx, "vpc-1", "", netip.MustParsePrefix("0.0.0.0/0"))) + require.Error(t, mgr.RemoveNATGatewaySubnetEgress(ctx, "", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) + require.Error(t, mgr.RemoveNATGatewaySubnetEgress(ctx, "vpc-1", "", netip.MustParsePrefix("0.0.0.0/0"))) +} + +// TestEnsureNATGatewaySubnetEgress_RequiresAttachedIGW asserts a clear error +// when AttachIGW hasn't run — the gateway nexthop must exist or we'd install +// an unreachable policy. NATGW depends on IGW per AWS. +func TestEnsureNATGatewaySubnetEgress_RequiresAttachedIGW(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + pool := &ExternalPoolConfig{Name: "p", PrefixLen: 24} // no Gateway → distributed link-local + mgr, _ := newTestIGWManager(t, m, policy.NATModeCentralized, pool, failingAllocator{}, nil) + + err := mgr.EnsureNATGatewaySubnetEgress(ctx, "vpc-1", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0")) + require.Error(t, err, "missing gateway nexthop must surface as an error") +} + +// TestRemoveNATGatewaySubnetEgress_PropagatesDeleteError verifies the manager +// surfaces an OVN delete error (e.g. router missing). policy.RouteManager. +// DeleteSubnetEgress targets the VPC router by name; with no router seeded the +// mock returns "router not found", which must bubble up. +func TestRemoveNATGatewaySubnetEgress_PropagatesDeleteError(t *testing.T) { + ctx := context.Background() + m := mock.New() + mgr, _ := newTestIGWManager(t, m, policy.NATModeDistributed, nil, LinkLocalAllocator{}, nil) + + err := mgr.RemoveNATGatewaySubnetEgress(ctx, "vpc-missing", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0")) + require.Error(t, err) +} + +// TestRemoveSubnetEgress_RejectsEmptyArgs covers the IGW-priority sibling of +// the NATGW empty-arg guard. Both vpcID and subnetID are mandatory. +func TestRemoveSubnetEgress_RejectsEmptyArgs(t *testing.T) { + ctx := context.Background() + m := mock.New() + mgr, _ := newTestIGWManager(t, m, policy.NATModeDistributed, nil, LinkLocalAllocator{}, nil) + + require.Error(t, mgr.RemoveSubnetEgress(ctx, "", "subnet-priv", netip.MustParsePrefix("0.0.0.0/0"))) + require.Error(t, mgr.RemoveSubnetEgress(ctx, "vpc-1", "", netip.MustParsePrefix("0.0.0.0/0"))) +} + +// TestRemoveSubnetEgress_PropagatesDeleteError exercises the delegate path +// to policy.RouteManager.DeleteSubnetEgress when the router doesn't exist. +func TestRemoveSubnetEgress_PropagatesDeleteError(t *testing.T) { + ctx := context.Background() + m := mock.New() + mgr, _ := newTestIGWManager(t, m, policy.NATModeDistributed, nil, LinkLocalAllocator{}, nil) + + err := mgr.RemoveSubnetEgress(ctx, "vpc-missing", "subnet-pub", netip.MustParsePrefix("0.0.0.0/0")) + require.Error(t, err) +} diff --git a/spinifex/network/ovn/client.go b/spinifex/network/ovn/client.go index 3f1dc25c0..afc9ae887 100644 --- a/spinifex/network/ovn/client.go +++ b/spinifex/network/ovn/client.go @@ -116,6 +116,14 @@ type Client interface { DeleteStaticRoute(ctx context.Context, routerName string, ipPrefix string) error FindStaticRoute(ctx context.Context, routerName, ipPrefix string) (*nbdb.LogicalRouterStaticRoute, error) + // Logical Router Policies (per-subnet egress steering). Identity is the + // (router, priority, match) triple — same triple replaces, missing rows + // on delete return nil (mirrors AddStaticRoute / DeleteStaticRoute). + AddLogicalRouterPolicy(ctx context.Context, routerName string, policy *nbdb.LogicalRouterPolicy) error + DeleteLogicalRouterPolicy(ctx context.Context, routerName string, priority int, match string) error + FindLogicalRouterPolicy(ctx context.Context, routerName string, priority int, match string) (*nbdb.LogicalRouterPolicy, error) + ListLogicalRouterPolicies(ctx context.Context, routerName string) ([]nbdb.LogicalRouterPolicy, error) + // Port Groups (security group enforcement) CreatePortGroup(ctx context.Context, name string, ports []string) error // EnsurePortGroup is the wait-op-protected create-or-get analogue of @@ -150,6 +158,16 @@ type Client interface { AddACLs(ctx context.Context, portGroupName string, specs []ACLSpec) error ClearACLs(ctx context.Context, portGroupName string) error + // ReplaceACLs atomically swaps the port group's ACL set: detach + delete + // the existing rows and attach freshly-created rows in ONE OVSDB + // transaction. SG mutations must use this instead of ClearACLs followed + // by AddACLs — the latter leaves the port group with zero ACLs between + // transactions, and a no-ACL port group on a tenant LSP path defaults + // to drop, producing observable mid-flight egress drops on connectionless + // traffic (ICMP) and on TCP SYNs that don't match an existing conntrack + // entry. + ReplaceACLs(ctx context.Context, portGroupName string, specs []ACLSpec) error + // Gateway Chassis (HA scheduling for gateway router ports) SetGatewayChassis(ctx context.Context, lrpName string, chassisName string, priority int) error GetGatewayChassisByName(ctx context.Context, name string) (*nbdb.GatewayChassis, error) diff --git a/spinifex/network/ovn/live.go b/spinifex/network/ovn/live.go index e23f0e9b5..79b974b89 100644 --- a/spinifex/network/ovn/live.go +++ b/spinifex/network/ovn/live.go @@ -1095,6 +1095,125 @@ func (c *LiveClient) DeleteStaticRoute(ctx context.Context, routerName string, i return nil } +// Logical Router Policies (per-subnet egress steering). + +func (c *LiveClient) AddLogicalRouterPolicy(ctx context.Context, routerName string, policy *nbdb.LogicalRouterPolicy) error { + if policy.UUID == "" { + policy.UUID = namedUUID("lrp_", fmt.Sprintf("%d:%s", policy.Priority, policy.Match)) + } + if policy.ExternalIDs == nil { + policy.ExternalIDs = map[string]string{} + } + if policy.Options == nil { + policy.Options = map[string]string{} + } + + createOps, err := c.client.Create(policy) + if err != nil { + return fmt.Errorf("create LR policy ops: %w", err) + } + + lr, err := c.GetLogicalRouter(ctx, routerName) + if err != nil { + return fmt.Errorf("get logical router for policy add: %w", err) + } + + mutateOps, err := c.client.Where(lr).Mutate(lr, model.Mutation{ + Field: &lr.Policies, + Mutator: "insert", + Value: []string{policy.UUID}, + }) + if err != nil { + return fmt.Errorf("mutate router policies ops: %w", err) + } + + if err := c.transactOps(ctx, append(createOps, mutateOps...)); err != nil { + return fmt.Errorf("add LR policy transact: %w", err) + } + return nil +} + +func (c *LiveClient) FindLogicalRouterPolicy(ctx context.Context, routerName string, priority int, match string) (*nbdb.LogicalRouterPolicy, error) { + lr, err := c.GetLogicalRouter(ctx, routerName) + if err != nil { + return nil, fmt.Errorf("get logical router for policy lookup: %w", err) + } + owned := make(map[string]struct{}, len(lr.Policies)) + for _, u := range lr.Policies { + owned[u] = struct{}{} + } + var policies []nbdb.LogicalRouterPolicy + if err := c.client.WhereCache(func(p *nbdb.LogicalRouterPolicy) bool { + if p.Priority != priority || p.Match != match { + return false + } + _, ok := owned[p.UUID] + return ok + }).List(ctx, &policies); err != nil { + return nil, fmt.Errorf("list LR policies: %w", err) + } + if len(policies) == 0 { + return nil, nil + } + return &policies[0], nil +} + +func (c *LiveClient) ListLogicalRouterPolicies(ctx context.Context, routerName string) ([]nbdb.LogicalRouterPolicy, error) { + lr, err := c.GetLogicalRouter(ctx, routerName) + if err != nil { + return nil, fmt.Errorf("get logical router for policy list: %w", err) + } + if len(lr.Policies) == 0 { + return nil, nil + } + owned := make(map[string]struct{}, len(lr.Policies)) + for _, u := range lr.Policies { + owned[u] = struct{}{} + } + var policies []nbdb.LogicalRouterPolicy + if err := c.client.WhereCache(func(p *nbdb.LogicalRouterPolicy) bool { + _, ok := owned[p.UUID] + return ok + }).List(ctx, &policies); err != nil { + return nil, fmt.Errorf("list LR policies: %w", err) + } + return policies, nil +} + +func (c *LiveClient) DeleteLogicalRouterPolicy(ctx context.Context, routerName string, priority int, match string) error { + existing, err := c.FindLogicalRouterPolicy(ctx, routerName, priority, match) + if err != nil { + return err + } + if existing == nil { + return nil + } + + lr, err := c.GetLogicalRouter(ctx, routerName) + if err != nil { + return fmt.Errorf("get logical router for policy delete: %w", err) + } + + mutateOps, err := c.client.Where(lr).Mutate(lr, model.Mutation{ + Field: &lr.Policies, + Mutator: "delete", + Value: []string{existing.UUID}, + }) + if err != nil { + return fmt.Errorf("mutate router policies ops: %w", err) + } + + deleteOps, err := c.client.Where(existing).Delete() + if err != nil { + return fmt.Errorf("delete LR policy ops: %w", err) + } + + if err := c.transactOps(ctx, append(mutateOps, deleteOps...)); err != nil { + return fmt.Errorf("delete LR policy transact: %w", err) + } + return nil +} + // Port Groups func (c *LiveClient) CreatePortGroup(ctx context.Context, name string, ports []string) error { @@ -1358,3 +1477,83 @@ func (c *LiveClient) ClearACLs(ctx context.Context, portGroupName string) error } return nil } + +// ReplaceACLs atomically swaps the port group's ACL set in a single OVSDB +// transaction: detach + delete every existing ACL row, create the new rows, +// attach them to the port group. Equivalent to ClearACLs followed by AddACLs +// without the mid-flight window where the port group has zero ACLs (which +// would otherwise default-drop tenant traffic on the egress test path). +func (c *LiveClient) ReplaceACLs(ctx context.Context, portGroupName string, specs []ACLSpec) error { + pg, err := c.getPortGroup(ctx, portGroupName) + if err != nil { + return fmt.Errorf("replace ACLs port group lookup: %w", err) + } + + var ops []ovsdb.Operation + + if len(pg.ACLs) > 0 { + detachOps, dErr := c.client.Where(pg).Mutate(pg, model.Mutation{ + Field: &pg.ACLs, + Mutator: ovsdb.MutateOperationDelete, + Value: pg.ACLs, + }) + if dErr != nil { + return fmt.Errorf("mutate port group ACLs (detach) ops: %w", dErr) + } + ops = append(ops, detachOps...) + for _, aclUUID := range pg.ACLs { + delOps, dErr := c.client.Where(&nbdb.ACL{UUID: aclUUID}).Delete() + if dErr != nil { + return fmt.Errorf("delete ACL ops: %w", dErr) + } + ops = append(ops, delOps...) + } + } + + uuids := make([]string, 0, len(specs)) + for i, spec := range specs { + acl := &nbdb.ACL{ + UUID: namedUUID("acl_", fmt.Sprintf("%s_%d_%s", portGroupName, i, spec.Direction)), + Direction: spec.Direction, + Priority: spec.Priority, + Match: spec.Match, + Action: spec.Action, + Log: spec.Log, + ExternalIDs: map[string]string{}, + } + if spec.Name != "" { + name := spec.Name + acl.Name = &name + } + if spec.Severity != "" { + severity := spec.Severity + acl.Severity = &severity + } + createOps, cErr := c.client.Create(acl) + if cErr != nil { + return fmt.Errorf("create ACL ops: %w", cErr) + } + ops = append(ops, createOps...) + uuids = append(uuids, acl.UUID) + } + + if len(uuids) > 0 { + attachOps, aErr := c.client.Where(pg).Mutate(pg, model.Mutation{ + Field: &pg.ACLs, + Mutator: ovsdb.MutateOperationInsert, + Value: uuids, + }) + if aErr != nil { + return fmt.Errorf("mutate port group ACLs (attach) ops: %w", aErr) + } + ops = append(ops, attachOps...) + } + + if len(ops) == 0 { + return nil + } + if err := c.transactOps(ctx, ops); err != nil { + return fmt.Errorf("replace ACLs transact: %w", err) + } + return nil +} diff --git a/spinifex/network/ovn/mock/mock.go b/spinifex/network/ovn/mock/mock.go index 08c03b7d8..fd23b60a5 100644 --- a/spinifex/network/ovn/mock/mock.go +++ b/spinifex/network/ovn/mock/mock.go @@ -25,6 +25,7 @@ type Client struct { DHCPOpts map[string]*nbdb.DHCPOptions NATs map[string]*nbdb.NAT // keyed by UUID StaticRoutes map[string]*nbdb.LogicalRouterStaticRoute // keyed by UUID + LRPolicies map[string]*nbdb.LogicalRouterPolicy // keyed by UUID PortGroups map[string]*nbdb.PortGroup // keyed by name ACLs map[string]*nbdb.ACL // keyed by UUID GatewayChassis map[string]*nbdb.GatewayChassis // keyed by UUID @@ -39,8 +40,10 @@ type Client struct { // AddACLErrAfter, when > 0, makes AddACLs return an error on the Nth // (1-indexed) call. Lets tests inject a transient failure mid-flow. - AddACLErrAfter int - AddACLCalls int + AddACLErrAfter int + AddACLCalls int + ClearACLCalls int + ReplaceACLCalls int } var _ ovn.Client = (*Client)(nil) @@ -55,6 +58,7 @@ func New() *Client { DHCPOpts: make(map[string]*nbdb.DHCPOptions), NATs: make(map[string]*nbdb.NAT), StaticRoutes: make(map[string]*nbdb.LogicalRouterStaticRoute), + LRPolicies: make(map[string]*nbdb.LogicalRouterPolicy), PortGroups: make(map[string]*nbdb.PortGroup), ACLs: make(map[string]*nbdb.ACL), GatewayChassis: make(map[string]*nbdb.GatewayChassis), @@ -644,6 +648,91 @@ func (m *Client) DeleteStaticRoute(_ context.Context, routerName string, ipPrefi return nil } +// Logical Router Policies + +func (m *Client) AddLogicalRouterPolicy(_ context.Context, routerName string, policy *nbdb.LogicalRouterPolicy) error { + m.Mu.Lock() + defer m.Mu.Unlock() + lr, exists := m.Routers[routerName] + if !exists { + return fmt.Errorf("logical router %q not found", routerName) + } + if policy.UUID == "" { + policy.UUID = utils.GenerateResourceID("lrp") + } + stored := *policy + m.LRPolicies[policy.UUID] = &stored + lr.Policies = append(lr.Policies, policy.UUID) + return nil +} + +func (m *Client) FindLogicalRouterPolicy(_ context.Context, routerName string, priority int, match string) (*nbdb.LogicalRouterPolicy, error) { + m.Mu.Lock() + defer m.Mu.Unlock() + lr, exists := m.Routers[routerName] + if !exists { + return nil, fmt.Errorf("logical router %q not found", routerName) + } + for _, uuid := range lr.Policies { + p, ok := m.LRPolicies[uuid] + if !ok { + continue + } + if p.Priority == priority && p.Match == match { + result := *p + return &result, nil + } + } + return nil, nil +} + +func (m *Client) ListLogicalRouterPolicies(_ context.Context, routerName string) ([]nbdb.LogicalRouterPolicy, error) { + m.Mu.Lock() + defer m.Mu.Unlock() + lr, exists := m.Routers[routerName] + if !exists { + return nil, fmt.Errorf("logical router %q not found", routerName) + } + out := make([]nbdb.LogicalRouterPolicy, 0, len(lr.Policies)) + for _, uuid := range lr.Policies { + if p, ok := m.LRPolicies[uuid]; ok { + out = append(out, *p) + } + } + return out, nil +} + +func (m *Client) DeleteLogicalRouterPolicy(_ context.Context, routerName string, priority int, match string) error { + m.Mu.Lock() + defer m.Mu.Unlock() + lr, exists := m.Routers[routerName] + if !exists { + return fmt.Errorf("logical router %q not found", routerName) + } + var foundUUID string + for _, uuid := range lr.Policies { + p, ok := m.LRPolicies[uuid] + if !ok { + continue + } + if p.Priority == priority && p.Match == match { + foundUUID = uuid + break + } + } + if foundUUID == "" { + return nil + } + for i, uuid := range lr.Policies { + if uuid == foundUUID { + lr.Policies = append(lr.Policies[:i], lr.Policies[i+1:]...) + break + } + } + delete(m.LRPolicies, foundUUID) + return nil +} + // Port Groups func (m *Client) CreatePortGroup(_ context.Context, name string, ports []string) error { @@ -806,6 +895,7 @@ func (m *Client) AddACLs(_ context.Context, portGroupName string, specs []ovn.AC func (m *Client) ClearACLs(_ context.Context, portGroupName string) error { m.Mu.Lock() defer m.Mu.Unlock() + m.ClearACLCalls++ pg, exists := m.PortGroups[portGroupName] if !exists { return fmt.Errorf("port group %q not found", portGroupName) @@ -817,6 +907,48 @@ func (m *Client) ClearACLs(_ context.Context, portGroupName string) error { return nil } +// ReplaceACLs atomically swaps the port group's ACL set under a single mutex +// hold — semantically equivalent to ClearACLs+AddACLs but with no observable +// mid-flight window where pg.ACLs is empty. +func (m *Client) ReplaceACLs(_ context.Context, portGroupName string, specs []ovn.ACLSpec) error { + m.Mu.Lock() + defer m.Mu.Unlock() + m.ReplaceACLCalls++ + m.AddACLCalls++ + if m.AddACLErrAfter > 0 && m.AddACLCalls == m.AddACLErrAfter { + return fmt.Errorf("injected ReplaceACLs failure on call %d", m.AddACLCalls) + } + pg, exists := m.PortGroups[portGroupName] + if !exists { + return fmt.Errorf("port group %q not found", portGroupName) + } + for _, aclUUID := range pg.ACLs { + delete(m.ACLs, aclUUID) + } + pg.ACLs = pg.ACLs[:0] + for _, spec := range specs { + acl := &nbdb.ACL{ + UUID: utils.GenerateResourceID("acl"), + Direction: spec.Direction, + Priority: spec.Priority, + Match: spec.Match, + Action: spec.Action, + Log: spec.Log, + } + if spec.Name != "" { + name := spec.Name + acl.Name = &name + } + if spec.Severity != "" { + severity := spec.Severity + acl.Severity = &severity + } + m.ACLs[acl.UUID] = acl + pg.ACLs = append(pg.ACLs, acl.UUID) + } + return nil +} + // ListLogicalRouterPorts returns every LRP across the mock state. func (m *Client) ListLogicalRouterPorts(_ context.Context) ([]nbdb.LogicalRouterPort, error) { m.Mu.Lock() diff --git a/spinifex/network/ovn/mock/mock_test.go b/spinifex/network/ovn/mock/mock_test.go index 2a260fbe9..d56c27ca4 100644 --- a/spinifex/network/ovn/mock/mock_test.go +++ b/spinifex/network/ovn/mock/mock_test.go @@ -285,3 +285,107 @@ func TestEnsurePortGroup_ConcurrentSingleSurvivor(t *testing.T) { t.Fatalf("expected 1 port group, got %d", len(rows)) } } + +func TestLogicalRouterPolicies_RouterMissing(t *testing.T) { + m := New() + _ = m.Connect(context.Background()) + ctx := context.Background() + + if err := m.AddLogicalRouterPolicy(ctx, "vpc-missing", &nbdb.LogicalRouterPolicy{ + Priority: 1000, Match: "inport == \"x\"", Action: "reroute", + }); err == nil { + t.Fatal("AddLogicalRouterPolicy: expected error for missing router") + } + + if _, err := m.FindLogicalRouterPolicy(ctx, "vpc-missing", 1000, "x"); err == nil { + t.Fatal("FindLogicalRouterPolicy: expected error for missing router") + } + + if _, err := m.ListLogicalRouterPolicies(ctx, "vpc-missing"); err == nil { + t.Fatal("ListLogicalRouterPolicies: expected error for missing router") + } + + if err := m.DeleteLogicalRouterPolicy(ctx, "vpc-missing", 1000, "x"); err == nil { + t.Fatal("DeleteLogicalRouterPolicy: expected error for missing router") + } +} + +func TestLogicalRouterPolicies_Roundtrip(t *testing.T) { + m := New() + _ = m.Connect(context.Background()) + ctx := context.Background() + + if err := m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-r1"}); err != nil { + t.Fatalf("CreateLogicalRouter: %v", err) + } + + nexthop := "192.168.1.1" + p1 := &nbdb.LogicalRouterPolicy{Priority: 1000, Match: `inport == "rtr-a"`, Action: "reroute", Nexthop: &nexthop} + p2 := &nbdb.LogicalRouterPolicy{Priority: 900, Match: `inport == "rtr-b"`, Action: "reroute", Nexthop: &nexthop} + + if err := m.AddLogicalRouterPolicy(ctx, "vpc-r1", p1); err != nil { + t.Fatalf("AddLogicalRouterPolicy p1: %v", err) + } + if err := m.AddLogicalRouterPolicy(ctx, "vpc-r1", p2); err != nil { + t.Fatalf("AddLogicalRouterPolicy p2: %v", err) + } + + got, err := m.FindLogicalRouterPolicy(ctx, "vpc-r1", 1000, `inport == "rtr-a"`) + if err != nil || got == nil { + t.Fatalf("FindLogicalRouterPolicy: %v %v", got, err) + } + if got.Match != `inport == "rtr-a"` { + t.Errorf("Find match mismatch: %q", got.Match) + } + + miss, err := m.FindLogicalRouterPolicy(ctx, "vpc-r1", 1000, `inport == "nope"`) + if err != nil || miss != nil { + t.Errorf("FindLogicalRouterPolicy (miss): %v %v", miss, err) + } + + policies, err := m.ListLogicalRouterPolicies(ctx, "vpc-r1") + if err != nil { + t.Fatalf("ListLogicalRouterPolicies: %v", err) + } + if len(policies) != 2 { + t.Fatalf("expected 2 policies, got %d", len(policies)) + } + + if err := m.DeleteLogicalRouterPolicy(ctx, "vpc-r1", 1000, `inport == "rtr-a"`); err != nil { + t.Fatalf("DeleteLogicalRouterPolicy: %v", err) + } + if err := m.DeleteLogicalRouterPolicy(ctx, "vpc-r1", 1000, `inport == "rtr-a"`); err != nil { + t.Errorf("DeleteLogicalRouterPolicy (idempotent): %v", err) + } + + policies, _ = m.ListLogicalRouterPolicies(ctx, "vpc-r1") + if len(policies) != 1 { + t.Errorf("expected 1 policy after delete, got %d", len(policies)) + } +} + +func TestLogicalRouterPolicies_DanglingUUIDsIgnored(t *testing.T) { + m := New() + _ = m.Connect(context.Background()) + ctx := context.Background() + + if err := m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{Name: "vpc-r1"}); err != nil { + t.Fatalf("CreateLogicalRouter: %v", err) + } + + m.Routers["vpc-r1"].Policies = []string{"missing-uuid", "also-missing"} + + if _, err := m.FindLogicalRouterPolicy(ctx, "vpc-r1", 1000, "x"); err != nil { + t.Fatalf("Find with dangling refs: %v", err) + } + if err := m.DeleteLogicalRouterPolicy(ctx, "vpc-r1", 1000, "x"); err != nil { + t.Fatalf("Delete with dangling refs: %v", err) + } + policies, err := m.ListLogicalRouterPolicies(ctx, "vpc-r1") + if err != nil { + t.Fatalf("List with dangling refs: %v", err) + } + if len(policies) != 0 { + t.Fatalf("expected 0 policies, got %d", len(policies)) + } +} diff --git a/spinifex/network/ovn/nbdb/model.go b/spinifex/network/ovn/nbdb/model.go index e1f5c32a1..9a922c283 100644 --- a/spinifex/network/ovn/nbdb/model.go +++ b/spinifex/network/ovn/nbdb/model.go @@ -83,6 +83,21 @@ type LogicalRouterStaticRoute struct { ExternalIDs map[string]string `ovsdb:"external_ids"` } +// LogicalRouterPolicy represents an OVN Logical_Router_Policy row attached +// to Logical_Router.policies. Used for per-subnet egress steering: the match +// expression typically combines an `inport == "rtr-"` filter with +// a destination predicate, and the action reroutes via a chosen nexthop. +type LogicalRouterPolicy struct { + UUID string `ovsdb:"_uuid"` + Priority int `ovsdb:"priority"` + Match string `ovsdb:"match"` + Action string `ovsdb:"action"` // "allow", "drop", "reroute" + Nexthop *string `ovsdb:"nexthop"` + Nexthops []string `ovsdb:"nexthops"` + Options map[string]string `ovsdb:"options"` + ExternalIDs map[string]string `ovsdb:"external_ids"` +} + // PortGroup represents an OVN Port_Group (used for SG enforcement). type PortGroup struct { UUID string `ovsdb:"_uuid"` @@ -135,6 +150,7 @@ func FullDatabaseModel() (model.ClientDBModel, error) { "DHCP_Options": &DHCPOptions{}, "NAT": &NAT{}, "Logical_Router_Static_Route": &LogicalRouterStaticRoute{}, + "Logical_Router_Policy": &LogicalRouterPolicy{}, "Gateway_Chassis": &GatewayChassis{}, "Port_Group": &PortGroup{}, "ACL": &ACL{}, diff --git a/spinifex/network/ovn/nbdb/model_test.go b/spinifex/network/ovn/nbdb/model_test.go index ad51352cb..367714395 100644 --- a/spinifex/network/ovn/nbdb/model_test.go +++ b/spinifex/network/ovn/nbdb/model_test.go @@ -22,6 +22,7 @@ func TestFullDatabaseModel(t *testing.T) { "DHCP_Options", "NAT", "Logical_Router_Static_Route", + "Logical_Router_Policy", "Gateway_Chassis", "Port_Group", "ACL", diff --git a/spinifex/network/policy/route.go b/spinifex/network/policy/route.go index c7ad83dde..7893e8595 100644 --- a/spinifex/network/policy/route.go +++ b/spinifex/network/policy/route.go @@ -10,7 +10,21 @@ import ( "github.com/mulgadc/spinifex/spinifex/network/topology" ) -const defaultRoutePrefix = "0.0.0.0/0" +const ( + defaultRoutePrefix = "0.0.0.0/0" + + // SubnetEgressPriorityIGW is the priority assigned to per-subnet default + // egress policies routing via an IGW. NATGW egress sits at a different + // priority so the route precedence is deterministic when both apply. + // SubnetEgressPriorityDrop sits above both reroute priorities; a subnet + // whose effective route table lacks 0.0.0.0/0 gets a drop policy at this + // priority to override the VPC LR's router-wide default static route + // (which is required so lr_in_ip_routing doesn't drop at priority 0 + // before lr_in_policy fires). + SubnetEgressPriorityDrop = 1100 + SubnetEgressPriorityIGW = 1000 + SubnetEgressPriorityNATGW = 900 +) // RouteSpec is a static route on a VPC's LogicalRouter. OutputPort is // required when Nexthop isn't directly-connected (e.g. IGW default route on @@ -21,6 +35,23 @@ type RouteSpec struct { OutputPort string } +// SubnetEgressSpec describes a per-subnet egress override installed as an +// OVN Logical_Router_Policy. The match is built from SubnetID (inport == +// "rtr-") AND ip4.dst == Prefix; the action is "reroute" via +// Nexthop, egressing through OutputPort. ExcludeCIDRs are appended as +// `&& ip4.dst != ` clauses so in-VPC traffic skips the reroute and +// follows the directly-connected route instead — without this, NATGW egress +// policies on a `0.0.0.0/0` prefix would intercept return traffic from +// peer subnets (bastion -> private SSH) and forward it to the public IP. +type SubnetEgressSpec struct { + SubnetID string + Prefix netip.Prefix + Nexthop string + OutputPort string + Priority int + ExcludeCIDRs []netip.Prefix +} + // RouteManager owns static routes on VPC LogicalRouters. Adds are // idempotent (no-op on match, delete-then-add on drift) so the reconciler // can replay safely. @@ -29,6 +60,24 @@ type RouteManager interface { DeleteDefaultRoute(ctx context.Context, vpcID string) error AddStaticRoute(ctx context.Context, vpcID string, route RouteSpec) error DeleteStaticRoute(ctx context.Context, vpcID string, prefix netip.Prefix) error + AddSubnetEgress(ctx context.Context, vpcID string, spec SubnetEgressSpec) error + DeleteSubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix, excludeCIDRs []netip.Prefix) error + AddSubnetEgressDrop(ctx context.Context, vpcID string, spec SubnetEgressDropSpec) error + DeleteSubnetEgressDrop(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix, excludeCIDRs []netip.Prefix) error +} + +// SubnetEgressDropSpec describes a per-subnet egress DROP override installed +// as an OVN Logical_Router_Policy at SubnetEgressPriorityDrop. Match shape +// mirrors SubnetEgressSpec (inport == rtr- && ip4.dst == prefix with +// optional ip4.dst != clauses) but the action is "drop" with no +// nexthop/output port. Used to gate subnets whose effective route table +// lacks a 0.0.0.0/0 entry; the policy fires after lr_in_ip_routing has +// already matched the VPC LR's router-wide default static route, killing +// the packet before egress. +type SubnetEgressDropSpec struct { + SubnetID string + Prefix netip.Prefix + ExcludeCIDRs []netip.Prefix } type routeManager struct { @@ -110,6 +159,138 @@ func (m *routeManager) deleteByPrefixString(ctx context.Context, vpcID, prefix s return nil } +func (m *routeManager) AddSubnetEgress(ctx context.Context, vpcID string, spec SubnetEgressSpec) error { + if spec.SubnetID == "" { + return fmt.Errorf("subnet egress: SubnetID required") + } + if spec.OutputPort == "" { + return fmt.Errorf("subnet egress: OutputPort required (ovn-northd drops policy reroute otherwise)") + } + if spec.Priority == 0 { + return fmt.Errorf("subnet egress: Priority required") + } + router := topology.VPCRouter(vpcID) + match := subnetEgressMatch(spec.SubnetID, spec.Prefix, spec.ExcludeCIDRs) + + existing, err := m.ovn.FindLogicalRouterPolicy(ctx, router, spec.Priority, match) + if err != nil { + return fmt.Errorf("find LR policy %q on %s: %w", match, router, err) + } + if existing != nil { + if policyMatches(existing, spec) { + return nil + } + if err := m.ovn.DeleteLogicalRouterPolicy(ctx, router, spec.Priority, match); err != nil { + return fmt.Errorf("delete drifted LR policy %q on %s: %w", match, router, err) + } + } + + nexthop := spec.Nexthop + row := &nbdb.LogicalRouterPolicy{ + Priority: spec.Priority, + Match: match, + Action: "reroute", + Nexthop: &nexthop, + Options: map[string]string{}, + ExternalIDs: map[string]string{ + "spinifex:subnet": spec.SubnetID, + "spinifex:output_port": spec.OutputPort, + }, + } + if err := m.ovn.AddLogicalRouterPolicy(ctx, router, row); err != nil { + return fmt.Errorf("add LR policy %q -> %s on %s: %w", match, spec.Nexthop, router, err) + } + return nil +} + +func (m *routeManager) DeleteSubnetEgress(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix, excludeCIDRs []netip.Prefix) error { + router := topology.VPCRouter(vpcID) + match := subnetEgressMatch(subnetID, prefix, excludeCIDRs) + if err := m.ovn.DeleteLogicalRouterPolicy(ctx, router, SubnetEgressPriorityIGW, match); err != nil { + return fmt.Errorf("delete IGW LR policy %q on %s: %w", match, router, err) + } + if err := m.ovn.DeleteLogicalRouterPolicy(ctx, router, SubnetEgressPriorityNATGW, match); err != nil { + return fmt.Errorf("delete NATGW LR policy %q on %s: %w", match, router, err) + } + return nil +} + +func (m *routeManager) AddSubnetEgressDrop(ctx context.Context, vpcID string, spec SubnetEgressDropSpec) error { + if spec.SubnetID == "" { + return fmt.Errorf("subnet egress drop: SubnetID required") + } + if !spec.Prefix.IsValid() { + return fmt.Errorf("subnet egress drop: Prefix required") + } + router := topology.VPCRouter(vpcID) + match := subnetEgressMatch(spec.SubnetID, spec.Prefix, spec.ExcludeCIDRs) + + existing, err := m.ovn.FindLogicalRouterPolicy(ctx, router, SubnetEgressPriorityDrop, match) + if err != nil { + return fmt.Errorf("find LR drop policy %q on %s: %w", match, router, err) + } + if existing != nil { + if dropPolicyMatches(existing) { + return nil + } + if err := m.ovn.DeleteLogicalRouterPolicy(ctx, router, SubnetEgressPriorityDrop, match); err != nil { + return fmt.Errorf("delete drifted LR drop policy %q on %s: %w", match, router, err) + } + } + + row := &nbdb.LogicalRouterPolicy{ + Priority: SubnetEgressPriorityDrop, + Match: match, + Action: "drop", + Options: map[string]string{}, + ExternalIDs: map[string]string{ + "spinifex:subnet": spec.SubnetID, + "spinifex:gate": "drop", + }, + } + if err := m.ovn.AddLogicalRouterPolicy(ctx, router, row); err != nil { + return fmt.Errorf("add LR drop policy %q on %s: %w", match, router, err) + } + return nil +} + +func (m *routeManager) DeleteSubnetEgressDrop(ctx context.Context, vpcID, subnetID string, prefix netip.Prefix, excludeCIDRs []netip.Prefix) error { + router := topology.VPCRouter(vpcID) + match := subnetEgressMatch(subnetID, prefix, excludeCIDRs) + if err := m.ovn.DeleteLogicalRouterPolicy(ctx, router, SubnetEgressPriorityDrop, match); err != nil { + return fmt.Errorf("delete LR drop policy %q on %s: %w", match, router, err) + } + return nil +} + +func subnetEgressMatch(subnetID string, prefix netip.Prefix, excludeCIDRs []netip.Prefix) string { + match := fmt.Sprintf(`inport == %q && ip4.dst == %s`, topology.SubnetRouterPort(subnetID), prefix.String()) + for _, ex := range excludeCIDRs { + if !ex.IsValid() { + continue + } + match += fmt.Sprintf(` && ip4.dst != %s`, ex.String()) + } + return match +} + +func policyMatches(existing *nbdb.LogicalRouterPolicy, want SubnetEgressSpec) bool { + if existing.Action != "reroute" { + return false + } + if existing.Nexthop == nil || *existing.Nexthop != want.Nexthop { + return false + } + if existing.ExternalIDs["spinifex:output_port"] != want.OutputPort { + return false + } + return true +} + +func dropPolicyMatches(existing *nbdb.LogicalRouterPolicy) bool { + return existing.Action == "drop" +} + func routeMatches(existing *nbdb.LogicalRouterStaticRoute, want RouteSpec) bool { if existing.Nexthop != want.Nexthop { return false diff --git a/spinifex/network/policy/route_test.go b/spinifex/network/policy/route_test.go index e16a11457..6fa5a4a33 100644 --- a/spinifex/network/policy/route_test.go +++ b/spinifex/network/policy/route_test.go @@ -102,3 +102,505 @@ func TestRouteManager_DeleteStaticRoute_RemovesRow(t *testing.T) { require.NoError(t, rm.DeleteStaticRoute(ctx, "vpc-1", prefix)) assert.Nil(t, findRoute(m, "10.42.0.0/24")) } + +func TestRouteManager_AddSubnetEgress_InstallsScopedPolicy(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + })) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + p := policies[0] + assert.Equal(t, "reroute", p.Action) + assert.Equal(t, SubnetEgressPriorityIGW, p.Priority) + require.NotNil(t, p.Nexthop) + assert.Equal(t, "192.168.1.1", *p.Nexthop) + assert.Contains(t, p.Match, topology.SubnetRouterPort("subnet-pub")) + assert.Contains(t, p.Match, "ip4.dst == 0.0.0.0/0") + assert.Equal(t, "subnet-pub", p.ExternalIDs["spinifex:subnet"]) + assert.Equal(t, "gw-vpc-1", p.ExternalIDs["spinifex:output_port"]) +} + +func TestRouteManager_AddSubnetEgress_IsIdempotent(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + spec := SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + } + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", spec)) + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", spec)) + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", spec)) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Len(t, policies, 1) +} + +func TestRouteManager_AddSubnetEgress_DriftReplacesNexthop(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + spec := SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + } + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", spec)) + spec.Nexthop = "192.168.1.254" + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", spec)) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + require.NotNil(t, policies[0].Nexthop) + assert.Equal(t, "192.168.1.254", *policies[0].Nexthop) +} + +func TestRouteManager_AddSubnetEgress_PerSubnetSeparate(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + base := SubnetEgressSpec{ + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + } + a := base + a.SubnetID = "subnet-a" + b := base + b.SubnetID = "subnet-b" + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", a)) + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", b)) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Len(t, policies, 2, "per-subnet egress policies must coexist on the same VPC router") +} + +func TestRouteManager_DeleteSubnetEgress_RemovesPolicy(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("0.0.0.0/0") + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: prefix, + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + })) + require.NoError(t, rm.DeleteSubnetEgress(ctx, "vpc-1", "subnet-pub", prefix, nil)) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Empty(t, policies) +} + +func TestRouteManager_DeleteSubnetEgress_IdempotentOnMissing(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + require.NoError(t, rm.DeleteSubnetEgress(ctx, "vpc-1", "subnet-pub", netip.MustParsePrefix("0.0.0.0/0"), nil)) +} + +func TestRouteManager_AddSubnetEgress_RejectsMissingFields(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + base := SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + } + + cases := map[string]func(s *SubnetEgressSpec){ + "missing SubnetID": func(s *SubnetEgressSpec) { s.SubnetID = "" }, + "missing OutputPort": func(s *SubnetEgressSpec) { s.OutputPort = "" }, + "zero Priority": func(s *SubnetEgressSpec) { s.Priority = 0 }, + } + for name, mut := range cases { + t.Run(name, func(t *testing.T) { + spec := base + mut(&spec) + err := rm.AddSubnetEgress(ctx, "vpc-1", spec) + require.Error(t, err) + }) + } +} + +func TestRouteManager_AddSubnetEgress_PropagatesFindError(t *testing.T) { + ctx := context.Background() + m := mock.New() + rm := NewRouteManager(m) + + err := rm.AddSubnetEgress(ctx, "vpc-missing", SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + Nexthop: "192.168.1.1", + OutputPort: "gw", + Priority: SubnetEgressPriorityIGW, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "find LR policy") +} + +func TestRouteManager_AddSubnetEgress_ReplacesPolicyOnActionDrift(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + router := topology.VPCRouter("vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("0.0.0.0/0") + match := subnetEgressMatch("subnet-pub", prefix, nil) + + stale := &nbdb.LogicalRouterPolicy{ + UUID: "drift-uuid", + Priority: SubnetEgressPriorityIGW, + Match: match, + Action: "drop", + ExternalIDs: map[string]string{"spinifex:output_port": "gw-vpc-1"}, + } + m.LRPolicies[stale.UUID] = stale + m.Routers[router].Policies = append(m.Routers[router].Policies, stale.UUID) + + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: prefix, + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + })) + + policies, err := m.ListLogicalRouterPolicies(ctx, router) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Equal(t, "reroute", policies[0].Action) +} + +func TestRouteManager_AddSubnetEgress_ReplacesPolicyOnOutputPortDrift(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + router := topology.VPCRouter("vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("0.0.0.0/0") + match := subnetEgressMatch("subnet-pub", prefix, nil) + nexthop := "192.168.1.1" + + stale := &nbdb.LogicalRouterPolicy{ + UUID: "drift-uuid", + Priority: SubnetEgressPriorityIGW, + Match: match, + Action: "reroute", + Nexthop: &nexthop, + ExternalIDs: map[string]string{"spinifex:output_port": "stale-port"}, + } + m.LRPolicies[stale.UUID] = stale + m.Routers[router].Policies = append(m.Routers[router].Policies, stale.UUID) + + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: prefix, + Nexthop: nexthop, + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + })) + + policies, err := m.ListLogicalRouterPolicies(ctx, router) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Equal(t, "gw-vpc-1", policies[0].ExternalIDs["spinifex:output_port"]) +} + +func TestRouteManager_DeleteSubnetEgress_PropagatesError(t *testing.T) { + ctx := context.Background() + m := mock.New() + rm := NewRouteManager(m) + + err := rm.DeleteSubnetEgress(ctx, "vpc-missing", "subnet-pub", netip.MustParsePrefix("0.0.0.0/0"), nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "delete IGW LR policy") +} + +func TestSubnetEgressMatch_AppendsExcludeCIDRs(t *testing.T) { + prefix := netip.MustParsePrefix("0.0.0.0/0") + excludes := []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/16"), + netip.MustParsePrefix("172.20.0.0/24"), + } + got := subnetEgressMatch("subnet-pub", prefix, excludes) + + assert.Contains(t, got, `inport == "rtr-subnet-pub"`) + assert.Contains(t, got, "ip4.dst == 0.0.0.0/0") + assert.Contains(t, got, "ip4.dst != 10.0.0.0/16") + assert.Contains(t, got, "ip4.dst != 172.20.0.0/24") +} + +func TestSubnetEgressMatch_SkipsInvalidExcludes(t *testing.T) { + got := subnetEgressMatch("subnet-pub", netip.MustParsePrefix("0.0.0.0/0"), []netip.Prefix{{}}) + assert.NotContains(t, got, "ip4.dst !=") +} + +func TestRouteManager_AddSubnetEgress_PersistsExcludeInMatch(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + excludes := []netip.Prefix{netip.MustParsePrefix("10.0.0.0/16")} + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityNATGW, + ExcludeCIDRs: excludes, + })) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Contains(t, policies[0].Match, "ip4.dst != 10.0.0.0/16") +} + +func TestRouteManager_AddSubnetEgressDrop_InstallsDropPolicy(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", SubnetEgressDropSpec{ + SubnetID: "subnet-pub", + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + })) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + p := policies[0] + assert.Equal(t, "drop", p.Action) + assert.Equal(t, SubnetEgressPriorityDrop, p.Priority) + assert.Nil(t, p.Nexthop, "drop policy must not carry a nexthop") + assert.Contains(t, p.Match, topology.SubnetRouterPort("subnet-pub")) + assert.Contains(t, p.Match, "ip4.dst == 0.0.0.0/0") + assert.Equal(t, "subnet-pub", p.ExternalIDs["spinifex:subnet"]) + assert.Equal(t, "drop", p.ExternalIDs["spinifex:gate"]) +} + +func TestRouteManager_AddSubnetEgressDrop_IsIdempotent(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + spec := SubnetEgressDropSpec{SubnetID: "subnet-pub", Prefix: netip.MustParsePrefix("0.0.0.0/0")} + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", spec)) + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", spec)) + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", spec)) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Len(t, policies, 1) +} + +func TestRouteManager_AddSubnetEgressDrop_PersistsExcludesInMatch(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + excludes := []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/16"), + netip.MustParsePrefix("169.254.0.0/16"), + netip.MustParsePrefix("224.0.0.0/4"), + } + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", SubnetEgressDropSpec{ + SubnetID: "subnet-pub", + Prefix: netip.MustParsePrefix("0.0.0.0/0"), + ExcludeCIDRs: excludes, + })) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Contains(t, policies[0].Match, "ip4.dst != 10.0.0.0/16") + assert.Contains(t, policies[0].Match, "ip4.dst != 169.254.0.0/16") + assert.Contains(t, policies[0].Match, "ip4.dst != 224.0.0.0/4") +} + +func TestRouteManager_AddSubnetEgressDrop_RejectsMissingFields(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + t.Run("missing SubnetID", func(t *testing.T) { + err := rm.AddSubnetEgressDrop(ctx, "vpc-1", SubnetEgressDropSpec{Prefix: netip.MustParsePrefix("0.0.0.0/0")}) + require.Error(t, err) + }) + t.Run("invalid Prefix", func(t *testing.T) { + err := rm.AddSubnetEgressDrop(ctx, "vpc-1", SubnetEgressDropSpec{SubnetID: "subnet-pub"}) + require.Error(t, err) + }) +} + +func TestRouteManager_AddSubnetEgressDrop_ReplacesPolicyOnActionDrift(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + router := topology.VPCRouter("vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("0.0.0.0/0") + match := subnetEgressMatch("subnet-pub", prefix, nil) + nexthop := "10.0.0.1" + + stale := &nbdb.LogicalRouterPolicy{ + UUID: "drift-uuid", + Priority: SubnetEgressPriorityDrop, + Match: match, + Action: "reroute", + Nexthop: &nexthop, + } + m.LRPolicies[stale.UUID] = stale + m.Routers[router].Policies = append(m.Routers[router].Policies, stale.UUID) + + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", SubnetEgressDropSpec{ + SubnetID: "subnet-pub", + Prefix: prefix, + })) + + policies, err := m.ListLogicalRouterPolicies(ctx, router) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Equal(t, "drop", policies[0].Action) + assert.Nil(t, policies[0].Nexthop) +} + +func TestRouteManager_DeleteSubnetEgressDrop_RemovesPolicy(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("0.0.0.0/0") + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", SubnetEgressDropSpec{ + SubnetID: "subnet-pub", + Prefix: prefix, + })) + require.NoError(t, rm.DeleteSubnetEgressDrop(ctx, "vpc-1", "subnet-pub", prefix, nil)) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Empty(t, policies) +} + +func TestRouteManager_DeleteSubnetEgressDrop_IdempotentOnMissing(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + require.NoError(t, rm.DeleteSubnetEgressDrop(ctx, "vpc-1", "subnet-pub", netip.MustParsePrefix("0.0.0.0/0"), nil)) +} + +func TestRouteManager_DeleteSubnetEgressDrop_MatchesAddedExcludes(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("0.0.0.0/0") + excludes := []netip.Prefix{netip.MustParsePrefix("10.0.0.0/16")} + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", SubnetEgressDropSpec{ + SubnetID: "subnet-pub", + Prefix: prefix, + ExcludeCIDRs: excludes, + })) + require.NoError(t, rm.DeleteSubnetEgressDrop(ctx, "vpc-1", "subnet-pub", prefix, excludes)) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Empty(t, policies) +} + +func TestRouteManager_DropAndRerouteCoexistAtDifferentPriorities(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("0.0.0.0/0") + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", SubnetEgressSpec{ + SubnetID: "subnet-a", + Prefix: prefix, + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityIGW, + })) + require.NoError(t, rm.AddSubnetEgressDrop(ctx, "vpc-1", SubnetEgressDropSpec{ + SubnetID: "subnet-b", + Prefix: prefix, + })) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Len(t, policies, 2, "different subnets can hold different action policies on the same VPC router") +} + +func TestRouteManager_DeleteSubnetEgress_MatchesAddedExcludes(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + rm := NewRouteManager(m) + + prefix := netip.MustParsePrefix("0.0.0.0/0") + excludes := []netip.Prefix{netip.MustParsePrefix("10.0.0.0/16")} + require.NoError(t, rm.AddSubnetEgress(ctx, "vpc-1", SubnetEgressSpec{ + SubnetID: "subnet-pub", + Prefix: prefix, + Nexthop: "192.168.1.1", + OutputPort: "gw-vpc-1", + Priority: SubnetEgressPriorityNATGW, + ExcludeCIDRs: excludes, + })) + require.NoError(t, rm.DeleteSubnetEgress(ctx, "vpc-1", "subnet-pub", prefix, excludes)) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Empty(t, policies, "delete must match same predicate as add when excludes are present") +} diff --git a/spinifex/network/policy/sg.go b/spinifex/network/policy/sg.go index 962593825..9f3b0fb05 100644 --- a/spinifex/network/policy/sg.go +++ b/spinifex/network/policy/sg.go @@ -18,8 +18,8 @@ type SGSpec struct { } // SecurityGroupManager attaches ACL policy to an SG's port group. EnsureSG -// and UpdateSG share replace-all semantics: each call clears then re-adds -// the full ACL set (infra + tenant) atomically per AddACLs transaction. +// and UpdateSG share replace-all semantics: each call replaces the full ACL +// set (infra + tenant) in one OVSDB transaction. type SecurityGroupManager interface { // EnsureSG sets the full ACL set; idempotent. EnsureSG(ctx context.Context, sg SGSpec) error @@ -60,17 +60,16 @@ func (m *sgManager) DeleteSG(ctx context.Context, groupID string) error { return nil } -// applyACLs clears the PG and re-adds infra + tenant ACLs. ClearACLs + -// AddACLs is not one OVSDB transaction; the observable gap is bounded by -// southbound flow-install latency. +// applyACLs atomically replaces the PG's ACL set with infra + tenant ACLs in +// a single OVSDB transaction. The previous ClearACLs+AddACLs split left the +// port group with zero ACLs between transactions, defaulting tenant LSP +// traffic to drop on connectionless flows (ICMP) and on TCP SYNs that +// don't match an existing conntrack entry. func (m *sgManager) applyACLs(ctx context.Context, sg SGSpec) error { pg := topology.SecurityGroupPortGroup(sg.GroupID) - if err := m.ovn.ClearACLs(ctx, pg); err != nil { - return fmt.Errorf("clear ACLs on %s: %w", pg, err) - } specs := append(InfrastructureACLs(pg), RuleACLSpecs(pg, sg.IngressRules, sg.EgressRules)...) - if err := m.ovn.AddACLs(ctx, pg, specs); err != nil { - return fmt.Errorf("add ACLs on %s: %w", pg, err) + if err := m.ovn.ReplaceACLs(ctx, pg, specs); err != nil { + return fmt.Errorf("replace ACLs on %s: %w", pg, err) } return nil } diff --git a/spinifex/network/policy/sg_test.go b/spinifex/network/policy/sg_test.go index 0f3595f65..58fbc0913 100644 --- a/spinifex/network/policy/sg_test.go +++ b/spinifex/network/policy/sg_test.go @@ -95,6 +95,31 @@ func TestSGManager_EnsureSG_MissingPortGroup_ReturnsError(t *testing.T) { assert.Contains(t, err.Error(), "port group") } +// TestSGManager_UpdateSG_AtomicReplace asserts EnsureSG / UpdateSG route +// through ovn.Client.ReplaceACLs (single OVSDB transaction) and never call +// ClearACLs. ClearACLs+AddACLs leaves the port group with zero ACLs between +// transactions, defaulting tenant LSP traffic to drop on connectionless +// flows (ICMP) and on TCP SYNs that miss conntrack — the symptom behind +// TestSecurityGroupEgress flakes. +func TestSGManager_UpdateSG_AtomicReplace(t *testing.T) { + ctx := context.Background() + m, pg := newSGMockWithPG(t, "sg-web") + sg := NewSecurityGroupManager(m) + + require.NoError(t, sg.EnsureSG(ctx, SGSpec{ + GroupID: "sg-web", + IngressRules: []Rule{{IPProtocol: "tcp", FromPort: 22, ToPort: 22, CIDR: "10.0.0.0/8"}}, + })) + require.NoError(t, sg.UpdateSG(ctx, SGSpec{ + GroupID: "sg-web", + IngressRules: []Rule{{IPProtocol: "tcp", FromPort: 80, ToPort: 80, CIDR: "0.0.0.0/0"}}, + })) + + assert.Equal(t, 2, m.ReplaceACLCalls, "EnsureSG+UpdateSG must call ReplaceACLs") + assert.Zero(t, m.ClearACLCalls, "EnsureSG/UpdateSG must NOT call ClearACLs") + assert.NotEmpty(t, m.PortGroups[pg].ACLs, "PG ACLs must never be empty after UpdateSG") +} + func TestSGManager_EnsureSG_Idempotent(t *testing.T) { ctx := context.Background() m, pg := newSGMockWithPG(t, "sg-web") diff --git a/spinifex/network/reconcile/apply.go b/spinifex/network/reconcile/apply.go index 63fce1a94..ff904e45f 100644 --- a/spinifex/network/reconcile/apply.go +++ b/spinifex/network/reconcile/apply.go @@ -269,6 +269,42 @@ func (r *reconciler) applyNATGWs(ctx context.Context, intent IntentState, _ Actu } } +// applyIGWRoutes installs per-subnet egress reroute policies for every IGW +// route in intent. Closes the bootstrap race where the publisher fires +// vpc.add-igw-route before subscribers attach: events vanish, KV retains the +// route, this pass re-derives and installs. +func (r *reconciler) applyIGWRoutes(ctx context.Context, intent IntentState, _ ActualState) { + for _, spec := range intent.IGWRoutes { + if err := r.igw.EnsureSubnetEgress(ctx, spec.VPCID, spec.SubnetID, spec.DestCIDR); err != nil { + slog.Error("reconcile/apply: EnsureSubnetEgress failed", + "vpc_id", spec.VPCID, "subnet_id", spec.SubnetID, "cidr", spec.DestCIDR.String(), "err", err) + } + } +} + +// applyNATGWRoutes is the NATGW priority sibling of applyIGWRoutes. +func (r *reconciler) applyNATGWRoutes(ctx context.Context, intent IntentState, _ ActualState) { + for _, spec := range intent.NATGWRoutes { + if err := r.igw.EnsureNATGatewaySubnetEgress(ctx, spec.VPCID, spec.SubnetID, spec.DestCIDR); err != nil { + slog.Error("reconcile/apply: EnsureNATGatewaySubnetEgress failed", + "vpc_id", spec.VPCID, "subnet_id", spec.SubnetID, "cidr", spec.DestCIDR.String(), "err", err) + } + } +} + +// applyDropGates installs a DROP policy at SubnetEgressPriorityDrop for every +// subnet whose VPC has an attached IGW but whose effective route table lacks +// a 0.0.0.0/0 IGW/NATGW entry. Without this, the VPC LR's router-wide default +// static route would let the subnet egress. +func (r *reconciler) applyDropGates(ctx context.Context, intent IntentState, _ ActualState) { + for _, spec := range intent.DropGates { + if err := r.igw.EnsureSubnetEgressDrop(ctx, spec.VPCID, spec.SubnetID, spec.DestCIDR); err != nil { + slog.Error("reconcile/apply: EnsureSubnetEgressDrop failed", + "vpc_id", spec.VPCID, "subnet_id", spec.SubnetID, "cidr", spec.DestCIDR.String(), "err", err) + } + } +} + // diffSets returns (desired - current, current - desired). func diffSets(desired, current []string) (add, remove []string) { desiredSet := make(map[string]struct{}, len(desired)) diff --git a/spinifex/network/reconcile/apply_test.go b/spinifex/network/reconcile/apply_test.go index ee7776116..a525b2bce 100644 --- a/spinifex/network/reconcile/apply_test.go +++ b/spinifex/network/reconcile/apply_test.go @@ -365,9 +365,12 @@ func freshIntent(t *testing.T) IntentState { 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{}, + IGWs: map[string]external.IGWSpec{}, + EIPs: map[string]policy.EIPSpec{}, + NATGWs: map[string]policy.NATGWSpec{}, + IGWRoutes: map[string]SubnetEgressIntent{}, + NATGWRoutes: map[string]SubnetEgressIntent{}, + DropGates: map[string]SubnetEgressIntent{}, } } diff --git a/spinifex/network/reconcile/intent.go b/spinifex/network/reconcile/intent.go index feca8b513..bfbccb3d3 100644 --- a/spinifex/network/reconcile/intent.go +++ b/spinifex/network/reconcile/intent.go @@ -25,13 +25,30 @@ import ( // IntentState is the desired OVN state derived from the JetStream KV // snapshot, scoped to the local AZ. Empty maps are valid. type IntentState struct { - 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 // attached only - EIPs map[string]policy.EIPSpec // keyed by logicalIP; associated only - NATGWs map[string]policy.NATGWSpec + 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 // attached only + EIPs map[string]policy.EIPSpec // keyed by logicalIP; associated only + NATGWs map[string]policy.NATGWSpec + IGWRoutes map[string]SubnetEgressIntent // per (subnet, dest) IGW egress reroute + NATGWRoutes map[string]SubnetEgressIntent // per (subnet, dest) NATGW egress reroute + DropGates map[string]SubnetEgressIntent // per (subnet, dest=0.0.0.0/0) drop policy +} + +// SubnetEgressIntent is a per-subnet default-route policy installed on the +// VPC LR. Mirrors the publishIGWRouteEvent fan-out: one entry per (subnet, +// destCIDR) that the source route table directs at an IGW or NAT gateway. +type SubnetEgressIntent struct { + VPCID string + SubnetID string + DestCIDR netip.Prefix +} + +// subnetEgressKey is the IGWRoutes/NATGWRoutes map key. +func subnetEgressKey(subnetID string, prefix netip.Prefix) string { + return subnetID + "|" + prefix.String() } // LoadIntentFromKV assembles IntentState scoped to localAZ. Missing buckets @@ -39,13 +56,16 @@ type IntentState struct { // localAZ`. Children inherit the filter transitively via their parent VPC. 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), + 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), + IGWRoutes: make(map[string]SubnetEgressIntent), + NATGWRoutes: make(map[string]SubnetEgressIntent), + DropGates: make(map[string]SubnetEgressIntent), } localVPCs, err := loadVPCs(js, localAZ, intent.VPCs) @@ -74,11 +94,131 @@ func LoadIntentFromKV(ctx context.Context, js nats.JetStreamContext, localAZ str if err := loadNATGWs(js, localVPCs, intent.Subnets, routeTables, intent.NATGWs); err != nil { return IntentState{}, err } + loadSubnetEgressRoutes(localVPCs, intent.Subnets, routeTables, intent.IGWRoutes, intent.NATGWRoutes) + loadSubnetDropGates(localVPCs, intent.Subnets, intent.IGWs, intent.IGWRoutes, intent.NATGWRoutes, intent.DropGates) _ = ctx return intent, nil } +// loadSubnetEgressRoutes fans IGW/NATGW routes out over the subnets that +// inherit them — explicit non-main associations plus, for the main RT, +// subnets in the same VPC with no explicit non-main association. Mirrors +// publishIGWRouteEvents/publishNatGatewayEvents so the reconcile pass +// reaches the same per-subnet set as the runtime event path. +func loadSubnetEgressRoutes( + localVPCs map[string]struct{}, + subnets map[string]topology.SubnetSpec, + routeTables []handlers_ec2_routetable.RouteTableRecord, + igwOut, natgwOut map[string]SubnetEgressIntent, +) { + subnetsByVPC := make(map[string][]string, len(localVPCs)) + for id, spec := range subnets { + subnetsByVPC[spec.VPCID] = append(subnetsByVPC[spec.VPCID], id) + } + + explicitByVPC := make(map[string]map[string]struct{}, len(routeTables)) + for _, rt := range routeTables { + if _, ok := localVPCs[rt.VpcId]; !ok { + continue + } + ex, ok := explicitByVPC[rt.VpcId] + if !ok { + ex = map[string]struct{}{} + explicitByVPC[rt.VpcId] = ex + } + for _, assoc := range rt.Associations { + if assoc.SubnetId == "" || assoc.Main { + continue + } + ex[assoc.SubnetId] = struct{}{} + } + } + + for _, rt := range routeTables { + if _, ok := localVPCs[rt.VpcId]; !ok { + continue + } + targets := map[string]struct{}{} + for _, assoc := range rt.Associations { + if assoc.SubnetId == "" || assoc.Main { + continue + } + targets[assoc.SubnetId] = struct{}{} + } + if rt.IsMain { + for _, subnetID := range subnetsByVPC[rt.VpcId] { + if _, ok := explicitByVPC[rt.VpcId][subnetID]; ok { + continue + } + targets[subnetID] = struct{}{} + } + } + for _, r := range rt.Routes { + if r.State != "" && !strings.EqualFold(r.State, "active") { + continue + } + prefix, err := netip.ParsePrefix(r.DestinationCidrBlock) + if err != nil { + slog.Warn("reconcile/intent: route CIDR parse failed", + "route_table_id", rt.RouteTableId, "cidr", r.DestinationCidrBlock, "err", err) + continue + } + var sink map[string]SubnetEgressIntent + switch { + case strings.HasPrefix(r.GatewayId, "igw-"): + sink = igwOut + case r.NatGatewayId != "": + sink = natgwOut + default: + continue + } + for subnetID := range targets { + sink[subnetEgressKey(subnetID, prefix)] = SubnetEgressIntent{ + VPCID: rt.VpcId, + SubnetID: subnetID, + DestCIDR: prefix, + } + } + } + } +} + +// loadSubnetDropGates emits one drop intent per subnet whose VPC has an +// attached IGW but which has no 0.0.0.0/0 reroute path (neither IGWRoutes +// nor NATGWRoutes carry an entry for it). VPCs without an attached IGW +// have no router-wide default static route, so lr_in_ip_routing already +// kills the packet — no drop policy needed. +func loadSubnetDropGates( + localVPCs map[string]struct{}, + subnets map[string]topology.SubnetSpec, + igws map[string]external.IGWSpec, + igwRoutes, natgwRoutes map[string]SubnetEgressIntent, + out map[string]SubnetEgressIntent, +) { + defaultPrefix := netip.MustParsePrefix("0.0.0.0/0") + for _, subnet := range subnets { + if _, ok := localVPCs[subnet.VPCID]; !ok { + continue + } + if _, ok := igws[subnet.VPCID]; !ok { + continue + } + key := subnetEgressKey(subnet.SubnetID, defaultPrefix) + if _, ok := igwRoutes[key]; ok { + continue + } + if _, ok := natgwRoutes[key]; ok { + continue + } + out[key] = SubnetEgressIntent{ + VPCID: subnet.VPCID, + SubnetID: subnet.SubnetID, + DestCIDR: defaultPrefix, + } + } +} + // matchesLocalAZ enforces §11.1: empty AZ counts as local (legacy records). func matchesLocalAZ(vpcAZ, localAZ string) bool { return vpcAZ == "" || vpcAZ == localAZ diff --git a/spinifex/network/reconcile/intent_test.go b/spinifex/network/reconcile/intent_test.go index 1431cb28a..e208c9290 100644 --- a/spinifex/network/reconcile/intent_test.go +++ b/spinifex/network/reconcile/intent_test.go @@ -3,6 +3,7 @@ package reconcile import ( "context" "encoding/json" + "net/netip" "testing" "time" @@ -254,11 +255,151 @@ func TestLoadIntentFromKV_NoBucketsTolerated(t *testing.T) { 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 { + if len(intent.VPCs)+len(intent.Subnets)+len(intent.Ports)+len(intent.SGs)+len(intent.IGWs)+len(intent.EIPs)+len(intent.NATGWs)+len(intent.IGWRoutes)+len(intent.NATGWRoutes) != 0 { t.Errorf("expected empty intent on fresh cluster, got %#v", intent) } } +// TestLoadIntentFromKV_IGWRoutesFanOutMainRT covers the bootstrap race: +// publisher fires vpc.add-igw-route before subscribers attach, the event +// is dropped, and KV is the only source of truth left. The reconcile pass +// must rediscover the per-subnet egress intent from the main RT and fan it +// out to every subnet (including implicit-main subnets with no explicit +// non-main association). +func TestLoadIntentFromKV_IGWRoutesFanOutMainRT(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: "172.31.0.0/16", AZ: "us-east-1a", + }), + }) + testutil.SeedKV(t, js, handlers_ec2_vpc.KVBucketSubnets, map[string][]byte{ + "acct/subnet-implicit": mustJSON(t, handlers_ec2_vpc.SubnetRecord{ + SubnetId: "subnet-implicit", VpcId: "vpc-local", CidrBlock: "172.31.0.0/20", + }), + "acct/subnet-explicit": mustJSON(t, handlers_ec2_vpc.SubnetRecord{ + SubnetId: "subnet-explicit", VpcId: "vpc-local", CidrBlock: "172.31.16.0/20", + }), + }) + testutil.SeedKV(t, js, handlers_ec2_routetable.KVBucketRouteTables, map[string][]byte{ + "acct/rtb-main": mustJSON(t, handlers_ec2_routetable.RouteTableRecord{ + RouteTableId: "rtb-main", VpcId: "vpc-local", IsMain: true, + Routes: []handlers_ec2_routetable.RouteRecord{ + {DestinationCidrBlock: "0.0.0.0/0", GatewayId: "igw-1", State: "active"}, + }, + }), + "acct/rtb-explicit": mustJSON(t, handlers_ec2_routetable.RouteTableRecord{ + RouteTableId: "rtb-explicit", VpcId: "vpc-local", + Associations: []handlers_ec2_routetable.AssociationRecord{ + {AssociationId: "rtbassoc-x", SubnetId: "subnet-explicit"}, + }, + }), + }) + + intent, err := LoadIntentFromKV(context.Background(), js, "us-east-1a") + if err != nil { + t.Fatalf("LoadIntentFromKV: %v", err) + } + + if len(intent.IGWRoutes) != 1 { + t.Fatalf("got %d IGWRoutes, want 1; routes=%#v", len(intent.IGWRoutes), intent.IGWRoutes) + } + for _, spec := range intent.IGWRoutes { + if spec.SubnetID != "subnet-implicit" { + t.Errorf("SubnetID=%q, want %q (explicit subnet is on rtb-explicit which has no IGW route)", + spec.SubnetID, "subnet-implicit") + } + if spec.DestCIDR.String() != "0.0.0.0/0" { + t.Errorf("DestCIDR=%q, want 0.0.0.0/0", spec.DestCIDR.String()) + } + } +} + +// TestLoadIntentFromKV_DropGatesForUnroutedSubnetWithIGW: a subnet whose +// effective RT has no 0.0.0.0/0 entry, in a VPC with an attached IGW, must +// surface a DropGates intent so the reconciler installs the per-subnet +// drop policy that gates the VPC LR's router-wide default static route. +func TestLoadIntentFromKV_DropGatesForUnroutedSubnetWithIGW(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: "172.31.0.0/16", AZ: "us-east-1a", + }), + }) + testutil.SeedKV(t, js, handlers_ec2_vpc.KVBucketSubnets, map[string][]byte{ + "acct/subnet-routed": mustJSON(t, handlers_ec2_vpc.SubnetRecord{ + SubnetId: "subnet-routed", VpcId: "vpc-local", CidrBlock: "172.31.0.0/20", + }), + "acct/subnet-isolated": mustJSON(t, handlers_ec2_vpc.SubnetRecord{ + SubnetId: "subnet-isolated", VpcId: "vpc-local", CidrBlock: "172.31.16.0/20", + }), + }) + testutil.SeedKV(t, js, handlers_ec2_igw.KVBucketIGW, map[string][]byte{ + "acct/igw-1": mustJSON(t, handlers_ec2_igw.IGWRecord{ + InternetGatewayId: "igw-1", VpcId: "vpc-local", State: "available", + }), + }) + testutil.SeedKV(t, js, handlers_ec2_routetable.KVBucketRouteTables, map[string][]byte{ + "acct/rtb-main": mustJSON(t, handlers_ec2_routetable.RouteTableRecord{ + RouteTableId: "rtb-main", VpcId: "vpc-local", IsMain: true, + Associations: []handlers_ec2_routetable.AssociationRecord{ + {AssociationId: "rtbassoc-r", SubnetId: "subnet-routed"}, + }, + Routes: []handlers_ec2_routetable.RouteRecord{ + {DestinationCidrBlock: "0.0.0.0/0", GatewayId: "igw-1", State: "active"}, + }, + }), + "acct/rtb-isolated": mustJSON(t, handlers_ec2_routetable.RouteTableRecord{ + RouteTableId: "rtb-isolated", VpcId: "vpc-local", + Associations: []handlers_ec2_routetable.AssociationRecord{ + {AssociationId: "rtbassoc-i", SubnetId: "subnet-isolated"}, + }, + }), + }) + + intent, err := LoadIntentFromKV(context.Background(), js, "us-east-1a") + if err != nil { + t.Fatalf("LoadIntentFromKV: %v", err) + } + if _, ok := intent.IGWRoutes[subnetEgressKey("subnet-routed", netip.MustParsePrefix("0.0.0.0/0"))]; !ok { + t.Errorf("subnet-routed missing IGW egress intent") + } + if _, ok := intent.DropGates[subnetEgressKey("subnet-routed", netip.MustParsePrefix("0.0.0.0/0"))]; ok { + t.Errorf("subnet-routed must not have a drop gate (it has an IGW route)") + } + if _, ok := intent.DropGates[subnetEgressKey("subnet-isolated", netip.MustParsePrefix("0.0.0.0/0"))]; !ok { + t.Errorf("subnet-isolated missing drop gate intent (no 0.0.0.0/0 in its RT, IGW is attached)") + } +} + +// TestLoadIntentFromKV_NoDropGateWithoutIGW: VPC without an attached IGW +// has no router-wide default static route, so unrouted subnets are already +// dropped by lr_in_ip_routing priority-0 — no drop policy needed. +func TestLoadIntentFromKV_NoDropGateWithoutIGW(t *testing.T) { + _, _, js := testutil.StartTestJetStream(t) + + testutil.SeedKV(t, js, handlers_ec2_vpc.KVBucketVPCs, map[string][]byte{ + "acct/vpc-air-gapped": mustJSON(t, handlers_ec2_vpc.VPCRecord{ + VpcId: "vpc-air-gapped", CidrBlock: "10.99.0.0/16", AZ: "us-east-1a", + }), + }) + 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-air-gapped", CidrBlock: "10.99.1.0/24", + }), + }) + + intent, err := LoadIntentFromKV(context.Background(), js, "us-east-1a") + if err != nil { + t.Fatalf("LoadIntentFromKV: %v", err) + } + if len(intent.DropGates) != 0 { + t.Errorf("VPC has no IGW: expected 0 drop gates, got %d (%v)", len(intent.DropGates), intent.DropGates) + } +} + func mustJSON(t *testing.T, v any) []byte { t.Helper() b, err := json.Marshal(v) diff --git a/spinifex/network/reconcile/reconcile.go b/spinifex/network/reconcile/reconcile.go index 33f4745cc..02946b490 100644 --- a/spinifex/network/reconcile/reconcile.go +++ b/spinifex/network/reconcile/reconcile.go @@ -129,6 +129,8 @@ func (r *reconciler) reconcile(ctx context.Context, intent IntentState, pruneOrp "intent_igws", len(intent.IGWs), "intent_eips", len(intent.EIPs), "intent_natgws", len(intent.NATGWs), + "intent_igw_routes", len(intent.IGWRoutes), + "intent_natgw_routes", len(intent.NATGWRoutes), ) r.applyVPCs(ctx, intent, actual) @@ -138,6 +140,9 @@ func (r *reconciler) reconcile(ctx context.Context, intent IntentState, pruneOrp r.applyIGWs(ctx, intent, actual) r.applyEIPs(ctx, intent, actual) r.applyNATGWs(ctx, intent, actual) + r.applyIGWRoutes(ctx, intent, actual) + r.applyNATGWRoutes(ctx, intent, actual) + r.applyDropGates(ctx, intent, actual) slog.Info("reconcile: complete") return nil diff --git a/spinifex/network/subscribers/events.go b/spinifex/network/subscribers/events.go index e5add8d4c..de3bb0442 100644 --- a/spinifex/network/subscribers/events.go +++ b/spinifex/network/subscribers/events.go @@ -2,22 +2,26 @@ package subscribers // NATS topic names for VPC lifecycle events published by daemon. const ( - TopicVPCCreate = "vpc.create" - TopicVPCDelete = "vpc.delete" - TopicSubnetCreate = "vpc.create-subnet" - TopicSubnetDelete = "vpc.delete-subnet" - TopicCreatePort = "vpc.create-port" - TopicDeletePort = "vpc.delete-port" - TopicUpdatePortSGs = "vpc.update-port-sgs" - TopicIGWAttach = "vpc.igw-attach" - TopicIGWDetach = "vpc.igw-detach" - TopicAddNAT = "vpc.add-nat" - TopicDeleteNAT = "vpc.delete-nat" - TopicAddNATGateway = "vpc.add-nat-gateway" - TopicDeleteNATGateway = "vpc.delete-nat-gateway" - TopicCreateSG = "vpc.create-sg" - TopicDeleteSG = "vpc.delete-sg" - TopicUpdateSG = "vpc.update-sg" + TopicVPCCreate = "vpc.create" + TopicVPCDelete = "vpc.delete" + TopicSubnetCreate = "vpc.create-subnet" + TopicSubnetDelete = "vpc.delete-subnet" + TopicCreatePort = "vpc.create-port" + TopicDeletePort = "vpc.delete-port" + TopicUpdatePortSGs = "vpc.update-port-sgs" + TopicIGWAttach = "vpc.igw-attach" + TopicIGWDetach = "vpc.igw-detach" + TopicAddNAT = "vpc.add-nat" + TopicDeleteNAT = "vpc.delete-nat" + TopicAddNATGateway = "vpc.add-nat-gateway" + TopicDeleteNATGateway = "vpc.delete-nat-gateway" + TopicAddIGWRoute = "vpc.add-igw-route" + TopicDeleteIGWRoute = "vpc.delete-igw-route" + TopicGateSubnetEgress = "vpc.gate-subnet-egress" + TopicUngateSubnetEgress = "vpc.ungate-subnet-egress" + TopicCreateSG = "vpc.create-sg" + TopicDeleteSG = "vpc.delete-sg" + TopicUpdateSG = "vpc.update-sg" ) // VPCEvent is published on vpc.create / vpc.delete. @@ -64,11 +68,49 @@ type NATEvent struct { } // NATGatewayEvent is published on vpc.add-nat-gateway / vpc.delete-nat-gateway. +// SubnetId + DestinationCidr identify the per-subnet egress policy installed +// alongside the SNAT rule so the LR has a default route for the private +// subnet (priority SubnetEgressPriorityNATGW). Without that policy, SNAT +// rewrites the source IP but the packet has no route to leave the LR. type NATGatewayEvent struct { - VpcId string `json:"vpc_id"` - NatGatewayId string `json:"nat_gateway_id"` - PublicIp string `json:"public_ip"` - SubnetCidr string `json:"subnet_cidr"` // private subnet CIDR for SNAT rule + VpcId string `json:"vpc_id"` + NatGatewayId string `json:"nat_gateway_id"` + PublicIp string `json:"public_ip"` + SubnetCidr string `json:"subnet_cidr"` // private subnet CIDR for SNAT logical_ip + SubnetId string `json:"subnet_id"` // private subnet ID for inport policy match + DestinationCidr string `json:"destination_cidr"` // route destination, typically 0.0.0.0/0 +} + +// IGWRouteEvent is published on vpc.add-igw-route / vpc.delete-igw-route +// when a route table associated with SubnetId carries (or loses) a route +// to InternetGatewayId for DestinationCidr. The subscriber installs (or +// removes) an OVN Logical_Router_Policy scoped to the subnet's inport. +type IGWRouteEvent struct { + VpcId string `json:"vpc_id"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` + InternetGatewayId string `json:"internet_gateway_id"` +} + +// SubnetEgressGateEvent is published on vpc.gate-subnet-egress when the +// effective route table of SubnetId loses (or never had) a DestinationCidr +// entry pointing at any egress target (IGW / NATGW). The subscriber installs +// an OVN Logical_Router_Policy DROP at SubnetEgressPriorityDrop so the +// subnet cannot reach external destinations via the VPC LR's router-wide +// default static route. +type SubnetEgressGateEvent struct { + VpcId string `json:"vpc_id"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` +} + +// SubnetEgressUngateEvent is published on vpc.ungate-subnet-egress when a +// previously-gated subnet acquires an egress target in its effective route +// table. The subscriber removes the DROP policy installed by the gate event. +type SubnetEgressUngateEvent struct { + VpcId string `json:"vpc_id"` + SubnetId string `json:"subnet_id"` + DestinationCidr string `json:"destination_cidr"` } // SGRule mirrors handlers/ec2/vpc.SGRule on the wire (kept local to avoid diff --git a/spinifex/network/subscribers/subscribers.go b/spinifex/network/subscribers/subscribers.go index be5089a39..fc3368bc5 100644 --- a/spinifex/network/subscribers/subscribers.go +++ b/spinifex/network/subscribers/subscribers.go @@ -78,6 +78,10 @@ func (s *Subscriber) Subscribe(nc *nats.Conn) ([]*nats.Subscription, error) { {TopicDeleteNAT, s.handleDeleteNAT}, {TopicAddNATGateway, s.handleAddNATGateway}, {TopicDeleteNATGateway, s.handleDeleteNATGateway}, + {TopicAddIGWRoute, s.handleAddIGWRoute}, + {TopicDeleteIGWRoute, s.handleDeleteIGWRoute}, + {TopicGateSubnetEgress, s.handleGateSubnetEgress}, + {TopicUngateSubnetEgress, s.handleUngateSubnetEgress}, {TopicCreateSG, s.handleCreateSG}, {TopicDeleteSG, s.handleDeleteSG}, {TopicUpdateSG, s.handleUpdateSG}, diff --git a/spinifex/network/subscribers/subscribers_test.go b/spinifex/network/subscribers/subscribers_test.go index 595553c82..1d2f3c106 100644 --- a/spinifex/network/subscribers/subscribers_test.go +++ b/spinifex/network/subscribers/subscribers_test.go @@ -6,8 +6,12 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/mulgadc/spinifex/spinifex/network/external" "github.com/mulgadc/spinifex/spinifex/network/ovn/mock" + "github.com/mulgadc/spinifex/spinifex/network/ovn/nbdb" "github.com/mulgadc/spinifex/spinifex/network/policy" "github.com/mulgadc/spinifex/spinifex/network/topology" "github.com/mulgadc/spinifex/spinifex/testutil" @@ -92,6 +96,8 @@ func TestSubscribe_RegistersAllTopics(t *testing.T) { TopicIGWAttach, TopicIGWDetach, TopicAddNAT, TopicDeleteNAT, TopicAddNATGateway, TopicDeleteNATGateway, + TopicAddIGWRoute, TopicDeleteIGWRoute, + TopicGateSubnetEgress, TopicUngateSubnetEgress, TopicCreateSG, TopicDeleteSG, TopicUpdateSG, } if len(subs) != len(wantTopics) { @@ -108,6 +114,374 @@ func TestSubscribe_RegistersAllTopics(t *testing.T) { } } +// TestHandleAddNATGateway_InstallsPerSubnetEgress drives the regression: the +// subscriber must install a Logical_Router_Policy at NATGW priority alongside +// the SNAT rule, otherwise private-subnet packets have no route off the LR +// and NAT GW egress fails (Phase 8d). Verifies the policy is installed +// against the IGW's gateway port, with the inport scoped to the private +// subnet, after a vpc.add-nat-gateway event. +func TestHandleAddNATGateway_InstallsPerSubnetEgress(t *testing.T) { + ctx := context.Background() + _, nc := testutil.StartTestNATS(t) + sub, m := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + // Bootstrap router + IGW so the gateway port + wan nexthop exist. + require.NoError(t, m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ + Name: topology.VPCRouter("vpc-1"), + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-1"}, + })) + require.NoError(t, sub.igw.AttachIGW(ctx, external.IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + evt := NATGatewayEvent{ + VpcId: "vpc-1", + NatGatewayId: "nat-1", + PublicIp: "192.168.1.50", + SubnetCidr: "10.0.11.0/24", + SubnetId: "subnet-priv", + DestinationCidr: "0.0.0.0/0", + } + data, err := json.Marshal(evt) + require.NoError(t, err) + require.NoError(t, nc.Publish(TopicAddNATGateway, data)) + + require.Eventually(t, func() bool { + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + if err != nil || len(policies) != 1 { + return false + } + return policies[0].Priority == policy.SubnetEgressPriorityNATGW + }, 2*time.Second, 20*time.Millisecond, "NATGW egress policy must be installed") + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Contains(t, policies[0].Match, topology.SubnetRouterPort("subnet-priv")) + assert.Equal(t, topology.GatewayRouterPort("vpc-1"), policies[0].ExternalIDs["spinifex:output_port"]) + + // Delete event must remove the policy. + require.NoError(t, nc.Publish(TopicDeleteNATGateway, data)) + require.Eventually(t, func() bool { + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + return err == nil && len(policies) == 0 + }, 2*time.Second, 20*time.Millisecond, "NATGW egress policy must be removed on delete event") +} + +// TestHandleAddNATGateway_InvalidCIDRSkipsPolicy: the SNAT install must still +// happen, but a malformed DestinationCidr must not install any LR policy. +func TestHandleAddNATGateway_InvalidCIDRSkipsPolicy(t *testing.T) { + ctx := context.Background() + _, nc := testutil.StartTestNATS(t) + sub, m := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + require.NoError(t, m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ + Name: topology.VPCRouter("vpc-1"), + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-1"}, + })) + require.NoError(t, sub.igw.AttachIGW(ctx, external.IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + evt := NATGatewayEvent{ + VpcId: "vpc-1", + NatGatewayId: "nat-1", + PublicIp: "192.168.1.50", + SubnetCidr: "10.0.11.0/24", + SubnetId: "subnet-priv", + DestinationCidr: "not-a-cidr", + } + data, err := json.Marshal(evt) + require.NoError(t, err) + require.NoError(t, nc.Publish(TopicAddNATGateway, data)) + + // SNAT lands eventually. + require.Eventually(t, func() bool { + nat, err := m.FindNATByExternalIP(ctx, "snat", "192.168.1.50") + return err == nil && nat != nil + }, time.Second, 20*time.Millisecond) + + // No policy installed — invalid CIDR short-circuits before EnsureNATGatewaySubnetEgress. + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Empty(t, policies) +} + +// TestHandleDeleteNATGateway_InvalidCIDRStillDetaches: malformed CIDR on the +// delete event must not block the SNAT teardown. +func TestHandleDeleteNATGateway_InvalidCIDRStillDetaches(t *testing.T) { + ctx := context.Background() + _, nc := testutil.StartTestNATS(t) + sub, m := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + require.NoError(t, m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ + Name: topology.VPCRouter("vpc-1"), + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-1"}, + })) + require.NoError(t, sub.igw.AttachIGW(ctx, external.IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + addEvt := NATGatewayEvent{ + VpcId: "vpc-1", NatGatewayId: "nat-1", PublicIp: "192.168.1.50", + SubnetCidr: "10.0.11.0/24", SubnetId: "subnet-priv", DestinationCidr: "0.0.0.0/0", + } + addData, err := json.Marshal(addEvt) + require.NoError(t, err) + require.NoError(t, nc.Publish(TopicAddNATGateway, addData)) + require.Eventually(t, func() bool { + nat, err := m.FindNATByExternalIP(ctx, "snat", "192.168.1.50") + return err == nil && nat != nil + }, time.Second, 20*time.Millisecond) + + delEvt := addEvt + delEvt.DestinationCidr = "not-a-cidr" + delData, err := json.Marshal(delEvt) + require.NoError(t, err) + require.NoError(t, nc.Publish(TopicDeleteNATGateway, delData)) + + require.Eventually(t, func() bool { + nat, err := m.FindNATByExternalIP(ctx, "snat", "192.168.1.50") + return err == nil && nat == nil + }, time.Second, 20*time.Millisecond, "SNAT must be removed even when CIDR is invalid") +} + +// TestHandleAddNATGateway_NoSubnetIDSkipsPolicy: legacy events without a +// subnet (or AWS gateway types that don't bind to a specific subnet) must +// install the SNAT rule and short-circuit before any policy work. +func TestHandleAddNATGateway_NoSubnetIDSkipsPolicy(t *testing.T) { + ctx := context.Background() + _, nc := testutil.StartTestNATS(t) + sub, m := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + require.NoError(t, m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ + Name: topology.VPCRouter("vpc-1"), + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-1"}, + })) + require.NoError(t, sub.igw.AttachIGW(ctx, external.IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + evt := NATGatewayEvent{ + VpcId: "vpc-1", NatGatewayId: "nat-1", PublicIp: "192.168.1.50", + SubnetCidr: "10.0.11.0/24", // SubnetId omitted + } + data, err := json.Marshal(evt) + require.NoError(t, err) + require.NoError(t, nc.Publish(TopicAddNATGateway, data)) + + require.Eventually(t, func() bool { + nat, err := m.FindNATByExternalIP(ctx, "snat", "192.168.1.50") + return err == nil && nat != nil + }, time.Second, 20*time.Millisecond) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Empty(t, policies, "missing SubnetId must skip Logical_Router_Policy install") +} + +// TestHandleAddNATGateway_DefaultsDestinationCidr verifies the empty-CIDR +// fallback to 0.0.0.0/0 — older publishers don't set DestinationCidr. +func TestHandleAddNATGateway_DefaultsDestinationCidr(t *testing.T) { + ctx := context.Background() + _, nc := testutil.StartTestNATS(t) + sub, m := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + require.NoError(t, m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ + Name: topology.VPCRouter("vpc-1"), + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-1"}, + })) + require.NoError(t, sub.igw.AttachIGW(ctx, external.IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + evt := NATGatewayEvent{ + VpcId: "vpc-1", NatGatewayId: "nat-1", PublicIp: "192.168.1.50", + SubnetCidr: "10.0.11.0/24", SubnetId: "subnet-priv", // DestinationCidr empty + } + data, err := json.Marshal(evt) + require.NoError(t, err) + require.NoError(t, nc.Publish(TopicAddNATGateway, data)) + + require.Eventually(t, func() bool { + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + return err == nil && len(policies) == 1 + }, time.Second, 20*time.Millisecond) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Contains(t, policies[0].Match, "ip4.dst == 0.0.0.0/0") +} + +// TestHandleAddIGWRoute_InstallsPolicy: full end-to-end add-igw-route flow. +func TestHandleAddIGWRoute_InstallsPolicy(t *testing.T) { + ctx := context.Background() + _, nc := testutil.StartTestNATS(t) + sub, m := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + require.NoError(t, m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ + Name: topology.VPCRouter("vpc-1"), + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-1"}, + })) + require.NoError(t, sub.igw.AttachIGW(ctx, external.IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + evt := IGWRouteEvent{ + VpcId: "vpc-1", SubnetId: "subnet-pub", DestinationCidr: "0.0.0.0/0", InternetGatewayId: "igw-1", + } + data, err := json.Marshal(evt) + require.NoError(t, err) + resp, err := nc.Request(TopicAddIGWRoute, data, requestTimeout) + require.NoError(t, err) + var env respondResponse + require.NoError(t, json.Unmarshal(resp.Data, &env)) + assert.True(t, env.Success) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + require.Len(t, policies, 1) + assert.Equal(t, policy.SubnetEgressPriorityIGW, policies[0].Priority) +} + +// TestHandleDeleteNATGateway_MissingRouterStillReturns covers the failure +// path where the underlying router has been torn down already: the handler +// must log + return rather than panic. Exercises both the +// RemoveNATGatewaySubnetEgress error branch and the DetachNATGateway error +// branch, and the empty-DestinationCidr default of 0.0.0.0/0. +func TestHandleDeleteNATGateway_MissingRouterStillReturns(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + sub, _ := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + evt := NATGatewayEvent{ + VpcId: "vpc-gone", + NatGatewayId: "nat-1", + PublicIp: "192.168.1.50", + SubnetCidr: "10.0.11.0/24", + SubnetId: "subnet-priv", + // DestinationCidr intentionally empty → handler must default to 0.0.0.0/0. + } + data, err := json.Marshal(evt) + require.NoError(t, err) + require.NoError(t, nc.Publish(TopicDeleteNATGateway, data)) + time.Sleep(50 * time.Millisecond) +} + +// TestHandleNATGateway_BadJSON: malformed payloads on add/delete must not +// panic the subscriber. Both topics are fire-and-forget (no reply). +func TestHandleNATGateway_BadJSON(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + sub, _ := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + require.NoError(t, nc.Publish(TopicAddNATGateway, []byte("not json"))) + require.NoError(t, nc.Publish(TopicDeleteNATGateway, []byte("not json"))) + // Give handlers a moment to run — nothing to assert except no panic. + time.Sleep(50 * time.Millisecond) +} + +// TestHandleIGWRoute_BadJSON: bad payload on add-igw-route must return an +// error envelope; the subscriber pattern uses respond() for request/reply. +func TestHandleIGWRoute_BadJSON(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + sub, _ := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + for _, topic := range []string{TopicAddIGWRoute, TopicDeleteIGWRoute} { + resp, err := nc.Request(topic, []byte("not json"), requestTimeout) + require.NoError(t, err) + var env respondResponse + require.NoError(t, json.Unmarshal(resp.Data, &env)) + assert.False(t, env.Success, "topic %s must return success=false on bad JSON", topic) + } +} + +// TestHandleAddIGWRoute_InvalidCIDR: malformed CIDR must be rejected via the +// reply envelope and must not install any policy. +func TestHandleAddIGWRoute_InvalidCIDR(t *testing.T) { + ctx := context.Background() + _, nc := testutil.StartTestNATS(t) + sub, m := newTestSubscriber(t) + subs, err := sub.Subscribe(nc) + require.NoError(t, err) + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + require.NoError(t, m.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ + Name: topology.VPCRouter("vpc-1"), + ExternalIDs: map[string]string{"spinifex:vpc_id": "vpc-1"}, + })) + require.NoError(t, sub.igw.AttachIGW(ctx, external.IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + evt := IGWRouteEvent{ + VpcId: "vpc-1", SubnetId: "subnet-pub", DestinationCidr: "not-a-cidr", InternetGatewayId: "igw-1", + } + data, err := json.Marshal(evt) + require.NoError(t, err) + resp, err := nc.Request(TopicAddIGWRoute, data, requestTimeout) + require.NoError(t, err) + var env respondResponse + require.NoError(t, json.Unmarshal(resp.Data, &env)) + assert.False(t, env.Success) + + policies, err := m.ListLogicalRouterPolicies(ctx, topology.VPCRouter("vpc-1")) + require.NoError(t, err) + assert.Empty(t, policies) +} + // Every request/reply topic must return a structured error envelope on bad // JSON. Fire-and-forget topics (NAT gateway) just must not panic. func TestBadJSON_AllRequestReplies(t *testing.T) { @@ -129,6 +503,7 @@ func TestBadJSON_AllRequestReplies(t *testing.T) { TopicCreatePort, TopicDeletePort, TopicUpdatePortSGs, TopicIGWAttach, TopicIGWDetach, TopicAddNAT, TopicDeleteNAT, + TopicGateSubnetEgress, TopicUngateSubnetEgress, TopicCreateSG, TopicDeleteSG, TopicUpdateSG, } for _, topic := range topics { diff --git a/spinifex/network/subscribers/topology.go b/spinifex/network/subscribers/topology.go index 9446b6170..0db002c9d 100644 --- a/spinifex/network/subscribers/topology.go +++ b/spinifex/network/subscribers/topology.go @@ -266,7 +266,8 @@ func (s *Subscriber) handleAddNATGateway(msg *nats.Msg) { slog.Error("subscribers: failed to unmarshal vpc.add-nat-gateway event", "err", err) return } - if err := s.natgw.AttachNATGateway(context.Background(), policy.NATGWSpec{ + ctx := context.Background() + if err := s.natgw.AttachNATGateway(ctx, policy.NATGWSpec{ VPCID: evt.VpcId, NATGatewayID: evt.NatGatewayId, PublicIP: evt.PublicIp, @@ -280,6 +281,33 @@ func (s *Subscriber) handleAddNATGateway(msg *nats.Msg) { slog.Info("subscribers: NAT Gateway SNAT rule added", "vpc_id", evt.VpcId, "natgw_id", evt.NatGatewayId, "public_ip", evt.PublicIp, "subnet_cidr", evt.SubnetCidr) + + // SNAT rewrites src IP but the LR still needs a route to egress. Install + // a per-subnet LR policy at NATGW priority so private-subnet packets are + // rerouted out the IGW gateway port (NATGW SNAT happens on the same LR + // before egress). + if evt.SubnetId == "" { + return + } + destCidr := evt.DestinationCidr + if destCidr == "" { + destCidr = "0.0.0.0/0" + } + prefix, err := netip.ParsePrefix(destCidr) + if err != nil { + slog.Error("subscribers: invalid destination CIDR in vpc.add-nat-gateway event", + "cidr", destCidr, "err", err) + return + } + if err := s.igw.EnsureNATGatewaySubnetEgress(ctx, evt.VpcId, evt.SubnetId, prefix); err != nil { + slog.Error("subscribers: EnsureNATGatewaySubnetEgress failed", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", destCidr, + "natgw_id", evt.NatGatewayId, "err", err) + return + } + slog.Info("subscribers: installed NATGW route policy", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", destCidr, + "natgw_id", evt.NatGatewayId) } func (s *Subscriber) handleDeleteNATGateway(msg *nats.Msg) { @@ -288,7 +316,26 @@ func (s *Subscriber) handleDeleteNATGateway(msg *nats.Msg) { slog.Error("subscribers: failed to unmarshal vpc.delete-nat-gateway event", "err", err) return } - if err := s.natgw.DetachNATGateway(context.Background(), evt.VpcId, evt.SubnetCidr); err != nil { + ctx := context.Background() + // Remove the per-subnet policy first so no packets are routed to the LR + // uplink after the SNAT rule is gone (which would leak un-NAT'd private IPs). + if evt.SubnetId != "" { + destCidr := evt.DestinationCidr + if destCidr == "" { + destCidr = "0.0.0.0/0" + } + if prefix, err := netip.ParsePrefix(destCidr); err != nil { + slog.Error("subscribers: invalid destination CIDR in vpc.delete-nat-gateway event", + "cidr", destCidr, "err", err) + } else if err := s.igw.RemoveNATGatewaySubnetEgress(ctx, evt.VpcId, evt.SubnetId, prefix); err != nil { + slog.Warn("subscribers: RemoveNATGatewaySubnetEgress failed", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", destCidr, "err", err) + } else { + slog.Info("subscribers: removed NATGW route policy", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", destCidr) + } + } + if err := s.natgw.DetachNATGateway(ctx, evt.VpcId, evt.SubnetCidr); err != nil { slog.Warn("subscribers: DeleteNATGateway failed", "vpc_id", evt.VpcId, "subnet_cidr", evt.SubnetCidr, "err", err) return @@ -296,3 +343,105 @@ func (s *Subscriber) handleDeleteNATGateway(msg *nats.Msg) { slog.Info("subscribers: NAT Gateway SNAT rule removed", "vpc_id", evt.VpcId, "natgw_id", evt.NatGatewayId, "subnet_cidr", evt.SubnetCidr) } + +func (s *Subscriber) handleAddIGWRoute(msg *nats.Msg) { + var evt IGWRouteEvent + if err := json.Unmarshal(msg.Data, &evt); err != nil { + slog.Error("subscribers: failed to unmarshal vpc.add-igw-route event", "err", err) + respond(msg, err) + return + } + prefix, err := netip.ParsePrefix(evt.DestinationCidr) + if err != nil { + slog.Error("subscribers: invalid destination CIDR in vpc.add-igw-route event", + "cidr", evt.DestinationCidr, "err", err) + respond(msg, err) + return + } + if err := s.igw.EnsureSubnetEgress(context.Background(), evt.VpcId, evt.SubnetId, prefix); err != nil { + slog.Error("subscribers: EnsureSubnetEgress failed", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", evt.DestinationCidr, + "igw_id", evt.InternetGatewayId, "err", err) + respond(msg, err) + return + } + slog.Info("subscribers: installed IGW route policy", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", evt.DestinationCidr, + "igw_id", evt.InternetGatewayId) + respond(msg, nil) +} + +func (s *Subscriber) handleDeleteIGWRoute(msg *nats.Msg) { + var evt IGWRouteEvent + if err := json.Unmarshal(msg.Data, &evt); err != nil { + slog.Error("subscribers: failed to unmarshal vpc.delete-igw-route event", "err", err) + respond(msg, err) + return + } + prefix, err := netip.ParsePrefix(evt.DestinationCidr) + if err != nil { + slog.Error("subscribers: invalid destination CIDR in vpc.delete-igw-route event", + "cidr", evt.DestinationCidr, "err", err) + respond(msg, err) + return + } + if err := s.igw.RemoveSubnetEgress(context.Background(), evt.VpcId, evt.SubnetId, prefix); err != nil { + slog.Warn("subscribers: RemoveSubnetEgress failed", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", evt.DestinationCidr, "err", err) + respond(msg, err) + return + } + slog.Info("subscribers: removed IGW route policy", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", evt.DestinationCidr) + respond(msg, nil) +} + +func (s *Subscriber) handleGateSubnetEgress(msg *nats.Msg) { + var evt SubnetEgressGateEvent + if err := json.Unmarshal(msg.Data, &evt); err != nil { + slog.Error("subscribers: failed to unmarshal vpc.gate-subnet-egress event", "err", err) + respond(msg, err) + return + } + prefix, err := netip.ParsePrefix(evt.DestinationCidr) + if err != nil { + slog.Error("subscribers: invalid destination CIDR in vpc.gate-subnet-egress event", + "cidr", evt.DestinationCidr, "err", err) + respond(msg, err) + return + } + if err := s.igw.EnsureSubnetEgressDrop(context.Background(), evt.VpcId, evt.SubnetId, prefix); err != nil { + slog.Error("subscribers: EnsureSubnetEgressDrop failed", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", evt.DestinationCidr, "err", err) + respond(msg, err) + return + } + slog.Info("subscribers: gated subnet egress", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", evt.DestinationCidr) + respond(msg, nil) +} + +func (s *Subscriber) handleUngateSubnetEgress(msg *nats.Msg) { + var evt SubnetEgressUngateEvent + if err := json.Unmarshal(msg.Data, &evt); err != nil { + slog.Error("subscribers: failed to unmarshal vpc.ungate-subnet-egress event", "err", err) + respond(msg, err) + return + } + prefix, err := netip.ParsePrefix(evt.DestinationCidr) + if err != nil { + slog.Error("subscribers: invalid destination CIDR in vpc.ungate-subnet-egress event", + "cidr", evt.DestinationCidr, "err", err) + respond(msg, err) + return + } + if err := s.igw.RemoveSubnetEgressDrop(context.Background(), evt.VpcId, evt.SubnetId, prefix); err != nil { + slog.Warn("subscribers: RemoveSubnetEgressDrop failed", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", evt.DestinationCidr, "err", err) + respond(msg, err) + return + } + slog.Info("subscribers: ungated subnet egress", + "vpc_id", evt.VpcId, "subnet_id", evt.SubnetId, "cidr", evt.DestinationCidr) + respond(msg, nil) +} diff --git a/tests/e2e/lb/lb_test.go b/tests/e2e/lb/lb_test.go index 6deca3f37..68722e869 100644 --- a/tests/e2e/lb/lb_test.go +++ b/tests/e2e/lb/lb_test.go @@ -114,15 +114,16 @@ func TestLoadBalancer(t *testing.T) { // --- Fixture: shared VPC, subnet, IGW, SG, app instances ---------------- type sharedFixture struct { - VPCID string - SubnetID string - IGWID string - SecurityGroup string - AMIID string - InstanceType string - AppInstanceIDs []string - ClientID string - ClientPublicIP string + VPCID string + SubnetID string + IGWID string + MainRouteTableID string + SecurityGroup string + AMIID string + InstanceType string + AppInstanceIDs []string + ClientID string + ClientPublicIP string } func setupSharedFixture(t *testing.T, c *harness.AWSClient, artifacts string) *sharedFixture { @@ -144,6 +145,8 @@ func setupSharedFixture(t *testing.T, c *harness.AWSClient, artifacts string) *s t.Cleanup(func() { deleteIGW(t, c, f) }) createSubnet(t, c, f) t.Cleanup(func() { deleteSubnet(t, c, f) }) + addPublicRoute(t, c, f) + t.Cleanup(func() { deletePublicRoute(t, c, f) }) configureDefaultSG(t, c, f) harness.Phase(t, "Launching app instances (2× %s)", f.InstanceType) @@ -320,6 +323,43 @@ func deleteSubnet(t *testing.T, c *harness.AWSClient, f *sharedFixture) { } } +// addPublicRoute installs 0.0.0.0/0 → IGW on the VPC's main route table. +// Without this, the daemon classifies the subnet as private and installs an +// OVN LR policy DROP on the subnet's egress (priority 1100), breaking the +// return path for inbound DNATed probes to the public IP. +func addPublicRoute(t *testing.T, c *harness.AWSClient, f *sharedFixture) { + t.Helper() + out, err := c.EC2.DescribeRouteTables(&ec2.DescribeRouteTablesInput{ + Filters: []*ec2.Filter{ + {Name: aws.String("vpc-id"), Values: []*string{aws.String(f.VPCID)}}, + {Name: aws.String("association.main"), Values: []*string{aws.String("true")}}, + }, + }) + require.NoError(t, err, "describe main route table") + require.NotEmpty(t, out.RouteTables, "VPC %s has no main route table", f.VPCID) + f.MainRouteTableID = aws.StringValue(out.RouteTables[0].RouteTableId) + _, err = c.EC2.CreateRoute(&ec2.CreateRouteInput{ + RouteTableId: aws.String(f.MainRouteTableID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String(f.IGWID), + }) + require.NoError(t, err, "create-route 0.0.0.0/0 -> IGW") + t.Logf("public route: %s 0.0.0.0/0 -> %s", f.MainRouteTableID, f.IGWID) +} + +func deletePublicRoute(t *testing.T, c *harness.AWSClient, f *sharedFixture) { + t.Helper() + if f.MainRouteTableID == "" { + return + } + if _, err := c.EC2.DeleteRoute(&ec2.DeleteRouteInput{ + RouteTableId: aws.String(f.MainRouteTableID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + }); err != nil { + t.Logf("delete public route: %v", err) + } +} + func configureDefaultSG(t *testing.T, c *harness.AWSClient, f *sharedFixture) { t.Helper() out, err := c.EC2.DescribeSecurityGroups(&ec2.DescribeSecurityGroupsInput{