ARO-24037 feat(azure): add ACR pull identity to worker cloud.conf and VMSS#8472
Open
twolff-gh wants to merge 6 commits into
Open
ARO-24037 feat(azure): add ACR pull identity to worker cloud.conf and VMSS#8472twolff-gh wants to merge 6 commits into
twolff-gh wants to merge 6 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
needs-rebase
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThese changes add optional image registry credentials to AzurePlatformSpec to specify a user-assigned managed identity (resourceID) for pulling images from Azure Container Registry. The control-plane operator propagates the managed identity ID into the generated Azure cloud config when present. The nodepool controller configures worker VM identity and user-assigned identities on Azure VM scale sets using the provided resourceID. Unit tests were added/updated to verify API serialization compatibility, cloud config output, secret contents, and machine template identity wiring. Sequence Diagram(s)sequenceDiagram
participant User
participant API_Server as Kubernetes API
participant CPO as Control-Plane Operator
participant NPC as NodePool Controller
participant Azure as Azure (VMSS / ACR)
User->>API_Server: Create/Update HostedCluster/NodePool\nwith platform.azure.imageRegistryCredentials.managedIdentity.resourceID
API_Server->>CPO: CPO watches HostedCluster spec
CPO->>CPO: adaptConfig reads managedIdentity.resourceID
CPO->>API_Server: Write ConfigMap (cloud.conf) including userAssignedIdentityID
API_Server->>NPC: NPC watches HostedCluster/NodePool spec
NPC->>NPC: azureMachineTemplateSpec reads managedIdentity.resourceID
NPC->>Azure: Create/Update VMSS with\nidentity: UserAssigned + UserAssignedIdentities[providerID]
Azure->>Azure: VMSS uses managed identity to pull images from ACR
Azure->>NPC: VMSS status/events
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
openshift-ci
Bot
added
area/cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/nodepool/azure.go`:
- Around line 248-256: The code is prepending capzutil.ProviderIDPrefix to the
user-assigned identity but CAPZ v1.21 requires a raw ARM resource ID; update the
block that populates azureMachineTemplate.Template.Spec.UserAssignedIdentities
so the ProviderID is set directly from
hostedCluster.Spec.Platform.Azure.AcrImagePullManagedIdentityID (remove
capzutil.ProviderIDPrefix) while keeping the same append logic and
VMIdentityUserAssigned assignment on
azureMachineTemplate.Template.Spec.Identity.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 651c0b52-b089-404a-a89f-e3b0947abee4
⛔ Files ignored due to path filters (40)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azureplatformspec.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/testdata/zz_fixture_TestConfigWithAcrMI.yamlis excluded by!**/testdata/**docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (5)
api/hypershift/v1beta1/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/azure_test.go
Author
|
/retest |
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
5491fa7 to
ba0c25b
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/hypershift/v1beta1/azure.go (2)
558-567: ⚡ Quick winConsider adding detailed component validation for consistency.
The validation pattern for
AzureManagedIdentityResourceIDuses a single rule that checks the overall format, but other ARM resource IDs in this file (e.g.,encryptionSetIDat lines 336-340,subnetIDat lines 428-435) include multiple detailed rules validating each component:
- Subscription ID as a valid UUID (8-4-4-4-12 format)
- Resource group name constraints (1-90 chars, valid characters, no trailing period)
- Resource/identity name constraints
Adding similar granular validation would improve input validation consistency and provide clearer error messages when individual components are malformed.
✨ Suggested additional validation rules
// +kubebuilder:validation:XValidation:rule="self.lowerAscii().matches('^/subscriptions/[^/]+/resourcegroups/[^/]+/providers/microsoft.managedidentity/userassignedidentities/[^/]+$')",message="must be an ARM resource ID in the format /subscriptions/{subscriptionID}/resourceGroups/{resourceGroupName}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/{identityName}" +// +kubebuilder:validation:XValidation:rule="self.split('/')[2].matches('^[{]?[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}[}]?$')",message="the subscriptionID must be a valid UUID in the form 8-4-4-4-12" +// +kubebuilder:validation:XValidation:rule=`self.split('/')[4].matches('[a-zA-Z0-9-_\\(\\)\\.]{1,90}')`,message="the resourceGroupName should be between 1 and 90 characters, consisting only of alphanumeric characters, hyphens, underscores, periods and parenthesis" +// +kubebuilder:validation:XValidation:rule="!self.split('/')[4].endsWith('.')",message="the resourceGroupName must not end with a period (.)" +// +kubebuilder:validation:XValidation:rule="self.split('/')[8].matches('[a-zA-Z0-9-_]{1,128}')",message="the identityName should be between 1 and 128 characters, consisting only of alphanumeric characters, hyphens and underscores" // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=512 type AzureManagedIdentityResourceID string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/hypershift/v1beta1/azure.go` around lines 558 - 567, Update the AzureManagedIdentityResourceID validation to add component-level checks similar to encryptionSetID and subnetID: keep the existing overall XValidation rule but add additional XValidation or kubebuilder:validation patterns that (1) validate the subscription ID is a UUID (8-4-4-4-12), (2) validate the resourceGroupName follows 1-90 char rules (allowed chars, no trailing period), and (3) validate the userAssignedIdentities/{identityName} segment enforces resource name constraints (length and allowed characters); target the AzureManagedIdentityResourceID type and mirror the same annotation style used for encryptionSetID and subnetID so each component produces a clear, specific error when invalid.
472-479: 💤 Low valueConsider whether
ImageRegistryCredentialsshould be immutable.Many infrastructure fields in
AzurePlatformSpecare marked immutable (e.g.,location,resourceGroup,vnetID,subnetID). SinceImageRegistryCredentialsconfigures VM scale set identity attachment, consider whether it should also be immutable once set, as changing VMSS identities post-creation can have operational implications.If mutability is intentional for operational flexibility, this is fine—just confirming the design choice.
💭 Optional immutability constraint
// imageRegistryCredentials configures authentication for worker nodes pulling images // from Azure Container Registry (ACR) using a user-assigned managed identity. // When set, the identity is attached to worker VM scale sets and its resource ID is written // into the worker cloud provider config so kubelet's credential provider can authenticate // to ACR without image pull secrets. // +// +kubebuilder:validation:XValidation:rule="!has(oldSelf.imageRegistryCredentials) || has(self.imageRegistryCredentials)",message="imageRegistryCredentials cannot be removed once set" // +optional ImageRegistryCredentials *AzureImageRegistryCredentials `json:"imageRegistryCredentials,omitempty"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/hypershift/v1beta1/azure.go` around lines 472 - 479, Decide and enforce immutability for the ImageRegistryCredentials field: either mark ImageRegistryCredentials on AzurePlatformSpec as immutable by adding the kubebuilder validation tag (e.g., +kubebuilder:validation:Immutable) and update the field comment to state it cannot be changed post-creation (since changing VMSS identities is disruptive), or if mutability is intentional, add a clear comment on ImageRegistryCredentials explaining why it must remain mutable and document any operational steps/impacts; update any validation logic or docs accordingly and reference the AzurePlatformSpec, ImageRegistryCredentials, and analogous immutable fields (location, resourceGroup, vnetID, subnetID) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/hypershift/v1beta1/azure.go`:
- Around line 558-567: Update the AzureManagedIdentityResourceID validation to
add component-level checks similar to encryptionSetID and subnetID: keep the
existing overall XValidation rule but add additional XValidation or
kubebuilder:validation patterns that (1) validate the subscription ID is a UUID
(8-4-4-4-12), (2) validate the resourceGroupName follows 1-90 char rules
(allowed chars, no trailing period), and (3) validate the
userAssignedIdentities/{identityName} segment enforces resource name constraints
(length and allowed characters); target the AzureManagedIdentityResourceID type
and mirror the same annotation style used for encryptionSetID and subnetID so
each component produces a clear, specific error when invalid.
- Around line 472-479: Decide and enforce immutability for the
ImageRegistryCredentials field: either mark ImageRegistryCredentials on
AzurePlatformSpec as immutable by adding the kubebuilder validation tag (e.g.,
+kubebuilder:validation:Immutable) and update the field comment to state it
cannot be changed post-creation (since changing VMSS identities is disruptive),
or if mutability is intentional, add a clear comment on ImageRegistryCredentials
explaining why it must remain mutable and document any operational
steps/impacts; update any validation logic or docs accordingly and reference the
AzurePlatformSpec, ImageRegistryCredentials, and analogous immutable fields
(location, resourceGroup, vnetID, subnetID) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f0a439d2-15ae-489a-aa26-a3dbf24251a3
⛔ Files ignored due to path filters (27)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (1)
api/hypershift/v1beta1/azure.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8472 +/- ##
==========================================
+ Coverage 40.00% 43.75% +3.75%
==========================================
Files 751 279 -472
Lines 92863 45961 -46902
==========================================
- Hits 37147 20109 -17038
+ Misses 53024 24314 -28710
+ Partials 2692 1538 -1154
... and 472 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
ba0c25b to
b80128b
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hypershift-operator/controllers/nodepool/azure.go (1)
248-257:⚠️ Potential issue | 🔴 Critical | ⚖️ Poor tradeoffRemove the
azure://prefix from ProviderID on line 254.CAPZ v1.21.0 expects the raw ARM resource ID format starting with
/subscriptions/without theazure://prefix. Adding the prefix causes parsing failures with error: "invalid resource ID: resource id 'azure:///subscriptions/...' must start with '/'".Diff
- ProviderID: capzutil.ProviderIDPrefix + resourceID, + ProviderID: resourceID,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hypershift-operator/controllers/nodepool/azure.go` around lines 248 - 257, The ProviderID is being constructed with an "azure://" prefix which CAPZ v1.21.0 rejects; in the hostedCluster block that sets azureMachineTemplate.Template.Spec.UserAssignedIdentities, strip any leading "azure://" (or "azure:///" variants) from hostedCluster.Spec.Platform.Azure.ImageRegistryCredentials.ManagedIdentity.ResourceID so resourceID starts with "/subscriptions/..." before concatenating with capzutil.ProviderIDPrefix and assigning to ProviderID; update the code that computes resourceID in that block (referencing hostedCluster, azureMachineTemplate.Template.Spec.UserAssignedIdentities and ProviderID) to perform this sanitization.
🧹 Nitpick comments (1)
api/hypershift/v1beta1/azure_test.go (1)
20-102: LGTM! Good N-1/N+1 compatibility coverage.The test correctly verifies:
- JSON serialization includes the field when set
- JSON omits the field when zero-valued (via
omitzero)- Legacy code (N-1) can unmarshal JSON from current version (N)
- Current version (N) preserves zero value when unmarshaling from N-1
The test follows table-driven patterns and "When ... it should ..." naming conventions as per coding guidelines.
♻️ Optional: Consider using gomega for more expressive assertions
import ( "encoding/json" "strings" "testing" + . "github.com/onsi/gomega" ) func TestAzurePlatformSpecSerializationCompatibility(t *testing.T) { + g := NewWithT(t) // ... if tt.expectFieldInJSON { - if !strings.Contains(jsonStr, `"imageRegistryCredentials"`) { - t.Errorf("expected imageRegistryCredentials in JSON, got: %s", jsonStr) - } + g.Expect(jsonStr).To(ContainSubstring(`"imageRegistryCredentials"`))This is purely optional; the current assertions work correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/hypershift/v1beta1/azure_test.go` around lines 20 - 102, The reviewer gave LGTM and suggested an optional refactor to use gomega for more expressive assertions; to apply this, update TestAzurePlatformSpecSerializationCompatibility to use gomega's NewWithT(t) and Expect(...) assertions instead of t.Fatalf/t.Errorf and manual if checks for JSON contents and field values (references: TestAzurePlatformSpecSerializationCompatibility, AzurePlatformSpec, azurePlatformSpecNMinus1, ImageRegistryCredentials, ManagedIdentity.ResourceID); add the gomega import and convert the key assertions (marshal/unmarshal errors, presence/absence of `"imageRegistryCredentials"`, expected ResourceID, Location and zero-value checks) to Expect-style calls while preserving the same test cases and logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@hypershift-operator/controllers/nodepool/azure.go`:
- Around line 248-257: The ProviderID is being constructed with an "azure://"
prefix which CAPZ v1.21.0 rejects; in the hostedCluster block that sets
azureMachineTemplate.Template.Spec.UserAssignedIdentities, strip any leading
"azure://" (or "azure:///" variants) from
hostedCluster.Spec.Platform.Azure.ImageRegistryCredentials.ManagedIdentity.ResourceID
so resourceID starts with "/subscriptions/..." before concatenating with
capzutil.ProviderIDPrefix and assigning to ProviderID; update the code that
computes resourceID in that block (referencing hostedCluster,
azureMachineTemplate.Template.Spec.UserAssignedIdentities and ProviderID) to
perform this sanitization.
---
Nitpick comments:
In `@api/hypershift/v1beta1/azure_test.go`:
- Around line 20-102: The reviewer gave LGTM and suggested an optional refactor
to use gomega for more expressive assertions; to apply this, update
TestAzurePlatformSpecSerializationCompatibility to use gomega's NewWithT(t) and
Expect(...) assertions instead of t.Fatalf/t.Errorf and manual if checks for
JSON contents and field values (references:
TestAzurePlatformSpecSerializationCompatibility, AzurePlatformSpec,
azurePlatformSpecNMinus1, ImageRegistryCredentials, ManagedIdentity.ResourceID);
add the gomega import and convert the key assertions (marshal/unmarshal errors,
presence/absence of `"imageRegistryCredentials"`, expected ResourceID, Location
and zero-value checks) to Expect-style calls while preserving the same test
cases and logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 457f2777-b2e1-49ac-af2d-51f62f556244
⛔ Files ignored due to path filters (43)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azureimageregistrycredentials.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/azureplatformspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/userassignedmanagedidentity.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/testdata/zz_fixture_TestConfigWithAcrMI.yamlis excluded by!**/testdata/**docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (6)
api/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/azure_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/azure_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config_test.go
- hypershift-operator/controllers/nodepool/azure_test.go
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
2 times, most recently
from
2889a65 to
5a90422
Compare
bennerv
reviewed
May 12, 2026
bennerv
reviewed
May 12, 2026
bennerv
reviewed
May 12, 2026
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
5a90422 to
76d2a13
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/hypershift/v1beta1/azure.go`:
- Around line 558-567: Update the AzureManagedIdentityResourceID type
validation: change the kubebuilder:validation:MinLength from 129 to 132 and
replace the permissive component regex with component-level validation similar
to encryptionSetID—require a UUID pattern for the subscriptionID, constrain
resourceGroupName to allowed characters and length (1–90), and enforce
identityName minimum length 3 and allowed characters; keep the overall
XValidation rule to match the complete ARM path but ensure its subpatterns
validate each component (subscription, resourceGroupName,
userAssignedIdentities/{identityName}) to mirror Azure's documented constraints.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 29e87b7e-4823-400b-a997-52ab9f368df6
⛔ Files ignored due to path filters (43)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azurecontainerregistrycredentials.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/azureplatformspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/userassignedmanagedidentity.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/testdata/zz_fixture_TestConfigWithAcrMI.yamlis excluded by!**/testdata/**docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (6)
api/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/azure_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/azure_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.go
- hypershift-operator/controllers/nodepool/azure.go
- api/hypershift/v1beta1/azure_test.go
- hypershift-operator/controllers/nodepool/azure_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config_test.go
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
44affdc to
80070fa
Compare
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
80070fa to
1286f9c
Compare
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
1286f9c to
8d18f2e
Compare
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
8d18f2e to
3bfca05
Compare
JoelSpeed
reviewed
May 14, 2026
| // HostedCluster, but it must be in the same Azure AD tenant. The management cluster's | ||
| // CAPZ identity must have Microsoft.ManagedIdentity/userAssignedIdentities/*/assign/action | ||
| // on the identity's scope to attach it to worker VM scale sets at creation time. | ||
| type AzureContainerRegistryCredentials struct { |
Contributor
There was a problem hiding this comment.
This appears to have taken a step backwards since the last review? Why did we lose the intermediate ManagedIdentity struct? Why did we lose the type alias with the validations assigned?
Also, on the thread where I mentioned splitting the subscriptionID, resourceGroupName and identityName into three separate fields, there was no actual response to say why we shouldn't do that
It seemed like @bryan-cox was in favour of that?
The current field has a hard UX for the end user, when most of the string is static, lets build that for them
Member
There was a problem hiding this comment.
Yeah, that's how we did it in other places for ARO HCP. However, I am amenable to keeping it like this if that is what the ARO HCP team wants.
This part of the API is not external as far as I know, and the only consumers are the ARO HCP folks themselves—unless that is incorrect, @bennerv?
Author
There was a problem hiding this comment.
This appears to have taken a step backwards since the last review? Why did we lose the intermediate ManagedIdentity struct? Why did we lose the type alias with the validations assigned?
Also, on the thread where I mentioned splitting the subscriptionID, resourceGroupName and identityName into three separate fields, there was no actual response to say why we shouldn't do that
I had attempted to accommodate by iterating on the PR locally and ended up walking back those changes. The fields got eaten during that. I'll be restoring them.
Author
There was a problem hiding this comment.
there was no actual response to say why we shouldn't do that
Apologies, I conflated the responses.
I mentioned splitting the subscriptionID, resourceGroupName and identityName
There are several places where the ARM API takes a resource ID directly - VNets, NSGs, others.
In this case it makes sense because it gets passed down to CAPZ that uses it directly. Decomposing it and then recomposing makes sense when they are independently meaningful to the API, but here I think they make more sense attached to the context of the full string being used as-is down further.
I'd argue that here it makes it harder for the customer because they have access to the full string directly with the proper sub and rg for the managed ID, and they would instead need to copy/paste the components out of the string or use code to present the fields properly. I don't know how most customers do this now though - copy/paste?
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
3bfca05 to
f14d7bb
Compare
openshift-ci
Bot
added
the
needs-rebase
Add types for configuring ACR image pull authentication via user-assigned managed identity: - AzureManagedIdentityResourceID: typed string with ARM resource ID CEL validation (MinLength=131, MaxLength=345) - UserAssignedManagedIdentity: wraps ResourceID for extensibility - AzureContainerRegistryCredentials: wraps ManagedIdentity, optional on AzurePlatformSpec The identity is attached to worker VMSS by CAPZ and its resource ID is written into worker cloud.conf for kubelet ACR credential provider authentication. The identity must be in the same Azure AD tenant but may reside in any subscription or resource group. Ref: ARO-24037 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
f14d7bb to
ecca3f6
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: twolff-gh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
ecca3f6 to
5193431
Compare
Contributor
|
@twolff-gh: The following tests failed, say
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. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When AzureContainerRegistryCredentials is set on the HostedCluster, append the managed identity to AzureMachineTemplate.UserAssignedIdentities so CAPZ attaches it to worker VMs at creation time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ker cloud.conf When AzureContainerRegistryCredentials.ManagedIdentity.ResourceID is set, CPO writes userAssignedIdentityID into the worker cloud.conf ConfigMap so kubelet's ACR credential provider can authenticate to ACR via IMDS. The value is intentionally omitted from the control plane Secret path (adaptConfigSecret) to avoid changing control plane component auth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add YAML-driven envtest cases validating CEL rules on AzureContainerRegistryCredentials.ManagedIdentity.ResourceID: - Valid ARM resource ID (pass) - Lowercase ARM segments (pass) - Invalid format, wrong provider, trailing slash (fail) - Removal after creation (pass, mutable) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
twolff-gh
force-pushed
the
ARO-24037-acr-pull-umi-cloud-conf
branch
from
5193431 to
9c73ad6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Adds an optional ImageRegistryCredentials field to AzurePlatformSpec that configures worker nodes to pull images from Azure Container Registry using a user-assigned managed identity.
When set, CPO writes userAssignedIdentityID into the worker cloud.conf and the NodePool controller attaches the MI to the VMSS via CAPZ.
This gives kubelet's ACR credential provider the identity it needs to authenticate without image pull secrets.
Which issue(s) this PR fixes:
Fixes ARO-24037
Special notes for your reviewer:
cloud-provider-azure accepts both ARM resource ID and client ID formats in userAssignedIdentityID — see auth_func.go#L80
We use the ARM resource ID since CAPZ also needs it for VMSS identity attachment.
Only the worker cloud.conf (ConfigMap) is modified — the CP cloud.conf (Secret) is unchanged.
Checklist:
Summary by CodeRabbit
New Features
Behavior Changes
Tests