From 72a6a59d8068da7dfa5adbbc160b06ae4e0c4c4e Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Tue, 2 Jun 2026 13:38:52 +1000 Subject: [PATCH] fix(network): inject GARP after AddEIP to refresh upstream ARP on EIP recycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ovn-controller emits gratuitous ARPs automatically when a NAT-bound LSP migrates chassis, but not when an external IP is reassigned to a different LR whose chassisredirect MAC differs from the prior occupant's. In CI single-host runs the WAN pool slice is small enough that the same EIP recycles across VPCs within the 60-300s kernel ARP timeout, so upstream caches keep pointing at the dead chassis-redirect MAC and packets to the new VM never arrive. Fix: add a post-AddEIP hook that runs `ovn-appctl -t ovn-controller inject-garp ` (OVN 22.09+). Port is the EIP's LSP in distributed NAT or "cr-gw-" in centralized NAT. Best-effort — failure logs a warning and the NAT install still succeeds. - network/host/garp.go: new L0 shell-out helper InjectGARP. - network/policy/nat.go: GARPEmitter option type, called after the flows-ready barrier in AddEIP. - vpcd: wire the production emitter that derives the right OVN port from the EIPSpec. - invariants: add ovn-appctl to the host-tool ban list so future callers are forced into network/host/. --- spinifex/network/host/garp.go | 43 +++++++++++++ spinifex/network/host/garp_test.go | 64 +++++++++++++++++++ .../invariants/host_containment_test.go | 9 +-- spinifex/network/policy/nat.go | 27 ++++++++ spinifex/network/policy/nat_test.go | 52 +++++++++++++++ spinifex/vpcd/vpcd.go | 27 +++++++- 6 files changed, 217 insertions(+), 5 deletions(-) create mode 100644 spinifex/network/host/garp.go create mode 100644 spinifex/network/host/garp_test.go diff --git a/spinifex/network/host/garp.go b/spinifex/network/host/garp.go new file mode 100644 index 000000000..c97dcca93 --- /dev/null +++ b/spinifex/network/host/garp.go @@ -0,0 +1,43 @@ +package host + +import ( + "context" + "fmt" + "strings" +) + +// InjectGARP triggers ovn-controller on the local chassis to emit a +// gratuitous ARP for every IP associated with logicalPort. Wraps +// `ovn-appctl -t ovn-controller inject-garp ` (OVN 22.09+). +// +// Required after AddEIP when the external IP is being recycled: OVN emits +// its automatic GARP only on LSP binding-chassis migration, so a fresh +// external_ip-to-LRP rebind on the same chassis leaves upstream ARP caches +// pointing at the prior chassis-redirect MAC until the kernel ARP timeout +// (60-300s) expires. +// +// Caller chooses the correct port: +// - distributed dnat_and_snat (logical_port + external_mac set): the LSP +// name (e.g. "port-eni-abc"). GARP fires from the LSP-binding chassis +// with external_mac. +// - centralized dnat_and_snat (no logical_port): the chassisredirect +// port for the VPC's gateway LRP ("cr-gw-"). GARP fires from +// the gw chassis with the LRP MAC. +// +// L0 method (ADR-0006 S2) — only network/host/ may shell out to host tools. +// Best-effort: callers must treat errors as warnings, not failures, because +// inject-garp may fail when ovn-controller is restarting or the port binding +// has not yet propagated to SBDB. +func InjectGARP(ctx context.Context, runner Runner, logicalPort string) error { + if logicalPort == "" { + return fmt.Errorf("host.InjectGARP: logicalPort required") + } + if runner == nil { + runner = NewExecRunner() + } + out, err := runner.Run(ctx, "ovn-appctl", "-t", "ovn-controller", "inject-garp", logicalPort) + if err != nil { + return fmt.Errorf("ovn-appctl inject-garp %s: %s: %w", logicalPort, strings.TrimSpace(string(out)), err) + } + return nil +} diff --git a/spinifex/network/host/garp_test.go b/spinifex/network/host/garp_test.go new file mode 100644 index 000000000..fd495c4ba --- /dev/null +++ b/spinifex/network/host/garp_test.go @@ -0,0 +1,64 @@ +package host + +import ( + "context" + "errors" + "strings" + "testing" +) + +type recordingRunner struct { + args [][]string + out []byte + err error +} + +func (r *recordingRunner) Run(_ context.Context, name string, args ...string) ([]byte, error) { + full := append([]string{name}, args...) + r.args = append(r.args, full) + return r.out, r.err +} + +func TestInjectGARP_ShellOutShape(t *testing.T) { + r := &recordingRunner{} + if err := InjectGARP(context.Background(), r, "port-eni-abc"); err != nil { + t.Fatalf("InjectGARP: %v", err) + } + if len(r.args) != 1 { + t.Fatalf("expected 1 call, got %d", len(r.args)) + } + got := strings.Join(r.args[0], " ") + want := "ovn-appctl -t ovn-controller inject-garp port-eni-abc" + if got != want { + t.Fatalf("argv mismatch\n got: %s\nwant: %s", got, want) + } +} + +func TestInjectGARP_CentralizedPort(t *testing.T) { + r := &recordingRunner{} + if err := InjectGARP(context.Background(), r, "cr-gw-vpc-abc"); err != nil { + t.Fatalf("InjectGARP: %v", err) + } + got := strings.Join(r.args[0], " ") + want := "ovn-appctl -t ovn-controller inject-garp cr-gw-vpc-abc" + if got != want { + t.Fatalf("argv mismatch\n got: %s\nwant: %s", got, want) + } +} + +func TestInjectGARP_MissingPort(t *testing.T) { + if err := InjectGARP(context.Background(), &recordingRunner{}, ""); err == nil { + t.Fatal("expected error for empty logicalPort") + } +} + +func TestInjectGARP_PropagatesRunnerError(t *testing.T) { + r := &recordingRunner{out: []byte("ovn-controller: no such port"), err: errors.New("exit 2")} + err := InjectGARP(context.Background(), r, "port-missing") + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "no such port") { + t.Fatalf("expected combined output in error, got %v", err) + } +} diff --git a/spinifex/network/invariants/host_containment_test.go b/spinifex/network/invariants/host_containment_test.go index ef2374997..e00a99fd2 100644 --- a/spinifex/network/invariants/host_containment_test.go +++ b/spinifex/network/invariants/host_containment_test.go @@ -23,10 +23,11 @@ func TestS2_BridgeModeContainedInL0(t *testing.T) { `above receive a typed enum at init time."` hostTools := map[string]struct{}{ - "ovs-vsctl": {}, - "ovs-ofctl": {}, - "ovn-nbctl": {}, - "ovn-sbctl": {}, + "ovs-vsctl": {}, + "ovs-ofctl": {}, + "ovn-nbctl": {}, + "ovn-sbctl": {}, + "ovn-appctl": {}, } roots := []string{ diff --git a/spinifex/network/policy/nat.go b/spinifex/network/policy/nat.go index a9d281ec7..dd6d82993 100644 --- a/spinifex/network/policy/nat.go +++ b/spinifex/network/policy/nat.go @@ -58,6 +58,17 @@ type NATManager interface { // `ovn-nbctl --wait=hv sync`; tests leave it nil. type FlowsBarrier func() error +// GARPEmitter forces ovn-controller to emit a gratuitous ARP for the EIP so +// upstream switches and host ARP caches relearn the MAC owning that IP. +// Required after AddEIP when the external IP is being recycled onto a +// different LR — ovn-controller skips its automatic GARP when the LSP +// binding stays on the same chassis, leaving upstream ARP entries pointed +// at the prior chassis-redirect MAC until the kernel ARP timeout expires +// (60-300s). +// +// Best-effort: implementations return errors but callers warn and proceed. +type GARPEmitter func(EIPSpec) error + type Option func(*natManager) // WithFlowsBarrier injects the post-write flow-install barrier so callers @@ -70,10 +81,22 @@ func WithFlowsBarrier(b FlowsBarrier) Option { } } +// WithGARPEmitter injects the post-AddEIP gratuitous-ARP hook. Without it, +// AddEIP relies on OVN's automatic GARP-on-binding-migration, which does not +// fire when the new LSP binds to the same chassis as the prior occupant. +func WithGARPEmitter(g GARPEmitter) Option { + return func(m *natManager) { + if g != nil { + m.garp = g + } + } +} + type natManager struct { ovn ovn.Client mode NATMode barrier FlowsBarrier + garp GARPEmitter } var _ NATManager = (*natManager)(nil) @@ -88,6 +111,7 @@ func NewNATManager(client ovn.Client, mode NATMode, opts ...Option) (NATManager, ovn: client, mode: mode, barrier: func() error { return nil }, + garp: func(EIPSpec) error { return nil }, } for _, opt := range opts { opt(m) @@ -143,6 +167,9 @@ func (m *natManager) AddEIP(ctx context.Context, eip EIPSpec) error { if err := m.barrier(); err != nil { slog.Warn("policy: AddEIP flows barrier failed", "external_ip", eip.ExternalIP, "logical_ip", eip.LogicalIP, "err", err) } + if err := m.garp(eip); err != nil { + slog.Warn("policy: AddEIP gratuitous-ARP emission failed", "external_ip", eip.ExternalIP, "logical_ip", eip.LogicalIP, "err", err) + } return nil } diff --git a/spinifex/network/policy/nat_test.go b/spinifex/network/policy/nat_test.go index 18b03b481..9b065d832 100644 --- a/spinifex/network/policy/nat_test.go +++ b/spinifex/network/policy/nat_test.go @@ -88,6 +88,58 @@ func TestNATManager_AddEIP_IdempotentSkip(t *testing.T) { assert.Equal(t, 1, barrierCalls, "FlowsBarrier must not fire on idempotent skip") } +func TestNATManager_AddEIP_GARP_FiresDistributed(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + var got []EIPSpec + nm, err := NewNATManager(m, NATModeDistributed, WithGARPEmitter(func(spec EIPSpec) error { + got = append(got, spec) + return nil + })) + require.NoError(t, err) + + spec := EIPSpec{ + VPCID: "vpc-1", ExternalIP: "1.2.3.4", LogicalIP: "10.0.0.5", + PortName: "port-eni-abc", MAC: "aa:bb:cc:dd:ee:ff", + } + require.NoError(t, nm.AddEIP(ctx, spec)) + require.Len(t, got, 1, "GARPEmitter must fire once per AddEIP") + assert.Equal(t, spec, got[0], "emitter must receive the full EIPSpec so it can derive the chassisredirect port") +} + +func TestNATManager_AddEIP_GARP_FiresCentralized(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + var got []EIPSpec + nm, err := NewNATManager(m, NATModeCentralized, WithGARPEmitter(func(spec EIPSpec) error { + got = append(got, spec) + return nil + })) + require.NoError(t, err) + + spec := EIPSpec{VPCID: "vpc-1", ExternalIP: "1.2.3.4", LogicalIP: "10.0.0.5"} + require.NoError(t, nm.AddEIP(ctx, spec)) + require.Len(t, got, 1, "GARPEmitter must fire in centralized mode too") + assert.Empty(t, got[0].PortName, "centralized AddEIP carries no LSP — emitter must derive cr-gw- itself") +} + +func TestNATManager_AddEIP_GARP_FailureNonFatal(t *testing.T) { + ctx := context.Background() + m := mock.New() + seedRouter(t, m, "vpc-1") + nm, err := NewNATManager(m, NATModeDistributed, WithGARPEmitter(func(EIPSpec) error { + return assert.AnError + })) + require.NoError(t, err) + + require.NoError(t, nm.AddEIP(ctx, EIPSpec{ + VPCID: "vpc-1", ExternalIP: "1.2.3.4", LogicalIP: "10.0.0.5", + }), "GARP emitter failure must not propagate") + assert.NotNil(t, findNAT(m, "dnat_and_snat", "10.0.0.5"), "NAT row must persist despite GARP failure") +} + func TestNATManager_AddEIP_NoBarrier_Default(t *testing.T) { ctx := context.Background() m := mock.New() diff --git a/spinifex/vpcd/vpcd.go b/spinifex/vpcd/vpcd.go index c8b8d9f18..7e3acf84c 100644 --- a/spinifex/vpcd/vpcd.go +++ b/spinifex/vpcd/vpcd.go @@ -503,7 +503,10 @@ func launchService(cfg *Config) error { topoMgr := topology.NewLiveManager(liveClient, topoOpts...) sgMgr := policy.NewSecurityGroupManager(liveClient) - natMgr, err := policy.NewNATManager(liveClient, natMode, policy.WithFlowsBarrier(waitForFlowsHV)) + natMgr, err := policy.NewNATManager(liveClient, natMode, + policy.WithFlowsBarrier(waitForFlowsHV), + policy.WithGARPEmitter(injectGARPEmitter()), + ) if err != nil { return fmt.Errorf("construct NAT manager: %w", err) } @@ -752,6 +755,28 @@ func resolveBridgeConfig(cfgBridgeMode, externalIface string) (string, string) { return bridgeMode, "br-wan" } +// injectGARPEmitter builds the post-AddEIP GARP hook. Picks the right OVN +// port for inject-garp based on whether the EIP was installed with a +// distributed-NAT logical_port: +// +// - distributed (eip.PortName set): inject-garp on the LSP itself. OVN +// fires GARP from the LSP-binding chassis with the external_mac, which +// is the MAC upstream should learn for the EIP. +// - centralized (no PortName): inject-garp on "cr-gw-" — the +// chassisredirect port for the VPC's gateway LRP. OVN fires GARP from +// the gw chassis with the LRP MAC. +func injectGARPEmitter() policy.GARPEmitter { + return func(spec policy.EIPSpec) error { + port := spec.PortName + if port == "" { + port = "cr-" + topology.GatewayRouterPort(spec.VPCID) + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return host.InjectGARP(ctx, nil, port) + } +} + // ifaceExists returns true when the kernel reports the named link. var ifaceExists = func(name string) bool { return exec.Command("ip", "link", "show", name).Run() == nil