From 98ae7ded0008be721cd2733ee42b4ddf9a2a3a6a Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Wed, 11 Oct 2023 17:41:46 +0200 Subject: [PATCH] Second take after validation work done in other PRs Signed-off-by: Mustafa Abdelrahman --- dataclients/kubernetes/clusterclient.go | 2 +- .../definitions/ingressvalidator.go | 40 +++++++++++--- .../definitions/routegroupvalidator.go | 22 +++++++- dataclients/kubernetes/kube.go | 3 ++ skipper.go | 3 +- webhook/admission/admission_test.go | 52 ++++++++++++++++--- .../testdata/ingress/invalid-filter-name.json | 39 ++++++++++++++ ...valid-eskip-filters-but-not-supported.json | 48 +++++++++++++++++ webhook/main.go | 25 +++++---- 9 files changed, 202 insertions(+), 32 deletions(-) create mode 100644 webhook/admission/testdata/ingress/invalid-filter-name.json create mode 100644 webhook/admission/testdata/rg/rg-with-valid-eskip-filters-but-not-supported.json diff --git a/dataclients/kubernetes/clusterclient.go b/dataclients/kubernetes/clusterclient.go index e15103e8d6..d5b0c0eaa7 100644 --- a/dataclients/kubernetes/clusterclient.go +++ b/dataclients/kubernetes/clusterclient.go @@ -172,7 +172,7 @@ func newClusterClient(o Options, apiURL, ingCls, rgCls string, quit <-chan struc httpClient: httpClient, apiURL: apiURL, certificateRegistry: o.CertificateRegistry, - routeGroupValidator: &definitions.RouteGroupValidator{}, + routeGroupValidator: &definitions.RouteGroupValidator{FiltersRegistry: o.FiltersRegistry}, enableEndpointSlices: o.KubernetesEnableEndpointslices, } diff --git a/dataclients/kubernetes/definitions/ingressvalidator.go b/dataclients/kubernetes/definitions/ingressvalidator.go index 1a7e2499d8..dd2213d2d4 100644 --- a/dataclients/kubernetes/definitions/ingressvalidator.go +++ b/dataclients/kubernetes/definitions/ingressvalidator.go @@ -4,9 +4,12 @@ import ( "fmt" "github.com/zalando/skipper/eskip" + "github.com/zalando/skipper/filters" ) -type IngressV1Validator struct{} +type IngressV1Validator struct { + FiltersRegistry filters.Registry +} func (igv *IngressV1Validator) Validate(item *IngressV1Item) error { var errs []error @@ -19,14 +22,19 @@ func (igv *IngressV1Validator) Validate(item *IngressV1Item) error { } func (igv *IngressV1Validator) validateFilterAnnotation(annotations map[string]string) error { + var errs []error if filters, ok := annotations[IngressFilterAnnotation]; ok { - _, err := eskip.ParseFilters(filters) + parsedFilters, err := eskip.ParseFilters(filters) if err != nil { - err = fmt.Errorf("invalid \"%s\" annotation: %w", IngressFilterAnnotation, err) + errs = append(errs, fmt.Errorf("invalid \"%s\" annotation: %w", IngressFilterAnnotation, err)) + } + + if igv.FiltersRegistry != nil { + errs = append(errs, igv.validateFiltersNames(parsedFilters)) } - return err } - return nil + + return errorsJoin(errs...) } func (igv *IngressV1Validator) validatePredicateAnnotation(annotations map[string]string) error { @@ -41,12 +49,28 @@ func (igv *IngressV1Validator) validatePredicateAnnotation(annotations map[strin } func (igv *IngressV1Validator) validateRoutesAnnotation(annotations map[string]string) error { + var errs []error if routes, ok := annotations[IngressRoutesAnnotation]; ok { - _, err := eskip.Parse(routes) + parsedRoutes, err := eskip.Parse(routes) if err != nil { - err = fmt.Errorf("invalid \"%s\" annotation: %w", IngressRoutesAnnotation, err) + errs = append(errs, fmt.Errorf("invalid \"%s\" annotation: %w", IngressRoutesAnnotation, err)) + } + + if igv.FiltersRegistry != nil { + for _, r := range parsedRoutes { + errs = append(errs, igv.validateFiltersNames(r.Filters)) + } + } + } + + return errorsJoin(errs...) +} + +func (igv *IngressV1Validator) validateFiltersNames(filters []*eskip.Filter) error { + for _, f := range filters { + if _, ok := igv.FiltersRegistry[f.Name]; !ok { + return fmt.Errorf("filter \"%s\" is not supported", f.Name) } - return err } return nil } diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go index 9159ea6703..6c13468ec8 100644 --- a/dataclients/kubernetes/definitions/routegroupvalidator.go +++ b/dataclients/kubernetes/definitions/routegroupvalidator.go @@ -1,10 +1,15 @@ package definitions import ( + "fmt" + "github.com/zalando/skipper/eskip" + "github.com/zalando/skipper/filters" ) -type RouteGroupValidator struct{} +type RouteGroupValidator struct { + FiltersRegistry filters.Registry +} var defaultRouteGroupValidator = &RouteGroupValidator{} @@ -56,14 +61,27 @@ func (rgv *RouteGroupValidator) filtersValidation(item *RouteGroupItem) error { var errs []error for _, r := range item.Spec.Routes { for _, f := range r.Filters { - _, err := eskip.ParseFilters(f) + parsedFilters, err := eskip.ParseFilters(f) errs = append(errs, err) + + if rgv.FiltersRegistry != nil { + errs = append(errs, rgv.validateFiltersNames(parsedFilters)) + } } } return errorsJoin(errs...) } +func (rgv *RouteGroupValidator) validateFiltersNames(filters []*eskip.Filter) error { + for _, f := range filters { + if _, ok := rgv.FiltersRegistry[f.Name]; !ok { + return fmt.Errorf("filter \"%s\" is not supported", f.Name) + } + } + return nil +} + func (rgv *RouteGroupValidator) predicatesValidation(item *RouteGroupItem) error { var errs []error for _, r := range item.Spec.Routes { diff --git a/dataclients/kubernetes/kube.go b/dataclients/kubernetes/kube.go index 5ca47e3e66..63238bb44f 100644 --- a/dataclients/kubernetes/kube.go +++ b/dataclients/kubernetes/kube.go @@ -240,6 +240,9 @@ type Options struct { // DefaultLoadBalancerAlgorithm sets the default algorithm to be used for load balancing between backend endpoints, // available options: roundRobin, consistentHash, random, powerOfRandomNChoices DefaultLoadBalancerAlgorithm string + + // FiltersRegistry is used to validate filters names in RouteGroups/Ingresses + FiltersRegistry filters.Registry } // Client is a Skipper DataClient implementation used to create routes based on Kubernetes Ingress settings. diff --git a/skipper.go b/skipper.go index d55b4938ce..09e0047e4d 100644 --- a/skipper.go +++ b/skipper.go @@ -919,8 +919,6 @@ type Options struct { OpenPolicyAgentCleanerInterval time.Duration OpenPolicyAgentStartupTimeout time.Duration - // EnableAdmissionWebhook runs skipper in admission webhook mode - EnableAdmissionWebhook bool // EnableValidationWebhook runs skipper in admission webhook mode // *IMPORTANT* This mode runs only the validation webhook server and does not start the proxy EnableValidationWebhook bool @@ -1983,6 +1981,7 @@ func run(o Options, sig chan os.Signal, idleConnsCH chan struct{}) error { o.ValidationWebhookAddr, o.ValidationWebhookTLSCertFile, o.ValidationWebhookTLSKeyFile, + o.filterRegistry(), ) } diff --git a/webhook/admission/admission_test.go b/webhook/admission/admission_test.go index d3869de555..848c436335 100644 --- a/webhook/admission/admission_test.go +++ b/webhook/admission/admission_test.go @@ -12,6 +12,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/zalando/skipper/dataclients/kubernetes/definitions" + "github.com/zalando/skipper/filters/builtin" ) const ( @@ -86,9 +88,10 @@ func TestUnsupportedContentType(t *testing.T) { func TestRouteGroupAdmitter(t *testing.T) { for _, tc := range []struct { - name string - inputFile string - message string + name string + inputFile string + message string + withFilterRegistry bool }{ { name: "allowed", @@ -103,6 +106,12 @@ func TestRouteGroupAdmitter(t *testing.T) { name: "valid eskip filters", inputFile: "rg-with-valid-eskip-filters.json", }, + { + name: "valid eskip filters but not supported", + inputFile: "rg-with-valid-eskip-filters-but-not-supported.json", + message: `filter \"test\" is not supported`, + withFilterRegistry: true, + }, { name: "invalid eskip filters", inputFile: "rg-with-invalid-eskip-filters.json", @@ -136,7 +145,15 @@ func TestRouteGroupAdmitter(t *testing.T) { req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() - rgAdm := &RouteGroupAdmitter{} + + var rgValidator *definitions.RouteGroupValidator + if tc.withFilterRegistry { + rgValidator = &definitions.RouteGroupValidator{FiltersRegistry: builtin.MakeRegistry()} + } else { + rgValidator = &definitions.RouteGroupValidator{} + } + + rgAdm := &RouteGroupAdmitter{RouteGroupValidator: rgValidator} h := Handler(rgAdm) h(w, req) @@ -154,9 +171,10 @@ func TestRouteGroupAdmitter(t *testing.T) { func TestIngressAdmitter(t *testing.T) { for _, tc := range []struct { - name string - inputFile string - message string + name string + inputFile string + message string + withFilterRegistry bool }{ { name: "allowed without annotations", @@ -171,6 +189,16 @@ func TestIngressAdmitter(t *testing.T) { inputFile: "invalid-filters.json", message: `invalid \"zalando.org/skipper-filter\" annotation: parse failed after token this, last route id: , position 9: syntax error`, }, + { + name: "Filter not found in filter registry", + inputFile: "invalid-filter-name.json", + message: `filter \"play\" is not supported`, + withFilterRegistry: true, + }, + { + name: "Filter not found in filter registry but valid eskip filters", + inputFile: "invalid-filter-name.json", + }, { name: "invalid eskip predicates", inputFile: "invalid-predicates.json", @@ -200,7 +228,15 @@ func TestIngressAdmitter(t *testing.T) { req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() - ingressAdm := &IngressAdmitter{} + + var ingressValidator *definitions.IngressV1Validator + if tc.withFilterRegistry { + ingressValidator = &definitions.IngressV1Validator{FiltersRegistry: builtin.MakeRegistry()} + } else { + ingressValidator = &definitions.IngressV1Validator{} + } + + ingressAdm := &IngressAdmitter{IngressValidator: ingressValidator} h := Handler(ingressAdm) h(w, req) diff --git a/webhook/admission/testdata/ingress/invalid-filter-name.json b/webhook/admission/testdata/ingress/invalid-filter-name.json new file mode 100644 index 0000000000..6eb9902a24 --- /dev/null +++ b/webhook/admission/testdata/ingress/invalid-filter-name.json @@ -0,0 +1,39 @@ +{ + "request": { + "uid": "req-uid", + "name": "req1", + "namespace": "n1", + "object": { + "metadata": { + "name": "ing1", + "namespace": "ing1", + "annotations": { + "zalando.org/skipper-filter": "play(10) -> inlineContent(\"This should't work\")" + } + }, + "spec": { + "rules": [ + { + "host": "example.com", + "http": { + "paths": [ + { + "backend": { + "service": { + "name": "example-service", + "port": { + "number": 80 + } + } + }, + "path": "/", + "pathType": "Prefix" + } + ] + } + } + ] + } + } + } +} diff --git a/webhook/admission/testdata/rg/rg-with-valid-eskip-filters-but-not-supported.json b/webhook/admission/testdata/rg/rg-with-valid-eskip-filters-but-not-supported.json new file mode 100644 index 0000000000..4e139d78d6 --- /dev/null +++ b/webhook/admission/testdata/rg/rg-with-valid-eskip-filters-but-not-supported.json @@ -0,0 +1,48 @@ +{ + "request": { + "uid": "req-uid", + "name": "req1", + "operation": "create", + "kind": { + "group": "zalando", + "version": "v1", + "kind": "RouteGroup" + }, + "namespace": "n1", + "object": { + "metadata": { + "name": "rg1", + "namespace": "n1" + }, + "spec": { + "backends": [ + { + "name": "backend", + "type": "shunt" + } + ], + "defaultBackends": [ + { + "backendName": "backend" + } + ], + "routes": [ + { + "backends": [ + { + "backendName": "backend" + } + ], + "filters": [ + "test(201)" + ], + "path": "/", + "predicates": [ + "Method(\"GET\")" + ] + } + ] + } + } + } +} diff --git a/webhook/main.go b/webhook/main.go index 4ecae86de8..66331135a1 100644 --- a/webhook/main.go +++ b/webhook/main.go @@ -11,6 +11,7 @@ import ( "github.com/prometheus/client_golang/prometheus/promhttp" log "github.com/sirupsen/logrus" "github.com/zalando/skipper/dataclients/kubernetes/definitions" + "github.com/zalando/skipper/filters" "github.com/zalando/skipper/webhook/admission" ) @@ -22,10 +23,11 @@ const ( var DefaultLogLevel = log.InfoLevel.String() type options struct { - loglevel string - certFile string - keyFile string - address string + loglevel string + certFile string + keyFile string + address string + filterRegistry filters.Registry } func (opts *options) parse() { @@ -51,12 +53,13 @@ func (opts *options) parse() { } -func Run(loglevel, address, certFile, keyFile string) { +func Run(loglevel, address, certFile, keyFile string, filterRegistry filters.Registry) { opts := &options{ - loglevel: loglevel, - address: address, - certFile: certFile, - keyFile: keyFile, + loglevel: loglevel, + address: address, + certFile: certFile, + keyFile: keyFile, + filterRegistry: filterRegistry, } run(opts) } @@ -64,8 +67,8 @@ func Run(loglevel, address, certFile, keyFile string) { func run(opts *options) { opts.parse() - rgAdmitter := &admission.RouteGroupAdmitter{RouteGroupValidator: &definitions.RouteGroupValidator{}} - ingressAdmitter := &admission.IngressAdmitter{IngressValidator: &definitions.IngressV1Validator{}} + rgAdmitter := &admission.RouteGroupAdmitter{RouteGroupValidator: &definitions.RouteGroupValidator{FiltersRegistry: opts.filterRegistry}} + ingressAdmitter := &admission.IngressAdmitter{IngressValidator: &definitions.IngressV1Validator{FiltersRegistry: opts.filterRegistry}} handler := http.NewServeMux() handler.Handle("/routegroups", admission.Handler(rgAdmitter)) handler.Handle("/ingresses", admission.Handler(ingressAdmitter))