Merge pull request #5387 from Lyndon-Li/issue-fix-5386

fix issue 5386

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.

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)
 	}

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",
 			},
 		},

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