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

internal/envoy: Enable gzip compression for grpc-web content types. #4403

Merged
merged 3 commits into from
Apr 14, 2022
Merged

Conversation

bourquep
Copy link
Contributor

When Envoy serves as a grpc-web gateway, HTTP requests/responses use one of the documented content-type, for which gzip compression is not enabled by default in Envoy.

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

@bourquep bourquep requested a review from a team as a code owner March 16, 2022 19:21
@bourquep bourquep requested review from tsaarni and skriss and removed request for a team March 16, 2022 19:21
@sunjayBhatia sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Mar 16, 2022
When Envoy serves as a `grpc-web` gateway, HTTP requests/responses
use one of the [documented content-type](https://github.com/grpc/grpc-web#wire-format-mode),
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>
Signed-off-by: Pascal Bourque <pascal@studyo.co>
@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #4403 (186043e) into main (04be670) will decrease coverage by 4.65%.
The diff coverage is 52.38%.

❗ Current head 186043e differs from pull request most recent head e2d6d9e. Consider uploading reports for the commit e2d6d9e to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4403      +/-   ##
==========================================
- Coverage   78.34%   73.68%   -4.66%     
==========================================
  Files         113      137      +24     
  Lines       10196    12340    +2144     
==========================================
+ Hits         7988     9093    +1105     
- Misses       2025     3049    +1024     
- Partials      183      198      +15     
Impacted Files Coverage Δ
cmd/contour/cli.go 0.00% <0.00%> (ø)
cmd/contour/contour.go 0.00% <0.00%> (ø)
cmd/contour/gatewayprovisioner.go 0.00% <0.00%> (ø)
cmd/contour/ingressstatus.go 34.78% <0.00%> (ø)
internal/annotation/annotations.go 100.00% <ø> (ø)
internal/dag/dag.go 94.94% <ø> (ø)
internal/envoy/v3/auth.go 100.00% <ø> (ø)
internal/provisioner/objects/rbac/rbac.go 0.00% <0.00%> (ø)
...ner/objects/rbac/serviceaccount/service_account.go 0.00% <0.00%> (ø)
internal/provisioner/objects/secret/secret.go 0.00% <0.00%> (ø)
... and 39 more

Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this change @bourquep!

@github-actions
Copy link

github-actions bot commented Apr 4, 2022

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 30 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 4, 2022
@sunjayBhatia sunjayBhatia removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 4, 2022
@youngnick
Copy link
Member

@projectcontour/maintainers, anyone else want to review this, before I hit merge?

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

Should we include more options for grpc-web content types e.g. plain application/grpc-web

see https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2

@bourquep
Copy link
Contributor Author

bourquep commented Apr 6, 2022

Should we include more options for grpc-web content types e.g. plain application/grpc-web

see https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2

Good point. I had based my PR on this document, from the readme of the grpc-web JS runtime. But indeed, all the formats defined in the grpc-web spec should probably be included.

I will update the PR soon.

@vahid-sohrabloo
Copy link

@youngnick When can you merge and release this PR?

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

looks good, I might make a follow up PR to shorten the test change a tad, just so we can reduce some duplication

@sunjayBhatia sunjayBhatia merged commit 4719dfd into projectcontour:main Apr 14, 2022
sunjayBhatia added a commit to sunjayBhatia/contour that referenced this pull request Apr 15, 2022
Follow up to projectcontour#4403 which
started using a deprecated field

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
sunjayBhatia added a commit that referenced this pull request Apr 18, 2022
Follow up to #4403 which
started using a deprecated field

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable gzip compression for additional content type(s)
4 participants