httproute: add duplicate filter validation #606
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
the
approved
hbagdi
force-pushed
the
feat/filter-validation
branch
from
5050c35 to
d3a5b86
Compare
|
/cc @danehans @cmluciano |
hbagdi
force-pushed
the
feat/filter-validation
branch
from
d3a5b86 to
31afff7
Compare
hbagdi
force-pushed
the
feat/filter-validation
branch
from
31afff7 to
4de2daf
Compare
| for _, filter := range rule.Filters { | ||
| counts[filter.Type]++ | ||
| } | ||
| // customer filters don't have any validation |
| }, | ||
| Filters: []gatewayv1a1.HTTPRouteFilter{ | ||
| { | ||
| Type: "RequestMirror", |
There was a problem hiding this comment.
nit: Maybe use the const type here and for all the other filter types?: HTTPRouteFilterRequestMirror
hbagdi
force-pushed
the
feat/filter-validation
branch
from
4de2daf to
0d49bd9
Compare
|
@stevesloka I've addressed your comments in the latest commit. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbagdi, stevesloka 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 |
| return validateHTTPRouteSpec(&route.Spec, field.NewPath("spec")) | ||
| } | ||
|
|
||
| // validateHTTPRouteSpec validates whether required fields of spec are set according to the |
There was a problem hiding this comment.
| // validateHTTPRouteSpec validates whether required fields of spec are set according to the | |
| // validateHTTPRouteSpec validates that required fields of spec are set according to the |
| @@ -75,3 +75,40 @@ func validateListenerHostname(listeners []gatewayv1a1.Listener, path *field.Path | |||
| } | |||
| return errs | |||
| } | |||
|
|
|||
| // ValidateHTTPRoute validates route according to the Gateway API specification. | |||
There was a problem hiding this comment.
| // ValidateHTTPRoute validates route according to the Gateway API specification. | |
| // ValidateHTTPRoute validates HTTPRoute according to the Gateway API specification. |
| counts[filter.Type]++ | ||
| } | ||
| // custom filters don't have any validation | ||
| counts[gatewayv1a1.HTTPRouteFilterExtensionRef] = 0 |
There was a problem hiding this comment.
I think it might be better to have a specific list of filters that can or can't be repeated. Maybe declare a variable at the top of this file that lists the kinds of filters that can be repeated? Something similar to the upstream networking validation.
| func stringPtr(s string) *string { | ||
| return &s | ||
| } | ||
|
|
||
| func int32Ptr(i int) *int32 { | ||
| result := int32(i) | ||
| return &result | ||
| } |
There was a problem hiding this comment.
Not sure if we want to add a dependency here, but this is largely similar to https://github.com/kubernetes/utils/tree/master/pointer.
|
|
||
| // ValidateHTTPRoute validates route according to the Gateway API specification. | ||
| // For additional details of the HTTPRoute spec, refer to: | ||
| // https://gateway-api.sigs.k8s.io/spec/#networking.x-k8s.io/v1alpha1.HTTPRoute |
There was a problem hiding this comment.
| // https://gateway-api.sigs.k8s.io/spec/#networking.x-k8s.io/v1alpha1.HTTPRoute | |
| // https://gateway-api.sigs.k8s.io/spec/#networking.x-k8s.io/v1alpha1.HTTPRoute |
A variation of this patch first appeared in kubernetes-sigs#506. This patch is derived from kubernetes-sigs#506 and has been reworked to include it in the validation package. Co-authored-by: Christopher M. Luciano <cmluciano@isovalent.com>
hbagdi
force-pushed
the
feat/filter-validation
branch
from
0d49bd9 to
977527f
Compare
|
@robscott I've address all of your comments. PTAL. |
|
Thanks! /lgtm |
k8s-ci-robot
added
the
lgtm
A variation of this patch first appeared in #506.
This patch is derived from #506 and has been reworked to include it in
the validation package.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Introduces validation for HTTPRoute resource.
This is part of a patch series to get #506 merged in incrementally.
Which issue(s) this PR fixes:
None
Does this PR introduce a user-facing change?: