diff --git a/changelogs/unreleased/5387-lyndon b/changelogs/unreleased/5387-lyndon new file mode 100644 index 000000000..63cf32bf3 --- /dev/null +++ b/changelogs/unreleased/5387-lyndon @@ -0,0 +1 @@ +Fix issue 5386: Velero providers a full URL as the S3Url while the underlying minio client only accept the host part of the URL as the endpoint and the schema should be specified separately. \ No newline at end of file diff --git a/pkg/repository/provider/unified_repo.go b/pkg/repository/provider/unified_repo.go index ec71ae4c9..6bb4f3844 100644 --- a/pkg/repository/provider/unified_repo.go +++ b/pkg/repository/provider/unified_repo.go @@ -19,7 +19,9 @@ package provider import ( "context" "fmt" + "net/url" "path" + "strconv" "strings" "time" @@ -435,6 +437,7 @@ func getStorageVariables(backupLocation *velerov1api.BackupStorageLocation, repo if backendType == repoconfig.AWSBackend { s3Url := config["s3Url"] + disableTls := false var err error if s3Url == "" { @@ -444,10 +447,24 @@ func getStorageVariables(backupLocation *velerov1api.BackupStorageLocation, repo } s3Url = fmt.Sprintf("s3-%s.amazonaws.com", region) + disableTls = false + } else { + url, err := url.Parse(s3Url) + if err != nil { + return map[string]string{}, errors.Wrapf(err, "error to parse s3Url %s", s3Url) + } + + if url.Path != "" && url.Path != "/" { + return map[string]string{}, errors.Errorf("path is not expected in s3Url %s", s3Url) + } + + s3Url = url.Host + disableTls = (url.Scheme == "http") } result[udmrepo.StoreOptionS3Endpoint] = strings.Trim(s3Url, "/") result[udmrepo.StoreOptionS3DisableTlsVerify] = config["insecureSkipTLSVerify"] + result[udmrepo.StoreOptionS3DisableTls] = strconv.FormatBool(disableTls) } else if backendType == repoconfig.AzureBackend { result[udmrepo.StoreOptionAzureDomain] = getAzureStorageDomain(config) } diff --git a/pkg/repository/provider/unified_repo_test.go b/pkg/repository/provider/unified_repo_test.go index fb1c9d170..e6f891cce 100644 --- a/pkg/repository/provider/unified_repo_test.go +++ b/pkg/repository/provider/unified_repo_test.go @@ -250,7 +250,7 @@ func TestGetStorageVariables(t *testing.T) { expectedErr: "invalid storage provider", }, { - name: "aws, ObjectStorage section not exists in BSL, s3Url exist", + name: "aws, ObjectStorage section not exists in BSL, s3Url exist, https", backupLocation: velerov1api.BackupStorageLocation{ Spec: velerov1api.BackupStorageLocationSpec{ Provider: "velero.io/aws", @@ -258,7 +258,7 @@ func TestGetStorageVariables(t *testing.T) { "bucket": "fake-bucket", "prefix": "fake-prefix", "region": "fake-region/", - "s3Url": "fake-url", + "s3Url": "https://fake-url/", "insecureSkipTLSVerify": "true", }, }, @@ -270,9 +270,28 @@ func TestGetStorageVariables(t *testing.T) { "region": "fake-region", "fspath": "", "endpoint": "fake-url", + "doNotUseTLS": "false", "skipTLSVerify": "true", }, }, + { + name: "aws, ObjectStorage section not exists in BSL, s3Url exist, invalid", + backupLocation: velerov1api.BackupStorageLocation{ + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "velero.io/aws", + Config: map[string]string{ + "bucket": "fake-bucket", + "prefix": "fake-prefix", + "region": "fake-region/", + "s3Url": "https://fake-url/fake-path", + "insecureSkipTLSVerify": "true", + }, + }, + }, + repoBackend: "fake-repo-type", + expected: map[string]string{}, + expectedErr: "path is not expected in s3Url https://fake-url/fake-path", + }, { name: "aws, ObjectStorage section not exists in BSL, s3Url not exist", backupLocation: velerov1api.BackupStorageLocation{ @@ -295,6 +314,7 @@ func TestGetStorageVariables(t *testing.T) { "region": "region from bucket: fake-bucket", "fspath": "", "endpoint": "s3-region from bucket: fake-bucket.amazonaws.com", + "doNotUseTLS": "false", "skipTLSVerify": "false", }, }, @@ -313,7 +333,7 @@ func TestGetStorageVariables(t *testing.T) { expectedErr: "error get s3 bucket region: fake error", }, { - name: "aws, ObjectStorage section exists in BSL, s3Url exist", + name: "aws, ObjectStorage section exists in BSL, s3Url exist, http", backupLocation: velerov1api.BackupStorageLocation{ Spec: velerov1api.BackupStorageLocationSpec{ Provider: "velero.io/aws", @@ -321,7 +341,7 @@ func TestGetStorageVariables(t *testing.T) { "bucket": "fake-bucket-config", "prefix": "fake-prefix-config", "region": "fake-region", - "s3Url": "fake-url", + "s3Url": "http://fake-url/", "insecureSkipTLSVerify": "false", }, StorageType: velerov1api.StorageType{ @@ -342,6 +362,7 @@ func TestGetStorageVariables(t *testing.T) { "region": "fake-region", "fspath": "", "endpoint": "fake-url", + "doNotUseTLS": "true", "skipTLSVerify": "false", }, }, diff --git a/pkg/repository/udmrepo/kopialib/backend/utils.go b/pkg/repository/udmrepo/kopialib/backend/utils.go index 5aab1f1af..eb673539f 100644 --- a/pkg/repository/udmrepo/kopialib/backend/utils.go +++ b/pkg/repository/udmrepo/kopialib/backend/utils.go @@ -39,12 +39,14 @@ func optionalHaveString(key string, flags map[string]string) string { func optionalHaveBool(ctx context.Context, key string, flags map[string]string) bool { if value, exist := flags[key]; exist { - ret, err := strconv.ParseBool(value) - if err == nil { - return ret - } + if value != "" { + ret, err := strconv.ParseBool(value) + if err == nil { + return ret + } - backendLog()(ctx).Errorf("Ignore %s, value [%s] is invalid, err %v", key, value, err) + backendLog()(ctx).Errorf("Ignore %s, value [%s] is invalid, err %v", key, value, err) + } } return false