fix: Make IPAM default route idempotent on ADD retries

internal/cni/netns.go

@@ -62,21 +62,82 @@ func configureInterfaceInNetns(
 
 // addAddrAndDefaultRoute assigns ipNet to link and, if gateway is set,
 // installs a default route via it. No-op when ipNet is nil (family not in
-// use for this attachment).
+// use for this attachment). Both the address and the route are added
+// idempotently: a call that finds its target already in place (e.g. a
+// retried CNI ADD against a netns that a previous, non-DEL'd ADD already
+// configured) succeeds without touching kernel state; a call that finds
+// different, conflicting state fails loudly instead of overwriting it.
 func addAddrAndDefaultRoute(
 	handle *netlink.Handle, link netlink.Link, ifName string, ipNet *net.IPNet, gateway net.IP,
 ) error {
 	if ipNet == nil {
 		return nil
 	}
 
-	if err := handle.AddrAdd(link, &netlink.Addr{IPNet: ipNet}); err != nil {
-		return fmt.Errorf("add IP %s to %q: %w", ipNet, ifName, err)
+	if err := addAddrIfMissing(handle, link, ifName, ipNet); err != nil {
+		return err
 	}
 
 	if gateway == nil {
 		return nil
 	}
+	return addDefaultRouteIfMissing(handle, link, ifName, gateway)
+}
+
+// addrFamily returns the netlink address family for an IP, so callers can
+// scope AddrList/RouteList lookups to the family being configured.
+func addrFamily(ip net.IP) int {
+	if ip.To4() != nil {
+		return netlink.FAMILY_V4
+	}
+	return netlink.FAMILY_V6
+}
+
+// addAddrIfMissing adds ipNet to link unless an identical address (same IP
+// and prefix length) is already present, in which case it is a no-op. A
+// link can legitimately carry multiple distinct addresses per family, so no
+// existing address is ever treated as a conflict here — only an exact match
+// short-circuits the add.
+func addAddrIfMissing(handle *netlink.Handle, link netlink.Link, ifName string, ipNet *net.IPNet) error {
+	want := netlink.Addr{IPNet: ipNet}
+
+	existing, err := handle.AddrList(link, addrFamily(ipNet.IP))
+	if err != nil {
+		return fmt.Errorf("list addresses on %q: %w", ifName, err)
+	}
+	for _, addr := range existing {
+		if addr.Equal(want) {
+			return nil // already configured by a previous ADD
+		}
+	}
+
+	if err := handle.AddrAdd(link, &want); err != nil {
+		return fmt.Errorf("add IP %s to %q: %w", ipNet, ifName, err)
+	}
+	return nil
+}
+
+// addDefaultRouteIfMissing installs a default route via gateway on link
+// unless a default route via that same gateway already exists, in which
+// case it is a no-op. If a default route via a *different* gateway already
+// exists, that's a real misconfiguration (not something a retried ADD
+// should paper over), so it is returned as an error instead.
+func addDefaultRouteIfMissing(handle *netlink.Handle, link netlink.Link, ifName string, gateway net.IP) error {
+	existing, err := handle.RouteList(link, addrFamily(gateway))
+	if err != nil {
+		return fmt.Errorf("list routes on %q: %w", ifName, err)
+	}
+	for _, r := range existing {
+		if !isDefaultRouteDst(r.Dst) {
+			continue
+		}
+		if r.Gw.Equal(gateway) {
+			return nil // already configured by a previous ADD
+		}
+		return fmt.Errorf("default route on %q already points via %s, refusing to add conflicting route via %s",
+			ifName, r.Gw, gateway)
+	}
+
 	// onlink: the IPv4 pool allocates a /32 host address (no on-link subnet
 	// route to the gateway), so the kernel refuses this route with
 	// ENETUNREACH unless told to treat the gateway as directly reachable.
@@ -94,6 +155,18 @@ func addAddrAndDefaultRoute(
 	return nil
 }
 
+// isDefaultRouteDst reports whether dst represents a default route
+// (0.0.0.0/0 or ::/0). netlink represents this as a nil Dst on routes it
+// creates itself, but routes read back from the kernel may instead carry an
+// explicit zero-length-prefix net.IPNet.
+func isDefaultRouteDst(dst *net.IPNet) bool {
+	if dst == nil {
+		return true
+	}
+	ones, _ := dst.Mask.Size()
+	return ones == 0
+}
+
 // readGuestInterface reads the MAC and MTU of the guest veth endpoint
 // inside the container network namespace.
 func readGuestInterface(netnsPath, ifName string) (string, int, error) {

internal/cni/netns_test.go

@@ -6,6 +6,7 @@ package cni
 
 import (
 	"fmt"
+	"net"
 	"os"
 	"strings"
 	"testing"
@@ -193,3 +194,171 @@ func TestCleanupContainerNetnsVeth(t *testing.T) {
 		_ = hostHandle.LinkDel(peer) //nolint:errcheck // best-effort cleanup
 	}
 }
+
+// ---- addAddrAndDefaultRoute idempotency -------------------------------------
+
+// testGateway/testIPNet mirror the environment described in the bug report:
+// an IPv6-only attachment with a /48-style pool and a single default route
+// per attachment/gateway.
+var (
+	testGateway = net.ParseIP("fd00:30:ff01::1")
+	testIPNet   = &net.IPNet{IP: net.ParseIP("fd00:30:ff01::2"), Mask: net.CIDRMask(96, 128)}
+)
+
+// defaultRouteVia returns the gateway of the default route on "test-dummy"
+// inside netnsPath, or nil if no default route is present. Fails the test on
+// any other error.
+func defaultRouteVia(t *testing.T, netnsPath string) net.IP {
+	t.Helper()
+
+	nsObj, err := ns.GetNS(netnsPath)
+	if err != nil {
+		t.Fatalf("open netns %q: %v", netnsPath, err)
+	}
+	defer nsObj.Close() //nolint:errcheck // best-effort cleanup
+
+	var gw net.IP
+	err = nsObj.Do(func(_ ns.NetNS) error {
+		handle, err := netlink.NewHandle()
+		if err != nil {
+			return err
+		}
+		defer handle.Close() //nolint:errcheck // netlink cleanup on teardown
+
+		link, err := handle.LinkByName("test-dummy")
+		if err != nil {
+			return fmt.Errorf("find interface %q: %w", "test-dummy", err)
+		}
+		routes, err := handle.RouteList(link, netlink.FAMILY_V6)
+		if err != nil {
+			return err
+		}
+		for _, r := range routes {
+			if isDefaultRouteDst(r.Dst) {
+				gw = r.Gw
+				return nil
+			}
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("read default route on test-dummy: %v", err)
+	}
+	return gw
+}
+
+// TestConfigureInterfaceInNetnsCleanAdd verifies a clean ADD installs the
+// expected address and default route on a fresh netns/interface.
+func TestConfigureInterfaceInNetnsCleanAdd(t *testing.T) {
+	netnsPath, cleanup := createTestNetnsWithDummy(t)
+	defer cleanup()
+
+	if err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, testGateway, nil, nil); err != nil {
+		t.Fatalf("configureInterfaceInNetns: %v", err)
+	}
+
+	gw := defaultRouteVia(t, netnsPath)
+	if gw == nil || !gw.Equal(testGateway) {
+		t.Fatalf("default route gateway = %v, want %v", gw, testGateway)
+	}
+}
+
+// TestConfigureInterfaceInNetnsAddTwiceIsIdempotent reproduces the reported
+// bug: a caller that retries CNI ADD against the same netns without an
+// intervening DEL (e.g. because an earlier ADD attempt was aborted before
+// its own cleanup ran) must not see "file exists" on the second attempt.
+func TestConfigureInterfaceInNetnsAddTwiceIsIdempotent(t *testing.T) {
+	netnsPath, cleanup := createTestNetnsWithDummy(t)
+	defer cleanup()
+
+	if err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, testGateway, nil, nil); err != nil {
+		t.Fatalf("first configureInterfaceInNetns: %v", err)
+	}
+	if err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, testGateway, nil, nil); err != nil {
+		t.Fatalf("second configureInterfaceInNetns (retry, no DEL in between): %v", err)
+	}
+
+	gw := defaultRouteVia(t, netnsPath)
+	if gw == nil || !gw.Equal(testGateway) {
+		t.Fatalf("default route gateway = %v, want %v", gw, testGateway)
+	}
+}
+
+// TestConfigureInterfaceInNetnsAddAfterDel verifies that once the route is
+// actually removed (what DEL achieves in production by moving the guest
+// veth end out of the netns), a subsequent ADD on the same netns succeeds
+// again rather than being permanently wedged.
+func TestConfigureInterfaceInNetnsAddAfterDel(t *testing.T) {
+	netnsPath, cleanup := createTestNetnsWithDummy(t)
+	defer cleanup()
+
+	if err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, testGateway, nil, nil); err != nil {
+		t.Fatalf("first configureInterfaceInNetns: %v", err)
+	}
+
+	// Simulate DEL: remove the address and route from the interface, the
+	// same net effect host-device DEL's netns move has in production.
+	nsObj, err := ns.GetNS(netnsPath)
+	if err != nil {
+		t.Fatalf("open netns %q: %v", netnsPath, err)
+	}
+	err = nsObj.Do(func(_ ns.NetNS) error {
+		handle, err := netlink.NewHandle()
+		if err != nil {
+			return err
+		}
+		defer handle.Close() //nolint:errcheck // netlink cleanup on teardown
+
+		link, err := handle.LinkByName("test-dummy")
+		if err != nil {
+			return err
+		}
+		if err := handle.RouteDel(&netlink.Route{
+			Gw: testGateway, LinkIndex: link.Attrs().Index, Flags: int(netlink.FLAG_ONLINK),
+		}); err != nil {
+			return fmt.Errorf("delete route: %w", err)
+		}
+		return handle.AddrDel(link, &netlink.Addr{IPNet: testIPNet})
+	})
+	nsObj.Close() //nolint:errcheck // best-effort cleanup
+	if err != nil {
+		t.Fatalf("simulate DEL: %v", err)
+	}
+
+	if err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, testGateway, nil, nil); err != nil {
+		t.Fatalf("configureInterfaceInNetns after DEL: %v", err)
+	}
+
+	gw := defaultRouteVia(t, netnsPath)
+	if gw == nil || !gw.Equal(testGateway) {
+		t.Fatalf("default route gateway = %v, want %v", gw, testGateway)
+	}
+}
+
+// TestConfigureInterfaceInNetnsConflictingGatewayErrors verifies that a
+// pre-existing default route via a *different* gateway is a real
+// misconfiguration and must still fail loudly rather than being papered
+// over by the idempotency check.
+func TestConfigureInterfaceInNetnsConflictingGatewayErrors(t *testing.T) {
+	netnsPath, cleanup := createTestNetnsWithDummy(t)
+	defer cleanup()
+
+	otherGateway := net.ParseIP("fd00:30:ff01::99")
+	if err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, otherGateway, nil, nil); err != nil {
+		t.Fatalf("install conflicting route: %v", err)
+	}
+
+	err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, testGateway, nil, nil)
+	if err == nil {
+		t.Fatal("expected error for conflicting default route gateway, got nil")
+	}
+	if !strings.Contains(err.Error(), "refusing to add conflicting route") {
+		t.Fatalf("error %q does not mention refusing conflicting route", err.Error())
+	}
+
+	// The pre-existing (different) route must be left untouched.
+	gw := defaultRouteVia(t, netnsPath)
+	if gw == nil || !gw.Equal(otherGateway) {
+		t.Fatalf("default route gateway = %v, want unchanged %v", gw, otherGateway)
+	}
+}

