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

feat: support session tokens in S3 client #596

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

hittingray
Copy link
Contributor

@hittingray hittingray commented Jan 4, 2024

This PR adds support for the S3 client to use ephemeral IAM credentials (e.g. an access/secret key pair + session token obtain through an assume role operation) or to use the new S3 access grants feature.

This is a pre-requisite to argoproj/argo-workflows#5446

The relevant PR in Argo Workflows is here: argoproj/argo-workflows#12467

I found the contributing guidelines for Argo Workflows, but couldn't find anything for this specific repo, so please let me know if anything is missing. Thank you!

@hittingray
Copy link
Contributor Author

I pretty much duplicated a test for the new ephemeral credentials which looks like it's failing the duplicated code quality gate. I've just pushed up a new commit which minimises the number of options checked in the test.

This comment was marked as resolved.

Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

code seems sensible, have you tested this end to end though ?

@isubasinghe
Copy link
Member

@alexmt @crenshaw-dev @sarabala1979 could one of you also please help review and merge this?

@hittingray
Copy link
Contributor Author

hittingray commented Feb 21, 2024

code seems sensible, have you tested this end to end though ?

@isubasinghe Yep, it's been tested e2e. We are currently running the fork of Argo Workflows I have in argoproj/argo-workflows#12467 and it works correctly.

@keymon

This comment was marked as spam.

@Joibel
Copy link
Member

Joibel commented Jun 21, 2024

This is blocked on the two CI checks which don't seem to have run. They don't seem to run for other changes too.

@hittingray
Copy link
Contributor Author

Is there a maintainer who would be able to kick off the builds? I don't believe I have permission to

@keymon
Copy link

keymon commented Jun 25, 2024 via email

@hittingray
Copy link
Contributor Author

Still the same. I had tried this before and it still requires workflow approval from a maintainer.

@Joibel
Copy link
Member

Joibel commented Jun 25, 2024

The rules in this repo are not the same as in the main repositories, I've set it running for you

@Joibel Joibel merged commit 8be8da6 into argoproj:master Jun 25, 2024
4 checks passed
@hittingray
Copy link
Contributor Author

Awesome, thanks @Joibel!

@keymon I'll get to the Workflows PR some time in the next couple days

@agilgur5 agilgur5 changed the title feat: Add support for session tokens to S3 client feat: support session tokens in S3 client Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants