Skip to content

Add support for ResponseHeaderModifier for HTTPRouteRule objects #1494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
333df3c
Add support for ResponseHeaderModifier for HTTPRouteRule objects
kevin85421 Jan 21, 2024
5cf3099
refactor validateFilterHeaderModifier
kevin85421 Jan 22, 2024
9d8188d
Rename ValidateRequestHeader to ValidateFilterHeader
kevin85421 Jan 22, 2024
0241709
Rename AddHeaders to AddHeaderDirectives
kevin85421 Jan 22, 2024
e159b03
Update servers_template.go
kevin85421 Jan 22, 2024
3d7e6bf
Pass conformance tests
kevin85421 Feb 4, 2024
649d90e
Remove Map
kevin85421 Feb 4, 2024
4274622
Remove fmt.Printf
kevin85421 Feb 4, 2024
4bbc00f
Change TODO to FIXME
kevin85421 Feb 4, 2024
9fd5050
Address comments
kevin85421 Feb 19, 2024
95e6039
Address comments
kevin85421 Feb 19, 2024
629ee12
Address comments
kevin85421 Feb 19, 2024
bcfa113
Address comments
kevin85421 Feb 19, 2024
d8b35c0
Address comments
kevin85421 Feb 19, 2024
d316a41
Address comments
kevin85421 Feb 19, 2024
9bf3447
Address comments
kevin85421 Feb 20, 2024
5fed90d
Merge remote-tracking branch 'upstream/main' into ResponseHeaderModifier
kevin85421 Feb 20, 2024
33e185b
Add HTTPRouteResponseHeaderModification to the conformance tests
kevin85421 Feb 20, 2024
c0f98ab
Add support for ResponseHeaderModifier for HTTPRouteRule objects
kevin85421 Feb 20, 2024
169aa1a
Merge remote-tracking branch 'upstream/main' into ResponseHeaderModifier
kevin85421 Feb 21, 2024
eef6224
Add support for ResponseHeaderModifier for HTTPRouteRule objects
kevin85421 Feb 25, 2024
a3eaa90
Add support for ResponseHeaderModifier for HTTPRouteRule objects
kevin85421 Feb 25, 2024
ddfd470
Merge remote-tracking branch 'upstream/main' into ResponseHeaderModifier
kevin85421 Feb 25, 2024
2213a64
Add support for ResponseHeaderModifier for HTTPRouteRule objects
kevin85421 Feb 25, 2024
12123fb
update
kevin85421 Mar 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion conformance/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ NGINX_PREFIX = $(PREFIX)/nginx
NGINX_PLUS_PREFIX ?= $(PREFIX)/nginx-plus
GW_API_VERSION ?= 1.0.0
GATEWAY_CLASS = nginx
SUPPORTED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080
SUPPORTED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification
KIND_IMAGE ?= $(shell grep -m1 'FROM kindest/node' <tests/Dockerfile | awk -F'[ ]' '{print $$2}')
KIND_KUBE_CONFIG=$${HOME}/.kube/kind/config
CONFORMANCE_TAG = latest
Expand Down
8 changes: 8 additions & 0 deletions internal/mode/static/nginx/config/http/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type Location struct {
HTTPMatchVar string
Rewrites []string
ProxySetHeaders []Header
ResponseHeaders ResponseHeaders
}

// Header defines a HTTP header to be passed to the proxied server.
Expand All @@ -27,6 +28,13 @@ type Header struct {
Value string
}

// ResponseHeaders holds all response headers to be added, set, or removed.
type ResponseHeaders struct {
Add []Header
Set []Header
Remove []string
}

