From 3415d9f3de4c2b680b559da482a4c6937c77c94e Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Mon, 1 Jun 2026 09:54:29 +1000 Subject: [PATCH] fix(network): IGW route-table AWS parity + per-subnet egress gating (#270) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(network): IGW default route AWS-parity via per-subnet LR policy ADR-0006's IGWManager.AttachIGW installed a VPC-router-wide 0.0.0.0/0 static route at attach time, violating AWS semantics: every subnet on the LR saw egress to the IGW regardless of its route table. Private subnets that should be IGW-isolated were not. Move IGW egress lifecycle onto the route-table API. CreateRoute(IGW, 0.0.0.0/0) emits vpc.add-igw-route per associated subnet; the network subscriber installs an OVN Logical_Router_Policy matching `inport == "rtr-" && ip4.dst == 0.0.0.0/0` with action `reroute` via the IGW gateway port. DeleteRoute / DisassociateRouteTable / ReplaceRouteTableAssociation tear it down. AssociateRouteTable emits add events so the terraform ordering (CreateRoute -> Associate) is honored. Additions: - nbdb.LogicalRouterPolicy + Logical_Router_Policy registration - ovn.Client {Add,Delete,Find,List}LogicalRouterPolicy + live + mock - policy.RouteManager.{Add,Delete}SubnetEgress with priority constants - external.IGWManager.{Ensure,Remove}SubnetEgress resolving nexthop + output port from the gateway state - subscribers handleAddIGWRoute / handleDeleteIGWRoute on vpc.{add,delete}-igw-route - routetable publishIGWRouteEvents / *ForAssociation / *ForSubnet Test updates: - network/policy: AddSubnetEgress idempotency, drift-replace, per-subnet separation, DeleteSubnetEgress - network/external: AttachIGW must NOT install a router-wide default route; EnsureSubnetEgress installs the per-subnet policy with the resolved nexthop (link-local fallback + allocator override paths) - handlers/ec2/routetable: CreateRoute(IGW), Associate, DeleteRoute, Disassociate publish events with the expected subnetID + igwID - ovn/nbdb: table-count + Logical_Router_Policy presence - subscribers: TestSubscribe_RegistersAllTopics covers the two new topics * fix(network): NATGW per-subnet egress + DeleteRoute event symmetry handlers/ec2/routetable: - DeleteRoute now publishes vpc.delete-nat-gateway per associated subnet (mirror of the IGW DeleteRoute path) so the subscriber tears down both the SNAT rule and the per-subnet egress policy. - publishNatGatewayEvent* signatures carry SubnetId + DestinationCidr so the subscriber can install the matching Logical_Router_Policy. network/external: - IGWManager: EnsureNATGatewaySubnetEgress / RemoveNATGatewaySubnetEgress install/remove a per-subnet LR policy at priority SubnetEgressPriorityNATGW. Same nexthop and output port as the IGW priority sibling; lower priority so an IGW route wins when both apply to the same subnet. - EnsureSubnetEgress delegates to ensureSubnetEgressAtPriority for shared validation, lookup, and slog tagging. network/subscribers: - handleAddNATGateway: after SNAT install, parse DestinationCidr (defaults to 0.0.0.0/0) and call EnsureNATGatewaySubnetEgress. Without this the LR has no route to egress NAT'd packets and private-subnet traffic is dropped (E2E Phase 8d regression). - handleDeleteNATGateway: remove the per-subnet policy FIRST, then detach SNAT, so packets are never routed unmasqueraded after SNAT removal. - NATGatewayEvent gains SubnetId + DestinationCidr. Tests: - policy.AddSubnetEgress validation + drift-replace + propagation paths. - mock LR-policy methods reject missing routers and tolerate dangling UUIDs. - subscribers cover NATGW add/delete with invalid CIDR, bad JSON, missing router, IGW-route bad JSON + invalid CIDR. Full end-to-end add+delete via NATS asserting policy lifecycle. - IGW manager covers RemoveSubnetEgress / RemoveNATGatewaySubnetEgress empty-arg rejection + delete error propagation. - routetable: DeleteRoute(NATGW) publishes per associated subnet. * fix(network): exclude VPC CIDR from per-subnet egress reroute predicate In-VPC traffic (e.g. bastion -> private SSH) was being intercepted by the NATGW per-subnet egress LR policy on `ip4.dst == 0.0.0.0/0` and forwarded out the gateway with SNAT, breaking peer-subnet connectivity. Append `&& ip4.dst != ` to the match so in-VPC destinations skip the reroute and follow the directly-connected route. Apply to both IGW (1000) and NATGW (900) priorities. CIDR is read from the LR ExternalIDs label populated at VPC create. DeleteSubnetEgress takes the same excludes so the match string is identical on cleanup. igwManager.vpcExcludeCIDRs fetches from the router each call; lookup failure returns nil and the policy installs without exclusions (preserves prior behaviour as fallback). * fix(routetable): publish per-subnet events for implicit main-RT membership E2E run 26620082197 RCA: subnets with no explicit RT association inherit the VPC main route table per AWS semantics, but publishIGWRouteEvents and publishNatGatewayEvents iterated only record.Associations and skipped Main:true/SubnetId:"" sentinels. Main RT carries exactly one such sentinel, so CreateRoute(igw)/CreateRoute(natgw) on the main RT fanned out to ZERO subnets — subscriber never called EnsureSubnetEgress, OVN LR policy never installed, ping 8.8.8.8 from default-VPC VMs dropped at the VPC router. Caused TestVPCSubnetE2E, TestNATGateway, TestMultinodeSpread/NATGatewayInternet to fail. Add subnetsImplicitlyOnMainRT helper that enumerates VPC subnets without explicit non-main associations, and fan publish to them when record.IsMain. Apply to all three publish helpers (IGW, NATGW, NATGW-delete). Symmetric fixes for effective-RT changes: - AssociateRouteTable: subnet leaves implicit main → emit deletes for main RT's routes before adds for explicit RT - DisassociateRouteTable: subnet returns to implicit main → emit adds for main RT's routes after deletes for departing RT * chore(e2e): capture per-LR lr-route-list/lr-policy-list/lr-nat-list ovn-nbctl show does not dump static routes or policies. Adds three per-LR captures so RCA can verify policy/route install state after the IGW per-subnet refactor. * chore(e2e): tcpdump br-wan + neigh/fdb/conntrack capture during tests EIP egress regression on feat/igw-route-table-aws-parity shows SNAT packets present (n_packets>0) but pings still time out. Post-teardown ovn.* snapshots can't tell whether replies actually arrived on br-wan or got dropped at L2 / by upstream router. start-net-capture.sh: nohup tcpdump on br-wan filtered to 192.168.0.0/23 (EIP pool) + ARP for same range, 200-byte snaplen, 64 MiB cap. Pair: capture-net-diag.sh stops it via /tmp/spinifex-e2e-tcpdump.pid and copies wan.pcap into net-diag/. Also added neigh/bridge-fdb/conntrack dumps to capture-net-diag.sh so L2/L3 binding state is visible alongside the existing OVN dumps. Wired both single-VM and multinode workflows to start capture before the e2e run; teardown step picks up the pcap automatically. * fix(reconcile): rebuild IGW/NATGW per-subnet egress from KV Publisher fires vpc.add-igw-route during default-VPC bootstrap before the subscriber daemon attaches to the topic. NATS core pub/sub drops the message, leaving the LR without a 0.0.0.0/0 policy and off-subnet egress broken on the affected VPC for the lifetime of the process. Reproduced in CI: TestVPCSubnetE2E bastion EIP could ARP and DNS its on-LAN neighbour but never reach 8.8.8.8. LoadIntentFromKV now fans every IGW or NAT-gateway RT route out over its explicit + implicit-main subnets and the reconcile pass invokes EnsureSubnetEgress / EnsureNATGatewaySubnetEgress for each one. Same shape as publishIGWRouteEvents so the bootstrap and the runtime event path produce identical OVN state. * fix(routes): serialize OVN egress wiring via request-reply vpc.igw-attach, vpc.add-igw-route and vpc.add-nat-gateway were fire-and-forget Publish, racing the subscriber's AttachIGW barrier. AttachInternetGateway returned before vpcd finished wiring the OVN gateway LRP, so a follow-up CreateRoute(IGW) published an add-igw-route event that the subscriber processed in a parallel goroutine — installing a Logical_Router_Policy with a nexthop the LRP didn't yet expose. northd compiled an inert reroute that never recovered, leaving the bastion's egress traffic blackholed (no packets dst 8.8.8.8 from the failing VPC's EIP even though local DNS succeeded). Switch the three add-* publishers to utils.RequestEvent. The subscriber ACKs once OVN state is committed, so the gateway handler can't issue subsequent route events against a half-wired router. delete-* paths stay Publish — teardown ordering doesn't matter. handleAddNATGateway now calls respond() at every exit so requesters don't hang. Route-table tests gain an ackSub wrapper (async subscribe + auto-Reply) so request-based publishers complete during unit tests. * Revert "fix(routes): serialize OVN egress wiring via request-reply" This reverts commit bf6f77d5bc8b21f72ed5867175d5a67c010066c8. * fix(igw): restore default static route in AttachIGW/DetachIGW OVN 25.03 LR pipeline runs lr_in_ip_routing (table 15) BEFORE lr_in_policy (table 17). When this branch swapped the per-VPC default static route for the per-subnet Logical_Router_Policy reroute installed by EnsureSubnetEgress, external destinations started dropping at routing priority-0 before the policy table ran. Reproduced locally: ovn-trace stops at table 15 drop, ping 8.8.8.8 from inside a fresh-VPC bastion produced zero packets on br-wan until `ovn-nbctl lr-route-add 0.0.0.0/0 ` was issued manually — after which the same ping succeeded immediately. dev still green because dev calls AddDefaultRoute on AttachIGW. Restore the original AddDefaultRoute / DeleteDefaultRoute calls; keep the per-subnet EnsureSubnetEgress machinery intact so future per-subnet egress gating (DROP policies for subnets whose RT lacks a 0.0.0.0/0 route) can layer on top. Flip the two TestAttachIGW assertions that pinned the "AttachIGW must not install a default static route" expectation. Pairs with revert of bf6f77d5 in the previous commit (request-reply serialization treated a non-existent race; real bug was missing routing fallback). * feat(network): per-subnet egress drop gating for AWS RT parity VPC LRs carry a router-wide 0.0.0.0/0 default static route while an IGW is attached (lr_in_ip_routing runs before lr_in_policy on OVN 25.03, so per- subnet reroute alone is insufficient). Subnets whose effective route table lacks 0.0.0.0/0 were previously able to egress via that default route, violating AWS semantics. Install an LR_Policy DROP at SubnetEgressPriorityDrop=1100 (above IGW=1000 and NATGW=900) for unrouted subnets. Match excludes VPC CIDR, link-local (169.254/16) and multicast (224/4) so intra-VPC, DHCP/metadata and multicast still flow. Wiring: - policy.RouteManager gains AddSubnetEgressDrop / DeleteSubnetEgressDrop. - external.IGWManager gains EnsureSubnetEgressDrop / RemoveSubnetEgressDrop. - New events: SubnetEgressGateEvent / SubnetEgressUngateEvent on topics vpc.gate-subnet-egress / vpc.ungate-subnet-egress. - routetable handler publishes gate/ungate alongside existing add/delete IGW/NATGW route events on every CreateRoute/DeleteRoute/Associate/ Disassociate/Replace mutation, based on subnet's recomputed effective RT. - reconciler bootstrap path emits DropGates intent for subnets in IGW- attached VPCs whose effective RT lacks 0.0.0.0/0. Tests cover primitive idempotency, drift recovery, event publishing per mutation surface, implicit-main fan-out, and intent computation. * fix(igw): fan out subnet egress gate decisions on IGW attach/detach The reconciler computes per-subnet DROP policies for any subnet in an IGW-attached VPC whose effective route table lacks 0.0.0.0/0, but those decisions were only re-evaluated by the periodic drift tick (every 5 min). AttachInternetGateway / DetachInternetGateway now publish gate/ungate events for every subnet in the VPC immediately, closing the window where a freshly-attached IGW could leave unrouted subnets unprotected. Wiring: IGWServiceImpl gains a nil-safe GatePublisher hook satisfied by RouteTableServiceImpl. Daemon installs the hook once both services exist. RouteTableServiceImpl.PublishGateDecisionsForVPC enumerates the VPC's subnets and reuses publishSubnetEgressGateDecision per subnet. Also fixes the LB e2e suite, which built VPC + IGW + subnet without ever creating a 0.0.0.0/0 → IGW route — relying on the pre-DROP-gate behaviour where unrouted subnets still had silent egress via the router-wide static route. The fixture now installs the explicit public route on the main RT (AWS-faithful semantics for a public subnet). * fix(igw): skip gate fan-out on attach to avoid bootstrap race AttachInternetGateway fan-out races with the default-VPC bootstrap path in ensureDefaultVPCInfrastructureFor, which attaches the IGW and immediately publishes a 0.0.0.0/0 → IGW CreateRoute event. The two events land on distinct NATS subjects (vpc.gate-subnet-egress vs vpc.ungate-subnet-egress) with no cross-subject ordering guarantee. When ungate arrived first the subscriber became a no-op and the subsequent gate installed a permanent DROP on the default subnet, killing the return path for guest SSH probes (TestMultinodeGuestSSH connect timeout to the instance public IP). Detach fan-out is retained — no follow-up CreateRoute fires after DetachInternetGateway so the path is race-free. Subnets without a default route still get DROPped during the reconciler's periodic drift pass (loadSubnetDropGates). The window is re-opened to ≤5 min on user-driven AttachInternetGateway, which is the original behaviour pre-mulga-siv-163. A follow-up bead can close that gap properly by converting the gate/ungate subjects into a single recompute trigger that re-reads effective RT state. * fix(vpc): prevent duplicate main route table from bootstrap race Concurrent multi-node daemons running EnsureDefaultVPC for the admin account (bootstrap-pinned VpcId) each minted a fresh random rtb-ID and Put it with IsMain=true. The KV ended up with two main route table records for the same VPC. CreateRoute's post-write gate recompute (per-subnet egress drop gating) then resolved effectiveRouteTable non-deterministically across the two IsMain=true records; when the orphan (route-less) RT won, the gate decision became "no default egress" and vpc.gate-subnet-egress fired. The subscriber installed a Priority-1100 DROP on the default subnet's egress, killing the return path for guest SSH probes (TestMultinodeGuestSSH connect timeout to 192.168.0.161 in run 26699045536). Two-part fix: 1. createMainRouteTable scans rtbKV for an existing IsMain=true record on the same VPC before Put-ing a new one. Race window narrows from "always duplicates under concurrent bootstrap" to a sub-ms TOCTOU between scan and Put. 2. effectiveRouteTable / mainRouteTable use a deterministic preferMain tiebreak when multiple IsMain=true records exist: more routes > oldest CreatedAt > smaller RouteTableId. Defensive — any duplicate that slips through (cross-AZ restore, manual KV edit, residual race) still produces consistent egress gate decisions instead of flapping on KV scan order. * fix(policy): atomic ACL replacement for SG mutations Add ovn.Client.ReplaceACLs — detach + delete existing ACL rows and attach freshly-created rows in one OVSDB transaction. EnsureSG and UpdateSG now funnel through ReplaceACLs instead of ClearACLs + AddACLs, closing the mid-flight window where the port group held zero ACLs and tenant LSP traffic defaulted to drop (observable on ICMP and on TCP SYNs that miss conntrack). Mock client exposes ClearACLCalls / ReplaceACLCalls counters so tests can assert atomicity. New TestSGManager_UpdateSG_AtomicReplace fails if SG mutations ever fall back to the split path. --- .github/scripts/capture-net-diag.sh | 33 + .github/scripts/start-net-capture.sh | 47 ++ .github/workflows/e2e-go.yml | 23 + spinifex/daemon/daemon.go | 7 + spinifex/handlers/ec2/igw/service_impl.go | 41 +- .../handlers/ec2/igw/service_impl_test.go | 89 +++ .../handlers/ec2/routetable/service_impl.go | 523 +++++++++++- .../ec2/routetable/service_impl_test.go | 741 +++++++++++++++++- spinifex/handlers/ec2/vpc/service_impl.go | 53 ++ .../handlers/ec2/vpc/service_impl_test.go | 52 ++ spinifex/network/external/igw.go | 148 ++++ spinifex/network/external/igw_test.go | 165 +++- spinifex/network/ovn/client.go | 18 + spinifex/network/ovn/live.go | 199 +++++ spinifex/network/ovn/mock/mock.go | 136 +++- spinifex/network/ovn/mock/mock_test.go | 104 +++ spinifex/network/ovn/nbdb/model.go | 16 + spinifex/network/ovn/nbdb/model_test.go | 1 + spinifex/network/policy/route.go | 183 ++++- spinifex/network/policy/route_test.go | 502 ++++++++++++ spinifex/network/policy/sg.go | 19 +- spinifex/network/policy/sg_test.go | 25 + spinifex/network/reconcile/apply.go | 36 + spinifex/network/reconcile/apply_test.go | 9 +- spinifex/network/reconcile/intent.go | 168 +++- spinifex/network/reconcile/intent_test.go | 143 +++- spinifex/network/reconcile/reconcile.go | 5 + spinifex/network/subscribers/events.go | 82 +- spinifex/network/subscribers/subscribers.go | 4 + .../network/subscribers/subscribers_test.go | 375 +++++++++ spinifex/network/subscribers/topology.go | 153 +++- tests/e2e/lb/lb_test.go | 58 +- 32 files changed, 4065 insertions(+), 93 deletions(-) create mode 100755 .github/scripts/start-net-capture.sh 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{