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

Tightening GRPCRoute validation, fixing gaps in HTTPRoute and Gateway webhook validation #1599

Merged
merged 5 commits into from
Dec 15, 2022
Merged

Tightening GRPCRoute validation, fixing gaps in HTTPRoute and Gateway webhook validation #1599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
59526d1
Tightening GRPCRoute Method and Service regex validation
robscott Dec 15, 2022
d31691b
Improving GRPCRoute webhook validation to more closely match HTTPRoute
robscott Dec 15, 2022
cd3a100
Updating Gateway and HTTPRoute validation to share code across API
robscott Dec 15, 2022
34f00f8
Cleanup from PR feedback
robscott Dec 15, 2022
22374cb
Final tweaks to GRPCRoute validation
robscott Dec 15, 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
12 changes: 10 additions & 2 deletions apis/v1alpha2/grpcroute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,18 +320,26 @@ type GRPCMethodMatch struct {
// match any service.
//
// At least one of Service and Method MUST be a non-empty string.
//
// A GRPC Service must be a valid Protobuf Type Name
// (https://protobuf.com/docs/language-spec#type-references).
//
// +optional
// +kubebuilder:validation:MaxLength=1024
// +kubebuilder:validation:Pattern=`^[^\/]*$`
// +kubebuilder:validation:Pattern=`^(?i)\.?[a-z_][a-z_0-9]*(\.[a-z_][a-z_0-9]*)*$`
Service *string `json:"service,omitempty"`

// Value of the method to match against. If left empty or omitted, will
// match all services.
//
// At least one of Service and Method MUST be a non-empty string.
//
// A GRPC Method must be a valid Protobuf Method
// (https://protobuf.com/docs/language-spec#methods).
//
// +optional
// +kubebuilder:validation:MaxLength=1024
// +kubebuilder:validation:Pattern=`^[^\/]*$`
// +kubebuilder:validation:Pattern=`^[A-Za-z_][A-Za-z_0-9]*$`
Method *string `json:"method,omitempty"`
}

Expand Down
59 changes: 2 additions & 57 deletions apis/v1alpha2/validation/gateway.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,10 @@ limitations under the License.
package validation

import (
"fmt"

"k8s.io/apimachinery/pkg/util/validation/field"

gatewayv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
gatewayv1b1 "sigs.k8s.io/gateway-api/apis/v1beta1"
gatewayvalidationv1b1 "sigs.k8s.io/gateway-api/apis/v1beta1/validation"
)

var (
// set of protocols for which we need to validate that hostname is empty
protocolsHostnameInvalid = map[gatewayv1a2.ProtocolType]struct{}{
gatewayv1b1.TCPProtocolType: {},
gatewayv1b1.UDPProtocolType: {},
}

// ValidateTLSCertificateRefs validates the certificateRefs
// must be set and not empty when tls config is set and
// TLSModeType is terminate
validateTLSCertificateRefs = gatewayvalidationv1b1.ValidateTLSCertificateRefs

// validateListenerTLSConfig validates TLS config must be set when protocol is HTTPS or TLS,
// and TLS config shall not be present when protocol is HTTP, TCP or UDP
validateListenerTLSConfig = gatewayvalidationv1b1.ValidateListenerTLSConfig
gatewayv1b1validation "sigs.k8s.io/gateway-api/apis/v1beta1/validation"
)

