api: add compressor/decompressor support #2474

soulxu · 2024-01-22T04:29:14Z

What type of PR is this?

"feat(translator): add new feature"

What this PR does / why we need it:
Add HTTP compressor/decompressor support

Which issue(s) this PR fixes:
Fixes #2451

Signed-off-by: He Jie Xu <[email protected]>

codecov · 2024-01-22T04:32:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e168ef9) 64.62% compared to head (2a7558c) 64.63%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2474   +/-   ##
=======================================
  Coverage   64.62%   64.63%           
=======================================
  Files         115      115           
  Lines       17513    17513           
=======================================
+ Hits        11318    11319    +1     
  Misses       5470     5470           
+ Partials      725      724    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

soulxu · 2024-01-22T04:33:49Z

In the end, we need to add compressor/decompressor API both for BackendTrafficPolicy and ClientTrafficPolicy.
The BackendTrafficPolicy defines the compressor/decompressor for connection between the envoy proxy and backend service.
The ClientTrafficPolicy defines the compressor/decompressor for connection between the downstream client and envoy proxy.

The envoy's compressor/decompressor provides a very flexible API. I also define the EnvoyGateway API as the most flexible. But if we think this is too complicated for the envoy gateway user, we can simplify it. Like we can assume that if the user wants to compress the request of the envoy proxy to the backend, then the user also wants to decompress the response of the envoy proxy to the backend, then we can make the API easier. Anyway, this is something we can consider.

Also I didn't finish all the compressor/decompressor lib definition since I want to get early feedback from the community first. Then after getting initial feedback, I will add the brotli and zstd compressor/decompressor lib support also.

cc @arkodg @akhenakh

soulxu · 2024-01-22T04:47:40Z

In the end, we need to add compressor/decompressor API both for BackendTrafficPolicy and ClientTrafficPolicy. The BackendTrafficPolicy defines the compressor/decompressor for connection between the envoy proxy and backend service. The ClientTrafficPolicy defines the compressor/decompressor for connection between the downstream client and envoy proxy.

@akhenakh the ClientTrafficPolicy should match your usecase. But let me know if I missed something. thanks!

Signed-off-by: He Jie Xu <[email protected]>

zhaohuabing

I suspect that request compression and response decompression are rarely used? We probably doesn't need to support them at the first iteration.

If that's true, this should be only in ClientTrafficPolicy.

arkodg · 2024-01-22T19:20:41Z

imo the deciding factor between BackendTrafficPolicy and ClientTrafficPolicy is whether compression is more likely to be configured by the app team or platform team
Id vote to expose minimal config in the first iteration e.g.

compression:
  type: Gzip

and either set defaults or rely on the envoy defaults

prefer if this API PR only focuses on compression (can raise another PR later from decompression) since sending compressed response to the browser seems like the most common use case to solve

soulxu · 2024-01-22T22:55:25Z

imo the deciding factor between BackendTrafficPolicy and ClientTrafficPolicy is whether compression is more likely to be configured by the app team or platform team

Id vote to expose minimal config in the first iteration e.g.
compression:
  type: Gzip
and either set defaults or rely on the envoy defaults

prefer if this API PR only focuses on compression (can raise another PR later from decompression) since sending compressed response to the browser seems like the most common use case to solve

@arkodg @zhaohuabing thanks for the feedback! I like the way just provide minimal config first, let us add more in the future. let me update the PR.

api/v1alpha1/backendtrafficpolicy_types.go

Signed-off-by: He Jie Xu <[email protected]>

soulxu · 2024-01-24T02:08:22Z

prefer if this API PR only focuses on compression (can raise another PR later from decompression) since sending compressed response to the browser seems like the most common use case to solve

@arkodg @zhaohuabing I only keep the compression for the client traffic policy and backend traffic policy. That means the initial implementation only allow two cases:

the compression of the response from the upstream to the envoy proxy
- fulfill the case of Support Compression #2451 (comment)
the compression of the request from envoy proxy to backend

Let me know if this matches what you expect.

Thanks!

zirain · 2024-01-24T02:40:20Z

@soulxu plz fix the conflict.

zhaohuabing · 2024-01-24T07:24:33Z

the compression of the request from envoy proxy to backend

I thought only compression on the response, possibly decompression for the request (this can be done later as @arkodg suggested)?

api/v1alpha1/compression_types.go

soulxu · 2024-01-24T23:51:51Z

the compression of the request from envoy proxy to backend

I thought only compression on the response, possibly decompression for the request (this can be done later as @arkodg suggested)?

cool, let me make it more simple, thanks!

arkodg · 2024-01-24T23:53:38Z

api/v1alpha1/clienttrafficpolicy_types.go

@@ -78,6 +78,10 @@ type ClientTrafficPolicySpec struct {
 	//
 	// +optional
 	Path *PathSettings `json:"path,omitempty"`
+	// The compression config for the downstream client connections.


lets define this config in one place for now, I vote to rm this one and keep the one in backendTrafficPolicy

Emm... then how we distinguish the upstream and downstream side compression in the future?

afaik compression is defined at a request/response level per listener/route in envoy https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/compressor_filter

Yes, for the only downstream side compression, we will enable the response direction compression by response_direction_config and disable the request direction compression by the request_direction_config

For the only upstream side compression, we will enable the requestion direction compression by request_direction_config and diable the response direction compression by response_direction_config

Yes, for the only downstream side compression, we will enable the response direction compression by response_direction_config and disable the request direction compression by the request_direction_config

This supposed to be defined by backend traffic policy.

For the only upstream side compression, we will enable the requestion direction compression by request_direction_config and diable the response direction compression by response_direction_config

This supposed to be defined by client traffic policy. That confuses me if define this in the backend traffic policy also. What we do when want to enable the dowstream side compression?

no, the BackendTrafficPolicy can be attached either to the Gateway or to the Route

oop, I missed that, thanks!

since the app teams also know the ability of the applciation

yeah since they have a better understanding of their L7 client, this is just my initial thought on where to put the config, would be good to hear counter arguments as well

@arkodg I got your reason for putting this in the BackendTrafficPolicy, but I also feel it is against the intuition of the users.

The most two common use cases for Compression and Decompression are:

Compression: compress the response from the backend to the client to save the network bandwidth.

Decompression: decompress the request from the client to the backend to avoid the backend to do the decompression.

From the user's point of view, these two use cases are more related to client-proxy communication. I guess the real issue here is that the ClientTrafficPolicy is not able to do per-route configuration.

imo the nouns we could have come up with were

trafficPolicy - to deal with L7 request/response transformations

clientConnectionPolicy - for client connection

backendConnectionPolicy - for backend connection

backendTrafficPolicy imo is combining backendConnectionPolicy + trafficPolicy

imo the nouns we could have come up with were

trafficPolicy - to deal with L7 request/response transformations

Just dumping some thoughts after reading those discussions. (learning a lot from it)

Thinking of the role for the compression API. I feel there are two roles may involve this.

One is the application developer, just as we discussed above, they know the capabilities of the application. They want to save the traffice between the application and the client.

The second one is the cluster operator, like the case of @zhaohuabing said, the cluster operator wants to save the bandwidth. (This is the initial case in my mind also)

For the second case, if it is under the service mesh case. There can be a sidecar proxy in the front of the backend endpoint. Then cluster operators needn't care about the application capabilities.

To fulfill the cases for this two roles, in the future, maybe we can have two APIs, ApplicationTrafficPolicy and TrafficPolicy. The ApplicationTrafficPolicy can override the TrafficPolicy.
I saw the similar case in the TLSBackendPolicy https://gateway-api.sigs.k8s.io/geps/gep-1897/#api

But yes, for today, we can just simplify and have one.

Signed-off-by: He Jie Xu <[email protected]>

api/v1alpha1/shared_types.go

arkodg · 2024-01-25T01:10:35Z

api/v1alpha1/backendtrafficpolicy_types.go

+	// The compression config for the http streams.
+	//
+	// +optional
+	Compression []*Compression `json:"compression,omitempty"`


why is this a list ?

to support enable multiple compression algorithm

is that a valid use case ?

rethinking, feels like a valid use case to enable both gzip and brotli and decide based on client capability.
for that case, should the config look more like

compression: gzip: {} brotli: {}

the type is no longer needed since we aren't picking 1
a list also works, but extra validation needs to be added to ensure a specific type is not enabled multiple times

yes, at least from the Envoy side #2474 (comment)

There is also option choose_first https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/compressor/v3/compressor.proto for the case of multiple compressors.

no, I don't think we are going to define the same compression twice. I think it is all about multiple different compression.

if so, prefer

compression: gzip: {} brotli: {}

in that case, imo we shouldnt use a list (which makes validation hard), instead use something like

compression: gzip: {} brotli: {}

emm...sorry, I found a case for multiple same compressor.

// There could be many compressors registered for the same content encoding, e.g. consider a // case when there are two gzip filters using different compression levels for different content // sizes. In such case we ignore duplicates (or different filters for the same encoding) // registered last.

https://github.com/envoyproxy/envoy/blob/45ab9cfa30c7631fe325847dd291e2dcc80c1894/source/extensions/filters/http/compressor/compressor_filter.cc#L389-L392

And we probably need the order of multiple compressors also.

api/v1alpha1/compression_types.go

Signed-off-by: He Jie Xu <[email protected]>

arkodg · 2024-01-26T23:10:21Z

api/v1alpha1/compression_types.go

+// GzipCompressor defines the config for the Gzip compressor. There are some configs
+// available from the Envoy Proxy. Currently only use the default value for configs.
+// For the default value for those configs can be reference here:
+// https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/compression/gzip/compressor/v3/gzip.proto#extension-envoy-compression-gzip-compressor


Suggested change

// GzipCompressor defines the config for the Gzip compressor. There are some configs

// available from the Envoy Proxy. Currently only use the default value for configs.

// For the default value for those configs can be reference here:

// https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/compression/gzip/compressor/v3/gzip.proto#extension-envoy-compression-gzip-compressor

// GzipCompressor defines the config for the Gzip compressor.

// T he default values can be found here:

// https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/compression/gzip/compressor/v3/gzip.proto#extension-envoy-compression-gzip-compressor

arkodg · 2024-01-26T23:11:00Z

api/v1alpha1/compression_types.go

+// the default value. For the default value can be reference here:
+// https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/compressor/v3/compressor.proto#extensions-filters-http-compressor-v3-compressor
+type Compression struct {
+	// CompressorType defines which type compressor wants to use for compression.


Suggested change

// CompressorType defines which type compressor wants to use for compression.

// CompressorType defines the compressor type to use for compression.

arkodg · 2024-01-26T23:11:18Z

api/v1alpha1/compression_types.go

+	// CompressorType defines which type compressor wants to use for compression.
+	//
+	// +required
+	Type CompressorType `json:"type,omitempty"`


Suggested change

Type CompressorType `json:"type,omitempty"`

Type CompressorType `json:"type"`

arkodg · 2024-01-26T23:13:53Z

api/v1alpha1/compression_types.go

+// Compression defines the config of compression for the http streams. Currently
+// only the minial config was added. All configs from the Envoy Proxy are using
+// the default value. For the default value can be reference here:
+// https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/compressor/v3/compressor.proto#extensions-filters-http-compressor-v3-compressor


Suggested change

// Compression defines the config of compression for the http streams. Currently

// only the minial config was added. All configs from the Envoy Proxy are using

// the default value. For the default value can be reference here:

// https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/compressor/v3/compressor.proto#extensions-filters-http-compressor-v3-compressor

// Compression defines the config of enabling compression.

// This can help reduce the bandwidth at the expense of higher CPU

Signed-off-by: He Jie Xu <[email protected]>

arkodg

LGTM thanks

soulxu added 6 commits January 22, 2024 00:47

Add API for compressor

9d259be

Signed-off-by: He Jie Xu <[email protected]>

update

7892a66

Signed-off-by: He Jie Xu <[email protected]>

update

91053d8

Signed-off-by: He Jie Xu <[email protected]>

update

9de7be5

Signed-off-by: He Jie Xu <[email protected]>

update doc

7973007

Signed-off-by: He Jie Xu <[email protected]>

generate manifests

0f3fa14

Signed-off-by: He Jie Xu <[email protected]>

soulxu requested a review from a team as a code owner January 22, 2024 04:29

soulxu changed the title ~~api: add Compressor support~~ api: add compressor/decompressor support Jan 22, 2024

soulxu added 2 commits January 22, 2024 05:40

generate

565e096

Signed-off-by: He Jie Xu <[email protected]>

Merge branch 'main' into compressor_support

7735e62

zhaohuabing reviewed Jan 22, 2024

View reviewed changes

zirain reviewed Jan 23, 2024

View reviewed changes

api/v1alpha1/backendtrafficpolicy_types.go Outdated Show resolved Hide resolved

soulxu added 3 commits January 24, 2024 01:59

address comments

4a121f5

Signed-off-by: He Jie Xu <[email protected]>

generate manifests

61cd1f0

Signed-off-by: He Jie Xu <[email protected]>

Merge branch 'main' into compressor_support

ab32e96

Merge branch 'main' into compressor_support

44ff825

arkodg reviewed Jan 24, 2024

View reviewed changes

api/v1alpha1/compression_types.go Outdated Show resolved Hide resolved

arkodg reviewed Jan 24, 2024

View reviewed changes

soulxu added 3 commits January 25, 2024 00:42

address comments

91ca6a2

Signed-off-by: He Jie Xu <[email protected]>

generate

6a97ebd

Signed-off-by: He Jie Xu <[email protected]>

update

582af88

Signed-off-by: He Jie Xu <[email protected]>

soulxu added 4 commits January 25, 2024 00:45

generate

1c7c5f7

Signed-off-by: He Jie Xu <[email protected]>

update comments

e80ad02

Signed-off-by: He Jie Xu <[email protected]>

generate

d069f46

Signed-off-by: He Jie Xu <[email protected]>

Merge branch 'main' into compressor_support

5830db7

arkodg reviewed Jan 25, 2024

View reviewed changes

api/v1alpha1/shared_types.go Outdated Show resolved Hide resolved

arkodg reviewed Jan 25, 2024

View reviewed changes

api/v1alpha1/compression_types.go Show resolved Hide resolved

soulxu added 4 commits January 25, 2024 06:45

address comments

03d872e

Signed-off-by: He Jie Xu <[email protected]>

generate

e91883f

Signed-off-by: He Jie Xu <[email protected]>

Merge branch 'main' into compressor_support

55b3c71

Merge branch 'main' into compressor_support

3d72544

arkodg reviewed Jan 26, 2024

View reviewed changes

soulxu added 3 commits January 26, 2024 23:17

address comments

5289a78

Signed-off-by: He Jie Xu <[email protected]>

generate

d3fdc64

Signed-off-by: He Jie Xu <[email protected]>

generate again

2a7558c

Signed-off-by: He Jie Xu <[email protected]>

arkodg approved these changes Jan 26, 2024

View reviewed changes

zirain approved these changes Jan 26, 2024

View reviewed changes

zirain merged commit 0bd88fe into envoyproxy:main Jan 26, 2024
20 checks passed

	// CompressorType defines which type compressor wants to use for compression.
	// CompressorType defines the compressor type to use for compression.

	Type CompressorType `json:"type,omitempty"`
	Type CompressorType `json:"type"`

api: add compressor/decompressor support #2474

api: add compressor/decompressor support #2474

Conversation

soulxu commented Jan 22, 2024

codecov bot commented Jan 22, 2024 • edited Loading

Codecov Report

soulxu commented Jan 22, 2024 • edited Loading

soulxu commented Jan 22, 2024

zhaohuabing left a comment • edited Loading

Choose a reason for hiding this comment

arkodg commented Jan 22, 2024

soulxu commented Jan 22, 2024

soulxu commented Jan 24, 2024 • edited Loading

zirain commented Jan 24, 2024

zhaohuabing commented Jan 24, 2024 • edited Loading

soulxu commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulxu Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulxu Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

arkodg Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2024 •

edited

Loading

soulxu commented Jan 22, 2024 •

edited

Loading

zhaohuabing left a comment •

edited

Loading

soulxu commented Jan 24, 2024 •

edited

Loading

zhaohuabing commented Jan 24, 2024 •

edited

Loading

soulxu Jan 25, 2024 •

edited

Loading

soulxu Jan 26, 2024 •

edited

Loading

arkodg Jan 25, 2024 •

edited

Loading