feat(aso): add azure workload identity label to pod template#4452
feat(aso): add azure workload identity label to pod template#4452synthe102 wants to merge 1 commit into
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions 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. |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @synthe102! |
|
Hi @synthe102. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4452 +/- ##
==========================================
+ Coverage 61.95% 61.96% +0.01%
==========================================
Files 188 188
Lines 18768 18768
==========================================
+ Hits 11628 11630 +2
+ Misses 6502 6500 -2
Partials 638 638 ☔ View full report in Codecov by Sentry. |
|
/ok-to-test |
| name: azureserviceoperator-controller-manager | ||
| - patch: |- # Add label for Azure workload identity webhook | ||
| - op: add | ||
| path: "/spec/template/metadata/labels/azure.workload.identity~1use" |
There was a problem hiding this comment.
@mattchr Is this not needed per your point here or was that a separate issue? #3113 (comment)
There was a problem hiding this comment.
It's needed if you want to leverage the Azure WI webhook to inject the creds based on your service account. I think that if capz-manager has this annotation, there's no reason to keep it from being added to ASO for consistency reasons.
There was a problem hiding this comment.
there's no reason to keep it from being added to ASO for consistency reasons.
This is true, but at the same time there is no reason to add it because it won't do anything for ASO. ASO doesn't need or use the WI webhook because it creates the credential without going to IMDS, which means it doesn't use the WI webhook at all.
Instead it uses the USE_WORKLOAD_IDENTITY_AUTH boolean for the global secret, or AUTH_MODE: "workloadidentity" for the aso-credential or per-resource secrets.
The reason we don't use the webhook is that the webhook is based on env variables in the pod, but because the pod is multitenant (can be using multiple identities 1 per namespace or 1 per resource even) the webhook doesn't really help us.
There was a problem hiding this comment.
Instead it uses the USE_WORKLOAD_IDENTITY_AUTH boolean for the global secret, or
AUTH_MODE: "workloadidentity"for theaso-credentialor per-resource secrets.
And CAPZ should configure this automatically for you based on the AzureClusterIdentity for the cluster.
There was a problem hiding this comment.
Thanks for all this context ! It makes sense. I can close this PR as it's not needed.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add the label
azure.workload.identity/use: "true"to ASO deployment pod template so credentials can be injected using the Azure Workload Identity webhook.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 #
Special notes for your reviewer:
TODOs:
Release note: