Update create EC2 options #133
Conversation
📝 WalkthroughWalkthroughThis pull request adds EC2 cross-account credential support: new CLI flags and options for target access keys and an auto-target-credentials mode; wiring of those options through create/patch flows; automatic fetching of target credentials and target AZ from the cluster; and writing target credentials into provider secrets. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / CLI
participant Cmd as Create/Patch Command
participant Provider as Provider Create/Patch Logic
participant AutoFetch as EC2 Auto-Populate Module
participant K8sAPI as Kubernetes API (Secrets/Nodes)
participant SecretMgr as Secret Creation/Update
User->>Cmd: run create/patch with flags (may include --auto-target-credentials)
Cmd->>Provider: call Create/Patch(..., ec2TargetAccessKeyID, ec2TargetSecretKey, autoTargetCredentials)
Provider->>AutoFetch: if autoTargetCredentials && missing fields -> AutoPopulateTargetOptions(...)
AutoFetch->>K8sAPI: Get secret `kube-system/aws-creds` / List nodes
K8sAPI-->>AutoFetch: return credentials and node labels
AutoFetch-->>Provider: return targetAccessKeyID, targetSecretKey, targetAZ (or error)
Provider->>SecretMgr: createSecret/updateSecret(..., targetAccessKeyID, targetSecretKey, targetAZ)
SecretMgr->>K8sAPI: Create/Update Secret with cross-account fields
K8sAPI-->>SecretMgr: success
SecretMgr-->>Provider: secret persisted
Provider-->>Cmd: complete (success/failure)
Cmd-->>User: CLI result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code graph analysis (3)pkg/cmd/create/provider/ec2/ec2.go (1)
pkg/cmd/create/provider/ec2/cluster.go (1)
pkg/cmd/patch/provider/patch.go (1)
🔇 Additional comments (10)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cmd/patch/provider/patch.go (1)
206-212: Inconsistent default behavior between create and patch.When patching
EC2TargetRegionwithout specifyingEC2TargetAZ, the code defaults to<region>a. However, in the create path withAutoTargetCredentialsenabled, the AZ is fetched from the cluster. This inconsistency could lead to unexpected configurations.Consider either:
- Removing the automatic default here and requiring explicit AZ when changing region
- Applying the same auto-detection logic when
AutoTargetCredentialsis enabledSuggested approach
if opts.EC2TargetRegion != "" { klog.V(2).Infof("Updating EC2 target-region to '%s'", opts.EC2TargetRegion) currentSettings["target-region"] = opts.EC2TargetRegion providerUpdated = true - // If no explicit target AZ is provided in this patch, apply the documented default "<target-region>a" - if opts.EC2TargetAZ == "" { + // Only default target-az if AutoTargetCredentials was not used + // (AutoTargetCredentials already sets AZ from cluster) + if opts.EC2TargetAZ == "" && !opts.AutoTargetCredentials { defaultAZ := opts.EC2TargetRegion + "a" - klog.V(2).Infof("No EC2 target-az provided, defaulting to '%s'", defaultAZ) + klog.V(2).Infof("No EC2 target-az provided and auto-detection disabled, defaulting to '%s'", defaultAZ) currentSettings["target-az"] = defaultAZ providerUpdated = true } }
🤖 Fix all issues with AI agents
In @pkg/cmd/patch/provider/patch.go:
- Around line 91-125: Duplicate auto-fetch logic in the EC2 patch path: when
providerType == "ec2" and opts.AutoTargetCredentials the block calling
ec2.FetchAWSCredentialsFromCluster and ec2.FetchTargetAZFromCluster is repeated;
extract that logic into a shared helper (e.g.,
ec2.AutoPopulateTargetOptions(configFlags *genericclioptions.ConfigFlags,
targetAccessKeyID, targetSecretKey, targetAZ, targetRegion *string) error)
inside the ec2 package, update pkg/cmd/patch/provider/patch.go to call
ec2.AutoPopulateTargetOptions(&opts.ConfigFlags, &opts.EC2TargetAccessKeyID,
&opts.EC2TargetSecretKey, &opts.EC2TargetAZ, &opts.EC2TargetRegion) when
opts.AutoTargetCredentials is true, and replace the duplicate block in
pkg/cmd/create/provider/ec2/ec2.go to call the same helper so all
credential/AZ/region auto-population is centralized.
🧹 Nitpick comments (7)
pkg/cmd/create/provider/ec2/secrets.go (1)
40-46: Consider validating that target credentials are provided together.The code adds target credentials independently if they're non-empty, but cross-account authentication typically requires both the access key ID and secret access key together. If only one is provided, it could lead to authentication failures that are harder to debug.
♻️ Proposed validation to ensure both credentials are provided together
// Add cross-account migration credentials (optional) - if targetAccessKeyID != "" { + if targetAccessKeyID != "" && targetSecretAccessKey != "" { secretData["targetAccessKeyId"] = []byte(targetAccessKeyID) - } - if targetSecretAccessKey != "" { secretData["targetSecretAccessKey"] = []byte(targetSecretAccessKey) + } else if targetAccessKeyID != "" || targetSecretAccessKey != "" { + return nil, fmt.Errorf("both targetAccessKeyID and targetSecretAccessKey must be provided together for cross-account migrations") }pkg/cmd/create/provider/ec2/cluster.go (1)
73-106: Consider the behavior in multi-AZ clusters.The function returns the first availability zone found on any worker node, but doesn't validate whether all worker nodes are in the same AZ or warn if multiple AZs are detected. In multi-AZ clusters, this could lead to unexpected behavior where the auto-detected AZ might not align with user expectations.
Consider adding logic to detect and warn about multi-AZ configurations, or document this limitation clearly.
♻️ Proposed enhancement to detect multi-AZ configuration
// Find the first node with a topology zone label + detectedAZ := "" + multiAZ := false for _, node := range nodes.Items { if zone, ok := node.Labels[topologyZoneLabel]; ok && zone != "" { + if detectedAZ == "" { + detectedAZ = zone + } else if detectedAZ != zone { + multiAZ = true + } - klog.V(2).Infof("Detected target availability zone '%s' from worker node '%s'", - zone, node.Name) - return zone, nil } } - return "", fmt.Errorf("no worker node found with topology zone label '%s'", topologyZoneLabel) + if detectedAZ == "" { + return "", fmt.Errorf("no worker node found with topology zone label '%s'", topologyZoneLabel) + } + + if multiAZ { + klog.V(1).Infof("Warning: Multiple availability zones detected in cluster. Using '%s' as target AZ", detectedAZ) + } else { + klog.V(2).Infof("Detected target availability zone '%s' from worker nodes", detectedAZ) + } + + return detectedAZ, nilcmd/create/provider.go (1)
116-118: Consider adding mutual exclusivity validation for credential flags.Users can currently specify both manual target credentials (
--target-access-key-id,--target-secret-access-key) and--auto-target-credentialssimultaneously, which could be confusing. It's unclear which would take precedence.Consider adding validation in the
PreRunEorRunEto ensure these options are mutually exclusive, or document the precedence behavior clearly.♻️ Proposed validation for mutual exclusivity
Add this validation in the
RunEfunction after line 77:cacert = string(fileContent) } + // Validate mutual exclusivity of manual and auto target credentials + if autoTargetCredentials && (ec2TargetAccessKeyID != "" || ec2TargetSecretKey != "") { + return fmt.Errorf("cannot specify both --auto-target-credentials and manual target credentials (--target-access-key-id, --target-secret-access-key)") + } + return provider.Create(kubeConfigFlags, providerType.GetValue(), name, namespace, secret,cmd/patch/provider.go (1)
85-87: Consider adding mutual exclusivity validation for credential flags.Similar to the create command, users can specify both manual target credentials and
--auto-target-credentialssimultaneously. This could lead to confusion about which credentials will be used.Consider adding the same validation as suggested for the create command to ensure these options are mutually exclusive.
♻️ Proposed validation for mutual exclusivity
Add this validation in the
RunEfunction after line 53:opts.InsecureSkipTLSChanged = cmd.Flag("provider-insecure-skip-tls").Changed opts.UseVddkAioOptimizationChanged = cmd.Flag("use-vddk-aio-optimization").Changed + // Validate mutual exclusivity of manual and auto target credentials + if opts.AutoTargetCredentials && (opts.EC2TargetAccessKeyID != "" || opts.EC2TargetSecretKey != "") { + return fmt.Errorf("cannot specify both --auto-target-credentials and manual target credentials (--target-access-key-id, --target-secret-access-key)") + } + return provider.PatchProvider(opts) }pkg/cmd/create/provider/ec2/ec2.go (1)
137-142: Validate AZ format before deriving region.The region derivation logic assumes the AZ always ends with a single character (e.g., "us-east-1a" → "us-east-1"). While this is standard for AWS, the code doesn't validate this assumption. If the AZ format is unexpected or malformed, this could produce an incorrect region.
🛡️ Add validation before deriving region
// Also set target region from target-az if not provided - if options.EC2TargetRegion == "" && len(targetAZ) > 1 { + if options.EC2TargetRegion == "" && len(targetAZ) > 2 { // Extract region from AZ (e.g., "us-east-1a" -> "us-east-1") - options.EC2TargetRegion = targetAZ[:len(targetAZ)-1] - fmt.Printf("Auto-detected target region: %s\n", options.EC2TargetRegion) + derivedRegion := targetAZ[:len(targetAZ)-1] + // Validate that the derived region looks reasonable (contains hyphens) + if strings.Contains(derivedRegion, "-") { + options.EC2TargetRegion = derivedRegion + fmt.Printf("Auto-detected target region: %s\n", options.EC2TargetRegion) + } }pkg/cmd/create/provider/create.go (1)
22-57: Extended function signature correctly propagates EC2 target credentials.The new parameters (
ec2TargetAccessKeyID,ec2TargetSecretKey,autoTargetCredentials) are correctly added to the function signature and properly populated in theProviderOptionsstruct.Optional future improvement: The
Createfunction signature now has 26 parameters, which impacts maintainability. Consider refactoring to use a configuration struct or options pattern in future work.pkg/cmd/patch/provider/patch.go (1)
118-123: Validate AZ format before deriving region.The region derivation uses the same fragile string slicing approach as in
ec2/ec2.go. Without format validation, unexpected AZ formats could produce incorrect regions.Apply the same validation logic recommended for
ec2/ec2.goline 137-142.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/create/provider.gocmd/patch/provider.gopkg/cmd/create/provider/create.gopkg/cmd/create/provider/ec2/cluster.gopkg/cmd/create/provider/ec2/ec2.gopkg/cmd/create/provider/ec2/secrets.gopkg/cmd/create/provider/providerutil/options.gopkg/cmd/patch/provider/patch.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/cmd/create/provider/ec2/ec2.go (1)
pkg/cmd/create/provider/ec2/cluster.go (2)
FetchAWSCredentialsFromCluster(33-71)FetchTargetAZFromCluster(75-106)
pkg/cmd/patch/provider/patch.go (1)
pkg/cmd/create/provider/ec2/cluster.go (2)
FetchAWSCredentialsFromCluster(33-71)FetchTargetAZFromCluster(75-106)
⏰ 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 kubectl-mtv
🔇 Additional comments (10)
pkg/cmd/create/provider/ec2/secrets.go (1)
16-16: LGTM! Function signature extended appropriately.The addition of
targetAccessKeyIDandtargetSecretAccessKeyparameters aligns with the cross-account migration feature. The parameter naming is consistent with AWS conventions.pkg/cmd/create/provider/providerutil/options.go (1)
26-31: LGTM! Well-documented field additions.The three new fields for cross-account EC2 migrations are clearly documented and follow consistent naming conventions. The comments effectively explain their purpose.
pkg/cmd/create/provider/ec2/cluster.go (3)
14-23: LGTM! Well-defined constants.The constants use standard Kubernetes and OpenShift conventions. The
aws-credssecret inkube-systemis indeed created by the OpenShift installer on AWS clusters, and the topology labels follow Kubernetes standards.
25-29: LGTM! Simple and clear credential structure.The struct provides a clean interface for returning AWS credentials with appropriately named fields.
31-71: LGTM! Robust credential fetching with good error handling.The function properly validates that both required credential fields exist in the secret and provides helpful error messages. The logging at verbosity level 2 is appropriate and doesn't expose sensitive data.
cmd/create/provider.go (2)
35-36: LGTM! Flag variables declared appropriately.The new variables for cross-account EC2 credentials follow the existing patterns and naming conventions.
81-82: LGTM! Parameters correctly passed to provider creation.The new credential parameters are properly threaded through to the
provider.Createfunction.pkg/cmd/create/provider/ec2/ec2.go (1)
196-198: LGTM!The extended
createSecretcall correctly passes the new EC2 target credential parameters for cross-account migration support.pkg/cmd/patch/provider/patch.go (2)
51-56: LGTM!The new EC2-related fields are correctly added to
PatchProviderOptionsto support cross-account migration credentials.
129-131: LGTM!The credential update logic correctly extends to handle EC2 cross-account target credentials. The
needsCredentialUpdatecondition, function signature, and EC2 case implementation all properly integrate the newEC2TargetAccessKeyIDandEC2TargetSecretKeyfields.Also applies to: 231-232, 335-335, 405-414
| // Auto-fetch target credentials and target-az from cluster if requested (EC2 only) | ||
| if providerType == "ec2" && opts.AutoTargetCredentials { | ||
| // Fetch AWS credentials from cluster secret (kube-system/aws-creds) | ||
| if opts.EC2TargetAccessKeyID == "" || opts.EC2TargetSecretKey == "" { | ||
| clusterCreds, err := ec2.FetchAWSCredentialsFromCluster(opts.ConfigFlags) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to auto-fetch target credentials: %v", err) | ||
| } | ||
| if opts.EC2TargetAccessKeyID == "" { | ||
| opts.EC2TargetAccessKeyID = clusterCreds.AccessKeyID | ||
| fmt.Printf("Auto-detected target access key ID from cluster secret\n") | ||
| } | ||
| if opts.EC2TargetSecretKey == "" { | ||
| opts.EC2TargetSecretKey = clusterCreds.SecretAccessKey | ||
| fmt.Printf("Auto-detected target secret access key from cluster secret\n") | ||
| } | ||
| } | ||
|
|
||
| // Auto-detect target-az from worker nodes if not provided | ||
| if opts.EC2TargetAZ == "" { | ||
| targetAZ, err := ec2.FetchTargetAZFromCluster(opts.ConfigFlags) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to auto-detect target availability zone: %v", err) | ||
| } | ||
| opts.EC2TargetAZ = targetAZ | ||
| fmt.Printf("Auto-detected target availability zone: %s\n", targetAZ) | ||
|
|
||
| // Also set target region from target-az if not provided | ||
| if opts.EC2TargetRegion == "" && len(targetAZ) > 1 { | ||
| // Extract region from AZ (e.g., "us-east-1a" -> "us-east-1") | ||
| opts.EC2TargetRegion = targetAZ[:len(targetAZ)-1] | ||
| fmt.Printf("Auto-detected target region: %s\n", opts.EC2TargetRegion) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated auto-fetch logic into a shared helper function.
This auto-fetch block duplicates the logic from pkg/cmd/create/provider/ec2/ec2.go lines 110-144. Code duplication makes maintenance harder—any bug fixes or enhancements must be applied in both places.
♻️ Recommended refactor
Extract the auto-fetch logic into a shared function in the ec2 package:
// In pkg/cmd/create/provider/ec2/ec2.go or a new helpers.go file
// AutoPopulateTargetOptions auto-fetches target credentials and AZ from the cluster
func AutoPopulateTargetOptions(configFlags *genericclioptions.ConfigFlags,
targetAccessKeyID, targetSecretKey, targetAZ, targetRegion *string) error {
// Fetch AWS credentials from cluster secret if needed
if *targetAccessKeyID == "" || *targetSecretKey == "" {
clusterCreds, err := FetchAWSCredentialsFromCluster(configFlags)
if err != nil {
return fmt.Errorf("failed to auto-fetch target credentials: %v", err)
}
if *targetAccessKeyID == "" {
*targetAccessKeyID = clusterCreds.AccessKeyID
fmt.Printf("Auto-detected target access key ID from cluster secret\n")
}
if *targetSecretKey == "" {
*targetSecretKey = clusterCreds.SecretAccessKey
fmt.Printf("Auto-detected target secret access key from cluster secret\n")
}
}
// Auto-detect target-az from worker nodes if not provided
if *targetAZ == "" {
az, err := FetchTargetAZFromCluster(configFlags)
if err != nil {
return fmt.Errorf("failed to auto-detect target availability zone: %v", err)
}
*targetAZ = az
fmt.Printf("Auto-detected target availability zone: %s\n", az)
// Also set target region from target-az if not provided
if *targetRegion == "" && len(az) > 1 {
*targetRegion = az[:len(az)-1]
fmt.Printf("Auto-detected target region: %s\n", *targetRegion)
}
}
return nil
}Then use it in both ec2.go and patch.go:
if options.AutoTargetCredentials {
- // Fetch AWS credentials from cluster secret (kube-system/aws-creds)
- if options.EC2TargetAccessKeyID == "" || options.EC2TargetSecretKey == "" {
- clusterCreds, err := FetchAWSCredentialsFromCluster(configFlags)
- ...
- }
+ if err := ec2.AutoPopulateTargetOptions(configFlags,
+ &options.EC2TargetAccessKeyID, &options.EC2TargetSecretKey,
+ &options.EC2TargetAZ, &options.EC2TargetRegion); err != nil {
+ return nil, nil, err
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @pkg/cmd/patch/provider/patch.go around lines 91 - 125, Duplicate auto-fetch
logic in the EC2 patch path: when providerType == "ec2" and
opts.AutoTargetCredentials the block calling ec2.FetchAWSCredentialsFromCluster
and ec2.FetchTargetAZFromCluster is repeated; extract that logic into a shared
helper (e.g., ec2.AutoPopulateTargetOptions(configFlags
*genericclioptions.ConfigFlags, targetAccessKeyID, targetSecretKey, targetAZ,
targetRegion *string) error) inside the ec2 package, update
pkg/cmd/patch/provider/patch.go to call
ec2.AutoPopulateTargetOptions(&opts.ConfigFlags, &opts.EC2TargetAccessKeyID,
&opts.EC2TargetSecretKey, &opts.EC2TargetAZ, &opts.EC2TargetRegion) when
opts.AutoTargetCredentials is true, and replace the duplicate block in
pkg/cmd/create/provider/ec2/ec2.go to call the same helper so all
credential/AZ/region auto-population is centralized.
Signed-off-by: yaacov <[email protected]>
yaacov
force-pushed
the
update-create-ec2
branch
from
25628de to
4bded78
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.