diff --git a/spinifex/network/external/igw.go b/spinifex/network/external/igw.go index 978bb4056..b1f43ba34 100644 --- a/spinifex/network/external/igw.go +++ b/spinifex/network/external/igw.go @@ -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) + } } } diff --git a/spinifex/network/host/mac_binding.go b/spinifex/network/host/mac_binding.go index cd07741e7..5cbbbde0c 100644 --- a/spinifex/network/host/mac_binding.go +++ b/spinifex/network/host/mac_binding.go @@ -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) { diff --git a/spinifex/network/host/mac_binding_test.go b/spinifex/network/host/mac_binding_test.go index 2509aacf8..43430ce01 100644 --- a/spinifex/network/host/mac_binding_test.go +++ b/spinifex/network/host/mac_binding_test.go @@ -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: 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: 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: 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: 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 {