From 748cf554cf08e596379f3244fcd7527cf3eca161 Mon Sep 17 00:00:00 2001 From: strekm Date: Thu, 13 Feb 2020 12:58:57 +0100 Subject: [PATCH] Validation for same rule path (#91) --- internal/validation/helpers.go | 28 +++++++-- internal/validation/validate.go | 2 +- internal/validation/validate_test.go | 88 +++++++++++++++++++++++++++- 3 files changed, 111 insertions(+), 7 deletions(-) diff --git a/internal/validation/helpers.go b/internal/validation/helpers.go index 87c091dc1..77a6d42c3 100644 --- a/internal/validation/helpers.go +++ b/internal/validation/helpers.go @@ -1,6 +1,7 @@ package validation import ( + "fmt" "net/url" "regexp" @@ -8,12 +9,28 @@ import ( ) func hasDuplicates(rules []gatewayv1alpha1.Rule) bool { - encountered := map[string]bool{} - // Create a map of all unique elements. - for v := range rules { - encountered[rules[v].Path] = true + duplicates := map[string]bool{} + + if len(rules) > 1 { + for _, rule := range rules { + if len(rule.Methods) > 0 { + for _, method := range rule.Methods { + tmp := fmt.Sprintf("%s:%s", rule.Path, method) + if duplicates[tmp] { + return true + } + duplicates[tmp] = true + } + } else { + if duplicates[rule.Path] { + return true + } + duplicates[rule.Path] = true + } + } } - return len(encountered) != len(rules) + + return false } func isValidURL(toTest string) bool { @@ -33,6 +50,7 @@ func ValidateDomainName(domain string) bool { return RegExp.MatchString(domain) } +//ValidateServiceName ? func ValidateServiceName(service string) bool { regExp := regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?\.[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) return regExp.MatchString(service) diff --git a/internal/validation/validate.go b/internal/validation/validate.go index c9c9fb19f..ce73fab14 100644 --- a/internal/validation/validate.go +++ b/internal/validation/validate.go @@ -114,7 +114,7 @@ func (v *APIRule) validateRules(attributePath string, rules []gatewayv1alpha1.Ru } if hasDuplicates(rules) { - problems = append(problems, Failure{AttributePath: attributePath, Message: "multiple rules defined for the same path"}) + problems = append(problems, Failure{AttributePath: attributePath, Message: "multiple rules defined for the same path and method"}) } for i, r := range rules { diff --git a/internal/validation/validate_test.go b/internal/validation/validate_test.go index 38f1ef6ec..0cba27c80 100644 --- a/internal/validation/validate_test.go +++ b/internal/validation/validate_test.go @@ -255,7 +255,7 @@ var _ = Describe("Validate function", func() { //then Expect(problems).To(HaveLen(6)) Expect(problems[0].AttributePath).To(Equal(".spec.rules")) - Expect(problems[0].Message).To(Equal("multiple rules defined for the same path")) + Expect(problems[0].Message).To(Equal("multiple rules defined for the same path and method")) Expect(problems[1].AttributePath).To(Equal(".spec.rules[0].accessStrategies[0].config")) Expect(problems[1].Message).To(Equal("strategy: noop does not support configuration")) @@ -273,6 +273,41 @@ var _ = Describe("Validate function", func() { Expect(problems[5].Message).To(Equal("No accessStrategies defined")) }) + It("Should fail for the same path and method", func() { + //given + testWhiteList := []string{"foo.bar", "bar.foo", "kyma.local"} + input := &gatewayv1alpha1.APIRule{ + Spec: gatewayv1alpha1.APIRuleSpec{ + Service: getService("foo-service", uint32(8080), "non-occupied-host.foo.bar"), + Rules: []gatewayv1alpha1.Rule{ + { + Path: "/abc", + AccessStrategies: []*rulev1alpha1.Authenticator{ + toAuthenticator("noop", emptyConfig()), + }, + Methods: []string{"GET"}, + }, + { + Path: "/abc", + AccessStrategies: []*rulev1alpha1.Authenticator{ + toAuthenticator("anonymous", emptyConfig()), + }, + Methods: []string{"GET", "POST"}, + }, + }, + }, + } + //when + problems := (&APIRule{ + DomainWhiteList: testWhiteList, + }).Validate(input, v1alpha3.VirtualServiceList{}) + + //then + Expect(problems).To(HaveLen(1)) + Expect(problems[0].AttributePath).To(Equal(".spec.rules")) + Expect(problems[0].Message).To(Equal("multiple rules defined for the same path and method")) + }) + It("Should succeed for valid input", func() { //given testWhiteList := []string{"foo.bar", "bar.foo", "kyma.local"} @@ -295,6 +330,14 @@ var _ = Describe("Validate function", func() { toAuthenticator("noop", emptyConfig()), }, }, + { + Path: "/abc", + AccessStrategies: []*rulev1alpha1.Authenticator{ + toAuthenticator("jwt", simpleJWTConfig()), + toAuthenticator("noop", emptyConfig()), + }, + Methods: []string{"GET"}, + }, { Path: "/bcd", AccessStrategies: []*rulev1alpha1.Authenticator{ @@ -318,6 +361,49 @@ var _ = Describe("Validate function", func() { //then Expect(problems).To(HaveLen(0)) }) + + It("Should succeed for the same path but different methods", func() { + //given + testWhiteList := []string{"foo.bar", "bar.foo", "kyma.local"} + + existingVS := v1alpha3.VirtualService{} + existingVS.OwnerReferences = []v1.OwnerReference{{UID: "12345"}} + existingVS.Spec.Hosts = []string{"occupied-host.foo.bar"} + + input := &gatewayv1alpha1.APIRule{ + ObjectMeta: v1.ObjectMeta{ + UID: "67890", + }, + Spec: gatewayv1alpha1.APIRuleSpec{ + Service: getService("foo-service", uint32(8080), "non-occupied-host.foo.bar"), + Rules: []gatewayv1alpha1.Rule{ + { + Path: "/abc", + AccessStrategies: []*rulev1alpha1.Authenticator{ + toAuthenticator("jwt", simpleJWTConfig()), + toAuthenticator("noop", emptyConfig()), + }, + Methods: []string{"POST"}, + }, + { + Path: "/abc", + AccessStrategies: []*rulev1alpha1.Authenticator{ + toAuthenticator("jwt", simpleJWTConfig()), + toAuthenticator("noop", emptyConfig()), + }, + Methods: []string{"GET"}, + }, + }, + }, + } + //when + problems := (&APIRule{ + DomainWhiteList: testWhiteList, + }).Validate(input, v1alpha3.VirtualServiceList{Items: []v1alpha3.VirtualService{existingVS}}) + + //then + Expect(problems).To(HaveLen(0)) + }) }) var _ = Describe("Validator for", func() {