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.

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

Merged
merged 5 commits into from
Dec 15, 2022

Conversation

robscott
Copy link
Member

@robscott robscott commented Dec 14, 2022

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?:

- GRPCRoute: Regex validation for Method and Service has been tightened to match GRPC spec.
- GRPCRoute: Webhook validation of GRPCRoute has been expanded to closely match HTTPRoute validation.
- HTTPRoute and Gateway: Gaps between webhook validation for v1alpha2 and v1beta1 have been closed.
- HTTPRoute: Validating webhook now ensures that Exact and Prefix path match values can now only include valid path values per RFC-3986. (RegularExpression path matches are not affected by this change).

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Dec 14, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 14, 2022
@robscott robscott added this to the v0.6.0 milestone Dec 14, 2022
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 14, 2022
apis/v1alpha2/validation/grpcroute.go Outdated Show resolved Hide resolved
apis/v1alpha2/validation/grpcroute.go Outdated Show resolved Hide resolved
apis/v1alpha2/validation/gateway.go Show resolved Hide resolved
apis/v1alpha2/validation/grpcroute.go Outdated Show resolved Hide resolved
apis/v1alpha2/validation/grpcroute.go Outdated Show resolved Hide resolved
apis/v1alpha2/validation/grpcroute.go Outdated Show resolved Hide resolved
apis/v1alpha2/validation/grpcroute.go Outdated Show resolved Hide resolved
Copy link
Contributor

@youngnick youngnick left a 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.

@robscott
Copy link
Member Author

Adding a hold since I want to wait for a couple reviews on this one.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2022
@robscott robscott changed the title Filling in gaps in GRPCRoute validation, sharing alpha and beta validation where possible Tightening GRPCRoute validation, fixing gaps in HTTPRoute and Gateway webhook validation Dec 15, 2022
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2022
@@ -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"))
Copy link
Member

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.

  1. If the condition matches, return errors immediately to avoid returning too many errors :
    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
  2. 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?

Copy link
Member Author

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"))
}
}

Copy link
Member

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.

Copy link
Member Author

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:

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.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2022
@robscott
Copy link
Member Author

Still need one more LGTM, but removing hold since I think we have consensus on the changes here.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2022
@shaneutt shaneutt self-requested a review December 15, 2022 21:12
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit c492764 into kubernetes-sigs:main Dec 15, 2022
@jackstine
Copy link
Contributor

I had a question about the validation here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants