feat(juicefs): clean fuse cache when JuiceFSRuntime shutdown#5650
feat(juicefs): clean fuse cache when JuiceFSRuntime shutdown#5650Sumitsh28 wants to merge 1 commit intofluid-cloudnative:masterfrom
Conversation
Signed-off-by: Sumitsh28 <sumit.off28@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Summary of ChangesHello @Sumitsh28, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Hi @Sumitsh28. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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 enhances the JuiceFSRuntime shutdown process by adding cleanup for the Fuse client cache. However, it introduces critical security vulnerabilities related to command injection in the getUUID function via user-controlled metadata URLs, and path traversal due to insufficient validation of user-supplied cache directories and UUIDs during path construction for cache deletion. Additionally, the implementation introduces significant code duplication in pkg/ddc/juicefs/shutdown.go regarding cache cleanup and directory retrieval logic, which should be refactored for better maintainability.
| if len(fusePods) > 0 { | ||
| // If UUID was not found from workers (e.g. workers already down), try to get it from fuse | ||
| if uuid == "" { | ||
| uuid, err = j.getUUID(fusePods[0], common.JuiceFSFuseContainer) |
There was a problem hiding this comment.
The call to j.getUUID at this line triggers a critical command injection vulnerability. The getUUID function retrieves a source value from a ConfigMap and interpolates it directly into a shell command string via operations.JuiceFileUtils.GetStatus. This allows for Remote Code Execution (RCE) if an attacker injects shell metacharacters into the metadata URL. This also contributes to significant code duplication with the worker cache cleanup logic, which could be refactored into a shared helper function for better maintainability.
| fuseOptions := runtime.Spec.Fuse.Options | ||
| for k, v := range fuseOptions { | ||
| if k == "cache-dir" { | ||
| cacheDirs = append(cacheDirs, strings.Split(v, ":")...) |
There was a problem hiding this comment.
The cache-dir option from fuse.options is split and appended to cacheDirs without sufficient validation. This introduces a path traversal vulnerability where a user could provide a path to a sensitive directory, potentially leading to arbitrary file deletion during cleanupCache. User-supplied paths must be validated to ensure they remain within expected boundaries. Additionally, the getFuseCacheDirs function is nearly identical to getCacheDirs, leading to code duplication. Consider creating a generic helper function to avoid this.
|
|
||
| cacheDirsToBeDeleted := []string{} | ||
| for _, cacheDir := range fuseCacheDirs { | ||
| cacheDirsToBeDeleted = append(cacheDirsToBeDeleted, filepath.Join(cacheDir, uuid, "raw/chunks")) |
There was a problem hiding this comment.
The cache cleanup logic is vulnerable to path traversal. The code constructs paths for deletion using filepath.Join(cacheDir, uuid, "raw/chunks"). Both cacheDir and uuid are influenced by user-provided configuration (e.g., fuse.options["cache-dir"] and the metadata source in Enterprise Edition).
An attacker can provide malicious values containing path traversal sequences (e.g., ../../../../etc) to point the deletion command to unintended locations. Although DeleteCacheDirs checks for a raw/chunks suffix, an attacker could still delete arbitrary directories that end with this suffix or create such a directory structure to facilitate deletion of sensitive data.
Note that this also affects the path construction on line 152.
1 participant
Ⅰ. Describe what this PR does
This PR implements the cleanup of the Fuse client cache during the
JuiceFSRuntimeshutdown process. Previously, only the Worker cache was removed, which could leave residual cache data on the nodes.Specific changes:
cleanupCacheinpkg/ddc/juicefs/shutdown.goto iterate through Fuse pods and clean their cache directories after cleaning the Worker cache.getFuseCacheDirsto correctly identify Fuse-specific cache paths, respecting bothTieredStoreconfigurations and Fuse-specific option overrides (e.g.,cache-dirinfuse.options).Ⅱ. Does this pull request fix one issue?
fixes #5230
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
pkg/ddc/juicefs/shutdown_test.go:getFuseCacheDirsto verify cache path resolution with various configurations.TestJuiceFSEngine_cleanupCacheto mock Fuse pods and verify that Fuse cache cleanup is triggered.Ⅳ. Describe how to verify it
Run the unit tests for the JuiceFS package:
go test -v ./pkg/ddc/juicefs