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

Azure SDK "track 2": authentication and secretstore/azure/keyvault #1290

Merged
merged 14 commits into from Nov 16, 2021

Conversation

ItalyPaleAle
Copy link
Contributor

Description

This PR is yet another step into addressing #1103.

The Azure SDK team is rebuilding all services SDKs essentially from scratch, in a more streamlined way. Sadly, that also comes with a complete rewrite of the auth logic, and the previous auth logic implemented for the "common Azure auth layer" is not compatible with the "track 2" Azure SDKs.

This PR updates the secretstore/azure/keyvault component to use the new Azure Key Vault secret store SDK (https://github.com/Azure/azure-sdk-for-go/tree/main/sdk/keyvault/azsecrets) that is part of "track 2".

Most importantly, however, this PR introduces support for the new Azure Identity client module (https://github.com/Azure/azure-sdk-for-go/tree/main/sdk/azidentity) which all the updated SDKs rely on.

While the internals of the Azure Key Vault secret store have changed, the code should be fully backwards-compatible for Dapr users.

PS: I have tested this against a live Key Vault resource in my Azure subscription, but I do not have a way to test it against Azure China or other clouds. If anyone could help with that, it would be great!

Issue reference

#1103 (doesn't close the issue, but it's part of that larger story)

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation: N/A

@berndverst
Copy link
Member

Thanks for the PR @ItalyPaleAle. I will review!

@ItalyPaleAle
Copy link
Contributor Author

Two notes after a code review:

  1. In track 2 SDKs, the APIs to control the telemetry data have changed. While this is specific to each component, it's not possible (or at least, not straightforward) to set the full User Agent anymore. Instead, the APIs expose an "ApplicationID" field that can be used to add a string of up to 24 characters. Currently I've set that to ""dapr-" + logger.DaprVersion which is the value of the old User-Agent. However the SDKs now append their own string to that, so the User-Agent that is sent will be something like dapr-1.4.0 azsecrets/v1.0.0 which includes the version of the SDK. If this is undesirable I can make the change to match the old behavior.
  2. In the old version, there were no timeouts and the component was using context.Background() everywhere. This is probably not desirable: a blocked network request would now hang the goroutine forever. The better option would be to use a context with a timeout. Speaking with @berndverst, he recommended not making this change now, so I've used context.TODO() instead for now

@berndverst
Copy link
Member

Thanks @ItalyPaleAle. Dapr will have to revisit the proper handling and setting of context throughout components as part of resiliency feature plans.

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #1290 (3ffc512) into master (09fb60c) will decrease coverage by 0.20%.
The diff coverage is 27.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1290      +/-   ##
==========================================
- Coverage   35.02%   34.82%   -0.21%     
==========================================
  Files         148      148              
  Lines       12825    12899      +74     
==========================================
  Hits         4492     4492              
- Misses       7853     7923      +70     
- Partials      480      484       +4     
Impacted Files Coverage Δ
bindings/azure/eventgrid/eventgrid.go 3.84% <0.00%> (ø)
authentication/azure/auth.go 40.82% <2.04%> (-15.27%) ⬇️
secretstores/azure/keyvault/keyvault.go 36.25% <33.33%> (+2.00%) ⬆️
pubsub/rabbitmq/rabbitmq.go 57.14% <46.03%> (-7.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad433a2...3ffc512. Read the comment docs.

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

Thanks for this PR enabling the Track 2 auth / identity libraries in addition to the existing ones.

I verified these additions (and existing tests) work. I updated my KeyVault certification tests which validate all auth mechanisms to run against this PR. The tests pass.

@berndverst
Copy link
Member

I also manually verified that managed identity works in production with these changes!

@berndverst
Copy link
Member

@greenie-msft @paulyuk you will enjoy this PR as this adds support for the Track 2 Identity libraries / the AAD auth mechanisms required for all Track 2 SDKs.

@berndverst
Copy link
Member

cc @artursouza this is ready for your review

@artursouza
Copy link
Member

LGTM overall, just one concern about using env variables in unit testing.

@artursouza artursouza merged commit 3eafb8b into dapr:master Nov 16, 2021
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amitm@at-bay.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 10, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amit.mor@hotmail.com>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 10, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Amit Mor <amit.mor@hotmail.com>
jigargandhi pushed a commit to jigargandhi/components-contrib that referenced this pull request Dec 12, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: jigargandhi <jigarr.gandhi@gmail.com>
beiwei30 pushed a commit to beiwei30/components-contrib that referenced this pull request Dec 14, 2021
…apr#1290)

* Authentication for new Azure SDK

* Updated keyvault to use new Azure SDK

* 🙈

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Update authentication/azure/auth.go

* Reintroduce changes from PR 1132 without updating ASB

* Lint 💄

* Marking contexts as TODO as they'll need a timeout
As per conversation with @berndverst

* Update certification tests with no auth libraries

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Ian Luo <ian.luo@gmail.com>
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.

None yet

3 participants