From 2b8a1794e7a7333358331ad454e73f58a3b1977c Mon Sep 17 00:00:00 2001 From: Patryk Rostkowski Date: Mon, 9 Jun 2025 22:11:58 +0200 Subject: [PATCH] fix: handle multi-value response headers as comma-separated per RFC 7230 This change updates response header modifier handling to comply with RFC 7230 by preserving header values containing commas as single strings. Request header behavior remains unchanged to avoid breaking existing functionality. Signed-off-by: Patryk Rostkowski --- internal/gatewayapi/filters.go | 4 +- internal/gatewayapi/filters_test.go | 100 ++++++++++++++++++++++++ internal/xds/translator/route.go | 4 + internal/xds/translator/route_test.go | 108 ++++++++++++++++++++++++++ 4 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 internal/gatewayapi/filters_test.go diff --git a/internal/gatewayapi/filters.go b/internal/gatewayapi/filters.go index 27c4a93ead..6636ff5460 100644 --- a/internal/gatewayapi/filters.go +++ b/internal/gatewayapi/filters.go @@ -617,7 +617,7 @@ func (t *Translator) processResponseHeaderModifierFilter( newHeader := ir.AddHeader{ Name: headerKey, Append: true, - Value: strings.Split(addHeader.Value, ","), + Value: []string{addHeader.Value}, } filterContext.AddResponseHeaders = append(filterContext.AddResponseHeaders, newHeader) @@ -672,7 +672,7 @@ func (t *Translator) processResponseHeaderModifierFilter( newHeader := ir.AddHeader{ Name: string(setHeader.Name), Append: false, - Value: strings.Split(setHeader.Value, ","), + Value: []string{setHeader.Value}, } filterContext.AddResponseHeaders = append(filterContext.AddResponseHeaders, newHeader) diff --git a/internal/gatewayapi/filters_test.go b/internal/gatewayapi/filters_test.go new file mode 100644 index 0000000000..f677e7822d --- /dev/null +++ b/internal/gatewayapi/filters_test.go @@ -0,0 +1,100 @@ +// Copyright Envoy Gateway Authors +// SPDX-License-Identifier: Apache-2.0 +// The full text of the Apache license is available in the LICENSE file at +// the root of the repo. + +package gatewayapi + +import ( + "reflect" + "testing" + + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + + "github.com/envoyproxy/gateway/internal/ir" +) + +func TestProcessResponseHeaderModifierFilter(t *testing.T) { + tests := []struct { + name string + filter *gwapiv1.HTTPHeaderFilter + expectedAdd []ir.AddHeader + expectedRemove []string + }{ + { + name: "Add headers", + filter: &gwapiv1.HTTPHeaderFilter{ + Add: []gwapiv1.HTTPHeader{ + {Name: "X-Test-Add", Value: "foo,bar"}, + }, + }, + expectedAdd: []ir.AddHeader{ + {Name: "X-Test-Add", Append: true, Value: []string{"foo,bar"}}, + }, + expectedRemove: nil, + }, + { + name: "Set headers", + filter: &gwapiv1.HTTPHeaderFilter{ + Set: []gwapiv1.HTTPHeader{ + {Name: "X-Test-Set", Value: "baz,qux"}, + }, + }, + expectedAdd: []ir.AddHeader{ + {Name: "X-Test-Set", Append: false, Value: []string{"baz,qux"}}, + }, + expectedRemove: nil, + }, + { + name: "Remove headers", + filter: &gwapiv1.HTTPHeaderFilter{ + Remove: []string{"X-Test-Remove", "X-Test-Remove2"}, + }, + expectedAdd: nil, + expectedRemove: []string{ + "X-Test-Remove", "X-Test-Remove2", + }, + }, + { + name: "Combined Add, Set, Remove", + filter: &gwapiv1.HTTPHeaderFilter{ + Add: []gwapiv1.HTTPHeader{ + {Name: "X-Add", Value: "val1,val2"}, + }, + Set: []gwapiv1.HTTPHeader{ + {Name: "X-Set", Value: "val3"}, + }, + Remove: []string{"X-Remove"}, + }, + expectedAdd: []ir.AddHeader{ + {Name: "X-Add", Append: true, Value: []string{"val1,val2"}}, + {Name: "X-Set", Append: false, Value: []string{"val3"}}, + }, + expectedRemove: []string{"X-Remove"}, + }, + { + name: "Empty config", + filter: &gwapiv1.HTTPHeaderFilter{}, + expectedAdd: nil, + expectedRemove: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + filterCtx := &HTTPFiltersContext{ + HTTPFilterIR: &HTTPFilterIR{}, + } + + translator := &Translator{} + translator.processResponseHeaderModifierFilter(tt.filter, filterCtx) + + if !reflect.DeepEqual(filterCtx.AddResponseHeaders, tt.expectedAdd) { + t.Errorf("unexpected AddResponseHeaders: got=%v, want=%v", filterCtx.AddResponseHeaders, tt.expectedAdd) + } + if !reflect.DeepEqual(filterCtx.RemoveResponseHeaders, tt.expectedRemove) { + t.Errorf("unexpected RemoveResponseHeaders: got=%v, want=%v", filterCtx.RemoveResponseHeaders, tt.expectedRemove) + } + }) + } +} diff --git a/internal/xds/translator/route.go b/internal/xds/translator/route.go index 2d61f73327..97f42c8858 100644 --- a/internal/xds/translator/route.go +++ b/internal/xds/translator/route.go @@ -522,6 +522,10 @@ func mirrorPercentByPolicy(mirror *ir.MirrorPolicy) *corev3.RuntimeFractionalPer } func buildXdsAddedHeaders(headersToAdd []ir.AddHeader) []*corev3.HeaderValueOption { + if len(headersToAdd) == 0 { + return nil + } + headerValueOptions := []*corev3.HeaderValueOption{} for _, header := range headersToAdd { diff --git a/internal/xds/translator/route_test.go b/internal/xds/translator/route_test.go index 817426c97b..7dbff0a979 100644 --- a/internal/xds/translator/route_test.go +++ b/internal/xds/translator/route_test.go @@ -9,6 +9,7 @@ import ( "reflect" "testing" + corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" "k8s.io/utils/ptr" @@ -171,3 +172,110 @@ func TestBuildUpgradeConfig(t *testing.T) { }) } } + +func TestBuildXdsAddedHeaders(t *testing.T) { + tests := []struct { + name string + headersToAdd []ir.AddHeader + want []*corev3.HeaderValueOption + }{ + { + name: "No headers", + headersToAdd: nil, + want: nil, + }, + { + name: "Single header, no value (should keep empty value)", + headersToAdd: []ir.AddHeader{ + {Name: "X-Test-Empty", Value: []string{}, Append: false}, + }, + want: []*corev3.HeaderValueOption{ + { + Header: &corev3.HeaderValue{Key: "X-Test-Empty"}, + AppendAction: corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD, + KeepEmptyValue: true, + }, + }, + }, + { + name: "Single header, one value, overwrite", + headersToAdd: []ir.AddHeader{ + {Name: "X-Test", Value: []string{"foo"}, Append: false}, + }, + want: []*corev3.HeaderValueOption{ + { + Header: &corev3.HeaderValue{Key: "X-Test", Value: "foo"}, + AppendAction: corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD, + KeepEmptyValue: false, + }, + }, + }, + { + name: "Single header, one value, append", + headersToAdd: []ir.AddHeader{ + {Name: "X-Test", Value: []string{"foo"}, Append: true}, + }, + want: []*corev3.HeaderValueOption{ + { + Header: &corev3.HeaderValue{Key: "X-Test", Value: "foo"}, + AppendAction: corev3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD, + KeepEmptyValue: false, + }, + }, + }, + { + name: "Header with multiple values (should join with comma)", + headersToAdd: []ir.AddHeader{ + {Name: "Cache-Control", Value: []string{"private,no-store"}, Append: false}, + }, + want: []*corev3.HeaderValueOption{ + { + Header: &corev3.HeaderValue{Key: "Cache-Control", Value: "private,no-store"}, + AppendAction: corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD, + KeepEmptyValue: false, + }, + }, + }, + { + name: "Multiple headers", + headersToAdd: []ir.AddHeader{ + {Name: "X-First", Value: []string{"foo"}, Append: false}, + {Name: "X-Second", Value: []string{"bar,baz"}, Append: true}, + }, + want: []*corev3.HeaderValueOption{ + { + Header: &corev3.HeaderValue{Key: "X-First", Value: "foo"}, + AppendAction: corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD, + KeepEmptyValue: false, + }, + { + Header: &corev3.HeaderValue{Key: "X-Second", Value: "bar,baz"}, + AppendAction: corev3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD, + KeepEmptyValue: false, + }, + }, + }, + { + name: "Header with explicit empty value string", + headersToAdd: []ir.AddHeader{ + {Name: "X-Explicit-Empty", Value: []string{""}, Append: false}, + }, + want: []*corev3.HeaderValueOption{ + { + Header: &corev3.HeaderValue{Key: "X-Explicit-Empty", Value: ""}, + AppendAction: corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD, + KeepEmptyValue: true, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := buildXdsAddedHeaders(tt.headersToAdd) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("buildXdsAddedHeaders() = %v, want %v", got, tt.want) + } + }) + } +}