From d733e188c9f159dd8d711a8b78376d47c5986f68 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Mon, 27 Nov 2023 18:25:56 +0100 Subject: [PATCH 1/4] Validate Ingress/RouteGroup doesn't have duplicate hosts 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/routegroups.json | 33 +++++++++++++++ .../validation/route-with-duplicate-hosts.log | 1 + .../route-with-duplicate-hosts.yaml | 28 +++++++++++++ 8 files changed, 165 insertions(+) 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 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..538cbc66f4 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.validateDuplicateHosts(item)) return errorsJoin(errs...) } @@ -50,3 +51,15 @@ func (igv *IngressV1Validator) validateRoutesAnnotation(annotations map[string]s } return nil } + +func (igv *IngressV1Validator) validateDuplicateHosts(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..8474c3cd6b 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.validateDuplicateHosts(item)) return errorsJoin(errs...) } @@ -110,6 +111,18 @@ func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error { return errorsJoin(errs...) } +func (rgv *RouteGroupValidator) validateDuplicateHosts(item *RouteGroupItem) error { + var errs []error + uniqueHosts := make(map[string]struct{}, len(item.Spec.Hosts)) + for _, hosts := range item.Spec.Hosts { + if _, ok := uniqueHosts[hosts]; ok { + errs = append(errs, fmt.Errorf("duplicate host %q", hosts)) + } + uniqueHosts[hosts] = 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/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 From c8f3b31f79cf6b82a8a99b26b7b44a7e254572e4 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Tue, 28 Nov 2023 16:16:45 +0100 Subject: [PATCH 2/4] Address comments Signed-off-by: Mustafa Abdelrahman --- .../kubernetes/definitions/ingressvalidator.go | 4 ++-- .../kubernetes/definitions/routegroupvalidator.go | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/dataclients/kubernetes/definitions/ingressvalidator.go b/dataclients/kubernetes/definitions/ingressvalidator.go index 538cbc66f4..3269b4bb73 100644 --- a/dataclients/kubernetes/definitions/ingressvalidator.go +++ b/dataclients/kubernetes/definitions/ingressvalidator.go @@ -14,7 +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.validateDuplicateHosts(item)) + errs = append(errs, igv.validateHosts(item)) return errorsJoin(errs...) } @@ -52,7 +52,7 @@ func (igv *IngressV1Validator) validateRoutesAnnotation(annotations map[string]s return nil } -func (igv *IngressV1Validator) validateDuplicateHosts(item *IngressV1Item) error { +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 { diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go index 8474c3cd6b..3f96167194 100644 --- a/dataclients/kubernetes/definitions/routegroupvalidator.go +++ b/dataclients/kubernetes/definitions/routegroupvalidator.go @@ -39,7 +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.validateDuplicateHosts(item)) + errs = append(errs, rgv.validateHosts(item)) return errorsJoin(errs...) } @@ -111,14 +111,14 @@ func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error { return errorsJoin(errs...) } -func (rgv *RouteGroupValidator) validateDuplicateHosts(item *RouteGroupItem) error { +func (rgv *RouteGroupValidator) validateHosts(item *RouteGroupItem) error { var errs []error uniqueHosts := make(map[string]struct{}, len(item.Spec.Hosts)) - for _, hosts := range item.Spec.Hosts { - if _, ok := uniqueHosts[hosts]; ok { - errs = append(errs, fmt.Errorf("duplicate host %q", hosts)) + for _, host := range item.Spec.Hosts { + if _, ok := uniqueHosts[host]; ok { + errs = append(errs, fmt.Errorf("duplicate host %q", host)) } - uniqueHosts[hosts] = struct{}{} + uniqueHosts[host] = struct{}{} } return errorsJoin(errs...) } From 2b646347ab9bbdd2ecdb84d0ebb1c76db99b4d92 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Tue, 28 Nov 2023 17:12:42 +0100 Subject: [PATCH 3/4] Update testcases with duplicate routes Signed-off-by: Mustafa Abdelrahman --- .../routegroups/convert/use-unique-hosts.eskip | 7 ------- .../testdata/routegroups/convert/use-unique-hosts.log | 1 + .../routegroups/examples/duplicate-host.eskip | 11 ----------- .../testdata/routegroups/examples/duplicate-host.log | 1 + 4 files changed, 2 insertions(+), 18 deletions(-) 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/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\\\" From 60a9c94d1d080be62a44a8cce6a16100bb84aeb4 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Mon, 22 Jan 2024 19:08:26 +0100 Subject: [PATCH 4/4] Fix testcase dropped from merging Signed-off-by: Mustafa Abdelrahman --- .../kubernetes/definitions/testdata/errorwrapdata/errors.log | 1 + 1 file changed, 1 insertion(+) 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"