Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 5875 #6283

Merged
merged 2 commits into from May 22, 2023
Merged

Fix issue 5875 #6283

merged 2 commits into from May 22, 2023

Conversation

Lyndon-Li
Copy link
Contributor

Fix issue #5875. Since Kopia has supported IAM, Velero should not require static credentials all the time

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2023

Codecov Report

Merging #6283 (660fbfa) into main (b891074) will decrease coverage by 0.03%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #6283      +/-   ##
==========================================
- Coverage   41.16%   41.14%   -0.03%     
==========================================
  Files         252      252              
  Lines       23530    23529       -1     
==========================================
- Hits         9686     9680       -6     
- Misses      13083    13088       +5     
  Partials      761      761              
Impacted Files Coverage Δ
pkg/backup/backup_pv_action.go 50.00% <ø> (ø)
pkg/repository/config/aws.go 22.72% <0.00%> (-1.67%) ⬇️
pkg/repository/udmrepo/kopialib/backend/s3.go 28.57% <0.00%> (-23.29%) ⬇️
pkg/repository/provider/unified_repo.go 54.91% <100.00%> (+0.24%) ⬆️

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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this happens, then there is no credential information in the result, but the IAM way also mounts the credential information into the Velero pod.
Is there a place where the Kopia Library can read credential information?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Also curious how kopia handles different auth mechanisms.

Copy link
Contributor Author

@Lyndon-Li Lyndon-Li May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kopia leverage on minio client to get credentials through IAM, the later queries the related environment variable AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE and then make the http call to get the credentials.
Kopia does few things like letting it go without checking static ak/sk and setting the http client for minio.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Velero needs is letting it go without checking static ak/sk as well.
For sure, the IAM role SA should be mounted to Velero pods, this has been done by another PR #5802

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, it looks weird to let it go without checking anything when getting credentials, so here I have modified the code to check the existence of the env variable AWS_ROLE_ARN, only when its value is not empty, we let it go with empty ak/sk

@Lyndon-Li Lyndon-Li force-pushed the issue-fix-5875 branch 2 times, most recently from f4e0b78 to d500965 Compare May 18, 2023 07:47
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@reasonerjt reasonerjt merged commit 5cb7217 into vmware-tanzu:main May 22, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants