Changes before moving PR from draft. No impact to tests or functional…

…ity 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>

azure/config.go

@@ -24,6 +24,14 @@ const (
 	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"
 
@@ -39,13 +47,20 @@ func init() {
 		if !ok {
 			return errors.New("missing account id")
 		}
+		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) {
 		acctName, ok := config.Config(ConfigAccount)
 		if !ok {
 			return nil, errors.New("missing account id")
 		}
+
 		var uploadConcurrency int
 		var err error
 		uploadConcurrencyStr, ok := config.Config(ConfigUploadConcurrency)

azure/container.go

@@ -53,8 +53,8 @@ func (c *container) PreSignRequest(ctx context.Context, method stow.ClientMethod
 
 	sasQueryParams, err := c.preSigner(ctx, sas.BlobSignatureValues{
 		Protocol:      sas.ProtocolHTTPS,
-		StartTime:     time.Now().UTC().Add(time.Minute * -5),
-		ExpiryTime:    time.Now().UTC().Add(params.ExpiresIn),
+		StartTime:     time.Now().UTC().Add(-1 * clockSkewBuffer),
+		ExpiryTime:    time.Now().UTC().Add(params.ExpiresIn + clockSkewBuffer),
 		ContainerName: containerName,
 		BlobName:      blobName,
 		Permissions:   permissions.String(),
@@ -92,9 +92,9 @@ func (c *container) Item(id string) (stow.Item, error) {
 	// Another reasonable alternative would be to just lower case all metadata keys, as is
 	// the case with s3: https://github.com/aws/aws-sdk-go/issues/445
 	//
-
+	//ctx := context.Background()
 	//blobClient := c.client.NewBlobClient(id)
-	//resp, err := blobClient.GetProperties(context.Background(), nil)
+	//resp, err := blobClient.GetProperties(ctx, nil)
 	//if err != nil {
 	//	if strings.Contains(err.Error(), "404") {
 	//		return nil, stow.ErrNotFound
@@ -117,6 +117,7 @@ func (c *container) Item(id string) (stow.Item, error) {
 }
 
 func (c *container) Items(prefix, cursor string, count int) ([]stow.Item, string, error) {
+	ctx := context.Background()
 	options := azcontainer.ListBlobsFlatOptions{
 		Prefix:     &prefix,
 		MaxResults: to.Ptr(int32(count)),
@@ -126,7 +127,7 @@ func (c *container) Items(prefix, cursor string, count int) ([]stow.Item, string
 		options.Marker = &cursor
 	}
 
-	listResp, err := c.client.NewListBlobsFlatPager(&options).NextPage(context.Background())
+	listResp, err := c.client.NewListBlobsFlatPager(&options).NextPage(ctx)
 	if err != nil {
 		return nil, "", err
 	}
@@ -148,6 +149,7 @@ func (c *container) Items(prefix, cursor string, count int) ([]stow.Item, string
 }
 
 func (c *container) Put(name string, r io.Reader, size int64, metadata map[string]interface{}) (stow.Item, error) {
+	ctx := context.Background()
 	mdParsed, err := makeAzureCompatMetadataMap(metadata)
 	if err != nil {
 		return nil, errors.Wrap(err, "unable to create or update Item, preparing metadata")
@@ -158,7 +160,7 @@ func (c *container) Put(name string, r io.Reader, size int64, metadata map[strin
 	var blobProps = &BlobProps{ContentLength: size}
 	f, match := r.(*os.File)
 	if match {
-		resp, err := blockClient.UploadFile(context.Background(), f, &blockblob.UploadFileOptions{
+		resp, err := blockClient.UploadFile(ctx, f, &blockblob.UploadFileOptions{
 			Concurrency: uint16(c.uploadConcurrency),
 			Metadata:    mdParsed,
 		})
@@ -168,7 +170,7 @@ func (c *container) Put(name string, r io.Reader, size int64, metadata map[strin
 		blobProps.ETag = *resp.ETag
 		blobProps.LastModified = *resp.LastModified
 	} else {
-		resp, err := blockClient.UploadStream(context.Background(), r, &blockblob.UploadStreamOptions{
+		resp, err := blockClient.UploadStream(ctx, r, &blockblob.UploadStreamOptions{
 			Concurrency: c.uploadConcurrency,
 			Metadata:    mdParsed,
 		})
@@ -190,17 +192,19 @@ func (c *container) Put(name string, r io.Reader, size int64, metadata map[strin
 }
 
 func (c *container) SetItemMetadata(itemName string, md map[string]string) error {
+	ctx := context.Background()
 	azCompatMap := make(map[string]*string, len(md))
 	for k, v := range md {
 		azCompatMap[k] = &v
 	}
 	_, err := c.client.NewBlobClient(itemName).SetMetadata(
-		context.Background(), azCompatMap, nil)
+		ctx, azCompatMap, nil)
 	return err
 }
 
 func (c *container) RemoveItem(id string) error {
-	_, err := c.client.NewBlobClient(id).Delete(context.Background(), nil)
+	ctx := context.Background()
+	_, err := c.client.NewBlobClient(id).Delete(ctx, nil)
 	return err
 }
 

azure/item.go

@@ -46,7 +46,8 @@ func (i *item) Size() (int64, error) {
 }
 
 func (i *item) Open() (io.ReadCloser, error) {
-	dlResp, err := i.client.DownloadStream(context.Background(), nil)
+	ctx := context.Background()
+	dlResp, err := i.client.DownloadStream(ctx, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -68,7 +69,8 @@ func (i *item) Metadata() (map[string]interface{}, error) {
 // OpenRange opens the item for reading starting at byte start and ending
 // at byte end.
 func (i *item) OpenRange(start, end uint64) (io.ReadCloser, error) {
-	resp, err := i.client.DownloadStream(context.Background(), &blob.DownloadStreamOptions{
+	ctx := context.Background()
+	resp, err := i.client.DownloadStream(ctx, &blob.DownloadStreamOptions{
 		Range: blob.HTTPRange{
 			Offset: int64(start),
 			Count:  int64(end) - int64(start) + 1,

azure/location.go

@@ -27,8 +27,9 @@ func (l *location) Close() error {
 var publicAccessTypeContainer = azcontainer.PublicAccessTypeContainer
 
 func (l *location) CreateContainer(name string) (stow.Container, error) {
+	ctx := context.Background()
 	resp, err := l.client.CreateContainer(
-		context.Background(),
+		ctx,
 		name,
 		&azblob.CreateContainerOptions{Access: &publicAccessTypeContainer})
 
@@ -57,6 +58,7 @@ func (l *location) CreateContainer(name string) (stow.Container, error) {
 }
 
 func (l *location) Containers(prefix, cursor string, count int) ([]stow.Container, string, error) {
+	ctx := context.Background()
 	params := azblob.ListContainersOptions{
 		MaxResults: to.Ptr(int32(count)),
 		Prefix:     &prefix,
@@ -66,7 +68,7 @@ func (l *location) Containers(prefix, cursor string, count int) ([]stow.Containe
 	}
 
 	pager := l.client.NewListContainersPager(&params)
-	resp, err := pager.NextPage(context.Background())
+	resp, err := pager.NextPage(ctx)
 	if err != nil {
 		return nil, cursor, err
 	}
@@ -133,7 +135,7 @@ func (l *location) ItemByURL(url *url.URL) (stow.Item, error) {
 }
 
 func (l *location) RemoveContainer(id string) error {
-	_, err := l.client.DeleteContainer(
-		context.Background(), id, &azblob.DeleteContainerOptions{})
+	ctx := context.Background()
+	_, err := l.client.DeleteContainer(ctx, id, &azblob.DeleteContainerOptions{})
 	return err
 }

azure/sdktools.go

@@ -34,7 +34,14 @@ func NewSharedKeyRequestPreSigner(accountName, key string) (RequestPreSigner, er
 	}, nil
 }
 
-var delegateKeyBufferDuration = time.Minute - 20
+// Azure recommends a 15 minute buffer for SAS tokens (and most tokens) in order to account
+// for clock skew between their systems and clients. From their docs...
+//
+// > remember that you may observe up to 15 minutes of clock skew in either direction
+// > on any request
+//
+// https://learn.microsoft.com/en-us/azure/storage/common/storage-sas-overview
+var clockSkewBuffer = time.Minute * 15
 
 // NewDelegatedKeyPreSigner will create a RequestPreSigner that worked with delegated
 // credentials, necessary when identity-based authentication (AD auth) is the used with
@@ -43,8 +50,8 @@ func NewDelegatedKeyPreSigner(serviceClient *service.Client) (RequestPreSigner,
 	return func(ctx context.Context, values sas.BlobSignatureValues) (sas.QueryParameters, error) {
 		// Create the delegate key with a time buffer, since the blob key's lifetime
 		// must fit within the delegate key's lifetime.
-		delegateCredsStartTime := values.StartTime.UTC().Add(-1 * delegateKeyBufferDuration)
-		delegateCredsEndTime := values.ExpiryTime.UTC().Add(delegateKeyBufferDuration)
+		delegateCredsStartTime := values.StartTime.UTC().Add(-1 * clockSkewBuffer)
+		delegateCredsEndTime := values.ExpiryTime.UTC().Add(clockSkewBuffer)
 
 		udc, err := serviceClient.GetUserDelegationCredential(
 			ctx,

azure/stow_test.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"reflect"
 	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/cheekybits/is"
@@ -125,3 +126,18 @@ func TestMakeAzureCompatMetadataMapFailureWithNonStringValues(t *testing.T) {
 	_, err := makeAzureCompatMetadataMap(m)
 	is.Err(err)
 }
+
+func TestRemovedConfigOptionsCausesFailure(t *testing.T) {
+	is := is.New(t)
+	for _, removedKey := range removedConfigKeys {
+		cfg := stow.ConfigMap{"account": "ignore"}
+		cfg[removedKey] = "anything"
+		err := stow.Validate("azure", cfg)
+		is.Err(err)
+		if !strings.Contains(err.Error(), "removed config option used") ||
+			!strings.Contains(err.Error(), removedKey) {
+			is.Failf("Unexpected error message: %s", err.Error())
+		}
+
+	}
+}

test/test.go

@@ -501,10 +501,9 @@ func checkMetadata(t *testing.T, is is.I, item stow.Item, md map[string]interfac
 		is.Failf("error retrieving item metadata: %v", err)
 	}
 
-	t.Logf("Item metadata: %v", itemMD)
-	t.Logf("Expected item metadata: %v", md)
-
 	if !reflect.DeepEqual(itemMD, md) {
+		t.Logf("Item metadata: %v", itemMD)
+		t.Logf("Expected item metadata: %v", md)
 		return errors.New("metadata mismatch")
 	}
 	return nil