Add new authentication type UserAssignedIdentityCredentials #5421

bryan-cox · 2025-02-10T17:42:52Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This pull request adds the capability to authenticate with Azure using a new authentication type available only to 1st party Microsoft applications, UserAssignedIdentityCredentials. More information on this new authentication type can be found here - https://github.com/Azure/msi-dataplane/blob/main/pkg/dataplane/reloadCredentials.go#L60.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
N/A

Special notes for your reviewer:

TODOs:

squashed commits
includes documentation
adds unit tests
cherry-pick candidate

Release note:

Adds a new authentication type for 1st party Microsoft applications - UserAssignedIdentityCredentials

k8s-ci-robot · 2025-02-10T17:42:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2025-02-10T17:53:19Z

Codecov Report

Attention: Patch coverage is 66.66667% with 15 lines in your changes missing coverage. Please review.

Project coverage is 52.58%. Comparing base (a5f0307) to head (b51f5eb).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
azure/credential_cache.go	0.00%	11 Missing ⚠️
api/v1beta1/azureclusteridentity_validation.go	0.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5421      +/-   ##
==========================================
+ Coverage   52.56%   52.58%   +0.02%     
==========================================
  Files         272      272              
  Lines       29423    29468      +45     
==========================================
+ Hits        15465    15495      +30     
- Misses      13151    13165      +14     
- Partials      807      808       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bryan-cox · 2025-02-10T19:42:52Z

/test all

bryan-cox · 2025-02-13T13:23:00Z

/test all

bryan-cox · 2025-02-13T14:59:01Z

/test pull-cluster-api-provider-azure-e2e-aks

willie-yao · 2025-02-26T21:11:07Z

/retest

willie-yao

Thanks for your work on this! Just a few comments regarding webhook validation and unit testing.

willie-yao · 2025-02-26T21:16:43Z

api/v1beta1/azureclusteridentity_types.go

