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

Don't send Content-Encoding header with streamed responses #1601

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

dkliban
Copy link
Member

@dkliban dkliban commented Aug 31, 2021

aiohttp automatically enflates gzipped responses. Pulp clients always receive
uncompressed responses when requesting on_demand content.

fixes: #9213

@pulpbot
Copy link
Member

pulpbot commented Aug 31, 2021

Attached issue: https://pulp.plan.io/issues/9213

@bmbouter
Copy link
Member

This makes sense for gzip, but what about other values of Content-Encoding besides gzip?

@dralley
Copy link
Contributor

dralley commented Aug 31, 2021

@bmbouter You're referring to the changelog entry?

@bmbouter
Copy link
Member

@bmbouter You're referring to the changelog entry?

I mean functionally. We know aiohttp decompresses gzip, but what about others? If aiohttp doesn't decompress it then removing the content encoding header would be incorrect. (I have no idea what aiohttp does).

@dkliban
Copy link
Member Author

dkliban commented Aug 31, 2021

@bmbouter You're referring to the changelog entry?

I mean functionally. We know aiohttp decompresses gzip, but what about others? If aiohttp doesn't decompress it then removing the content encoding header would be incorrect. (I have no idea what aiohttp does).

All of the 4 possible values for Content-Encoding imply compression[0]. aiohttp decompresses 'gzip' and 'deflate' by default. 'brotli' encoding support can be enabled by installing 'brotlipy'[1]. 'encode' does not seem to be used much by servers due to a pattent issue. It doesn't look like aiohttp has support for auto inflating the response when the Content-Encoding is 'compress'.

[0] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding#directives
[1] https://docs.aiohttp.org/en/stable/client_quickstart.html?highlight=deflate#binary-response-content

@bmbouter
Copy link
Member

bmbouter commented Sep 1, 2021

To be sure we're not regressing, what if we only discard the Content-Encoding header if it's value is gzip?

@dralley
Copy link
Contributor

dralley commented Sep 1, 2021

@bmbouter Because aiohttp can't handle other compression types, I'm pretty sure it would set Accept-Encoding such that the server would never provide them. Although we should check that we're not forwarding those headers from the client.

@bmbouter
Copy link
Member

bmbouter commented Sep 1, 2021

@bmbouter Because aiohttp can't handle other compression types, I'm pretty sure it would set Accept-Encoding such that the server would never provide them. Although we should check that we're not forwarding those headers from the client.

Does it also accept deflate and optionally brotli too? https://docs.aiohttp.org/en/stable/client_quickstart.html?highlight=deflate#binary-response-content

@dralley
Copy link
Contributor

dralley commented Sep 1, 2021

@bmbouter
Copy link
Member

bmbouter commented Sep 1, 2021

@dralley so what would you recommend given that?

@dralley
Copy link
Contributor

dralley commented Sep 1, 2021

@bmbouter If aiohttp is setting the Accept-Encoding header on the request, then it shouldn't receive any format that aiohttp cannot deal with - so as long as we are not forwarding Accept-Encoding from the client's original request, stripping the Content-Encoding header would be fine for all possible encoding types.

@bmbouter
Copy link
Member

bmbouter commented Sep 1, 2021

@bmbouter If aiohttp is setting the Accept-Encoding header on the request, then it shouldn't receive any format that aiohttp cannot deal with - so as long as we are not forwarding Accept-Encoding from the client's original request, stripping the Content-Encoding header would be fine for all possible encoding types.

Well said. I agree completely.

@dkliban can you confirm via local testing that your patch is resolving the issue? You were able to reproduce the issue right?

@@ -0,0 +1 @@
Fixed bug where uncompressed on_demand content was streamed with "Content-Encoding: gzip" header.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this?

Suggested change
Fixed bug where uncompressed on_demand content was streamed with "Content-Encoding: gzip" header.
Fixed bug where ``pulpcore-content`` decompressed data while incorrectly advertising to clients it was still compressed via the ``Content-Encoding: gzip`` header.

@dkliban
Copy link
Member Author

dkliban commented Sep 1, 2021

@bmbouter If aiohttp is setting the Accept-Encoding header on the request, then it shouldn't receive any format that aiohttp cannot deal with - so as long as we are not forwarding Accept-Encoding from the client's original request, stripping the Content-Encoding header would be fine for all possible encoding types.

Well said. I agree completely.

@dkliban can you confirm via local testing that your patch is resolving the issue? You were able to reproduce the issue right?

I reproduced the issue locally and confirmed that the patch resolves it.

aiohttp automatically enflates gzipped responses. Pulp clients always receive
uncompressed responses when requesting on_demand content.

fixes: #9213
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thanks!

@dkliban dkliban merged commit 6d05548 into pulp:master Sep 1, 2021
@dkliban dkliban deleted the 9213 branch September 1, 2021 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants