From 6c33066d3e44bab91ebb0b7aff904d68e0532c98 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Tue, 23 Jan 2024 15:08:18 +0100 Subject: [PATCH] validator: make sure Ingress/RouteGroup doesn't have duplicate hosts (#2753) Signed-off-by: Mustafa Abdelrahman --- cmd/webhook/admission/admission_test.go | 5 +++ .../invalid-ingress-with-duplicate-hosts.json | 41 +++++++++++++++++++ .../testdata/rg/rg-with-duplicate-hosts.json | 31 ++++++++++++++ .../definitions/ingressvalidator.go | 13 ++++++ .../definitions/routegroupvalidator.go | 13 ++++++ .../testdata/errorwrapdata/errors.log | 1 + .../testdata/errorwrapdata/routegroups.json | 33 +++++++++++++++ .../validation/route-with-duplicate-hosts.log | 1 + .../route-with-duplicate-hosts.yaml | 28 +++++++++++++ .../convert/use-unique-hosts.eskip | 7 ---- .../routegroups/convert/use-unique-hosts.log | 1 + .../routegroups/examples/duplicate-host.eskip | 11 ----- .../routegroups/examples/duplicate-host.log | 1 + 13 files changed, 168 insertions(+), 18 deletions(-) create mode 100644 cmd/webhook/admission/testdata/ingress/invalid-ingress-with-duplicate-hosts.json create mode 100644 cmd/webhook/admission/testdata/rg/rg-with-duplicate-hosts.json create mode 100644 dataclients/kubernetes/definitions/testdata/validation/route-with-duplicate-hosts.log create mode 100644 dataclients/kubernetes/definitions/testdata/validation/route-with-duplicate-hosts.yaml create mode 100644 dataclients/kubernetes/testdata/routegroups/convert/use-unique-hosts.log create mode 100644 dataclients/kubernetes/testdata/routegroups/examples/duplicate-host.log diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go index 424fb0b9a7..d878cc8f3d 100644 --- 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 new file mode 100644 index 0000000000..7fe8aa0e56 --- /dev/null +++ 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 new file mode 100644 index 0000000000..ad025b3745 --- /dev/null +++ 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 index 1a7e2499d8..3269b4bb73 100644 --- 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 index 24b003f0f2..3f96167194 100644 --- 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 index a710227c99..837e22133b 100644 --- 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 index c512a644bc..01e0a52515 100644 --- 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 new file mode 100644 index 0000000000..f66ece565e --- /dev/null +++ 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 new file mode 100644 index 0000000000..0a2e5b7a4d --- /dev/null +++ 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 index 14fc745d0a..e69de29bb2 100644 --- 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") - -> ; - -kube_rg____app_example_org__catchall__0_0: Host("^(app[.]example[.]org[.]?(:[0-9]+)?)$") -> ; -kube_rg____example_org__catchall__0_0: Host("^(example[.]org[.]?(:[0-9]+)?)$") -> ; diff --git a/dataclients/kubernetes/testdata/routegroups/convert/use-unique-hosts.log b/dataclients/kubernetes/testdata/routegroups/convert/use-unique-hosts.log new file mode 100644 index 0000000000..ea6724ab89 --- /dev/null +++ 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 index abca05daa8..e69de29bb2 100644 --- 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]+)?)$/) - -> ; - -kube_rg__default__myapp__all__1_0: Path("/articles/shoes") && Host(/^(example[.]org[.]?(:[0-9]+)?)$/) - -> ; - -kube_rg__default__myapp__all__2_0: Path("/order") && Host(/^(example[.]org[.]?(:[0-9]+)?)$/) - -> ; - -kube_rg____example_org__catchall__0_0: Host(/^(example[.]org[.]?(:[0-9]+)?)$/) - -> ; diff --git a/dataclients/kubernetes/testdata/routegroups/examples/duplicate-host.log b/dataclients/kubernetes/testdata/routegroups/examples/duplicate-host.log new file mode 100644 index 0000000000..f66ece565e --- /dev/null +++ b/dataclients/kubernetes/testdata/routegroups/examples/duplicate-host.log @@ -0,0 +1 @@ +duplicate host \\\"example\.org\\\"