Fix ACR MSI cross-subscription authentication error #77245

norshtein · 2019-04-30T03:44:20Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Method Provide() in interface DockerConfigProvider now takes image name as its parameter(added in a recent PR #75587). So we can parse the login server from the image name, and then interact with the login server to get docker login credential.
Which issue(s) this PR fixes:

Fixes #67892

Special notes for your reviewer:

Does this PR introduce a user-facing change?: no user-facing change

release note:

fix issue that pull image failed from a cross-subscription Azure Container Registry when using MSI to authenticate

/sig azure
/assign @andyzhangx @feiskyer

k8s-ci-robot · 2019-04-30T03:44:28Z

Hi @norshtein. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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/test-infra repository.

andyzhangx · 2019-04-30T04:06:28Z

/ok-to-test

andyzhangx

could you also check what if ACR is a public repo, does this PR also work? And what if that image is a docker hub image ...

andyzhangx · 2019-04-30T04:12:13Z

pkg/credentialprovider/azure/azure_credentials.go

-		klog.V(4).Infof("listing registries")
-		result, err := a.registryClient.List(ctx)
+		// Read login server from image
+		parts := strings.Split(image, "/")


would you use regexp.MustCompile to parse login server, here is an example:

kubernetes/pkg/volume/azure_dd/azure_common.go

Line 216 in 1608578

matches := lunPathRE.FindStringSubmatch(deviceInfo)

And also there should be error handling in case image is an invalid image url

I have switched to use regular expression. For a public ACR, below code will handle for it
https://github.com/kubernetes/kubernetes/pull/77245/files#diff-65860720abfa0057bc9fe725fde21bedR220 . For other image registries, they will be filtered out.

andyzhangx · 2019-04-30T04:13:07Z

pkg/credentialprovider/azure/azure_credentials.go


 	if a.config.UseManagedIdentityExtension {
-		klog.V(4).Infof("listing registries")
-		result, err := a.registryClient.List(ctx)
+		// Read login server from image


provide an example format for image address and also print out the original image url in the log

Added an example in comment

feiskyer · 2019-04-30T05:14:15Z

pkg/credentialprovider/azure/azure_credentials.go


 	if a.config.UseManagedIdentityExtension {
-		klog.V(4).Infof("listing registries")
-		result, err := a.registryClient.List(ctx)
+		// Read login server from image


should filter out non-ACR images here.

Code updated.

feiskyer · 2019-04-30T05:15:47Z

@norshtein Could you also add a unit test for this change?

/priority important-soon

norshtein · 2019-04-30T05:37:54Z

@norshtein Could you also add a unit test for this change?

/priority important-soon

If the credential type is MSI, it will interact with MSI endpoint to get a token and write the token to DockerConfigEntry: https://github.com/kubernetes/kubernetes/pull/77245/files#diff-65860720abfa0057bc9fe725fde21bedR255 . It is not a static token similar as service principal. Seems it's hard to add unit test for it...

andyzhangx · 2019-04-30T05:46:09Z

for the unit test, I think you could move following code into a seperate function, e.g. isACRImage:

		// Parse login server from parameter `image`. An example of parameter `image`: foo.azurecr.io/bar/imageName:version
		regEx, err := regexp.Compile(`.*\.azurecr\.io|.*\.azurecr\.cn|.*\.azurecr\.de|.*\.azurecr\.us`)
		if err != nil {
			klog.Error("Unexpected error: failed to construct regular expression: %v", err)
		}
		match := regEx.FindAllString(image, -1)
		if len(match) != 1 {
			klog.V(4).Infof("Found the image is not in acr, will not generate the token using MSI")
		}

Then you could add all kinds of unit tests, that would be testable

norshtein · 2019-04-30T05:47:50Z

Cool. Thanks for the reminding, will do.

andyzhangx · 2019-04-30T05:47:05Z

pkg/credentialprovider/azure/azure_credentials.go

 		if err != nil {
-			klog.Errorf("Failed to list registries: %v", err)
-			return cfg
+			klog.Error("Unexpected error: failed to construct regular expression: %v", err)


only log error here?

Please refer to latest commit.

It would be OK to only log error, the error is not fatal and we can't do anything about it. For the worst case, cfg is not correctly filled and pulling image will fail, which is acceptable.

andyzhangx · 2019-04-30T05:51:07Z

pkg/credentialprovider/azure/azure_credentials.go

-			klog.V(2).Infof("loginServer: %s", loginServer)
+		match := regEx.FindAllString(image, -1)
+		if len(match) != 1 {
+			klog.V(4).Infof("Found the image is not in acr, will not generate the token using MSI")


klog.V(4).Infof("image(%s) is not from ACR, skip MSI authentication", image)

andyzhangx · 2019-04-30T05:52:20Z

pkg/credentialprovider/azure/azure_credentials.go

 			cred, err := getACRDockerEntryFromARMToken(a, loginServer)
 			if err != nil {
-				continue
+				klog.Errorf("Failed to get docker entry: %v", err)


klog.Errorf("getACRDockerEntryFromARMToken(%s) failed with %v", loginServer, err)

andyzhangx · 2019-04-30T07:04:47Z

pkg/credentialprovider/azure/azure_credentials_test.go

@@ -93,4 +93,17 @@ func Test(t *testing.T) {
 			t.Errorf("Missing expected registry: %s", registryName)
 		}
 	}
+
+	invalidImage := "invalidImage"


pls add a separate unit test, you could refer to follow unit test as example:

kubernetes/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go

Line 52 in cf22c56

func TestGetLastSegment(t *testing.T) {

Added in a new function.

@norshtein I mean also copy the test code style like following, it's more maintainable:

tests := []struct { ID string expected string expectErr bool }{ { ID: "", expected: "", expectErr: true, }, { ID: "foo/", expected: "", expectErr: true, }, { ID: "foo/bar", expected: "bar", expectErr: false, }, { ID: "foo/bar/baz", expected: "baz", expectErr: false, }, } for _, test := range tests { s, e := getLastSegment(test.ID) if test.expectErr && e == nil { t.Errorf("Expected err, but it was nil") continue } if !test.expectErr && e != nil { t.Errorf("Unexpected error: %v", e) continue } if s != test.expected { t.Errorf("expected: %s, got %s", test.expected, s) } } }

andyzhangx · 2019-04-30T07:57:38Z

pkg/credentialprovider/azure/azure_credentials.go

+// If the provided image is not an acr image, this function will return empty string
+func parseACRLoginServerFromImage(image string) string {
+	// Parse login server from parameter `image`. An example of parameter `image`: foo.azurecr.io/bar/imageName:version
+	regEx, err := regexp.Compile(`.*\.azurecr\.io|.*\.azurecr\.cn|.*\.azurecr\.de|.*\.azurecr\.us`)


I think use regexp.MustCompile is better, define that var as global, if there is error, it won't pass on e2e test, no need to check error every time:

kubernetes/pkg/volume/azure_dd/azure_common.go

Line 65 in 1608578

lunPathRE = regexp.MustCompile(`/dev/disk/azure/scsi(?:.*)/lun(.+)`)

andyzhangx · 2019-04-30T07:59:02Z

pkg/credentialprovider/azure/azure_credentials.go

@@ -229,6 +223,10 @@ func getLoginServer(registry containerregistry.Registry) string {
 }

 func getACRDockerEntryFromARMToken(a *acrProvider, loginServer string) (*credentialprovider.DockerConfigEntry, error) {
+	// Run EnsureFresh to make sure the token is valid and does not expire
+	if err := a.servicePrincipalToken.EnsureFresh(); err != nil {
+		klog.Errorf("Failed to ensure fresh service principal token: %v", err)


shall we return error here? If return error, no need to log every error

andyzhangx · 2019-04-30T07:59:32Z

pkg/credentialprovider/azure/azure_credentials.go

@@ -182,26 +183,19 @@ func (a *acrProvider) Enabled() bool {
 }

 func (a *acrProvider) Provide(image string) credentialprovider.DockerConfig {
+	klog.V(2).Infof("try to provide secret for image %s", image)


klog.V(4).Infof is better, V(2) is the dispalying log level by default, it would flush the log file

andyzhangx · 2019-04-30T08:05:35Z

BTW, pls sqush all commits into one @norshtein

norshtein · 2019-04-30T08:21:18Z

Updated and rebased. PTAL @andyzhangx

andyzhangx · 2019-04-30T08:29:21Z

pkg/credentialprovider/azure/azure_credentials.go

+// parseACRLoginServerFromImage takes image as parameter and returns login server of it.
+// If the provided image is not an acr image, this function will return empty string
+func parseACRLoginServerFromImage(image string) string {
+	// Parse login server from parameter `image`. An example of parameter `image`: foo.azurecr.io/bar/imageName:version


remove this comment since it's already exists in acrRE

andyzhangx

looks much better now, one small comment about test coverage.

andyzhangx · 2019-04-30T08:44:26Z

pkg/credentialprovider/azure/azure_credentials_test.go

+			expected: "",
+		},
+		{
+			image:    "foo.azurecr.io/bar/image:version",


add test for azurecr.cn

andyzhangx

/lgtm
@feiskyer do you have any comments?

feiskyer

/lgtm
/approve

k8s-ci-robot · 2019-04-30T09:18:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, norshtein

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/credentialprovider/azure/OWNERS~~ [feiskyer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

norshtein · 2019-04-30T09:26:23Z

Many thanks for reviewing! @andyzhangx @feiskyer 😄

andyzhangx · 2019-04-30T09:32:07Z

/test pull-kubernetes-kubemark-e2e-gce-big

fejta-bot · 2019-04-30T12:22:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

tedyu · 2019-04-30T22:59:39Z

pkg/credentialprovider/azure/azure_credentials.go

+	if len(match) == 1 {
+		return match[0]
+	}
+	return ""


Should there be a log for len(match) > 1 ?

The image name is already logged here. len(match) > 1 indicates a wrong image is provided, credential provider can't handle this, in later code flow, no credential will be provided and this error will be caught outside the credential provider when trying to pull the image. Does this make sense?

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 30, 2019

k8s-ci-robot assigned andyzhangx and feiskyer Apr 30, 2019

k8s-ci-robot requested review from feiskyer and khenidak April 30, 2019 03:45

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 30, 2019

andyzhangx suggested changes Apr 30, 2019

View reviewed changes

feiskyer reviewed Apr 30, 2019

View reviewed changes

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 30, 2019

feiskyer added this to In progress in Provider Azure via automation Apr 30, 2019

feiskyer moved this from In progress to Needs Review in Provider Azure Apr 30, 2019

andyzhangx reviewed Apr 30, 2019

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 30, 2019

andyzhangx reviewed Apr 30, 2019

View reviewed changes

norshtein force-pushed the acr-msi-fix branch from b7ee036 to 376eb0a Compare April 30, 2019 08:20

andyzhangx reviewed Apr 30, 2019

View reviewed changes

norshtein force-pushed the acr-msi-fix branch from 376eb0a to 26770f2 Compare April 30, 2019 08:32

norshtein changed the title ~~fix: fix ACR MSI cross-subscription authentication error~~ Fix ACR MSI cross-subscription authentication error Apr 30, 2019

Fix ACR MSI cross-subscription authentication error

b5cdb78

norshtein force-pushed the acr-msi-fix branch from 26770f2 to b5cdb78 Compare April 30, 2019 08:40

andyzhangx reviewed Apr 30, 2019

View reviewed changes

Add more test cases

a2f4f51

norshtein force-pushed the acr-msi-fix branch from e132cfc to a2f4f51 Compare April 30, 2019 08:52

andyzhangx approved these changes Apr 30, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2019

feiskyer reviewed Apr 30, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2019

k8s-ci-robot merged commit c419640 into kubernetes:master Apr 30, 2019

Provider Azure automation moved this from Needs Review to Done Apr 30, 2019

tedyu reviewed Apr 30, 2019

View reviewed changes

andyzhangx mentioned this pull request Jan 28, 2020

[Azure] UseManagedIdentityExtension is broken for Auth with Azure container registry #87579

Closed

TBBle mentioned this pull request Jun 15, 2020

Cross-account ECR Support (Specify RegistryIds to GetAuthorizationTokenInput) #28241

Closed

Fix ACR MSI cross-subscription authentication error #77245

Fix ACR MSI cross-subscription authentication error #77245

Conversation

norshtein commented Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

andyzhangx commented Apr 30, 2019

andyzhangx left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Apr 30, 2019

norshtein commented Apr 30, 2019

andyzhangx commented Apr 30, 2019 • edited

norshtein commented Apr 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Apr 30, 2019

norshtein commented Apr 30, 2019

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

feiskyer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 30, 2019

norshtein commented Apr 30, 2019

andyzhangx commented Apr 30, 2019

fejta-bot commented Apr 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx left a comment •

edited

andyzhangx commented Apr 30, 2019 •

edited