Address review comments

Signed-off-by: Antonin Bas <[email protected]>

pkg/agent/cniserver/ipam/hostlocal/gc.go

@@ -22,12 +22,14 @@ import (
 	"runtime"
 	"strings"
 
+	"github.com/containernetworking/plugins/plugins/ipam/host-local/backend/disk"
 	"github.com/spf13/afero"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 )
 
-const dataDir = "/var/lib/cni/networks"
+// dataDir is a variable so it can be overridden by tests if needed
+var dataDir = "/var/lib/cni/networks"
 
 func networkDir(network string) string {
 	return filepath.Join(dataDir, network)
@@ -48,7 +50,7 @@ func GarbageCollectContainerIPs(network string, desiredIPs sets.Set[string]) err
 		return fmt.Errorf("path '%s' is not a directory: %w", dir, err)
 	}
 
-	lk, err := NewFileLock(dataDir)
+	lk, err := disk.NewFileLock(dir)
 	if err != nil {
 		return err
 	}
@@ -75,33 +77,32 @@ func gcContainerIPs(fs afero.Fs, dir string, desiredIPs sets.Set[string]) error
 		return fmt.Errorf("error when gathering IP filenames in the host-local data directory: %w", err)
 	}
 
-	allocatedIPs := sets.New[string]()
+	hasRemovalError := false
 	for _, p := range paths {
 		ip := getIPFromPath(p)
 		if net.ParseIP(ip) == nil {
 			// not a valid IP, nothing to do
 			continue
 		}
-		allocatedIPs.Insert(ip)
 		if desiredIPs.Has(ip) {
 			// IP is in-use
 			continue
 		}
 		if err := fs.Remove(p); err != nil {
 			klog.ErrorS(err, "Failed to release unused IP from host-local IPAM plugin", "IP", ip)
+			hasRemovalError = true
 			continue
 		}
-		allocatedIPs.Delete(ip)
 		klog.InfoS("Unused IP was successfully released from host-local IPAM plugin", "IP", ip)
 	}
 
-	if allocatedIPs.Difference(desiredIPs).Len() > 0 {
+	if hasRemovalError {
 		return fmt.Errorf("not all unused IPs could be released from host-local IPAM plugin, some IPs may be leaked")
 	}
 
-	// Note that it is perfectly possible for some IPs to be in desiredIPs but not in
-	// allocatedIPs. This can be the case when another IPAM plugin (e.g., AntreaIPAM) is also
-	// used.
+	// Note that it is perfectly possible for some IPs to be in desiredIPs but not in the
+	// host-local data directory. This can be the case when another IPAM plugin (e.g.,
+	// AntreaIPAM) is also used.
 
 	return nil
 }

pkg/agent/cniserver/ipam/hostlocal/gc_test.go

@@ -176,3 +176,55 @@ func TestGcContainerIPs(t *testing.T) {
 	runTests(t, false)
 	runTests(t, true)
 }
+
+// TestGarbageCollectContainerIPs tests some edge cases and logic that depends on the real OS
+// filesystem. The actual GC logic is tested by TestGcContainerIPs.
+func TestGarbageCollectContainerIPs(t *testing.T) {
+	ips := sets.New[string]()
+	tempDir, err := os.MkdirTemp("", "test-networks")
+	require.NoError(t, err)
+	savedDir := dataDir
+	defer func() {
+		dataDir = savedDir
+	}()
+	dataDir = tempDir
+	defer os.RemoveAll(tempDir)
+
+	idx := 0
+	networkName := func() string {
+		return fmt.Sprintf("net%d", idx)
+	}
+
+	lockFile := func(network string) string {
+		return filepath.Join(tempDir, network, "lock")
+	}
+
+	t.Run("missing directory", func(t *testing.T) {
+		network := networkName()
+		// there is no directory in tempDir for the "antrea" network
+		// we don't expect an error, and the lock file should not be created
+		require.NoError(t, GarbageCollectContainerIPs(network, ips))
+		assert.NoFileExists(t, lockFile(network))
+	})
+
+	t.Run("not a directory", func(t *testing.T) {
+		network := networkName()
+		netDir := filepath.Join(tempDir, network)
+		// create a file instead of a directory: GarbageCollectContainerIPs should return an
+		// error
+		_, err := os.Create(netDir)
+		require.NoError(t, err)
+		defer os.Remove(netDir)
+		assert.ErrorContains(t, GarbageCollectContainerIPs(network, ips), "not a directory")
+	})
+
+	t.Run("lock file created", func(t *testing.T) {
+		network := networkName()
+		netDir := filepath.Join(tempDir, network)
+		require.NoError(t, os.Mkdir(netDir, 0o755))
+		defer os.RemoveAll(netDir)
+		// make sure that the lock file is created in the right place
+		require.NoError(t, GarbageCollectContainerIPs(network, ips))
+		assert.FileExists(t, lockFile(network))
+	})
+}