Second take after validation work done in other PRs

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

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,
 	}
 

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
 }

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 {

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.

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(),
 		)
 	}
 

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)

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"
+                                }
+                            ]
+                        }
+                    }
+                ]
+            }
+        }
+    }
+}

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\")"
+            ]
+          }
+        ]
+      }
+    }
+  }
+}

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,21 +53,22 @@ 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)
 }
 
 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))