diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go index 118c5ba380..ff7abe9f3f 100644 --- a/cmd/webhook/admission/admission_test.go +++ b/cmd/webhook/admission/admission_test.go @@ -200,7 +200,7 @@ func TestIngressAdmitter(t *testing.T) { { name: "invalid eskip routes", inputFile: "invalid-routes.json", - message: `invalid \"zalando.org/skipper-routes\" annotation: invalid predicate count arg`, + message: `invalid \"zalando.org/skipper-routes\" annotation: invalid route \"r1\": Header predicate expects 2 string arguments`, }, { name: "invalid eskip filters and predicates", diff --git a/dataclients/kubernetes/testdata/ingressV1/ingress-data/ing-with-invalid-routes-annotation-missing-header-argument.log b/dataclients/kubernetes/testdata/ingressV1/ingress-data/ing-with-invalid-routes-annotation-missing-header-argument.log index 5f5ab84cfe..473a4cf4c8 100644 --- a/dataclients/kubernetes/testdata/ingressV1/ingress-data/ing-with-invalid-routes-annotation-missing-header-argument.log +++ b/dataclients/kubernetes/testdata/ingressV1/ingress-data/ing-with-invalid-routes-annotation-missing-header-argument.log @@ -1 +1 @@ -\[ingress\] invalid \\\"zalando\.org\/skipper-routes\\\" annotation: invalid predicate count arg +\[ingress\] invalid \\\"zalando\.org\/skipper-routes\\\" annotation: invalid route \\\"r1\\\": Header predicate expects 2 string arguments diff --git a/eskip/eskip.go b/eskip/eskip.go index 229789b873..5bb5d39855 100644 --- a/eskip/eskip.go +++ b/eskip/eskip.go @@ -17,8 +17,6 @@ import ( const duplicateHeaderPredicateErrorFmt = "duplicate header predicate: %s" var ( - invalidPredicateArgError = errors.New("invalid predicate arg") - invalidPredicateArgCountError = errors.New("invalid predicate count arg") duplicatePathTreePredicateError = errors.New("duplicate path tree predicate") duplicateMethodPredicateError = errors.New("duplicate method predicate") ) @@ -446,17 +444,25 @@ func (t BackendType) String() string { } // Expects exactly n arguments of type string, or fails. -func getStringArgs(n int, args []interface{}) ([]string, error) { - if len(args) != n { - return nil, invalidPredicateArgCountError +func getStringArgs(p *Predicate, n int) ([]string, error) { + failure := func() ([]string, error) { + if n == 1 { + return nil, fmt.Errorf("%s predicate expects 1 string argument", p.Name) + } else { + return nil, fmt.Errorf("%s predicate expects %d string arguments", p.Name, n) + } + } + + if len(p.Args) != n { + return failure() } sargs := make([]string, n) - for i, a := range args { + for i, a := range p.Args { if sa, ok := a.(string); ok { sargs[i] = sa } else { - return nil, invalidPredicateArgError + return failure() } } @@ -475,26 +481,22 @@ func applyPredicates(route *Route, proute *parsedRoute) error { ) for _, p := range proute.predicates { - if err != nil { - return err - } - switch p.Name { case "Path": if pathSet { return duplicatePathTreePredicateError } - if args, err = getStringArgs(1, p.Args); err == nil { + if args, err = getStringArgs(p, 1); err == nil { route.Path = args[0] pathSet = true } case "Host": - if args, err = getStringArgs(1, p.Args); err == nil { + if args, err = getStringArgs(p, 1); err == nil { route.HostRegexps = append(route.HostRegexps, args[0]) } case "PathRegexp": - if args, err = getStringArgs(1, p.Args); err == nil { + if args, err = getStringArgs(p, 1); err == nil { route.PathRegexps = append(route.PathRegexps, args[0]) } case "Method": @@ -502,12 +504,12 @@ func applyPredicates(route *Route, proute *parsedRoute) error { return duplicateMethodPredicateError } - if args, err = getStringArgs(1, p.Args); err == nil { + if args, err = getStringArgs(p, 1); err == nil { route.Method = args[0] methodSet = true } case "HeaderRegexp": - if args, err = getStringArgs(2, p.Args); err == nil { + if args, err = getStringArgs(p, 2); err == nil { if route.HeaderRegexps == nil { route.HeaderRegexps = make(map[string][]string) } @@ -515,7 +517,7 @@ func applyPredicates(route *Route, proute *parsedRoute) error { route.HeaderRegexps[args[0]] = append(route.HeaderRegexps[args[0]], args[1]) } case "Header": - if args, err = getStringArgs(2, p.Args); err == nil { + if args, err = getStringArgs(p, 2); err == nil { if route.Headers == nil { route.Headers = make(map[string]string) } @@ -531,9 +533,13 @@ func applyPredicates(route *Route, proute *parsedRoute) error { default: route.Predicates = append(route.Predicates, p) } + + if err != nil { + return fmt.Errorf("invalid route %q: %w", proute.id, err) + } } - return err + return nil } // Converts a parsing route objects to the exported route definition with diff --git a/eskip/eskip_test.go b/eskip/eskip_test.go index 33dbee39f7..8cac584a47 100644 --- a/eskip/eskip_test.go +++ b/eskip/eskip_test.go @@ -72,7 +72,12 @@ func TestParse(t *testing.T) { "invalid method predicate", `Path("/endpoint") && Method("GET", "POST") -> "https://www.example.org"`, nil, - "invalid predicate count arg", + `invalid route "": Method predicate expects 1 string argument`, + }, { + "invalid header predicate", + `foo: Path("/endpoint") && Header("Foo") -> "https://www.example.org";`, + nil, + `invalid route "foo": Header predicate expects 2 string arguments`, }, { "host regexps", `Host(/^www[.]/) && Host(/[.]org$/) -> "https://www.example.org"`,