DM-42568: Handle zero-byte files in S3 presigned URLs #78
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
==========================================
+ Coverage 86.45% 86.52% +0.07%
==========================================
Files 27 27
Lines 4193 4201 +8
Branches 848 850 +2
==========================================
+ Hits 3625 3635 +10
+ Misses 447 445 -2
Partials 121 121 ☔ View full report in Codecov by Sentry. |
Fix an issue for presigned S3 HTTP URLs with a zero byte length where HttpResourcePath.exists() would always return False, and HttpResourcePath.size() would throw an exception. Because we emulate HEAD using a 1-byte GET request when checking S3 presigned HTTP URLs, the server returns 416 status instead of 206 because the range is longer than the file.
dhirving
force-pushed
the
tickets/DM-42568
branch
from
4c51664
to
fd452ff
Compare
| # byte file since we asked for 1 byte Range which is longer | ||
| # than the file. | ||
| # | ||
| # Servers are supposed to include a Content-Range header in |
There was a problem hiding this comment.
I will say in passing that we have in the past managed to get Google to fix their S3 server implementation.
What happens if you have a GCS signed URL? Same problem or does it really matter whether the URI was s3 or gs originally?
There was a problem hiding this comment.
Not sure -- I don't have a convenient way to generate signed GCS URLs.
The 416 thing isn't Google-specific and is standards-compliant behavior, btw, so this change isn't only necessary for Google compatibility. I just failed to consider what would happen with 0-byte files when I did the initial implementation.
The part that is non-compliant is that it doesn't also return a Content-Range including the size (specified as a SHOULD in the HTTP RFC). Unless something else really weird and non-compliant is happening that size will always be 0 so I don't think it's especially important.
(If the server doesn't want to or can't deal with the Range at all it's supposed to return 200 with the full content instead, which will result in correct behavior since we go down the branch that looks at Content-Length. Slightly rude to the server but probably mostly theoretical -- implementations of S3 will almost certainly support Range requests.)
It looks like the HttpResourceHandle also assumes it's at EOF and bails early without trying to look at Content-Range if it gets a 416, so I don't think it's a problem.
Fix an issue for presigned S3 HTTP URLs with a zero byte length where HttpResourcePath.exists() would always return False, and HttpResourcePath.size() would throw an exception.
Because we emulate HEAD using a 1-byte GET request when checking S3 presigned HTTP URLs, the server returns 416 status instead of 206 because the range is longer than the file.
Checklist
doc/changes