Skip to content
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.

Sign up for GitHub

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

Jump to bottom

apis: validate header and query param matches #1230

Merged
merged 3 commits into from
Jun 29, 2022
Merged

apis: validate header and query param matches #1230

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac39b93
apis: validate header and query param matches
skriss Jun 27, 2022
eb6321c
check specific errors in tests, fix route match error paths
skriss Jun 28, 2022
87b2f8c
make linter happy
skriss Jun 28, 2022
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
4 changes: 4 additions & 0 deletions apis/v1alpha2/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
47 changes: 47 additions & 0 deletions apis/v1alpha2/validation/httproute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package validation

import (
"fmt"
"net/http"
"strings"

"k8s.io/apimachinery/pkg/util/validation/field"
Expand Down Expand Up @@ -56,6 +57,12 @@ func validateHTTPRouteSpec(spec *gatewayv1a2.HTTPRouteSpec, path *field.Path) fi
if m.Path != nil {
errs = append(errs, validateHTTPPathMatch(m.Path, path.Child("matches").Index(j).Child("path"))...)
}
if len(m.Headers) > 0 {
errs = append(errs, validateHTTPHeaderMatches(m.Headers, path.Child("matches").Index(j).Child("headers"))...)
}
if len(m.QueryParams) > 0 {
errs = append(errs, validateHTTPQueryParamMatches(m.QueryParams, path.Child("matches").Index(j).Child("queryParams"))...)
}
}
}
errs = append(errs, validateHTTPRouteBackendServicePorts(spec.Rules, path.Child("rules"))...)
Expand Down Expand Up @@ -159,6 +166,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 {
Expand Down
122 changes: 122 additions & 0 deletions apis/v1alpha2/validation/httproute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,128 @@ func TestValidateHTTPPathMatch(t *testing.T) {
}
}

func TestValidateHTTPHeaderMatches(t *testing.T) {
tests := []struct {
name string
headerMatches []gatewayv1a2.HTTPHeaderMatch
errCount int
}{{
name: "no header matches",
headerMatches: nil,
errCount: 0,
}, {
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"},
},
errCount: 0,
}, {
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"},
},
errCount: 1,
}, {
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"},
},
errCount: 1,
}}

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(errs) != tc.errCount {
t.Errorf("got %d errors, want %d errors: %s", len(errs), tc.errCount, errs)
}
})
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a bit of a nitpick: How do you feel about verifying the error contents as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I can do that; the rest of the tests in this file are only verifying error counts so I went with that for consistency but agree it'd be a bit better to verify specific errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in eb6321c


func TestValidateHTTPQueryParamMatches(t *testing.T) {
tests := []struct {
name string
queryParamMatches []gatewayv1a2.HTTPQueryParamMatch
errCount int
}{{
name: "no query param matches",
queryParamMatches: nil,
errCount: 0,
}, {
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"},
},
errCount: 0,
}, {
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"},
},
errCount: 1,
}, {
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"},
},
errCount: 0,
}}

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(errs) != tc.errCount {
t.Errorf("got %d errors, want %d errors: %s", len(errs), tc.errCount, errs)
}
})
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the above, any thoughts on verifying the error contents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in eb6321c


func TestValidateServicePort(t *testing.T) {
portPtr := func(n int) *gatewayv1a2.PortNumber {
p := gatewayv1a2.PortNumber(n)
Expand Down
4 changes: 4 additions & 0 deletions apis/v1beta1/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
47 changes: 47 additions & 0 deletions apis/v1beta1/validation/httproute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package validation

import (
"fmt"
"net/http"
"strings"

"k8s.io/apimachinery/pkg/util/validation/field"
Expand Down Expand Up @@ -56,6 +57,12 @@ func validateHTTPRouteSpec(spec *gatewayv1a2.HTTPRouteSpec, path *field.Path) fi
if m.Path != nil {
errs = append(errs, validateHTTPPathMatch(m.Path, path.Child("matches").Index(j).Child("path"))...)
}
if len(m.Headers) > 0 {
errs = append(errs, validateHTTPHeaderMatches(m.Headers, path.Child("matches").Index(j).Child("headers"))...)
}
if len(m.QueryParams) > 0 {
errs = append(errs, validateHTTPQueryParamMatches(m.QueryParams, path.Child("matches").Index(j).Child("queryParams"))...)
}
}
}
errs = append(errs, validateHTTPRouteBackendServicePorts(spec.Rules, path.Child("rules"))...)
Expand Down Expand Up @@ -159,6 +166,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 {
Expand Down
Loading