Add support for temporary AWS credentials to access S3#693
Add support for temporary AWS credentials to access S3#693rapids-bot[bot] merged 17 commits intorapidsai:branch-25.06from
Conversation
This changes the API for constructing an S3Endpoint so this is a breaking change for code that uses the C++ API. The python layer around it does not use the parameters to pass the credentials (it relies on the env vars), so it is unaffected.
kingcrimsontianyu
added
feature request
|
I think the Python part could be briefly updated in this PR. Currently we only deal with the required parameters We may consider the support for programatically setting the optional aws-related parameters in Python in a future PR if there is a need (in addition to the current env var approach). |
|
Thank you for the comments @kingcrimsontianyu, I will address them tomorrow! |
|
/ok to test 78e519b |
|
@joosthooz I'll add a few more suggestions in a couple of hours to fix the CI issue. Thank you! |
cpp/include/kvikio/remote_handle.hpp
Outdated
| std::optional<std::string> aws_region = std::nullopt, | ||
| std::optional<std::string> aws_access_key = std::nullopt, | ||
| std::optional<std::string> aws_secret_access_key = std::nullopt, | ||
| std::optional<std::string> aws_session_token = std::nullopt, |
There was a problem hiding this comment.
Can we add the new parameter to the end of the argument list, to avoid an API break? The types of aws_session_token and aws_endpoint_url are identical, so users of the old API will now be passing an endpoint as if it was a session token.
There was a problem hiding this comment.
I've made the change. It would be desirable to keep them grouped together but a hidden breaking API change sounds troublesome.
There was a problem hiding this comment.
Turns out I only changed the order in the .cpp file, should be fixed now! Also added missing documentation for the new parameter to that function.
# Conflicts: # cpp/src/remote_handle.cpp
|
/ok to test |
@kingcrimsontianyu, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test 416a004 |
kingcrimsontianyu
left a comment
kingcrimsontianyu
left a comment
There was a problem hiding this comment.
Please address https://github.com/rapidsai/kvikio/pull/693/files#r2063590434 so as to fix the CI issue.
|
Sorry @kingcrimsontianyu , I was certain I had removed that include but it turns out I didn't! Should be good now. |
|
/ok to test |
@kingcrimsontianyu, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test bfb9a65 |
kingcrimsontianyu
left a comment
kingcrimsontianyu
left a comment
There was a problem hiding this comment.
Thanks for raising the issue and coming up with a solution!
|
/merge |
|
Looks like this may have broken the cudf build. |
|
Could you elaborate a bit? I'm not able to reproduce the build fail locally with CUDA 12.8 pip cuDF. |
|
Sorry, I meant to add a link to the CI failures that are now occurring |
5 participants
This PR adds the
x-amz-security-tokenheader which is needed when using temporary credentials (These start withASIA). According to the curl docs, the list of headers needs to be freed after use, So I added a destructor toS3Endpointthat does that.Important to note is that I added a parameter for the session token, but it comes directly after the other credential parameters and before the endpoint override, making this a breaking change for code that interacts directly with these C++ APIs! But it makes it consistent with the other values; otherwise 1 of them would need to be passed only via the env var.
It seems there might be another API change here #700 so maybe both changes can be done in the same release.
I hope this is useful for others too, looking forward to any comments!
Fixes #584 (@TomAugspurger)