feat(artifacts): support ephemeral credentials for S3. Fixes #5446

[hittingray]

Fixes #5446

Motivation

We would like to use the new S3 access grants feature, which uses only temporary credentials. The aforementioned issue addresses temporary credentials in general (prior to recently though, this would have only been for IAM role).

Modifications

Firstly, some changes needed to be made in the Argo common pkg repo: argoproj/pkg#596
I am still waiting for these changes to be reviewed and therefore still have the forked dependency in my go.mod. This is quite a small set of changes, as support for temporary credentials was already added to the AWS client in general previously.

Inside the Argo Workflows repo, I have made changes to how the S3 artifact driver is initialised, which mainly includes looking for the session token and using it if applicable. There is also a change on the workflow pod side to fetch the relevant secret if applicable.

Verification

I have written a couple unit tests to verify the driver and workflow creation behaviour. There was no test for creation of the driver, so I took the liberty of writing a small test to ensure the behaviour of specifically the changes I made. For the changes in s3.go, the s3client struct is not exported from argoproj/pkg, so it made testing the changes I made impossible. I feel the changes made in this file are fairly small anyway.

I have also tested the changes by attempting to write into a S3 bucket in a workflow, using both IAM role and S3 access grant credentials. The behaviour is as expected; and when the credentials are invalid, or expired, an appropriate error message is returned.

[isubasinghe]

Please see the comments made, generally looks good though

[hittingray]

@keymon Still waiting on the argo/pkg PR which you already commented on. There's no point me making the changes requested until that one has been merged in 😞

[hittingray]

@isubasinghe I've updated the PR, would appreciate a review when you have time, thank you!

[hittingray]

@isubasinghe Sorry to ping again, but would be good if you could review the changes here. The PR has been sitting around for ages and I'd like to avoid having to address merge conflicts in the code gen again.

[keymon]

Yes, please, add support for this as it blocks some local development and unit testing

argoproj/pkg#596 has been merged and this is ready to merge!

[isubasinghe]

@hittingray will re-review this tomorrow.

[hittingray]

@isubasinghe I've rebased it on latest main just now. It seems like the Windows unit tests in http_test.go are failing. I haven't touched anything there, so not sure what the problem is. Flakey test or something wrong with the test setup? Could we just re-run the Windows tests?

Edit: Seems like just pushing another commit to trigger it fixed it, so flakey test it is.

[isubasinghe]

@hittingray Don't stress about the windows tests, they are broken and are not required for merging.

[hittingray]

@isubasinghe No worries then. It's ready for your review otherwise then :)

[isubasinghe]

Just some minor changes

[isubasinghe]

@agilgur5 can you review the documentation for me, you are a bit more knowledgeable there about conventions.

[agilgur5]

Docs review in-line below as requested.
Note that this page has yet to be fully re-written to match the style guide, so some of the style you copied from here is actually non-conformant. A more recent example to copy from is #13376

[agilgur5]

There's still a few more sections I need to think about the edits for, but the in-line comments cover 80-90% (enough to iterate on)

[isubasinghe]

@hittingray could you please make the suggested changes?

[agilgur5]

This looks much better, thanks for improving the docs greatly!

I left a few more comments on things that were missed, as well as the remaining sections I hadn't covered

[agilgur5]

Suggested change

Argo will not refresh them for you.

I had suggested removing this before as it is redundant with the above sentence.

We can leave it in if you feel strongly about it, but you hadn't commented there, so not sure if leaving this in intentional

[hittingray]

I'm ok to remove it. Just wanted to make it clear, but I guess the first sentence is already pretty clear.

[agilgur5]

Yea "external mechanism" == "not Argo". The most explicit way would be to recommend some tooling, but I actually don't know of any that do this out of the box

[hittingray]

Yeh, I don't know off the top of my head. For the access grants, we wrote our own admission controller which retrieves the credentials and injects them at workflow creation time, but it's not open source (for now).

[hittingray]

@agilgur5 Thanks for the reviews and sorry for all the back-and-forth! Fixed up again with your suggestions.

[agilgur5]

No worries, iteration is progress and progress is good!
I could also make the changes myself, but if we have a contributor willing to learn the docs style guide etc, I prefer to use a review as a teaching experience 🙂

[agilgur5]

Docs LGTM ✅

Thanks @hittingray for your dedication to getting this PR in over a long time period and pushing through all the iteration and upstream changes! ❤️

@isubasinghe you didn't approve this before, so this is now blocked on your previous change request

[isubasinghe]

LGTM

[agilgur5]

I did a partial backport/cherry-pick of the docs changes to existing features here to release-3.5 in 9b337f8 as I thought they were a substantial improvement.