Skip to content

feat(aso): add azure workload identity label to pod template #4452

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

Closed
synthe102 wants to merge 1 commit into from
Closed

feat(aso): add azure workload identity label to pod template #4452

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions config/aso/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ patches:
version: v1
kind: Deployment
name: azureserviceoperator-controller-manager
- patch: |- # Add label for Azure workload identity webhook
- op: add
path: "/spec/template/metadata/labels/azure.workload.identity~1use"
Comment thread
mboersma marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@nojnhuh nojnhuh Jan 11, 2024

Choose a reason for hiding this comment

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

@mattchr Is this not needed per your point here or was that a separate issue? #3113 (comment)

Copy link
Copy Markdown
Author

@synthe102 synthe102 Jan 12, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@nojnhuh nojnhuh Jan 12, 2024

Choose a reason for hiding this comment

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

Sorry, wrong Matt. @matthchr

Copy link
Copy Markdown

@matthchr matthchr Jan 12, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@nojnhuh nojnhuh Jan 12, 2024

Choose a reason for hiding this comment

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

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.

And CAPZ should configure this automatically for you based on the AzureClusterIdentity for the cluster.

Copy link
Copy Markdown
Author

@synthe102 synthe102 Jan 12, 2024

Choose a reason for hiding this comment

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

Thanks for all this context ! It makes sense. I can close this PR as it's not needed.

value: "true"
target:
group: apps
version: v1
kind: Deployment
name: azureserviceoperator-controller-manager
- patch: |- # remove permissions to manage CRDs
$patch: delete
apiVersion: rbac.authorization.k8s.io/v1
Expand Down