-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add ux for workload identity for cloud provider azure #4138
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
// ToDo: Find a way to make this configurable for a user. | ||
// This is the path where the projected service account token should be present for | ||
// cloud provider azure. | ||
aadFederatedTokenFilePath = "/var/run/secrets/azure/tokens/azure-identity-token" //nolint:gosec // Path of projected service account token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of making an API change to configure this. I think we should first go with using an annotation in AzureClusterIdentity
object.
@CecileRobertMichon @mboersma -- thoughts??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this file get on the VM?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4138 +/- ##
==========================================
- Coverage 57.83% 57.79% -0.04%
==========================================
Files 187 187
Lines 19195 19208 +13
==========================================
Hits 11101 11101
- Misses 7466 7479 +13
Partials 628 628 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Ashutosh Kumar <[email protected]>
e1f353c
to
4bf0aa8
Compare
Raised a PR in cloud provider azure to project service account token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Only suggestion from my end would be to add a test case for workload identity here
/assign |
@sonasingh46 could you take what you have in the PR description and document it in https://capz.sigs.k8s.io/topics/vm-identity? |
@@ -544,6 +544,8 @@ const ( | |||
VMIdentitySystemAssigned VMIdentity = "SystemAssigned" | |||
// VMIdentityUserAssigned ... | |||
VMIdentityUserAssigned VMIdentity = "UserAssigned" | |||
// VMIdentityWorkloadIdentity ... | |||
VMIdentityWorkloadIdentity VMIdentity = "WorkloadIdentity" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when WorkloadIdentity
type is configured on the VM, do we need to make any changes to the Azure VM configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Azure VM configuration -- if you mean the configuration from Azure API point of view, then no.
We just need a change in azure.json -- and azure.json object is built reading this.
https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/4138/files#diff-7d084ba6e9e15005850c1b44d72d457cef09f12badee416beda37f957a242364R211
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you mean the configuration from Azure API point of view, then no.
yes, that's what I meant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sonasingh46 can we also update the e2e test that tests workload identity to use this flow so it can be e2e tested?
/test optional |
@sonasingh46: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
/test pull-cluster-api-provider-azure-e2e-optional |
Working on e2e test, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a discussion with @sonasingh46 this morning about the PR.
This PR as-is is not sufficient for using workload identity (WI) with the workload clusters. The --service-account-issuer-url
in the workload cluster will need to be configured with a valid URL and FIC needs to be configured for cloud provider in workload cluster to use WI.
- We'll need to come up with the design for using the existing storage account to host the issuer URL (discovery document and JWKS endpoint).
- Need to configure that URL as
--service-account-issuer-url
in the Kube API server. - Handle rotation of the signing keys + updating JWKS endpoint with the new keys (adding new keys to the list after rotation).
Summarizing few other details I mentioned to @sonasingh46
- Signing keys should be 1:1 with workload cluster and should not be re-used.
- Issuer URL is unique per workload cluster.
If we want to merge this PR as a first step, it would be fine. For testing in CI, we'll need to pre-create the signing keys and issuer URL (similar to the one we have for management cluster in CI) and use those as input for the workload cluster created to test WI.
// secret is not needed in workload identity. | ||
controlPlaneConfig.AadClientSecret = "" | ||
controlPlaneConfig.UseFederatedWorkloadIdentityExtension = true | ||
controlPlaneConfig.AADFederatedTokenFile = aadFederatedTokenFilePath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this to actually work with the workload cluster, the --service-account-issuer-url
needs to be configured with an OIDC issuer URL that's https and publicly accessible serving the discovery document and jwks endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workload cluster that will be created will have the --service-account-issuer-url
provided via the workload cluster template.
Copying the yaml from the PR description
kubeadmConfigSpec:
clusterConfiguration:
apiServer:
extraArgs:
cloud-provider: external
# This flag needs to be passed for the workload cluster to be able to use workload identity
service-account-issuer: <your-service-issuer-url>
# This flag needs to be passed for the workload cluster to be able to use workload identity
service-account-key-file: /etc/kubernetes/pki/sa.pub
# This flag needs to be passed for the workload cluster to be able to use workload identity
service-account-signing-key-file: /etc/kubernetes/pki/sa.key
timeoutForControlPlane: 20m
controllerManager:
extraArgs:
allocate-node-cidrs: "false"
cloud-provider: external
cluster-name: azwi-cluster
# This flag needs to be passed for the workload cluster to be able to use workload identity
service-account-private-key-file: /etc/kubernetes/pki/sa.key
I did not get time to work on this. I will try to take a shot on this weekend and update. Also, if anyone feels that they can help move this faster than me,(and this issue is being a blocker) I am happy to assign this work and provide assistance here or on slack if needed. cc @CecileRobertMichon @mboersma |
/assign |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
Is this still being worked on or applicable with all the workload identity changes? @mboersma @sonasingh46 |
Hey @jackfrancis , what are your thoughts on this PR? |
What this PR does / why we need it:
This PR adds capability to specify to use workload identity for cloud provider azure on workload clusters on azure.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #3589
Special notes for your reviewer:
Contains an API change where there is one more acceptable value for
AzureMachineTemplate
.spec.identity and the new acceptable value isWorkloadIdentity
To create a workload cluster with workload identity enabled, certain flags needs to be passed on kube-apiserver and kube-controller-manager which is describe in a comment in the full workload cluster YAML pasted below.
cherry-pick candidate
How this will work:
Assume you have a management cluster with the workload identity enabled. Now, you can create a workload cluster which will have workload identity enabled on it.
Here are the pre-reqiuistes steps that should be followed which is already documented here
SERVICE_ISSUER_URL
Distribute the key pairs using the following steps:
<your-workload-cluster-name>-sa
in the same namespace as that you the workload clusterCluster
object.Now following is a sample workload cluster YAML that can be used to create a workload cluster which will ensure that the cloud provider azure uses workload identity.
TODOs:
Release note: