From 40a1b62c09f2a1bdb38b05a0439119f613f0ba22 Mon Sep 17 00:00:00 2001 From: Jalaja Date: Tue, 23 Mar 2021 19:16:37 +0000 Subject: [PATCH 1/3] use region input to detect the Bucket region Signed-off-by: Jalaja Ganapathy Signed-off-by: Jalaja --- changelogs/unreleased/3617-jala-dx | 1 + pkg/restic/config.go | 4 ++++ 2 files changed, 5 insertions(+) create mode 100644 changelogs/unreleased/3617-jala-dx diff --git a/changelogs/unreleased/3617-jala-dx b/changelogs/unreleased/3617-jala-dx new file mode 100644 index 0000000000..425fcd08ce --- /dev/null +++ b/changelogs/unreleased/3617-jala-dx @@ -0,0 +1 @@ +Region is calculated incorrectly when gov account is used. diff --git a/pkg/restic/config.go b/pkg/restic/config.go index 452adbc0cc..be207506e2 100644 --- a/pkg/restic/config.go +++ b/pkg/restic/config.go @@ -64,10 +64,14 @@ func getRepoPrefix(location *velerov1api.BackupStorageLocation) (string, error) switch backendType { case AWSBackend: var url string + var region string switch { // non-AWS, S3-compatible object store case location.Spec.Config["s3Url"] != "": url = location.Spec.Config["s3Url"] + case location.Spec.Config["region"] != "": + region = location.Spec.Config["region"] + url = fmt.Sprintf("s3-%s.amazonaws.com", region) default: region, err := getAWSBucketRegion(bucket) if err != nil { From a95b035bf32ee043f3e249b5d79089d3de0ef32a Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Wed, 9 Jun 2021 12:08:19 -0400 Subject: [PATCH 2/3] Refactor GetRepoIdentifier tests and add new case Also refactor the AWS `getRepoPrefix` logic to remove use of `switch` statement. Signed-off-by: Bridget McErlean --- pkg/restic/config.go | 24 ++- pkg/restic/config_test.go | 307 +++++++++++++++++++++++--------------- 2 files changed, 194 insertions(+), 137 deletions(-) diff --git a/pkg/restic/config.go b/pkg/restic/config.go index be207506e2..b5471fa37c 100644 --- a/pkg/restic/config.go +++ b/pkg/restic/config.go @@ -64,25 +64,23 @@ func getRepoPrefix(location *velerov1api.BackupStorageLocation) (string, error) switch backendType { case AWSBackend: var url string - var region string - switch { // non-AWS, S3-compatible object store - case location.Spec.Config["s3Url"] != "": - url = location.Spec.Config["s3Url"] - case location.Spec.Config["region"] != "": - region = location.Spec.Config["region"] - url = fmt.Sprintf("s3-%s.amazonaws.com", region) - default: - region, err := getAWSBucketRegion(bucket) + if s3Url := location.Spec.Config["s3Url"]; s3Url != "" { + url = strings.TrimSuffix(s3Url, "/") + } else { + var err error + region := location.Spec.Config["region"] + if region == "" { + region, err = getAWSBucketRegion(bucket) + } if err != nil { url = "s3.amazonaws.com" - break + } else { + url = fmt.Sprintf("s3-%s.amazonaws.com", region) } - - url = fmt.Sprintf("s3-%s.amazonaws.com", region) } - return fmt.Sprintf("s3:%s/%s", strings.TrimSuffix(url, "/"), path.Join(bucket, prefix)), nil + return fmt.Sprintf("s3:%s/%s", url, path.Join(bucket, prefix)), nil case AzureBackend: return fmt.Sprintf("azure:%s:/%s", bucket, prefix), nil case GCPBackend: diff --git a/pkg/restic/config_test.go b/pkg/restic/config_test.go index 67133aab6c..ad24131049 100644 --- a/pkg/restic/config_test.go +++ b/pkg/restic/config_test.go @@ -26,156 +26,215 @@ import ( ) func TestGetRepoIdentifier(t *testing.T) { - // if getAWSBucketRegion returns an error, use default "s3.amazonaws.com/..." URL - getAWSBucketRegion = func(string) (string, error) { - return "", errors.New("no region found") - } - - backupLocation := &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "aws", - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket", - Prefix: "prefix", + testCases := []struct { + name string + bsl *velerov1api.BackupStorageLocation + repoName string + getAWSBucketRegion func(string) (string, error) + expected string + expectedErr string + }{ + { + name: "error is returned if BSL uses unsupported provider and resticRepoPrefix is not set", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "unsupported-provider", + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket-2", + Prefix: "prefix-2", + }, + }, }, }, + repoName: "repo-1", + expectedErr: "restic repository prefix (resticRepoPrefix) not specified in backup storage location's config", }, - } - id, err := GetRepoIdentifier(backupLocation, "repo-1") - assert.NoError(t, err) - assert.Equal(t, "s3:s3.amazonaws.com/bucket/prefix/restic/repo-1", id) - - // stub implementation of getAWSBucketRegion - getAWSBucketRegion = func(string) (string, error) { - return "us-west-2", nil - } - - backupLocation = &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "aws", - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket", + { + name: "resticRepoPrefix in BSL config is used if set", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "custom-repo-identifier", + Config: map[string]string{ + "resticRepoPrefix": "custom:prefix:/restic", + }, + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket", + Prefix: "prefix", + }, + }, }, }, + repoName: "repo-1", + expected: "custom:prefix:/restic/repo-1", }, - } - id, err = GetRepoIdentifier(backupLocation, "repo-1") - assert.NoError(t, err) - assert.Equal(t, "s3:s3-us-west-2.amazonaws.com/bucket/restic/repo-1", id) - - backupLocation = &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "aws", - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket", - Prefix: "prefix", + { + name: "s3.amazonaws.com URL format is used if region cannot be determined for AWS BSL", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket", + }, + }, }, }, + repoName: "repo-1", + getAWSBucketRegion: func(string) (string, error) { + return "", errors.New("no region found") + }, + expected: "s3:s3.amazonaws.com/bucket/restic/repo-1", }, - } - id, err = GetRepoIdentifier(backupLocation, "repo-1") - assert.NoError(t, err) - assert.Equal(t, "s3:s3-us-west-2.amazonaws.com/bucket/prefix/restic/repo-1", id) - - backupLocation = &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "aws", - Config: map[string]string{ - "s3Url": "alternate-url", - }, - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket", - Prefix: "prefix", + { + name: "s3.s3-.amazonaws.com URL format is used if region can be determined for AWS BSL", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket", + }, + }, }, }, + repoName: "repo-1", + getAWSBucketRegion: func(string) (string, error) { + return "eu-west-1", nil + }, + expected: "s3:s3-eu-west-1.amazonaws.com/bucket/restic/repo-1", }, - } - id, err = GetRepoIdentifier(backupLocation, "repo-1") - assert.NoError(t, err) - assert.Equal(t, "s3:alternate-url/bucket/prefix/restic/repo-1", id) - - backupLocation = &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "aws", - Config: map[string]string{ - "s3Url": "alternate-url-with-trailing-slash/", - }, - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket", - Prefix: "prefix", + { + name: "prefix is included in repo identifier if set for AWS BSL", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket", + Prefix: "prefix", + }, + }, }, }, + repoName: "repo-1", + getAWSBucketRegion: func(string) (string, error) { + return "eu-west-1", nil + }, + expected: "s3:s3-eu-west-1.amazonaws.com/bucket/prefix/restic/repo-1", }, - } - id, err = GetRepoIdentifier(backupLocation, "repo-1") - assert.NoError(t, err) - assert.Equal(t, "s3:alternate-url-with-trailing-slash/bucket/prefix/restic/repo-1", id) - - backupLocation = &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "azure", - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket", - Prefix: "prefix", + { + name: "s3Url is used in repo identifier if set for AWS BSL", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "s3Url": "alternate-url", + }, + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket", + Prefix: "prefix", + }, + }, }, }, + repoName: "repo-1", + getAWSBucketRegion: func(string) (string, error) { + return "eu-west-1", nil + }, + expected: "s3:alternate-url/bucket/prefix/restic/repo-1", }, - } - id, err = GetRepoIdentifier(backupLocation, "repo-1") - assert.NoError(t, err) - assert.Equal(t, "azure:bucket:/prefix/restic/repo-1", id) - - backupLocation = &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "gcp", - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket-2", - Prefix: "prefix-2", + { + name: "region is used in repo identifier if set for AWS BSL", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "region": "us-west-1", + }, + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket", + Prefix: "prefix", + }, + }, }, }, + repoName: "aws-repo", + getAWSBucketRegion: func(string) (string, error) { + return "eu-west-1", nil + }, + expected: "s3:s3-us-west-1.amazonaws.com/bucket/prefix/restic/aws-repo", }, - } - id, err = GetRepoIdentifier(backupLocation, "repo-2") - assert.NoError(t, err) - assert.Equal(t, "gs:bucket-2:/prefix-2/restic/repo-2", id) - - backupLocation = &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "unsupported-provider", - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket-2", - Prefix: "prefix-2", + { + name: "trailing slash in s3Url is not included in repo identifier for AWS BSL", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "s3Url": "alternate-url-with-trailing-slash/", + }, + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket", + Prefix: "prefix", + }, + }, }, }, + repoName: "aws-repo", + getAWSBucketRegion: func(string) (string, error) { + return "eu-west-1", nil + }, + expected: "s3:alternate-url-with-trailing-slash/bucket/prefix/restic/aws-repo", }, - } - id, err = GetRepoIdentifier(backupLocation, "repo-1") - assert.EqualError(t, err, "restic repository prefix (resticRepoPrefix) not specified in backup storage location's config") - assert.Empty(t, id) - - backupLocation = &velerov1api.BackupStorageLocation{ - Spec: velerov1api.BackupStorageLocationSpec{ - Provider: "custom-repo-identifier", - Config: map[string]string{ - "resticRepoPrefix": "custom:prefix:/restic", - }, - StorageType: velerov1api.StorageType{ - ObjectStorage: &velerov1api.ObjectStorageLocation{ - Bucket: "bucket", - Prefix: "prefix", + { + name: "repo identifier includes bucket and prefix for Azure BSL", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "azure", + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "azure-bucket", + Prefix: "azure-prefix", + }, + }, }, }, + repoName: "azure-repo", + expected: "azure:azure-bucket:/azure-prefix/restic/azure-repo", }, + { + name: "repo identifier includes bucket and prefix for GCP BSL", + bsl: &velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "gcp", + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "gcp-bucket", + Prefix: "gcp-prefix", + }, + }, + }, + }, + repoName: "gcp-repo", + expected: "gs:gcp-bucket:/gcp-prefix/restic/gcp-repo", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + getAWSBucketRegion = tc.getAWSBucketRegion + id, err := GetRepoIdentifier(tc.bsl, tc.repoName) + assert.Equal(t, tc.expected, id) + if tc.expectedErr == "" { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, tc.expectedErr) + assert.Empty(t, id) + } + }) } - id, err = GetRepoIdentifier(backupLocation, "repo-1") - assert.NoError(t, err) - assert.Equal(t, "custom:prefix:/restic/repo-1", id) } From 1495d61a680532bc3fc3968aa648a5ffe8c3ecde Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Wed, 9 Jun 2021 13:30:44 -0400 Subject: [PATCH 3/3] Update changelog for new PR. Signed-off-by: Bridget McErlean --- changelogs/unreleased/3617-jala-dx | 1 - changelogs/unreleased/3857-zubron | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelogs/unreleased/3617-jala-dx create mode 100644 changelogs/unreleased/3857-zubron diff --git a/changelogs/unreleased/3617-jala-dx b/changelogs/unreleased/3617-jala-dx deleted file mode 100644 index 425fcd08ce..0000000000 --- a/changelogs/unreleased/3617-jala-dx +++ /dev/null @@ -1 +0,0 @@ -Region is calculated incorrectly when gov account is used. diff --git a/changelogs/unreleased/3857-zubron b/changelogs/unreleased/3857-zubron new file mode 100644 index 0000000000..9037a0bf5f --- /dev/null +++ b/changelogs/unreleased/3857-zubron @@ -0,0 +1 @@ +Use region specified in the BackupStorageLocation spec when getting restic repo identifier. Originally fixed by @jala-dx in #3617.