Skip to content
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

FIX: don't use etags for post-upload verification #21923

Merged
merged 1 commit into from Jul 7, 2023

Conversation

mpalmer
Copy link
Contributor

@mpalmer mpalmer commented Jun 5, 2023

They don't work for server-side encryption with customer keys, and so instead we just use Content-MD5 to ensure there was no corruption in transit, which is the best we can do.

See also: https://meta.discourse.org/t/s3-uploads-incompatible-with-server-side-encryption/266853

No tests were changed because, as far as I can tell, there are no tests for this area of the codebase.

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/s3-uploads-incompatible-with-server-side-encryption/266853/6

They don't work for server-side encryption with customer keys, and so instead we just use Content-MD5 to ensure there was no corruption in transit, which is the best we can do.

See also: https://meta.discourse.org/t/s3-uploads-incompatible-with-server-side-encryption/266853
Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this Matt, apologies for the delay. I checked through our various repos and SKIP_ETAG_VERIFY doesn't even seem to be used anywhere, and confirmed there is unfortunately no specs for this.

@ZogStriP ZogStriP merged commit bd9c919 into discourse:main Jul 7, 2023
12 of 13 checks passed
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/s3-uploads-incompatible-with-server-side-encryption/266853/12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants