feat(storage): retain file permission and owner when adding via confi… #589
Conversation
…gMap Signed-off-by: Smuu <[email protected]>
WalkthroughThe changes in this pull request involve modifications across multiple files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
pkg/k8s/pod.go (2)
442-460: Remove Unused Commented-Out CodeThe block of commented-out code from lines 442-460 increases clutter and may cause confusion. If this code is no longer needed, consider removing it to improve code readability.
538-543: Handle Errors in Volume Copy OperationsThe shell commands for copying volumes may fail under certain conditions (e.g., empty directories, permission issues). Consider adding error handling or logging to aid in debugging if the copy operations do not execute as expected.
pkg/instance/build.go (1)
6-6: Remove Unused Import or Document Its PurposeThe newly added import of
"os"should be used within the code. If it's necessary for the modifications, ensure its usage is clear; otherwise, consider removing it to keep imports clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
pkg/instance/build.go(2 hunks)pkg/instance/errors.go(1 hunks)pkg/instance/execution.go(0 hunks)pkg/instance/resources.go(2 hunks)pkg/instance/storage.go(3 hunks)pkg/k8s/pod.go(8 hunks)pkg/k8s/types.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/instance/execution.go
🔇 Additional comments (12)
pkg/k8s/pod.go (6)
385-394: Verify Volume Creation When Volumes Are Absent but Files Exist
The added logic in buildPodVolumes introduces an EmptyDir volume when there are files but no volumes. Verify that this behavior aligns with the intended functionality and that it doesn't introduce unexpected side effects.
403-403: Change Default File Mode to 0600 Enhances Security
Changing the DefaultMode to 0600 strengthens security by limiting file permissions. Confirm that all components depending on these files can operate correctly with the restricted permissions.
428-437: Ensure Correct Volume Mounts for Files Without Volumes
The code in buildContainerVolumes handles scenarios where files are present without volumes by creating unique mounts. Verify that all necessary directories are correctly mounted and accessible within the container.
470-488: Validate Volume Mount Logic in buildInitContainerVolumes
The added logic ensures that when no volumes exist but files do, the necessary volume mounts are created in the init container. Verify that the MountPath and SubPath are correctly computed, especially when dealing with nested directories.
549-551: Ensure Command String Is Properly Terminated
Trimming the trailing && from the command string ensures that the shell does not encounter syntax errors. Confirm that this logic correctly handles all cases, especially when no commands are added.
590-590: Re-evaluate Condition for Initializing Containers
The change in the conditional statement in prepareInitContainers might skip initialization when both volumes and files are absent. Ensure that this behavior is intentional and aligns with the application's requirements.
pkg/instance/resources.go (2)
92-95: Check for Errors When Deploying Files
The updated deployStorage method now checks if there are files before attempting deployment. Ensure that this condition correctly handles cases where deployFiles might return errors, and that errors are properly propagated and logged.
125-128: Consistent Error Handling in Resource Destruction
The destroyResources method now includes a conditional check before destroying files. Verify that this change aligns with the intended cleanup logic and that all resources are thoroughly cleaned up when an instance is destroyed.
pkg/instance/storage.go (3)
Line range hint 43-53: Update Variable Naming for Clarity in AddFile Method
The variable buildDirPath replaces dstPath, enhancing clarity. Ensure that all references to dstPath are correctly updated throughout the method to prevent any confusion or errors.
257-276: Error Handling and Permission Enforcement in copyFileToBuildDir
The updated copyFileToBuildDir method now preserves source file permissions. Ensure that this behavior is intended and does not introduce security concerns. Additionally, verify that errors are adequately handled, and consider logging any critical errors to aid in debugging.
287-288: Ensure Correct Permission Format
The permission is formatted using fmt.Sprintf("%o", srcInfo.Mode().Perm()). Verify that this produces the correct permission string expected by downstream components, especially if leading zeros are necessary.
pkg/instance/errors.go (1)
107-107: LGTM!
The error variable ErrFailedToSetPermissions follows the established pattern and provides a clear error message that aligns with the PR's objective of handling file permission operations.
Signed-off-by: Smuu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
pkg/instance/storage.go (2)
275-278: Remove redundant Chmod callThe
os.Chmodcall is redundant as the file permissions are already set byos.OpenFileon line 265 usingsrcInfo.Mode().Perm().- // Ensure the destination file has the same permissions as the source file - if err := os.Chmod(dstPath, srcInfo.Mode().Perm()); err != nil { - return "", ErrFailedToSetPermissions.WithParams(dstPath).Wrap(err) - }
293-299: Enhance chown validation with regexThe current validation only checks for the presence and count of parts split by ":". Consider using a regex pattern to ensure the format matches exactly two numbers.
+ const chownPattern = `^\d+:\d+$` parts := strings.Split(chown, ":") - if len(parts) != 2 { + if !regexp.MustCompile(chownPattern).MatchString(chown) { return ErrInvalidFormat.WithParams(chown) } - for _, part := range parts { - if _, err := strconv.ParseInt(part, 10, 64); err != nil { - return ErrFailedToConvertToInt64.WithParams(part).Wrap(err) - } - }pkg/k8s/pod.go (3)
72-75: Add documentation for File struct fieldsPlease add documentation for the new fields to explain their purpose and expected format.
type File struct { - Source string - Dest string - Chown string - Permission string + // Source is the path to the source file on the host + Source string + // Dest is the path where the file should be placed in the container + Dest string + // Chown specifies the user:group ownership in format "uid:gid" + Chown string + // Permission specifies the file mode in octal format (e.g., "644") + Permission string }
445-461: Remove commented out codeRemove the commented out code block as it adds noise to the codebase. If this code is important for future reference, consider adding it to the documentation or commit message instead.
- // for n, file := range files { - // shouldAddFile := true - // for _, volume := range volumes { - // if strings.HasPrefix(file.Dest, volume.Path) { - // shouldAddFile = false - // break - // } - // } - // if shouldAddFile { - // containerFiles = append(containerFiles, v1.VolumeMount{ - // Name: name + podFilesConfigmapNameSuffix, - // MountPath: file.Dest, - // SubPath: fmt.Sprintf("%d", n), - // }) - // } - // }
Line range hint
429-489: Simplify volume mount logicThe volume mount logic in both
buildContainerVolumesandbuildInitContainerVolumesis similar and could be refactored into a shared helper function to reduce code duplication.+func buildVolumeMounts(name string, volumes []*Volume, files []*File, isInit bool) []v1.VolumeMount { + var mounts []v1.VolumeMount + + // Handle volumes + if len(volumes) > 0 { + if isInit { + mounts = append(mounts, v1.VolumeMount{ + Name: name, + MountPath: knuuPath, + }) + } else { + for _, volume := range volumes { + mounts = append(mounts, v1.VolumeMount{ + Name: name, + MountPath: volume.Path, + SubPath: strings.TrimLeft(volume.Path, "/"), + }) + } + } + } + + // Handle files when no volumes + if len(volumes) == 0 && len(files) > 0 { + uniquePaths := make(map[string]bool) + for _, file := range files { + uniquePaths[filepath.Dir(file.Dest)] = true + } + + for path := range uniquePaths { + mount := v1.VolumeMount{ + Name: name, + } + if isInit { + mount.MountPath = knuuPath + path + } else { + mount.MountPath = path + } + mount.SubPath = strings.TrimPrefix(path, "/") + mounts = append(mounts, mount) + } + } + + return mounts +}Then use it in both functions:
func buildContainerVolumes(name string, volumes []*Volume, files []*File) []v1.VolumeMount { - // Current implementation + return buildVolumeMounts(name, volumes, files, false) } func buildInitContainerVolumes(name string, volumes []*Volume, files []*File) []v1.VolumeMount { - // Current implementation + return buildVolumeMounts(name, volumes, files, true) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/instance/storage.go(4 hunks)pkg/k8s/pod.go(10 hunks)pkg/sidecars/observability/obsy.go(1 hunks)
🔇 Additional comments (2)
pkg/sidecars/observability/obsy.go (1)
23-23: Verify user ID 10001 exists in the container image
The ownership has been changed from root (0:0) to user:group 10001:10001. While using a non-root user is a security best practice, we need to ensure this user exists in the container image.
pkg/k8s/pod.go (1)
526-535:
Secure command construction against injection
The current implementation directly interpolates user-provided values into shell commands, which could lead to command injection. Consider validating the inputs and using proper shell escaping.
+ // Helper function to validate and escape shell arguments
+ shellEscape := func(s string) string {
+ return strings.ReplaceAll(s, "'", "'\"'\"'")
+ }
+
+ // Validate chown format
+ if !regexp.MustCompile(`^\d+:\d+$`).MatchString(chown) {
+ return nil, fmt.Errorf("invalid chown format: %s", chown)
+ }
+
+ // Validate permission format
+ if !regexp.MustCompile(`^[0-7]{3,4}$`).MatchString(permission) {
+ return nil, fmt.Errorf("invalid permission format: %s", permission)
+ }
+
if chown != "" {
- addFileToKnuu += fmt.Sprintf("chown %s %s && ", chown, filepath.Join(knuuPath, file.Dest))
+ addFileToKnuu += fmt.Sprintf("chown '%s' '%s' && ",
+ shellEscape(chown),
+ shellEscape(filepath.Join(knuuPath, file.Dest)))
}
if permission != "" {
- addFileToKnuu += fmt.Sprintf("chmod %s %s && ", permission, filepath.Join(knuuPath, file.Dest))
+ addFileToKnuu += fmt.Sprintf("chmod '%s' '%s' && ",
+ shellEscape(permission),
+ shellEscape(filepath.Join(knuuPath, file.Dest)))
}Likely invalid or redundant comment.
Signed-off-by: Smuu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
pkg/instance/storage.go (3)
291-294: Consider removing redundant chmod operation.The destination file already inherits the source file's permissions through
OpenFile(dstPath, os.O_CREATE|os.O_WRONLY, srcInfo.Mode().Perm()). The additionalos.Chmodcall appears redundant.- // Ensure the destination file has the same permissions as the source file - if err := os.Chmod(dstPath, srcInfo.Mode().Perm()); err != nil { - return "", ErrFailedToSetPermissions.WithParams(dstPath).Wrap(err) - }
309-316: Consider using regex for chown validation.The current string split validation could be replaced with a more precise regex pattern to validate the user:group format.
- parts := strings.Split(chown, ":") - if len(parts) != 2 { - return ErrInvalidFormat.WithParams(chown) - } - for _, part := range parts { - if _, err := strconv.ParseInt(part, 10, 64); err != nil { - return ErrFailedToConvertToInt64.WithParams(part).Wrap(err) - } - } + if matched, err := regexp.MatchString(`^\d+:\d+$`, chown); err != nil || !matched { + return ErrInvalidFormat.WithParams(chown) + }
145-147: Extract hardcoded file permission as a constant.The permission value
0644should be defined as a named constant at the package level for better maintainability and reusability.+const ( + // DefaultTempFilePermissions represents the default permissions for temporary files + DefaultTempFilePermissions = 0644 +) - if err := tmpfile.Chmod(0644); err != nil { + if err := tmpfile.Chmod(DefaultTempFilePermissions); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/instance/build.go(4 hunks)pkg/instance/errors.go(2 hunks)pkg/instance/storage.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/instance/errors.go
🔇 Additional comments (2)
pkg/instance/build.go (1)
237-242: LGTM! Improved error handling for temporary directory creation.
The updated implementation properly handles errors and uses a descriptive prefix for temporary directories. The function is now more robust and follows best practices.
pkg/instance/storage.go (1)
276-279: LGTM! Robust error handling for file operations.
The error handling is thorough and follows best practices with custom error types, proper error wrapping, and descriptive messages.
Also applies to: 302-302
Signed-off-by: Smuu <[email protected]>
Signed-off-by: Smuu <[email protected]>
Signed-off-by: Smuu <[email protected]>
Signed-off-by: Smuu <[email protected]>
Signed-off-by: Smuu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pkg/instance/storage.go (1)
285-303: LGTM! Consider adding a comment explaining the double chmod.The implementation correctly preserves source file permissions. While there are two chmod operations, this was confirmed as necessary during development.
Add a comment explaining why both chmod operations are needed:
srcInfo, err := srcFile.Stat() if err != nil { return "", ErrFailedToGetSrcFileInfo.WithParams(src).Wrap(err) } + // First chmod during file creation dst, err := os.OpenFile(dstPath, os.O_CREATE|os.O_WRONLY, srcInfo.Mode().Perm()) if err != nil { return "", ErrFailedToCreateDestFile.WithParams(dstPath).Wrap(err) } defer dst.Close() if _, err := io.Copy(dst, srcFile); err != nil { return "", ErrFailedToCopyFile.WithParams(src, dstPath).Wrap(err) } + // Second chmod to ensure correct permissions are set if err := os.Chmod(dstPath, srcInfo.Mode().Perm()); err != nil { return "", ErrFailedToSetPermissions.WithParams(dstPath).Wrap(err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
pkg/instance/build.go(5 hunks)pkg/instance/errors.go(2 hunks)pkg/instance/storage.go(7 hunks)pkg/k8s/pod.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/instance/errors.go
- pkg/instance/build.go
- pkg/k8s/pod.go
🧰 Additional context used
📓 Learnings (1)
pkg/instance/storage.go (1)
Learnt from: smuu
PR: celestiaorg/knuu#589
File: pkg/instance/storage.go:299-317
Timestamp: 2024-12-06T09:02:56.486Z
Learning: In `pkg/instance/storage.go`, the `chown` argument is validated in the `validateFileArgs` function before calling `addFileToInstance`, so further validation within `addFileToInstance` is not required.
🔇 Additional comments (2)
pkg/instance/storage.go (2)
25-25: LGTM! Good practice using a named constant for file permissions.
The defaultFilePermission constant with value 0644 is appropriate for regular files, providing read/write access to the owner and read-only access to others.
308-317: LGTM! Good implementation of permission preservation.
The function correctly:
- Checks source file existence
- Retrieves source file permissions
- Formats permissions in octal format
- Creates the file with proper metadata
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
pkg/instance/errors.go(2 hunks)pkg/instance/execution.go(0 hunks)pkg/instance/storage.go(8 hunks)pkg/k8s/pod.go(10 hunks)
💤 Files with no reviewable changes (1)
- pkg/instance/execution.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/instance/errors.go
🧰 Additional context used
📓 Learnings (2)
pkg/k8s/pod.go (2)
Learnt from: smuu
PR: celestiaorg/knuu#589
File: pkg/k8s/pod.go:104-109
Timestamp: 2024-12-06T08:56:03.468Z
Learning: The `NewFile` method in `pkg/k8s/pod.go` is used internally and doesn't require external documentation updates when its signature changes.
Learnt from: smuu
PR: celestiaorg/knuu#589
File: pkg/k8s/pod.go:72-75
Timestamp: 2024-12-06T08:55:14.704Z
Learning: In `pkg/k8s/pod.go`, the `Chown` and `Permission` fields in the `File` struct are validated when adding a file, ensuring proper sanitization against security risks like command injection or malformed commands.
pkg/instance/storage.go (1)
Learnt from: smuu
PR: celestiaorg/knuu#589
File: pkg/instance/storage.go:299-317
Timestamp: 2024-12-06T09:02:56.486Z
Learning: In `pkg/instance/storage.go`, the `chown` argument is validated in the `validateFileArgs` function before calling `addFileToInstance`, so further validation within `addFileToInstance` is not required.
🪛 GitHub Check: test (./e2e/netshaper, 60m)
pkg/instance/storage.go
[failure] 349-349:
no new variables on left side of :=
🪛 GitHub Check: test (./e2e/system, 15m)
pkg/instance/storage.go
[failure] 349-349:
no new variables on left side of :=
🪛 GitHub Check: test (./e2e/sidecars, 5m)
pkg/instance/storage.go
[failure] 349-349:
no new variables on left side of :=
🪛 GitHub Check: test (./e2e/basic, 15m)
pkg/instance/storage.go
[failure] 349-349:
no new variables on left side of :=
🪛 GitHub Check: Run govulncheck
pkg/instance/storage.go
[failure] 349-349:
no new variables on left side of :=
🪛 GitHub Check: test (./pkg/..., 10m)
pkg/instance/storage.go
[failure] 349-349:
no new variables on left side of :=
🪛 GitHub Check: golangci-lint
pkg/instance/storage.go
[failure] 349-349:
no new variables on left side of :=) (typecheck)
🔇 Additional comments (14)
pkg/instance/storage.go (6)
27-28: LGTM! Good choice of default file permissions.
The constant defaultFilePermission = 0644 provides a secure default that follows the principle of least privilege.
Line range hint 47-64: LGTM! Improved error handling and logging.
The changes enhance the robustness of file handling by:
- Properly handling build directory paths
- Adding appropriate error handling
- Including helpful debug logging
Also applies to: 67-75
109-113: LGTM! Consistent handling of build directory.
The changes maintain consistency with the AddFile method and include proper error handling and logging.
Also applies to: 127-135
156-158: LGTM! Proper file permission handling.
The changes ensure consistent file permissions by using the secure defaultFilePermission constant.
317-325: LGTM! Improved file metadata handling.
The changes properly capture and preserve the source file's permissions and ownership.
269-274: 🛠️ Refactor suggestion
Remove redundant chown validation.
Based on the provided learning, the chown format is already validated before calling addFileToInstance. This additional validation is redundant and can be removed.
- parts := strings.Split(chown, ":")
- for _, part := range parts {
- if _, err := strconv.ParseInt(part, 10, 64); err != nil {
- return ErrFailedToConvertToInt64.WithParams(part).Wrap(err)
- }
- }pkg/k8s/pod.go (8)
32-32: LGTM! Good choice of init container image.
Using nicolaka/netshoot as a fixed image for init containers is appropriate as it contains the necessary tools for file operations, which may not be available in application containers.
74-77: LGTM! Well-structured file metadata fields.
The addition of Chown and Permission fields to the File struct properly captures file ownership and permissions. Based on the learnings, these fields are validated when adding a file, ensuring proper sanitization.
106-111: LGTM! Method signature aligns with struct changes.
The NewFile method has been properly updated to handle the new Chown and Permission fields.
388-396: LGTM! Proper volume handling for files.
The changes correctly handle scenarios where only files are present by:
- Creating an EmptyDir volume when needed
- Setting up appropriate volume mounts
Also applies to: 431-440
456-476: LGTM! Clear separation of volume handling logic.
The changes properly handle volume mounting in init containers, with clear separation between volume and file handling. The comments explain the purpose of each condition, improving code readability.
513-522: LGTM! Secure file metadata handling.
The changes properly handle file ownership and permissions in the init container. The inputs are validated when adding a file, ensuring protection against command injection.
526-531: LGTM! Improved volume command handling.
The changes enhance the volume command construction with proper ownership handling and path management.
630-636: LGTM! Proper sidecar initialization.
The changes correctly handle sidecar initialization by:
- Creating init containers for sidecars
- Appending sidecar containers and volumes
- Supporting multiple init containers
Signed-off-by: Smuu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/instance/storage.go (1)
299-312: Document the necessity of double permission setting.The code sets permissions twice: once during file creation and once after copying. While this might be necessary based on development experience, it would be helpful to document why both operations are required.
Add a comment explaining why both permission settings are necessary:
dst, err := os.OpenFile(dstPath, os.O_CREATE|os.O_WRONLY, srcInfo.Mode().Perm()) if err != nil { return "", ErrFailedToCreateDestFile.WithParams(dstPath).Wrap(err) } defer dst.Close() if _, err := io.Copy(dst, srcFile); err != nil { return "", ErrFailedToCopyFile.WithParams(src, dstPath).Wrap(err) } + // Second chmod required to ensure permissions are set correctly + // as initial permission setting may not persist through all filesystems if err := os.Chmod(dstPath, srcInfo.Mode().Perm()); err != nil { return "", ErrFailedToSetPermissions.WithParams(dstPath).Wrap(err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/instance/storage.go(8 hunks)
🧰 Additional context used
📓 Learnings (1)
pkg/instance/storage.go (1)
Learnt from: smuu
PR: celestiaorg/knuu#589
File: pkg/instance/storage.go:299-317
Timestamp: 2024-12-06T09:02:56.486Z
Learning: In `pkg/instance/storage.go`, the `chown` argument is validated in the `validateFileArgs` function before calling `addFileToInstance`, so further validation within `addFileToInstance` is not required.
🔇 Additional comments (4)
pkg/instance/storage.go (4)
27-28: LGTM! Good practice defining file permissions as a constant.
The standard Unix permission 0644 (rw-r--r--) is appropriately defined as a constant.
269-274: Remove redundant chown validation.
This validation is redundant as the chown format is already validated in validateFileArgs before calling addFileToInstance.
343-343: Fix variable declaration syntax.
The := operator is used incorrectly here as the variable file was already declared.
Line range hint 47-77: Consider potential race conditions in file operations.
The file operations between getting the build directory and using it are not atomic. In a concurrent environment, the build directory could be modified between these operations.
Consider implementing a file operation lock mechanism or using atomic operations to prevent potential race conditions.
…gMap
Overview
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores