fix: match ACR regional endpoints and DNL-enabled login servers in credential provider - #10731
fix: match ACR regional endpoints and DNL-enabled login servers in credential provider#10731pujitha24 wants to merge 1 commit into
Conversation
…edential provider /kind bug #### What this PR does / why we need it: The acr-credential-provider's `acrRE` regex, used to detect whether an image reference targets Azure Container Registry, only matched a single alphanumeric label before the `azurecr.*` domain (e.g. `foo.azurecr.io`). It rejected two valid ACR login server formats: - Registries with Domain Name Label (DNL) enabled, whose login server includes a dash-separated hash, e.g. `myregistry-abc123.azurecr.io`. - ACR regional/replica endpoints, which add one extra DNS label, e.g. `myregistry.eastus.azurecr.io`. When `parseACRLoginServerFromImage` fails to recognize such a hostname, `GetCredentials` returns an empty `Auth` map for that image (see azure_credentials.go), so kubelet attempts an anonymous pull. For private images hosted on a DNL-enabled or regional-endpoint registry, that pull then fails with an authentication error. Public images and registries using the existing plain `registry.azurecr.io` format are unaffected. #### Approach Update `acrRE` to allow dashes within a DNS label and one optional additional dot-separated label before the `azurecr.*` suffix, while keeping the domain suffix itself a literal, anchored match. The existing anti-spoofing behavior in `parseACRLoginServerFromImage` (requiring whatever follows the matched registry to be empty or start with `/`) is untouched, so hostnames like `foo.azurecr.io.evil.com` are still correctly rejected. #### Which issue(s) this PR fixes: #### Special notes for your reviewer: Added test cases for a DNL-style login server (`foo-abc123.azurecr.io`) and a regional endpoint (`foo.eastus.azurecr.io`), and updated the `foo-azurecr-io.azurecr.cn` case, which previously documented the bug (expected `""`) and now correctly matches. Verified the existing `foo.azurecr.io.azurecr.cn` anti-spoofing test case still returns `""` unchanged. #### Does this PR introduce a user-facing change? ```release-note Fixed the acr-credential-provider incorrectly treating images hosted on Azure Container Registry regional endpoints or registries with Domain Name Label (DNL) enabled as non-ACR images, which caused private image pulls from those registries to fail with an authentication error. ``` #### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: ```docs ``` Validation: `go build ./pkg/credentialprovider/...` and `go test ./pkg/credentialprovider/...` both pass (all existing and new subtests of TestParseACRLoginServerFromImage pass). Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
Welcome @pujitha24! |
|
Hi @pujitha24. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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: pujitha24 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 |
There was a problem hiding this comment.
Pull request overview
Fixes ACR hostname detection in the acr-credential-provider so kubelet can retrieve credentials for Azure Container Registry images hosted on regional/replica endpoints and registries with Domain Name Label (DNL) enabled.
Changes:
- Broadened the ACR login-server regex to support dashes in registry labels and one optional extra DNS label for regional endpoints.
- Updated/added unit tests to cover DNL-style and regional-endpoint hostnames and to reflect the corrected matching behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/credentialprovider/azure_credentials.go | Updates acrRE to match DNL-enabled registry names and regional endpoint formats while preserving existing anti-spoofing behavior via the post-regex suffix check. |
| pkg/credentialprovider/azure_credentials_test.go | Expands TestParseACRLoginServerFromImage with new cases and updates an existing case to validate the broader regex matching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/ok-to-test |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
The two failing jobs here ( This PR only touches Happy to rebase or investigate further if it would help. |
c0a94e6 to
7c80930
Compare
|
@pujitha24: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
/kind bug
What this PR does / why we need it:
The acr-credential-provider's
acrREregex, used to detect whether animage reference targets Azure Container Registry, only matched a single
alphanumeric label before the
azurecr.*domain (e.g.foo.azurecr.io).It rejected two valid ACR login server formats:
includes a dash-separated hash, e.g.
myregistry-abc123.azurecr.io.myregistry.eastus.azurecr.io.When
parseACRLoginServerFromImagefails to recognize such a hostname,GetCredentialsreturns an emptyAuthmap for that image (seeazure_credentials.go), so kubelet attempts an anonymous pull. For
private images hosted on a DNL-enabled or regional-endpoint registry,
that pull then fails with an authentication error. Public images and
registries using the existing plain
registry.azurecr.ioformat areunaffected.
Approach
Update
acrREto allow dashes within a DNS label and one optionaladditional dot-separated label before the
azurecr.*suffix, whilekeeping the domain suffix itself a literal, anchored match. The
existing anti-spoofing behavior in
parseACRLoginServerFromImage(requiring whatever follows the matched registry to be empty or start
with
/) is untouched, so hostnames likefoo.azurecr.io.evil.comarestill correctly rejected.
Which issue(s) this PR fixes:
Fixes #9975
Special notes for your reviewer:
Added test cases for a DNL-style login server (
foo-abc123.azurecr.io)and a regional endpoint (
foo.eastus.azurecr.io), and updated thefoo-azurecr-io.azurecr.cncase, which previously documented the bug(expected
"") and now correctly matches. Verified the existingfoo.azurecr.io.azurecr.cnanti-spoofing test case still returns""unchanged.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Validation:
go build ./pkg/credentialprovider/...andgo test ./pkg/credentialprovider/...both pass (all existing and newsubtests of TestParseACRLoginServerFromImage pass).
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com