Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Copilot
Copy link

@Copilot Copilot AI commented May 19, 2025

Issue

In the releaseAll function of the CNS fsnotify package, when errors occur during file operations (opening or reading files), the code logs the error but continues execution. This can lead to the podInterfaceID variable having an empty string value, which when passed to releaseIP could potentially fail to release IP addresses properly.

Changes

This PR fixes the error handling in the releaseAll method to properly handle file operation errors:

  1. Added continue statements after file open errors to skip to the next containerID in the loop
  2. Moved the file.Close() call before checking for read errors to ensure the file is always closed regardless of read errors
  3. Added a continue statement after file read errors to skip to the next containerID in the loop

Testing

Added a new test (TestReleaseAll) that verifies:

  • The code handles invalid files correctly (e.g., directories that can't be opened for reading)
  • Only valid files are processed and their IPs are released
  • Invalid entries remain in the pendingDelete map for potential retry

Also updated existing tests to use temporary directories instead of trying to access root filesystem paths, ensuring more reliable test execution in different environments.

Fixes #3557.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] CNS Possibly Not Handling Error Correctly and Possibly Not Releasing IP Address Fix CNS error handling in fsnotify to prevent possible IP address leaks May 19, 2025
@Copilot Copilot AI requested a review from rbtr May 19, 2025 20:41
Copilot finished work on behalf of rbtr May 19, 2025 20:41
@rbtr rbtr requested a review from MikeZappa87 May 19, 2025 20:45
@@ -60,13 +60,15 @@ func (w *watcher) releaseAll(ctx context.Context) {
file, err := os.Open(filepath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filepath pkg might be a better option here, unsure if CNS runs on both windows/linux

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code to use filepath.Join for path construction instead of string concatenation. This change improves cross-platform compatibility for both Windows and Linux environments. The update has been applied in all three instances where paths were being constructed (632dd28).

Copilot finished work on behalf of MikeZappa87 May 19, 2025 21:02
@@ -56,17 +57,19 @@ 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a reader of the logs, do I have enough information here to troubleshoot? Keep in mind the reader might be not familiar with the code or be technical.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've enhanced the error log messages to provide much more context for troubleshooting. The updated logs now include the filepath, containerID, and clearer descriptions of the operations being performed. This makes the logs more helpful for users who might not be familiar with the code. These improvements have been implemented in commit ce76903.

Copilot finished work on behalf of MikeZappa87 May 19, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CNS Possibly Not Handling Error Correctly and Possibly Not Releasing IP Address
3 participants