Feat/add short lived docker pull tokens#4486
Conversation
* fix: update API key status label from 'Potential issues' to 'High Error Rate' Changed the validation-issues status label to more clearly communicate that the key is receiving invalid requests, rather than implying the API or key itself is broken. Changes: - Label: 'Potential issues' → 'High Error Rate' - Tooltip: Updated to clarify that requests are invalid (rate limited, unauthorized, etc.) rather than suggesting system issues Fixes #4474 * chore: apply biome formatting * fix: update status label to 'Elevated Rejections' per review --------- Co-authored-by: CodeReaper <148160799+MichaelUnkey@users.noreply.github.com>
* removed un used components * updated members refs --------- Co-authored-by: James P <james@unkey.dev> Co-authored-by: Andreas Thomas <dev@chronark.com>
Flo4604
force-pushed
the
feat/add-short-lived-docker-pull-tokens
branch
from
cf43448 to
e8e70be
Compare
Flo4604
requested review from
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
go/apps/preflight/config.go (1)
13-13: Consider adding validation forDepotTokenwhen Depot registry is used.The
DepotTokenfield is added but not validated inValidate(). If Depot-based registry credentials are required for the preflight service to function correctly, consider adding conditional validation. However, if it's optional (only needed when pulling from Depot registries), the current approach is acceptable.If validation is needed:
return assert.All( assert.NotEmpty(c.TLSCertFile, "tls-cert-file is required"), assert.NotEmpty(c.TLSKeyFile, "tls-key-file is required"), assert.NotEmpty(c.UnkeyEnvImage, "unkey-env-image is required"), assert.NotEmpty(c.KraneEndpoint, "krane-endpoint is required"), + // Add if Depot token is required: + // assert.NotEmpty(c.DepotToken, "depot-token is required"), )go/apps/preflight/run.go (1)
93-98: Consider handling errors from the background cleanup goroutine.The cleanup service is started as a fire-and-forget goroutine. If
Start()returns an error or panics, it will be silently lost. Consider wrapping with error logging:- go cleanupService.Start(ctx) + go func() { + if err := cleanupService.Start(ctx); err != nil { + logger.Error("cleanup service failed", "error", err) + } + }()Alternatively, if
Start()is designed to run indefinitely and only returns on context cancellation, this pattern is acceptable but worth documenting.go/apps/preflight/internal/services/registry/credentials/depot.go (2)
32-47: Consider configuring HTTP client timeout for production resilience.The constructor uses
http.DefaultClient, which has no timeout and can block indefinitely if the Depot API is unresponsive. For production resilience, consider creating a client with appropriate timeouts.Apply this diff to add timeout configuration:
func NewDepot(cfg *DepotConfig) *Depot { + httpClient := &http.Client{ + Timeout: 30 * time.Second, + } return &Depot{ logger: cfg.Logger, buildClient: cliv1connect.NewBuildServiceClient( - http.DefaultClient, + httpClient, depotAPIURL, connect.WithInterceptors(connect.UnaryInterceptorFunc(func(next connect.UnaryFunc) connect.UnaryFunc { return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) { req.Header().Set("Authorization", "Bearer "+cfg.Token) return next(ctx, req) } })), ), } }
82-108: Simplify the validation check.The condition
len(parts) == 0on Line 103 is unreachable becausestrings.SplitN(path, "/", 2)always returns at least one element (even for an empty string). The checkparts[0] == ""is sufficient.Apply this diff:
// The project ID is the first path segment parts := strings.SplitN(path, "/", 2) - if len(parts) == 0 || parts[0] == "" { + if parts[0] == "" { return "", fmt.Errorf("could not extract project ID from image: %s", image) }go/apps/ctrl/workflows/deploy/deploy_handler.go (1)
133-146: Consider renaming the local variable to avoid shadowing.The local variable
reqon Line 133 shadows the outerreqparameter (Line 24), which could cause confusion. Consider using a more descriptive name likedeploymentReqorkraneReq.Apply this diff:
_, err = restate.Run(ctx, func(stepCtx restate.RunContext) (restate.Void, error) { - req := &kranev1.DeploymentRequest{ + deploymentReq := &kranev1.DeploymentRequest{ Namespace: hardcodedNamespace, DeploymentId: deployment.ID, Image: dockerImage, Replicas: 1, CpuMillicores: 512, MemorySizeMib: 512, EnvironmentSlug: environment.Slug, EncryptedSecretsBlob: encryptedSecretsBlob, EnvironmentId: deployment.EnvironmentID, BuildId: buildID, } _, err = w.krane.CreateDeployment(stepCtx, connect.NewRequest(&kranev1.CreateDeploymentRequest{ - Deployment: req, + Deployment: deploymentReq, }))go/apps/preflight/internal/services/mutator/patch.go (1)
10-23: Consider validating ImagePullPolicy value.Line 14 casts
m.unkeyEnvImagePullPolicytocorev1.PullPolicywithout validation. If an invalid value is provided in configuration, this could cause issues. Consider validating the policy value during Mutator initialization or at this point.Example validation in the constructor:
func New(cfg Config) *Mutator { policy := corev1.PullPolicy(cfg.UnkeyEnvImagePullPolicy) if policy != corev1.PullAlways && policy != corev1.PullNever && policy != corev1.PullIfNotPresent { // log warning or return error } // ... rest of constructor }go/k8s/manifests/preflight.yaml (1)
80-84: Document the depot-credentials secret requirement.The configuration references a
depot-credentialssecret that must be created before deploying preflight. Consider adding a comment in the manifest explaining how to create this secret, similar to the TLS secret guidance on lines 39-41.Add a comment block above the env section:
+ # Depot credentials secret must be created before deployment: + # kubectl create secret generic depot-credentials -n unkey --from-literal=token=YOUR_DEPOT_TOKEN - name: UNKEY_DEPOT_TOKEN valueFrom: secretKeyRef: name: depot-credentials key: tokengo/apps/preflight/internal/services/mutator/mutator.go (1)
301-319: Consider extracting shared validation logic.The
isSecretValidmethod is duplicated in bothmutator/mutator.goandcleanup/cleanup.gowith identical implementations. Consider extracting this to a shared location to follow the DRY principle.For example, create a shared package:
// go/apps/preflight/internal/services/registry/credentials/validation.go package credentials func IsSecretValid(secret *corev1.Secret, expiresAtAnnotation string) bool { // shared implementation }Then both mutator and cleanup can import and use this shared function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
apps/dashboard/gen/proto/krane/v1/deployment_pb.tsis excluded by!**/gen/**go/gen/proto/krane/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx(2 hunks)apps/engineering/content/docs/architecture/services/ingress.mdx(8 hunks)go/Tiltfile(5 hunks)go/apps/ctrl/workflows/certificate/service.go(1 hunks)go/apps/ctrl/workflows/deploy/deploy_handler.go(3 hunks)go/apps/krane/backend/kubernetes/deployment_create.go(1 hunks)go/apps/preflight/config.go(2 hunks)go/apps/preflight/internal/services/cleanup/cleanup.go(1 hunks)go/apps/preflight/internal/services/mutator/config.go(2 hunks)go/apps/preflight/internal/services/mutator/mutator.go(1 hunks)go/apps/preflight/internal/services/mutator/patch.go(3 hunks)go/apps/preflight/internal/services/registry/credentials/credentials.go(1 hunks)go/apps/preflight/internal/services/registry/credentials/depot.go(1 hunks)go/apps/preflight/internal/services/registry/registry.go(4 hunks)go/apps/preflight/routes/mutate/handler.go(1 hunks)go/apps/preflight/run.go(6 hunks)go/apps/secrets-webhook/internal/services/mutator/mutator.go(0 hunks)go/cmd/preflight/main.go(2 hunks)go/cmd/run/main.go(4 hunks)go/cmd/unkey-env/Dockerfile(2 hunks)go/go.mod(1 hunks)go/k8s/manifests/preflight.yaml(6 hunks)go/proto/krane/v1/deployment.proto(1 hunks)
💤 Files with no reviewable changes (1)
- go/apps/secrets-webhook/internal/services/mutator/mutator.go
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-07-28T19:42:37.047Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/app/(app)/projects/page.tsx:74-81
Timestamp: 2025-07-28T19:42:37.047Z
Learning: In apps/dashboard/app/(app)/projects/page.tsx, the user mcstepp prefers to keep placeholder functions like generateSlug inline during POC/demonstration phases rather than extracting them to utility modules, with plans to refactor later when the feature matures beyond the proof-of-concept stage.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx
📚 Learning: 2025-09-11T14:12:30.570Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/apps/ctrl/run.go:292-299
Timestamp: 2025-09-11T14:12:30.570Z
Learning: In go/apps/ctrl/config.go, the Validate() method already includes validation for Cloudflare API token using assert.NotEmpty() when both c.Acme.Enabled and c.Acme.Cloudflare.Enabled are true, with the error message "cloudflare API token is required when cloudflare is enabled".
Applied to files:
go/apps/preflight/config.go
📚 Learning: 2025-09-11T14:12:30.570Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/apps/ctrl/run.go:292-299
Timestamp: 2025-09-11T14:12:30.570Z
Learning: In go/apps/ctrl/config.go, the Validate() method already includes validation for Cloudflare API token when ACME and Cloudflare are both enabled, checking that cfg.Acme.Cloudflare.ApiToken is not empty.
Applied to files:
go/apps/preflight/config.go
📚 Learning: 2025-09-24T18:57:34.843Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4010
File: QUICKSTART-DEPLOY.md:17-17
Timestamp: 2025-09-24T18:57:34.843Z
Learning: In the Unkey deployment platform, API key environment variables use component-specific naming but share the same secret value: UNKEY_API_KEY for the ctrl service (validator), API_KEY for the CLI client, and CTRL_API_KEY for the dashboard client. The ctrl service acts as the source of truth for validation.
Applied to files:
go/Tiltfilego/cmd/run/main.gogo/k8s/manifests/preflight.yaml
📚 Learning: 2025-09-01T15:10:44.959Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 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/krane/v1/deployment.protogo/apps/krane/backend/kubernetes/deployment_create.goapps/engineering/content/docs/architecture/services/ingress.mdx
📚 Learning: 2025-10-15T10:12:40.810Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 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/krane/v1/deployment.protogo/cmd/unkey-env/Dockerfilego/apps/krane/backend/kubernetes/deployment_create.gogo/apps/ctrl/workflows/deploy/deploy_handler.go
📚 Learning: 2025-07-15T14:45:18.920Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3564
File: go/cmd/cli/commands/deploy/flags.go:17-20
Timestamp: 2025-07-15T14:45:18.920Z
Learning: In the go/cmd/cli/commands/deploy/ directory, ogzhanolguncu prefers to keep potentially temporary features (like UNKEY_DOCKER_REGISTRY environment variable) undocumented in help text if they might be deleted in the future, to avoid documentation churn.
Applied to files:
go/cmd/unkey-env/Dockerfilego/apps/preflight/internal/services/mutator/config.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/cmd/unkey-env/Dockerfilego/apps/preflight/internal/services/mutator/config.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/cmd/unkey-env/Dockerfile
📚 Learning: 2025-07-16T09:18:45.379Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3564
File: go/cmd/cli/commands/deploy/deploy.go:153-158
Timestamp: 2025-07-16T09:18:45.379Z
Learning: In the go/cmd/cli/commands/deploy/ CLI codebase, ogzhanolguncu prefers to allow deployment to continue even when Docker push fails (around lines 153-158 in deploy.go) because the team is working locally and needs this behavior for local development workflows where registry access might not be available.
Applied to files:
go/apps/ctrl/workflows/deploy/deploy_handler.go
📚 Learning: 2025-09-16T19:08:44.174Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3980
File: go/k8s/manifests/dashboard.yaml:41-42
Timestamp: 2025-09-16T19:08:44.174Z
Learning: For local development Kubernetes manifests (typically indicated by paths containing "local" or environment variables like UNKEY_REGION: "local"), hardcoded generic credentials in environment variables are acceptable for convenience. Security recommendations about using Secrets should be reserved for production or staging environments.
Applied to files:
go/cmd/run/main.gogo/k8s/manifests/preflight.yaml
📚 Learning: 2025-09-01T16:08:10.327Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3899
File: go/proto/metald/v1/deployment.proto:45-52
Timestamp: 2025-09-01T16:08:10.327Z
Learning: In the unkey/unkey repository metald service, the host:port pairs in Instance messages within GetDeploymentResponse are consumed by upstream services in a tightly controlled manner, with the host:port being mapped from DNS entries rather than being used for arbitrary client connections.
Applied to files:
go/k8s/manifests/preflight.yaml
🧬 Code graph analysis (8)
go/apps/preflight/internal/services/registry/credentials/credentials.go (1)
go/apps/preflight/internal/services/registry/registry.go (1)
Registry(33-37)
go/apps/preflight/run.go (7)
go/apps/preflight/internal/services/mutator/mutator.go (1)
New(32-43)go/apps/preflight/internal/services/cleanup/cleanup.go (2)
New(30-35)Config(20-23)go/apps/preflight/internal/services/registry/registry.go (3)
New(39-45)Registry(33-37)Config(27-31)go/apps/preflight/internal/services/registry/credentials/credentials.go (2)
Registry(10-17)NewManager(66-68)go/apps/preflight/internal/services/registry/credentials/depot.go (2)
NewDepot(33-47)DepotConfig(27-30)go/apps/preflight/internal/services/mutator/config.go (1)
Config(26-35)go/apps/preflight/config.go (1)
Config(5-14)
go/apps/ctrl/workflows/certificate/service.go (1)
go/apps/ctrl/services/acme/providers/provider.go (1)
Provider(34-39)
go/apps/preflight/internal/services/mutator/config.go (3)
go/apps/preflight/internal/services/registry/credentials/credentials.go (2)
Registry(10-17)Manager(61-63)go/apps/preflight/internal/services/registry/registry.go (1)
Registry(33-37)go/apps/preflight/internal/services/mutator/mutator.go (1)
Mutator(21-30)
go/cmd/preflight/main.go (2)
go/apps/preflight/config.go (1)
Config(5-14)go/apps/preflight/run.go (1)
Run(23-108)
go/cmd/run/main.go (1)
go/cmd/preflight/main.go (1)
Cmd(10-32)
go/apps/preflight/internal/services/cleanup/cleanup.go (2)
go/apps/preflight/internal/services/mutator/mutator.go (1)
New(32-43)go/apps/preflight/internal/services/registry/registry.go (2)
New(39-45)Config(27-31)
go/apps/preflight/internal/services/mutator/mutator.go (4)
go/apps/preflight/internal/services/registry/credentials/credentials.go (2)
Registry(10-17)Manager(61-63)go/apps/preflight/internal/services/registry/registry.go (3)
Registry(33-37)New(39-45)Config(27-31)go/apps/preflight/internal/services/mutator/config.go (1)
Config(26-35)go/apps/preflight/config.go (1)
Config(5-14)
⏰ 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). (6)
- GitHub Check: Build / Build
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Lint Go Code / Lint
- GitHub Check: Test Packages / Test
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
🔇 Additional comments (48)
go/apps/ctrl/workflows/certificate/service.go (1)
27-28: Inline comment formatting aligns with Go idioms. The updated style is clean and follows community conventions.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx (3)
93-98: Formatting changes in useEffect.
102-113: Formatting changes in addEntry function.
208-217: Formatting changes in toast.promise success message.go/cmd/unkey-env/Dockerfile (2)
4-4: Install upx for binary compression.UPX is being added to compress the Go binary. Ensure this doesn't introduce runtime compatibility issues.
15-17: Verify the UPX-compressed binary functions correctly.The binary is being compressed with UPX's best compression and LZMA algorithm. While this reduces image size, Go binaries compressed with UPX can occasionally exhibit unexpected behavior or compatibility issues (especially in constrained environments like scratch containers).
Confirm that:
- The
unkey-envbinary executes correctly after UPX compression in the Kubernetes environment.- All functionality (token fetching, Kubernetes API calls, secret patching) works as expected with the compressed binary.
- No crashes, segfaults, or behavioral changes occur due to compression.
The PR objectives mention "Running a build in Kubernetes should start the pod" as a test, but explicit verification of the compressed binary's correctness in that environment would reduce risk.
go/proto/krane/v1/deployment.proto (1)
38-40: LGTM!The new
build_idfield is correctly added with the next sequential field number (10) and has an appropriate comment explaining its purpose for scoped registry pull tokens. This aligns with the existing pattern wheredeployment_idis received from upstream services. Based on learnings, this follows the service-to-service communication pattern used in this repository.go/apps/preflight/internal/services/registry/credentials/credentials.go (2)
70-80: LGTM!The
GetCredentialsmethod correctly iterates through registries to find a match and returns(nil, nil)when no registry handles the image. The comment clearly documents this behavior, allowing callers to distinguish between "no credentials needed" and actual errors.
9-17: Clean interface design.The
Registryinterface is well-defined with clear method signatures and documentation. ThebuildIDparameter inGetCredentialsenables the build-scoped token pattern described in the PR objectives.apps/engineering/content/docs/architecture/services/ingress.mdx (1)
182-186: LGTM!The code example correctly shows
p.BaseDomainfor the target URL construction. The formatting changes throughout the file are minor whitespace adjustments that don't affect content.go/apps/preflight/routes/mutate/handler.go (1)
13-13: LGTM!The import path update from
secrets-webhooktopreflightis consistent with the service rename described in the PR objectives.go/apps/preflight/config.go (1)
1-1: LGTM!Package rename from
webhooktopreflightaligns with the PR objective of renaming the service.go/go.mod (1)
18-18: Dependency bump verified—no security advisories.The
depot-golibrary is bumped to v0.5.2 (released December 9, 2025) to support the new Depot-based credential retrieval feature. No published security advisories exist for this library, and the version is available on GitHub.go/cmd/run/main.go (1)
12-12: Clean rename from secrets-webhook to preflight.All references consistently updated: import path, command registration, and help text. The description accurately reflects the webhook's expanded responsibility for credentials injection.
Also applies to: 32-32, 47-47, 59-59
go/cmd/preflight/main.go (2)
28-29: Verify ifdepot-tokenshould be strictly required.The flag is marked
cli.Required(), butrun.go(lines 42-48) conditionally initializes Depot credentials only whencfg.DepotToken != "". This suggests the runtime logic treats it as optional.If Depot integration is mandatory for all deployments,
Required()is correct. If it should be optional (e.g., for local development or alternative registries), consider removingRequired()or adding a default empty value. Please clarify the intended behavior.
34-50: LGTM - Configuration wiring is correct.The action function properly maps CLI flags to the config struct and validates before execution. The double validation (line 46 here and line 24 in
run.go) provides defense-in-depth for both CLI and programmatic callers.go/apps/preflight/run.go (2)
40-55: Credentials manager setup is well-structured.The conditional Depot initialization with logging provides good observability. The variadic
NewManager(registries...)pattern gracefully handles the case where no registries are configured.
71-80: Mutator configuration is complete.All required dependencies are properly wired. The explicit passing of both
RegistryandCredentialsto the Mutator (even though Registry also holds credentials internally) provides flexibility for future use cases.go/Tiltfile (2)
261-310: Comprehensive rename from secrets-webhook to preflight.All references updated consistently:
- TLS certificate generation targets
preflight.unkey.svc- Docker image renamed to
unkey-preflight:latest- Entrypoint correctly uses
run preflight- Manifest path updated to
preflight.yaml- Dependencies and resource names aligned
The dependency on
krane(line 302) is appropriate since preflight communicates with krane for secrets retrieval.
36-36: Service activation flags consistently updated.The
start_preflightflag is properly integrated into the service activation logic, build conditions, and active services list.Also applies to: 148-148, 378-378
go/apps/krane/backend/kubernetes/deployment_create.go (1)
102-105: Build ID annotation enables preflight webhook integration, but handle empty values gracefully.The
unkey.com/build-idannotation correctly propagates the build identity to pods, allowing the preflight webhook to fetch scoped Depot pull tokens. The implementation passes buildID directly to Depot's GetPullToken API (depot.go:71), even when the value is an empty string (since proto3 fields default to empty string when unset). Confirm with the Depot API team that empty buildID values are handled appropriately—either by returning unscoped tokens or failing gracefully—to ensure deployments without explicit build IDs do not experience unexpected credential failures.go/apps/preflight/internal/services/registry/credentials/depot.go (3)
49-51: LGTM!The prefix-based matching logic correctly identifies Depot registry images.
53-66: LGTM!The credential retrieval flow is well-structured with proper error handling and documentation.
68-80: LGTM!The token retrieval implementation is straightforward with proper error wrapping.
go/apps/ctrl/workflows/deploy/deploy_handler.go (2)
72-99: LGTM!The refactoring to return
BuildResponseinstead of a plain string enables proper propagation of build metadata (includingbuildID) to downstream components.
98-99: LGTM!The extraction of image name and build ID from the build response is straightforward and correct.
go/apps/preflight/internal/services/mutator/config.go (3)
26-35: LGTM!The Config struct properly exposes dependencies needed by the Mutator, enabling clean dependency injection.
42-44: LGTM!Moving annotation handling to the Mutator improves encapsulation.
46-64: LGTM!The refactoring correctly uses the Mutator's internal helpers instead of Config methods.
go/apps/preflight/internal/services/mutator/patch.go (1)
36-124: LGTM!The addition of the
buildIDparameter enables build-scoped credential retrieval, properly integrating with the new token-based authentication flow.go/apps/preflight/internal/services/cleanup/cleanup.go (5)
14-18: LGTM!The cleanup interval (1 hour) appropriately aligns with the token TTL (55 minutes with buffer), ensuring timely cleanup without excessive API calls.
20-35: LGTM!The service structure follows standard patterns for dependency injection.
37-52: LGTM!The background loop correctly performs an initial cleanup before waiting for the first tick, ensuring expired secrets are cleaned up promptly on startup.
54-89: LGTM!The cleanup logic is resilient, continuing to process remaining secrets even when individual deletions fail. Cross-namespace secret listing is appropriate for cluster-scoped cleanup.
91-109: LGTM!The validation logic is conservative, treating any parsing errors or missing annotations as invalid, which ensures cleanup of malformed secrets.
go/apps/preflight/internal/services/registry/registry.go (3)
27-45: LGTM!The Config-based constructor properly enables dependency injection of the credentials manager.
47-101: LGTM!The credentials-first flow properly integrates the new token-based authentication while preserving the existing k8schain fallback for non-managed registries. The build ID is correctly propagated for build-scoped tokens.
177-183: LGTM!The OS selection logic correctly maps macOS development environments to Linux (the target deployment platform) while allowing other environments to use their native OS.
go/k8s/manifests/preflight.yaml (2)
14-23: LGTM!The added
createanddeleteverbs for secrets are necessary for the new functionality: creating imagePullSecrets and cleaning up expired credentials.
166-166: LGTM!The change from
NonetoNoneOnDryRuncorrectly reflects that the webhook now has side effects (creating imagePullSecrets) during normal operation.go/apps/preflight/internal/services/mutator/mutator.go (8)
21-43: LGTM!The Mutator structure properly encapsulates all dependencies needed for pod mutation and credential management.
51-58: LGTM!The mutation eligibility check correctly uses the deployment ID annotation to determine when to inject the unkey-env sidecar.
60-130: LGTM!The mutation orchestration is well-structured, properly handling image credentials, init containers, and per-container patches. The build ID is correctly extracted and propagated throughout the mutation flow.
132-152: LGTM!The image collection logic correctly identifies and deduplicates all images requiring credentials from both regular and init containers.
154-159: LGTM!The 55-minute token TTL provides appropriate buffer for clock skew and processing time while aligning with the 1-hour cleanup interval.
161-217: LGTM!The pull secret injection logic is resilient and correctly handles both empty and existing imagePullSecrets lists. The approach of continuing on errors ensures partial success rather than complete failure.
219-299: LGTM!The secret management logic is well-designed:
- Reuses valid secrets to minimize API calls
- Handles race conditions gracefully
- Uses proper Kubernetes secret types and metadata
- Implements deterministic naming for cross-pod secret sharing
321-326: LGTM!The deterministic secret naming strategy using SHA-256 enables efficient secret reuse across multiple pods using the same image, reducing API calls and improving performance.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/apps/preflight/internal/services/registry/registry.go (1)
59-80: Edge case: credentials manager matches but returns empty credentials.If
r.credentials.Matches()returnstruebutGetCredentialsreturns anildockerConfig or an emptyAuthsmap, the code silently falls back to k8schain. For a managed private registry like Depot, this fallback would likely fail anyway and could make debugging harder.Consider adding a warning log or explicit handling when the credentials manager matches but produces no usable credentials:
if r.credentials != nil && r.credentials.Matches(container.Image) { dockerConfig, credErr := r.credentials.GetCredentials(ctx, container.Image, buildID) if credErr != nil { return nil, fmt.Errorf("failed to get credentials for %q: %w", container.Image, credErr) } if dockerConfig != nil { // Use credentials from our manager for registry, auth := range dockerConfig.Auths { r.logger.Debug("using credentials from manager", "image", container.Image, "registry", registry, ) options = append(options, remote.WithAuth(&authn.Basic{ Username: auth.Username, Password: auth.Password, })) break // Only need one auth } - } + } else { + r.logger.Warn("credentials manager matched but returned no credentials", + "image", container.Image, + ) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/apps/preflight/internal/services/registry/registry.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
go/apps/preflight/internal/services/registry/registry.go (3)
go/apps/preflight/internal/services/registry/credentials/credentials.go (2)
Manager(61-63)Registry(10-17)go/apps/preflight/internal/services/mutator/mutator.go (1)
New(32-43)go/apps/preflight/internal/services/mutator/config.go (1)
Config(26-35)
⏰ 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). (5)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Lint Go Code / Lint
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (4)
go/apps/preflight/internal/services/registry/registry.go (4)
27-31: LGTM!The
Configstruct follows idiomatic Go patterns for dependency injection. Using a pointer forCredentialscorrectly allows it to be optional/nil.
82-98: LGTM!The k8schain fallback correctly preserves the existing authentication flow for non-managed registries. The condition
len(options) == 0cleanly gates the fallback, and image pull secrets are properly collected from the pod spec.
100-100: LGTM!Propagating the context ensures proper cancellation and timeout handling for remote operations.
177-179: Implementation is correct; AI summary appears inaccurate.The code correctly returns a constant
"linux"fortargetOS(), which is appropriate since Kubernetes pods run on Linux nodes regardless of where the webhook runs (e.g., during local development on macOS). The AI summary's claim about conditionalruntime.GOOSlogic doesn't match the actual implementation.
6 participants
What does this PR do?
Renames secrets-webhook to preflight -> as it runs before the pod takes off (gets launched)
feel free to suggest better names or just rename it. maybe launchpad
We now inject a shortlived (1hr ttl) depot.dev token when creating the pod per secret and clean that up later at some point.
Secrets are named by image hash, so multiple pods with the same image share one secret/token
Expired secrets are deleted hourly by a background goroutine
Type of change
How should this be tested?
Running a build in k8s should work and start the pod!
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated