fix(network): seed static OVN MAC binding for gateway nexthop (#530)

TestNATGateway flaked with 100% egress loss: the tenant VPC gateway router's OVN MAC_Binding had no entry for its upstream default-route nexthop. OVN resolves that nexthop via lazy dynamic ARP on first egress; if that ARP is lost during gateway bring-up, SNAT'd packets die in lr_in_arp_resolve and no reply returns. MAC_Binding is per (datapath, logical_port, ip), so a sibling VPC router resolving the same nexthop does not help. Deterministically seed a static MAC binding for the nexthop at IGW attach. The nexthop MAC is the host's own default-gateway MAC (the host shares the physical uplink), read from ip neigh and written via ovn-nbctl static-mac-binding-add (northd programs it into SB MAC_Binding, GC-safe). Host layer L0 owns the shell-out per ADR-0006 S2; igw calls it via an injected hook. Best-effort and idempotent: a missing MAC leaves dynamic ARP as the fallback, and it re-runs on every AttachIGW so a binding lost to a later flap self-heals.
2026-10-15 17:11:30 +00:00 · 2026-07-14 08:24:35 +10:00
parent 6253c3d0ae
commit 97a03e4d1e
5 changed files with 384 additions and 4 deletions
@@ -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.
@@ -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"}))
+}
@@ -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 ""
+}
@@ -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")
+	}
+}
@@ -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)