s3: Allow turning off checksums in S3 Puts #85
Conversation
sinkingpoint
force-pushed
the
sinkingpoint/allow_disabling_checksums
branch
from
66bb9ac to
053782a
Compare
There was a problem hiding this comment.
Generally it looks good. I am okay with the naming but let's see if other maintainers have other suggestions.
It would be better if we can have some doc/comment saying what is this configuration for and what S3 compatible backends might need to turn on/off this flag.
|
I am slightly worried that there could be many people out there facing this problem and they won't easily notice. I think we should include the md5 always and by default to try to avoid these situations. I personally don't foresee any issues with this. Do you think it could cause problems, @sinkingpoint @yeya24? |
|
This problem also exists for Backblaze B2. This change would be super-useful as currently we can not use Thanos with B2 and multipart. Forcing MD5 is also ok for us, but I guess someone that use AWS S3 could benefit from |
|
IMHO |
sinkingpoint
force-pushed
the
sinkingpoint/allow_disabling_checksums
branch
from
053782a to
22cac33
Compare
|
Updated the name to be the same as minio. That makes sense - its a lot more explicit |
|
Defaulting to true, to disable the header by default seems like a good idea - technically speaking that would restore the previous behavior because otherwise the prior PR was a breaking change |
|
I see it is still set to false. Do we want to make it default to true before I merge this |
|
I'm not a big fan of how they handle these things over at Minio: minio/minio-go#1791. I'm 100% in favor of this project keeping the compatibility with as many s3-compatible object storage providers as possible (that's the whole purpose) without requiring users to manually disable some "random" flag because it messes up everything else besides s3 and minio itself. So I think we should set that flag to |
sinkingpoint
force-pushed
the
sinkingpoint/allow_disabling_checksums
branch
from
22cac33 to
bddc107
Compare
thanos-io/thanos#6746 bumped objstore, which bumped minio, which made Thanos incompatible with otherwise compliant S3 backends that do not support the x-amz-checksum header. This adds in a `PutWithMD5` (open to naming changes) config entry to the S3 bucket config that allows reverting back to MD5 checksums which _are_ supported. Signed-off-by: sinkingpoint <[email protected]>
sinkingpoint
force-pushed
the
sinkingpoint/allow_disabling_checksums
branch
from
bddc107 to
5ce1435
Compare
|
Added the |
|
for reference, Cloudflare r2 doesn't seem to need this any more |
thanos-io/thanos#6746 bumped objstore, which bumped minio, which made Thanos incompatible with otherwise compliant S3 backends that do not support the x-amz-checksum header.
Changes
PutWithMD5(open to naming changes) config entry to the S3 bucket config that allows turning off this header so that Thanos supports these backends.Verification
Running this at Cloudflare on our r2 backed Thanos that doesn't support this header