-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat(Azure): add support for workload identity #3111
Closed
weisdd
wants to merge
7
commits into
kubernetes-sigs:master
from
weisdd:feature/azure-workload-identity
Closed
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e556a4b
Add support for Workload Identity for Azure provider
weisdd 366c2df
Added a draft with Workload Identity documentation
weisdd 573c521
fix: use ResourceManagerEndpoint URL from the environment
weisdd 32de891
fix: expose refresh error
weisdd 19a27f8
feat: add an option to override ClientID, added tests and more comments
weisdd ebf6c70
fix: update azure guide to reflect the pod label requirements for wor…
weisdd 582b0ee
chore: update azure guide to reflect the fact that Workload Identity …
weisdd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@weisdd - Went through the tutorial to test out building AKS cluster with workload identity and getting external DNS fully working only using managed identity. Thanks so much for the hard work on this and really hoping we can get this merged in the near future.
There was one step that was missing which is adding the necessary annotations to the pod (not just the service account). There were a few errors that keep showing up about the token missing from the azure.json file, so after some research found this portion in the docs that should be included in this tutorial for others that are trying to enable workload identity for their AKS clusters.
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 pod label was not a requirement during the original private preview. It was announced/communicated it was going to be required for GA, and was then subsequently rolled out to AKS in the next update (still during preview!). Point being, it wasn't a requirement when those instructions were written 😄
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.
@slickmode Thanks for the review!
Indeed, the tutorial was written before v1.0.0-beta.0 was released last month, which introduced the need for the extra label to limit the number of pods intercepted by a webhook (through an object selector) and enforce
failurePolicy: Fail
. As far as I understand, the design for annotations and labels is finalized now, there's already v1.0.0-rc.0 release, v1.0.0 is expected in a few weeks time (hopefully, it won't get delayed).I'd be happy to update my PR to include the label in the guide, just wasn't sure if there's any chance for the PR to be reviewed.
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.
@slickmode I've just pushed the updated guide to make sure we won't forget it whenever maintainers get to review the PR.
@pinkfloydx33 thanks for your comments here and in other places, they're always helpful :)