[Shared Mount] NSV - Add start gcsfuse#1463
Conversation
|
@chrisThePattyEater: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the mounting flow to start GCSFuse by sending a gRPC request to the mounter pod, utilizing a symlinked socket path to bypass path length limitations, and adds error-reading capabilities. The review feedback identifies several critical issues: the need to use an absolute path and remove stale symlinks for idempotency, correcting the error handling for missing files, preventing symlink traversal vulnerabilities, and overriding the symlink base path in unit tests to prevent permission failures.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
from
aa54d3a to
3b326fe
Compare
chrisThePattyEater
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the start of the GCSFuse flow in the CSI driver's NodeStageVolume operation by introducing a gRPC connection over a unix socket to the mounter pod. It also adds logic to read GCSFuse error files to surface failures to the user, along with corresponding unit tests. The review feedback highlights a critical security vulnerability regarding potential symlink traversal when reading the error file, suggests removing stale symlinks to ensure idempotency, points out a variable shadowing issue, and recommends adding a unit test to verify symlink detection.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
2 times, most recently
from
e842f99 to
c69dbf9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the mountToNode function to establish a gRPC connection to the mounter pod via a symbolic link and trigger the GCSFuse mount process during NodeStageVolume. The review feedback highlights critical issues: an incorrect socket path resolution, the need to clear stale error files before mounting, and a potential write failure on read-only filesystems when creating the symlink in the working directory. Suggestions are provided to resolve these issues by passing the correct path, clearing stale files, using /tmp for the symlink, and updating the corresponding unit tests.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
2 times, most recently
from
891a725 to
87b039f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the GCSFuse mount flow by introducing the mountToNode function, which connects to the mounter pod via a gRPC client using a symbolic link to bypass Unix domain socket path length limitations. It also adds error-handling logic to surface GCSFuse errors and includes initial unit tests. The review feedback highlights a critical security vulnerability regarding the use of the world-writable /tmp directory for symlinks, suggesting a transition to a secure, root-owned directory. Additionally, the feedback recommends clearing stale error files before new mount attempts to avoid false failure detections, along with corresponding updates to the unit tests.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
2 times, most recently
from
c0368c8 to
7b1558b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the GCSFuse startup flow within the CSI driver's node staging process by introducing the mountToNode function, which establishes a gRPC connection to the mounter pod via a symbolic link. Feedback on these changes highlights two key issues: first, a potential symlink traversal vulnerability where the socket file should be verified as not being a symbolic link before connecting; second, a Go anti-pattern in the unit tests where defer is used inside a for loop instead of utilizing t.Cleanup inside the t.Run block.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
3 times, most recently
from
0b5437d to
982a613
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the GCSFuse mount flow by introducing a mountToNode function that establishes a gRPC connection to the mounter server via a temporary symbolic link. Feedback on these changes highlights three key areas for improvement: resolving the socket file path with filepath.EvalSymlinks to prevent symlink traversal attacks, avoiding a data race in parallel tests caused by modifying the global symlinkBaseDir variable, and replacing the deprecated grpc.WithInsecure() call with grpc.WithTransportCredentials(insecure.NewCredentials()).
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
2 times, most recently
from
e0315e7 to
b18caf0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the GCSFuse start flow by introducing the mountToNode function, which uses a symbolic link to connect to the mounter server via GRPC, bypassing socket path length limitations. It also updates the node staging process and adds corresponding unit tests. Key feedback highlights a potential directory-level symlink traversal vulnerability that should be mitigated by resolving and validating the socket path, and recommends refactoring the global symlinkBaseDir variable into the driver configuration or nodeServer struct to prevent global data races and allow tests to run in parallel.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
from
7be02a8 to
57453a7
Compare
| }, | ||
| SharedMountOptions: &driver.SharedMountOptions{ | ||
| MounterPodImage: *mounterPodImage, | ||
| FuseSocketDir: *fuseSocketDir, |
There was a problem hiding this comment.
Can you also put the emptyDirBasePath function here, as we discussed via chat?
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
from
57453a7 to
c2f3679
Compare
| // Create a symlink to bypass the 108-character limit for Unix domain sockets | ||
| // when dialing the connection from the Node Server. | ||
| if s.driver.config.FeatureOptions == nil || s.driver.config.FeatureOptions.SharedMountOptions == nil || s.driver.config.FeatureOptions.SharedMountOptions.FuseSocketDir == "" { | ||
| return fmt.Errorf("FuseSocketDir is not configured") |
There was a problem hiding this comment.
This error returns no error code, and L741 doesn't return anything either, so this is going to be "Unknown". Please make this an internal error in exactly this line. We should be notified via SLO if this is missing. Also, change message to "fuse socket dir is not configured" to avoid returning implementation details to the user.
| } | ||
| symlink := filepath.Join(s.driver.config.FeatureOptions.SharedMountOptions.FuseSocketDir, mountSocketDir, podUID) | ||
| if err := os.MkdirAll(filepath.Dir(symlink), 0750); err != nil { | ||
| return fmt.Errorf("failed to create dir for symlink %q: %w", symlink, err) |
There was a problem hiding this comment.
Again, no error code. This should be Internal.
| mounterPodMountDir = "mount-dir" | ||
| mounterPodSocketFile = "mounter.sock" | ||
| mounterPodManagedImageKeyword = "managed" | ||
| mountSocketDir = "mount-socket" |
There was a problem hiding this comment.
mounterPodSocketDir, to keep consistency with every other const above.
| } | ||
|
|
||
| if err := os.Symlink(socketFile, symlink); err != nil { | ||
| return fmt.Errorf("failed to create symlink to %q: %w", socketFile, err) |
There was a problem hiding this comment.
Return internal error.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
3 times, most recently
from
d42bbbd to
3b9c2c9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the GCS CSI driver configuration by moving EmptyDirBasePath into SharedMountOptions under FeatureOptions and introducing a new FuseSocketDir configuration. It also implements the mountToNode method in node.go to establish a connection to the mounter server using a symlink to bypass the Unix domain socket character limit, along with corresponding unit tests. Feedback on the changes highlights that status.Errorf does not support the %w verb for error wrapping and recommends using %v instead. Additionally, the reviewer noted redundant nil checks in mountToNode that occur after dereferencing, which could lead to a panic, and suggested refactoring the checks to fail fast.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
from
3b9c2c9 to
77b6bb5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request restructures the CSI driver configuration by moving EmptyDirBasePath into FeatureOptions.SharedMountOptions and introducing FuseSocketDir to handle Unix domain socket connections to the mounter pod. It also implements a mountToNode method to establish gRPC connections with the mounter pod. The review feedback highlights a critical nil pointer dereference risk in mountToNode where configuration options are accessed before being validated, and suggests making the new TestMountToNode test hermetic by overriding the base path with a temporary directory.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
from
77b6bb5 to
edf89c7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the GCS FUSE CSI driver configuration by moving EmptyDirBasePath into SharedMountOptions and introducing FuseSocketDir. It also implements the mountToNode method in nodeServer to establish a gRPC connection to the mounter pod using a Unix domain socket, utilizing a symlink to bypass the 108-character path limit. Feedback on the changes highlights a potential security vulnerability where socketFile should be verified using os.Lstat to ensure it is not a symlink, preventing symlink traversal or hijacking attacks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the CSI driver to support mounting to nodes via a gRPC connection to the mounter pod. It introduces FuseSocketDir and moves EmptyDirBasePath into SharedMountOptions. It also implements the mountToNode method in node.go which establishes a gRPC connection using a Unix domain socket, utilizing a symlink to bypass the 108-character path limit. A test for mountToNode is added. Feedback suggests removing the creation of a dummy socket file in the test since grpc.NewClient is asynchronous and the actual gRPC call is currently commented out.
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
from
edf89c7 to
fcfe2ff
Compare
chrisThePattyEater
force-pushed
the
waitForMounterPodServer
branch
from
fcfe2ff to
7163f4b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chrisThePattyEater, uriel-guzman The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
chrisThePattyEater
merged commit 85c4d66
into
GoogleCloudPlatform:main
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds the logic to send a grpc request to the mounter pods grpc server and start the GCSFuse process.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: