updates based on reviews

apis/v1alpha1/nginxproxy_types.go

@@ -54,7 +54,7 @@ type NginxProxySpec struct {
 	// +optional
 	Telemetry *Telemetry `json:"telemetry,omitempty"`
 	// RewriteClientIP defines configuration for rewriting the client IP to the original client's IP.
-	// +kubebuilder:validation:XValidation:message="if mode is set, trustedAddresses is a required field",rule="!(has(self.mode) && !has(self.trustedAddresses))"
+	// +kubebuilder:validation:XValidation:message="if mode is set, trustedAddresses is a required field",rule="!(has(self.mode) && (!has(self.trustedAddresses) || size(self.trustedAddresses) == 0))"
 	//
 	// +optional
 	//nolint:lll
@@ -132,12 +132,12 @@ type RewriteClientIP struct {
 	// +optional
 	Mode *RewriteClientIPModeType `json:"mode,omitempty"`
 
-	// SetIPRecursively configures whether recursive search is used when selecting the client's address from.
+	// SetIPRecursively configures whether recursive search is used when selecting the client's address from
 	// the X-Forwarded-For header. It is used in conjunction with TrustedAddresses.
 	// If enabled, NGINX will recurse on the values in X-Forwarded-Header from the end of array
 	// to start of array and select the first untrusted IP.
 	// For example, if X-Forwarded-For is [11.11.11.11, 22.22.22.22, 55.55.55.1],
-	// and TrustedAddresses is set to 55.55.55.1/0, NGINX will rewrite the client IP to 22.22.22.22.
+	// and TrustedAddresses is set to 55.55.55.1, NGINX will rewrite the client IP to 22.22.22.22.
 	// If disabled, NGINX will select the IP at the end of the array.
 	// In the previous example, 55.55.55.1 would be selected.
 	// Sets NGINX directive real_ip_recursive: https://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive
@@ -149,7 +149,7 @@ type RewriteClientIP struct {
 	// If a request comes from a trusted address, NGINX will rewrite the client IP information,
 	// and forward it to the backend in the X-Forwarded-For* and X-Real-IP headers.
 	// If the request does not come from a trusted address, NGINX will not rewrite the client IP information.
-	// Addresses must be provided as CIDR blocks: 10.0.0.0/32, 192.33.21/0.
+	// Addresses must be provided as CIDR blocks or IP address: 10.0.0.0, 192.33.21/24, fe80::1/128.
 	// To trust all addresses (not recommended), set to 0.0.0.0/0.
 	// If no addresses are provided, NGINX will not rewrite the client IP information.
 	// Sets NGINX directive set_real_ip_from: https://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from
@@ -179,8 +179,8 @@ const (
 	RewriteClientIPModeXForwardedFor RewriteClientIPModeType = "XForwardedFor"
 )
 
-// TrustedAddress is a string value representing a CIDR block.
-// Examples: 0.0.0.0/0
+// TrustedAddress is a string value representing a CIDR block or an IP address.
+// Examples: 10.0.0.2/32, 10.0.0.1, fe80::1/128, ::1/24.
 //
 // +kubebuilder:validation:Pattern=`^(?:(?:[0-9]{1,3}\.){3}[0-9]{1,3}(?:\/(?:[0-9]|[12][0-9]|3[0-2]))?|(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}(?:\/(?:[0-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8]))?)$`
 //

charts/nginx-gateway-fabric/values.yaml

@@ -94,8 +94,8 @@ nginx:
     # disableHTTP2: false
     # ipFamily: dual
     # rewriteClientIP:
-    #   mode: "XForwadedFor"
-    #   # -- The trusted addresses field needs to be replaced with the load balancer's IP address.
+    #   mode: "XForwardedFor"
+    #   # -- Set to the CIDR range or IP of the proxy that sits in front of NGINX Gateway Fabric.
     #   trustedAddresses: []
     #   setIPRecursively: true
     # telemetry:

config/crd/bases/gateway.nginx.org_nginxproxies.yaml

@@ -79,12 +79,12 @@ spec:
                     type: string
                   setIPRecursively:
                     description: |-
-                      SetIPRecursively configures whether recursive search is used when selecting the client's address from.
+                      SetIPRecursively configures whether recursive search is used when selecting the client's address from
                       the X-Forwarded-For header. It is used in conjunction with TrustedAddresses.
                       If enabled, NGINX will recurse on the values in X-Forwarded-Header from the end of array
                       to start of array and select the first untrusted IP.
                       For example, if X-Forwarded-For is [11.11.11.11, 22.22.22.22, 55.55.55.1],
-                      and TrustedAddresses is set to 55.55.55.1/0, NGINX will rewrite the client IP to 22.22.22.22.
+                      and TrustedAddresses is set to 55.55.55.1, NGINX will rewrite the client IP to 22.22.22.22.
                       If disabled, NGINX will select the IP at the end of the array.
                       In the previous example, 55.55.55.1 would be selected.
                       Sets NGINX directive real_ip_recursive: https://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive
@@ -95,15 +95,15 @@ spec:
                       If a request comes from a trusted address, NGINX will rewrite the client IP information,
                       and forward it to the backend in the X-Forwarded-For* and X-Real-IP headers.
                       If the request does not come from a trusted address, NGINX will not rewrite the client IP information.
-                      Addresses must be provided as CIDR blocks: 10.0.0.0/32, 192.33.21/0.
+                      Addresses must be provided as CIDR blocks or IP address: 10.0.0.0, 192.33.21/24, fe80::1/128.
                       To trust all addresses (not recommended), set to 0.0.0.0/0.
                       If no addresses are provided, NGINX will not rewrite the client IP information.
                       Sets NGINX directive set_real_ip_from: https://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from
                       This field is required if mode is set.
                     items:
                       description: |-
-                        TrustedAddress is a string value representing a CIDR block.
-                        Examples: 0.0.0.0/0
+                        TrustedAddress is a string value representing a CIDR block or an IP address.
+                        Examples: 10.0.0.2/32, 10.0.0.1, fe80::1/128, ::1/24.
                       pattern: ^(?:(?:[0-9]{1,3}\.){3}[0-9]{1,3}(?:\/(?:[0-9]|[12][0-9]|3[0-2]))?|(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}(?:\/(?:[0-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8]))?)$
                       type: string
                     maxItems: 16
@@ -112,7 +112,8 @@ spec:
                 type: object
                 x-kubernetes-validations:
                 - message: if mode is set, trustedAddresses is a required field
-                  rule: '!(has(self.mode) && !has(self.trustedAddresses))'
+                  rule: '!(has(self.mode) && (!has(self.trustedAddresses) || size(self.trustedAddresses)
+                    == 0))'
               telemetry:
                 description: Telemetry specifies the OpenTelemetry configuration.
                 properties:

deploy/crds.yaml

@@ -664,12 +664,12 @@ spec:
                     type: string
                   setIPRecursively:
                     description: |-
-                      SetIPRecursively configures whether recursive search is used when selecting the client's address from.
+                      SetIPRecursively configures whether recursive search is used when selecting the client's address from
                       the X-Forwarded-For header. It is used in conjunction with TrustedAddresses.
                       If enabled, NGINX will recurse on the values in X-Forwarded-Header from the end of array
                       to start of array and select the first untrusted IP.
                       For example, if X-Forwarded-For is [11.11.11.11, 22.22.22.22, 55.55.55.1],
-                      and TrustedAddresses is set to 55.55.55.1/0, NGINX will rewrite the client IP to 22.22.22.22.
+                      and TrustedAddresses is set to 55.55.55.1, NGINX will rewrite the client IP to 22.22.22.22.
                       If disabled, NGINX will select the IP at the end of the array.
                       In the previous example, 55.55.55.1 would be selected.
                       Sets NGINX directive real_ip_recursive: https://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive
@@ -680,15 +680,15 @@ spec:
                       If a request comes from a trusted address, NGINX will rewrite the client IP information,
                       and forward it to the backend in the X-Forwarded-For* and X-Real-IP headers.
                       If the request does not come from a trusted address, NGINX will not rewrite the client IP information.
-                      Addresses must be provided as CIDR blocks: 10.0.0.0/32, 192.33.21/0.
+                      Addresses must be provided as CIDR blocks or IP address: 10.0.0.0, 192.33.21/24, fe80::1/128.
                       To trust all addresses (not recommended), set to 0.0.0.0/0.
                       If no addresses are provided, NGINX will not rewrite the client IP information.
                       Sets NGINX directive set_real_ip_from: https://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_ip_from
                       This field is required if mode is set.
                     items:
                       description: |-
-                        TrustedAddress is a string value representing a CIDR block.
-                        Examples: 0.0.0.0/0
+                        TrustedAddress is a string value representing a CIDR block or an IP address.
+                        Examples: 10.0.0.2/32, 10.0.0.1, fe80::1/128, ::1/24.
                       pattern: ^(?:(?:[0-9]{1,3}\.){3}[0-9]{1,3}(?:\/(?:[0-9]|[12][0-9]|3[0-2]))?|(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}(?:\/(?:[0-9]|[1-9][0-9]|1[0-1][0-9]|12[0-8]))?)$
                       type: string
                     maxItems: 16
@@ -697,7 +697,8 @@ spec:
                 type: object
                 x-kubernetes-validations:
                 - message: if mode is set, trustedAddresses is a required field
-                  rule: '!(has(self.mode) && !has(self.trustedAddresses))'
+                  rule: '!(has(self.mode) && (!has(self.trustedAddresses) || size(self.trustedAddresses)
+                    == 0))'
               telemetry:
                 description: Telemetry specifies the OpenTelemetry configuration.
                 properties:

