feat: add cluster deploy command for parallel IAM and VPC deployment#11
feat: add cluster deploy command for parallel IAM and VPC deployment#11rrp-bot wants to merge 6 commits into
Conversation
Adds a new `rosactl cluster deploy` command that creates both IAM and VPC
CloudFormation stacks concurrently using goroutines, reducing total deployment
time compared to running them sequentially.
Usage:
rosactl cluster deploy CLUSTER_NAME \
--oidc-issuer-url https://... \
--region us-east-1 \
--availability-zones us-east-1a,us-east-1b,us-east-1c
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughAdds a new top-level Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "rosactl (cluster deploy)"
participant Flags as "Flag parsing"
participant OIDC as "OIDC provider (TLS thumbprint)"
participant AWS as "AWS SDK / Config"
participant IAM as "CloudFormation (IAM stack)"
participant VPC as "CloudFormation (VPC stack)"
User->>CLI: run `rosactl cluster deploy <name>` with flags
CLI->>Flags: parse flags, require: region, availability-zones
CLI->>CLI: populate IAM & VPC CreateOptions
CLI->>AWS: load config (region)
par Concurrent operations
CLI->>OIDC: fetch TLS thumbprint (if needed)
OIDC-->>CLI: return thumbprint
CLI->>IAM: RunCreate(ctx, iamOpts)
IAM-->>CLI: result (success|error)
and
CLI->>VPC: RunCreate(ctx, vpcOpts)
VPC-->>CLI: result (success|error)
end
CLI->>CLI: collect results, cancel on first error
alt all success
CLI-->>User: print completion message
else any failure
CLI-->>User: return aggregated error report
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
I don't like that it's called |
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringVar(&opts.oidcIssuerURL, "oidc-issuer-url", "", "OIDC issuer URL from Management Cluster (required)") |
There was a problem hiding this comment.
There must be a way to combine these from the definitions already - I hate having to maintain two lists of options. One for the combined and one for the separate tasks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/commands/cluster/deploy.go (2)
159-172: Consider canceling remaining operations on first failure.Currently, if one deployment fails, the other continues to completion. This is reasonable for showing all errors, but it may waste time and resources if one operation fails early.
Consider using a cancellable context with
context.WithCancelto abort the remaining operation when the first failure occurs. This is optional since the current behavior has the benefit of reporting all errors at once.♻️ Optional: Cancel on first failure
+ ctx, cancel := context.WithCancel(ctx) + defer cancel() + results := make(chan deployResult, 2) var wg sync.WaitGroup // IAM deployment goroutine wg.Add(1) go func() { defer wg.Done() // ... existing code ... if err != nil { + cancel() results <- deployResult{"IAM", err} return } // ... }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/cluster/deploy.go` around lines 159 - 172, The loop collecting results currently waits for all goroutines to finish even if one fails; modify the deployment to use a cancellable context (context.WithCancel) that you pass into the worker goroutines which send into the results channel, and call cancel() as soon as you observe the first non-nil result.err in the result-reading loop (the loop that appends to errs). Ensure each worker (the functions that read from the shared context or are spawned where results are produced) checks ctx.Done() and returns early when cancelled so work is aborted, and keep the existing aggregation of errs (errs slice and the final fmt.Errorf) so you still report errors from already completed tasks.
100-126: Concurrent output may interleave.Both goroutines use
fmt.Printf/fmt.Printlnwithout synchronization, which can cause interleaved output in the terminal. This is a minor cosmetic issue and doesn't affect correctness.If cleaner output is desired, consider buffering logs per goroutine and printing them sequentially after completion, or using a synchronized logger.
Also applies to: 130-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/cluster/deploy.go` around lines 100 - 126, The goroutine that calls crypto.GetOIDCThumbprint and clusteriam.CreateIAM (and the analogous goroutine at 130-152) prints directly with fmt.Printf/Println causing interleaved terminal output; change these goroutines to accumulate their log text into a local buffer (e.g., strings.Builder or bytes.Buffer) instead of printing immediately, then emit the full buffered string once at the end (after resp handling) or send it along with deployResult so a single synchronized emitter prints outputs, or alternatively use a shared logger guarded by a sync.Mutex; update the goroutines that reference opts.oidcIssuerURL, opts.clusterName, clusteriam.CreateIAM, crypto.GetOIDCThumbprint, wg and results accordingly so printing is performed atomically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/commands/cluster/deploy.go`:
- Around line 159-172: The loop collecting results currently waits for all
goroutines to finish even if one fails; modify the deployment to use a
cancellable context (context.WithCancel) that you pass into the worker
goroutines which send into the results channel, and call cancel() as soon as you
observe the first non-nil result.err in the result-reading loop (the loop that
appends to errs). Ensure each worker (the functions that read from the shared
context or are spawned where results are produced) checks ctx.Done() and returns
early when cancelled so work is aborted, and keep the existing aggregation of
errs (errs slice and the final fmt.Errorf) so you still report errors from
already completed tasks.
- Around line 100-126: The goroutine that calls crypto.GetOIDCThumbprint and
clusteriam.CreateIAM (and the analogous goroutine at 130-152) prints directly
with fmt.Printf/Println causing interleaved terminal output; change these
goroutines to accumulate their log text into a local buffer (e.g.,
strings.Builder or bytes.Buffer) instead of printing immediately, then emit the
full buffered string once at the end (after resp handling) or send it along with
deployResult so a single synchronized emitter prints outputs, or alternatively
use a shared logger guarded by a sync.Mutex; update the goroutines that
reference opts.oidcIssuerURL, opts.clusterName, clusteriam.CreateIAM,
crypto.GetOIDCThumbprint, wg and results accordingly so printing is performed
atomically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68aef778-5590-4c5b-97ad-6037e8d09121
📒 Files selected for processing (3)
internal/commands/cluster/cluster.gointernal/commands/cluster/deploy.gointernal/commands/root.go
Export CreateOptions, AddFlags, and RunCreate from the clusteriam and clustervpc command packages so the deploy command composes them directly instead of maintaining a second copy of every flag. Adding or changing a flag on cluster-iam create or cluster-vpc create now automatically applies to cluster deploy with no extra work. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Done — refactored in the latest commit. cmdiam.AddFlags(cmd, &opts.iam)
cmdvpc.AddFlags(cmd, &opts.vpc)Adding or changing a flag on either individual create command now automatically applies to |
…definitions --region is already defined as a persistent flag on the root command, making it available to every subcommand automatically. Remove the per-command local flag registrations and read the inherited value in each RunE instead. Validation (empty region) is done in RunCreate/runDeploy since marking an inherited persistent flag required per-command would affect all commands. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Updated in the latest commit. |
Add context.WithCancel so that if IAM or VPC creation fails the other operation's context is cancelled, aborting it at the next AWS SDK call rather than running it to completion unnecessarily. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed both CodeRabbit nitpicks: Context cancellation — implemented. A Interleaved output — acknowledged as a known trade-off. Both goroutines delegate to |
…mand Adds a Ginkgo spec that exercises `rosactl cluster deploy` against LocalStack, verifying that both the IAM and VPC CloudFormation stacks are created by a single command invocation. Follows the same conventions as the existing IAM/VPC tests: - Accepts CREATE_COMPLETE or CREATE_FAILED (LocalStack resource limitations) - Skips gracefully in offline environments where the OIDC thumbprint TLS connection cannot reach s3.amazonaws.com, which also triggers context cancellation of the VPC goroutine Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Added a LocalStack integration test for The test runs |
repositoryName was declared in the var block but never assigned before being passed to CreateRepository, causing ECR to reject it with InvalidParameterException on every Lambda test run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/commands/clusteriam/create.go (1)
23-28: Keep the required-flag contract with the reusable helper.
AddFlagsis now the reuse point, but the fact that--oidc-issuer-urlis mandatory still lives outside it. That makes future composite commands easy to wire incorrectly and lets help/validation drift from the standalone command.Also applies to: 57-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/clusteriam/create.go` around lines 23 - 28, AddFlags currently registers the --oidc-issuer-url flag but doesn't enforce the "required" contract; update AddFlags (and the corresponding reuse point for the other occurrence) to mark the flag required after registering it (e.g., call cmd.MarkFlagRequired("oidc-issuer-url")) so the requirement lives with the reusable helper; reference AddFlags, CreateOptions, and the cmd.Flags().StringVar registration when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/commands/clustervpc/create.go`:
- Around line 78-93: RunCreate currently only validates Region; add the same
upfront ClusterName validation used in internal/commands/clusteriam/create.go so
invalid cluster names are rejected before any VPC work begins. In RunCreate
(function RunCreate, CreateOptions struct usage), check opts.ClusterName at the
top and call the shared validation helper (or extract the validation logic from
clusteriam/create.go) to return an error when the name is invalid, ensuring the
VPC workflow never starts for bad names.
- Around line 26-34: The flag description in AddFlags incorrectly advertises
"--availability-zones" as optional/auto-detected which is not true for all
callers; update the StringVar call for opts.AvailabilityZones in AddFlags to use
a neutral description like "Comma-separated availability zones" (remove
"(optional, auto-detected if empty)"), and let specific commands that require
the flag override or mark it required in their own command setup rather than
relying on this shared helper.
In `@test/localstack/localstack_test.go`:
- Around line 309-313: The test hardcodes availability zones
"us-east-1a,us-east-1b,us-east-1c" when constructing deployCmd
(exec.CommandContext with arguments "cluster", "deploy", testCluster, etc.),
which breaks when AWS_REGION is not us-east-1; change the AZ argument to be
derived from the awsRegion variable by programmatically constructing the three
AZ strings (e.g., awsRegion+"a", awsRegion+"b", awsRegion+"c") joined with
commas and pass that computed string for "--availability-zones" so the test uses
the correct AZs for the configured region.
- Around line 323-359: The current DescribeStacks error handling for the VPC and
IAM checks (calls to cfnClient.DescribeStacks around vpcStackName and
iamStackName) treats any "does not exist" error as an expected offline skip;
change this to only Skip when the error text clearly indicates the known
thumbprint/TLS failure (e.g., error.Error() contains "thumbprint" or
"certificate" or the specific TLS/OIDC thumbprint failure string you use
elsewhere), and for any other error call Expect(err).NotTo(HaveOccurred(), ...)
so unrelated deploy failures fail the spec; update both the VPC and IAM
DescribeStacks error branches to perform this specific-message check before
skipping.
---
Nitpick comments:
In `@internal/commands/clusteriam/create.go`:
- Around line 23-28: AddFlags currently registers the --oidc-issuer-url flag but
doesn't enforce the "required" contract; update AddFlags (and the corresponding
reuse point for the other occurrence) to mark the flag required after
registering it (e.g., call cmd.MarkFlagRequired("oidc-issuer-url")) so the
requirement lives with the reusable helper; reference AddFlags, CreateOptions,
and the cmd.Flags().StringVar registration when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07106456-f3f9-4f4d-9f51-9e22854addf5
📒 Files selected for processing (5)
internal/commands/cluster/deploy.gointernal/commands/clusteriam/create.gointernal/commands/clustervpc/create.gotest/localstack/lambda_test.gotest/localstack/localstack_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/commands/cluster/deploy.go
| // AddFlags registers the VPC-specific flags on cmd bound to opts. | ||
| // --region is intentionally excluded: it is a persistent root-level flag | ||
| // inherited by all commands. | ||
| func AddFlags(cmd *cobra.Command, opts *CreateOptions) { | ||
| cmd.Flags().StringVar(&opts.VpcCidr, "vpc-cidr", "10.0.0.0/16", "CIDR block for the VPC") | ||
| cmd.Flags().StringVar(&opts.PublicSubnetCidrs, "public-subnet-cidrs", "10.0.101.0/24,10.0.102.0/24,10.0.103.0/24", "Comma-separated public subnet CIDRs") | ||
| cmd.Flags().StringVar(&opts.PrivateSubnetCidrs, "private-subnet-cidrs", "10.0.0.0/19,10.0.32.0/19,10.0.64.0/19", "Comma-separated private subnet CIDRs") | ||
| cmd.Flags().StringVar(&opts.AvailabilityZones, "availability-zones", "", "Comma-separated availability zones (optional, auto-detected if empty)") | ||
| cmd.Flags().BoolVar(&opts.SingleNatGateway, "single-nat-gateway", true, "Use single NAT gateway (true=cost savings, false=HA per-AZ)") |
There was a problem hiding this comment.
Don't advertise --availability-zones as optional in the shared helper.
AddFlags now feeds cluster deploy too, where --availability-zones is required. Reusing the current description will make rosactl cluster deploy --help contradict the command's actual contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/commands/clustervpc/create.go` around lines 26 - 34, The flag
description in AddFlags incorrectly advertises "--availability-zones" as
optional/auto-detected which is not true for all callers; update the StringVar
call for opts.AvailabilityZones in AddFlags to use a neutral description like
"Comma-separated availability zones" (remove "(optional, auto-detected if
empty)"), and let specific commands that require the flag override or mark it
required in their own command setup rather than relying on this shared helper.
| func RunCreate(ctx context.Context, opts *CreateOptions) error { | ||
| if opts.Region == "" { | ||
| return fmt.Errorf("--region is required") | ||
| } | ||
|
|
||
| fmt.Printf("🌐 Creating cluster VPC resources for: %s\n", opts.ClusterName) | ||
| fmt.Printf(" Region: %s\n", opts.Region) | ||
| fmt.Printf(" VPC CIDR: %s\n", opts.VpcCidr) | ||
| fmt.Printf(" Single NAT Gateway: %t\n", opts.SingleNatGateway) | ||
| fmt.Println() | ||
|
|
||
| // Load AWS config | ||
| cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(opts.region)) | ||
| cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(opts.Region)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load AWS config: %w", err) | ||
| } |
There was a problem hiding this comment.
Validate ClusterName before the VPC workflow starts.
internal/commands/clusteriam/create.go:67-70 rejects invalid names up front, but RunCreate here only checks Region. In the parallel deploy flow, that can turn one bad cluster name into inconsistent IAM/VPC behavior instead of one deterministic validation failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/commands/clustervpc/create.go` around lines 78 - 93, RunCreate
currently only validates Region; add the same upfront ClusterName validation
used in internal/commands/clusteriam/create.go so invalid cluster names are
rejected before any VPC work begins. In RunCreate (function RunCreate,
CreateOptions struct usage), check opts.ClusterName at the top and call the
shared validation helper (or extract the validation logic from
clusteriam/create.go) to return an error when the name is invalid, ensuring the
VPC workflow never starts for bad names.
| deployCmd := exec.CommandContext(ctx, binaryPath, "cluster", "deploy", testCluster, | ||
| "--region", awsRegion, | ||
| "--oidc-issuer-url", "https://test-oidc.s3.amazonaws.com", | ||
| "--availability-zones", "us-east-1a,us-east-1b,us-east-1c", | ||
| "--single-nat-gateway=false", |
There was a problem hiding this comment.
Derive the AZ list from awsRegion.
This spec reads AWS_REGION from the environment, but the new command still hardcodes us-east-1a/b/c. Any non-default region will fail for the wrong reason.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/localstack/localstack_test.go` around lines 309 - 313, The test
hardcodes availability zones "us-east-1a,us-east-1b,us-east-1c" when
constructing deployCmd (exec.CommandContext with arguments "cluster", "deploy",
testCluster, etc.), which breaks when AWS_REGION is not us-east-1; change the AZ
argument to be derived from the awsRegion variable by programmatically
constructing the three AZ strings (e.g., awsRegion+"a", awsRegion+"b",
awsRegion+"c") joined with commas and pass that computed string for
"--availability-zones" so the test uses the correct AZs for the configured
region.
| // Both CloudFormation stacks run in parallel; allow enough time for both. | ||
| Eventually(deploySession, 120*time.Second).Should(gexec.Exit()) | ||
|
|
||
| output := string(deploySession.Out.Contents()) | ||
| errOutput := string(deploySession.Err.Contents()) | ||
| GinkgoWriter.Printf("\nCLI Deploy Output:\n%s\n", output) | ||
| if errOutput != "" { | ||
| GinkgoWriter.Printf("\nCLI Deploy Errors:\n%s\n", errOutput) | ||
| } | ||
|
|
||
| Expect(output).To(ContainSubstring("Deploying cluster resources")) | ||
|
|
||
| // VPC does not require a network round-trip for thumbprint fetching, so | ||
| // it should always be attempted. If it is absent the IAM thumbprint fetch | ||
| // failed and its context cancellation aborted VPC before it started — | ||
| // acceptable in offline environments. | ||
| By("Verifying VPC stack was created") | ||
| vpcResult, err := cfnClient.DescribeStacks(ctx, &cloudformation.DescribeStacksInput{ | ||
| StackName: aws.String(vpcStackName), | ||
| }) | ||
| if err != nil && strings.Contains(err.Error(), "does not exist") { | ||
| Skip("VPC stack not created — IAM thumbprint failure likely cancelled VPC via context (expected in offline environments)") | ||
| } | ||
| Expect(err).NotTo(HaveOccurred(), "VPC stack should exist after deploy command") | ||
| Expect(vpcResult.Stacks).To(HaveLen(1)) | ||
| vpcStatus := string(vpcResult.Stacks[0].StackStatus) | ||
| GinkgoWriter.Printf("VPC Stack status: %s\n", vpcStatus) | ||
| // Accept CREATE_COMPLETE or CREATE_FAILED (LocalStack NAT Gateway limitation) | ||
| Expect(vpcStatus).To(Or(Equal("CREATE_COMPLETE"), Equal("CREATE_FAILED"))) | ||
|
|
||
| By("Verifying IAM stack was created") | ||
| iamResult, err := cfnClient.DescribeStacks(ctx, &cloudformation.DescribeStacksInput{ | ||
| StackName: aws.String(iamStackName), | ||
| }) | ||
| if err != nil && strings.Contains(err.Error(), "does not exist") { | ||
| Skip("IAM stack not created — OIDC thumbprint fetch likely failed in LocalStack (expected in offline environments)") | ||
| } |
There was a problem hiding this comment.
Don't convert any early deploy failure into a skip.
errOutput is only logged, and both missing-stack branches call Skip(...) on a plain "does not exist" result. That means unrelated regressions in cluster deploy can be reported as expected offline behavior instead of failing the spec. Please gate the skip on the known thumbprint/TLS failure message and fail everything else.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/localstack/localstack_test.go` around lines 323 - 359, The current
DescribeStacks error handling for the VPC and IAM checks (calls to
cfnClient.DescribeStacks around vpcStackName and iamStackName) treats any "does
not exist" error as an expected offline skip; change this to only Skip when the
error text clearly indicates the known thumbprint/TLS failure (e.g.,
error.Error() contains "thumbprint" or "certificate" or the specific TLS/OIDC
thumbprint failure string you use elsewhere), and for any other error call
Expect(err).NotTo(HaveOccurred(), ...) so unrelated deploy failures fail the
spec; update both the VPC and IAM DescribeStacks error branches to perform this
specific-message check before skipping.
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
Summary
rosactl cluster deploy CLUSTER_NAMEcommand that deploys both IAM and VPC CloudFormation stacks concurrently using goroutinescluster-iam createandcluster-vpc createsequentially--oidc-issuer-url,--region, and--availability-zonesare requiredUsage
Optional VPC flags (
--vpc-cidr,--public-subnet-cidrs,--private-subnet-cidrs,--single-nat-gateway) carry the same defaults ascluster-vpc create.Implementation
internal/commands/clusterwithcluster.go(parent command) anddeploy.go(deploy subcommand)clustercommand is registered alongside existing commands inroot.goTest plan
go build ./...passes (verified locally)rosactl cluster --helpshows thedeploysubcommandrosactl cluster deploy --helpshows all expected flagsrosa-<name>-iamandrosa-<name>-vpcstacks are created🤖 Generated with Claude Code