Merge pull request #2754 from zalando/cw2023freeze

Cw2023freeze

cmd/webhook/admission/admission_test.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"github.com/zalando/skipper/dataclients/kubernetes/definitions"
 )
 
 const (
@@ -122,6 +123,21 @@ 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 \" \"`,
+		},
+		{
+			name:      "routegroup with invalid backends",
+			inputFile: "rg-with-invalid-backend-path.json",
+			message:   `backend address \"http://example.com/foo\" contains path, query or missing scheme\nbackend address \"http://example.com/foo/bar\" contains path, query or missing scheme\nbackend address \"http://example.com/foo/\" contains path, query or missing scheme\nbackend address \"/foo\" contains path, query or missing scheme\nbackend address \"http://example.com/\" contains path, query or missing scheme\nbackend address \"example.com/\" contains path, query or missing scheme\nbackend address \"example.com/foo\" contains path, query or missing scheme\nbackend address \"http://example.com?foo=bar\" contains path, query or missing scheme\nbackend address \"example.com\" contains path, query or missing scheme`,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			expectedResponse := responseAllowedFmt
@@ -136,7 +152,7 @@ func TestRouteGroupAdmitter(t *testing.T) {
 			req.Header.Set("Content-Type", "application/json")
 
 			w := httptest.NewRecorder()
-			rgAdm := &RouteGroupAdmitter{}
+			rgAdm := &RouteGroupAdmitter{RouteGroupValidator: &definitions.RouteGroupValidator{}}
 
 			h := Handler(rgAdm)
 			h(w, req)
@@ -200,7 +216,7 @@ func TestIngressAdmitter(t *testing.T) {
 			req.Header.Set("Content-Type", "application/json")
 
 			w := httptest.NewRecorder()
-			ingressAdm := &IngressAdmitter{}
+			ingressAdm := &IngressAdmitter{IngressValidator: &definitions.IngressV1Validator{}}
 
 			h := Handler(ingressAdm)
 			h(w, req)
@@ -217,8 +233,8 @@ func TestIngressAdmitter(t *testing.T) {
 }
 
 func TestMalformedRequests(t *testing.T) {
-	routeGroupHandler := Handler(&RouteGroupAdmitter{})
-	ingressHandler := Handler(&IngressAdmitter{})
+	routeGroupHandler := Handler(&RouteGroupAdmitter{RouteGroupValidator: &definitions.RouteGroupValidator{}})
+	ingressHandler := Handler(&IngressAdmitter{IngressValidator: &definitions.IngressV1Validator{}})
 
 	for _, tc := range []struct {
 		name           string

cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json

@@ -0,0 +1,103 @@
+{
+  "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": "valid-backend1",
+            "type": "shunt"
+          },
+          {
+            "name": "valid-backend3",
+            "type": "network",
+            "address": "http://example.com"
+          },
+          {
+            "name": "invalid-backend2",
+            "type": "network",
+            "address": "http://example.com/foo"
+          },
+          {
+            "name": "invalid-backend3",
+            "type": "network",
+            "address": "http://example.com/foo/bar"
+          },
+          {
+            "name": "invalid-backend4",
+            "type": "network",
+            "address": "http://example.com/foo/"
+          },
+          {
+            "name": "invalid-backend5",
+            "type": "network",
+            "address": "/foo"
+          },
+          {
+            "name": "valid-backend2",
+            "type": "network",
+            "address": "http://user:[email protected]"
+          },
+          {
+            "name": "invalid-backend6",
+            "type": "network",
+            "address": "http://example.com/"
+          },
+          {
+            "name": "invalid-backend7",
+            "type": "network",
+            "address": "example.com/"
+          },
+          {
+            "name" : "invalid-backend8",
+            "type" : "network",
+            "address" : "example.com/foo"
+          },
+          {
+            "name" : "invalid-backend9",
+            "type" : "network",
+            "address" : "http://example.com?foo=bar"
+          },
+          {
+            "name": "invalid-backend10",
+            "type": "network",
+            "address": "example.com"
+          }
+        ],
+        "defaultBackends": [
+          {
+            "backendName": "valid-backend1"
+          }
+        ],
+        "routes": [
+          {
+            "backends": [
+              {
+                "backendName": "valid-backend1"
+              }
+            ],
+            "filters": [
+              "status(201)"
+            ],
+            "path": "/",
+            "predicates": [
+              "Method(\"GET\")"
+            ]
+          }
+        ]
+      }
+    }
+  }
+}

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

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

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

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")

dataclients/kubernetes/definitions/routegroupvalidator.go

@@ -1,11 +1,20 @@
 package definitions
 
 import (
+	"errors"
+	"fmt"
+	"net/url"
+
 	"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
@@ -27,8 +36,9 @@ func (rgv *RouteGroupValidator) Validate(item *RouteGroupItem) error {
 		return err
 	}
 	var errs []error
-	errs = append(errs, rgv.filtersValidation(item))
-	errs = append(errs, rgv.predicatesValidation(item))
+	errs = append(errs, rgv.validateFilters(item))
+	errs = append(errs, rgv.validatePredicates(item))
+	errs = append(errs, rgv.validateBackends(item))
 
 	return errorsJoin(errs...)
 }
@@ -52,24 +62,47 @@ func (rgv *RouteGroupValidator) basicValidation(r *RouteGroupItem) error {
 	return nil
 }
 
-func (rgv *RouteGroupValidator) filtersValidation(item *RouteGroupItem) error {
+func (rgv *RouteGroupValidator) validateFilters(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 %q", errSingleFilterExpected, f))
+			}
 		}
 	}
 
 	return errorsJoin(errs...)
 }
 
-func (rgv *RouteGroupValidator) predicatesValidation(item *RouteGroupItem) error {
+func (rgv *RouteGroupValidator) validatePredicates(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 %q", errSinglePredicateExpected, p))
+			}
+		}
+	}
+	return errorsJoin(errs...)
+}
+
+func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error {
+	var errs []error
+	for _, backend := range item.Spec.Backends {
+		if backend.Type == eskip.NetworkBackend {
+			address, err := url.Parse(backend.Address)
+			if err != nil {
+				errs = append(errs, fmt.Errorf("failed to parse backend address %q: %w", backend.Address, err))
+			} else if address.Path != "" || address.RawQuery != "" || address.Scheme == "" {
+				errs = append(errs, fmt.Errorf("backend address %q contains path, query or missing scheme", backend.Address))
+			}
 		}
 	}
 	return errorsJoin(errs...)
@@ -131,14 +164,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
 	}