Add host-local IPAM GC on startup (#5660)

During CNIServer reconciliation, we perform host-local IPAM garbage
collection (GC) by comparing the set of IPs allocated to local Pods and
the set of IPs currently reserved by the plugin. We release any IP
reserved by the plugin that is not in-use by a local Pod. The purpose is
to avoid leaking IP addresses when there is a bug in the container
runtime, which has happened in the past.

Two key design choices that were made:
* We do not invoke CNI DEL to release IPs, instead we access the
  host-local data which is persisted on the Node, and modify it as
  needed.
* We do not rely on the interface store (as persisted to OVSDB) to
  determine the set of IPs that may have been leaked. In case of an
  Antrea bug, it could be possible (although unlikely) for an IP to
  still be allocated by host-local but be missing from the interface
  store. Instead, we list all allocated IPs from the host-local data (an
  allocated IP corresponds to one disk file).

This approach is essentially the same as our existing script:
https://github.com/antrea-io/antrea/blob/main/hack/gc-host-local.sh

Fixes #4326

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

go.mod

@@ -90,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

go.sum

@@ -123,6 +123,8 @@ github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRF
 github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d/go.mod h1:rBZYJk541a8SKzHPHnH3zbiI+7dagKZ0cgpgrD7Fyho=
 github.com/alessio/shellescape v1.2.2/go.mod h1:PZAiSCk0LJaZkiCSkPv8qIobYglO3FPpyFjDCtHLS30=
 github.com/alexflint/go-filemutex v0.0.0-20171022225611-72bdc8eae2ae/go.mod h1:CgnQgUtFrFz9mxFNtED3jI5tLDjKlOM+oUF/sTk6ps0=
+github.com/alexflint/go-filemutex v1.2.0 h1:1v0TJPDtlhgpW4nJ+GvxCLSlUDC3+gW0CQQvlmfDR/s=
+github.com/alexflint/go-filemutex v1.2.0/go.mod h1:mYyQSWvw9Tx2/H2n9qXPb52tTYfE0pZAWcBq5mK025c=
 github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8=
 github.com/andybalholm/brotli v1.0.4 h1:V7DdXeJtZscaqfNuAdSRuRFzuiKlHSC/Zh3zl9qY3JY=
 github.com/andybalholm/brotli v1.0.4/go.mod h1:fO7iG3H7G2nSZ7m0zPUDn85XEX2GTukHGRSepvi9Eig=

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

@@ -0,0 +1,118 @@
+// Copyright 2023 Antrea Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package hostlocal
+
+import (
+	"fmt"
+	"net"
+	"os"
+	"path/filepath"
+	"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"
+)
+
+// 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)
+}
+
+// This is a hacky approach as we access the internals of the host-local plugin,
+// instead of using the CNI interface. However, crafting a CNI DEL request from
+// scratch would also be hacky.
+func GarbageCollectContainerIPs(network string, desiredIPs sets.Set[string]) error {
+	dir := networkDir(network)
+
+	info, err := os.Stat(dir)
+	if os.IsNotExist(err) {
+		klog.V(2).InfoS("Host-local IPAM data directory does not exist, nothing to do", "dir", dir)
+		return nil
+	}
+	if !info.IsDir() {
+		return fmt.Errorf("path '%s' is not a directory: %w", dir, err)
+	}
+
+	lk, err := disk.NewFileLock(dir)
+	if err != nil {
+		return err
+	}
+	defer lk.Close()
+	lk.Lock()
+	defer lk.Unlock()
+
+	fs := afero.NewOsFs()
+	return gcContainerIPs(fs, dir, desiredIPs)
+}
+
+// Internal version of GarbageCollectContainerIPs which does not acquire the
+// file lock and can work with an arbitrary afero filesystem.
+func gcContainerIPs(fs afero.Fs, dir string, desiredIPs sets.Set[string]) error {
+	paths := make([]string, 0)
+
+	if err := afero.Walk(fs, dir, func(path string, info os.FileInfo, err error) error {
+		if err != nil || info.IsDir() {
+			return nil
+		}
+		paths = append(paths, path)
+		return nil
+	}); err != nil {
+		return fmt.Errorf("error when gathering IP filenames in the host-local data directory: %w", err)
+	}
+
+	hasRemovalError := false
+	for _, p := range paths {
+		ip := getIPFromPath(p)
+		if net.ParseIP(ip) == nil {
+			// not a valid IP, nothing to do
+			continue
+		}
+		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
+		}
+		klog.InfoS("Unused IP was successfully released from host-local IPAM plugin", "IP", ip)
+	}
+
+	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 the
+	// host-local data directory. This can be the case when another IPAM plugin (e.g.,
+	// AntreaIPAM) is also used.
+
+	return nil
+}
+
+func getIPFromPath(path string) string {
+	fname := filepath.Base(path)
+	// need to unespace IPv6 addresses on Windows
+	// see https://github.com/containernetworking/plugins/blob/38f18d26ecfef550b8bac02656cc11103fd7cff1/plugins/ipam/host-local/backend/disk/backend.go#L197
+	if runtime.GOOS == "windows" {
+		fname = strings.ReplaceAll(fname, "_", ":")
+	}
+	return fname
+}

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

