From 00382cf738599b88889bf2427d2eb527a9cd1ab6 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Thu, 30 Jul 2026 22:55:07 -0400 Subject: [PATCH] fix: Make IPAM default route idempotent on ADD retries A CNI ADD retried against a netns that a previous, non-DEL'd ADD had already configured failed with "add default route ... file exists" instead of succeeding, leaving pods stuck in ContainerCreating. addAddrAndDefaultRoute now checks existing kernel state before adding: identical state is a no-op, while a default route via a different gateway is a loud conflict error instead of being silently overwritten or masked. DEL's host-device delegation error (the mechanism that flushes the route/address by moving the interface out of the netns) is now logged instead of silently discarded, so a failed cleanup is diagnosable. --- internal/cni/netns.go | 79 ++++++++++++++++- internal/cni/netns_test.go | 169 +++++++++++++++++++++++++++++++++++++ internal/cni/ops_del.go | 23 ++++- 3 files changed, 264 insertions(+), 7 deletions(-) diff --git a/internal/cni/netns.go b/internal/cni/netns.go index a2e5e5f..f308233 100644 --- a/internal/cni/netns.go +++ b/internal/cni/netns.go @@ -62,7 +62,11 @@ 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 { @@ -70,13 +74,70 @@ func addAddrAndDefaultRoute( 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) { diff --git a/internal/cni/netns_test.go b/internal/cni/netns_test.go index 8c28824..4019af8 100644 --- a/internal/cni/netns_test.go +++ b/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) + } +} diff --git a/internal/cni/ops_del.go b/internal/cni/ops_del.go index 892c06b..5346da1 100644 --- a/internal/cni/ops_del.go +++ b/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,