Parse predicates eskip also

Signed-off-by: Mustafa Abdelrahman <[email protected]>
zalando · Aug 1, 2023 · ed092f3 · ed092f3
1 parent 35b2d99
commit ed092f3
Show file tree

Hide file tree

Showing 8 changed files with 192 additions and 34 deletions.
diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go
@@ -13,7 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-var (
+const (
 	responseAllowedFmt = `{
 		"kind": "AdmissionReview",
 		"apiVersion": "admission.k8s.io/v1",
@@ -103,10 +103,24 @@ func TestRouteGroupAdmitter(t *testing.T) {
 			inputFile: "rg-with-valid-eskip-filters.json",
 		},
 		{
-			name:      "invalid eskip filters", // This means that eskip parser failed but doesn't mean filters exist
+			name:      "invalid eskip filters",
 			inputFile: "rg-with-invalid-eskip-filters.json",
 			message:   "could not validate RouteGroup, parse failed after token status, last route id: , position 11: syntax error",
 		},
+		{
+			name:      "valid eskip predicates",
+			inputFile: "rg-with-valid-eskip-predicates.json",
+		},
+		{
+			name:      "invalid eskip predicates",
+			inputFile: "rg-with-invalid-eskip-predicates.json",
+			message:   "could not validate RouteGroup, parse failed after token Method, last route id: Method, position 6: syntax error",
+		},
+		{
+			name:      "invalid eskip filters and predicates",
+			inputFile: "rg-with-invalid-eskip-filters-and-predicates.json",
+			message:   "could not validate RouteGroup, parse failed after token status, last route id: , position 11: syntax error\\nparse failed after token Method, last route id: Method, position 6: syntax error",
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			result := responseAllowedFmt

diff --git a/cmd/webhook/admission/testdata/rg-with-invalid-eskip-filters-and-predicates.json b/cmd/webhook/admission/testdata/rg-with-invalid-eskip-filters-and-predicates.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": [
+              "status&(201)"
+            ],
+            "path": "/",
+            "predicates": [
+              "Method&(\"GET\")"
+            ]
+          }
+        ]
+      }
+    }
+  }
+}
diff --git a/cmd/webhook/admission/testdata/rg-with-invalid-eskip-predicates.json b/cmd/webhook/admission/testdata/rg-with-invalid-eskip-predicates.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": [
+              "status(201)"
+            ],
+            "path": "/",
+            "predicates": [
+              "Method&(\"GET\")"
+            ]
+          }
+        ]
+      }
+    }
+  }
+}
diff --git a/cmd/webhook/admission/testdata/rg-with-valid-eskip-predicates.json b/cmd/webhook/admission/testdata/rg-with-valid-eskip-predicates.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": [
+              "status(201)"
+            ],
+            "path": "/",
+            "predicates": [
+              "Method(\"GET\")"
+            ]
+          }
+        ]
+      }
+    }
+  }
+}
diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go
@@ -4,16 +4,13 @@ import (
 	"errors"
 
 	"github.com/zalando/skipper/eskip"
-	"github.com/zalando/skipper/filters"
 )
 
-type RouteGroupValidator struct {
-	FilterRegistry filters.Registry
-}
+type RouteGroupValidator struct{}
 
 var defaultRouteGroupValidator = &RouteGroupValidator{}
 
-// validateRouteGroup validates a RouteGroupItem
+// ValidateRouteGroup validates a RouteGroupItem
 func ValidateRouteGroup(rg *RouteGroupItem) error {
 	return defaultRouteGroupValidator.Validate(rg)
 }
@@ -28,35 +25,18 @@ func ValidateRouteGroupList(rgl RouteGroupList) error {
 
 func (rgv *RouteGroupValidator) Validate(item *RouteGroupItem) error {
 	err := rgv.basicValidation(item)
-	// if basic validation fails we don't need to run other validations, otherwise join errors.
 	if err != nil {
 		return err
 	}
-	return rgv.filtersValidation(item)
-}
-
-func (rgv *RouteGroupValidator) basicValidation(item *RouteGroupItem) error {
-	return item.validate()
-}
+	err = errors.Join(err, rgv.filtersValidation(item))
 
-func (rgv *RouteGroupValidator) filtersValidation(item *RouteGroupItem) error {
-	return validateRouteGroupFilters(item)
-}
-
-func validateRouteGroupFilters(rg *RouteGroupItem) error {
-	var err error
-	for _, r := range rg.Spec.Routes {
-		for _, f := range r.Filters {
-			_, e := eskip.ParseFilters(f)
-			err = errors.Join(err, e)
-		}
-	}
+	err = errors.Join(err, rgv.predicatesValidation(item))
 
 	return err
 }
 
 // TODO: we need to pass namespace/name in all errors
-func (r *RouteGroupItem) validate() error {
+func (rgv *RouteGroupValidator) basicValidation(r *RouteGroupItem) error {
 	// has metadata and name:
 	if r == nil || validate(r.Metadata) != nil {
 		return errRouteGroupWithoutName
@@ -74,6 +54,29 @@ func (r *RouteGroupItem) validate() error {
 	return nil
 }
 
+func (rgv *RouteGroupValidator) filtersValidation(item *RouteGroupItem) error {
+	var err error
+	for _, r := range item.Spec.Routes {
+		for _, f := range r.Filters {
+			_, e := eskip.ParseFilters(f)
+			err = errors.Join(err, e)
+		}
+	}
+
+	return err
+}
+
+func (rgv *RouteGroupValidator) predicatesValidation(item *RouteGroupItem) error {
+	var err error
+	for _, r := range item.Spec.Routes {
+		for _, p := range r.Predicates {
+			_, e := eskip.ParsePredicates(p)
+			err = errors.Join(err, e)
+		}
+	}
+	return err
+}
+
 // TODO: we need to pass namespace/name in all errors
 func (rg *RouteGroupSpec) validate() error {
 	// has at least one backend:

diff --git a/dataclients/kubernetes/logger_test.go b/dataclients/kubernetes/logger_test.go
@@ -16,7 +16,8 @@ import (
 )
 
 func TestLogger(t *testing.T) {
-	manifest, err := os.Open("testdata/routegroups/convert/failing-predicate.yaml")
+	// TODO: with validattion changes we need to update/refactor this test
+	manifest, err := os.Open("testdata/routegroups/convert/missing-service.yaml")
 	require.NoError(t, err)
 	defer manifest.Close()
 

diff --git a/dataclients/kubernetes/testdata/routegroups/convert/failing-predicate.log b/dataclients/kubernetes/testdata/routegroups/convert/failing-predicate.log
@@ -1,3 +1 @@
-\[eskip\] predicate
-parse failed
-kind=RouteGroup name=myapp ns=foo
+\[routegroup\] parse failed after token Header, last route id: Header, position 6: syntax error
diff --git a/...nts/kubernetes/testdata/routegroups/convert/single-failing-route-makes-the-group-fail.log b/...nts/kubernetes/testdata/routegroups/convert/single-failing-route-makes-the-group-fail.log
@@ -1,3 +1 @@
-Error transforming
-\[eskip\] predicate
-kind=RouteGroup name=failing
+\[routegroup\] parse failed after token foo, last route id: foo, position 3: syntax error