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.

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: add support for Workload Identity #443

Closed
wants to merge 4 commits into from
Closed

feat: add support for Workload Identity #443

wants to merge 4 commits into from

Conversation

weisdd
Copy link

@weisdd weisdd commented Dec 9, 2022

This PR adds support for Workload Identity.

Implementation details

  • As mentioned in the code, adal does not offer native capability to re-read federated token from disk, thus a simple wrapper is required to generate a new SPT and get an access token from there;
  • While working on a similar PR for cert-manager, there was no need to introduce an additional flag for workload identity as the app is agnostic in that sense and calls such mechanisms "ambient". Here, on the other hand, there's a configuration flag UseManagedIdentityExtension, so I thought it would make sense to have UseWorkloadIdentityExtension then;

Tests

Terraform lab with instructions is shared here: https://github.com/weisdd/akv2k8s-pr-443

Potential TODO

  • If you're looking for some tests, then I can potentially add something similar to what I did in cert-manager like having a test web-server that ensures token renewal (link);

Another PR for the same functionality

Aside from a tiny fix for ACR credentials, the rest of the code was prepared last Saturday, just didn't have time to properly test it throughout the week due to excessive load at work. And it happened that another PR was raised today #442 before I did mine. - It tries to achieve the same goal, but under the existing UseManagedIdentityExtension flag and without precautions for adal tokens. Though, in the end, the only important thing is to have the functionality implemented, so I don't mind if my PR gets closed in favour of that one.

Closes: #378

Signed-off-by: Igor Beliakov <[email protected]>
@weisdd weisdd marked this pull request as draft December 9, 2022 19:16
@weisdd
Copy link
Author

weisdd commented Dec 10, 2022

Upd: added terraform lab to the PR description

@cgroschupp
Copy link
Contributor

What is the difference between the akvs-secret-app-acr and akvs-secret-app test? Except of course the registry where the image is located.

@weisdd
Copy link
Author

weisdd commented Dec 12, 2022

@cgroschupp Indeed, the only difference is the private ACR in use for one of the deployments.

It's just when I started working on the PR, I added a deployment for env injection, and then added another one for private ACR. In the end, just one deployment with the private ACR is enough to cover both env injection and adal authentication.

@181192
Copy link
Collaborator

181192 commented Dec 28, 2022

@weisdd Thank you so much for your effort, very much appreciated. I'm closing this in favor for PR in #442 and the usage of azidentity sdk. I do like your solution on using UseWorkloadIdentityExtension

@181192 181192 closed this Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Azure Workload Identity Support
3 participants