Adding Azure Blob support #114
Conversation
|
Thank you for this PR. I'll be taking a look at this soon. |
ahouene
left a comment
ahouene
left a comment
There was a problem hiding this comment.
While this works as intended, there are a couple small quirks that need addressing before merging.
Co-authored-by: Rod <[email protected]>
|
Thanks for the review @ahouene ! And 🤦 on the todos still in the code. I think I've addressed all you comments now, ready for another review |
|
Thanks for the quick reply! I'm trying to get back to you early this week! |
ahouene
left a comment
ahouene
left a comment
There was a problem hiding this comment.
Thanks for the changes! Just a couple last things I see before merging!
backends/azure/azure.go
Outdated
| // DefaultEndpointURL is the default the local Azurite default endpoint | ||
| DefaultEndpointURL = "https://mycontainer.blob.core.windows.net/" |
There was a problem hiding this comment.
The URL includes an arbitrary account name, so it will have no use as a default (except maybe for the one person that has the account mycontainer :)).
I don't think that a constant "default endpoint" value maps well with the way Azure works. Instead, given what I see in the docs I may suggest the following (I'm open to other options if you think of any!): if an endpoint is provided in the options, use it as "${endpoint_url}/${account_name}", else use "https://${account_name}.blob.core.windows.net".
There was a problem hiding this comment.
I ended up removing the defaultendpoint const as we don't actually need it.
|
|
||
| var azuriteContainer *azurite.AzuriteContainer | ||
|
|
||
| func getBackend(ctx context.Context, t *testing.T) (b *Backend) { |
There was a problem hiding this comment.
Since we're in tests and the t parameter is available, t.Log/t.Fatalf should be preferred to fmt.Println/log.Printf.
Luit
left a comment
Luit
left a comment
There was a problem hiding this comment.
Hi, I've been looking over my colleagues shoulder, I hope you don't mind me barging in with another review. Just two more things I want to drop in here.
backends/azure/azure.go
Outdated
| // If UseEnvCreds is set, we will attempt to use the environment variables and the Azure service principle based `azidentity.NewDefaultAzureCredential()` method | ||
| // https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azidentity/README.md#service-principal-with-secret | ||
| if opt.UseEnvCreds { | ||
| // Test if the environment variables are set | ||
| _, ok := os.LookupEnv("AZURE_CLIENT_ID") | ||
| if !ok { | ||
| return nil, errors.New("AZURE_CLIENT_ID could not be found") | ||
| } | ||
|
|
||
| _, ok = os.LookupEnv("AZURE_TENANT_ID") | ||
| if !ok { | ||
| return nil, errors.New("AZURE_TENANT_ID could not be found") | ||
| } | ||
|
|
||
| _, ok = os.LookupEnv("AZURE_CLIENT_SECRET") | ||
| if !ok { | ||
| return nil, errors.New("AZURE_CLIENT_SECRET could not be found") | ||
| } | ||
|
|
||
| cred, err := azidentity.NewDefaultAzureCredential(nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| client, err = azblob.NewClient(endpoint, cred, nil) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| cred, err := azblob.NewSharedKeyCredential(accountName, opt.AccountKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| client, err = azblob.NewClientWithSharedKeyCredential(endpoint, cred, nil) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
To my (Azure-)untrained eye, this seems convoluted. From what I can glean from the azidentity documentation linked in the comment, and a quick look at further documentation of authentication for Azure Blob Storage, I'd prefer this to be setup differently. I think the default way to authenticate should be to use azidentity.NewDefaultAzureCredential, without any checking of environment variables up front to allow it to use other kinds of credentials as well. A selectable alternative should be the azblob-specific Shared Key credential.
There was a problem hiding this comment.
To clarify: I mean having a UseSharedKey (or UseSharedKeyCredential) option instead of an UseEnvCreds, and not checking the env vars because DefaultAzureCredential can do much more than just this env variant.
backends/azure/azure.go
Outdated
| if o.Container == "" { | ||
| return fmt.Errorf("azure storage.options: container is required") | ||
| } |
There was a problem hiding this comment.
With the (Options).UseEnvCreds short-circuit above, this check isn't happening in that case. Should this check be done first?
Co-authored-by: Luit van Drongelen <[email protected]>
3 participants
This pull request adds comprehensive support for Azure Blob Storage as a backend for the project, including implementation, metrics, and thorough testing. The changes introduce new code for Azure integration, update documentation and dependencies, and provide conformance and streaming tests to ensure reliability and feature parity with existing storage backends.