validator: make sure Ingress/RouteGroup doesn't have duplicate hosts (#…

…2753) Signed-off-by: Mustafa Abdelrahman <[email protected]>
zalando · Jan 23, 2024 · 6c33066 · 6c33066
1 parent cb7d01c
commit 6c33066
Show file tree

Hide file tree

Showing 13 changed files with 168 additions and 18 deletions.
diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go
@@ -138,6 +138,11 @@ func TestRouteGroupAdmitter(t *testing.T) {
 			inputFile: "rg-with-invalid-backend-path.json",
 			message:   `backend address \"http://example.com/foo\" does not match scheme://host format\nbackend address \"http://example.com/foo/bar\" does not match scheme://host format\nbackend address \"http://example.com/foo/\" does not match scheme://host format\nbackend address \"/foo\" does not match scheme://host format\nbackend address \"http://example.com/\" does not match scheme://host format\nbackend address \"example.com/\" does not match scheme://host format\nbackend address \"example.com/foo\" does not match scheme://host format\nbackend address \"http://example.com?foo=bar\" does not match scheme://host format\nbackend address \"example.com\" does not match scheme://host format`,
 		},
+		{
+			name:      "routegroup with duplicate hosts",
+			inputFile: "rg-with-duplicate-hosts.json",
+			message:   `duplicate host \"example.org\"`,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			expectedResponse := responseAllowedFmt

diff --git a/cmd/webhook/admission/testdata/ingress/invalid-ingress-with-duplicate-hosts.json b/cmd/webhook/admission/testdata/ingress/invalid-ingress-with-duplicate-hosts.json
@@ -0,0 +1,41 @@
+{
+    "request": {
+        "uid": "req-uid",
+        "name": "req1",
+        "namespace": "n1",
+        "object": {
+            "metadata": {
+                "name": "ing1",
+                "namespace": "ing1",
+                "annotations": {
+                    "zalando.org/skipper-filter": "status(200) -> inlineContent(\"This should work\")",
+                    "zalando.org/skipper-predicate": "Header(\"test\", \"test\") && Path(\"/login\")",
+                    "zalando.org/skipper-routes": "r1: Header(\"test2\", \"test2\") && Path(\"/login\") -> status(200) -> \"http://foo.test\""
+                }
+            },
+            "spec": {
+                "rules": [
+                    {
+                        "host": "example.com",
+                        "http": {
+                            "paths": [
+                                {
+                                    "backend": {
+                                        "service": {
+                                            "name": "example-service",
+                                            "port": {
+                                                "number": 80
+                                            }
+                                        }
+                                    },
+                                    "path": "/",
+                                    "pathType": "Prefix"
+                                }
+                            ]
+                        }
+                    }
+                ]
+            }
+        }
+    }
+}
diff --git a/cmd/webhook/admission/testdata/rg/rg-with-duplicate-hosts.json b/cmd/webhook/admission/testdata/rg/rg-with-duplicate-hosts.json
@@ -0,0 +1,31 @@
+{
+  "request": {
+    "uid": "req-uid",
+    "name": "req1",
+    "namespace": "n1",
+    "object": {
+      "metadata": {
+        "name": "rg1",
+        "namespace": "n1"
+      },
+      "spec": {
+        "backends": [
+          {
+            "name": "backend",
+            "type": "shunt"
+          }
+        ],
+        "defaultBackends": [
+          {
+            "backendName": "backend"
+          }
+        ],
+        "hosts": [
+          "example.org",
+          "example.com",
+          "example.org"
+        ]
+      }
+    }
+  }
+}
diff --git a/dataclients/kubernetes/definitions/ingressvalidator.go b/dataclients/kubernetes/definitions/ingressvalidator.go
@@ -14,6 +14,7 @@ func (igv *IngressV1Validator) Validate(item *IngressV1Item) error {
 	errs = append(errs, igv.validateFilterAnnotation(item.Metadata.Annotations))
 	errs = append(errs, igv.validatePredicateAnnotation(item.Metadata.Annotations))
 	errs = append(errs, igv.validateRoutesAnnotation(item.Metadata.Annotations))
+	errs = append(errs, igv.validateHosts(item))
 
 	return errorsJoin(errs...)
 }
@@ -50,3 +51,15 @@ func (igv *IngressV1Validator) validateRoutesAnnotation(annotations map[string]s
 	}
 	return nil
 }
+
+func (igv *IngressV1Validator) validateHosts(item *IngressV1Item) error {
+	var errs []error
+	uniqueHosts := make(map[string]struct{}, len(item.Spec.Rules))
+	for _, rule := range item.Spec.Rules {
+		if _, ok := uniqueHosts[rule.Host]; ok {
+			errs = append(errs, fmt.Errorf("duplicate host %q", rule.Host))
+		}
+		uniqueHosts[rule.Host] = struct{}{}
+	}
+	return errorsJoin(errs...)
+}
diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go
@@ -39,6 +39,7 @@ func (rgv *RouteGroupValidator) Validate(item *RouteGroupItem) error {
 	errs = append(errs, rgv.validateFilters(item))
 	errs = append(errs, rgv.validatePredicates(item))
 	errs = append(errs, rgv.validateBackends(item))
+	errs = append(errs, rgv.validateHosts(item))
 
 	return errorsJoin(errs...)
 }
