-
Notifications
You must be signed in to change notification settings - Fork 681
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?
Changes from 16 commits
b4d5b67
081a47c
818ccf0
7ddb6f0
8df07e2
100bf40
56e4ee7
548981e
934ca48
2efb693
b0cfe71
72e0e9c
73c4b67
235acc3
7e843b7
36dbadb
f7b261b
b68f0b1
5dcdc33
3d83394
cda9c76
ff84c37
5725a53
630541e
4610b80
45ea946
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Copyright Project Contour Authors | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package v1alpha1 | ||
|
||
import "fmt" | ||
|
||
type EnvoyCompressionType string | ||
|
||
func (c EnvoyCompressionType) Validate() error { | ||
switch c { | ||
case BrotliCompression, DisabledCompression, GzipCompression, ZstdCompression: | ||
return nil | ||
default: | ||
return fmt.Errorf("invalid compression type: %q", c) | ||
} | ||
} | ||
|
||
const ( | ||
// BrotliCompression specifies brotli as the default HTTP filter chain compression mechanism | ||
BrotliCompression EnvoyCompressionType = "brotli" | ||
|
||
// DisabledCompression specifies that there will be no compression in the default HTTP filter chain | ||
DisabledCompression EnvoyCompressionType = "disabled" | ||
|
||
// GzipCompression specifies gzip as the default HTTP filter chain compression mechanism | ||
GzipCompression EnvoyCompressionType = "gzip" | ||
|
||
// ZstdCompression specifies zstd as the default HTTP filter chain compression mechanism | ||
ZstdCompression EnvoyCompressionType = "zstd" | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright Project Contour Authors | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package v1alpha1_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
contour_v1alpha1 "github.com/projectcontour/contour/apis/projectcontour/v1alpha1" | ||
) | ||
|
||
func TestValidateEnvoyCompressionType(t *testing.T) { | ||
require.Error(t, contour_v1alpha1.EnvoyCompressionType("").Validate()) | ||
require.Error(t, contour_v1alpha1.EnvoyCompressionType("foo").Validate()) | ||
|
||
require.NoError(t, contour_v1alpha1.BrotliCompression.Validate()) | ||
require.NoError(t, contour_v1alpha1.DisabledCompression.Validate()) | ||
require.NoError(t, contour_v1alpha1.GzipCompression.Validate()) | ||
require.NoError(t, contour_v1alpha1.ZstdCompression.Validate()) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -348,6 +348,12 @@ type EnvoyListenerConfig struct { | |
// +optional | ||
UseProxyProto *bool `json:"useProxyProtocol,omitempty"` | ||
|
||
// Compression selects the compression type applied in the compression HTTP filter of the default Listener filters. | ||
// Values: `gzip` (default), `brotli`, `zstd`, `disabled`. | ||
// Setting this to `disabled` will make Envoy skip "Accept-Encoding: gzip,deflate" request header and always return uncompressed response | ||
// +optional | ||
Compression EnvoyCompressionType `json:"compression,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API here is fairly constrained so we might want to think through how would we extend it if anyone comes up with the feature request to tune the I think adding another field called
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did think (briefly) about making the configurability greater, and/or having a wider range of compression algorithms. In the end I decided against it, at least for this PR -
Believe it or not I actually took a bit of inspiration from the Contour Philosophy 😆 (I did read around on how to contribute), particularly the bit on Sensible Defaults. I felt these kind of gave permission to simply offer a choice of a few widely used algorithms, using default settings. While a strategy field could be added here, it would be something of a no-op feature unless we took the time to implement at least basic configurability in it. I feel this would be best left to another PR. Would that be OK? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also now that I think of it, the feature here of most interest to us in our project is the compression disabling, so from an admittedly selfish point of view I would like to leave the configurability to a separate bit of work. Like everyone else we're very busy, and I have ended up having to do most of this work in my own time in the evenings and weekends :-( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be transparent I dont want to design how we extend it but I am trying to think through if we leave enough space to extend the api.
My only issue is that if we go with the approach here and in 3 months we wanted to add another
If we think that these are fine options I think the api is good. I am just proponent of adding another
Yeah, a lot of the people contributing to this project to this into this capacity so I feel you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will defer to @projectcontour/maintainers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah now that you have laid it out like that I see what you mean. Let me take a look at it, hopefully this wouldn't be too large a change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hi @davinci26 I have taken a stab at this in 5dcdc33, could you have a look and see what you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still need to test this, I see some failures in e2e and will want also to run it on a local cluster, will try to get that done this week and add results in a comment. |
||
|
||
// DisableAllowChunkedLength disables 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. included in 5dcdc33 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,6 +125,7 @@ | |
} | ||
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)) | ||
serve.Flag("config-path", "Path to base configuration.").Short('c').PlaceHolder("/path/to/file").Action(parseConfig).ExistingFileVar(&configFile) | ||
serve.Flag("contour-cafile", "CA bundle file name for serving gRPC with TLS.").Envar("CONTOUR_CAFILE").StringVar(&ctx.caFile) | ||
serve.Flag("contour-cert-file", "Contour certificate file name for serving gRPC over TLS.").PlaceHolder("/path/to/file").Envar("CONTOUR_CERT_FILE").StringVar(&ctx.contourCert) | ||
|
@@ -447,6 +448,7 @@ | |
} | ||
|
||
listenerConfig := xdscache_v3.ListenerConfig{ | ||
Compression: contourConfiguration.Envoy.Listener.Compression, | ||
UseProxyProto: *contourConfiguration.Envoy.Listener.UseProxyProto, | ||
HTTPAccessLog: contourConfiguration.Envoy.HTTPListener.AccessLog, | ||
HTTPSAccessLog: contourConfiguration.Envoy.HTTPSListener.AccessLog, | ||
|
@@ -471,6 +473,8 @@ | |
SocketOptions: contourConfiguration.Envoy.Listener.SocketOptions, | ||
} | ||
|
||
s.log.WithField("context", "listenerConfig").Infof("compression setting: %s", listenerConfig.Compression) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this from debugging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually left this in here deliberately - I think it's quite nice to be able to see the choice of algorithm reflected in the initial log output. But if that's not a typical contour thing to do I'll take it out. Let me know what would be preferred. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. took this out in b68f0b1 |
||
|
||
if listenerConfig.TracingConfig, err = s.setupTracingService(contourConfiguration.Tracing); err != nil { | ||
return err | ||
} | ||
|
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.
Should we use
// +kubebuilder:validation:Enum=gzip,brotli,zstd,disabled
as well?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.
👍 sounds good, I will add that
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.
hi @davinci26 added this in f7b261b
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.
Example: