From 124e142583c97ad06bd92b0dd1a2cc42969153e9 Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Wed, 10 May 2023 19:30:01 +0800 Subject: [PATCH] fix issue 5875 Signed-off-by: Lyndon-Li --- changelogs/unreleased/6283-Lyndon-Li | 1 + pkg/repository/config/aws.go | 12 +++++--- pkg/repository/provider/unified_repo.go | 10 ++++--- pkg/repository/provider/unified_repo_test.go | 28 ++++++++++++++----- pkg/repository/udmrepo/kopialib/backend/s3.go | 12 ++------ .../udmrepo/kopialib/backend/s3_test.go | 15 ---------- 6 files changed, 38 insertions(+), 40 deletions(-) create mode 100644 changelogs/unreleased/6283-Lyndon-Li diff --git a/changelogs/unreleased/6283-Lyndon-Li b/changelogs/unreleased/6283-Lyndon-Li new file mode 100644 index 0000000000..0464dda0e4 --- /dev/null +++ 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 \ No newline at end of file diff --git a/pkg/repository/config/aws.go b/pkg/repository/config/aws.go index a3c63b949c..17fc6d217e 100644 --- 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 index 65571044fc..7a6212f3e4 100644 --- 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 index 0a0401aecf..098e3a23bf 100644 --- 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 index 0a6f56bb63..b3891a36a3 100644 --- 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 index 3aa793a71c..c1f5e036b1 100644 --- 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 {