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 64.28571% with 15 lines in your changes missing coverage. Please review.

Project coverage is 52.57%. Comparing base (057907a) to head (4814884).

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.55%   52.57%   +0.02%     
==========================================
  Files         272      272              
  Lines       29423    29465      +42     
==========================================
+ Hits        15463    15492      +29     
- Misses      13153    13165      +12     
- Partials      807      808       +1

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

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.

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

api/v1beta1/azureclusteridentity_types.go

Comment on line 47 needs to be updated to include the new IdentityType

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)

willie-yao · 2025-02-26T21:49:14Z

Also, it looks like the netlify jobs are failing with internal error: package "k8s.io/apimachinery/pkg/fields" without types was imported from "sigs.k8s.io/controller-runtime/pkg/internal/field/selector"

https://app.netlify.com/sites/kubernetes-sigs-cluster-api-provider-azure/deploys/67bf477902eca200082ebf2f

k8s-ci-robot · 2025-02-27T17:54:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from willie-yao. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

bryan-cox · 2025-02-27T17:57:35Z

During the CAPZ SIG meeting today, we discussed what to do about the Golang bump being brought in when I included the ASO code needed for this new authentication type. The project I'm on isn't using ASO so I don't believe I need this newly added ASO code and this new authentication type is only for 1st party Microsoft applications. I don't believe many other projects will be using this new authentication type soon.

Given these reasons, I am planning to:

remove the ASO parts so the Golang bump issue will be eliminated
test this PR without the ASO part in my project to make sure its working as expected
will introduce the ASO part later when CAPZ has bumped up to Golang v1.23

If the ASO part is indeed needed now, I'll reach back out.

This commit adds documentation for UserAssignedIdentityCredentials. Signed-off-by: Bryan Cox <brcox@redhat.com>

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 <brcox@redhat.com>

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 reviewed Feb 26, 2025

View reviewed changes

willie-yao self-assigned this Feb 26, 2025

bryan-cox force-pushed the add-UAIdentityCreds branch from 3c7c616 to f79a44c Compare February 27, 2025 17:54

bryan-cox force-pushed the add-UAIdentityCreds branch 5 times, most recently from aeba66e to feb5f4b Compare March 4, 2025 17:40

Add doc for UserAssignedIdentityCredentials

380ec5b

This commit adds documentation for UserAssignedIdentityCredentials. Signed-off-by: Bryan Cox <brcox@redhat.com>

bryan-cox force-pushed the add-UAIdentityCreds branch 2 times, most recently from d38d830 to 74df21d Compare March 4, 2025 17:53

bryan-cox force-pushed the add-UAIdentityCreds branch from 74df21d to 4814884 Compare March 4, 2025 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new authentication type UserAssignedIdentityCredentials #5421

Add new authentication type UserAssignedIdentityCredentials #5421

bryan-cox commented Feb 10, 2025

k8s-ci-robot commented Feb 10, 2025

codecov bot commented Feb 10, 2025 •

edited

Loading

bryan-cox commented Feb 10, 2025

bryan-cox commented Feb 13, 2025

bryan-cox commented Feb 13, 2025

willie-yao commented Feb 26, 2025

willie-yao left a comment

willie-yao Feb 26, 2025

bryan-cox Feb 27, 2025

willie-yao Feb 26, 2025

willie-yao Feb 26, 2025

bryan-cox Feb 27, 2025

willie-yao Feb 26, 2025

willie-yao Feb 26, 2025

bryan-cox Feb 27, 2025

willie-yao commented Feb 26, 2025

k8s-ci-robot commented Feb 27, 2025

bryan-cox commented Feb 27, 2025

Add new authentication type UserAssignedIdentityCredentials #5421

Are you sure you want to change the base?

Add new authentication type UserAssignedIdentityCredentials #5421

Conversation

bryan-cox commented Feb 10, 2025

k8s-ci-robot commented Feb 10, 2025

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

bryan-cox commented Feb 10, 2025

bryan-cox commented Feb 13, 2025

bryan-cox commented Feb 13, 2025

willie-yao commented Feb 26, 2025

willie-yao left a 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

willie-yao commented Feb 26, 2025

k8s-ci-robot commented Feb 27, 2025

bryan-cox commented Feb 27, 2025

codecov bot commented Feb 10, 2025 •

edited

Loading