Conversation
3c8f941 to
1724326
Compare
| query_params = parse_qs(urlparse(url).query) | ||
| # Pre-signed S3 URIs are signed for "GET" and will result in a 403 when running | ||
| # HEAD on them, so download them all at once instead. | ||
| signed = "X-Amz-Signature" in query_params or "Signature" in query_params |
There was a problem hiding this comment.
is this based query param sniffing based on a standard? Or is this just an ad-hoc observation?
an alternative could be to try the HEAD and swallow its failure
There was a problem hiding this comment.
Actually, brought up by my colleague @pvbouwel :
in CDSE we do have a pre-signed URL implementation with X-Amz-Signature headers where HEAD is supported, so this PR would introduce a performance regression and reduce stability
There was a problem hiding this comment.
I don't think it's a good idea in general to swallow failures, it's usually the easy fix in the short-term but with time you run into other problems that would have been discovered/fixable if it wasn't for the masking of failures.
Amazon doesn't support third-party S3 implementations, so I doubt they have made any kind of official standard, but the X-Amz-Signature is from S3 Signature Version 4 (https://docs.aws.amazon.com/AmazonS3/latest/developerguide/bucket-policy-s3-sigv4-conditions.html). CloudFlare mentions the X-Amz-Signature in the examples in their documentation though: https://developers.cloudflare.com/r2/api/s3/presigned-urls/
The Signature is from Version 2 and it's what I get with our boto3 configuration. I'm sure it's somewhere in the AWS SDK documentation, but their documentation is massive so I can't find a link.
Putting in "signature version 4 url example" into the search box at the Amazon documentation puts out this as example though:
https://examplebucket.s3.amazonaws.com/test.txt
?X-Amz-Algorithm=AWS4-HMAC-SHA256
&X-Amz-Credential=AKIAIOSFODNN7EXAMPLE%2F20130524%2Fus-east-1%2Fs3%2Faws4_request
&X-Amz-Date=20130524T000000Z
&X-Amz-Expires=86400
&X-Amz-SignedHeaders=host
&X-Amz-Signature=<signature-value>
and "signature version 2 url example" in the search box at Amazon documentation puts out this as example:
GET /photos/puppy.jpg
?AWSAccessKeyId=AKIAIOSFODNN7EXAMPLE&Expires=1141889120&Signature=vjbyPxybdZaNmGa%2ByT272YEAiv4%3D HTTP/1.1
Host: awsexamplebucket1.s3.us-west-1.amazonaws.com
Date: Mon, 26 Mar 2007 19:37:58 +0000
If you're worried that Amazon will suddenly change the existing parameter names in version 2/version 4, I don't think that is a big risk since that will make all the current pre-signed URIs in their system invalid, but there's obviously no guarantee that the next signature version will use the same parameter name as version 2/4. It's also quite possible that version 4 will be the latest and greatest for many years since I assume they had competent people doing the design.
There was a problem hiding this comment.
@pvbouwel I'm quite curious, could you please explain a bit more about your server/storage setup? Have you reconfigured Ceph to admit signed URLs even for HEAD requests, or are you running some other S3 server? And why would a non-ranged request reduce stability/performance?
There was a problem hiding this comment.
Hey @pjonsson,
We actually run against different storage backends which have different levels of support for features. In order to harmonize it and to allow using features that we are not supported on all of our backends we have a proxy layer. This proxy allows vending temporary credentials and a enforcing IAM policies (even for backends where that functionality is not available. It supports SigV4 signing and pre-signed links but since it itself does the authN/authZ it also allows a special flavour of pre-signed URLs where a user can sign an url with a query parameter X-Proxy-Head-As-Get which would result in a pre-signed URL that can be used for both HEAD and GET requests.
Non-ranged requests reduce stability/performance because they correspond to:
- longer download times
- which for example can give problems when a backend node is removed it generally stops accepting new requests and stays for a limited amount of time to round-up ongoing requests.
- also gives a longer exposure window for intermittent issues
- bigger downloads
- This can give problems with network components on route between client and server. These components are out of our control as a service provider (e.g. a transparent proxy on the client side which downloads the file and scans it content could first download the whole file and only then after assuring it safe passes it on to the client for big downloads this could leave the client hanging for to long triggering timeout mechanisms)
There was a problem hiding this comment.
Also on a more general note: the python client must be back-end agnostic, so we should not bake in any S3 related assumptions about how download links are constructed or handled. We can only work on standards concerning ranged downloading
There was a problem hiding this comment.
@pvbouwel I'm not sure how popular your proxy is among commercial cloud providers, but I'm glad the signed URIs work for your situation.
Are you saying that downloading a small part of a big file is why the stability/performance is improved, or are you making some kind of statistical argument for why downloading the entirety of a big file in chunks is better for stability/performance? That it's faster/better to download 5k instead of 50MB is obvious to me, but my intuition about downloading a 50MB file in 5k chunks isn't that it would be better, the transfer time would roughly be the same if we ignore the size of the extra HTTP headers and various network details like time for establishing new connections, TCP slow start, and so on. (File and chunk size were arbitrary choices to convey my example, I'm aware that a 5k chunk size is probably a bad choice in practice.)
There was a problem hiding this comment.
@pjonsson ; I indeed do not want to claim that downloading a file chunked will be faster end-to-end (especially not with the approach of iterating over chunks rather than performing parallel requests). But the intermediate steps last less long and connection that last longer have different ways of being cut due to network constraints outside our control as a service provider. For 2 digit MB files our users rarely encounter issues but for artifacts exceeding a size of 1GB it was not uncommon to get user contacts where they were not able to download the results.
So with the initial PR those users would experience a regression for our deployments that go through our S3proxy because it would see the query parameter and do a single download.
Your rework would work with my S3Proxy and would avoid regression but as Stefaan points out might provide issues with other backends.
So the try catch he mentions is a quick fix if their is high urgency.
A more involved solution could be that you change that initial get to specify the range with the chunk size and then check:
- where ranges supported?
- If not then a 200 was returned then you know you would have the full body already downloaded so make sure to store that
- If yes either a 206 was returned and chunked size corresponds with the advertised full size then artifact was smaller or equal to chunk size so also no additional fetches needed
- If yes either a 206 was returned but not having full size, continue iteration to get more chunks
- If yes either 416 fallback like with the range 0:0
That should work regardless of backend and even optimize the flow for artifacts < chunk_size
|
bit short on time here, so quick summary/brainstorm:
The |
e068f2f to
04508b3
Compare
| from_bytes = int(search.group(1)) | ||
| to_bytes = int(search.group(2)) | ||
| assert from_bytes < to_bytes | ||
| assert from_bytes <= to_bytes |
There was a problem hiding this comment.
Change isn't directly related to this PR, except that these tests fail because the previous assert is too strict.
Running HEAD on a pre-signed S3 URI gives a 403 response because the URI is signed for GET. Use a GET with a Range 0-0 request instead of HEAD to figure out if the server accepts range requests. Fixes Open-EO#939
04508b3 to
a17ea61
Compare
| return TIFF_CONTENT[from_bytes : to_bytes + 1] | ||
| assert from_bytes <= to_bytes | ||
| sliced_content = TIFF_CONTENT[from_bytes : to_bytes + 1] | ||
| context.status_code = 206 |
There was a problem hiding this comment.
Unless I'm misunderstanding something, the mock didn't respond with the right headers/status code for range requests earlier.
It's not necessarily what you had in mind, but I think I've implemented this. |
soxofaan
left a comment
soxofaan
left a comment
There was a problem hiding this comment.
I'm still thinking doing try: head, except: pass (swallow failure) is the easiest quickfix for this problem if you want a quick solution
doing it smarter (without a try) isn't as trivial as it seems I'm afraid
(but still on the table as a longer term solution of course)
| # to figure out if the server supports range requests. | ||
| # Trying to GET a 0-byte file will give a 416-response, so accept that | ||
| # since the download will succeed. | ||
| head = self.get(url, headers={"Range": "bytes=0-0"}, expected_status=[200, 206, 416], stream=True) |
There was a problem hiding this comment.
I'm not sure this doing a GET with 0-range headers is a good idea: this will only work if the server supports range. If the server doesn't then the user will download the same, possibly large, payload twice (once for this head-emulation and once for doing the actual download a couple of lines lower in _download_all_at_once
Ok, I've marked this as a draft and opened #943 with the quick fix. I managed to narrow the scope of the changes to the HEAD request further so everything except pre-signed S3 URIs behave exactly the same as before, and pre-signed S3 URIs that get a 403 response from HEAD will try to do the full download. Non-pre-signed URIs and pre-signed URIs running against VITO's proxy will use the same chunked download as before.
Yes, I had a look at the refactoring before updating this yesterday and the refactoring is more invasive than one would hope. Edit: I also split out the test mock updates to a separate PR (#944) since they are just general improvements in the test machinery and weren't really related to this PR. |
|
overruled by #943 I think |
3 participants
Running HEAD on a pre-signed S3 URI
gives a 403 response because the URI
is signed for GET. Use a GET with
a Range 0-0 request instead of HEAD
to figure out if the server accepts
range requests.
Fixes #939