fix: add files to image issue #596
Conversation
WalkthroughThe pull request introduces comprehensive changes across multiple packages, focusing on improving file management, volume handling, and observability configurations. Key modifications include enhancing storage and build processes in the instance management system, updating ConfigMap and pod configurations, and refactoring port settings for observability sidecars. The changes aim to provide more robust error handling, flexible file and volume management, and improved initialization image support. Changes
Assessment against linked issues
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
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: 4
🔭 Outside diff range comments (1)
pkg/instance/storage.go (1)
Line range hint
385-425: Handle potential nil map assignments indeployFilesIn
deployFiles, the mapdirs[filepath.Dir(file.Dest)]may result in a nil map if not properly initialized. Ensure that the map is initialized before assigning values to preventnilmap assignment errors.Apply this diff to initialize the map correctly:
for _, file := range s.files { // read out file content and assign to variable srcFile, err := os.Open(file.Source) if err != nil { return ErrFailedToOpenFile.Wrap(err) } defer srcFile.Close() fileContentBytes, err := io.ReadAll(srcFile) if err != nil { return ErrFailedToReadFile.Wrap(err) } var ( fileContent = string(fileContentBytes) keyName = filepath.Base(file.Dest) ) + dir := filepath.Dir(file.Dest) + if _, ok := dirs[dir]; !ok { + dirs[dir] = make(map[string]string) + } - if _, ok := dirs[filepath.Dir(file.Dest)]; !ok { - dirs[filepath.Dir(file.Dest)] = make(map[string]string) - } - dirs[filepath.Dir(file.Dest)][keyName] = fileContent + dirs[dir][keyName] = fileContent }
🧹 Nitpick comments (9)
pkg/instance/build.go (1)
318-319: Fix potential issue with build directory pathIn
getBuildDir, the current implementation appendsb.instance.nametotmpDir, which may not be necessary sincetmpDiris already unique due toos.MkdirTemp. This could lead to unnecessarily long file paths or redundant directories. Consider usingtmpDirdirectly as the build directory.Apply this diff to simplify the build directory path:
func (b *build) getBuildDir() (string, error) { if b.buildDir != "" { return b.buildDir, nil } tmpDir, err := os.MkdirTemp("", "knuu-build-*") if err != nil { return "", err } - b.buildDir = filepath.Join(tmpDir, b.instance.name) + b.buildDir = tmpDir return b.buildDir, nil }pkg/k8s/pod.go (2)
403-411: Avoid mounting files to critical directories directlyIn
buildContainerVolumes, mounting files directly to critical directories like/etc,/bin, etc., may cause permission issues or read-only file system errors. Consider alternative approaches, such as mounting to non-critical directories or adjusting the application to read from alternative paths.
Line range hint
486-505: Simplify init container command constructionThe command construction in
buildInitContainerCommandis complex and may be prone to errors with multiple string concatenations. Consider using a bytes buffer or an array of commands to build the command string more safely and readably.pkg/names/names.go (1)
20-23: Consider using a stronger hash function than SHA-1The function
HashWithLengthuses SHA-1 to generate a hash. SHA-1 is considered insecure due to known vulnerabilities. Even if used for non-cryptographic purposes, it's recommended to use a more secure hash function like SHA-256 to prevent potential collisions.Apply this diff to use SHA-256 instead:
import ( - "crypto/sha1" + "crypto/sha256" "encoding/hex" "fmt" "github.com/google/uuid" ) func HashWithLength(input string, length int) string { - hash := sha1.Sum([]byte(input)) + hash := sha256.Sum256([]byte(input)) return hex.EncodeToString(hash[:])[:length] }pkg/k8s/volume.go (1)
17-22: Consider adding field validation for File structThe File struct contains critical fields that could benefit from validation:
- Source/Dest paths should be non-empty and well-formed
- Chown should follow the format "uid:gid"
- Permission should be a valid Unix permission string
Consider adding a validation function:
func validateFile(f *File) error { if f.Source == "" || f.Dest == "" { return fmt.Errorf("source and destination paths cannot be empty") } if f.Chown != "" { if !regexp.MustCompile(`^\d+:\d+$`).MatchString(f.Chown) { return fmt.Errorf("invalid chown format, expected uid:gid") } } if f.Permission != "" { if !regexp.MustCompile(`^[0-7]{3,4}$`).MatchString(f.Permission) { return fmt.Errorf("invalid permission format") } } return nil }pkg/k8s/configmap.go (2)
8-8: Fix import orderingThe import statement needs to be reordered according to goimports standards with the -local flag.
-"github.com/celestiaorg/knuu/pkg/names" +"github.com/celestiaorg/knuu/pkg/names"🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: File is not
goimports-ed with -local github.com/celestiaorg(goimports)
135-142: Add error handling for empty inputsThe PrepareConfigMapName function should validate its inputs.
Consider adding input validation:
func PrepareConfigMapName(instanceName, dirPath string) string { + if instanceName == "" || dirPath == "" { + return "" + } maxInstanceNameLength := validation.DNS1123SubdomainMaxLength - configMapNameHashSuffixLength - 1 if len(instanceName) > maxInstanceNameLength { instanceName = instanceName[:maxInstanceNameLength] } hash := names.HashWithLength(dirPath, configMapNameHashSuffixLength) return fmt.Sprintf("%s-%s", instanceName, hash) }pkg/k8s/errors.go (1)
150-150: LGTM! The error definition follows the established pattern.The error variable is well-defined and provides clear context for path validation scenarios.
Consider adding a period at the end of the error message for consistency with other error messages in the file:
- ErrDestNotSubpath = errors.New("DestNotSubpath", "destination %s is not a subpath of volume path %s") + ErrDestNotSubpath = errors.New("DestNotSubpath", "destination %s is not a subpath of volume path %s.")pkg/instance/errors.go (1)
235-235: Consider simplifying the error variable name.The error definition is good, but the variable name could be more concise.
- ErrFileAlreadyExistsInTheVolumePath = errors.New("FileAlreadyExistsInTheVolumePath", "file %s already exists in the volume path %s") + ErrFileExistsInVolumePath = errors.New("FileExistsInVolumePath", "file %s already exists in volume path %s.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e/basic/observability_test.go(5 hunks)e2e/suite.go(2 hunks)e2e/system/build_image_test.go(1 hunks)pkg/instance/build.go(4 hunks)pkg/instance/errors.go(1 hunks)pkg/instance/execution.go(1 hunks)pkg/instance/storage.go(5 hunks)pkg/k8s/configmap.go(2 hunks)pkg/k8s/errors.go(1 hunks)pkg/k8s/pod.go(10 hunks)pkg/k8s/volume.go(1 hunks)pkg/names/names.go(2 hunks)pkg/sidecars/observability/obsy.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/instance/execution.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/k8s/configmap.go
8-8: File is not goimports-ed with -local github.com/celestiaorg
(goimports)
🔇 Additional comments (12)
e2e/suite.go (3)
34-35: LGTM: Thread-safe cleanup control flag added
The addition of skipCleanup using atomic.Bool ensures thread-safe access to the cleanup control flag.
62-65: LGTM: Clear warning message for skipped cleanup
The warning message clearly indicates that skipping cleanup is for debugging only and reminds developers to remove the SkipCleanup() call before merging.
72-74: LGTM: Simple and effective cleanup control method
The SkipCleanup method provides a clean interface to control cleanup behavior.
pkg/sidecars/observability/obsy.go (2)
105-107: LGTM: Port configuration updated in Initialize method
The port configuration is correctly updated to use the new DefaultTelemetryPort.
15-16: LGTM: Port configuration aligned with standards
DefaultOtelOtlpPortis now set to 4318, which is the standard OTLP HTTP port- New
DefaultTelemetryPortconstant provides clear separation of concerns
Let's verify if these ports are used consistently across the codebase:
✅ Verification successful
Let me verify the usage of the ports in the codebase by checking the implementation details of the referenced files.
Based on the code analysis, I can now generate the final response:
Port configuration changes are correctly implemented and consistent
The port changes are properly implemented across the codebase:
DefaultOtelOtlpPort(4318) is used in e2e tests and configurationDefaultTelemetryPort(8888) is correctly referenced ininternalTelemetryEndpointconstant- Both ports are properly utilized in the telemetry configuration setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any hardcoded port numbers that might need updating
rg -n "4318|8888" --type go
Length of output: 453
Script:
#!/bin/bash
# Check the implementation details around these ports
cat pkg/sidecars/observability/otel.go pkg/sidecars/observability/obsy.go
Length of output: 17149
e2e/basic/observability_test.go (5)
21-23: LGTM: Constants aligned with observability package
The new telemetryPort constant correctly references observability.DefaultTelemetryPort.
25-26: LGTM: Improved retry configuration
The increased retry intervals (3s) and timeout (5m) provide better stability for the tests.
62-62: LGTM: Updated port configurations and OTLP exporter URL
- Prometheus target now uses the correct telemetry port
- OTLP exporter URL properly includes the HTTP scheme
Also applies to: 72-72, 74-74
108-112: LGTM: Enhanced error logging
Added response body content to error logs for better debugging capabilities.
132-132: LGTM: Dynamic port configuration in test command
The test command correctly uses the otlpPort constant for consistency.
e2e/system/build_image_test.go (1)
80-80: LGTM: Improved ownership specification
The change from "root:root" to "0:0" is more precise as it uses numeric UIDs directly.
pkg/instance/errors.go (1)
236-236: LGTM! The error definition follows the established pattern.
The error message is consistent with other state-related errors in the file.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/instance/build.go (1)
18-20: Consider pinning the Alpine version for better reproducibility.Using the
latesttag can lead to unexpected behavior when the base image is updated. Consider pinning to a specific Alpine version (e.g.,alpine:3.19).- baseInitImageName = "alpine:latest" + baseInitImageName = "alpine:3.19"pkg/k8s/pod.go (2)
Line range hint
343-425: LGTM! Robust volume handling with security considerations.The implementation correctly:
- Handles both volumes and files
- Checks for critical directories
- Uses proper path manipulation
Consider pre-allocating the slice to improve performance slightly:
- var podVolumes []v1.Volume + podVolumes := make([]v1.Volume, 0, len(volumes)+len(uniqueDirs))
642-667: LGTM! Consider using a slice for critical directories.The implementation is clean and comprehensive. Consider using a slice and strings.HasPrefix for more flexible path matching:
func isCriticalDir(dir string) bool { - criticalDirs := map[string]bool{ - "/etc": true, - "/bin": true, - "/sbin": true, - "/lib": true, - "/lib64": true, - "/dev": true, - "/proc": true, - "/sys": true, - "/run": true, - "/boot": true, - "/usr": true, - "/var": true, - "/root": true, - } - return criticalDirs[dir] + criticalDirs := []string{ + "/etc/", "/bin/", "/sbin/", "/lib", + "/dev/", "/proc/", "/sys/", "/run/", + "/boot/", "/usr/", "/var/", "/root/", + } + for _, prefix := range criticalDirs { + if strings.HasPrefix(dir, prefix) { + return true + } + } + return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/instance/build.go(4 hunks)pkg/k8s/pod.go(10 hunks)
🧰 Additional context used
📓 Learnings (1)
pkg/instance/build.go (1)
Learnt from: mojtaba-esk
PR: celestiaorg/knuu#596
File: pkg/instance/build.go:168-216
Timestamp: 2024-12-18T11:52:38.566Z
Learning: When no files are added to a volume during the 'Preparing' state, the init image can remain unset as there is nothing to populate in the init container. This is intended behavior to avoid creating an unnecessary init container.
🔇 Additional comments (6)
pkg/instance/build.go (4)
25-25: LGTM!
The new field is appropriately placed and correctly typed.
168-216: LGTM! Implementation is clean and follows the builder pattern.
The function correctly:
- Validates instance state
- Handles volumes and files
- Only builds when necessary (as per learnings)
234-238: LGTM! Proper integration of init image building.
The changes correctly integrate the init image building process into the commit flow.
208-208: Address the TODO comment about custom registry.
The comment references PR #593 for custom registry updates.
pkg/k8s/pod.go (2)
38-38: LGTM!
The InitImageName field is appropriately placed and correctly typed.
Line range hint 577-592: LGTM! Clear and concise initialization logic.
The function correctly checks all necessary conditions before creating init containers.
| // TODO: this code works only for one volume, need to fix it | ||
| for _, volume := range volumes { | ||
| knuuVolumePath := fmt.Sprintf("%s%s", knuuPath, volume.Path) | ||
| knuuVolumePath := knuuPath // volume is mounted to the same path so no need to join the path here |
There was a problem hiding this comment.
Address TODO: Multiple volume support limitation.
The current implementation only works with one volume. This limitation should be addressed.
Would you like me to help implement support for multiple volumes?
Closes #595
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation