MIDRC-1226 STREAMING-UNSIGNED-PAYLOAD-TRAILER support#102
MIDRC-1226 STREAMING-UNSIGNED-PAYLOAD-TRAILER support#102paulineribeyre merged 25 commits intomasterfrom
Conversation
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
paulineribeyre
force-pushed
the
debug-chunk
branch
from
28f15e5 to
74c5206
Compare
|
|
||
| def _image_to_regex(image: str) -> str: | ||
| """ | ||
| r""" |
There was a problem hiding this comment.
Fix for SyntaxWarning: invalid escape sequence '\Z'
|
Test summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
nss10
left a comment
nss10
left a comment
There was a problem hiding this comment.
Couple of questions before approving
gen3workflow/routes/s3.py
Outdated
| # get the name of the user's bucket and ensure the user is making a call to their own bucket | ||
| logger.info(f"Incoming S3 request from user '{user_id}': '{request.method} {path}'") | ||
| user_bucket = aws_utils.get_safe_name_from_hostname(user_id) | ||
| if request.method == "GET" and path == "s3": |
There was a problem hiding this comment.
Oh! Can someone make a request to get the names of all the buckets in an account when they interact via gen3-workflow? Would it be possible to add a test around it? And will the path variable be exactly equal to s3.
Also, what would happen when someone tries ls s3://{someone else's bucket}
There was a problem hiding this comment.
Yes the path is exactly "s3". Right now listing the buckets doesn't work, i tried to get it working and only return the user's bucket, but it wasn't easy and i thought it's not worth the trouble, since "ls s3://{user_bucket}" works.
"ls s3://{someone else's bucket}" would return a 403 through the same check as other calls (L188).
I'll have a look at adding a unit test for ls
There was a problem hiding this comment.
Turns out the path is only "s3" when hitting "/s3", and it's otherwise empty. I added a commit to reject all "list all buckets" requests + a unit test.
"ls s3://{user_bucket}" and "ls s3://{someone else's bucket}" are already tested by existing unit tests, for example this one
| logger.debug( | ||
| f"Received a failure status code from AWS: {response.status_code}. {response.text=}" | ||
| ) | ||
| logger.debug(f"Received a failure status code from AWS: {response.status_code}") |
There was a problem hiding this comment.
What do we get in the {response.text=} field? is not useful?
There was a problem hiding this comment.
it's also logged on L347 so it's duplicated, except in the case of a 404, in which case we don't need extra details
Pull Request Test Coverage Report for Build 22506519115Details
💛 - Coveralls |
nss10
left a comment
nss10
left a comment
There was a problem hiding this comment.
Great work! Just one more question. I can approve right after that :)
| # "All buckets" listing requests also land here and are not supported, since users can only | ||
| # access their own bucket. | ||
| if request.method == "GET" and path in ("", "s3"): | ||
| err_msg = f"If you are using the S3 endpoint: 's3 ls' not supported, use 's3 ls s3://<your bucket>' instead. If you are trying to reach the Gen3-Workflow API, try '/_status'." |
There was a problem hiding this comment.
Okay! I see we are explicitly asking them to try /_status if they need it. Can't we still support get_status when path is empty? Do you think that would be complicated?
There was a problem hiding this comment.
It gets confusing because if you use "aws s3 ls" against the S3 endpoint exposed at root, the AWS CLI can't parse the response from "get status"
|
|
||
|
|
||
| @pytest.mark.parametrize("client", [{"get_url": True}], indirect=True) | ||
| def test_s3_endpoint_list_buckets(s3_client): |
3 participants
Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/MIDRC-1226
New Features
Breaking Changes
/anymore, only at/_statusBug Fixes
Improvements
Dependency updates
Deployment changes