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.

Sign up for GitHub

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

Jump to bottom

Enable gzip compression for additional content type(s) #4345

Closed
sunjayBhatia opened this issue Feb 14, 2022 · 18 comments · Fixed by #4403
Closed

Enable gzip compression for additional content type(s) #4345

sunjayBhatia opened this issue Feb 14, 2022 · 18 comments · Fixed by #4403
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature.

Comments

@sunjayBhatia
Copy link
Member

If I wanted to enable compression for another content type, how should I proceed?

Originally posted by @bourquep in #731 (comment)
@sunjayBhatia
Copy link
Member Author

@bourquep started a new issue for this discussion so we can track it correctly

@sunjayBhatia sunjayBhatia added kind/feature Categorizes issue or PR as related to a new feature. lifecycle/needs-triage Indicates that an issue needs to be triaged by a project contributor. labels Feb 14, 2022
@sunjayBhatia
Copy link
Member Author

Currently only gzip and brotli compression are supported in Envoy: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/compression/compression

What sort of content compression are you looking for?

@bourquep
Copy link
Contributor

We use Grpc-web (via Envoy via Contour), our payloads are application/grpc-web+proto

@bourquep
Copy link
Contributor

Just to clarify: I am not looking to enable other compression algorithms, gzip is fine. I need to configure Envoy to gzip-compress another content-type besides those listed in #731

@sunjayBhatia sunjayBhatia changed the title Enable compression for another content type besides gzip Enable gzip compression for additional content type Feb 14, 2022
@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Feb 14, 2022 •
edited
Loading

you would have to application/grpc-web+proto to the defaults from the compressor configuration: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/compressor/v3/compressor.proto#extensions-filters-http-compressor-v3-compressor

contour doesnt modify them here:

contour/internal/envoy/v3/listener.go

Line 235 in 3498b96

TypedConfig: protobuf.MustMarshalAny(&envoy_compressor_v3.Compressor{

seems reasonable to add it by default, esp. since we default to adding the grpc-web filter as well, so if a client says they accept gzip encoding they get it without having to set anything in contour config

@sunjayBhatia sunjayBhatia changed the title Enable gzip compression for additional content type Enable gzip compression for additional content type(s) Feb 14, 2022
@bourquep
Copy link
Contributor

Meanwhile, is there anything I can do to override Envoy's configuration to enable gzip compression for Grpc-web traffic?

@youngnick
Copy link
Member

Right now, there is no way to override Envoy's config for this at the moment, sorry.

@sunjayBhatia
Copy link
Member Author

If you’re open to submitting a PR we would gladly take it 👍🏽

@bourquep
Copy link
Contributor

I might give it a shot!

@bourquep
Copy link
Contributor

I gave it a fair try, but I am totally illiterate in Go and in Envoy configuration schema. I'm afraid I'd break something if I ventured any further. 🤷

@sunjayBhatia sunjayBhatia added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Feb 16, 2022
@bourquep
Copy link
Contributor

I gave it another try, and I have something that looks in order:

https://github.com/Studyo/contour/commit/79ba90776b88ed201b20864860c37cf0cf718287

But I'm not sure about having to copy-paste the headers in each test though, and I couldn't figure out how to write feature and/or end-to-end tests for it.

I will only be to test it in my cluster after I have upgraded Contour to the latest version, which should be soon.

In other words, I have a commit but not a PR yet. 🤷‍♂️

@bourquep
Copy link
Contributor

I have back ported the commit to Contour 1.14.1 (the version we are using in prod), and tested with an updated image.

Unfortunately, it doesn't work - the Grpc-web responses are not compressed. I tried moving the grpcweb filter above the compressor filter, still no luck.

Then I researched a bit more and found this: envoyproxy/envoy#6632

Looks like Envoy isn't able to compress Grpc-web responses after all.

@youngnick
Copy link
Member

Oh, that sucks, sorry @bourquep. Let's mark this as blocked on Envoy for now then.

@youngnick youngnick added blocked/needs-envoy Categorizes the issue or PR as blocked because it needs changes in Envoy. and removed lifecycle/needs-triage Indicates that an issue needs to be triaged by a project contributor. labels Feb 18, 2022
@bourquep
Copy link
Contributor

There is hope: I upgraded Contour to 1.15.1 (was on 1.14.1), then applied my patch to enable gzip on Grpc-web payloads, and it works!

@sunjayBhatia
Copy link
Member Author

Nice! Yeah the Envoy 1.18 release notes mention compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting envoy.reloadable_features.enable_compression_without_content_length_header to false.: https://www.envoyproxy.io/docs/envoy/latest/version_history/v1.18.0#new-features

@sunjayBhatia sunjayBhatia removed the blocked/needs-envoy Categorizes the issue or PR as blocked because it needs changes in Envoy. label Feb 18, 2022
@sunjayBhatia
Copy link
Member Author

Unfortunately Contour 1.15 is a pretty old release and I'm not sure we will backport the change that far, but if you're willing to submit a PR we can at least enable it in main

@bourquep
Copy link
Contributor

@sunjayBhatia Indeed, I did not expect you to backport, I just wanted to be able to test my patch with the release that I have in production.

Do you think that my commit mentioned above is enough for a PR? I haven't been able to add any feature or e2e tests...

@bourquep bourquep mentioned this issue Mar 16, 2022
Merged
@bourquep
Copy link
Contributor

@sunjayBhatia I've just submitted #4403 for this!

sunjayBhatia pushed a commit that referenced this issue Apr 14, 2022
@bourquep
internal/envoy: Enable gzip compression for grpc-web content types. (#…
4719dfd 
…4403)

When Envoy serves as a `grpc-web` gateway, HTTP requests/responses
use one of the [documented content-type](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2),
for which gzip compression is _not_ enabled by [default in Envoy](https://www.envoyproxy.io/docs/envoy/v1.17.4/api-v2/config/filter/http/compressor/v2/compressor.proto#envoy-api-msg-config-filter-http-compressor-v2-compressor).

This MR adds the `grpc-web` content types to Envoy's default ones,
so they are gzip-compressed by default.

Fixes #4345

Signed-off-by: Pascal Bourque <pascal@studyo.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

internal/envoy: Enable gzip compression for grpc-web content types.
3 participants
@bourquep @sunjayBhatia @youngnick