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.

Sign up for GitHub

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

Jump to bottom

add support for workload identity #2924

Closed
wants to merge 8 commits into from
Closed

add support for workload identity #2924

wants to merge 8 commits into from

Conversation

sonasingh46
Copy link
Contributor

@sonasingh46 sonasingh46 commented Dec 10, 2022 •
edited
Loading

What type of PR is this?
This PR enables capz to use Azure Workload Identity.

What this PR does / why we need it:
Implementation of proposal #2814

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 #2205

Special notes for your reviewer:

  • This PR should be merged after merging https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2748[Done]
  • The test Jobs should pass once preset is enabled on all the Jobs. [Done]
  • We will need to convert all the templates to using user-assigned-identity. [Done]
  • Move all the e2e tests to be using workload identity. [Done]
  • Current developer experience changes with the current change for tilt. make tilt-up with the current changes will require all the devs to set up the workload identity pre-requisites and expose 3 env variables. Do you think we should continue with this or have some env vars e.g. ENABLE_WORKLOAD_IDENTITY=true explicitly specified to do so. And if not, the usual way of make tilt-up will work. [Needs Review/Discussion]

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Enhancement to use Azure Workload Identity

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 10, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 10, 2022
@sonasingh46 sonasingh46 changed the title [WIP]: Workload identity implemntation [WIP]: Workload identity implementation Dec 10, 2022
@sonasingh46 sonasingh46 force-pushed the workload_identity_implemntation branch 2 times, most recently from 0b23c7d to 0d5fa7c Compare December 10, 2022 18:45
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 10, 2022
@sonasingh46
Copy link
Contributor Author

@CecileRobertMichon -- I have worked on top of this PR #2748 commit of @r4f4

Once that PR gets merged, I will rebase it.

Pending Items to remove WIP :

  • CI setup for azwi which requires storing key pair and publishing OIDC URL
  • A way to copy key pair on the workload cluster as IIRC one e2e test creates a workload cluster and then converts it for management cluster for v1alpha3 --> v1alpha4 upgrade.

Feedback, suggestion are welcome.

cc @jackfrancis @mboersma @aramase

@sonasingh46 sonasingh46 marked this pull request as ready for review December 10, 2022 18:55
@jackfrancis jackfrancis added this to the next milestone Dec 15, 2022
@sonasingh46 sonasingh46 force-pushed the workload_identity_implemntation branch from 0d5fa7c to 3325b26 Compare December 16, 2022 08:05
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2022
@aramase
Copy link
Member

aramase commented Dec 19, 2022

/cc

@k8s-ci-robot k8s-ci-robot requested a review from aramase December 19, 2022 18:12
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 25, 2022
@sonasingh46 sonasingh46 force-pushed the workload_identity_implemntation branch from 3325b26 to 18ce3e5 Compare January 14, 2023 22:54
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2023
@jackfrancis jackfrancis modified the milestones: next, v1.8 Jan 19, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2023
@sonasingh46 sonasingh46 force-pushed the workload_identity_implemntation branch from 18ce3e5 to 7d9e6e4 Compare February 7, 2023 13:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2023
@mboersma mboersma modified the milestones: v1.9, v1.10 May 3, 2023
@sonasingh46 sonasingh46 force-pushed the workload_identity_implemntation branch from b7d0fe8 to d65818a Compare May 6, 2023 15:21
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 6, 2023
@sonasingh46
Copy link
Contributor Author

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

@sonasingh46 sonasingh46 force-pushed the workload_identity_implemntation branch from d65818a to 9c3e3ee Compare May 6, 2023 15:56
@sonasingh46
Copy link
Contributor Author

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

2 similar comments
@sonasingh46
Copy link
Contributor Author

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

@sonasingh46
Copy link
Contributor Author

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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2023
@sonasingh46
azwi integration to capz
d2b7d4f 
Signed-off-by: Ashutosh Kumar <[email protected]>
@sonasingh46
incorporate review comments
006802c 
Signed-off-by: Ashutosh Kumar <[email protected]>
@sonasingh46
add tenant id to test
abfd02e 
Signed-off-by: Ashutosh Kumar <[email protected]>
@sonasingh46
add dummy tenant ID
385ee2b 
Signed-off-by: Ashutosh Kumar <[email protected]>
@sonasingh46 sonasingh46 force-pushed the workload_identity_implemntation branch from 1a0d4eb to 385ee2b Compare May 13, 2023 02:31
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 13, 2023
@sonasingh46
Copy link
Contributor Author

/retest

@sonasingh46
Copy link
Contributor Author

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

@sonasingh46 sonasingh46 force-pushed the workload_identity_implemntation branch from b977f6f to 442971d Compare May 13, 2023 13:59
@sonasingh46
fix entrypoint ci
2d37296 
Signed-off-by: Ashutosh Kumar <[email protected]>
@sonasingh46 sonasingh46 changed the title Workload identity implementation add support for workload identity May 15, 2023
@sonasingh46
Copy link
Contributor Author

Added a different PR #3583 just to enable workload identity capability in capz to reduce the scope of PR.
Other template changes and moving all ci test to worklaod identity can be done in subsequent PRs.

Did not change in this PR itself to keep the change ready so that it is easy to pick the change from here for the subsequent PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@aramase aramase aramase requested changes

@alexeldeib alexeldeib Awaiting requested review from alexeldeib

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

@devigned devigned Awaiting requested review from devigned

@Jont828 Jont828 Awaiting requested review from Jont828

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
CAPZ Planning
Archived in project
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.

Migrate from AAD pod identity to Azure Workload Identity
9 participants
@sonasingh46 @k8s-ci-robot @aramase @dlipovetsky @codecov-commenter @dlom @CecileRobertMichon @mboersma @jackfrancis