Azure AD authentication support (and SDK upgrade) (#9)

* A refactor of the `azure` module to support authenticating via either Azure AD or shared keys. This required a larger-than-expected series of changes because the Azure SDK now very different than the one stow used before. This is an early commit to get changes out there for discussion. Included here is:

- An upgrade of the Azure SDK used for the project (needed to support AD auth). This, unfortunately also brought in a whole series if indirect upgrades so testing will be needed.
- Support for authenticating via either Azure AD or Shared keys
- Removal of several azure config values (ConfigAPIVersion, ConfigUseHTTPS) and addition of ConfigUploadConcurrency.
- Removal of the multi-part upload code for Azure, this is now included in the SDK
- Addition of new presigned url tests into the general stow test/ module.
- Addition of a new terraform module to stand up an azure storage account for testing.

Signed-off-by: Terence Kent <terence.kent@mcg.com>

* fix misspellings (doh)

Signed-off-by: Terence Kent <terence.kent@mcg.com>

* Update azure/config.go

Accepting len(var)==0 vs var==""

Co-authored-by: Haytham Abuelfutuh <haytham@afutuh.com>
Signed-off-by: Terence Kent <terence.kent@mcg.com>

* Update azure/config.go

Accepting improved error message

Co-authored-by: Haytham Abuelfutuh <haytham@afutuh.com>
Signed-off-by: Terence Kent <terence.kent@mcg.com>

* Changes before moving PR from draft. No impact to tests or functionality at this point:

- Removed all inline context.Background() calls when using Azure SDK methods. Now set an explicit `ctx` variable at the top of each function.
- Added comments around the clock skew code, and moved all clock skews to 15 minutes (the azure-recommended value). Also left a comment explaining the decision.
- Made the use of removed config values an error state for validation. Added a test to ensure this happens.
- Moved some errant logging in the `checkMetadata` test method to only show up in inequality, to keep test output clean.

Signed-off-by: Terence Kent <terence.kent@mcg.com>

* Missed that the previous version of the Azure implementation silently ignored failures due to creating an already-existing container! The code from the old SDK to ignore "already exists" was preserved, but not working with the new SDK.

- Updated the check to work with the new SDK.
-  Added a general test to confirm this behavior.

Note: It's debatable if this should be the default behavior for all stow implementations. However, it seems like whatever the desired behavior is, it should be consistent across all implementations.
Signed-off-by: Terence Kent <terence.kent@mcg.com>

---------

Signed-off-by: Terence Kent <terence.kent@mcg.com>
Co-authored-by: Terence Kent <terence.kent@mcg.com>
Co-authored-by: Haytham Abuelfutuh <haytham@afutuh.com>

Author: 3 people

.gitignore

@@ -30,3 +30,4 @@ tests.xml
 vendor/
 
 .vscode/
+.terragrunt-cache

azure/config.go

@@ -2,74 +2,82 @@ package azure
 
 import (
 	"errors"
+	"fmt"
+	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
+	"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob"
 	"net/url"
 	"strconv"
 
-	"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob"
-
-	az "github.com/Azure/azure-sdk-for-go/storage"
 	"github.com/flyteorg/stow"
 )
 
 // ConfigAccount should be the name of your storage account in the Azure portal
 // ConfigKey should be an access key
-// ConfigBaseUrl is the base URL of the cloud you want to connect to. The default
-// is Azure Public cloud
-// ConfigAPIVersion is the Azure Storage API version string used when a
-// client is created.
-// ConfigUseHTTPS specifies whether you want to use HTTPS to connect
+// ConfigDomainSuffix the domain suffix to use for storage account communication. The default is the Azure Public cloud
+// ConfigUploadConcurrency the upload concurrency to use when uploading. Default is 4.
+// ConfigBaseUrlDepreciated Kept for backwards compatability, use ConfigDomainSuffix instead
 const (
-	ConfigAccount    = "account"
-	ConfigKey        = "key"
-	ConfigBaseUrl    = "base_url"
-	ConfigAPIVersion = "api_version"
-	ConfigUseHTTPS   = "use_https"
+	ConfigAccount            = "account"
+	ConfigKey                = "key"
+	ConfigDomainSuffix       = "domain_suffix"
+	ConfigUploadConcurrency  = "upload_concurrency"
+	ConfigBaseUrlDepreciated = "base_url"
 )
 
+// Removed configuration values, will cause failures if used.
+const (
+	ConfigUseHttpsRemoved   = "use_https"
+	ConfigApiVersionRemoved = "api_version"
+)
+
+var removedConfigKeys = []string{ConfigUseHttpsRemoved, ConfigApiVersionRemoved}
+
 // Kind is the kind of Location this package provides.
 const Kind = "azure"
-const defaultBaseUrl = "core.windows.net"
-const defaultAPIVersion = "2018-03-28"
-const defaultHTTPSStr = "true"
+
+// defaultDomainSuffix is the domain suffix for the Azure Public Cloud
+const defaultDomainSuffix = "core.windows.net"
+
+// defaultUploadConcurrency is the default upload concurrency
+const defaultUploadConcurrency = 4
 
 func init() {
 	validatefn := func(config stow.Config) error {
 		_, ok := config.Config(ConfigAccount)
 		if !ok {
 			return errors.New("missing account id")
 		}
-		_, ok = config.Config(ConfigKey)
-		if !ok {
-			return errors.New("missing auth key")
+		for _, removedConfigKey := range removedConfigKeys {
+			_, ok = config.Config(removedConfigKey)
+			if ok {
+				return fmt.Errorf("removed config option used [%s]", removedConfigKey)
+			}
 		}
 		return nil
 	}
 	makefn := func(config stow.Config) (stow.Location, error) {
-		_, ok := config.Config(ConfigAccount)
+		acctName, ok := config.Config(ConfigAccount)
 		if !ok {
 			return nil, errors.New("missing account id")
 		}
-		_, ok = config.Config(ConfigKey)
-		if !ok {
-			return nil, errors.New("missing auth key")
-		}
-		l := &location{
-			config: config,
-		}
 
-		acc, key, env, api, https, err := getAccount(l.config)
-		if err != nil {
-			return nil, err
+		var uploadConcurrency int
+		var err error
+		uploadConcurrencyStr, ok := config.Config(ConfigUploadConcurrency)
+		if !ok || len(uploadConcurrencyStr) == 0 {
+			uploadConcurrency = defaultUploadConcurrency
+		} else {
+			uploadConcurrency, err = strconv.Atoi(uploadConcurrencyStr)
+			if err != nil {
+				return nil, fmt.Errorf("invalid upload concurrency [%v]", uploadConcurrency)
+			}
 		}
-
-		l.account = acc
-
-		l.client, err = newBlobStorageClient(acc, key, env, api, https)
-		if err != nil {
-			return nil, err
+		l := &location{
+			accountName:       acctName,
+			uploadConcurrency: uploadConcurrency,
 		}
 
-		l.sharedCreds, err = azblob.NewSharedKeyCredential(acc, key)
+		l.client, l.preSigner, err = makeAccountClient(config)
 		if err != nil {
 			return nil, err
 		}
@@ -88,50 +96,69 @@ func init() {
 	stow.Register(Kind, makefn, kindfn, validatefn)
 }
 
-func getAccount(cfg stow.Config) (account, key string, baseUrl string, APIVersion string, useHTTPS bool, err error) {
-	acc, ok := cfg.Config(ConfigAccount)
+// makeAccountClient is a factory function for producing client instances
+func makeAccountClient(cfg stow.Config) (*azblob.Client, RequestPreSigner, error) {
+	accountName, ok := cfg.Config(ConfigAccount)
 	if !ok {
-		return "", "", "", "", false, errors.New("missing account id")
+		return nil, nil, errors.New("missing account id")
 	}
 
-	key, ok = cfg.Config(ConfigKey)
-	if !ok {
-		return "", "", "", "", false, errors.New("missing auth key")
-	}
-
-	baseUrl = getBaseAzureUrlOrDefault(cfg)
+	domainSuffix := resolveAzureDomainSuffix(cfg)
+	serviceUrl := fmt.Sprintf("https://%s.blob.%s", accountName, domainSuffix)
 
-	APIVersion, ok = cfg.Config(ConfigAPIVersion)
-	if !ok {
-		APIVersion = defaultAPIVersion
+	key, ok := cfg.Config(ConfigKey)
+	if ok && key != "" {
+		return newSharedKeyClient(accountName, key, serviceUrl)
 	}
+	return newDefaultAzureIdentityClient(serviceUrl)
+}
 
-	var useHTTPSStr string
-	useHTTPSStr, ok = cfg.Config(ConfigUseHTTPS)
-	if !ok {
-		useHTTPSStr = defaultHTTPSStr
+// newSharedKeyClient creates client objects for working with a storage account
+// using shared keys.
+func newSharedKeyClient(accountName, key, serviceUrl string) (*azblob.Client, RequestPreSigner, error) {
+	sharedKeyCred, err := azblob.NewSharedKeyCredential(accountName, key)
+	if err != nil {
+		return nil, nil, err
 	}
-	useHTTPS, err = strconv.ParseBool(useHTTPSStr)
+	client, err := azblob.NewClientWithSharedKeyCredential(
+		serviceUrl,
+		sharedKeyCred,
+		nil)
 	if err != nil {
-		return "", "", "", "", false, errors.New("invalid value for use_https_str")
+		return nil, nil, err
 	}
-	return acc, key, baseUrl, APIVersion, useHTTPS, nil
+	preSigner, err := NewSharedKeyRequestPreSigner(accountName, key)
+	if err != nil {
+		return nil, nil, err
+	}
+	return client, preSigner, nil
 }
 
-func getBaseAzureUrlOrDefault(cfg stow.Config) string {
-	baseUrl, ok := cfg.Config(ConfigBaseUrl)
-	if !ok || baseUrl == "" {
-		baseUrl = defaultBaseUrl
+// newDefaultAzureIdentityClient creates client objects for working with a storage
+// account using Azure AD auth, resolved using the default Azure credential chain.
+func newDefaultAzureIdentityClient(serviceUrl string) (*azblob.Client, RequestPreSigner, error) {
+	cred, err := azidentity.NewDefaultAzureCredential(nil)
+	if err != nil {
+		return nil, nil, err
 	}
-	return baseUrl
+	client, err := azblob.NewClient(serviceUrl, cred, nil)
+	if err != nil {
+		return nil, nil, err
+	}
+	preSigner, err := NewDelegatedKeyPreSigner(client.ServiceClient())
+	return client, preSigner, nil
 }
 
-func newBlobStorageClient(account, key string, baseUrl string, APIVersion string, useHTTPS bool) (*az.BlobStorageClient, error) {
-	basicClient, err := az.NewClient(account, key, baseUrl, APIVersion, useHTTPS)
-	if err != nil {
-		return nil, errors.New("bad credentials")
+// resolveAzureDomainSuffix returns the Azure domain suffix to use
+func resolveAzureDomainSuffix(cfg stow.Config) string {
+	domainSuffix, ok := cfg.Config(ConfigDomainSuffix)
+	if ok && domainSuffix != "" {
+		return domainSuffix
 	}
 
-	client := basicClient.GetBlobService()
-	return &client, err
+	domainSuffix, ok = cfg.Config(ConfigBaseUrlDepreciated)
+	if ok && domainSuffix != "" {
+		return domainSuffix
+	}
+	return defaultDomainSuffix
 }