-
Notifications
You must be signed in to change notification settings - Fork 472
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
Tightening GRPCRoute validation, fixing gaps in HTTPRoute and Gateway webhook validation #1599
Conversation
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 once the existing comments are resolved, although I'm still in favor of extra protection on top of the CRD validation in the webhook for paths and methods.
a341f9c
to
2b71c68
Compare
2b71c68
to
cd3a100
Compare
Adding a hold since I want to wait for a couple reviews on this one. /hold |
ffaf774
to
34f00f8
Compare
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
@@ -54,8 +69,129 @@ func validateRuleMatches(matches []gatewayv1a2.GRPCRouteMatch, path *field.Path) | |||
var errs field.ErrorList | |||
for i, m := range matches { | |||
if m.Method != nil && m.Method.Service == nil && m.Method.Method == nil { | |||
errs = append(errs, field.Required(path.Index(i).Child("methods"), "one or both of `service` or `method` must be specified")) | |||
return errs | |||
errs = append(errs, field.Required(path.Index(i).Child("method"), "one or both of `service` or `method` must be specified")) |
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.
There are two different methods to handle return errs
in webhook validation now.
- If the condition matches, return errors immediately to avoid returning too many errors :
gateway-api/apis/v1beta1/validation/common.go
Lines 52 to 54 in df03bd5
if len(s[0]) == 0 || len(*targetSection) == 0 { errs = append(errs, field.Required(path.Child("parentRefs"), "sectionNames must be specified when more than one parentRef refers to the same parent")) return errs - Return errors at the end of the function like your code.
I think we should unify the ways to return errors to users, returning all errors or returning the first error. What do you think?
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.
If needed we can file a follow up for this since it doesn't feel directly tied to this PR. In general I think we should return as many errors as we reasonably can so users don't have take multiple iterations to fix a set of problems. I think that largely matches what we're already doing, but your specific example may be an example where it makes sense to return both errors.
errs = append(errs, field.Invalid(path, http.CanonicalHeaderKey(name), "cannot match the same header multiple times in the same rule")) | ||
} | ||
} | ||
|
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.
Please remove the blank line.
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.
I think this may just be a preference thing but I actually prefer a bit more whitespace like this. In this case, this func is copied almost verbatim from HTTPRoute and maintains the same spacing as the original function:
gateway-api/apis/v1beta1/validation/httproute.go
Lines 180 to 196 in df03bd5
func validateHTTPHeaderMatches(matches []gatewayv1b1.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 | |
} |
If you feel strongly about where we should have new lines in our functions it may be worth a follow up issue to discuss more.
Still need one more LGTM, but removing hold since I think we have consensus on the changes here. /hold cancel |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnossen, robscott, shaneutt 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 |
I had a question about the validation here |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR started with a single goal - address feedback from #1538 by filling in missing GRPCRoute webhook validation. As I started that process, I realized that we'd introduced some significant gaps between v1a2 and v1b1 validation among the resources that had graduated. This also updates that logic to share code where possible and provide a consistent experience.
Does this PR introduce a user-facing change?: