Create validator object to give more validation flexibility (#2484)

* Create `validator` object to give more validation flexiability Signed-off-by: Mustafa Abdelrahman <[email protected]> * Change under the hood for now to not break the API Signed-off-by: Mustafa Abdelrahman <[email protected]> * Start with `eskip.ParseFilters` and fix failing tests logs Signed-off-by: Mustafa Abdelrahman <[email protected]> * Join filters issues Signed-off-by: Mustafa Abdelrahman <[email protected]> * Fail after basic validation fails otherwise join errors Signed-off-by: Mustafa Abdelrahman <[email protected]> * Refactor & add more validation webhook tests Signed-off-by: Mustafa Abdelrahman <[email protected]> * Parse `predicates` eskip also Signed-off-by: Mustafa Abdelrahman <[email protected]> * Update failing eskip parsing tests to indicate that it only generate eskip in case of all valid grammer. Signed-off-by: Mustafa Abdelrahman <[email protected]> * Wait for `go1.21` to use `errors.Join` for now use an adhoc wrapper Signed-off-by: Mustafa Abdelrahman <[email protected]> * Use `errorJoin` for wrapping errors and keep `ValidateRouteGroups` naming Signed-off-by: Mustafa Abdelrahman <[email protected]> * Cleanup leftovers & typos Signed-off-by: Mustafa Abdelrahman <[email protected]>
zalando · Aug 2, 2023 · 92e0d11 · 92e0d11
1 parent 6ea4968
commit 92e0d11
Show file tree

Hide file tree

Showing 18 changed files with 595 additions and 271 deletions.
diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go
@@ -2,15 +2,40 @@ package admission
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+const (
+	responseAllowedFmt = `{
+		"kind": "AdmissionReview",
+		"apiVersion": "admission.k8s.io/v1",
+		"response": {
+			"uid": "req-uid",
+			"allowed": true
+		}
+	}`
+
+	responseNotAllowedFmt = `{
+		"kind": "AdmissionReview",
+		"apiVersion": "admission.k8s.io/v1",
+		"response": {
+			"uid": "req-uid",
+			"allowed": false,
+			"status": {
+				"message": "%s"
+			}
+		}
+	}`
+)
+
 type testAdmitter struct {
 	// validate validates & plugs parameters for Admit
 	validate func(response *admissionRequest) (*admissionResponse, error)
@@ -60,78 +85,53 @@ func TestUnsupportedContentType(t *testing.T) {
 
 func TestRouteGroupAdmitter(t *testing.T) {
 	for _, tc := range []struct {
-		name   string
-		input  string
-		result string
+		name      string
+		inputFile string
+		message   string
 	}{
 		{
-			name: "allowed",
-			input: `{
-				"request": {
-					"uid": "req-uid",
-					"name": "req1",
-					"namespace": "n1",
-					"object": {
-						"metadata": {
-							"name": "rg1",
-							"namespace": "n1"
-						},
-						"spec": {
-							"backends": [
-								{
-									"name": "backend",
-									"type": "shunt"
-								}
-							],
-							"defaultBackends": [
-								{
-									"backendName": "backend"
-								}
-							]
-						}
-					}
-				}
-			}`,
-			result: `{
-				"kind": "AdmissionReview",
-				"apiVersion": "admission.k8s.io/v1",
-				"response": {
-					"uid": "req-uid",
-					"allowed": true
-				}
-			}`,
+			name:      "allowed",
+			inputFile: "valid-rg.json",
+		},
+		{
+			name:      "not allowed",
+			inputFile: "invalid-rg.json",
+			message:   "could not validate RouteGroup, error in route group n1/rg1: route group without spec",
+		},
+		{
+			name:      "valid eskip filters",
+			inputFile: "rg-with-valid-eskip-filters.json",
+		},
+		{
+			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: "not allowed",
-			input: `{
-				"request": {
-					"uid": "req-uid",
-					"name": "req1",
-					"namespace": "n1",
-					"object": {
-						"metadata": {
-							"name": "rg1",
-							"namespace": "n1"
-						}
-					}
-				}
-			}`,
-			result: `{
-				"kind": "AdmissionReview",
-				"apiVersion": "admission.k8s.io/v1",
-				"response": {
-					"uid": "req-uid",
-					"allowed": false,
-					"status": {
-						"message":
-						"could not validate RouteGroup, error in route group n1/rg1: route group without spec"
-					}
-				}
-			}`,
+			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) {
-			req := httptest.NewRequest("POST", "http://example.com/foo", bytes.NewBuffer([]byte(tc.input)))
+			expectedResponse := responseAllowedFmt
+			if len(tc.message) > 0 {
+				expectedResponse = fmt.Sprintf(responseNotAllowedFmt, tc.message)
+			}
+
+			input, err := os.ReadFile("testdata/" + tc.inputFile)
+			require.NoError(t, err)
+
+			req := httptest.NewRequest("POST", "http://example.com/foo", bytes.NewBuffer(input))
 			req.Header.Set("Content-Type", "application/json")
 
 			w := httptest.NewRecorder()
@@ -146,7 +146,7 @@ func TestRouteGroupAdmitter(t *testing.T) {
 			require.NoError(t, err)
 			resp.Body.Close()
 
-			assert.JSONEq(t, tc.result, string(rb))
+			assert.JSONEq(t, expectedResponse, string(rb))
 		})
 	}
 }
diff --git a/cmd/webhook/admission/testdata/invalid-rg.json b/cmd/webhook/admission/testdata/invalid-rg.json
@@ -0,0 +1,13 @@
+{
+  "request": {
+    "uid": "req-uid",
+    "name": "req1",
+    "namespace": "n1",
+    "object": {
+      "metadata": {
+        "name": "rg1",
+        "namespace": "n1"
+      }
+    }
+  }
+}
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-filters.json b/cmd/webhook/admission/testdata/rg-with-invalid-eskip-filters.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-filters.json b/cmd/webhook/admission/testdata/rg-with-valid-eskip-filters.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\")"
+            ]
+          }
+        ]
+      }
+    }
+  }
+}