Skip to content

Conversation

MartinKlefas
Copy link
Contributor

To make sure I'm targeting the right region (specifically, eu-west-2), I've explicitly set the region_name parameter during the boto3 client initialization. Without this setting, there's a risk the client could default to us-east-1 or pick up a region from system AWS configuration files.

By being explicit about the region_name, I've ensured the client recognizes the intended region, which should lead to the generation of presigned URLs that correctly match the desired AWS zone. I've tried to read the region from the bucket URL and tried to do that for a variety of common providers. It could also be addressed via an environment variable if that brings it inline better with your design philosophy

This PR addresses #126

@justinmerrell
Copy link
Contributor

May you address the Code scanning results / CodeQL results?

@MartinKlefas
Copy link
Contributor Author

May you address the Code scanning results / CodeQL results?

I've done that now - the code still fails the pytest though, as "https://your-bucket-endpoint-url.com'" doesn't contain a valid s3 compliant URL and so there's no region to be found and submitted in the boto3 client initialisation.

I don't know if it's possible to change pytest so that it submits a compliant dummy url: https://testbucket.s3.testregion.amazonaws.com or something?

@justinmerrell
Copy link
Contributor

Thank you for the correction, I see that I need to add the trigger to run that workflow on PRs as well.

Do you have screenshots of the exact test error?

@MartinKlefas
Copy link
Contributor Author

Please see below:
image
I suppose a good workaround would be to return the default region from env if the use case isn't found, rather than None?

@justinmerrell
Copy link
Contributor

Oh, I see, it is just the assertion test that needs to be updated to include "region_name=None" for the expected call.

@MartinKlefas
Copy link
Contributor Author

Oh, I see, it is just the assertion test that needs to be updated to include "region_name=None" for the expected call.

Is that something I should be doing? Sorry I'm a bit new to all this!

@justinmerrell
Copy link
Contributor

Feel free to take a crack at it; otherwise, I'd be glad to take the PR over the finish line. Your contribution is already greatly appreciated!

In the rp_upload test file there should be something like "assert called with" and then has the client input but will be missing the region_name

@MartinKlefas
Copy link
Contributor Author

Feel free to take a crack at it; otherwise, I'd be glad to take the PR over the finish line. Your contribution is already greatly appreciated!

In the rp_upload test file there should be something like "assert called with" and then has the client input but will be missing the region_name

I've made those changes now, but not sure how to get that other end-to-end test to run - the error coming up is about docker usernames:
Run docker/login-action@v3
Error: Username and password required
Not sure how or where to put in dummy ones to get the test to pass I'm afraid.

@justinmerrell justinmerrell merged commit d47d9c5 into runpod:main Sep 19, 2023
@justinmerrell
Copy link
Contributor

That last test needs to be run from our account. PR looks good; thanks for the submission!

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.

2 participants