-
Notifications
You must be signed in to change notification settings - Fork 682
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
disableCompression: Expose configuration to toggle Envoy GZIP compression on the responses #6546
base: main
Are you sure you want to change the base?
Conversation
Hi @chaosbox! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace |
…jectcontour#6543) Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 3.3.0 to 3.4.0. - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](docker/setup-buildx-action@d70bba7...4fd8129) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: chaosbox <[email protected]>
…ponse compression Signed-off-by: chaosbox <[email protected]>
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
Ping |
# Conflicts: # .github/workflows/build_main.yaml # .github/workflows/build_tag.yaml # .github/workflows/prbuild.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included suggestions inline for some linter nags (link).
Please add the new disableCompression
option in "Configuration File" chapter in site/content/docs/main/configuration.md.
Please add also a changelog file.
Thanks very much for the feedback @tsaarni 🙇 we'll have a look at this and try to get back with updates as soon as possible. |
Many thanks for taking the time to provide the lint fixes. We'll update the docs and add the changelog as soon as possible. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6546 +/- ##
==========================================
- Coverage 81.76% 80.97% -0.79%
==========================================
Files 133 133
Lines 15944 20052 +4108
==========================================
+ Hits 13037 16238 +3201
- Misses 2614 3519 +905
- Partials 293 295 +2
|
619ac16
to
16f72d7
Compare
Co-authored-by: Tero Saarni <[email protected]> Signed-off-by: Geoff Macartney <[email protected]>
Co-authored-by: Tero Saarni <[email protected]> Signed-off-by: Geoff Macartney <[email protected]>
Co-authored-by: Tero Saarni <[email protected]> Signed-off-by: Geoff Macartney <[email protected]>
Co-authored-by: Tero Saarni <[email protected]> Signed-off-by: Geoff Macartney <[email protected]>
Co-authored-by: Tero Saarni <[email protected]> Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I have one api comment
that I left open but I think the code overall lgtm and useful.
@@ -0,0 +1 @@ | |||
Add "compression" flag to set/disable the compression type applied in the default HTTP filter chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might need update as well based on the new api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
included in 5dcdc33
Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
Avoid NPE in parameters.Validate also tidy flag handling in registerServe, don't set compression struct unless parameter is supplied. Signed-off-by: Geoff Macartney <[email protected]>
Have added one more fix of a test, but I still need to finish testing on-cluster, will update the ticket when I get that completed. |
Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
Hello @davinci26 @tsaarni, I have updated the code per the proposal to support better API extensibility by wrapping the settings in a struct. I believe this PR is ready for review. Some results below from testing on a cluster: defaultout of the box behaviour curl -v -H "Accept-Encoding: gzip,deflate" $URL | gzip -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 09:36:04 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: gzip
< vary: Accept-Encoding
< server: envoy
<
{
"hostname": "podinfo-5bd5b49f6d-n847k", response was gzip. parameter disabledadded to container args: - --compression=disabled curl: $ curl -v -H "Accept-Encoding: gzip,deflate" $URL
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 09:40:55 GMT
< content-length: 353
< x-envoy-upstream-service-time: 1
< server: envoy
<
{
"hostname": "podinfo-5bd5b49f6d-n847k", No compression applied, it has been disabled. parameter gzipset container args to have - --compression=gzip curl $ curl -v -H "Accept-Encoding: gzip,deflate" $URL | gzip -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 09:44:05 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: gzip
< vary: Accept-Encoding
< server: envoy
<
{
"hostname": "podinfo-5bd5b49f6d-n847k", parameter brotliset container args to have - --compression=brotli curl $ curl -v -H "Accept-Encoding: br,deflate" $URL | brotli -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 09:46:35 GMT
< x-envoy-upstream-service-time: 2
< content-encoding: br
< vary: Accept-Encoding
< server: envoy
<
{
"hostname": "podinfo-5bd5b49f6d-n847k", brotli encoding applied parameter zstdset container args with - --compression=zstd curl $ curl -v -H "Accept-Encoding: zstd,deflate" $URL | zstd -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 09:49:23 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: zstd
< vary: Accept-Encoding
< server: envoy
<
{
"hostname": "podinfo-5bd5b49f6d-n847k", zstd encoding applied config disabledRemove compression flag from container args and set config map to have: apiVersion: v1
data:
contour.yaml: |-
compression:
algorithm: disabled
... Restart contour pods. Do curl requesting gzip $ curl -v -H "Accept-Encoding: gzip,deflate" $URL
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 09:54:23 GMT
< content-length: 353
< x-envoy-upstream-service-time: 1
< server: envoy
<
{
"hostname": "podinfo-5bd5b49f6d-n847k",
"version": "6.0.0", compression is disabled. config gzipedit config apiVersion: v1
data:
contour.yaml: |-
compression:
algorithm: gzip
... Restart contour and curl: $ curl -v -H "Accept-Encoding: gzip,deflate" $URL | gzip -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 09:56:37 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: gzip
< vary: Accept-Encoding
< server: envoy
<
{
"hostname": "podinfo-5bd5b49f6d-n847k", response encoded with gzip config brotliedit config to apiVersion: v1
data:
contour.yaml: |-
compression:
algorithm: brotli Restart contour and curl: $ curl -v -H "Accept-Encoding: br,deflate" $URL | brotli -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 09:59:48 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: br
< vary: Accept-Encoding
< server: envoy
{
"hostname": "podinfo-5bd5b49f6d-n847k", config zstdedit config to apiVersion: v1
data:
contour.yaml: |-
compression:
algorithm: zstd curl: $ curl -v -H "Accept-Encoding: zstd,deflate" $URL | zstd -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 29 Oct 2024 10:29:20 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: zstd
< vary: Accept-Encoding
< server: envoy
<
{
"hostname": "podinfo-5bd5b49f6d-n847k", zstd compression applied. |
Hello @tsaarni, @davinci26, would you be able to review this? Thanks. |
@geomacy will take a look later today. |
hi @davinci26 @tsaarni did you get a chance to look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the long delay! I've finally had a chance to review and added comments inline.
The PR title and description are slightly outdated after the latest changes, and could use an update.
Thank you for your efforts and patience!
cmd/contour/serve.go
Outdated
serve.Flag("accesslog-format", "Format for Envoy access logs.").PlaceHolder("<envoy|json>").StringVar((*string)(&ctx.Config.AccessLogFormat)) | ||
|
||
serve.Flag("compression", "Set or disable compression type in default Listener filters.").PlaceHolder("<gzip|brotli|zstd|disabled>").StringVar((*string)(&ctx.Config.Compression.Algorithm)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the command line option and make configuration available only through config file and ContourConfiguration
CRD.
The command line options currently are a bit of a mess, so we previously agreed to avoid introducing new options there unless absolutely necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 95d38af
func (a CompressionAlgorithm) Validate() error { | ||
switch a { | ||
case BrotliCompression, DisabledCompression, GzipCompression, ZstdCompression: | ||
return nil | ||
default: | ||
return fmt.Errorf("invalid compression type: %q", a) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be cleaner to treat an empty string as a valid value here, rather than completely bypassing validation in Parameters.Validate()
? While algorithm is the only field currently, that might change.
Because +optional
string in Go becomes an empty string when user did not set the field, it might be best to consider it as valid algorithm name in the Go side (CRD validation for the field can still reject it). It is already taken into account elsewhere in this change: inhttpConnectionManagerBuilder.DefaultFilters()
you've added switch-default branch which will use gzip
for empty algorithm string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 366f48d
"application/grpc-web-text", "application/grpc-web-text+proto", "application/grpc-web-text+thrift", | ||
var compressor proto.Message = &envoy_compression_gzip_compressor_v3.Gzip{} | ||
compressorName := string(contour_v1alpha1.GzipCompression) | ||
if b.compression != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that DefaultFilters()
function used to set up a set of fixed filters - I don't see any of the other builder method impacting the default set except the new Compression()
method, which now also makes it important in which order the builder methods are called - Now the configuration would be different depending if Compression()
is called before or after DefaultFilters()
.
Maybe it would be better to (1) remove compressor from the default set completely and only add it when the new Compression()
builder method is called? Or maybe some other alternative: (2) instantiate gzip
by default and modify it later according to chosen algorithm when Get()
is called or (3) delay adding compressor until Get()
but add it by default at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that the way the code is at present means the order does matter. At the same time the intent of the PR is to allow a way of changing the compression algorithm in the default filter chain (as opposed to something more generic), so it kind of makes sense that DefaultFilters()
no longer sets up a fixed set. Maybe Compression()
is a bad name for that method, perhaps it should be something like OverrideDefaultCompression()
, and documented more clearly to explain the behaviour including how it needs to be called before DefaultFilters()
if it is to do anything.
I've been trying to avoid changing the code in any way that would risk changing behaviour for users who don't opt in to the new method. At a basic level this certainly means we can't change the signatures say of HTTPConnectionManagerBuilder()
or DefaultFilters()
itself, but I would also want to avoid changing the behaviour of DefaultFilters()
in the absence of using the new compression method. So in thinking about these options:
About 1. Remove compressor from the default set entirely.
I wouldn't be keen to do this as the behaviour of adding compression is effectively already part of the contract of what DefaultFilters()
does. Changing this could change the behaviour of downstream users' code.
Option 3 is similar, in that you're changing the order of filters (currently default filters put compression filter first). Less dramatic than option 1 but still a change in behaviour.
Option 2 might be possible - I guess Get()
could scan the filter chain looking for one named "envoy.filters.http.compressor" (value of CompressorFilterName
as set in DefaultFilters()
at present) to update. Would that be good enough? Can it update the first such filter it finds, on the assumption that there will be only one?
I'd like to be sure about any changes before I make them here, but my feeling is that perhaps no change is needed - other than renaming the Compression()
func and providing better documentation. What do you think?
| debug | boolean | `false` | Enables debug logging. | | ||
| default-http-versions | string array | <code style="white-space:nowrap">HTTP/1.1</code> <br> <code style="white-space:nowrap">HTTP/2</code> | This array specifies the HTTP versions that Contour should program Envoy to serve. HTTP versions are specified as strings of the form "HTTP/x", where "x" represents the version number. | | ||
| disableAllowChunkedLength | boolean | `false` | If this field is true, Contour will disable the RFC-compliant Envoy behavior to strip the `Content-Length` header if `Transfer-Encoding: chunked` is also set. This is an emergency off-switch to revert back to Envoy's default behavior in case of failures. | | ||
| compression | string | `gzip` | Sets the compression type applied in the compression HTTP filter of the default Listener filters. Setting this to `disabled` will make Envoy skip "Accept-Encoding: gzip,deflate" request header and always return uncompressed response. Values:`gzip` (default), `brotli`, `zstd`, `disabled`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compression here is still string
while it should be CompressionAlgorithm
. The new type needs a new table with algorithm
field with the content described here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see 1a6465f
Many thanks for the comments @tsaarni will have a look through and apply changes as soon as possible. |
Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
Co-authored-by: Tero Saarni <[email protected]> Signed-off-by: Geoff Macartney <[email protected]>
Co-authored-by: Tero Saarni <[email protected]> Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
from review comment: Let's remove the command line option and make configuration available only through config file and ContourConfiguration CRD. The command line options currently are a bit of a mess, so we previously agreed to avoid introducing new options there unless absolutely necessary. Signed-off-by: Geoff Macartney <[email protected]>
From review: Would it be cleaner to treat an empty string as a valid value here, rather than completely bypassing validation in Parameters.Validate()? While algorithm is the only field currently, that might change. Signed-off-by: Geoff Macartney <[email protected]>
Signed-off-by: Geoff Macartney <[email protected]>
hi @tsaarni, thanks for the comments and suggestions. I have a branch in preparation to do these and will merge it here when I see tests pass. What do you think about the notes here on the question about the builder methods? |
Signed-off-by: Geoff Macartney <[email protected]>
For #6511
This PR adds,
Related #310, there had been mentions about disabling compression, the ticket we had raised shows the reason where disabling compression can bring cost benefits.