Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR replaces Metald with Krane across control-plane and deployment flows, introduces a new Krane service (Docker and Kubernetes backends), updates ctrl to use Krane, extends docs with new CLI pages/flags, modifies dev/deploy tooling, and removes the builderd, assetmanagerd, billaged, and metald apps and their clients, docs, and systemd artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Ctrl
participant KraneAPI as Krane Service
participant Backend as Backend (Docker/K8s)
participant Infra as Containers/StatefulSet
User->>Ctrl: Deploy request (workspace/project/env)
Ctrl->>KraneAPI: CreateDeployment(req)
KraneAPI->>Backend: Provision replicas
Backend->>Infra: Start containers/pods
KraneAPI-->>Ctrl: CreateDeploymentResponse(PENDING)
loop Poll until ready
Ctrl->>KraneAPI: GetDeployment(deployment_id)
KraneAPI->>Backend: Inspect instances
Backend-->>KraneAPI: Instances & statuses
KraneAPI-->>Ctrl: GetDeploymentResponse(instances,status)
end
Ctrl->>Ctrl: Upsert partition DB, gateway config
Ctrl-->>User: Deployment ready (instances, addresses)
sequenceDiagram
autonumber
participant Krane as Krane Run()
participant Logger
participant Backend as Backend (select)
participant HTTP as h2c Server
Krane->>Krane: Validate Config
Krane->>Logger: Init with context (instance, region, version)
alt Backend = Kubernetes
Krane->>Backend: Init K8s clientset (+optional eviction)
else Backend = Docker
Krane->>Backend: Init Docker client (socket)
end
Krane->>HTTP: Register DeploymentService
Krane->>HTTP: Start server (h2c)
Note over Krane,HTTP: Wait for shutdown signal and stop gracefully
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
chronark
changed the title
chronark
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
bot
requested review from
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/Tiltfile (1)
318-333: Include Krane inactive_services.We now advertise a Krane URL, but
active_servicesnever appends'krane', so the “Services running” summary skips it. Addif start_krane: active_services.append('krane')to keep the UI consistent.
🧹 Nitpick comments (2)
go/Tiltfile (1)
9-9: Document Krane in the services flag help.The help text for the
servicesflag still omitskrane, sotilt --helpwon’t tell folks that the new service is selectable. Please add it for accuracy.go/apps/krane/backend/kubernetes/service.go (1)
38-58: Allow out-of-cluster kubeconfig as a fallback.Hard-coding
rest.InClusterConfig()means the Kubernetes backend can only run from inside a cluster. Please consider falling back toclientcmd.BuildConfigFromFlags("", os.Getenv("KUBECONFIG"))(or an equivalent hook) so we can exercise the k8s backend from local/staging environments too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (36)
go/deploy/pkg/observability/interceptors/go.sumis excluded by!**/*.sumgo/deploy/pkg/spiffe/go.sumis excluded by!**/*.sumgo/deploy/pkg/telemetry/go.sumis excluded by!**/*.sumgo/deploy/pkg/tls/go.sumis excluded by!**/*.sumgo/gen/proto/assetmanagerd/v1/asset.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/assetmanagerd/v1/assetmanagerdv1connect/asset.connect.gois excluded by!**/gen/**go/gen/proto/billaged/v1/billagedv1connect/billing.connect.gois excluded by!**/gen/**go/gen/proto/billaged/v1/billing.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/builderd/v1/builder.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/builderd/v1/builderdv1connect/builder.connect.gois excluded by!**/gen/**go/gen/proto/krane/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/krane/v1/kranev1connect/deployment.connect.gois excluded by!**/gen/**go/gen/proto/metald/v1/deployment.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/metald.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/metaldv1connect/metald.connect.gois excluded by!**/gen/**go/gen/proto/metald/v1/network.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/storage.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metald/v1/vm.pb.gois excluded by!**/*.pb.go,!**/gen/**go/go.sumis excluded by!**/*.suminternal/proto/generated/assetmanagerd/v1/asset_pb.tsis excluded by!**/generated/**internal/proto/generated/billaged/v1/billing_pb.tsis excluded by!**/generated/**internal/proto/generated/builderd/v1/builder_pb.tsis excluded by!**/generated/**internal/proto/generated/ctrl/v1/acme_pb.tsis excluded by!**/generated/**internal/proto/generated/ctrl/v1/build_pb.tsis excluded by!**/generated/**internal/proto/generated/ctrl/v1/deployment_pb.tsis excluded by!**/generated/**internal/proto/generated/ctrl/v1/openapi_pb.tsis excluded by!**/generated/**internal/proto/generated/ctrl/v1/service_pb.tsis excluded by!**/generated/**internal/proto/generated/krane/v1/deployment_pb.tsis excluded by!**/generated/**internal/proto/generated/metald/v1/deployment_pb.tsis excluded by!**/generated/**internal/proto/generated/metald/v1/metald_pb.tsis excluded by!**/generated/**internal/proto/generated/metald/v1/network_pb.tsis excluded by!**/generated/**internal/proto/generated/metald/v1/storage_pb.tsis excluded by!**/generated/**internal/proto/generated/metald/v1/vm_pb.tsis excluded by!**/generated/**internal/proto/generated/partition/v1/gateway_pb.tsis excluded by!**/generated/**internal/proto/generated/vault/v1/object_pb.tsis excluded by!**/generated/**internal/proto/generated/vault/v1/service_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (107)
apps/engineering/content/docs/cli/run/ctrl/index.mdx(2 hunks)deploy.bash(0 hunks)deployment/04-seed-workspace.sql(1 hunks)deployment/docker-compose.yaml(3 hunks)go/Dockerfile(1 hunks)go/Makefile(1 hunks)go/Tiltfile(5 hunks)go/apps/assetmanagerd/.gitignore(0 hunks)go/apps/assetmanagerd/CHANGELOG.md(0 hunks)go/apps/assetmanagerd/Makefile(0 hunks)go/apps/assetmanagerd/README.md(0 hunks)go/apps/assetmanagerd/TODO.md(0 hunks)go/apps/assetmanagerd/client/Makefile(0 hunks)go/apps/assetmanagerd/client/client.go(0 hunks)go/apps/assetmanagerd/client/types.go(0 hunks)go/apps/assetmanagerd/cmd/assetmanagerd-cli/main.go(0 hunks)go/apps/assetmanagerd/cmd/assetmanagerd/main.go(0 hunks)go/apps/assetmanagerd/contrib/systemd/assetmanagerd.service(0 hunks)go/apps/assetmanagerd/environment.example(0 hunks)go/apps/assetmanagerd/internal/builderd/client.go(0 hunks)go/apps/assetmanagerd/internal/config/config.go(0 hunks)go/apps/assetmanagerd/internal/observability/otel.go(0 hunks)go/apps/assetmanagerd/internal/registry/registry.go(0 hunks)go/apps/assetmanagerd/internal/service/service.go(0 hunks)go/apps/assetmanagerd/internal/storage/local.go(0 hunks)go/apps/assetmanagerd/internal/storage/storage.go(0 hunks)go/apps/billaged/.gitignore(0 hunks)go/apps/billaged/CHANGELOG.md(0 hunks)go/apps/billaged/DOCUMENTATION_REPORT.md(0 hunks)go/apps/billaged/Makefile(0 hunks)go/apps/billaged/README.md(0 hunks)go/apps/billaged/TODO.md(0 hunks)go/apps/billaged/client/Makefile(0 hunks)go/apps/billaged/client/client.go(0 hunks)go/apps/billaged/client/types.go(0 hunks)go/apps/billaged/cmd/billaged-cli/main.go(0 hunks)go/apps/billaged/cmd/billaged/main.go(0 hunks)go/apps/billaged/contrib/grafana-dashboards/README.md(0 hunks)go/apps/billaged/contrib/systemd/README.md(0 hunks)go/apps/billaged/contrib/systemd/billaged.env.example(0 hunks)go/apps/billaged/contrib/systemd/billaged.service(0 hunks)go/apps/billaged/environment.example(0 hunks)go/apps/billaged/internal/aggregator/aggregator.go(0 hunks)go/apps/billaged/internal/config/config.go(0 hunks)go/apps/billaged/internal/observability/metrics.go(0 hunks)go/apps/billaged/internal/observability/otel.go(0 hunks)go/apps/billaged/internal/service/billing.go(0 hunks)go/apps/builderd/.gitignore(0 hunks)go/apps/builderd/CHANGELOG.md(0 hunks)go/apps/builderd/Makefile(0 hunks)go/apps/builderd/README.md(0 hunks)go/apps/builderd/client/Makefile(0 hunks)go/apps/builderd/client/client.go(0 hunks)go/apps/builderd/client/types.go(0 hunks)go/apps/builderd/cmd/builderd-cli/main.go(0 hunks)go/apps/builderd/cmd/builderd/main.go(0 hunks)go/apps/builderd/cmd/builderd/shutdown_test.go(0 hunks)go/apps/builderd/contrib/grafana-dashboards/README.md(0 hunks)go/apps/builderd/contrib/systemd/README.md(0 hunks)go/apps/builderd/contrib/systemd/builderd.env.example(0 hunks)go/apps/builderd/contrib/systemd/builderd.service(0 hunks)go/apps/builderd/internal/assetmanager/client.go(0 hunks)go/apps/builderd/internal/assets/base.go(0 hunks)go/apps/builderd/internal/config/config.go(0 hunks)go/apps/builderd/internal/executor/docker.go(0 hunks)go/apps/builderd/internal/executor/docker_pipeline.go(0 hunks)go/apps/builderd/internal/executor/docker_steps.go(0 hunks)go/apps/builderd/internal/executor/registry.go(0 hunks)go/apps/builderd/internal/executor/steps.go(0 hunks)go/apps/builderd/internal/executor/types.go(0 hunks)go/apps/builderd/internal/observability/metrics.go(0 hunks)go/apps/builderd/internal/observability/otel.go(0 hunks)go/apps/builderd/internal/service/builder.go(0 hunks)go/apps/ctrl/config.go(1 hunks)go/apps/ctrl/run.go(4 hunks)go/apps/ctrl/services/deployment/deploy_workflow.go(7 hunks)go/apps/krane/backend/docker/create_deployment.go(1 hunks)go/apps/krane/backend/docker/delete_deployment.go(1 hunks)go/apps/krane/backend/docker/get_deployment.go(1 hunks)go/apps/krane/backend/docker/service.go(1 hunks)go/apps/krane/backend/kubernetes/create_deployment.go(1 hunks)go/apps/krane/backend/kubernetes/delete_deployment.go(1 hunks)go/apps/krane/backend/kubernetes/eviction.go(1 hunks)go/apps/krane/backend/kubernetes/get_deployment.go(1 hunks)go/apps/krane/backend/kubernetes/id.go(1 hunks)go/apps/krane/backend/kubernetes/service.go(1 hunks)go/apps/krane/config.go(1 hunks)go/apps/krane/doc.go(1 hunks)go/apps/krane/run.go(1 hunks)go/apps/metald/.gitignore(0 hunks)go/apps/metald/CHANGELOG.md(0 hunks)go/apps/metald/Makefile(0 hunks)go/apps/metald/README.md(0 hunks)go/apps/metald/backend.go(0 hunks)go/apps/metald/backend_linux.go(0 hunks)go/apps/metald/backend_stub.go(0 hunks)go/apps/metald/client/Makefile(0 hunks)go/apps/metald/client/README.md(0 hunks)go/apps/metald/client/client.go(0 hunks)go/apps/metald/client/cmd/metald-cli/main.go(0 hunks)go/apps/metald/client/vmconfig.go(0 hunks)go/apps/metald/cmd/metald-init/INIT_PROCESS_GUIDE.md(0 hunks)go/apps/metald/cmd/metald-init/Makefile(0 hunks)go/apps/metald/cmd/metald-init/container.cmd(0 hunks)go/apps/metald/cmd/metald-init/go.mod(0 hunks)go/apps/metald/cmd/metald-init/main.go(0 hunks)go/apps/metald/cmd/metald-init/test-all.sh(0 hunks)
⛔ Files not processed due to max files limit (3)
- go/apps/metald/cmd/metald-init/test-env.sh
- go/apps/metald/cmd/metald-init/test-signals.sh
- go/apps/metald/cmd/metald-init/test-zombie-reap.sh
💤 Files with no reviewable changes (85)
- go/apps/billaged/contrib/systemd/README.md
- go/apps/billaged/DOCUMENTATION_REPORT.md
- deploy.bash
- go/apps/billaged/README.md
- go/apps/builderd/.gitignore
- go/apps/metald/backend_linux.go
- go/apps/metald/cmd/metald-init/Makefile
- go/apps/metald/backend_stub.go
- go/apps/billaged/contrib/systemd/billaged.env.example
- go/apps/metald/cmd/metald-init/test-all.sh
- go/apps/builderd/client/Makefile
- go/apps/metald/client/cmd/metald-cli/main.go
- go/apps/assetmanagerd/internal/storage/storage.go
- go/apps/assetmanagerd/cmd/assetmanagerd-cli/main.go
- go/apps/assetmanagerd/client/types.go
- go/apps/builderd/cmd/builderd-cli/main.go
- go/apps/assetmanagerd/CHANGELOG.md
- go/apps/metald/README.md
- go/apps/billaged/internal/aggregator/aggregator.go
- go/apps/assetmanagerd/contrib/systemd/assetmanagerd.service
- go/apps/builderd/internal/executor/registry.go
- go/apps/builderd/internal/assets/base.go
- go/apps/billaged/internal/config/config.go
- go/apps/builderd/contrib/grafana-dashboards/README.md
- go/apps/billaged/internal/observability/metrics.go
- go/apps/builderd/cmd/builderd/shutdown_test.go
- go/apps/billaged/contrib/grafana-dashboards/README.md
- go/apps/billaged/environment.example
- go/apps/builderd/contrib/systemd/README.md
- go/apps/metald/client/vmconfig.go
- go/apps/metald/backend.go
- go/apps/assetmanagerd/internal/config/config.go
- go/apps/billaged/Makefile
- go/apps/assetmanagerd/internal/service/service.go
- go/apps/assetmanagerd/README.md
- go/apps/assetmanagerd/internal/builderd/client.go
- go/apps/metald/cmd/metald-init/go.mod
- go/apps/builderd/Makefile
- go/apps/assetmanagerd/client/client.go
- go/apps/billaged/cmd/billaged/main.go
- go/apps/builderd/internal/executor/types.go
- go/apps/builderd/client/client.go
- go/apps/billaged/client/types.go
- go/apps/builderd/contrib/systemd/builderd.service
- go/apps/builderd/CHANGELOG.md
- go/apps/metald/cmd/metald-init/main.go
- go/apps/builderd/internal/assetmanager/client.go
- go/apps/builderd/cmd/builderd/main.go
- go/apps/assetmanagerd/Makefile
- go/apps/billaged/client/Makefile
- go/apps/metald/CHANGELOG.md
- go/apps/billaged/cmd/billaged-cli/main.go
- go/apps/builderd/internal/observability/metrics.go
- go/apps/billaged/internal/service/billing.go
- go/apps/metald/client/README.md
- go/apps/builderd/contrib/systemd/builderd.env.example
- go/apps/metald/client/client.go
- go/apps/metald/client/Makefile
- go/apps/metald/cmd/metald-init/container.cmd
- go/apps/billaged/client/client.go
- go/apps/billaged/CHANGELOG.md
- go/apps/builderd/internal/executor/docker_steps.go
- go/apps/assetmanagerd/cmd/assetmanagerd/main.go
- go/apps/builderd/internal/executor/docker_pipeline.go
- go/apps/assetmanagerd/internal/observability/otel.go
- go/apps/billaged/contrib/systemd/billaged.service
- go/apps/assetmanagerd/.gitignore
- go/apps/billaged/.gitignore
- go/apps/billaged/TODO.md
- go/apps/builderd/client/types.go
- go/apps/builderd/README.md
- go/apps/assetmanagerd/environment.example
- go/apps/assetmanagerd/client/Makefile
- go/apps/builderd/internal/observability/otel.go
- go/apps/assetmanagerd/TODO.md
- go/apps/billaged/internal/observability/otel.go
- go/apps/metald/cmd/metald-init/INIT_PROCESS_GUIDE.md
- go/apps/builderd/internal/executor/docker.go
- go/apps/assetmanagerd/internal/storage/local.go
- go/apps/builderd/internal/service/builder.go
- go/apps/builderd/internal/executor/steps.go
- go/apps/metald/Makefile
- go/apps/metald/.gitignore
- go/apps/assetmanagerd/internal/registry/registry.go
- go/apps/builderd/internal/config/config.go
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-01T02:33:43.791Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.791Z
Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.
Applied to files:
go/Makefile
📚 Learning: 2025-09-01T16:08:10.327Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/deployment.proto:45-52
Timestamp: 2025-09-01T16:08:10.327Z
Learning: In the unkey/unkey repository metald service, the host:port pairs in Instance messages within GetDeploymentResponse are consumed by upstream services in a tightly controlled manner, with the host:port being mapped from DNS entries rather than being used for arbitrary client connections.
Applied to files:
deployment/docker-compose.yaml
📚 Learning: 2025-09-24T18:57:34.843Z
Learnt from: mcstepp
PR: unkeyed/unkey#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:
deployment/docker-compose.yamlapps/engineering/content/docs/cli/run/ctrl/index.mdx
📚 Learning: 2025-09-11T08:17:21.423Z
Learnt from: Flo4604
PR: unkeyed/unkey#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/apps/krane/backend/kubernetes/service.go
📚 Learning: 2025-07-15T14:59:30.212Z
Learnt from: chronark
PR: unkeyed/unkey#3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Applied to files:
go/apps/ctrl/services/deployment/deploy_workflow.gogo/apps/ctrl/run.go
📚 Learning: 2025-09-01T01:57:42.227Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/metald.proto:5-9
Timestamp: 2025-09-01T01:57:42.227Z
Learning: In the unkeyed/unkey repository, buf is configured to properly resolve metald proto imports like "metald/v1/vm.proto" without needing the full "go/proto/" prefix. The buf lint command `buf lint --path proto/metald` passes successfully with these relative import paths.
Applied to files:
go/apps/ctrl/services/deployment/deploy_workflow.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#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/apps/ctrl/run.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
PR: unkeyed/unkey#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/apps/ctrl/run.go
🔇 Additional comments (9)
deployment/04-seed-workspace.sql (1)
71-117: Remove the Chronark test seed block before merging.These inserts are personal/local data that shouldn’t live in the shared seed script, and they still reference the
org_chronarkorg that isn’t part of the committed seed set. Please drop this entire block so the seed file stays environment-agnostic.go/Makefile (1)
75-92: Good call gating on Krane readiness.Thanks for adding the readiness wait—this keeps
make k8s-upfrom returning before the new Krane service is healthy, which should prevent the usual race we’ve hit when dependent services come up first.go/Dockerfile (1)
5-7: Nice cache-friendly layer split.COPYing the module files first lets Docker reuse the download layer whenever dependencies stay the same, so rebuilds should be much faster.
go/apps/krane/backend/docker/service.go (1)
21-46: Initialization looks solid
Line 35’s ping guard is a nice touch—it fails fast when the socket is misconfigured, which should save a lot of head scratching later.apps/engineering/content/docs/cli/run/ctrl/index.mdx (1)
115-120: Docs match the new Krane wiring
Line 115’s rename to--krane-addresspaired withUNKEY_KRANE_ADDRESSkeeps the CLI docs consistent with the backend changes—thanks for keeping the paper trail tidy.deployment/docker-compose.yaml (2)
267-287: Krane service wiring looks solid.The Docker backend configuration and socket mount line up with what ctrl expects from
UNKEY_KRANE_ADDRESS, so local orchestration should keep working.
309-321: ctrl dependency update makes sense.Waiting for krane to be up before booting ctrl and switching to
UNKEY_KRANE_ADDRESSkeeps the control plane aligned with the new service layout.go/apps/ctrl/run.go (1)
181-184: Use real Krane credentials instead of hard-coded dev token.Line [182] now forces every Krane call to use the literal string
"Bearer dev_user_ctrl". Krane follows the Unkey API auth model, which requires a valid root key in theAuthorization: Bearer …header; shipping a placeholder breaks any non-dev environment (401s) and leaks a fake credential into production code (unkey.com). Please thread the correct token (or omit the header when SPIFFE is in use) via configuration instead of hard-coding it.⛔ Skipped due to learnings
Learnt from: mcstepp PR: unkeyed/unkey#4010 File: go/cmd/deploy/control_plane.go:64-69 Timestamp: 2025-09-25T15:12:21.699Z 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.go/apps/ctrl/services/deployment/deploy_workflow.go (1)
185-205: Toolchain supports integer range loops Thego.moddirective is set to Go 1.25 and the build environment runs Go 1.24.1, both ≥ 1.22, sofor i := range 300compiles successfully—no changes needed.
chronark
changed the title
|
Thank you for following the naming conventions for pull request titles! 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/engineering/content/docs/cli/run/api/index.mdx (1)
179-183: Document the correct flag type.The
--max-request-body-sizeflag accepts a numeric value (bytes), so publishing the type as “unknown” is misleading in generated docs. Please mark it as an integer to match the CLI surface.-- - **Type:** unknown +- **Type:** integer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/proto/generated/krane/v1/deployment_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (12)
apps/engineering/content/docs/architecture/services/meta.json(1 hunks)apps/engineering/content/docs/cli/deploy/index.mdx(5 hunks)apps/engineering/content/docs/cli/gw/index.mdx(1 hunks)apps/engineering/content/docs/cli/run/api/index.mdx(3 hunks)apps/engineering/content/docs/cli/run/ctrl/index.mdx(4 hunks)apps/engineering/content/docs/cli/run/gw/index.mdx(1 hunks)apps/engineering/content/docs/cli/run/index.mdx(2 hunks)apps/engineering/content/docs/cli/run/krane/index.mdx(1 hunks)go/apps/ctrl/config.go(1 hunks)go/apps/ctrl/middleware/auth.go(1 hunks)go/apps/krane/backend/kubernetes/create_deployment.go(1 hunks)go/apps/krane/backend/kubernetes/get_deployment.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/engineering/content/docs/architecture/services/meta.json
- apps/engineering/content/docs/cli/run/krane/index.mdx
- apps/engineering/content/docs/cli/run/gw/index.mdx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-24T18:57:34.843Z
Learnt from: mcstepp
PR: unkeyed/unkey#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:
apps/engineering/content/docs/cli/run/index.mdxapps/engineering/content/docs/cli/deploy/index.mdx
📚 Learning: 2025-09-11T08:17:21.423Z
Learnt from: Flo4604
PR: unkeyed/unkey#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/apps/krane/backend/kubernetes/get_deployment.go
📚 Learning: 2025-07-15T14:45:18.920Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#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:
apps/engineering/content/docs/cli/deploy/index.mdx
🧬 Code graph analysis (2)
go/apps/krane/backend/kubernetes/create_deployment.go (1)
go/gen/proto/krane/v1/deployment.pb.go (7)
CreateDeploymentRequest(160-165)CreateDeploymentRequest(178-178)CreateDeploymentRequest(193-195)CreateDeploymentResponse(204-209)CreateDeploymentResponse(222-222)CreateDeploymentResponse(237-239)DeploymentStatus_DEPLOYMENT_STATUS_PENDING(28-28)
go/apps/krane/backend/kubernetes/get_deployment.go (2)
internal/proto/generated/krane/v1/deployment_pb.ts (3)
GetDeploymentRequest(172-182)GetDeploymentResponse(195-200)Instance(213-228)go/gen/proto/krane/v1/deployment.pb.go (17)
GetDeploymentRequest(424-430)GetDeploymentRequest(443-443)GetDeploymentRequest(458-460)GetDeploymentResponse(476-481)GetDeploymentResponse(494-494)GetDeploymentResponse(509-511)DeploymentStatus(24-24)DeploymentStatus(59-61)DeploymentStatus(63-65)DeploymentStatus(72-74)DeploymentStatus_DEPLOYMENT_STATUS_RUNNING(29-29)DeploymentStatus_DEPLOYMENT_STATUS_PENDING(28-28)Instance(520-527)Instance(540-540)Instance(555-557)DeploymentStatus_DEPLOYMENT_STATUS_TERMINATING(30-30)DeploymentStatus_DEPLOYMENT_STATUS_UNSPECIFIED(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
🔇 Additional comments (1)
go/apps/krane/backend/kubernetes/get_deployment.go (1)
61-68: ReturnNotFoundwhen the Service is missing.Line 61 retrieves the Service, but on any error we jump straight to
CodeInternaland discard the underlying cause. If the Service was deleted or never created, callers now get a 500 instead of a 404, which breaks the API contract and hides the remediation path. Please branch onerrors.IsNotFound(err)and wrap other failures with%wso we preserve the root cause.- service, err := k.clientset.CoreV1().Services(req.Msg.GetNamespace()).Get(ctx, k8sDeploymentID, metav1.GetOptions{}) - if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("could not load service: %s", k8sDeploymentID)) - } - var port int32 = 8080 // default - if err == nil && len(service.Spec.Ports) > 0 { + service, err := k.clientset.CoreV1().Services(req.Msg.GetNamespace()).Get(ctx, k8sDeploymentID, metav1.GetOptions{}) + if err != nil { + if errors.IsNotFound(err) { + return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("service not found: %s", k8sDeploymentID)) + } + return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("failed to load service %s: %w", k8sDeploymentID, err)) + } + var port int32 = 8080 // default + if len(service.Spec.Ports) > 0 { port = service.Spec.Ports[0].Port }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
go/apps/krane/backend/kubernetes/delete_deployment.go (1)
15-21: Doc vs behavior: clarify “complete cleanup” or include PVC/aux cleanup.StatefulSet PVCs aren’t removed by deleting the StatefulSet; headless Services, ConfigMaps/Secrets (if created) may also remain. Either scope the comment to “Service and StatefulSet” or add label‑selected cleanup for PVCs (and any per‑deployment ConfigMaps/Secrets) to match the create path.
If you want, I can scan create_deployment for the labels used and draft a safe DeleteCollection for PVCs that matches them.
deployment/05-seed-chronark.sql (4)
1-3: Make this a dev‑only CLI seed (keep prod DB clean).Good callout in the comment. Prefer a
dev seed chronarkCLI task or a gated migration that only runs in local/dev to avoid shipping test data to shared/staging/prod.I can scaffold a small CLI subcommand that runs these statements conditionally by env.
28-41: Keep created_at immutable on upsert; align timestamp naming.
- Same concern: resetting
projects.created_aton duplicate. Prefer no‑op or update anupdated_atcolumn.- Consider standardizing on either
created_at_m(ms BIGINT) orcreated_at(TIMESTAMP[3]) across tables to avoid confusion.Suggested tweak:
- UNIX_TIMESTAMP() * 1000 -) ON DUPLICATE KEY UPDATE created_at = UNIX_TIMESTAMP() * 1000; + CAST(UNIX_TIMESTAMP()*1000 AS UNSIGNED) +) ON DUPLICATE KEY UPDATE id = id;Please confirm
projects.created_attype; if it’s TIMESTAMP/DATETIME, useNOW(3)(and drop the *1000).
42-55: Environment seed: same created_at upsert + possible required columns.
- Apply the same no‑op approach for
created_at.- Double‑check if
environmentsrequires anameor other non‑null columns beyondslug; if so, this insert may fail in strict mode.Proposed change:
- UNIX_TIMESTAMP() * 1000 -) ON DUPLICATE KEY UPDATE created_at = UNIX_TIMESTAMP() * 1000; + CAST(UNIX_TIMESTAMP()*1000 AS UNSIGNED) +) ON DUPLICATE KEY UPDATE id = id;Optional: wrap the three inserts in a single transaction for atomicity.
START TRANSACTION; -- inserts ... COMMIT;Use the repo scan from the previous comment to confirm required columns and types for
environments.
9-25: Don’t resetcreated_at_mon workspace upsertUpdating
created_at_mon duplicate overwrites the original creation time; use a no-op or updateupdated_at_minstead.-) ON DUPLICATE KEY UPDATE created_at_m = UNIX_TIMESTAMP() * 1000; +) ON DUPLICATE KEY UPDATE id = id;go/apps/krane/backend/docker/get_deployment.go (1)
37-38: Reduce noisy logs: avoid dumping full container struct at info levelLogging the entire
types.Containerstruct at info will spam logs and may include labels you don’t want at this level. Log only ID/state (and maybe names) or downgrade to debug.go/apps/krane/backend/docker/delete_deployment.go (3)
20-21: Log message nit: say “deleting deployment”Minor copy/paste: this handler deletes, not “gets”.
Apply:
- d.logger.Info("getting deployment", "deployment_id", deploymentID) + d.logger.Info("deleting deployment", "deployment_id", deploymentID)
32-41: Don’t abort on first removal failure; attempt best-effort cleanup and report aggregateContinuing removal improves cleanup resilience; log failures and return an aggregated error at the end.
Apply:
- for _, c := range containers { - err := d.client.ContainerRemove(ctx, c.ID, container.RemoveOptions{ - RemoveVolumes: true, - RemoveLinks: true, - Force: true, - }) - if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("failed to remove container: %w", err)) - } - } + var errs []error + for _, c := range containers { + if err := d.client.ContainerRemove(ctx, c.ID, container.RemoveOptions{ + RemoveVolumes: true, + RemoveLinks: true, + Force: true, + }); err != nil { + d.logger.Info("failed to remove container", "container_id", c.ID, "err", err) + errs = append(errs, err) + } + } + if len(errs) > 0 { + return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("failed to remove %d/%d containers; first error: %w", len(errs), len(containers), errs[0])) + }
22-30: Optional: graceful stop before force-removalIf runtime allows, consider a short stop timeout before forced removal to let processes exit cleanly (especially if they flush files). Keep
Force: trueas fallback.Would you like a patch using the SDK’s
ContainerStopwith a small timeout, aligned to the exact docker package version you have?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deployment/05-seed-chronark.sql(1 hunks)go/apps/krane/backend/docker/delete_deployment.go(1 hunks)go/apps/krane/backend/docker/get_deployment.go(1 hunks)go/apps/krane/backend/kubernetes/delete_deployment.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
go/apps/krane/backend/docker/get_deployment.go (2)
internal/proto/generated/krane/v1/deployment_pb.ts (3)
GetDeploymentRequest(172-182)GetDeploymentResponse(195-200)Instance(213-228)go/gen/proto/krane/v1/deployment.pb.go (17)
GetDeploymentRequest(424-430)GetDeploymentRequest(443-443)GetDeploymentRequest(458-460)GetDeploymentResponse(476-481)GetDeploymentResponse(494-494)GetDeploymentResponse(509-511)Instance(520-527)Instance(540-540)Instance(555-557)DeploymentStatus(24-24)DeploymentStatus(59-61)DeploymentStatus(63-65)DeploymentStatus(72-74)DeploymentStatus_DEPLOYMENT_STATUS_UNSPECIFIED(27-27)DeploymentStatus_DEPLOYMENT_STATUS_RUNNING(29-29)DeploymentStatus_DEPLOYMENT_STATUS_TERMINATING(30-30)DeploymentStatus_DEPLOYMENT_STATUS_PENDING(28-28)
⏰ 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). (1)
- GitHub Check: Test Packages / Test
🔇 Additional comments (3)
go/apps/krane/backend/kubernetes/delete_deployment.go (2)
29-42: Good: delete is idempotent on NotFound.Ignoring
NotFoundkeeps deletes safe to retry. Propagation policy set to Background is appropriate here.
22-22: Normalize IDs withsafeIDForK8s(prevents undeletable resources).
strings.ReplaceAlldoesn’t lowercase, soMy_AppbecomesMy-Appwhile creation likely usedmy-app. Deletion can silently miss resources. Use the shared helper.Apply this diff:
import ( "context" "fmt" - "strings" "connectrpc.com/connect" kranev1 "github.com/unkeyed/unkey/go/gen/proto/krane/v1" "github.com/unkeyed/unkey/go/pkg/ptr" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ - k8sDeploymentID := strings.ReplaceAll(req.Msg.GetDeploymentId(), "_", "-") + k8sDeploymentID := safeIDForK8s(req.Msg.GetDeploymentId())go/apps/krane/backend/docker/get_deployment.go (1)
50-61: Guard against missing/zero published ports before indexing and build address robustlyIndexing
c.Ports[0]can panic when no ports are published;PublicPortmay also be 0. Add a safe selection for a published TCP port and skip instances without one. Also avoid hard-codinghost.docker.internalwhich may not resolve on Linux; prefer a simple platform fallback.Apply:
@@ - d.logger.Info("deployment found", - "deployment_id", deploymentID, - "container_id", c.ID, - "status", status.String(), - "port", c.Ports[0].PublicPort, - ) - - res.Instances = append(res.Instances, &kranev1.Instance{ - Id: c.ID, - Address: fmt.Sprintf("host.docker.internal:%d", c.Ports[0].PublicPort), - Status: status, - }) + // Find a published TCP port + var port uint16 + for _, p := range c.Ports { + if p.PublicPort > 0 && p.Type == "tcp" { + port = p.PublicPort + break + } + } + if port == 0 { + d.logger.Info("container has no published ports", "deployment_id", deploymentID, "container_id", c.ID) + continue + } + + host := "host.docker.internal" + if runtime.GOOS == "linux" { + host = "localhost" + } + + d.logger.Info("deployment found", + "deployment_id", deploymentID, + "container_id", c.ID, + "status", status.String(), + "port", port, + ) + + res.Instances = append(res.Instances, &kranev1.Instance{ + Id: c.ID, + Address: fmt.Sprintf("%s:%d", host, port), + Status: status, + })Add the import in the import block:
+ "runtime"
2 participants
This looks horrible cause it removes all of the metald stuff. focus on the edited/created files in go/apps/krane instead. it's really not a lot of code.
This is a pretty clean service to handle scheduling inside k8s and docker locally to get a fresh start without too much claude baggage.
It replaces metald as the service that sits inside kubernetes to allow our control plane to create workloads.
To test: simply do what you have always done to deploy, this should be a drop in replacement