-
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 all 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.
IMHO this should be part of the deployment. https://github.com/kubernetes-sigs/external-dns/blob/master/charts/external-dns/templates/deployment.yaml
Add the client_id to the values.yaml and if .Values.provider==azure (
provider
already exists in the values.yaml) and if the client_id for workload identity was set, add the label to the deployment plus add the annotation to the service accountThere 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.
@sebader Many months ago, when I was working on this section, I just tried to make it look similar to other sections of the guide (to keep the guide consistent). When looking at the official external-dns chart, it doesn't seem to have much focus on offering something special for the providers external-dns has integrations with. So, I'm not sure if the changes you suggested can get accepted by maintainers. Though, we can simply rely on
podLabels
andserviceAccount.annotations
of the existing chart.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 lean towards sebader in that I don't think patching should be encouraged but I think I lean a bit towards weisdd with that podLabels, podAnnotations and serviceAccount.labels is the way to go and explicitly setting them. Having logic for automatically setting them feels fragile since Microsoft keeps changing the way things behave.