@@ -62,6 +62,14 @@ type AzureClusterIdentitySpec struct {
 	// CertPath is the path where certificates exist. When set, it takes precedence over ClientSecret for types that use certs like ServicePrincipalCertificate.
 	// +optional
 	CertPath string `json:"certPath,omitempty"`
+	// UserAssignedIdentityCredentialsPath is the path where an existing JSON file exists containing the JSON format of
+	// a UserAssignedIdentityCredentials struct.
+	// See the msi-dataplane for more details on UserAssignedIdentityCredentials - https://github.com/Azure/msi-dataplane/blob/63fb37d3a1aaac130120624674df795d2e088083/pkg/dataplane/internal/generated_client.go#L156C6-L156C37


nit: Are you able to shorten this link here to main or another branch rather than the full sha?

This actually worked out because they moved the place where this was defined 😅

willie-yao · 2025-02-26T21:36:26Z

azure/scope/identity.go

+		if authErr != nil {
+			return nil, authErr
+		}
+


Are you able to add some test cases to TestGetTokenCredential to cover this new flow?

willie-yao · 2025-02-26T21:45:13Z

api/v1beta1/azureclusteridentity_types.go

+	// UserAssignedIdentityCredentialsCloudType is used with UserAssignedIdentityCredentialsPath to specify the Cloud
+	// type. Can only be one of the following values: public, china, or usgovernment
+	// If a value is not specified, defaults to AzurePublicCloud
+	UserAssignedIdentityCredentialsCloudType string `json:"userAssignedIdentityCredentialsCloudType,omitempty"`


For these new types, can you add some defaulting and validating webhooks to azureclusteridentity_webhook.go and azureclusteridentity_validation.go respectively? For example, these should not be set if IdentityType is not UserAssignedIdentityCredential. Some logic also needs to exist to set the default for UserAssignedIdentityCredentialsCloudType depending on IdentityType`

Hey @willie-yao I updated azureclusteridentity_validation.go. I don't understand what is needed for 'azureclusteridentity_webhook.go' as I didn't see any defaulting for other identity types, but I could be missing it.

api/v1beta1/azureclusteridentity_types.go

willie-yao · 2025-02-26T21:47:40Z

go.mod

@@ -1,8 +1,8 @@
 module sigs.k8s.io/cluster-api-provider-azure

-go 1.22.7
+go 1.23.0


Can this go bump be its own PR that we merge before this one?

This comment is OBE based on - #5421 (comment)

nawazkh

Sharing some of my thoughts..

api/v1beta1/azurecluster_validation.go

api/v1beta1/azureclusteridentity_validation.go

azure/scope/identity.go

api/v1beta1/azureclusteridentity_types.go

This commit adds documentation for UserAssignedIdentityCredentials. Signed-off-by: Bryan Cox <[email protected]>

nawazkh · 2025-03-06T22:23:19Z

/test ls

k8s-ci-robot · 2025-03-06T22:23:22Z

@nawazkh: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-apiversion-upgrade

/test pull-cluster-api-provider-azure-build

/test pull-cluster-api-provider-azure-ci-entrypoint

/test pull-cluster-api-provider-azure-e2e

/test pull-cluster-api-provider-azure-e2e-aks

/test pull-cluster-api-provider-azure-test

/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff

/test pull-cluster-api-provider-azure-apiserver-ilb

/test pull-cluster-api-provider-azure-capi-e2e

/test pull-cluster-api-provider-azure-conformance

/test pull-cluster-api-provider-azure-conformance-custom-builds

/test pull-cluster-api-provider-azure-conformance-dual-stack-with-ci-artifacts

/test pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts

/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts

/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts-dra

/test pull-cluster-api-provider-azure-e2e-optional

/test pull-cluster-api-provider-azure-e2e-workload-upgrade

/test pull-cluster-api-provider-azure-load-test-custom-builds

/test pull-cluster-api-provider-azure-windows-custom-builds

/test pull-cluster-api-provider-azure-windows-with-ci-artifacts

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff

pull-cluster-api-provider-azure-apiversion-upgrade

pull-cluster-api-provider-azure-build

pull-cluster-api-provider-azure-capi-e2e

pull-cluster-api-provider-azure-ci-entrypoint

pull-cluster-api-provider-azure-e2e

pull-cluster-api-provider-azure-e2e-aks

pull-cluster-api-provider-azure-test

pull-cluster-api-provider-azure-verify

In response to this:

/test ls

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-sigs/prow repository.

nawazkh · 2025-03-06T22:23:48Z

/test pull-cluster-api-provider-azure-e2e-optional

azure/scope/identity.go

nawazkh · 2025-03-06T22:31:56Z

Thank you for putting this together!
/lgtm

k8s-ci-robot · 2025-03-06T22:32:02Z

LGTM label has been added.

Git tree hash: 42b42043afee4e6d6bfd007c9e269529bfa48a3b

This commit adds a new authentication type, UserAssignedIdentityCredentials. This allows a 1st party Microsoft application to authenticate using a managed identity's certificate, which is accessed through the MSI data plane. More information on this authentication type can be found here - https://github .com/Azure/msi-dataplane/blob/63fb37d3a1aaac130120624674df795d2e088083 /pkg/dataplane/reloadCredentials.go#L60. Signed-off-by: Bryan Cox <[email protected]>

bryan-cox · 2025-03-07T13:27:42Z

/retest-required

bryan-cox · 2025-03-07T15:45:53Z

/retest-required

nawazkh · 2025-03-07T19:37:16Z

/retest

bryan-cox · 2025-03-10T13:50:23Z

@nawazkh can I get another tag on this one please?

nawazkh · 2025-03-10T16:56:47Z

/lgtm
/approve

k8s-ci-robot · 2025-03-10T16:56:53Z

LGTM label has been added.

Git tree hash: 2231b0e6809f85e2750efe57bf089a36c71dbd53

k8s-ci-robot · 2025-03-10T16:56:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nawazkh

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~~ [nawazkh]

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

k8s-ci-robot requested review from Jont828 and jsturtevant February 10, 2025 17:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 10, 2025

bryan-cox changed the title ~~Add ua identity creds~~ Add new authentication type UserAssignedIdentityCredential Feb 10, 2025

bryan-cox changed the title ~~Add new authentication type UserAssignedIdentityCredential~~ Add new authentication type UserAssignedIdentityCredentials Feb 10, 2025

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 10, 2025

bryan-cox force-pushed the add-UAIdentityCreds branch from bd3805c to f56022b Compare February 10, 2025 19:28

bryan-cox force-pushed the add-UAIdentityCreds branch from f56022b to 7ccf8c6 Compare February 12, 2025 14:23

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2025

bryan-cox force-pushed the add-UAIdentityCreds branch from 7ccf8c6 to c87574e Compare February 13, 2025 13:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2025

bryan-cox force-pushed the add-UAIdentityCreds branch from c87574e to 3c7c616 Compare February 26, 2025 16:55

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2025

bryan-cox marked this pull request as ready for review February 26, 2025 17:41

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2025

k8s-ci-robot requested a review from willie-yao February 26, 2025 17:42

willie-yao moved this from Todo to Needs Review in CAPZ Planning Feb 26, 2025

willie-yao reviewed Feb 26, 2025

View reviewed changes

nawazkh reviewed Mar 6, 2025

View reviewed changes

Add doc for UserAssignedIdentityCredentials

cfee1d2

This commit adds documentation for UserAssignedIdentityCredentials. Signed-off-by: Bryan Cox <[email protected]>

bryan-cox force-pushed the add-UAIdentityCreds branch from 4814884 to 5adb9d7 Compare March 6, 2025 20:36

nawazkh reviewed Mar 6, 2025

View reviewed changes

azure/scope/identity.go Show resolved Hide resolved

k8s-ci-robot assigned nawazkh Mar 6, 2025

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

bryan-cox force-pushed the add-UAIdentityCreds branch from 5adb9d7 to b51f5eb Compare March 7, 2025 10:47

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2025

k8s-ci-robot requested review from nawazkh and willie-yao March 7, 2025 10:47

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

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

k8s-ci-robot merged commit 03b8cc1 into kubernetes-sigs:main Mar 10, 2025
24 of 25 checks passed

github-project-automation bot moved this from Wait-On-Author to Done in CAPZ Planning Mar 10, 2025

bryan-cox deleted the add-UAIdentityCreds branch March 10, 2025 16:59

bryan-cox restored the add-UAIdentityCreds branch March 10, 2025 17:13

bryan-cox mentioned this pull request Mar 10, 2025

CNTRLPLANE-105: Add new authentication type UserAssignedIdentityCredentials openshift/cluster-api-provider-azure#333

Merged

Add new authentication type UserAssignedIdentityCredentials #5421

Add new authentication type UserAssignedIdentityCredentials #5421

Uh oh!

Conversation

bryan-cox commented Feb 10, 2025

Uh oh!

k8s-ci-robot commented Feb 10, 2025

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bryan-cox commented Feb 10, 2025

Uh oh!

bryan-cox commented Feb 13, 2025

Uh oh!

bryan-cox commented Feb 13, 2025

Uh oh!

willie-yao commented Feb 26, 2025

Uh oh!

willie-yao left a comment

Choose a reason for hiding this comment

Uh oh!

willie-yao Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

bryan-cox Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

willie-yao Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

willie-yao Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

bryan-cox Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willie-yao Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

bryan-cox Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

nawazkh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nawazkh commented Mar 6, 2025

Uh oh!

k8s-ci-robot commented Mar 6, 2025

Uh oh!

nawazkh commented Mar 6, 2025

Uh oh!

Uh oh!

nawazkh commented Mar 6, 2025

Uh oh!

k8s-ci-robot commented Mar 6, 2025

Uh oh!

bryan-cox commented Mar 7, 2025

Uh oh!

bryan-cox commented Mar 7, 2025

Uh oh!

nawazkh commented Mar 7, 2025

Uh oh!

bryan-cox commented Mar 10, 2025

Uh oh!

nawazkh commented Mar 10, 2025

Uh oh!

k8s-ci-robot commented Mar 10, 2025

Uh oh!

k8s-ci-robot commented Mar 10, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 10, 2025 •

edited

Loading