@@ -110,6 +111,18 @@ func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error {
 	return errorsJoin(errs...)
 }
 
+func (rgv *RouteGroupValidator) validateHosts(item *RouteGroupItem) error {
+	var errs []error
+	uniqueHosts := make(map[string]struct{}, len(item.Spec.Hosts))
+	for _, host := range item.Spec.Hosts {
+		if _, ok := uniqueHosts[host]; ok {
+			errs = append(errs, fmt.Errorf("duplicate host %q", host))
+		}
+		uniqueHosts[host] = struct{}{}
+	}
+	return errorsJoin(errs...)
+}
+
 // 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/definitions/testdata/errorwrapdata/errors.log b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log
@@ -11,3 +11,4 @@ backend address "/foo" does not match scheme://host format
 backend address "example.com/" does not match scheme://host format
 backend address "http://example.com?foo=bar" does not match scheme://host format
 backend address "example.com" does not match scheme://host format
+duplicate host "test.example.com"
diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json
@@ -210,6 +210,39 @@
                     }
                 ]
             }
+        },
+        {
+            "apiVersion": "zalando.org/v1",
+            "metadata": {
+                "name": "rg1"
+            },
+            "kind": "RouteGroup",
+            "spec": {
+                "backends": [
+                    {
+                        "name": "shunt",
+                        "type": "shunt"
+                    }
+                ],
+                "hosts": [
+                    "test.example.com",
+                    "test.example.com",
+                    "test2.example.com"
+                ],
+                "routes": [
+                    {
+                        "backends": [
+                            {
+                                "backendName": "shunt"
+                            }
+                        ],
+                        "filters": [
+                            "inlineContent(\"/foo\")"
+                        ],
+                        "path": "/foo"
+                    }
+                ]
+            }
         }
     ]
 }
diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-duplicate-hosts.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-duplicate-hosts.log
@@ -0,0 +1 @@
+duplicate host \\\"example\.org\\\"
diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-duplicate-hosts.yaml b/dataclients/kubernetes/definitions/testdata/validation/route-with-duplicate-hosts.yaml
@@ -0,0 +1,28 @@
+apiVersion: zalando.org/v1
+kind: RouteGroup
+metadata:
+  name: test-route-group
+spec:
+  hosts:
+  - example.org
+  - example.org
+  - example.com
+  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/dataclients/kubernetes/testdata/routegroups/convert/use-unique-hosts.eskip b/dataclients/kubernetes/testdata/routegroups/convert/use-unique-hosts.eskip
@@ -1,7 +0,0 @@
-kube_rg__default__myapp__all__0_0:
-	Host("^(app[.]example[.]org[.]?(:[0-9]+)?|example[.]org[.]?(:[0-9]+)?)$")
-	&& Path("/app")
-	-> <roundRobin, "http://10.2.4.8:80", "http://10.2.4.16:80">;
-
-kube_rg____app_example_org__catchall__0_0: Host("^(app[.]example[.]org[.]?(:[0-9]+)?)$") -> <shunt>;
-kube_rg____example_org__catchall__0_0: Host("^(example[.]org[.]?(:[0-9]+)?)$") -> <shunt>;

diff --git a/dataclients/kubernetes/testdata/routegroups/convert/use-unique-hosts.log b/dataclients/kubernetes/testdata/routegroups/convert/use-unique-hosts.log
@@ -0,0 +1 @@
+duplicate host \\\"app\.example\.org\\\"
diff --git a/dataclients/kubernetes/testdata/routegroups/examples/duplicate-host.eskip b/dataclients/kubernetes/testdata/routegroups/examples/duplicate-host.eskip
@@ -1,11 +0,0 @@
-kube_rg__default__myapp__all__0_0: Path("/articles") && Host(/^(example[.]org[.]?(:[0-9]+)?)$/)
-  -> <roundRobin, "http://10.2.9.103:7272", "http://10.2.9.104:7272">;
-
-kube_rg__default__myapp__all__1_0: Path("/articles/shoes") && Host(/^(example[.]org[.]?(:[0-9]+)?)$/)
-  -> <roundRobin, "http://10.2.9.103:7272", "http://10.2.9.104:7272">;
-
-kube_rg__default__myapp__all__2_0: Path("/order") && Host(/^(example[.]org[.]?(:[0-9]+)?)$/)
-  -> <roundRobin, "http://10.2.9.103:7272", "http://10.2.9.104:7272">;
-
-kube_rg____example_org__catchall__0_0: Host(/^(example[.]org[.]?(:[0-9]+)?)$/)
-  -> <shunt>;

diff --git a/dataclients/kubernetes/testdata/routegroups/examples/duplicate-host.log b/dataclients/kubernetes/testdata/routegroups/examples/duplicate-host.log
@@ -0,0 +1 @@
+duplicate host \\\"example\.org\\\"