Support Azure resource ID as `userAssignedIdentityID` #1290

ArchangelSDY · 2022-03-20T06:43:01Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support Azure resource ID as userAssignedIdentityID. This makes it a bit more convenient as resource ID is more user-friendly.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Support ARM resource ID as `userAssignedIdentityID`

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-03-20T06:43:09Z

Welcome @ArchangelSDY!

It looks like this is your first PR to kubernetes-sigs/cloud-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cloud-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-03-20T06:43:09Z

Hi @ArchangelSDY. 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 /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.

feiskyer · 2022-03-21T00:49:00Z

pkg/auth/azure_auth.go

-			return adal.NewServicePrincipalTokenFromMSIWithUserAssignedID(msiEndpoint,
-				resource,
-				config.UserAssignedIdentityID)
+			if config.UserAssignedIdentityID[0] == '/' {


what about changing the validation to something like below:

var isResourceID = regexp.MustCompile( `(?i)/subscriptions/(.+?)/resourcegroups/(.+?)/providers/Microsoft..*`).MatchString if if isResourceID(config.UserAssignedIdentityID) {...}

Good idea. Updated to use a standard function azure.ParseResourceID()

nilo19 · 2022-03-25T03:09:26Z

pkg/auth/azure_auth.go

+					resource,
+					config.UserAssignedIdentityID)
+			} else {
+				klog.V(4).Info("azure: User Assigned MSI ID is client ID")


should be error here instead of info

Resource ID paring error doesn't necessarily mean a config error because it could be a client id (GUID). I will include the error message in the info log for better diagnostics.

nilo19 · 2022-03-25T03:10:08Z

Is it possible to enrich unit test for this change?

nilo19 · 2022-03-25T03:10:17Z

/ok-to-test

ArchangelSDY · 2022-03-25T05:49:37Z

Is it possible to enrich unit test for this change?

Added some unit tests.

nilo19 · 2022-03-25T06:49:03Z

Don't forget to squash the commits.

This makes it a bit more convenient.

ArchangelSDY · 2022-03-25T07:42:35Z

Don't forget to squash the commits.

Squashed.

ArchangelSDY · 2022-03-25T08:52:17Z

/retest

nilo19 · 2022-03-27T14:14:41Z

/retest

nilo19 · 2022-03-29T02:08:08Z

/lgtm
/approve

k8s-ci-robot · 2022-03-29T02:08:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArchangelSDY, nilo19

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:

~~OWNERS~~ [nilo19]

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

jackfrancis · 2022-04-28T21:54:55Z

are we planning on cherry-picking this PR?

feiskyer · 2022-04-29T08:15:50Z

@lzhecheng could you help to cherry-pick this PR?

lzhecheng · 2022-04-29T08:32:05Z

/cherrypick release-1.23

lzhecheng · 2022-04-29T08:32:12Z

/cherrypick release-1.1

lzhecheng · 2022-04-29T08:32:28Z

/cherrypick release-1.0

k8s-infra-cherrypick-robot · 2022-04-29T08:32:41Z

@lzhecheng: new pull request created: #1616

In response to this:

/cherrypick release-1.23

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.

k8s-infra-cherrypick-robot · 2022-04-29T08:32:48Z

@lzhecheng: new pull request created: #1617

In response to this:

/cherrypick release-1.1

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.

k8s-infra-cherrypick-robot · 2022-04-29T08:33:05Z

@lzhecheng: new pull request created: #1618

In response to this:

/cherrypick release-1.0

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 · 2022-08-12T08:34:33Z

pkg/auth/azure_auth.go

+					config.UserAssignedIdentityID)
+			}
+
+			klog.V(4).Info("azure: User Assigned MSI ID is client ID. Resource ID parsing error: %+v", err)


Hi @ArchangelSDY , I got this error now, from current code logic, looks like it's just an info log, I think we need to refine the logging, otherwise there will be following error as long as User Assigned MSI ID is client ID

[pod/csi-blob-node-8trh8/blob] I0812 03:15:51.934107 1 azure_auth.go:245] Using AzurePublicCloud environment [pod/csi-blob-node-8trh8/blob] I0812 03:15:51.934127 1 azure_auth.go:96] azure: using managed identity extension to retrieve access token [pod/csi-blob-node-8trh8/blob] I0812 03:15:51.934131 1 azure_auth.go:102] azure: using User Assigned MSI ID to retrieve access token [pod/csi-blob-node-8trh8/blob] I0812 03:15:51.934162 1 azure_auth.go:113] azure: User Assigned MSI ID is client ID. Resource ID parsing error: %+vparsing failed for 82e9bd91-c283-4b2d-8708-6d11b07d630d. Invalid resource Id format

anyway, let me refine the logging to fix this error msg

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 20, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 20, 2022

k8s-ci-robot requested review from feiskyer and lzhecheng March 20, 2022 06:43

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 20, 2022

ArchangelSDY changed the title ~~Support ARM resource ID as userAssignedIdentityID~~ Support Azure resource ID as userAssignedIdentityID Mar 20, 2022

feiskyer reviewed Mar 21, 2022

View reviewed changes

nilo19 reviewed Mar 25, 2022

View reviewed changes

ArchangelSDY force-pushed the ua-msi-resource-id branch from dbf9cb8 to 945b6f1 Compare March 25, 2022 07:41

Support ARM resource ID as userAssignedIdentityID

945b6f1

This makes it a bit more convenient.

k8s-ci-robot assigned nilo19 Mar 29, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2022

k8s-ci-robot merged commit b61509f into kubernetes-sigs:master Mar 29, 2022

This was referenced Apr 29, 2022

[release-1.23] Support Azure resource ID as userAssignedIdentityID #1616

Merged

[release-1.1] Support Azure resource ID as userAssignedIdentityID #1617

Merged

k8s-infra-cherrypick-robot mentioned this pull request Apr 29, 2022

[release-1.0] Support Azure resource ID as userAssignedIdentityID #1618

Closed

This was referenced Apr 29, 2022

Use MSI ClientID as userAssignedIdentityID in azure.json kubernetes-sigs/cluster-api-provider-azure#2214

Merged

Fix case of userAssignedIdentityID in JSON serialization kubernetes-sigs/cluster-api-provider-azure#2268

Merged

jwtty mentioned this pull request May 17, 2022

[release-1.0]Support Azure resource ID as userAssignedIdentityID #1740

Merged

andyzhangx reviewed Aug 12, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Azure resource ID as `userAssignedIdentityID` #1290

Support Azure resource ID as `userAssignedIdentityID` #1290

ArchangelSDY commented Mar 20, 2022 •

edited

Loading

k8s-ci-robot commented Mar 20, 2022

k8s-ci-robot commented Mar 20, 2022

feiskyer Mar 21, 2022

ArchangelSDY Mar 24, 2022

nilo19 Mar 25, 2022

ArchangelSDY Mar 25, 2022

nilo19 commented Mar 25, 2022

nilo19 commented Mar 25, 2022

ArchangelSDY commented Mar 25, 2022

nilo19 commented Mar 25, 2022

ArchangelSDY commented Mar 25, 2022

ArchangelSDY commented Mar 25, 2022

nilo19 commented Mar 27, 2022

nilo19 commented Mar 29, 2022

k8s-ci-robot commented Mar 29, 2022

jackfrancis commented Apr 28, 2022

feiskyer commented Apr 29, 2022

lzhecheng commented Apr 29, 2022

lzhecheng commented Apr 29, 2022

lzhecheng commented Apr 29, 2022

k8s-infra-cherrypick-robot commented Apr 29, 2022

k8s-infra-cherrypick-robot commented Apr 29, 2022

k8s-infra-cherrypick-robot commented Apr 29, 2022

andyzhangx Aug 12, 2022

andyzhangx Aug 12, 2022

Support Azure resource ID as userAssignedIdentityID #1290

Support Azure resource ID as userAssignedIdentityID #1290

Conversation

ArchangelSDY commented Mar 20, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Mar 20, 2022

k8s-ci-robot commented Mar 20, 2022

feiskyer Mar 21, 2022

Choose a reason for hiding this comment

ArchangelSDY Mar 24, 2022

Choose a reason for hiding this comment

nilo19 Mar 25, 2022

Choose a reason for hiding this comment

ArchangelSDY Mar 25, 2022

Choose a reason for hiding this comment

nilo19 commented Mar 25, 2022

nilo19 commented Mar 25, 2022

ArchangelSDY commented Mar 25, 2022

nilo19 commented Mar 25, 2022

ArchangelSDY commented Mar 25, 2022

ArchangelSDY commented Mar 25, 2022

nilo19 commented Mar 27, 2022

nilo19 commented Mar 29, 2022

k8s-ci-robot commented Mar 29, 2022

jackfrancis commented Apr 28, 2022

feiskyer commented Apr 29, 2022

lzhecheng commented Apr 29, 2022

lzhecheng commented Apr 29, 2022

lzhecheng commented Apr 29, 2022

k8s-infra-cherrypick-robot commented Apr 29, 2022

k8s-infra-cherrypick-robot commented Apr 29, 2022

k8s-infra-cherrypick-robot commented Apr 29, 2022

andyzhangx Aug 12, 2022

Choose a reason for hiding this comment

andyzhangx Aug 12, 2022

Choose a reason for hiding this comment

Support Azure resource ID as `userAssignedIdentityID` #1290

Support Azure resource ID as `userAssignedIdentityID` #1290

ArchangelSDY commented Mar 20, 2022 •

edited

Loading