// ValidateGateway validates gw according to the Gateway API specification.
Expand All @@ -51,40 +31,5 @@ var (
// Validation that is not possible with CRD annotations may be added here in the future.
// See https://github.com/kubernetes-sigs/gateway-api/issues/868 for more information.
func ValidateGateway(gw *gatewayv1a2.Gateway) field.ErrorList {
return validateGatewaySpec(&gw.Spec, field.NewPath("spec"))
}

// validateGatewaySpec validates whether required fields of spec are set according to the
// Gateway API specification.
func validateGatewaySpec(spec *gatewayv1a2.GatewaySpec, path *field.Path) field.ErrorList {
var errs field.ErrorList
errs = append(errs, validateGatewayListeners(spec.Listeners, path.Child("listeners"))...)
return errs
}

// validateGatewayListeners validates whether required fields of listeners are set according
// to the Gateway API specification.
func validateGatewayListeners(listeners []gatewayv1a2.Listener, path *field.Path) field.ErrorList {
var errs field.ErrorList
errs = append(errs, validateListenerTLSConfig(listeners, path)...)
errs = append(errs, validateListenerHostname(listeners, path)...)
errs = append(errs, validateTLSCertificateRefs(listeners, path)...)
return errs
}

func isProtocolInSubset(protocol gatewayv1a2.ProtocolType, set map[gatewayv1a2.ProtocolType]struct{}) bool {
_, ok := set[protocol]
return ok
}

// validateListenerHostname validates each listener hostname
// should be empty in case protocol is TCP or UDP
func validateListenerHostname(listeners []gatewayv1a2.Listener, path *field.Path) field.ErrorList {
var errs field.ErrorList
for i, h := range listeners {
if isProtocolInSubset(h.Protocol, protocolsHostnameInvalid) && h.Hostname != nil {
errs = append(errs, field.Forbidden(path.Index(i).Child("hostname"), fmt.Sprintf("should be empty for protocol %v", h.Protocol)))
}
}
return errs
return gatewayv1b1validation.ValidateGatewaySpec(&gw.Spec, field.NewPath("spec"))
}
robscott marked this conversation as resolved.
Show resolved Hide resolved
140 changes: 138 additions & 2 deletions apis/v1alpha2/validation/grpcroute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,22 @@ limitations under the License.
package validation

import (
"net/http"
"strings"

"k8s.io/apimachinery/pkg/util/validation/field"

gatewayv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
)

var (
// repeatableGRPCRouteFilters are filter types that are allowed to be
// repeated multiple times in a rule.
repeatableGRPCRouteFilters = []gatewayv1a2.GRPCRouteFilterType{
gatewayv1a2.GRPCRouteFilterExtensionRef,
}
)

// ValidateGRPCRoute validates GRPCRoute according to the Gateway API specification.
// For additional details of the GRPCRoute spec, refer to:
// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.GRPCRoute
Expand All @@ -44,6 +55,10 @@ func validateGRPCRouteRules(rules []gatewayv1a2.GRPCRouteRule, path *field.Path)
var errs field.ErrorList
for i, rule := range rules {
errs = append(errs, validateRuleMatches(rule.Matches, path.Index(i).Child("matches"))...)
errs = append(errs, validateGRPCRouteFilters(rule.Filters, path.Index(i).Child(("filters")))...)
for j, backendRef := range rule.BackendRefs {
errs = append(errs, validateGRPCRouteFilters(backendRef.Filters, path.Child("rules").Index(i).Child("backendRefs").Index(j))...)
}
}
return errs
}
Expand All @@ -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 :

    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
  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.

}
if m.Headers != nil {
errs = append(errs, validateGRPCHeaderMatches(m.Headers, path.Index(i).Child("headers"))...)
}
}
return errs
}

// validateGRPCHeaderMatches validates that no header name is matched more than
// once (case-insensitive), and that at least one of service or method was
// provided.
func validateGRPCHeaderMatches(matches []gatewayv1a2.GRPCHeaderMatch, 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"))
}
}

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:

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.

return errs
}

// validateGRPCRouteFilterType validates that only the expected fields are
// set for the specified filter type.
func validateGRPCRouteFilterType(filter gatewayv1a2.GRPCRouteFilter, path *field.Path) field.ErrorList {
var errs field.ErrorList
if filter.ExtensionRef != nil && filter.Type != gatewayv1a2.GRPCRouteFilterExtensionRef {
errs = append(errs, field.Invalid(path, filter.ExtensionRef, "must be nil if the GRPCRouteFilter.Type is not ExtensionRef"))
}
if filter.ExtensionRef == nil && filter.Type == gatewayv1a2.GRPCRouteFilterExtensionRef {
errs = append(errs, field.Required(path, "filter.ExtensionRef must be specified for ExtensionRef GRPCRouteFilter.Type"))
}
if filter.RequestHeaderModifier != nil && filter.Type != gatewayv1a2.GRPCRouteFilterRequestHeaderModifier {
errs = append(errs, field.Invalid(path, filter.RequestHeaderModifier, "must be nil if the GRPCRouteFilter.Type is not RequestHeaderModifier"))
}
if filter.RequestHeaderModifier == nil && filter.Type == gatewayv1a2.GRPCRouteFilterRequestHeaderModifier {
errs = append(errs, field.Required(path, "filter.RequestHeaderModifier must be specified for RequestHeaderModifier GRPCRouteFilter.Type"))
}
if filter.ResponseHeaderModifier != nil && filter.Type != gatewayv1a2.GRPCRouteFilterResponseHeaderModifier {
errs = append(errs, field.Invalid(path, filter.ResponseHeaderModifier, "must be nil if the GRPCRouteFilter.Type is not ResponseHeaderModifier"))
}
if filter.ResponseHeaderModifier == nil && filter.Type == gatewayv1a2.GRPCRouteFilterResponseHeaderModifier {
errs = append(errs, field.Required(path, "filter.ResponseHeaderModifier must be specified for ResponseHeaderModifier GRPCRouteFilter.Type"))
}
if filter.RequestMirror != nil && filter.Type != gatewayv1a2.GRPCRouteFilterRequestMirror {
errs = append(errs, field.Invalid(path, filter.RequestMirror, "must be nil if the GRPCRouteFilter.Type is not RequestMirror"))
}
if filter.RequestMirror == nil && filter.Type == gatewayv1a2.GRPCRouteFilterRequestMirror {
errs = append(errs, field.Required(path, "filter.RequestMirror must be specified for RequestMirror GRPCRouteFilter.Type"))
}
return errs
}

// validateGRPCRouteFilters validates that a list of core and extended filters
// is used at most once and that the filter type matches its value
func validateGRPCRouteFilters(filters []gatewayv1a2.GRPCRouteFilter, path *field.Path) field.ErrorList {
var errs field.ErrorList
counts := map[gatewayv1a2.GRPCRouteFilterType]int{}

for i, filter := range filters {
counts[filter.Type]++
if filter.RequestHeaderModifier != nil {
errs = append(errs, validateGRPCHeaderModifier(*filter.RequestHeaderModifier, path.Index(i).Child("requestHeaderModifier"))...)
}
if filter.ResponseHeaderModifier != nil {
errs = append(errs, validateGRPCHeaderModifier(*filter.ResponseHeaderModifier, path.Index(i).Child("responseHeaderModifier"))...)
}
errs = append(errs, validateGRPCRouteFilterType(filter, path.Index(i))...)
}
// custom filters don't have any validation
for _, key := range repeatableGRPCRouteFilters {
delete(counts, key)
}

for filterType, count := range counts {
if count > 1 {
errs = append(errs, field.Invalid(path, filterType, "cannot be used multiple times in the same rule"))
}
}
return errs
}

robscott marked this conversation as resolved.
Show resolved Hide resolved
// validateGRPCHeaderModifier ensures that multiple actions cannot be set for
// the same header.
func validateGRPCHeaderModifier(filter gatewayv1a2.HTTPHeaderFilter, path *field.Path) field.ErrorList {
var errs field.ErrorList
singleAction := make(map[string]bool)
for i, action := range filter.Add {
if needsErr, ok := singleAction[strings.ToLower(string(action.Name))]; ok {
if needsErr {
errs = append(errs, field.Invalid(path.Child("add"), filter.Add[i], "cannot specify multiple actions for header"))
}
singleAction[strings.ToLower(string(action.Name))] = false
} else {
singleAction[strings.ToLower(string(action.Name))] = true
}
}
for i, action := range filter.Set {
if needsErr, ok := singleAction[strings.ToLower(string(action.Name))]; ok {
if needsErr {
errs = append(errs, field.Invalid(path.Child("set"), filter.Set[i], "cannot specify multiple actions for header"))
}
singleAction[strings.ToLower(string(action.Name))] = false
} else {
singleAction[strings.ToLower(string(action.Name))] = true
}
}
for i, name := range filter.Remove {
if needsErr, ok := singleAction[strings.ToLower(name)]; ok {
if needsErr {
errs = append(errs, field.Invalid(path.Child("remove"), filter.Remove[i], "cannot specify multiple actions for header"))
}
singleAction[strings.ToLower(name)] = false
} else {
singleAction[strings.ToLower(name)] = true
}
}
return errs
Expand Down
Loading