CNTRLPLANE-2172: feat(install): register Azure private platform flags#7912
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughAdds Azure private-platform support to the install command: new Options fields, CLI flags, and platform validation for Azure; includes unit tests for Azure validation. No runtime wiring beyond flag acceptance and validation is implemented in this diff. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
area/platform/azure
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
approved
bryan-cox
force-pushed
the
azure-install-flags
branch
from
3a63fa1 to
b66e14f
Compare
bryan-cox
changed the title
![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
🧹 Nitpick comments (2)
cmd/install/install.go (2)
172-177: Consider adding minimal validation for Azure flags now.The Azure case accepts all flags without validation, which could lead to silent misconfiguration. Even though full wiring is deferred to CNTRLPLANE-2172, consider adding basic validation now to catch obvious errors early:
- Mutual exclusivity:
--azure-private-credsvs--azure-private-secret- Mutual exclusivity: credential file approach vs managed identity approach (
--azure-pls-managed-identity-client-id)- Required combinations: if managed identity is specified,
--azure-pls-subscription-idmay be requiredThis would be consistent with AWS validation (lines 162-165) which validates required flag combinations.
♻️ Suggested validation to add
case hyperv1.AzurePlatform: - // Azure private platform flags are accepted but not yet wired up. - // Full support is coming in the CNTRLPLANE-2172 feature PR. + // Validate mutual exclusivity of credential sources + if len(o.AzurePrivateCreds) > 0 && len(o.AzurePrivateCredentialsSecret) > 0 { + errs = append(errs, fmt.Errorf("only one of --azure-private-creds or --azure-private-secret is supported")) + } + if len(o.AzurePrivateCreds) > 0 && len(o.AzurePLSManagedIdentityClientID) > 0 { + errs = append(errs, fmt.Errorf("--azure-private-creds and --azure-pls-managed-identity-client-id are mutually exclusive")) + } + // Note: Full wiring is coming in the CNTRLPLANE-2172 feature PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/install.go` around lines 172 - 177, In the AzurePlatform switch branch in install.go, add minimal flag validation similar to the AWS checks: ensure --azure-private-creds and --azure-private-secret are mutually exclusive, ensure the credential-file approach and the managed-identity approach are mutually exclusive (i.e., --azure-private-creds/--azure-private-secret vs --azure-pls-managed-identity-client-id), and if --azure-pls-managed-identity-client-id is provided require --azure-pls-subscription-id; when a rule fails append an error to errs (same pattern as the AWS validation block) so consumers get early actionable errors.
1297-1319: Note: Azure credentials wiring is missing (intentionally deferred).The
setupAuth()function handles AWS credentials (lines 1298-1318) but doesn't yet handle Azure credentials. This is consistent with the PR objectives stating that wiring is deferred to CNTRLPLANE-2172.When implementing the full wiring, you'll need to add an
AzurePlatformcase here to:
- Read credentials from
opts.AzurePrivateCredsfile or referenceopts.AzurePrivateCredentialsSecret- Create the appropriate secret for Azure Private Link Service operations
Would you like me to draft the Azure credentials handling code for the follow-up PR?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/install.go` around lines 1297 - 1319, The setupAuth() switch currently handles AWS only; add an AzurePlatform case in the switch over hyperv1.PlatformType(opts.PrivatePlatform) to mirror the AWS logic: if opts.AzurePrivateCreds is set, read the file (like os.ReadFile used for AWS), build an assets.HyperShiftOperatorCredentialsSecret with Namespace operatorNamespace, CredsBytes from the file, and CredsKey from the Azure secret key option, then append it to objects; else if opts.AzurePrivateCredentialsSecret is set, create a corev1.Secret stub with ObjectMeta namespace operatorNamespace.Name and Name set to opts.AzurePrivateCredentialsSecret and assign it to operatorCredentialsSecret. Ensure you use the same variable names (operatorCredentialsSecret, objects) and patterns as the AWS case so wiring matches for future CNTRLPLANE-2172 work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/install/install.go`:
- Around line 311-316: The help text claims flags are required but Validate()
doesn't enforce them; either add validation in Validate() to enforce that when
opts.AzurePLSManagedIdentityClientID is non-empty then
opts.AzurePLSSubscriptionID must be set, and that when opts.PrivatePlatform ==
"Azure" (self-managed) then opts.AzurePLSResourceGroup must be set, or change
the flag help strings for AzurePLSSubscriptionID and AzurePLSResourceGroup to
say "will be required when Azure support is fully implemented" (update the
StringVar calls for AzurePLSSubscriptionID and AzurePLSResourceGroup
accordingly); reference the Validate() function and the opts fields
AzurePLSManagedIdentityClientID, AzurePLSSubscriptionID, AzurePLSResourceGroup,
and PrivatePlatform to locate the code.
---
Nitpick comments:
In `@cmd/install/install.go`:
- Around line 172-177: In the AzurePlatform switch branch in install.go, add
minimal flag validation similar to the AWS checks: ensure --azure-private-creds
and --azure-private-secret are mutually exclusive, ensure the credential-file
approach and the managed-identity approach are mutually exclusive (i.e.,
--azure-private-creds/--azure-private-secret vs
--azure-pls-managed-identity-client-id), and if
--azure-pls-managed-identity-client-id is provided require
--azure-pls-subscription-id; when a rule fails append an error to errs (same
pattern as the AWS validation block) so consumers get early actionable errors.
- Around line 1297-1319: The setupAuth() switch currently handles AWS only; add
an AzurePlatform case in the switch over
hyperv1.PlatformType(opts.PrivatePlatform) to mirror the AWS logic: if
opts.AzurePrivateCreds is set, read the file (like os.ReadFile used for AWS),
build an assets.HyperShiftOperatorCredentialsSecret with Namespace
operatorNamespace, CredsBytes from the file, and CredsKey from the Azure secret
key option, then append it to objects; else if
opts.AzurePrivateCredentialsSecret is set, create a corev1.Secret stub with
ObjectMeta namespace operatorNamespace.Name and Name set to
opts.AzurePrivateCredentialsSecret and assign it to operatorCredentialsSecret.
Ensure you use the same variable names (operatorCredentialsSecret, objects) and
patterns as the AWS case so wiring matches for future CNTRLPLANE-2172 work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a64402e7-922f-490a-a2ba-6dc2d46543a3
📒 Files selected for processing (1)
cmd/install/install.go
| cmd.PersistentFlags().StringVar(&opts.AzurePrivateCreds, "azure-private-creds", opts.AzurePrivateCreds, "Path to an Azure credentials file with privileges sufficient to manage private cluster resources") | ||
| cmd.PersistentFlags().StringVar(&opts.AzurePrivateCredentialsSecret, "azure-private-secret", "", "Name of an existing secret containing the Azure private link credentials") | ||
| cmd.PersistentFlags().StringVar(&opts.AzurePrivateCredentialsSecretKey, "azure-private-secret-key", "credentials", "Name of the secret key containing the Azure private link credentials") | ||
| cmd.PersistentFlags().StringVar(&opts.AzurePLSManagedIdentityClientID, "azure-pls-managed-identity-client-id", "", "Client ID of the managed identity for Azure Private Link Service operations (alternative to credential file; uses Azure Workload Identity federation)") |
There was a problem hiding this comment.
is this alternative mutually exclusive? can that and any other permutation not allowed for this new input be enforced in Validate()?
There was a problem hiding this comment.
Good question. Yes, the managed identity approach (--azure-pls-managed-identity-client-id) is mutually exclusive with the credential file approach (--azure-private-creds / --azure-private-secret). I've added validation in Validate() to enforce this, along with requiring --azure-pls-subscription-id when using managed identity. Tests added for all permutations.
AI-assisted response via Claude Code
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped a question, let me know for tagging.
| case hyperv1.NonePlatform: | ||
| default: | ||
| errs = append(errs, fmt.Errorf("--private-platform must be either %s, %s, or %s", hyperv1.AWSPlatform, hyperv1.GCPPlatform, hyperv1.NonePlatform)) | ||
| errs = append(errs, fmt.Errorf("--private-platform must be either %s, %s, %s, or %s", hyperv1.AWSPlatform, hyperv1.AzurePlatform, hyperv1.GCPPlatform, hyperv1.NonePlatform)) |
There was a problem hiding this comment.
NonePlatform is something we still want to support in the near future?
There was a problem hiding this comment.
Yes, NonePlatform is the default value and is used when the operator does not need to support private cluster connectivity for any platform. It remains the common case for development environments and management clusters that don't manage private clusters.
AI-assisted response via Claude Code
bryan-cox
force-pushed
the
azure-install-flags
branch
from
b66e14f to
2ba48fc
Compare
bryan-cox
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references CNTRLPLANE-2172 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: This pull request references CNTRLPLANE-2172 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/install/install.go`:
- Around line 172-184: The validation currently accepts Azure in Validate() but
the install path doesn't wire Azure creds into setupAuth() or pass Azure
private-platform settings into setupOperatorResources(), nor validates/consumes
--azure-pls-resource-group; fix by either reverting Azure acceptance in the
switch (keep Azure rejected until wiring lands) or wire Azure through: update
setupAuth() to materialize Azure secret/PLS settings when o.AzurePrivateCreds /
o.AzurePrivateCredentialsSecret / o.AzurePLSManagedIdentityClientID /
o.AzurePLSSubscriptionID / o.AzurePLSResourceGroup are provided, validate
--azure-pls-resource-group in the same validation block alongside
AzurePLSSubscriptionID, and ensure setupOperatorResources() receives the Azure
private-platform settings so operator manifests include the Azure configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b55e3b4-4449-4b07-ac6d-9d4fa8dc75cc
📒 Files selected for processing (2)
cmd/install/install.gocmd/install/install_test.go
| case hyperv1.AzurePlatform: | ||
| if len(o.AzurePrivateCreds) > 0 && len(o.AzurePLSManagedIdentityClientID) > 0 { | ||
| errs = append(errs, fmt.Errorf("--azure-private-creds and --azure-pls-managed-identity-client-id are mutually exclusive")) | ||
| } | ||
| if len(o.AzurePrivateCredentialsSecret) > 0 && len(o.AzurePLSManagedIdentityClientID) > 0 { | ||
| errs = append(errs, fmt.Errorf("--azure-private-secret and --azure-pls-managed-identity-client-id are mutually exclusive")) | ||
| } | ||
| if len(o.AzurePLSManagedIdentityClientID) > 0 && len(o.AzurePLSSubscriptionID) == 0 { | ||
| errs = append(errs, fmt.Errorf("--azure-pls-subscription-id is required with --azure-pls-managed-identity-client-id")) | ||
| } | ||
| case hyperv1.NonePlatform: | ||
| default: | ||
| errs = append(errs, fmt.Errorf("--private-platform must be either %s, %s, or %s", hyperv1.AWSPlatform, hyperv1.GCPPlatform, hyperv1.NonePlatform)) | ||
| errs = append(errs, fmt.Errorf("--private-platform must be either %s, %s, %s, or %s", hyperv1.AWSPlatform, hyperv1.AzurePlatform, hyperv1.GCPPlatform, hyperv1.NonePlatform)) |
There was a problem hiding this comment.
Azure private-platform is accepted before the install path actually uses the Azure inputs.
Validate() now treats Azure as supported, and the CLI exposes the new Azure flags, but this file still only materializes private-platform credentials in setupAuth() for AWS and only passes AWS/GCP private-platform settings into setupOperatorResources(). --azure-pls-resource-group is also documented on Line 323 but never validated or consumed. As written, hypershift install --private-platform=Azure ... can succeed while rendering an operator deployment with none of the Azure-specific configuration attached.
Either keep Azure rejected here until the manifest wiring lands, or thread the Azure secret/PLS settings through in this PR.
Also applies to: 313-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/install/install.go` around lines 172 - 184, The validation currently
accepts Azure in Validate() but the install path doesn't wire Azure creds into
setupAuth() or pass Azure private-platform settings into
setupOperatorResources(), nor validates/consumes --azure-pls-resource-group; fix
by either reverting Azure acceptance in the switch (keep Azure rejected until
wiring lands) or wire Azure through: update setupAuth() to materialize Azure
secret/PLS settings when o.AzurePrivateCreds / o.AzurePrivateCredentialsSecret /
o.AzurePLSManagedIdentityClientID / o.AzurePLSSubscriptionID /
o.AzurePLSResourceGroup are provided, validate --azure-pls-resource-group in the
same validation block alongside AzurePLSSubscriptionID, and ensure
setupOperatorResources() receives the Azure private-platform settings so
operator manifests include the Azure configuration.
|
Scheduling tests matching the |
|
/verified bypass |
openshift-ci-robot
added
the
verified
|
@bryan-cox: The DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Add Azure-specific flags for the HyperShift install command to support private cluster connectivity via Azure Private Link Service (PLS). New flags: - --azure-private-creds: path to Azure credentials file - --azure-private-secret: name of existing credentials secret - --azure-private-secret-key: key within the credentials secret - --azure-pls-managed-identity-client-id: managed identity for PLS ops - --azure-pls-subscription-id: Azure subscription for PLS operations - --azure-pls-resource-group: resource group for PLS and load balancers The credential file and managed identity approaches are mutually exclusive. When using managed identity, the subscription ID is required. Updates --private-platform to accept "Azure" alongside existing "AWS", "GCP", and "None" values. Ref: CNTRLPLANE-2172 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
azure-install-flags
branch
from
2ba48fc to
55a6d6e
Compare
openshift-ci-robot
removed
the
verified
|
@bryan-cox: This pull request references CNTRLPLANE-2172 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/install/install_test.go (1)
173-212: Good test coverage for Azure private platform validation scenarios.The tests appropriately cover the mutual exclusivity constraints and required field combinations:
- Lines 173-181: Managed identity + creds file → error ✓
- Lines 182-190: Managed identity + secret → error ✓
- Lines 191-197: Managed identity without subscription ID → error ✓
- Lines 198-205: Managed identity with subscription ID → success ✓
- Lines 206-212: Creds file alone → success ✓
Consider adding a test case for
AzurePrivateCredentialsSecretalone (without managed identity) to verify it succeeds, similar to the AWS secret-only case at lines 41-48.💡 Suggested additional test case
"When Azure private platform with secret it should succeed": { inputOptions: Options{ PrivatePlatform: string(hyperv1.AzurePlatform), AzurePrivateCredentialsSecret: "my-secret", }, expectError: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/install_test.go` around lines 173 - 212, Add a new unit test case to cover the scenario where AzurePrivateCredentialsSecret is provided alone: update the test table in cmd/install/install_test.go to include a case named like "When Azure private platform with secret it should succeed" that uses Options with PrivatePlatform set to string(hyperv1.AzurePlatform) and AzurePrivateCredentialsSecret set to "my-secret", and set expectError to false; ensure this mirrors the AWS secret-only case pattern and validates the validation logic around AzurePrivateCredentialsSecret versus managed identity fields (AzurePLSManagedIdentityClientID, AzurePLSSubscriptionID).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/install/install.go`:
- Around line 172-181: The Azure branch in the switch for AzurePlatform
currently allows no authentication/config to be provided; either enforce the
missing validations to match AWS or explicitly document the intentional
deferment—update the case handling for AzurePlatform (symbols: AzurePlatform,
AzurePrivateCreds, AzurePrivateCredentialsSecret,
AzurePLSManagedIdentityClientID, AzurePLSSubscriptionID, AzurePLSResourceGroup)
by adding the same required checks as AWS (e.g., require credentials or managed
identity and required subscription/region/resource-group) or, if deferred
intentionally, add a clear code comment above the AzurePlatform case explaining
that authentication/resource-group validation is intentionally postponed to the
feature PR to avoid confusion.
---
Nitpick comments:
In `@cmd/install/install_test.go`:
- Around line 173-212: Add a new unit test case to cover the scenario where
AzurePrivateCredentialsSecret is provided alone: update the test table in
cmd/install/install_test.go to include a case named like "When Azure private
platform with secret it should succeed" that uses Options with PrivatePlatform
set to string(hyperv1.AzurePlatform) and AzurePrivateCredentialsSecret set to
"my-secret", and set expectError to false; ensure this mirrors the AWS
secret-only case pattern and validates the validation logic around
AzurePrivateCredentialsSecret versus managed identity fields
(AzurePLSManagedIdentityClientID, AzurePLSSubscriptionID).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 535c3312-cb2b-468d-a6d0-9f639f4add0b
📒 Files selected for processing (2)
cmd/install/install.gocmd/install/install_test.go
| case hyperv1.AzurePlatform: | ||
| if len(o.AzurePrivateCreds) > 0 && len(o.AzurePLSManagedIdentityClientID) > 0 { | ||
| errs = append(errs, fmt.Errorf("--azure-private-creds and --azure-pls-managed-identity-client-id are mutually exclusive")) | ||
| } | ||
| if len(o.AzurePrivateCredentialsSecret) > 0 && len(o.AzurePLSManagedIdentityClientID) > 0 { | ||
| errs = append(errs, fmt.Errorf("--azure-private-secret and --azure-pls-managed-identity-client-id are mutually exclusive")) | ||
| } | ||
| if len(o.AzurePLSManagedIdentityClientID) > 0 && len(o.AzurePLSSubscriptionID) == 0 { | ||
| errs = append(errs, fmt.Errorf("--azure-pls-subscription-id is required with --azure-pls-managed-identity-client-id")) | ||
| } |
There was a problem hiding this comment.
Validation allows Azure platform with no authentication configured.
Unlike the AWS case (lines 162-165) which requires credentials and region, the Azure validation permits --private-platform=Azure with no credentials or managed identity specified. While this may be intentional for the flag registration phase, it differs from AWS behavior.
Additionally, the help text for --azure-pls-resource-group (line 323) states it's "required with --private-platform=Azure for self-managed clusters", but this requirement isn't enforced in validation.
If these are intentional deferrals until the feature PR, consider adding a brief code comment to clarify the intent and prevent future confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/install/install.go` around lines 172 - 181, The Azure branch in the
switch for AzurePlatform currently allows no authentication/config to be
provided; either enforce the missing validations to match AWS or explicitly
document the intentional deferment—update the case handling for AzurePlatform
(symbols: AzurePlatform, AzurePrivateCreds, AzurePrivateCredentialsSecret,
AzurePLSManagedIdentityClientID, AzurePLSSubscriptionID, AzurePLSResourceGroup)
by adding the same required checks as AWS (e.g., require credentials or managed
identity and required subscription/region/resource-group) or, if deferred
intentionally, add a clear code comment above the AzurePlatform case explaining
that authentication/resource-group validation is intentionally postponed to the
feature PR to avoid confusion.
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
|
/test e2e-aws |
|
/retest |
|
/verified bypass |
openshift-ci-robot
added
the
verified
|
@bryan-cox: The DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 participants
Summary
Register
--azure-private-creds,--azure-private-secret,--azure-private-secret-key,--azure-pls-managed-identity-client-id,--azure-pls-subscription-id, and--azure-pls-resource-groupflags onhypershift install, and acceptAzureas a valid--private-platformvalue.The flags are registered but not yet wired to operator deployment logic. This unblocks the CI job in the release repo PR.
PR ordering
hypershift install --private-platform=Azure --azure-private-creds=... --azure-pls-resource-group=...doesn't error with "unknown flag"e2e-azure-self-managedCI job which invokeshypershift installwith these flagsTest plan
go build ./cmd/install/...compiles cleanlygo test ./cmd/install/...passesJIRA: CNTRLPLANE-2172
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests