fix issue 5875

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
vmware-tanzu · May 18, 2023 · 1c04019 · 1c04019
1 parent 9fea274
commit 1c04019
Show file tree

Hide file tree

Showing 6 changed files with 40 additions and 41 deletions.
diff --git a/changelogs/unreleased/6283-Lyndon-Li b/changelogs/unreleased/6283-Lyndon-Li
@@ -0,0 +1 @@
+Fix issue #5875. Since Kopia has supported IAM, Velero should not require static credentials all the time
diff --git a/pkg/repository/config/aws.go b/pkg/repository/config/aws.go
@@ -35,6 +35,8 @@ const (
 	awsCredentialsFileEnvVar = "AWS_SHARED_CREDENTIALS_FILE"
 )
 
+var AWSCredFileMissingError = errors.New("missing credential file")
+
 // GetS3ResticEnvVars gets the environment variables that restic
 // relies on (AWS_PROFILE) based on info in the provided object
 // storage location config map.
@@ -61,7 +63,7 @@ func GetS3Credentials(config map[string]string) (credentials.Value, error) {
 	}
 
 	if credentialsFile == "" {
-		return credentials.Value{}, errors.New("missing credential file")
+		return credentials.Value{}, AWSCredFileMissingError
 	}
 
 	creds := credentials.NewSharedCredentials(credentialsFile, "")

diff --git a/pkg/repository/provider/unified_repo.go b/pkg/repository/provider/unified_repo.go
@@ -54,7 +54,7 @@ var getAzureStorageDomain = repoconfig.GetAzureStorageDomain
 
 type localFuncTable struct {
 	getStorageVariables   func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error)
-	getStorageCredentials func(*velerov1api.BackupStorageLocation, credentials.FileStore) (map[string]string, error)
+	getStorageCredentials func(*velerov1api.BackupStorageLocation, credentials.FileStore, logrus.FieldLogger) (map[string]string, error)
 }
 
 var funcTable = localFuncTable{
@@ -344,7 +344,7 @@ func (urp *unifiedRepoProvider) GetStoreOptions(param interface{}) (map[string]s
 		return map[string]string{}, errors.Wrap(err, "error to get storage variables")
 	}
 
-	storeCred, err := funcTable.getStorageCredentials(repoParam.BackupLocation, urp.credentialGetter.FromFile)
+	storeCred, err := funcTable.getStorageCredentials(repoParam.BackupLocation, urp.credentialGetter.FromFile, urp.log)
 	if err != nil {
 		return map[string]string{}, errors.Wrap(err, "error to get repo credentials")
 	}
@@ -391,7 +391,7 @@ func getStorageType(backupLocation *velerov1api.BackupStorageLocation) string {
 	}
 }
 
-func getStorageCredentials(backupLocation *velerov1api.BackupStorageLocation, credentialsFileStore credentials.FileStore) (map[string]string, error) {
+func getStorageCredentials(backupLocation *velerov1api.BackupStorageLocation, credentialsFileStore credentials.FileStore, log logrus.FieldLogger) (map[string]string, error) {
 	result := make(map[string]string)
 	var err error
 
@@ -419,14 +419,16 @@ func getStorageCredentials(backupLocation *velerov1api.BackupStorageLocation, cr
 	switch backendType {
 	case repoconfig.AWSBackend:
 		credValue, err := getS3Credentials(config)
-		if err != nil {
+		if err == nil {
+			result[udmrepo.StoreOptionS3KeyID] = credValue.AccessKeyID
+			result[udmrepo.StoreOptionS3Provider] = credValue.ProviderName
+			result[udmrepo.StoreOptionS3SecretKey] = credValue.SecretAccessKey
+			result[udmrepo.StoreOptionS3Token] = credValue.SessionToken
+		} else if err == repoconfig.AWSCredFileMissingError {
+			log.Warn("AWS credential file is not present, so no credentials are provided")
+		} else {
 			return map[string]string{}, errors.Wrap(err, "error get s3 credentials")
 		}
-		result[udmrepo.StoreOptionS3KeyID] = credValue.AccessKeyID
-		result[udmrepo.StoreOptionS3Provider] = credValue.ProviderName
-		result[udmrepo.StoreOptionS3SecretKey] = credValue.SecretAccessKey
-		result[udmrepo.StoreOptionS3Token] = credValue.SessionToken
-
 	case repoconfig.AzureBackend:
 		storageAccount, accountKey, err := getAzureCredentials(config)
 		if err != nil {

diff --git a/pkg/repository/provider/unified_repo_test.go b/pkg/repository/provider/unified_repo_test.go
@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	awscredentials "github.com/aws/aws-sdk-go/aws/credentials"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -34,6 +35,8 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/repository/udmrepo"
 	reposervicenmocks "github.com/vmware-tanzu/velero/pkg/repository/udmrepo/mocks"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
+
+	repoconfig "github.com/vmware-tanzu/velero/pkg/repository/config"
 )
 
 func TestGetStorageCredentials(t *testing.T) {
@@ -144,6 +147,20 @@ func TestGetStorageCredentials(t *testing.T) {
 			expected:      map[string]string{},
 			expectedErr:   "error get s3 credentials: fake error",
 		},
+		{
+			name: "aws, credential file not exist",
+			backupLocation: velerov1api.BackupStorageLocation{
+				Spec: velerov1api.BackupStorageLocationSpec{
+					Provider: "velero.io/aws",
+					Config:   map[string]string{},
+				},
+			},
+			getS3Credentials: func(config map[string]string) (awscredentials.Value, error) {
+				return awscredentials.Value{}, repoconfig.AWSCredFileMissingError
+			},
+			credFileStore: new(credmock.FileStore),
+			expected:      map[string]string{},
+		},
 		{
 			name: "azure, Credential section exists in BSL",
 			backupLocation: velerov1api.BackupStorageLocation{
@@ -215,7 +232,7 @@ func TestGetStorageCredentials(t *testing.T) {
 				fileStore = tc.credFileStore
 			}
 
-			actual, err := getStorageCredentials(&tc.backupLocation, fileStore)
+			actual, err := getStorageCredentials(&tc.backupLocation, fileStore, velerotest.NewLogger())
 
 			require.Equal(t, tc.expected, actual)
 
@@ -581,7 +598,7 @@ func TestGetStoreOptions(t *testing.T) {
 				getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
-				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) {
+				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore, logrus.FieldLogger) (map[string]string, error) {
 					return map[string]string{}, errors.New("fake-error-3")
 				},
 			},
@@ -652,7 +669,7 @@ func TestPrepareRepo(t *testing.T) {
 				getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
-				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) {
+				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore, logrus.FieldLogger) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
 			},
@@ -672,7 +689,7 @@ func TestPrepareRepo(t *testing.T) {
 				getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
-				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) {
+				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore, logrus.FieldLogger) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
 			},
@@ -748,7 +765,7 @@ func TestForget(t *testing.T) {
 				getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
-				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) {
+				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore, logrus.FieldLogger) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
 			},
@@ -772,7 +789,7 @@ func TestForget(t *testing.T) {
 				getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
-				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) {
+				getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore, logrus.FieldLogger) (map[string]string, error) {
 					return map[string]string{}, nil
 				},
 			},

diff --git a/pkg/repository/udmrepo/kopialib/backend/s3.go b/pkg/repository/udmrepo/kopialib/backend/s3.go
@@ -36,16 +36,8 @@ func (c *S3Backend) Setup(ctx context.Context, flags map[string]string) error {
 		return err
 	}
 
-	c.options.AccessKeyID, err = mustHaveString(udmrepo.StoreOptionS3KeyID, flags)
-	if err != nil {
-		return err
-	}
-
-	c.options.SecretAccessKey, err = mustHaveString(udmrepo.StoreOptionS3SecretKey, flags)
-	if err != nil {
-		return err
-	}
-
+	c.options.AccessKeyID = optionalHaveString(udmrepo.StoreOptionS3KeyID, flags)
+	c.options.SecretAccessKey = optionalHaveString(udmrepo.StoreOptionS3SecretKey, flags)
 	c.options.Endpoint = optionalHaveString(udmrepo.StoreOptionS3Endpoint, flags)
 	c.options.Region = optionalHaveString(udmrepo.StoreOptionOssRegion, flags)
 	c.options.Prefix = optionalHaveString(udmrepo.StoreOptionPrefix, flags)

diff --git a/pkg/repository/udmrepo/kopialib/backend/s3_test.go b/pkg/repository/udmrepo/kopialib/backend/s3_test.go
@@ -36,21 +36,6 @@ func TestS3Setup(t *testing.T) {
 			flags:       map[string]string{},
 			expectedErr: "key " + udmrepo.StoreOptionOssBucket + " not found",
 		},
-		{
-			name: "must have access key Id",
-			flags: map[string]string{
-				udmrepo.StoreOptionOssBucket: "fake-bucket",
-			},
-			expectedErr: "key " + udmrepo.StoreOptionS3KeyID + " not found",
-		},
-		{
-			name: "must have access key",
-			flags: map[string]string{
-				udmrepo.StoreOptionOssBucket: "fake-bucket",
-				udmrepo.StoreOptionS3KeyID:   "fake-key-id",
-			},
-			expectedErr: "key " + udmrepo.StoreOptionS3SecretKey + " not found",
-		},
 	}
 
 	for _, tc := range testCases {