ARO-24037 feat(azure): add ACR pull identity to worker cloud.conf and VMSS#8472
ARO-24037 feat(azure): add ACR pull identity to worker cloud.conf and VMSS#8472twolff-gh wants to merge 6 commits intoopenshift:mainfrom
Conversation
Add optional field for specifying a user-assigned managed identity ARM resource ID on AzurePlatformSpec. When set, the identity is attached to worker VMSS and written into the worker cloud provider config for ACR image pull authentication. Includes CEL validation for ARM resource ID format with case-insensitive matching, featuregated CRD manifests, and deepcopy generation. Signed-off-by: Todd Wolff <twolff@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Todd Wolff <twolff@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wire AcrImagePullManagedIdentityID from HostedCluster into the CAPZ AzureMachineTemplate. When set, the identity is added to the VMSS UserAssignedIdentities list so worker nodes can authenticate to ACR via IMDS. Signed-off-by: Todd Wolff <twolff@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cloud.conf Write the ACR managed identity resource ID into the worker cloud.conf via adaptConfig. The cloud-provider-azure auth library auto-detects the ARM resource ID format and resolves it for IMDS token acquisition. Only the worker ConfigMap is modified — the control plane Secret is unchanged, verified by test. Signed-off-by: Todd Wolff <twolff@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add envtest cases for AcrImagePullManagedIdentityID CEL validation: - Valid ARM resource ID format (mixed case) passes - Lowercase ARM resource ID passes (lowerAscii matching) - Invalid format rejected with descriptive error - Wrong provider namespace rejected Signed-off-by: Todd Wolff <twolff@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Todd Wolff <twolff@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[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 |
📝 WalkthroughWalkthroughThese changes add support for specifying an Azure user-assigned managed identity for pulling images from Azure Container Registry (ACR) on worker VMs. A new optional field 🚥 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}
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
| if hostedCluster != nil && hostedCluster.Spec.Platform.Azure != nil && hostedCluster.Spec.Platform.Azure.AcrImagePullManagedIdentityID != nil { | ||
| azureMachineTemplate.Template.Spec.Identity = capiazure.VMIdentityUserAssigned | ||
| azureMachineTemplate.Template.Spec.UserAssignedIdentities = append( | ||
| azureMachineTemplate.Template.Spec.UserAssignedIdentities, | ||
| capiazure.UserAssignedIdentity{ | ||
| ProviderID: capzutil.ProviderIDPrefix + *hostedCluster.Spec.Platform.Azure.AcrImagePullManagedIdentityID, | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For sigs.k8s.io/cluster-api-provider-azure v1.21.0, what exact format is required for AzureMachineSpec.UserAssignedIdentities[].ProviderID: raw ARM resource ID (/subscriptions/...) or provider ID with azure:/// prefix?
💡 Result:
For sigs.k8s.io/cluster-api-provider-azure v1.21.0, AzureMachineSpec.UserAssignedIdentities[].ProviderID must be the raw Azure ARM resource ID string in the form “/subscriptions/.../resourceGroups/.../providers/Microsoft.ManagedIdentity/userAssignedIdentities/...”, i.e., it should NOT include the “azure:///” scheme prefix.[1][2] Why: CAPZ’s identity parsing for user-assigned identities rejects values that start with “azure:///” as invalid resource IDs (it requires the ID to start with “/”), and CAPZ’s own documentation for identity ProviderID examples uses the raw ARM resource ID format.[1][2]
Citations:
- 1: userAssignedIdentities providerID value format unexpectedly changed in CAPZ v1.9.0 kubernetes-sigs/cluster-api-provider-azure#3597
- 2: https://capz.sigs.k8s.io/topics/vm-identity.html
Remove the capzutil.ProviderIDPrefix — CAPZ v1.21 expects raw ARM resource IDs.
Line 253 must use the ARM ID directly without the azure:/// prefix. CAPZ's identity parsing explicitly rejects provider IDs starting with the scheme prefix and requires the raw format /subscriptions/.../resourceGroups/.../providers/Microsoft.ManagedIdentity/userAssignedIdentities/....
Update to:
ProviderID: *hostedCluster.Spec.Platform.Azure.AcrImagePullManagedIdentityID,
🤖 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 - 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.
|
/retest |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis is not a test failure — no Prow CI test jobs ran at all. The tide merge controller reports the PR is unmergeable due to git merge conflicts. The PR branch is based on commit Root CauseThe PR branch was forked from
The PR adds a new Recommendations
Evidence
|
1 participant
What this PR does / why we need it:
Adds an optional acrImagePullManagedIdentityID 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.
This is the AKS-equivalent
--attach-acrpattern for ARO-HCP. The cloud-provider-azure auth library auto-detects ARM resource IDs vs client IDs in the userAssignedIdentityID field, so a single field serves both CAPZ attachment and cloud.conf configuration.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
Tests