blob/azureblob: add azure cli authorizer and URL params

[ekini]

Fixes #3012

First of all this PR adds a new variable AZURE_BLOB_AUTH_VIA_CLI to enable authorisation using CLI.
Secondly, to make it useful, it adds 2 URL parameters, similar to #3008, to specify storage account name and subscription id.

A reminder needs to be put somewhere that Blob authorisation via CLI requires "Storage Blob Data Contributor" role (it seems like it requires this specific role even if you've got "Contributor" already).

I've chosen AZURE_BLOB_AUTH_VIA_CLI as the name of the env var to so it doesn't look like one of the standard Azure env vars.

[codecov]

Codecov Report

Merging #3072 (ae46ce1) into master (bb89706) will increase coverage by 0.51%.
The diff coverage is 79.66%.

@@            Coverage Diff             @@
##           master    #3072      +/-   ##
==========================================
+ Coverage   71.89%   72.40%   +0.51%     
==========================================
  Files         112      112              
  Lines       13853    14216     +363     
==========================================
+ Hits         9959    10293     +334     
- Misses       3195     3210      +15     
- Partials      699      713      +14     

Impacted Files	Coverage Δ
blob/azureblob/azureblob.go	77.85% <79.66%> (-0.17%)	⬇️
internal/retry/retry.go	88.23% <0.00%> (-11.77%)	⬇️
blob/s3blob/s3blob.go	90.62% <0.00%> (+0.08%)	⬆️
aws/aws.go	88.05% <0.00%> (+1.49%)	⬆️
docstore/docstore.go	89.56% <0.00%> (+1.79%)	⬆️
server/health/sqlhealth/sqlhealth.go	80.95% <0.00%> (+19.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb89706...ae46ce1. Read the comment docs.

[ekini]

Hmm, that failing mac os test seems to be flaky?

[vangent]

Can you sync to HEAD and try again?

[ekini]

Ok, that helped. Also I renamed account_name query parameter to "storage_account" to show that works the same as AZURE_STORAGE_ACCOUNT.
Previously I took account_name because it's AccountName in the code :)

[vangent]

Sorry, I don't like that you've changed this whole function from running once to running every time a URL is opened. Why is that? IIUC, it's because you added some URL parameters that affect the account being used. We generally have avoided doing that in favor of using environment variables, see some discussion here:
https://gocloud.dev/concepts/urls/#muxes

[ekini]

Oh, there is a reason behind it.
If we compare before and after:

1. Before, to open Azure Blob Storage bucket (account/container in Azure terms), one would need to set env vars AZURE_STORAGE_ACCOUNT/AZURE_STORAGE_KEY or AZURE_STORAGE_CONNECTION_STRING, which can obviously be set only once for a program. Previously it wasn't possible to use Azure AD authorization to access blob storage. But it is possible now, according to the doc.
2. Now, since it's possible to be authorized using CLI, one can open several storage accounts in one program, using different storage_account URL parameter, or even use different subscriptions (or a set one, because otherwise it will use the default subscription which is not what everyone wants).

If we keep o.init.Do it will be surprising for users when they'll try to use different subscriptions or accounts but the lib will always use the first one.

[vangent]

OK, I get it. This is a major change though; the whole point of the lazyCredsOpener is that it opens a single session, once. If you get rid of the init.Do and run everything every time, there is no point in that whole struct anymore, might as well do everything in the actual opener.

I'm not sure that's a good idea either though.

Sadly, Azure authorization seems super complex, people keep making contributions that add new ways to do it, so this code is pretty hairy at this point and I would need to take quite some time to understand it again.

It's possible that we should not do this, and instead say that if you want to use multiple Azure accounts, you need to either create your own URLOpener for them, or you need to use the non-URL openers.

[ekini]

I've taken some time to think about it and haven't been able to figure out a better way.

I've basically renamed lazyCredsOpener to routerCredsOpener. It is possible to encapsulate that logic into func (o *URLOpener) OpenBucketURL but that would make it super-complex.

I wouldn't keep init.Do either, because intuitively users of the library would expect to use multiple Azure accounts (subscriptions in Azure terms) if they supply different subscription_id parameters.

Additionally, just for extra justification of the added complexity and major changes:
Currently an azure blob URL looks like azblob://container that doesn't provide full information about the object you want to open, because containers live in storage accounts, which live in subscriptions. So that information has to be passed using different ways.
After the PR it will look like azblob://container?storage_account=account&subscription_id=xxx-xxx-xxx which is enough to figure out how to access the storage container.

[vangent]

Hi ekini, thanks for the PR and thinking about this.

However, I think I'm going to stick with my intuition above and say that we don't want to support opening multiple storage accounts via changes in the URL. We've avoided putting things like that in URLs on purpose, and instead used environment variables for the most part.

Here's an Azure example from secrets for using the Azure CLI:
https://github.com/google/go-cloud/blob/master/secrets/azurekeyvault/akv.go#L97

It uses an environment variable to toggle it on and off.

If you want/need to connect to different storage accounts/subscriptions, you can either use the regular openers, or create custom URLOpeners (one for each account, by creating the appropriate Pipeline).