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.

Sign up for GitHub

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

Jump to bottom

Add support for Azure storage account user assigned identity #5891

Merged
merged 1 commit into from
May 30, 2022
Merged

Add support for Azure storage account user assigned identity #5891

merged 1 commit into from
May 30, 2022

Conversation

stevehipwell
Copy link
Contributor

What this PR does / why we need it:

This PR adds support for authenticating to an Azure storage account via a user defined identity and is based on the Thanos implementation.

This PR also tidies up the existing code for Azure storage account and makes the documentation easier to understand.

Which issue(s) this PR fixes:
Fixes #5710.

Special notes for your reviewer:
I haven't tested this in Azure yet as I'd like a review in principal first.

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@stevehipwell stevehipwell requested review from KMiller-Grafana and a team as code owners April 12, 2022 14:36
@CLAassistant
Copy link

CLAassistant commented Apr 12, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@stevehipwell stevehipwell force-pushed the azure-user-assigned-id branch 3 times, most recently from 3a745d1 to 19ed883 Compare April 13, 2022 08:16
chaudum
chaudum reviewed Apr 13, 2022
View reviewed changes
Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

This PR looks good, thanks for the contribution! I added a few minor comments.

pkg/storage/bucket/azure/config_test.go Outdated Show resolved Hide resolved
pkg/storage/chunk/client/azure/blob_storage_client.go Outdated Show resolved Hide resolved
pkg/storage/chunk/client/azure/blob_storage_client.go Show resolved Hide resolved
pkg/storage/bucket/azure/config.go Outdated Show resolved Hide resolved
@stevehipwell stevehipwell force-pushed the azure-user-assigned-id branch from 19ed883 to 7bad83c Compare April 13, 2022 09:39
chaudum
chaudum approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again for the contribution! I don't think a changelog entry is necessary for this small change, is it @slim-bean ?

@stevehipwell
Copy link
Contributor Author

@chaudum I wasn't sure if your CHANGELOG was auto generated or not, if it's manual I'm happy to add something as this is likely a useful change and an aid to adoption in Azure.

@stevehipwell
Copy link
Contributor Author

@chaudum are you waiting on anything from me?

@cyriltovena cyriltovena enabled auto-merge (squash) May 2, 2022 08:21
@cyriltovena
Copy link
Contributor

Let's just merge conflict.

@stevehipwell stevehipwell mentioned this pull request May 3, 2022
Merged
@stevehipwell
Copy link
Contributor Author

@cyriltovena I need to wait for a reply to #6015 (comment) before I can rebase this branch but as soon as i hear back I'll get it back into a mergeable state.

auto-merge was automatically disabled May 6, 2022 16:41

Head branch was pushed to by a user without write access

@stevehipwell stevehipwell force-pushed the azure-user-assigned-id branch from 7bad83c to 9c31812 Compare May 6, 2022 16:41
@stevehipwell
Copy link
Contributor Author

@cyriltovena I've rebased assuming that the changes made by @trevorwhitney in #6015 are correct even though I still haven't heard back.

CC @owen-d

KMiller-Grafana
KMiller-Grafana suggested changes May 6, 2022
View reviewed changes
Copy link
Contributor

@KMiller-Grafana KMiller-Grafana left a comment

Choose a reason for hiding this comment

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

Small changes to capitalization needed.

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
docs/sources/storage/_index.md Outdated Show resolved Hide resolved
@stevehipwell stevehipwell force-pushed the azure-user-assigned-id branch from 9c31812 to cdd65b2 Compare May 9, 2022 07:40
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@stevehipwell
Copy link
Contributor Author

@cyriltovena how are we looking for getting this merged?

@stevehipwell
Copy link
Contributor Author

@KMiller-Grafana could you update your approval or let me know if there is anything else required?

KMiller-Grafana
KMiller-Grafana approved these changes May 20, 2022
View reviewed changes
Copy link
Contributor

@KMiller-Grafana KMiller-Grafana 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 docs changes.

@stevehipwell
Copy link
Contributor Author

@cyriltovena is there anything else needed before we can get this merged?

@stevehipwell
Copy link
Contributor Author

@chaudum what do we need to do to get this merged?

@chaudum
Copy link
Contributor

chaudum commented May 30, 2022

Hey @stevehipwell sorry for letting you wait. Unfortunately, I don't have the permissions to merge, but I'll escalate it :)

@kavirajk kavirajk merged commit 7cc6aa4 into grafana:main May 30, 2022
sandeepsukhani
sandeepsukhani approved these changes May 30, 2022
View reviewed changes
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

LGTM

@stevehipwell stevehipwell deleted the azure-user-assigned-id branch May 30, 2022 09:52
@marc-sensenich marc-sensenich mentioned this pull request May 31, 2022
Merged
3 tasks
This was referenced Jun 28, 2022
Closed
Draft
@stevehipwell
Copy link
Contributor Author

@chaudum is there an ETA for when these changes will be released? Also is there a pre-release version which would have these changes in it?

@stevehipwell
Copy link
Contributor Author

@slim-bean it looks like v2.6.0 has this feature in it but it doesn't appear in the CHANGELOG?

@osg-grafana osg-grafana added type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories and removed area/docs labels Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KMiller-Grafana KMiller-Grafana KMiller-Grafana approved these changes

@kavirajk kavirajk kavirajk approved these changes

@sandeepsukhani sandeepsukhani sandeepsukhani approved these changes

@chaudum chaudum chaudum approved these changes

Assignees
No one assigned
Labels
size/L type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for Azure user assigned managed identity
9 participants
@stevehipwell @CLAassistant @cyriltovena @grafanabot @chaudum @sandeepsukhani @kavirajk @KMiller-Grafana @osg-grafana