Add NKG-specific field validation for HTTPRoutes (#455)

* Add NKG-specific field validation for HTTPRoutes

- Introduce HTTPFieldsValidator interface for validating fields of
HTTP-related Gateway API resources according to the data-plane specific
rules.
- Validate HTTPRoute resources when building the graph using data-plane
agnostic rules.
- Validate HTTPRoute resources when building the graph using
HTTPFieldsValidator according to the data-plane rules.
- Implement an HTTPFieldsValidator for NGINX-specific validation rules.

Fixes #412

* Apply suggestions on GitHub
Co-authored-by: Kate Osborn <[email protected]>

internal/helpers/helpers.go

@@ -9,8 +9,8 @@ import (
 // Diff prints the diff between two structs.
 // It is useful in testing to compare two structs when they are large. In such a case, without Diff it will be difficult
 // to pinpoint the difference between the two structs.
-func Diff(x, y interface{}) string {
-	r := cmp.Diff(x, y)
+func Diff(want, got any) string {
+	r := cmp.Diff(want, got)
 
 	if r != "" {
 		return "(-want +got)\n" + r
@@ -57,3 +57,8 @@ func GetTLSModePointer(t v1beta1.TLSModeType) *v1beta1.TLSModeType {
 func GetBoolPointer(b bool) *bool {
 	return &b
 }
+
+// GetPointer takes a value of any type and returns a pointer to it.
+func GetPointer[T any](v T) *T {
+	return &v
+}

internal/manager/manager.go

@@ -14,20 +14,22 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	k8spredicate "sigs.k8s.io/controller-runtime/pkg/predicate"
 	gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
-	"sigs.k8s.io/gateway-api/apis/v1beta1/validation"
+	gwapivalidation "sigs.k8s.io/gateway-api/apis/v1beta1/validation"
 
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/config"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/events"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/filter"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/index"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/predicate"
 	ngxcfg "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config"
+	ngxvalidation "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config/validation"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file"
 	ngxruntime "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/runtime"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/resolver"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets"
+	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/status"
 )
 
@@ -80,13 +82,13 @@ func Start(cfg config.Config) error {
 		{
 			objectType: &gatewayv1beta1.Gateway{},
 			options: []controllerOption{
-				withWebhookValidator(createValidator(validation.ValidateGateway)),
+				withWebhookValidator(createValidator(gwapivalidation.ValidateGateway)),
 			},
 		},
 		{
 			objectType: &gatewayv1beta1.HTTPRoute{},
 			options: []controllerOption{
-				withWebhookValidator(createValidator(validation.ValidateHTTPRoute)),
+				withWebhookValidator(createValidator(gwapivalidation.ValidateHTTPRoute)),
 			},
 		},
 		{
@@ -129,6 +131,9 @@ func Start(cfg config.Config) error {
 		ServiceResolver:      resolver.NewServiceResolverImpl(mgr.GetClient()),
 		RelationshipCapturer: relationship.NewCapturerImpl(),
 		Logger:               cfg.Logger.WithName("changeProcessor"),
+		Validators: validation.Validators{
+			HTTPFieldsValidator: ngxvalidation.HTTPValidator{},
+		},
 	})
 
 	configGenerator := ngxcfg.NewGeneratorImpl()

internal/nginx/config/generator.go

@@ -24,6 +24,10 @@ func NewGeneratorImpl() GeneratorImpl {
 // executeFunc is a function that generates NGINX configuration from internal representation.
 type executeFunc func(configuration dataplane.Configuration) []byte
 
+// Generate generates NGINX configuration from internal representation.
+// It is the responsibility of the caller to validate the configuration before calling this function.
+// In case of invalid configuration, NGINX will fail to reload or could be configured with malicious configuration.
+// To validate, use the validators from the validation package.
 func (g GeneratorImpl) Generate(conf dataplane.Configuration) []byte {
 	var generated []byte
 	for _, execute := range getExecuteFuncs() {

internal/nginx/config/http/config.go

@@ -20,7 +20,7 @@ type Location struct {
 
 // Return represents an HTTP return.
 type Return struct {
-	URL  string
+	Body string
 	Code StatusCode
 }
 
@@ -38,6 +38,8 @@ const (
 	StatusFound StatusCode = 302
 	// StatusNotFound is the HTTP 404 status code.
 	StatusNotFound StatusCode = 404
+	// StatusInternalServerError is the HTTP 500 status code.
+	StatusInternalServerError StatusCode = 500
 )
 
 // Upstream holds all configuration for an HTTP upstream.

internal/nginx/config/servers.go

@@ -14,7 +14,11 @@ import (
 
 var serversTemplate = gotemplate.Must(gotemplate.New("servers").Parse(serversTemplateText))
 
-const rootPath = "/"
+const (
+	// HeaderMatchSeparator is the separator for constructing header-based match for NJS.
+	HeaderMatchSeparator = ":"
+	rootPath             = "/"
+)
 
 func executeServers(conf dataplane.Configuration) []byte {
 	servers := createServers(conf.HTTPServers, conf.SSLServers)
@@ -106,16 +110,19 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int) []http.Lo
 				matches = append(matches, createHTTPMatch(m, path))
 			}
 
-			// FIXME(pleshakov): There could be a case when the filter has the type set but not the corresponding field.
+			if r.Filters.InvalidFilter != nil {
+				loc.Return = &http.Return{Code: http.StatusInternalServerError}
+				locs = append(locs, loc)
+				continue
+			}
+
+			// There could be a case when the filter has the type set but not the corresponding field.
 			// For example, type is v1beta1.HTTPRouteFilterRequestRedirect, but RequestRedirect field is nil.
-			// The validation webhook catches that.
-			// If it doesn't work as expected, such situation is silently handled below in findFirstFilters.
-			// Consider reporting an error. But that should be done in a separate validation layer.
+			// The imported Webhook validation webhook catches that.
 
 			// RequestRedirect and proxying are mutually exclusive.
 			if r.Filters.RequestRedirect != nil {
 				loc.Return = createReturnValForRedirectFilter(r.Filters.RequestRedirect, listenerPort)
-
 				locs = append(locs, loc)
 				continue
 			}
@@ -172,9 +179,6 @@ func createReturnValForRedirectFilter(filter *v1beta1.HTTPRequestRedirectFilter,
 		hostname = string(*filter.Hostname)
 	}
 
-	// FIXME(pleshakov): Unknown values here must result in the implementation setting the Attached Condition for
-	// the Route to  `status: False`, with a Reason of `UnsupportedValue`. In that case, all routes of the Route will be
-	// ignored. NGINX will return 500. This should be implemented in the validation layer.
 	code := http.StatusFound
 	if filter.StatusCode != nil {
 		code = http.StatusCode(*filter.StatusCode)
@@ -185,15 +189,14 @@ func createReturnValForRedirectFilter(filter *v1beta1.HTTPRequestRedirectFilter,
 		port = int(*filter.Port)
 	}
 
-	// FIXME(pleshakov): Same as the FIXME about StatusCode above.
 	scheme := "$scheme"
 	if filter.Scheme != nil {
 		scheme = *filter.Scheme
 	}
 
 	return &http.Return{
 		Code: code,
-		URL:  fmt.Sprintf("%s://%s:%d$request_uri", scheme, hostname, port),
+		Body: fmt.Sprintf("%s://%s:%d$request_uri", scheme, hostname, port),
 	}
 }
 
@@ -275,7 +278,7 @@ func createQueryParamKeyValString(p v1beta1.HTTPQueryParamMatch) string {
 // We preserve the case of the name here because NGINX allows us to look up the header names in a case-insensitive
 // manner.
 func createHeaderKeyValString(h v1beta1.HTTPHeaderMatch) string {
-	return string(h.Name) + ":" + h.Value
+	return string(h.Name) + HeaderMatchSeparator + h.Value
 }
 
 func isPathOnlyMatch(match v1beta1.HTTPRouteMatch) bool {

internal/nginx/config/servers_template.go

@@ -36,7 +36,7 @@ server {
 		{{ end }}
 
 		{{ if $l.Return }}
-		return {{ $l.Return.Code }} {{ $l.Return.URL }};
+		return {{ $l.Return.Code }} "{{ $l.Return.Body }}";
 		{{ end }}
 
 		{{ if $l.HTTPMatchVar }}

internal/nginx/config/servers_test.go

@@ -271,6 +271,16 @@ func TestCreateServers(t *testing.T) {
 					},
 					// redirect is set in the corresponding state.MatchRule
 				},
+				{
+					// A match with an invalid filter
+					Matches: []v1beta1.HTTPRouteMatch{
+						{
+							Path: &v1beta1.HTTPPathMatch{
+								Value: helpers.GetPointer("/invalid-filter"),
+							},
+						},
+					},
+				},
 			},
 		},
 	}
@@ -324,6 +334,8 @@ func TestCreateServers(t *testing.T) {
 
 	filterGroup2 := graph.BackendGroup{Source: hrNsName, RuleIdx: 4}
 
+	invalidFilterGroup := graph.BackendGroup{Source: hrNsName, RuleIdx: 5}
+
 	cafePathRules := []dataplane.PathRule{
 		{
 			Path: "/",
@@ -403,6 +415,20 @@ func TestCreateServers(t *testing.T) {
 				},
 			},
 		},
+		{
+			Path: "/invalid-filter",
+			MatchRules: []dataplane.MatchRule{
+				{
+					MatchIdx: 0,
+					RuleIdx:  5,
+					Source:   hr,
+					Filters: dataplane.Filters{
+						InvalidFilter: &dataplane.InvalidFilter{},
+					},
+					BackendGroup: invalidFilterGroup,
+				},
+			},
+		},
 	}
 
 	httpServers := []dataplane.VirtualServer{
@@ -491,14 +517,20 @@ func TestCreateServers(t *testing.T) {
 				Path: "/redirect-implicit-port",
 				Return: &http.Return{
 					Code: 302,
-					URL:  fmt.Sprintf("$scheme://foo.example.com:%d$request_uri", port),
+					Body: fmt.Sprintf("$scheme://foo.example.com:%d$request_uri", port),
 				},
 			},
 			{
 				Path: "/redirect-explicit-port",
 				Return: &http.Return{
 					Code: 302,
-					URL:  "$scheme://bar.example.com:8080$request_uri",
+					Body: "$scheme://bar.example.com:8080$request_uri",
+				},
+			},
+			{
+				Path: "/invalid-filter",
+				Return: &http.Return{
+					Code: http.StatusInternalServerError,
 				},
 			},
 		}
@@ -522,11 +554,10 @@ func TestCreateServers(t *testing.T) {
 		},
 	}
 
-	result := createServers(httpServers, sslServers)
+	g := NewGomegaWithT(t)
 
-	if diff := cmp.Diff(expectedServers, result); diff != "" {
-		t.Errorf("createServers() mismatch (-want +got):\n%s", diff)
-	}
+	result := createServers(httpServers, sslServers)
+	g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty())
 }
 
 func TestCreateLocationsRootPath(t *testing.T) {
@@ -713,7 +744,7 @@ func TestCreateReturnValForRedirectFilter(t *testing.T) {
 			filter: &v1beta1.HTTPRequestRedirectFilter{},
 			expected: &http.Return{
 				Code: http.StatusFound,
-				URL:  "$scheme://$host:123$request_uri",
+				Body: "$scheme://$host:123$request_uri",
 			},
 			msg: "all fields are empty",
 		},
@@ -726,7 +757,7 @@ func TestCreateReturnValForRedirectFilter(t *testing.T) {
 			},
 			expected: &http.Return{
 				Code: 101,
-				URL:  "https://foo.example.com:2022$request_uri",
+				Body: "https://foo.example.com:2022$request_uri",
 			},
 			msg: "all fields are set",
 		},

internal/nginx/config/validation/common.go

@@ -0,0 +1,48 @@
+package validation
+
+import (
+	"errors"
+	"regexp"
+
+	k8svalidation "k8s.io/apimachinery/pkg/util/validation"
+)
+
+const (
+	escapedStringsFmt    = `([^"\\]|\\.)*`
+	escapedStringsErrMsg = `must have all '"' (double quotes) escaped and must not end with an unescaped '\' ` +
+		`(backslash)`
+)
+
+var escapedStringsFmtRegexp = regexp.MustCompile("^" + escapedStringsFmt + "$")
+
+// validateEscapedString is used to validate a string that is surrounded by " in the NGINX config for a directive
+// that doesn't support any regex rules or variables (it doesn't try to expand the variable name behind $).
+// For example, server_name "hello $not_a_var world"
+// If the value is invalid, the function returns an error that includes the specified examples of valid values.
+func validateEscapedString(value string, examples []string) error {
+	if !escapedStringsFmtRegexp.MatchString(value) {
+		msg := k8svalidation.RegexError(escapedStringsErrMsg, escapedStringsFmt, examples...)
+		return errors.New(msg)
+	}
+	return nil
+}
+
+const (
+	escapedStringsNoVarExpansionFmt           = `([^"$\\]|\\[^$])*`
+	escapedStringsNoVarExpansionErrMsg string = `a valid header must have all '"' escaped and must not contain any ` +
+		`'$' or end with an unescaped '\'`
+)
+
+var escapedStringsNoVarExpansionFmtRegexp = regexp.MustCompile("^" + escapedStringsNoVarExpansionFmt + "$")
+
+// validateEscapedStringNoVarExpansion is the same as validateEscapedString except it doesn't allow $ to
+// prevent variable expansion.
+// If the value is invalid, the function returns an error that includes the specified examples of valid values.
+func validateEscapedStringNoVarExpansion(value string, examples []string) error {
+	if !escapedStringsNoVarExpansionFmtRegexp.MatchString(value) {
+		msg := k8svalidation.RegexError(escapedStringsNoVarExpansionErrMsg, escapedStringsNoVarExpansionFmt,
+			examples...)
+		return errors.New(msg)
+	}
+	return nil
+}

internal/nginx/config/validation/common_test.go

@@ -0,0 +1,32 @@
+package validation
+
+import (
+	"testing"
+)
+
+func TestValidateEscapedString(t *testing.T) {
+	validator := func(value string) error { return validateEscapedString(value, []string{"example"}) }
+
+	testValidValuesForSimpleValidator(t, validator,
+		`test`,
+		`test test`,
+		`\"`,
+		`\\`)
+	testInvalidValuesForSimpleValidator(t, validator,
+		`\`,
+		`test"test`)
+}
+
+func TestValidateEscapedStringNoVarExpansion(t *testing.T) {
+	validator := func(value string) error { return validateEscapedStringNoVarExpansion(value, []string{"example"}) }
+
+	testValidValuesForSimpleValidator(t, validator,
+		`test`,
+		`test test`,
+		`\"`,
+		`\\`)
+	testInvalidValuesForSimpleValidator(t, validator,
+		`\`,
+		`test"test`,
+		`$test`)
+}

internal/nginx/config/validation/doc.go

@@ -0,0 +1,15 @@
+/*
+Package validation includes validators to validate values that will propagate to the NGINX configuration.
+
+The validation rules prevent two cases:
+(1) Invalid values. Such values will cause NGINX to fail to reload the configuration.
+(2) Malicious values. Such values will cause NGINX to succeed to reload, but will configure NGINX maliciously, outside
+of the NKG capabilities. For example, configuring NGINX to serve the contents of the file system of its container.
+
+The validation rules are based on the types in the parent config package and how they are used in the NGINX
+configuration templates. Changes to those might require changing the validation rules.
+
+The rules are much looser for NGINX than for the Gateway API. However, some valid Gateway API values are not valid for
+NGINX.
+*/
+package validation