Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughImplements encrypted-secrets-by-blob flow across the stack: environment-scoped vault encryption, a Krane Secrets RPC, unkey-env runtime injector, a Kubernetes mutating webhook to inject init containers, and related config/CLI/k8s manifest changes replacing per-variable secrets with an encrypted blob model. Changes
Sequence Diagram(s)sequenceDiagram
participant Ctrl as Ctrl Service
participant Krane as Krane (create deployment)
participant K8s as Kubernetes API
participant Webhook as Secrets Webhook
participant Init as unkey-env Init Container
participant SecretsRPC as Krane SecretsService
Ctrl->>Ctrl: Encrypt SecretsConfig -> encrypted_blob (vault, environment_id)
Ctrl->>Krane: CreateDeploymentRequest (encrypted_blob, environment_id, environment_slug)
Krane->>K8s: Create StatefulSet/Pod with annotations (unkey.com/inject=true)
K8s->>Webhook: Admission request (Pod CREATE)
Webhook->>Webhook: Load annotations -> build JSON Patch (inject unkey-env init container + env)
Webhook-->>K8s: Respond with mutated Pod spec
K8s->>Init: Start init container (env includes UNKEY_SECRETS_BLOB, token, deployment-id)
Init->>SecretsRPC: DecryptSecretsBlob(encrypted_blob, environment_id, token, deployment_id)
SecretsRPC->>SecretsRPC: Validate token (TokenReview) & deployment-id annotation
SecretsRPC->>SecretsRPC: Decrypt blob via Vault (keyring=environment_id) -> plaintext env map
SecretsRPC-->>Init: Return env vars
Init->>Init: Export env vars and exec application container
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Key areas requiring careful review:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (7)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/pkg/cli/parser.go (1)
90-95: Three existing commands will break with this validation change—update them immediately.Commands
version get,version rollback, andhealthcheckall callcmd.Args()but lackAcceptsArgs: true, causing them to fail when receiving positional arguments. These must be updated before this change is merged.Additionally, the error message could be clearer. Instead of saying "unexpected argument" with only "Available flags," explicitly state that the command does not accept positional arguments. Consider:
if len(commandArgs) > 0 && !c.AcceptsArgs { - availableFlags := c.getAvailableFlags() - return fmt.Errorf("unexpected argument: %s\nAvailable flags: %s", - commandArgs[0], availableFlags) + return fmt.Errorf("unexpected positional argument: %s\nThis command does not accept positional arguments.\nAvailable flags: %s", + commandArgs[0], c.getAvailableFlags()) }
🧹 Nitpick comments (16)
go/apps/krane/config.go (1)
10-16: Consider consolidating duplicate S3Config structs.The S3Config struct is duplicated across multiple files (go/apps/ctrl/config.go, go/pkg/vault/storage/s3.go, go/apps/ctrl/services/build/storage/s3.go). This creates maintenance overhead and potential inconsistencies.
Consider extracting S3Config to a shared package (e.g.,
go/pkg/configorgo/pkg/storage) and importing it where needed:// In go/pkg/storage/s3_config.go or similar package storage type S3Config struct { URL string Bucket string AccessKeyID string AccessKeySecret string }Then import and use it in each location.
go/apps/secrets-webhook/run.go (1)
21-77: LGTM - Well-structured webhook server initialization.The Run function follows good practices:
- Early config validation
- Proper error wrapping at each step
- In-cluster K8s client setup (correct for admission webhooks)
- TLS configuration (required for webhooks)
- Middleware stack with panic recovery and logging
- Clear startup logging
Consider adding graceful shutdown handling if the zen.Server doesn't already handle context cancellation. Example pattern:
errChan := make(chan error, 1) go func() { errChan <- server.Serve(ctx, ln) }() select { case <-ctx.Done(): // Graceful shutdown with timeout shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() return server.Shutdown(shutdownCtx) case err := <-errChan: return err }Only if zen.Server.Serve() doesn't already respect context cancellation.
go/apps/secrets-webhook/config.go (1)
14-28: Consider adding validation for file existence and URL format.The current validation checks for non-empty strings but doesn't verify:
- TLS certificate and key files exist and are readable
- KraneEndpoint is a valid URL
- UnkeyEnvImage has valid image reference format
Enhance validation to catch configuration errors earlier:
+import ( + "fmt" + "net/url" + "os" +) + func (c *Config) Validate() error { if c.HttpPort == 0 { c.HttpPort = 8443 } if c.AnnotationPrefix == "" { c.AnnotationPrefix = "unkey.com" } - return assert.All( + err := 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"), ) + if err != nil { + return err + } + + // Validate TLS files exist + if _, err := os.Stat(c.TLSCertFile); err != nil { + return fmt.Errorf("tls-cert-file not found: %w", err) + } + if _, err := os.Stat(c.TLSKeyFile); err != nil { + return fmt.Errorf("tls-key-file not found: %w", err) + } + + // Validate KraneEndpoint is a valid URL + if _, err := url.Parse(c.KraneEndpoint); err != nil { + return fmt.Errorf("invalid krane-endpoint URL: %w", err) + } + + return nil }go/apps/krane/secrets/token/k8s_validator.go (1)
61-64: Consider making annotation key configurable.The annotation key
"unkey.com/deployment-id"is hardcoded, but the webhook config accepts anAnnotationPrefix. For consistency, consider using a configurable annotation key.Update K8sValidatorConfig to include the annotation prefix:
type K8sValidatorConfig struct { Clientset kubernetes.Interface + AnnotationPrefix string } type K8sValidator struct { clientset kubernetes.Interface + annotationPrefix string } func NewK8sValidator(cfg K8sValidatorConfig) *K8sValidator { - return &K8sValidator{clientset: cfg.Clientset} + return &K8sValidator{ + clientset: cfg.Clientset, + annotationPrefix: cfg.AnnotationPrefix, + } }Then use it:
- podDeploymentID := pod.Annotations["unkey.com/deployment-id"] + annotationKey := fmt.Sprintf("%s/deployment-id", v.annotationPrefix) + podDeploymentID := pod.Annotations[annotationKey] if podDeploymentID == "" { - return nil, fmt.Errorf("pod %s missing unkey.com/deployment-id annotation", podName) + return nil, fmt.Errorf("pod %s missing %s annotation", podName, annotationKey) }go/cmd/krane/main.go (1)
51-52: Add validation for base64-encoded master keys.The flag description mentions "base64 encoded" but there's no validation to ensure the provided keys are valid base64 strings.
Add validation in the krane.Config.Validate() method or after flag parsing:
import "encoding/base64" // In validation or after parsing flags for i, key := range config.VaultMasterKeys { if _, err := base64.StdEncoding.DecodeString(key); err != nil { return cli.Exit(fmt.Sprintf("vault-master-keys[%d] is not valid base64: %v", i, err), 1) } }This provides early feedback if keys are malformed.
go/Tiltfile (1)
265-277: TLS secret generation looks correct, but consider error handling.The mkcert command and kubectl secret creation are properly sequenced. However, if
mkcertfails, the script continues and may leave stale temporary files. Consider usingset -eor similar in the shell command:''' + set -e mkcert -cert-file /tmp/secrets-webhook-tls.crt -key-file /tmp/secrets-webhook-tls.key \go/pkg/secrets/provider/krane_vault.go (2)
72-82: Consider adding error context for RPC failures.When DecryptSecretsBlob fails, the error lacks context about which operation failed. This makes debugging harder in logs.
resp, err := p.client.DecryptSecretsBlob(ctx, connect.NewRequest(&kranev1.DecryptSecretsBlobRequest{ EncryptedBlob: opts.EncryptedBlob, EnvironmentId: opts.EnvironmentID, Token: token, DeploymentId: opts.DeploymentID, })) if err != nil { - return nil, err + return nil, fmt.Errorf("DecryptSecretsBlob RPC failed: %w", err) }
84-94: Fallback path also benefits from error context.resp, err := p.client.GetDeploymentSecrets(ctx, connect.NewRequest(&kranev1.GetDeploymentSecretsRequest{ DeploymentId: opts.DeploymentID, Token: token, })) if err != nil { - return nil, err + return nil, fmt.Errorf("GetDeploymentSecrets RPC failed: %w", err) }go/k8s/manifests/secrets-webhook.yaml (1)
60-91: Consider adding security context for defense in depth.While acceptable for local development per project conventions, adding explicit security settings improves the manifest's production-readiness and addresses static analysis findings (CKV_K8S_20, CKV_K8S_23).
spec: serviceAccountName: secrets-webhook + securityContext: + runAsNonRoot: true + runAsUser: 65534 containers: - name: webhook image: unkey-secrets-webhook:latest imagePullPolicy: IfNotPresent command: ["/unkey", "run", "secrets-webhook"] + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALLgo/apps/krane/backend/docker/deployment_create.go (1)
140-171: Consider batching vault decryption calls for performance.The current implementation makes N+1 vault calls (1 for the blob + N for each secret value). For deployments with many secrets, this could add latency during container creation.
If the vault service supports batch decryption, consider batching these calls. However, if this is acceptable for the current scale, this can be deferred.
go/apps/krane/secrets/service.go (2)
55-62: Token validation error wrapping leaks internal details.Wrapping the original validation error in the Connect error message may expose internal implementation details to clients. Consider returning a generic "authentication failed" message without the underlying error.
_, err := s.tokenValidator.Validate(ctx, requestToken, deploymentID) if err != nil { s.logger.Warn("token validation failed", "deployment_id", deploymentID, "error", err, ) - return nil, connect.NewError(connect.CodeUnauthenticated, fmt.Errorf("invalid or expired token: %w", err)) + return nil, connect.NewError(connect.CodeUnauthenticated, fmt.Errorf("invalid or expired token")) }
91-106: Consider batching secret decryptions or adding rate limiting.Each secret is decrypted individually in a sequential loop, resulting in N+1 Vault calls (1 for blob + N for secrets). For deployments with many secrets, this could cause performance bottlenecks and increase latency.
If the Vault service supports batch decryption, consider using it here. Otherwise, this may be acceptable for the initial implementation if secret counts are typically low.
go/proto/krane/v1/secrets.proto (1)
12-13: Use proto3 deprecation annotation for deprecated RPC.The comment indicates deprecation, but adding the protobuf
deprecatedoption provides tooling support and generates deprecation warnings in generated code.// GetDeploymentSecrets returns decrypted environment variables for a deployment. // Authentication is via a token generated at deployment creation time. // DEPRECATED: Use DecryptSecretsBlob instead for better performance. - rpc GetDeploymentSecrets(GetDeploymentSecretsRequest) returns (GetDeploymentSecretsResponse); + rpc GetDeploymentSecrets(GetDeploymentSecretsRequest) returns (GetDeploymentSecretsResponse) { + option deprecated = true; + }go/apps/secrets-webhook/internal/services/registry/registry.go (1)
63-66: Remote registry calls lack timeout protection.The
remote.Getcall uses the provided context but if no deadline is set upstream, this could hang indefinitely when registries are slow or unresponsive, blocking the admission webhook.Consider adding a timeout context if not already enforced by the caller:
// Example: wrap with timeout if needed ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel()go/apps/secrets-webhook/internal/services/mutator/mutator.go (2)
59-89: Consider extracting repeated patch-building logic into a helper function.The pattern of checking array emptiness and constructing conditional JSON Patch operations is duplicated for both init containers (lines 62-74) and volumes (lines 76-89). Extracting this into a helper would improve maintainability.
Consider adding a helper function like:
func addArrayElement(patches *[]map[string]interface{}, arrayPath string, value interface{}, isEmpty bool) { if isEmpty { *patches = append(*patches, map[string]interface{}{ "op": "add", "path": arrayPath, "value": []interface{}{value}, }) } else { *patches = append(*patches, map[string]interface{}{ "op": "add", "path": arrayPath + "/-", "value": value, }) } }Then use it:
addArrayElement(&patches, "/spec/initContainers", initContainer, len(pod.Spec.InitContainers) == 0) addArrayElement(&patches, "/spec/volumes", volume, len(pod.Spec.Volumes) == 0)
1-109: Consider adding package and method documentation.While the code is clean and self-explanatory, adding documentation would improve maintainability:
- Package-level doc explaining the mutation service's role
- Godoc comments on exported types (Mutator, Result) and methods (ShouldMutate, Mutate)
Example:
// Package mutator provides Kubernetes pod mutation services for injecting // the unkey-env init container and related configuration. // Mutator orchestrates pod mutations for secrets injection based on // pod annotations and configuration. type Mutator struct { ... } // ShouldMutate returns true if the pod should be mutated based on the // presence of required annotations. func (m *Mutator) ShouldMutate(pod *corev1.Pod) bool { ... } // Mutate applies JSON Patch operations to inject init containers, volumes, // and container modifications for secrets handling. func (m *Mutator) Mutate(ctx context.Context, pod *corev1.Pod, namespace string) (*Result, error) { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
apps/dashboard/gen/proto/ctrl/v1/secrets_pb.tsis excluded by!**/gen/**apps/dashboard/gen/proto/krane/v1/deployment_pb.tsis excluded by!**/gen/**apps/dashboard/gen/proto/krane/v1/secrets_pb.tsis excluded by!**/gen/**go/gen/proto/ctrl/v1/ctrlv1connect/secrets.connect.gois excluded by!**/gen/**go/gen/proto/ctrl/v1/secrets.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/krane/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/krane/v1/kranev1connect/secrets.connect.gois excluded by!**/gen/**go/gen/proto/krane/v1/secrets.pb.gois excluded by!**/*.pb.go,!**/gen/**go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
apps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts(1 hunks)apps/dashboard/lib/trpc/routers/deploy/env-vars/decrypt.ts(2 hunks)apps/dashboard/lib/trpc/routers/deploy/env-vars/update.ts(2 hunks)deployment/docker-compose.yaml(1 hunks)go/.gitignore(1 hunks)go/Dockerfile.unkey-env(1 hunks)go/Tiltfile(5 hunks)go/apps/ctrl/workflows/deploy/deploy_handler.go(1 hunks)go/apps/krane/backend/docker/deployment_create.go(4 hunks)go/apps/krane/backend/docker/service.go(4 hunks)go/apps/krane/backend/kubernetes/deployment_create.go(3 hunks)go/apps/krane/backend/kubernetes/service.go(3 hunks)go/apps/krane/config.go(2 hunks)go/apps/krane/run.go(3 hunks)go/apps/krane/secrets/service.go(1 hunks)go/apps/krane/secrets/token/k8s_validator.go(1 hunks)go/apps/krane/secrets/token/validator.go(1 hunks)go/apps/secrets-webhook/config.go(1 hunks)go/apps/secrets-webhook/internal/services/mutator/config.go(1 hunks)go/apps/secrets-webhook/internal/services/mutator/mutator.go(1 hunks)go/apps/secrets-webhook/internal/services/mutator/patch.go(1 hunks)go/apps/secrets-webhook/internal/services/registry/registry.go(1 hunks)go/apps/secrets-webhook/routes/healthz/handler.go(1 hunks)go/apps/secrets-webhook/routes/mutate/handler.go(1 hunks)go/apps/secrets-webhook/run.go(1 hunks)go/cmd/krane/main.go(2 hunks)go/cmd/run/main.go(3 hunks)go/cmd/secrets-webhook/main.go(1 hunks)go/go.mod(9 hunks)go/k8s/manifests/krane.yaml(1 hunks)go/k8s/manifests/rbac.yaml(2 hunks)go/k8s/manifests/secrets-webhook.yaml(1 hunks)go/pkg/cli/command.go(1 hunks)go/pkg/cli/parser.go(1 hunks)go/pkg/secrets/provider/krane_vault.go(1 hunks)go/pkg/secrets/provider/provider.go(1 hunks)go/proto/ctrl/v1/secrets.proto(1 hunks)go/proto/krane/v1/deployment.proto(1 hunks)go/proto/krane/v1/secrets.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
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.
📚 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/.gitignore
📚 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/.gitignore
📚 Learning: 2025-08-08T16:07:48.307Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:37-40
Timestamp: 2025-08-08T16:07:48.307Z
Learning: Repo unkeyed/unkey — pnpm immutable installs are enforced by setting the CI environment variable; any truthy value (e.g., "1" or "true") is acceptable. Do not require the literal string "true". Applies to .github/actions/setup-node/action.yaml and all workflows using pnpm install.
Applied to files:
go/.gitignore
📚 Learning: 2025-08-08T15:37:14.734Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/build.yaml:14-17
Timestamp: 2025-08-08T15:37:14.734Z
Learning: Repo: unkeyed/unkey — CI behavior: We rely on CI=true to make pnpm install behave as --frozen-lockfile. Don’t suggest adding --frozen-lockfile in .github/actions/setup-node/action.yaml or workflows like .github/workflows/build.yaml.
Applied to files:
go/.gitignore
📚 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/.gitignorego/k8s/manifests/krane.yamldeployment/docker-compose.yamlgo/apps/krane/backend/docker/deployment_create.gogo/Tiltfilego/cmd/run/main.go
📚 Learning: 2025-06-02T11:09:58.791Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3292
File: apps/dashboard/lib/trpc/routers/key/create.ts:11-14
Timestamp: 2025-06-02T11:09:58.791Z
Learning: In the unkey codebase, TypeScript and the env() function implementation already provide sufficient validation for environment variables, so additional runtime error handling for missing env vars is not needed.
Applied to files:
go/.gitignoreapps/dashboard/lib/trpc/routers/deploy/env-vars/update.tsapps/dashboard/lib/trpc/routers/deploy/env-vars/decrypt.tsapps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts
📚 Learning: 2025-08-08T16:10:00.224Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/.gitignore
📚 Learning: 2025-08-08T15:10:46.436Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/.gitignore
📚 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/krane/secrets/token/validator.gogo/apps/secrets-webhook/config.gogo/apps/krane/secrets/token/k8s_validator.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/krane/secrets/token/validator.gogo/apps/secrets-webhook/config.gogo/apps/krane/secrets/token/k8s_validator.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/k8s/manifests/krane.yamldeployment/docker-compose.yamlgo/k8s/manifests/secrets-webhook.yamlgo/cmd/run/main.go
📚 Learning: 2025-08-31T19:30:38.171Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3853
File: apps/dashboard/lib/trpc/routers/deploy/project/envs/list.ts:75-87
Timestamp: 2025-08-31T19:30:38.171Z
Learning: In apps/dashboard/lib/trpc/routers/deploy/project/envs/list.ts, the getEnvs procedure currently uses mock data (VARIABLES) and ignores the projectId input parameter. This is intentional temporary behavior - the user ogzhanolguncu indicated they plan to hook this up to the database later.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/env-vars/update.tsapps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts
📚 Learning: 2025-09-12T18:11:33.481Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: apps/dashboard/lib/trpc/routers/deploy/rollback.ts:23-24
Timestamp: 2025-09-12T18:11:33.481Z
Learning: In apps/dashboard/lib/trpc/routers/deploy/rollback.ts, the CTRL_URL environment variable should fail fast with a clear error message if missing in non-development environments, rather than defaulting to localhost which can mask production configuration issues.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/env-vars/update.tsapps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts
📚 Learning: 2025-09-25T18:49:11.451Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4010
File: apps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts:39-44
Timestamp: 2025-09-25T18:49:11.451Z
Learning: In apps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts and similar files, mcstepp prefers to keep the demo API key authentication simple without additional validation complexity, since it's temporary code that will be replaced after the demo phase.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/env-vars/update.tsapps/dashboard/lib/trpc/routers/deploy/env-vars/decrypt.tsapps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts
📚 Learning: 2025-06-02T11:08:56.397Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3292
File: apps/dashboard/lib/vault.ts:80-97
Timestamp: 2025-06-02T11:08:56.397Z
Learning: The vault.ts file in apps/dashboard/lib/vault.ts is a duplicate of the vault package from the `api` directory and should be kept consistent with that original implementation.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/env-vars/update.tsapps/dashboard/lib/trpc/routers/deploy/env-vars/decrypt.tsapps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/env-vars/update.ts
📚 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/proto/ctrl/v1/secrets.protogo/apps/krane/backend/docker/deployment_create.gogo/proto/krane/v1/secrets.proto
📚 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/proto/ctrl/v1/secrets.protogo/apps/krane/backend/docker/deployment_create.gogo/Dockerfile.unkey-envgo/apps/krane/backend/kubernetes/deployment_create.gogo/proto/krane/v1/secrets.proto
📚 Learning: 2025-07-22T09:02:12.495Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3638
File: deployment/docker-compose.yaml:81-94
Timestamp: 2025-07-22T09:02:12.495Z
Learning: The docker-compose.yaml file in deployment/ is specifically for development environments, not production. Kafka and other service configurations in this file should be optimized for development convenience rather than production security/hardening.
Applied to files:
deployment/docker-compose.yaml
📚 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/apps/krane/backend/docker/deployment_create.gogo/apps/ctrl/workflows/deploy/deploy_handler.go
📚 Learning: 2025-09-24T19:58:24.572Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4022
File: go/pkg/zen/middleware_timeout.go:30-35
Timestamp: 2025-09-24T19:58:24.572Z
Learning: The zen framework in go/pkg/zen passes context as a separate parameter to handlers (ctx context.Context, s *Session) rather than through a Session.Context() method. The Session struct does not have a Context() method, and handlers access context through the ctx parameter, not through the session.
Applied to files:
go/apps/secrets-webhook/routes/healthz/handler.go
📚 Learning: 2025-09-15T18:12:01.503Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 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/krane/run.go
📚 Learning: 2025-06-02T11:09:05.843Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3292
File: apps/dashboard/lib/vault.ts:60-78
Timestamp: 2025-06-02T11:09:05.843Z
Learning: The vault implementation in `apps/dashboard/lib/vault.ts` is a duplicate of the vault package from `api` and should be kept consistent with the original implementation.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts
🧬 Code graph analysis (19)
go/apps/secrets-webhook/config.go (3)
go/apps/krane/config.go (1)
Config(32-113)go/apps/krane/secrets/service.go (1)
Config(19-23)go/pkg/secrets/provider/provider.go (1)
Config(49-55)
go/apps/secrets-webhook/run.go (6)
go/apps/secrets-webhook/config.go (1)
Config(5-12)go/apps/secrets-webhook/internal/services/mutator/config.go (1)
Config(17-21)go/apps/secrets-webhook/internal/services/registry/registry.go (1)
New(27-32)go/pkg/zen/middleware_panic_recovery.go (1)
WithPanicRecovery(18-62)go/apps/secrets-webhook/routes/mutate/handler.go (1)
Handler(18-21)go/apps/secrets-webhook/routes/healthz/handler.go (1)
Handler(10-10)
go/pkg/secrets/provider/provider.go (1)
go/pkg/secrets/provider/krane_vault.go (1)
NewKraneVaultProvider(23-37)
go/apps/krane/secrets/token/k8s_validator.go (1)
go/apps/krane/secrets/token/validator.go (1)
ValidationResult(5-7)
go/apps/krane/secrets/service.go (3)
go/apps/krane/secrets/token/validator.go (1)
Validator(9-11)go/gen/proto/krane/v1/secrets.pb.go (6)
DecryptSecretsBlobRequest(123-136)DecryptSecretsBlobRequest(149-149)DecryptSecretsBlobRequest(164-166)DecryptSecretsBlobResponse(196-202)DecryptSecretsBlobResponse(215-215)DecryptSecretsBlobResponse(230-232)go/gen/proto/ctrl/v1/secrets.pb.go (3)
SecretsConfig(26-32)SecretsConfig(45-45)SecretsConfig(60-62)
go/apps/krane/backend/docker/service.go (1)
go/apps/krane/secrets/service.go (1)
Service(25-30)
go/apps/krane/backend/docker/deployment_create.go (3)
go/apps/krane/backend/docker/service.go (1)
Config(38-45)apps/dashboard/gen/proto/ctrl/v1/secrets_pb.ts (1)
SecretsConfig(21-28)go/gen/proto/ctrl/v1/secrets.pb.go (3)
SecretsConfig(26-32)SecretsConfig(45-45)SecretsConfig(60-62)
go/apps/secrets-webhook/internal/services/mutator/patch.go (2)
go/apps/secrets-webhook/internal/services/mutator/mutator.go (1)
Mutator(14-18)go/apps/secrets-webhook/internal/services/mutator/config.go (1)
ServiceAccountTokenPath(9-9)
go/cmd/krane/main.go (3)
go/pkg/cli/flag.go (3)
StringSlice(622-656)EnvVar(320-339)String(419-453)go/apps/krane/config.go (1)
S3Config(11-16)go/pkg/vault/storage/s3.go (1)
S3Config(25-31)
go/apps/secrets-webhook/internal/services/mutator/mutator.go (3)
go/apps/secrets-webhook/config.go (1)
Config(5-12)go/apps/secrets-webhook/internal/services/mutator/config.go (2)
Config(17-21)AnnotationDeploymentID(13-13)go/apps/secrets-webhook/internal/services/registry/registry.go (2)
Registry(22-25)New(27-32)
go/apps/secrets-webhook/routes/healthz/handler.go (2)
go/apps/secrets-webhook/routes/mutate/handler.go (1)
Handler(18-21)go/pkg/zen/session.go (1)
Session(31-48)
go/apps/krane/run.go (6)
go/apps/krane/secrets/service.go (3)
Service(25-30)New(32-39)Config(19-23)go/apps/krane/config.go (3)
S3Config(11-16)Config(32-113)Kubernetes(28-28)go/apps/krane/backend/kubernetes/service.go (2)
New(43-64)Config(27-37)go/pkg/vault/storage/interface.go (1)
Storage(15-30)go/apps/krane/secrets/token/validator.go (1)
Validator(9-11)go/apps/krane/secrets/token/k8s_validator.go (2)
NewK8sValidator(21-23)K8sValidatorConfig(13-15)
go/apps/ctrl/workflows/deploy/deploy_handler.go (4)
go/gen/proto/ctrl/v1/secrets.pb.go (3)
SecretsConfig(26-32)SecretsConfig(45-45)SecretsConfig(60-62)go/gen/proto/vault/v1/service.pb.go (3)
EncryptRequest(104-110)EncryptRequest(123-123)EncryptRequest(138-140)go/gen/proto/krane/v1/deployment.pb.go (6)
CreateDeploymentRequest(189-194)CreateDeploymentRequest(207-207)CreateDeploymentRequest(222-224)DeploymentRequest(76-94)DeploymentRequest(107-107)DeploymentRequest(122-124)go/gen/proto/ctrl/v1/deployment.pb.go (6)
CreateDeploymentRequest(136-154)CreateDeploymentRequest(167-167)CreateDeploymentRequest(182-184)Deployment(530-564)Deployment(577-577)Deployment(592-594)
go/apps/secrets-webhook/routes/mutate/handler.go (3)
go/apps/secrets-webhook/internal/services/mutator/mutator.go (2)
Mutator(14-18)Result(28-32)go/pkg/zen/session.go (1)
Session(31-48)go/pkg/zen/request_util.go (1)
BindBody(10-23)
go/cmd/secrets-webhook/main.go (3)
go/apps/secrets-webhook/config.go (1)
Config(5-12)go/apps/secrets-webhook/internal/services/mutator/config.go (1)
Config(17-21)go/apps/secrets-webhook/run.go (1)
Run(21-78)
go/apps/secrets-webhook/internal/services/registry/registry.go (2)
go/cmd/secrets-webhook/main.go (1)
Cmd(10-28)go/apps/secrets-webhook/internal/services/mutator/config.go (1)
Config(17-21)
go/apps/krane/config.go (3)
go/apps/ctrl/config.go (1)
S3Config(19-25)go/apps/ctrl/services/build/storage/s3.go (1)
S3Config(24-31)go/pkg/vault/storage/s3.go (1)
S3Config(25-31)
go/apps/secrets-webhook/internal/services/mutator/config.go (2)
go/apps/secrets-webhook/config.go (1)
Config(5-12)go/apps/secrets-webhook/internal/services/mutator/mutator.go (1)
Mutator(14-18)
go/cmd/run/main.go (3)
go/cmd/secrets-webhook/main.go (1)
Cmd(10-28)go/cmd/healthcheck/main.go (1)
Cmd(11-28)go/pkg/cli/command.go (2)
Action(21-21)Command(24-41)
🪛 Buf (1.61.0)
go/proto/krane/v1/secrets.proto
3-3: Files with package "krane.v1" must be within a directory "krane/v1" relative to root but were in directory "go/proto/krane/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 Checkov (3.2.334)
go/k8s/manifests/secrets-webhook.yaml
[medium] 44-110: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 44-110: Minimize the admission of root containers
(CKV_K8S_23)
🪛 Gitleaks (8.30.0)
deployment/docker-compose.yaml
[high] 384-384: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
go/apps/secrets-webhook/internal/services/mutator/config.go (3)
17-22: Consider aligning naming with top-level webhook config for clarity.The app-level config uses
KraneEndpoint, while this struct usesDefaultProviderEndpoint. If they refer to the same thing, renaming or adding a brief comment here could reduce mental mapping overhead between config layers.
24-26: Consider normalizingAnnotationPrefixinGetAnnotation.Right now any trailing
/(or an empty prefix) will produce slightly odd keys (e.g.secrets.unkey.dev//deployment-idor/deployment-id). Either:
- normalize the prefix here (e.g. trim a trailing
/), or- enforce a “no trailing slash, non-empty” invariant at config load time,
so annotation keys are always well-formed.
33-48: Clarify error semantics for missing annotations and empty provider endpoint.The logic is generally solid, but a few behaviors are worth double‑checking at the call site:
- Missing deployment annotation returns an error. Ensure this doesn’t cause the webhook to reject pods that aren’t opted in (i.e. error should translate to “don’t mutate” rather than failing the AdmissionReview for normal pods).
- If both the annotation and
DefaultProviderEndpointare empty, you’ll silently return apodConfigwith an emptyProviderEndpoint. Decide whether that should instead be treated as a configuration error and surfaced clearly.m.cfgis assumed non‑nil; make sureMutatorconstruction always enforces that.If these are already handled higher up (e.g. by gating
loadPodConfigon annotation presence or validating config on startup), a short comment near the call site or here would make the intent explicit.go/pkg/secrets/provider/krane_vault.go (1)
84-84: Consider returning an error when EncryptedBlob is missing.Returning an empty map when
EncryptedBlobis not provided may silently mask missing configuration. Since the blob-based flow is the primary path (per PR description), consider returning an explicit error to signal that secrets cannot be fetched without the blob, making misconfigurations easier to detect.Apply this diff to return an error instead:
- return make(map[string]string), nil + return nil, fmt.Errorf("no encrypted blob provided - cannot fetch secrets without UNKEY_SECRETS_BLOB")Add the import:
import ( "context" + "fmt" "net/http"go/proto/krane/v1/secrets.proto (1)
3-3: Consider aligning proto directory structure with package name.Buf lint reports that package
krane.v1should be in directorykrane/v1relative to root, but is currently ingo/proto/krane/v1. While this may be an intentional convention for your codebase, aligning with standard proto directory structure improves compatibility with proto tooling and follows common industry practices.Based on learnings, if there's a specific reason for the
go/protoprefix structure in this codebase, this can be safely ignored. Otherwise, consider moving proto files to match the package structure for better tooling compatibility.go/apps/secrets-webhook/routes/mutate/handler.go (1)
58-64: Use a consistent namespace source in logsYou log the request namespace earlier via
namespace := admissionReview.Request.Namespacebut later usepod.Namespace. For admission requests the request namespace is always set, while the object’s namespace can be empty on some operations. Using the samenamespacevariable keeps logs consistent.- if result.Mutated { - h.Logger.Info("mutated pod", "pod", pod.Name, "namespace", pod.Namespace, "message", result.Message) + if result.Mutated { + h.Logger.Info("mutated pod", "pod", pod.Name, "namespace", namespace, "message", result.Message) return h.sendResponseWithPatch(s, admissionReview.Request.UID, result.Patch) } - h.Logger.Info("skipped pod mutation", "pod", pod.Name, "namespace", pod.Namespace, "message", result.Message) + h.Logger.Info("skipped pod mutation", "pod", pod.Name, "namespace", namespace, "message", result.Message)go/apps/secrets-webhook/internal/services/registry/registry.go (1)
150-155: Avoid forcing non‑arm64 architectures to amd64 intargetArch
targetArchcurrently maps every non‑arm64GOARCH todefaultArch(amd64). That’s fine for today’s clusters but unnecessarily constrains future platforms; you can safely default toruntime.GOARCHinstead.-func targetArch() string { - arch := runtime.GOARCH - if arch == "arm64" { - return "arm64" - } - return defaultArch -} +func targetArch() string { + // Preserve explicit arm64 handling, but otherwise use the actual GOARCH + // so multi-arch indexes can match additional platforms if present. + if runtime.GOARCH == "arm64" { + return "arm64" + } + return runtime.GOARCH +}go/k8s/manifests/secrets-webhook.yaml (2)
62-107: Lock down containersecurityContextto address privilege/root findingsThe webhook container currently runs without an explicit
securityContext, and static analysis flags potential privilege escalation and root usage. Given this is a network-facing control-plane component, it’s worth explicitly constraining it (assuming the image supports non‑root).For example:
containers: - name: webhook image: unkey-secrets-webhook:latest imagePullPolicy: IfNotPresent command: ["/unkey", "run", "secrets-webhook"] + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsNonRoot: true + runAsUser: 65532 # or whatever UID your image is built to run asYou may need to adjust
runAsUserto match howunkey-secrets-webhookis built.
128-143: Treat the committedcaBundleas a placeholder, not a shared defaultThe
caBundlevalue is a concrete base64-encoded CA, but the comment above instructs developers to generate their own frommkcert. Anyone reusing this manifest as‑is will get a mismatched CA unless they remember to replace it.Consider either:
- Replacing the literal with a clearly invalid placeholder (e.g.
"REPLACE_WITH_MKCERT_CABUNDLE") and relying on the comment, or- Managing the
MutatingWebhookConfigurationvia tooling/templating (Tilt, Helm, kustomize) so the CA is injected at deploy time instead of being hard-coded in Git.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
apps/dashboard/gen/proto/krane/v1/secrets_pb.tsis excluded by!**/gen/**go/gen/proto/krane/v1/kranev1connect/secrets.connect.gois excluded by!**/gen/**go/gen/proto/krane/v1/secrets.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (15)
go/apps/krane/secrets/token/k8s_validator.go(1 hunks)go/apps/secrets-webhook/config.go(1 hunks)go/apps/secrets-webhook/internal/services/mutator/config.go(1 hunks)go/apps/secrets-webhook/internal/services/mutator/patch.go(1 hunks)go/apps/secrets-webhook/internal/services/registry/registry.go(1 hunks)go/apps/secrets-webhook/routes/mutate/handler.go(1 hunks)go/apps/secrets-webhook/run.go(1 hunks)go/cmd/healthcheck/main.go(1 hunks)go/cmd/secrets-webhook/main.go(1 hunks)go/cmd/version/main.go(2 hunks)go/k8s/manifests/observability.yaml(2 hunks)go/k8s/manifests/secrets-webhook.yaml(1 hunks)go/pkg/cli/parser.go(1 hunks)go/pkg/secrets/provider/krane_vault.go(1 hunks)go/proto/krane/v1/secrets.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- go/apps/secrets-webhook/run.go
- go/cmd/secrets-webhook/main.go
- go/apps/secrets-webhook/internal/services/mutator/patch.go
- go/apps/krane/secrets/token/k8s_validator.go
- go/pkg/cli/parser.go
- go/apps/secrets-webhook/config.go
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: For repo unkeyed/unkey and PR review workflows: When imeyer comments "issue" on a thread, automatically create a thorough GitHub issue (sections: Summary, Impact, Where, Repro/Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and the specific comment, and assign the issue to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
📚 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/k8s/manifests/secrets-webhook.yaml
📚 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/secrets.proto
📚 Learning: 2025-09-25T15:12:21.739Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4010
File: go/cmd/deploy/control_plane.go:64-69
Timestamp: 2025-09-25T15:12:21.739Z
Learning: The API key authentication system in go/cmd/deploy/control_plane.go and related files is temporary demo code that will be replaced with a proper authentication system for private beta.
Applied to files:
go/proto/krane/v1/secrets.proto
📚 Learning: 2025-09-11T08:17:21.423Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/apps/ctrl/services/deployment/fallbacks/k8s.go:236-247
Timestamp: 2025-09-11T08:17:21.423Z
Learning: The K8s fallback backend in go/apps/ctrl/services/deployment/fallbacks/k8s.go is specifically designed for scenarios where the ctrl service runs inside a Kubernetes cluster. It should not be used when ctrl runs outside the cluster - in those cases, the Docker fallback should be used instead.
Applied to files:
go/proto/krane/v1/secrets.proto
📚 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/secrets.proto
🧬 Code graph analysis (5)
go/pkg/secrets/provider/krane_vault.go (3)
go/gen/proto/krane/v1/kranev1connect/secrets.connect.go (1)
SecretsServiceClient(42-47)go/pkg/secrets/provider/provider.go (3)
Config(49-55)KraneVault(15-15)FetchOptions(29-46)go/gen/proto/krane/v1/secrets.pb.go (3)
DecryptSecretsBlobRequest(24-37)DecryptSecretsBlobRequest(50-50)DecryptSecretsBlobRequest(65-67)
go/cmd/version/main.go (2)
go/pkg/cli/flag.go (1)
Flag(20-27)go/pkg/cli/command.go (1)
Command(24-41)
go/apps/secrets-webhook/internal/services/mutator/config.go (3)
go/apps/secrets-webhook/config.go (1)
Config(5-13)go/apps/krane/config.go (1)
Config(32-113)go/apps/secrets-webhook/internal/services/mutator/mutator.go (1)
Mutator(14-18)
go/cmd/healthcheck/main.go (2)
go/pkg/cli/command.go (1)
Command(24-41)go/pkg/cli/flag.go (1)
Flag(20-27)
go/apps/secrets-webhook/routes/mutate/handler.go (3)
go/apps/secrets-webhook/internal/services/mutator/mutator.go (2)
Mutator(14-18)Result(28-32)go/pkg/zen/session.go (1)
Session(31-48)go/pkg/zen/request_util.go (1)
BindBody(10-23)
🪛 Buf (1.61.0)
go/proto/krane/v1/secrets.proto
3-3: Files with package "krane.v1" must be within a directory "krane/v1" relative to root but were in directory "go/proto/krane/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 Checkov (3.2.334)
go/k8s/manifests/secrets-webhook.yaml
[medium] 44-112: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 44-112: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (14)
go/cmd/version/main.go (2)
33-67: LGTM! Correct argument handling enabled.The
AcceptsArgs: truefield correctly enables positional argument acceptance for thegetcommand, which expects a version ID argument. The Action function properly validates that at least one argument is provided (lines 50-52) before using it.
107-153: LGTM! Correct argument handling enabled.The
AcceptsArgs: truefield correctly enables positional argument acceptance for therollbackcommand, which expects both hostname and version ID arguments. The Action function properly validates that at least two arguments are provided (lines 128-130) before using them.go/cmd/healthcheck/main.go (1)
11-54: LGTM! Correct argument handling enabled.The
AcceptsArgs: truefield correctly enables positional argument acceptance for thehealthcheckcommand, which expects a URL argument. The Action function properly validates that at least one argument is provided (lines 33-35) before using it.go/apps/secrets-webhook/internal/services/mutator/config.go (3)
3-10: Constants for unkey-env volume and service account token path look correct.Names and paths match the typical init-container pattern and default SA token mount location; no changes needed here.
12-15: Annotation suffix constants are straightforward and scoped.
"deployment-id"and"provider-endpoint"are clear suffixes and keep the prefix concerns separated inConfig.
28-31:podConfigkeeps only mutation-relevant data and is nicely scoped.The struct holds just what the mutator needs to build patches, keeping admission logic decoupled from full pod metadata.
go/k8s/manifests/observability.yaml (2)
111-122: No breaking change detected from port 8889 removal.Comprehensive search of the repository found no references to port 8889 or any dependencies on
otel-collector:8889. The Prometheus configuration scrapes metrics fromctrl:8084, not from the otel-collector service. This removal does not impact any verified integrations.
92-97: Image change verified as stable and properly configured.The switch to
grafana/otel-lgtm:0.11.7is an official, maintained image published by Grafana with cryptographic signatures. No codebase dependencies exist on the removed configuration (port 8889 and ConfigMap), and the simplified setup with image defaults is compatible with the current Prometheus scraping configuration (which only targetsctrl:8084, not the otel-collector).The port configuration is correct: Grafana (3000), OTLP gRPC (4317), and OTLP HTTP (4318) are all properly exposed in the Service definition. Consider documenting in the PR description why this observability infrastructure upgrade is bundled with the webhook changes to clarify scope.
go/pkg/secrets/provider/krane_vault.go (3)
16-21: LGTM - Clean provider struct definition.The struct appropriately holds the gRPC client and endpoint. Note that the
endpointfield is stored but not used after construction; it may be useful for logging or debugging in future iterations.
23-38: LGTM - Constructor validates inputs and sets reasonable defaults.The 30-second HTTP timeout is appropriate for a secrets fetch operation, and creating the client once per provider instance is efficient.
66-82: Consider using more granular error codes to distinguish retryable from non-retryable failures.The current error handling has limitations:
- Token validation failures (K8s API errors, pod lookup failures, missing annotations, deployment ID mismatch) all return
connect.CodeUnauthenticated, making it impossible for callers to distinguish between transient failures (K8s API outage, retryable) and permanent auth issues (invalid token, wrong deployment ID, non-retryable).- Decryption failures (vault service errors, malformed blob, wrong environment ID) all return
connect.CodeInternal, preventing callers from distinguishing between transient vault failures (retryable) and corrupted/invalid blobs (non-retryable).Consider using distinct error codes (e.g.,
CodeUnavailablefor transient K8s/vault failures,CodeInvalidArgumentfor malformed blobs,CodePermissionDeniedfor authorization mismatches) to allow callers to implement appropriate retry logic.go/proto/krane/v1/secrets.proto (3)
7-14: LGTM - Well-documented service definition.The service documentation clearly explains the purpose (decrypt secrets blob, avoid DB lookups) and authentication mechanism. The removal of the previously deprecated
GetDeploymentSecretsRPC simplifies the API surface.
16-29: LGTM - Request message includes all necessary fields.The request appropriately includes the encrypted blob, keyring identifier (environment_id), authentication token, and deployment identifier for validation. All fields being required simplifies validation logic.
31-34: LGTM - Response structure is simple and appropriate.The response returns a map of decrypted environment variables. Error handling via gRPC status codes (rather than an explicit error field) follows standard gRPC conventions.
Flo4604
requested review from
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
Flo4604
force-pushed
the
fix/clean-local-k8s
branch
from
a238c5f to
b5bf49a
Compare
Flo4604
force-pushed
the
feat/better-k8s-injection
branch
from
0769551 to
1e045a9
Compare
Flo4604
force-pushed
the
fix/clean-local-k8s
branch
from
b5bf49a to
9fb5cce
Compare
Flo4604
force-pushed
the
feat/better-k8s-injection
branch
from
39329ab to
0374ba7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
go/cmd/healthcheck/main.go (1)
11-38: AcceptsArgs correctly enabled for positional URL; optionally validate extra argsSetting
AcceptsArgs: truematches the existingrunActionbehavior that expects a positional<url>and fixes compatibility with the updated CLI parser. No functional issues here.If you want stricter UX, you could also error when more than one positional arg is provided instead of silently ignoring extras:
func runAction(ctx context.Context, cmd *cli.Command) error { args := cmd.Args() if len(args) == 0 { return fmt.Errorf("you must provide a url like so: 'unkey healthcheck <url>'") } - url := args[0] + if len(args) > 1 { + return fmt.Errorf("unexpected extra arguments; usage: 'unkey healthcheck <url>'") + } + + url := args[0]apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx (2)
157-175:getErrorsvalidation & dependencies look correct; duplicate check is fine but O(n²)Wrapping
getErrorsinuseCallbackwith[entries, getExistingEnvVar]prevents stale closures and matches how it’s used. If this list ever grows large, you could optionally precompute a key→count map once perentrieschange instead of callingentries.filterin eachgetErrorsinvocation, but it’s not critical for typical env-var counts.
177-184:validEntriesmemo now correctly tracksgetErrors; dependency set is coherentSwitching the dependency from
getExistingEnvVartogetErrorsensuresvalidEntriesrecomputes whenever the validation logic orgetExistingEnvVarchanges. Includingentriesas well is slightly redundant but harmless; you could rely solely ongetErrorsin the deps if you want to tighten it up.go/pkg/secrets/provider/krane_vault.go (1)
29-32: Consider making HTTP timeout configurable.The 30-second timeout is hardcoded. For flexibility across different deployment environments, consider accepting this as a configuration option or defining it as a package-level constant.
go/apps/krane/run.go (1)
86-93: Consider consistent error variable naming.Lines 82-84 use
vaultStorageErrbut lines 91-93 reuse the outererrvariable. For consistency and clarity, consider usingvaultSvcError similar:- vaultSvc, err = vault.New(vault.Config{ + var vaultSvcErr error + vaultSvc, vaultSvcErr = vault.New(vault.Config{ Logger: logger, Storage: vaultStorage, MasterKeys: cfg.VaultMasterKeys, }) - if err != nil { - return fmt.Errorf("unable to create vault service: %w", err) + if vaultSvcErr != nil { + return fmt.Errorf("unable to create vault service: %w", vaultSvcErr) }go/Tiltfile (1)
265-277: Inline documentation for mkcert dependency already exists, but consider README documentation.The code includes an inline comment at line 266 stating "(mkcert must be installed)". However, this tool is not documented in README files. For better developer onboarding, consider adding mkcert to the project's setup documentation or development prerequisites section.
go/pkg/secrets/provider/provider.go (1)
36-41: Consider documenting mutual exclusivity enforcement for Token/TokenPath.The comment states "If set, Token is ignored" for TokenPath, but this behavior is implicit and relies on the provider implementation. For clarity and defensive coding, consider either:
- Adding validation in
FetchSecretsimplementations to warn/log if both are set, or- Clarifying in the doc comment that callers should set only one.
This is a minor documentation improvement for API clarity.
go/k8s/manifests/secrets-webhook.yaml (2)
62-93: Add securityContext to address container security best practices.Static analysis flags that the container lacks explicit security settings. While acceptable for local development, adding a
securityContextimproves security posture and serves as documentation for production requirements.containers: - name: webhook image: unkey-secrets-webhook:latest imagePullPolicy: IfNotPresent command: ["/unkey", "run", "secrets-webhook"] + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL env: - name: WEBHOOK_PORT
159-159:failurePolicy: Ignoresilently skips injection on webhook errors.With
Ignore, if the webhook is unavailable or errors, pods will be created without secret injection. This is appropriate for local dev to prevent blocking pod creation, but in production you may wantFailto ensure secrets are always injected. Consider adding a comment noting this trade-off.- failurePolicy: Ignore + failurePolicy: Ignore # Use 'Fail' in production to enforce secret injectiongo/apps/krane/backend/kubernetes/deployment_create.go (1)
122-124: Container name "todo" should be replaced with a meaningful name.The container is named "todo" which appears to be a placeholder. Consider using a descriptive name like "app", "workload", or deriving it from the deployment context.
Containers: []corev1.Container{ { - Name: "todo", + Name: "app", Image: req.Msg.GetDeployment().GetImage(),go/apps/ctrl/workflows/deploy/deploy_handler.go (2)
114-130: Clarify the purpose of unmarshalingsecretsConfigbefore encryption.The code unmarshals
deployment.SecretsConfigintosecretsConfig(line 117-120) but then encrypts the rawdeployment.SecretsConfigbytes directly (line 124). The unmarshaledsecretsConfigvariable is never used afterward.If the unmarshal is purely for validation, consider adding a comment. Otherwise, if not needed, it could be removed to simplify the code.
if len(deployment.SecretsConfig) > 0 && w.vault != nil { + // Validate the secrets config format before encrypting var secretsConfig ctrlv1.SecretsConfig if err = protojson.Unmarshal(deployment.SecretsConfig, &secretsConfig); err != nil { return nil, fmt.Errorf("invalid secrets config: %w", err) }
226-229: Early return inside the instance loop may skip remaining instances.When
allReadyis true and the code returns at line 227, it's inside thefor _, instance := range resp.Msg.GetInstances()loop. This means if the first instance is ready, it returns immediately without checking/upserting the remaining instances.This appears intentional (return as soon as all checked instances are ready), but note that
allReadyis reset totrueat line 181 before the inner loop and only set tofalseif any instance is not running. The logic is correct, but the early return placement could be moved outside the inner loop for clarity.for _, instance := range resp.Msg.GetInstances() { if instance.GetStatus() != kranev1.DeploymentStatus_DEPLOYMENT_STATUS_RUNNING { allReady = false } // ... instance upsert logic ... - if allReady { - return resp.Msg.GetInstances(), nil - } } + if allReady { + return resp.Msg.GetInstances(), nil + }go/apps/secrets-webhook/routes/mutate/handler.go (1)
67-79: Consider including the message for allowed responses too.Currently, the message is only set when
allowedis false (line 74). For allowed responses with a message (like "pod not annotated for injection"), the message is silently discarded. This may be intentional for brevity, but could hinder debugging.- if message != "" && !allowed { + if message != "" { response.Response.Result = &metav1.Status{Message: message} }go/apps/krane/backend/docker/deployment_create.go (2)
100-133: Consider cleanup/rollback on partial replica creation failuresThe per-replica creation loop with instance-specific env (including
UNKEY_INSTANCE_ID) looks correct and matches the new model. One behavioral change, though: if creating or starting replica N fails, replicas[0..N-1]remain running while the RPC returns an error. That can leave “orphaned” containers for a deployment that the control plane considers failed.If this matters operationally, consider tracking created container IDs and doing a best-effort rollback on error (stop + remove) before returning. Something like:
created := make([]string, 0, len(deployment.GetReplicas())) for i := range deployment.GetReplicas() { // create container... created = append(created, resp.ID) // start container... if err != nil { // best-effort rollback for _, id := range created { _ = d.client.ContainerRemove(ctx, id, container.RemoveOptions{Force: true}) } return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("failed to start container: %w", err)) } }Not strictly required for correctness, but it would make failure semantics cleaner and reduce stray containers.
140-171:decryptSecretsflow is sound; watch for future perf characteristics with many secretsThe three-step decrypt flow (outer blob →
SecretsConfigJSON → per-secret decrypt) is logically consistent with the proto shape and keeps individual secret ciphertexts opaque at rest. Error handling is also reasonable (fail fast with contextual errors per key).One thing to keep in mind is that this results in
1 + len(secrets)decrypt calls per deployment. If environments ever carry a large number of secrets, this could become a latency hotspot; at that point a bulk decrypt API on Vault or batched requests might be worth considering, but it’s not a blocker for this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
apps/dashboard/gen/proto/ctrl/v1/secrets_pb.tsis excluded by!**/gen/**apps/dashboard/gen/proto/krane/v1/deployment_pb.tsis excluded by!**/gen/**apps/dashboard/gen/proto/krane/v1/secrets_pb.tsis excluded by!**/gen/**go/gen/proto/krane/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/krane/v1/kranev1connect/secrets.connect.gois excluded by!**/gen/**go/gen/proto/krane/v1/secrets.pb.gois excluded by!**/*.pb.go,!**/gen/**go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx(2 hunks)apps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts(1 hunks)apps/dashboard/lib/trpc/routers/deploy/env-vars/decrypt.ts(2 hunks)apps/dashboard/lib/trpc/routers/deploy/env-vars/update.ts(2 hunks)deployment/docker-compose.yaml(1 hunks)go/.gitignore(1 hunks)go/Dockerfile.unkey-env(1 hunks)go/Tiltfile(5 hunks)go/apps/ctrl/workflows/deploy/deploy_handler.go(3 hunks)go/apps/krane/backend/docker/deployment_create.go(5 hunks)go/apps/krane/backend/docker/service.go(4 hunks)go/apps/krane/backend/kubernetes/deployment_create.go(5 hunks)go/apps/krane/backend/kubernetes/service.go(3 hunks)go/apps/krane/config.go(2 hunks)go/apps/krane/run.go(3 hunks)go/apps/krane/secrets/service.go(1 hunks)go/apps/krane/secrets/token/k8s_validator.go(1 hunks)go/apps/krane/secrets/token/validator.go(1 hunks)go/apps/secrets-webhook/config.go(1 hunks)go/apps/secrets-webhook/internal/services/mutator/config.go(1 hunks)go/apps/secrets-webhook/internal/services/mutator/mutator.go(1 hunks)go/apps/secrets-webhook/internal/services/mutator/patch.go(1 hunks)go/apps/secrets-webhook/internal/services/registry/registry.go(1 hunks)go/apps/secrets-webhook/routes/healthz/handler.go(1 hunks)go/apps/secrets-webhook/routes/mutate/handler.go(1 hunks)go/apps/secrets-webhook/run.go(1 hunks)go/cmd/ctrl/main.go(2 hunks)go/cmd/dev/seed/ingress.go(1 hunks)go/cmd/healthcheck/main.go(1 hunks)go/cmd/krane/main.go(2 hunks)go/cmd/run/main.go(3 hunks)go/cmd/secrets-webhook/main.go(1 hunks)go/cmd/unkey-env/Dockerfile(1 hunks)go/cmd/unkey-env/main.go(1 hunks)go/cmd/version/main.go(2 hunks)go/go.mod(9 hunks)go/k8s/manifests/krane.yaml(1 hunks)go/k8s/manifests/observability.yaml(2 hunks)go/k8s/manifests/rbac.yaml(2 hunks)go/k8s/manifests/secrets-webhook.yaml(1 hunks)go/pkg/cli/command.go(1 hunks)go/pkg/cli/parser.go(1 hunks)go/pkg/secrets/provider/krane_vault.go(1 hunks)go/pkg/secrets/provider/provider.go(1 hunks)go/proto/krane/v1/deployment.proto(1 hunks)go/proto/krane/v1/secrets.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- go/pkg/cli/parser.go
- go/apps/secrets-webhook/config.go
- go/apps/krane/backend/kubernetes/service.go
- go/apps/krane/secrets/token/validator.go
- go/proto/krane/v1/deployment.proto
- go/pkg/cli/command.go
- go/cmd/version/main.go
- go/apps/krane/config.go
- go/apps/krane/secrets/service.go
- apps/dashboard/lib/trpc/routers/deploy/env-vars/decrypt.ts
- go/apps/secrets-webhook/internal/services/mutator/config.go
- go/Dockerfile.unkey-env
- go/apps/krane/backend/docker/service.go
- apps/dashboard/lib/trpc/routers/deploy/env-vars/create.ts
- apps/dashboard/lib/trpc/routers/deploy/env-vars/update.ts
- go/k8s/manifests/krane.yaml
- go/cmd/secrets-webhook/main.go
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
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.
📚 Learning: 2025-06-02T11:09:58.791Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3292
File: apps/dashboard/lib/trpc/routers/key/create.ts:11-14
Timestamp: 2025-06-02T11:09:58.791Z
Learning: In the unkey codebase, TypeScript and the env() function implementation already provide sufficient validation for environment variables, so additional runtime error handling for missing env vars is not needed.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx
📚 Learning: 2024-10-04T17:27:09.821Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2146
File: apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx:74-75
Timestamp: 2024-10-04T17:27:09.821Z
Learning: In `apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx`, the hidden `<input>` elements for `workspaceId` and `keyAuthId` work correctly without being registered with React Hook Form.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx
📚 Learning: 2025-08-08T14:59:52.283Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Applied to files:
go/apps/secrets-webhook/routes/mutate/handler.gogo/.gitignore
📚 Learning: 2025-08-08T16:10:00.224Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/apps/secrets-webhook/routes/mutate/handler.gogo/.gitignore
📚 Learning: 2025-08-08T15:10:46.436Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/apps/secrets-webhook/routes/mutate/handler.gogo/.gitignore
📚 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/k8s/manifests/secrets-webhook.yamlgo/cmd/run/main.godeployment/docker-compose.yaml
📚 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/krane/secrets/token/k8s_validator.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 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/krane/secrets/token/k8s_validator.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/.gitignorego/cmd/unkey-env/main.godeployment/docker-compose.yaml
📚 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/apps/krane/backend/docker/deployment_create.gogo/.gitignorego/apps/secrets-webhook/internal/services/mutator/patch.gogo/proto/krane/v1/secrets.protogo/cmd/unkey-env/Dockerfilego/apps/krane/backend/kubernetes/deployment_create.go
📚 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/apps/krane/backend/docker/deployment_create.gogo/proto/krane/v1/secrets.protogo/apps/krane/backend/kubernetes/deployment_create.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/apps/krane/backend/docker/deployment_create.gogo/cmd/unkey-env/main.gogo/apps/secrets-webhook/internal/services/mutator/patch.gogo/cmd/unkey-env/Dockerfile
📚 Learning: 2025-09-24T19:58:24.572Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4022
File: go/pkg/zen/middleware_timeout.go:30-35
Timestamp: 2025-09-24T19:58:24.572Z
Learning: The zen framework in go/pkg/zen passes context as a separate parameter to handlers (ctx context.Context, s *Session) rather than through a Session.Context() method. The Session struct does not have a Context() method, and handlers access context through the ctx parameter, not through the session.
Applied to files:
go/apps/secrets-webhook/routes/healthz/handler.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/.gitignorego/cmd/unkey-env/Dockerfile
📚 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/.gitignorego/cmd/unkey-env/Dockerfile
📚 Learning: 2025-09-12T08:01:20.792Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/pkg/db/acme_challenge_update_verified_with_expiry.sql_generated.go:31-39
Timestamp: 2025-09-12T08:01:20.792Z
Learning: Do not review or suggest changes to files with sql_generated.go suffix or other files marked as auto-generated (containing "Code generated by" comments), as these are generated by tools like sqlc and changes would be overwritten on regeneration.
Applied to files:
go/.gitignore
📚 Learning: 2025-11-10T11:50:46.281Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4292
File: go/apps/api/openapi/openapi-generated.yaml:386-387
Timestamp: 2025-11-10T11:50:46.281Z
Learning: Repo unkeyed/unkey: It’s acceptable for pagination properties (e.g., V2IdentitiesListIdentitiesResponseBody.pagination) to omit x-go-type-skip-optional-pointer and x-go-type-skip-optional-pointer-with-omitzero; do not flag this in future reviews.
Applied to files:
go/.gitignore
📚 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, overlays are required to downconvert 3.1 union nullability to 3.0-style nullable before running oapi-codegen; config.yaml’s output-options.overlay is not recognized by oapi-codegen, so overlays must be applied in a pre-step (programmatic or CLI) prior to codegen.
Applied to files:
go/.gitignore
📚 Learning: 2025-08-08T16:07:48.307Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:37-40
Timestamp: 2025-08-08T16:07:48.307Z
Learning: Repo unkeyed/unkey — pnpm immutable installs are enforced by setting the CI environment variable; any truthy value (e.g., "1" or "true") is acceptable. Do not require the literal string "true". Applies to .github/actions/setup-node/action.yaml and all workflows using pnpm install.
Applied to files:
go/.gitignore
📚 Learning: 2025-08-08T15:37:14.734Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/build.yaml:14-17
Timestamp: 2025-08-08T15:37:14.734Z
Learning: Repo: unkeyed/unkey — CI behavior: We rely on CI=true to make pnpm install behave as --frozen-lockfile. Don’t suggest adding --frozen-lockfile in .github/actions/setup-node/action.yaml or workflows like .github/workflows/build.yaml.
Applied to files:
go/.gitignore
📚 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/secrets-webhook/internal/services/mutator/patch.go
📚 Learning: 2025-09-11T08:17:21.423Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/apps/ctrl/services/deployment/fallbacks/k8s.go:236-247
Timestamp: 2025-09-11T08:17:21.423Z
Learning: The K8s fallback backend in go/apps/ctrl/services/deployment/fallbacks/k8s.go is specifically designed for scenarios where the ctrl service runs inside a Kubernetes cluster. It should not be used when ctrl runs outside the cluster - in those cases, the Docker fallback should be used instead.
Applied to files:
go/proto/krane/v1/secrets.protogo/apps/krane/backend/kubernetes/deployment_create.go
📚 Learning: 2025-09-25T15:12:21.739Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4010
File: go/cmd/deploy/control_plane.go:64-69
Timestamp: 2025-09-25T15:12:21.739Z
Learning: The API key authentication system in go/cmd/deploy/control_plane.go and related files is temporary demo code that will be replaced with a proper authentication system for private beta.
Applied to files:
go/proto/krane/v1/secrets.proto
📚 Learning: 2025-09-15T18:12:01.503Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 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/krane/run.go
📚 Learning: 2025-07-22T09:02:12.495Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3638
File: deployment/docker-compose.yaml:81-94
Timestamp: 2025-07-22T09:02:12.495Z
Learning: The docker-compose.yaml file in deployment/ is specifically for development environments, not production. Kafka and other service configurations in this file should be optimized for development convenience rather than production security/hardening.
Applied to files:
deployment/docker-compose.yaml
🧬 Code graph analysis (12)
go/cmd/krane/main.go (3)
go/pkg/cli/flag.go (3)
StringSlice(622-656)EnvVar(320-339)String(419-453)go/apps/krane/config.go (1)
S3Config(11-16)go/pkg/vault/storage/s3.go (1)
S3Config(25-31)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx (1)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/types.ts (1)
ENV_VAR_KEY_REGEX(5-5)
go/apps/ctrl/workflows/deploy/deploy_handler.go (4)
go/gen/proto/ctrl/v1/secrets.pb.go (3)
SecretsConfig(26-32)SecretsConfig(45-45)SecretsConfig(60-62)go/gen/proto/vault/v1/service.pb.go (3)
EncryptRequest(104-110)EncryptRequest(123-123)EncryptRequest(138-140)go/gen/proto/krane/v1/deployment.pb.go (3)
DeploymentRequest(76-94)DeploymentRequest(107-107)DeploymentRequest(122-124)go/pkg/db/models_generated.go (1)
Deployment(778-796)
go/pkg/secrets/provider/provider.go (1)
go/pkg/secrets/provider/krane_vault.go (1)
NewKraneVaultProvider(24-38)
go/apps/krane/secrets/token/k8s_validator.go (1)
go/apps/krane/secrets/token/validator.go (1)
ValidationResult(5-7)
go/apps/krane/backend/docker/deployment_create.go (3)
go/apps/krane/secrets/service.go (1)
Config(19-23)go/apps/krane/backend/docker/service.go (1)
Config(38-45)go/gen/proto/ctrl/v1/secrets.pb.go (3)
SecretsConfig(26-32)SecretsConfig(45-45)SecretsConfig(60-62)
go/cmd/healthcheck/main.go (1)
go/pkg/cli/command.go (1)
Command(24-41)
go/cmd/run/main.go (2)
go/cmd/secrets-webhook/main.go (1)
Cmd(10-30)go/pkg/cli/command.go (2)
Action(21-21)Command(24-41)
go/apps/secrets-webhook/internal/services/mutator/mutator.go (1)
go/apps/secrets-webhook/internal/services/mutator/config.go (2)
Config(18-23)AnnotationDeploymentID(14-14)
go/cmd/unkey-env/main.go (3)
go/pkg/secrets/provider/provider.go (3)
Type(11-11)KraneVault(15-15)FetchOptions(29-46)go/pkg/cli/flag.go (5)
Flag(20-27)String(419-453)Default(364-416)Required(298-317)Bool(497-535)go/pkg/cli/command.go (2)
Action(21-21)Exit(326-330)
go/apps/secrets-webhook/internal/services/mutator/patch.go (2)
go/apps/secrets-webhook/internal/services/mutator/mutator.go (1)
Mutator(14-18)go/apps/secrets-webhook/internal/services/mutator/config.go (1)
ServiceAccountTokenPath(10-10)
go/apps/krane/run.go (5)
go/apps/krane/secrets/service.go (3)
Service(25-30)New(32-39)Config(19-23)go/apps/krane/config.go (3)
S3Config(11-16)Config(32-113)Kubernetes(28-28)go/pkg/vault/storage/interface.go (1)
Storage(15-30)go/apps/krane/secrets/token/validator.go (1)
Validator(9-11)go/apps/krane/secrets/token/k8s_validator.go (2)
NewK8sValidator(21-23)K8sValidatorConfig(13-15)
🪛 Buf (1.61.0)
go/proto/krane/v1/secrets.proto
3-3: Files with package "krane.v1" must be within a directory "krane/v1" relative to root but were in directory "go/proto/krane/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 Checkov (3.2.334)
go/k8s/manifests/secrets-webhook.yaml
[medium] 44-112: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 44-112: Minimize the admission of root containers
(CKV_K8S_23)
🪛 Gitleaks (8.30.0)
deployment/docker-compose.yaml
[high] 384-384: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: Test API / API Test Local
- GitHub Check: Lint Go Code / Lint
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Build / Build
🔇 Additional comments (45)
go/k8s/manifests/observability.yaml (2)
111-122: Service port configuration aligns with deployment changes.The service ports correctly reflect the updated container ports: Grafana (3000) is now exposed, and OTLP gRPC/HTTP (4317/4318) are maintained. The removal of the Prometheus port (8889) is consistent with the image change. No remaining references to the old Prometheus metrics endpoint (tcp/8889) exist in the Kubernetes manifests, and no ServiceMonitor resources are configured that would break with this change.
93-98: No issues identified with the image change to grafana/otel-lgtm:0.11.7.The codebase contains no references to port 8889, confirming the Prometheus metrics endpoint removal is safe. Prometheus metrics are exported via the Go client library and scraped separately by the dedicated Prometheus deployment—not from the otel-collector service. The grafana/otel-lgtm:0.11.7 image natively supports OTLP on ports 4317 (gRPC) and 4318 (HTTP) per its documented specifications, and the change is appropriately designed as a self-contained observability stack alongside the existing Prometheus setup.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/env-variables-section/add-env-vars.tsx (1)
5-5: Import ofuseCallbackmatches new hook usageThe additional
useCallbackimport is consistent with the newgetErrorshook usage below; no issues here.go/.gitignore (1)
1-2: LGTM!Anchoring the ignore patterns with leading slashes correctly restricts the match to the repository root, preventing inadvertent exclusion of nested files or directories with similar names.
go/k8s/manifests/rbac.yaml (2)
13-14: LGTM!The updated comments accurately document the security boundary: customer workloads have no Kubernetes API permissions, and the mounted token is solely for authenticating with the secrets provider.
58-61: LGTM!The TokenReview permission is correctly scoped to the krane service account and limited to the create verb, enabling token validation for the secrets injection workflow while following least-privilege principles.
go/apps/krane/secrets/token/k8s_validator.go (2)
51-58: Past review concern addressed.The username validation now correctly enforces the expected format by checking for exactly 4 parts and verifying the
system:serviceaccount:prefix, as suggested in the previous review.
25-75: LGTM!The validation flow is well-structured: it validates the token via TokenReview, extracts and validates the service account username format, retrieves the pod, and verifies the deployment ID annotation matches the request. Error handling is thorough and provides clear diagnostics.
go/apps/secrets-webhook/routes/healthz/handler.go (1)
10-22: LGTM!The health check handler follows the zen framework conventions and correctly implements a simple GET /healthz endpoint suitable for Kubernetes probes.
go/apps/secrets-webhook/internal/services/registry/registry.go (2)
88-96: Past review concern addressed.The code now attempts to match the runtime platform before falling back to the first manifest, and logs a warning when no matching platform is found. This addresses the previous concern about ignoring platform in multi-arch indexes.
141-153: LGTM!The platform detection helpers make reasonable assumptions: mapping macOS to Linux for cross-platform development and using the runtime architecture. The inline comment on lines 149-150 clearly documents the assumption that the webhook runs on the same architecture as the workloads.
go/cmd/dev/seed/ingress.go (1)
115-115: LGTM!The explicit
nilvalue forSecretsConfigis appropriate for development seed data where secrets injection is not required.go/cmd/ctrl/main.go (1)
189-189: Verify the intentional omission of external URLs for vault S3 configurations.Both
VaultS3andAcmeVaultS3configurations setExternalURL: ""explicitly, but there are no corresponding CLI flags to configure external URLs for these vaults (unlikeBuildS3which has--build-s3-external-url). This suggests the vault S3 backends are intended to be internal-only. Please confirm this is the intended design.If the vault configurations should support external URLs, consider adding CLI flags similar to
--build-s3-external-url:+ cli.String("vault-s3-external-url", "S3 external endpoint URL for general vault", + cli.EnvVar("UNKEY_VAULT_S3_EXTERNAL_URL")), + cli.String("acme-vault-s3-external-url", "S3 external endpoint URL for ACME vault", + cli.EnvVar("UNKEY_ACME_VAULT_S3_EXTERNAL_URL")),And update the struct initializations:
VaultS3: ctrl.S3Config{ URL: cmd.String("vault-s3-url"), Bucket: cmd.String("vault-s3-bucket"), AccessKeyID: cmd.String("vault-s3-access-key-id"), AccessKeySecret: cmd.String("vault-s3-access-key-secret"), - ExternalURL: "", + ExternalURL: cmd.String("vault-s3-external-url"), },Also applies to: 198-198
go/cmd/unkey-env/Dockerfile (2)
2-14: LGTM!The builder stage follows best practices: uses Alpine for a smaller base, leverages layer caching by downloading modules before copying source, and builds with
CGO_ENABLED=0(required for the scratch runtime) with strip flags to minimize binary size.
17-23: LGTM!The scratch-based final stage produces a minimal container with only the static binary. The
ENTRYPOINTallows the image to be runnable directly while also supporting the intended use case of copying the binary to pods via an init container.go/cmd/run/main.go (1)
12-12: LGTM!The secrets-webhook service is properly integrated into the run command. The import, command registration, and documentation are consistent with the existing service patterns.
Also applies to: 32-32, 47-47, 54-59
deployment/docker-compose.yaml (1)
384-384: LGTM!The addition of
UNKEY_ACME_VAULT_MASTER_KEYSaligns with the existing Vault configuration pattern in this development docker-compose file. Based on learnings, hardcoded credentials in development Kubernetes manifests and docker-compose files are acceptable for local development convenience.go/apps/secrets-webhook/run.go (1)
21-79: Clean webhook server implementation.The bootstrap flow properly validates configuration, initializes K8s client, loads TLS certificates, and registers routes with appropriate middleware. Error handling is consistent with wrapped errors for debugging.
go/Tiltfile (1)
281-286: LGTM!The unkey-env image build properly uses static compilation (
CGO_ENABLED=0) and includes the correct dependencies for triggering rebuilds.go/cmd/krane/main.go (1)
49-60: LGTM!The Vault configuration flags are properly defined with consistent naming conventions (
UNKEY_VAULT_*) and correctly wired into thekrane.Configstruct. The optional nature of these flags aligns with the conditional Vault initialization inrun.go.Also applies to: 86-92
go/proto/krane/v1/secrets.proto (1)
1-34: LGTM!The proto definition is well-structured with clear documentation. The
DecryptSecretsBlobRPC provides a clean interface for blob-based secret decryption with appropriate authentication fields. The Buf static analysis warning about package directory is a false positive given the project's directory structure.go/apps/krane/run.go (2)
72-95: LGTM!The Vault service initialization is properly guarded with checks for both
VaultMasterKeysandVaultS3.URL. Error handling is consistent and logging provides good observability.
146-157: Secrets service only available on Kubernetes backend.The Secrets service requires both
vaultSvcandtokenValidator. SincetokenValidatoris only created for the Kubernetes backend (lines 118-121), the Secrets service will never be registered when using the Docker backend. This appears intentional for the current scope, but ensure this limitation is documented if Docker-based deployments should eventually support secret injection.go/pkg/secrets/provider/provider.go (1)
57-67: LGTM! Clean factory pattern with proper sentinel error wrapping.The factory correctly wraps
ErrProviderNotFoundwith context using%w, enabling callers to useerrors.Is()for error checking while still providing the unknown provider type in the message.go/k8s/manifests/secrets-webhook.yaml (1)
142-142: Hardcoded caBundle requires manual rotation.The caBundle contains a base64-encoded mkcert certificate. Per the retrieved learnings, this is acceptable for local development. For production, consider using cert-manager with automatic CA injection via the
cert-manager.io/inject-ca-fromannotation to avoid manual certificate management.Based on learnings, this is acceptable for local dev environments.
go/apps/krane/backend/kubernetes/deployment_create.go (3)
101-105: LGTM! Annotation and label enable webhook-based secret injection.The
unkey.com/inject: truelabel triggers the MutatingWebhookConfiguration's objectSelector, and theunkey.com/deployment-idannotation provides the deployment context needed for secret retrieval. This correctly integrates with the secrets-webhook defined in the manifest.
203-216: LGTM! OwnerReferences enable automatic Service cleanup.Setting the StatefulSet as the owner of the Service ensures Kubernetes garbage collection will automatically delete the Service when the StatefulSet is deleted. Combined with the manual rollback on line 196-198, this provides robust cleanup in both success and failure scenarios.
162-167: The implementation is correct. TheEncryptedSecretsBlobis raw bytes from the proto, which must be base64-encoded to be set as a string environment variable. The consumer inunkey-envexplicitly expects base64-encoded input (as documented in the flag at line 64 ofgo/cmd/unkey-env/main.go) and immediately decodes it back to bytes for processing. No double-encoding occurs.go/apps/ctrl/workflows/deploy/deploy_handler.go (2)
131-154: LGTM! Deployment request correctly passes encrypted secrets context.The deployment request properly includes
EnvironmentSlug,EncryptedSecretsBlob, andEnvironmentId, which aligns with the updatedDeploymentRequestproto and enables the secrets-webhook to decrypt secrets at runtime using the correct keyring.
122-129: Verify the vault service'sGetEncrypted()return type and any encoding mechanisms.The string-to-byte conversion may be appropriate if
GetEncrypted()returns a plaintext string representation of binary data. However, if the encrypted data is base64-encoded or uses another text encoding, converting it directly to[]byte()could lose important encoding information. Confirm the vault service's contract for theEncryptresponse and whether the encrypted data should remain as a string or be converted to bytes at this point.go/apps/secrets-webhook/routes/mutate/handler.go (1)
26-65: LGTM on the admission handler flow.The handler correctly:
- Validates the admission review request with nil check
- Extracts and unmarshals the pod from the request
- Delegates mutation to the Mutator service
- Returns appropriate responses based on mutation result
The contextual logging with pod name, namespace, and UID is helpful for debugging.
go/apps/secrets-webhook/internal/services/mutator/mutator.go (3)
14-26: LGTM on the Mutator structure and constructor.Clean dependency injection pattern with config, logger, and registry.
34-40: LGTM on the mutation eligibility check.The nil guard on annotations prevents panics, and the deployment ID annotation check aligns with the PR's design.
42-109: LGTM on the mutation orchestration logic.The patch construction correctly handles:
- Empty vs. existing
initContainersarrays (usingaddvs.add/-)- Empty vs. existing
volumesarrays- Per-container patch delegation with proper error propagation
The JSON patch semantics are correctly applied.
go/cmd/unkey-env/main.go (5)
27-46: LGTM on Config struct and validation.The validation correctly enforces required fields and the mutual exclusivity of
Tokenvs.TokenPath.
48-74: LGTM on CLI flag definitions.Good use of environment variables for configuration, appropriate required/optional flag semantics.
103-114: Minor: Logger enrichment only happens in debug mode.The logger is enriched with deployment context only when
cfg.Debugis true (line 106), butlogger.Debug()calls won't output anyway when debug is disabled. This is fine, just noting the coupling.
148-160: LGTM on environment variable sanitization.Good security practice:
- Removes internal
UNKEY_*vars (except allowlisted ones) before exec- Logs only keys, not values, when setting secrets
162-176: LGTM on the exec logic.The use of
syscall.Execcorrectly replaces the process image, which is the intended behavior for an "exec into" wrapper. The unreachablereturn nilis necessary for Go's type system.go/apps/secrets-webhook/internal/services/mutator/patch.go (5)
10-28: LGTM on the init container construction.The previous issue with hardcoded
PullNeverhas been addressed. The pull policy now defaults toPullIfNotPresentwith a configurable override.
30-39: LGTM on the volume construction.Using
StorageMediumMemoryfor the EmptyDir is appropriate for theunkey-envbinary.
72-87: Env var patching logic is now correct.The previous bug where the condition was checked per-iteration has been fixed. The logic now correctly:
- Adds all env vars as an array if
container.Envis empty- Appends each env var individually if
container.Envalready exists
89-126: LGTM on command/args patching logic.The logic correctly handles all scenarios:
- No Command, no Args: Uses
Entrypoint + Cmdfrom image config- No Command, has Args: Uses
Entrypoint + container.Args(image's Cmd is skipped per Docker convention)- Has Command: Uses
Command + ArgsThe resulting container will run
unkey-env [original-entrypoint/command] [original-args].
131-137: No changes needed — environment variable injection is complete for the current design.The
buildEnvVars()function correctly injects the three variables that thepodConfigstruct supports (UNKEY_PROVIDER_ENDPOINT,UNKEY_DEPLOYMENT_ID,UNKEY_TOKEN_PATH). The struct does not include fields forUNKEY_ENVIRONMENT_IDorUNKEY_SECRETS_BLOB, so these variables cannot be injected without first adding them to the struct and loading them from pod annotations. If those variables are needed in the future, the architecture already supports extending it through the annotation-based configuration pattern used byloadPodConfig().Likely an incorrect or invalid review comment.
go/apps/krane/backend/docker/deployment_create.go (1)
11-14: New proto/vault imports and protojson usage are coherentThe added imports (ctrlv1, vaultv1, protojson) align with the new
decryptSecretshelper and are scoped correctly to this file; no issues from a dependency or layering standpoint.
* feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes * feat: dashboard UI for environment variables management * fix comment and rename file * fix file export name * Remove unnecessary comments from add-env-vars * add toasts for environment variable operations * [autofix.ci] apply automated fixes * fix: add try/catch error handling to env var mutations * unfmt file * [autofix.ci] apply automated fixes * feat: decrypt env vars in CTRL workflow before passing to Krane * feat: inject env vars into pod spec via Krane * feat: add customer-workload service account for pod isolation * remove gw from k8s manifest, add agent fix ctrl vault for certs * seperate master keys too * add inital webhook stuff * add generated stuff * adjust comments * use otel lgtm stack in k8s too * fix some rabbit comments * fix some rabbit comments * get rid of some unncessary comments * actually add unkey env cmd gitignores... * fix golint issues (#4477) * [autofix.ci] apply automated fixes * fix fmt * linter be happy --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com>
* feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes * feat: dashboard UI for environment variables management * fix comment and rename file * fix file export name * Remove unnecessary comments from add-env-vars * add toasts for environment variable operations * [autofix.ci] apply automated fixes * fix: add try/catch error handling to env var mutations * unfmt file * [autofix.ci] apply automated fixes * feat: decrypt env vars in CTRL workflow before passing to Krane * feat: inject env vars into pod spec via Krane * feat: add customer-workload service account for pod isolation * remove gw from k8s manifest, add agent fix ctrl vault for certs * seperate master keys too * add inital webhook stuff * add generated stuff * adjust comments * use otel lgtm stack in k8s too * fix some rabbit comments * fix some rabbit comments * get rid of some unncessary comments * actually add unkey env cmd gitignores... * fix golint issues * Fix/update validation issues status label (#4478) * 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> * chore: Remove un-used UI components (#4472) * removed un used components * updated members refs --------- Co-authored-by: James P <james@unkey.dev> Co-authored-by: Andreas Thomas <dev@chronark.com> * perf: fix n+1 (#4484) * fix: add 403 error when 0 key verification perms (#4483) * fix: add 403 error when 0 key verification perms * cleanup tests * feat: add environment variables db schema and queries (#4450) * feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars (#4451) * feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> * feat: add GetPullToken * feat: dashboard UI for environment variables management (#4452) * feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes * feat: dashboard UI for environment variables management * fix comment and rename file * fix file export name * Remove unnecessary comments from add-env-vars * add toasts for environment variable operations * [autofix.ci] apply automated fixes * fix: add try/catch error handling to env var mutations * unfmt file * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> * feat: decrypt env vars in CTRL workflow before passing to Krane (#4453) * feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes * feat: dashboard UI for environment variables management * fix comment and rename file * fix file export name * Remove unnecessary comments from add-env-vars * add toasts for environment variable operations * [autofix.ci] apply automated fixes * fix: add try/catch error handling to env var mutations * unfmt file * [autofix.ci] apply automated fixes * feat: decrypt env vars in CTRL workflow before passing to Krane --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> * feat: inject env vars into pod spec via Krane (#4454) * feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes * feat: dashboard UI for environment variables management * fix comment and rename file * fix file export name * Remove unnecessary comments from add-env-vars * add toasts for environment variable operations * [autofix.ci] apply automated fixes * fix: add try/catch error handling to env var mutations * unfmt file * [autofix.ci] apply automated fixes * feat: decrypt env vars in CTRL workflow before passing to Krane * feat: inject env vars into pod spec via Krane --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> * feat: add customer-workload service account for pod isolation (#4455) * feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes * feat: dashboard UI for environment variables management * fix comment and rename file * fix file export name * Remove unnecessary comments from add-env-vars * add toasts for environment variable operations * [autofix.ci] apply automated fixes * fix: add try/catch error handling to env var mutations * unfmt file * [autofix.ci] apply automated fixes * feat: decrypt env vars in CTRL workflow before passing to Krane * feat: inject env vars into pod spec via Krane * feat: add customer-workload service account for pod isolation --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> * remove gw from k8s manifest, add agent fix ctrl vault for certs (#4463) * feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes * feat: dashboard UI for environment variables management * fix comment and rename file * fix file export name * Remove unnecessary comments from add-env-vars * add toasts for environment variable operations * [autofix.ci] apply automated fixes * fix: add try/catch error handling to env var mutations * unfmt file * [autofix.ci] apply automated fixes * feat: decrypt env vars in CTRL workflow before passing to Krane * feat: inject env vars into pod spec via Krane * feat: add customer-workload service account for pod isolation * remove gw from k8s manifest, add agent fix ctrl vault for certs * seperate master keys too --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> * chore: Make Stripe Great Again (#4479) * fix: Make stripe webhooks more robust * chore: Move alert to UI (#4485) * Moved alert to ui and swapped usages * feat: better env var injection (#4468) * feat: add environment variables db schema and queries * fix db query * feat: add SecretsConfig proto for encrypted env vars * [autofix.ci] apply automated fixes * feat: dashboard UI for environment variables management * fix comment and rename file * fix file export name * Remove unnecessary comments from add-env-vars * add toasts for environment variable operations * [autofix.ci] apply automated fixes * fix: add try/catch error handling to env var mutations * unfmt file * [autofix.ci] apply automated fixes * feat: decrypt env vars in CTRL workflow before passing to Krane * feat: inject env vars into pod spec via Krane * feat: add customer-workload service account for pod isolation * remove gw from k8s manifest, add agent fix ctrl vault for certs * seperate master keys too * add inital webhook stuff * add generated stuff * adjust comments * use otel lgtm stack in k8s too * fix some rabbit comments * fix some rabbit comments * get rid of some unncessary comments * actually add unkey env cmd gitignores... * fix golint issues (#4477) * [autofix.ci] apply automated fixes * fix fmt * linter be happy --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> * make token pod owned * feat: add lets encrypt challenges (#4471) * feat: add lets encrypt challenges * always disable cname following * cleanup some code * cleanup some code * cleanup some code * cleanup some code * cleanup some code * fix golint issues * fix golint issues * fmt * remove old webhook code * remove old webhook code * make build id not optiona * cleanup * cleanup * fmt * fmt --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: abhay <88815641+theabhayprajapati@users.noreply.github.com> Co-authored-by: CodeReaper <148160799+MichaelUnkey@users.noreply.github.com> Co-authored-by: James P <james@unkey.dev> Co-authored-by: Andreas Thomas <dev@chronark.com>
2 participants
What does this PR do?
Secrets injection via mutating webhook
Adds a Kubernetes mutating webhook to inject environment variables at runtime without storing them in K8s Secrets or pod specs (where they'd be plaintext in etcd).
Based on https://bank-vaults.dev/docs/mutating-webhook/.
How it works:
Secrets are never stored in etcd or visible via kubectl describe - they only exist in process memory at runtime.
Also changes the keyring for the env vars to be the environment id opposed to the workspace id.
Secrets Injection Architecture
Blame me if something looks off.
sequenceDiagram participant User participant Ctrl as ctrl participant Vault participant Krane as krane participant K8s as K8s API participant Webhook as secrets-webhook participant Pod participant App as Customer App rect rgb(240, 248, 255) Note over User,Krane: Deployment Creation User->>Ctrl: Create deployment Ctrl->>Vault: Encrypt secrets (env_id keyring) Vault-->>Ctrl: Encrypted blob Ctrl->>Krane: CreateDeployment(encrypted_blob) Krane->>K8s: Create Pod Note right of Krane: Annotations:<br/>unkey.com/inject: true<br/>unkey.com/deployment-id: d_xxx<br/>Env: UNKEY_SECRETS_BLOB end rect rgb(255, 245, 238) Note over K8s,Webhook: Pod Mutation K8s->>Webhook: AdmissionReview Webhook->>Webhook: Build JSON patches Note right of Webhook: 1. Add init container<br/>2. Add volume<br/>3. Rewrite cmd → unkey-env<br/>4. Add UNKEY_* env vars Webhook-->>K8s: Patches K8s->>Pod: Create mutated pod end rect rgb(240, 255, 240) Note over Pod,App: Runtime Secrets Injection Pod->>Pod: Init: copy /unkey-env Pod->>Pod: Start container Pod->>Krane: DecryptSecretsBlob(token, deployment_id, blob) Krane->>K8s: TokenReview (validate SA token) K8s-->>Krane: ✓ Authenticated + pod name Krane->>K8s: Get Pod annotations K8s-->>Krane: unkey.com/deployment-id Krane->>Krane: Verify annotation matches Krane->>Vault: Decrypt blob Vault-->>Krane: Plaintext secrets Krane-->>Pod: {KEY: value, ...} Pod->>Pod: Set env vars Pod->>Pod: Sanitize UNKEY_* vars Pod->>App: syscall.Exec(original cmd) Note right of App: App runs with<br/>secrets as env vars endArchitecture Overview
flowchart TB subgraph CP[Control Plane] ctrl[ctrl] vault[(Vault<br/>S3 + KEK)] end subgraph K8s[Kubernetes Cluster] api[K8s API Server] webhook[secrets-webhook<br/>MutatingAdmissionWebhook] krane[krane<br/>SecretsService] subgraph pod[Customer Pod] init[init: copy-unkey-env] env[unkey-env] app[Customer App] end end ctrl -->|1. Encrypt secrets| vault ctrl -->|2. CreateDeployment| krane krane -->|3. Create Pod| api api <-->|4. AdmissionReview| webhook api -->|5. Create Pod| pod init -.->|6. Copy binary| env env -->|7. DecryptSecretsBlob| krane krane -->|8. TokenReview| api krane -->|9. Decrypt| vault env -.->|10. exec with env| app style vault fill:#ffec99 style webhook fill:#ffc9c9 style krane fill:#b2f2bb style env fill:#ffe066 style app fill:#69db7cComponent Responsibilities
flowchart LR subgraph webhook[secrets-webhook] direction TB w1[Intercept pod creation] w2[Check annotations] w3[Inject init container] w4[Rewrite command] w5[Add env vars] end subgraph krane[krane SecretsService] direction TB k1[Validate SA token] k2[Check pod annotation] k3[Decrypt blob via Vault] k4[Return plaintext] end subgraph unkeyenv[unkey-env] direction TB u1[Read SA token] u2[Call krane API] u3[Set env vars] u4[Sanitize UNKEY_*] u5[exec original cmd] end webhook --> krane --> unkeyenvSecurity Model
flowchart TB subgraph auth[Authentication] sa[Service Account Token] tr[TokenReview API] ann[Pod Annotation Check] sa --> tr --> ann end subgraph encryption[Encryption at Rest] outer[Outer layer: SecretsConfig blob] inner[Inner layer: Each secret value] kek[KEK in Vault] outer --> kek inner --> kek end subgraph k8s_isolation[K8s Isolation] ns[Per-customer namespace] rbac[customer-workload SA has no K8s API access] end subgraph env_isolation[Env Var Isolation] sanitize[Sanitize internal UNKEY_* vars before exec] allowlist[Only pass UNKEY_DEPLOYMENT_ID, UNKEY_REGION, etc] end auth -->|✓ Valid| encryptionType of change
How should this be tested?
make dev
run new deployment and any env vars that have been set in the env before deployment should be in the container try using the demo_api as context and call the /env endpoint.
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated