Added helpers for predicates and filters

[peterklijn]

Draft PR related to feature request 1537

This PR includes:

• Helpers for all predicates (including tests)
• Helpers for all filters (including tests)
• Fixes in filter documentation
• Minor changes in some predicates and filters in order to expose their logic for the helpers and tests

Added filter and predicate helpers for all filters and predicates (except deprecated ones).

All helpers are in helpers/filters/filters.go and helpers/predicates/predicates.go as placing them in the predicates and filter packages creates circular dependency errors :(

All helpers have tests, most that call the PredicateSpec.Create and FilterSpec.CreateFilter methods to validate that the arguments are valid (some filters are very hard to test, and predicates don't have a PredicateSpec).

Some predicate and filter logic has been moved or made public in order to be able to test them, mostly the names.

How the helpers differ from the eskip predicates/filters

The goal is to keep the code helpers as close to the eskip implementations as possible, throughout the helpers you'll see 2 types of ways how they differ:

1. Optional parameters, this is not possible in Go, and using empty values can be a bit dangerous as they can have a meaning, so I went with the more more verbose style of multiple functions, e.a.: Cookie() and CookieWithSettings.
2. Use proper types instead of strings, this is done for regexp.Regepx, time.Time, time.Duration, etc.

Still to do

• Add tests for some of the logic in the helpers
• Add documentation to the methods

[szuecs]

Please create a separate error, such that errors.Is() can match the formated error for example you can do: fmt.Errorf( "%w, %s", errNotAString, p.Name)
and later check for errors.Is(errNotAString, err) (positions of args could be different, did not checked them)

[szuecs]

From my side looks good. What do others think @aryszka @AlexanderYastrebov ?

[aryszka]

Thanks! As the direction, it looks good to me. I have currently two rather burocratic remarks:

• let's not create packages called like 'helpers', too generic and invites future maintainers to overload the functionality provided by them by unrelated concepts and features. I would be fine, if the proposed helpers/filters and helpers/predicates packages would be placed under the current eskip package, as eskip/filters and eskip/predicates. (As I believe they won't ever have any heavy dependencies on other parts of Skipper, therefore there's no real risk of ending up with circular deps.)
• the functionality in the helpers package (KV* etc), seems to me a bit of an overkill. How would the proposed helpers work without them? Are they critical from the perspective of convenience of routes defined in code?

[peterklijn]

Hi @aryszka,

Thanks for your feedback!

Thanks! As the direction, it looks good to me. I have currently two rather burocratic remarks:

• let's not create packages called like 'helpers', too generic and invites future maintainers to overload the functionality provided by them by unrelated concepts and features. I would be fine, if the proposed helpers/filters and helpers/predicates packages would be placed under the current eskip package, as eskip/filters and eskip/predicates. (As I believe they won't ever have any heavy dependencies on other parts of Skipper, therefore there's no real risk of ending up with circular deps.)

I agree, I'm not super happy with the helpers package. I actually started with having the helpers in the predicates and filters packages directly in the first commit. However this didn't work due to cyclic dependencies. Sadly I can't find the build results as I think they're too old and Github removed them.

For predicates I think it's easy to fix, as it's just the error that's causing it, but for the filters package it's a bit more involved, and my initial approach was to not change the Skipper codebase too much while adding these helpers. Do you have any suggestions?

• the functionality in the helpers package (KV* etc), seems to me a bit of an overkill. How would the proposed helpers work without them? Are they critical from the perspective of convenience of routes defined in code?

I agree that the KVPair is a bit overkill, the only benefit it gives is that it guarantees an even number of arguments. I added them to have consistency with the JWTPayloadAnyKVRegexp filter, where the arguments are any number of string, regex tuples. I think there's a benefit of using Golang native types like regexp.Regexp instead of using strings.
This way you have compiler guarantees that it are valid regexes. Like how the 'Before' predicate uses golang's date.Time and not a string in RFC3339 format.

But using these kind of constructs are something think we shouldn't have, I can remove them, you just lose some compiler help. What do you think?

// JWTPayloadAnyKVRegexp
 func JWTPayloadAnyKVRegexp(pairs ...helpers.KVRegexPair) *eskip.Predicate {
 	return &eskip.Predicate{
 		Name: auth.MatchJWTPayloadAnyKVRegexpName,
 		Args: helpers.KVRegexPairToArgs(pairs),
 	}
 }