mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-15 17:11:30 +00:00
fix(network): inject GARP after AddEIP to refresh upstream ARP on EIP recycle
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 <port>` (OVN 22.09+). Port is the EIP's LSP in distributed NAT or "cr-gw-<vpcID>" 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/.
This commit is contained in:
@@ -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 <port>` (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-<vpcID>"). 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
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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{
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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-<vpc> 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()
|
||||
|
||||
+26
-1
@@ -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-<vpcID>" — 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
|
||||
|
||||
Reference in New Issue
Block a user