From 3571339fd64553dd3bad2eeede0d0d747ee80fc1 Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Fri, 28 Oct 2022 16:36:27 +0800 Subject: [PATCH] issue fix 5505 Signed-off-by: Lyndon-Li --- changelogs/unreleased/5512-lyndon | 1 + pkg/repository/config/azure.go | 24 ++++++--- pkg/repository/config/azure_test.go | 46 +++++++++++++++++ pkg/repository/provider/unified_repo.go | 7 ++- pkg/repository/provider/unified_repo_test.go | 54 ++++++++++++++------ 5 files changed, 110 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/5512-lyndon diff --git a/changelogs/unreleased/5512-lyndon b/changelogs/unreleased/5512-lyndon new file mode 100644 index 0000000000..2712304b38 --- /dev/null +++ b/changelogs/unreleased/5512-lyndon @@ -0,0 +1 @@ +Fix issue 5505: the pod volume backups/restores except the first one fail under the kopia path if "AZURE_CLOUD_NAME" is specified diff --git a/pkg/repository/config/azure.go b/pkg/repository/config/azure.go index c90c4f0ac7..7716c73c7e 100644 --- a/pkg/repository/config/azure.go +++ b/pkg/repository/config/azure.go @@ -18,6 +18,7 @@ package config import ( "context" + "fmt" "os" "strings" @@ -211,13 +212,15 @@ func getRequiredValues(getValue func(string) string, keys ...string) error { } // GetAzureStorageDomain gets the Azure storage domain required by a Azure blob connection, -// if the provided config doean't have the value, get it from system's environment variables -func GetAzureStorageDomain(config map[string]string) string { - if domain, exist := config[storageDomainConfigKey]; exist { - return domain - } else { - return os.Getenv(cloudNameEnvVar) +// if the provided credential file doesn't have the value, get it from system's environment variables +func GetAzureStorageDomain(config map[string]string) (string, error) { + credentialsFile := selectCredentialsFile(config) + + if err := loadCredentialsIntoEnv(credentialsFile); err != nil { + return "", err } + + return getStorageDomainFromCloudName(os.Getenv(cloudNameEnvVar)) } func GetAzureCredentials(config map[string]string) (string, string, error) { @@ -228,3 +231,12 @@ func GetAzureCredentials(config map[string]string) (string, string, error) { return config[storageAccountConfigKey], storageAccountKey, nil } + +func getStorageDomainFromCloudName(cloudName string) (string, error) { + env, err := parseAzureEnvironment(cloudName) + if err != nil { + return "", errors.Wrapf(err, "unable to parse azure env from cloud name %s", cloudName) + } + + return fmt.Sprintf("blob.%s", env.StorageEndpointSuffix), nil +} diff --git a/pkg/repository/config/azure_test.go b/pkg/repository/config/azure_test.go index d20ac2e28b..f32f87ce01 100644 --- a/pkg/repository/config/azure_test.go +++ b/pkg/repository/config/azure_test.go @@ -20,6 +20,7 @@ import ( "os" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -86,3 +87,48 @@ func TestSelectCredentialsFile(t *testing.T) { }) } } + +func TestGetStorageDomainFromCloudName(t *testing.T) { + testCases := []struct { + name string + cloudName string + expected string + expectedErr string + }{ + { + name: "get azure env fail", + cloudName: "fake-cloud", + expectedErr: "unable to parse azure env from cloud name fake-cloud: autorest/azure: There is no cloud environment matching the name \"FAKE-CLOUD\"", + }, + { + name: "cloud name is empty", + cloudName: "", + expected: "blob.core.windows.net", + }, + { + name: "azure public cloud", + cloudName: "AzurePublicCloud", + expected: "blob.core.windows.net", + }, + { + + name: "azure China cloud", + cloudName: "AzureChinaCloud", + expected: "blob.core.chinacloudapi.cn", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + domain, err := getStorageDomainFromCloudName(tc.cloudName) + + require.Equal(t, tc.expected, domain) + + if tc.expectedErr == "" { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, tc.expectedErr) + assert.Empty(t, domain) + } + }) + } +} diff --git a/pkg/repository/provider/unified_repo.go b/pkg/repository/provider/unified_repo.go index 30d1c97aaa..47d8feee54 100644 --- a/pkg/repository/provider/unified_repo.go +++ b/pkg/repository/provider/unified_repo.go @@ -466,7 +466,12 @@ func getStorageVariables(backupLocation *velerov1api.BackupStorageLocation, repo result[udmrepo.StoreOptionS3DisableTlsVerify] = config["insecureSkipTLSVerify"] result[udmrepo.StoreOptionS3DisableTls] = strconv.FormatBool(disableTls) } else if backendType == repoconfig.AzureBackend { - result[udmrepo.StoreOptionAzureDomain] = getAzureStorageDomain(config) + domain, err := getAzureStorageDomain(config) + if err != nil { + return map[string]string{}, errors.Wrapf(err, "error to get azure storage domain") + } + + result[udmrepo.StoreOptionAzureDomain] = domain } result[udmrepo.StoreOptionOssBucket] = bucket diff --git a/pkg/repository/provider/unified_repo_test.go b/pkg/repository/provider/unified_repo_test.go index c8121058c6..e33059c2b7 100644 --- a/pkg/repository/provider/unified_repo_test.go +++ b/pkg/repository/provider/unified_repo_test.go @@ -235,7 +235,7 @@ func TestGetStorageVariables(t *testing.T) { repoName string repoBackend string getS3BucketRegion func(string) (string, error) - getAzureStorageDomain func(map[string]string) string + getAzureStorageDomain func(map[string]string) (string, error) expected map[string]string expectedErr string }{ @@ -366,17 +366,42 @@ func TestGetStorageVariables(t *testing.T) { "skipTLSVerify": "false", }, }, + { + name: "azure, getAzureStorageDomain fail", + backupLocation: velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "velero.io/azure", + Config: map[string]string{ + "bucket": "fake-bucket-config", + "prefix": "fake-prefix-config", + "region": "fake-region", + "fspath": "", + }, + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "fake-bucket-object-store", + Prefix: "fake-prefix-object-store", + }, + }, + }, + }, + getAzureStorageDomain: func(config map[string]string) (string, error) { + return "", errors.New("fake error") + }, + repoBackend: "fake-repo-type", + expected: map[string]string{}, + expectedErr: "error to get azure storage domain: fake error", + }, { name: "azure, ObjectStorage section exists in BSL", backupLocation: velerov1api.BackupStorageLocation{ Spec: velerov1api.BackupStorageLocationSpec{ Provider: "velero.io/azure", Config: map[string]string{ - "bucket": "fake-bucket-config", - "prefix": "fake-prefix-config", - "region": "fake-region", - "fspath": "", - "storageDomain": "fake-domain", + "bucket": "fake-bucket-config", + "prefix": "fake-prefix-config", + "region": "fake-region", + "fspath": "", }, StorageType: velerov1api.StorageType{ ObjectStorage: &velerov1api.ObjectStorageLocation{ @@ -386,8 +411,8 @@ func TestGetStorageVariables(t *testing.T) { }, }, }, - getAzureStorageDomain: func(config map[string]string) string { - return config["storageDomain"] + getAzureStorageDomain: func(config map[string]string) (string, error) { + return "fake-domain", nil }, repoBackend: "fake-repo-type", expected: map[string]string{ @@ -404,18 +429,17 @@ func TestGetStorageVariables(t *testing.T) { Spec: velerov1api.BackupStorageLocationSpec{ Provider: "velero.io/azure", Config: map[string]string{ - "bucket": "fake-bucket", - "prefix": "fake-prefix", - "region": "fake-region", - "fspath": "", - "storageDomain": "fake-domain", + "bucket": "fake-bucket", + "prefix": "fake-prefix", + "region": "fake-region", + "fspath": "", }, }, }, repoName: "//fake-name//", repoBackend: "fake-repo-type", - getAzureStorageDomain: func(config map[string]string) string { - return config["storageDomain"] + getAzureStorageDomain: func(config map[string]string) (string, error) { + return "fake-domain", nil }, expected: map[string]string{ "bucket": "fake-bucket",