// Return represents an HTTP return.
type Return struct {
Body string
Expand Down
28 changes: 24 additions & 4 deletions internal/mode/static/nginx/config/servers.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,13 +247,15 @@ func updateLocationsForFilters(

rewrites := createRewritesValForRewriteFilter(filters.RequestURLRewrite, path)
proxySetHeaders := generateProxySetHeaders(&matchRule.Filters)
responseHeaders := generateResponseHeaders(&matchRule.Filters)
for i := range buildLocations {
if rewrites != nil {
if rewrites.Rewrite != "" {
buildLocations[i].Rewrites = append(buildLocations[i].Rewrites, rewrites.Rewrite)
}
}
buildLocations[i].ProxySetHeaders = proxySetHeaders
buildLocations[i].ResponseHeaders = responseHeaders
buildLocations[i].ProxySSLVerify = createProxyTLSFromBackends(matchRule.BackendGroup.Backends)
proxyPass := createProxyPass(
matchRule.BackendGroup,
Expand Down Expand Up @@ -517,11 +519,11 @@ func generateProxySetHeaders(filters *dataplane.HTTPFilters) []http.Header {
headerLen := len(headerFilter.Add) + len(headerFilter.Set) + len(headerFilter.Remove) + len(headers)
proxySetHeaders := make([]http.Header, 0, headerLen)
if len(headerFilter.Add) > 0 {
addHeaders := convertAddHeaders(headerFilter.Add)
addHeaders := createHeadersWithVarName(headerFilter.Add)
proxySetHeaders = append(proxySetHeaders, addHeaders...)
}
if len(headerFilter.Set) > 0 {
setHeaders := convertSetHeaders(headerFilter.Set)
setHeaders := createHeaders(headerFilter.Set)
proxySetHeaders = append(proxySetHeaders, setHeaders...)
}
// If the value of a header field is an empty string then this field will not be passed to a proxied server
Expand All @@ -535,7 +537,25 @@ func generateProxySetHeaders(filters *dataplane.HTTPFilters) []http.Header {
return append(proxySetHeaders, headers...)
}

func convertAddHeaders(headers []dataplane.HTTPHeader) []http.Header {
func generateResponseHeaders(filters *dataplane.HTTPFilters) http.ResponseHeaders {
if filters == nil || filters.ResponseHeaderModifiers == nil {
return http.ResponseHeaders{}
}

headerFilter := filters.ResponseHeaderModifiers
responseRemoveHeaders := make([]string, len(headerFilter.Remove))

// Make a deep copy to prevent the slice from being accidentally modified.
copy(responseRemoveHeaders, headerFilter.Remove)

return http.ResponseHeaders{
Add: createHeaders(headerFilter.Add),
Set: createHeaders(headerFilter.Set),
Remove: responseRemoveHeaders,
}
}

func createHeadersWithVarName(headers []dataplane.HTTPHeader) []http.Header {
locHeaders := make([]http.Header, 0, len(headers))
for _, h := range headers {
mapVarName := "${" + generateAddHeaderMapVariableName(h.Name) + "}"
Expand All @@ -547,7 +567,7 @@ func convertAddHeaders(headers []dataplane.HTTPHeader) []http.Header {
return locHeaders
}

func convertSetHeaders(headers []dataplane.HTTPHeader) []http.Header {
func createHeaders(headers []dataplane.HTTPHeader) []http.Header {
locHeaders := make([]http.Header, 0, len(headers))
for _, h := range headers {
locHeaders = append(locHeaders, http.Header{
Expand Down
10 changes: 10 additions & 0 deletions internal/mode/static/nginx/config/servers_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ server {
{{ range $h := $l.ProxySetHeaders }}
proxy_set_header {{ $h.Name }} "{{ $h.Value }}";
{{- end }}
{{ range $h := $l.ResponseHeaders.Add }}
add_header {{ $h.Name }} "{{ $h.Value }}" always;
{{- end }}
{{ range $h := $l.ResponseHeaders.Set }}
proxy_hide_header {{ $h.Name }};
add_header {{ $h.Name }} "{{ $h.Value }}" always;
{{- end }}
{{ range $h := $l.ResponseHeaders.Remove }}
proxy_hide_header {{ $h }};
{{- end }}
proxy_http_version 1.1;
proxy_pass {{ $l.ProxyPass }};
{{- if $l.ProxySSLVerify }}
Expand Down
26 changes: 26 additions & 0 deletions internal/mode/static/nginx/config/servers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -582,16 +582,19 @@ func TestCreateServers(t *testing.T) {
Path: "@rule0-route0",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "@rule0-route1",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "@rule0-route2",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/",
Expand All @@ -601,6 +604,7 @@ func TestCreateServers(t *testing.T) {
Path: "@rule1-route0",
ProxyPass: "http://$test__route1_rule1$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/test/",
Expand All @@ -610,16 +614,19 @@ func TestCreateServers(t *testing.T) {
Path: "/path-only/",
ProxyPass: "http://invalid-backend-ref$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "= /path-only",
ProxyPass: "http://invalid-backend-ref$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/backend-tls-policy/",
ProxyPass: "https://test_btp_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
ProxySSLVerify: &http.ProxySSLVerify{
Name: "test-btp.example.com",
TrustedCertificate: "/etc/nginx/secrets/test-btp.crt",
Expand All @@ -629,6 +636,7 @@ func TestCreateServers(t *testing.T) {
Path: "= /backend-tls-policy",
ProxyPass: "https://test_btp_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
ProxySSLVerify: &http.ProxySSLVerify{
Name: "test-btp.example.com",
TrustedCertificate: "/etc/nginx/secrets/test-btp.crt",
Expand Down Expand Up @@ -682,18 +690,21 @@ func TestCreateServers(t *testing.T) {
Rewrites: []string{"^ /replacement break"},
ProxyPass: "http://test_foo_80",
ProxySetHeaders: rewriteProxySetHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "= /rewrite",
Rewrites: []string{"^ /replacement break"},
ProxyPass: "http://test_foo_80",
ProxySetHeaders: rewriteProxySetHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "@rule8-route0",
Rewrites: []string{"^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"},
ProxyPass: "http://test_foo_80",
ProxySetHeaders: rewriteProxySetHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/rewrite-with-headers/",
Expand Down Expand Up @@ -733,11 +744,13 @@ func TestCreateServers(t *testing.T) {
Path: "= /exact",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "@rule12-route0",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "= /test",
Expand Down Expand Up @@ -768,6 +781,7 @@ func TestCreateServers(t *testing.T) {
Value: "$connection_upgrade",
},
},
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "= /proxy-set-headers",
Expand All @@ -794,6 +808,7 @@ func TestCreateServers(t *testing.T) {
Value: "$connection_upgrade",
},
},
ResponseHeaders: http.ResponseHeaders{},
},
}
}
Expand Down Expand Up @@ -900,11 +915,13 @@ func TestCreateServersConflicts(t *testing.T) {
Path: "/coffee/",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "= /coffee",
ProxyPass: "http://test_bar_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
createDefaultRootLocation(),
},
Expand Down Expand Up @@ -938,11 +955,13 @@ func TestCreateServersConflicts(t *testing.T) {
Path: "= /coffee",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/coffee/",
ProxyPass: "http://test_bar_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
createDefaultRootLocation(),
},
Expand Down Expand Up @@ -986,11 +1005,13 @@ func TestCreateServersConflicts(t *testing.T) {
Path: "/coffee/",
ProxyPass: "http://test_bar_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "= /coffee",
ProxyPass: "http://test_baz_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
createDefaultRootLocation(),
},
Expand Down Expand Up @@ -1095,11 +1116,13 @@ func TestCreateLocationsRootPath(t *testing.T) {
Path: "/path-1",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/path-2",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/",
Expand All @@ -1117,16 +1140,19 @@ func TestCreateLocationsRootPath(t *testing.T) {
Path: "/path-1",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/path-2",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
{
Path: "/",
ProxyPass: "http://test_foo_80$request_uri",
ProxySetHeaders: baseHeaders,
ResponseHeaders: http.ResponseHeaders{},
},
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some unit tests for the generateResponseHeaders function?

Let's also add to the createServers test to cover the case where multiple compatible filters are defined. All filters should be compatible except URLRewrite and RequestRedirect.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@ func (HTTPURLRewriteValidator) ValidateRewritePath(path string) error {
return nil
}

func (HTTPRequestHeaderValidator) ValidateRequestHeaderName(name string) error {
func (HTTPRequestHeaderValidator) ValidateFilterHeaderName(name string) error {
return validateHeaderName(name)
}

var requestHeaderValueExamples = []string{"my-header-value", "example/12345=="}

func (HTTPRequestHeaderValidator) ValidateRequestHeaderValue(value string) error {
func (HTTPRequestHeaderValidator) ValidateFilterHeaderValue(value string) error {
// Variables in header values are supported by NGINX but not required by the Gateway API.
return validateEscapedStringNoVarExpansion(value, requestHeaderValueExamples)
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,25 +88,25 @@ func TestValidateRewritePath(t *testing.T) {
)
}

func TestValidateRequestHeaderName(t *testing.T) {
func TestValidateFilterHeaderName(t *testing.T) {
validator := HTTPRequestHeaderValidator{}

testValidValuesForSimpleValidator(
t,
validator.ValidateRequestHeaderName,
validator.ValidateFilterHeaderName,
"Content-Encoding",
"MyBespokeHeader",
)

testInvalidValuesForSimpleValidator(t, validator.ValidateRequestHeaderName, "$Content-Encoding")
testInvalidValuesForSimpleValidator(t, validator.ValidateFilterHeaderName, "$Content-Encoding")
}

func TestValidateRequestHeaderValue(t *testing.T) {
func TestValidateFilterHeaderValue(t *testing.T) {
validator := HTTPRequestHeaderValidator{}

testValidValuesForSimpleValidator(
t,
validator.ValidateRequestHeaderValue,
validator.ValidateFilterHeaderValue,
"my-cookie-name",
"ssl_(server_name}",
"example/1234==",
Expand All @@ -115,7 +115,7 @@ func TestValidateRequestHeaderValue(t *testing.T) {

testInvalidValuesForSimpleValidator(
t,
validator.ValidateRequestHeaderValue,
validator.ValidateFilterHeaderValue,
"$Content-Encoding",
`"example"`,
)
Expand Down
5 changes: 5 additions & 0 deletions internal/mode/static/state/dataplane/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,11 @@ func createHTTPFilters(filters []v1.HTTPRouteFilter) HTTPFilters {
// using the first filter
result.RequestHeaderModifiers = convertHTTPHeaderFilter(f.RequestHeaderModifier)
}
case v1.HTTPRouteFilterResponseHeaderModifier:
if result.ResponseHeaderModifiers == nil {
// using the first filter
result.ResponseHeaderModifiers = convertHTTPHeaderFilter(f.ResponseHeaderModifier)
}
}
}
return result
Expand Down
2 changes: 2 additions & 0 deletions internal/mode/static/state/dataplane/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ type HTTPFilters struct {
RequestURLRewrite *HTTPURLRewriteFilter
// RequestHeaderModifiers holds the HTTPHeaderFilter.
RequestHeaderModifiers *HTTPHeaderFilter
// ResponseHeaderModifiers holds the HTTPHeaderFilter.
ResponseHeaderModifiers *HTTPHeaderFilter
}

// HTTPHeader represents an HTTP header.
Expand Down
Loading