Address review comments

Signed-off-by: Antonin Bas <abas@vmware.com>

.github/dependabot.yml

@@ -21,8 +21,6 @@ updates:
     - dependency-name: "antrea.io/ofnet"
     - dependency-name: "antrea.io/libOpenflow"
     - dependency-name: "github.com/ClickHouse/clickhouse-go/v2" # auto-upgrade involves dependency conflicts
-    - dependency-name: "github.com/alexflint/go-filemutex"
-      update-types: ["version-update:semver-major"] # ignore major updates only
   - package-ecosystem: "github-actions"
     # Workflow files stored in the default location of `.github/workflows`
     directory: "/"

build/yamls/antrea.yml

@@ -5528,7 +5528,7 @@ data:
 
     # Enable flowexporter which exports polled conntrack connections as IPFIX flow records from each
     # agent to a configured collector.
-    #  FlowExporter: false
+      FlowExporter: true
 
     # Enable collecting and exposing NetworkPolicy statistics.
     #  NetworkPolicyStats: true
@@ -5700,7 +5700,7 @@ data:
       # IPFIX flow records from each agent to a configured collector. To enable this
       # feature, you need to set "enable" to true, and ensure that the FlowExporter
       # feature gate is also enabled.
-      enable: false
+      enable: true
       # Provide the IPFIX collector address as a string with format <HOST>:[<PORT>][:<PROTO>].
       # HOST can either be the DNS name, IP, or Service name of the Flow Collector. If
       # using an IP, it can be either IPv4 or IPv6. However, IPv6 address should be

build/yamls/flow-aggregator.yml

@@ -292,7 +292,7 @@ data:
     # FlowLogger contains configuration options for writing flow records to a local log file.
     flowLogger:
       # Enable is the switch to enable writing flow records to a local log file.
-      enable: false
+      enable: true
 
       # Path is the path to the local log file.
       path: "/tmp/antrea-flows.log"

go.mod

@@ -11,7 +11,6 @@ require (
 	github.com/Microsoft/go-winio v0.6.1
 	github.com/Microsoft/hcsshim v0.9.10
 	github.com/TomCodeLV/OVSDB-golang-lib v0.0.0-20200116135253-9bbdfadcd881
-	github.com/alexflint/go-filemutex v1.2.0
 	github.com/aws/aws-sdk-go-v2 v1.16.10
 	github.com/aws/aws-sdk-go-v2/config v1.16.0
 	github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.11.23
@@ -91,6 +90,7 @@ require (
 	github.com/MakeNowJust/heredoc v1.0.0 // indirect
 	github.com/NYTimes/gziphandler v1.1.1 // indirect
 	github.com/VividCortex/ewma v1.2.0 // indirect
+	github.com/alexflint/go-filemutex v1.2.0 // indirect
 	github.com/andybalholm/brotli v1.0.4 // indirect
 	github.com/antlr/antlr4/runtime/Go/antlr v1.4.10 // indirect
 	github.com/armon/go-metrics v0.0.0-20180917152333-f0300d1749da // indirect

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,56 @@ 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 {
+		idx++
+		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))
+	})
+}