-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
plugins/rest: Add Azure Managed Identities Auth Plugin #3952
plugins/rest: Add Azure Managed Identities Auth Plugin #3952
Conversation
570d5bd
to
3129a4b
Compare
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.
Looks like a good first step to me! Formatting looks a little off, but I guess go fmt will take care of that. Looking forward to see this done!
I did some manual testing via the following process:
From the above I'm fairly confident in the correctness of the plugin, but to fully test I would need to use bundles. I'm a bit stuck here. I've created an Azure storage account, but am not entirely sure how to upload a bundle, and assign it managed identities so I can access it from my VM. Any help would be much appreciated. |
c0b3426
to
122636e
Compare
Thanks David! And great work on this. I don't have the time to test this week but will be happy to do it next week. Since you've verified the token gets fetched we could have it merged before that, but on the other hand there's almost a month before next release, so there's no stress to make it into that. I'll get back to you once I've taken it for a test drive :) |
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.
Great work! Left a few comments.
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.
Looks pretty good, and it's an impressive first contribution 👏 -- some nitpicks inline 🙃 👇
We're trying not to use any assertions framework in OPA's tests. You might notice that there are tests using github.com/stretchr/testify
, but they are only in vendored code (or code copied into the project, like the jwt and jsonschema bits).
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.
Nothing to add as far as I am concerned. Thanks for bearing with me on the nitpicks! 👍
Looks like we're in no hurry to merge the PR. I will try to get to manual testing again soon, though I've been struggling a bit due to lack of Azure/OPA knowledge. If you could also take a look next week that'd be great, then we can circle back. Thanks for the review and all the support. |
8cf5cf0
to
861af28
Compare
I decided to give it another go, and was successfully able to test this plugin end-to-end by pulling a bundle from Azure storage using managed identities. This is my first time using Azure and first time downloading OPA, so if any of the following doesn't make any sense feel free to let me know. Here is my full procedure:
I may be missing a few small steps since I had to try many things but this should cover almost everything. If everything looks good, I think the PR is ready for merge. |
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.
LGTM. Impressive first contribution, and great seeing how you've responded to PR feedback. Awesome! 👏
This has been my experience too working with Azure in the past. I think your docs should be amended with this detail so that others don't get stuck here... even if it's technically no auth plug-in responsibility. It's not of much use though if people can't use it to actually fetch anything 😄 |
@Scowluga for future reference, you can skip the "Build OPA" step when testing out PRs: On the "checks" tab for this PR, selecting "PR Checks" gets you to this page, and if you scroll down there, you'll see the binaries zip archive, which contains all the opa binaries built from this branch: |
This commit adds an HTTP auth plugin that fetches bearer access tokens using managed identities for Azure resources. This plugin will complement the existing AWS and GCP auth plugins. Signed-off-by: David Lu <david.scowluga@gmail.com>
861af28
to
ed38222
Compare
Thanks @srenatus for the tip, I've somehow never actually clicked that "Checks" tab before 😄 I added the |
We have testify in our go.mod. This sometimes misleads contributors (including myself!) to think that we can use testify in test code. Since testify is a common testing package, many go developers default to using it. I'm not sure if we want to merge this as we might rather leave these vendored packages untouched, but it was 10 minutes of work and I was interested to see if we can avoid issues like this in future: * #5753 (comment) * #5447 (comment) * #3952 (review) Signed-off-by: Charlie Egan <charlie@styra.com>
This PR introduces an HTTP auth plugin for Azure managed identities. Tokens are acquired following Azure docs, and each of the HTTP request params can be specified using the plugin.
Related to #2938 - adding a GCP metadata auth plugin
Fixes #3916
Signed-off-by: David Lu david.scowluga@gmail.com