-
Notifications
You must be signed in to change notification settings - Fork 468
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
apis: validate header and query param matches #1230
apis: validate header and query param matches #1230
Conversation
Adds webhook validation to ensure that no HTTP header or query param is matched more than once in a given route rule. Signed-off-by: Steve Kriss <krisss@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers, thanks for taking the time and thanks for adding the validation!
Just a couple smaller comments but I think this is just about ready to go.
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) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in eb6321c
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shaneutt, skriss The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds webhook validation to ensure that no
HTTP header or query param is matched more
than once in a given route rule. Also updates
the godoc for HTTPQueryParamMatch to be
consistent with HTTPHeaderMatch.
Which issue(s) this PR fixes:
Fixes #1225
Does this PR introduce a user-facing change?: