diff --git a/spinifex/network/external/igw.go b/spinifex/network/external/igw.go index 550cfbe60..73bcfec7b 100644 --- a/spinifex/network/external/igw.go +++ b/spinifex/network/external/igw.go @@ -71,6 +71,10 @@ type IGWManagerConfig struct { NATMode policy.NATMode FlowsBarrier FlowsBarrier RoutedIngress *RoutedIngressHooks + // NexthopSeed installs a static OVN MAC binding for the gateway router's + // upstream nexthop, removing the dependency on lazy dynamic ARP. Optional; + // nil skips seeding. + NexthopSeed func(ctx context.Context, lrpName, nexthopIP string) error } type igwManager struct { @@ -83,6 +87,7 @@ type igwManager struct { natMode policy.NATMode barrier FlowsBarrier routedIngress *RoutedIngressHooks + nexthopSeed func(ctx context.Context, lrpName, nexthopIP string) error } var _ IGWManager = (*igwManager)(nil) @@ -115,6 +120,7 @@ func NewIGWManager(cfg IGWManagerConfig) (IGWManager, error) { natMode: cfg.NATMode, barrier: barrier, routedIngress: cfg.RoutedIngress, + nexthopSeed: cfg.NexthopSeed, }, nil } @@ -209,6 +215,13 @@ func (m *igwManager) AttachIGW(ctx context.Context, spec IGWSpec) error { return fmt.Errorf("add default route on %s: %w", routerName, err) } + if m.nexthopSeed != nil && m.gatewayOwnsNAT() && wanNexthop != "" { + if err := m.nexthopSeed(ctx, gwPortName, wanNexthop); err != nil { + slog.Warn("external: seed nexthop MAC binding failed; egress relies on dynamic ARP", + "vpc_id", spec.VPCID, "gw_port", gwPortName, "nexthop", wanNexthop, "err", err) + } + } + // Routed mode: egress SNATs the VPC CIDR to the gateway LRP transit IP so // the host only ever masquerades the transit /24, and the host gets an // ingress route to the VPC CIDR. DetachIGW reverses both. diff --git a/spinifex/network/external/igw_test.go b/spinifex/network/external/igw_test.go index e1c801f06..5c17927d0 100644 --- a/spinifex/network/external/igw_test.go +++ b/spinifex/network/external/igw_test.go @@ -28,11 +28,26 @@ func seedVPCRouter(t *testing.T, m *mock.Client, vpcID, cidr string) { } func newTestIGWManager(t *testing.T, m *mock.Client, mode policy.NATMode, pool *ExternalPoolConfig, allocator GatewayIPAllocator, chassis []string) (IGWManager, *int) { + t.Helper() + mgr, calls, err := newTestIGWManagerWithSeed(t, m, mode, pool, allocator, chassis, nil) + require.NoError(t, err) + return mgr, calls +} + +// nexthopSeedCall records a single NexthopSeed invocation's arguments. +type nexthopSeedCall struct { + lrpName string + nexthopIP string +} + +// newTestIGWManagerWithSeed is newTestIGWManager plus an optional capturing +// NexthopSeed hook; pass nil to leave it unset (mirrors newTestIGWManager). +func newTestIGWManagerWithSeed(t *testing.T, m *mock.Client, mode policy.NATMode, pool *ExternalPoolConfig, allocator GatewayIPAllocator, chassis []string, seedCalls *[]nexthopSeedCall) (IGWManager, *int, error) { t.Helper() nm, err := policy.NewNATManager(m, mode) require.NoError(t, err) barrierCalls := 0 - mgr, err := NewIGWManager(IGWManagerConfig{ + cfg := IGWManagerConfig{ OVN: m, Routes: policy.NewRouteManager(m), NAT: nm, @@ -44,9 +59,15 @@ func newTestIGWManager(t *testing.T, m *mock.Client, mode policy.NATMode, pool * barrierCalls++ return nil }, - }) - require.NoError(t, err) - return mgr, &barrierCalls + } + if seedCalls != nil { + cfg.NexthopSeed = func(_ context.Context, lrpName, nexthopIP string) error { + *seedCalls = append(*seedCalls, nexthopSeedCall{lrpName: lrpName, nexthopIP: nexthopIP}) + return nil + } + } + mgr, err := NewIGWManager(cfg) + return mgr, &barrierCalls, err } func TestNewIGWManager_RejectsMissingDeps(t *testing.T) { @@ -549,3 +570,63 @@ func TestRemoveSubnetEgress_PropagatesDeleteError(t *testing.T) { err := mgr.RemoveSubnetEgress(ctx, "vpc-missing", "subnet-pub", netip.MustParsePrefix("0.0.0.0/0")) require.Error(t, err) } + +// TestAttachIGW_NexthopSeed_CalledWhenGatewayOwnsNAT covers centralized and +// routed modes, both of which give the gateway LRP a real pool IP and a +// non-empty wanNexthop — AttachIGW must invoke NexthopSeed with the gateway +// port name and the resolved nexthop right after AddDefaultRoute. +func TestAttachIGW_NexthopSeed_CalledWhenGatewayOwnsNAT(t *testing.T) { + for _, mode := range []policy.NATMode{policy.NATModeCentralized, policy.NATModeRouted} { + t.Run(mode.String(), func(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + pool := &ExternalPoolConfig{ + Name: "p", Gateway: "192.168.1.1", PrefixLen: 24, + GwLrpRangeStart: "192.168.1.240", GwLrpRangeEnd: "192.168.1.243", + } + var seedCalls []nexthopSeedCall + mgr, _, err := newTestIGWManagerWithSeed(t, m, mode, pool, NewStaticRangeAllocator(m), []string{"chassis-a"}, &seedCalls) + require.NoError(t, err) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + require.Len(t, seedCalls, 1, "NexthopSeed must be invoked exactly once on attach") + assert.Equal(t, topology.GatewayRouterPort("vpc-1"), seedCalls[0].lrpName) + assert.Equal(t, "192.168.1.1", seedCalls[0].nexthopIP) + }) + } +} + +// TestAttachIGW_NexthopSeed_NotCalledWhenModeIsDistributed guards the +// gatewayOwnsNAT() gate: distributed mode never owns NAT on the gateway +// chassis, so seeding a static binding there would be meaningless. +func TestAttachIGW_NexthopSeed_NotCalledWhenModeIsDistributed(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + pool := &ExternalPoolConfig{Name: "p", Gateway: "192.168.1.1", PrefixLen: 24} + var seedCalls []nexthopSeedCall + mgr, _, err := newTestIGWManagerWithSeed(t, m, policy.NATModeDistributed, pool, LinkLocalAllocator{}, []string{"chassis-a"}, &seedCalls) + require.NoError(t, err) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) + + assert.Empty(t, seedCalls, "NexthopSeed must not be invoked outside gatewayOwnsNAT modes") +} + +// TestAttachIGW_NexthopSeed_NilHookSkipped asserts a nil NexthopSeed (the +// default in all existing tests/wiring paths) is simply never called, and +// AttachIGW proceeds normally. +func TestAttachIGW_NexthopSeed_NilHookSkipped(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedVPCRouter(t, m, "vpc-1", "10.0.0.0/16") + pool := &ExternalPoolConfig{ + Name: "p", Gateway: "192.168.1.1", PrefixLen: 24, + GwLrpRangeStart: "192.168.1.240", GwLrpRangeEnd: "192.168.1.243", + } + mgr, _ := newTestIGWManager(t, m, policy.NATModeCentralized, pool, NewStaticRangeAllocator(m), []string{"chassis-a"}) + + require.NoError(t, mgr.AttachIGW(ctx, IGWSpec{VPCID: "vpc-1", InternetGatewayID: "igw-1"})) +} diff --git a/spinifex/network/host/mac_binding.go b/spinifex/network/host/mac_binding.go new file mode 100644 index 000000000..2a62d763c --- /dev/null +++ b/spinifex/network/host/mac_binding.go @@ -0,0 +1,98 @@ +package host + +import ( + "context" + "fmt" + "log/slog" + "strings" +) + +// SeedNexthopMAC installs a static OVN MAC binding on the gateway LRP (lrpName) +// for the router's upstream default-route nexthop, so egress does not depend on +// lazy dynamic ARP — which can be lost during gateway bring-up, stranding SNAT'd +// egress in lr_in_arp_resolve (100% loss). The nexthop MAC is the host's own +// default-gateway MAC (the host shares the physical uplink), read from the kernel +// neigh table and primed with one ping when absent. Idempotent: an existing +// binding for the same lrpName+ip is replaced. Best-effort: a missing MAC logs +// and returns nil, leaving dynamic ARP as the fallback. +func SeedNexthopMAC(ctx context.Context, runner Runner, lrpName, nexthopIP string) error { + if lrpName == "" || nexthopIP == "" { + return nil + } + if runner == nil { + runner = NewExecRunner() + } + + dev, err := nexthopDev(ctx, runner, nexthopIP) + if err != nil { + return fmt.Errorf("resolve egress dev for nexthop %s: %w", nexthopIP, err) + } + + mac := nexthopMAC(ctx, runner, nexthopIP, dev) + if mac == "" { + // Prime the kernel neigh table once, then re-read. + _, _ = runner.Run(ctx, "ping", "-c", "1", "-W", "1", nexthopIP) + mac = nexthopMAC(ctx, runner, nexthopIP, dev) + } + if mac == "" { + slog.Warn("host: nexthop MAC unresolved; leaving dynamic ARP fallback", + "lrp", lrpName, "nexthop", nexthopIP, "dev", dev) + return nil + } + + // Idempotent: drop any stale binding (best-effort) before adding. + _, _ = runner.Run(ctx, "ovn-nbctl", "--if-exists", "static-mac-binding-del", lrpName, nexthopIP) + if _, err := runner.Run(ctx, "ovn-nbctl", "static-mac-binding-add", lrpName, nexthopIP, mac); err != nil { + return fmt.Errorf("ovn-nbctl static-mac-binding-add %s %s %s: %w", lrpName, nexthopIP, mac, err) + } + + slog.Info("host: seeded static OVN MAC binding for gateway nexthop", + "lrp", lrpName, "nexthop", nexthopIP, "mac", mac) + return nil +} + +// nexthopDev resolves the egress interface the kernel uses to reach ip, via +// `ip route get`. Mirrors routeDev in gateway_claim.go but through Runner. +func nexthopDev(ctx context.Context, runner Runner, ip string) (string, error) { + out, err := runner.Run(ctx, "ip", "route", "get", ip) + if err != nil { + return "", fmt.Errorf("ip route get %s: %w", ip, err) + } + fields := strings.Fields(string(out)) + for i, f := range fields { + if f == "dev" && i+1 < len(fields) { + return fields[i+1], nil + } + } + return "", fmt.Errorf("ip route get %s: no dev in %q", ip, strings.TrimSpace(string(out))) +} + +// nexthopMAC reads the kernel neigh table for ip on dev and extracts a usable +// lladdr MAC. Returns "" on a missing/unresolved entry. +func nexthopMAC(ctx context.Context, runner Runner, ip, dev string) string { + out, err := runner.Run(ctx, "ip", "neigh", "show", ip, "dev", dev) + if err != nil { + return "" + } + return parseNeighMAC(string(out)) +} + +// parseNeighMAC extracts the lladdr token from an `ip neigh show` line, e.g. +// "192.168.1.1 dev br-wan lladdr 04:f4:1c:fd:56:27 REACHABLE". Returns "" for +// an unresolved (FAILED/INCOMPLETE) or empty entry. +func parseNeighMAC(out string) string { + out = strings.TrimSpace(out) + if out == "" || !strings.Contains(out, "lladdr") { + return "" + } + if strings.Contains(out, "FAILED") || strings.Contains(out, "INCOMPLETE") { + return "" + } + fields := strings.Fields(out) + for i, f := range fields { + if f == "lladdr" && i+1 < len(fields) { + return fields[i+1] + } + } + return "" +} diff --git a/spinifex/network/host/mac_binding_test.go b/spinifex/network/host/mac_binding_test.go new file mode 100644 index 000000000..e197118ba --- /dev/null +++ b/spinifex/network/host/mac_binding_test.go @@ -0,0 +1,185 @@ +package host + +import ( + "context" + "errors" + "strings" + "testing" +) + +// scriptedRunner returns canned output keyed by the joined argv, so a test can +// drive different responses for `ip route get`, `ip neigh show` (before/after +// a ping prime), and the ovn-nbctl static-mac-binding calls. +type scriptedRunner struct { + responses map[string][]byte + errors map[string]error + calls [][]string +} + +func (r *scriptedRunner) Run(_ context.Context, name string, args ...string) ([]byte, error) { + full := append([]string{name}, args...) + r.calls = append(r.calls, full) + key := strings.Join(full, " ") + return r.responses[key], r.errors[key] +} + +func (r *scriptedRunner) callArgs(prefix string) [][]string { + var out [][]string + for _, c := range r.calls { + if strings.HasPrefix(strings.Join(c, " "), prefix) { + out = append(out, c) + } + } + return out +} + +func TestSeedNexthopMAC_ResolvedOnFirstRead(t *testing.T) { + r := &scriptedRunner{ + responses: map[string][]byte{ + "ip route get 192.168.1.1": []byte("192.168.1.1 dev br-wan src 192.168.1.50"), + "ip neigh show 192.168.1.1 dev br-wan": []byte("192.168.1.1 dev br-wan lladdr 04:f4:1c:fd:56:27 REACHABLE"), + }, + } + if err := SeedNexthopMAC(context.Background(), r, "gw-vpc-1", "192.168.1.1"); err != nil { + t.Fatalf("SeedNexthopMAC: %v", err) + } + if calls := r.callArgs("ping"); len(calls) != 0 { + t.Fatalf("expected no ping prime when neigh resolves first read, got %v", calls) + } + delCalls := r.callArgs("ovn-nbctl --if-exists static-mac-binding-del") + if len(delCalls) != 1 { + t.Fatalf("expected 1 static-mac-binding-del call, got %d: %v", len(delCalls), r.calls) + } + wantDel := "ovn-nbctl --if-exists static-mac-binding-del gw-vpc-1 192.168.1.1" + if got := strings.Join(delCalls[0], " "); got != wantDel { + t.Fatalf("del argv mismatch\n got: %s\nwant: %s", got, wantDel) + } + addCalls := r.callArgs("ovn-nbctl static-mac-binding-add") + if len(addCalls) != 1 { + t.Fatalf("expected 1 static-mac-binding-add call, got %d: %v", len(addCalls), r.calls) + } + wantAdd := "ovn-nbctl static-mac-binding-add gw-vpc-1 192.168.1.1 04:f4:1c:fd:56:27" + if got := strings.Join(addCalls[0], " "); got != wantAdd { + t.Fatalf("add argv mismatch\n got: %s\nwant: %s", got, wantAdd) + } +} + +func TestSeedNexthopMAC_ResolvedAfterPing(t *testing.T) { + neighKey := "ip neigh show 192.168.1.1 dev br-wan" + r := &scriptedRunner{ + responses: map[string][]byte{ + "ip route get 192.168.1.1": []byte("192.168.1.1 dev br-wan src 192.168.1.50"), + }, + } + // Wrap Run to alternate the neigh response: unresolved, then resolved. + base := r + wrapper := &sequencedNeighRunner{ + scriptedRunner: base, + neighKey: neighKey, + firstOut: []byte(""), + secondOut: []byte("192.168.1.1 dev br-wan lladdr 04:f4:1c:fd:56:27 REACHABLE"), + } + if err := SeedNexthopMAC(context.Background(), wrapper, "gw-vpc-1", "192.168.1.1"); err != nil { + t.Fatalf("SeedNexthopMAC: %v", err) + } + if wrapper.neighCalls != 2 { + t.Fatalf("expected 2 neigh reads (before/after ping), got %d", wrapper.neighCalls) + } + pingCalls := wrapper.callArgs("ping") + if len(pingCalls) != 1 { + t.Fatalf("expected exactly 1 ping prime, got %d: %v", len(pingCalls), wrapper.calls) + } + wantPing := "ping -c 1 -W 1 192.168.1.1" + if got := strings.Join(pingCalls[0], " "); got != wantPing { + t.Fatalf("ping argv mismatch\n got: %s\nwant: %s", got, wantPing) + } + addCalls := wrapper.callArgs("ovn-nbctl static-mac-binding-add") + if len(addCalls) != 1 { + t.Fatalf("expected seed to proceed after ping resolves the MAC, got %d add calls", len(addCalls)) + } +} + +// sequencedNeighRunner returns firstOut on the first `ip neigh show` call for +// neighKey and secondOut thereafter, delegating everything else (including +// call recording) to the embedded scriptedRunner. +type sequencedNeighRunner struct { + *scriptedRunner + + neighKey string + firstOut []byte + secondOut []byte + neighCalls int +} + +func (s *sequencedNeighRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + full := append([]string{name}, args...) + s.calls = append(s.calls, full) + if strings.Join(full, " ") == s.neighKey { + s.neighCalls++ + if s.neighCalls == 1 { + return s.firstOut, nil + } + return s.secondOut, nil + } + key := strings.Join(full, " ") + return s.responses[key], s.errors[key] +} + +func TestSeedNexthopMAC_UnresolvedEvenAfterPing(t *testing.T) { + r := &scriptedRunner{ + responses: map[string][]byte{ + "ip route get 192.168.1.1": []byte("192.168.1.1 dev br-wan src 192.168.1.50"), + "ip neigh show 192.168.1.1 dev br-wan": []byte("192.168.1.1 dev br-wan INCOMPLETE"), + }, + } + if err := SeedNexthopMAC(context.Background(), r, "gw-vpc-1", "192.168.1.1"); err != nil { + t.Fatalf("SeedNexthopMAC must return nil (best-effort) on unresolved MAC, got: %v", err) + } + if calls := r.callArgs("ping"); len(calls) != 1 { + t.Fatalf("expected exactly 1 ping prime attempt, got %d", len(calls)) + } + if calls := r.callArgs("ovn-nbctl"); len(calls) != 0 { + t.Fatalf("expected no ovn-nbctl calls when MAC stays unresolved, got %v", calls) + } +} + +func TestSeedNexthopMAC_EmptyArgsNoop(t *testing.T) { + r := &scriptedRunner{} + if err := SeedNexthopMAC(context.Background(), r, "", "192.168.1.1"); err != nil { + t.Fatalf("expected nil for empty lrpName, got %v", err) + } + if err := SeedNexthopMAC(context.Background(), r, "gw-vpc-1", ""); err != nil { + t.Fatalf("expected nil for empty nexthopIP, got %v", err) + } + if len(r.calls) != 0 { + t.Fatalf("expected no commands issued for empty args, got %v", r.calls) + } +} + +func TestSeedNexthopMAC_RouteGetFailurePropagates(t *testing.T) { + r := &scriptedRunner{ + errors: map[string]error{ + "ip route get 192.168.1.1": errors.New("network unreachable"), + }, + } + err := SeedNexthopMAC(context.Background(), r, "gw-vpc-1", "192.168.1.1") + if err == nil { + t.Fatal("expected error when route resolution fails") + } +} + +func TestSeedNexthopMAC_AddFailurePropagates(t *testing.T) { + r := &scriptedRunner{ + responses: map[string][]byte{ + "ip route get 192.168.1.1": []byte("192.168.1.1 dev br-wan src 192.168.1.50"), + "ip neigh show 192.168.1.1 dev br-wan": []byte("192.168.1.1 dev br-wan lladdr 04:f4:1c:fd:56:27 REACHABLE"), + }, + errors: map[string]error{ + "ovn-nbctl static-mac-binding-add gw-vpc-1 192.168.1.1 04:f4:1c:fd:56:27": errors.New("exit 1"), + }, + } + err := SeedNexthopMAC(context.Background(), r, "gw-vpc-1", "192.168.1.1") + if err == nil { + t.Fatal("expected error when static-mac-binding-add fails") + } +} diff --git a/spinifex/vpcd/vpcd.go b/spinifex/vpcd/vpcd.go index cceeb63e2..2018de813 100644 --- a/spinifex/vpcd/vpcd.go +++ b/spinifex/vpcd/vpcd.go @@ -575,6 +575,9 @@ func launchService(cfg *Config) error { NATMode: natMode, FlowsBarrier: waitForFlowsHV, RoutedIngress: routedIngress, + NexthopSeed: func(ctx context.Context, lrpName, nexthopIP string) error { + return host.SeedNexthopMAC(ctx, host.NewExecRunner(), lrpName, nexthopIP) + }, }) if err != nil { return fmt.Errorf("construct IGW manager: %w", err)