Prevent multiple items per yaml item (#2691)

* Don't allow more than one filter/predicate per yaml/json item Signed-off-by: Mustafa Abdelrahman <[email protected]>
zalando · Nov 27, 2023 · b761eb9 · b761eb9
1 parent d0f80cd
commit b761eb9
Show file tree

Hide file tree

Showing 14 changed files with 265 additions and 21 deletions.
diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go
@@ -122,6 +122,16 @@ func TestRouteGroupAdmitter(t *testing.T) {
 			inputFile: "rg-with-invalid-eskip-filters-and-predicates.json",
 			message:   "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",
 		},
+		{
+			name:      "invalid routgroup multiple filters per json/yaml array item",
+			inputFile: "rg-with-multiple-filters.json",
+			message:   `single filter expected at \"status(201) -> inlineContent(\"hi\")\"\nsingle filter expected at \" \"`,
+		},
+		{
+			name:      "invalid routgroup multiple predicates per json/yaml array item",
+			inputFile: "rg-with-multiple-predicates.json",
+			message:   `single predicate expected at \"Method(\"GET\") && Path(\"/\")\"\nsingle predicate expected at \" \"`,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			expectedResponse := responseAllowedFmt

diff --git a/cmd/webhook/admission/testdata/rg/rg-with-multiple-filters.json b/cmd/webhook/admission/testdata/rg/rg-with-multiple-filters.json
@@ -0,0 +1,49 @@
+{
+  "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) -> inlineContent(\"hi\")",
+              " "
+            ],
+            "path": "/",
+            "predicates": [
+              "Method(\"GET\")"
+            ]
+          }
+        ]
+      }
+    }
+  }
+}
diff --git a/cmd/webhook/admission/testdata/rg/rg-with-multiple-predicates.json b/cmd/webhook/admission/testdata/rg/rg-with-multiple-predicates.json
@@ -0,0 +1,49 @@
+{
+  "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\") && Path(\"/\")",
+              " "
+            ]
+          }
+        ]
+      }
+    }
+  }
+}
diff --git a/dataclients/kubernetes/definitions/definitions_test.go b/dataclients/kubernetes/definitions/definitions_test.go
@@ -2,6 +2,7 @@ package definitions_test
 
 import (
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -18,10 +19,15 @@ func TestValidateRouteGroups(t *testing.T) {
 	data, err := os.ReadFile("testdata/errorwrapdata/routegroups.json")
 	require.NoError(t, err)
 
+	logs, err := os.ReadFile("testdata/errorwrapdata/errors.log")
+	require.NoError(t, err)
+
+	logsString := strings.TrimSuffix(string(logs), "\n")
+
 	rgl, err := definitions.ParseRouteGroupsJSON(data)
 	require.NoError(t, err)
 
 	err = definitions.ValidateRouteGroups(&rgl)
 
-	assert.EqualError(t, err, "route group without name\nerror in route group default/rg1: route group without backend")
+	assert.EqualError(t, err, logsString)
 }
diff --git a/dataclients/kubernetes/definitions/routegroups.go b/dataclients/kubernetes/definitions/routegroups.go
@@ -25,8 +25,6 @@ var (
 	errRouteGroupWithoutName    = errors.New("route group without name")
 	errRouteGroupWithoutSpec    = errors.New("route group without spec")
 	errInvalidRouteSpec         = errors.New("invalid route spec")
-	errInvalidPredicate         = errors.New("invalid predicate")
-	errInvalidFilter            = errors.New("invalid filter")
 	errInvalidMethod            = errors.New("invalid method")
 	errBothPathAndPathSubtree   = errors.New("path and path subtree in the same route")
 	errMissingBackendReference  = errors.New("missing backend reference")

diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go
@@ -1,11 +1,19 @@
 package definitions
 
 import (
+	"errors"
+	"fmt"
+
 	"github.com/zalando/skipper/eskip"
 )
 
 type RouteGroupValidator struct{}
 
+var (
+	errSingleFilterExpected    = errors.New("single filter expected")
+	errSinglePredicateExpected = errors.New("single predicate expected")
+)
+
 var defaultRouteGroupValidator = &RouteGroupValidator{}
 
 // ValidateRouteGroup validates a RouteGroupItem
@@ -56,8 +64,12 @@ 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)
-			errs = append(errs, err)
+			filters, err := eskip.ParseFilters(f)
+			if err != nil {
+				errs = append(errs, err)
+			} else if len(filters) != 1 {
+				errs = append(errs, fmt.Errorf("%w at \"%s\"", errSingleFilterExpected, f))
+			}
 		}
 	}
 
@@ -68,8 +80,12 @@ func (rgv *RouteGroupValidator) predicatesValidation(item *RouteGroupItem) error
 	var errs []error
 	for _, r := range item.Spec.Routes {
 		for _, p := range r.Predicates {
-			_, err := eskip.ParsePredicates(p)
-			errs = append(errs, err)
+			predicates, err := eskip.ParsePredicates(p)
+			if err != nil {
+				errs = append(errs, err)
+			} else if len(predicates) != 1 {
+				errs = append(errs, fmt.Errorf("%w at \"%s\"", errSinglePredicateExpected, p))
+			}
 		}
 	}
 	return errorsJoin(errs...)
