feat: Unkey Builds #4098
feat: Unkey Builds #4098
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds a pluggable S3-backed build system and BuildService (CreateBuild, GenerateUploadURL); switches deployments to a BuildContext/docker_image oneof with nested GitCommitInfo; implements Docker and Depot build backends, S3 presign storage, CLI upload flow, docs/scripts, and DB schema to track depot_project_id. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Developer CLI
participant Ctrl as Control Plane (BuildService)
participant S3 as S3 Storage
participant WF as Deploy Workflow
participant BE as Build Backend (Docker/Depot)
participant KR as Krane
CLI->>Ctrl: GenerateUploadURL(unkey_project_id)
Ctrl-->>CLI: {upload_url, context_key}
CLI->>S3: PUT tar.gz -> upload_url
CLI->>Ctrl: CreateDeployment(context_key, dockerfile_path?)
Ctrl->>WF: Start workflow
rect rgba(230,245,255,0.6)
note over WF: Build phase
WF->>Ctrl: CreateBuild(context_key,dockerfile_path,unkey_project_id,deployment_id)
Ctrl->>BE: CreateBuild(...)
BE-->>Ctrl: {image_name, build_id}
end
rect rgba(240,255,230,0.6)
note over WF: Deploy phase
WF->>KR: CreateDeployment(image_name,...)
KR-->>WF: status updates
end
WF-->>Ctrl: Final status
Ctrl-->>CLI: Deployment result
sequenceDiagram
autonumber
participant Ctrl as Control Plane
participant Depot as Depot API/Registry
participant S3 as S3 Storage
participant Docker as Local Docker
Ctrl->>S3: Presign PUT for context_key
S3-->>Ctrl: upload_url
Ctrl->>Depot: Ensure/get-or-create project
Ctrl->>Depot: Create build
Depot-->>Ctrl: Build ID / build stream
Depot->>Ctrl: BuildKit session / build stream
Ctrl->>Docker: Pull built image from registry
Docker-->>Ctrl: Image available locally
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)go/apps/ctrl/services/build/backend/depot/create_build.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
go/cmd/deploy/control_plane.go (2)
178-214: The previously flagged empty string pointer issue remains unresolved.Lines 184 and 189 still pass empty string pointers for optional fields (
KeyspaceIdandDockerFilePath), which prevents server-side defaults from being applied.As noted in the past review, use a helper that returns
nilfor empty strings. The maintainer preference is to usepkg/ptrfor this purpose rather than a custom helper.Example using pkg/ptr:
import "github.com/unkeyed/unkey/go/pkg/ptr" // In CreateDeploymentRequest: KeyspaceId: ptr.String(c.opts.KeyspaceID), // returns nil if empty DockerFilePath: ptr.String(c.opts.Dockerfile), // returns nil if empty
135-176: The previously flagged security and privacy issues remain unresolved.The concerns raised in the earlier review are still present:
- Security:
/tmp/ctrlwith0o777and tar file with0o666create insecure permissions- Privacy: No
.dockerignorehandling risks uploading secrets (.env,.git, etc.)- Robustness: No check for
tarcommand availabilityThese issues were acknowledged by the team and deferred to a subtask. Please ensure they are tracked and addressed before production use.
go/apps/ctrl/services/build/backend/depot/create_build.go (1)
259-259: Logging key/value mismatch and incomplete valuesThe log pairs are misaligned and
unkey_project_idvalue is missing; also pass the string value for depot_project_id.Apply this diff:
- s.logger.Info("Returning existing depot project", "depot_project_id", project.DepotProjectID, "unkey_project_id", "project_name", projectName) + s.logger.Info("Returning existing depot project", + "depot_project_id", project.DepotProjectID.String, + "unkey_project_id", unkeyProjectID, + "project_name", projectName, + )go/apps/ctrl/config.go (1)
140-146: Missing validation for required Depot configuration fields.When using the Depot backend,
APIUrl,RegistryUrl, andUsernameare also required (not justAccessToken). Missing these values will cause runtime failures during build operations.Apply this diff:
case BuildBackendDepot: return assert.All( + assert.NotEmpty(c.Depot.APIUrl, "Depot API URL is required when using Depot backend"), + assert.NotEmpty(c.Depot.RegistryUrl, "Depot registry URL is required when using Depot backend"), + assert.NotEmpty(c.Depot.Username, "Depot registry username is required when using Depot backend"), + assert.NotEmpty(c.Depot.AccessToken, "Depot access token is required when using Depot backend"), assert.NotEmpty(c.BuildS3.URL, "build S3 URL is required when using Depot backend"), assert.NotEmpty(c.BuildS3.Bucket, "build S3 bucket is required when using Depot backend"), assert.NotEmpty(c.BuildS3.AccessKeyID, "build S3 access key ID is required when using Depot backend"), assert.NotEmpty(c.BuildS3.AccessKeySecret, "build S3 access key secret is required when using Depot backend"), - assert.NotEmpty(c.Depot.AccessToken, "Depot access token is required when using Depot backend"), )go/apps/ctrl/services/build/backend/docker/create_build.go (1)
31-36: Missing validation for required DeploymentId field.The
DeploymentIdis used in the image name construction (line 72), but it's not validated here. If empty, it would produce a malformed image name ending with a hyphen.Apply this diff:
if err := assert.All( assert.NotEmpty(req.Msg.ContextKey, "contextKey is required"), assert.NotEmpty(req.Msg.UnkeyProjectId, "unkeyProjectID is required"), + assert.NotEmpty(req.Msg.DeploymentId, "deploymentId is required"), ); err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, err) }
🧹 Nitpick comments (8)
go/apps/ctrl/services/build/backend/depot/create_build.go (3)
60-61: Consider making architecture configurable.The architecture is hardcoded to
amd64. While past discussions indicate this is acceptable for now, consider making this configurable via the DepotConfig to support multi-architecture deployments in the future.
273-273: Hardcoded region limits deployment flexibility.The Depot project region is hardcoded to
us-east-1. Consider making this configurable via DepotConfig to support deployments closer to users or to comply with data residency requirements.Example configuration addition:
type DepotConfig struct { // ... existing fields ... Region string // e.g., "us-east-1", "eu-west-1" }
146-147: Document fragile ordering dependency more clearly.The comment indicates that the order of
s.registryUrlanddepotProjectIDmust never change, but this coupling is fragile and error-prone. Consider extracting this logic into a named function with validation to prevent future mistakes.Example refactor:
// buildDepotImageName constructs the image name for Depot registry. // The order of registry and projectID is required by Depot's API. func buildDepotImageName(registryUrl, depotProjectID, unkeyProjectID, deploymentID string) string { return fmt.Sprintf("%s/%s:%s-%s", registryUrl, depotProjectID, unkeyProjectID, deploymentID) }go/apps/ctrl/services/build/backend/docker/create_build.go (2)
38-39: Architecture mismatch between backends.The Docker backend uses
amd64while the Depot backend (create_build.go:60) also usesamd64. This is consistent, but consider extracting this as a shared constant to ensure both backends remain aligned.Example:
// In a shared package or constants file: const DefaultBuildArchitecture = "amd64" const DefaultBuildPlatform = "linux/" + DefaultBuildArchitecture
111-112: Silently ignoring JSON parse errors.If JSON unmarshaling fails, the error is silently ignored with
continue. This could mask legitimate build output parsing issues. Consider logging the unmarshal error at debug level.var resp dockerBuildResponse if err := json.Unmarshal(scanner.Bytes(), &resp); err != nil { + d.logger.Debug("Failed to parse build output line", "error", err) continue }go/apps/ctrl/services/build/storage/s3.go (3)
36-46: Using deprecated AWS SDK v2 endpoint resolver.The code uses
EndpointResolverWithOptionsFuncwhich is deprecated in AWS SDK v2. While it works with the nolint directive, consider migrating to the newerBaseEndpointresolver approach for long-term maintainability.Based on learnings: AWS SDK v2 recommends using
WithBaseEndpointfor custom endpoints:cfg, err := awsConfig.LoadDefaultConfig(context.Background(), awsConfig.WithBaseEndpoint(config.S3URL), awsConfig.WithCredentialsProvider(credentials.NewStaticCredentialsProvider( config.S3AccessKeyID, config.S3AccessKeySecret, "")), awsConfig.WithRegion("auto"), awsConfig.WithRetryMode(aws.RetryModeStandard), awsConfig.WithRetryMaxAttempts(3), )
48-48: "auto" region may not work with all S3-compatible services.While "auto" region works with Cloudflare R2, not all S3-compatible services support this. Consider making the region configurable in S3Config for broader compatibility.
60-65: Bucket creation error handling could be more specific.The code checks if the error string contains "BucketAlreadyOwnedByYou", but this is fragile string matching. Consider using AWS error type checking for more robust error handling.
import ( "github.com/aws/smithy-go" ) _, err = client.CreateBucket(context.Background(), &awsS3.CreateBucketInput{ Bucket: aws.String(config.S3Bucket), }) if err != nil { var ae smithy.APIError if errors.As(err, &ae) && ae.ErrorCode() == "BucketAlreadyOwnedByYou" { // Bucket already exists, continue } else { return nil, fmt.Errorf("failed to create bucket: %w", err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
internal/proto/generated/ctrl/v1/build_pb.tsis excluded by!**/generated/**internal/proto/generated/ctrl/v1/deployment_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (7)
go/apps/ctrl/config.go(4 hunks)go/apps/ctrl/services/build/backend/depot/create_build.go(1 hunks)go/apps/ctrl/services/build/backend/docker/create_build.go(1 hunks)go/apps/ctrl/services/build/storage/s3.go(1 hunks)go/cmd/deploy/control_plane.go(4 hunks)go/pkg/db/schema.sql(1 hunks)internal/db/src/schema/projects.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go/pkg/db/schema.sql
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T10:12:40.775Z
Learnt from: Flo4604
PR: unkeyed/unkey#4098
File: go/proto/ctrl/v1/deployment.proto:33-36
Timestamp: 2025-10-15T10:12:40.775Z
Learning: In the Unkey codebase proto files (ctrl/v1/build.proto, ctrl/v1/deployment.proto, hydra/v1/deployment.proto), use `dockerfile_path` (not `docker_file_path`) for consistency in generated Go field names.
Applied to files:
go/cmd/deploy/control_plane.go
🧬 Code graph analysis (5)
go/apps/ctrl/services/build/storage/s3.go (2)
go/apps/ctrl/config.go (1)
S3Config(18-23)go/pkg/vault/storage/s3.go (2)
S3Config(25-31)NewS3(33-70)
go/apps/ctrl/services/build/backend/docker/create_build.go (2)
go/apps/ctrl/services/build/backend/docker/service.go (1)
Docker(11-17)go/gen/proto/ctrl/v1/build.pb.go (6)
CreateBuildRequest(24-32)CreateBuildRequest(45-45)CreateBuildRequest(60-62)CreateBuildResponse(92-99)CreateBuildResponse(112-112)CreateBuildResponse(127-129)
go/apps/ctrl/services/build/backend/depot/create_build.go (3)
go/apps/ctrl/services/build/backend/depot/service.go (1)
Depot(11-21)go/gen/proto/ctrl/v1/build.pb.go (6)
CreateBuildRequest(24-32)CreateBuildRequest(45-45)CreateBuildRequest(60-62)CreateBuildResponse(92-99)CreateBuildResponse(112-112)CreateBuildResponse(127-129)go/pkg/db/project_update_depot_id.sql_generated.go (1)
UpdateProjectDepotIDParams(21-25)
go/apps/ctrl/config.go (3)
go/apps/ctrl/services/build/storage/s3.go (1)
S3Config(24-30)go/pkg/vault/storage/s3.go (1)
S3Config(25-31)go/apps/ctrl/services/build/backend/depot/service.go (1)
Depot(11-21)
go/cmd/deploy/control_plane.go (5)
go/gen/proto/ctrl/v1/ctrlv1connect/build.connect.go (2)
BuildServiceClient(45-48)NewBuildServiceClient(57-74)go/cmd/deploy/main.go (1)
DeployOptions(80-95)internal/proto/generated/ctrl/v1/build_pb.ts (1)
GenerateUploadURLRequest(94-101)internal/proto/generated/ctrl/v1/deployment_pb.ts (3)
CreateDeploymentRequest(21-99)Deployment(173-297)GetDeploymentRequest(137-142)go/gen/proto/ctrl/v1/deployment.pb.go (13)
CreateDeploymentRequest(136-157)CreateDeploymentRequest(170-170)CreateDeploymentRequest(185-187)SourceType(87-87)SourceType(119-121)SourceType(123-125)SourceType(132-134)Deployment(413-447)Deployment(460-460)Deployment(475-477)GetDeploymentRequest(325-330)GetDeploymentRequest(343-343)GetDeploymentRequest(358-360)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
🔇 Additional comments (8)
go/cmd/deploy/control_plane.go (5)
40-57: LGTM! Clean dual-client architecture.The separation of deployment and build clients improves modularity and follows single responsibility principle.
59-94: LGTM! Well-structured upload flow.The method properly orchestrates presigned URL generation, tar creation, upload, and cleanup with appropriate error handling and validation.
96-133: LGTM! Robust presigned URL upload implementation.Proper file handling, correct status code checks (200/204 for S3), reasonable 5-minute timeout, and thorough error handling.
216-234: LGTM! Clean migration to deployment ID.The method correctly uses the new
deploymentIDparameter anddeploymentClient.
236-288: LGTM! Improved logging formatting.The multiline logging format enhances readability while maintaining the same polling logic.
go/apps/ctrl/config.go (3)
11-16: LGTM!The BuildBackend type and constants provide a clear abstraction for backend selection.
56-65: LGTM!The DepotConfig struct is well-documented with clear field descriptions.
154-155: LGTM!Good use of
fmt.Errorffor clear error messaging when an unknown backend is provided.
ogzhanolguncu
force-pushed
the
depot-poc
branch
from
18ebfca to
7f61242
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
go/cmd/deploy/control_plane.go (2)
135-176: Harden tar creation: secure temp, avoid world-w perms, check tar, respect ignores
- Security: avoid 0777 dir and 0666 files under fixed /tmp/ctrl. Use os.TempDir(), 0700 dir, and keep CreateTemp’s 0600.
- Robustness: guard with exec.LookPath("tar"); fail fast if missing.
- Privacy/size: at minimum, exclude .git, node_modules, and .env; follow-up to parse .dockerignore.
Apply:
- sharedDir := "/tmp/ctrl" - if err := os.MkdirAll(sharedDir, 0o777); err != nil { + sharedDir := filepath.Join(os.TempDir(), "ctrl") + if err := os.MkdirAll(sharedDir, 0o700); err != nil { return "", fmt.Errorf("failed to create shared dir: %w", err) } @@ tmpFile.Close() tarPath := tmpFile.Name() - if err := os.Chmod(tarPath, 0o666); err != nil { - os.Remove(tarPath) - return "", fmt.Errorf("failed to set file permissions: %w", err) - } + // Keep 0600 from CreateTemp - cmd := exec.Command("tar", "-czf", tarPath, "-C", absContextPath, ".") + if _, err := exec.LookPath("tar"); err != nil { + os.Remove(tarPath) + return "", fmt.Errorf("tar not found in PATH: %w", err) + } + cmd := exec.Command( + "tar", + "-czf", tarPath, + "--exclude=.git", + "--exclude=node_modules", + "--exclude=.env", + "-C", absContextPath, + ".", + )Follow-up: parse .dockerignore for full fidelity.
182-195: Don’t send empty optional strings; send nil when unsetPassing &c.opts.KeyspaceID / &c.opts.Dockerfile when empty marks fields as set. Use nil for empty to let server defaults apply.
Apply:
- createReq := connect.NewRequest(&ctrlv1.CreateDeploymentRequest{ + var keyspaceIDPtr *string + if c.opts.KeyspaceID != "" { + keyspaceIDPtr = &c.opts.KeyspaceID + } + var dockerfilePtr *string + if c.opts.Dockerfile != "" { + dockerfilePtr = &c.opts.Dockerfile + } + createReq := connect.NewRequest(&ctrlv1.CreateDeploymentRequest{ ProjectId: c.opts.ProjectID, - KeyspaceId: &c.opts.KeyspaceID, + KeyspaceId: keyspaceIDPtr, Branch: c.opts.Branch, SourceType: ctrlv1.SourceType_SOURCE_TYPE_CLI_UPLOAD, EnvironmentSlug: c.opts.Environment, ContextKey: contextKey, - DockerfilePath: &c.opts.Dockerfile, + DockerfilePath: dockerfilePtr, GitCommitSha: commitInfo.CommitSHA, GitCommitMessage: commitInfo.Message, GitCommitAuthorHandle: commitInfo.AuthorHandle, GitCommitAuthorAvatarUrl: commitInfo.AuthorAvatarURL, GitCommitTimestamp: commitInfo.CommitTimestamp, })go/proto/ctrl/v1/deployment.proto (1)
33-47: Add docker_image to support image-only deployments (non-breaking)We need a path to deploy pre-built images. Add an optional docker_image (new tag) and document XOR with context_key. Keep existing tags to avoid wire breaks.
Apply:
message CreateDeploymentRequest { @@ // context_key must be provided string context_key = 6; // S3 key for uploaded build context optional string dockerfile_path = 7; // Path to Dockerfile within context (default: "Dockerfile") @@ // Keyspace ID for authentication optional string keyspace_id = 13; + + // Optional: deploy a pre-built image; when set, server must ignore context_key/dockerfile_path + optional string docker_image = 14; }
🧹 Nitpick comments (2)
go/apps/ctrl/services/deployment/create_deployment.go (2)
114-135: Persist build source for traceability (context_key/dockerfile_path)I don’t see these fields stored in the deployment record. Without them, debugging and audit are harder. Please persist them (columns or metadata JSON), or confirm they are recorded elsewhere.
141-147: Avoid logging full context_key (possible leakage)If the S3 key is sensitive, log a hash or prefix instead of the full key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go/gen/proto/ctrl/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**internal/proto/generated/ctrl/v1/build_pb.tsis excluded by!**/generated/**internal/proto/generated/ctrl/v1/deployment_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
go/apps/ctrl/services/deployment/create_deployment.go(3 hunks)go/cmd/deploy/control_plane.go(4 hunks)go/proto/ctrl/v1/deployment.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T10:12:40.775Z
Learnt from: Flo4604
PR: unkeyed/unkey#4098
File: go/proto/ctrl/v1/deployment.proto:33-36
Timestamp: 2025-10-15T10:12:40.775Z
Learning: In the Unkey codebase proto files (ctrl/v1/build.proto, ctrl/v1/deployment.proto, hydra/v1/deployment.proto), use `dockerfile_path` (not `docker_file_path`) for consistency in generated Go field names.
Applied to files:
go/proto/ctrl/v1/deployment.protogo/cmd/deploy/control_plane.go
🧬 Code graph analysis (1)
go/cmd/deploy/control_plane.go (5)
go/gen/proto/ctrl/v1/ctrlv1connect/build.connect.go (2)
BuildServiceClient(45-48)NewBuildServiceClient(57-74)go/cmd/deploy/main.go (1)
DeployOptions(80-95)internal/proto/generated/ctrl/v1/build_pb.ts (1)
GenerateUploadURLRequest(94-101)go/pkg/git/git.go (1)
GetInfo(43-100)go/gen/proto/ctrl/v1/deployment.pb.go (10)
CreateDeploymentRequest(136-157)CreateDeploymentRequest(170-170)CreateDeploymentRequest(185-187)SourceType(87-87)SourceType(119-121)SourceType(123-125)SourceType(132-134)Deployment(413-447)Deployment(460-460)Deployment(475-477)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (2)
go/proto/ctrl/v1/deployment.proto (1)
33-36: Field naming LGTM (dockerfile_path) and context_key additionConsistent with other protos. Good.
Based on learnings
go/cmd/deploy/control_plane.go (1)
97-133: Confirm presigned URL header expectationsSome S3 presigns fail if unexpected Content-Type is sent. Verify server signs without Content-Type constraints, or align the header value (e.g., application/octet-stream).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/apps/ctrl/workflows/deploy/deploy_handler.go (1)
203-209: Syntax error: invalid for-range over integer literalUse a counted loop.
- for i range 300 { + for i := 0; i < 300; i++ {
♻️ Duplicate comments (3)
go/apps/ctrl/services/build/backend/depot/create_build.go (1)
257-261: Fix misaligned log key/value pairs and value types [duplicate of prior review]Pass proper values and use the string of DepotProjectID.
- s.logger.Info("Returning existing depot project", "depot_project_id", project.DepotProjectID, "unkey_project_id", "project_name", projectName) + s.logger.Info("Returning existing depot project", + "depot_project_id", project.DepotProjectID.String, + "unkey_project_id", unkeyProjectID, + "project_name", projectName, + )go/cmd/deploy/control_plane.go (2)
136-177: Harden temp handling; avoid permissive perms; check tar; respect ignoresWorld-writable /tmp/ctrl (0777) and 0666 files are unsafe, and relying on external tar without .dockerignore filtering risks secret leakage and portability issues.
Apply minimal hardening:
- sharedDir := "/tmp/ctrl" - if err := os.MkdirAll(sharedDir, 0o777); err != nil { + sharedDir := filepath.Join(os.TempDir(), "ctrl") + if err := os.MkdirAll(sharedDir, 0o700); err != nil { return "", fmt.Errorf("failed to create shared dir: %w", err) } @@ - tmpFile, err := os.CreateTemp(sharedDir, "build-context-*.tar.gz") + tmpFile, err := os.CreateTemp(sharedDir, "build-context-*.tar.gz") if err != nil { return "", fmt.Errorf("failed to create temp file: %w", err) } tmpFile.Close() tarPath := tmpFile.Name() - if err := os.Chmod(tarPath, 0o666); err != nil { - os.Remove(tarPath) - return "", fmt.Errorf("failed to set file permissions: %w", err) - } + // Keep 0600 from CreateTemp; no chmod needed + if _, err := exec.LookPath("tar"); err != nil { + os.Remove(tarPath) + return "", fmt.Errorf("tar not found in PATH: %w", err) + } cmd := exec.Command("tar", "-czf", tarPath, "-C", absContextPath, ".")Follow-ups:
- Respect .dockerignore/.gitignore during archiving to avoid uploading secrets and bloat. We can help wire an exclude list or implement Go-based tar with filtering. (As discussed earlier)
189-201: Don’t send empty optional strings; set KeyspaceId only when non-emptyEmpty string pointers mark fields as “set” in protobuf oneofs/optionals. Only set KeyspaceId when non-empty to allow server defaults.
Apply:
req := &ctrlv1.CreateDeploymentRequest{ ProjectId: c.opts.ProjectID, - KeyspaceId: &c.opts.KeyspaceID, Branch: c.opts.Branch, EnvironmentSlug: c.opts.Environment, GitCommit: &ctrlv1.GitCommitInfo{ CommitSha: commitInfo.CommitSHA, CommitMessage: commitInfo.Message, AuthorHandle: commitInfo.AuthorHandle, AuthorAvatarUrl: commitInfo.AuthorAvatarURL, Timestamp: commitInfo.CommitTimestamp, }, } +if c.opts.KeyspaceID != "" { + req.KeyspaceId = ptr.P(c.opts.KeyspaceID) +}Also applies to: 203-217
🧹 Nitpick comments (8)
go/proto/hydra/v1/deployment.proto (1)
18-21: Enforce XOR with oneof; align with ctrl protoUse oneof for context_key vs docker_image to encode the contract in the schema.
Apply:
- // Build source fields, exactly one of (context_key, docker_image) must be set - optional string context_key = 3; - optional string dockerfile_path = 4; - optional string docker_image = 5; + // Build source: exactly one must be set + oneof source { + string context_key = 3; + string docker_image = 5; + } + optional string dockerfile_path = 4;As per coding guidelines and learnings
go/apps/ctrl/services/deployment/create_deployment.go (1)
182-188: Propagate defaulted dockerfile_path to Hydra requestYou resolve a default dockerfilePath but don’t set it on deployReq if unset in the incoming message.
case *ctrlv1.CreateDeploymentRequest_BuildContext: - deployReq.ContextKey = proto.String(source.BuildContext.GetContextKey()) - deployReq.DockerfilePath = source.BuildContext.DockerfilePath + deployReq.ContextKey = proto.String(contextKey) + // Always pass the resolved path (defaults to ./Dockerfile if empty) + deployReq.DockerfilePath = proto.String(dockerfilePath)Also applies to: 105-112
go/apps/ctrl/services/build/backend/depot/create_build.go (1)
192-200: Don’t spam stdout with build statusStreaming raw JSON to os.Stdout is noisy; use logger at debug level or buffer and print on error.
- buildStatusCh := make(chan *client.SolveStatus, 10) - go func() { - enc := json.NewEncoder(os.Stdout) - enc.SetIndent("", " ") - for status := range buildStatusCh { - _ = enc.Encode(status) - } - }() + buildStatusCh := make(chan *client.SolveStatus, 10) + // Optionally: forward summarized events to logger at debug level + // or drain silently unless there is an error.Refer to prior discussion in review thread
go/apps/ctrl/workflows/deploy/deploy_handler.go (2)
217-221: Log deployment status value, not the whole messageUse the enum/status field.
- w.logger.Info("deployment status", - "deployment_id", deployment.ID, - "status", resp.Msg, - ) + w.logger.Info("deployment status", + "deployment_id", deployment.ID, + "status", resp.Msg.GetStatus(), + )
276-305: Add HTTP timeout for OpenAPI scraping (optional)Avoid indefinite hangs on unresponsive instances.
- resp, err := http.DefaultClient.Get(openapiURL) + httpClient := &http.Client{Timeout: 5 * time.Second} + resp, err := httpClient.Get(openapiURL)go/apps/ctrl/services/deployment/create_deployment_simple_test.go (1)
14-24: Avoid duplicating service validation logic in testsvalidateTimestamp mirrors service behavior and can drift. Prefer importing a shared validator or exposing a small internal helper for tests.
go/cmd/deploy/control_plane.go (1)
268-269: Make build polling timeout configurable and increase default. Fixed 5 minutes may time out remote backend builds; introduce a CLI/config flag and set the default to 20–30 minutes.go/cmd/deploy/main.go (1)
69-86: Remove unused DeployOptions fields: Registry, SkipPush, LinuxThese fields are populated from CLI flags but never read anywhere in the codebase. They are dead code from the previous client-side build flow and should be removed or explicitly marked as deprecated to avoid confusion.
Lines 78, 80, 85 (field definitions) and 187, 189, 194 (flag assignments) can be safely removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go/gen/proto/ctrl/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/hydra/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**internal/proto/generated/ctrl/v1/deployment_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
go/apps/ctrl/services/build/backend/depot/create_build.go(1 hunks)go/apps/ctrl/services/deployment/create_deployment.go(5 hunks)go/apps/ctrl/services/deployment/create_deployment_simple_test.go(18 hunks)go/apps/ctrl/workflows/deploy/deploy_handler.go(3 hunks)go/cmd/deploy/control_plane.go(5 hunks)go/cmd/deploy/main.go(5 hunks)go/proto/ctrl/v1/deployment.proto(1 hunks)go/proto/hydra/v1/deployment.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-15T10:12:40.810Z
Learnt from: Flo4604
PR: unkeyed/unkey#4098
File: go/proto/ctrl/v1/deployment.proto:33-36
Timestamp: 2025-10-15T10:12:40.810Z
Learning: In the Unkey codebase proto files (ctrl/v1/build.proto, ctrl/v1/deployment.proto, hydra/v1/deployment.proto), use `dockerfile_path` (not `docker_file_path`) for consistency in generated Go field names.
Applied to files:
go/proto/hydra/v1/deployment.protogo/apps/ctrl/workflows/deploy/deploy_handler.gogo/apps/ctrl/services/deployment/create_deployment.gogo/proto/ctrl/v1/deployment.protogo/cmd/deploy/control_plane.go
📚 Learning: 2025-09-01T15:10:44.959Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/deployment.proto:7-15
Timestamp: 2025-09-01T15:10:44.959Z
Learning: In the unkey/unkey repository, the metald service receives deployment_id values from upstream services rather than generating them internally. The deployment_id field in DeploymentRequest is part of a service-to-service communication pattern.
Applied to files:
go/proto/hydra/v1/deployment.proto
🧬 Code graph analysis (6)
go/apps/ctrl/services/build/backend/depot/create_build.go (3)
go/apps/ctrl/services/build/backend/depot/service.go (1)
Depot(11-21)go/gen/proto/ctrl/v1/build.pb.go (6)
CreateBuildRequest(24-32)CreateBuildRequest(45-45)CreateBuildRequest(60-62)CreateBuildResponse(92-99)CreateBuildResponse(112-112)CreateBuildResponse(127-129)go/pkg/db/project_update_depot_id.sql_generated.go (1)
UpdateProjectDepotIDParams(21-25)
go/apps/ctrl/workflows/deploy/deploy_handler.go (3)
go/pkg/db/models_generated.go (2)
DeploymentsStatusBuilding(197-197)DeploymentsStatusDeploying(198-198)go/pkg/db/deployment_step_insert.sql_generated.go (1)
InsertDeploymentStepParams(33-40)go/gen/proto/ctrl/v1/build.pb.go (3)
CreateBuildRequest(24-32)CreateBuildRequest(45-45)CreateBuildRequest(60-62)
go/apps/ctrl/services/deployment/create_deployment.go (2)
go/gen/proto/ctrl/v1/deployment.pb.go (7)
CreateDeploymentRequest_BuildContext(250-252)CreateDeploymentRequest_BuildContext(258-258)BuildContext(262-268)BuildContext(281-281)BuildContext(296-298)CreateDeploymentRequest_DockerImage(254-256)CreateDeploymentRequest_DockerImage(260-260)go/gen/proto/hydra/v1/deployment.pb.go (3)
DeployRequest(25-35)DeployRequest(48-48)DeployRequest(63-65)
go/cmd/deploy/main.go (1)
go/cmd/deploy/control_plane.go (1)
NewControlPlaneClient(48-58)
go/apps/ctrl/services/deployment/create_deployment_simple_test.go (2)
go/gen/proto/ctrl/v1/deployment.pb.go (11)
GitCommitInfo(314-323)GitCommitInfo(336-336)GitCommitInfo(351-353)CreateDeploymentRequest(136-154)CreateDeploymentRequest(167-167)CreateDeploymentRequest(182-184)CreateDeploymentRequest_BuildContext(250-252)CreateDeploymentRequest_BuildContext(258-258)BuildContext(262-268)BuildContext(281-281)BuildContext(296-298)go/pkg/db/deployment_insert.sql_generated.go (1)
InsertDeploymentParams(51-67)
go/cmd/deploy/control_plane.go (5)
go/gen/proto/ctrl/v1/ctrlv1connect/build.connect.go (2)
BuildServiceClient(45-48)NewBuildServiceClient(57-74)go/cmd/deploy/main.go (1)
DeployOptions(70-86)go/gen/proto/ctrl/v1/build.pb.go (3)
GenerateUploadURLRequest(152-157)GenerateUploadURLRequest(170-170)GenerateUploadURLRequest(185-187)go/pkg/git/git.go (1)
GetInfo(43-100)go/gen/proto/ctrl/v1/deployment.pb.go (16)
CreateDeploymentRequest(136-154)CreateDeploymentRequest(167-167)CreateDeploymentRequest(182-184)GitCommitInfo(314-323)GitCommitInfo(336-336)GitCommitInfo(351-353)CreateDeploymentRequest_BuildContext(250-252)CreateDeploymentRequest_BuildContext(258-258)BuildContext(262-268)BuildContext(281-281)BuildContext(296-298)CreateDeploymentRequest_DockerImage(254-256)CreateDeploymentRequest_DockerImage(260-260)Deployment(530-564)Deployment(577-577)Deployment(592-594)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
🔇 Additional comments (6)
go/apps/ctrl/services/deployment/create_deployment.go (1)
170-180: Verify KeyAuth vs Keyspace mappingHydra expects KeyAuthId but ctrl request carries keyspace_id. Confirm these IDs are the same domain object; otherwise, add a distinct field or translation.
As per coding guidelines
go/proto/ctrl/v1/deployment.proto (1)
30-46: Schema surface looks consistent (oneof + dockerfile_path)The oneof source and dockerfile_path naming align with the rest of the system.
Based on learnings
go/cmd/deploy/main.go (3)
40-45: New step messages look goodClear, consistent with the new context-key flow.
220-255: Context-upload + createDeployment flow LGTMGood separation of prebuilt-image vs context build; clear UX messages and error surfacing.
100-101: Clarify product intent: docker-image flag contradicts PR summaryThe docker-image flag and prebuilt image support are fully implemented and active in the codebase, contradicting the PR summary stating they were removed. The flag is:
- Exposed in CLI (go/cmd/deploy/main.go:100)
- Integrated end-to-end through control plane → service → workflow layers
- A first-class oneof source alongside build_context at proto level
- Documented with example usage (go/cmd/deploy/main.go:143)
Confirm whether this PR intends to:
- Keep prebuilt image support (expected given the code), and update the PR summary, or
- Remove it, in which case all these references require cleanup.
go/apps/ctrl/services/deployment/create_deployment_simple_test.go (1)
265-487: Field mapping assertions are thoroughCovers populated/empty/mixed Git fields and timestamp validity against db insert params. Good guardrails.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
go/apps/ctrl/services/build/backend/docker/generate_upload_url.go (1)
13-42: Duplication acknowledged in previous review.This implementation duplicates depot/generate_upload_url.go as noted in previous comments. The team has agreed to consolidate this in a follow-up.
go/apps/ctrl/run.go (2)
173-223: Type mismatch already flagged in previous review.The past review correctly identified that
buildServiceis declared asBuildServiceClientbut assigned handler implementations. The suggested fix using an adapter or separate handler/client variables remains valid.
328-336: Client/handler confusion continues from line 173.This follows from the type mismatch at line 173. Once that's resolved with separate handler and client variables, pass the handler to
NewBuildServiceHandlerand the client todeployment.New.go/apps/ctrl/services/build/backend/docker/create_build.go (2)
68-73: Image name normalization risk acknowledged.Lowercasing the concatenated IDs could cause collisions if projects differ only by case. This was noted in previous reviews as a potential issue for case-sensitive uniqueness.
31-36: Add DeploymentId validation.
DeploymentIdis required for generating the image name (line 72) but is not validated. Add it to the validation checks to fail fast on missing input.Apply this diff:
if err := assert.All( assert.NotEmpty(req.Msg.ContextKey, "contextKey is required"), assert.NotEmpty(req.Msg.UnkeyProjectId, "unkeyProjectID is required"), + assert.NotEmpty(req.Msg.DeploymentId, "deploymentId is required"), ); err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, err) }go/apps/ctrl/services/build/backend/depot/create_build.go (3)
59-61: Architecture should default to amd64.Hardcoding
arm64will break common x86_64 environments. As noted in previous reviews, default toamd64or make this configurable.
247-250: Fix logging key/value alignment.The log statement has misaligned key/value pairs and incomplete values, as flagged in previous reviews.
Apply this diff:
- s.logger.Info("Returning existing depot project", "depot_project_id", project.DepotProjectID, "unkey_project_id", "project_name", projectName) + s.logger.Info("Returning existing depot project", + "depot_project_id", project.DepotProjectID.String, + "unkey_project_id", unkeyProjectID, + "project_name", projectName, + )
313-324: Use standard Docker registry auth types.As noted in previous reviews, use the engine's
registry.AuthConfigandStdEncodinginstead of CLI types and URL encoding.Apply this diff:
+ // import "github.com/docker/docker/api/types/registry" - authConfig := types.AuthConfig{ + authConfig := registry.AuthConfig{ Username: s.username, Password: s.accessToken, ServerAddress: s.registryUrl, } // Encode auth to base64 JSON authJSON, err := json.Marshal(authConfig) if err != nil { return fmt.Errorf("failed to marshal auth: %w", err) } - encodedAuth := base64.URLEncoding.EncodeToString(authJSON) + encodedAuth := base64.StdEncoding.EncodeToString(authJSON)go/apps/ctrl/services/build/storage/s3.go (1)
1-180: Code duplication with vault storage acknowledged.Previous reviews identified significant overlap with
go/pkg/vault/storage/s3.go. The team has agreed to extract shared logic into a common package in a follow-up.go/apps/ctrl/config.go (1)
139-147: Missing validation for Depot configuration fields.The Depot backend requires
APIUrl,RegistryUrl, andUsername(as seen in depot/service.go), but the validation only checksAccessToken. Add validation for these required fields to prevent runtime errors.Apply this diff:
case BuildBackendDepot: return assert.All( + assert.NotEmpty(c.Depot.APIUrl, "Depot API URL is required when using Depot backend"), + assert.NotEmpty(c.Depot.RegistryUrl, "Depot registry URL is required when using Depot backend"), + assert.NotEmpty(c.Depot.Username, "Depot username is required when using Depot backend"), + assert.NotEmpty(c.Depot.AccessToken, "Depot access token is required when using Depot backend"), assert.NotEmpty(c.BuildS3.URL, "build S3 URL is required when using Depot backend"), assert.NotEmpty(c.BuildS3.Bucket, "build S3 bucket is required when using Depot backend"), assert.NotEmpty(c.BuildS3.AccessKeyID, "build S3 access key ID is required when using Depot backend"), assert.NotEmpty(c.BuildS3.AccessKeySecret, "build S3 access key secret is required when using Depot backend"), - assert.NotEmpty(c.Depot.AccessToken, "Depot access token is required when using Depot backend"), )
🧹 Nitpick comments (1)
QUICKSTART-DEPLOY.md (1)
1-278: Documentation is comprehensive and well-structured.The quickstart guide clearly explains the new build backend flow, configuration options, and troubleshooting steps. The Depot and Docker backend options are well-documented with concrete examples.
Optional: Consider adding language specifiers to fenced code blocks (lines 133, 141, 219) for better syntax highlighting, though this is a minor documentation style preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
QUICKSTART-DEPLOY.md(4 hunks)deployment/docker-compose.yaml(2 hunks)go/apps/ctrl/config.go(4 hunks)go/apps/ctrl/run.go(4 hunks)go/apps/ctrl/services/build/backend/depot/create_build.go(1 hunks)go/apps/ctrl/services/build/backend/docker/create_build.go(1 hunks)go/apps/ctrl/services/build/backend/docker/generate_upload_url.go(1 hunks)go/apps/ctrl/services/build/storage/s3.go(1 hunks)go/cmd/ctrl/main.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go/cmd/ctrl/main.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-08T14:55:11.981Z
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: deployment/docker-compose.yaml:179-184
Timestamp: 2025-08-08T14:55:11.981Z
Learning: In deployment/docker-compose.yaml (development only), MinIO is configured with API on 3902 and console on 3903; ports should map 3902:3902 and 3903:3903 to match MINIO_API_PORT_NUMBER and MINIO_CONSOLE_PORT_NUMBER.
Applied to files:
deployment/docker-compose.yaml
📚 Learning: 2025-09-15T18:12:01.503Z
Learnt from: mcstepp
PR: unkeyed/unkey#3952
File: go/apps/ctrl/run.go:212-213
Timestamp: 2025-09-15T18:12:01.503Z
Learning: In Go connect-go generated handlers, mux.Handle can accept New*ServiceHandler functions directly even though they return (string, http.Handler) tuples. The pattern mux.Handle(ctrlv1connect.New*ServiceHandler(...)) is valid and compiles successfully in unkey codebase.
Applied to files:
go/apps/ctrl/run.go
🧬 Code graph analysis (6)
go/apps/ctrl/services/build/backend/docker/generate_upload_url.go (3)
go/apps/ctrl/services/build/backend/docker/service.go (1)
Docker(11-17)internal/proto/generated/ctrl/v1/build_pb.ts (2)
GenerateUploadURLRequest(94-101)GenerateUploadURLResponse(114-135)go/gen/proto/ctrl/v1/build.pb.go (6)
GenerateUploadURLRequest(152-157)GenerateUploadURLRequest(170-170)GenerateUploadURLRequest(185-187)GenerateUploadURLResponse(196-203)GenerateUploadURLResponse(216-216)GenerateUploadURLResponse(231-233)
go/apps/ctrl/config.go (3)
go/apps/ctrl/services/build/storage/s3.go (1)
S3Config(25-32)go/pkg/vault/storage/s3.go (1)
S3Config(25-31)go/apps/ctrl/services/build/backend/depot/service.go (1)
Depot(11-21)
go/apps/ctrl/services/build/backend/docker/create_build.go (2)
go/apps/ctrl/services/build/backend/docker/service.go (1)
Docker(11-17)go/gen/proto/ctrl/v1/build.pb.go (6)
CreateBuildRequest(24-32)CreateBuildRequest(45-45)CreateBuildRequest(60-62)CreateBuildResponse(92-99)CreateBuildResponse(112-112)CreateBuildResponse(127-129)
go/apps/ctrl/services/build/backend/depot/create_build.go (4)
go/apps/ctrl/services/build/backend/depot/service.go (1)
Depot(11-21)go/gen/proto/ctrl/v1/build.pb.go (6)
CreateBuildRequest(24-32)CreateBuildRequest(45-45)CreateBuildRequest(60-62)CreateBuildResponse(92-99)CreateBuildResponse(112-112)CreateBuildResponse(127-129)go/pkg/db/project_update_depot_id.sql_generated.go (1)
UpdateProjectDepotIDParams(21-25)go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/apps/ctrl/run.go (7)
go/gen/proto/ctrl/v1/ctrlv1connect/build.connect.go (2)
BuildServiceClient(45-48)NewBuildServiceHandler(103-127)go/apps/ctrl/config.go (5)
BuildBackend(11-11)BuildBackendDocker(15-15)S3Config(18-24)Config(68-129)BuildBackendDepot(14-14)go/apps/ctrl/services/build/storage/s3.go (2)
NewS3(34-111)S3Config(25-32)go/apps/ctrl/services/build/backend/docker/service.go (2)
New(26-34)Config(19-24)go/apps/ctrl/services/build/backend/depot/service.go (3)
New(34-46)Config(23-32)Depot(11-21)go/apps/ctrl/services/deployment/service.go (2)
New(27-36)Config(19-25)go/apps/ctrl/workflows/deploy/service.go (2)
New(57-66)Config(36-54)
go/apps/ctrl/services/build/storage/s3.go (3)
go/apps/ctrl/config.go (1)
S3Config(18-24)go/pkg/vault/storage/s3.go (2)
S3Config(25-31)NewS3(33-70)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
🪛 Gitleaks (8.28.0)
QUICKSTART-DEPLOY.md
[high] 212-213: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 markdownlint-cli2 (0.18.1)
QUICKSTART-DEPLOY.md
133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
163-163: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
169-169: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
219-219: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
deployment/docker-compose.yaml (1)
170-170: LGTM! Port mapping corrected.The port mapping now correctly aligns with MINIO_CONSOLE_PORT_NUMBER.
Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (10)
go/apps/ctrl/services/build/backend/docker/generate_upload_url.go (1)
13-42: Duplication acknowledged; consider collision-resistant key generation.The duplication with
depot/generate_upload_url.gois already tracked for consolidation. Additionally, usingtime.Now().UnixNano()for uniqueness could theoretically have collisions if multiple requests arrive in rapid succession (though unlikely in practice).As mentioned in past reviews, consolidate the shared logic into a common helper. For additional robustness, consider using a UUID or combining timestamp with a random component:
+ "crypto/rand" + "encoding/hex" // Generate unique S3 key for this build context + randomBytes := make([]byte, 8) + rand.Read(randomBytes) contextKey := fmt.Sprintf("%s/%d-%s.tar.gz", req.Msg.UnkeyProjectId, time.Now().UnixNano(), + hex.EncodeToString(randomBytes)) - time.Now().UnixNano())This eliminates theoretical collision risk while maintaining chronological ordering.
go/apps/ctrl/services/build/backend/docker/create_build.go (2)
31-36: Add validation for DeploymentId.The validation checks
ContextKeyandUnkeyProjectIdbut omitsDeploymentId, which is used in the image name construction (line 70-73). Missing this validation could lead to ambiguous image names or runtime errors.Apply this diff to add the validation:
if err := assert.All( assert.NotEmpty(req.Msg.ContextKey, "contextKey is required"), assert.NotEmpty(req.Msg.UnkeyProjectId, "unkeyProjectID is required"), + assert.NotEmpty(req.Msg.DeploymentId, "deploymentId is required"), ); err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, err) }
68-73: Image name normalization could cause collisions.Lowercasing the concatenated image name can cause unintended collisions if
UnkeyProjectIdandDeploymentIddiffer only in case.Consider one of these approaches:
- Validate that inputs are already lowercase and reject mixed-case identifiers
- Include a deterministic hash component to preserve uniqueness
Example validation approach:
+ if strings.ToLower(req.Msg.UnkeyProjectId) != req.Msg.UnkeyProjectId { + return nil, connect.NewError(connect.CodeInvalidArgument, + fmt.Errorf("unkeyProjectId must be lowercase")) + } + if strings.ToLower(req.Msg.DeploymentId) != req.Msg.DeploymentId { + return nil, connect.NewError(connect.CodeInvalidArgument, + fmt.Errorf("deploymentId must be lowercase")) + } imageName := strings.ToLower(fmt.Sprintf("%s-%s", req.Msg.UnkeyProjectId, req.Msg.DeploymentId, ))go/apps/krane/config.go (1)
62-77: Add validation for depot and registry configuration.The new fields (
DepotToken,RegistryURL,RegistryUsername,RegistryPassword) lack validation in theValidate()method. If these fields are required for specific backend configurations, their absence could cause runtime failures.Based on coding guidelines, add validation to ensure required fields are present when depot backend is selected:
func (c Config) Validate() error { if c.Backend == Docker && c.DockerSocketPath == "" { return fmt.Errorf("--docker-socket is required when backend is docker") } + + // Validate registry credentials are complete if any are provided + registryFieldsSet := 0 + if c.RegistryURL != "" { registryFieldsSet++ } + if c.RegistryUsername != "" { registryFieldsSet++ } + if c.RegistryPassword != "" { registryFieldsSet++ } + if registryFieldsSet > 0 && registryFieldsSet < 3 { + return fmt.Errorf("all registry fields (url, username, password) must be provided together") + } return nil }Note: If there's a
BuildBackendfield indicating depot usage, add validation thatDepotTokenis required whenBuildBackend == BuildBackendDepot.go/apps/ctrl/services/build/storage/s3.go (1)
33-107: Deduplicate S3 init with vault storage; extract shared helperThis duplicates
go/pkg/vault/storage/s3.go. Extract common endpoint resolver, config loading, bucket creation, and presigner setup into a shared package (e.g.,go/pkg/storage/s3) and use it in both places.go/apps/ctrl/services/build/backend/depot/create_build.go (2)
54-56: Default platform should be amd64 and configurable (avoid hardcoding arm64)Hardcoding arm64 breaks common x86_64 environments. Default to amd64; make it configurable later.
Apply:
- const architecture = "arm64" + // TODO: make configurable via config/request; default to the most common architecture + const architecture = "amd64" platform := fmt.Sprintf("linux/%s", architecture) @@ - buildkit, buildErr := machine.Acquire(ctx, buildResp.ID, buildResp.Token, architecture) + buildkit, buildErr := machine.Acquire(ctx, buildResp.ID, buildResp.Token, architecture)Also applies to: 113-121
226-233: Fix logging key/value mismatch and use string for depot_project_idPairs are misaligned and
DepotProjectIDis asql.NullString. Log.Stringand includeunkey_project_id.Apply:
- s.logger.Info( - "Returning existing depot project", - "depot_project_id", - project.DepotProjectID, - "unkey_project_id", - "project_name", - projectName, - ) + s.logger.Info("Returning existing depot project", + "depot_project_id", project.DepotProjectID.String, + "unkey_project_id", unkeyProjectID, + "project_name", projectName, + )go/apps/ctrl/run.go (3)
185-210: Compile-time mismatch: handler assigned to client variable and passed as handler laterYou assign a server handler to a
BuildServiceClientand pass that same var toNewBuildServiceHandler(which expects a handler). This will not compile. Keep distincthandlerandclientvars; wrap the handler with a thin in-process client.Apply:
- var buildService ctrlv1connect.BuildServiceClient + var buildHandler ctrlv1connect.BuildServiceHandler + var buildClient ctrlv1connect.BuildServiceClient switch cfg.BuildBackend { case BuildBackendDocker: - buildService = docker.New(docker.Config{ + buildHandler = docker.New(docker.Config{ DB: database, Logger: logger, - Storage: buildStorage, + Storage: buildS3, }) logger.Info("Using Docker build backend", "presign_url", cfg.BuildS3.ExternalURL) case BuildBackendDepot: - buildService = depot.New(depot.Config{ + buildHandler = depot.New(depot.Config{ InstanceID: cfg.InstanceID, DB: database, APIUrl: cfg.Depot.APIUrl, RegistryUrl: cfg.Depot.RegistryUrl, Username: cfg.Depot.Username, AccessToken: cfg.Depot.AccessToken, Logger: logger, - Storage: buildStorage, + Storage: buildS3, }) logger.Info("Using Depot build backend") default: return fmt.Errorf("unknown build backend: %s (must be 'docker' or 'depot')", cfg.BuildBackend) } + // Local in-process client adapter + buildClient = buildHandlerClient{h: buildHandler}Add this adapter near the bottom/top of the file:
type buildHandlerClient struct{ h ctrlv1connect.BuildServiceHandler } func (c buildHandlerClient) CreateBuild(ctx context.Context, req *connect.Request[ctrlv1.CreateBuildRequest]) (*connect.Response[ctrlv1.CreateBuildResponse], error) { return c.h.CreateBuild(ctx, req) } func (c buildHandlerClient) GenerateUploadURL(ctx context.Context, req *connect.Request[ctrlv1.GenerateUploadURLRequest]) (*connect.Response[ctrlv1.GenerateUploadURLResponse], error) { return c.h.GenerateUploadURL(ctx, req) }Based on learnings
212-214: Pass the client to workflows; keep handler for HTTP muxEnsure the deploy workflow gets the client, not the handler.
Apply:
- restateClient := restateIngress.NewClient(cfg.Restate.IngressURL) + restateClient := restateIngress.NewClient(cfg.Restate.IngressURL) @@ - BuildClient: buildService, + BuildClient: buildClient,Also applies to: 221-223
313-321: Use the handler in HTTP mux; pass the client into servicesSwap
buildServiceforbuildHandlerin mux; passbuildClientinto deployment service.Apply:
- mux.Handle(ctrlv1connect.NewBuildServiceHandler(buildService, connectOptions...)) + mux.Handle(ctrlv1connect.NewBuildServiceHandler(buildHandler, connectOptions...)) @@ - BuildService: buildService, + BuildService: buildClient,
🧹 Nitpick comments (11)
go/Dockerfile.tilt (2)
1-1: Consider pinning the Alpine version for reproducible builds.Using
alpine:latestintroduces non-deterministic builds as the base image can change over time, potentially causing unexpected breakages or behavior changes.Apply this diff to pin to a specific Alpine version:
-FROM alpine:latest +FROM alpine:3.21
9-10: The chmod command may be redundant.If the source binary in
bin/unkeyalready has executable permissions, thechmod +xcommand is unnecessary. COPY preserves file permissions by default.Verify if the source binary already has executable permissions. If so, you can remove these lines:
-# Make it executable -RUN chmod +x /unkey -go/apps/ctrl/services/build/backend/docker/create_build.go (1)
109-123: Only the first build error is captured.The loop breaks on the first error encountered, which means subsequent errors in the build stream won't be logged. This could hide valuable debugging information about cascading failures.
Consider collecting all errors or at least logging them before breaking:
for scanner.Scan() { var resp dockerBuildResponse if err := json.Unmarshal(scanner.Bytes(), &resp); err != nil { continue } if resp.Error != "" { - buildError = fmt.Errorf("%s", resp.ErrorDetail.Message) + if buildError == nil { + buildError = fmt.Errorf("%s", resp.ErrorDetail.Message) + } d.logger.Error("Build failed", "error", resp.ErrorDetail.Message, "image_name", imageName, "unkey_project_id", req.Msg.UnkeyProjectId) - break + // Continue to log all errors } }go/apps/krane/backend/kubernetes/create_deployment.go (1)
150-160: Consider making registry configuration values configurable.The registry URL prefix (
registry.depot.dev/) and secret name (depot-registry) are hardcoded. If additional registries or different secret names are needed in the future, this will require code changes.Consider accepting these as configuration parameters in the
k8sstruct or as part of the deployment request. This would allow for more flexible registry support without code changes.go/k8s/manifests/ctrl.yaml (1)
81-82: Consider making the S3 bucket name configurable.The bucket name
build-contextsis hardcoded. If different environments or tenants require separate buckets, this will need to be changed in the manifest.Consider moving this to the
depot-credentialssecret or a ConfigMap to allow environment-specific configuration without manifest changes.deployment/docker-compose.yaml (1)
289-291: Registry and build configuration is duplicated across services.The registry, build S3, and depot configuration blocks appear in multiple services (krane, ctrl, restate). While docker-compose doesn't have built-in config inheritance, this duplication increases maintenance burden.
Consider using YAML anchors to reduce duplication:
x-registry-config: ®istry-config UNKEY_REGISTRY_URL: "${UNKEY_REGISTRY_URL:-}" UNKEY_REGISTRY_USERNAME: "${UNKEY_REGISTRY_USERNAME:-}" UNKEY_REGISTRY_PASSWORD: "${UNKEY_REGISTRY_PASSWORD:-}" x-build-config: &build-config UNKEY_BUILD_S3_URL: "${UNKEY_BUILD_S3_URL:-http://s3:3902}" UNKEY_BUILD_S3_EXTERNAL_URL: "${UNKEY_BUILD_S3_EXTERNAL_URL:-}" UNKEY_BUILD_S3_BUCKET: "build-contexts" # ... etc services: krane: environment: <<: *registry-config # other krane-specific vars ctrl: environment: <<: *build-config # other ctrl-specific varsAlso applies to: 362-380
go/apps/krane/backend/docker/service.go (2)
55-56: Docker host string may be malformed (extra slash) depending on SocketPath
fmt.Sprintf("unix:///%s", cfg.SocketPath)yieldsunix:////var/run/docker.sockwhenSocketPathstarts with “/”. Normalize to exactlyunix:///….Apply:
- client.WithHost(fmt.Sprintf("unix:///%s", cfg.SocketPath)), + // Ensure exactly one leading slash after unix:// + client.WithHost(func() string { + p := cfg.SocketPath + if strings.HasPrefix(p, "/") { + return "unix://" + p // -> unix:///var/run/docker.sock + } + return "unix:///" + p + }()),(Import "strings".)
106-139: Differentiate “not found” from other inspect errors; avoid masking real failuresTreating any
ImageInspecterror as “missing” hides permission/daemon errors. Use Docker’serrdefs.IsNotFound.Apply:
- _, err := d.client.ImageInspect(ctx, imageName) - if err == nil { + _, err := d.client.ImageInspect(ctx, imageName) + if err == nil { d.logger.Info("using existing local image", "image", imageName) return nil - } + } + if !errdefs.IsNotFound(err) { + return fmt.Errorf("failed to inspect image %q: %w", imageName, err) + }(Import "github.com/docker/docker/errdefs".)
go/apps/ctrl/services/build/backend/depot/create_build.go (1)
237-252: Region is hardcoded; consider making it configurable
RegionId: "us-east-1"might be wrong for some accounts. Read from config/env or infer from project/tenant settings.go/apps/ctrl/run.go (1)
173-180: Variable name shadows import; prefer a distinct name
buildStorage := buildStorage.NewS3(...)shadows the imported package alias. Rename the variable (e.g.,buildS3) to avoid confusion.Apply:
- buildStorage, err := buildStorage.NewS3(buildStorage.S3Config{ + buildS3, err := buildStorage.NewS3(buildStorage.S3Config{…and update subsequent references in this function.
go/apps/ctrl/services/build/storage/s3.go (1)
86-100: Consider adding retry options and (if needed) path-style addressing
- Align with vault S3 client by enabling standard retry mode and limited attempts.
- If targeting MinIO/R2 variants that require path-style, set
awsS3.NewFromConfig(cfg, func(o *awsS3.Options){ o.UsePathStyle = true }).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
deployment/docker-compose.yaml(3 hunks)deployment/setup-build-backend.sh(1 hunks)go/Dockerfile.tilt(1 hunks)go/apps/ctrl/run.go(3 hunks)go/apps/ctrl/services/build/backend/depot/create_build.go(1 hunks)go/apps/ctrl/services/build/backend/docker/create_build.go(1 hunks)go/apps/ctrl/services/build/backend/docker/generate_upload_url.go(1 hunks)go/apps/ctrl/services/build/storage/s3.go(1 hunks)go/apps/krane/backend/docker/create_deployment.go(1 hunks)go/apps/krane/backend/docker/service.go(2 hunks)go/apps/krane/backend/kubernetes/create_deployment.go(2 hunks)go/apps/krane/config.go(1 hunks)go/apps/krane/run.go(1 hunks)go/cmd/ctrl/main.go(2 hunks)go/cmd/krane/main.go(3 hunks)go/k8s/manifests/ctrl.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- go/apps/krane/backend/docker/create_deployment.go
- go/cmd/krane/main.go
- deployment/setup-build-backend.sh
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-15T18:12:01.503Z
Learnt from: mcstepp
PR: unkeyed/unkey#3952
File: go/apps/ctrl/run.go:212-213
Timestamp: 2025-09-15T18:12:01.503Z
Learning: In Go connect-go generated handlers, mux.Handle can accept New*ServiceHandler functions directly even though they return (string, http.Handler) tuples. The pattern mux.Handle(ctrlv1connect.New*ServiceHandler(...)) is valid and compiles successfully in unkey codebase.
Applied to files:
go/apps/ctrl/run.go
📚 Learning: 2025-08-08T14:55:11.981Z
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: deployment/docker-compose.yaml:179-184
Timestamp: 2025-08-08T14:55:11.981Z
Learning: In deployment/docker-compose.yaml (development only), MinIO is configured with API on 3902 and console on 3903; ports should map 3902:3902 and 3903:3903 to match MINIO_API_PORT_NUMBER and MINIO_CONSOLE_PORT_NUMBER.
Applied to files:
deployment/docker-compose.yaml
🧬 Code graph analysis (8)
go/apps/ctrl/services/build/backend/docker/create_build.go (3)
go/apps/ctrl/services/build/backend/docker/service.go (1)
Docker(11-17)internal/proto/generated/ctrl/v1/build_pb.ts (2)
CreateBuildRequest(21-47)CreateBuildResponse(60-81)go/gen/proto/ctrl/v1/build.pb.go (6)
CreateBuildRequest(24-32)CreateBuildRequest(45-45)CreateBuildRequest(60-62)CreateBuildResponse(92-99)CreateBuildResponse(112-112)CreateBuildResponse(127-129)
go/apps/ctrl/run.go (7)
go/apps/ctrl/services/build/storage/s3.go (2)
NewS3(33-107)S3Config(24-31)go/apps/ctrl/config.go (5)
S3Config(18-24)BuildBackend(11-11)BuildBackendDocker(15-15)Config(68-129)BuildBackendDepot(14-14)go/gen/proto/ctrl/v1/ctrlv1connect/build.connect.go (1)
BuildServiceClient(45-48)go/apps/ctrl/services/build/backend/depot/service.go (3)
New(34-46)Config(23-32)Depot(11-21)go/apps/ctrl/services/build/backend/docker/service.go (2)
New(26-34)Config(19-24)go/apps/ctrl/workflows/deploy/service.go (2)
New(57-66)Config(36-54)go/apps/ctrl/services/deployment/service.go (2)
New(27-36)Config(19-25)
go/cmd/ctrl/main.go (5)
go/pkg/cli/flag.go (4)
String(419-451)Default(364-416)EnvVar(320-339)Required(298-317)go/apps/ctrl/config.go (3)
BuildBackend(11-11)S3Config(18-24)DepotConfig(57-66)go/apps/ctrl/services/build/storage/s3.go (1)
S3Config(24-31)go/pkg/vault/storage/s3.go (1)
S3Config(25-31)go/apps/ctrl/services/build/backend/depot/service.go (1)
Depot(11-21)
go/apps/ctrl/services/build/backend/docker/generate_upload_url.go (3)
go/apps/ctrl/services/build/backend/docker/service.go (1)
Docker(11-17)internal/proto/generated/ctrl/v1/build_pb.ts (2)
GenerateUploadURLRequest(94-101)GenerateUploadURLResponse(114-135)go/gen/proto/ctrl/v1/build.pb.go (6)
GenerateUploadURLRequest(152-157)GenerateUploadURLRequest(170-170)GenerateUploadURLRequest(185-187)GenerateUploadURLResponse(196-203)GenerateUploadURLResponse(216-216)GenerateUploadURLResponse(231-233)
go/apps/krane/run.go (2)
go/apps/krane/backend/docker/service.go (2)
New(49-101)Config(33-38)go/apps/krane/config.go (1)
Config(24-103)
go/apps/ctrl/services/build/storage/s3.go (2)
go/apps/ctrl/config.go (1)
S3Config(18-24)go/pkg/vault/storage/s3.go (2)
S3Config(25-31)NewS3(33-70)
go/apps/ctrl/services/build/backend/depot/create_build.go (3)
go/apps/ctrl/services/build/backend/depot/service.go (1)
Depot(11-21)go/gen/proto/ctrl/v1/build.pb.go (6)
CreateBuildRequest(24-32)CreateBuildRequest(45-45)CreateBuildRequest(60-62)CreateBuildResponse(92-99)CreateBuildResponse(112-112)CreateBuildResponse(127-129)go/pkg/db/project_update_depot_id.sql_generated.go (1)
UpdateProjectDepotIDParams(21-25)
go/apps/krane/backend/docker/service.go (1)
go/apps/krane/config.go (1)
Config(24-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
🔇 Additional comments (6)
go/apps/ctrl/services/build/backend/docker/create_build.go (1)
38-39: Platform is hardcoded to linux/amd64.The architecture is fixed to
amd64, which may not suit all deployment targets. Consider whether multi-architecture support or configurable platform selection is needed.If multi-architecture builds are planned, consider making the platform configurable via the request or service configuration.
deployment/docker-compose.yaml (1)
169-170: Port mapping correction aligns with MinIO configuration.The change from
2903:3903to3903:3903correctly maps the host port to match theMINIO_CONSOLE_PORT_NUMBERenvironment variable. This ensures the MinIO console is accessible on the expected port.Based on learnings
go/apps/krane/run.go (1)
85-90: LGTM! Clean refactor to use configuration struct.The change from passing individual parameters to using a
docker.Configstruct improves maintainability and aligns with the broader registry integration. The addition of registry credentials enables authenticated image pulls from private registries.go/apps/krane/backend/kubernetes/create_deployment.go (1)
152-152: The prefix check is correct and aligns with how images are constructed.Based on verification of the codebase:
- Image names are constructed as
registry.depot.dev/project:tag(line 141 ofcreate_build.goviafmt.Sprintf("%s/%s:%s-%s", s.registryUrl, ...))- Configuration consistently defines
RegistryUrlasregistry.depot.devwithout protocol (config.go, krane/config.go)- No code prepends protocol prefixes to image strings
- The
registry.depot.dev/prefix check at line 152 correctly matches the actual format used throughout the codebaseThe review comment's concern about protocol prefixes is unsupported by evidence.
Likely an incorrect or invalid review comment.
go/cmd/ctrl/main.go (2)
169-177: LGTM: Build S3 config wiringProperly maps internal URL and optional ExternalURL to ctrl config.
179-186: Depot credentials are optional here; ensure backend-specific validationFor
build-backend=depot, validatedepot-api-url,depot-registry-url,depot-username, anddepot-access-tokenare non-empty inconfig.Validate(); otherwiseCreateBuildwill fail at runtime.
3 participants
What does this PR do?
This PR adds a two different ways of building images from scratch for Unkey Deploy. The first one is, like before, a fully local docker setup no additional setup required. Second one is, depot.dev. Depot allows us to build images in isolation so we can safely run untrusted user code. Also, we are using their registry to upload right after a successful build. Depot also helps us build images really fast coz of their monstrous machines and heavy caching.
This PR also gets rid of
docker-imagearg, it was kinda half baked in our code and until we decide how we handle that scenario I wanted to have a cleaner code. Our code now mostly relies ondepot|dockerandS3- any blob storage will work.Flow of execution is as follows:
tarfile. We nowtaruser's code on the client before we upload it to S3, because it's faster to upload zipped files.tarfilecreateDeploymentusing thiscontextKey+DockerfilepathcreateDeploymentcalls thecreateBuildservicecreateBuildrequests apresignedGetUrland passes that todepotdirectly. Sincedepotusesbuildkitand buildkit can work with URLs directly that makes things easy for us.depotis done building and pushing to registry, we let krane service handle the rest.The flow above is for
depot, butdockeris even simpler than that - we just do everything like above but we don't push to depot we build it locally.By the way, there' duplicated piece of code called
generate_build_url.goI wasn't sure where to put that file so I duplicated it for both of our build backend. I'm open to suggestions.P.S: I made a db change to store
depot_project_idfor mapping our clients to depot projects for better isolation. We'll need a schema update.Type of change
How should this be tested?
QUICKSTART-DEPLOYand setup your environment./deploymentsand run./setup-build-backend.sh dockerto test it with docker.R2|S3account if you wanna test depot.dev and probably ask an invite from me or Andreas, so I can give a key to you. coz you will need a depot token.projectID/godirminiolocally read the setup section inQUICKSTART-DEPLOYagain. You probably missed/etc/hostupdate part.Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated