fix issue 5875

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

Hide file tree

Showing 6 changed files with 38 additions and 40 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
@@ -54,23 +54,27 @@ func GetS3ResticEnvVars(config map[string]string) (map[string]string, error) {
 
 // GetS3Credentials gets the S3 credential values according to the information
 // of the provided config or the system's environment variables
-func GetS3Credentials(config map[string]string) (credentials.Value, error) {
+func GetS3Credentials(config map[string]string) (*credentials.Value, error) {
+	if len(os.Getenv("AWS_ROLE_ARN")) > 0 {
+		return nil, nil
+	}
+
 	credentialsFile := config[CredentialsFileKey]
 	if credentialsFile == "" {
 		credentialsFile = os.Getenv("AWS_SHARED_CREDENTIALS_FILE")
 	}
 
 	if credentialsFile == "" {
-		return credentials.Value{}, errors.New("missing credential file")
+		return nil, errors.New("missing credential file")
 	}
 
 	creds := credentials.NewSharedCredentials(credentialsFile, "")
 	credValue, err := creds.Get()
 	if err != nil {
-		return credValue, err
+		return nil, err
 	}
 
-	return credValue, nil
+	return &credValue, nil
 }
 
 // GetAWSBucketRegion returns the AWS region that a bucket is in, or an error

diff --git a/pkg/repository/provider/unified_repo.go b/pkg/repository/provider/unified_repo.go
@@ -422,11 +422,13 @@ func getStorageCredentials(backupLocation *velerov1api.BackupStorageLocation, cr
 		if err != nil {
 			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
 
+		if credValue != nil {
+			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
@@ -44,7 +44,7 @@ func TestGetStorageCredentials(t *testing.T) {
 		credStoreError      error
 		credStorePath       string
 		getAzureCredentials func(map[string]string) (string, string, error)
-		getS3Credentials    func(map[string]string) (awscredentials.Value, error)
+		getS3Credentials    func(map[string]string) (*awscredentials.Value, error)
 		getGCPCredentials   func(map[string]string) string
 		expected            map[string]string
 		expectedErr         string
@@ -88,8 +88,8 @@ func TestGetStorageCredentials(t *testing.T) {
 					},
 				},
 			},
-			getS3Credentials: func(config map[string]string) (awscredentials.Value, error) {
-				return awscredentials.Value{
+			getS3Credentials: func(config map[string]string) (*awscredentials.Value, error) {
+				return &awscredentials.Value{
 					AccessKeyID: "from: " + config["credentialsFile"],
 				}, nil
 			},
@@ -114,8 +114,8 @@ func TestGetStorageCredentials(t *testing.T) {
 			},
 			credFileStore: new(credmock.FileStore),
 			credStorePath: "credentials-from-credential-key",
-			getS3Credentials: func(config map[string]string) (awscredentials.Value, error) {
-				return awscredentials.Value{
+			getS3Credentials: func(config map[string]string) (*awscredentials.Value, error) {
+				return &awscredentials.Value{
 					AccessKeyID: "from: " + config["credentialsFile"],
 				}, nil
 			},
@@ -137,13 +137,27 @@ func TestGetStorageCredentials(t *testing.T) {
 					},
 				},
 			},
-			getS3Credentials: func(config map[string]string) (awscredentials.Value, error) {
-				return awscredentials.Value{}, errors.New("fake error")
+			getS3Credentials: func(config map[string]string) (*awscredentials.Value, error) {
+				return nil, errors.New("fake error")
 			},
 			credFileStore: new(credmock.FileStore),
 			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 nil, nil
+			},
+			credFileStore: new(credmock.FileStore),
+			expected:      map[string]string{},
+		},
 		{
 			name: "azure, Credential section exists in BSL",
 			backupLocation: velerov1api.BackupStorageLocation{

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 {