internal/cni/ops_del.go

@@ -40,11 +40,26 @@ func cmdDel(args *skel.CmdArgs) error {
 		}
 	}
 
-	// Forward DEL to host-device delegated plugin (CNI spec §4).
-	// host-device DEL is idempotent — missing devices are not errors.
-	// Only applies to veth mode; tap mode has no host-device delegation.
+	// Forward DEL to host-device delegated plugin (CNI spec §4). This moves
+	// the guest veth end back out of the container netns, which as a side
+	// effect flushes the addresses/routes galactic-cni's IPAM step installed
+	// on it — the primary mechanism for cleaning those up. Only applies to
+	// veth mode; tap mode has no host-device delegation.
+	//
+	// DEL must always return success per the CNI spec, so an error here
+	// (e.g. the device was never moved into the netns because ADD failed
+	// before reaching that step, or the netns is already gone) is logged
+	// rather than propagated. A logged failure here is the signal to look
+	// for: it means the route/address were NOT flushed via this path and may
+	// still be sitting in the container netns for whatever picks up
+	// args.Netns next — see addAddrIfMissing/addDefaultRouteIfMissing in
+	// netns.go, which is what makes a subsequent ADD retry against that
+	// leftover state safe instead of failing with "file exists".
 	if pluginConf.InterfaceType == interfaceTypeVeth {
-		_ = hostDevice("DEL", args, pluginConf)
+		if err := hostDevice("DEL", args, pluginConf); err != nil {
+			slog.Warn("DEL: host-device DEL failed, guest interface (and any route/address) may still be in the netns",
+				"err", err, "containerID", args.ContainerID, "netns", args.Netns)
+		}
 	}
 
 	// Shared resources (VRF, veth/tap, routes, SRv6 ingress, BGPAdvertisement,