@@ -131,14 +147,6 @@ func (r *RouteSpec) validate(hasDefault bool, backends map[string]bool) error {
 		return errBothPathAndPathSubtree
 	}
 
-	if hasEmpty(r.Predicates) {
-		return errInvalidPredicate
-	}
-
-	if hasEmpty(r.Filters) {
-		return errInvalidFilter
-	}
-
 	if hasEmpty(r.Methods) {
 		return errInvalidMethod
 	}

diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log
@@ -0,0 +1,6 @@
+route group without name
+error in route group default/rg1: route group without backend
+single predicate expected at "Path("/foo") && Method("GET")"
+single predicate expected at ""
+single filter expected at "inlineContent("/foo") -> status(200)"
+single filter expected at " "
diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json
@@ -45,10 +45,41 @@
             "kind": "RouteGroup",
             "spec": {
                 "backends": [
-                    { 
+                    {
+                        "name": "shunt",
+                        "type": "shunt"
+                    }
+                ],
+                "hosts": [
+                    "test.example.com"
+                ],
+                "routes": [
+                    {
+                        "backends": [
+                            {
+                                "backendName": "shunt"
+                            }
+                        ],
+                        "filters": [
+                            "inlineContent(\"/foo\")"
+                        ],
+                        "path": "/foo"
+                    }
+                ]
+            }
+        },
+        {
+            "apiVersion": "zalando.org/v1",
+            "metadata": {
+                "name": "rg1"
+            },
+            "kind": "RouteGroup",
+            "spec": {
+                "backends": [
+                    {
                         "name": "shunt",
                         "type": "shunt"
-                    } 
+                    }
                 ],
                 "hosts": [
                     "test.example.com"
@@ -60,13 +91,49 @@
                                 "backendName": "shunt"
                             }
                         ],
+                        "predicates": [
+                            "Path(\"/foo\") && Method(\"GET\")",
+                            ""
+                        ],
                         "filters": [
                             "inlineContent(\"/foo\")"
                         ],
                         "path": "/foo"
                     }
                 ]
             }
+        },
+        {
+            "apiVersion": "zalando.org/v1",
+            "metadata": {
+                "name": "rg1"
+            },
+            "kind": "RouteGroup",
+            "spec": {
+                "backends": [
+                    {
+                        "name": "shunt",
+                        "type": "shunt"
+                    }
+                ],
+                "hosts": [
+                    "test.example.com"
+                ],
+                "routes": [
+                    {
+                        "backends": [
+                            {
+                                "backendName": "shunt"
+                            }
+                        ],
+                        "filters": [
+                            "inlineContent(\"/foo\") -> status(200)",
+                            " "
+                        ],
+                        "path": "/foo"
+                    }
+                ]
+            }
         }
     ]
 }
diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-empty-filter.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-empty-filter.log
@@ -1,2 +1 @@
-test-route-group
-invalid filter
+single filter expected
diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-empty-predicate.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-empty-predicate.log
@@ -1,2 +1 @@
-test-route-group
-invalid predicate
+single predicate expected
diff --git a/...ients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log b/...ients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log
@@ -0,0 +1 @@
+single filter expected
diff --git a/...ents/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.yaml b/...ents/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.yaml
@@ -0,0 +1,25 @@
+apiVersion: zalando.org/v1
+kind: RouteGroup
+metadata:
+  name: test-route-group
+spec:
+  hosts:
+  - example.org
+  backends:
+  - name: app
+    type: service
+    serviceName: app-svc
+    servicePort: 80
+  defaultBackends:
+  - backendName: app
+  routes:
+  - path: /
+    methods:
+    - GET
+    - HEAD
+    predicates:
+    - Foo("X-Bar", "42")
+    filters:
+    - foo(42) -> bar(24)
+    backends:
+    - backendName: app
diff --git a/...ts/kubernetes/definitions/testdata/validation/route-with-multiple-predicates-one-item.log b/...ts/kubernetes/definitions/testdata/validation/route-with-multiple-predicates-one-item.log
@@ -0,0 +1 @@
+single predicate expected
diff --git a/...s/kubernetes/definitions/testdata/validation/route-with-multiple-predicates-one-item.yaml b/...s/kubernetes/definitions/testdata/validation/route-with-multiple-predicates-one-item.yaml
@@ -0,0 +1,26 @@
+apiVersion: zalando.org/v1
+kind: RouteGroup
+metadata:
+  name: test-route-group
+spec:
+  hosts:
+  - example.org
+  backends:
+  - name: app
+    type: service
+    serviceName: app-svc
+    servicePort: 80
+  defaultBackends:
+  - backendName: app
+  routes:
+  - path: /
+    methods:
+    - GET
+    - HEAD
+    predicates:
+    - Foo("X-Bar", "42") && Bar("X-Foo", "24")
+    filters:
+    - foo(42)
+    - bar(24)
+    backends:
+    - backendName: app