Skip to content
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

proposal for workload identity integration #2814

Merged

Conversation

sonasingh46
Copy link
Contributor

@sonasingh46 sonasingh46 commented Nov 15, 2022

Signed-off-by: Ashutosh Kumar [email protected]

What this PR does / why we need it:

Proposal for Azure Workload Identity integration to 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):
Design proposal for #2205

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

proposal for workload identity integration

@k8s-ci-robot k8s-ci-robot added 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2022
@sonasingh46 sonasingh46 force-pushed the workload_identity_proposal branch from e89a08f to b3960be Compare November 15, 2022 20:37
@sonasingh46 sonasingh46 mentioned this pull request Nov 17, 2022
3 tasks
@jackfrancis jackfrancis added this to the v1.7 milestone Nov 17, 2022
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great research work @sonasingh46 and thank you @aramase for all the support. I'm excited to get this in!

docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
@dtzar
Copy link
Contributor

dtzar commented Dec 5, 2022

@sonasingh46 - thank you for this proposal and agreeing to help here. This is a very important enhancement needed by many for CAPZ. When do you think you'll have time to address feedback mentioned here and/or start on the coding work?

@sonasingh46
Copy link
Contributor Author

@dtzar -- Thanks for chiming in. I have addressed the feedback in the proposal. I will raise a PR in parallel so that we can review and get early and quick feedback.
On a high level the only challenge right now I see is the Key Distribution to the control plane nodes. I will cc you to the relevant section on the proposal which mentions this.

@jackfrancis
Copy link
Contributor

Let's prioritize starting a lazy consensus timer roughly a week before the planned 1.7.0 release date (10 Jan 2023)

Copy link
Member

@yastij yastij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few comments

@jackfrancis jackfrancis modified the milestones: v1.7, v1.8 Jan 11, 2023
@sonasingh46 sonasingh46 force-pushed the workload_identity_proposal branch from 8f94209 to 2760ccb Compare February 7, 2023 19:19
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
docs/proposals/20221611-workload-identity-integration.md Outdated Show resolved Hide resolved
Comment on lines +253 to +241
Key pair can be distributed to a workload cluster by creating a secret with the name as `<cluster-name>-sa` encompassing the key pair.
Follow this [link](https://cluster-api.sigs.k8s.io/tasks/certs/using-custom-certificates.html) for more details.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this key pair is different from the signing keys used in the kind cluster. Why is this note part of the design doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signing keys that are used in kind cluster can be distributed to the workload cluster by creating a secret. Now, that same key pairs will be used by the apiserver and controller manager.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the need/use case to do that? is it optional or mandatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is optional.

Use case is the following:

  • I set up a kind management cluster and have key pairs that can be used with workload identity.
  • I can easily just distribute these key pairs to the workload cluster that I created using this as I will be using workload identity on the workload cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I will have to setup workload identity with keys that got generated for the workload cluster which resides in /etc/kubernetes/pki directory.

Or I will have to rotate the keys with the keys with which the workload identity credential is configured. (As kubeadm generate key pairs by default if not specified by the secret)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can easily just distribute these key pairs to the workload cluster that I created using this as I will be using workload identity on the workload cluster.

Although this makes it easier for the user to setup WI in their workload cluster with less work in CAPZ, I wouldn't recommend doing this. As long as there is a 1:1 mapping between management cluster and workload cluster it's fine but if a single management cluster is used to create multiple workload clusters, allowing this would enable reusing same signing keys + issuer in different clusters which is insecure.

Copy link
Contributor Author

@sonasingh46 sonasingh46 May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this makes it easier for the user to setup WI in their workload cluster with less work in CAPZ, I wouldn't recommend doing this. As long as there is a 1:1 mapping between management cluster and workload cluster it's fine but if a single management cluster is used to create multiple workload clusters, allowing this would enable reusing same signing keys + issuer in different clusters which is insecure.

Agreed. I think this does not come as recommendation. Though if this gives in impression of a recommendation like this, I will remove this section.

If you see the user story Management Cluster Via Pivoting -- this is where this becomes useful.
https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2814/files/d62c690a3f2cb8a3fcb2e8de44f25e42b678eaed#diff-8f168651958e0a88d9402ac98e3b8764c9a4b54d29de8284def5e2c9070dd11bR158

So what happens in this case is --

  1. The cloud admin creates a kind cluster
  2. The cloud admin create a workload cluster on azure ( 3 CP + 3 worker nodes )
  3. The cloud admin wants to now convert this workload cluster to a mgmt cluster and use workload identity.
  4. The cloud admins nukes the kind cluster.

( This is the pivoting process followed by a lot of user to create a full fledged production management cluster )

So at step 2 if keys are not explicitly specified via the secret then kubeadm generates default ones and now there are some extra steps rotating those. So just to keep the life simple this is the approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this will be helpful in CI.
We can pre create the secret using the same keys in the kind cluster in the CI. And now the cloud provider azure pods in workload cluster that got created will be capable of using workload identity.

@sonasingh46 sonasingh46 force-pushed the workload_identity_proposal branch from d62c690 to e283547 Compare June 8, 2023 15:08
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 8, 2023
@sonasingh46 sonasingh46 force-pushed the workload_identity_proposal branch 2 times, most recently from 3896a6e to f6c3507 Compare June 8, 2023 15:11
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/assign @aramase

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 8ab538a7a2faed2a2474f3dbad231168d20decd4

@sonasingh46 sonasingh46 force-pushed the workload_identity_proposal branch from f6c3507 to d9674a3 Compare June 14, 2023 19:14
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2023
@sonasingh46 sonasingh46 force-pushed the workload_identity_proposal branch from d9674a3 to 42b729a Compare June 14, 2023 19:19
@sonasingh46 sonasingh46 force-pushed the workload_identity_proposal branch from 42b729a to 6394ec3 Compare June 14, 2023 19:24
Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ac4f2b654bc105bbabe3972eac0dae12cc3358f1

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve
/hold until EOD 6/15

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2023
@CecileRobertMichon
Copy link
Contributor

/hold cancel

Congrats @sonasingh46 🎉

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2023
@k8s-ci-robot k8s-ci-robot merged commit 135f578 into kubernetes-sigs:main Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.