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 Azure Workload Identity authentication support #2195

Merged
merged 4 commits into from
Mar 15, 2023
Merged

Add Azure Workload Identity authentication support #2195

merged 4 commits into from
Mar 15, 2023

Conversation

LambArchie
Copy link
Contributor

@LambArchie LambArchie commented Mar 13, 2023

What this PR does:
Adds support for Azure Workload Identity authentication.

Partially based off the following PR grafana/loki#7540 which implemented Azure Workload Identity into Loki.

Checklist

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

This is my first time writing anything in Go, so I might have made a mistake with the syntax. If I have feel free to fix it or let me know and I'll fix it myself.
I have tested this and it seems to work so I'm hoping I haven't made a mistake but you never know.

@CLAassistant
Copy link

CLAassistant commented Mar 13, 2023

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.

Thanks for the PR! I added some thoughts on cleanup. I'm also concerned about changing this functionality as its difficult to test and I am far from an Azure auth expert. We will still be willing to merge, but any testing you can do on your end to feel confident in this change would be appreciated.

tempodb/backend/azure/azure_helpers.go Outdated Show resolved Hide resolved
tempodb/backend/azure/azure_helpers.go Outdated Show resolved Hide resolved
tempodb/backend/azure/azure_helpers.go Outdated Show resolved Hide resolved
@LambArchie
Copy link
Contributor Author

Thanks for the PR! I added some thoughts on cleanup. I'm also concerned about changing this functionality as its difficult to test and I am far from an Azure auth expert. We will still be willing to merge, but any testing you can do on your end to feel confident in this change would be appreciated.

I tested my original changes and it seemed to work, including token renewal as I continued to see changes on the test azure storage account 32 hours after I initially started tempo (tokens are renewed every hour but if not are valid for 24h).

I will test again with these changes to ensure no regression has occurred

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.

LGTM. Nice improvement!

@joe-elliott joe-elliott merged commit 8924977 into grafana:main Mar 15, 2023
@LambArchie LambArchie deleted the azure-workload-identity branch March 15, 2023 14:48
mdisibio pushed a commit to mdisibio/tempo that referenced this pull request Apr 18, 2023
* Add Azure Workload Identity authentication support

* Linting

* Automate getting Azure AD Endpoint

* PR Feedback
@Ivalberto
Copy link

What is the rigth configuration to have tempo working with workload identity ? I receiving failed to created store : getting storage container: open: no such file or directory. My container exist, relation between sa and manage identity also exist

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.

4 participants