Conversation
|
📝 WalkthroughWalkthroughMigrates metald client and CLI from metal/vmprovisioner/v1 to metald/v1 protos, replaces tenant/project/environment scoping with deployment-centric APIs, adds deployment RPCs, removes VM config file helpers, refactors CLI commands, and strips tenant-auth forwarding/interceptors from client/server and observability stacks. Updates Go toolchains and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as metald-cli
participant Client as Client (metald/v1)
participant Server as metald VMService
Note over CLI,Client: Deployment lifecycle (new APIs)
User->>CLI: metald create-deployment --deployment-id X
CLI->>Client: CreateDeployment(req: CreateDeploymentRequest)
Client->>Server: CreateDeployment (connect RPC)
Server-->>Client: CreateDeploymentResponse (vm_ids)
Client-->>CLI: resp
CLI-->>User: print vm_ids / JSON
sequenceDiagram
autonumber
participant Caller as Client
participant Interceptors as Default Interceptors
participant Server as metald VMService
Note right of Interceptors: After changes<br/>- No TenantAuth<br/>- No Tenant forwarding
Caller->>Interceptors: RPC (e.g., BootVm)
Interceptors->>Server: Forward request (trace+metrics only)
Server-->>Interceptors: Response
Interceptors-->>Caller: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
go/deploy/metald/client/go.mod (1)
3-3: Align Go toolchain versions across all modules to 1.25.0.
Several go.mod files still declare older Go versions (e.g., apps/agent at 1.23.0, apps/chproxy at 1.24.0, go/demo_api at 1.24). Update these—and ensure CI/build images and anytoolchaindirectives—so every module uses Go 1.25.0 to prevent build drift.go/deploy/metald/client/client.go (4)
40-48: Stale client fields; drop tenant/project/environment or set them.
tenantID,projectID, andenvironmentIDare never set. Keeping them leads to empty headers and misleading getters.Apply this diff to streamline the client state:
type Client struct { - vmService metaldv1connect.VmServiceClient - tlsProvider tls.Provider - tenantID string - projectID string - environmentID string - serverAddr string + vmService metaldv1connect.VmServiceClient + tlsProvider tls.Provider + serverAddr string }
240-271: Rename and simplify transport; avoid stale tenant headers.Keep only
Authorizationand an optionalX-Deployment-ID. Injecting empty X-Tenant/Project/Environment is misleading.Apply:
-// tenantTransport adds authentication and tenant isolation headers to all requests -type tenantTransport struct { - Base http.RoundTripper - EnvironmentID string - ProjectID string - TenantID string -} +// tenantTransport attaches auth and deployment context headers. +type tenantTransport struct { + Base http.RoundTripper + UserID string + DeploymentID string +} func (t *tenantTransport) RoundTrip(req *http.Request) (*http.Response, error) { // Clone the request to avoid modifying the original req2 := req.Clone(req.Context()) if req2.Header == nil { req2.Header = make(http.Header) } - // Set Authorization header with development token format - // AIDEV-BUSINESS_RULE: In development, use "dev_user_<id>" format - // TODO: Update to proper JWT tokens in production - req2.Header.Set("Authorization", fmt.Sprintf("Bearer dev_user_%s", t.TenantID)) - - // Also set X-Tenant-ID header for tenant identification - req2.Header.Set("X-Tenant-ID", t.TenantID) - req2.Header.Set("X-Project-ID", t.ProjectID) - req2.Header.Set("X-Environment-ID", t.EnvironmentID) + // Dev-only token; TODO: replace with real JWT in production + if t.UserID != "" { + req2.Header.Set("Authorization", fmt.Sprintf("Bearer dev_user_%s", t.UserID)) + } + if t.DeploymentID != "" { + req2.Header.Set("X-Deployment-ID", t.DeploymentID) + } // Use the base transport, or default if nil base := t.Base if base == nil { base = http.DefaultTransport } return base.RoundTrip(req2) }
25-38: Remove or wireConfig.DeploymentID
Config.DeploymentIDis declared in the client config but never stored or used inNewor any RPC call. Either drop this field from theConfigstruct or assign it toClientand apply it to outgoing requests (e.g. set the RPCDeploymentIdor include it in your transport).
194-202: RemovetenantIDand its accessor from the Metald client
In go/deploy/metald/client/client.go, thetenantIDfield is never assigned inNewandGetTenantID()will always return an empty string. Drop both the field and method. If you need to exposeConfig.DeploymentID, add aGetDeploymentID()accessor instead.go/deploy/metald/client/cmd/metald-cli/main.go (3)
51-61: Passdeployment-idinto the client config.You define the flag but don’t wire it. This blocks header propagation if the client uses it.
config := client.Config{ ServerAddress: *serverAddr, UserID: *userID, + DeploymentID: *deploymentID, TLSMode: *tlsMode, SPIFFESocketPath: *spiffeSocket, TLSCertFile: *tlsCert, TLSKeyFile: *tlsKey, TLSCAFile: *tlsCA, Timeout: *timeout, }
118-141: Update usage: removeconfig-genandconfig-validateif they’re gone.Help text still lists deprecated commands, which confuses users.
reboot <vm-id> [force] Reboot a running VM create-and-boot [vm-id] Create and immediately boot a VM - config-gen Generate a VM configuration file - config-validate <file> Validate a VM configuration file
429-434: Fix newline in debug log.
"/n"prints literally; use"\n". Also consider gating behind a-vflag.-log.Printf("createReq: %+v/n", createReq) +log.Printf("createReq: %+v\n", createReq)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go/deploy/metald/client/go.sumis excluded by!**/*.sumgo/deploy/metald/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
go/deploy/metald/client/client.go(6 hunks)go/deploy/metald/client/cmd/metald-cli/main.go(18 hunks)go/deploy/metald/client/config.go(0 hunks)go/deploy/metald/client/go.mod(1 hunks)go/deploy/metald/client/types.go(0 hunks)go/deploy/metald/cmd/metald/main.go(0 hunks)go/deploy/metald/go.mod(4 hunks)go/deploy/metald/internal/service/auth.go(1 hunks)go/deploy/metald/internal/service/deployment.go(1 hunks)go/deploy/metald/internal/service/vm.go(0 hunks)go/deploy/pkg/observability/interceptors/client.go(0 hunks)go/deploy/pkg/observability/interceptors/interceptors.go(0 hunks)go/deploy/pkg/observability/interceptors/logging.go(0 hunks)go/deploy/pkg/observability/interceptors/metrics.go(0 hunks)go/deploy/pkg/observability/interceptors/tenant.go(0 hunks)
💤 Files with no reviewable changes (9)
- go/deploy/pkg/observability/interceptors/metrics.go
- go/deploy/metald/cmd/metald/main.go
- go/deploy/pkg/observability/interceptors/logging.go
- go/deploy/pkg/observability/interceptors/interceptors.go
- go/deploy/metald/internal/service/vm.go
- go/deploy/pkg/observability/interceptors/client.go
- go/deploy/metald/client/config.go
- go/deploy/pkg/observability/interceptors/tenant.go
- go/deploy/metald/client/types.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T15:10:44.959Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/deployment.proto:7-15
Timestamp: 2025-09-01T15:10:44.959Z
Learning: In the unkey/unkey repository, the metald service receives deployment_id values from upstream services rather than generating them internally. The deployment_id field in DeploymentRequest is part of a service-to-service communication pattern.
Applied to files:
go/deploy/metald/internal/service/deployment.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/deploy/metald/client/go.modgo/deploy/metald/go.mod
⏰ 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: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (11)
go/deploy/metald/client/go.mod (1)
11-24: Adjust go mod tidy -compat flag to your Go toolchain
The-compat=1.25flag isn’t supported (max on your setup is Go 1.24.1). Replace with:# In go/deploy/metald/client go mod tidy -compat=1.24.1 # or omit -compat if on Go ≤1.24 git diff -- go.mod go.sum govulncheck ./... || true go list -m -json \ google.golang.org/grpc \ google.golang.org/protobuf \ google.golang.org/genproto/googleapis/rpc \ | sed -n 's/^{\|"Path"\|"Version"\|"Time"\|}/&/p'Confirm no CVEs and that these modules stay in sync with others.
⛔ Skipped due to learnings
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.go/deploy/metald/internal/service/deployment.go (1)
17-46: The verification script is running and will fetch the proto definition and Go imports for inspection.go/deploy/metald/internal/service/auth.go (1)
3-186: Verify default interceptors include authentication enforcement
I couldn’t locate the NewDefaultInterceptors definition in the repo—please confirm that it wires in tenant-auth and bearer-token checks. If it doesn’t, add a minimal, opt-out Bearer interceptor (gated by METALD_AUTH_DISABLED) as shown in the original suggestion.go/deploy/metald/go.mod (2)
16-25: OTel/OTLP/grpc versions look consistent; nice bump.The OTel stack is aligned at 1.38.0 and grpc at 1.75.0; good modernization.
Also applies to: 79-86
83-86: Ensure genproto pseudo-versions match grpc v1.75.0
Confirm that the two genproto entries in go/deploy/metald/go.mod (google.golang.org/genproto/googleapis/api and rpc at v0.0.0-20250826171959-ef028d996bc1) exactly align with the upstream grpc v1.75.0 go.mod pins; any drift can cause symbol mismatches in transitive deps.go/deploy/metald/client/client.go (1)
112-192: RPC method rewrites to metald/v1 look good.Thin wrappers around connect.NewRequest with proper error wrapping; returning
resp.Msgkeeps the API clean.go/deploy/metald/client/cmd/metald-cli/main.go (5)
338-350: Good: consistent JSON/text outputs and state echoing across pause/resume/reboot.User-facing UX is consistent and predictable.
Also applies to: 365-376, 407-410
462-499: Create-deployment validates inputs and surfaces IDs cleanly.Solid UX, returns IDs and counts; good defaults for
vm-count.
160-172: Verify and wire CLI options into VmConfig
CreateVmRequest.Config is empty—map your CLI flags to VmConfig (e.g. VcpuCount, MemorySizeMib, DockerImage, Template/Profile, ForceBuild) and confirm each field name in your generated metaldv1.pb.go.
470-499: Full deployment request correct; no FieldMask support
Verified thatUpdateDeploymentRequestindeployment.proto(line 27) and the generated Go struct indeployment.pb.go(line 188) include noFieldMaskfield—partial updates aren’t supported, so sending all fields is appropriate.
321-327: No action required: handleList reads CPUs and memory from the top-level VmInfo fields (ListVmsResponse.Vms []*VmInfo), while handleInfo reads them from the nested VmConfig in GetVmInfoResponse.Config, exactly as defined in the proto.Likely an incorrect or invalid review comment.
2 participants
This PR is more cleanup to realign the proper package names and get the client back into working order. I've removed more of the superfluous Claude nonsense and simplified a lot of the interactions based on the previous proto changes too.
Summary by CodeRabbit
New Features
Refactor
Chores