fix: seed the gateway nexthop MAC binding for a host-local nexthop (#981)

2026-10-15 09:01:31 +00:00 · 2026-09-02 14:22:43 +10:00
parent dc9b8429ae
commit e8f1718ebd
3 changed files with 243 additions and 18 deletions
@@ -227,8 +227,15 @@ func (m *igwManager) AttachIGW(ctx context.Context, spec IGWSpec) error {

 	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's nexthop is host-local, so nothing answers an ARP for
+			// it and the seed is the only way egress ever resolves.
+			if m.natMode == policy.NATModeRouted {
+				slog.Error("external: seed nexthop MAC binding failed; routed-NAT egress will black-hole",
+					"vpc_id", spec.VPCID, "gw_port", gwPortName, "nexthop", wanNexthop, "err", err)
+			} else {
+				slog.Warn("external: seed nexthop MAC binding failed; egress relies on dynamic ARP",
+					"vpc_id", spec.VPCID, "gw_port", gwPortName, "nexthop", wanNexthop, "err", err)
+			}
 		}
 	}

@@ -10,13 +10,16 @@ import (
 // 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. nbAddr selects the NB DB
-// to write to (empty uses the local default socket); a compute node runs no
-// database, so a write without it would never reach the cluster.
+// egress in lr_in_arp_resolve (100% loss). For a remote nexthop the 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; for a host-local
+// nexthop (routed NAT) it is the MAC of the link carrying that address.
+// Idempotent: an existing binding for the same lrpName+ip is replaced. A remote
+// nexthop that stays unresolved is best-effort — it logs and returns nil, leaving
+// dynamic ARP as the fallback. A host-local one returns an error instead, because
+// nothing there will ever answer an ARP and the egress loss is permanent. nbAddr
+// selects the NB DB to write to (empty uses the local default socket); a compute
+// node runs no database, so a write without it would never reach the cluster.
 func SeedNexthopMAC(ctx context.Context, runner Runner, nbAddr, lrpName, nexthopIP string) error {
 	if lrpName == "" || nexthopIP == "" {
 		return nil
@@ -30,16 +33,29 @@ func SeedNexthopMAC(ctx context.Context, runner Runner, nbAddr, lrpName, nexthop
 		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)
+	var mac string
+	if dev == localRouteDev {
+		// Routed NAT: the nexthop is an address on this host, so no neigh entry
+		// can ever exist and pinging it would prime nothing.
+		mac, err = localNexthopMAC(ctx, runner, nexthopIP)
+		if err != nil {
+			// A host-local nexthop has no dynamic-ARP fallback, so an unresolved
+			// binding black-holes egress permanently rather than degrading it.
+			return fmt.Errorf("resolve host-local nexthop %s for %s: %w; egress will black-hole — check %s is up carrying %s",
+				nexthopIP, lrpName, err, NATTransitHostEnd, NATTransitGatewayCIDR)
+		}
+	} else {
 		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
+		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
+		}
 	}

 	var db []string
@@ -58,6 +74,60 @@ func SeedNexthopMAC(ctx context.Context, runner Runner, nbAddr, lrpName, nexthop
 	return nil
 }

+// localRouteDev is the dev `ip route get` reports for an address owned by this
+// host; the address itself still lives on a real link.
+const localRouteDev = "lo"
+
+// localNexthopMAC returns the MAC of the link carrying ip, for a nexthop the
+// kernel routes to lo. In routed NAT that is the host end of the transit veth,
+// which is what OVN must address to reach the nexthop over the uplink bridge.
+// Every failure is distinguishable: `ip addr show to` exits 0 with no output
+// when nothing matches, so a non-nil error there is never the no-match case.
+func localNexthopMAC(ctx context.Context, runner Runner, ip string) (string, error) {
+	out, err := runner.Run(ctx, "ip", "-4", "-o", "addr", "show", "to", ip+"/32")
+	if err != nil {
+		return "", fmt.Errorf("ip -4 -o addr show to %s/32: %w", ip, err)
+	}
+	dev := parseAddrShowDev(string(out))
+	if dev == "" {
+		return "", fmt.Errorf("no link carries %s", ip)
+	}
+	link, err := runner.Run(ctx, "ip", "-o", "link", "show", "dev", dev)
+	if err != nil {
+		return "", fmt.Errorf("ip -o link show dev %s: %w", dev, err)
+	}
+	mac := parseLinkEtherMAC(string(link))
+	if mac == "" {
+		return "", fmt.Errorf("link %s carrying %s has no ethernet address", dev, ip)
+	}
+	return mac, nil
+}
+
+// parseAddrShowDev returns the first non-loopback link name from `ip -o addr
+// show` output, e.g. "5: spx-nat-host    inet 100.127.0.1/24 scope global ...".
+func parseAddrShowDev(out string) string {
+	for line := range strings.SplitSeq(strings.TrimSpace(out), "\n") {
+		fields := strings.Fields(line)
+		if len(fields) < 2 || fields[1] == localRouteDev {
+			continue
+		}
+		return fields[1]
+	}
+	return ""
+}
+
+// parseLinkEtherMAC extracts the link/ether address from `ip -o link show`
+// output. Returns "" for a link with no ethernet address (loopback, tun).
+func parseLinkEtherMAC(out string) string {
+	fields := strings.Fields(out)
+	for i, f := range fields {
+		if f == "link/ether" && i+1 < len(fields) {
+			return fields[i+1]
+		}
+	}
+	return ""
+}
+
 // 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) {
@@ -171,6 +171,154 @@ func TestSeedNexthopMAC_UnresolvedEvenAfterPing(t *testing.T) {
 	}
 }

+// Routed NAT points the gateway router at 100.127.0.1, an address on this
+// host's own transit veth. `ip route get` answers lo and no neigh entry can
+// ever exist, so the MAC has to come from the link that owns the address.
+func TestSeedNexthopMAC_LocalNexthopResolvesFromOwningLink(t *testing.T) {
+	r := &scriptedRunner{
+		responses: map[string][]byte{
+			"ip route get 100.127.0.1": []byte("local 100.127.0.1 dev lo src 100.127.0.1 uid 0"),
+			"ip -4 -o addr show to 100.127.0.1/32": []byte(
+				"7: spx-nat-host    inet 100.127.0.1/24 scope global spx-nat-host\\       valid_lft forever preferred_lft forever"),
+			"ip -o link show dev spx-nat-host": []byte(
+				"7: spx-nat-host@spx-nat-ovs: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000\\    link/ether 6a:1b:2c:3d:4e:5f brd ff:ff:ff:ff:ff:ff"),
+		},
+	}
+	if err := SeedNexthopMAC(context.Background(), r, "", "gw-vpc-1", "100.127.0.1"); err != nil {
+		t.Fatalf("SeedNexthopMAC: %v", err)
+	}
+	if calls := r.callArgs("ping"); len(calls) != 0 {
+		t.Fatalf("expected no ping prime for a host-local nexthop, got %v", calls)
+	}
+	if calls := r.callArgs("ip neigh"); len(calls) != 0 {
+		t.Fatalf("expected no neigh read for a host-local nexthop, got %v", calls)
+	}
+	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 100.127.0.1 6a:1b:2c:3d:4e:5f"
+	if got := strings.Join(addCalls[0], " "); got != wantAdd {
+		t.Fatalf("add argv mismatch\n got: %s\nwant: %s", got, wantAdd)
+	}
+}
+
+// Dynamic ARP cannot rescue a host-local nexthop, so an unresolved one is
+// permanent egress loss and must surface as an error, not a best-effort nil.
+func TestSeedNexthopMAC_LocalNexthopNoOwningLink(t *testing.T) {
+	r := &scriptedRunner{
+		responses: map[string][]byte{
+			"ip route get 100.127.0.1":             []byte("local 100.127.0.1 dev lo src 100.127.0.1 uid 0"),
+			"ip -4 -o addr show to 100.127.0.1/32": []byte(""),
+		},
+	}
+	err := SeedNexthopMAC(context.Background(), r, "", "gw-vpc-1", "100.127.0.1")
+	if err == nil {
+		t.Fatal("expected an error when no link carries a host-local nexthop")
+	}
+	if !strings.Contains(err.Error(), "no link carries 100.127.0.1") {
+		t.Fatalf("error must name the unresolved nexthop, got: %v", err)
+	}
+	if !strings.Contains(err.Error(), NATTransitHostEnd) {
+		t.Fatalf("error must point at %s for remediation, got: %v", NATTransitHostEnd, err)
+	}
+	if calls := r.callArgs("ovn-nbctl"); len(calls) != 0 {
+		t.Fatalf("expected no ovn-nbctl calls when no link owns the nexthop, got %v", calls)
+	}
+}
+
+// A nexthop on a link with no ethernet address (tun, dummy) has no MAC to bind
+// to. The addr lookup must name that link so ip link show is actually reached.
+func TestSeedNexthopMAC_LocalNexthopLinkHasNoEtherAddr(t *testing.T) {
+	r := &scriptedRunner{
+		responses: map[string][]byte{
+			"ip route get 100.127.0.1": []byte("local 100.127.0.1 dev lo src 100.127.0.1 uid 0"),
+			"ip -4 -o addr show to 100.127.0.1/32": []byte(
+				"9: spx-nat-tun    inet 100.127.0.1/24 scope global spx-nat-tun\\       valid_lft forever preferred_lft forever"),
+			"ip -o link show dev spx-nat-tun": []byte(
+				"9: spx-nat-tun: <POINTOPOINT,MULTICAST,NOARP,UP> mtu 1500 qdisc fq_codel state UNKNOWN mode DEFAULT group default qlen 500\\    link/none"),
+		},
+	}
+	err := SeedNexthopMAC(context.Background(), r, "", "gw-vpc-1", "100.127.0.1")
+	if err == nil {
+		t.Fatal("expected an error when the owning link has no ethernet address")
+	}
+	if !strings.Contains(err.Error(), "spx-nat-tun") {
+		t.Fatalf("error must name the owning link, got: %v", err)
+	}
+	if calls := r.callArgs("ip -o link show"); len(calls) != 1 {
+		t.Fatalf("expected the link show to be reached exactly once, got %d: %v", len(calls), r.calls)
+	}
+	if calls := r.callArgs("ovn-nbctl"); len(calls) != 0 {
+		t.Fatalf("expected no ovn-nbctl calls for a link with no MAC, got %v", calls)
+	}
+}
+
+// `ip addr show to` exits 0 with empty output when nothing matches, so a real
+// error there is always unexpected and its diagnostic must reach the caller.
+func TestSeedNexthopMAC_LocalNexthopAddrShowErrorPropagates(t *testing.T) {
+	r := &scriptedRunner{
+		responses: map[string][]byte{
+			"ip route get 100.127.0.1": []byte("local 100.127.0.1 dev lo src 100.127.0.1 uid 0"),
+		},
+		errors: map[string]error{
+			"ip -4 -o addr show to 100.127.0.1/32": errors.New("sudo: a password is required"),
+		},
+	}
+	err := SeedNexthopMAC(context.Background(), r, "", "gw-vpc-1", "100.127.0.1")
+	if err == nil {
+		t.Fatal("expected the addr show failure to propagate, not be swallowed")
+	}
+	if !strings.Contains(err.Error(), "sudo: a password is required") {
+		t.Fatalf("underlying diagnostic must survive, got: %v", err)
+	}
+	if calls := r.callArgs("ovn-nbctl"); len(calls) != 0 {
+		t.Fatalf("expected no ovn-nbctl calls after a failed lookup, got %v", calls)
+	}
+}
+
+// A link that vanishes between the addr lookup and the link read is a TOCTOU
+// signal, not a missing MAC — it must not collapse into the same outcome.
+func TestSeedNexthopMAC_LocalNexthopLinkShowErrorPropagates(t *testing.T) {
+	r := &scriptedRunner{
+		responses: map[string][]byte{
+			"ip route get 100.127.0.1": []byte("local 100.127.0.1 dev lo src 100.127.0.1 uid 0"),
+			"ip -4 -o addr show to 100.127.0.1/32": []byte(
+				"7: spx-nat-host    inet 100.127.0.1/24 scope global spx-nat-host\\       valid_lft forever preferred_lft forever"),
+		},
+		errors: map[string]error{
+			"ip -o link show dev spx-nat-host": errors.New(`Device "spx-nat-host" does not exist.`),
+		},
+	}
+	err := SeedNexthopMAC(context.Background(), r, "", "gw-vpc-1", "100.127.0.1")
+	if err == nil {
+		t.Fatal("expected the link show failure to propagate, not be swallowed")
+	}
+	if !strings.Contains(err.Error(), "does not exist") {
+		t.Fatalf("underlying diagnostic must survive, got: %v", err)
+	}
+	if calls := r.callArgs("ovn-nbctl"); len(calls) != 0 {
+		t.Fatalf("expected no ovn-nbctl calls after a failed lookup, got %v", calls)
+	}
+}
+
+func TestParseLinkEtherMAC(t *testing.T) {
+	cases := []struct {
+		name string
+		out  string
+		want string
+	}{
+		{"ether", "7: spx-nat-host@spx-nat-ovs: <UP> mtu 1500\\    link/ether 6a:1b:2c:3d:4e:5f brd ff:ff:ff:ff:ff:ff", "6a:1b:2c:3d:4e:5f"},
+		{"loopback", "1: lo: <LOOPBACK,UP> mtu 65536\\    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00", ""},
+		{"empty", "", ""},
+	}
+	for _, tc := range cases {
+		if got := parseLinkEtherMAC(tc.out); got != tc.want {
+			t.Errorf("%s: parseLinkEtherMAC = %q, want %q", tc.name, got, tc.want)
+		}
+	}
+}
+
 func TestSeedNexthopMAC_EmptyArgsNoop(t *testing.T) {
 	r := &scriptedRunner{}
 	if err := SeedNexthopMAC(context.Background(), r, "", "", "192.168.1.1"); err != nil {