contour icon indicating copy to clipboard operation
contour copied to clipboard

disableCompression: Expose configuration to toggle Envoy GZIP compression on the responses

Open chaosbox opened this issue 1 year ago • 10 comments
trafficstars

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.

chaosbox avatar Jul 09 '24 09:07 chaosbox

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

github-actions[bot] avatar Jul 09 '24 09:07 github-actions[bot]

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

github-actions[bot] avatar Jul 25 '24 00:07 github-actions[bot]

Ping

chaosbox avatar Jul 26 '24 10:07 chaosbox

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.

geomacy avatar Aug 19 '24 16:08 geomacy

Many thanks for taking the time to provide the lint fixes. We'll update the docs and add the changelog as soon as possible.

geomacy avatar Aug 21 '24 08:08 geomacy

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.

Files with missing lines Patch % Lines
pkg/config/parameters.go 57.14% 2 Missing and 1 partial :warning:
internal/envoy/v3/listener.go 96.07% 1 Missing and 1 partial :warning:
cmd/contour/serve.go 0.00% 1 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@            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.

codecov[bot] avatar Aug 21 '24 08:08 codecov[bot]

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.

geomacy avatar Aug 22 '24 16:08 geomacy

@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?

aecay avatar Aug 27 '24 17:08 aecay

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

geomacy avatar Oct 09 '24 08:10 geomacy

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.

geomacy avatar Oct 09 '24 08:10 geomacy

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.

geomacy avatar Oct 25 '24 16:10 geomacy

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.

geomacy avatar Oct 29 '24 11:10 geomacy

Hello @tsaarni, @davinci26, would you be able to review this? Thanks.

geomacy avatar Nov 05 '24 13:11 geomacy

@geomacy will take a look later today.

davinci26 avatar Nov 05 '24 13:11 davinci26

hi @davinci26 @tsaarni did you get a chance to look at this?

geomacy avatar Nov 13 '24 15:11 geomacy

Many thanks for the comments @tsaarni will have a look through and apply changes as soon as possible.

geomacy avatar Nov 22 '24 16:11 geomacy

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?

geomacy avatar Nov 26 '24 17:11 geomacy

hi @tsaarni what do you think about the above changes and question?

geomacy avatar Nov 28 '24 16:11 geomacy

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.

geomacy avatar Dec 06 '24 16:12 geomacy

@tsaarni any thoughts please?

geomacy avatar Dec 11 '24 12:12 geomacy

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!

geomacy avatar Dec 18 '24 20:12 geomacy

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.

geomacy avatar Jan 02 '25 15:01 geomacy

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

github-actions[bot] avatar Jan 29 '25 00:01 github-actions[bot]

hello @tsaarni I had to do another lint fix on the PR, can you enable tests again?

geomacy avatar Mar 07 '25 22:03 geomacy

Many thanks @tsaarni it's really great to see this merged. Thanks for your patience in turn!

geomacy avatar Mar 10 '25 23:03 geomacy