lib/backup/azremote: follow-up for 5fd3aef549

- Mention that credentials can be configured via env variables at both vmbackup and vmrestore docs. - Make clear that the AZURE_STORAGE_DOMAIN env var is optional at https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables - Use string literals as is for env variable names instead of indirecting them via string constants. This makes easier to read and understand the code. These environment variable names aren't going to change in the future, so there is no sense in hiding them under string constants with some other names. - Refer to https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables in error messages when auth creds are improperly configured. This should simplify figuring out how to fix the error. - Simplify the code a bit at FS.newClient(), so it is easier to follow it now. While at it, remove the check when superflouos environment variables are set, since it is too fragile and it looks like it doesn't help properly configuring vmbackup / vmrestore. - Remove envLookuper indirection - just use 'func(name string) (string, bool)' type inline. This simplifies code reading and understanding. - Split TestFSInit() into TestFSInit_Failure() and TestFSInit_Success(). This simplifies the test code, so it should be easier to maintain in the future. Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6518 Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5984
2024-11-21 14:44:00 +00:00 · 2024-07-17 17:42:20 +02:00 · 2024-07-17 17:42:20 +02:00 · 65ce4e30ab
commit 65ce4e30ab
parent eaed0465d2
5 changed files with 154 additions and 106 deletions
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@ -18,8 +18,9 @@ The following `tip` changes can be tested by building VictoriaMetrics components
 * [How to build vmagent](https://docs.victoriametrics.com/vmagent/#how-to-build-from-sources)
 * [How to build vmalert](https://docs.victoriametrics.com/vmalert/#how-to-build-from-sources)
 * [How to build vmauth](https://docs.victoriametrics.com/vmauth/#how-to-build-from-sources)
+* [How to build vmbackup](https://docs.victoriametrics.com/vmbackup/#how-to-build-from-sources)
+* [How to build vmrestore](https://docs.victoriametrics.com/vmrestore/#how-to-build-from-sources)
 * [How to build vmctl](https://docs.victoriametrics.com/vmctl/#how-to-build)
-* [How to build vmbackup](https://docs.victoriametrics.com/vmbackup/index.html#how-to-build-from-sources)

 Metrics of the latest version of VictoriaMetrics cluster are available for viewing at our
 [sandbox](https://play-grafana.victoriametrics.com/d/oS7Bi_0Wz_vm/victoriametrics-cluster-vm).
@ -56,8 +57,8 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/).
 * FEATURE: [VictoriaMetrics cluster](https://docs.victoriametrics.com/cluster-victoriametrics/): do not retry RPC calls to vmstorage nodes if [complexity limits](https://docs.victoriametrics.com/#resource-usage-limits) were exceeded.
 * FEATURE: [vmalert](https://docs.victoriametrics.com/vmalert/): make `-replay.timeTo` optional in [replay mode](https://docs.victoriametrics.com/vmalert/#rules-backfilling). When omitted, the current timestamp will be used. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6492).
 * FEATURE: [vmui](https://docs.victoriametrics.com/#vmui): show compacted result in the JSON tab for query results. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6559).  
-* FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup/index.html): add support of using Azure Managed Identity and default credentials lookup when performing backups. See configuration docs [here](https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5984) for the details. Thanks to @justinrush for the [pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6518).
-* FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup/index.html): allow overriding Azure storage domain when performing backups. See configuration docs [here](https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5984) for the details. Thanks to @justinrush for the [pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6518).
+* FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup/) and [vmrestore](https://docs.victoriametrics.com/vmrestore/): add support for [Azure Managed Identity](https://learn.microsoft.com/en-us/entra/identity/managed-identities-azure-resources/overview) and default credentials lookup. See [these docs](https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables) and [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5984) for the details. Thanks to @justinrush for the [pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6518).
+* FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup/) and [vmrestore](https://docs.victoriametrics.com/vmbackup/): allow overriding Azure storage domain when performing backups via `AZURE_STORAGE_DOMAIN` environment variable. See [these docs](https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables) and [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5984). Thanks to @justinrush for the [pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6518).
 * FEATURE: [streaming aggregation](https://docs.victoriametrics.com/stream-aggregation/): prevent having duplicated aggregation function as `outputs` in one [aggregation config](https://docs.victoriametrics.com/stream-aggregation/#stream-aggregation-config). It also prevents using `outputs: ["quantiles(0.5)", "quantiles(0.9)"]` instead of  `outputs: ["quantiles(0.5, 0.9)"]`, as the former has higher computation cost for producing the same result.

 * BUGFIX: [vmgateway](https://docs.victoriametrics.com/vmgateway/): properly apply read and write based rate limits. See this [issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6148) for details.
--- a/docs/vmbackup.md
+++ b/docs/vmbackup.md
@ -207,10 +207,11 @@ Obtaining credentials from env variables.
  Also you can set env variable `AWS_SHARED_CREDENTIALS_FILE` with path to credentials file.
 - For GCE cloud storage set env variable `GOOGLE_APPLICATION_CREDENTIALS` with path to credentials file.
 - For Azure storage use one of these env variables:
-   - `AZURE_STORAGE_ACCOUNT_NAME` and `AZURE_STORAGE_ACCOUNT_KEY`: Use a specific account name and key (either primary or secondary).
-   - `AZURE_STORAGE_ACCOUNT_CONNECTION_STRING`: Use a connection string (must be either SAS Token or Account/Key)
-   - `AZURE_USE_DEFAULT_CREDENTIAL` and `AZURE_STORAGE_ACCOUNT_NAME`: Use the `DefaultAzureCredential` to allow the azure library to search multiple options (for example, managed identity related variables).
-   - `AZURE_STORAGE_DOMAIN`: Optionally override the default blob domain for the Azure storage service.
+   - `AZURE_STORAGE_ACCOUNT_CONNECTION_STRING`: use a connection string (must be either SAS Token or Account/Key)
+   - `AZURE_STORAGE_ACCOUNT_NAME` and `AZURE_STORAGE_ACCOUNT_KEY`: use a specific account name and key (either primary or secondary)
+   - `AZURE_USE_DEFAULT_CREDENTIAL` and `AZURE_STORAGE_ACCOUNT_NAME`: use the `DefaultAzureCredential` to allow the Azure library
+     to search for multiple options (for example, managed identity related variables).
+  The `AZURE_STORAGE_DOMAIN` can be used for optionally overriding the default domain for the Azure storage service.

 Please, note that `vmbackup` will use credentials provided by cloud providers metadata service [when applicable](https://docs.victoriametrics.com/vmbackup/#using-cloud-providers-metadata-service).

--- a/docs/vmrestore.md
+++ b/docs/vmrestore.md
@ -44,6 +44,7 @@ i.e. the end result would be similar to [rsync --delete](https://askubuntu.com/q

 ## Troubleshooting

+* See [how to setup credentials via environment variables](https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables).
 * If `vmrestore` eats all the network bandwidth, then set `-maxBytesPerSecond` to the desired value.
 * If `vmrestore` has been interrupted due to temporary error, then just restart it with the same args. It will resume the restore process.

--- a/lib/backup/azremote/azblob.go
+++ b/lib/backup/azremote/azblob.go
@ -24,15 +24,6 @@ import (
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/logger"
 )

-const (
-	envStorageAcctName           = "AZURE_STORAGE_ACCOUNT_NAME"
-	envStorageAccKey             = "AZURE_STORAGE_ACCOUNT_KEY"
-	envStorageAccCs              = "AZURE_STORAGE_ACCOUNT_CONNECTION_STRING"
-	envStorageDomain             = "AZURE_STORAGE_DOMAIN"
-	envStorageDefault            = "AZURE_USE_DEFAULT_CREDENTIAL"
-	storageErrorCodeBlobNotFound = "BlobNotFound"
-)
-
 // FS represents filesystem for backups in Azure Blob Storage.
 //
 // Init must be called before calling other FS methods.
@ -44,18 +35,17 @@ type FS struct {
 	Dir string

 	client *container.Client
-	env    envLookuper
+
+	// envLoookupFunc is used for looking up environment variables in tests.
+	envLookupFunc func(name string) (string, bool)
 }

 // Init initializes fs.
 //
 // The returned fs must be stopped when no long needed with MustStop call.
 func (fs *FS) Init() error {
-	switch {
-	case fs.client != nil:
+	if fs.client != nil {
 		logger.Panicf("BUG: fs.Init has been already called")
-	case fs.env == nil:
-		fs.env = envtemplate.LookupEnv
 	}

 	fs.Dir = cleanDirectory(fs.Dir)
@ -72,58 +62,68 @@ func (fs *FS) Init() error {
 }

 func (fs *FS) newClient() (*service.Client, error) {
-	connString, hasConnString := fs.env(envStorageAccCs)
-	accountName, hasAccountName := fs.env(envStorageAcctName)
-	accountKey, hasAccountKey := fs.env(envStorageAccKey)
-	useDefault, _ := fs.env(envStorageDefault)
-
-	domain := "blob.core.windows.net"
-	if storageDomain, ok := fs.env(envStorageDomain); ok {
-		logger.Infof("Overriding default Azure blob domain with %q", storageDomain)
-		domain = storageDomain
+	connString := fs.env("AZURE_STORAGE_ACCOUNT_CONNECTION_STRING")
+	if connString != "" {
+		logger.Infof("creating AZBlob service client from connection string defined at AZURE_STORAGE_ACCOUNT_CONNECTION_STRING")
+		return service.NewClientFromConnectionString(connString, nil)
 	}

-	// not used if connection string is set
-	serviceURL := fmt.Sprintf("https://%s.%s/", accountName, domain)
+	accountKey := fs.env("AZURE_STORAGE_ACCOUNT_KEY")
+	if accountKey != "" {
+		logger.Infof("creating AZBlob service client from account name and key")

-	switch {
-	// can't specify any combination of more than one credential
-	case moreThanOne(hasConnString, (hasAccountName && hasAccountKey), (useDefault == "true" && hasAccountName)):
-		return nil, fmt.Errorf("failed to process credentials: only one of %s, %s and %s, or %s and %s can be specified",
-			envStorageAccCs,
-			envStorageAcctName,
-			envStorageAccKey,
-			envStorageAcctName,
-			envStorageDefault,
-		)
-	case hasConnString:
-		logger.Infof("Creating AZBlob service client from connection string")
-		return service.NewClientFromConnectionString(connString, nil)
-	case hasAccountName && hasAccountKey:
-		logger.Infof("Creating AZBlob service client from account name and key")
+		accountName := fs.env("AZURE_STORAGE_ACCOUNT_NAME")
+		if accountName == "" {
+			return nil, fmt.Errorf("missing AZURE_STORAGE_ACCOUNT_NAME environment variable when AZURE_STORAGE_ACCOUNT_KEY is set; " +
+				"see https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables")
+		}
 		creds, err := azblob.NewSharedKeyCredential(accountName, accountKey)
 		if err != nil {
 			return nil, fmt.Errorf("failed to create Shared Key credentials: %w", err)
 		}
+		serviceURL := fs.getServiceURL(accountName)
 		return service.NewClientWithSharedKeyCredential(serviceURL, creds, nil)
-	case useDefault == "true" && hasAccountName:
-		logger.Infof("Creating AZBlob service client from default credential")
+	}
+
+	useDefault := fs.env("AZURE_USE_DEFAULT_CREDENTIAL")
+	if useDefault == "true" {
+		logger.Infof("creating AZBlob service client from default credentials")
 		creds, err := azidentity.NewDefaultAzureCredential(nil)
 		if err != nil {
 			return nil, fmt.Errorf("failed to create default Azure credentials: %w", err)
 		}
+
+		accountName := fs.env("AZURE_STORAGE_ACCOUNT_NAME")
+		if accountName == "" {
+			return nil, fmt.Errorf("missing AZURE_STORAGE_ACCOUNT_NAME environment variable when AZURE_USE_DEFAULT_CREDENTIAL=true is set; " +
+				"see https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables")
+		}
+
+		serviceURL := fs.getServiceURL(accountName)
 		return service.NewClient(serviceURL, creds, nil)
-	default:
-		return nil, fmt.Errorf(
-			`failed to detect credentials for AZBlob. 
-Ensure that one of the options is set: connection string at %q; shared key at %q and %q; account name at %q and set %q to "true"`,
-			envStorageAccCs,
-			envStorageAcctName,
-			envStorageAccKey,
-			envStorageAcctName,
-			envStorageDefault,
-		)
 	}
+
+	return nil, fmt.Errorf("failed to detect credentials for AZBlob; ensure that one of the options listed at " +
+		"https://docs.victoriametrics.com/vmbackup/#providing-credentials-via-env-variables is set")
+}
+
+func (fs *FS) env(name string) string {
+	if fs.envLookupFunc != nil {
+		v, _ := fs.envLookupFunc(name)
+		return v
+	}
+	v, _ := envtemplate.LookupEnv(name)
+	return v
+}
+
+func (fs *FS) getServiceURL(accountName string) string {
+	domain := "blob.core.windows.net"
+	storageDomain := fs.env("AZURE_STORAGE_DOMAIN")
+	if storageDomain != "" {
+		logger.Infof("overriding default Azure blob domain with AZURE_STORAGE_DOMAIN=%q", storageDomain)
+		domain = storageDomain
+	}
+	return fmt.Sprintf("https://%s.%s/", accountName, domain)
 }

 // MustStop stops fs.
@ -390,7 +390,7 @@ func (fs *FS) HasFile(filePath string) (bool, error) {
 	_, err := bc.GetProperties(ctx, nil)
 	var azerr *azcore.ResponseError
 	if errors.As(err, &azerr) {
-		if azerr.ErrorCode == storageErrorCodeBlobNotFound {
+		if azerr.ErrorCode == "BlobNotFound" {
 			return false, nil
 		}
 		logger.Errorf("GetProperties(%q) returned %s", bc.URL(), err)
@ -416,26 +416,9 @@ func (fs *FS) ReadFile(filePath string) ([]byte, error) {
 	return b, nil
 }

-// envLookuper is for looking up environment variables. It is
-// needed to allow unit tests to provide alternate values since the envtemplate
-// package uses a singleton to read all environment variables into memory at
-// init time.
-type envLookuper func(name string) (string, bool)
-
-func moreThanOne(vals ...bool) bool {
-	var n int
-
-	for _, v := range vals {
-		if v {
-			n++
-		}
-	}
-	return n > 1
-}
-
-// cleanDirectory ensures that the directory is properly formatted for Azure
-// Blob Storage. It removes any leading slashes and ensures that the directory
-// ends with a trailing slash.
+// cleanDirectory ensures that the directory is properly formatted for Azure Blob Storage.
+//
+// It removes any leading slashes and ensures that the directory ends with a trailing slash.
 func cleanDirectory(dir string) string {
 	for strings.HasPrefix(dir, "/") {
 		dir = dir[1:]
--- a/lib/backup/azremote/azblob_test.go
+++ b/lib/backup/azremote/azblob_test.go
@ -20,42 +20,104 @@ func TestCleanDirectory(t *testing.T) {
 	f("foo", "foo/")
 }

-func TestFSInit(t *testing.T) {
-	f := func(expErr string, params ...string) {
+func TestFSInit_Failure(t *testing.T) {
+	f := func(envArgs map[string]string, errStrExpected string) {
 		t.Helper()

-		env := make(testEnv)
-		for i := 0; i < len(params); i += 2 {
-			env[params[i]] = params[i+1]
+		fs := &FS{
+			Dir: "foo",
 		}
+		env := testEnv(envArgs)
+		fs.envLookupFunc = env.LookupEnv

-		fs := &FS{Dir: "foo"}
-		fs.env = env.LookupEnv
 		err := fs.Init()
-		if err != nil {
-			if expErr == "" {
-				t.Fatalf("unexpected error %v", err)
-			}
-			if !strings.Contains(err.Error(), expErr) {
-				t.Fatalf("expected error: \n%q, \ngot: \n%v", expErr, err)
-			}
-			return
+		if err == nil {
+			t.Fatalf("expecting non-nil error")
 		}
-		if expErr != "" {
-			t.Fatalf("expected to have an error %q, instead got nil", expErr)
+		errStr := err.Error()
+		if !strings.Contains(errStr, errStrExpected) {
+			t.Fatalf("expecting %q in the error %q", errStrExpected, errStr)
 		}
 	}

-	f("", envStorageAccCs, "BlobEndpoint=https://test.blob.core.windows.net/;SharedAccessSignature=")
-	f("", envStorageAcctName, "test", envStorageAccKey, "dGVhcG90Cg==")
-	f("", envStorageDefault, "true", envStorageAcctName, "test")
-	f("", envStorageAcctName, "test", envStorageAccKey, "dGVhcG90Cg==", envStorageDomain, "foo.bar")
+	var envArgs map[string]string

-	f("failed to detect credentials for AZBlob")
-	f("failed to detect credentials for AZBlob", envStorageAcctName, "test")
-	f("failed to create Shared Key", envStorageAcctName, "", envStorageAccKey, "!")
-	f("connection string is either blank or malformed", envStorageAccCs, "")
-	f("failed to process credentials: only one of", envStorageAccCs, "teapot", envStorageAcctName, "test", envStorageAccKey, "dGVhcG90Cg==")
+	f(envArgs, "failed to detect credentials for AZBlob")
+
+	envArgs = map[string]string{
+		"AZURE_STORAGE_ACCOUNT_NAME": "test",
+	}
+	f(envArgs, "failed to detect credentials for AZBlob")
+
+	envArgs = map[string]string{
+		"AZURE_STORAGE_ACCOUNT_NAME": "",
+		"AZURE_STORAGE_ACCOUNT_KEY":  "!",
+	}
+	f(envArgs, "missing AZURE_STORAGE_ACCOUNT_NAME")
+
+	envArgs = map[string]string{
+		"AZURE_STORAGE_ACCOUNT_NAME": "foo",
+		"AZURE_STORAGE_ACCOUNT_KEY":  "!",
+	}
+	f(envArgs, "failed to create Shared Key credentials")
+
+	envArgs = map[string]string{
+		"AZURE_STORAGE_ACCOUNT_CONNECTION_STRING": "foobar",
+	}
+	f(envArgs, "connection string is either blank or malformed")
+
+	envArgs = map[string]string{
+		"AZURE_STORAGE_ACCOUNT_CONNECTION_STRING": "teapot",
+		"AZURE_STORAGE_ACCOUNT_NAME":              "test",
+		"AZURE_STORAGE_ACCOUNT_KEY":               "dGVhcG90Cg==",
+	}
+	f(envArgs, "connection string is either blank or malformed")
+
+	envArgs = map[string]string{
+		"AZURE_USE_DEFAULT_CREDENTIAL": "true",
+	}
+	f(envArgs, "missing AZURE_STORAGE_ACCOUNT_NAME")
+}
+
+func TestFSInit_Success(t *testing.T) {
+	f := func(envArgs map[string]string) {
+		t.Helper()
+
+		fs := &FS{
+			Dir: "foo",
+		}
+		env := testEnv(envArgs)
+		fs.envLookupFunc = env.LookupEnv
+
+		err := fs.Init()
+		if err != nil {
+			t.Fatalf("unexpected error at fs.Init(): %s", err)
+		}
+	}
+
+	envArgs := map[string]string{
+		"AZURE_STORAGE_ACCOUNT_CONNECTION_STRING": "BlobEndpoint=https://test.blob.core.windows.net/;SharedAccessSignature=",
+	}
+	f(envArgs)
+
+	envArgs = map[string]string{
+		"AZURE_STORAGE_ACCOUNT_NAME": "test",
+		"AZURE_STORAGE_ACCOUNT_KEY":  "dGVhcG90Cg==",
+	}
+	f(envArgs)
+
+	envArgs = map[string]string{
+		"AZURE_USE_DEFAULT_CREDENTIAL": "true",
+		"AZURE_STORAGE_ACCOUNT_NAME":   "test",
+	}
+	f(envArgs)
+
+	envArgs = map[string]string{
+		"AZURE_STORAGE_ACCOUNT_NAME": "test",
+		"AZURE_STORAGE_ACCOUNT_KEY":  "dGVhcG90Cg==",
+		"AZURE_STORAGE_DOMAIN":       "foo.bar",
+	}
+	f(envArgs)
 }

 type testEnv map[string]string