fix: protect cleanupContainerNetns from deleting non-veth interfaces

The cleanupContainerNetns function in the galactic-cni plugin deleted any
interface matching the expected name without verifying it was the veth
endpoint created by this plugin. If another tool or race condition created
an interface with the same name, it could be accidentally removed, breaking
network connectivity for an unrelated workload.

- Add a type assertion to verify the link is a *netlink.Veth before deletion
- Return a descriptive error when a non-veth interface is found at the expected name
- Document the veth-only deletion behavior in the function comment
- Add integration tests covering non-veth rejection, idempotent missing-interface handling, and successful veth cleanup

fixes #159

Author: privateip

internal/cni/netns.go

@@ -87,9 +87,12 @@ func readGuestInterface(netnsPath, ifName string) (string, int, error) {
 	return mac, mtu, err
 }
 
-// cleanupContainerNetns removes any existing interface with the given name
+// cleanupContainerNetns removes any existing veth interface with the given name
 // from the container network namespace. This is needed to handle stale state
 // from previous CNI ADD runs that may have left interfaces behind.
+//
+// Only *netlink.Veth interfaces are deleted; other types produce a clear error
+// to prevent accidental deletion of unrelated interfaces.
 func cleanupContainerNetns(netnsPath, ifName string) error {
 	containerNS, err := ns.GetNS(netnsPath)
 	if err != nil {
@@ -109,6 +112,9 @@ func cleanupContainerNetns(netnsPath, ifName string) error {
 			// Interface does not exist in container netns — nothing to clean up.
 			return nil
 		}
+		if _, ok := link.(*netlink.Veth); !ok {
+			return fmt.Errorf("interface %q is not a veth (type: %T), refusing to delete", ifName, link)
+		}
 		if err := handle.LinkDel(link); err != nil {
 			return fmt.Errorf("delete stale interface %q in container netns: %w", ifName, err)
 		}

internal/cni/netns_test.go

@@ -0,0 +1,195 @@
+// Copyright 2025 Datum Cloud, Inc.
+//
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package cni
+
+import (
+	"fmt"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/containernetworking/plugins/pkg/ns"
+	"github.com/vishvananda/netlink"
+)
+
+// requireRoot skips the test when not running as root.
+// Network namespace operations (unshare, veth creation) require CAP_SYS_ADMIN.
+func requireRoot(t *testing.T) {
+	t.Helper()
+	if os.Getuid() != 0 {
+		t.Skip("skipping: requires root (CAP_SYS_ADMIN)")
+	}
+}
+
+// createTestNetnsWithDummy creates a temporary network namespace, creates a
+// dummy interface inside it, and returns the netns path and a cleanup function.
+func createTestNetnsWithDummy(t *testing.T) (netnsPath string, cleanup func()) {
+	t.Helper()
+
+	requireRoot(t)
+
+	nsObj, err := ns.TempNetNS()
+	if err != nil {
+		t.Fatalf("create new netns: %v", err)
+	}
+
+	err = nsObj.Do(func(_ ns.NetNS) error {
+		handle, err := netlink.NewHandle()
+		if err != nil {
+			return err
+		}
+		defer handle.Close() //nolint:errcheck // best-effort cleanup
+
+		dummy := &netlink.Dummy{
+			LinkAttrs: netlink.LinkAttrs{Name: "test-dummy"},
+		}
+		if err := handle.LinkAdd(dummy); err != nil {
+			return fmt.Errorf("add dummy link: %w", err)
+		}
+		if err := handle.LinkSetUp(dummy); err != nil {
+			return fmt.Errorf("set dummy link up: %w", err)
+		}
+		return nil
+	})
+	if err != nil {
+		nsObj.Close() //nolint:errcheck // best-effort cleanup
+		t.Fatalf("setup dummy interface: %v", err)
+	}
+
+	cleanup = func() {
+		_ = nsObj.Do(func(_ ns.NetNS) error {
+			handle, err := netlink.NewHandle()
+			if err != nil {
+				return err
+			}
+			defer handle.Close() //nolint:errcheck // best-effort cleanup
+			link, err := handle.LinkByName("test-dummy")
+			if err != nil {
+				return nil // already gone
+			}
+			return handle.LinkDel(link)
+		})
+		nsObj.Close() //nolint:errcheck // best-effort cleanup
+	}
+
+	return nsObj.Path(), cleanup
+}
+
+func TestCleanupContainerNetnsNonVeth(t *testing.T) {
+	requireRoot(t)
+
+	netnsPath, cleanup := createTestNetnsWithDummy(t)
+	defer cleanup()
+
+	err := cleanupContainerNetns(netnsPath, "test-dummy")
+	if err == nil {
+		t.Fatal("expected error for non-veth interface, got nil")
+	}
+	if !strings.Contains(err.Error(), "is not a veth") {
+		t.Fatalf("error %q does not contain 'is not a veth'", err.Error())
+	}
+	if !strings.Contains(err.Error(), "test-dummy") {
+		t.Fatalf("error %q does not contain interface name 'test-dummy'", err.Error())
+	}
+}
+
+func TestCleanupContainerNetnsNonExistent(t *testing.T) {
+	requireRoot(t)
+
+	netnsPath, cleanup := createTestNetnsWithDummy(t)
+	defer cleanup()
+
+	// Cleanup a non-existent interface should return nil (idempotent).
+	err := cleanupContainerNetns(netnsPath, "does-not-exist")
+	if err != nil {
+		t.Fatalf("expected nil for non-existent interface, got: %v", err)
+	}
+}
+
+func TestCleanupContainerNetnsVeth(t *testing.T) {
+	requireRoot(t)
+
+	// Create a temporary network namespace with a veth pair.
+	nsObj, err := ns.TempNetNS()
+	if err != nil {
+		t.Fatalf("create new netns: %v", err)
+	}
+	defer nsObj.Close() //nolint:errcheck // best-effort cleanup
+
+	// Create a veth pair: one end in the new namespace ("test-veth"),
+	// the other end in the host namespace ("test-veth-peer").
+	hostVethName := "test-veth-host"
+	guestVethName := "test-veth"
+
+	err = nsObj.Do(func(_ ns.NetNS) error {
+		handle, err := netlink.NewHandle()
+		if err != nil {
+			return err
+		}
+		defer handle.Close() //nolint:errcheck // best-effort cleanup
+
+		veth := &netlink.Veth{
+			LinkAttrs: netlink.LinkAttrs{Name: guestVethName},
+			PeerName:  hostVethName,
+		}
+		if err := handle.LinkAdd(veth); err != nil {
+			return fmt.Errorf("add veth link: %w", err)
+		}
+		if err := handle.LinkSetUp(veth); err != nil {
+			return fmt.Errorf("set veth up: %w", err)
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("setup veth pair: %v", err)
+	}
+
+	// Verify the peer exists in host namespace.
+	hostHandle, err := netlink.NewHandle()
+	if err != nil {
+		t.Fatalf("create host netlink handle: %v", err)
+	}
+	defer hostHandle.Close() //nolint:errcheck // best-effort cleanup
+
+	_, err = hostHandle.LinkByName(hostVethName)
+	if err != nil {
+		t.Fatalf("veth peer %q not found in host ns: %v", hostVethName, err)
+	}
+
+	// Now call cleanupContainerNetns — it should succeed and delete the veth.
+	err = cleanupContainerNetns(nsObj.Path(), guestVethName)
+	if err != nil {
+		t.Fatalf("cleanupContainerNetns(veth) returned error: %v", err)
+	}
+
+	// Verify the veth is gone from the container namespace.
+	err = nsObj.Do(func(_ ns.NetNS) error {
+		handle, err := netlink.NewHandle()
+		if err != nil {
+			return err
+		}
+		defer handle.Close() //nolint:errcheck // best-effort cleanup
+		_, err = handle.LinkByName(guestVethName)
+		if err == nil {
+			return fmt.Errorf("interface %q still exists after cleanup", guestVethName)
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Verify the host-side peer still exists (only guest veth was deleted).
+	_, err = hostHandle.LinkByName(hostVethName)
+	if err != nil {
+		t.Fatalf("veth peer %q was unexpectedly deleted from host ns: %v", hostVethName, err)
+	}
+
+	// Clean up the host-side peer.
+	peer, _ := hostHandle.LinkByName(hostVethName)
+	if peer != nil {
+		_ = hostHandle.LinkDel(peer) //nolint:errcheck // best-effort cleanup
+	}
+}