From ff0dc4256226f6b634a46de854792c2e2a15aa1c Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Thu, 4 Jun 2026 16:33:45 +1000 Subject: [PATCH] feat: IMDS subnet level (#292) --- .../handlers/ec2/instance/helpers_test.go | 16 ++ .../handlers/ec2/instance/service_impl.go | 34 ++- .../ec2/instance/service_impl_test.go | 14 ++ spinifex/handlers/imds/buckets.go | 16 +- spinifex/handlers/imds/buckets_test.go | 8 +- spinifex/handlers/imds/constants.go | 4 +- spinifex/handlers/imds/metadata.go | 24 +- spinifex/handlers/imds/server.go | 151 +++++++----- spinifex/handlers/imds/server_test.go | 73 +++--- spinifex/handlers/imds/service.go | 8 +- spinifex/handlers/imds/veth_store.go | 57 ++--- spinifex/handlers/imds/veth_store_test.go | 33 +-- spinifex/network/external/igw.go | 16 +- .../external/imds_datapath_contract_test.go | 110 +++++---- spinifex/network/external/imds_topology.go | 214 +++++++----------- .../network/external/imds_topology_test.go | 162 +++++++------ spinifex/network/external/invariants_test.go | 72 +++--- spinifex/network/host/imds_names.go | 46 ++-- spinifex/network/host/imds_veth.go | 85 +++---- spinifex/network/host/imds_veth_test.go | 45 ++-- spinifex/network/reconcile/apply.go | 30 +-- spinifex/network/reconcile/invariants_test.go | 50 ---- spinifex/network/subscribers/topology.go | 37 +-- spinifex/network/topology/dhcp_options.go | 10 +- .../network/topology/dhcp_options_test.go | 6 +- spinifex/network/topology/manager_live.go | 2 - spinifex/network/topology/names.go | 21 +- spinifex/vpcd/vpcd.go | 6 +- tests/e2e/harness/imds_diag.go | 59 ++--- tests/e2e/harness/ovn.go | 11 + tests/e2e/single/imds_test.go | 152 ++++++++++--- tests/e2e/single/tests_test.go | 6 +- 32 files changed, 839 insertions(+), 739 deletions(-) diff --git a/spinifex/handlers/ec2/instance/helpers_test.go b/spinifex/handlers/ec2/instance/helpers_test.go index 5f690b912..782272d7d 100644 --- a/spinifex/handlers/ec2/instance/helpers_test.go +++ b/spinifex/handlers/ec2/instance/helpers_test.go @@ -1,6 +1,7 @@ package handlers_ec2_instance import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -17,6 +18,9 @@ func TestGenerateNetworkConfig_OneEmpty(t *testing.T) { cfg := generateNetworkConfig("02:00:00:aa:bb:cc", "", "", "", nil) assert.Contains(t, cfg, "vpc0:", "eniMAC alone should produce per-interface config") assert.NotContains(t, cfg, "dev0:", "no dev NIC without devMAC") + // The IMDS on-link route rides vpc0 even with no dev NIC / extra ENIs — the + // common production shape — so guard it here too, not only on the multi-NIC path. + assert.Contains(t, cfg, "to: 169.254.169.254/32", "vpc0 must carry the IMDS on-link route") cfg = generateNetworkConfig("", "02:00:00:dd:ee:ff", "", "", nil) assert.Equal(t, cloudInitNetworkConfigWildcard, cfg, "should fall back to wildcard if eniMAC empty") @@ -67,5 +71,17 @@ func TestGenerateNetworkConfig_MgmtIPWithoutMAC(t *testing.T) { assert.NotContains(t, cfg, "mgmt0:", "mgmt NIC should not appear without MAC") } +func TestGenerateNetworkConfig_IMDSOnLinkRoute(t *testing.T) { + // The primary VPC NIC carries an on-link route to the IMDS metadata IP so the + // guest ARPs 169.254.169.254 directly; the per-subnet localport answers. + cfg := generateNetworkConfig("02:00:00:aa:bb:cc", "02:00:00:dd:ee:ff", "", "", + []string{"02:00:00:bb:bb:bb"}) + assert.Contains(t, cfg, "to: 169.254.169.254/32") + assert.Contains(t, cfg, "scope: link") + // Only the primary NIC (vpc0) gets it — not extra VPC NICs or the dev NIC, + // matching AWS (IMDS reached via the primary ENI). + assert.Equal(t, 1, strings.Count(cfg, "169.254.169.254/32")) +} + // Route for multi-node LB mgmt traffic is handled via the fw_cfg netcfg // blob delivered to the LB microVM, not in the network-config. diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index 92e3aa606..370b7e5f3 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/spinifex/handlers/ec2/instance/service_impl.go @@ -24,6 +24,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/filterutil" "github.com/mulgadc/spinifex/spinifex/gpu" handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" + handlers_imds "github.com/mulgadc/spinifex/spinifex/handlers/imds" "github.com/mulgadc/spinifex/spinifex/instancetypes" "github.com/mulgadc/spinifex/spinifex/network/topology" "github.com/mulgadc/spinifex/spinifex/objectstore" @@ -133,6 +134,14 @@ func selectNetworkConfigForFamily(distroFamily, eniMAC, devMAC, mgmtMAC, mgmtIP // // The dev NIC still gets an IP via DHCP (needed for hostfwd port forwarding) // but dhcp4-overrides prevents it from installing routes or DNS. +// +// The primary VPC NIC (vpc0) also carries an on-link route to the IMDS metadata +// IP (169.254.169.254): under the L2 datapath the metadata localport lives on +// the guest's subnet switch and answers ARP directly, so the guest must treat +// the address as on-link rather than steering it via the default gateway. The +// route is reached via the primary ENI, matching AWS; it is not delivered via +// DHCP option 121 (which would force the default route into option 121 per +// RFC 3442) and the default gateway stays option 3. func generateNetworkConfig(eniMAC, devMAC, mgmtMAC, mgmtIP string, extraENIMACs []string) string { if eniMAC == "" { return cloudInitNetworkConfigWildcard @@ -145,7 +154,10 @@ func generateNetworkConfig(eniMAC, devMAC, mgmtMAC, mgmtIP string, extraENIMACs macaddress: "%s" dhcp4: true dhcp-identifier: mac -`, eniMAC) + routes: + - to: %s/32 + scope: link +`, eniMAC, handlers_imds.MetaDataServerIP) for i, mac := range extraENIMACs { if mac == "" { @@ -215,9 +227,9 @@ type CloudInitData struct { // RHEL-family guest. Returns empty strings when eniMAC is empty (wildcard / // non-VPC path) so NM's default autoconnect handles the interface. // -// Mirrors generateNetworkConfig's per-interface structure: vpc0 + optional -// vpc1..N for extra ENIs, dev0 (DHCP with route/DNS suppression), mgmt0 -// (static). Owning the keyfile bytes directly avoids relying on cloud-init's +// Mirrors generateNetworkConfig's per-interface structure: vpc0 (DHCP + on-link +// IMDS route) + optional vpc1..N for extra ENIs, dev0 (DHCP with route/DNS +// suppression), mgmt0 (static). Owning the keyfile bytes directly avoids relying on cloud-init's // v2-to-NM renderer round-tripping dhcp-identifier: mac → dhcp-client-id=mac, // which OVN's MAC-keyed DHCP requires. // @@ -236,7 +248,7 @@ func buildRHELCloudInit(eniMAC, devMAC, mgmtMAC, mgmtIP string, extraENIMACs []s rc.WriteString(" - [ restorecon, -R, /etc/NetworkManager/system-connections/ ]\n") rc.WriteString(" - [ nmcli, connection, reload ]\n") - appendRHELDHCPKeyfile(&wf, "vpc0", eniMAC, false) + appendRHELDHCPKeyfile(&wf, "vpc0", eniMAC, false, true) rc.WriteString(" - [ nmcli, connection, up, vpc0 ]\n") for i, mac := range extraENIMACs { @@ -244,12 +256,12 @@ func buildRHELCloudInit(eniMAC, devMAC, mgmtMAC, mgmtIP string, extraENIMACs []s continue } name := fmt.Sprintf("vpc%d", i+1) - appendRHELDHCPKeyfile(&wf, name, mac, false) + appendRHELDHCPKeyfile(&wf, name, mac, false, false) fmt.Fprintf(&rc, " - [ nmcli, connection, up, %s ]\n", name) } if devMAC != "" { - appendRHELDHCPKeyfile(&wf, "dev0", devMAC, true) + appendRHELDHCPKeyfile(&wf, "dev0", devMAC, true, false) rc.WriteString(" - [ nmcli, connection, up, dev0 ]\n") } @@ -264,7 +276,7 @@ func buildRHELCloudInit(eniMAC, devMAC, mgmtMAC, mgmtIP string, extraENIMACs []s return strings.TrimRight(wf.String(), "\n"), strings.TrimRight(rc.String(), "\n") } -func appendRHELDHCPKeyfile(b *strings.Builder, name, mac string, suppressDefaults bool) { +func appendRHELDHCPKeyfile(b *strings.Builder, name, mac string, suppressDefaults, imdsRoute bool) { fmt.Fprintf(b, " - path: /etc/NetworkManager/system-connections/%s.nmconnection\n", name) b.WriteString(" owner: root:root\n") b.WriteString(" permissions: '0600'\n") @@ -283,6 +295,12 @@ func appendRHELDHCPKeyfile(b *strings.Builder, name, mac string, suppressDefault b.WriteString(" never-default=true\n") b.WriteString(" ignore-auto-dns=true\n") } + if imdsRoute { + // On-link route to the IMDS metadata IP so the guest ARPs 169.254.169.254 + // directly on this NIC; the per-subnet localport answers. No next hop = + // link scope. Matches the netplan path in generateNetworkConfig. + fmt.Fprintf(b, " route1=%s/32\n", handlers_imds.MetaDataServerIP) + } b.WriteString(" [ipv6]\n") b.WriteString(" method=disabled\n") } diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index 2f0f45adc..b2da93053 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/spinifex/handlers/ec2/instance/service_impl_test.go @@ -383,6 +383,8 @@ func TestBuildRHELCloudInit(t *testing.T) { assert.Contains(t, wf, "dhcp-client-id=mac") assert.Contains(t, wf, "method=auto") assert.NotContains(t, wf, "never-default") + // The IMDS on-link route rides vpc0 even with no dev NIC / extra ENIs. + assert.Contains(t, wf, "route1=169.254.169.254/32", "vpc0 must carry the IMDS on-link route") assert.Contains(t, rc, " - [ restorecon, -R, /etc/NetworkManager/system-connections/ ]") assert.Contains(t, rc, " - [ nmcli, connection, reload ]") assert.Contains(t, rc, " - [ nmcli, connection, up, vpc0 ]") @@ -416,6 +418,18 @@ func TestBuildRHELCloudInit(t *testing.T) { assert.NotContains(t, wf, "vpc1.nmconnection") assert.Contains(t, wf, "vpc2.nmconnection") }) + + t.Run("vpc0 carries on-link IMDS route, others do not", func(t *testing.T) { + wf, _ := buildRHELCloudInit( + "02:00:00:00:00:01", + "02:00:00:00:00:99", + "", "", + []string{"02:00:00:00:00:02"}, + ) + assert.Contains(t, wf, "route1=169.254.169.254/32") + // Primary NIC only — not extra VPC NICs or the dev NIC. + assert.Equal(t, 1, strings.Count(wf, "route1=169.254.169.254/32")) + }) } func TestCloudInitTemplate_FamilyBranching(t *testing.T) { diff --git a/spinifex/handlers/imds/buckets.go b/spinifex/handlers/imds/buckets.go index fbcb6679b..6e2b68fad 100644 --- a/spinifex/handlers/imds/buckets.go +++ b/spinifex/handlers/imds/buckets.go @@ -8,11 +8,11 @@ import ( ) const ( - // KVBucketIMDSVPCVeth is the canonical record that a VPC has IMDS plumbing - // installed. Keyed by vpcID, the value carries the per-VPC veth/LSP spec the - // BindManager replays to materialise host-side state on every chassis. - KVBucketIMDSVPCVeth = "spinifex-network-imds-vpc-veth" - KVBucketIMDSVPCVethVersion = 1 + // KVBucketIMDSSubnetVeth is the canonical record that a subnet has an IMDS + // localport installed. Keyed by subnetID, the value carries the localport + // spec the BindManager replays to materialise host-side state on every chassis. + KVBucketIMDSSubnetVeth = "spinifex-network-imds-subnet-veth" + KVBucketIMDSSubnetVethVersion = 1 // KVBucketENIByVPCIP is a pure reverse index (vpcID/ip → eniID) so the IMDS // handler can resolve a request's datapath-attested source IP to an ENI in @@ -24,8 +24,8 @@ const ( // InitBuckets opens (or creates) both IMDS KV buckets and runs any pending // migrations. History is fixed at 1: both are write-once-per-key, so retained // revisions only waste storage and lengthen tombstone lifetime. -func InitBuckets(js nats.JetStreamContext, replicas int) (vpcVeth, eniByIP nats.KeyValue, err error) { - vpcVeth, err = initIMDSBucket(js, KVBucketIMDSVPCVeth, KVBucketIMDSVPCVethVersion, replicas) +func InitBuckets(js nats.JetStreamContext, replicas int) (subnetVeth, eniByIP nats.KeyValue, err error) { + subnetVeth, err = initIMDSBucket(js, KVBucketIMDSSubnetVeth, KVBucketIMDSSubnetVethVersion, replicas) if err != nil { return nil, nil, err } @@ -35,7 +35,7 @@ func InitBuckets(js nats.JetStreamContext, replicas int) (vpcVeth, eniByIP nats. return nil, nil, err } - return vpcVeth, eniByIP, nil + return subnetVeth, eniByIP, nil } // InitENIByIPBucket opens (or creates) just the eni-by-vpc-ip reverse-index diff --git a/spinifex/handlers/imds/buckets_test.go b/spinifex/handlers/imds/buckets_test.go index ac02d1460..4857cefb3 100644 --- a/spinifex/handlers/imds/buckets_test.go +++ b/spinifex/handlers/imds/buckets_test.go @@ -19,13 +19,13 @@ func TestInitENIByIPBucket(t *testing.T) { func TestInitBuckets_ReplicaClampAndIdempotent(t *testing.T) { _, _, js := testutil.StartTestJetStream(t) - vpcVeth, eniByIP, err := InitBuckets(js, 0) + subnetVeth, eniByIP, err := InitBuckets(js, 0) require.NoError(t, err) - require.NotNil(t, vpcVeth) + require.NotNil(t, subnetVeth) require.NotNil(t, eniByIP) - vpcVeth2, eniByIP2, err := InitBuckets(js, 1) + subnetVeth2, eniByIP2, err := InitBuckets(js, 1) require.NoError(t, err) - require.NotNil(t, vpcVeth2) + require.NotNil(t, subnetVeth2) require.NotNil(t, eniByIP2) } diff --git a/spinifex/handlers/imds/constants.go b/spinifex/handlers/imds/constants.go index 6210dece2..d5e0e7ea9 100644 --- a/spinifex/handlers/imds/constants.go +++ b/spinifex/handlers/imds/constants.go @@ -1,6 +1,6 @@ package handlers_imds // MetaDataServerIP is the link-local address the EC2 Instance Metadata Service is -// served on, identical to AWS. Exported so the network topology layer can set it as -// options:arp_proxy on every subnet router LSP, answering ARP for any guest. +// served on, identical to AWS. Exported so the network topology layer can claim it +// on the subnet-switch localport that answers IMDS over L2 for every guest. const MetaDataServerIP = "169.254.169.254" diff --git a/spinifex/handlers/imds/metadata.go b/spinifex/handlers/imds/metadata.go index cabe07310..7dc9509e5 100644 --- a/spinifex/handlers/imds/metadata.go +++ b/spinifex/handlers/imds/metadata.go @@ -105,25 +105,27 @@ func (s *IMDSServiceImpl) handleMetadata(w http.ResponseWriter, r *http.Request) s.dispatch(w, r, eni) } -// resolveCaller maps the request's (vpcID-from-context, source-IP) to the -// owning ENI, or nil when no mapping exists (logged for operator triage). A -// backend error is also surfaced as nil → the caller 404s, never 500s, matching -// AWS's opaque "eventually available" boot posture. +// resolveCaller maps the request's (vpcID-from-context, source-IP) to the owning +// ENI, or nil when no mapping exists (logged for operator triage). The listener +// identifies the subnet; its statically-resolved VPC is what keys the +// eni-by-vpc-ip index. A backend error is also surfaced as nil → the caller +// 404s, never 500s, matching AWS's opaque "eventually available" boot posture. func (s *IMDSServiceImpl) resolveCaller(r *http.Request) *eniFacts { vpcID, _ := r.Context().Value(ctxKeyVPCID).(string) + subnetID, _ := r.Context().Value(ctxKeySubnetID).(string) srcIP := utils.ClientIP(r.RemoteAddr) if vpcID == "" || srcIP == "" { - slog.Warn("IMDS: request missing VPC context or source IP", "vpc_id", vpcID, "remote_addr", r.RemoteAddr) + slog.Warn("IMDS: request missing VPC context or source IP", "vpc_id", vpcID, "subnet_id", subnetID, "remote_addr", r.RemoteAddr) return nil } eni, err := s.resolver.resolveENI(vpcID, srcIP) if err != nil { - slog.Error("IMDS: ENI resolution failed", "vpc_id", vpcID, "src_ip", srcIP, "err", err) + slog.Error("IMDS: ENI resolution failed", "vpc_id", vpcID, "subnet_id", subnetID, "src_ip", srcIP, "err", err) return nil } if eni == nil { - slog.Warn("IMDS: no ENI for source IP", "vpc_id", vpcID, "src_ip", srcIP) + slog.Warn("IMDS: no ENI for source IP", "vpc_id", vpcID, "subnet_id", subnetID, "src_ip", srcIP) return nil } return eni @@ -381,7 +383,11 @@ func writeJSON(w http.ResponseWriter, v any) { } // ctxKey is the unexported context-key type for values the bind manager threads -// into each request (the VPC ID the listener's veth maps to). +// into each request: the subnet the listener serves and that subnet's +// statically-resolved owning VPC (which keys the eni-by-vpc-ip index). type ctxKey int -const ctxKeyVPCID ctxKey = iota +const ( + ctxKeyVPCID ctxKey = iota + ctxKeySubnetID +) diff --git a/spinifex/handlers/imds/server.go b/spinifex/handlers/imds/server.go index 2443ff140..edc2bf9e5 100644 --- a/spinifex/handlers/imds/server.go +++ b/spinifex/handlers/imds/server.go @@ -7,6 +7,7 @@ import ( "log/slog" "net" "net/http" + "net/netip" "os" "runtime" "sync" @@ -18,20 +19,21 @@ import ( "golang.org/x/sys/unix" ) -// ensureVethFunc / removeVethFunc are the per-VPC host veth lifecycle hooks, +// ensureVethFunc / removeVethFunc are the per-subnet host veth lifecycle hooks, // injected (not imported) to avoid an import cycle via network/host. vpcd passes // host.EnsureIMDSVeth / host.RemoveIMDSVeth; tests pass root-free fakes. ensure -// returns the per-VPC netns the listener must be created in plus the host-end -// veth name (now living inside that netns). -type ensureVethFunc func(ctx context.Context, vpcID string) (netnsName, hostEnd string, err error) -type removeVethFunc func(ctx context.Context, vpcID string) error +// takes the subnet CIDR (added on-link in the netns so the reply resolves the +// guest by ARP) and returns the per-subnet netns the listener must be created in +// plus the host-end veth name (now living inside that netns). +type ensureVethFunc func(ctx context.Context, subnetID string, cidr netip.Prefix) (netnsName, hostEnd string, err error) +type removeVethFunc func(ctx context.Context, subnetID string) error -// listenFunc binds a TCP listener on 169.254.169.254:80 inside the VPC's netns, +// listenFunc binds a TCP listener on 169.254.169.254:80 inside the subnet's netns, // scoped to its host veth. Swappable in tests so bind-manager logic runs without // CAP_NET_ADMIN or a real netns. An empty netnsName binds in the current netns. type listenFunc func(ctx context.Context, netnsName, hostEnd string) (net.Listener, error) -// activeBinding is the per-VPC realised state: a listener bound to the VPC's +// activeBinding is the per-subnet realised state: a listener bound to the subnet's // host veth and the http.Server serving it. type activeBinding struct { listener net.Listener @@ -39,22 +41,24 @@ type activeBinding struct { } // bindManager realises the per-chassis IMDS listener stack. It runs on every -// chassis and eagerly materialises a veth + OVS port + listener for every VPC in -// the vpc-veth bucket — lifecycle is "watch the bucket, ensure local state". +// chassis and eagerly materialises a veth + OVS port + listener for every subnet in +// the subnet-veth bucket — lifecycle is "watch the bucket, ensure local state". type bindManager struct { - kv nats.KeyValue // spinifex-network-imds-vpc-veth + kv nats.KeyValue // spinifex-network-imds-subnet-veth + store *VethStore // reads the subnet CIDR the host on-link route needs handler http.Handler ensureVeth ensureVethFunc removeVeth removeVethFunc listen listenFunc mu sync.Mutex - active map[string]*activeBinding // vpcID → binding + active map[string]*activeBinding // subnetID → binding } func newBindManager(kv nats.KeyValue, handler http.Handler, ensure ensureVethFunc, remove removeVethFunc, listen listenFunc) *bindManager { return &bindManager{ kv: kv, + store: NewVethStore(kv), handler: handler, ensureVeth: ensure, removeVeth: remove, @@ -63,7 +67,7 @@ func newBindManager(kv nats.KeyValue, handler http.Handler, ensure ensureVethFun } } -// sync reads the full vpc-veth bucket and binds a listener for every entry. Run +// sync reads the full subnet-veth bucket and binds a listener for every entry. Run // once at startup; idempotent, so a re-sync after a transient error is safe. func (b *bindManager) sync(ctx context.Context) error { keys, err := b.kv.Keys() @@ -71,26 +75,55 @@ func (b *bindManager) sync(ctx context.Context) error { if errors.Is(err, nats.ErrNoKeysFound) { return nil } - return fmt.Errorf("list vpc-veth keys: %w", err) + return fmt.Errorf("list subnet-veth keys: %w", err) } - for _, vpcID := range keys { - if vpcID == utils.VersionKey { - continue // schema-version marker written by migrate.RunKV, not a VPC + for _, subnetID := range keys { + if subnetID == utils.VersionKey { + continue // schema-version marker written by migrate.RunKV, not a subnet } - if err := b.bind(ctx, vpcID); err != nil { - slog.Error("IMDS: bind failed during sync", "vpc_id", vpcID, "err", err) + vpcID, cidr, err := b.subnetBinding(subnetID) + if err != nil { + slog.Error("IMDS: resolve subnet binding during sync", "subnet_id", subnetID, "err", err) + continue + } + if err := b.bind(ctx, subnetID, vpcID, cidr); err != nil { + slog.Error("IMDS: bind failed during sync", "subnet_id", subnetID, "err", err) } } return nil } -// watch reacts to vpc-veth bucket changes for the life of ctx: a Put binds the -// VPC (if not already bound), a Delete unbinds it and tears down the veth. Both +// subnetBinding reads the persisted veth record for subnetID and returns the two +// facts bind needs: the owning VPC (the subnet→VPC static lookup — the listener +// identifies the subnet, the eni-by-vpc-ip index is keyed vpcID/ip) and the +// subnet CIDR (added on-link in the netns so the reply resolves the guest by +// ARP). A record missing either field is malformed; bind cannot proceed without +// them, and the reconciler re-publishes a complete record to converge. +func (b *bindManager) subnetBinding(subnetID string) (vpcID string, cidr netip.Prefix, err error) { + rec, err := b.store.Get(subnetID) + if err != nil { + return "", netip.Prefix{}, err + } + if rec == nil { + return "", netip.Prefix{}, fmt.Errorf("no veth record for %s", subnetID) + } + if rec.VPCID == "" { + return "", netip.Prefix{}, fmt.Errorf("veth record for %s has no vpc_id", subnetID) + } + cidr, err = netip.ParsePrefix(rec.SubnetCIDR) + if err != nil { + return "", netip.Prefix{}, fmt.Errorf("parse subnet cidr %q: %w", rec.SubnetCIDR, err) + } + return rec.VPCID, cidr, nil +} + +// watch reacts to subnet-veth bucket changes for the life of ctx: a Put binds the +// subnet (if not already bound), a Delete unbinds it and tears down the veth. Both // transitions are idempotent. func (b *bindManager) watch(ctx context.Context) { watcher, err := b.kv.WatchAll(nats.Context(ctx)) if err != nil { - slog.Error("IMDS: failed to start vpc-veth watcher", "err", err) + slog.Error("IMDS: failed to start subnet-veth watcher", "err", err) return } defer func() { _ = watcher.Stop() }() @@ -109,12 +142,17 @@ func (b *bindManager) watch(ctx context.Context) { continue } if entry.Key() == utils.VersionKey { - continue // schema-version marker written by migrate.RunKV, not a VPC + continue // schema-version marker written by migrate.RunKV, not a subnet } switch entry.Operation() { case nats.KeyValuePut: - if err := b.bind(ctx, entry.Key()); err != nil { - slog.Error("IMDS: bind failed on watch", "vpc_id", entry.Key(), "err", err) + vpcID, cidr, err := b.subnetBinding(entry.Key()) + if err != nil { + slog.Error("IMDS: resolve subnet binding on watch", "subnet_id", entry.Key(), "err", err) + continue + } + if err := b.bind(ctx, entry.Key(), vpcID, cidr); err != nil { + slog.Error("IMDS: bind failed on watch", "subnet_id", entry.Key(), "err", err) } case nats.KeyValueDelete, nats.KeyValuePurge: b.unbind(ctx, entry.Key()) @@ -123,17 +161,19 @@ func (b *bindManager) watch(ctx context.Context) { } } -// bind ensures the host veth then opens a device-scoped listener for vpcID. A -// no-op if the VPC is already bound. -func (b *bindManager) bind(ctx context.Context, vpcID string) error { +// bind ensures the subnet's host veth (with the subnet CIDR on-link for the reply +// path) then opens a device-scoped listener for subnetID. vpcID is the subnet's +// statically-resolved owning VPC, threaded into every request so the handler can +// key the eni-by-vpc-ip index. A no-op if already bound. +func (b *bindManager) bind(ctx context.Context, subnetID, vpcID string, cidr netip.Prefix) error { b.mu.Lock() - if _, ok := b.active[vpcID]; ok { + if _, ok := b.active[subnetID]; ok { b.mu.Unlock() return nil } b.mu.Unlock() - netnsName, hostEnd, err := b.ensureVeth(ctx, vpcID) + netnsName, hostEnd, err := b.ensureVeth(ctx, subnetID, cidr) if err != nil { return fmt.Errorf("ensure veth: %w", err) } @@ -146,49 +186,51 @@ func (b *bindManager) bind(ctx context.Context, vpcID string) error { server := &http.Server{ Handler: b.handler, ReadHeaderTimeout: 10 * time.Second, - // Tag every request from this listener with its VPC ID. The handler - // pairs it with the datapath-attested source IP to resolve the ENI. + // Tag every request from this listener with the subnet it serves and that + // subnet's owning VPC. The handler pairs the VPC with the datapath-attested + // source IP to resolve the ENI; the subnet ID rides along for triage. BaseContext: func(net.Listener) context.Context { - return context.WithValue(ctx, ctxKeyVPCID, vpcID) + c := context.WithValue(ctx, ctxKeyVPCID, vpcID) + return context.WithValue(c, ctxKeySubnetID, subnetID) }, } b.mu.Lock() // Re-check under lock in case a concurrent bind won the race. - if _, ok := b.active[vpcID]; ok { + if _, ok := b.active[subnetID]; ok { b.mu.Unlock() _ = listener.Close() return nil } - b.active[vpcID] = &activeBinding{listener: listener, server: server} + b.active[subnetID] = &activeBinding{listener: listener, server: server} b.mu.Unlock() go func() { if err := server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) { - slog.Error("IMDS: listener serve exited", "vpc_id", vpcID, "host_end", hostEnd, "err", err) + slog.Error("IMDS: listener serve exited", "subnet_id", subnetID, "host_end", hostEnd, "err", err) } }() - slog.Info("IMDS: listener bound", "vpc_id", vpcID, "netns", netnsName, "host_end", hostEnd, "addr", listener.Addr().String()) + slog.Info("IMDS: listener bound", "subnet_id", subnetID, "vpc_id", vpcID, "netns", netnsName, "host_end", hostEnd, "addr", listener.Addr().String()) return nil } -// unbind closes the VPC's listener and removes its host veth. Idempotent. -func (b *bindManager) unbind(ctx context.Context, vpcID string) { +// unbind closes the subnet's listener and removes its host veth. Idempotent. +func (b *bindManager) unbind(ctx context.Context, subnetID string) { b.mu.Lock() - binding := b.active[vpcID] - delete(b.active, vpcID) + binding := b.active[subnetID] + delete(b.active, subnetID) b.mu.Unlock() if binding != nil { if err := binding.server.Close(); err != nil { - slog.Warn("IMDS: listener close failed", "vpc_id", vpcID, "err", err) + slog.Warn("IMDS: listener close failed", "subnet_id", subnetID, "err", err) } } - if err := b.removeVeth(ctx, vpcID); err != nil { - slog.Warn("IMDS: veth removal failed", "vpc_id", vpcID, "err", err) + if err := b.removeVeth(ctx, subnetID); err != nil { + slog.Warn("IMDS: veth removal failed", "subnet_id", subnetID, "err", err) } - slog.Info("IMDS: listener unbound", "vpc_id", vpcID) + slog.Info("IMDS: listener unbound", "subnet_id", subnetID) } // shutdown closes every active listener. The host veths are intentionally left @@ -198,21 +240,22 @@ func (b *bindManager) unbind(ctx context.Context, vpcID string) { func (b *bindManager) shutdown() { b.mu.Lock() defer b.mu.Unlock() - for vpcID, binding := range b.active { + for subnetID, binding := range b.active { if err := binding.server.Close(); err != nil { - slog.Warn("IMDS: listener close failed during shutdown", "vpc_id", vpcID, "err", err) + slog.Warn("IMDS: listener close failed during shutdown", "subnet_id", subnetID, "err", err) } - delete(b.active, vpcID) + delete(b.active, subnetID) } } -// bindLocalListener opens a TCP listener on 169.254.169.254:80 inside the VPC's -// netns, where the host-end veth carries 169.254.169.254/30 with a default route -// via the .253 LRP — so the host-served reply has a real L3 next-hop back to the -// guest (SO_BINDTODEVICE alone in the root netns could not route the SYN-ACK). -// The netns also isolates VPCs whose CIDRs overlap, since each is its own routing -// domain. The socket must be *created* in the netns, so we enter it on a locked -// thread for the socket call and restore the root netns before serving. +// bindLocalListener opens a TCP listener on 169.254.169.254:80 inside the subnet's +// netns, where the host-end veth carries 169.254.169.254/30 with the subnet CIDR +// on-link — so the host-served reply resolves the guest by ARP over the localport +// on the guest's own L2 switch (SO_BINDTODEVICE alone in the root netns could not +// route the SYN-ACK). The netns also isolates subnets whose CIDRs overlap, since +// each is its own routing domain. The socket must be *created* in the netns, so we +// enter it on a locked thread for the socket call and restore the root netns before +// serving. // SO_BINDTODEVICE (belt-and-braces — there is only one veth in the netns) and // SO_REUSEADDR (fast rebind on vpcd restart) are still set on the fd. func bindLocalListener(ctx context.Context, netnsName, hostEnd string) (net.Listener, error) { diff --git a/spinifex/handlers/imds/server_test.go b/spinifex/handlers/imds/server_test.go index 59acaea75..5fd500999 100644 --- a/spinifex/handlers/imds/server_test.go +++ b/spinifex/handlers/imds/server_test.go @@ -5,6 +5,7 @@ import ( "io" "net" "net/http" + "net/netip" "sync" "sync/atomic" "testing" @@ -32,33 +33,44 @@ func loopbackListen(addrs *sync.Map) listenFunc { } func TestBindManager_BindServesWithVPCContext(t *testing.T) { - const hostEnd = "imds-h-abc12345" + const ( + hostEnd = "imds-h-abc12345" + subnetID = "subnet-abc12345" + vpcID = "vpc-abc12345" + ) var ensured, removed atomic.Int32 - ensure := func(_ context.Context, _ string) (string, string, error) { + var ensuredKey, removedKey atomic.Value + ensure := func(_ context.Context, key string, _ netip.Prefix) (string, string, error) { ensured.Add(1) + ensuredKey.Store(key) return "", hostEnd, nil } - remove := func(_ context.Context, _ string) error { + remove := func(_ context.Context, key string) error { removed.Add(1) + removedKey.Store(key) return nil } - // Echo the VPC ID the bind manager threaded into the request context — this - // is the (VPC-ID, source-IP) → identity path the security model relies on. + // Echo the VPC + subnet the bind manager threaded into the request context. + // The VPC keys the eni-by-vpc-ip index — the (VPC-ID, source-IP) → identity + // path the security model relies on; the subnet rides along for triage. handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - vpcID, _ := r.Context().Value(ctxKeyVPCID).(string) - _, _ = io.WriteString(w, vpcID) + ctxVPC, _ := r.Context().Value(ctxKeyVPCID).(string) + ctxSubnet, _ := r.Context().Value(ctxKeySubnetID).(string) + _, _ = io.WriteString(w, ctxVPC+"|"+ctxSubnet) }) var addrs sync.Map bm := newBindManager(nil, handler, ensure, remove, loopbackListen(&addrs)) ctx := context.Background() + cidr := netip.MustParsePrefix("10.211.0.0/16") - require.NoError(t, bm.bind(ctx, "vpc-abc12345")) + require.NoError(t, bm.bind(ctx, subnetID, vpcID, cidr)) assert.Equal(t, int32(1), ensured.Load()) + assert.Equal(t, subnetID, ensuredKey.Load(), "veth lifecycle is keyed by subnet") - // Second bind of the same VPC is a no-op (idempotent), no extra veth ensure. - require.NoError(t, bm.bind(ctx, "vpc-abc12345")) + // Second bind of the same key is a no-op (idempotent), no extra veth ensure. + require.NoError(t, bm.bind(ctx, subnetID, vpcID, cidr)) assert.Equal(t, int32(1), ensured.Load()) raw, ok := addrs.Load(hostEnd) @@ -70,11 +82,12 @@ func TestBindManager_BindServesWithVPCContext(t *testing.T) { require.NoError(t, err) body, _ := io.ReadAll(resp.Body) _ = resp.Body.Close() - assert.Equal(t, "vpc-abc12345", string(body), "handler must see the listener's VPC ID") + assert.Equal(t, vpcID+"|"+subnetID, string(body), "handler must see the listener's VPC and subnet") - // Unbind closes the listener and tears down the veth. - bm.unbind(ctx, "vpc-abc12345") + // Unbind closes the listener and tears down the veth, keyed by subnet. + bm.unbind(ctx, subnetID) assert.Equal(t, int32(1), removed.Load()) + assert.Equal(t, subnetID, removedKey.Load(), "veth removal is keyed by subnet") client := http.Client{Timeout: 500 * time.Millisecond} _, err = client.Get("http://" + addr + prefixMetaData) @@ -82,41 +95,45 @@ func TestBindManager_BindServesWithVPCContext(t *testing.T) { } func TestBindManager_BindPropagatesVethError(t *testing.T) { - ensure := func(_ context.Context, _ string) (string, string, error) { + ensure := func(_ context.Context, _ string, _ netip.Prefix) (string, string, error) { return "", "", errEnsureFailed } bm := newBindManager(nil, http.NewServeMux(), ensure, func(context.Context, string) error { return nil }, loopbackListen(&sync.Map{})) - err := bm.bind(context.Background(), "vpc-x") + err := bm.bind(context.Background(), "subnet-x", "vpc-x", netip.MustParsePrefix("10.211.0.0/16")) require.Error(t, err) } var errEnsureFailed = io.ErrUnexpectedEOF // TestBindManager_SyncSkipsVersionKey guards that the schema-version marker -// migrate.RunKV stamps into the vpc-veth bucket is not mistaken for a VPC ID -// and made to bring up a bogus veth + listener. +// migrate.RunKV stamps into the subnet-veth bucket is not mistaken for a subnet +// ID and made to bring up a bogus veth + listener. func TestBindManager_SyncSkipsVersionKey(t *testing.T) { _, _, js := testutil.StartTestJetStream(t) - vpcVeth, _, err := InitBuckets(js, 1) + subnetVeth, _, err := InitBuckets(js, 1) require.NoError(t, err) - // InitBuckets → migrate.RunKV stamps utils.VersionKey; add one real VPC too. - _, err = vpcVeth.PutString("vpc-abcdef12", "{}") - require.NoError(t, err) + // InitBuckets → migrate.RunKV stamps utils.VersionKey; add one real subnet + // record (sync resolves its CIDR from the record before binding). + require.NoError(t, NewVethStore(subnetVeth).Put(SubnetVethRecord{ + SubnetID: "subnet-abcdef12", + VPCID: "vpc-abcdef12", + SubnetCIDR: "10.211.0.0/16", + })) var bound sync.Map - ensure := func(_ context.Context, vpcID string) (string, string, error) { - bound.Store(vpcID, true) - return "imds-" + vpcID, "imds-h-" + vpcID, nil + ensure := func(_ context.Context, subnetID string, _ netip.Prefix) (string, string, error) { + bound.Store(subnetID, true) + return "imds-" + subnetID, "imds-h-" + subnetID, nil } remove := func(context.Context, string) error { return nil } - bm := newBindManager(vpcVeth, http.NewServeMux(), ensure, remove, loopbackListen(&sync.Map{})) + bm := newBindManager(subnetVeth, http.NewServeMux(), ensure, remove, loopbackListen(&sync.Map{})) require.NoError(t, bm.sync(context.Background())) defer bm.shutdown() _, versionBound := bound.Load(utils.VersionKey) - assert.False(t, versionBound, "_version marker must not be bound as a VPC") - _, vpcBound := bound.Load("vpc-abcdef12") - assert.True(t, vpcBound, "real VPC entry must be bound") + assert.False(t, versionBound, "_version marker must not be bound as a subnet") + _, subnetBound := bound.Load("subnet-abcdef12") + assert.True(t, subnetBound, "real subnet entry must be bound") } diff --git a/spinifex/handlers/imds/service.go b/spinifex/handlers/imds/service.go index 417111e3a..0f3c6df92 100644 --- a/spinifex/handlers/imds/service.go +++ b/spinifex/handlers/imds/service.go @@ -26,7 +26,7 @@ type profileLookup interface { // IMDSService is the host-served EC2 Instance Metadata Service. It runs in // vpcd on every chassis, serving 169.254.169.254 to local guest VMs over -// per-VPC veths. Run owns the listener lifecycle and blocks until ctx is done. +// per-subnet veths. Run owns the listener lifecycle and blocks until ctx is done. type IMDSService interface { Run(ctx context.Context) error } @@ -83,9 +83,9 @@ func NewIMDSServiceImpl(natsConn *nats.Conn, sts stsAssumer, iamSvc profileLooku if err != nil { return nil, fmt.Errorf("open %s bucket: %w", kvBucketENIs, err) } - vethKV, err := js.KeyValue(KVBucketIMDSVPCVeth) + vethKV, err := js.KeyValue(KVBucketIMDSSubnetVeth) if err != nil { - return nil, fmt.Errorf("open %s bucket: %w", KVBucketIMDSVPCVeth, err) + return nil, fmt.Errorf("open %s bucket: %w", KVBucketIMDSSubnetVeth, err) } // The SG bucket is owned by the VPC service and only feeds the non-critical @@ -113,7 +113,7 @@ func NewIMDSServiceImpl(natsConn *nats.Conn, sts stsAssumer, iamSvc profileLooku slog.Info("IMDS service initialized", "index_bucket", KVBucketENIByVPCIP, - "veth_bucket", KVBucketIMDSVPCVeth) + "veth_bucket", KVBucketIMDSSubnetVeth) return svc, nil } diff --git a/spinifex/handlers/imds/veth_store.go b/spinifex/handlers/imds/veth_store.go index 353ba7647..4e574457c 100644 --- a/spinifex/handlers/imds/veth_store.go +++ b/spinifex/handlers/imds/veth_store.go @@ -8,65 +8,68 @@ import ( "github.com/nats-io/nats.go" ) -// VPCVethRecord is the persisted per-VPC IMDS plumbing record (KVBucketIMDSVPCVeth, -// keyed by VPC ID): the signal that a VPC's IMDS OVN topology is installed, replayed -// by each chassis's BindManager. It carries only the LSP MAC and LRP /30 the host can't re-derive. -type VPCVethRecord struct { - VPCID string `json:"vpc_id"` - ShortVPCID string `json:"short_vpc_id"` - IMDSPortMAC string `json:"imds_port_mac"` - LRPNetwork string `json:"lrp_network"` - CreatedAt string `json:"created_at"` +// SubnetVethRecord is the persisted per-subnet IMDS plumbing record +// (KVBucketIMDSSubnetVeth, keyed by subnet ID): the signal that a subnet's IMDS +// localport is installed, replayed by each chassis's BindManager. It carries the +// LSP MAC and the subnet CIDR the host can't re-derive, plus the owning VPC for +// the subnet→VPC reverse lookup. +type SubnetVethRecord struct { + SubnetID string `json:"subnet_id"` + ShortSubnetID string `json:"short_subnet_id"` + VPCID string `json:"vpc_id"` + IMDSPortMAC string `json:"imds_port_mac"` + SubnetCIDR string `json:"subnet_cidr"` + CreatedAt string `json:"created_at"` } -// VethStore is the persistence surface for VPCVethRecord rows in -// KVBucketIMDSVPCVeth. Backed by a NATS JetStream KV bucket opened by +// VethStore is the persistence surface for SubnetVethRecord rows in +// KVBucketIMDSSubnetVeth. Backed by a NATS JetStream KV bucket opened by // InitBuckets in production; tests inject the handle via NewVethStore. type VethStore struct { kv nats.KeyValue } -// NewVethStore wraps an already-opened vpc-veth KV bucket. +// NewVethStore wraps an already-opened subnet-veth KV bucket. func NewVethStore(kv nats.KeyValue) *VethStore { return &VethStore{kv: kv} } -// Get returns the record for vpcID, or (nil, nil) when absent. -func (s *VethStore) Get(vpcID string) (*VPCVethRecord, error) { - raw, err := s.kv.Get(vpcID) +// Get returns the record for subnetID, or (nil, nil) when absent. +func (s *VethStore) Get(subnetID string) (*SubnetVethRecord, error) { + raw, err := s.kv.Get(subnetID) if err != nil { if errors.Is(err, nats.ErrKeyNotFound) { return nil, nil } - return nil, fmt.Errorf("get imds veth record %q: %w", vpcID, err) + return nil, fmt.Errorf("get imds veth record %q: %w", subnetID, err) } - var rec VPCVethRecord + var rec SubnetVethRecord if err := json.Unmarshal(raw.Value(), &rec); err != nil { - return nil, fmt.Errorf("unmarshal imds veth record %q: %w", vpcID, err) + return nil, fmt.Errorf("unmarshal imds veth record %q: %w", subnetID, err) } return &rec, nil } -// Put writes the record keyed by its VPCID, overwriting any prior entry. -func (s *VethStore) Put(rec VPCVethRecord) error { - if rec.VPCID == "" { - return errors.New("imds veth store put: vpc_id is empty") +// Put writes the record keyed by its SubnetID, overwriting any prior entry. +func (s *VethStore) Put(rec SubnetVethRecord) error { + if rec.SubnetID == "" { + return errors.New("imds veth store put: subnet_id is empty") } data, err := json.Marshal(rec) if err != nil { return fmt.Errorf("marshal imds veth record: %w", err) } - if _, err := s.kv.Put(rec.VPCID, data); err != nil { - return fmt.Errorf("put imds veth record %q: %w", rec.VPCID, err) + if _, err := s.kv.Put(rec.SubnetID, data); err != nil { + return fmt.Errorf("put imds veth record %q: %w", rec.SubnetID, err) } return nil } // Delete removes the record. Idempotent: a missing key returns nil. -func (s *VethStore) Delete(vpcID string) error { - if err := s.kv.Delete(vpcID); err != nil { +func (s *VethStore) Delete(subnetID string) error { + if err := s.kv.Delete(subnetID); err != nil { if errors.Is(err, nats.ErrKeyNotFound) { return nil } - return fmt.Errorf("delete imds veth record %q: %w", vpcID, err) + return fmt.Errorf("delete imds veth record %q: %w", subnetID, err) } return nil } diff --git a/spinifex/handlers/imds/veth_store_test.go b/spinifex/handlers/imds/veth_store_test.go index b0b41ae93..65fad3404 100644 --- a/spinifex/handlers/imds/veth_store_test.go +++ b/spinifex/handlers/imds/veth_store_test.go @@ -11,23 +11,24 @@ import ( func newTestVethStore(t *testing.T) *VethStore { t.Helper() _, _, js := testutil.StartTestJetStream(t) - vpcVeth, _, err := InitBuckets(js, 1) + subnetVeth, _, err := InitBuckets(js, 1) require.NoError(t, err) - return NewVethStore(vpcVeth) + return NewVethStore(subnetVeth) } func TestVethStore_PutGetRoundTrip(t *testing.T) { s := newTestVethStore(t) - rec := VPCVethRecord{ - VPCID: "vpc-abc12345", - ShortVPCID: "abc12345", - IMDSPortMAC: "02:aa:bb:cc:dd:ee", - LRPNetwork: "169.254.169.253/30", - CreatedAt: "2026-05-29T00:00:00Z", + rec := SubnetVethRecord{ + SubnetID: "subnet-abc12345", + ShortSubnetID: "abc12345", + VPCID: "vpc-abc12345", + IMDSPortMAC: "02:aa:bb:cc:dd:ee", + SubnetCIDR: "10.0.1.0/24", + CreatedAt: "2026-05-29T00:00:00Z", } require.NoError(t, s.Put(rec)) - got, err := s.Get("vpc-abc12345") + got, err := s.Get("subnet-abc12345") require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, rec, *got) @@ -35,23 +36,23 @@ func TestVethStore_PutGetRoundTrip(t *testing.T) { func TestVethStore_GetAbsentReturnsNil(t *testing.T) { s := newTestVethStore(t) - got, err := s.Get("vpc-nope") + got, err := s.Get("subnet-nope") require.NoError(t, err) assert.Nil(t, got) } -func TestVethStore_PutRejectsEmptyVPCID(t *testing.T) { +func TestVethStore_PutRejectsEmptySubnetID(t *testing.T) { s := newTestVethStore(t) - require.Error(t, s.Put(VPCVethRecord{ShortVPCID: "abc12345"})) + require.Error(t, s.Put(SubnetVethRecord{ShortSubnetID: "abc12345"})) } func TestVethStore_DeleteIsIdempotent(t *testing.T) { s := newTestVethStore(t) - require.NoError(t, s.Delete("vpc-missing")) + require.NoError(t, s.Delete("subnet-missing")) - require.NoError(t, s.Put(VPCVethRecord{VPCID: "vpc-1"})) - require.NoError(t, s.Delete("vpc-1")) - got, err := s.Get("vpc-1") + require.NoError(t, s.Put(SubnetVethRecord{SubnetID: "subnet-1"})) + require.NoError(t, s.Delete("subnet-1")) + got, err := s.Get("subnet-1") require.NoError(t, err) assert.Nil(t, got) } diff --git a/spinifex/network/external/igw.go b/spinifex/network/external/igw.go index 63473523f..62075025b 100644 --- a/spinifex/network/external/igw.go +++ b/spinifex/network/external/igw.go @@ -428,8 +428,8 @@ func (m *igwManager) RemoveSystemInstanceEgress(ctx context.Context, vpcID, subn } // 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. +// link-local (DHCP and friends — traffic that should never egress the gateway) +// 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") @@ -450,13 +450,11 @@ func (m *igwManager) dropExcludeCIDRs(ctx context.Context, vpcID string) []netip // rerouteExcludeCIDRs returns the exclusion list for a per-subnet egress // reroute policy: VPC CIDR (if discoverable) plus link-local. The reroute -// matches 0.0.0.0/0 — the widest possible scope — so it would otherwise -// hijack 169.254.169.254 (IMDS) out the gateway and defeat the per-VPC IMDS -// /32 static route, which fires earlier in lr_in_ip_routing but is overridden -// by this lr_in_policy reroute. Excluding link-local lets IMDS fall through to -// the /32. Multicast is not excluded here (unlike the drop policy): a reroute -// only diverts traffic that already has somewhere to go, so leaking multicast -// to the gateway is harmless, whereas the drop policy would kill it outright. +// matches 0.0.0.0/0 — the widest possible scope — so it would otherwise divert +// link-local (169.254.0.0/16) out the gateway, where it has no business going. +// Multicast is not excluded here (unlike the drop policy): a reroute only +// diverts traffic that already has somewhere to go, so leaking multicast to the +// gateway is harmless, whereas the drop policy would kill it outright. func (m *igwManager) rerouteExcludeCIDRs(ctx context.Context, vpcID string) []netip.Prefix { excludes := m.vpcExcludeCIDRs(ctx, vpcID) return append(excludes, linkLocalCIDR) diff --git a/spinifex/network/external/imds_datapath_contract_test.go b/spinifex/network/external/imds_datapath_contract_test.go index 76b33d7ad..d126cfdb1 100644 --- a/spinifex/network/external/imds_datapath_contract_test.go +++ b/spinifex/network/external/imds_datapath_contract_test.go @@ -16,36 +16,31 @@ import ( "github.com/mulgadc/spinifex/spinifex/network/topology" ) -// IMDS datapath contract — desk-speed pipeline oracle. +// IMDS datapath contract — desk-speed oracle. // -// Every IMDS datapath bug to date (the egress-reroute hijack of the /32, the -// reply-path break) lived inside the VPC logical-router pipeline and was only -// caught by a full VM-boot E2E run, one bug per deploy. This test makes the -// load-bearing half of that pipeline assertable in `go test`: it builds the -// REAL managers (IGW egress reroute + drop, IMDS topology) against the OVN mock -// for two VPCs that share the *identical* CIDR — the overlapping-CIDR case that -// is the whole point of per-VPC IMDS identity — then runs a faithful, minimal -// simulation of OVN's two ingress stages (lr_in_policy, then lr_in_ip_routing) -// and asserts a guest packet to 169.254.169.254 reaches the IMDS LRP rather -// than being rerouted out the WAN or dropped. +// Every IMDS datapath bug in the v1 grind (the egress-reroute hijack of the +// /32, the multi-hop reply break) lived inside the VPC logical-router pipeline. +// The L2 datapath takes IMDS off the router entirely: the localport that claims +// 169.254.169.254 sits directly on the guest's subnet switch, so the request is +// answered over a single L2 hop and never enters lr_in_*. This test asserts that +// structural property — the localport is on the subnet LS and NO IMDS object +// (LRP, /32 route, dedicated switch) exists on the VPC router — for two VPCs that +// share the *identical* CIDR (the overlapping-CIDR case per-subnet identity is +// built for). // -// A true round-trip oracle is ovn-trace against a live SB DB; that belongs in -// the e2e harness. This is its desk-speed counterpart over the same NB rows — -// it runs on every `make preflight`, so a future broad lr_in_policy rule that -// forgets to spare link-local fails here instead of in a deploy. +// The LR-pipeline simulator below is retained for TestIMDSDatapathContract_OracleHasTeeth, +// which proves a broad lr_in_policy reroute WOULD have hijacked IMDS had it ever +// been routed — documenting exactly why moving IMDS to L2 removes the bug class. -const ( - imdsContractGuestIP = "10.211.0.5" - imdsContractCIDR = "10.211.0.0/16" -) +const imdsContractCIDR = "10.211.0.0/16" -func TestIMDSDatapathContract_SurvivesVPCPipeline_OverlappingCIDRs(t *testing.T) { +func TestIMDSDatapathContract_AnsweredAtL2_OverlappingCIDRs(t *testing.T) { ctx := context.Background() m := mock.New() - // Two VPCs on the IDENTICAL CIDR, each with internet egress and a subnet - // whose first guest gets the same private IP. The OVN NB rows for the two - // must each independently route IMDS correctly. + // Two VPCs on the IDENTICAL CIDR, each with internet egress (the broad + // 0.0.0.0/0 reroute + drop that historically threatened IMDS) and a subnet + // switch carrying the IMDS localport. vpcs := []struct{ vpcID, subnetID string }{ {"vpc-aaaa1111", "subnet-aaaa1111"}, {"vpc-bbbb2222", "subnet-bbbb2222"}, @@ -60,25 +55,27 @@ func TestIMDSDatapathContract_SurvivesVPCPipeline_OverlappingCIDRs(t *testing.T) for _, v := range vpcs { seedVPCRouter(t, m, v.vpcID, imdsContractCIDR) + seedSubnetSwitch(t, m, v.subnetID) require.NoError(t, igwMgr.AttachIGW(ctx, IGWSpec{VPCID: v.vpcID, InternetGatewayID: "igw-" + v.vpcID})) // The broad 0.0.0.0/0 egress reroute + drop — the pipeline stages that - // previously hijacked / killed the IMDS /32 before #19 excluded link-local. + // hijacked / killed the IMDS /32 in v1 before #19 excluded link-local. require.NoError(t, igwMgr.EnsureSubnetEgress(ctx, v.vpcID, v.subnetID, defaultRoute)) require.NoError(t, igwMgr.EnsureSubnetEgressDrop(ctx, v.vpcID, v.subnetID, defaultRoute)) - // The IMDS /32 static route + imds-lrp. - _, err := imdsMgr.EnsureForVPC(ctx, v.vpcID) + // The IMDS localport on the subnet switch. + _, err := imdsMgr.EnsureForSubnet(ctx, v.subnetID, v.vpcID, netip.MustParsePrefix(imdsContractCIDR)) require.NoError(t, err) } for _, v := range vpcs { - assertIMDSDatapathContract(t, m, v.vpcID, v.subnetID, imdsContractGuestIP) + assertIMDSAnsweredAtL2(t, m, v.vpcID, v.subnetID) } } -// TestIMDSDatapathContract_OracleHasTeeth proves the simulator actually detects -// the regression it guards against: a 0.0.0.0/0 egress reroute that does NOT -// exclude link-local (the pre-#19 bug) must be reported as diverting IMDS. -// Without this, a vacuously-passing contract test would give false confidence. +// TestIMDSDatapathContract_OracleHasTeeth proves the LR-pipeline simulator +// actually detects the regression it documents: a 0.0.0.0/0 egress reroute that +// does NOT exclude link-local (the pre-#19 bug) must be reported as diverting +// IMDS. Under L2 this can no longer reach IMDS — but the simulator keeps the +// historical hazard legible and the oracle non-vacuous. func TestIMDSDatapathContract_OracleHasTeeth(t *testing.T) { ctx := context.Background() m := mock.New() @@ -89,7 +86,7 @@ func TestIMDSDatapathContract_OracleHasTeeth(t *testing.T) { gw := "169.254.0.2" // Reroute excluding ONLY the VPC CIDR — link-local omitted, reproducing the // bug #19 fixed. IMDS (169.254.169.254) is in 0.0.0.0/0 and not in the VPC - // CIDR, so this policy hijacks it. + // CIDR, so this policy would hijack it had IMDS ever been routed. require.NoError(t, rm.AddSubnetEgress(ctx, vpcID, policy.SubnetEgressSpec{ SubnetID: subnetID, Prefix: netip.MustParsePrefix("0.0.0.0/0"), @@ -104,37 +101,34 @@ func TestIMDSDatapathContract_OracleHasTeeth(t *testing.T) { "simulator must detect a link-local-unaware reroute as hijacking IMDS; if this fails the oracle is blind") } -// assertIMDSDatapathContract is the contract: a guest packet to -// 169.254.169.254 must reach the IMDS LRP, and the IMDS reply must not be -// caught by any egress policy. -func assertIMDSDatapathContract(t *testing.T, m *mock.Client, vpcID, subnetID, guestIP string) { +// assertIMDSAnsweredAtL2 is the contract: 169.254.169.254 is claimed by a +// localport on the guest's subnet switch (so it is answered over one L2 hop), +// and NO IMDS object exists on the VPC router (so the LR pipeline cannot shadow +// it — the entire v1 bug class is structurally impossible). +func assertIMDSAnsweredAtL2(t *testing.T, m *mock.Client, vpcID, subnetID string) { t.Helper() - imdsLRP := topology.IMDSRouterPort(vpcID) + spec := IMDSSpecForSubnet(subnetID) - // Request: guest -> 169.254.169.254, ingressing on the subnet LRP. - req := resolveEgress(t, m, vpcID, subnetID, handlers_imds.MetaDataServerIP) - assert.Falsef(t, req.diverted, - "IMDS contract (%s): an lr_in_policy reroute on 0.0.0.0/0 hijacks 169.254.169.254 out the WAN; "+ - "every broad egress reroute MUST exclude 169.254.0.0/16 (see decision #19)", vpcID) - assert.Falsef(t, req.dropped, - "IMDS contract (%s): an lr_in_policy drop kills 169.254.169.254; "+ - "the per-subnet drop policy MUST exclude 169.254.0.0/16", vpcID) - assert.Equalf(t, "169.254.169.254/32", req.routePrefix, - "IMDS contract (%s): 169.254.169.254 must be resolved by the /32 IMDS static route, not a broader route", vpcID) - assert.Equalf(t, imdsLRP, req.outputPort, - "IMDS contract (%s): the IMDS /32 must egress via %s", vpcID, imdsLRP) + lsp, err := m.GetLogicalSwitchPort(context.Background(), spec.LSPName) + require.NoErrorf(t, err, "IMDS contract (%s): no localport on the subnet switch %s", vpcID, spec.LSName) + assert.Equalf(t, "localport", lsp.Type, + "IMDS contract (%s): the IMDS port must be a localport so every chassis self-serves it over L2", vpcID) + assert.Emptyf(t, lsp.PortSecurity, + "IMDS contract (%s): the IMDS localport must carry no port_security (the host sources the frames)", vpcID) + assert.Equalf(t, []string{spec.LSPMAC + " " + handlers_imds.MetaDataServerIP}, lsp.Addresses, + "IMDS contract (%s): the localport must claim %s on the subnet switch", vpcID, handlers_imds.MetaDataServerIP) - // Reply: 169.254.169.254 -> guest, ingressing on the IMDS LRP. The egress - // policies are inport-scoped to the subnet LRP, so none may catch the reply; - // a future inport-less broad policy would break IMDS replies and fail here. - reply := resolveEgressFrom(t, m, vpcID, imdsLRP, guestIP) - assert.Falsef(t, reply.diverted, - "IMDS contract (%s): an lr_in_policy must not divert the IMDS reply to %s (policies must stay inport-scoped)", vpcID, guestIP) - assert.Falsef(t, reply.dropped, - "IMDS contract (%s): an lr_in_policy must not drop the IMDS reply to %s", vpcID, guestIP) + // No IMDS object on the router: the request never enters lr_in_*. + assert.Nilf(t, findIMDSRoute(m, handlers_imds.MetaDataServerIP+"/32"), + "IMDS contract (%s): no %s/32 static route may exist — IMDS must not transit the VPC router", + vpcID, handlers_imds.MetaDataServerIP) + for name := range m.RouterPorts { + assert.Falsef(t, strings.HasPrefix(name, "imds-"), + "IMDS contract (%s): found IMDS LRP %q — IMDS must not transit the VPC router", vpcID, name) + } } -// --- minimal OVN LR ingress-pipeline model ------------------------------------ +// --- minimal OVN LR ingress-pipeline model (used by OracleHasTeeth) ----------- type egressResult struct { diverted bool // a reroute lr_in_policy matched diff --git a/spinifex/network/external/imds_topology.go b/spinifex/network/external/imds_topology.go index 2a8ba646a..7d65af0a1 100644 --- a/spinifex/network/external/imds_topology.go +++ b/spinifex/network/external/imds_topology.go @@ -12,135 +12,102 @@ import ( "github.com/mulgadc/spinifex/spinifex/network/host" "github.com/mulgadc/spinifex/spinifex/network/ovn" "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/utils" ) -// imdsLRPNetwork is the /30 the IMDS LRP terminates on the VPC router. .253 is -// the LRP, .254 is the host-owned IMDS LSP (MetaDataServerIP); the /30 makes -// the route to .254 directly-connected on the IMDS switch. -const imdsLRPNetwork = "169.254.169.253/30" - -// metaDataServerRoute is the /32 the VPC router uses to send IMDS traffic out -// the IMDS LRP instead of the WAN default. -var metaDataServerRoute = netip.PrefixFrom(netip.MustParseAddr(handlers_imds.MetaDataServerIP), 32) - -// IMDSVPCSpec is the resolved set of OVN + host names for a VPC's IMDS -// plumbing. Every field is a deterministic function of the VPC ID, so callers -// can rebuild it from the VPC ID alone (BindManager does this per bucket entry). -type IMDSVPCSpec struct { - VPCID string - ShortVPCID string // last 8 chars of VPCID - LSName string // imds-ls-{vpcID} - LRPName string // imds-lrp-{vpcID} - RouterLSPName string // imds-rtr-{vpcID} (type=router) - LSPName string // imds-port-{vpcID} (localport; host veth binds here) - LSPMAC string // 02:.. — HashMAC("imds-"+vpcID) - LRPNetwork string // imdsLRPNetwork - OVSEndName string // imds-o-{shortVpcID} - HostEndName string // imds-h-{shortVpcID} +// IMDSSubnetSpec is the resolved set of OVN + host names for a subnet's IMDS +// localport. Every field is a deterministic function of the subnet ID, so +// callers can rebuild it from the subnet ID alone (BindManager does this per +// bucket entry). +type IMDSSubnetSpec struct { + SubnetID string + ShortSubnetID string // last 8 chars of SubnetID + LSName string // subnet-{subnetID} — the guest's own switch + LSPName string // imds-port-{subnetID} (localport; host veth binds here) + LSPMAC string // 02:.. — HashMAC("imds-"+subnetID) + OVSEndName string // imds-o-{shortSubnetID} + HostEndName string // imds-h-{shortSubnetID} } -// IMDSTopologyManager installs and removes the per-VPC OVN topology that makes -// 169.254.169.254 routable to the host-owned IMDS localport. Idempotent. Lives in -// external (not policy): it composes L2 creation with L3 routing (ADR-0006 S5). +// IMDSTopologyManager installs and removes the per-subnet localport that claims +// 169.254.169.254 directly on the guest's subnet switch, so metadata is reached +// over a single L2 hop on the guest's own broadcast domain — no router in the +// path. Idempotent. type IMDSTopologyManager interface { - EnsureForVPC(ctx context.Context, vpcID string) (IMDSVPCSpec, error) - RemoveForVPC(ctx context.Context, vpcID string) error + EnsureForSubnet(ctx context.Context, subnetID, vpcID string, cidr netip.Prefix) (IMDSSubnetSpec, error) + RemoveForSubnet(ctx context.Context, subnetID string) error } type imdsTopologyManager struct { - ovn ovn.Client - routes policy.RouteManager - store *handlers_imds.VethStore + ovn ovn.Client + store *handlers_imds.VethStore } var _ IMDSTopologyManager = (*imdsTopologyManager)(nil) // NewIMDSTopologyManager constructs an IMDSTopologyManager. store is the -// vpc-veth KV bucket the installer publishes records to. -func NewIMDSTopologyManager(client ovn.Client, routes policy.RouteManager, store *handlers_imds.VethStore) (IMDSTopologyManager, error) { +// subnet-veth KV bucket the installer publishes records to. +func NewIMDSTopologyManager(client ovn.Client, store *handlers_imds.VethStore) (IMDSTopologyManager, error) { switch { case client == nil: return nil, errors.New("IMDSTopologyManager: OVN client required") - case routes == nil: - return nil, errors.New("IMDSTopologyManager: RouteManager required") case store == nil: return nil, errors.New("IMDSTopologyManager: VethStore required") } - return &imdsTopologyManager{ovn: client, routes: routes, store: store}, nil + return &imdsTopologyManager{ovn: client, store: store}, nil } -// IMDSSpecForVPC returns the deterministic name/MAC set for a VPC's IMDS -// plumbing without touching OVN or the bucket. -func IMDSSpecForVPC(vpcID string) IMDSVPCSpec { - return IMDSVPCSpec{ - VPCID: vpcID, - ShortVPCID: host.IMDSShortVPCID(vpcID), - LSName: topology.IMDSSwitch(vpcID), - LRPName: topology.IMDSRouterPort(vpcID), - RouterLSPName: topology.IMDSSwitchRouterPort(vpcID), - LSPName: topology.IMDSPort(vpcID), - LSPMAC: utils.HashMAC("imds-" + vpcID), - LRPNetwork: imdsLRPNetwork, - OVSEndName: host.IMDSOVSPortName(vpcID), - HostEndName: host.IMDSHostVethName(vpcID), +// IMDSSpecForSubnet returns the deterministic name/MAC set for a subnet's IMDS +// localport without touching OVN or the bucket. +func IMDSSpecForSubnet(subnetID string) IMDSSubnetSpec { + return IMDSSubnetSpec{ + SubnetID: subnetID, + ShortSubnetID: host.IMDSShortSubnetID(subnetID), + LSName: topology.SubnetSwitch(subnetID), + LSPName: topology.IMDSPort(subnetID), + LSPMAC: utils.HashMAC("imds-" + subnetID), + OVSEndName: host.IMDSOVSPortName(subnetID), + HostEndName: host.IMDSHostVethName(subnetID), } } -// EnsureForVPC installs the IMDS OVN topology for vpcID and publishes the vpc-veth -// record. Idempotent: a present record short-circuits, and each OVN object is created -// only when absent so a lost record still converges. The VPC router must already exist. -func (m *imdsTopologyManager) EnsureForVPC(ctx context.Context, vpcID string) (IMDSVPCSpec, error) { +// EnsureForSubnet installs the IMDS localport on subnet-{subnetID} and publishes +// the subnet-veth record. Idempotent: a present record short-circuits, and the +// localport is created only when absent so a lost record still converges. The +// subnet switch must already exist (the subnet lifecycle owns it). cidr is +// persisted in the record so the host reply path resolves the guest on-link; +// vpcID is persisted so the IMDS handler can resolve the subnet→VPC mapping the +// eni-by-vpc-ip index needs (the index stays keyed vpcID/ip). +func (m *imdsTopologyManager) EnsureForSubnet(ctx context.Context, subnetID, vpcID string, cidr netip.Prefix) (IMDSSubnetSpec, error) { + if subnetID == "" { + return IMDSSubnetSpec{}, errors.New("EnsureForSubnet: subnetID required") + } if vpcID == "" { - return IMDSVPCSpec{}, errors.New("EnsureForVPC: vpcID required") + return IMDSSubnetSpec{}, errors.New("EnsureForSubnet: vpcID required") } - spec := IMDSSpecForVPC(vpcID) + if !cidr.IsValid() { + return IMDSSubnetSpec{}, errors.New("EnsureForSubnet: cidr required") + } + spec := IMDSSpecForSubnet(subnetID) - // The bucket record is the publish gate: present means topology is + // The bucket record is the publish gate: present means the localport is // installed and BindManagers have (or will) materialise host state. - rec, err := m.store.Get(vpcID) + rec, err := m.store.Get(subnetID) if err != nil { - return IMDSVPCSpec{}, fmt.Errorf("read imds veth record for %s: %w", vpcID, err) + return IMDSSubnetSpec{}, fmt.Errorf("read imds veth record for %s: %w", subnetID, err) } if rec != nil { return spec, nil } - router := topology.VPCRouter(vpcID) - extIDs := map[string]string{"spinifex:vpc_id": vpcID, "spinifex:role": "imds"} + extIDs := map[string]string{"spinifex:subnet_id": subnetID, "spinifex:role": "imds"} - if _, err := m.ovn.EnsureLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: spec.LSName, ExternalIDs: extIDs}); err != nil { - return IMDSVPCSpec{}, fmt.Errorf("ensure imds switch %s: %w", spec.LSName, err) - } - - if _, err := m.ovn.GetLogicalRouterPort(ctx, spec.LRPName); err != nil { - if cErr := m.ovn.CreateLogicalRouterPort(ctx, router, &nbdb.LogicalRouterPort{ - Name: spec.LRPName, - MAC: utils.HashMAC(spec.LRPName), - Networks: []string{spec.LRPNetwork}, - ExternalIDs: extIDs, - }); cErr != nil { - return IMDSVPCSpec{}, fmt.Errorf("create imds router port %s: %w", spec.LRPName, cErr) - } - } - - if _, err := m.ovn.GetLogicalSwitchPort(ctx, spec.RouterLSPName); err != nil { - if cErr := m.ovn.CreateLogicalSwitchPort(ctx, spec.LSName, &nbdb.LogicalSwitchPort{ - Name: spec.RouterLSPName, - Type: "router", - Addresses: []string{"router"}, - Options: map[string]string{"router-port": spec.LRPName}, - ExternalIDs: extIDs, - }); cErr != nil { - return IMDSVPCSpec{}, fmt.Errorf("create imds router LSP %s: %w", spec.RouterLSPName, cErr) - } - } - - // Host-owned localport: no port_security (the host is trusted to source - // 169.254.169.254 frames, and port_security would make ovn-controller drop - // reply frames from the host veth's MAC). localport binds on every chassis. + // Host-owned localport on the guest's subnet switch: no port_security (the + // host sources 169.254.169.254 frames, and port_security would make + // ovn-controller drop reply frames from the host veth's MAC) and no SG + // membership (so the guest's SG ACLs never match it). localport binds on + // every chassis. if _, err := m.ovn.GetLogicalSwitchPort(ctx, spec.LSPName); err != nil { if cErr := m.ovn.CreateLogicalSwitchPort(ctx, spec.LSName, &nbdb.LogicalSwitchPort{ Name: spec.LSPName, @@ -148,62 +115,43 @@ func (m *imdsTopologyManager) EnsureForVPC(ctx context.Context, vpcID string) (I Addresses: []string{spec.LSPMAC + " " + handlers_imds.MetaDataServerIP}, ExternalIDs: extIDs, }); cErr != nil { - return IMDSVPCSpec{}, fmt.Errorf("create imds localport %s: %w", spec.LSPName, cErr) + return IMDSSubnetSpec{}, fmt.Errorf("create imds localport %s on %s: %w", spec.LSPName, spec.LSName, cErr) } } - if err := m.routes.AddStaticRoute(ctx, vpcID, policy.RouteSpec{ - Prefix: metaDataServerRoute, - Nexthop: handlers_imds.MetaDataServerIP, - OutputPort: spec.LRPName, + if err := m.store.Put(handlers_imds.SubnetVethRecord{ + SubnetID: subnetID, + ShortSubnetID: spec.ShortSubnetID, + VPCID: vpcID, + IMDSPortMAC: spec.LSPMAC, + SubnetCIDR: cidr.String(), + CreatedAt: time.Now().UTC().Format(time.RFC3339), }); err != nil { - return IMDSVPCSpec{}, fmt.Errorf("add imds static route on %s: %w", router, err) + return IMDSSubnetSpec{}, fmt.Errorf("publish imds veth record for %s: %w", subnetID, err) } - if err := m.store.Put(handlers_imds.VPCVethRecord{ - VPCID: vpcID, - ShortVPCID: spec.ShortVPCID, - IMDSPortMAC: spec.LSPMAC, - LRPNetwork: spec.LRPNetwork, - CreatedAt: time.Now().UTC().Format(time.RFC3339), - }); err != nil { - return IMDSVPCSpec{}, fmt.Errorf("publish imds veth record for %s: %w", vpcID, err) - } - - slog.Info("external: installed IMDS topology", - "vpc_id", vpcID, "imds_switch", spec.LSName, "imds_lrp", spec.LRPName, "imds_port", spec.LSPName) + slog.Info("external: installed IMDS localport", + "subnet_id", subnetID, "subnet_switch", spec.LSName, "imds_port", spec.LSPName) return spec, nil } -// RemoveForVPC tears down the IMDS OVN topology and the vpc-veth record. -// Idempotent and drift-tolerant: missing OVN objects are logged and skipped so the -// record delete always runs. Must run before the VPC router (it holds the LRP/route). -func (m *imdsTopologyManager) RemoveForVPC(ctx context.Context, vpcID string) error { - if vpcID == "" { - return errors.New("RemoveForVPC: vpcID required") +// RemoveForSubnet deletes the IMDS localport and the subnet-veth record. The +// subnet switch is left in place (the subnet lifecycle owns it). Idempotent and +// drift-tolerant: a missing localport is logged and skipped so the record delete +// always runs. Must run before the subnet switch goes away. +func (m *imdsTopologyManager) RemoveForSubnet(ctx context.Context, subnetID string) error { + if subnetID == "" { + return errors.New("RemoveForSubnet: subnetID required") } - spec := IMDSSpecForVPC(vpcID) - router := topology.VPCRouter(vpcID) + spec := IMDSSpecForSubnet(subnetID) - if err := m.routes.DeleteStaticRoute(ctx, vpcID, metaDataServerRoute); err != nil { - slog.Warn("external: delete imds static route failed", "vpc_id", vpcID, "err", err) - } if err := m.ovn.DeleteLogicalSwitchPort(ctx, spec.LSName, spec.LSPName); err != nil { slog.Warn("external: delete imds localport failed", "port", spec.LSPName, "err", err) } - if err := m.ovn.DeleteLogicalSwitchPort(ctx, spec.LSName, spec.RouterLSPName); err != nil { - slog.Warn("external: delete imds router LSP failed", "port", spec.RouterLSPName, "err", err) - } - if err := m.ovn.DeleteLogicalRouterPort(ctx, router, spec.LRPName); err != nil { - slog.Warn("external: delete imds router port failed", "port", spec.LRPName, "err", err) - } - if err := m.ovn.DeleteLogicalSwitch(ctx, spec.LSName); err != nil { - slog.Warn("external: delete imds switch failed", "switch", spec.LSName, "err", err) - } - if err := m.store.Delete(vpcID); err != nil { - return fmt.Errorf("delete imds veth record for %s: %w", vpcID, err) + if err := m.store.Delete(subnetID); err != nil { + return fmt.Errorf("delete imds veth record for %s: %w", subnetID, err) } - slog.Info("external: removed IMDS topology", "vpc_id", vpcID) + slog.Info("external: removed IMDS localport", "subnet_id", subnetID) return nil } diff --git a/spinifex/network/external/imds_topology_test.go b/spinifex/network/external/imds_topology_test.go index 475faf3a0..7cfeaa5ad 100644 --- a/spinifex/network/external/imds_topology_test.go +++ b/spinifex/network/external/imds_topology_test.go @@ -2,6 +2,8 @@ package external import ( "context" + "net/netip" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -10,21 +12,38 @@ import ( handlers_imds "github.com/mulgadc/spinifex/spinifex/handlers/imds" "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" ) +const ( + imdsTestSubnetID = "subnet-0a1b2c3d4e5f6789" + imdsTestVPCID = "vpc-0a1b2c3d4e5f6789" + imdsTestCIDR = "10.0.1.0/24" +) + func newIMDSManager(t *testing.T, m *mock.Client) (IMDSTopologyManager, *handlers_imds.VethStore) { t.Helper() _, _, js := testutil.StartTestJetStream(t) - vpcVeth, _, err := handlers_imds.InitBuckets(js, 1) + subnetVeth, _, err := handlers_imds.InitBuckets(js, 1) require.NoError(t, err) - store := handlers_imds.NewVethStore(vpcVeth) - mgr, err := NewIMDSTopologyManager(m, policy.NewRouteManager(m), store) + store := handlers_imds.NewVethStore(subnetVeth) + mgr, err := NewIMDSTopologyManager(m, store) require.NoError(t, err) return mgr, store } +// seedSubnetSwitch creates the subnet logical switch the IMDS localport attaches +// to. The subnet lifecycle owns the switch; EnsureForSubnet only adds the localport. +func seedSubnetSwitch(t *testing.T, m *mock.Client, subnetID string) { + t.Helper() + _, err := m.EnsureLogicalSwitch(context.Background(), &nbdb.LogicalSwitch{ + Name: topology.SubnetSwitch(subnetID), + ExternalIDs: map[string]string{"spinifex:subnet_id": subnetID}, + }) + require.NoError(t, err) +} + func findIMDSRoute(m *mock.Client, prefix string) *nbdb.LogicalRouterStaticRoute { for _, r := range m.StaticRoutes { if r.IPPrefix == prefix { @@ -37,132 +56,139 @@ func findIMDSRoute(m *mock.Client, prefix string) *nbdb.LogicalRouterStaticRoute func TestNewIMDSTopologyManager_RejectsMissingDeps(t *testing.T) { m := mock.New() _, _, js := testutil.StartTestJetStream(t) - vpcVeth, _, err := handlers_imds.InitBuckets(js, 1) + subnetVeth, _, err := handlers_imds.InitBuckets(js, 1) require.NoError(t, err) - store := handlers_imds.NewVethStore(vpcVeth) + store := handlers_imds.NewVethStore(subnetVeth) - _, err = NewIMDSTopologyManager(nil, policy.NewRouteManager(m), store) + _, err = NewIMDSTopologyManager(nil, store) require.Error(t, err) - _, err = NewIMDSTopologyManager(m, nil, store) - require.Error(t, err) - _, err = NewIMDSTopologyManager(m, policy.NewRouteManager(m), nil) + _, err = NewIMDSTopologyManager(m, nil) require.Error(t, err) } -func TestIMDSEnsureForVPC_InstallsTopology(t *testing.T) { +func TestIMDSEnsureForSubnet_InstallsLocalportOnSubnetLS(t *testing.T) { ctx := context.Background() m := mock.New() - seedVPCRouter(t, m, "vpc-0a1b2c3d4e5f6789", "10.0.0.0/16") + seedSubnetSwitch(t, m, imdsTestSubnetID) mgr, store := newIMDSManager(t, m) - spec, err := mgr.EnsureForVPC(ctx, "vpc-0a1b2c3d4e5f6789") + spec, err := mgr.EnsureForSubnet(ctx, imdsTestSubnetID, imdsTestVPCID, netip.MustParsePrefix(imdsTestCIDR)) require.NoError(t, err) - // Resolved names are deterministic functions of the VPC ID. - assert.Equal(t, "imds-ls-vpc-0a1b2c3d4e5f6789", spec.LSName) - assert.Equal(t, "imds-lrp-vpc-0a1b2c3d4e5f6789", spec.LRPName) - assert.Equal(t, "imds-port-vpc-0a1b2c3d4e5f6789", spec.LSPName) - assert.Equal(t, "4e5f6789", spec.ShortVPCID) + // Resolved names are deterministic functions of the subnet ID. + assert.Equal(t, "subnet-"+imdsTestSubnetID, spec.LSName) + assert.Equal(t, "imds-port-"+imdsTestSubnetID, spec.LSPName) + assert.Equal(t, "4e5f6789", spec.ShortSubnetID) assert.Equal(t, "imds-o-4e5f6789", spec.OVSEndName) assert.Equal(t, "imds-h-4e5f6789", spec.HostEndName) - // Logical switch exists, tagged as IMDS. - ls, err := m.GetLogicalSwitch(ctx, spec.LSName) - require.NoError(t, err) - assert.Equal(t, "imds", ls.ExternalIDs["spinifex:role"]) + // The localport is created directly on the guest's subnet switch — no IMDS + // switch, LRP, router-LSP, or static route exists. + assert.Len(t, m.Switches, 1, "only the subnet switch should exist; no dedicated IMDS switch") + assert.Empty(t, m.RouterPorts, "no IMDS LRP under the L2 datapath") + assert.Empty(t, m.StaticRoutes, "no 169.254.169.254/32 static route under the L2 datapath") + assert.Nil(t, findIMDSRoute(m, handlers_imds.MetaDataServerIP+"/32")) - // LRP on the VPC router holds the /30. - lrp, err := m.GetLogicalRouterPort(ctx, spec.LRPName) - require.NoError(t, err) - assert.Equal(t, []string{imdsLRPNetwork}, lrp.Networks) - - // type=router LSP peers the switch with the LRP. - rlsp, err := m.GetLogicalSwitchPort(ctx, spec.RouterLSPName) - require.NoError(t, err) - assert.Equal(t, "router", rlsp.Type) - assert.Equal(t, spec.LRPName, rlsp.Options["router-port"]) - - // Host-owned localport: claims the MetaData IP, no port_security. + // Host-owned localport: claims the MetaData IP, no port_security, no SG. lsp, err := m.GetLogicalSwitchPort(ctx, spec.LSPName) require.NoError(t, err) assert.Equal(t, "localport", lsp.Type) assert.Empty(t, lsp.PortSecurity) assert.Equal(t, []string{spec.LSPMAC + " " + handlers_imds.MetaDataServerIP}, lsp.Addresses) - // Static route on vpc-{vpcID} points 169.254.169.254/32 at the IMDS LRP. - route := findIMDSRoute(m, handlers_imds.MetaDataServerIP+"/32") - require.NotNil(t, route) - assert.Equal(t, handlers_imds.MetaDataServerIP, route.Nexthop) - require.NotNil(t, route.OutputPort) - assert.Equal(t, spec.LRPName, *route.OutputPort) + // ...and it is a member of the subnet switch. + ls, err := m.GetLogicalSwitch(ctx, spec.LSName) + require.NoError(t, err) + assert.True(t, slices.Contains(ls.Ports, lsp.UUID), "localport must be a port on the subnet switch") - // Record published to the bucket. - rec, err := store.Get("vpc-0a1b2c3d4e5f6789") + // Record published to the bucket, keyed by subnet, carrying the owning VPC + // (the subnet→VPC static lookup the IMDS handler needs) and the CIDR. + rec, err := store.Get(imdsTestSubnetID) require.NoError(t, err) require.NotNil(t, rec) - assert.Equal(t, spec.ShortVPCID, rec.ShortVPCID) + assert.Equal(t, spec.ShortSubnetID, rec.ShortSubnetID) + assert.Equal(t, imdsTestVPCID, rec.VPCID) assert.Equal(t, spec.LSPMAC, rec.IMDSPortMAC) - assert.Equal(t, imdsLRPNetwork, rec.LRPNetwork) + assert.Equal(t, imdsTestCIDR, rec.SubnetCIDR) assert.NotEmpty(t, rec.CreatedAt) } -func TestIMDSEnsureForVPC_Idempotent(t *testing.T) { +func TestIMDSEnsureForSubnet_RequiresCIDR(t *testing.T) { ctx := context.Background() m := mock.New() - seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + seedSubnetSwitch(t, m, imdsTestSubnetID) + mgr, _ := newIMDSManager(t, m) + + _, err := mgr.EnsureForSubnet(ctx, imdsTestSubnetID, imdsTestVPCID, netip.Prefix{}) + require.Error(t, err) +} + +func TestIMDSEnsureForSubnet_RequiresVPCID(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedSubnetSwitch(t, m, imdsTestSubnetID) + mgr, _ := newIMDSManager(t, m) + + // vpcID is persisted in the record for the IMDS handler's subnet→VPC lookup; + // without it the eni-by-vpc-ip index can't be keyed, so install must fail loud. + _, err := mgr.EnsureForSubnet(ctx, imdsTestSubnetID, "", netip.MustParsePrefix(imdsTestCIDR)) + require.Error(t, err) +} + +func TestIMDSEnsureForSubnet_Idempotent(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedSubnetSwitch(t, m, "subnet-1") mgr, _ := newIMDSManager(t, m) for range 3 { - _, err := mgr.EnsureForVPC(ctx, "vpc-1") + _, err := mgr.EnsureForSubnet(ctx, "subnet-1", "vpc-1", netip.MustParsePrefix(imdsTestCIDR)) require.NoError(t, err) } - // Exactly one IMDS switch and one static route. + // Exactly one switch (the subnet) and exactly one localport. assert.Len(t, m.Switches, 1) - count := 0 - for _, r := range m.StaticRoutes { - if r.IPPrefix == handlers_imds.MetaDataServerIP+"/32" { - count++ - } - } - assert.Equal(t, 1, count) - lsp, err := m.GetLogicalSwitchPort(ctx, "imds-port-vpc-1") + assert.Len(t, m.Ports, 1) + lsp, err := m.GetLogicalSwitchPort(ctx, "imds-port-subnet-1") require.NoError(t, err) assert.Equal(t, "localport", lsp.Type) } -func TestIMDSEnsureForVPC_MissingRouterErrors(t *testing.T) { +func TestIMDSEnsureForSubnet_MissingSwitchErrors(t *testing.T) { ctx := context.Background() m := mock.New() - // No router seeded — the LRP create has nowhere to land. + // No subnet switch seeded — the localport create has nowhere to land. mgr, store := newIMDSManager(t, m) - _, err := mgr.EnsureForVPC(ctx, "vpc-missing") + _, err := mgr.EnsureForSubnet(ctx, "subnet-missing", "vpc-1", netip.MustParsePrefix(imdsTestCIDR)) require.Error(t, err) // No record published when install fails. - rec, err := store.Get("vpc-missing") + rec, err := store.Get("subnet-missing") require.NoError(t, err) assert.Nil(t, rec) } -func TestIMDSRemoveForVPC_TearsDownAndIsIdempotent(t *testing.T) { +func TestIMDSRemoveForSubnet_TearsDownLocalportKeepsSwitch(t *testing.T) { ctx := context.Background() m := mock.New() - seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + seedSubnetSwitch(t, m, "subnet-1") mgr, store := newIMDSManager(t, m) - _, err := mgr.EnsureForVPC(ctx, "vpc-1") + spec, err := mgr.EnsureForSubnet(ctx, "subnet-1", "vpc-1", netip.MustParsePrefix(imdsTestCIDR)) require.NoError(t, err) - require.NoError(t, mgr.RemoveForVPC(ctx, "vpc-1")) + require.NoError(t, mgr.RemoveForSubnet(ctx, "subnet-1")) - assert.Empty(t, m.Switches) - assert.Nil(t, findIMDSRoute(m, handlers_imds.MetaDataServerIP+"/32")) - rec, err := store.Get("vpc-1") + // Localport and record gone; the subnet switch survives (subnet lifecycle owns it). + _, err = m.GetLogicalSwitchPort(ctx, spec.LSPName) + require.Error(t, err) + _, err = m.GetLogicalSwitch(ctx, spec.LSName) + require.NoError(t, err) + rec, err := store.Get("subnet-1") require.NoError(t, err) assert.Nil(t, rec) // Second remove is a no-op. - require.NoError(t, mgr.RemoveForVPC(ctx, "vpc-1")) + require.NoError(t, mgr.RemoveForSubnet(ctx, "subnet-1")) } diff --git a/spinifex/network/external/invariants_test.go b/spinifex/network/external/invariants_test.go index c07a4af17..e0f89001e 100644 --- a/spinifex/network/external/invariants_test.go +++ b/spinifex/network/external/invariants_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/netip" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -20,20 +21,22 @@ import ( // in either is a privilege-escalation or single-point-of-failure vector, so the // failure messages quote the offending design clause verbatim. -// TestI2_IMDSLSPMustBeLocalportNoPortSecurity asserts the host-owned -// imds-port-{vpcID} LSP is a localport with no port_security. The host — not a -// guest — sources 169.254.169.254 frames on this port, so port_security would -// be actively harmful (it pins the allowed src MAC to the LSP's claimed MAC, -// and ovn-controller would drop reply frames egressing from the host veth's -// MAC). A regular (non-localport) LSP would bind to a single chassis, defeating -// the per-chassis self-serve design. -func TestI2_IMDSLSPMustBeLocalportNoPortSecurity(t *testing.T) { +// TestI2_IMDSLocalportOnSubnetSwitchNoPortSecurity asserts the host-owned +// imds-port-{subnetID} LSP is a localport on the guest's subnet switch with no +// port_security. The host — not a guest — sources 169.254.169.254 frames on this +// port, so port_security would be actively harmful (it pins the allowed src MAC +// to the LSP's claimed MAC, and ovn-controller would drop reply frames egressing +// from the host veth's MAC). A regular (non-localport) LSP would bind to a single +// chassis, defeating the per-chassis self-serve design. Living on the subnet +// switch is the L2 datapath itself: the guest reaches metadata in one hop on its +// own broadcast domain, with no router in the path to shadow it. +func TestI2_IMDSLocalportOnSubnetSwitchNoPortSecurity(t *testing.T) { ctx := context.Background() m := mock.New() - seedVPCRouter(t, m, "vpc-0a1b2c3d4e5f6789", "10.0.0.0/16") + seedSubnetSwitch(t, m, imdsTestSubnetID) mgr, _ := newIMDSManager(t, m) - spec, err := mgr.EnsureForVPC(ctx, "vpc-0a1b2c3d4e5f6789") + spec, err := mgr.EnsureForSubnet(ctx, imdsTestSubnetID, imdsTestVPCID, netip.MustParsePrefix(imdsTestCIDR)) require.NoError(t, err) lsp, err := m.GetLogicalSwitchPort(ctx, spec.LSPName) @@ -42,48 +45,33 @@ func TestI2_IMDSLSPMustBeLocalportNoPortSecurity(t *testing.T) { if lsp.Type != "localport" { t.Errorf("imds LSP %s Type = %q, want \"localport\": a regular LSP binds to "+ "one chassis only, forcing IMDS through Geneve and creating a single point "+ - "of failure per VPC", spec.LSPName, lsp.Type) + "of failure per subnet", spec.LSPName, lsp.Type) } if len(lsp.PortSecurity) != 0 { t.Errorf("imds LSP %s PortSecurity = %v, want empty: port_security set on the "+ "host-owned LSP would cause ovn-controller to drop reply frames from the "+ "host veth's MAC", spec.LSPName, lsp.PortSecurity) } -} + assert.Equal(t, []string{spec.LSPMAC + " " + handlers_imds.MetaDataServerIP}, lsp.Addresses, + "the localport must claim %s on the subnet switch", handlers_imds.MetaDataServerIP) -// TestI3_IMDSStaticRouteShape asserts the static route on vpc-{vpcID} that -// diverts 169.254.169.254 off the WAN default and out the IMDS LRP has the -// exact shape the datapath depends on. A drifted prefix, nexthop, or output -// port silently sends IMDS traffic to the WAN, where it disappears. -func TestI3_IMDSStaticRouteShape(t *testing.T) { - ctx := context.Background() - m := mock.New() - seedVPCRouter(t, m, "vpc-0a1b2c3d4e5f6789", "10.0.0.0/16") - mgr, _ := newIMDSManager(t, m) - - spec, err := mgr.EnsureForVPC(ctx, "vpc-0a1b2c3d4e5f6789") + // It must be a port on the subnet switch — that is what makes the datapath L2. + ls, err := m.GetLogicalSwitch(ctx, spec.LSName) require.NoError(t, err) - - route := findIMDSRoute(m, handlers_imds.MetaDataServerIP+"/32") - require.NotNil(t, route, "no static route for %s/32 on the VPC router — IMDS traffic "+ - "falls through to the WAN default", handlers_imds.MetaDataServerIP) - - assert.Equal(t, handlers_imds.MetaDataServerIP+"/32", route.IPPrefix) - assert.Equal(t, handlers_imds.MetaDataServerIP, route.Nexthop) - require.NotNil(t, route.OutputPort, "IMDS static route has no output port — OVN cannot "+ - "resolve which LRP to send 169.254.169.254 traffic out of") - assert.Equal(t, spec.LRPName, *route.OutputPort) + if !slices.Contains(ls.Ports, lsp.UUID) { + t.Errorf("imds localport %s is not a port on subnet switch %s: the L2 datapath "+ + "requires the localport on the guest's own broadcast domain, not a dedicated "+ + "IMDS switch reached through the VPC router", spec.LSPName, spec.LSName) + } } // TestI4_SubnetEgressRerouteMustExcludeLinkLocal asserts every per-subnet // internet-egress reroute policy excludes 169.254.0.0/16. The reroute matches -// 0.0.0.0/0 — the widest possible scope — and fires in lr_in_policy AFTER the -// IMDS /32 static route matches in lr_in_ip_routing, so without this exclusion -// it overrides the /32 and SNAT-reroutes 169.254.169.254 out the WAN, where -// IMDS disappears (the guest's PUT for a token never reaches the per-VPC netns -// listener). Both the IGW- and NATGW-priority reroutes carry the gate, so both -// must spare link-local. A drifted exclude list silently breaks IMDS on any -// VPC with internet egress. +// 0.0.0.0/0 — the widest possible scope — so without this exclusion it would +// SNAT-reroute link-local traffic out the WAN. Link-local is host-scoped by +// definition and must never egress to the provider network. Both the IGW- and +// NATGW-priority reroutes carry the gate, so both must spare link-local. A +// drifted exclude list silently leaks 169.254.0.0/16 onto the internet. func TestI4_SubnetEgressRerouteMustExcludeLinkLocal(t *testing.T) { linkLocal := fmt.Sprintf("ip4.dst != %s", linkLocalCIDR.String()) @@ -114,8 +102,8 @@ func TestI4_SubnetEgressRerouteMustExcludeLinkLocal(t *testing.T) { require.NoError(t, err) require.Len(t, policies, 1) assert.Contains(t, policies[0].Match, linkLocal, - "%s egress reroute %q does not exclude %s — it will hijack 169.254.169.254 "+ - "out the WAN and override the IMDS /32 static route, making IMDS unreachable", + "%s egress reroute %q does not exclude %s — it will SNAT-reroute "+ + "link-local traffic out the WAN; link-local must never egress", tc.name, policies[0].Match, linkLocalCIDR) }) } diff --git a/spinifex/network/host/imds_names.go b/spinifex/network/host/imds_names.go index 95957d863..fea55b4c5 100644 --- a/spinifex/network/host/imds_names.go +++ b/spinifex/network/host/imds_names.go @@ -1,34 +1,36 @@ package host -// IMDS per-VPC veth naming. The names live here (not in network/topology) so +// IMDS per-subnet veth naming. The names live here (not in network/topology) so // the IMDS BindManager can derive them without importing topology, which would // form an import cycle (topology imports handlers/imds for MetaDataServerIP). -// IMDSShortVPCID is the last 8 chars of the VPC ID. Linux IFNAMSIZ caps interface -// names at 15 chars, so the IMDS veth names key off this short form. Deterministic -// and stable: every chassis serving IMDS for VPC X derives the same names. -func IMDSShortVPCID(vpcID string) string { - if len(vpcID) <= 8 { - return vpcID +// IMDSShortSubnetID is the last 8 chars of the subnet ID. Linux IFNAMSIZ caps +// interface names at 15 chars, so the IMDS veth names key off this short form. +// Deterministic and stable: every chassis serving IMDS for subnet X derives the +// same names. +func IMDSShortSubnetID(subnetID string) string { + if len(subnetID) <= 8 { + return subnetID } - return vpcID[len(vpcID)-8:] + return subnetID[len(subnetID)-8:] } -// IMDSOVSPortName is the br-int side of the per-VPC IMDS veth pair, bound to the +// IMDSOVSPortName is the br-int side of the per-subnet IMDS veth pair, bound to the // imds-port LSP via external_ids:iface-id. The 7-char prefix keeps "imds-o-" + the -// 8-char short VPC ID at exactly IFNAMSIZ-1 (15); longer fails `ip link add`. -func IMDSOVSPortName(vpcID string) string { return "imds-o-" + IMDSShortVPCID(vpcID) } +// 8-char short subnet ID at exactly IFNAMSIZ-1 (15); longer fails `ip link add`. +func IMDSOVSPortName(subnetID string) string { return "imds-o-" + IMDSShortSubnetID(subnetID) } -// IMDSHostVethName is the host-end side of the per-VPC IMDS veth pair — the +// IMDSHostVethName is the host-end side of the per-subnet IMDS veth pair — the // device the IMDS listener binds to via SO_BINDTODEVICE. Same 7-char prefix -// budget as IMDSOVSPortName: "imds-h-" + 8-char short VPC ID = 15 chars. It -// lives inside the VPC's netns (see IMDSNetnsName), not the root netns. -func IMDSHostVethName(vpcID string) string { return "imds-h-" + IMDSShortVPCID(vpcID) } +// budget as IMDSOVSPortName: "imds-h-" + 8-char short subnet ID = 15 chars. It +// lives inside the subnet's netns (see IMDSNetnsName), not the root netns. +func IMDSHostVethName(subnetID string) string { return "imds-h-" + IMDSShortSubnetID(subnetID) } -// IMDSNetnsName is the per-VPC network namespace the IMDS host-end veth and its -// listener live in. The netns gives the reply path a real L3 next-hop — the -// host end carries 169.254.169.254/30 with a default route via the .253 LRP — -// and structurally isolates VPCs with overlapping CIDRs, since each netns is its -// own routing domain. Netns names are filesystem-scoped, not IFNAMSIZ-bound, so -// "imds-" + 8-char short VPC ID (13 chars) is comfortably within budget. -func IMDSNetnsName(vpcID string) string { return "imds-" + IMDSShortVPCID(vpcID) } +// IMDSNetnsName is the per-subnet network namespace the IMDS host-end veth and its +// listener live in. The netns gives the reply path a real L3 next-hop — the host +// end carries 169.254.169.254/30 with the subnet CIDR on-link, so the reply to the +// guest resolves by ARP over the localport — and structurally isolates subnets +// with overlapping CIDRs, since each netns is its own routing domain. Netns names +// are filesystem-scoped, not IFNAMSIZ-bound, so "imds-" + 8-char short subnet ID +// (13 chars) is comfortably within budget. +func IMDSNetnsName(subnetID string) string { return "imds-" + IMDSShortSubnetID(subnetID) } diff --git a/spinifex/network/host/imds_veth.go b/spinifex/network/host/imds_veth.go index 18ec0ca86..94a8679b5 100644 --- a/spinifex/network/host/imds_veth.go +++ b/spinifex/network/host/imds_veth.go @@ -4,44 +4,46 @@ import ( "context" "fmt" "log/slog" + "net/netip" "strings" "github.com/mulgadc/spinifex/spinifex/utils" ) -const ( - // imdsNetnsAddr is the host/listener side of the IMDS /30, assigned to the - // veth host end inside the per-VPC netns. .254 is MetaDataServerIP; the /30 - // makes .253 (the LRP) directly connected so the reply path has a next-hop. - imdsNetnsAddr = "169.254.169.254/30" - // imdsNetnsGateway is the IMDS LRP (.253) — the netns default gateway that - // routes guest replies back into OVN. Mirrors external.imdsLRPNetwork's .253. - imdsNetnsGateway = "169.254.169.253" -) +// imdsNetnsAddr is the host/listener side of the IMDS address, assigned to the +// veth host end inside the per-subnet netns. .254 is MetaDataServerIP; the /30 is +// kept (no longer load-bearing for a next-hop) and the reply path resolves the +// guest via the subnet CIDR added on-link out the same veth. +const imdsNetnsAddr = "169.254.169.254/30" // imdsPortLSPName is the OVN logical-switch-port the OVS-side veth binds to via // external_ids:iface-id. It mirrors topology.IMDSPort but is duplicated here to // avoid an import cycle (host ← handlers/imds ← topology). -func imdsPortLSPName(vpcID string) string { return "imds-port-" + vpcID } +func imdsPortLSPName(subnetID string) string { return "imds-port-" + subnetID } // imdsLSPMAC is the logical MAC OVN advertises for 169.254.169.254 (the localport // address). The netns veth host end MUST carry this exact MAC: OVN resolves .254 // to it and delivers IMDS request frames with dl_dst set to it, so a kernel-random // veth MAC makes the netns drop every frame not-for-me and the listener never sees -// a SYN. Mirrors external.IMDSSpecForVPC's LSPMAC, duplicated here (like +// a SYN. Mirrors external.IMDSSpecForSubnet's LSPMAC, duplicated here (like // imdsPortLSPName) to avoid the import cycle. -func imdsLSPMAC(vpcID string) string { return utils.HashMAC("imds-" + vpcID) } +func imdsLSPMAC(subnetID string) string { return utils.HashMAC("imds-" + subnetID) } -// EnsureIMDSVeth idempotently creates the per-VPC IMDS veth pair, moves the host end -// into a dedicated netns with 169.254.169.254/30 + a default route via the .253 LRP, -// and attaches the OVS end to br-int so ovn-controller binds the localport here. The +// EnsureIMDSVeth idempotently creates the per-subnet IMDS veth pair, moves the host +// end into a dedicated netns with 169.254.169.254/30 + the subnet CIDR on-link, and +// attaches the OVS end to br-int so ovn-controller binds the localport here. The // netns gives the host-served reply a real L3 next-hop (SO_BINDTODEVICE alone cannot -// route the SYN-ACK) and isolates overlapping VPC CIDRs into separate routing domains. -// Returns the netns name and host-end name the listener enters and binds. -func EnsureIMDSVeth(ctx context.Context, vpcID string) (netnsName, hostEndName string, err error) { - ovsEnd := IMDSOVSPortName(vpcID) - hostEnd := IMDSHostVethName(vpcID) - netns := IMDSNetnsName(vpcID) +// route the SYN-ACK): the on-link subnet route makes the guest directly reachable, so +// the reply resolves by ARP over the localport on the guest's own L2 switch. The netns +// also isolates overlapping subnet CIDRs into separate routing domains. Returns the +// netns name and host-end name the listener enters and binds. +func EnsureIMDSVeth(ctx context.Context, subnetID string, cidr netip.Prefix) (netnsName, hostEndName string, err error) { + if !cidr.IsValid() { + return "", "", fmt.Errorf("EnsureIMDSVeth: subnet %s requires a valid CIDR", subnetID) + } + ovsEnd := IMDSOVSPortName(subnetID) + hostEnd := IMDSHostVethName(subnetID) + netns := IMDSNetnsName(subnetID) // Idempotency probe: the full plumbing (veth pair, netns, host-end address + // route) exists from a prior boot only when the OVS end is a port on br-int @@ -52,13 +54,13 @@ func EnsureIMDSVeth(ctx context.Context, vpcID string) (netnsName, hostEndName s // plumbing down and rebuild it below. if imdsOVSPortOnBrInt(ovsEnd) { if netnsEnterable(netns) { - slog.Debug("IMDS veth already present", "vpc", vpcID, "ovs_end", ovsEnd, "host_end", hostEnd, "netns", netns) + slog.Debug("IMDS veth already present", "subnet", subnetID, "ovs_end", ovsEnd, "host_end", hostEnd, "netns", netns) return netns, hostEnd, nil } slog.Warn("IMDS netns unenterable behind a live OVS port (stale handle), rebuilding plumbing", - "vpc", vpcID, "netns", netns, "ovs_end", ovsEnd) + "subnet", subnetID, "netns", netns, "ovs_end", ovsEnd) if err := removeIMDSPlumbing(ovsEnd, hostEnd, netns); err != nil { - slog.Warn("Failed to tear down stale IMDS plumbing before rebuild", "vpc", vpcID, "err", err) + slog.Warn("Failed to tear down stale IMDS plumbing before rebuild", "subnet", subnetID, "err", err) } } @@ -68,40 +70,41 @@ func EnsureIMDSVeth(ctx context.Context, vpcID string) (netnsName, hostEndName s if out, err := utils.SudoCommand("ip", "link", "add", ovsEnd, "type", "veth", "peer", "name", hostEnd).CombinedOutput(); err != nil { if cleanErr := removeIMDSPlumbing(ovsEnd, hostEnd, netns); cleanErr != nil { - slog.Warn("Failed to clean up IMDS plumbing after veth-create failure", "vpc", vpcID, "err", cleanErr) + slog.Warn("Failed to clean up IMDS plumbing after veth-create failure", "subnet", subnetID, "err", cleanErr) } return "", "", fmt.Errorf("create IMDS veth pair %s/%s: %s: %w", ovsEnd, hostEnd, strings.TrimSpace(string(out)), err) } - if err := configureIMDSNetns(netns, hostEnd, ovsEnd, imdsLSPMAC(vpcID)); err != nil { + if err := configureIMDSNetns(netns, hostEnd, ovsEnd, imdsLSPMAC(subnetID), cidr); err != nil { if cleanErr := removeIMDSPlumbing(ovsEnd, hostEnd, netns); cleanErr != nil { - slog.Warn("Failed to clean up IMDS plumbing after netns config failure", "vpc", vpcID, "err", cleanErr) + slog.Warn("Failed to clean up IMDS plumbing after netns config failure", "subnet", subnetID, "err", cleanErr) } return "", "", err } - ifaceID := imdsPortLSPName(vpcID) + ifaceID := imdsPortLSPName(subnetID) if out, err := utils.SudoCommand("ovs-vsctl", "add-port", "br-int", ovsEnd, "--", "set", "Interface", ovsEnd, "external_ids:iface-id="+ifaceID).CombinedOutput(); err != nil { if cleanErr := removeIMDSPlumbing(ovsEnd, hostEnd, netns); cleanErr != nil { - slog.Warn("Failed to clean up IMDS plumbing after OVS failure", "vpc", vpcID, "err", cleanErr) + slog.Warn("Failed to clean up IMDS plumbing after OVS failure", "subnet", subnetID, "err", cleanErr) } return "", "", fmt.Errorf("add IMDS veth %s to br-int: %s: %w", ovsEnd, strings.TrimSpace(string(out)), err) } - slog.Info("IMDS veth plumbing complete", "vpc", vpcID, "ovs_end", ovsEnd, "host_end", hostEnd, "netns", netns, "iface_id", ifaceID) + slog.Info("IMDS veth plumbing complete", "subnet", subnetID, "ovs_end", ovsEnd, "host_end", hostEnd, "netns", netns, "iface_id", ifaceID) return netns, hostEnd, nil } // configureIMDSNetns moves the host end into the netns, sets its MAC to the // localport's logical MAC, brings it (and lo) up, and assigns the IMDS address + -// default route so the host-served reply has a real next-hop. The OVS end stays in -// the root netns for ovn-controller to bind. The MAC set is the request-path -// contract: OVN delivers frames with dl_dst = the localport MAC, so the veth host -// end must own that MAC or the netns silently drops them. The addr/route adds -// tolerate "File exists" so a re-run after a partial failure converges rather than -// wedging. -func configureIMDSNetns(netns, hostEnd, ovsEnd, hostEndMAC string) error { +// the subnet CIDR on-link so the host-served reply resolves the guest by ARP over +// the localport (a single L2 hop on the guest's own switch — no router next-hop). +// The OVS end stays in the root netns for ovn-controller to bind. The MAC set is +// the request-path contract: OVN delivers frames with dl_dst = the localport MAC, +// so the veth host end must own that MAC or the netns silently drops them. The +// addr/route adds tolerate "File exists" so a re-run after a partial failure +// converges rather than wedging. +func configureIMDSNetns(netns, hostEnd, ovsEnd, hostEndMAC string, cidr netip.Prefix) error { if out, err := utils.SudoCommand("ip", "link", "set", ovsEnd, "up").CombinedOutput(); err != nil { return fmt.Errorf("bring up IMDS OVS end %s: %s: %w", ovsEnd, strings.TrimSpace(string(out)), err) } @@ -119,7 +122,7 @@ func configureIMDSNetns(netns, hostEnd, ovsEnd, hostEndMAC string) error { if err := ipNetnsTolerate(netns, "File exists", "addr", "add", imdsNetnsAddr, "dev", hostEnd); err != nil { return err } - return ipNetnsTolerate(netns, "File exists", "route", "add", "default", "via", imdsNetnsGateway) + return ipNetnsTolerate(netns, "File exists", "route", "add", cidr.String(), "dev", hostEnd) } // ensureNetns creates the netns, treating "already exists" as success — but @@ -180,9 +183,9 @@ func ipNetnsTolerate(netns, tolerate string, args ...string) error { // RemoveIMDSVeth detaches the OVS port, deletes the netns (which destroys the // host end and thus the veth pair), and clears any root-netns remnant. Idempotent: -// safe to call for a VPC that never had a veth on this chassis. -func RemoveIMDSVeth(ctx context.Context, vpcID string) error { - return removeIMDSPlumbing(IMDSOVSPortName(vpcID), IMDSHostVethName(vpcID), IMDSNetnsName(vpcID)) +// safe to call for a subnet that never had a veth on this chassis. +func RemoveIMDSVeth(ctx context.Context, subnetID string) error { + return removeIMDSPlumbing(IMDSOVSPortName(subnetID), IMDSHostVethName(subnetID), IMDSNetnsName(subnetID)) } // removeIMDSPlumbing tears down the OVS port, the netns, and any leftover veth. diff --git a/spinifex/network/host/imds_veth_test.go b/spinifex/network/host/imds_veth_test.go index 1ece52750..55f846b07 100644 --- a/spinifex/network/host/imds_veth_test.go +++ b/spinifex/network/host/imds_veth_test.go @@ -2,6 +2,7 @@ package host import ( "context" + "net/netip" "os/exec" "slices" "strings" @@ -10,7 +11,9 @@ import ( "github.com/mulgadc/spinifex/spinifex/utils" ) -const testVPCID = "vpc-0123456789abcdef" // short form: "89abcdef" +const testSubnetID = "subnet-0123456789abcdef" // short form: "89abcdef" + +var testSubnetCIDR = netip.MustParsePrefix("10.211.0.0/16") func recordSudo(t *testing.T, fail func(name string, args []string) *exec.Cmd) *[][]string { t.Helper() @@ -36,7 +39,7 @@ func TestEnsureIMDSVeth_HappyPath(t *testing.T) { return nil }) - netns, hostEnd, err := EnsureIMDSVeth(context.Background(), testVPCID) + netns, hostEnd, err := EnsureIMDSVeth(context.Background(), testSubnetID, testSubnetCIDR) if err != nil { t.Fatalf("EnsureIMDSVeth: %v", err) } @@ -53,14 +56,14 @@ func TestEnsureIMDSVeth_HappyPath(t *testing.T) { {"ip", "link", "add", "imds-o-89abcdef", "type", "veth", "peer", "name", "imds-h-89abcdef"}, {"ip", "link", "set", "imds-o-89abcdef", "up"}, {"ip", "link", "set", "imds-h-89abcdef", "netns", "imds-89abcdef"}, - {"ip", "-n", "imds-89abcdef", "link", "set", "imds-h-89abcdef", "address", utils.HashMAC("imds-" + testVPCID)}, + {"ip", "-n", "imds-89abcdef", "link", "set", "imds-h-89abcdef", "address", utils.HashMAC("imds-" + testSubnetID)}, {"ip", "-n", "imds-89abcdef", "link", "set", "lo", "up"}, {"ip", "-n", "imds-89abcdef", "link", "set", "imds-h-89abcdef", "up"}, {"ip", "-n", "imds-89abcdef", "addr", "add", "169.254.169.254/30", "dev", "imds-h-89abcdef"}, - {"ip", "-n", "imds-89abcdef", "route", "add", "default", "via", "169.254.169.253"}, + {"ip", "-n", "imds-89abcdef", "route", "add", "10.211.0.0/16", "dev", "imds-h-89abcdef"}, {"ovs-vsctl", "add-port", "br-int", "imds-o-89abcdef", "--", "set", "Interface", "imds-o-89abcdef", - "external_ids:iface-id=imds-port-" + testVPCID}, + "external_ids:iface-id=imds-port-" + testSubnetID}, } if len(*calls) != len(want) { t.Fatalf("got %d calls, want %d: %v", len(*calls), len(want), *calls) @@ -82,7 +85,7 @@ func TestEnsureIMDSVeth_Idempotent(t *testing.T) { return nil }) - netns, hostEnd, err := EnsureIMDSVeth(context.Background(), testVPCID) + netns, hostEnd, err := EnsureIMDSVeth(context.Background(), testSubnetID, testSubnetCIDR) if err != nil { t.Fatalf("EnsureIMDSVeth: %v", err) } @@ -122,7 +125,7 @@ func TestEnsureIMDSVeth_StaleNetnsBehindLivePort_Rebuilds(t *testing.T) { return nil }) - if _, _, err := EnsureIMDSVeth(context.Background(), testVPCID); err != nil { + if _, _, err := EnsureIMDSVeth(context.Background(), testSubnetID, testSubnetCIDR); err != nil { t.Fatalf("EnsureIMDSVeth: %v", err) } @@ -170,7 +173,7 @@ func TestEnsureIMDSVeth_StaleNetnsRecreated(t *testing.T) { return nil }) - if _, _, err := EnsureIMDSVeth(context.Background(), testVPCID); err != nil { + if _, _, err := EnsureIMDSVeth(context.Background(), testSubnetID, testSubnetCIDR); err != nil { t.Fatalf("EnsureIMDSVeth: %v", err) } @@ -208,7 +211,7 @@ func TestEnsureIMDSVeth_NetnsExistsTolerated(t *testing.T) { return nil }) - if _, _, err := EnsureIMDSVeth(context.Background(), testVPCID); err != nil { + if _, _, err := EnsureIMDSVeth(context.Background(), testSubnetID, testSubnetCIDR); err != nil { t.Fatalf("expected nil for pre-existing netns, got %v", err) } } @@ -224,7 +227,7 @@ func TestEnsureIMDSVeth_AddPortFailureCleansUp(t *testing.T) { return nil }) - _, _, err := EnsureIMDSVeth(context.Background(), testVPCID) + _, _, err := EnsureIMDSVeth(context.Background(), testSubnetID, testSubnetCIDR) if err == nil || !strings.Contains(err.Error(), "add IMDS veth") { t.Fatalf("expected add-port error, got %v", err) } @@ -257,7 +260,7 @@ func TestEnsureIMDSVeth_LinkAddFailureSurfaces(t *testing.T) { return nil }) - _, _, err := EnsureIMDSVeth(context.Background(), testVPCID) + _, _, err := EnsureIMDSVeth(context.Background(), testSubnetID, testSubnetCIDR) if err == nil || !strings.Contains(err.Error(), "create IMDS veth pair") { t.Fatalf("expected create error, got %v", err) } @@ -276,7 +279,7 @@ func TestEnsureIMDSVeth_AddrFailureCleansUp(t *testing.T) { return nil }) - _, _, err := EnsureIMDSVeth(context.Background(), testVPCID) + _, _, err := EnsureIMDSVeth(context.Background(), testSubnetID, testSubnetCIDR) if err == nil || !strings.Contains(err.Error(), "addr add") { t.Fatalf("expected addr-add error, got %v", err) } @@ -294,7 +297,7 @@ func TestEnsureIMDSVeth_AddrFailureCleansUp(t *testing.T) { func TestRemoveIMDSVeth(t *testing.T) { calls := recordSudo(t, nil) - if err := RemoveIMDSVeth(context.Background(), testVPCID); err != nil { + if err := RemoveIMDSVeth(context.Background(), testSubnetID); err != nil { t.Fatalf("RemoveIMDSVeth: %v", err) } @@ -325,26 +328,26 @@ func TestRemoveIMDSVeth_MissingDeviceIsNotError(t *testing.T) { return nil }) - if err := RemoveIMDSVeth(context.Background(), testVPCID); err != nil { + if err := RemoveIMDSVeth(context.Background(), testSubnetID); err != nil { t.Fatalf("expected nil for absent device, got %v", err) } } // TestIMDSVethNamesWithinIFNAMSIZ guards the load-bearing constraint that both // veth-end names fit Linux's IFNAMSIZ-1 (15 chars). A longer name makes -// `ip link add` fail with "name too long" for every VPC, silently taking IMDS +// `ip link add` fail with "name too long" for every subnet, silently taking IMDS // offline cluster-wide — and the SudoCommand mock would never catch it. func TestIMDSVethNamesWithinIFNAMSIZ(t *testing.T) { const ifnamsizMax = 15 - for _, vpcID := range []string{ - "vpc-0123456789abcdef", - "vpc-0a1b2c3d4e5f6a7b8", - "vpc-deadbeef", + for _, subnetID := range []string{ + "subnet-0123456789abcdef", + "subnet-0a1b2c3d4e5f6a7b8", + "subnet-deadbeef", "short", } { - for _, name := range []string{IMDSOVSPortName(vpcID), IMDSHostVethName(vpcID)} { + for _, name := range []string{IMDSOVSPortName(subnetID), IMDSHostVethName(subnetID)} { if len(name) > ifnamsizMax { - t.Errorf("veth name %q (%d chars) exceeds IFNAMSIZ-1 (%d) for vpc %q", name, len(name), ifnamsizMax, vpcID) + t.Errorf("veth name %q (%d chars) exceeds IFNAMSIZ-1 (%d) for subnet %q", name, len(name), ifnamsizMax, subnetID) } } } diff --git a/spinifex/network/reconcile/apply.go b/spinifex/network/reconcile/apply.go index bc8d62218..31eeaf9fb 100644 --- a/spinifex/network/reconcile/apply.go +++ b/spinifex/network/reconcile/apply.go @@ -5,7 +5,6 @@ import ( "fmt" "log/slog" - handlers_imds "github.com/mulgadc/spinifex/spinifex/handlers/imds" "github.com/mulgadc/spinifex/spinifex/network/ovn/nbdb" "github.com/mulgadc/spinifex/spinifex/network/topology" "github.com/mulgadc/spinifex/spinifex/utils" @@ -31,13 +30,6 @@ func (r *reconciler) applyVPCs(ctx context.Context, intent IntentState, actual A actual.Routers[routerName] = struct{}{} slog.Info("reconcile/apply: ensured VPC router", "vpc_id", vpcID, "router", routerName) } - // Install IMDS topology for every intent VPC (idempotent via the - // vpc-veth bucket gate); the router must exist for the IMDS LRP. - if r.imds != nil { - if _, err := r.imds.EnsureForVPC(ctx, vpcID); err != nil { - slog.Error("reconcile/apply: IMDS EnsureForVPC failed", "vpc_id", vpcID, "err", err) - } - } } } @@ -90,14 +82,13 @@ func (r *reconciler) applySubnets(ctx context.Context, intent IntentState, actua actual.RouterPorts[routerPortName] = struct{}{} } - if existing, err := r.ovn.GetLogicalSwitchPort(ctx, switchRouterPortName); err != nil { + if _, err := r.ovn.GetLogicalSwitchPort(ctx, switchRouterPortName); err != nil { lsp := &nbdb.LogicalSwitchPort{ Name: switchRouterPortName, Type: "router", Addresses: []string{"router"}, Options: map[string]string{ "router-port": routerPortName, - "arp_proxy": handlers_imds.MetaDataServerIP, }, ExternalIDs: map[string]string{ "spinifex:subnet_id": subnetID, @@ -108,18 +99,13 @@ func (r *reconciler) applySubnets(ctx context.Context, intent IntentState, actua slog.Error("reconcile/apply: create subnet router-LSP failed", "subnet_id", subnetID, "err", err) continue } - } else if existing.Options["arp_proxy"] != handlers_imds.MetaDataServerIP { - // Drift convergence: a subnet router LSP created before IMDS proxy-ARP - // landed won't gain arp_proxy from a redeploy alone (the create branch - // only fires when absent), so patch it in place to make it IMDS-reachable. - if existing.Options == nil { - existing.Options = map[string]string{} - } - existing.Options["arp_proxy"] = handlers_imds.MetaDataServerIP - if err := r.ovn.UpdateLogicalSwitchPort(ctx, existing); err != nil { - slog.Error("reconcile/apply: patch subnet router-LSP arp_proxy failed", "subnet_id", subnetID, "err", err) - } else { - slog.Info("reconcile/apply: patched subnet router-LSP with IMDS arp_proxy", "subnet_id", subnetID, "port", switchRouterPortName) + } + + // The IMDS localport rides on the subnet switch (idempotent via the + // subnet-veth bucket gate); the switch must exist first. + if r.imds != nil { + if _, err := r.imds.EnsureForSubnet(ctx, subnetID, spec.VPCID, spec.CIDR); err != nil { + slog.Error("reconcile/apply: IMDS EnsureForSubnet failed", "subnet_id", subnetID, "err", err) } } diff --git a/spinifex/network/reconcile/invariants_test.go b/spinifex/network/reconcile/invariants_test.go index fb3ab56c3..8afa80b77 100644 --- a/spinifex/network/reconcile/invariants_test.go +++ b/spinifex/network/reconcile/invariants_test.go @@ -6,7 +6,6 @@ import ( "net/netip" "testing" - imds "github.com/mulgadc/spinifex/spinifex/handlers/imds" "github.com/mulgadc/spinifex/spinifex/network/ovn/mock" "github.com/mulgadc/spinifex/spinifex/network/topology" ) @@ -84,52 +83,3 @@ func assertPortSecurity(t *testing.T, m *mock.Client, portName, want string) { "identity must match", portName, lsp.Addresses, lsp.PortSecurity) } } - -// TestI5_SubnetRouterPortProxiesIMDSARP asserts the subnet router LSP carries -// options:arp_proxy = 169.254.169.254, so the subnet LRP answers ARP/ND for the -// IMDS address link-local. Both emission paths (live EnsureSubnet, reconciler -// applySubnets) are checked so they cannot diverge. This replaces the former -// DHCP option-121 mechanism: proxy-ARP reaches DHCP and fully static guests -// identically (AWS Nitro parity), whereas option 121 never reached a guest with -// no DHCP client. -func TestI5_SubnetRouterPortProxiesIMDSARP(t *testing.T) { - ctx := context.Background() - const cidr = "10.0.1.0/24" - - t.Run("live", func(t *testing.T) { - m := mock.New() - mgr := topology.NewLiveManager(m) - if err := mgr.EnsureVPC(ctx, topology.VPCSpec{VPCID: "vpc-a", CIDR: netip.MustParsePrefix("10.0.0.0/16")}); err != nil { - t.Fatalf("EnsureVPC: %v", err) - } - if err := mgr.EnsureSubnet(ctx, topology.SubnetSpec{SubnetID: "subnet-a", VPCID: "vpc-a", CIDR: netip.MustParsePrefix(cidr)}); err != nil { - t.Fatalf("EnsureSubnet: %v", err) - } - assertARPProxy(t, m, topology.SubnetSwitchRouterPort("subnet-a")) - }) - - t.Run("reconciler", func(t *testing.T) { - rec, m := newTestReconciler(t) - if err := rec.Reconcile(ctx, freshIntent(t)); err != nil { - t.Fatalf("Reconcile: %v", err) - } - assertARPProxy(t, m, topology.SubnetSwitchRouterPort("subnet-a")) - }) -} - -// assertARPProxy fails with the ADR clause unless the subnet router LSP carries -// options:arp_proxy for the IMDS address. -func assertARPProxy(t *testing.T, m *mock.Client, portName string) { - t.Helper() - lsp, err := m.GetLogicalSwitchPort(context.Background(), portName) - if err != nil { - t.Fatalf("subnet router LSP %s missing: %v", portName, err) - } - if lsp.Options["arp_proxy"] != imds.MetaDataServerIP { - t.Errorf("subnet router LSP %s options:arp_proxy = %q, want %q: subnet router "+ - "ports without options:arp_proxy for %s leave IMDS unreachable for any guest "+ - "that treats the address as link-local (the common static-IP and "+ - "NetworkManager/RHEL/Ubuntu case)", - portName, lsp.Options["arp_proxy"], imds.MetaDataServerIP, imds.MetaDataServerIP) - } -} diff --git a/spinifex/network/subscribers/topology.go b/spinifex/network/subscribers/topology.go index e46af64dc..20499b104 100644 --- a/spinifex/network/subscribers/topology.go +++ b/spinifex/network/subscribers/topology.go @@ -37,14 +37,6 @@ func (s *Subscriber) handleVPCCreate(msg *nats.Msg) { respond(msg, err) return } - // IMDS topology rides on every VPC (private instances still need metadata + - // role creds). Install is best-effort — the reconciler re-ensures it idempotently, - // so a transient OVN failure here must not fail CreateVpc. - if s.imds != nil { - if _, err := s.imds.EnsureForVPC(ctx, evt.VpcId); err != nil { - slog.Warn("subscribers: IMDS EnsureForVPC failed; reconciler will converge", "vpc_id", evt.VpcId, "err", err) - } - } respond(msg, nil) } @@ -56,15 +48,6 @@ func (s *Subscriber) handleVPCDelete(msg *nats.Msg) { return } ctx := context.Background() - // Remove IMDS topology before the router goes away — the IMDS LRP and - // static route live on vpc-{vpcID}. - if s.imds != nil { - if err := s.imds.RemoveForVPC(ctx, evt.VpcId); err != nil { - slog.Error("subscribers: IMDS RemoveForVPC failed", "vpc_id", evt.VpcId, "err", err) - respond(msg, err) - return - } - } if err := s.topology.DeleteVPC(ctx, evt.VpcId); err != nil { slog.Error("subscribers: DeleteVPC failed", "vpc_id", evt.VpcId, "err", err) respond(msg, err) @@ -104,6 +87,14 @@ func (s *Subscriber) handleSubnetCreate(msg *nats.Msg) { respond(msg, err) return } + // The IMDS localport lives on every subnet switch (guests reach metadata over + // one L2 hop). Install is best-effort — the reconciler re-ensures it + // idempotently, so a transient OVN failure here must not fail CreateSubnet. + if s.imds != nil { + if _, err := s.imds.EnsureForSubnet(ctx, evt.SubnetId, evt.VpcId, cidr); err != nil { + slog.Warn("subscribers: IMDS EnsureForSubnet failed; reconciler will converge", "subnet_id", evt.SubnetId, "err", err) + } + } respond(msg, nil) } @@ -120,7 +111,17 @@ func (s *Subscriber) handleSubnetDelete(msg *nats.Msg) { spec.CIDR = cidr } } - if err := s.topology.DeleteSubnet(context.Background(), spec); err != nil { + ctx := context.Background() + // Remove the IMDS localport before the subnet switch goes away — the + // localport lives on subnet-{subnetID}. + if s.imds != nil { + if err := s.imds.RemoveForSubnet(ctx, evt.SubnetId); err != nil { + slog.Error("subscribers: IMDS RemoveForSubnet failed", "subnet_id", evt.SubnetId, "err", err) + respond(msg, err) + return + } + } + if err := s.topology.DeleteSubnet(ctx, spec); err != nil { slog.Error("subscribers: DeleteSubnet failed", "subnet_id", evt.SubnetId, "err", err) respond(msg, err) return diff --git a/spinifex/network/topology/dhcp_options.go b/spinifex/network/topology/dhcp_options.go index 61a1dd77f..b1c323539 100644 --- a/spinifex/network/topology/dhcp_options.go +++ b/spinifex/network/topology/dhcp_options.go @@ -5,9 +5,13 @@ package topology // call it so the two paths cannot drift — they previously hard-coded divergent // dns_server values (live used the configured server, the reconciler "8.8.8.8"). // -// IMDS reachability (169.254.169.254) is no longer steered via DHCP option 121; it -// is answered link-local by the subnet router LSP's options:arp_proxy, so DHCP and -// static guests reach IMDS identically. The default gateway stays option 3 (router). +// IMDS reachability (169.254.169.254) is not steered via DHCP option 121: the +// default gateway stays option 3 (router), and the localport on the subnet +// switch answers ARP for the metadata IP directly (one L2 hop, no router). The +// guest's on-link route to 169.254.169.254 is delivered out-of-band by the +// cloud-init network-config the instance handler renders (NoCloud seed) — see +// generateNetworkConfig in handlers/ec2/instance — so it reaches DHCP and static +// guests alike without folding the default route into option 121 (RFC 3442). func BuildSubnetDHCPOptions(gwIP, routerMAC, dnsServer string) map[string]string { return map[string]string{ "server_id": gwIP, diff --git a/spinifex/network/topology/dhcp_options_test.go b/spinifex/network/topology/dhcp_options_test.go index e639efe0e..2c72051f9 100644 --- a/spinifex/network/topology/dhcp_options_test.go +++ b/spinifex/network/topology/dhcp_options_test.go @@ -2,15 +2,15 @@ package topology import "testing" -// IMDS reachability moved from DHCP option 121 to the subnet router LSP's -// options:arp_proxy (Step 14), so BuildSubnetDHCPOptions must no longer emit a +// IMDS reachability is served by a localport on the subnet switch (answered over +// L2), not via DHCP option 121, so BuildSubnetDHCPOptions must no longer emit a // classless_static_route key. The default gateway is still carried by option 3 // (router), and dns_server/server_id remain present. func TestBuildSubnetDHCPOptions_NoClasslessStaticRoute(t *testing.T) { opts := BuildSubnetDHCPOptions("10.0.1.1", "02:00:00:00:00:01", "{8.8.8.8, 1.1.1.1}") if _, ok := opts["classless_static_route"]; ok { - t.Errorf("classless_static_route must be absent (IMDS now uses subnet-router proxy-ARP, not DHCP option 121); got %q", opts["classless_static_route"]) + t.Errorf("classless_static_route must be absent (IMDS is served by a subnet-switch localport over L2, not DHCP option 121); got %q", opts["classless_static_route"]) } if got := opts["router"]; got != "10.0.1.1" { t.Errorf("router = %q, want %q", got, "10.0.1.1") diff --git a/spinifex/network/topology/manager_live.go b/spinifex/network/topology/manager_live.go index 5f12f2d34..fe6f576c3 100644 --- a/spinifex/network/topology/manager_live.go +++ b/spinifex/network/topology/manager_live.go @@ -9,7 +9,6 @@ import ( "strconv" "strings" - handlers_imds "github.com/mulgadc/spinifex/spinifex/handlers/imds" "github.com/mulgadc/spinifex/spinifex/network/ovn" "github.com/mulgadc/spinifex/spinifex/network/ovn/nbdb" "github.com/mulgadc/spinifex/spinifex/utils" @@ -206,7 +205,6 @@ func (m *liveManager) EnsureSubnet(ctx context.Context, spec SubnetSpec) error { Addresses: []string{"router"}, Options: map[string]string{ "router-port": routerPortName, - "arp_proxy": handlers_imds.MetaDataServerIP, }, ExternalIDs: map[string]string{ "spinifex:subnet_id": spec.SubnetID, diff --git a/spinifex/network/topology/names.go b/spinifex/network/topology/names.go index 9212aebd7..62fa12488 100644 --- a/spinifex/network/topology/names.go +++ b/spinifex/network/topology/names.go @@ -36,22 +36,11 @@ func ExternalSwitch(vpcID string) string { return "ext-" + vpcID } // external switch onto the host uplink. func ExternalLocalnetPort(vpcID string) string { return "ext-port-" + vpcID } -// IMDSSwitch is the OVN logical switch dedicated to IMDS for a VPC. The -// host-owned 169.254.169.254 LSP lives here, off the subnet switches. -func IMDSSwitch(vpcID string) string { return "imds-ls-" + vpcID } - -// IMDSRouterPort is the LRP on the VPC router that terminates the IMDS /30 and -// is the output port for the 169.254.169.254/32 static route. -func IMDSRouterPort(vpcID string) string { return "imds-lrp-" + vpcID } - -// IMDSSwitchRouterPort is the type=router LSP on the IMDS switch peered with -// IMDSRouterPort. -func IMDSSwitchRouterPort(vpcID string) string { return "imds-rtr-" + vpcID } - -// IMDSPort is the host-owned localport LSP that claims 169.254.169.254 on the -// IMDS switch. ovn-controller binds it on every chassis with a matching -// iface-id OVS port, so each chassis self-serves IMDS for its local VMs. -func IMDSPort(vpcID string) string { return "imds-port-" + vpcID } +// IMDSPort is the host-owned localport LSP that claims 169.254.169.254 directly +// on the subnet switch (SubnetSwitch). ovn-controller binds it on every chassis +// with a matching iface-id OVS port, so each chassis self-serves IMDS for its +// local VMs over a single L2 hop on the guest's own broadcast domain. +func IMDSPort(subnetID string) string { return "imds-port-" + subnetID } // SecurityGroupPortGroup is the OVN port group name for an SG. // OVN names match [a-zA-Z_][a-zA-Z0-9_]*, so hyphens become underscores. diff --git a/spinifex/vpcd/vpcd.go b/spinifex/vpcd/vpcd.go index 9d995bb4d..24ea93058 100644 --- a/spinifex/vpcd/vpcd.go +++ b/spinifex/vpcd/vpcd.go @@ -525,14 +525,14 @@ func launchService(cfg *Config) error { return fmt.Errorf("get JetStream context: %w", err) } - // IMDS per-VPC OVN topology installer. Replicas track the chassis count so - // the vpc-veth bucket survives a node loss (create-or-open: awsgw inits the + // IMDS per-subnet localport installer. Replicas track the chassis count so + // the subnet-veth bucket survives a node loss (create-or-open: awsgw inits the // same bucket; first writer wins). imdsVethKV, _, err := handlers_imds.InitBuckets(js, max(len(chassisNames), 1)) if err != nil { return fmt.Errorf("init imds buckets: %w", err) } - imdsTopoMgr, err := external.NewIMDSTopologyManager(liveClient, routeMgr, handlers_imds.NewVethStore(imdsVethKV)) + imdsTopoMgr, err := external.NewIMDSTopologyManager(liveClient, handlers_imds.NewVethStore(imdsVethKV)) if err != nil { return fmt.Errorf("construct IMDS topology manager: %w", err) } diff --git a/tests/e2e/harness/imds_diag.go b/tests/e2e/harness/imds_diag.go index 9a02451aa..4f960fac7 100644 --- a/tests/e2e/harness/imds_diag.go +++ b/tests/e2e/harness/imds_diag.go @@ -12,19 +12,20 @@ import ( // DumpIMDSDatapathDiagnostics emits a triage bundle for an IMDS reachability // failure (guest curl to 169.254.169.254 times out). It captures both halves of -// the per-VPC datapath so a triager can tell request-path from reply-path: +// the per-subnet L2 datapath so a triager can tell request-path from reply-path: // -// - OVN realisation: is imds-port- bound/up on this chassis, is the -// 169.254.169.254/32 static route installed on the VPC LR, is the localport -// LSP defined as expected. -// - Host realisation (inside the per-VPC netns imds-): the imds-h- -// veth's address/route/neighbour state and the listener socket — the -// load-bearing question is whether the netns has a real L3 path back to -// guestIP. After the per-VPC-netns fix the veth carries 169.254.169.254/30 -// with a default route via the .253 LRP, so a healthy dump shows the IPv4 -// address, a neighbour entry for .253, and a reply route via .253; the -// pre-fix failure showed an addressless veth that accepted the SYN but never -// returned the SYN-ACK. +// - OVN realisation: is imds-port- bound/up on this chassis, is the +// localport LSP defined as expected. There is no VPC-LR static route to check +// under L2 — the localport sits directly on the subnet switch, so the request +// never enters a router. +// - Host realisation (inside the per-subnet netns imds-): the +// imds-h- veth's address/route/neighbour state and the listener socket +// — the load-bearing question is whether the netns has a real L2 path back to +// guestIP. The veth carries 169.254.169.254/30 plus the subnet CIDR on-link, so +// a healthy dump shows the IPv4 address, an on-link reply route out the veth +// (no .253 gateway hop), and a resolved neighbour entry for guestIP; a broken +// dump shows an addressless veth that accepted the SYN but never returned the +// SYN-ACK. // - In-flight evidence: conntrack (per-netns) for 169.254.169.254 (a stuck // SYN_RECV is the smoking gun that the host received the SYN and replied but // the ACK never came back) and the br-int flows touching the IMDS / guest @@ -33,10 +34,10 @@ import ( // Non-fatal — runs purely for log/artifact signal so the test's own Fatal still // wins. Skips silently when OVN tooling / passwordless sudo aren't available, so // it's safe to call from developer laptops. -func DumpIMDSDatapathDiagnostics(t *testing.T, vpcID, guestIP, artifactDir string) { +func DumpIMDSDatapathDiagnostics(t *testing.T, subnetID, guestIP, artifactDir string) { t.Helper() - fmt.Printf("\n%s%s── DIAGNOSTICS: IMDS datapath (vpc=%s guest=%s) ──%s\n", - colorBold, colorCyan, vpcID, guestIP, colorReset) + fmt.Printf("\n%s%s── DIAGNOSTICS: IMDS datapath (subnet=%s guest=%s) ──%s\n", + colorBold, colorCyan, subnetID, guestIP, colorReset) defer fmt.Printf("%s%s── END DIAGNOSTICS ──%s\n\n", colorBold, colorCyan, colorReset) if _, err := exec.LookPath("ovn-nbctl"); err != nil { @@ -48,14 +49,14 @@ func DumpIMDSDatapathDiagnostics(t *testing.T, vpcID, guestIP, artifactDir strin return } - imdsPort := "imds-port-" + vpcID - hostEnd := host.IMDSHostVethName(vpcID) - ovsEnd := host.IMDSOVSPortName(vpcID) - netns := host.IMDSNetnsName(vpcID) - vpcRouter := "vpc-" + vpcID + imdsPort := "imds-port-" + subnetID + subnetSwitch := "subnet-" + subnetID + hostEnd := host.IMDSHostVethName(subnetID) + ovsEnd := host.IMDSOVSPortName(subnetID) + netns := host.IMDSNetnsName(subnetID) const imdsIP = "169.254.169.254" - // nsExec wraps an argv to run inside the per-VPC netns, where the host veth + // nsExec wraps an argv to run inside the per-subnet netns, where the host veth // end and the IMDS listener now live. nsExec := func(argv ...string) []string { return append([]string{"ip", "netns", "exec", netns}, argv...) @@ -75,30 +76,30 @@ func DumpIMDSDatapathDiagnostics(t *testing.T, vpcID, guestIP, artifactDir strin "find", "Logical_Switch_Port", "name=" + imdsPort}, }, { - filename: "imds-lr-routes.txt", - label: "ovn-nbctl lr-route-list (169.254.169.254/32 static route?)", - argv: []string{"ovn-nbctl", "lr-route-list", vpcRouter}, - grepFor: []string{imdsIP}, + filename: "imds-subnet-ports.txt", + label: "ovn-nbctl lsp-list subnet switch (localport present on the guest's switch?)", + argv: []string{"ovn-nbctl", "lsp-list", subnetSwitch}, + grepFor: []string{imdsPort}, }, { filename: "imds-netns.txt", - label: "ip netns list (per-VPC IMDS netns present?)", + label: "ip netns list (per-subnet IMDS netns present?)", argv: []string{"ip", "netns", "list"}, grepFor: []string{netns}, }, { filename: "imds-host-veth-addr.txt", - label: "ip -d addr show imds-h in netns (expect 169.254.169.254/30)", + label: "ip -d addr show imds-h in netns (expect 169.254.169.254/30 + subnet CIDR on-link)", argv: nsExec("ip", "-d", "addr", "show", "dev", hostEnd), }, { filename: "imds-host-route-to-guest.txt", - label: "ip route get in netns (expect via 169.254.169.253)", + label: "ip route get in netns (expect on-link dev imds-h, no gateway hop)", argv: nsExec("ip", "route", "get", guestIP), }, { filename: "imds-host-neigh.txt", - label: "ip neigh show in netns (expect .253 LRP entry)", + label: "ip neigh show in netns (expect a resolved entry for the guest IP)", argv: nsExec("ip", "neigh", "show"), }, { diff --git a/tests/e2e/harness/ovn.go b/tests/e2e/harness/ovn.go index fb8533a9b..105363cc5 100644 --- a/tests/e2e/harness/ovn.go +++ b/tests/e2e/harness/ovn.go @@ -38,6 +38,17 @@ func OvnSbctl(t *testing.T, args ...string) string { return runOvn(t, "ovn-sbctl", args...) } +// OvnTrace runs `sudo -n ovn-trace ` and returns the detailed pipeline +// trace. Usage is `ovn-trace [options] DATAPATH MICROFLOW`; pass any `--ct=...` +// options first, then the datapath, then the microflow. The detailed trace names +// each logical-flow table it traverses (`ls_in_*`, `lr_in_*`, …) and prints the +// final `/* output to "" */`, so callers can assert both the delivery port +// and the absence of router stages. +func OvnTrace(t *testing.T, args ...string) string { + t.Helper() + return runOvn(t, "ovn-trace", args...) +} + func runOvn(t *testing.T, tool string, args ...string) string { t.Helper() full := append([]string{"-n", tool}, args...) diff --git a/tests/e2e/single/imds_test.go b/tests/e2e/single/imds_test.go index c585f43b1..d41cac97d 100644 --- a/tests/e2e/single/imds_test.go +++ b/tests/e2e/single/imds_test.go @@ -25,8 +25,9 @@ const ( imdsRoleName = "imds-e2e-role" imdsProfileName = "imds-e2e-profile" - // metaURL is the AWS-compatible link-local IMDS endpoint, served from the - // host per-VPC via SO_BINDTODEVICE on the imds-h- veth. + // metaURL is the AWS-compatible link-local IMDS endpoint, answered L2 on each + // subnet's own switch from the host via SO_BINDTODEVICE on the + // imds-h- veth (no router, no proxy-ARP, no /32 route). metaURL = "http://169.254.169.254" // imdsUserData is a no-op cloud-config carrying a unique marker so the @@ -66,8 +67,13 @@ type imdsCredDoc struct { // issuance, the v2-only stance (tokenless / garbage-token GET → 401), the // metadata fields, the instance-role credential path + a wire round-trip // proving the ASIA creds resolve to the instance assumed-role ARN, the -// /latest/user-data round-trip, and the DHCP option-121 guest route. -// - The per-VPC datapath invariant: imds-port- is a localport LSP. +// /latest/user-data round-trip, and the on-link +// metadata route the guest's cloud-init network-config installs to reach the +// subnet localport. +// - The per-subnet L2 datapath invariant: imds-port- is a localport +// LSP on the guest's own subnet switch, and ovn-trace confirms the request +// and the established reply both resolve over a single L2 hop with no logical +// router in the path to shadow them. // - Cross-VPC isolation: VM X and VM Y share the same source IP in different // VPCs, yet each IMDS returns its OWN instance-id — the load-bearing // (VPC-ID, source-IP) → ENI boundary (plan §Datapath-attested identity). @@ -84,18 +90,22 @@ func runIMDS(t *testing.T, fix *Fixture) { imdsEnsureRoleProfile(t, fix, adminAccount) // Two VPCs, identical subnet CIDR → the first VM in each gets the same IP. - vpcX, subX, sgX := imdsEnsureProbeVPC(t, fix, imdsProbeVPCCIDR, imdsProbeSubnetCIDR) - vpcY, subY, sgY := imdsEnsureProbeVPC(t, fix, imdsProbeVPCCIDR, imdsProbeSubnetCIDR) + // The VPC IDs are logged inside imdsEnsureProbeVPC; the L2 datapath and the + // isolation assertions key off the subnet and the instance identity, not the + // VPC ID, so they aren't bound here. + _, subX, sgX := imdsEnsureProbeVPC(t, fix, imdsProbeVPCCIDR, imdsProbeSubnetCIDR) + _, subY, sgY := imdsEnsureProbeVPC(t, fix, imdsProbeVPCCIDR, imdsProbeSubnetCIDR) - // VM X — profile-bound + user-data; drives the full surface. - idX, privX, tgtX := imdsProbe(t, fix, keyPath, imdsVMSpec{ + // VM X — profile-bound + user-data; drives the full surface. eniX builds the + // guest LSP name (port-) for the ovn-trace datapath assertion. + idX, privX, eniX, tgtX := imdsProbe(t, fix, keyPath, imdsVMSpec{ subnetID: subX, sgID: sgX, profileName: imdsProfileName, userData: imdsUserData, }) // VM Y — isolation peer; no profile needed for the instance-id probe. - idY, privY, tgtY := imdsProbe(t, fix, keyPath, imdsVMSpec{ + idY, privY, _, tgtY := imdsProbe(t, fix, keyPath, imdsVMSpec{ subnetID: subY, sgID: sgY, }) @@ -105,30 +115,27 @@ func runIMDS(t *testing.T, fix *Fixture) { "(IPAM is deterministic at network+4) — got %s vs %s", privX, privY) harness.Detail(t, "vm_x", idX, "vm_y", idY, "shared_priv", privX) - // --- Per-VPC OVN datapath invariant: imds-port- is a localport ------ - harness.Step(t, "ovn-nbctl imds-port-%s must be a localport LSP", vpcX) - imdsLSP := "imds-port-" + vpcX // mirrors topology.IMDSPort (inlined, as datapath_test does) + // --- Per-subnet L2 datapath invariant: imds-port- is a localport --- + harness.Step(t, "ovn-nbctl imds-port-%s must be a localport LSP", subX) + imdsLSP := "imds-port-" + subX // mirrors topology.IMDSPort (inlined, as datapath_test does) lspType := harness.OvnNbctl(t, "--no-leader-only", "--bare", "--columns=type", "find", "logical_switch_port", "name="+imdsLSP) require.Equalf(t, "localport", lspType, "imds-port LSP %s must be type=localport so every chassis self-serves IMDS "+ "(a regular LSP binds one chassis and forces Geneve tunnelling)", imdsLSP) - // --- Subnet-router proxy-ARP makes 169.254.169.254 reachable link-local -- - // IMDS reachability no longer depends on a DHCP option-121 route in the - // guest; the subnet router LSP answers ARP for the IMDS address via OVN - // options:arp_proxy, so DHCP and fully static guests reach it identically. - harness.Step(t, "subnet router LSP rtr-port-%s must carry options:arp_proxy", subX) - rtrLSP := "rtr-port-" + subX // mirrors topology.SubnetSwitchRouterPort - lspOpts := harness.OvnNbctl(t, "--no-leader-only", "--bare", "--columns=options", - "find", "logical_switch_port", "name="+rtrLSP) - require.Containsf(t, lspOpts, "arp_proxy=169.254.169.254", - "subnet router LSP %s must set options:arp_proxy=169.254.169.254 so the subnet LRP "+ - "answers ARP for IMDS link-local (static-IP and NetworkManager/RHEL/Ubuntu guests)", rtrLSP) + // --- ovn-trace: the L2 round-trip stays off the router (Phase-0 gate) ----- + // The defining property of the L2 datapath is that 169.254.169.254 is answered + // on the guest's own broadcast domain, so neither the request nor the + // established reply may enter the VPC logical router — where v1's reroute, + // /32-static-route and hop-limit bugs all lived. Trace both directions on the + // subnet switch and assert each resolves over a single L2 hop with no + // lr_in_*/lr_out_* stage and lands on the expected port. + imdsAssertL2RoundTrip(t, subX, eniX, privX) // --- IMDSv2 token issuance ---------------------------------------------- harness.Step(t, "PUT /latest/api/token (IMDSv2)") - tokenX := imdsAwaitToken(t, fix, tgtX, vpcX, privX) + tokenX := imdsAwaitToken(t, fix, tgtX, subX, privX) harness.Detail(t, "token_len", len(tokenX)) // --- v2-only stance: tokenless + garbage-token GET → 401 ----------------- @@ -194,10 +201,11 @@ func runIMDS(t *testing.T, fix *Fixture) { // --- Cross-VPC isolation ------------------------------------------------- // VM X and VM Y query 169.254.169.254 from the IDENTICAL source IP, in - // different VPCs. Each must get its own instance-id; a leak here means the - // per-VPC SO_BINDTODEVICE veth / (VPC-ID, source-IP) → ENI mapping is broken. + // different VPCs (hence different subnets). Each must get its own instance-id; + // a leak here means the per-subnet SO_BINDTODEVICE veth / (VPC-ID, source-IP) → + // ENI mapping is broken. harness.Step(t, "cross-VPC isolation: shared IP %s, distinct identities", privX) - tokenY := imdsAwaitToken(t, fix, tgtY, vpcY, privY) + tokenY := imdsAwaitToken(t, fix, tgtY, subY, privY) gotY := imdsGet(t, tgtY, tokenY, "/latest/meta-data/instance-id") require.Equalf(t, idY, gotY, "VM Y IMDS must return VM Y's instance-id (got %q)", gotY) require.NotEqualf(t, idX, gotY, @@ -219,9 +227,10 @@ type imdsVMSpec struct { } // imdsProbe launches a VM per spec, waits for it to run + become SSH-reachable, -// and returns (instanceID, privateIP, sshTarget). Registers terminate cleanup -// so the VM is torn down before its VPC/subnet are deleted (LIFO). -func imdsProbe(t *testing.T, fix *Fixture, keyPath string, spec imdsVMSpec) (string, string, harness.SSHTarget) { +// and returns (instanceID, privateIP, eniID, sshTarget). eniID names the guest +// LSP (port-) for the ovn-trace datapath assertion. Registers terminate +// cleanup so the VM is torn down before its VPC/subnet are deleted (LIFO). +func imdsProbe(t *testing.T, fix *Fixture, keyPath string, spec imdsVMSpec) (string, string, string, harness.SSHTarget) { t.Helper() id := imdsRunVM(t, fix, spec) t.Cleanup(func() { @@ -234,11 +243,82 @@ func imdsProbe(t *testing.T, fix *Fixture, keyPath string, spec imdsVMSpec) (str inst := harness.WaitForInstanceState(t, fix.AWS, id, "running") priv := aws.StringValue(inst.PrivateIpAddress) require.NotEmptyf(t, priv, "instance %s has no PrivateIpAddress", id) + eni := primaryENI(t, inst) host, port := harness.InstancePublicSSHHost(t, inst) harness.Step(t, "wait for %s SSH at %s:%d", id, host, port) waitForSSHHandshake(t, host, port, keyPath) - return id, priv, harness.SSHTarget{User: "ec2-user", Host: host, Port: port, KeyPath: keyPath} + return id, priv, eni, harness.SSHTarget{User: "ec2-user", Host: host, Port: port, KeyPath: keyPath} +} + +// imdsAssertL2RoundTrip ovn-traces the IMDS request and its established reply on +// the guest's subnet switch and asserts both resolve over a single L2 hop with no +// logical-router stage. This promotes the Phase-0 gate to a standing assertion: +// the L2 datapath's defining property is that IMDS never enters the VPC LR, so an +// lr_in_*/lr_out_* stage appearing in either direction means the relocation +// regressed back toward the v1 router pipeline. +func imdsAssertL2RoundTrip(t *testing.T, subnetID, eniID, guestIP string) { + t.Helper() + subnetSwitch := "subnet-" + subnetID // topology.SubnetSwitch + imdsLSP := "imds-port-" + subnetID // topology.IMDSPort + guestLSP := "port-" + eniID // topology.Port + + // MACs come from NB, the datapath's own source of truth: the localport and the + // guest LSP each advertise " " in their addresses column. + imdsMAC := imdsPortMAC(t, imdsLSP) + guestMAC := imdsPortMAC(t, guestLSP) + + // Request: a NEW guest -> 169.254.169.254 SYN must be delivered L2 to the + // localport, with no router stage. + harness.Step(t, "ovn-trace request %s -> 169.254.169.254 lands L2 on %s", guestIP, imdsLSP) + reqFlow := fmt.Sprintf( + `inport=="%s" && eth.src==%s && eth.dst==%s && ip4.src==%s && ip4.dst==169.254.169.254 && ip.ttl==64 && tcp && tcp.src==40000 && tcp.dst==80`, + guestLSP, guestMAC, imdsMAC, guestIP) + reqTrace := harness.OvnTrace(t, "--no-leader-only", subnetSwitch, reqFlow) + imdsAssertNoRouterStage(t, "request", reqTrace) + require.Containsf(t, reqTrace, `output to "`+imdsLSP+`"`, + "IMDS request must be delivered L2 to the localport %s; trace:\n%s", imdsLSP, reqTrace) + + // Reply: 169.254.169.254 -> guest is the established conntrack reply. Supply + // the reply ct-state to BOTH ct_next stages (ingress + egress) — a single --ct + // leaves the egress ct_next defaulting back to `est` without `rpl` and prints a + // spurious ct_mark.blocked drop (the Phase-0 false alarm). The reply landing on + // the guest also proves the guest's own SG ACLs never match the localport's + // frames. + harness.Step(t, "ovn-trace reply 169.254.169.254 -> %s lands L2 on %s", guestIP, guestLSP) + replyFlow := fmt.Sprintf( + `inport=="%s" && eth.src==%s && eth.dst==%s && ip4.src==169.254.169.254 && ip4.dst==%s && ip.ttl==64 && tcp && tcp.src==80 && tcp.dst==40000`, + imdsLSP, imdsMAC, guestMAC, guestIP) + replyTrace := harness.OvnTrace(t, "--no-leader-only", "--ct=est,rpl", "--ct=est,rpl", subnetSwitch, replyFlow) + imdsAssertNoRouterStage(t, "reply", replyTrace) + require.Containsf(t, replyTrace, `output to "`+guestLSP+`"`, + "IMDS established reply must be delivered L2 to the guest %s (the guest's own SG "+ + "must not drop the localport's reply); trace:\n%s", guestLSP, replyTrace) +} + +// imdsAssertNoRouterStage fails if an ovn-trace shows the packet entering a +// logical router (any lr_in_*/lr_out_* stage). The L2 IMDS datapath must answer +// 169.254.169.254 on the subnet switch and never transit the VPC LR. +func imdsAssertNoRouterStage(t *testing.T, dir, trace string) { + t.Helper() + for _, stage := range []string{"lr_in_", "lr_out_"} { + require.NotContainsf(t, trace, stage, + "IMDS %s traversed a logical router (%s stage present) — the L2 datapath must "+ + "answer 169.254.169.254 on the subnet switch and never enter the VPC LR; trace:\n%s", + dir, stage, trace) + } +} + +// imdsPortMAC returns the MAC an LSP advertises in its addresses column +// (" "). Fatal if the LSP is missing or carries no MAC, since the +// ovn-trace microflow cannot be built without it. +func imdsPortMAC(t *testing.T, lsp string) string { + t.Helper() + addrs := harness.OvnNbctl(t, "--no-leader-only", "--bare", "--columns=addresses", + "find", "logical_switch_port", "name="+lsp) + fields := strings.Fields(addrs) + require.NotEmptyf(t, fields, "LSP %s has no addresses (got %q); cannot build ovn-trace microflow", lsp, addrs) + return fields[0] } // imdsRunVM launches a single instance per spec and returns its ID. AMI / type / @@ -395,11 +475,11 @@ func imdsEnsureRoleProfile(t *testing.T, fix *Fixture, adminAccount string) { // imdsAwaitToken PUTs an IMDSv2 token from inside the guest, retrying to ride // out the cold-start window before BindManager.Sync + the ENI reverse-index -// land. Returns the token. On timeout it dumps the per-VPC IMDS datapath +// land. Returns the token. On timeout it dumps the per-subnet IMDS datapath // (OVN realisation + host veth route/neigh + conntrack) before failing, so a // reachability timeout (exit 28) is triaged as request-path vs reply-path // rather than a bare "condition not met". -func imdsAwaitToken(t *testing.T, fix *Fixture, tgt harness.SSHTarget, vpcID, guestIP string) string { +func imdsAwaitToken(t *testing.T, fix *Fixture, tgt harness.SSHTarget, subnetID, guestIP string) string { t.Helper() deadline := time.Now().Add(90 * time.Second) var lastErr error @@ -416,9 +496,9 @@ func imdsAwaitToken(t *testing.T, fix *Fixture, tgt harness.SSHTarget, vpcID, gu return strings.TrimSpace(out) } if time.Now().After(deadline) { - harness.DumpIMDSDatapathDiagnostics(t, vpcID, guestIP, fix.ArtifactDir(t)) - t.Fatalf("imdsAwaitToken: token never minted within 90s (vpc=%s guest=%s): %v "+ - "(see IMDS datapath diagnostics above)", vpcID, guestIP, lastErr) + harness.DumpIMDSDatapathDiagnostics(t, subnetID, guestIP, fix.ArtifactDir(t)) + t.Fatalf("imdsAwaitToken: token never minted within 90s (subnet=%s guest=%s): %v "+ + "(see IMDS datapath diagnostics above)", subnetID, guestIP, lastErr) } time.Sleep(3 * time.Second) } diff --git a/tests/e2e/single/tests_test.go b/tests/e2e/single/tests_test.go index 4141ac9cf..9f8569cde 100644 --- a/tests/e2e/single/tests_test.go +++ b/tests/e2e/single/tests_test.go @@ -227,9 +227,9 @@ func TestSTSAssumeRoleAndGetCallerIdentity(t *testing.T) { // TestIMDS exercises the host-served IMDSv2 surface end-to-end: token // issuance, the v2-only stance, the metadata surface, the -// instance-role credential path + wire round-trip, DHCP option 121, the -// per-VPC localport datapath, and cross-VPC source-IP isolation. Owns its own -// role/profile + a second VPC, but sequential: it launches profile-bound VMs +// instance-role credential path + wire round-trip, the per-subnet L2 localport +// datapath (ovn-trace round-trip), and cross-VPC source-IP isolation. Owns its +// own role/profile + a second VPC, but sequential: it launches profile-bound VMs // and a fresh VPC, so it must not race the singleton-mutation tests above. func TestIMDS(t *testing.T) { runIMDS(t, requireSingleNodeFixture(t))