From 5296ff64dfde9b8536c618a2ad140826d4b2bc1c Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Wed, 26 Jul 2023 17:25:33 +0200 Subject: [PATCH] Create `validator` object to give more validation flexiability Signed-off-by: Mustafa Abdelrahman --- cmd/webhook/admission/admission.go | 4 +- dataclients/kubernetes/clusterclient.go | 4 +- .../kubernetes/definitions/routegroups.go | 275 -------------- .../definitions/routegroupvalidator.go | 335 ++++++++++++++++++ 4 files changed, 340 insertions(+), 278 deletions(-) create mode 100644 dataclients/kubernetes/definitions/routegroupvalidator.go diff --git a/cmd/webhook/admission/admission.go b/cmd/webhook/admission/admission.go index a13dfe3437..0c9ad54ff5 100644 --- a/cmd/webhook/admission/admission.go +++ b/cmd/webhook/admission/admission.go @@ -83,8 +83,8 @@ func (RouteGroupAdmitter) admit(req *admissionRequest) (*admissionResponse, erro }, }, nil } - - err = definitions.ValidateRouteGroup(&rgItem) + rgValidator := definitions.RouteGroupValidator{} + err = rgValidator.Validate(&rgItem) if err != nil { emsg := fmt.Sprintf("could not validate RouteGroup, %v", err) log.Error(emsg) diff --git a/dataclients/kubernetes/clusterclient.go b/dataclients/kubernetes/clusterclient.go index 1b16fa4a3b..bef92b49b1 100644 --- a/dataclients/kubernetes/clusterclient.go +++ b/dataclients/kubernetes/clusterclient.go @@ -358,10 +358,12 @@ func (c *clusterClient) LoadRouteGroups() ([]*definitions.RouteGroupItem, error) return nil, err } + rgValidator := definitions.RouteGroupValidator{} + rgs := make([]*definitions.RouteGroupItem, 0, len(rgl.Items)) for _, i := range rgl.Items { // Validate RouteGroup item. - if err := definitions.ValidateRouteGroup(i); err != nil { + if err := rgValidator.Validate(i); err != nil { log.Errorf("[routegroup] %v", err) continue } diff --git a/dataclients/kubernetes/definitions/routegroups.go b/dataclients/kubernetes/definitions/routegroups.go index f2a82f7a49..a0971a5225 100644 --- a/dataclients/kubernetes/definitions/routegroups.go +++ b/dataclients/kubernetes/definitions/routegroups.go @@ -1,13 +1,11 @@ package definitions import ( - "encoding/json" "fmt" "github.com/pkg/errors" "github.com/zalando/skipper/eskip" "github.com/zalando/skipper/loadbalancer" - "gopkg.in/yaml.v2" ) // adding Kubernetes specific backend types here. To be discussed. @@ -202,276 +200,3 @@ func missingEndpoints(backendName string) error { func routeGroupError(m *Metadata, err error) error { return fmt.Errorf("error in route group %s/%s: %w", namespaceString(m.Namespace), m.Name, err) } - -func uniqueStrings(s []string) []string { - u := make([]string, 0, len(s)) - m := make(map[string]bool) - for _, si := range s { - if m[si] { - continue - } - - m[si] = true - u = append(u, si) - } - - return u -} - -func backendTypeFromString(s string) (eskip.BackendType, error) { - switch s { - case "", "service": - return ServiceBackend, nil - default: - return eskip.BackendTypeFromString(s) - } -} - -func (sb *SkipperBackend) validate() error { - if sb.parseError != nil { - return sb.parseError - } - - if sb == nil || sb.Name == "" { - return errUnnamedBackend - } - - switch { - case sb.Type == eskip.NetworkBackend && sb.Address == "": - return missingAddress(sb.Name) - case sb.Type == ServiceBackend && sb.ServiceName == "": - return missingServiceName(sb.Name) - case sb.Type == ServiceBackend && - (sb.ServicePort == 0 || sb.ServicePort != int(uint16(sb.ServicePort))): - return invalidServicePort(sb.Name, sb.ServicePort) - case sb.Type == eskip.LBBackend && len(sb.Endpoints) == 0: - return missingEndpoints(sb.Name) - } - - return nil -} - -// UnmarshalJSON creates a new skipperBackend, safe to be called on nil pointer -func (sb *SkipperBackend) UnmarshalJSON(value []byte) error { - if sb == nil { - return nil - } - - var p skipperBackendParser - if err := json.Unmarshal(value, &p); err != nil { - return err - } - - var perr error - bt, err := backendTypeFromString(p.Type) - if err != nil { - // we cannot return an error here, because then the parsing of - // all route groups would fail. We'll report the error in the - // validation phase, only for the containing route group - perr = err - } - - a, err := loadbalancer.AlgorithmFromString(p.Algorithm) - if err != nil { - // we cannot return an error here, because then the parsing of - // all route groups would fail. We'll report the error in the - // validation phase, only for the containing route group - perr = err - } - - if a == loadbalancer.None { - a = loadbalancer.RoundRobin - } - - var b SkipperBackend - b.Name = p.Name - b.Type = bt - b.Address = p.Address - b.ServiceName = p.ServiceName - b.ServicePort = p.ServicePort - b.Algorithm = a - b.Endpoints = p.Endpoints - b.parseError = perr - - *sb = b - return nil -} - -func (rg *RouteGroupSpec) UniqueHosts() []string { - return uniqueStrings(rg.Hosts) -} - -func hasEmpty(s []string) bool { - for _, si := range s { - if si == "" { - return true - } - } - - return false -} - -func (brs BackendReferences) validate(backends map[string]bool) error { - if brs == nil { - return nil - } - names := make(map[string]struct{}, len(brs)) - for _, br := range brs { - if _, ok := names[br.BackendName]; ok { - return duplicateBackendReference(br.BackendName) - } - names[br.BackendName] = struct{}{} - - if err := br.validate(backends); err != nil { - return err - } - } - return nil -} - -func (br *BackendReference) validate(backends map[string]bool) error { - if br == nil || br.BackendName == "" { - return errUnnamedBackendReference - } - - if !backends[br.BackendName] { - return invalidBackendReference(br.BackendName) - } - - if br.Weight < 0 { - return invalidBackendWeight(br.BackendName, br.Weight) - } - - return nil -} - -func (r *RouteSpec) UniqueMethods() []string { - return uniqueStrings(r.Methods) -} - -// TODO: we need to pass namespace/name in all errors -func (rg *RouteGroupSpec) validate() error { - // has at least one backend: - if len(rg.Backends) == 0 { - return errRouteGroupWithoutBackend - } - - // backends valid and have unique names: - backends := make(map[string]bool) - for _, b := range rg.Backends { - if backends[b.Name] { - return backendsWithDuplicateName(b.Name) - } - - backends[b.Name] = true - if err := b.validate(); err != nil { - return invalidBackend(b.Name, err) - } - } - - hasDefault := len(rg.DefaultBackends) > 0 - if err := rg.DefaultBackends.validate(backends); err != nil { - return err - } - - if !hasDefault && len(rg.Routes) == 0 { - return errMissingBackendReference - } - - for i, r := range rg.Routes { - if err := r.validate(hasDefault, backends); err != nil { - return invalidRoute(i, err) - } - } - - return nil -} - -// TODO: we need to pass namespace/name in all errors -func (r *RouteSpec) validate(hasDefault bool, backends map[string]bool) error { - if r == nil { - return errInvalidRouteSpec - } - - if !hasDefault && len(r.Backends) == 0 { - return errMissingBackendReference - } - - if err := r.Backends.validate(backends); err != nil { - return err - } - - if r.Path != "" && r.PathSubtree != "" { - return errBothPathAndPathSubtree - } - - if hasEmpty(r.Predicates) { - return errInvalidPredicate - } - - if hasEmpty(r.Filters) { - return errInvalidFilter - } - - if hasEmpty(r.Methods) { - return errInvalidMethod - } - - return nil -} - -// TODO: we need to pass namespace/name in all errors -func (r *RouteGroupItem) validate() error { - // has metadata and name: - if r == nil || validate(r.Metadata) != nil { - return errRouteGroupWithoutName - } - - // has spec: - if r.Spec == nil { - return routeGroupError(r.Metadata, errRouteGroupWithoutSpec) - } - - if err := r.Spec.validate(); err != nil { - return routeGroupError(r.Metadata, err) - } - - return nil -} - -// ParseRouteGroupsJSON parses a json list of RouteGroups into RouteGroupList -func ParseRouteGroupsJSON(d []byte) (RouteGroupList, error) { - var rl RouteGroupList - err := json.Unmarshal(d, &rl) - return rl, err -} - -// ParseRouteGroupsYAML parses a YAML list of RouteGroups into RouteGroupList -func ParseRouteGroupsYAML(d []byte) (RouteGroupList, error) { - var rl RouteGroupList - err := yaml.Unmarshal(d, &rl) - return rl, err -} - -// ValidateRouteGroup validates a RouteGroupItem -func ValidateRouteGroup(rg *RouteGroupItem) error { - return rg.validate() -} - -// ValidateRouteGroups validates a RouteGroupList -func ValidateRouteGroups(rl *RouteGroupList) error { - var err error - // avoid the user having to repeatedly validate to discover all errors - for _, i := range rl.Items { - nerr := ValidateRouteGroup(i) - if nerr != nil { - err = errors.Wrap(err, nerr.Error()) - } - } - - if err != nil { - return err - } - - return nil -} diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go new file mode 100644 index 0000000000..3113f80f86 --- /dev/null +++ b/dataclients/kubernetes/definitions/routegroupvalidator.go @@ -0,0 +1,335 @@ +package definitions + +import ( + "encoding/json" + + "github.com/pkg/errors" + "github.com/zalando/skipper/eskip" + "github.com/zalando/skipper/filters" + "github.com/zalando/skipper/loadbalancer" + "gopkg.in/yaml.v2" +) + +func uniqueStrings(s []string) []string { + u := make([]string, 0, len(s)) + m := make(map[string]bool) + for _, si := range s { + if m[si] { + continue + } + + m[si] = true + u = append(u, si) + } + + return u +} + +func backendTypeFromString(s string) (eskip.BackendType, error) { + switch s { + case "", "service": + return ServiceBackend, nil + default: + return eskip.BackendTypeFromString(s) + } +} + +func (sb *SkipperBackend) validate() error { + if sb.parseError != nil { + return sb.parseError + } + + if sb == nil || sb.Name == "" { + return errUnnamedBackend + } + + switch { + case sb.Type == eskip.NetworkBackend && sb.Address == "": + return missingAddress(sb.Name) + case sb.Type == ServiceBackend && sb.ServiceName == "": + return missingServiceName(sb.Name) + case sb.Type == ServiceBackend && + (sb.ServicePort == 0 || sb.ServicePort != int(uint16(sb.ServicePort))): + return invalidServicePort(sb.Name, sb.ServicePort) + case sb.Type == eskip.LBBackend && len(sb.Endpoints) == 0: + return missingEndpoints(sb.Name) + } + + return nil +} + +// UnmarshalJSON creates a new skipperBackend, safe to be called on nil pointer +func (sb *SkipperBackend) UnmarshalJSON(value []byte) error { + if sb == nil { + return nil + } + + var p skipperBackendParser + if err := json.Unmarshal(value, &p); err != nil { + return err + } + + var perr error + bt, err := backendTypeFromString(p.Type) + if err != nil { + // we cannot return an error here, because then the parsing of + // all route groups would fail. We'll report the error in the + // validation phase, only for the containing route group + perr = err + } + + a, err := loadbalancer.AlgorithmFromString(p.Algorithm) + if err != nil { + // we cannot return an error here, because then the parsing of + // all route groups would fail. We'll report the error in the + // validation phase, only for the containing route group + perr = err + } + + if a == loadbalancer.None { + a = loadbalancer.RoundRobin + } + + var b SkipperBackend + b.Name = p.Name + b.Type = bt + b.Address = p.Address + b.ServiceName = p.ServiceName + b.ServicePort = p.ServicePort + b.Algorithm = a + b.Endpoints = p.Endpoints + b.parseError = perr + + *sb = b + return nil +} + +func (rg *RouteGroupSpec) UniqueHosts() []string { + return uniqueStrings(rg.Hosts) +} + +func hasEmpty(s []string) bool { + for _, si := range s { + if si == "" { + return true + } + } + + return false +} + +func (brs BackendReferences) validate(backends map[string]bool) error { + if brs == nil { + return nil + } + names := make(map[string]struct{}, len(brs)) + for _, br := range brs { + if _, ok := names[br.BackendName]; ok { + return duplicateBackendReference(br.BackendName) + } + names[br.BackendName] = struct{}{} + + if err := br.validate(backends); err != nil { + return err + } + } + return nil +} + +func (br *BackendReference) validate(backends map[string]bool) error { + if br == nil || br.BackendName == "" { + return errUnnamedBackendReference + } + + if !backends[br.BackendName] { + return invalidBackendReference(br.BackendName) + } + + if br.Weight < 0 { + return invalidBackendWeight(br.BackendName, br.Weight) + } + + return nil +} + +func (r *RouteSpec) UniqueMethods() []string { + return uniqueStrings(r.Methods) +} + +// TODO: we need to pass namespace/name in all errors +func (rg *RouteGroupSpec) validate() error { + // has at least one backend: + if len(rg.Backends) == 0 { + return errRouteGroupWithoutBackend + } + + // backends valid and have unique names: + backends := make(map[string]bool) + for _, b := range rg.Backends { + if backends[b.Name] { + return backendsWithDuplicateName(b.Name) + } + + backends[b.Name] = true + if err := b.validate(); err != nil { + return invalidBackend(b.Name, err) + } + } + + hasDefault := len(rg.DefaultBackends) > 0 + if err := rg.DefaultBackends.validate(backends); err != nil { + return err + } + + if !hasDefault && len(rg.Routes) == 0 { + return errMissingBackendReference + } + + for i, r := range rg.Routes { + if err := r.validate(hasDefault, backends); err != nil { + return invalidRoute(i, err) + } + } + + return nil +} + +// TODO: we need to pass namespace/name in all errors +func (r *RouteSpec) validate(hasDefault bool, backends map[string]bool) error { + if r == nil { + return errInvalidRouteSpec + } + + if !hasDefault && len(r.Backends) == 0 { + return errMissingBackendReference + } + + if err := r.Backends.validate(backends); err != nil { + return err + } + + if r.Path != "" && r.PathSubtree != "" { + return errBothPathAndPathSubtree + } + + if hasEmpty(r.Predicates) { + return errInvalidPredicate + } + + if hasEmpty(r.Filters) { + return errInvalidFilter + } + + if hasEmpty(r.Methods) { + return errInvalidMethod + } + + return nil +} + +// TODO: we need to pass namespace/name in all errors +func (r *RouteGroupItem) validate() error { + // has metadata and name: + if r == nil || validate(r.Metadata) != nil { + return errRouteGroupWithoutName + } + + // has spec: + if r.Spec == nil { + return routeGroupError(r.Metadata, errRouteGroupWithoutSpec) + } + + if err := r.Spec.validate(); err != nil { + return routeGroupError(r.Metadata, err) + } + + return nil +} + +// ParseRouteGroupsJSON parses a json list of RouteGroups into RouteGroupList +func ParseRouteGroupsJSON(d []byte) (RouteGroupList, error) { + var rl RouteGroupList + err := json.Unmarshal(d, &rl) + return rl, err +} + +// ParseRouteGroupsYAML parses a YAML list of RouteGroups into RouteGroupList +func ParseRouteGroupsYAML(d []byte) (RouteGroupList, error) { + var rl RouteGroupList + err := yaml.Unmarshal(d, &rl) + return rl, err +} + +type RouteGroupValidator struct { + FilterRegistry filters.Registry +} + +func (rgv *RouteGroupValidator) Validate(item *RouteGroupItem) error { + err := validateRouteGroup(item) + + if rgv.FilterRegistry != nil { + nerr := validateRouteGroupFilters(item, rgv.FilterRegistry) + if nerr != nil { + err = errors.Wrap(err, nerr.Error()) + } + } + + return err +} + +// validateRouteGroup validates a RouteGroupItem +func validateRouteGroup(rg *RouteGroupItem) error { + return rg.validate() +} + +func validateRouteGroupFilters(rg *RouteGroupItem, fr filters.Registry) error { + // basic for now + for _, r := range rg.Spec.Routes { + for _, f := range r.Filters { + parsedFilter, err := eskip.ParseFilters(f) + if err != nil { + return err + } + if _, ok := fr[parsedFilter[0].Name]; !ok { + return errors.Errorf("filter %s not found", parsedFilter[0].Name) + } + } + } + + return nil +} + +func (rglv *RouteGroupValidator) ValidateList(rl *RouteGroupList) error { + err := validateRouteGroups(rl) + if err != nil { + return err + } + if rglv.FilterRegistry != nil { + for _, rg := range rl.Items { + nerr := validateRouteGroupFilters(rg, rglv.FilterRegistry) + if nerr != nil { + err = errors.Wrap(err, nerr.Error()) + } + } + } + + return nil +} + +// validateRouteGroups validates a RouteGroupList +func validateRouteGroups(rl *RouteGroupList) error { + var err error + // avoid the user having to repeatedly validate to discover all errors + for _, i := range rl.Items { + nerr := validateRouteGroup(i) + if nerr != nil { + err = errors.Wrap(err, nerr.Error()) + } + } + + if err != nil { + return err + } + + return nil +}