From 655e921c8206117a046d597b3764c08fd6cfd570 Mon Sep 17 00:00:00 2001 From: Jakub Warczarek Date: Wed, 18 Dec 2024 01:02:53 +0100 Subject: [PATCH] fix: no redundant space in header `Location` for `HTTPRoute` with requestRedirect filter (#6855) --- CHANGELOG.md | 3 + .../translator/subtranslator/httproute.go | 62 +++++++------ .../subtranslator/httproute_atc_test.go | 12 ++- .../subtranslator/httproute_test.go | 86 +++++++++---------- .../translator/translate_httproute_test.go | 14 +-- 5 files changed, 100 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ed0f25db0..cf69da8bd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -201,6 +201,9 @@ Adding a new version? You'll need three changes: - Fixed validation of JWT credentials using non HMAC algorithms where `secret` field was incorrectly required. [#6848](https://github.com/Kong/kubernetes-ingress-controller/pull/6848) +- There is no redundant space in header `Location` when `HTTPRoute` with + requestRedirect filter is used. + [#6855](https://github.com/Kong/kubernetes-ingress-controller/pull/6855) ### Added diff --git a/internal/dataplane/translator/subtranslator/httproute.go b/internal/dataplane/translator/subtranslator/httproute.go index 3e01a1c806..60385191db 100644 --- a/internal/dataplane/translator/subtranslator/httproute.go +++ b/internal/dataplane/translator/subtranslator/httproute.go @@ -1145,14 +1145,8 @@ func schemeHostPortFromHTTPPathModifier(modifier *gatewayapi.HTTPRequestRedirect // TODO: According to the spec of gateway API, the original scheme should be kept if no scheme is given. // So the logic may be changed if Kong gateway supports: // https://github.com/Kong/kubernetes-ingress-controller/issues/6856 - scheme := "http" - if modifier.Scheme != nil { - scheme = *modifier.Scheme - } - hostname := "" - if modifier.Hostname != nil { - hostname = string(*modifier.Hostname) - } + scheme := lo.FromPtrOr(modifier.Scheme, "http") + hostname := string(lo.FromPtrOr(modifier.Hostname, "")) // As gateway API specified, if `Port` is nil, port should be left empty for using well-known port for the scheme. // This changed the behavior in previous versions of using port `80` if the port is not given. portStr := "" @@ -1173,29 +1167,30 @@ func generateRequestRedirectKongPlugin(modifier *gatewayapi.HTTPRequestRedirectF }, } - var locationHeader string - if modifier.Path != nil && modifier.Path.Type == gatewayapi.FullPathHTTPPathModifier && modifier.Path.ReplaceFullPath != nil { // only ReplaceFullPath currently supported path = *modifier.Path.ReplaceFullPath } + + const headerLocationKey = "Location" + var headerLocation Header if modifier.Hostname != nil { scheme, hostPort := schemeHostPortFromHTTPPathModifier(modifier) location := pathlib.Join(hostPort, path) if scheme != "" { location = scheme + "://" + location } - locationHeader = "Location: " + location + headerLocation = NewHeader(headerLocationKey, location) } else { - locationHeader = "Location: " + path + headerLocation = NewHeader(headerLocationKey, path) } transformerPlugin := transformerPlugin{ Type: transformerPluginTypeResponse, Add: TransformerPluginConfig{ - Headers: []string{locationHeader}, + Headers: []Header{headerLocation}, }, } @@ -1277,16 +1272,32 @@ type transformerPlugin struct { Remove TransformerPluginConfig `json:"remove,omitempty"` } +// Header represents a header as a string "key:value" or just "key". This is the format accepted +// by TransformerPluginConfig. +type Header string + +// NewHeader creates a new Header with the given key and value in format accepted by TransformerPluginConfig. +// For key-only header (e.g. for remove filter), set value to an empty string. +func NewHeader(key, value string) Header { + // It's important to return the value in the format "key:value" (without a space after the colon) + // to not have a redundant whitespace (some browser does not trim it). + if value != "" { + return Header(fmt.Sprintf("%s:%s", key, value)) + } + // Having only key is correct in the case of remove filter. + return Header(key) +} + // TransformerPluginConfig is a configuration for request-transformer and response-transformer plugins' // "add", "append", and "remove" fields. type TransformerPluginConfig struct { - Headers []string `json:"headers,omitempty"` + Headers []Header `json:"headers,omitempty"` } // TransformerPluginReplaceConfig is a configuration for request-transformer and response-transformer plugins' // "replace" field. type TransformerPluginReplaceConfig struct { - Headers []string `json:"headers,omitempty"` + Headers []Header `json:"headers,omitempty"` URI string `json:"uri,omitempty"` } @@ -1297,7 +1308,7 @@ func generateHeaderModifierKongPlugin(modifier *gatewayapi.HTTPHeaderFilter, plu // modifier.Set is converted to a pair composed of "replace" and "add" if modifier.Set != nil { - setModifiers := make([]string, 0, len(modifier.Set)) + setModifiers := make([]Header, 0, len(modifier.Set)) for _, s := range modifier.Set { setModifiers = append(setModifiers, kongHeaderFormatter(s)) } @@ -1311,7 +1322,7 @@ func generateHeaderModifierKongPlugin(modifier *gatewayapi.HTTPHeaderFilter, plu // modifier.Add is converted to "append" if modifier.Add != nil { - appendModifiers := make([]string, 0, len(modifier.Add)) + appendModifiers := make([]Header, 0, len(modifier.Add)) for _, a := range modifier.Add { appendModifiers = append(appendModifiers, kongHeaderFormatter(a)) } @@ -1322,15 +1333,17 @@ func generateHeaderModifierKongPlugin(modifier *gatewayapi.HTTPHeaderFilter, plu if modifier.Remove != nil { plugin.Remove = TransformerPluginConfig{ - Headers: modifier.Remove, + Headers: lo.Map(modifier.Remove, func(headerKey string, _ int) Header { + return NewHeader(headerKey, "") + }), } } return plugin } -func kongHeaderFormatter(header gatewayapi.HTTPHeader) string { - return fmt.Sprintf("%s:%s", header.Name, header.Value) +func kongHeaderFormatter(header gatewayapi.HTTPHeader) Header { + return NewHeader(string(header.Name), header.Value) } func generateRequestTransformerForURLRewrite( @@ -1390,17 +1403,14 @@ func generateRequestTransformerForURLRewritePath( func generateRequestTransformerForURLRewriteHostname( filter *gatewayapi.HTTPURLRewriteFilter, ) transformerPlugin { + hostHeader := []Header{NewHeader("host", string(*filter.Hostname))} return transformerPlugin{ Type: transformerPluginTypeRequest, Replace: TransformerPluginReplaceConfig{ - Headers: []string{ - fmt.Sprintf("host:%s", string(*filter.Hostname)), - }, + Headers: hostHeader, }, Add: TransformerPluginConfig{ - Headers: []string{ - fmt.Sprintf("host:%s", string(*filter.Hostname)), - }, + Headers: hostHeader, }, } } diff --git a/internal/dataplane/translator/subtranslator/httproute_atc_test.go b/internal/dataplane/translator/subtranslator/httproute_atc_test.go index 7f8e6ee4a4..998c2c9359 100644 --- a/internal/dataplane/translator/subtranslator/httproute_atc_test.go +++ b/internal/dataplane/translator/subtranslator/httproute_atc_test.go @@ -141,7 +141,9 @@ func TestGenerateKongExpressionRoutesFromHTTPRouteMatches(t *testing.T) { Name: kong.String("response-transformer"), Config: kong.Configuration{ "add": TransformerPluginConfig{ - Headers: []string{`Location: http://a.foo.com/exact/0`}, + Headers: []Header{ + NewHeader("Location", "http://a.foo.com/exact/0"), + }, }, }, }, @@ -167,7 +169,9 @@ func TestGenerateKongExpressionRoutesFromHTTPRouteMatches(t *testing.T) { Name: kong.String("response-transformer"), Config: kong.Configuration{ "add": TransformerPluginConfig{ - Headers: []string{`Location: http://a.foo.com/exact/1`}, + Headers: []Header{ + NewHeader("Location", "http://a.foo.com/exact/1"), + }, }, }, }, @@ -203,7 +207,9 @@ func TestGenerateKongExpressionRoutesFromHTTPRouteMatches(t *testing.T) { Name: kong.String("request-transformer"), Config: kong.Configuration{ "append": TransformerPluginConfig{ - Headers: []string{"foo:bar"}, + Headers: []Header{ + NewHeader("foo", "bar"), + }, }, }, }, diff --git a/internal/dataplane/translator/subtranslator/httproute_test.go b/internal/dataplane/translator/subtranslator/httproute_test.go index 29eee2259a..69e467505e 100644 --- a/internal/dataplane/translator/subtranslator/httproute_test.go +++ b/internal/dataplane/translator/subtranslator/httproute_test.go @@ -62,23 +62,23 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { Name: kong.String("request-transformer"), Config: kong.Configuration{ "add": TransformerPluginConfig{ - Headers: []string{ - "header-to-set:bar", + Headers: []Header{ + NewHeader("header-to-set", "bar"), }, }, "append": TransformerPluginConfig{ - Headers: []string{ - "header-to-add:foo", + Headers: []Header{ + NewHeader("header-to-add", "foo"), }, }, "remove": TransformerPluginConfig{ - Headers: []string{ - "header-to-remove", + Headers: []Header{ + NewHeader("header-to-remove", ""), }, }, "replace": TransformerPluginReplaceConfig{ - Headers: []string{ - "header-to-set:bar", + Headers: []Header{ + NewHeader("header-to-set", "bar"), }, }, }, @@ -108,8 +108,8 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { Name: kong.String("response-transformer"), Config: kong.Configuration{ "add": TransformerPluginConfig{ - Headers: []string{ - "Location: http://example.org/test", + Headers: []Header{ + NewHeader("Location", "http://example.org/test"), }, }, }, @@ -143,23 +143,23 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { Name: kong.String("response-transformer"), Config: kong.Configuration{ "add": TransformerPluginConfig{ - Headers: []string{ - "header-to-set:bar", + Headers: []Header{ + NewHeader("header-to-set", "bar"), }, }, "append": TransformerPluginConfig{ - Headers: []string{ - "header-to-add:foo", + Headers: []Header{ + NewHeader("header-to-add", "foo"), }, }, "remove": TransformerPluginConfig{ - Headers: []string{ - "header-to-remove", + Headers: []Header{ + NewHeader("header-to-remove", ""), }, }, "replace": TransformerPluginReplaceConfig{ - Headers: []string{ - "header-to-set:bar", + Headers: []Header{ + NewHeader("header-to-set", "bar"), }, }, }, @@ -167,7 +167,7 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { }, }, { - name: "valid extensionrefs filters", + name: "valid extension refs filters", filters: []gatewayapi.HTTPRouteFilter{ { Type: gatewayapi.HTTPRouteFilterExtensionRef, @@ -196,7 +196,7 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { expectedPlugins: nil, }, { - name: "invalid extensionrefs filter group", + name: "invalid extension refs filter group", filters: []gatewayapi.HTTPRouteFilter{ { Type: gatewayapi.HTTPRouteFilterExtensionRef, @@ -210,7 +210,7 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { expectedErr: errors.New("plugin wrong.group/KongPlugin unsupported"), }, { - name: "invalid extensionrefs filter kind", + name: "invalid extension refs filter kind", filters: []gatewayapi.HTTPRouteFilter{ { Type: gatewayapi.HTTPRouteFilterExtensionRef, @@ -253,14 +253,14 @@ func TestGeneratePluginsFromHTTPRouteFilters(t *testing.T) { Name: kong.String("request-transformer"), Config: kong.Configuration{ "add": TransformerPluginConfig{ - Headers: []string{ - "header-to-set:bar", + Headers: []Header{ + NewHeader("header-to-set", "bar"), }, }, "replace": TransformerPluginReplaceConfig{ URI: "/new$(uri_captures[1])", - Headers: []string{ - "header-to-set:bar", + Headers: []Header{ + NewHeader("header-to-set", "bar"), }, }, }, @@ -494,13 +494,13 @@ func TestGenerateRequestTransformerForURLRewrite(t *testing.T) { expected: []transformerPlugin{{ Type: transformerPluginTypeRequest, Replace: TransformerPluginReplaceConfig{ - Headers: []string{ - "host:replaced.host", + Headers: []Header{ + NewHeader("host", "replaced.host"), }, }, Add: TransformerPluginConfig{ - Headers: []string{ - "host:replaced.host", + Headers: []Header{ + NewHeader("host", "replaced.host"), }, }, }}, @@ -526,13 +526,13 @@ func TestGenerateRequestTransformerForURLRewrite(t *testing.T) { { Type: transformerPluginTypeRequest, Replace: TransformerPluginReplaceConfig{ - Headers: []string{ - "host:replaced.host", + Headers: []Header{ + NewHeader("host", "replaced.host"), }, }, Add: TransformerPluginConfig{ - Headers: []string{ - "host:replaced.host", + Headers: []Header{ + NewHeader("host", "replaced.host"), }, }, }, @@ -679,7 +679,7 @@ func TestMergePluginsOfTheSameType(t *testing.T) { { Type: transformerPluginTypeRequest, Add: TransformerPluginConfig{ - Headers: []string{"header1:value1"}, + Headers: []Header{NewHeader("header1", "value1")}, }, Replace: TransformerPluginReplaceConfig{ URI: "path1", @@ -688,7 +688,7 @@ func TestMergePluginsOfTheSameType(t *testing.T) { { Type: transformerPluginTypeRequest, Add: TransformerPluginConfig{ - Headers: []string{"header2:value2"}, + Headers: []Header{NewHeader("header2", "value2")}, }, Replace: TransformerPluginReplaceConfig{ URI: "path2", @@ -699,7 +699,7 @@ func TestMergePluginsOfTheSameType(t *testing.T) { { Type: transformerPluginTypeRequest, Add: TransformerPluginConfig{ - Headers: []string{"header1:value1", "header2:value2"}, + Headers: []Header{NewHeader("header1", "value1"), NewHeader("header2", "value2")}, }, Replace: TransformerPluginReplaceConfig{ URI: "path1", @@ -1022,7 +1022,7 @@ func TestTranslateHTTPRoutesToKongstateServices(t *testing.T) { Kind: "Service", } - refernceGrantTypeMeta := metav1.TypeMeta{ + referenceGrantTypeMeta := metav1.TypeMeta{ APIVersion: "gateway.networking.k8s.io/v1beta1", Kind: "ReferenceGrant", } @@ -1271,7 +1271,7 @@ func TestTranslateHTTPRoutesToKongstateServices(t *testing.T) { }, referenceGrants: []*gatewayapi.ReferenceGrant{ { - TypeMeta: refernceGrantTypeMeta, + TypeMeta: referenceGrantTypeMeta, ObjectMeta: metav1.ObjectMeta{ Namespace: "default", Name: "grant-from-httproute-to-service", @@ -1507,7 +1507,7 @@ func TestTranslateHTTPRoutesToKongstateServices(t *testing.T) { }, referenceGrants: []*gatewayapi.ReferenceGrant{ { - TypeMeta: refernceGrantTypeMeta, + TypeMeta: referenceGrantTypeMeta, ObjectMeta: metav1.ObjectMeta{ Namespace: "default", Name: "grant-from-httproute-to-service", @@ -1684,7 +1684,7 @@ func TestTranslateHTTPRoutesToKongstateServices(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { logger := logr.Discard() - fakestore, err := store.NewFakeStore(store.FakeObjects{ + fakeStore, err := store.NewFakeStore(store.FakeObjects{ Services: tc.k8sServices, ReferenceGrants: tc.referenceGrants, }) @@ -1700,7 +1700,7 @@ func TestTranslateHTTPRoutesToKongstateServices(t *testing.T) { ExpressionRoutes: false, SupportRedirectPlugin: false, } - translationResult := TranslateHTTPRoutesToKongstateServices(logger, fakestore, tc.httpRoutes, translateOptions) + translationResult := TranslateHTTPRoutesToKongstateServices(logger, fakeStore, tc.httpRoutes, translateOptions) require.Len(t, translationResult.HTTPRouteNameToTranslationErrors, 0, "Should not get translation errors in translating") kongstateServices := translationResult.ServiceNameToKongstateService @@ -1712,10 +1712,10 @@ func TestTranslateHTTPRoutesToKongstateServices(t *testing.T) { require.Equal(t, *expectedService.Name, *s.Name, "Service %s should have expected name inside", serviceName) require.Equal(t, *expectedService.Host, *s.Host, "Service %s should have expected host", serviceName) // compare backends. - require.Lenf(t, s.Backends, len(expectedService.Backends), "Service %s should have expected number of backends") + require.Len(t, s.Backends, len(expectedService.Backends), "Service %s should have expected number of backends", serviceName) backendCompareMsg := `Service %s backend %d should have expected %s` // service name, backend index, field name for i, expectedBackend := range expectedService.Backends { - require.Equalf(t, s.Backends[i].Namespace(), expectedBackend.Namespace(), backendCompareMsg, serviceName, i, "namespace") + require.Equal(t, s.Backends[i].Namespace(), expectedBackend.Namespace(), backendCompareMsg, serviceName, i, "namespace") require.Equal(t, s.Backends[i].Name(), expectedBackend.Name(), backendCompareMsg, serviceName, i, "name") require.Equal(t, s.Backends[i].PortDef(), expectedBackend.PortDef(), backendCompareMsg, serviceName, i, "port") require.Equal(t, s.Backends[i].Weight(), expectedBackend.Weight(), backendCompareMsg, serviceName, i, "weight") diff --git a/internal/dataplane/translator/translate_httproute_test.go b/internal/dataplane/translator/translate_httproute_test.go index 55d0dfdee3..7b8025e1ce 100644 --- a/internal/dataplane/translator/translate_httproute_test.go +++ b/internal/dataplane/translator/translate_httproute_test.go @@ -1052,7 +1052,7 @@ func TestIngressRulesFromHTTPRoutes(t *testing.T) { Name: kong.String("request-transformer"), Config: kong.Configuration{ "append": subtranslator.TransformerPluginConfig{ - Headers: []string{"X-Test-Header-1:test-value-1"}, + Headers: []subtranslator.Header{subtranslator.NewHeader("X-Test-Header-1", "test-value-1")}, }, }, Tags: []*string{ @@ -1092,7 +1092,7 @@ func TestIngressRulesFromHTTPRoutes(t *testing.T) { Name: kong.String("request-transformer"), Config: kong.Configuration{ "append": subtranslator.TransformerPluginConfig{ - Headers: []string{"X-Test-Header-2:test-value-2"}, + Headers: []subtranslator.Header{subtranslator.NewHeader("X-Test-Header-2", "test-value-2")}, }, }, Tags: []*string{ @@ -1476,7 +1476,7 @@ func TestIngressRulesFromHTTPRoutes(t *testing.T) { Name: kong.String("request-transformer"), Config: kong.Configuration{ "append": subtranslator.TransformerPluginConfig{ - Headers: []string{"X-Test-Header-1:test-value-1"}, + Headers: []subtranslator.Header{subtranslator.NewHeader("X-Test-Header-1", "test-value-1")}, }, }, Tags: []*string{ @@ -2101,7 +2101,9 @@ func TestIngressRulesFromHTTPRoutesCombinedServicesAcrossHTTPRoutes(t *testing.T Name: kong.String("response-transformer"), Config: kong.Configuration{ "add": subtranslator.TransformerPluginConfig{ - Headers: []string{"Location: http://konghq.com/kong"}, + Headers: []subtranslator.Header{ + subtranslator.NewHeader("Location", "http://konghq.com/kong"), + }, }, }, Tags: []*string{ @@ -2153,7 +2155,9 @@ func TestIngressRulesFromHTTPRoutesCombinedServicesAcrossHTTPRoutes(t *testing.T Name: kong.String("response-transformer"), Config: kong.Configuration{ "add": subtranslator.TransformerPluginConfig{ - Headers: []string{"Location: http://kumahq.com/kuma"}, + Headers: []subtranslator.Header{ + subtranslator.NewHeader("Location", "http://kumahq.com/kuma"), + }, }, }, Tags: []*string{