Fix CNS error handling in fsnotify to prevent possible IP address leaks

cns/fsnotify/fsnotify.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"io/fs"
 	"os"
+	"path/filepath"
 	"sync"
 	"time"
 
@@ -56,17 +57,25 @@ func (w *watcher) releaseAll(ctx context.Context) {
 	defer w.lock.Unlock()
 	for containerID := range w.pendingDelete {
 		// read file contents
-		filepath := w.path + "/" + containerID
+		filepath := filepath.Join(w.path, containerID)
 		file, err := os.Open(filepath)
 		if err != nil {
-			w.log.Error("failed to open file", zap.Error(err))
+			w.log.Error("failed to open container file for IP release", 
+				zap.String("filepath", filepath), 
+				zap.String("containerID", containerID), 
+				zap.Error(err))
+			continue
 		}
 
 		data, errReadingFile := io.ReadAll(file)
+		file.Close()
 		if errReadingFile != nil {
-			w.log.Error("failed to read file content", zap.Error(errReadingFile))
+			w.log.Error("failed to read container file content for IP release", 
+				zap.String("filepath", filepath), 
+				zap.String("containerID", containerID), 
+				zap.Error(errReadingFile))
+			continue
 		}
-		file.Close()
 		podInterfaceID := string(data)
 
 		w.log.Info("releasing IP for missed delete", zap.String("podInterfaceID", podInterfaceID), zap.String("containerID", containerID))
@@ -77,7 +86,10 @@ func (w *watcher) releaseAll(ctx context.Context) {
 		w.log.Info("successfully released IP for missed delete", zap.String("containerID", containerID))
 		delete(w.pendingDelete, containerID)
 		if err := removeFile(containerID, w.path); err != nil {
-			w.log.Error("failed to remove file for missed delete", zap.Error(err))
+			w.log.Error("failed to remove container file after IP release", 
+				zap.String("containerID", containerID),
+				zap.String("path", w.path),
+				zap.Error(err))
 		}
 	}
 }
@@ -177,7 +189,7 @@ func (w *watcher) Start(ctx context.Context) error {
 
 // AddFile creates new file using the containerID as name
 func AddFile(podInterfaceID, containerID, path string) error {
-	filepath := path + "/" + containerID
+	filepath := filepath.Join(path, containerID)
 	f, err := os.Create(filepath)
 	if err != nil {
 		return errors.Wrap(err, "error creating file")
@@ -191,7 +203,7 @@ func AddFile(podInterfaceID, containerID, path string) error {
 
 // removeFile removes the file based on containerID
 func removeFile(containerID, path string) error {
-	filepath := path + "/" + containerID
+	filepath := filepath.Join(path, containerID)
 	if err := os.Remove(filepath); err != nil {
 		return errors.Wrap(err, "error deleting file")
 	}

cns/fsnotify/fsnotify_test.go

@@ -1,13 +1,83 @@
 package fsnotify
 
 import (
+	"context"
 	"os"
 	"testing"
 
+	"github.com/Azure/azure-container-networking/cns"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
 )
 
+// Mock implementation of ReleaseIPsClient for testing
+type mockReleaseIPsClient struct {
+	releaseIPCalled bool
+	containerIDs    []string
+	podInterfaceIDs []string
+}
+
+func (m *mockReleaseIPsClient) ReleaseIPs(_ context.Context, ipconfig cns.IPConfigsRequest) error {
+	m.releaseIPCalled = true
+	m.containerIDs = append(m.containerIDs, ipconfig.InfraContainerID)
+	m.podInterfaceIDs = append(m.podInterfaceIDs, ipconfig.PodInterfaceID)
+	return nil
+}
+
+func TestReleaseAll(t *testing.T) {
+	// Create a temporary directory for tests
+	tempDir, err := os.MkdirTemp("", "fsnotify-test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempDir)
+
+	// Create test files
+	validContainerID := "valid-container"
+	validPodInterfaceID := "valid-pod-interface"
+	err = AddFile(validPodInterfaceID, validContainerID, tempDir)
+	require.NoError(t, err)
+
+	// Create a directory with the name of a containerID to simulate "file is a directory" error
+	invalidContainerID := "invalid-container"
+	err = os.Mkdir(tempDir+"/"+invalidContainerID, 0755)
+	require.NoError(t, err)
+
+	// Create a test watcher
+	mockClient := &mockReleaseIPsClient{}
+	logger, _ := zap.NewDevelopment()
+	w := &watcher{
+		cli:           mockClient,
+		path:          tempDir,
+		log:           logger,
+		pendingDelete: map[string]struct{}{},
+	}
+
+	// Add valid and invalid containerIDs to pendingDelete
+	w.pendingDelete[validContainerID] = struct{}{}
+	w.pendingDelete[invalidContainerID] = struct{}{}
+
+	// Test releaseAll
+	w.releaseAll(context.Background())
+
+	// Verify that ReleaseIPs was called only for the valid container
+	assert.True(t, mockClient.releaseIPCalled)
+	assert.Equal(t, []string{validContainerID}, mockClient.containerIDs)
+	assert.Equal(t, []string{validPodInterfaceID}, mockClient.podInterfaceIDs)
+
+	// Verify that only the valid container was removed from pendingDelete
+	_, validExists := w.pendingDelete[validContainerID]
+	assert.False(t, validExists, "valid container should be removed from pendingDelete")
+
+	_, invalidExists := w.pendingDelete[invalidContainerID]
+	assert.True(t, invalidExists, "invalid container should still be in pendingDelete")
+}
+
 func TestAddFile(t *testing.T) {
+	// Create a temporary directory for tests
+	tempDir, err := os.MkdirTemp("", "fsnotify-addfile-test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempDir)
+
 	type args struct {
 		podInterfaceID string
 		containerID    string
@@ -23,7 +93,7 @@ func TestAddFile(t *testing.T) {
 			args: args{
 				podInterfaceID: "123",
 				containerID:    "67890",
-				path:           "/bad/path",
+				path:           tempDir + "/nonexistent",
 			},
 			wantErr: true,
 		},
@@ -32,15 +102,13 @@ func TestAddFile(t *testing.T) {
 			args: args{
 				podInterfaceID: "345",
 				containerID:    "12345",
-				path:           "/path/we/want",
+				path:           tempDir,
 			},
 			wantErr: false,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := os.MkdirAll("/path/we/want", 0o777)
-			require.NoError(t, err)
 			if err := AddFile(tt.args.podInterfaceID, tt.args.containerID, tt.args.path); (err != nil) != tt.wantErr {
 				t.Errorf("WatcherAddFile() error = %v, wantErr %v", err, tt.wantErr)
 			}
@@ -49,6 +117,11 @@ func TestAddFile(t *testing.T) {
 }
 
 func TestWatcherRemoveFile(t *testing.T) {
+	// Create a temporary directory for tests
+	tempDir, err := os.MkdirTemp("", "fsnotify-removefile-test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempDir)
+
 	type args struct {
 		containerID string
 		path        string
@@ -62,23 +135,27 @@ func TestWatcherRemoveFile(t *testing.T) {
 			name: "remove file fail",
 			args: args{
 				containerID: "12345",
-				path:        "/bad/path",
+				path:        tempDir + "/nonexistent",
 			},
 			wantErr: true,
 		},
 		{
-			name: "no such directory, add fail",
+			name: "remove existing file",
 			args: args{
 				containerID: "67890",
-				path:        "/path/we/want",
+				path:        tempDir,
 			},
 			wantErr: false,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := os.MkdirAll("/path/we/want/67890", 0o777)
-			require.NoError(t, err)
+			if tt.name == "remove existing file" {
+				// Create the file to be removed
+				err := AddFile("test", tt.args.containerID, tt.args.path)
+				require.NoError(t, err)
+			}
+
 			if err := removeFile(tt.args.containerID, tt.args.path); (err != nil) != tt.wantErr {
 				t.Errorf("WatcherRemoveFile() error = %v, wantErr %v", err, tt.wantErr)
 			}