@@ -0,0 +1,231 @@
+// Copyright 2023 Antrea Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package hostlocal
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/util/sets"
+)
+
+type testFs struct {
+	afero.Fs
+	removeError  error
+	removedFiles []string
+}
+
+func (fs *testFs) Remove(name string) error {
+	if fs.removeError != nil {
+		err := &os.PathError{Op: "remove", Path: name, Err: fs.removeError}
+		// reset error
+		fs.removeError = nil
+		return err
+	}
+	if err := fs.Fs.Remove(name); err != nil {
+		return err
+	}
+	fs.removedFiles = append(fs.removedFiles, name)
+	return nil
+}
+
+// forceRemoveError forces the next Remove call to fail. Error will be cleared after the first call.
+func (fs *testFs) forceRemoveError() {
+	fs.removeError = fmt.Errorf("permission denied")
+}
+
+func (fs *testFs) removedIPs() sets.Set[string] {
+	s := sets.New[string]()
+	for _, p := range fs.removedFiles {
+		s.Insert(getIPFromPath(p))
+	}
+	return s
+}
+
+// from https://github.com/containernetworking/plugins/blob/38f18d26ecfef550b8bac02656cc11103fd7cff1/plugins/ipam/host-local/backend/disk/backend.go#L197
+func getEscapedPath(dir string, fname string) string {
+	if runtime.GOOS == "windows" {
+		fname = strings.ReplaceAll(fname, ":", "_")
+	}
+	return filepath.Join(dir, fname)
+}
+
+func allocateIPs(t *testing.T, fs afero.Fs, dir string, ips ...string) {
+	for _, ip := range ips {
+		path := getEscapedPath(dir, ip)
+		// The real host-local IPAM plugin writes the container ID + interface name to the
+		// file, but it is irrelevant in our case.
+		require.NoError(t, afero.WriteFile(fs, path, []byte("foo"), 0o600))
+	}
+}
+
+func TestGcContainerIPs(t *testing.T) {
+	dir := networkDir("antrea")
+
+	newTestFs := func() *testFs {
+		return &testFs{
+			Fs: afero.NewMemMapFs(),
+		}
+	}
+
+	t.Run("missing directory", func(t *testing.T) {
+		fs := newTestFs()
+		// create the plugin data directory, but not the "network" sub-directory
+		require.NoError(t, fs.MkdirAll(dataDir, 0o755))
+		assert.NoError(t, gcContainerIPs(fs, dir, sets.New[string]()))
+		removedIPs := fs.removedIPs()
+		assert.Empty(t, removedIPs)
+	})
+
+	t.Run("remove error", func(t *testing.T) {
+		ips := []string{"10.0.0.1", "10.0.0.2"}
+		fs := newTestFs()
+		require.NoError(t, fs.MkdirAll(dir, 0o755))
+		allocateIPs(t, fs, dir, ips...)
+		fs.forceRemoveError()
+		require.Error(t, gcContainerIPs(fs, dir, sets.New[string]()))
+		// one of the IPs will fail to be released, the other one will succeed
+		removedIPs := fs.removedIPs()
+		assert.Len(t, removedIPs, 1)
+	})
+
+	resolveIP := func(id int, ipv6 bool) string {
+		if ipv6 {
+			return fmt.Sprintf("2001:db8:a::%d", id)
+		} else {
+			return fmt.Sprintf("10.0.0.%d", id)
+		}
+	}
+
+	// some success test cases, will be run for both IPv4 and IPv6
+	testCases := []struct {
+		name               string
+		desiredIPs         []int
+		allocatedIPs       []int
+		expectedRemovedIPs []int
+	}{
+		{
+			name:               "same sets",
+			desiredIPs:         []int{1, 2},
+			allocatedIPs:       []int{1, 2},
+			expectedRemovedIPs: []int{},
+		},
+		{
+			name:               "multiple removals",
+			desiredIPs:         []int{1, 3},
+			allocatedIPs:       []int{1, 2, 3, 4},
+			expectedRemovedIPs: []int{2, 4},
+		},
+		{
+			name:               "extra ip",
+			desiredIPs:         []int{1, 2, 3},
+			allocatedIPs:       []int{1, 2},
+			expectedRemovedIPs: []int{},
+		},
+	}
+
+	runTests := func(t *testing.T, ipv6 bool) {
+		name := "ipv4"
+		if ipv6 {
+			name = "ipv6"
+		}
+
+		toIPSet := func(ids []int) sets.Set[string] {
+			ips := sets.New[string]()
+			for _, id := range ids {
+				ip := resolveIP(id, ipv6)
+				require.NotEmpty(t, ip)
+				ips.Insert(ip)
+			}
+			return ips
+		}
+
+		t.Run(name, func(t *testing.T) {
+			for _, tc := range testCases {
+				fs := newTestFs()
+				require.NoError(t, fs.MkdirAll(dir, 0o755))
+				desiredIPs := toIPSet(tc.desiredIPs)
+				allocatedIPs := toIPSet(tc.allocatedIPs)
+				expectedRemovedIPs := toIPSet(tc.expectedRemovedIPs)
+				allocateIPs(t, fs, dir, allocatedIPs.UnsortedList()...)
+				require.NoError(t, gcContainerIPs(fs, dir, desiredIPs))
+				assert.Equal(t, expectedRemovedIPs, fs.removedIPs())
+			}
+		})
+	}
+
+	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))
+	})
+}

