Added support for azure workload identity #5390

stijndehaes · 2022-12-28T09:41:21Z

Which component this PR applies to?

What type of PR is this?

/kind feature

What this PR does / why we need it:

It adds support for workload identity to the cluster autoscaler on azure

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added support for workload identity on azure to the cluster autoscaler

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

k8s-ci-robot · 2022-12-28T09:41:29Z

Welcome @stijndehaes!

It looks like this is your first PR to kubernetes/autoscaler 🎉. 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/autoscaler 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. 😃

tallaxes

Thank you for your contribution! Could you please describe the use case this is targeting (or link to an existing issue)?

cluster-autoscaler/cloudprovider/azure/azure_client.go

tallaxes · 2023-01-07T22:53:55Z

cluster-autoscaler/cloudprovider/azure/azure_config.go

+		if tenantId := os.Getenv("AZURE_TENANT_ID"); tenantId != "" {
+			cfg.TenantID = tenantId
+		}
 		cfg.AADClientID = os.Getenv("ARM_CLIENT_ID")
+		if clientId := os.Getenv("AZURE_CLIENT_ID"); clientId != "" {
+			cfg.AADClientID = clientId
+		}


This lets one override ARM_TENANT/CLIENT_ID with AZURE_TENANT/CLIENT_ID. It is not clear why supporting this is needed (if there is a good reason, please explain) and it introduces (an admittedly small) risk of breaking existing deployments. Most importantly, this is not related to introducing workload identity, which is the target of this PR. Please remove, and - if there is a good reason - introduce in a separate PR.

why doing an override here?

These override are here because of the project https://github.com/Azure/azure-workload-identity,
The default environment values it sets are:

AZURE_TENANT_ID

AZURE_CLIENT_ID

AZURE_FEDERATED_TOKEN_FILE

AZURE_AUTHORITY_HOST

To have better interoperability I added these aliases. I can remove them though and add them in a new PR

Thanks for sharing the contexts, it makes sense.

@feiskyer thank you for the feedback!

tallaxes · 2023-01-07T23:00:42Z

charts/cluster-autoscaler/values.yaml

+# azureUseWorkloadIdentityExtension -- Whether to use Azure's workload identity extension for credentials.
+azureUseWorkloadIdentityExtension: false
+
 # azureUseManagedIdentityExtension -- Whether to use Azure's managed identity extension for credentials. If using MSI, ensure subscription ID, resource group, and azure AKS cluster name are set.
 azureUseManagedIdentityExtension: false


Only one of these can be used at a time (if both are set, the current implementation will use workload identity). Please document (and consider adding a check in code that both are not set at the same time).

I added an error in the code, and added documentation that you should not set both

tallaxes · 2023-01-09T16:31:11Z

/label area/provider/azure

k8s-ci-robot · 2023-01-09T16:31:14Z

@tallaxes: The label(s) /label area/provider/azure cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label area/provider/azure

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 · 2023-01-10T01:56:24Z

/area provider/azure

feiskyer · 2023-01-10T01:58:09Z

charts/cluster-autoscaler/values.yaml

@@ -95,6 +95,9 @@ azureClusterName: ""
 # Required if `cloudProvider=azure`
 azureNodeResourceGroup: ""

+# azureUseWorkloadIdentityExtension -- Whether to use Azure's workload identity extension for credentials.
+azureUseWorkloadIdentityExtension: false


+1 to mention here WorkloadIdentity couldn't be used together with other author approaches.

And how AADFederatedTokenFile could be set in the helm chart?

I added documentation to the readme

feiskyer

/lgtm

feiskyer · 2023-01-11T07:57:42Z

@gjtempleton could you help to review the charts changes?

stijndehaes · 2023-01-19T15:21:13Z

@gjtempleton friendly reminder to help with the review of the chart changes :)
@tallaxes if you could give feedback on the changes I made on your feedback that would be great!

tallaxes · 2023-01-21T06:00:54Z

/lgtm

tallaxes · 2023-01-21T06:01:58Z

/assign @gjtempleton

tallaxes

/lgtm

tallaxes · 2023-01-21T06:05:00Z

/assign @gjtempleton

gjtempleton

One request and a couple of tiny typos, but otherwise this LGTM, thanks for the PR!

charts/cluster-autoscaler/Chart.yaml

charts/cluster-autoscaler/values.yaml

stijndehaes · 2023-01-23T09:15:28Z

One request and a couple of tiny typos, but otherwise this LGTM, thanks for the PR!

@gjtempleton I implemented the changes! Thank you for the review.

stijndehaes · 2023-01-31T12:48:28Z

@gjtempleton @tallaxes @feiskyer Can you guys give me a last LGTM or tell me what needs to be changed still? :)

feiskyer

/lgtm

charts/cluster-autoscaler/Chart.yaml

stijndehaes · 2023-02-01T14:34:45Z

@gjtempleton thank you for checking, I have made the changes. If ok can you approve?

@feiskyer I am going to need your approval again sorry for this.

gjtempleton · 2023-02-01T17:33:47Z

/lgtm
/approve

k8s-ci-robot · 2023-02-01T17:33:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, gjtempleton, stijndehaes, tallaxes

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:

