eskip: improve invalid predicate arguments error message

Add route id and predicate name to the error message.

Signed-off-by: Alexander Yastrebov <[email protected]>

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",

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

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,47 +481,43 @@ 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":
 			if methodSet {
 				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)
 				}
 
 				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

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"`,