AWS initial implementation #448
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces provider-aware region selection; moves cluster infrastructure setup to post-creation using the new cluster ID; adds interactive machine creation prompts; extends the infrastructure interface with CreateMachine and integrates provider-specific machine setup, including a full AWS setup implementation; adjusts execution/spec skipping and command utilities; minor TUI formatting tweak. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (cluster create)
participant API as Control Plane API
participant Infra as Infrastructure Setup
User->>CLI: cluster create (provider, region)
Note over CLI: Regions sourced via providerRegions
CLI->>API: POST /cli/cluster (create)
API-->>CLI: 201 { clusterID, ... }
Note right of CLI: New flow: setup runs after ID available
CLI->>Infra: Setup(ctx, logger, provider, region, clusterID, token)
Infra-->>CLI: Setup result (success/error)
CLI-->>User: Output (JSON or spinner result)
sequenceDiagram
autonumber
actor User
participant CLI as CLI (machine create)
participant API as Control Plane API
participant Provider as Provider Setup
alt Interactive (TTY, missing args)
CLI->>CLI: Prompt for cluster
CLI->>CLI: Derive provider, prompt for region
else Non-interactive
CLI->>User: Error: missing args
end
CLI->>API: POST /cli/machine (clusterID, provider, region)
API-->>CLI: 201 { machineID, token, provider }
opt Provider-specific setup
CLI->>Provider: CreateMachine(ctx, logger, region, token, clusterID)
alt Setup fails
CLI->>API: DELETE /cli/machine (machineID)
Provider-->>CLI: Error returned
CLI-->>User: Error (includes provider setup failure)
else Setup succeeds
Provider-->>CLI: OK
CLI-->>User: Machine created
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
# Conflicts: # Makefile # go.mod # go.sum
# Conflicts: # cmd/cluster.go # go.mod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/infrastructure/cluster.go (1)
272-274: Restore the real cluster token when callingSetup.We’re throwing away the API-provided bootstrap token by creating a new
ClusterwithToken: "". Providers like GCP pipeCLUSTER_TOKENstraight into their launch commands (e.g.,--metadata=user-data={CLUSTER_TOKEN}), so this change now boots nodes without any credentials and they fail to register. Please pass the actualclusterreturned fromCreateCluster(or at least its realToken) intoinfrastructure.Setup.- if err := infrastructure.Setup(ctx, logger, &infrastructure.Cluster{ID: cluster.ID, Token: "", Provider: provider, Name: name, Type: size, Region: region}, format); err != nil { + if err := infrastructure.Setup(ctx, logger, cluster, format); err != nil {cmd/machine.go (1)
325-332: Add structured and quiet output modes to machine-create to prevent token exposure
- Avoid unconditionally printing resp.Token in cmd/machine.go (machineCreateCmd).
- Add
--format=jsonand--quietflags to machine-create; emit token only in JSON or when--quietis specified.- By default, print minimal info (ID only) or route token to stderr/mask it.
- Confirm the returned token is one-time or short-lived.
cmd/cluster.go (1)
492-492: Help text lists unsupported provider "other".Validator rejects "other" but help suggests it. Fix the flag help to avoid a broken UX.
Apply this diff:
- clusterNewCmd.Flags().String("provider", "", "The infrastructure provider (gcp, aws, azure, vmware, other)") + clusterNewCmd.Flags().String("provider", "", "The infrastructure provider (gcp, aws, azure, vmware)")
🧹 Nitpick comments (9)
cmd/machine.go (5)
284-299: Place “create” under the management group; docs look goodCreate is a mutating command and should live under the “management” group for consistency with remove. Suggest switching GroupID.
var machineCreateCmd = &cobra.Command{ Use: "create [cluster_id] [provider] [region]", - GroupID: "info", + GroupID: "management", Short: "Create a new machine for a cluster", Long: `Create a new machine for a cluster.
307-324: Handle partial args (1 or 2) + interactive prompts; don’t ignore provided cluster_idCurrently, any call with fewer than 3 args ignores provided values and re-prompts, which is surprising UX. Support 1- and 2-arg forms and only prompt for missing pieces. Error clearly in non-interactive mode.
- var clusterID, provider, region string - - // If all arguments provided, use them directly - if len(args) == 3 { - clusterID = args[0] - provider = args[1] - region = args[2] - } else if tui.HasTTY { - // Interactive mode - prompt for missing values - cluster := promptForClusterSelection(ctx, logger, apiUrl, apikey) - provider = cluster.Provider - region = promptForRegionSelection(ctx, logger, provider) - clusterID = cluster.ID - } else { - // Non-interactive mode - require all arguments - errsystem.New(errsystem.ErrMissingRequiredArgument, fmt.Errorf("cluster_id, provider, and region are required in non-interactive mode"), errsystem.WithContextMessage("Missing required arguments")).ShowErrorAndExit() - } + var clusterID, provider, region string + switch len(args) { + case 3: + clusterID, provider, region = args[0], args[1], args[2] + case 2: + clusterID, provider = args[0], args[1] + if tui.HasTTY { + region = promptForRegionSelection(ctx, logger, provider) + } else { + errsystem.New(errsystem.ErrMissingRequiredArgument, fmt.Errorf("region is required in non-interactive mode"), errsystem.WithContextMessage("Missing required arguments")).ShowErrorAndExit() + } + case 1: + clusterID = args[0] + // Resolve cluster to derive provider + var selected infrastructure.Cluster + tui.ShowSpinner(fmt.Sprintf("Resolving cluster %s...", clusterID), func() { + clusters, err := infrastructure.ListClusters(ctx, logger, apiUrl, apikey) + if err != nil { + errsystem.New(errsystem.ErrApiRequest, err, errsystem.WithContextMessage("Failed to resolve cluster")).ShowErrorAndExit() + } + for _, c := range clusters { + if c.ID == clusterID || c.Name == clusterID { + selected = c + break + } + } + }) + if selected.ID == "" { + errsystem.New(errsystem.ErrInvalidArgumentProvided, fmt.Errorf("cluster %q not found", clusterID), errsystem.WithContextMessage("Invalid cluster")).ShowErrorAndExit() + } + provider = selected.Provider + if tui.HasTTY { + region = promptForRegionSelection(ctx, logger, provider) + } else { + errsystem.New(errsystem.ErrMissingRequiredArgument, fmt.Errorf("region is required in non-interactive mode"), errsystem.WithContextMessage("Missing required arguments")).ShowErrorAndExit() + } + default: + if tui.HasTTY { + cluster := promptForClusterSelection(ctx, logger, apiUrl, apikey) + provider = cluster.Provider + region = promptForRegionSelection(ctx, logger, provider) + clusterID = cluster.ID + } else { + errsystem.New(errsystem.ErrMissingRequiredArgument, fmt.Errorf("cluster_id, provider, and region are required in non-interactive mode"), errsystem.WithContextMessage("Missing required arguments")).ShowErrorAndExit() + } + }
395-401: Region selection: remove stray print and guard empty option setAvoid extra stdout noise and fail fast if the provider has no regions configured.
func promptForRegionSelection(ctx context.Context, logger logger.Logger, provider string) string { // Get regions for the provider (reuse the same logic from cluster.go) - fmt.Println("Provider:", provider) opts := getRegionsForProvider(provider) + if len(opts) == 0 { + errsystem.New(errsystem.ErrInvalidArgumentProvided, fmt.Errorf("no regions available for provider %q", provider), errsystem.WithContextMessage("Region selection failed")).ShowErrorAndExit() + } return tui.Select(logger, "Which region should we use?", "The region to deploy the machine", opts) }Also verify getRegionsForProvider exists in this package; otherwise this won’t compile. See verification script below.
327-330: Use errsystem for consistency instead of logger.FatalOther paths use errsystem.New(...).ShowErrorAndExit(). Keep error handling consistent here.
- resp, err := infrastructure.CreateMachine(ctx, logger, apiUrl, apikey, clusterID, orgId, provider, region) - if err != nil { - logger.Fatal("error creating machine: %s", err) - } + resp, err := infrastructure.CreateMachine(ctx, logger, apiUrl, apikey, clusterID, orgId, provider, region) + if err != nil { + errsystem.New(errsystem.ErrApiRequest, err, errsystem.WithContextMessage("Failed to create machine")).ShowErrorAndExit() + }
15-16: Import alias “logger” collides with local variable name “logger”Local vars named logger shadow the imported package name; this is easy to confuse. Consider aliasing the package (e.g., logx) or renaming locals (e.g., log).
cmd/cluster.go (1)
85-93: Avoid silently defaulting to GCP regions for unknown providers.Returning GCP options can confuse users and lead to wrong regions. Prefer empty options and fall back to a free-form input prompt.
internal/infrastructure/aws.go (3)
490-499: Remove stray fmt.Println(spec); avoid noisy output.This prints the execution spec every run. Drop it or downgrade to debug logging.
Apply this diff:
func getAWSClusterSpecification(envs map[string]any) string { spec := awsClusterSpecification - fmt.Println(spec) // Replace variables in the JSON string
234-239: Scope down IAM policy Resource to the current account.Using arn ... :*:secret:... grants cross-account scope. Prefer the caller’s account ID.
As a follow-up, capture AWS_ACCOUNT_ID via sts get-caller-identity and template:
- Add AWS_ACCOUNT_ID to envs in Setup.
- Update the policy Resource to arn:aws:secretsmanager:{AWS_REGION}:{AWS_ACCOUNT_ID}:secret:{AWS_SECRET_NAME}*
188-196: Open SSH (0.0.0.0/0). Consider restricting.Port 22 from anywhere is risky. Restrict to a CIDR input or make SSH optional.
Is global SSH ingress required for typical deployments, or can we scope it to the operator’s IP/CIDR?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/cluster.go(5 hunks)cmd/machine.go(4 hunks)internal/infrastructure/aws.go(1 hunks)internal/infrastructure/cluster.go(1 hunks)internal/infrastructure/gcp.go(1 hunks)internal/infrastructure/infrastructure.go(1 hunks)internal/infrastructure/spec.go(1 hunks)internal/infrastructure/tui.go(1 hunks)internal/infrastructure/util.go(3 hunks)
🔇 Additional comments (2)
cmd/machine.go (1)
325-326: Helper functions verified promptForClusterOrganization and getRegionsForProvider are defined in cmd/cluster.go with signatures matching their usage; no changes needed.internal/infrastructure/aws.go (1)
321-333: Verify user-data semantics.run-instances --user-data is passed the raw {CLUSTER_TOKEN}. Confirm your AMI expects this exact payload (not a script/cloud-init multipart). If not, encode or use file://.
* Add infra commands * fixes * more visual improvements * reorder org * wip changes * wip changes * fixes * machine create * use ecdsa * more cleanup * clean up go mod 🧹 * fix thing * AWS initial implementation (#448) * AWS initil implementation * more work on getting aws working from scratch * refactor awsSpecification commands * More work on gettting AWS working * cluster create and machine create commands fixes * checks if clustering is enabled for the authenticated user * remove debug logs * Small nits from Coder Rabbit * cleanup * fix create image command for aws * latest * indirect vuln * fixes * more fixes * errrors! * make the clipboard mode work better * hidden * some feedback --------- Co-authored-by: Robin Diddams <robindiddams@gmail.com> Co-authored-by: Pedro Enrique <pedro.tma@gmail.com>
Summary by CodeRabbit
New Features
Bug Fixes