pkg/agent/cniserver/ipam/ipam_delegator.go

@@ -22,8 +22,10 @@ import (
 	"github.com/containernetworking/cni/pkg/invoke"
 	"github.com/containernetworking/cni/pkg/types"
 	current "github.com/containernetworking/cni/pkg/types/100"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 
+	"antrea.io/antrea/pkg/agent/cniserver/ipam/hostlocal"
 	argtypes "antrea.io/antrea/pkg/agent/cniserver/types"
 )
 
@@ -86,6 +88,18 @@ func (d *IPAMDelegator) Check(args *invoke.Args, k8sArgs *argtypes.K8sArgs, netw
 	return true, nil
 }
 
+// GarbageCollectContainerIPs will release IPs allocated by the delegated IPAM
+// plugin that are no longer in-use (if there is any). It should be called on an
+// agent restart to provide garbage collection for IPs, and to avoid IP leakage
+// in case of missed CNI DEL events. Normally, it is not Antrea's responsibility
+// to implement this, as the above layers should ensure that there is always one
+// successful CNI DEL for every corresponding CNI ADD. However, we include this
+// support to increase robustness in case of a container runtime bug.
+// Only the host-local plugin is supported.
+func GarbageCollectContainerIPs(network string, desiredIPs sets.Set[string]) error {
+	return hostlocal.GarbageCollectContainerIPs(network, desiredIPs)
+}
+
 var defaultExec invoke.Exec = &invoke.DefaultExec{
 	RawExec: &invoke.RawExec{Stderr: os.Stderr},
 }