diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go index 2dfef92eeaa..31624f74c3d 100644 --- a/internal/gatewayapi/backendtrafficpolicy.go +++ b/internal/gatewayapi/backendtrafficpolicy.go @@ -1590,13 +1590,14 @@ func buildCompression(compression, compressor []*egv1a1.Compression) []*ir.Compr // Handle the Compressor field first (higher priority) if len(compressor) > 0 { irCompression := make([]*ir.Compression, 0, len(compressor)) - for _, c := range compressor { + for i, c := range compressor { // Only add compression if the corresponding compressor not null if (c.Type == egv1a1.GzipCompressorType && c.Gzip != nil) || (c.Type == egv1a1.BrotliCompressorType && c.Brotli != nil) || (c.Type == egv1a1.ZstdCompressorType && c.Zstd != nil) { irCompression = append(irCompression, &ir.Compression{ - Type: c.Type, + Type: c.Type, + ChooseFirst: i == 0, // only the first compressor is marked as ChooseFirst }) } } @@ -1608,9 +1609,10 @@ func buildCompression(compression, compressor []*egv1a1.Compression) []*ir.Compr return nil } irCompression := make([]*ir.Compression, 0, len(compression)) - for _, c := range compression { + for i, c := range compression { irCompression = append(irCompression, &ir.Compression{ - Type: c.Type, + Type: c.Type, + ChooseFirst: i == 0, // only the first compressor is marked as ChooseFirst }) } diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-compression-json-merge.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-compression-json-merge.out.yaml index dc312308151..eccc8045f1e 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-compression-json-merge.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-compression-json-merge.out.yaml @@ -235,7 +235,8 @@ xdsIR: prefix: / traffic: compression: - - type: Brotli + - chooseFirst: true + type: Brotli - type: Zstd readyListener: address: 0.0.0.0 diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-compression-strategic-merge.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-compression-strategic-merge.out.yaml index 3497cc5b6fc..333d68bec61 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-compression-strategic-merge.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-compression-strategic-merge.out.yaml @@ -374,7 +374,8 @@ xdsIR: prefix: /admin traffic: compression: - - type: Gzip + - chooseFirst: true + type: Gzip - destination: metadata: kind: HTTPRoute @@ -406,7 +407,8 @@ xdsIR: prefix: /api traffic: compression: - - type: Brotli + - chooseFirst: true + type: Brotli - type: Zstd - destination: metadata: @@ -439,7 +441,8 @@ xdsIR: prefix: / traffic: compression: - - type: Brotli + - chooseFirst: true + type: Brotli - type: Zstd - type: Gzip readyListener: diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-compression.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-compression.out.yaml index 2c6a9a94609..e7336372839 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-compression.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-compression.out.yaml @@ -198,7 +198,8 @@ xdsIR: prefix: / traffic: compression: - - type: Brotli + - chooseFirst: true + type: Brotli - type: Gzip - type: Zstd readyListener: diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-basic.enable.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-basic.enable.out.yaml index 3643a423633..49e88555dd7 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-basic.enable.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-basic.enable.out.yaml @@ -201,7 +201,8 @@ xdsIR: prefix: / traffic: compression: - - type: Brotli + - chooseFirst: true + type: Brotli - type: Gzip - type: Zstd readyListener: diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-json-merge-enable-only-gzip.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-json-merge-enable-only-gzip.out.yaml index b18c321f718..bb5d30d941b 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-json-merge-enable-only-gzip.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-json-merge-enable-only-gzip.out.yaml @@ -233,7 +233,8 @@ xdsIR: prefix: / traffic: compression: - - type: Gzip + - chooseFirst: true + type: Gzip readyListener: address: 0.0.0.0 ipFamily: IPv4 diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-json-merge-enable-only-zstd.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-json-merge-enable-only-zstd.out.yaml index fee028e6fbf..57035619404 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-json-merge-enable-only-zstd.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-json-merge-enable-only-zstd.out.yaml @@ -233,7 +233,8 @@ xdsIR: prefix: / traffic: compression: - - type: Zstd + - chooseFirst: true + type: Zstd readyListener: address: 0.0.0.0 ipFamily: IPv4 diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-strategic-merge.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-strategic-merge.out.yaml index af59cfae324..0d00ac95620 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-strategic-merge.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-compressor-strategic-merge.out.yaml @@ -375,7 +375,8 @@ xdsIR: prefix: /admin traffic: compression: - - type: Gzip + - chooseFirst: true + type: Gzip - destination: metadata: kind: HTTPRoute @@ -407,7 +408,8 @@ xdsIR: prefix: /api traffic: compression: - - type: Brotli + - chooseFirst: true + type: Brotli - destination: metadata: kind: HTTPRoute @@ -439,7 +441,8 @@ xdsIR: prefix: / traffic: compression: - - type: Brotli + - chooseFirst: true + type: Brotli - type: Gzip readyListener: address: 0.0.0.0 diff --git a/internal/ir/xds.go b/internal/ir/xds.go index bc4f6175a30..03288b49341 100644 --- a/internal/ir/xds.go +++ b/internal/ir/xds.go @@ -891,6 +891,8 @@ type HeaderBasedSessionPersistence struct { type Compression struct { // Type of compression to be used. Type egv1a1.CompressorType `json:"type" yaml:"type"` + // ChooseFirst indicates this compressor is preferred when q-values in Accept-Encoding are equal. + ChooseFirst bool `json:"chooseFirst,omitempty" yaml:"chooseFirst,omitempty"` } // TrafficFeatures holds the information associated with the Backend Traffic Policy. diff --git a/internal/xds/translator/compressor.go b/internal/xds/translator/compressor.go index fc36166c17a..0ed502953c9 100644 --- a/internal/xds/translator/compressor.go +++ b/internal/xds/translator/compressor.go @@ -8,6 +8,7 @@ package translator import ( "errors" "fmt" + "slices" "strings" corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -51,11 +52,10 @@ func (*compressor) patchHCM(mgr *hcmv3.HttpConnectionManager, irListener *ir.HTT for _, route := range irListener.Routes { if route.Traffic != nil && route.Traffic.Compression != nil { - for i, irComp := range route.Traffic.Compression { + for _, irComp := range route.Traffic.Compression { filterName := compressorFilterName(irComp.Type) if !hcmContainsFilter(mgr, filterName) { - chooseFirst := i == 0 - if filter, err = buildCompressorFilter(irComp, chooseFirst); err != nil { + if filter, err = buildCompressorFilter(irComp); err != nil { return err } mgr.HttpFilters = append(mgr.HttpFilters, filter) @@ -72,7 +72,7 @@ func compressorFilterName(compressorType egv1a1.CompressorType) string { } // buildCompressorFilter builds a compressor filter with the provided compressionType. -func buildCompressorFilter(compression *ir.Compression, chooseFirst bool) (*hcmv3.HttpFilter, error) { +func buildCompressorFilter(compression *ir.Compression) (*hcmv3.HttpFilter, error) { var ( compressorProto *compressorv3.Compressor extensionName string @@ -105,7 +105,7 @@ func buildCompressorFilter(compression *ir.Compression, chooseFirst bool) (*hcmv }, } - if chooseFirst { + if compression.ChooseFirst { compressorProto.ChooseFirst = true } @@ -151,6 +151,7 @@ func (*compressor) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute, _ *ir route.TypedPerFilterConfig = make(map[string]*anypb.Any) } + compressorProto := compressorPerRouteConfig() for _, irComp := range irRoute.Traffic.Compression { filterName := compressorFilterName(irComp.Type) if _, ok := perFilterCfg[filterName]; ok { @@ -160,7 +161,6 @@ func (*compressor) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute, _ *ir filterName, route) } - compressorProto := compressorPerRouteConfig() if compressorAny, err = proto.ToAnyWithValidation(compressorProto); err != nil { return err } @@ -168,6 +168,11 @@ func (*compressor) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute, _ *ir route.TypedPerFilterConfig[filterName] = compressorAny } + // Ensure accept-encoding from the request to prevent double compression. + if !slices.Contains(route.RequestHeadersToRemove, "accept-encoding") { + route.RequestHeadersToRemove = append(route.RequestHeadersToRemove, "accept-encoding") + } + return nil } diff --git a/internal/xds/translator/testdata/out/xds-ir/compression.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/compression.listeners.yaml index 2e08d7f16a7..73b8355bd83 100644 --- a/internal/xds/translator/testdata/out/xds-ir/compression.listeners.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/compression.listeners.yaml @@ -18,7 +18,6 @@ name: envoy.filters.http.compressor.brotli typedConfig: '@type': type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor - chooseFirst: true compressorLibrary: name: envoy.compression.brotli.compressor typedConfig: diff --git a/internal/xds/translator/testdata/out/xds-ir/compression.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/compression.routes.yaml index 01a3beb5f78..a7f48fe5e1e 100644 --- a/internal/xds/translator/testdata/out/xds-ir/compression.routes.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/compression.routes.yaml @@ -23,6 +23,8 @@ name: httproute-1 namespace: default name: httproute/default/httproute-1/rule/0/match/0/gateway_envoyproxy_io + requestHeadersToRemove: + - accept-encoding route: cluster: httproute/default/httproute-1/rule/0 upgradeConfigs: diff --git a/test/e2e/tests/compression.go b/test/e2e/tests/compression.go index 92022c264c8..ddb50b75160 100644 --- a/test/e2e/tests/compression.go +++ b/test/e2e/tests/compression.go @@ -16,6 +16,7 @@ import ( "net" nethttp "net/http" "net/http/httputil" + "strings" "testing" "github.com/andybalholm/brotli" @@ -55,8 +56,14 @@ var CompressionTest = suite.ConformanceTest{ testCompression(t, suite, egv1a1.ZstdCompressorType) }) - t.Run("HTTPRoute with brotli compression chooseFirst", func(t *testing.T) { - testCompressionChooseFirst(t, suite, egv1a1.BrotliCompressorType) + t.Run("HTTPRoute with multiple compressors chooseFirst", func(t *testing.T) { + testCompression( + t, + suite, + egv1a1.BrotliCompressorType, + egv1a1.GzipCompressorType, + egv1a1.ZstdCompressorType, + ) }) t.Run("HTTPRoute without compression", func(t *testing.T) { @@ -77,7 +84,7 @@ var CompressionTest = suite.ConformanceTest{ Request: http.Request{ Path: "/no-compression", Headers: map[string]string{ - "Accept-encoding": "gzip", + "accept-encoding": "gzip", }, }, Response: http.Response{ @@ -92,7 +99,7 @@ var CompressionTest = suite.ConformanceTest{ }, } -func testCompression(t *testing.T, suite *suite.ConformanceTestSuite, compressionType egv1a1.CompressorType) { +func testCompression(t *testing.T, suite *suite.ConformanceTestSuite, compressionTypes ...egv1a1.CompressorType) { ns := "gateway-conformance-infra" routeNN := types.NamespacedName{Name: "compression", Namespace: ns} gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns} @@ -106,52 +113,29 @@ func testCompression(t *testing.T, suite *suite.ConformanceTestSuite, compressio } BackendTrafficPolicyMustBeAccepted(t, suite.Client, types.NamespacedName{Name: "compression", Namespace: ns}, suite.ControllerName, ancestorRef) - encoding := ContentEncoding(compressionType) - expectedResponse := http.ExpectedResponse{ - Request: http.Request{ - Path: "/compression", - Headers: map[string]string{ - "Accept-encoding": encoding, - }, - }, - Response: http.Response{ - StatusCodes: []int{200}, - Headers: map[string]string{ - "content-encoding": encoding, - }, - }, - Namespace: ns, - } - roundTripper := &CompressionRoundTripper{Debug: suite.Debug, TimeoutConfig: suite.TimeoutConfig} - http.MakeRequestAndExpectEventuallyConsistentResponse(t, roundTripper, suite.TimeoutConfig, gwAddr, expectedResponse) -} - -func testCompressionChooseFirst(t *testing.T, suite *suite.ConformanceTestSuite, compressionType egv1a1.CompressorType) { - ns := "gateway-conformance-infra" - routeNN := types.NamespacedName{Name: "compression", Namespace: ns} - gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns} - gwAddr := kubernetes.GatewayAndRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), &gwapiv1.HTTPRoute{}, false, routeNN) - - ancestorRef := gwapiv1.ParentReference{ - Group: gatewayapi.GroupPtr(gwapiv1.GroupName), - Kind: gatewayapi.KindPtr(resource.KindGateway), - Namespace: gatewayapi.NamespacePtr(gwNN.Namespace), - Name: gwapiv1.ObjectName(gwNN.Name), + encodings := make([]string, len(compressionTypes)) + for i, ct := range compressionTypes { + encodings[i] = ContentEncoding(ct) } - BackendTrafficPolicyMustBeAccepted(t, suite.Client, types.NamespacedName{Name: "compression", Namespace: ns}, suite.ControllerName, ancestorRef) - encoding := ContentEncoding(compressionType) + acceptEncoding := strings.Join(encodings, ", ") expectedResponse := http.ExpectedResponse{ Request: http.Request{ Path: "/compression", Headers: map[string]string{ - "Accept-encoding": "gzip, br, zstd", + "accept-encoding": acceptEncoding, + }, + }, + ExpectedRequest: &http.ExpectedRequest{ + Request: http.Request{ + Path: "/compression", }, + AbsentHeaders: []string{"accept-encoding"}, }, Response: http.Response{ StatusCodes: []int{200}, Headers: map[string]string{ - "content-encoding": encoding, + "content-encoding": encodings[0], }, }, Namespace: ns,