#9 Azure AD authentication support (and SDK upgrade) Pull request opened by tkent What's here This PR adds support for using Azure AD authentication to the

azure

module. About the changes The latest version of the azure golang SDK was needed for this and, unfortunately, this required a very large set of changes. The

azure

module previously used the

storage

SDK, which has been removed, and the new SDK is very different. The key changes included are: • Replacement of nearly all the Azure SDK interactions. • Addition of a new interface to support presigned urls using either authentication source (see

RequestPreSigner

) • Removal of the multi-part upload support (this is now handled in the newer SDK) • (BREAKING) Removal of the

use_https

and

api_version

configuration options (more on that below) • Addition of a new

upload_concurrency

configuration option. • Rename of the config key

base_url

to

domain_suffix

. • Addition of new general stow tests for presigned requests (

ContainerPreSignRequest

) • Addition of a new general stow test for big files (

BigFileUpload

) Things to talk about The Azure SDK changes forced a few decisions that I want to call out: The removal of

use_https

This just seems like a misconfiguration risk with no clear benefit. MSFT recommends using HTTPS instead of HTTP for storage account access (here) and I can't imagine a scene where

http

would be preferred. The removal of

api_version

Newer versions of the SDK do not really support changing the API version. If you check the SDK code you see this comment:

Set with caution as this package version has not been tested with arbitrary service versions.

Again, this seems like a misconfiguration risk without a clear benefit. Presigned Requests and the shared keys disabled flag When shared keys are disabled on a storage account, you might expect presigned keys to be blocked as well. However, "user delegated SAS tokens" will continue to work even in this case (docs here). There is just one big caveat. The Azure AD identity used for authentication to the storage account must have the

Microsoft.Storage/storageAccounts/blobServices/generateUserDelegationKey

privilege (docs here). Performance of the Presigned Request implementation when using AD auth The current presigned request implementation for AD auth will not perform well if a large volume of requests are needed. Each pre-signing requires a distinct delegate key which is an API call to Azure. This could be mitigated, though not easily, by obtaining a long-lived delegate key and renewing it as necessary. I opted not to include that, assuming large volumes of pre-signed requests are unlikely. Lack of

context

in the stow interfaces The methods of the stow interfaces generally do not include a

context.Context

in their argument signature. The newer azure SDK, however, requires a

context.Context

for nearly every operation. I chose to use

context.Background

every time this came up, but

context.TODO

would have been just as appropriate. Testing A big challenge for this particular change is testing. To make it a bit easier for a reviewer to kick the tires, a terraform module was added under the

tests/

directory. In there is a readme for using the module to stand up and teardown a storage account. Unfortunately, even with the fixture and existing stow tests, there is quite a bit of real-world testing this SDK swap will need. Update I've put together some instructions for testing this change end to end within a local

flyte

build using the master branch of

flyteorg/flyte

. See this gist. A few things to note if following that guide: • For some reason, when Flyte first initializes the configured blob container in the Azure Storage Account, you will get errors (appears to be a race condition). The container is successfully created though. I haven't looked into this. • If you delete the storage container Flyte is using (for example, to start over) the Azure APIs will return errors trying to make a new container with the same name for a few minutes. This will clear up once the container is truly deleted, usually within 5 minutes. flyteorg/stow ✅ All checks have passed 2/2 successful checks