From f0483b152fbc8b98e15bc790b36dbfa962e61eb4 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Mon, 6 Nov 2023 19:20:41 +0100 Subject: [PATCH 1/7] Validate that network backends doesn't have paths Signed-off-by: Mustafa Abdelrahman --- .../definitions/routegroupvalidator.go | 15 +++++ .../testdata/errorwrapdata/errors.log | 4 ++ .../testdata/errorwrapdata/routegroups.json | 56 +++++++++++++++++++ .../route-with-invalid-backend-with-path.log | 1 + .../route-with-invalid-backend-with-path.yaml | 41 ++++++++++++++ .../route-with-multiple-filters-one-item.log | 2 +- 6 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log create mode 100644 dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go index 4911f7910c..bb3d3fe72c 100644 --- a/dataclients/kubernetes/definitions/routegroupvalidator.go +++ b/dataclients/kubernetes/definitions/routegroupvalidator.go @@ -3,6 +3,7 @@ package definitions import ( "errors" "fmt" + "net/url" "github.com/zalando/skipper/eskip" ) @@ -37,6 +38,7 @@ func (rgv *RouteGroupValidator) Validate(item *RouteGroupItem) error { var errs []error errs = append(errs, rgv.filtersValidation(item)) errs = append(errs, rgv.predicatesValidation(item)) + errs = append(errs, rgv.validateBackends(item)) return errorsJoin(errs...) } @@ -91,6 +93,19 @@ func (rgv *RouteGroupValidator) predicatesValidation(item *RouteGroupItem) error return errorsJoin(errs...) } +func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error { + var errs []error + for _, backend := range item.Spec.Backends { + address, err := url.Parse(backend.Address) + if err != nil { + errs = append(errs, fmt.Errorf("failed to parse backend address %s: %w", backend.Address, err)) + } else if address.Path != "" { + errs = append(errs, fmt.Errorf("backend address %s contains path", backend.Address)) + } + } + 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 836138e99f..889bcb07f8 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log @@ -4,3 +4,7 @@ single predicate expected at "Path(\"/foo\") && Method(\"GET\")" single predicate expected at "" single filter expected at "inlineContent(\"/foo\") -> status(200)" single filter expected at " " +backend address "http://example.com/foo" contains path +backend address "http://example.com/foo/bar" contains path +backend address "http://example.com/foo/" contains path +backend address "/foo" contains path diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json index 270d491ec9..68436db467 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json @@ -134,6 +134,62 @@ } ] } + }, + { + "apiVersion": "zalando.org/v1", + "metadata": { + "name": "rg1" + }, + "kind": "RouteGroup", + "spec": { + "backends": [ + { + "name": "backend1", + "type": "network", + "address": "http://example.com" + }, + { + "name": "backend2", + "type": "network", + "address": "http://example.com/foo" + }, + { + "name": "backend3", + "type": "network", + "address": "http://example.com/foo/bar" + }, + { + "name": "backend4", + "type": "network", + "address": "http://example.com/foo/" + }, + { + "name": "backend5", + "type": "network", + "address": "/foo" + }, + { + "name": "shunt", + "type": "shunt" + } + ], + "hosts": [ + "test.example.com" + ], + "routes": [ + { + "backends": [ + { + "backendName": "shunt" + } + ], + "filters": [ + "inlineContent(\"/foo\")" + ], + "path": "/foo" + } + ] + } } ] } diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log new file mode 100644 index 0000000000..442e2e4bbf --- /dev/null +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log @@ -0,0 +1 @@ +backend address \"http://example.com/foo\" contains path\nbackend address \"http://example.com/foo/bar\" contains path\nbackend address \"/foo\" contains path\nbackend address \"/foo/bar\" contains path \ No newline at end of file diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml new file mode 100644 index 0000000000..d09f0ac319 --- /dev/null +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml @@ -0,0 +1,41 @@ +apiVersion: zalando.org/v1 +kind: RouteGroup +metadata: + name: test-route-group +spec: + hosts: + - example.org + backends: + - name: app + type: service + serviceName: app-svc + servicePort: 80 + - name: backend1 + type: network + address: http://example.com + - name: backend2 + type: network + address: http://example.com/foo + - name: backend3 + type: network + address: http://example.com/foo/bar + - name: backend4 + type: network + address: /foo + - name: backend5 + type: network + address: /foo/bar + 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/definitions/testdata/validation/route-with-multiple-filters-one-item.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log index 6300163089..e2640cbb65 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log @@ -1 +1 @@ -single filter expected +single filter expected at "foo(42) -> bar(24)" \ No newline at end of file From 4a315091cc78d7f189d627649b6ac95b0903dcaf Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Tue, 7 Nov 2023 16:53:23 +0100 Subject: [PATCH 2/7] Fix tests and add more Signed-off-by: Mustafa Abdelrahman --- cmd/webhook/admission/admission_test.go | 5 + .../rg/rg-with-invalid-backend-path.json | 93 +++++++++++++++++++ .../definitions/routegroupvalidator.go | 4 +- .../testdata/errorwrapdata/errors.log | 1 + .../testdata/errorwrapdata/routegroups.json | 10 ++ .../route-with-invalid-backend-with-path.log | 7 +- .../route-with-invalid-backend-with-path.yaml | 9 ++ .../route-with-multiple-filters-one-item.log | 2 +- ...oute-with-multiple-predicates-one-item.log | 2 +- 9 files changed, 128 insertions(+), 5 deletions(-) create mode 100644 cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go index a2ad4b26da..f93bc1b100 100644 --- a/cmd/webhook/admission/admission_test.go +++ b/cmd/webhook/admission/admission_test.go @@ -133,6 +133,11 @@ func TestRouteGroupAdmitter(t *testing.T) { 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\nbackend address \"http://example.com/foo/bar\" contains path\nbackend address \"http://example.com/foo/\" contains path\nbackend address \"/foo\" contains path\nbackend address \"http://example.com/\" contains path\nbackend address \"example.com/\" contains path\nbackend address \"example.com/foo\" contains path`, + }, } { t.Run(tc.name, func(t *testing.T) { expectedResponse := responseAllowedFmt diff --git a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json new file mode 100644 index 0000000000..410b4c8641 --- /dev/null +++ b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json @@ -0,0 +1,93 @@ +{ + "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": "invalid-backend1", + "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:pass@example.com" + }, + { + "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" + } + ], + "defaultBackends": [ + { + "backendName": "valid-backend1" + } + ], + "routes": [ + { + "backends": [ + { + "backendName": "valid-backend1" + } + ], + "filters": [ + "status(201)" + ], + "path": "/", + "predicates": [ + "Method(\"GET\")" + ] + } + ] + } + } + } +} diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go index bb3d3fe72c..aa8c2916f5 100644 --- a/dataclients/kubernetes/definitions/routegroupvalidator.go +++ b/dataclients/kubernetes/definitions/routegroupvalidator.go @@ -98,9 +98,9 @@ func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error { for _, backend := range item.Spec.Backends { address, err := url.Parse(backend.Address) if err != nil { - errs = append(errs, fmt.Errorf("failed to parse backend address %s: %w", backend.Address, err)) + errs = append(errs, fmt.Errorf("failed to parse backend address %q: %w", backend.Address, err)) } else if address.Path != "" { - errs = append(errs, fmt.Errorf("backend address %s contains path", backend.Address)) + errs = append(errs, fmt.Errorf("backend address %q contains path", backend.Address)) } } return errorsJoin(errs...) diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log index 889bcb07f8..0f8258ab43 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log @@ -8,3 +8,4 @@ backend address "http://example.com/foo" contains path backend address "http://example.com/foo/bar" contains path backend address "http://example.com/foo/" contains path backend address "/foo" contains path +backend address "example.com/" contains path diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json index 68436db467..8f0f8b7873 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json @@ -168,6 +168,16 @@ "type": "network", "address": "/foo" }, + { + "name": "backend6", + "type": "network", + "address": "example.com/" + }, + { + "name": "backend7", + "type": "network", + "address": "http://user:pass@example.com" + }, { "name": "shunt", "type": "shunt" diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log index 442e2e4bbf..267c10995c 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log @@ -1 +1,6 @@ -backend address \"http://example.com/foo\" contains path\nbackend address \"http://example.com/foo/bar\" contains path\nbackend address \"/foo\" contains path\nbackend address \"/foo/bar\" contains path \ No newline at end of file +backend address \\\"http://example\.com/foo\\\" contains path +backend address \\\"http://example\.com/foo/bar\\\" contains path +backend address \\\"/foo\\\" contains path +backend address \\\"/foo/bar\\\" contains path +backend address \\\"example\.com/foo\\\" contains path +backend address \\\"http://example\.com/\\\" contains path diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml index d09f0ac319..42592498b4 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml @@ -25,6 +25,15 @@ spec: - name: backend5 type: network address: /foo/bar + - name: backend6 + type: network + address: example.com/foo + - name: backend7 + type: network + address: http://example.com/ + - name: backend8 + type: network + address: http://user:pass@example.com defaultBackends: - backendName: app routes: diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log index e2640cbb65..1b78680a21 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-filters-one-item.log @@ -1 +1 @@ -single filter expected at "foo(42) -> bar(24)" \ No newline at end of file +single filter expected at \\\"foo\(42\) -> bar\(24\)\\\" diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-predicates-one-item.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-predicates-one-item.log index 6ec8ec4cbd..ee42e11293 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-predicates-one-item.log +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-multiple-predicates-one-item.log @@ -1 +1 @@ -single predicate expected +single predicate expected at \\\"Foo\(\\\\\\"X-Bar\\\\\\", \\\\\\"42\\\\\\"\) && Bar\(\\\\\\"X-Foo\\\\\\", \\\\\\"24\\\\\\"\)\\\" From c07d0dd0ae94ae1fbfdcd9ec449d3bf65d6f3670 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Wed, 8 Nov 2023 11:19:14 +0100 Subject: [PATCH 3/7] Include query params into validation Signed-off-by: Mustafa Abdelrahman --- cmd/webhook/admission/admission_test.go | 2 +- .../admission/testdata/rg/rg-with-invalid-backend-path.json | 5 +++++ dataclients/kubernetes/definitions/routegroupvalidator.go | 2 +- .../kubernetes/definitions/testdata/errorwrapdata/errors.log | 1 + .../definitions/testdata/errorwrapdata/routegroups.json | 5 +++++ .../validation/route-with-invalid-backend-with-path.log | 1 + .../validation/route-with-invalid-backend-with-path.yaml | 3 +++ 7 files changed, 17 insertions(+), 2 deletions(-) diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go index f93bc1b100..1591a50011 100644 --- a/cmd/webhook/admission/admission_test.go +++ b/cmd/webhook/admission/admission_test.go @@ -136,7 +136,7 @@ func TestRouteGroupAdmitter(t *testing.T) { { name: "routegroup with invalid backends", inputFile: "rg-with-invalid-backend-path.json", - message: `backend address \"http://example.com/foo\" contains path\nbackend address \"http://example.com/foo/bar\" contains path\nbackend address \"http://example.com/foo/\" contains path\nbackend address \"/foo\" contains path\nbackend address \"http://example.com/\" contains path\nbackend address \"example.com/\" contains path\nbackend address \"example.com/foo\" contains path`, + message: `backend address \"http://example.com/foo\" contains path\nbackend address \"http://example.com/foo/bar\" contains path\nbackend address \"http://example.com/foo/\" contains path\nbackend address \"/foo\" contains path\nbackend address \"http://example.com/\" contains path\nbackend address \"example.com/\" contains path\nbackend address \"example.com/foo\" contains path\nbackend address \"http://example.com?foo=bar\" contains path`, }, } { t.Run(tc.name, func(t *testing.T) { diff --git a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json index 410b4c8641..3061a889b2 100644 --- a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json +++ b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json @@ -64,6 +64,11 @@ "name" : "invalid-backend8", "type" : "network", "address" : "example.com/foo" + }, + { + "name" : "invalid-backend9", + "type" : "network", + "address" : "http://example.com?foo=bar" } ], "defaultBackends": [ diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go index aa8c2916f5..982b11b64a 100644 --- a/dataclients/kubernetes/definitions/routegroupvalidator.go +++ b/dataclients/kubernetes/definitions/routegroupvalidator.go @@ -99,7 +99,7 @@ func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error { 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 != "" { + } else if address.Path != "" || address.RawQuery != "" { errs = append(errs, fmt.Errorf("backend address %q contains path", backend.Address)) } } diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log index 0f8258ab43..18f92eef53 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log @@ -9,3 +9,4 @@ backend address "http://example.com/foo/bar" contains path backend address "http://example.com/foo/" contains path backend address "/foo" contains path backend address "example.com/" contains path +backend address "http://example.com?foo=bar" contains path diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json index 8f0f8b7873..63b851771e 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json @@ -178,6 +178,11 @@ "type": "network", "address": "http://user:pass@example.com" }, + { + "name": "backend8", + "type": "network", + "address": "http://example.com?foo=bar" + }, { "name": "shunt", "type": "shunt" diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log index 267c10995c..bb1e36a1f1 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log @@ -4,3 +4,4 @@ backend address \\\"/foo\\\" contains path backend address \\\"/foo/bar\\\" contains path backend address \\\"example\.com/foo\\\" contains path backend address \\\"http://example\.com/\\\" contains path +backend address \\\"http://example\.com\?foo=bar\\\" contains path diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml index 42592498b4..2b76f57e0a 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml @@ -34,6 +34,9 @@ spec: - name: backend8 type: network address: http://user:pass@example.com + - name: backend9 + type: network + address: http://example.com?foo=bar defaultBackends: - backendName: app routes: From 883e57ce14ffe6216e43a6931ab309c2e9bdea2d Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Wed, 8 Nov 2023 17:27:13 +0100 Subject: [PATCH 4/7] Append error message Signed-off-by: Mustafa Abdelrahman --- cmd/webhook/admission/admission_test.go | 2 +- .../kubernetes/definitions/routegroupvalidator.go | 2 +- .../definitions/testdata/errorwrapdata/errors.log | 12 ++++++------ .../route-with-invalid-backend-with-path.log | 14 +++++++------- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go index 1591a50011..2876f38e95 100644 --- a/cmd/webhook/admission/admission_test.go +++ b/cmd/webhook/admission/admission_test.go @@ -136,7 +136,7 @@ func TestRouteGroupAdmitter(t *testing.T) { { name: "routegroup with invalid backends", inputFile: "rg-with-invalid-backend-path.json", - message: `backend address \"http://example.com/foo\" contains path\nbackend address \"http://example.com/foo/bar\" contains path\nbackend address \"http://example.com/foo/\" contains path\nbackend address \"/foo\" contains path\nbackend address \"http://example.com/\" contains path\nbackend address \"example.com/\" contains path\nbackend address \"example.com/foo\" contains path\nbackend address \"http://example.com?foo=bar\" contains path`, + message: `backend address \"http://example.com/foo\" contains path or query\nbackend address \"http://example.com/foo/bar\" contains path or query\nbackend address \"http://example.com/foo/\" contains path or query\nbackend address \"/foo\" contains path or query\nbackend address \"http://example.com/\" contains path or query\nbackend address \"example.com/\" contains path or query\nbackend address \"example.com/foo\" contains path or query\nbackend address \"http://example.com?foo=bar\" contains path or query`, }, } { t.Run(tc.name, func(t *testing.T) { diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go index 982b11b64a..e8b766a2a0 100644 --- a/dataclients/kubernetes/definitions/routegroupvalidator.go +++ b/dataclients/kubernetes/definitions/routegroupvalidator.go @@ -100,7 +100,7 @@ func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error { if err != nil { errs = append(errs, fmt.Errorf("failed to parse backend address %q: %w", backend.Address, err)) } else if address.Path != "" || address.RawQuery != "" { - errs = append(errs, fmt.Errorf("backend address %q contains path", backend.Address)) + errs = append(errs, fmt.Errorf("backend address %q contains path or query", backend.Address)) } } return errorsJoin(errs...) diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log index 18f92eef53..e47711a162 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log @@ -4,9 +4,9 @@ single predicate expected at "Path(\"/foo\") && Method(\"GET\")" single predicate expected at "" single filter expected at "inlineContent(\"/foo\") -> status(200)" single filter expected at " " -backend address "http://example.com/foo" contains path -backend address "http://example.com/foo/bar" contains path -backend address "http://example.com/foo/" contains path -backend address "/foo" contains path -backend address "example.com/" contains path -backend address "http://example.com?foo=bar" contains path +backend address "http://example.com/foo" contains path or query +backend address "http://example.com/foo/bar" contains path or query +backend address "http://example.com/foo/" contains path or query +backend address "/foo" contains path or query +backend address "example.com/" contains path or query +backend address "http://example.com?foo=bar" contains path or query diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log index bb1e36a1f1..4d72f0b8e3 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log @@ -1,7 +1,7 @@ -backend address \\\"http://example\.com/foo\\\" contains path -backend address \\\"http://example\.com/foo/bar\\\" contains path -backend address \\\"/foo\\\" contains path -backend address \\\"/foo/bar\\\" contains path -backend address \\\"example\.com/foo\\\" contains path -backend address \\\"http://example\.com/\\\" contains path -backend address \\\"http://example\.com\?foo=bar\\\" contains path +backend address \\\"http://example\.com/foo\\\" contains path or query +backend address \\\"http://example\.com/foo/bar\\\" contains path or query +backend address \\\"/foo\\\" contains path or query +backend address \\\"/foo/bar\\\" contains path or query +backend address \\\"example\.com/foo\\\" contains path or query +backend address \\\"http://example\.com/\\\" contains path or query +backend address \\\"http://example\.com\?foo=bar\\\" contains path or query From e00e7bc90cb926fc78928fb477a70f471733b2d5 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Wed, 8 Nov 2023 17:29:25 +0100 Subject: [PATCH 5/7] Rename test case Signed-off-by: Mustafa Abdelrahman --- .../admission/testdata/rg/rg-with-invalid-backend-path.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json index 3061a889b2..9c242ac73c 100644 --- a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json +++ b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json @@ -21,7 +21,7 @@ "type": "shunt" }, { - "name": "invalid-backend1", + "name": "valid-backend2", "type": "network", "address": "http://example.com" }, From c2aa5cc48695b157068defe1fac9a1c29446455a Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Wed, 8 Nov 2023 17:38:36 +0100 Subject: [PATCH 6/7] Rename backend because of duplicate one Signed-off-by: Mustafa Abdelrahman --- .../admission/testdata/rg/rg-with-invalid-backend-path.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json index 9c242ac73c..addbd743c0 100644 --- a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json +++ b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json @@ -21,7 +21,7 @@ "type": "shunt" }, { - "name": "valid-backend2", + "name": "valid-backend3", "type": "network", "address": "http://example.com" }, From a1d31ccf17a41f8d5705d03d8a9b2dac229e6ec4 Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Wed, 15 Nov 2023 15:52:41 +0100 Subject: [PATCH 7/7] Include missing scheme, rename functions for consistant naming and ensure we only validate network backends. Signed-off-by: Mustafa Abdelrahman --- cmd/webhook/admission/admission_test.go | 2 +- .../rg/rg-with-invalid-backend-path.json | 5 +++++ .../definitions/routegroupvalidator.go | 20 ++++++++++--------- .../testdata/errorwrapdata/errors.log | 13 ++++++------ .../testdata/errorwrapdata/routegroups.json | 5 +++++ .../route-with-invalid-backend-with-path.log | 15 +++++++------- .../route-with-invalid-backend-with-path.yaml | 3 +++ 7 files changed, 40 insertions(+), 23 deletions(-) diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go index 2876f38e95..9563c2a7d3 100644 --- a/cmd/webhook/admission/admission_test.go +++ b/cmd/webhook/admission/admission_test.go @@ -136,7 +136,7 @@ func TestRouteGroupAdmitter(t *testing.T) { { name: "routegroup with invalid backends", inputFile: "rg-with-invalid-backend-path.json", - message: `backend address \"http://example.com/foo\" contains path or query\nbackend address \"http://example.com/foo/bar\" contains path or query\nbackend address \"http://example.com/foo/\" contains path or query\nbackend address \"/foo\" contains path or query\nbackend address \"http://example.com/\" contains path or query\nbackend address \"example.com/\" contains path or query\nbackend address \"example.com/foo\" contains path or query\nbackend address \"http://example.com?foo=bar\" contains path or query`, + 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) { diff --git a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json index addbd743c0..d604b97244 100644 --- a/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json +++ b/cmd/webhook/admission/testdata/rg/rg-with-invalid-backend-path.json @@ -69,6 +69,11 @@ "name" : "invalid-backend9", "type" : "network", "address" : "http://example.com?foo=bar" + }, + { + "name": "invalid-backend10", + "type": "network", + "address": "example.com" } ], "defaultBackends": [ diff --git a/dataclients/kubernetes/definitions/routegroupvalidator.go b/dataclients/kubernetes/definitions/routegroupvalidator.go index e8b766a2a0..21fa31427b 100644 --- a/dataclients/kubernetes/definitions/routegroupvalidator.go +++ b/dataclients/kubernetes/definitions/routegroupvalidator.go @@ -36,8 +36,8 @@ 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...) @@ -62,7 +62,7 @@ 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 { @@ -78,7 +78,7 @@ func (rgv *RouteGroupValidator) filtersValidation(item *RouteGroupItem) error { 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 { @@ -96,11 +96,13 @@ func (rgv *RouteGroupValidator) predicatesValidation(item *RouteGroupItem) error func (rgv *RouteGroupValidator) validateBackends(item *RouteGroupItem) error { var errs []error for _, backend := range item.Spec.Backends { - 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 != "" { - errs = append(errs, fmt.Errorf("backend address %q contains path or query", backend.Address)) + 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...) diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log index e47711a162..b5c5856864 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/errors.log @@ -4,9 +4,10 @@ single predicate expected at "Path(\"/foo\") && Method(\"GET\")" single predicate expected at "" single filter expected at "inlineContent(\"/foo\") -> status(200)" single filter expected at " " -backend address "http://example.com/foo" contains path or query -backend address "http://example.com/foo/bar" contains path or query -backend address "http://example.com/foo/" contains path or query -backend address "/foo" contains path or query -backend address "example.com/" contains path or query -backend address "http://example.com?foo=bar" contains path or query +backend address "http://example.com/foo" contains path, query or missing scheme +backend address "http://example.com/foo/bar" contains path, query or missing scheme +backend address "http://example.com/foo/" contains path, query or missing scheme +backend address "/foo" contains path, query or missing scheme +backend address "example.com/" contains path, query or missing scheme +backend address "http://example.com?foo=bar" contains path, query or missing scheme +backend address "example.com" contains path, query or missing scheme diff --git a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json index 63b851771e..c512a644bc 100644 --- a/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json +++ b/dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.json @@ -183,6 +183,11 @@ "type": "network", "address": "http://example.com?foo=bar" }, + { + "name": "backend9", + "type": "network", + "address": "example.com" + }, { "name": "shunt", "type": "shunt" diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log index 4d72f0b8e3..c6d33f774f 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.log @@ -1,7 +1,8 @@ -backend address \\\"http://example\.com/foo\\\" contains path or query -backend address \\\"http://example\.com/foo/bar\\\" contains path or query -backend address \\\"/foo\\\" contains path or query -backend address \\\"/foo/bar\\\" contains path or query -backend address \\\"example\.com/foo\\\" contains path or query -backend address \\\"http://example\.com/\\\" contains path or query -backend address \\\"http://example\.com\?foo=bar\\\" contains path or query +backend address \\\"http://example\.com/foo\\\" contains path, query or missing scheme +backend address \\\"http://example\.com/foo/bar\\\" contains path, query or missing scheme +backend address \\\"/foo\\\" contains path, query or missing scheme +backend address \\\"/foo/bar\\\" contains path, query or missing scheme +backend address \\\"example\.com/foo\\\" contains path, query or missing scheme +backend address \\\"http://example\.com/\\\" contains path, query or missing scheme +backend address \\\"http://example\.com\?foo=bar\\\" contains path, query or missing scheme +backend address \\\"example\.com\\\" contains path, query or missing scheme diff --git a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml index 2b76f57e0a..dc7b64a935 100644 --- a/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml +++ b/dataclients/kubernetes/definitions/testdata/validation/route-with-invalid-backend-with-path.yaml @@ -37,6 +37,9 @@ spec: - name: backend9 type: network address: http://example.com?foo=bar + - name: backend10 + type: network + address: example.com defaultBackends: - backendName: app routes: