OCPBUGS-86616,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
OCPBUGS-86616,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)
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 41.54% 41.55% +0.01%
==========================================
Files 758 758
Lines 93838 93854 +16
==========================================
+ Hits 38986 39005 +19
+ Misses 52107 52100 -7
- Partials 2745 2749 +4
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
|
/lgtm |
Contributor
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
JoelSpeed
reviewed
Jun 1, 2026
|
|
||
| // UserAssignedManagedIdentity identifies a user-assigned managed identity by its ARM resource ID. | ||
| type UserAssignedManagedIdentity struct { | ||
| // resourceID is the ARM resource ID of the user-assigned managed identity. |
Contributor
There was a problem hiding this comment.
Needs to be explained in this godoc so that it ends up in the generated docs the user would see with kubectl explain.
Please expand this comment on the field to explain the valid input
Author
|
/retest-required |
1 similar comment
Author
|
/retest-required |
Member
|
/retest |
Member
|
@twolff-gh since you already lost lgtm, would you mind rebasing the branch please? 😄 That should fix the codecov/project error you are seeing. |
Author
Will do! |
JoelSpeed
reviewed
Jun 5, 2026
JoelSpeed
left a comment
JoelSpeed
left a comment
Contributor
There was a problem hiding this comment.
/approve for API
Thanks @twolff-gh
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, JoelSpeed, twolff-gh 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 |
enxebre
reviewed
Jun 9, 2026
enxebre
reviewed
Jun 9, 2026
Member
|
how does this relate to UseManagedIdentityExtension input in the config file? should if influence it? |
Member
|
dropped some questions, lgtm overall |
Author
Theyre related but I dont think we need to change UseManagedIdentityExtension here. Its already set true on the ARO HCP worker cloud.conf. userAssignedIdentityID is read by the kubelet credential provider for ACR token acquisition. It needs UseManagedIdentityExtension=true as a prereq, which is already set. What do you think? Hopefully this helped 🙏 |
Contributor
|
@twolff-gh: The following test 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. |
machi1990
reviewed
Jun 10, 2026
Add AzureContainerRegistryConfig to AzurePlatformSpec with support for managed-identity-based ACR authentication on worker nodes. New types: - AzureContainerRegistryConfig: top-level container registry config - AzureContainerRegistryCredentialConfig: credential type selection - AzureContainerRegistryCredentialType: enum (ManagedIdentity) - UserAssignedManagedIdentity: UMI resource ID reference - AzureManagedIdentityResourceID: validated Azure resource ID type Includes CEL validation rules for credential type enum and Azure resource ID format, plus mutual exclusivity constraints. Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
When containerRegistry.credentials.managedIdentity is set on the HostedCluster, attach the specified user-assigned managed identity to worker VMSS via CAPZ's AzureMachineTemplate identity list. This enables kubelet's ACR credential provider to authenticate via IMDS on the worker node. Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…ker cloud.conf When containerRegistry.credentials.managedIdentity is configured, write the userAssignedIdentityID field into the worker cloud.conf ConfigMap so kubelet's ACR credential provider can locate the correct identity via IMDS. The CP cloud.conf Secret (used by CCM/CNO with workload identity federation) intentionally does NOT include this field. Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Add YAML-driven test cases for containerRegistry field validation: - Valid ManagedIdentity credential type with proper resource ID - Invalid credential type rejection - Malformed Azure resource ID rejection - Missing required fields when type is set Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
This confirms the root cause. The failing check run ( Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe failure is caused by a stale Codecov check run attached to an older commit ( Here is the sequence of events:
The -0.04% coverage drop on the old commit was caused by 9 files with "indirect coverage changes" — code that existed before the PR but whose coverage numbers shifted due to test suite changes between the base and that intermediate commit. The PR's own patch coverage is 83.33% (3 lines uncovered in The 3 uncovered lines in Recommendations
Evidence
|
12 tasks
|
/lgtm |
Contributor
|
Scheduling tests matching the |
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
7 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