~~charts/OWNERS~~ [gjtempleton]
~~cluster-autoscaler/cloudprovider/azure/OWNERS~~ [feiskyer,gjtempleton,tallaxes]

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 added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 28, 2022

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 28, 2022

k8s-ci-robot requested a review from gjtempleton December 28, 2022 09:41

k8s-ci-robot added the area/cluster-autoscaler label Dec 28, 2022

k8s-ci-robot requested a review from tallaxes December 28, 2022 09:41

k8s-ci-robot added the area/helm-charts label Dec 28, 2022

stijndehaes force-pushed the feature/add-support-for-workload-identity branch 3 times, most recently from f8126b7 to 8b045b1 Compare December 28, 2022 09:57

tallaxes suggested changes Jan 7, 2023

View reviewed changes

k8s-ci-robot added the area/provider/azure Issues or PRs related to azure provider label Jan 10, 2023

feiskyer reviewed Jan 10, 2023

View reviewed changes

stijndehaes force-pushed the feature/add-support-for-workload-identity branch from 8b045b1 to 2f3fb3f Compare January 10, 2023 14:12

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 Jan 10, 2023

stijndehaes force-pushed the feature/add-support-for-workload-identity branch 2 times, most recently from 24a3d4b to e9366fd Compare January 10, 2023 14:16

feiskyer approved these changes Jan 11, 2023

View reviewed changes

k8s-ci-robot assigned feiskyer Jan 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2023

stijndehaes requested review from tallaxes and removed request for gjtempleton January 19, 2023 15:20

k8s-ci-robot assigned tallaxes Jan 21, 2023

k8s-ci-robot assigned gjtempleton Jan 21, 2023

tallaxes approved these changes Jan 21, 2023

View reviewed changes

gjtempleton requested changes Jan 23, 2023

View reviewed changes

charts/cluster-autoscaler/Chart.yaml Outdated Show resolved Hide resolved

charts/cluster-autoscaler/values.yaml Outdated Show resolved Hide resolved

charts/cluster-autoscaler/values.yaml Outdated Show resolved Hide resolved

stijndehaes force-pushed the feature/add-support-for-workload-identity branch from e9366fd to 5959e07 Compare January 23, 2023 09:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2023

feiskyer approved these changes Feb 1, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2023

gjtempleton requested changes Feb 1, 2023

View reviewed changes

charts/cluster-autoscaler/Chart.yaml Outdated Show resolved Hide resolved

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 1, 2023

Added support for azure workload identity

ef295a3

stijndehaes force-pushed the feature/add-support-for-workload-identity branch from 704857d to ef295a3 Compare February 1, 2023 14:33

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2023

k8s-ci-robot merged commit 9158196 into kubernetes:master Feb 1, 2023

stijndehaes deleted the feature/add-support-for-workload-identity branch February 2, 2023 07:13

comtalyst mentioned this pull request Aug 28, 2024

chore: backport latest VMs pool, HasInstance(), cloud-provider-azure defork, misc inconsistencies, for 1.27 #7218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for azure workload identity #5390

Added support for azure workload identity #5390

stijndehaes commented Dec 28, 2022

k8s-ci-robot commented Dec 28, 2022

tallaxes left a comment

tallaxes Jan 7, 2023

feiskyer Jan 10, 2023

stijndehaes Jan 10, 2023

feiskyer Jan 11, 2023

stijndehaes Jan 12, 2023

tallaxes Jan 7, 2023

stijndehaes Jan 10, 2023

tallaxes commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

feiskyer commented Jan 10, 2023

feiskyer Jan 10, 2023

feiskyer Jan 10, 2023

stijndehaes Jan 10, 2023

feiskyer left a comment

feiskyer commented Jan 11, 2023

stijndehaes commented Jan 19, 2023

tallaxes commented Jan 21, 2023

tallaxes commented Jan 21, 2023

tallaxes left a comment

tallaxes commented Jan 21, 2023

gjtempleton left a comment

stijndehaes commented Jan 23, 2023

stijndehaes commented Jan 31, 2023

feiskyer left a comment

stijndehaes commented Feb 1, 2023

gjtempleton commented Feb 1, 2023

k8s-ci-robot commented Feb 1, 2023

Added support for azure workload identity #5390

Added support for azure workload identity #5390

Conversation

stijndehaes commented Dec 28, 2022

Which component this PR applies to?

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 Dec 28, 2022

tallaxes 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

tallaxes commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

feiskyer commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer left a comment

Choose a reason for hiding this comment

feiskyer commented Jan 11, 2023

stijndehaes commented Jan 19, 2023

tallaxes commented Jan 21, 2023

tallaxes commented Jan 21, 2023

tallaxes left a comment

Choose a reason for hiding this comment

tallaxes commented Jan 21, 2023

gjtempleton left a comment

Choose a reason for hiding this comment

stijndehaes commented Jan 23, 2023

stijndehaes commented Jan 31, 2023

feiskyer left a comment

Choose a reason for hiding this comment

stijndehaes commented Feb 1, 2023

gjtempleton commented Feb 1, 2023

k8s-ci-robot commented Feb 1, 2023