diff --git a/cns/fsnotify/fsnotify.go b/cns/fsnotify/fsnotify.go index 16bff5a49d..54a661d8dd 100644 --- a/cns/fsnotify/fsnotify.go +++ b/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") } diff --git a/cns/fsnotify/fsnotify_test.go b/cns/fsnotify/fsnotify_test.go index d5e081321f..66ea7838fc 100644 --- a/cns/fsnotify/fsnotify_test.go +++ b/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) }