Merge pull request #1230 from skriss/pr-dupe-match-validation

apis: validate header and query param matches
kubernetes-sigs · Jun 29, 2022 · 6f51a1a · 6f51a1a
2 parents e000a8c + 87b2f8c
commit 6f51a1a
Show file tree

Hide file tree

Showing 8 changed files with 392 additions and 6 deletions.
diff --git a/apis/v1alpha2/httproute_types.go b/apis/v1alpha2/httproute_types.go
@@ -403,6 +403,10 @@ type HTTPQueryParamMatch struct {
 	// exact string match. (See
 	// https://tools.ietf.org/html/rfc7230#section-2.7.3).
 	//
+	// If multiple entries specify equivalent query param names, only the first
+	// entry with an equivalent name MUST be considered for a match. Subsequent
+	// entries with an equivalent query param name MUST be ignored.
+	//
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=256
 	Name string `json:"name"`

diff --git a/apis/v1alpha2/validation/httproute.go b/apis/v1alpha2/validation/httproute.go
@@ -18,6 +18,7 @@ package validation
 
 import (
 	"fmt"
+	"net/http"
 	"strings"
 
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -53,8 +54,16 @@ func validateHTTPRouteSpec(spec *gatewayv1a2.HTTPRouteSpec, path *field.Path) fi
 			errs = append(errs, validateHTTPRouteFilters(backendRef.Filters, rule.Matches, path.Child("rules").Index(i).Child("backendsrefs").Index(j))...)
 		}
 		for j, m := range rule.Matches {
+			matchPath := path.Child("rules").Index(i).Child("matches").Index(j)
+
 			if m.Path != nil {
-				errs = append(errs, validateHTTPPathMatch(m.Path, path.Child("matches").Index(j).Child("path"))...)
+				errs = append(errs, validateHTTPPathMatch(m.Path, matchPath.Child("path"))...)
+			}
+			if len(m.Headers) > 0 {
+				errs = append(errs, validateHTTPHeaderMatches(m.Headers, matchPath.Child("headers"))...)
+			}
+			if len(m.QueryParams) > 0 {
+				errs = append(errs, validateHTTPQueryParamMatches(m.QueryParams, matchPath.Child("queryParams"))...)
 			}
 		}
 	}
@@ -159,6 +168,46 @@ func validateHTTPPathMatch(path *gatewayv1a2.HTTPPathMatch, fldPath *field.Path)
 	return allErrs
 }
 
+// validateHTTPHeaderMatches validates that no header name
+// is matched more than once (case-insensitive).
+func validateHTTPHeaderMatches(matches []gatewayv1a2.HTTPHeaderMatch, path *field.Path) field.ErrorList {
+	var errs field.ErrorList
+	counts := map[string]int{}
+
+	for _, match := range matches {
+		// Header names are case-insensitive.
+		counts[strings.ToLower(string(match.Name))]++
+	}
+
+	for name, count := range counts {
+		if count > 1 {
+			errs = append(errs, field.Invalid(path, http.CanonicalHeaderKey(name), "cannot match the same header multiple times in the same rule"))
+		}
+	}
+
+	return errs
+}
+
+// validateHTTPQueryParamMatches validates that no query param name
+// is matched more than once (case-sensitive).
+func validateHTTPQueryParamMatches(matches []gatewayv1a2.HTTPQueryParamMatch, path *field.Path) field.ErrorList {
+	var errs field.ErrorList
+	counts := map[string]int{}
+
+	for _, match := range matches {
+		// Query param names are case-sensitive.
+		counts[string(match.Name)]++
+	}
+
+	for name, count := range counts {
+		if count > 1 {
+			errs = append(errs, field.Invalid(path, name, "cannot match the same query parameter multiple times in the same rule"))
+		}
+	}
+
+	return errs
+}
+
 // validateHTTPRouteFilterTypeMatchesValue validates that only the expected fields are
 //// set for the specified filter type.
 func validateHTTPRouteFilterTypeMatchesValue(filter gatewayv1a2.HTTPRouteFilter, path *field.Path) field.ErrorList {

diff --git a/apis/v1alpha2/validation/httproute_test.go b/apis/v1alpha2/validation/httproute_test.go
@@ -19,6 +19,8 @@ package validation
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	utilpointer "k8s.io/utils/pointer"
 
@@ -572,6 +574,134 @@ func TestValidateHTTPPathMatch(t *testing.T) {
 	}
 }
 
+func TestValidateHTTPHeaderMatches(t *testing.T) {
+	tests := []struct {
+		name          string
+		headerMatches []gatewayv1a2.HTTPHeaderMatch
+		expectErr     string
+	}{{
+		name:          "no header matches",
+		headerMatches: nil,
+		expectErr:     "",
+	}, {
+		name: "no header matched more than once",
+		headerMatches: []gatewayv1a2.HTTPHeaderMatch{
+			{Name: "Header-Name-1", Value: "val-1"},
+			{Name: "Header-Name-2", Value: "val-2"},
+			{Name: "Header-Name-3", Value: "val-3"},
+		},
+		expectErr: "",
+	}, {
+		name: "header matched more than once (same case)",
+		headerMatches: []gatewayv1a2.HTTPHeaderMatch{
+			{Name: "Header-Name-1", Value: "val-1"},
+			{Name: "Header-Name-2", Value: "val-2"},
+			{Name: "Header-Name-1", Value: "val-3"},
+		},
+		expectErr: "spec.rules[0].matches[0].headers: Invalid value: \"Header-Name-1\": cannot match the same header multiple times in the same rule",
+	}, {
+		name: "header matched more than once (different case)",
+		headerMatches: []gatewayv1a2.HTTPHeaderMatch{
+			{Name: "Header-Name-1", Value: "val-1"},
+			{Name: "Header-Name-2", Value: "val-2"},
+			{Name: "HEADER-NAME-2", Value: "val-3"},
+		},
+		expectErr: "spec.rules[0].matches[0].headers: Invalid value: \"Header-Name-2\": cannot match the same header multiple times in the same rule",
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			route := gatewayv1a2.HTTPRoute{Spec: gatewayv1a2.HTTPRouteSpec{
+				Rules: []gatewayv1a2.HTTPRouteRule{{
+					Matches: []gatewayv1a2.HTTPRouteMatch{{
+						Headers: tc.headerMatches,
+					}},
+					BackendRefs: []gatewayv1a2.HTTPBackendRef{{
+						BackendRef: gatewayv1a2.BackendRef{
+							BackendObjectReference: gatewayv1a2.BackendObjectReference{
+								Name: gatewayv1a2.ObjectName("test"),
+								Port: utils.PortNumberPtr(8080),
+							},
+						},
+					}},
+				}},
+			}}
+
+			errs := ValidateHTTPRoute(&route)
+			if len(tc.expectErr) == 0 {
+				assert.Emptyf(t, errs, "expected no errors, got %d errors: %s", len(errs), errs)
+			} else {
+				require.Lenf(t, errs, 1, "expected one error, got %d errors: %s", len(errs), errs)
+				assert.Equal(t, tc.expectErr, errs[0].Error())
+			}
+		})
+	}
+}
+
+func TestValidateHTTPQueryParamMatches(t *testing.T) {
+	tests := []struct {
+		name              string
+		queryParamMatches []gatewayv1a2.HTTPQueryParamMatch
+		expectErr         string
+	}{{
+		name:              "no query param matches",
+		queryParamMatches: nil,
+		expectErr:         "",
+	}, {
+		name: "no query param matched more than once",
+		queryParamMatches: []gatewayv1a2.HTTPQueryParamMatch{
+			{Name: "query-param-1", Value: "val-1"},
+			{Name: "query-param-2", Value: "val-2"},
+			{Name: "query-param-3", Value: "val-3"},
+		},
+		expectErr: "",
+	}, {
+		name: "query param matched more than once",
+		queryParamMatches: []gatewayv1a2.HTTPQueryParamMatch{
+			{Name: "query-param-1", Value: "val-1"},
+			{Name: "query-param-2", Value: "val-2"},
+			{Name: "query-param-1", Value: "val-3"},
+		},
+		expectErr: "spec.rules[0].matches[0].queryParams: Invalid value: \"query-param-1\": cannot match the same query parameter multiple times in the same rule",
+	}, {
+		name: "query param names with different casing are not considered duplicates",
+		queryParamMatches: []gatewayv1a2.HTTPQueryParamMatch{
+			{Name: "query-param-1", Value: "val-1"},
+			{Name: "query-param-2", Value: "val-2"},
+			{Name: "QUERY-PARAM-1", Value: "val-3"},
+		},
+		expectErr: "",
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			route := gatewayv1a2.HTTPRoute{Spec: gatewayv1a2.HTTPRouteSpec{
+				Rules: []gatewayv1a2.HTTPRouteRule{{
+					Matches: []gatewayv1a2.HTTPRouteMatch{{
+						QueryParams: tc.queryParamMatches,
+					}},
+					BackendRefs: []gatewayv1a2.HTTPBackendRef{{
+						BackendRef: gatewayv1a2.BackendRef{
+							BackendObjectReference: gatewayv1a2.BackendObjectReference{
+								Name: gatewayv1a2.ObjectName("test"),
+								Port: utils.PortNumberPtr(8080),
+							},
+						},
+					}},
+				}},
+			}}
+
+			errs := ValidateHTTPRoute(&route)
+			if len(tc.expectErr) == 0 {
+				assert.Emptyf(t, errs, "expected no errors, got %d errors: %s", len(errs), errs)
+			} else {
+				require.Lenf(t, errs, 1, "expected one error, got %d errors: %s", len(errs), errs)
+				assert.Equal(t, tc.expectErr, errs[0].Error())
+			}
+		})
+	}
+}
+
 func TestValidateServicePort(t *testing.T) {
 	portPtr := func(n int) *gatewayv1a2.PortNumber {
 		p := gatewayv1a2.PortNumber(n)

diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go
@@ -402,6 +402,10 @@ type HTTPQueryParamMatch struct {
 	// exact string match. (See
 	// https://tools.ietf.org/html/rfc7230#section-2.7.3).
 	//
+	// If multiple entries specify equivalent query param names, only the first
+	// entry with an equivalent name MUST be considered for a match. Subsequent
+	// entries with an equivalent query param name MUST be ignored.
+	//
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=256
 	Name string `json:"name"`

diff --git a/apis/v1beta1/validation/httproute.go b/apis/v1beta1/validation/httproute.go
@@ -18,6 +18,7 @@ package validation
 
 import (
 	"fmt"
+	"net/http"
 	"strings"
 
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -53,8 +54,16 @@ func validateHTTPRouteSpec(spec *gatewayv1a2.HTTPRouteSpec, path *field.Path) fi
 			errs = append(errs, validateHTTPRouteFilters(backendRef.Filters, rule.Matches, path.Child("rules").Index(i).Child("backendsrefs").Index(j))...)
 		}
 		for j, m := range rule.Matches {
+			matchPath := path.Child("rules").Index(i).Child("matches").Index(j)
+
 			if m.Path != nil {
-				errs = append(errs, validateHTTPPathMatch(m.Path, path.Child("matches").Index(j).Child("path"))...)
+				errs = append(errs, validateHTTPPathMatch(m.Path, matchPath.Child("path"))...)
+			}
+			if len(m.Headers) > 0 {
+				errs = append(errs, validateHTTPHeaderMatches(m.Headers, matchPath.Child("headers"))...)
+			}
+			if len(m.QueryParams) > 0 {
+				errs = append(errs, validateHTTPQueryParamMatches(m.QueryParams, matchPath.Child("queryParams"))...)
 			}
 		}
 	}
@@ -159,6 +168,46 @@ func validateHTTPPathMatch(path *gatewayv1a2.HTTPPathMatch, fldPath *field.Path)
 	return allErrs
 }
 
+// validateHTTPHeaderMatches validates that no header name
+// is matched more than once (case-insensitive).
+func validateHTTPHeaderMatches(matches []gatewayv1a2.HTTPHeaderMatch, path *field.Path) field.ErrorList {
+	var errs field.ErrorList
+	counts := map[string]int{}
+
+	for _, match := range matches {
+		// Header names are case-insensitive.
+		counts[strings.ToLower(string(match.Name))]++
+	}
+
+	for name, count := range counts {
+		if count > 1 {
+			errs = append(errs, field.Invalid(path, http.CanonicalHeaderKey(name), "cannot match the same header multiple times in the same rule"))
+		}
+	}
+
+	return errs
+}
+
+// validateHTTPQueryParamMatches validates that no query param name
+// is matched more than once (case-sensitive).
+func validateHTTPQueryParamMatches(matches []gatewayv1a2.HTTPQueryParamMatch, path *field.Path) field.ErrorList {
+	var errs field.ErrorList
+	counts := map[string]int{}
+
+	for _, match := range matches {
+		// Query param names are case-sensitive.
+		counts[string(match.Name)]++
+	}
+
+	for name, count := range counts {
+		if count > 1 {
+			errs = append(errs, field.Invalid(path, name, "cannot match the same query parameter multiple times in the same rule"))
+		}
+	}
+
+	return errs
+}
+
 // validateHTTPRouteFilterTypeMatchesValue validates that only the expected fields are
 //// set for the specified filter type.
 func validateHTTPRouteFilterTypeMatchesValue(filter gatewayv1a2.HTTPRouteFilter, path *field.Path) field.ErrorList {