diff --git a/internal/cni/cni_test.go b/internal/cni/cni_test.go index 389adc5..5d316f0 100644 --- a/internal/cni/cni_test.go +++ b/internal/cni/cni_test.go @@ -1233,6 +1233,50 @@ func TestCmdDelIdempotentMissingResources(t *testing.T) { } } +// TestCmdDelFlushesGuestNetnsConfig reproduces the production incident: a +// hostNetwork pod's Multus secondary attachment resolves args.Netns to the +// same namespace the guest link already lives in, so host-device DEL's +// move-back-out-of-the-netns — and the kernel's implicit address/route +// flush that only fires on a *real* namespace change — never happens. +// cmdDel must flush the guest interface's address/default route itself +// (flushGuestNetnsConfig), not rely solely on that side effect, or the next +// ADD against the same interface wedges with "add default route ...: file +// exists". +func TestCmdDelFlushesGuestNetnsConfig(t *testing.T) { + requireRoot(t) + + netnsPath, cleanup := createTestNetnsWithDummy(t) + defer cleanup() + + if err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, testGateway, nil, nil); err != nil { + t.Fatalf("configureInterfaceInNetns: %v", err) + } + + conf := fmt.Sprintf( + `{"cniVersion":"1.0.0","name":"test",`+ + `"type":"galactic-cni","vpc":"%s",`+ + `"vpcattachment":"%s","interface_type":"veth"}`, + testVPC, testAttachment, + ) + args := &skel.CmdArgs{ + ContainerID: testContainerID, + Netns: netnsPath, + IfName: "test-dummy", + StdinData: []byte(conf), + } + + // host-device DEL will fail (no host-device binary next to the test + // binary) — that's expected and non-fatal, matching production when the + // move-based side effect doesn't apply anyway. + if err := cmdDel(args); err != nil { + t.Fatalf("cmdDel returned error = %v, want nil", err) + } + + if gw := defaultRouteVia(t, netnsPath); gw != nil { + t.Fatalf("default route gateway = %v, want nil (flushed by DEL)", gw) + } +} + func mustParseCIDR(t *testing.T, cidr string) *net.IPNet { t.Helper() _, ipnet, err := net.ParseCIDR(cidr) diff --git a/internal/cni/netns.go b/internal/cni/netns.go index f308233..404f207 100644 --- a/internal/cni/netns.go +++ b/internal/cni/netns.go @@ -167,6 +167,75 @@ func isDefaultRouteDst(dst *net.IPNet) bool { return ones == 0 } +// flushGuestNetnsConfig removes any IP addresses and default routes +// configured on ifName inside the container netns, without deleting the +// link itself. No-op if ifName is not present. +// +// DEL normally relies on hostDevice DEL moving the guest veth end back out +// of the container netns to flush this state as a side effect: the kernel +// strips a link's addresses/routes when it genuinely crosses a namespace +// boundary. But that move is a no-op — and so triggers no such flush — when +// the "container" netns is the same namespace the link already lives in +// (e.g. a hostNetwork pod with a Multus secondary attachment, where +// args.Netns resolves to the host's own root netns rather than a distinct +// per-sandbox namespace). Left unflushed, that state survives indefinitely +// (there is no ephemeral sandbox netns to tear down and reclaim it), and the +// next ADD on that same interface fails with "file exists" trying to add a +// default route that never went away. Calling this unconditionally in DEL, +// ahead of hostDevice DEL, makes cleanup reliable regardless of whether the +// move-triggered flush fires. +func flushGuestNetnsConfig(netnsPath, ifName string) error { + containerNS, err := ns.GetNS(netnsPath) + if err != nil { + return fmt.Errorf("get container netns %q: %w", netnsPath, err) + } + defer containerNS.Close() //nolint:errcheck // netns close on teardown + + return containerNS.Do(func(_ ns.NetNS) error { + handle, err := netlink.NewHandle() + if err != nil { + return fmt.Errorf("create netlink handle: %w", err) + } + defer handle.Close() //nolint:errcheck // netlink cleanup on teardown + + link, err := handle.LinkByName(ifName) + if err != nil { + return nil // interface not present — nothing to flush + } + + for _, family := range []int{netlink.FAMILY_V6, netlink.FAMILY_V4} { + routes, err := handle.RouteList(link, family) + if err != nil { + return fmt.Errorf("list routes on %q: %w", ifName, err) + } + for _, route := range routes { + if !isDefaultRouteDst(route.Dst) { + continue + } + if err := handle.RouteDel(&route); err != nil { + return fmt.Errorf("delete default route on %q: %w", ifName, err) + } + } + + addrs, err := handle.AddrList(link, family) + if err != nil { + return fmt.Errorf("list addresses on %q: %w", ifName, err) + } + for _, addr := range addrs { + if addr.IP.IsLinkLocalUnicast() { + continue + } + if err := handle.AddrDel(link, &addr); err != nil { + return fmt.Errorf("delete address %s on %q: %w", addr.IPNet, ifName, err) + } + } + } + + slog.Debug("netns: flushed guest interface addresses/routes", "ifName", ifName, "netns", netnsPath) + return nil + }) +} + // 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 4019af8..0d8e38b 100644 --- a/internal/cni/netns_test.go +++ b/internal/cni/netns_test.go @@ -362,3 +362,106 @@ func TestConfigureInterfaceInNetnsConflictingGatewayErrors(t *testing.T) { t.Fatalf("default route gateway = %v, want unchanged %v", gw, otherGateway) } } + +// ---- flushGuestNetnsConfig --------------------------------------------- + +// TestFlushGuestNetnsConfigRemovesAddressAndRoute verifies that a configured +// address and default route are both removed, while the link itself +// survives (unlike cleanupContainerNetns, which deletes the whole +// interface). +func TestFlushGuestNetnsConfigRemovesAddressAndRoute(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) + } + + if err := flushGuestNetnsConfig(netnsPath, "test-dummy"); err != nil { + t.Fatalf("flushGuestNetnsConfig: %v", err) + } + + if gw := defaultRouteVia(t, netnsPath); gw != nil { + t.Fatalf("default route gateway = %v, want nil (flushed)", gw) + } + + nsObj, err := ns.GetNS(netnsPath) + if err != nil { + t.Fatalf("open netns %q: %v", netnsPath, err) + } + defer nsObj.Close() //nolint:errcheck // best-effort cleanup + + 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("link should still exist after flush: %w", err) + } + addrs, err := handle.AddrList(link, netlink.FAMILY_V6) + if err != nil { + return err + } + for _, a := range addrs { + if a.Equal(netlink.Addr{IPNet: testIPNet}) { + return fmt.Errorf("address %s still present after flush", testIPNet) + } + } + return nil + }) + if err != nil { + t.Fatal(err) + } +} + +// TestFlushGuestNetnsConfigNonExistentInterface verifies the flush is a +// no-op (not an error) when the named interface isn't present — the case +// where host-device DEL never got as far as moving anything in. +func TestFlushGuestNetnsConfigNonExistentInterface(t *testing.T) { + netnsPath, cleanup := createTestNetnsWithDummy(t) + defer cleanup() + + if err := flushGuestNetnsConfig(netnsPath, "does-not-exist"); err != nil { + t.Fatalf("expected nil for non-existent interface, got: %v", err) + } +} + +// TestFlushGuestNetnsConfigThenRetryAddSucceeds reproduces the production +// incident: a hostNetwork pod's Multus secondary attachment resolves +// args.Netns to the same namespace the guest link already lives in, so +// host-device DEL's cross-namespace move — and the kernel's implicit +// address/route flush that comes with a *real* namespace change — never +// happens. Without an explicit flush, the leftover default route wedges the +// next ADD with "file exists" even though the link's own idempotency check +// (TestConfigureInterfaceInNetnsAddTwiceIsIdempotent) only covers a +// same-link retry, not one where cleanup ran in between but the flush +// silently no-opped. +func TestFlushGuestNetnsConfigThenRetryAddSucceeds(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 for the same-namespace (hostNetwork) case: the + // move-based flush hostDevice DEL relies on doesn't fire, so DEL's only + // effective cleanup is the explicit flush. + if err := flushGuestNetnsConfig(netnsPath, "test-dummy"); err != nil { + t.Fatalf("flushGuestNetnsConfig: %v", err) + } + + // The retried ADD must now succeed against the same link. + if err := configureInterfaceInNetns(netnsPath, "test-dummy", testIPNet, testGateway, nil, nil); err != nil { + t.Fatalf("configureInterfaceInNetns after flush: %v", err) + } + + gw := defaultRouteVia(t, netnsPath) + if gw == nil || !gw.Equal(testGateway) { + t.Fatalf("default route gateway = %v, want %v", gw, testGateway) + } +} diff --git a/internal/cni/ops_del.go b/internal/cni/ops_del.go index 5346da1..8539f9c 100644 --- a/internal/cni/ops_del.go +++ b/internal/cni/ops_del.go @@ -40,24 +40,35 @@ func cmdDel(args *skel.CmdArgs) error { } } + // Explicitly flush the address/default-route galactic-cni's IPAM step + // installed on the guest interface, ahead of host-device delegation. + // hostDevice DEL's move of the guest veth end back out of the container + // netns normally flushes this as a side effect of crossing a namespace + // boundary, but that side effect never fires when args.Netns is the same + // namespace the link already lives in (e.g. a hostNetwork pod with a + // Multus secondary attachment) — the move is then a no-op, and the + // leftover route survives indefinitely since there's no ephemeral + // sandbox netns to reclaim it, wedging the next ADD with "file exists". + // Only applies to veth mode; tap mode has no guest-side netns config. + if pluginConf.InterfaceType == interfaceTypeVeth { + if err := flushGuestNetnsConfig(args.Netns, args.IfName); err != nil { + slog.Warn("DEL: failed to flush guest interface address/route, may still be in the netns", + "err", err, "containerID", args.ContainerID, "netns", args.Netns) + } + } + // 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. + // the guest veth end back out of the container netns and restores its + // original (host-side) name. 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". + // rather than propagated. if pluginConf.InterfaceType == interfaceTypeVeth { 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", + slog.Warn("DEL: host-device DEL failed, guest interface may still be in the netns", "err", err, "containerID", args.ContainerID, "netns", args.Netns) } }