chore: cleanup unkey env and add goreleaser cfg#4575
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe pull request refactors the secrets injection system by renaming the "unkey-env" binary to "inject". This includes creating a new Go implementation in cmd/inject, deleting the legacy cmd/unkey-env implementation, introducing build and release configurations, and updating all dependent configurations across the preflight service, Kubernetes manifests, and infrastructure code. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 7
🤖 Fix all issues with AI agents
In @cmd/inject/command.go:
- Around line 10-34: The validation currently allows both Token and TokenPath or
neither; change the validation used by the command (the method that currently
checks Token != "" || TokenPath != "") to require exactly one of Token XOR
TokenPath and return a clear error if both or neither are set. Also add a check
that EnvironmentID is non-empty when SecretsBlob (secrets-blob) is provided
(analogous to how DeploymentID and Endpoint are validated), since EnvironmentID
is passed into FetchSecrets(); return an error if missing. Finally, introduce
provider-specific validation by switching on the provider flag value (compare
against provider.KraneVault and other provider constants) and enforce any
provider-specific required flags (e.g., for KraneVault ensure Endpoint and
DeploymentID are present), returning descriptive errors for missing
provider-required fields. Ensure all errors are surfaced from the command
validation path so Action/run.go (FetchSecrets) receives only valid inputs.
In @cmd/inject/config.go:
- Around line 33-46: The validate() method on config currently skips checking
for an EnvironmentID when hasSecrets() is true; add a check to the assert.All
block to require EnvironmentID (e.g., assert.NotEmpty(c.EnvironmentID,
"environment-id is required when secrets-blob is provided")) so blob decryption
always has the keyring specified; update config.validate to include this new
assertion alongside Endpoint, DeploymentID, and token/token-path checks.
In @cmd/inject/main.go:
- Around line 1-14: Add the missing import for the package that defines cmd.Run:
update the import block to include the module path for the local command package
that exposes Run (import it as cmd if it's named differently), so main can call
cmd.Run without a compile error; ensure the import path matches your module and
the package actually declares Run(ctx context.Context, args []string) error.
In @cmd/inject/run.go:
- Around line 17-44: The run function currently calls execCommand(cfg.Args,
logger) without validating cfg.Args, which can cause a panic when cfg.Args is
empty; add a defensive check in run before calling execCommand: if len(cfg.Args)
== 0 return a clear error (or fmt.Errorf) indicating missing command/arguments
so the program fails gracefully; update any tests or callers expecting panic to
expect this error instead.
In @pkg/secrets/provider/krane_vault.go:
- Around line 73-87: When reading a token from a file in
resolveToken(FetchOptions), trim the token and validate it is non-empty before
returning; if the trimmed token is empty return an error (e.g., via
assert.NotEmpty or fmt.Errorf) stating the token is required. Specifically,
after os.ReadFile in resolveToken, call strings.TrimSpace on tokenBytes, check
that the result is not empty, and only then return it; otherwise return a clear
error instead of returning an empty string.
In @svc/preflight/config.go:
- Around line 5-13: Add validation for Config.InjectImagePullPolicy so invalid
strings can't propagate: ensure the field is non-empty and matches a kube-style
allowlist (e.g., "Always", "IfNotPresent", "Never") or your chosen set.
Implement this check in a Config.Validate() method (or in the loader that
constructs Config) and return an error when the value is empty or not in the
allowlist; reference the Config type and InjectImagePullPolicy field when
locating where to add the check. Ensure callers of Config.Validate() reject
invalid configs so manifests/mutator logic never receive bad pull-policy values.
🧹 Nitpick comments (5)
cmd/inject/Dockerfile (1)
16-17: Consider testing the upx-compressed binary.The
upx --best --lzmacompression is very aggressive and can occasionally cause issues with Go binaries (runtime panics, binary corruption). Ensure the compressed binary is thoroughly tested before deployment.svc/preflight/internal/services/mutator/mutator.go (1)
133-133: Consider preserving component specificity in the log message.The message change from "injected unkey-env for deployment %s" to "injected secrets for deployment %s" makes the log less specific. If multiple injection mechanisms are introduced in the future, this generic message could make troubleshooting more difficult.
Consider: "injected secrets via inject for deployment %s" or simply "injected inject container for deployment %s" to maintain component traceability in logs.
cmd/preflight/main.go (1)
20-23: Consider compatibility aliases + revisitlatestdefault vs pull policyRemoving the old
unkey-env-*flags/env vars is a breaking CLI/config change; if you want smoother upgrades, consider adding hidden/deprecated aliases that map to the new keys.Also,
inject:latestcombined withIfNotPresentis a common footgun (stale “latest” on nodes). If “latest” is the default, consider defaulting pull policy toAlways, or default the image to a versioned/fully-qualified ref.Proposed compatibility sketch (minimal)
Flags: []cli.Flag{ @@ cli.String("inject-image", "Container image for inject binary", cli.Default("inject:latest"), cli.EnvVar("INJECT_IMAGE")), cli.String("inject-image-pull-policy", "Image pull policy (Always, IfNotPresent, Never)", cli.Default("IfNotPresent"), cli.EnvVar("INJECT_IMAGE_PULL_POLICY")), + // Back-compat (optional): consider hiding/deprecating these if cli supports it + cli.String("unkey-env-image", "DEPRECATED: use --inject-image", + cli.EnvVar("UNKEY_ENV_IMAGE")), + cli.String("unkey-env-image-pull-policy", "DEPRECATED: use --inject-image-pull-policy", + cli.EnvVar("UNKEY_ENV_IMAGE_PULL_POLICY")),cmd/inject/config.go (1)
8-15: Nit: considermap[string]struct{}for set semanticsNot required, but avoids storing redundant booleans and signals intent.
cmd/inject/run.go (1)
86-99: Remove unreachable code aftersyscall.Exec(optional).On success,
syscall.Execnever returns, soreturn nilis dead code.Proposed tweak
if err := syscall.Exec(binary, args, os.Environ()); err != nil { return fmt.Errorf("exec failed: %w", err) } - - return nil }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gen/proto/krane/v1/scheduler.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (24)
.github/workflows/release-inject.yaml.gitignoreDockerfile.unkey-envTiltfilecmd/inject/.goreleaser.yamlcmd/inject/Dockerfilecmd/inject/command.gocmd/inject/config.gocmd/inject/main.gocmd/inject/run.gocmd/preflight/main.gocmd/unkey-env/Dockerfilecmd/unkey-env/main.gok8s/manifests/preflight.yamlk8s/manifests/rbac.yamlpkg/secrets/provider/krane_vault.gopkg/secrets/provider/provider.gosvc/krane/proto/krane/v1/scheduler.protosvc/krane/proto/krane/v1/secrets.protosvc/preflight/config.gosvc/preflight/internal/services/mutator/config.gosvc/preflight/internal/services/mutator/mutator.gosvc/preflight/internal/services/mutator/patch.gosvc/preflight/run.go
💤 Files with no reviewable changes (3)
- Dockerfile.unkey-env
- cmd/unkey-env/Dockerfile
- cmd/unkey-env/main.go
🧰 Additional context used
🧠 Learnings (9)
📓 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: 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.
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.
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.
📚 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:
k8s/manifests/preflight.yamlcmd/preflight/main.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:
k8s/manifests/preflight.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 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:
cmd/inject/config.gosvc/preflight/config.go
📚 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:
svc/krane/proto/krane/v1/scheduler.protosvc/preflight/internal/services/mutator/mutator.gosvc/preflight/internal/services/mutator/config.gosvc/preflight/internal/services/mutator/patch.gocmd/preflight/main.go
📚 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:
.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:
.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, 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:
Tiltfile
📚 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:
cmd/preflight/main.go
🧬 Code graph analysis (4)
cmd/inject/config.go (1)
pkg/secrets/provider/provider.go (2)
Provider(19-26)Type(11-11)
cmd/inject/command.go (1)
pkg/secrets/provider/provider.go (3)
KraneVault(15-15)Provider(19-26)Type(11-11)
svc/preflight/internal/services/mutator/patch.go (1)
svc/preflight/internal/services/mutator/mutator.go (1)
Mutator(28-36)
pkg/secrets/provider/krane_vault.go (1)
pkg/secrets/provider/provider.go (1)
FetchOptions(29-46)
🪛 actionlint (1.7.10)
.github/workflows/release-inject.yaml
17-17: label "depot-ubuntu-24.04-4" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (26)
svc/krane/proto/krane/v1/secrets.proto (1)
8-8: Documentation update aligns with "unkey-env" → "inject" refactor.The comment clearly identifies the new component name and its deployment context. No structural or functional changes to the service definition.
svc/krane/proto/krane/v1/scheduler.proto (1)
152-154: Documentation accurately reflects the "inject" runtime secret decryption flow.The updated comments clearly explain how encrypted secrets are handled: the blob is set as an environment variable, and the
injectbinary callsDecryptSecretsBlobat runtime. No changes to the field definition or proto contract..gitignore (1)
36-39: Verify completeness of the rename and pattern name consistency.There appears to be an inconsistency between the AI summary and the annotated code:
- The summary states: "Replaced ignore pattern
/unkey-envwith/inject"- The annotated code shows:
/unkey(line 36, unmarked) followed by/inject(line 37, marked with~)Please clarify:
- Was the original pattern
/unkeyor/unkey-env? (The code shows/unkeybut the summary mentions/unkey-env)- Should the
/unkeyentry at line 36 also be removed or replaced as part of this rename, or is it a separate, unrelated pattern?- If both should be changed, why is line 36 not marked with
~in the diff?The goreleaser additions (
dist/) at lines 38–39 look appropriate.pkg/secrets/provider/provider.go (1)
2-2: LGTM! Documentation updated to reflect the rename.The comment update aligns with the PR's objective to rename "unkey-env" to "inject" throughout the codebase.
svc/preflight/internal/services/mutator/config.go (2)
13-15: LGTM! Constants renamed consistently.The internal constants have been renamed from
unkeyEnv*toinject*to align with the new inject workflow.
31-32: All references to renamed fields have been successfully updated.The search confirmed that no references to the old field names (
UnkeyEnvImage,UnkeyEnvImagePullPolicy) exist in the codebase, and the new field names (InjectImage,InjectImagePullPolicy) are consistently used across all relevant files:svc/preflight/config.go,svc/preflight/internal/services/mutator/config.go,svc/preflight/internal/services/mutator/mutator.go,svc/preflight/run.go, andcmd/preflight/main.go.cmd/inject/Dockerfile (2)
22-22: LGTM! CA certificates are properly included.Adding CA certificates is essential for the inject binary to make HTTPS calls to secrets providers.
2-2: No action required.Go 1.25 is the latest stable version as of January 2026 (specifically 1.25.5, released December 2, 2025). The Dockerfile is using an appropriate, current Go version.
k8s/manifests/rbac.yaml (1)
14-14: LGTM! Comment updated to reflect the rename.The comment has been updated from "unkey-env" to "inject" to align with the new naming convention.
k8s/manifests/preflight.yaml (1)
74-77: LGTM! Clean environment variable rename.The environment variable names have been updated from
UNKEY_ENV_IMAGEandUNKEY_ENV_IMAGE_PULL_POLICYtoINJECT_IMAGEandINJECT_IMAGE_PULL_POLICY, aligning with the component rename throughout the PR. The values are appropriate for local development.svc/preflight/internal/services/mutator/mutator.go (1)
33-34: LGTM! Field renames align with component renaming.The struct fields and constructor initialization have been correctly updated to use
injectImageandinjectImagePullPolicy, maintaining consistency with the broader inject naming convention.Also applies to: 44-45
cmd/inject/command.go (1)
36-54: LGTM! Clean action handler with proper error handling.The action function correctly:
- Builds configuration from CLI flags
- Validates before execution
- Returns appropriate exit codes on validation failure
- Delegates execution to the
run()function.github/workflows/release-inject.yaml (2)
17-17: Static analysis warning about runner label is expected.The
depot-ubuntu-24.04-4runner is flagged as unknown by actionlint because it's a custom runner used by the Unkey organization (likely provided by Depot, as evidenced by Depot integration elsewhere in the codebase). This is acceptable for organization-specific infrastructure.If this causes confusion for contributors, consider adding a comment in the workflow file explaining the custom runner.
Based on learnings, if this review concern warrants tracking but can be deferred, would you like me to create a GitHub issue with detailed sections (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan)?
1-51: LGTM! Well-structured release workflow with security best practices.The workflow correctly:
- Uses semantic version tag triggers for automated releases
- Pins actions to specific commit SHAs for supply chain security
- Sets appropriate permissions (contents, packages)
- Configures concurrency control to prevent parallel releases
- Authenticates with GHCR for Docker image publishing
- Extracts version from tags properly
- Uses goreleaser-pro with dedicated config
svc/preflight/run.go (1)
76-77: LGTM! Configuration field updates align with the rename.The mutator configuration initialization has been correctly updated to use
InjectImageandInjectImagePullPolicy, maintaining consistency with the field renames in the mutator package.cmd/preflight/main.go (1)
32-41: Config wiring to Inject looks consistent*The command → config mapping is straightforward and matches the renamed fields.
cmd/inject/.goreleaser.yaml (1)
1-57: Tag format is correct—no action needed on.VersionnormalizationGoReleaser v2 automatically strips the leading
vfrom git tags. If your tag isv1.2.3,{{ .Version }}produces1.2.3, resulting in clean GHCR tags likeghcr.io/unkeyed/inject:1.2.3-amd64(standard Docker convention). No explicit trimming or normalization is needed.Regarding the changelog filter: if your team uses many
chore:commits, the exclusion of^chore:may result in sparse release notes. Review your commit conventions to confirm this won't leave releases undocumented.svc/preflight/internal/services/mutator/patch.go (3)
25-34: Volume rename toinjectVolumeNamelooks consistent.
48-121: EnsureinjectBinarypoints into the mounted volume path.You set container
commandtoinjectBinary(Line 112) and mount theEmptyDiratinjectMountPath(Line 50). IfinjectBinaryis not located underinjectMountPath, the exec will fail in the mutated container.This is especially important given the init-container copy step.
10-22: No action required. The code is correct and does not have a mount shadowing bug.The init container copies from
/inject(image filesystem) to/inject-bin/inject(destination in the shared EmptyDir volume). These are different paths—/injectis not masked by the volume mount at/inject-bin. The pattern is sound: the source path on the image and the destination path in the shared volume are properly separated.Likely an incorrect or invalid review comment.
cmd/inject/run.go (2)
46-75: Timeout + error wrapping look good for secret fetch.
77-84: Env sanitization approach is reasonable; ensure allowlist is intentional.Tiltfile (4)
21-23: Suppressinginject:latestwarnings matches the new injected image name.
285-292:build-injectlocal_resource wiring looks consistent withinject:latest.
307-309: Preflight deps includebuild-inject(good).
151-160: No stale references found after renaming tobuild-unkey.The codebase contains no references to
unkey-compileorunkey-env, confirming the rename is clean.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @svc/preflight/config.go:
- Line 26: In Validate(), treat an empty InjectImagePullPolicy as a valid
default (mirror HttpPort behavior) by assigning a sensible default value before
checking the validImagePullPolicies map (e.g., set c.InjectImagePullPolicy =
"IfNotPresent" when it is ""), and also add an explicit check that c.InjectImage
is non-empty (return assert.True/False with a clear message) so callers that
don't go through the CLI still pass validation; update the check using the
validImagePullPolicies[c.InjectImagePullPolicy] lookup after the defaulting.
Ensure you modify the Validate method that references validImagePullPolicies and
the fields InjectImagePullPolicy and InjectImage.
- Around line 12-19: Validate() currently doesn't ensure TLSCertFile and
TLSKeyFile are set even though preflight.Run calls tls.NewFromFiles() and
cmd/preflight/main.go uses cmd.RequireString(); update the Validate() method to
check TLSCertFile and TLSKeyFile are non-empty and return a descriptive error
(or aggregate error) if either is missing so misconfiguration is caught early
before preflight.Run executes.
🧹 Nitpick comments (5)
svc/preflight/config.go (1)
5-9: TreatvalidImagePullPoliciesas an immutable set (avoid bool-map footguns).
map[string]boolworks, but it’s easy to accidentally “validate” unknown keys by setting them elsewhere. Consider usingmap[string]struct{}and_, ok := ...in Validate.Proposed refactor
-var validImagePullPolicies = map[string]bool{ - "Always": true, - "IfNotPresent": true, - "Never": true, -} +var validImagePullPolicies = map[string]struct{}{ + "Always": {}, + "IfNotPresent": {}, + "Never": {}, +}cmd/preflight/main.go (1)
20-23: Breaking change: flag/env rename—consider a compatibility alias (even if hidden) for one release.
If existing users are onUNKEY_ENV_*/unkey-env-*, this will break upgrades unless all manifests/helm/terraform are updated in lockstep. If you want a soft migration, you can accept both and prefer the new value when both are set.pkg/secrets/provider/krane_vault.go (1)
44-46: Consider returning nil for consistency.Returning
nilinstead of an empty map would be more idiomatic when no secrets are present, though the current approach is also valid and prevents potential nil pointer issues downstream.♻️ Optional refactor
if len(opts.Encrypted) == 0 { - return make(map[string]string), nil + return nil, nil }cmd/inject/run.go (2)
18-20: Redundant validation check.This check is redundant since
config.validate()already ensuresArgsis non-empty. Consider removing this for cleaner code.♻️ Optional refactor
func run(ctx context.Context, cfg config) error { - if len(cfg.Args) == 0 { - return fmt.Errorf("no command specified") - } - logger := logging.New()
90-103: Unreachable return statement.Line 102's
return nilis unreachable becausesyscall.Execreplaces the current process on success and only returns on failure (which is handled on line 99). While harmless, removing it would make the control flow clearer.♻️ Optional refactor
if err := syscall.Exec(binary, args, os.Environ()); err != nil { return fmt.Errorf("exec failed: %w", err) } - - return nil }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/release_inject.yamlcmd/inject/config.gocmd/inject/run.gocmd/preflight/main.gopkg/secrets/provider/krane_vault.gosvc/preflight/config.go
🧰 Additional context used
🧠 Learnings (10)
📓 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: 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.
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.
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.
📚 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:
cmd/preflight/main.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:
cmd/preflight/main.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:
cmd/preflight/main.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:
pkg/secrets/provider/krane_vault.gosvc/preflight/config.gocmd/inject/config.go
📚 Learning: 2025-09-11T14:12:30.570Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3944
File: go/apps/ctrl/run.go:292-299
Timestamp: 2025-09-11T14:12:30.570Z
Learning: In go/apps/ctrl/config.go, the Validate() method already includes validation for Cloudflare API token when ACME and Cloudflare are both enabled, checking that cfg.Acme.Cloudflare.ApiToken is not empty.
Applied to files:
pkg/secrets/provider/krane_vault.gosvc/preflight/config.go
📚 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:
pkg/secrets/provider/krane_vault.go
📚 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: 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.
Applied to files:
pkg/secrets/provider/krane_vault.go
📚 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:
pkg/secrets/provider/krane_vault.go
📚 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:
pkg/secrets/provider/krane_vault.go
🧬 Code graph analysis (3)
pkg/secrets/provider/krane_vault.go (1)
pkg/secrets/provider/provider.go (1)
FetchOptions(29-46)
cmd/inject/run.go (1)
pkg/secrets/provider/provider.go (5)
New(61-68)Config(49-55)Type(11-11)Provider(19-26)FetchOptions(29-46)
cmd/inject/config.go (1)
pkg/secrets/provider/provider.go (3)
Provider(19-26)Type(11-11)KraneVault(15-15)
🪛 actionlint (1.7.10)
.github/workflows/release_inject.yaml
17-17: label "depot-ubuntu-24.04-4" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (13)
cmd/preflight/main.go (1)
34-41: Config wiring is consistent; consider usingRequireStringforinject-imageif you ever drop the default.
With a default,cmd.String(...)is fine. If the image must always be explicit in prod, making it required at the CLI layer would catch misconfig earlier..github/workflows/release_inject.yaml (2)
1-51: LGTM - Well-structured release workflow.The workflow follows GoReleaser best practices with appropriate permissions, concurrency control, and Docker image building setup. The tag pattern and version extraction logic align with the inject command structure.
17-17: No action needed. The runner labeldepot-ubuntu-24.04-4is correctly configured. It is consistently used across 19 workflow files throughout the project, including critical test and deployment jobs, confirming the project has access to these Depot.dev runners.pkg/secrets/provider/krane_vault.go (4)
19-35: LGTM - Cleaner struct design.Removing the
endpointfield from the struct is appropriate since it's only needed during client initialization. The constructor still validates the endpoint is provided before creating the client.
48-51: LGTM - Clean token resolution.Extracting token resolution into a separate helper improves testability and makes the code easier to maintain.
53-71: LGTM - Streamlined decryption flow.The updated validation correctly requires
EnvironmentIDfor blob decryption, and the simplified flow that always usesDecryptSecretsBlobis clearer than the previous conditional logic.
73-91: LGTM - Robust token resolution.The helper properly handles both token sources with appropriate validation, including trimming whitespace and ensuring the token file is not empty. Error messages are clear and actionable.
cmd/inject/config.go (3)
10-29: LGTM - Well-designed config structure.The allowedUnkeyVars whitelist appropriately preserves deployment and environment context variables while clearing sensitive authentication data. The config struct fields align well with the provider's requirements.
31-33: LGTM - Clean helper method.Simple and clear helper that improves code readability.
35-55: LGTM - Comprehensive validation logic.The validation correctly enforces all requirements:
- Command arguments are mandatory
- XOR logic ensures exactly one of Token or TokenPath is provided
- Provider-specific requirements are properly validated
- Clear error messages guide users to fix configuration issues
cmd/inject/run.go (3)
22-48: LGTM - Well-structured execution flow.The function correctly orders operations: clears sensitive environment variables before injecting secrets, provides per-variable error handling, and includes helpful debug logging. The comment on lines 24-26 clearly explains the rationale for the clearing order.
50-79: LGTM - Robust secret fetching.The function properly initializes the provider, uses a reasonable timeout, handles base64 decoding, and passes all required options to the provider. Error messages provide helpful context.
81-88: LGTM - Safe environment cleanup.The function correctly identifies and removes sensitive UNKEY_* variables while preserving those in the allowlist. The use of
strings.Cutis appropriate and efficient.
Flo4604
requested review from
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
3 participants
What does this PR do?
Fixes #4561
Also renames unkey-env to
inject. I hate naming things.Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated