Easier use of eskip.Route when using Skipper as code

[peterklijn]

Is your feature request related to a problem? Please describe. When using Skipper as a product, and defining routes via the eskip format, Skipper has good and clear documentation. But when using Skipper as a framework, and defining routes in code, documentation is sparse and I often find myself browsing through the source code to understand how to set predicates and filters.

Describe the solution you would like In addition to providing more documentation for Skipper as a framework, I think some helper functions could really improve the usability of Skipper as a framework. These helpers will not only more the code more concise and readable, they will also serve as documentation.

Take the following route as an example (inspired by the sticky traffic predicate example):

import (
    "github.com/zalando/skipper/eskip"
    "github.com/zalando/skipper/filters/cookie"
    "github.com/zalando/skipper/predicates/traffic"
    "github.com/zalando/skipper/routing"
)

routes := []*eskip.Route{
    {
        Id: "example__catalog_test_a",
        Filters: []*eskip.Filter{
            {
                Name: cookie.ResponseCookieFilterName,
                Args: []interface{}{"catalog-test", "A"},
            },
        },
        Predicates: []*eskip.Predicate{
            {
                Name: routing.PathName,
                Args: []interface{}{"/catalog"},
            },
            {
                Name: traffic.PredicateName,
                Args: []interface{}{.15, "catalog-test", "A"},
            },
        },
        Backend: "http://catalog-test-a",
    },
    {
        Id: "example__catalog_test_b",
        Filters: []*eskip.Filter{
            {
                Name: cookie.ResponseCookieFilterName,
                Args: []interface{}{"catalog-test", "B"},
            },
        },
        Predicates: []*eskip.Predicate{
            {
                Name: routing.PathName,
                Args: []interface{}{"/catalog"},
            },
            {
                Name: traffic.PredicateName,
                Args: []interface{}{.3, "catalog-test", "B"},
            },
        },
        Backend: "http://catalog-test-b",
    },
    {
        Id: "example__catalog",
        Filters: []*eskip.Filter{
            {
                Name: cookie.ResponseCookieFilterName,
                Args: []interface{}{"catalog-test", "default"},
            },
        },
        Predicates: []*eskip.Predicate{
            {
                Name: routing.PathName,
                Args: []interface{}{"/catalog"},
            },
        },
        Backend: "http://catalog",
    },
}

Predicates and filters come from different packages (routing, traffic, cookie), so it's not immediately clear to me that they are predicates / filters. The args are always []interface{}, which makes complete sense when using the eskip syntax, but in code don't provide you with much help.

Instead, we could add helpers that create these Predicates and Filters for us. Take a look at this code, which provides the same routes:

import (
    "github.com/zalando/skipper/eskip"
    "github.com/zalando/skipper/helpers/filters"
    "github.com/zalando/skipper/helpers/predicates"
)

routes := []*eskip.Route{
    {
        Id: "example__catalog_test_a",
        Filters: []*eskip.Filter{
            filters.ResponseCookie("catalog-test", "A"),
        },
        Predicates: []*eskip.Predicate{
            predicates.Path("/catalog"),
            predicates.TrafficSticky(.15, "catalog-test", "A"),
        },
        Backend: "http://catalog-test-a",
    },
    {
        Id: "example__catalog_test_b",
        Filters: []*eskip.Filter{
            filters.ResponseCookie("catalog-test", "B"),
        },
        Predicates: []*eskip.Predicate{
            predicates.Path("/catalog"),
            predicates.TrafficSticky(.3, "catalog-test", "B"),
        },
        Backend: "http://catalog-test-b",
    },
    {
        Id: "example__catalog",
        Filters: []*eskip.Filter{
            filters.ResponseCookie("catalog-test", "default"),
        },
        Predicates: []*eskip.Predicate{
            predicates.Path("/catalog"),
        },
        Backend: "http://catalog",
    },
}

Here, all predicates come from the predicate package, and because they're Go functions, the arguments are fixed and of the right type.

I would like to know if this would be something you are adding to the core framework? I'm happy to add the predicate and filter helpers :) I made a draft PR with some examples.

Additional context (optional) A draft PR can be found here.

Would you like to work on it? Yes

[AlexanderYastrebov]

The issue with such helpers (IMO) is that they tend to diverge from filter/predicate implementations as they evolve so I think their maintenance cost/value ratio should be carefully considered here. Of course lib users always may introduce client-side helpers as they please.

If the goal is pure aesthetics brought by DSL then parsing eskip is also an option: https://github.com/zalando/skipper/blob/3507fc92fe7a19e9d98a055506403b27cfd279f6/eskip/example_test.go#L25-L60

As I am a bystander lets see what maintainers would say.

[peterklijn]

Thanks for your reply Alexander,

I did not consider the aspect that helpers might diverge from the actual implementations. You made a good point there.

The goal would not be purely aesthetics, although it does help, and I think readability of code is not a minor argument.

Instead, the main goal is easy of use. Writing rules in the eskip format is well documented, however the documentation of for using Skipper as a framework is sparse, and I often found myself searching through the source code to achieve something trivial.

