Adds support for configuration of different types of compression, including none #6546
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] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: chaosbox <ram29@bskyb.com>
…ponse compression Signed-off-by: chaosbox <ram29@bskyb.com>
|
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.
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 80.70% 80.72% +0.02%
==========================================
Files 131 131
Lines 19816 19868 +52
==========================================
+ Hits 15993 16039 +46
- Misses 3533 3537 +4
- Partials 290 292 +2
🚀 New features to boost your workflow:
|
619ac16 to
16f72d7
Compare
Co-authored-by: Tero Saarni <tero.saarni@est.tech> Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Co-authored-by: Tero Saarni <tero.saarni@est.tech> Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Co-authored-by: Tero Saarni <tero.saarni@est.tech> Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Co-authored-by: Tero Saarni <tero.saarni@est.tech> Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Co-authored-by: Tero Saarni <tero.saarni@est.tech> Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
|
hi @tsaarni what do you think about the above changes and question? |
a better name is SetDefaultFilterCompression Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
|
@tsaarni any thoughts please? |
|
hi @tsaarni, @davinci26, @sunjayBhatia we would really appreciate it if you could get back to us on this pull request, which has been sitting for some weeks. I believe from the above conversations that this is either ready for final approval and merge, or very nearly so. The changes on this PR have been going on for a long time and it would be really good to get it finished off at last. Thanks! |
|
hi @tsaarni, @davinci26, @sunjayBhatia please let us know how we can get this ticket reviewed and accepted. We would really like to get this out of the way if at all possible. |
|
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 |
There was a problem hiding this comment.
Apologies for being unavailable to review once again :( Aside from the minor comments, the PR looks good to go.
hi @tsaarni I have thought about the above further and I do feel that the clarification of the method's usage is the best approach - I have added this in 2f5e157.
I've now also tried to implement various alternatives on top of this PR, including adding Compressor with an explicit call rather than as default. However, there are so many unit tests to adjust accordingly, that it becomes unnecessarily high impact. Instead, I have added suggestion to add panic() to enforce the correct order.
|
|
||
| // EnvoyCompression defines configuration related to compression in the default HTTP Listener filter chain. | ||
| type EnvoyCompression struct { | ||
| // Algorithm selects the compression type applied in the compression HTTP filter of the default Listener filters. |
There was a problem hiding this comment.
| // Algorithm selects the compression type applied in the compression HTTP filter of the default Listener filters. | |
| // Algorithm selects the response compression type applied in the compression HTTP filter of the default Listener filters. |
| type EnvoyCompression struct { | ||
| // Algorithm 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 |
There was a problem hiding this comment.
| // Setting this to `disabled` will make Envoy skip "Accept-Encoding: gzip,deflate" request header and always return uncompressed response | |
| // Setting this to `disabled` will make Envoy skip "Accept-Encoding: gzip,deflate" request header and always return uncompressed response. |
| // SetDefaultFilterCompression configures the builder to set the compression method applied by DefaultFilters() to the | ||
| // given value `compressor`. When chaining builder method calls, this method should be called before DefaultFilters(). | ||
| func (b *httpConnectionManagerBuilder) SetDefaultFilterCompression(compressor *contour_v1alpha1.EnvoyCompression) *httpConnectionManagerBuilder { | ||
| b.compression = compressor |
There was a problem hiding this comment.
I'm not sure if SetDefaultFilterCompression() communicates that this function must be called in a specific order? Personally I don't find it immediately clear. We could simply add enforce with panic and use the previous name Compression() which sounded better to me.
| b.compression = compressor | |
| if len(b.filters) > 0 { | |
| panic("Compression must be set before adding filters") | |
| } | |
| b.compression = compressor |
The approach has precedence, as simiar pattern is already used here.
There was a problem hiding this comment.
Thanks @tsaarni will make those changes as soon as possible.
There was a problem hiding this comment.
I believe make generate needs to be run once again, and the generated files should be committed. Additionally, the following patch aligns with the latest changes related to HTTPConnectionManagerBuilder and fixes the compile errors.
diff --git a/internal/featuretests/v3/compression_test.go b/internal/featuretests/v3/compression_test.go
index b374ed92..94e28756 100644
--- a/internal/featuretests/v3/compression_test.go
+++ b/internal/featuretests/v3/compression_test.go
@@ -28,6 +28,10 @@ import (
)
func TestCompression(t *testing.T) {
+ envoyGen := envoy_v3.NewEnvoyGen(envoy_v3.EnvoyGenOpt{
+ XDSClusterName: envoy_v3.DefaultXDSClusterName,
+ })
+
tests := map[string]struct {
algorithm contour_v1alpha1.CompressionAlgorithm
want contour_v1alpha1.CompressionAlgorithm
@@ -75,7 +79,7 @@ func TestCompression(t *testing.T) {
rh.OnAdd(s1)
rh.OnAdd(hp1)
httpListener := defaultHTTPListener()
- httpListener.FilterChains = envoy_v3.FilterChains(envoy_v3.HTTPConnectionManagerBuilder().
+ httpListener.FilterChains = envoy_v3.FilterChains(envoyGen.HTTPConnectionManagerBuilder().
Compression(&contour_v1alpha1.EnvoyCompression{
Algorithm: tc.want,
}).
diff --git a/internal/xdscache/v3/listener_test.go b/internal/xdscache/v3/listener_test.go
index 6e6e994f..299f9963 100644
--- a/internal/xdscache/v3/listener_test.go
+++ b/internal/xdscache/v3/listener_test.go
@@ -3093,7 +3093,7 @@ func TestListenerVisit(t *testing.T) {
Name: ENVOY_HTTP_LISTENER,
Address: envoy_v3.SocketAddress("0.0.0.0", 8080),
FilterChains: envoy_v3.FilterChains(
- envoy_v3.HTTPConnectionManagerBuilder().
+ envoyGen.HTTPConnectionManagerBuilder().
Compression(&contour_v1alpha1.EnvoyCompression{
Algorithm: contour_v1alpha1.DisabledCompression,
}).
@@ -3139,7 +3139,7 @@ func TestListenerVisit(t *testing.T) {
Name: ENVOY_HTTP_LISTENER,
Address: envoy_v3.SocketAddress("0.0.0.0", 8080),
FilterChains: envoy_v3.FilterChains(
- envoy_v3.HTTPConnectionManagerBuilder().
+ envoyGen.HTTPConnectionManagerBuilder().
Compression(&contour_v1alpha1.EnvoyCompression{
Algorithm: contour_v1alpha1.GzipCompression,
}).
@@ -3185,7 +3185,7 @@ func TestListenerVisit(t *testing.T) {
Name: ENVOY_HTTP_LISTENER,
Address: envoy_v3.SocketAddress("0.0.0.0", 8080),
FilterChains: envoy_v3.FilterChains(
- envoy_v3.HTTPConnectionManagerBuilder().
+ envoyGen.HTTPConnectionManagerBuilder().
Compression(&contour_v1alpha1.EnvoyCompression{
Algorithm: contour_v1alpha1.BrotliCompression,
}).
@@ -3231,7 +3231,7 @@ func TestListenerVisit(t *testing.T) {
Name: ENVOY_HTTP_LISTENER,
Address: envoy_v3.SocketAddress("0.0.0.0", 8080),
FilterChains: envoy_v3.FilterChains(
- envoy_v3.HTTPConnectionManagerBuilder().
+ envoyGen.HTTPConnectionManagerBuilder().
Compression(&contour_v1alpha1.EnvoyCompression{
Algorithm: contour_v1alpha1.ZstdCompression,
}).
@@ -3277,7 +3277,7 @@ func TestListenerVisit(t *testing.T) {
Name: ENVOY_HTTP_LISTENER,
Address: envoy_v3.SocketAddress("0.0.0.0", 8080),
FilterChains: envoy_v3.FilterChains(
- envoy_v3.HTTPConnectionManagerBuilder().
+ envoyGen.HTTPConnectionManagerBuilder().
Compression(&contour_v1alpha1.EnvoyCompression{
Algorithm: contour_v1alpha1.GzipCompression,
}).There was a problem hiding this comment.
hi @tsaarni thanks, yes I have work in progress on this here: chaosbox#5 that fixes the above, but I see there are also test failures that I haven't had the chance to look at yet. I am sorry this is taking so long, I am completely flat out at the moment and haven't got to look at this. I hope to get a chance to get on to it soon, thanks for your patience!!
There was a problem hiding this comment.
No problem! Lets get back to this when you have time.
There was a problem hiding this comment.
hi @tsaarni that's all merged now, it would be great if you could approve to run the tests.
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
|
hello @tsaarni I had to do another lint fix on the PR, can you enable tests again? |
|
Many thanks @tsaarni it's really great to see this merged. Thanks for your patience in turn! |
For #6511
This PR adds support for configuration of different types of compression, including none, in the default HTTP filter chain.
Related #310, there had been mentions about disabling compression, the ticket we had raised shows the reason where disabling compression can bring cost benefits.