Feat: supporrt custom registry #593
Conversation
WalkthroughThis pull request introduces a comprehensive set of changes across multiple packages, focusing on enhancing the builder and registry functionality in the Knuu project. The modifications include adding new registry implementations (Default, DockerHub, Scaleway), updating the builder interfaces, improving error handling, and introducing more flexible image building and management capabilities. The changes streamline the image building process, provide more robust registry interactions, and enhance the overall system's flexibility for handling different registry types and build scenarios. Changes
Sequence DiagramsequenceDiagram
participant User
participant Knuu
participant ImageBuilder
participant Registry
participant K8s
User->>Knuu: Initialize with options
Knuu->>ImageBuilder: Create default builder
ImageBuilder->>Registry: Select registry type
Registry-->>ImageBuilder: Return registry config
User->>Knuu: Build image from Git repo
Knuu->>ImageBuilder: Trigger build
ImageBuilder->>K8s: Create build job
K8s-->>ImageBuilder: Return build logs
ImageBuilder->>Registry: Push image
Registry-->>ImageBuilder: Confirm push
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)pkg/instance/execution.go (2)
The new k8s package import is properly placed and necessary for the registry support implementation. Line range hint The simplified clone implementation could lead to nil pointer dereferences in code that expects a valid instance. Consider documenting this behavior or preserving necessary fields. Let's check for potential issues: 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: 16
🧹 Outside diff range and nitpick comments (14)
pkg/cert/cert.go (2)
35-36: Make Subject Organization ConfigurableThe
Organizationfield in the certificate subject is hardcoded as"Dynamic Registry". To enhance flexibility and reusability of this function, consider making the organization configurable through a parameter.
55-56: Adjust PEM Encoding for PKCS#8 Private KeyIf switching to PKCS#8 encoding, update the PEM block type accordingly to
"PRIVATE KEY".Apply this diff to adjust the PEM block type:
- keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: privBytes}) + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: privBytes})pkg/builder/docker/docker.go (1)
Line range hint
45-78: ValidateImageNameBefore BuildingThe
Buildmethod relies onb.ImageNamefor tagging and pushing the Docker image. Ifb.ImageNameis empty, this could cause the build or push to fail. Consider adding validation to check ifb.ImageNameis set and return an informative error if not.Apply this diff to add validation:
func (d *Docker) Build(_ context.Context, b builder.BuilderOptions) (logs string, err error) { + if b.ImageName == "" { + return "", fmt.Errorf("ImageName is required") + }pkg/builder/registry/registry.go (2)
7-7: Enhance documentation for GenerateConfig method.The comment should provide more details about the expected JSON structure and potential error cases.
- GenerateConfig() ([]byte, error) // Generate a json config for the registry + GenerateConfig() ([]byte, error) // Generates registry authentication configuration in JSON format. Returns error if generation fails.
17-23: Optimize string concatenation in ToString method.Consider using strings.Builder for better performance, especially when dealing with multiple concatenations.
func (r *ResolvedImage) ToString() string { + var b strings.Builder + b.WriteString(r.Prefix) + b.WriteString("/") + b.WriteString(r.Name) if r.Tag == "" { - return fmt.Sprintf("%s/%s", r.Prefix, r.Name) + return b.String() } - return fmt.Sprintf("%s/%s:%s", r.Prefix, r.Name, r.Tag) + b.WriteString(":") + b.WriteString(r.Tag) + return b.String() }pkg/builder/builder.go (2)
23-23: Document image name format requirementsSince this field accepts custom image names from users, the comment should specify any format requirements or restrictions.
- ImageName string // Custom image name (if provided by the user) + ImageName string // Custom image name (if provided by the user). Must follow Docker image naming conventions.
40-40: Add validation for empty buildContextWhile there's an error check for empty buildContext, the function continues to call hashString which is unnecessary in this case.
func DefaultImageName(buildContext string) (string, error) { if buildContext == "" { return "", ErrBuildContextEmpty } - return hashString(buildContext) + hash, err := hashString(buildContext) + if err != nil { + return "", fmt.Errorf("failed to hash build context: %w", err) + } + return hash, nil }pkg/builder/registry/dockerhub.go (1)
8-11: Consider using HTTPS registry addressThe
dockerHubRegistryAddressshould use HTTPS format for consistency withdockerHubRegistryPushURL.- dockerHubRegistryAddress = "docker.io" + dockerHubRegistryAddress = "https://docker.io"e2e/registry/suite_setup_test.go (2)
20-23: Consider environment-based timeout configurationThe hardcoded 100-minute timeout might be excessive for local testing. Consider making it configurable via environment variables.
- testTimeout = time.Minute * 100 // the same time that is used in the ci/cd pipeline + defaultTimeout = time.Minute * 100 + testTimeout = func() time.Duration { + if val := os.Getenv("TEST_TIMEOUT"); val != "" { + if d, err := time.ParseDuration(val); err == nil { + return d + } + } + return defaultTimeout + }()
45-48: Remove or implement commented codeThe commented Scaleway registry code should either be implemented or removed to maintain code cleanliness.
e2e/registry/build_image_test.go (1)
13-15: Consider making Git repository details configurableThe Git repository and branch are hardcoded constants. Since this is a test dependency, consider:
- Making these configurable through environment variables
- Documenting why this specific branch is protected and what Dockerfile it contains
- Adding a comment about the branch protection requirements
const ( - gitRepo = "https://github.com/celestiaorg/knuu.git" - gitBranch = "test/build-from-git" // This branch has a Dockerfile and is protected as to not be deleted + defaultGitRepo = "https://github.com/celestiaorg/knuu.git" + defaultGitBranch = "test/build-from-git" ) + +// getGitTestConfig returns the Git repository configuration for testing. +// The test branch must contain a Dockerfile and should be protected to ensure test stability. +func getGitTestConfig() (repo, branch string) { + repo = os.Getenv("TEST_GIT_REPO") + if repo == "" { + repo = defaultGitRepo + } + branch = os.Getenv("TEST_GIT_BRANCH") + if branch == "" { + branch = defaultGitBranch + } + return +}pkg/builder/kaniko/kaniko_test.go (1)
69-73: Add test coverage for registry-specific errorsThe context cancellation test should be expanded to cover registry-specific error cases.
buildOptions := builder.BuilderOptions{ ImageName: testImage, BuildContext: "git://example.com/repo", + Registry: builder.RegistryOptions{ + URL: "invalid-registry-url", + }, }pkg/knuu/knuu.go (1)
174-183: Consider adding registry configuration to default builder setup.While the default image builder setup is clean, it might need to support registry configuration to align with the PR's objective of supporting custom registries.
Consider modifying the setup to include registry configuration:
func (k *Knuu) setupDefaultImageBuilder() error { if k.ImageBuilder != nil { return nil } + registry, err := registry.NewDefaultRegistry(k.SystemDependencies) + if err != nil { + return fmt.Errorf("failed to create default registry: %w", err) + } + k.ImageBuilder = &kaniko.Kaniko{ SystemDependencies: k.SystemDependencies, + Registry: registry, } return nil }pkg/instance/network.go (1)
19-19: Consider renaming the field for better clarityThe field name
notHeadlessuses a double negative which can be confusing. Consider renaming it touseClusterIPorenableClusterIPto better reflect its purpose.- notHeadless bool + useClusterIP bool
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
e2e/registry/build_image_test.go(1 hunks)e2e/registry/suite_setup_test.go(1 hunks)e2e/suite.go(1 hunks)go.mod(0 hunks)pkg/builder/builder.go(2 hunks)pkg/builder/docker/docker.go(3 hunks)pkg/builder/docker/errors.go(1 hunks)pkg/builder/kaniko/errors.go(1 hunks)pkg/builder/kaniko/kaniko.go(7 hunks)pkg/builder/kaniko/kaniko_test.go(2 hunks)pkg/builder/registry/default.go(1 hunks)pkg/builder/registry/dockerhub.go(1 hunks)pkg/builder/registry/errors.go(1 hunks)pkg/builder/registry/registry.go(1 hunks)pkg/builder/registry/scaleway.go(1 hunks)pkg/cert/cert.go(1 hunks)pkg/container/docker.go(2 hunks)pkg/instance/build.go(2 hunks)pkg/instance/execution.go(2 hunks)pkg/instance/network.go(4 hunks)pkg/k8s/pod.go(2 hunks)pkg/k8s/security.go(1 hunks)pkg/k8s/types.go(1 hunks)pkg/knuu/errors.go(1 hunks)pkg/knuu/knuu.go(2 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🔇 Additional comments (27)
pkg/cert/cert.go (1)
27-30: Ensure Unique Serial Numbers for Certificates
Currently, the serial number is generated using rand.Int, which might produce duplicate serial numbers over many invocations. For compliance with certificate standards and to prevent potential issues, consider ensuring that serial numbers are unique across all certificates issued by this function.
pkg/builder/docker/docker.go (3)
29-43: Verify Default Registry Initialization
When reg is nil, the New constructor assigns it using registry.NewDefault(). Ensure that the default registry is properly configured and accessible in the deployment environment to prevent runtime issues.
97-103: Ensure Cache Repository is Accessible
In the CacheOptions method, the cache repository URL is constructed using d.registry.ResolvedImage. Verify that this URL is valid and that the registry is properly configured to accept cache images.
105-111: Handle Errors in DefaultImage Method
The DefaultImage method correctly generates a default image name. Ensure that any errors returned by builder.DefaultImageName are properly handled and provide sufficient context to aid in debugging.
pkg/builder/kaniko/kaniko.go (6)
46-67: Properly Validate System Dependencies in Constructor
The New function thoroughly checks for nil values in sysDeps, K8sClient, and MinioClient, ensuring that all required dependencies are available. This prevents potential nil pointer dereferences at runtime.
110-125: Consistent Implementation of Cache Options
The CacheOptions and DefaultImage methods correctly implement caching configurations and default image naming. This ensures consistency across different builder implementations.
218-229: Set DOCKER_CONFIG for Registry Authentication
Setting the DOCKER_CONFIG environment variable allows Kaniko to find Docker configuration files for registry authentication. Ensure that the configuration directory (/kaniko/.docker) is properly populated with the necessary credentials.
245-248: Error Handling in Registry Configuration Setup
The call to setupRegistryConfig in prepareJob correctly handles errors. Confirm that any errors from setting up the registry configuration are properly logged and do not leave the system in an inconsistent state.
320-327: Include Build Context in Kaniko Arguments
In prepareArgs, the build context is set using --context=<context>. Ensure that when using a tar archive or remote context, the context argument is correctly formatted for Kaniko.
348-390:
Secure Handling of Registry Credentials
The setupRegistryConfig method writes registry credentials into the Kaniko build environment. Ensure that:
- Sensitive data is not logged or exposed.
- Access to the credentials is restricted to the necessary containers.
- The in-cluster secrets management complies with security best practices.
pkg/builder/registry/errors.go (1)
7-10: Define Clear and Specific Error Messages
The error variables ErrRegionNamespaceUsernamePasswordRequired and ErrUsernamePasswordRequired are clearly defined. Ensure that these errors are used appropriately to provide meaningful feedback when required parameters are missing.
pkg/builder/docker/errors.go (1)
17-17: LGTM: Error definition follows established patterns
The new error variable follows the existing error naming and message conventions, providing clear feedback about K8s client requirements.
pkg/builder/builder.go (1)
17-19: Consider performance implications of passing BuilderOptions by value
The Build method signature changed from pointer to value. For large BuilderOptions structs, this could impact performance due to copying.
Consider keeping the pointer receiver if BuilderOptions grows larger or contains reference types.
pkg/builder/registry/dockerhub.go (1)
53-59: LGTM: ResolvedImage implementation
The implementation correctly follows the Registry interface contract and properly constructs the image reference.
e2e/registry/suite_setup_test.go (1)
48-48:
Potential issue: Nil registry initialization
Setting registry to nil might cause issues if the code assumes a valid registry instance. Consider providing a default registry implementation.
Let's verify if nil registry is handled properly:
e2e/suite.go (1)
59-59:
Critical: Disabled cleanup may cause resource leaks
The early return prevents cleanup of Knuu resources, which could:
- Lead to resource leaks in the Kubernetes cluster
- Affect subsequent test runs
- Increase costs due to lingering resources
Consider implementing proper cleanup or documenting why cleanup is intentionally skipped.
Let's verify the impact:
e2e/registry/build_image_test.go (1)
61-68: Consider handling Git credentials securely
The current implementation passes empty credentials. For private repositories, credentials should be handled securely.
err = target.Build().SetGitRepo(ctx, builder.GitContext{
Repo: gitRepo,
Branch: gitBranch,
- Username: "",
- Password: "",
+ Username: os.Getenv("TEST_GIT_USERNAME"),
+ Password: os.Getenv("TEST_GIT_TOKEN"),
})pkg/builder/kaniko/errors.go (1)
34-37: LGTM! Well-structured error definitions
The new error types follow the existing pattern and provide clear, specific error messages for dependency and configuration issues.
pkg/k8s/types.go (1)
35-35: LGTM! Well-designed interface addition.
The new CreateTLSSecret method is a clean addition to the interface, following consistent patterns with other methods and supporting the custom registry feature.
pkg/container/docker.go (2)
115-119: LGTM! Clean BuilderOptions usage.
The simplified BuilderOptions construction improves code clarity while maintaining functionality.
141-146: Verify cache behavior across different registries.
While the addition of cache options is good, we should ensure it works consistently across different registry types.
✅ Verification successful
Cache implementation is consistent across builders and registries
Based on the code analysis, the cache behavior is well-implemented and consistent:
- Both Kaniko and Docker builders implement identical cache behavior:
- Both enable caching by default
- Both use the same cache repository naming scheme via
DefaultCacheRepoName - Both utilize the registry's
ResolvedImagemethod for cache repository resolution
- The registry abstraction (
pkg/builder/registry/registry.go) ensures consistent behavior across different registry types (default ttl.sh, Scaleway, DockerHub)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for registry-specific cache implementations
ast-grep --pattern 'func (r $_) CacheOptions() $_'
Length of output: 52
Script:
#!/bin/bash
# Let's search for CacheOptions implementations and usages with more context
rg "CacheOptions" -A 3
# Also check for cache-related configurations and registry interactions
rg "registry" -A 3 -g "*.go"
Length of output: 12796
pkg/knuu/knuu.go (1)
231-232: LGTM! Clean error handling.
The integration of setupDefaultImageBuilder in setDefaults is clean and maintains proper error handling.
pkg/instance/build.go (1)
89-94: LGTM: Improved image name resolution and error handling
The changes enhance the code by:
- Using the instance's ImageBuilder for resolving default image names
- Simplifying error handling
- Properly passing the image name to the git repo build process
Also applies to: 102-102, 114-114
pkg/instance/network.go (1)
149-151: LGTM: Consistent implementation of headless service support
The headless service configuration is properly implemented and consistently used in both service deployment and patching operations.
Also applies to: 170-170, 200-200
pkg/k8s/pod.go (1)
57-65: LGTM: PodConfig struct changes.
The addition of ImagePullSecrets field to support custom registries is well-structured and follows Kubernetes conventions.
pkg/knuu/errors.go (2)
213-213: LGTM: Error definition for local registry creation.
The new error variable follows the established pattern and provides a clear message for registry creation failures.
213-213: Verify error handling consistency.
Let's ensure the new error is used consistently and check for other registry-related error patterns.
✅ Verification successful
Let me gather more context about the error handling patterns in the registry-related code to make a more informed decision.
Let me check one more aspect - the actual usage of this error in the codebase to understand the context better.
Based on the comprehensive analysis of the error handling patterns in the codebase, I can now provide the final response:
Error handling is consistent with codebase patterns.
The ErrCannotCreateLocalRegistry error follows the established error handling patterns in the codebase:
- It uses the consistent error creation pattern via
errors.New() - The error code "CannotCreateLocalRegistry" follows the PascalCase naming convention used throughout
- The error message is concise and follows the same style as other registry-related errors
- It's properly grouped with other infrastructure-related errors in the errors.go file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling consistency for registry-related operations
# Check usage of the new error
echo "Checking usage of ErrCannotCreateLocalRegistry:"
rg "ErrCannotCreateLocalRegistry" --type go
# Check for other registry-related error patterns
echo -e "\nChecking other registry-related error patterns:"
rg "registry.*err(or)?" --type go
Length of output: 1100
Script:
#!/bin/bash
# Check implementation of registry-related functions for error handling patterns
echo "Checking registry-related function implementations:"
ast-grep --pattern 'func $$$registry$$$($$$) ($$$, error) {
$$$
}'
# Check error returns in registry-related code
echo -e "\nChecking error returns in registry context:"
rg "return.*err" -B 2 --type go | rg -B 2 "registry"
Length of output: 469
Script:
#!/bin/bash
# Search for local registry creation related code
echo "Checking local registry creation context:"
rg "create.*local.*registry" -B 2 -A 2 --type go
# Check the error handling in the specific file
echo -e "\nChecking error handling in the file containing ErrCannotCreateLocalRegistry:"
cat pkg/knuu/errors.go
Length of output: 28159
pkg/cert/cert.go
Outdated
| privBytes, err := x509.MarshalECPrivateKey(priv) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use PKCS#8 Encoding for Private Key
The function uses x509.MarshalECPrivateKey to encode the private key, which outputs in SEC 1 format. This format is considered deprecated in favor of PKCS#8. Using x509.MarshalPKCS8PrivateKey ensures better compatibility and adherence to current standards.
Apply this diff to update the private key encoding:
- privBytes, err := x509.MarshalECPrivateKey(priv)
+ privBytes, err := x509.MarshalPKCS8PrivateKey(priv)Also, handle the returned error appropriately since x509.MarshalPKCS8PrivateKey returns an interface{} and an error.
Committable suggestion skipped: line range outside the PR's diff.
| func (k *Kaniko) Build(ctx context.Context, b builder.BuilderOptions) (logs string, err error) { | ||
| if b.ImageName == "" { | ||
| ri, err := k.DefaultImage(b.BuildContext) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| b.ImageName = ri.ToString() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Modifying Passed-by-Value Structure
In the Build method, builder.BuilderOptions is passed by value. Assigning to b.ImageName does not affect the original structure outside the function scope. To ensure the ImageName is correctly used throughout the method, assign it to a local variable.
Apply this diff to use a local variable:
func (k *Kaniko) Build(ctx context.Context, b builder.BuilderOptions) (logs string, err error) {
+ var imageName string
if b.ImageName == "" {
ri, err := k.DefaultImage(b.BuildContext)
if err != nil {
return "", err
}
- b.ImageName = ri.ToString()
+ imageName = ri.ToString()
} else {
+ imageName = b.ImageName
}
job, err := k.prepareJob(ctx, &b, imageName)Also, update the prepareJob method to accept imageName as a parameter.
Committable suggestion skipped: line range outside the PR's diff.
| type ResolvedImage struct { | ||
| Prefix string // could be registry address + repository/namespace... | ||
| Name string | ||
| Tag string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required fields in ResolvedImage.
The struct should validate that essential fields (Prefix, Name) are not empty when used.
Consider adding a validation method:
+func (r *ResolvedImage) Validate() error {
+ if r.Prefix == "" {
+ return fmt.Errorf("prefix cannot be empty")
+ }
+ if r.Name == "" {
+ return fmt.Errorf("name cannot be empty")
+ }
+ return nil
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ResolvedImage struct { | |
| Prefix string // could be registry address + repository/namespace... | |
| Name string | |
| Tag string | |
| } | |
| type ResolvedImage struct { | |
| Prefix string // could be registry address + repository/namespace... | |
| Name string | |
| Tag string | |
| } | |
| func (r *ResolvedImage) Validate() error { | |
| if r.Prefix == "" { | |
| return fmt.Errorf("prefix cannot be empty") | |
| } | |
| if r.Name == "" { | |
| return fmt.Errorf("name cannot be empty") | |
| } | |
| return nil | |
| } |
| buildOptions := builder.BuilderOptions{ | ||
| ImageName: testImage, | ||
| BuildContext: blCtx, | ||
| Destination: testDestination, | ||
| Args: []builder.ArgInterface{&builder.BuildArg{Value: "SOME_ARG=some_value"}}, | ||
| Cache: cacheOpts, | ||
| Cache: kb.CacheOptions(), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test cases for custom registry configurations
Since this PR adds support for custom registries, consider adding test cases that verify:
- Different registry configurations
- Registry authentication
- Error cases for invalid registry settings
buildOptions := builder.BuilderOptions{
ImageName: testImage,
BuildContext: blCtx,
Args: []builder.ArgInterface{&builder.BuildArg{Value: "SOME_ARG=some_value"}},
Cache: kb.CacheOptions(),
+ Registry: builder.RegistryOptions{
+ URL: "registry.example.com",
+ Username: "test-user",
+ Password: "test-pass",
+ },
}Committable suggestion skipped: line range outside the PR's diff.
pkg/instance/execution.go
Outdated
| @@ -417,6 +418,7 @@ func (e *execution) prepareReplicaSetConfig() k8s.ReplicaSetConfig { | |||
| ServiceAccountName: e.instance.name, | |||
| ContainerConfig: containerConfig, | |||
| SidecarConfigs: sidecarConfigs, | |||
| ImagePullSecrets: []v1.LocalObjectReference{{Name: "registry-cert-secret"}}, | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Registry secret name should be configurable
The hardcoded registry secret name "registry-cert-secret" should be made configurable through the instance options for the following reasons:
- The
PodConfigstruct already has anImagePullSecretsfield designed for this purpose - There's a commented-out line in pod.go suggesting this was meant to be configurable:
// ImagePullSecrets: spec.ImagePullSecrets, - Different environments or users might need different registry secrets
Suggested locations to address:
- Add configuration option in
pkg/knuu/knuu.go:Optionsstruct - Update
pkg/instance/execution.goto use the configured value instead of hardcoding - Uncomment and utilize the
ImagePullSecretsfield inpkg/k8s/pod.go
🔗 Analysis chain
Consider parameterizing the registry secret name
The registry secret name is currently hardcoded. Consider:
- Making it configurable through the instance options
- Adding a check for the secret's existence before using it
Let me search for more context about how this registry secret is used in the codebase.
Let me check the instance configuration to see if there's already a mechanism for configuring image pull secrets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the registry secret exists in the cluster
# Check if the secret exists in the current namespace
kubectl get secret registry-cert-secret -o json 2>/dev/null | jq -r '.metadata.name'
Length of output: 72
Script:
#!/bin/bash
# Search for references to registry-cert-secret
rg "registry-cert-secret" -A 3 -B 3
# Search for ImagePullSecrets configuration patterns
ast-grep --pattern 'ImagePullSecrets: $_'
# Look for configuration structs that might contain registry settings
rg "type.*Config" -A 10
Length of output: 6749
Script:
#!/bin/bash
# Search for instance configuration structures and initialization
rg "type.*Instance" -A 10
rg "type.*Options" -A 10
# Look for other image pull secret configurations
rg "ImagePullSecrets" -B 5
Length of output: 5896
pkg/k8s/pod.go
Outdated
| @@ -623,6 +624,7 @@ func (c *Client) preparePodSpec(spec PodConfig, init bool) v1.PodSpec { | |||
| InitContainers: c.prepareInitContainers(spec.ContainerConfig, init), | |||
| Containers: []v1.Container{prepareContainer(spec.ContainerConfig)}, | |||
| Volumes: preparePodVolumes(spec.ContainerConfig), | |||
| // ImagePullSecrets: spec.ImagePullSecrets, | |||
There was a problem hiding this comment.
Uncomment ImagePullSecrets configuration.
The ImagePullSecrets configuration is commented out, which prevents the use of custom registries. This is inconsistent with the PR objective of supporting registries other than ttl.sh.
Apply this diff to enable the feature:
- // ImagePullSecrets: spec.ImagePullSecrets,
+ ImagePullSecrets: spec.ImagePullSecrets,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ImagePullSecrets: spec.ImagePullSecrets, | |
| ImagePullSecrets: spec.ImagePullSecrets, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pkg/builder/kaniko/kaniko.go (1)
15-15: Format Imports with goimportsAccording to static analysis, the file is not formatted with
goimportsusing the-local github.com/celestiaorgflag. Please rungoimportswith the appropriate flags to correctly format and group imports.🧰 Tools
🪛 golangci-lint (1.62.2)
15-15: File is not
goimports-ed with -local github.com/celestiaorg(goimports)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/builder/kaniko/kaniko.go(8 hunks)pkg/builder/kaniko/kaniko_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/builder/kaniko/kaniko.go
15-15: File is not goimports-ed with -local github.com/celestiaorg
(goimports)
🔇 Additional comments (2)
pkg/builder/kaniko/kaniko.go (1)
65-73:
Avoid Modifying Passed-by-Value Parameter
Assigning a value to b.ImageName inside the Build function will not affect the original BuilderOptions outside of the function since b is passed by value. To ensure the updated ImageName is used throughout the method, assign it to a local variable (e.g., imageName) and pass it to subsequent functions such as prepareJob.
Apply this diff to fix the issue:
func (k *Kaniko) Build(ctx context.Context, b builder.BuilderOptions) (logs string, err error) {
+ var imageName string
if b.ImageName == "" {
ri, err := k.DefaultImage(b.BuildContext)
if err != nil {
return "", err
}
- b.ImageName = ri.ToString()
+ imageName = ri.ToString()
} else {
+ imageName = b.ImageName
}Also, update the prepareJob method to accept imageName as a parameter:
job, err := k.prepareJob(ctx, &b, imageName)pkg/builder/kaniko/kaniko_test.go (1)
43-47: Add Tests for Custom Registry Configurations
Since this PR introduces support for custom registries, it's important to add test cases that verify:
- Different registry configurations.
- Registry authentication.
- Error handling for invalid registry settings.
Consider extending your test cases by adding registry options to buildOptions. For example:
buildOptions := builder.BuilderOptions{
ImageName: testImage,
BuildContext: blCtx,
Args: []builder.ArgInterface{&builder.BuildArg{Value: "SOME_ARG=some_value"}},
Cache: kb.CacheOptions(),
+ Registry: builder.RegistryOptions{
+ URL: "registry.example.com",
+ Username: "test-user",
+ Password: "test-pass",
+ },
}This will help ensure that the custom registry functionality works as expected under various scenarios.
| Args: []string{"echo '" + string(configContent) + "' > " + filepath.Join(configDir, configFile)}, | ||
| VolumeMounts: []v1.VolumeMount{ | ||
| { | ||
| Name: kanikoVolName, | ||
| MountPath: configDir, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Potential Command Injection Vulnerability
The configContent is embedded directly into a shell command without proper sanitization. If configContent contains special characters or shell syntax, it could lead to command injection or errors during execution.
To fix this issue, avoid inserting configContent directly into the shell command. Instead, pass configContent as an environment variable, which is safer and prevents shell injection. Then, write the environment variable to the file in the init container.
Apply this change:
initContainer := v1.Container{
Name: "setup-registry-config",
Image: "busybox:latest",
Command: []string{"/bin/sh", "-c"},
- Args: []string{"echo '" + string(configContent) + "' > " + filepath.Join(configDir, configFile)},
+ Args: []string{"echo \"$CONFIG_CONTENT\" > " + filepath.Join(configDir, configFile)},
+ Env: []v1.EnvVar{
+ {
+ Name: "CONFIG_CONTENT",
+ Value: string(configContent),
+ },
+ },
VolumeMounts: []v1.VolumeMount{
{
Name: kanikoVolName,
MountPath: configDir,
},
},
}This approach safely passes configContent as an environment variable, preventing potential command injection vulnerabilities.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Args: []string{"echo '" + string(configContent) + "' > " + filepath.Join(configDir, configFile)}, | |
| VolumeMounts: []v1.VolumeMount{ | |
| { | |
| Name: kanikoVolName, | |
| MountPath: configDir, | |
| }, | |
| }, | |
| } | |
| Args: []string{"echo \"$CONFIG_CONTENT\" > " + filepath.Join(configDir, configFile)}, | |
| Env: []v1.EnvVar{ | |
| { | |
| Name: "CONFIG_CONTENT", | |
| Value: string(configContent), | |
| }, | |
| }, | |
| VolumeMounts: []v1.VolumeMount{ | |
| { | |
| Name: kanikoVolName, | |
| MountPath: configDir, | |
| }, | |
| }, | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/builder/kaniko/kaniko.go (2)
38-63: LGTM! Consider adding godoc comments.The constructor implementation is solid with proper validation of dependencies. Consider adding godoc comments for the exported
Newfunction to document the requirements and behavior.+// New creates a new Kaniko builder instance. +// It requires sysDeps.K8sClient for kubernetes operations and sysDeps.MinioClient if building from directory context. +// If reg is nil, it will use the default registry (ttl.sh). func New(sysDeps *system.SystemDependencies, reg registry.Registry) (*Kaniko, error) {
38-39: Great architectural design!The registry integration through an interface is well-designed:
- Enables easy addition of new registry types
- Maintains backward compatibility with default registry
- Follows interface segregation principle
Also applies to: 44-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/builder/kaniko/kaniko.go(8 hunks)pkg/instance/execution.go(2 hunks)pkg/knuu/knuu.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/knuu/knuu.go
🔇 Additional comments (6)
pkg/instance/execution.go (2)
10-11: LGTM: Import statement is correctly added
The addition of the k8s core/v1 API import is necessary for the ImagePullSecrets configuration.
421-421:
Registry secret name should be configurable
The hardcoded registry secret name "registry-cert-secret" should be made configurable to support different registry configurations as mentioned in the PR objectives.
Consider:
- Adding a configuration option in the instance options struct
- Using the configured value instead of hardcoding
- Allowing multiple secrets to be specified when needed
- ImagePullSecrets: []v1.LocalObjectReference{{Name: "registry-cert-secret"}},
+ ImagePullSecrets: e.instance.build.imagePullSecrets,This requires adding the following to the build configuration:
type BuildConfig struct {
// existing fields...
imagePullSecrets []v1.LocalObjectReference
}
func (b *BuildConfig) SetImagePullSecrets(secrets []v1.LocalObjectReference) {
b.imagePullSecrets = secrets
}pkg/builder/kaniko/kaniko.go (4)
106-120: LGTM! Clean and focused implementations.
Both methods are well-implemented with clear responsibilities and good integration with the registry interface.
256-258: LGTM! Good validation.
Early validation of MinioClient requirement helps prevent runtime errors.
320-327: LGTM! Good logging integration.
The debug logging integration is well implemented with proper verbosity control.
368-368:
Security: Fix potential command injection vulnerability
The direct concatenation of configContent into the shell command is unsafe.
| @@ -62,12 +97,28 @@ func (k *Kaniko) Build(ctx context.Context, b *builder.BuilderOptions) (logs str | |||
| } | |||
|
|
|||
| if kJob.Status.Succeeded == 0 { | |||
| return logs, ErrBuildFailed | |||
| return logs, ErrBuildFailed.Wrap(fmt.Errorf("logs: ...%s", logs[MaxLogsLengthOnError:])) | |||
There was a problem hiding this comment.
Fix error log truncation logic
The current implementation shows only the end of the logs (logs[MaxLogsLengthOnError:]), which:
- Shows the last part instead of the beginning of the logs where errors typically appear
- Could panic if logs length is less than MaxLogsLengthOnError
- return logs, ErrBuildFailed.Wrap(fmt.Errorf("logs: ...%s", logs[MaxLogsLengthOnError:]))
+ logLen := len(logs)
+ if logLen > MaxLogsLengthOnError {
+ logs = logs[:MaxLogsLengthOnError] + "..."
+ }
+ return logs, ErrBuildFailed.Wrap(fmt.Errorf("logs: %s", logs))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return logs, ErrBuildFailed.Wrap(fmt.Errorf("logs: ...%s", logs[MaxLogsLengthOnError:])) | |
| logLen := len(logs) | |
| if logLen > MaxLogsLengthOnError { | |
| logs = logs[:MaxLogsLengthOnError] + "..." | |
| } | |
| return logs, ErrBuildFailed.Wrap(fmt.Errorf("logs: %s", logs)) |
pkg/cert/cert.go
Outdated
There was a problem hiding this comment.
What is this used for? I cannot see any usage of GenerateSelfSignedCert in this PR.
There was a problem hiding this comment.
oh this was for the local ttl registry idea which did not work with k8s at the end, will remove it.
In order to support long running tests,
ttl.shis not enough as it keeps an image up to 24 hours. This PR proposes a support to accept custom registries. The default registry is stillttl.shso current tests and codes work. However, customized code is developed to supportDockerHubandScalewayregistries and since it is implemented as an Interface, we/user can add further registries if it is needed.Note: some of the tests are failing because of this: #595 which is tracked in another issue
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores