internal/envoy: support multiple HTTP compression algorithms - #7699
internal/envoy: support multiple HTTP compression algorithms#7699anonrig wants to merge 3 commits into
Conversation
|
@tsaarni would you mind taking a look? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7699 +/- ##
==========================================
- Coverage 84.04% 84.03% -0.02%
==========================================
Files 131 131
Lines 14190 14187 -3
==========================================
- Hits 11926 11922 -4
- Misses 2264 2265 +1
🚀 New features to boost your workflow:
|
2a04f3d to
319ce36
Compare
Add compression.algorithms so Envoy can negotiate gzip, brotli, and zstd from Accept-Encoding. The existing algorithm field remains a single-algorithm shorthand and is mutually exclusive with algorithms. Fixes projectcontour#7698 Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
319ce36 to
042ce5d
Compare
tsaarni
left a comment
There was a problem hiding this comment.
@anonrig Thanks for contributing!
I've done a partial review and the PR looks good. I left some inline comments.
One larger suggestion: I think algorithms should be a pointer to a slice so it becomes a "tri-state":
- omitted for Envoy default
- empty list to disable compression
- populated list to use the listed algorithms
Algorithms *[]CompressionAlgorithm `json:"algorithms,omitempty"`This would let users disable compression via algorithms: [] instead of using different attribute for that. We can mark algorithm field deprecated.
Also same tri-state would make sense for the config file struct.
| // EnvoyCompression defines configuration related to compression in the default HTTP Listener filter chain. | ||
| type EnvoyCompression struct { |
There was a problem hiding this comment.
We could give early feedback by adding this CEL expression here
| // EnvoyCompression defines configuration related to compression in the default HTTP Listener filter chain. | |
| type EnvoyCompression struct { | |
| // EnvoyCompression defines configuration related to compression in the default HTTP Listener filter chain. | |
| // +kubebuilder:validation:XValidation:rule="!(has(self.algorithm) && has(self.algorithms))",message="compression algorithm and algorithms are mutually exclusive" | |
| type EnvoyCompression struct { |
There was a problem hiding this comment.
Added. EnvoyCompression now has this CEL rule so the API server rejects algorithm and algorithms together.
| }) | ||
| } | ||
|
|
||
| func TestHTTPConnectionManagerCompressionAlgorithms(t *testing.T) { |
There was a problem hiding this comment.
To test that DefaultFilters() sets "choose first" and "remove accept-encoding" flags correctly we could have the different scenarios here:
func TestHTTPConnectionManagerCompressionAlgorithms(t *testing.T) {
envoyGen := NewEnvoyGen(EnvoyGenOpt{
XDSClusterName: DefaultXDSClusterName,
})
type compressorExpectation struct {
name string
compressorLibraryName string
compressorLibrary proto.Message
chooseFirst bool
removeAcceptEncoding bool
}
tests := map[string]struct {
algorithms []contour_v1alpha1.CompressionAlgorithm
want []compressorExpectation
}{
"one algorithm": {
algorithms: []contour_v1alpha1.CompressionAlgorithm{contour_v1alpha1.ZstdCompression},
want: []compressorExpectation{
{name: CompressorFilterName, compressorLibraryName: "zstd", compressorLibrary: &envoy_compression_zstd_compressor_v3.Zstd{}},
},
},
"two algorithms": {
algorithms: []contour_v1alpha1.CompressionAlgorithm{
contour_v1alpha1.BrotliCompression,
contour_v1alpha1.GzipCompression,
},
want: []compressorExpectation{
{name: CompressorFilterName + ".brotli", compressorLibraryName: "brotli", compressorLibrary: &envoy_compression_brotli_compressor_v3.Brotli{}, chooseFirst: true},
{name: CompressorFilterName + ".gzip", compressorLibraryName: "gzip", compressorLibrary: &envoy_compression_gzip_compressor_v3.Gzip{}, removeAcceptEncoding: true},
},
},
"three algorithms": {
algorithms: []contour_v1alpha1.CompressionAlgorithm{
contour_v1alpha1.GzipCompression,
contour_v1alpha1.BrotliCompression,
contour_v1alpha1.ZstdCompression,
},
want: []compressorExpectation{
{name: CompressorFilterName + ".gzip", compressorLibraryName: "gzip", compressorLibrary: &envoy_compression_gzip_compressor_v3.Gzip{}, chooseFirst: true},
{name: CompressorFilterName + ".brotli", compressorLibraryName: "brotli", compressorLibrary: &envoy_compression_brotli_compressor_v3.Brotli{}},
{name: CompressorFilterName + ".zstd", compressorLibraryName: "zstd", compressorLibrary: &envoy_compression_zstd_compressor_v3.Zstd{}, removeAcceptEncoding: true},
},
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
gotFilter := envoyGen.HTTPConnectionManagerBuilder().
Compression(&contour_v1alpha1.EnvoyCompression{Algorithms: tc.algorithms}).
DefaultFilters().
Get()
var got envoy_filter_network_http_connection_manager_v3.HttpConnectionManager
require.NoError(t, gotFilter.GetTypedConfig().UnmarshalTo(&got))
require.GreaterOrEqual(t, len(got.HttpFilters), len(tc.want)+1)
assert.Equal(t, GRPCWebFilterName, got.HttpFilters[len(tc.want)].Name)
for i, want := range tc.want {
var compressor envoy_filter_http_compressor_v3.Compressor
require.NoError(t, got.HttpFilters[i].GetTypedConfig().UnmarshalTo(&compressor))
assert.Equal(t, want.name, got.HttpFilters[i].Name)
require.NotNil(t, compressor.CompressorLibrary)
require.NotNil(t, compressor.CompressorLibrary.TypedConfig)
assert.Equal(t, want.compressorLibraryName, compressor.CompressorLibrary.Name)
assert.True(t, compressor.CompressorLibrary.TypedConfig.MessageIs(want.compressorLibrary))
assert.Equal(t, want.chooseFirst, compressor.ChooseFirst)
assert.Equal(t, want.removeAcceptEncoding, compressor.ResponseDirectionConfig.RemoveAcceptEncodingHeader)
}
})
}
}There was a problem hiding this comment.
Added, including the one/two/three algorithm cases plus an empty-list case now that algorithms: [] disables compression.
There was a problem hiding this comment.
For clarity:
| // Allowable values are defined as names of well known compression algorithms. |
since the same type is used for both Algorithm and Algorithms, where in one case disable is allowed and in the other it is not.
There was a problem hiding this comment.
Updated. The type comment no longer mentions disabled, since that value is only valid on the deprecated algorithm field.
|
Sounds good. I'll update the PR. Thank you for the review. |
Address review feedback on projectcontour#7699 by making algorithms a pointer to a slice so omitted, empty, and populated lists are distinct. An empty list disables compression. Mark algorithm deprecated, add CEL mutual-exclusivity validation, and expand compressor filter tests.
|
@tsaarni thanks for the review. I updated the PR to match the feedback:
|
Address review feedback on projectcontour#7699 by making algorithms a pointer to a slice so omitted, empty, and populated lists are distinct. An empty list disables compression. Mark algorithm deprecated, add CEL mutual-exclusivity validation, and expand compressor filter tests. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
e1ebfdc to
02b8c26
Compare
staticcheck SA1019 flags the deprecated algorithm field. Keep reading it for backward compatibility, matching the existing SubjectName exclusion. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
|
@tsaarni can you re-review? |
Thanks for contributing to Contour!
Adds
compression.algorithmsso Contour can program more than one Envoy compressor filter and negotiate the encoding fromAccept-Encoding.The existing
algorithmfield is unchanged as a single-algorithm shorthand, includingdisabled. The two fields are mutually exclusive.When more than one algorithm is configured:
choose_firston the first entry)Accept-Encodingis stripped on the last filter so backends do not compress first and hide the other encodingsAlso documents the brotli and zstd Envoy compressor extensions in the compatibility matrix.
Fixes #7698