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

when msi is enabled, use a msi authorizer to fetch the storage accoun… #4100

Closed

Conversation

guojing013214
Copy link

@guojing013214 guojing013214 commented Apr 22, 2021

What this PR does:
In some Azure environments, sensitive information such as Storage Account Key is not allowed in the application configuration file. Azure recommends using MSI to obtain the read and write permissions of the Storage Account.

Three config options are added to BlobStorageConfig.
MSIEnabled: whether to use msi to access storage account
MSIResource: The Azure Management URI
ResourceGroupName: the resource group name which the target storage account belong to
SubscriptionId: the subscription id where current env belong to

Imported two denpendencies: azure-sdk-for-go and go-autorest

With the above changes, use a MSI authorizer to interact with storage account and get the key of it. Then update this key into the original field. Thus I think we can archive the goal with minimal cost.

Checklist

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

@guojing013214 guojing013214 force-pushed the support-azuremsi branch 5 times, most recently from 84a026a to eb87dde Compare April 23, 2021 02:07
@simonswine
Copy link
Contributor

I think having MSI is a valuable feature for people on Azure.

Just to make you aware that the chunk client is only one of the two object store clients in cortex and it is only used for chunk storage. When block storage is used we are actually using the client part of the thanos project.

I would suggest we align the configuration options closely to thanos-io/thanos#3970

…t key

Signed-off-by: guojing013214 <jing.guo@kyligence.io>
@guojing013214
Copy link
Author

guojing013214 commented May 13, 2021

I align the configuration and set the msi switch to MSIResource, to use MSI, at least two aditional config need to be specify: MSIResource and Resource group name(haven't found a simple way to get the resource group name XD)
Also try to automatically to get the subscription id if user did not specify.
@simonswine

@pracucci pracucci added the storage/chunks Chunks storage engine label May 31, 2021
@pracucci
Copy link
Contributor

Just to make you aware that the chunk client is only one of the two object store clients in cortex and it is only used for chunk storage. When block storage is used we are actually using the client part of the thanos project.

@guojing013214 Sorry for reiterating on this point but I would like to confirm you're actually using Cortex with the (old) chunks storage and not the (new) blocks storage. The changes done here affect only the Cortex chunks storage: are you running it?

@guojing013214
Copy link
Author

guojing013214 commented Jun 7, 2021

@pracucci yes, I am running the (old) chunks storage by Loki, here is my case: I am running Loki on Azure, and using azure storage account to save the chunks. But I need MSI to accomplish the authentication in Azure and also want to keep up with the version of open source, that's why I commit this PR.

About the (new) blocks storage, could you please tell me where the code located(Are you suggesting to support MSI in this feature too), I'm a new guy to Loki and Cortex, but I would like to contribute in my spare time.

Or if you have better plan to support MSI in Loki(Loki is using cortex to interact with storage account), I will be very grateful and pleased to follow up(I'm not quite sure if the way in This PR is the best way).

@pracucci
Copy link
Contributor

We've deprecated the chunks storage in Cortex and Loki is forking it into their own repo (see grafana/loki#3842). I would suggest to discuss this change with the Loki community as soon as grafana/loki#3842 is merged.

@pracucci pracucci closed this Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
storage/chunks Chunks storage engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants