feat(azure): enable ACR image pulls via managed identity on worker nodes#8205
feat(azure): enable ACR image pulls via managed identity on worker nodes#8205twolff-gh wants to merge 5 commits intoopenshift:mainfrom
Conversation
|
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. |
|
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:
📝 WalkthroughWalkthroughAdds an optional ImageRegistryCredentials field to AzureNodePoolPlatform and a new AzureImageRegistryCredentials type with validation (ManagedIdentity, Registries). The operator sets VMIdentityUserAssigned and UserAssignedIdentities on Azure VM specs when credentials are present. It generates a worker MachineConfig (Ignition v3.2.0) embedding a kubelet credential-provider YAML and an acr-azure.json payload derived from Azure identifiers. Tests cover API JSON compatibility, credential-provider and acr JSON generation, Ignition contents, and MachineConfig creation. Kubeapilinter exclusions were added for the new field and test-only structs. Sequence Diagram(s)sequenceDiagram
actor Admin as Admin/User
participant NP as NodePool Spec
participant Op as Operator
participant AzureSpec as Azure VM Template
participant MCO as MachineConfig Generator
participant Ign as Ignition Config
participant Node as Worker Node
Admin->>NP: Create/Update NodePool with ImageRegistryCredentials
Op->>NP: Reconcile NodePool
Op->>AzureSpec: Build azureMachineTemplateSpec()
alt ImageRegistryCredentials set
AzureSpec->>AzureSpec: Set Identity = VMIdentityUserAssigned
AzureSpec->>AzureSpec: Add UserAssignedIdentities (ManagedIdentity)
end
Op->>MCO: generateMCORawConfig()
MCO->>MCO: getACRCredentialProviderConfig()
alt Azure + ImageRegistryCredentials
MCO->>MCO: generateACRCredentialProviderMachineConfig()
MCO->>Ign: Create Ignition v3.2.0 with:
Ign->>Ign: /etc/kubernetes/credential-providers/acr-credential-provider.yaml
Ign->>Ign: /etc/kubernetes/acr-azure.json
MCO->>Op: Return MachineConfig (wrapped in ConfigMap)
end
Op->>Node: Apply MachineConfig / Ignition
Node->>Node: Kubelet uses credential provider (ManagedIdentity)
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 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
do-not-merge/needs-area
needs-rebase
|
[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 |
openshift-ci
bot
added
area/api
| short-lived tokens for ACR image pulls instead of relying on static pull secrets. | ||
| Changing this field will trigger a node rollout. | ||
| properties: | ||
| managedIdentity: |
There was a problem hiding this comment.
for standalone or other cases, do we want to support system-assigned identity? If so, we wouldn't have a customer pass these in at creation time.
There was a problem hiding this comment.
For the scope of this feature, I believe it was only for user-assigned identities. I'll confirm.
If we needed to support system-assigned, I wonder if it would need to be populated differently / new field?
There was a problem hiding this comment.
Yes, user-assigned was the main ask, but system-assigned would likely follow in the future.
Do they make sense as separate work - user now and system as another PR after?
There was a problem hiding this comment.
if system-assigned identities are added per-VM, then this doesn't make sense from a feature perspective. Maybe try and see what happens, then ignore it or implement it as you see fit.
There was a problem hiding this comment.
From what I see, a cluster needs to be created before getting a principal id which some process would need permissions to access customer resources.
Does the customer trust the platform to write IAM policies on their container registry? (Don't know)
Seems like a followup item. I'll gladly look into
…/R3) Address bennerv review comments R2 and R3 on PR openshift#8205: make the registries field optional (remove +required and MinItems=1), allow empty registries to fall through to default wildcard patterns (*.azurecr.io, etc.), and update tests accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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 the current code and only fix it if needed.
Inline comments:
In `@hypershift-operator/controllers/nodepool/config.go`:
- Around line 242-248: The code dereferences
cg.hostedCluster.Spec.Platform.Azure directly which can panic; before calling
generateACRCredentialProviderMachineConfig, add a guard that
cg.hostedCluster.Spec.Platform != nil and cg.hostedCluster.Spec.Platform.Azure
!= nil, and handle the nil case (return an appropriate error or skip registry
credential config) so you don't call generateACRCredentialProviderMachineConfig
with a nil azureSpec; keep references to
cg.nodePool.Spec.Platform.Azure.ImageRegistryCredentials and the
generateACRCredentialProviderMachineConfig call when adding the guard and
subsequent handling.
🪄 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: Pro Plus
Run ID: 51b240dc-cd08-46f4-8ba4-b589a0fd067b
⛔ Files ignored due to path filters (14)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azureimageregistrycredentials.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/azurenodepoolplatform.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/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 (8)
api/.golangci.ymlapi/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/azure_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/azure_acr.gohypershift-operator/controllers/nodepool/azure_acr_test.gohypershift-operator/controllers/nodepool/azure_test.gohypershift-operator/controllers/nodepool/config.go
twolff-gh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
…/R3) Address bennerv review comments R2 and R3 on PR openshift#8205: make the registries field optional (remove +required and MinItems=1), allow empty registries to fall through to default wildcard patterns (*.azurecr.io, etc.), and update tests accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
twolff-gh
force-pushed
the
azure-acr-mi-api
branch
from
e11b55c to
18f22d6
Compare
openshift-ci
bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/hypershift/v1beta1/azure.go (1)
139-153: Consider validatingregistriesentries against the documented formats.The docs say values should be ACR hostnames or wildcard patterns, but current validation accepts any non-empty string.
Suggested schema refinement
// +kubebuilder:validation:items:MinLength=1 // +kubebuilder:validation:items:MaxLength=256 + // +kubebuilder:validation:items:Pattern=`^((\*\.)?[a-z0-9]([a-z0-9-]{0,251}[a-z0-9])?\.(azurecr\.io|azurecr\.cn|azurecr\.de|azurecr\.us))$` // +listType=set Registries []string `json:"registries,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/azure.go` around lines 139 - 153, The Registries slice accepts any non-empty string but should only allow ACR hostnames or wildcard patterns; update the Registries field validation by adding an items-level kubebuilder pattern (e.g. +kubebuilder:validation:items:Pattern="^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)+$") so each entry matches either a hostname or a leading-*. wildcard hostname (apply the pattern to the Registries field declaration and adjust the comment to reflect the stricter schema), ensuring MinLength/MaxLength and MinItems/MaxItems remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/hypershift/v1beta1/azure.go`:
- Around line 134-137: The current XValidation on the ManagedIdentity field only
enforces path shape and allows non-UUID subscription IDs; update the XValidation
rule for ManagedIdentity to validate the subscription segment is a GUID and
tighten other segments (resource group and identity name) with appropriate regex
constraints so malformed IDs are rejected at admission time; locate the
ManagedIdentity string tag in azure.go and replace the existing rule with a
stricter regex that enforces subscription GUID format (e.g. 8-4-4-4-12 hex
bytes) and valid resourceGroup/identity name characters and lengths.
---
Nitpick comments:
In `@api/hypershift/v1beta1/azure.go`:
- Around line 139-153: The Registries slice accepts any non-empty string but
should only allow ACR hostnames or wildcard patterns; update the Registries
field validation by adding an items-level kubebuilder pattern (e.g.
+kubebuilder:validation:items:Pattern="^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)+$")
so each entry matches either a hostname or a leading-*. wildcard hostname (apply
the pattern to the Registries field declaration and adjust the comment to
reflect the stricter schema), ensuring MinLength/MaxLength and MinItems/MaxItems
remain unchanged.
🪄 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: Pro Plus
Run ID: 3b163967-081b-4fb7-bdb9-0fee70cf5f36
⛔ Files ignored due to path filters (14)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azureimageregistrycredentials.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/azurenodepoolplatform.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/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 (8)
api/.golangci.ymlapi/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/azure_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/azure_acr.gohypershift-operator/controllers/nodepool/azure_acr_test.gohypershift-operator/controllers/nodepool/azure_test.gohypershift-operator/controllers/nodepool/config.go
✅ Files skipped from review due to trivial changes (3)
- hypershift-operator/controllers/nodepool/azure.go
- api/.golangci.yml
- hypershift-operator/controllers/nodepool/azure_acr.go
🚧 Files skipped from review as they are similar to previous changes (4)
- hypershift-operator/controllers/nodepool/azure_test.go
- hypershift-operator/controllers/nodepool/config.go
- api/hypershift/v1beta1/azure_test.go
- hypershift-operator/controllers/nodepool/azure_acr_test.go
twolff-gh
force-pushed
the
azure-acr-mi-api
branch
from
6ef165b to
1419528
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hypershift-operator/controllers/nodepool/azure_acr_test.go (1)
503-534:⚠️ Potential issue | 🟡 MinorAlign test name with actual assertions in the “default wildcards only” case.
This case name claims wildcard-only verification, but the assertions only check generic MachineConfig markers. Either assert wildcard content explicitly or rename the case to match what it currently validates.
♻️ Minimal fix (rename to match current assertions)
- name: "When registries is empty it should return a ConfigMap with default wildcards only", + name: "When registries is empty it should return a ConfigMap with credential provider MachineConfig",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/nodepool/azure_acr_test.go` around lines 503 - 534, The test case name "When registries is empty it should return a ConfigMap with default wildcards only" is misleading given the assertions in the validate closure (they check for "50-acr-credential-provider" and "MachineConfig" in mcYAML). Rename the test case title string to reflect what is actually asserted (e.g., mention MachineConfig and 50-acr-credential-provider) OR modify the validate function (the block referencing mcYAML and TokenSecretConfigKey) to assert wildcard content explicitly; the minimal fix is to update the test case name string to match the validate function's expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hypershift-operator/controllers/nodepool/azure_acr_test.go`:
- Around line 503-534: The test case name "When registries is empty it should
return a ConfigMap with default wildcards only" is misleading given the
assertions in the validate closure (they check for "50-acr-credential-provider"
and "MachineConfig" in mcYAML). Rename the test case title string to reflect
what is actually asserted (e.g., mention MachineConfig and
50-acr-credential-provider) OR modify the validate function (the block
referencing mcYAML and TokenSecretConfigKey) to assert wildcard content
explicitly; the minimal fix is to update the test case name string to match the
validate function's expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b8b43b0-d345-448e-877c-8c1e0b186d55
⛔ Files ignored due to path filters (15)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azureimageregistrycredentials.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/azurenodepoolplatform.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/stable.nodepools.azure.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/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 (8)
api/.golangci.ymlapi/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/azure_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/azure_acr.gohypershift-operator/controllers/nodepool/azure_acr_test.gohypershift-operator/controllers/nodepool/azure_test.gohypershift-operator/controllers/nodepool/config.go
✅ Files skipped from review due to trivial changes (2)
- hypershift-operator/controllers/nodepool/azure.go
- hypershift-operator/controllers/nodepool/azure_acr.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/.golangci.yml
- api/hypershift/v1beta1/azure.go
| // acrDefaultMatchImages lists the default wildcard patterns for Azure Container Registry | ||
| // endpoints across all Azure clouds. These patterns are always appended to the user-specified | ||
| // registries to ensure the credential provider covers all standard ACR endpoints. | ||
| var acrDefaultMatchImages = []string{ |
There was a problem hiding this comment.
Can these not be defaulted in the api for the registries?
There was a problem hiding this comment.
From what I found, these come from MCO credential provider config, if changed upstream, API default could go stale. Does that sound accurate?
Can or should we change this at the MCO config?
| // multiple registries. When specified, these registries are added to the credential | ||
| // provider's matchImages list alongside the default wildcard patterns (*.azurecr.io, | ||
| // *.azurecr.cn, *.azurecr.de, *.azurecr.us). When omitted, only the default wildcard | ||
| // patterns are used, which cover all standard Azure Container Registry endpoints. |
There was a problem hiding this comment.
Can we call out a customer only gets 16 of the 20 spots since 4 are used for the wildcards?
There was a problem hiding this comment.
It sounds like this is additive. If the hard limit is 20 including the defaults, you should limit this to only what the user can actually provide.
There was a problem hiding this comment.
MaxItems=16 added. user gets 16 slots, 4 defaults added by controller.
There was a problem hiding this comment.
Removed the registries field. It was in the original WIP, but doesnt make sense in this final version.
Adds more complexity than necessary
The default patterns copied in plus the RBAC for ids should be enough
|
|
||
| azureMachineTemplate.Template.Spec.SSHPublicKey = dummySSHKey | ||
|
|
||
| if nodePool.Spec.Platform.Azure.ImageRegistryCredentials != nil { |
There was a problem hiding this comment.
Add comment documenting that identity assignment currently replaces (not appends), and should be refactored to append if additional identities are needed in the future.
| // When configured, worker nodes will use the acr-credential-provider plugin to obtain | ||
| // short-lived tokens for ACR image pulls instead of relying on static pull secrets. | ||
| // Changing this field will trigger a node rollout. | ||
| // +optional |
There was a problem hiding this comment.
What happens if I don't configure this field?
There was a problem hiding this comment.
Added: When not configured, no additional image credential provider is set up and worker nodes use the default pull secret for image authentication.
| // multiple registries. When specified, these registries are added to the credential | ||
| // provider's matchImages list alongside the default wildcard patterns (*.azurecr.io, | ||
| // *.azurecr.cn, *.azurecr.de, *.azurecr.us). When omitted, only the default wildcard | ||
| // patterns are used, which cover all standard Azure Container Registry endpoints. |
There was a problem hiding this comment.
It sounds like this is additive. If the hard limit is 20 including the defaults, you should limit this to only what the user can actually provide.
| // multiple registries. When specified, these registries are added to the credential | ||
| // provider's matchImages list alongside the default wildcard patterns (*.azurecr.io, | ||
| // *.azurecr.cn, *.azurecr.de, *.azurecr.us). When omitted, only the default wildcard |
There was a problem hiding this comment.
Am I allowed to specify an entry that matches one of these default patterns?
There was a problem hiding this comment.
Controller duplicates silently. Duplicate is dropped.
Does this work?
There was a problem hiding this comment.
De-duplication is fine - should we prevent users from specifying the wildcard patterns that are applied by default though? It doesn't look like there would be any value to the users to be able to specify those aside from unnecessarily consuming some of their 16 slots.
Do we anticipate these defaults potentially changing much in the future (i.e removing one of them)? If so, it could make sense to allow end-users to explicitly provide them to be more resilient to changes in the defaults.
There was a problem hiding this comment.
Removed the registries field. It was in the original WIP, but doesnt make sense in this final version.
Adds more complexity than necessary
The default patterns copied in plus the RBAC for ids should be enough
twolff-gh
force-pushed
the
azure-acr-mi-api
branch
from
1419528 to
aef1ea8
Compare
twolff-gh
force-pushed
the
azure-acr-mi-api
branch
from
aef1ea8 to
2caafeb
Compare
twolff-gh
force-pushed
the
azure-acr-mi-api
branch
from
2caafeb to
2b43b77
Compare
|
/retest |
| // +kubebuilder:validation:MaxItems=16 | ||
| // +kubebuilder:validation:items:MinLength=1 | ||
| // +kubebuilder:validation:items:MaxLength=253 | ||
| // +kubebuilder:validation:items:Pattern=`^(\*\.)?[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9])?)+$` |
There was a problem hiding this comment.
Instead of using the Pattern marker, we generally recommend using the XValidation marker to enforce the pattern via a CEL expression and supplying a human-readable error message on validation failure.
Using the pattern marker means that the end user is provided the literal regex as part of the validation failure instead of a user-friendly message that describes the constraints.
| // Format: /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/{identityName} | ||
| // | ||
| // +required | ||
| ManagedIdentity AzureManagedIdentityResourceID `json:"managedIdentity,omitempty"` |
There was a problem hiding this comment.
Include the length constraints for this field (enforced by the type) in the GoDoc here as well please
|
/uncc |
twolff-gh
force-pushed
the
azure-acr-mi-api
branch
from
2b43b77 to
2af9a37
Compare
|
Now I have all the evidence needed for the complete analysis. Here is the report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis failure is a pre-existing flaky test bug in the envtest framework, not caused by PR #8205's changes. The failing test ( Root CauseThe root cause is a CRD lifecycle race condition in Execution sequence in
The race:
Why The Eventually(func() error {
return k8sClient.Get(ctx, client.ObjectKeyFromObject(crd), crd)
}, "30s").ShouldNot(Succeed())
Why it's flaky (not deterministic): The failure depends on how fast the API server finalizes CRD deletion, which varies by Kubernetes version and CI node load. Under light load, the CRD may be fully deleted before the next test starts. Under heavier load (or with slower K8s versions), the CRD lingers in Recommendations
Evidence
|
…Identity Add UserAssignedManagedIdentity struct with resourceID (required, ARM format for CAPZ VMSS attachment) and clientID (optional, for credential provider config). Azure IMDS docs describe userAssignedIdentityID as a client ID; cloud-provider-azure auth_func.go accepts both formats. As a first-party Azure service, we support both. Add AzureImageRegistryCredentials on AzureNodePoolPlatform using the new struct. No Registries field — controller uses hardcoded wildcard patterns covering all standard ACR endpoints. 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>
…ntroller When imageRegistryCredentials.managedIdentity is set on a NodePool: - CAPI: AzureMachineTemplate gets Identity=UserAssigned with the MI resource ID on the worker VMSS - MachineConfig: 50-acr-credential-provider generates kubelet CredentialProviderConfig with wildcard ACR patterns plus azure.json with IMDS-based managed identity authentication - If clientID is provided, it is used in azure.json instead of resourceID (cloud-provider-azure accepts both formats) Signed-off-by: Todd Wolff <twolff@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Envtest YAML suite with 3 cases: no imageRegistryCredentials (pass), valid managedIdentity with resourceID (pass), invalid resourceID format (fail with ARM resource ID error message). 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>
twolff-gh
force-pushed
the
azure-acr-mi-api
branch
from
2af9a37 to
de3bfd7
Compare
|
@twolff-gh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Followup to #7793
Add support for configuring kubelet's image credential provider to
authenticate to Azure Container Registry (ACR) using a user-assigned
managed identity. This eliminates the need for static pull secrets
by leveraging IMDS-based token acquisition on worker nodes.
Summary by CodeRabbit
New Features
Bug Fixes
Tests