contour
contour copied to clipboard
disableCompression: Expose configuration to toggle Envoy GZIP compression on the responses
For https://github.com/projectcontour/contour/issues/6511
This PR adds,
- Listener configuration that exposes a boolean flag to disable compression, by default compression is enabled. This also provides us a way to disable if the users prefer to trade network for CPU, especially when teams want to run lean Envoy instances and rely on horizontal scalability.
- We will run the test build for a while in our cluster to show the actual cost benefit.
Related https://github.com/projectcontour/contour/issues/310, there had been mentions about disabling compression, the ticket we had raised shows the reason where disabling compression can bring cost benefits.
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
The Contour project currently lacks enough contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:
- After 14d of inactivity, lifecycle/stale is applied
- After 30d of inactivity since lifecycle/stale was applied, the PR is closed
You can:
- Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
- Mark this PR as fresh by commenting or pushing a commit
- Close this PR
- Offer to help out with triage
Please send feedback to the #contour channel in the Kubernetes Slack
Ping
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 Report
Attention: Patch coverage is 92.30769% with 6 lines in your changes missing coverage. Please review.
Project coverage is 80.72%. Comparing base (
38346c5) to head (30c8fb6). Report is 20 commits behind head on main.
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
| Files with missing lines | Coverage Δ | |
|---|---|---|
| cmd/contour/servecontext.go | 84.91% <100.00%> (+0.53%) |
:arrow_up: |
| internal/xdscache/v3/listener.go | 91.71% <100.00%> (+0.07%) |
:arrow_up: |
| cmd/contour/serve.go | 21.89% <0.00%> (-0.03%) |
:arrow_down: |
| internal/envoy/v3/listener.go | 98.27% <96.07%> (-0.24%) |
:arrow_down: |
| pkg/config/parameters.go | 87.04% <57.14%> (-0.65%) |
:arrow_down: |
🚀 New features to boost your workflow:
- ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Hello @tsaarni I have made the changes to the documentation you mentioned.
The first commit 934ca48 is a pure format change to tidy the table borders; the diff therefore looks worse than it is, but is more readable with whitespace diff comparison turned off. The actual addition of the flag to the docs is a one-liner in 2efb693.
@davinci26 that's an interesting idea. It goes beyond what was mentioned in #310 which -- although it's far from a formal design process (so early in contour's lifecylce) -- is at least some semblance of a "blessing" of the API change in this PR. If we were to go for the alternative API you proposed (which is IMO richer/better overall), would it require going through the proposal process since it's not something that has been (afaik) so far envisioned/discussed?
Hi @davinci26 @tsaarni I have updated the branch according to your comment above, so that it is now possible to request different types of compression, including none. Rather than a separate nocompression flag, I've just provided an additional compression option "disabled".
The supported types of compression are those included in go-control-plane/envoy/extensions/compression, namely
- gzip
- brotli
- zstd
This can be configured with the flag --compression or in config.
In-cluster tests
Tests against a deployment of "podinfo" on an in-house cluster.
out-of-the-box default behaviour
Ask for gzip encoding and gunzip the output:
~ [56]$ curl -v -H 'Accept-Encoding: gzip,deflate' $URL/env | gzip -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 08 Oct 2024 13:41:14 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: gzip
< vary: Accept-Encoding
< server: envoy
<
{ [283 bytes data]
100 293 0 293 0 0 1421 0 --:--:-- --:--:-- --:--:-- 1422
...
[
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"HOSTNAME=podinfo-9f86b555d-c6cz7",
"PODINFO_UI_COLOR=#34577c",
Note the content-encoding: gzip in the response - default behaviour is to gzip responses in response to the accept encoding request header.
disabled
set the flag in config (we hold values for the config in a configmap):
apiVersion: v1
data:
contour.yaml: |-
compression: disabled
accesslog-format: json
...
Repeat the command above but this time should get text back and not need to un-gzip:
~ $ curl -v -H 'Accept-Encoding: gzip,deflate' $URL/env
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 08 Oct 2024 13:50:11 GMT
< content-length: 814
< x-envoy-upstream-service-time: 1
< server: envoy
<
[
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"HOSTNAME=podinfo-9f86b555d-c6cz7",
"PODINFO_UI_COLOR=#34577c",
"PODINFO_SERVICE_HOST=192.168.243.43",
No content-encoding: gzip in the response. Compression has been disabled.
brotli
Request brotli encoding on the response and decode it with the brotli CLI tool:
~ $ curl -v -H 'Accept-Encoding: br,deflate' $URL/env | brotli -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 08 Oct 2024 13:53:45 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: br
< vary: Accept-Encoding
< server: envoy
<
{ [287 bytes data]
100 288 0 288 0 0 1020 0 --:--:-- --:--:-- --:--:-- 1021
...
[
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"HOSTNAME=podinfo-9f86b555d-c6cz7",
"PODINFO_UI_COLOR=#34577c",
Note content-encoding: br in the response headers and the successful decode of the response. Brotli encoding was successfully requested.
zstd
Request zstd encoding on the response and decode it with the zstd CLI tool:
~ $ curl -v -H 'Accept-Encoding: zstd,deflate' $URL/env | zstd -d
...
< HTTP/2 200
< content-type: application/json; charset=utf-8
< x-content-type-options: nosniff
< date: Tue, 08 Oct 2024 14:03:27 GMT
< x-envoy-upstream-service-time: 1
< content-encoding: zstd
< vary: Accept-Encoding
< server: envoy
<
{ [302 bytes data]
100 302 0 302 0 0 1089 0 --:--:-- --:--:-- --:--:-- 1090
...
[
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"HOSTNAME=podinfo-9f86b555d-c6cz7",
Note the content-encoding: zstd in the response headers and the successful decompression.
zstd encoding was successfully requested.
Invalid setting
Test the validation on the compression setting in contour config. Set an invalid value for the compression flag and restart contour pods. Pods fail to start with a simple error message recording the invalid value:
~ $ kubectl --context sky-pre-dev-euwest1 -n contour-main-external logs contour-contour-6fc5b659c5-22d5g
time="2024-10-08T14:07:11Z" level=info msg="maxprocs: Leaving GOMAXPROCS=16: CPU quota undefined"
contour: error: invalid Contour configuration: invalid compression type: "bogus", try --help
Could you kindly review and enable the build workflows on this to see if they all pass? In the branch where it was built I was getting an odd error on the upgrade tests, as noted here, and I wasn't sure if that might be because that was just a branch.
The upgrade tests are failing with
./test/scripts/kind-load-contour-image.sh
fatal: No names found, cannot describe anything.
CONTOUR_UPGRADE_FROM_VERSION= \
CONTOUR_E2E_IMAGE=ghcr.io/projectcontour/contour:7e843b7a \
go run github.com/onsi/ginkgo/v2/ginkgo -tags=e2e -mod=readonly -randomize-all -poll-progress-after=300s -v ./test/e2e/upgrade
Not 100% sure why that is, possibly something to do with actions/checkout on a branch? I think this change is sufficiently advanced to make it worth merging to chaosbox/main, and working on resolving any futher test failures there.
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.
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:
default
out 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 disabled
added 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 gzip
set 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 brotli
set 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 zstd
set 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 disabled
Remove 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 gzip
edit 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 brotli
edit 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 zstd
edit 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?
Many thanks for the comments @tsaarni will have a look through and apply changes as soon as possible.
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. [Update: changes added now].
What do you think about the notes here on the question about the builder methods?
hi @tsaarni what do you think about the above changes and question?
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.
Please have another look at the latest batch of commits, thanks.
@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:
- After 14d of inactivity, lifecycle/stale is applied
- After 30d of inactivity since lifecycle/stale was applied, the PR is closed
You can:
- Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
- Mark this PR as fresh by commenting or pushing a commit
- Close this PR
- Offer to help out with triage
Please send feedback to the #contour channel in the Kubernetes Slack
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!