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

Add support for Azure Storage Account managed identities #1457

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

marc-sensenich
Copy link
Contributor

What this PR does:

This PR adds support for authenticating to the Azure backend with either a system or user defined managed identity and is based on grafana/loki#5891

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:
I've tested that Tempo is still able to flush blocks to the Azure Storage Account when using a user-assigned identity and when the Storage Account has access keys disabled.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented May 31, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Nice addition. Thanks for the PR! I had a few questions throughout the change that would be helpful to get some answers on. Other thoughts:

  • I'm unfamiliar with the auth system you're integrating with. If you had any links to help me understand what this code is supposed to do it will help me review and accept.
  • Thank you for the docs change! Nice work!
  • We will need a changelog entry to get this merged. I would call this an ENHANCEMENT
  • Testing code like this can be notoriously difficult, but any tests you could add would be valuable. Functions like getStorageAccountName would be a good candidate. I understand that testing the integration with the auth would be quite difficult.

tempodb/backend/azure/azure_helpers.go Show resolved Hide resolved
tempodb/backend/azure/azure_helpers.go Show resolved Hide resolved
tempodb/backend/azure/azure_helpers.go Show resolved Hide resolved
@marc-sensenich
Copy link
Contributor Author

@joe-elliott In regards to your point

I'm unfamiliar with the auth system you're integrating with. If you had any links to help me understand what this code is supposed to do it will help me review and accept.

Here's the documentation from Microsoft surrounding Managed Identities for your reading https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/overview. And then documentation around using Azure AD for accessing Azure Storage https://docs.microsoft.com/en-us/azure/storage/blobs/authorize-access-azure-active-directory. The key take away is that it allows for applications to authenticate with Azure AD to access Azure AD protected resources without having static credentials or having Ops teams having to manage static credentials.

To put it concretely in terms of Tempo, Tempo would no longer need to have access to an Azure Storage Account access key to access Azure Storage and would instead be able to use Azure AD auth to align with Microsoft's recommendation https://docs.microsoft.com/en-us/azure/storage/common/storage-account-keys-manage?tabs=azure-portal#protect-your-access-keys for block streaming. This would then enable Ops teams to then disable access keys and use only Azure AD auth for Storage Accounts that Tempo is using.

@marc-sensenich
Copy link
Contributor Author

Test cases for getStorageAccountName and getStorageAccountKey added in 33ebaef

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Nice addition! Appreciate the feature as well as the detailed and quick responses.

@joe-elliott joe-elliott merged commit 93a1359 into grafana:main Jun 2, 2022
@marc-sensenich
Copy link
Contributor Author

You're welcome @joe-elliott and thank you as well for your quick review on this!

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.

3 participants