internal/mode/static/nginx/config/http/config.go

@@ -2,7 +2,10 @@ package http
 
 import "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared"
 
-const InternalRoutePathPrefix = "/_ngf-internal"
+const (
+	InternalRoutePathPrefix = "/_ngf-internal"
+	HTTPSScheme             = "https"
+)
 
 // Server holds all configuration for an HTTP server.
 type Server struct {

internal/mode/static/nginx/config/servers.go

@@ -800,14 +800,7 @@ func generateProxySetHeaders(filters *dataplane.HTTPFilters, grpc bool) []http.H
 		copy(headers, grpcBaseHeaders)
 	}
 
-	if filters != nil && filters.RequestURLRewrite != nil && filters.RequestURLRewrite.Hostname != nil {
-		for i, header := range headers {
-			if header.Name == "Host" {
-				headers[i].Value = *filters.RequestURLRewrite.Hostname
-				break
-			}
-		}
-	}
+	setHeaderForHTTPSRedirect(filters, headers)
 
 	if filters == nil || filters.RequestHeaderModifiers == nil {
 		return headers
@@ -836,6 +829,29 @@ func generateProxySetHeaders(filters *dataplane.HTTPFilters, grpc bool) []http.H
 	return append(proxySetHeaders, headers...)
 }
 
+func setHeaderForHTTPSRedirect(filters *dataplane.HTTPFilters, headers []http.Header) {
+	if filters != nil {
+		if filters.RequestURLRewrite != nil && filters.RequestURLRewrite.Hostname != nil {
+			for i, header := range headers {
+				if header.Name == "Host" {
+					headers[i].Value = *filters.RequestURLRewrite.Hostname
+					break
+				}
+			}
+		}
+		if filters.RequestRedirect != nil &&
+			filters.RequestRedirect.Scheme != nil &&
+			*filters.RequestRedirect.Scheme == http.HTTPSScheme {
+			for i, header := range headers {
+				if header.Name == "X-Forwarded-Proto" {
+					headers[i].Value = http.HTTPSScheme
+					return
+				}
+			}
+		}
+	}
+}
+
 func generateResponseHeaders(filters *dataplane.HTTPFilters) http.ResponseHeaders {
 	if filters == nil || filters.ResponseHeaderModifiers == nil {
 		return http.ResponseHeaders{}
@@ -912,7 +928,7 @@ func getRewriteClientIPSettings(rewriteIPConfig dataplane.RewriteClientIPSetting
 
 	return shared.RewriteClientIPSettings{
 		RealIPHeader:  string(rewriteIPConfig.Mode),
-		RealIPFrom:    rewriteIPConfig.TrustedCIDRs,
+		RealIPFrom:    rewriteIPConfig.TrustedAddresses,
 		Recursive:     rewriteIPConfig.IPRecursive,
 		ProxyProtocol: proxyProtocol,
 	}

internal/mode/static/nginx/config/servers_template.go

@@ -13,7 +13,7 @@ server {
     listen [::]:{{ $s.Listen }} ssl default_server{{ $.RewriteClientIP.ProxyProtocol }};
         {{- end }}
     ssl_reject_handshake on;
-		{{- range $cidr := $.RewriteClientIP.RealIPFrom }}
+        {{- range $cidr := $.RewriteClientIP.RealIPFrom }}
     set_real_ip_from {{ $cidr }};
         {{- end}}
         {{- if $.RewriteClientIP.RealIPHeader}}
@@ -77,7 +77,7 @@ server {
     include {{ $i.Name }};
         {{- end }}
 
-		{{- range $cidr := $.RewriteClientIP.RealIPFrom }}
+        {{- range $cidr := $.RewriteClientIP.RealIPFrom }}
     set_real_ip_from {{ $cidr }};
         {{- end}}
         {{- if $.RewriteClientIP.RealIPHeader}}

internal/mode/static/nginx/config/servers_test.go

@@ -321,18 +321,15 @@ func TestExecuteServers_RewriteClientIP(t *testing.T) {
 				BaseHTTPConfig: dataplane.BaseHTTPConfig{
 					IPFamily: dataplane.Dual,
 					RewriteClientIPSettings: dataplane.RewriteClientIPSettings{
-						Mode:         dataplane.RewriteIPModeProxyProtocol,
-						TrustedCIDRs: []string{"0.0.0.0/0"},
-						IPRecursive:  true,
+						Mode:             dataplane.RewriteIPModeProxyProtocol,
+						TrustedAddresses: []string{"10.56.73.51/32"},
+						IPRecursive:      false,
 					},
 				},
 			},
 			expectedHTTPConfig: map[string]int{
-				"set_real_ip_from 0.0.0.0/0;":                              4,
+				"set_real_ip_from 10.56.73.51/32;":                         4,
 				"real_ip_header proxy_protocol;":                           4,
-				"real_ip_recursive on;":                                    4,
-				"proxy_protocol on;":                                       0,
-				"set_real_ip_from unix:;":                                  0,
 				"listen 8080 default_server proxy_protocol;":               1,
 				"listen 8080 proxy_protocol;":                              1,
 				"listen 8443 ssl default_server proxy_protocol;":           1,
@@ -355,17 +352,18 @@ func TestExecuteServers_RewriteClientIP(t *testing.T) {
 				BaseHTTPConfig: dataplane.BaseHTTPConfig{
 					IPFamily: dataplane.Dual,
 					RewriteClientIPSettings: dataplane.RewriteClientIPSettings{
-						Mode:         dataplane.RewriteIPModeXForwardedFor,
-						TrustedCIDRs: []string{"0.0.0.0/0"},
-						IPRecursive:  true,
+						Mode:             dataplane.RewriteIPModeXForwardedFor,
+						TrustedAddresses: []string{"10.1.1.3/32", "2.2.2.2", "2001:db8::/32"},
+						IPRecursive:      true,
 					},
 				},
 			},
 			expectedHTTPConfig: map[string]int{
-				"set_real_ip_from 0.0.0.0/0;":                              4,
+				"set_real_ip_from 10.1.1.3/32;":                            4,
+				"set_real_ip_from 2.2.2.2;":                                4,
+				"set_real_ip_from 2001:db8::/32;":                          4,
 				"real_ip_header X-Forwarded-For;":                          4,
 				"real_ip_recursive on;":                                    4,
-				"proxy_protocol on;":                                       0,
 				"listen 8080 default_server;":                              1,
 				"listen 8080;":                                             1,
 				"listen 8443 ssl default_server;":                          1,
@@ -2682,6 +2680,48 @@ func TestGenerateProxySetHeaders(t *testing.T) {
 				},
 			},
 		},
+		{
+			msg: "request redirect filter with https scheme",
+			filters: &dataplane.HTTPFilters{
+				RequestRedirect: &dataplane.HTTPRequestRedirectFilter{
+					Scheme: helpers.GetPointer("https"),
+				},
+			},
+			expectedHeaders: []http.Header{
+				{
+					Name:  "Host",
+					Value: "$gw_api_compliant_host",
+				},
+				{
+					Name:  "X-Forwarded-For",
+					Value: "$proxy_add_x_forwarded_for",
+				},
+				{
+					Name:  "Upgrade",
+					Value: "$http_upgrade",
+				},
+				{
+					Name:  "Connection",
+					Value: "$connection_upgrade",
+				},
+				{
+					Name:  "X-Real-IP",
+					Value: "$remote_addr",
+				},
+				{
+					Name:  "X-Forwarded-Proto",
+					Value: "https",
+				},
+				{
+					Name:  "X-Forwarded-Host",
+					Value: "$host",
+				},
+				{
+					Name:  "X-Forwarded-Port",
+					Value: "$server_port",
+				},
+			},
+		},
 	}
 
 	for _, tc := range tests {

internal/mode/static/nginx/config/stream_servers.go

@@ -37,7 +37,6 @@ func createStreamServers(conf dataplane.Configuration) []stream.Server {
 	}
 
 	streamServers := make([]stream.Server, 0, len(conf.TLSPassthroughServers)*2)
-	var streamServer stream.Server
 	portSet := make(map[int32]struct{})
 	upstreams := make(map[string]dataplane.Upstream)
 
@@ -48,15 +47,15 @@ func createStreamServers(conf dataplane.Configuration) []stream.Server {
 	for _, server := range conf.TLSPassthroughServers {
 		if u, ok := upstreams[server.UpstreamName]; ok && server.UpstreamName != "" {
 			if server.Hostname != "" && len(u.Endpoints) > 0 {
-				streamServer = stream.Server{
+				streamServer := stream.Server{
 					Listen:     getSocketNameTLS(server.Port, server.Hostname),
 					StatusZone: server.Hostname,
 					ProxyPass:  server.UpstreamName,
 					IsSocket:   true,
 				}
+				// set rewriteClientIP settings as this is a socket stream server
 				streamServer.RewriteClientIP = getRewriteClientIPSettingsForStream(
 					conf.BaseHTTPConfig.RewriteClientIPSettings,
-					streamServer.IsSocket,
 				)
 				streamServers = append(streamServers, streamServer)
 			}
@@ -68,31 +67,25 @@ func createStreamServers(conf dataplane.Configuration) []stream.Server {
 
 		portSet[server.Port] = struct{}{}
 
-		streamServer = stream.Server{
+		streamServer := stream.Server{
 			Listen:     fmt.Sprint(server.Port),
 			StatusZone: server.Hostname,
 			Pass:       getTLSPassthroughVarName(server.Port),
 			SSLPreread: true,
 		}
-		streamServer.RewriteClientIP = getRewriteClientIPSettingsForStream(
-			conf.BaseHTTPConfig.RewriteClientIPSettings,
-			streamServer.IsSocket,
-		)
 		streamServers = append(streamServers, streamServer)
 	}
-
 	return streamServers
 }
 
 func getRewriteClientIPSettingsForStream(
 	rewriteConfig dataplane.RewriteClientIPSettings,
-	isSocket bool,
 ) shared.RewriteClientIPSettings {
 	proxyEnabled := rewriteConfig.Mode == dataplane.RewriteIPModeProxyProtocol
-	if isSocket && proxyEnabled {
+	if proxyEnabled {
 		return shared.RewriteClientIPSettings{
 			ProxyProtocol: shared.ProxyProtocolDirective,
-			RealIPFrom:    rewriteConfig.TrustedCIDRs,
+			RealIPFrom:    rewriteConfig.TrustedAddresses,
 		}
 	}
 

internal/mode/static/nginx/config/stream_servers_template.go

@@ -3,10 +3,8 @@ package config
 const streamServersTemplateText = `
 {{- range $s := .Servers }}
 server {
-	{{- if and ($.IPFamily.IPv4) (not $s.IsSocket) }}
-    listen {{ $s.Listen }};
-	{{- else if $s.IsSocket }}
-	listen {{ $s.Listen }}{{ $s.RewriteClientIP.ProxyProtocol }};
+	{{- if or ($.IPFamily.IPv4) ($s.IsSocket) }}
+    listen {{ $s.Listen }}{{ $s.RewriteClientIP.ProxyProtocol }};
 	{{- end }}
 	{{- if and ($.IPFamily.IPv6) (not $s.IsSocket) }}
     listen [::]:{{ $s.Listen }};