Let me give you an example: When you want to define a eskip.Route, you very likely want to specify a path. However, the path attribute is deprecated in favour for a Path predicate, but does not mention how to use that.

The path predicate resides in the routing package, outside of the predicates package. This took me quite some time to find, as searching for "Path" obviously results in many hits. Instead I found it by looking at other predicates, and recognising that all predicates seem to have a constant with the same naming convention (<Name>Name), which limited the search scope.

Obviously instead of helpers, the usage of the Path predicate could have also been documented, and I would like to add documentation for code usage at a later point, but I would much rather document:

import "github.com/zalando/skipper/helpers/predicates"

predicates.Path("/catalog")

Instead of the current:

import (
    "github.com/zalando/skipper/eskip"
    "github.com/zalando/skipper/routing"
)

eskip.Predicate{
    Name: routing.PathName,
    Args: []interface{}{"/catalog"},
}

Apart from the verbosity, you need to explain which arguments need to go in the []interface{}, what the argument types are and in which order. This would result in more verbose documentation, and I think a bigger change of becoming out of sync with the implementation.

Regarding the client-side helpers, you are absolutely right, and this is also what I started doing in our project. Additionally I could make a small open source library, which exposes just the helpers, but the outreach would be limited :)

[szuecs]

Do you read godocs?

Developer/library docs are considered https://godoc.org/github.com/zalando/skipper Or https://pkg.go.dev/github.com/zalando/skipper

Maybe the more high level functions also make sense, but I also see the diverge problem. One other good thing is that if we would have that in the past we could easily refactor the Path predicates from routing to predicate/path....

@aryszka wdyt?

[peterklijn]

Hi Sandor, I've read them, and they have helped us a lot with setting up Skipper as a framework. I think that the high level functions together with a few lines of Godoc would dramatically improve pages like this one: https://godoc.org/github.com/zalando/skipper/predicates

[szuecs]

@peterklijn thanks great to here. The review can take a bit. We run a bit low on time. I hope next week we have time to make sure the backlog of reviews is clean again.

[peterklijn]

Thanks for getting back to me @szuecs. Please keep in mind that the PR is in no way done, it's incomplete, the build fails and there are no tests.

I added it to show what I meant, and am happy to work on it if you guys are willing to merge it, hence this issue :)

[aryszka]

Thanks for raising this topic! I got a weird idea: if we can apply some convention in the naming of these functions, then they might become the most straightforward input for a reflection based validation.

Nevertheless, I agree that the godoc around the filters and predicates is ripe for an overhaul, regardless of the linked pull request.

[szuecs]

I guess a decision needs to be done if we want to have simpler interface functions for library users or not @aryszka. I see @AlexanderYastrebov point, but honestly I like the point of readability and it could simplify some code in dataclients and tests.

Sorry @peterklijn , we are running a bit low on capacity right now. Black Friday preparations everywhere. 😀

[peterklijn]

Hi guys, could you share your thoughts on how to proceed with this please? @szuecs @AlexanderYastrebov @aryszka 😇

[szuecs]

I hope we can think about this next week. Sorry that it takes so long.

[aryszka]

I think we should proceed with this. Let me explain why.

One concern raised by @AlexanderYastrebov is that if the set of helpers diverge from the implementation. It's true that it puts some maintenance burden on the contributors to the filters and predicates, to keep it in sync. But, I can imagine that we could also generate this code, based on the actual filters and predicates. Generating this code would be a bit bigger task now, though, because we don't have a formal specification of the argument arity and types for the filters and the predicates. But if we had this information, we could use it for other features, as well: e.g. the validatation of the filters and predicates beyond just the current plain syntax validation during the parsing, and also before the route processing phase.

The validation is a separate topic, of course. Currently this validation happens in the FilterSpec.CreateFilter, with handwritten rules, and failures appear only in the application logs. Also, if a set of routes belong together, a validation error (failing CreateFilter) in one of the routes results in only that single route not being applied, while the rest is applied and potentially resulting in invalid routing.

So, why I am writing here about the validation: if we had this information about the 'signature' of the filters and predicates, like Status(statusCode int), then this information could serve as the input for both:

• the helper functions discussed here
• a reflection or code generation based validation logic

Currently, I don't have a clear design yet for how to represent this signature, but coming back to the helper functions, if we follow the proposed approach, I see a good chance to be able to refactor it in the future such that:

• we eliminate the additional maintenance burden
• we can support the route validation from the same input

...which would be a great win in my view. So, in my opinion, we can proceed with the proposed approach.

[szuecs]

Thanks @aryszka , the only thing to add is that we could likely generate parts of the filter/predicate reference documentation, too.

[aryszka]

we could likely generate parts of the filter/predicate reference documentation, too.

Excellent point! The docs are currently a point where need to be conscious about keeping the godoc and the reference docs in sync "manually".

[peterklijn]

Hey all,

I've been quite busy this weekend with the first implementation, and would appreciate some feedback 😇. I grossly underestimated the amount of filters and predicates, and the work required to make helpers for all.

https://github.com/zalando/skipper/pull/1536

There's still some work to be done, but I'm getting close :)