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

Allow environment variables for Azure storage credentials #1147

Merged
merged 6 commits into from
Dec 8, 2021

Conversation

zalegrala
Copy link
Contributor

@zalegrala zalegrala commented Dec 1, 2021

What this PR does:
Allow default values for Azure storage to be pulled from the environment.
Update examples and operations to use the newer backend-agnostic metric name.

Checklist

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

@@ -47,8 +48,8 @@ func (cfg *Config) RegisterFlagsAndApplyDefaults(prefix string, f *flag.FlagSet)
cfg.Trace.Block.SearchPageSizeBytes = 1024 * 1024 // 1 MB

cfg.Trace.Azure = &azure.Config{}
f.StringVar(&cfg.Trace.Azure.StorageAccountName.Value, util.PrefixConfig(prefix, "trace.azure.storage-account-name"), "", "Azure storage account name.")
f.StringVar(&cfg.Trace.Azure.StorageAccountKey.Value, util.PrefixConfig(prefix, "trace.azure.storage-account-key"), "", "Azure storage access key.")
f.StringVar(&cfg.Trace.Azure.StorageAccountName.Value, util.PrefixConfig(prefix, "trace.azure.storage-account-name"), os.Getenv("AZURE_STORAGE_ACCOUNT_NAME"), "Azure storage account name.")
Copy link
Member

Choose a reason for hiding this comment

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

Does the Azure client not pick these up automatically? The AWS client will check all the standard methods of providing auth including env vars if they are not explicitly provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look again because that would be my preference also. Doing it here means that the current client implementation can remain the same.

https://github.com/grafana/tempo/blob/main/tempodb/backend/azure/azure_helpers.go#L23

I also looked briefly at updating the client, since the one we are using seems to be deprecated, or at least they are encouraging users to migrate. But I didn't have the context on all the changes in that update since I'm new to Azure and learning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they are loading environment variables in the client, I don't see where. And they don't make it easy to find if they support some. I think the updated client might though.

@joe-elliott
Copy link
Member

joe-elliott commented Dec 1, 2021

Instead of setting the defaults I would prefer checking for values during the creation of the backend. If these two config fields are not set at that time then checking the env vars.

Also, any idea what the standards are here? It seems like the cli uses different env vars:
Azure/azure-cli#5358
and thanos uses different ones as well:
https://github.com/thanos-io/thanos/blob/main/pkg/objstore/azure/azure.go#L425-L426

Also note that this can already be done b/c Tempo can interpolate env vars into the config if you set the --config.expand-env parameter. However, if there is an actual standard here I don't mind supporting it directly.

@zalegrala
Copy link
Contributor Author

https://docs.microsoft.com/en-us/cli/azure/storage?view=azure-cli-latest#az_storage_remove-optional-parameters

Good call on checking the CLI, we can use that our standard.

I saw the config.expand-env bit from some Loki references but it seemed to make the jsonnet a more verbose. I'll test that to find out if that's true and drop the commit from this PR if we don't need it. In the meantime I've updated the PR to use the CLI values. I'll deploy a canary tomorrow.

@zalegrala
Copy link
Contributor Author

Oh to your other point about relocating the loading, I'll follow up on that too. I noticed that if those values aren't set, we could improve the error.

@zalegrala zalegrala force-pushed the azureDeploy branch 4 times, most recently from 320c08b to e1ef0ea Compare December 2, 2021 17:21
@zalegrala zalegrala changed the title Set environment default values for Azure storage Allow environment variables for Azure storage credentials Dec 2, 2021
@zalegrala zalegrala marked this pull request as ready for review December 2, 2021 17:45
@zalegrala
Copy link
Contributor Author

I've moved the environment loading where we make use of the variables. If we'd prefer to move that to the New(), I can do that.

@joe-elliott
Copy link
Member

This is looking really good. I have one more ask and then we'll merge. Can you please add azure docs similar to our S3/GCS docs. You don't need to include all the permissions if you don't know them right now (but I'd ask that you come back and add them in the future):

Link to them here:

The following example shows common options. For further platform-specific information refer to the following:
* [GCS](gcs/)
* [S3](s3/)

and then add a doc similar to this:

https://grafana.com/docs/tempo/latest/configuration/s3/

@zalegrala
Copy link
Contributor Author

I've included some initial docs for what we have currently. I'll also keep them updated as I find new information, but I think that should get us going.

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.

2 participants