-
Notifications
You must be signed in to change notification settings - Fork 670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add extra header to signed url #4971
Conversation
Signed-off-by: Kevin Su <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4971 +/- ##
==========================================
+ Coverage 58.97% 59.00% +0.02%
==========================================
Files 645 645
Lines 55561 55578 +17
==========================================
+ Hits 32766 32792 +26
+ Misses 20200 20194 -6
+ Partials 2595 2592 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
cc @ddl-ebrown @vsbus as well |
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Thanks for taking this on so quickly @pingsutw ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I've two comments:
- Maintain the right abstractions,
storage
package should be the one handling the different behaviors between different storage backends. If you find yourself adding half the logic within one package and the other half in another package, something smells... - Backward compatibility:
a. Overwriting files in GCS and Azure just never worked... no need to attempt to maintain backward compatibility here.
b. We should always add the ContentMD5 tag in all storage providers when CreateSignedURL is called
c. When the SDK calls CreateSignedURL, we should check if the target exists and has that tag set, we should compare the request's MD5 to that value.
d. When the SDK calls CreateSignedURL, if the target doesn't have MD5 tag set, We should compare it against ETag for S3 and just fail for all others (existing behavior)
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Why are the changes needed?
metadata.etag
does not equal contentMD5.What changes were proposed in this pull request?
x-amz-meta-flyteContentMD5
,x-goog-meta-flyteContentMD5
, andx-ms-meta-flyteContentMD5
) to signed URLflyteContentMD5
is equal to ContentMD5How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Blocked by flyteorg/stow#13
Docs link
NA