skipper route CRD

[vetinari]

We're currently running skoap with a single route like this

   Path("/*") && Method("GET")
              -> auth("/services")
              -> dropRequestHeader("Authorization")
              -> basicAuth("username", "secret")
              -> "https://special.service.example.com";

with auth("/services") -> oauthTokeninfoAllKV("realm","/services") for newer skipper versions.

We would now need an ingress definition with incoming host names defined and one disconnected kubernetes service, that is not deleted in later and has a special meaning for this route. We have no pods involved in this route. The current zalando.org/skipper-routes annotation can be used as workaround, but needs to have a kubernetes service that exists, too.
https://github.com/zalando/skipper/issues/512

[szuecs]

We might want to think about having a customer skipper route CRD for the use case of custom skipper routes.

[szuecs]

Request for comments:

apiVersion: zalando.org/v1
kind: SkipperRoute
metadata:
  name: "{{{APPLICATION}}}-blogposts-put"
  labels:
    application: "{{{APPLICATION}}}"
    version: "{{{BUILD_VERSION}}}"
spec:
  ingress: "name-reference"  # optional, copy base predicates and filters from ingress
  filters: # optional
  - localRatelimit(“1m”, 10)
  - enableAccessLog()
  - flowId("reuse")
  predicates:
  - JWTPayloadAllKV("iss", "https://idp.example.com", “https://identity.example.com/role”, “publisher”)
  - Method(“PUT")
  - PathSubtree(“/blogposts/:id”)

My Questions:
- [ ] How should the behavior of the status field be?

• first applied?
• should all instances add themselves, but how to delete them again?
• does it makes sense to have a status or do we need some other data?
  
  
  
  • [ ] Anything you miss in this CRD to support your use cases?
  
  

[szuecs]

@aermakov-zalando do you have ideas for the status field, because you talked about "the usual pattern is that the controller writes the latest value from metadata.generation into status.observedGeneration when it does stuff". One problem is that we have N skippers and only one object status field, but maybe you have an idea.

[aermakov-zalando]

No idea. I'd either create a separate controller that would get the results from all Skipper instances and update the status, or use leader election in Skippers and collect the data in one of the instances. Both of these would be annoying to implement, though.

[aermakov-zalando]

A couple of suggestions:

• Use a list of routes instead of just one. This will simplify managing more complicated overrides.
• Add the hostname explicitly and make it required. Right now it's very easy to accidentally screw up everything running in the cluster.

[szuecs]

We think about creating a RouteGroup CRD that can be traffic switched as a set of routes with https://github.com/zalando-incubator/stackset-controller

[szuecs]

We started to work on the skipper RouteGroup CRD. Some example yaml files you can see in https://github.com/zalando/skipper/tree/feature/routegroup-crd/dataclients/kubernetes/fixtures

Why:

• more DRY object than ingress (hosts and backends separated from path rules)
• enable skipper specific features
• we don't want to have an object with multiple hostnames that have different backends
• orchestrate traffic switching for a full RouteGroup

Examples

Minimal example

apiVersion: zalando.org/v1
kind: RouteGroup
metadata:
  name: my-routes
spec:
  hosts:
  - foo.example.org
  backends:
  - serviceName: foo-service
    servicePort: 80
  paths:
  - path: /

route + redirect route

One example case that happens more often is only possible with more than 1 ingress: ingress + redirect:

apiVersion: zalando.org/v1
kind: RouteGroup
metadata:
  name: my-routes
spec:
  backends:
  - serviceName: foo-service
    servicePort: 80
  paths:
  - path: /login
    method: get
    configs:
    - filters:
      - redirectTo(301, "https://login.example.org")
  - path: /

complex routes

apiVersion: zalando.org/v1
kind: RouteGroup
metadata:
  name: my-routes
spec:
  hosts:
  - www.complex.example.org
  - complex.example.org
  backends:
  - serviceName: foo-service-v1
    servicePort: 80
  paths:
  - path: /api/resource
    method: post
    configs:
    - filters:
      - ratelimit(20, "1m")
      - oauthTokeninfoAllKV("iss", "https://accounts.google.com", "email", "[email protected]")
      predicates:
      - JWTPayloadAllKV("iss", "https://accounts.google.com", "email", "[email protected]")
  - path: /api/resource/*
    method:  get
    configs:
    - filters:
      - clientRatelimit(100, "1m", "Authorization")
      - oauthTokeninfoAllScope("read.resource", "list.resource")

Traffic switching case

apiVersion: zalando.org/v1
kind: RouteGroup
metadata:
  name: my-routes
  annotations:
    zalando.org/backend-weights:
      {"foo-service-v1": 80, "foo-service-v2": 20}
spec:
  backends:
  - serviceName: foo-service-v1
    servicePort: 80
  - serviceName: foo-service-v2
    servicePort: 80
  paths:
  - path: /api/resource
    method: post
    configs:
    - filters:
      - ratelimit(20, "1m")
      - oauthTokeninfoAllKV("iss", "https://accounts.google.com", "email", "[email protected]")
      predicates:
      - JWTPayloadAllKV("iss", "https://accounts.google.com", "email", "[email protected]")
  - path: /api/resource/*
    method:  get

A/B test

A/B test via cookie canary

• 10% chance to get cookie for service-a
• rest service-b

apiVersion: zalando.org/v1
kind: RouteGroup
metadata:
  name: my-routes
spec:
  backends:
  - serviceName: service-b
    servicePort: 80
  paths:
  - path: /
    config:
    - filters:
      - responseCookie("canary", "A")
      predicates:
      - Traffic(.1)
    backends:
    - serviceName: service-a
      servicePort: 80
  - path: /
    config:
    - filters:
      - responseCookie("canary", "B")
  - path: /
    config:
    - predicates:
      - Cookie("canary", "team-foo")
  - path: /
    config:
    - predicates:
      - Cookie("canary", "A")
    backends:
    - serviceName: service-a
      servicePort: 80
  - path: /
    config:
    - predicates:
      - Cookie("canary", "B")

[aermakov-zalando]

Looks very nice! A couple of suggestions/questions:

• Move filters/predicates into the path object. The extra config key doesn't really bring any value here. They also probably shouldn't be an array, unless I'm missing a use case?
• Traffic switching: make it a first class field (either in spec or in status). It's a new resource anyway, no need to have it as a weird annotation.
• Is path here an explicit path, or a prefix? 
• How would custom destinations work in this case (<shunt>, or a URL)? Maybe worth adding a destination key, with several options, e.g.

   destination:
     shunt: true

   destination:
     url: https://foo.zalan.do

   destination:
     backends:
       - serviceName: service-a
         servicePort: 80

[aryszka]

for destination: let's use a backend type enumeration, shunt/loopback/network/service, where service is the default and resolves to the LB endpoints.

[szuecs]

@aermakov-zalando The config key is used to have filters+predicates as a list of configs, which is an internal use case with fabric. I am open for renaming config

The problem we discussed with the traffic switching is that we have multiple controllers writing this object and if we have the annotation it's more safe (IIRC) to change from stackset controller the traffic settings. The problem with status is that it should reflect the current state not the goal.

The destinations I don't fully understand now with @aryszka's addition. Do you mean

destination:
  backend_type: <shunt>
  backends:
    - serviceName: service-a
      servicePort: 80

Additional questions: Would we ignore backends, if we have a backend_type?

[aermakov-zalando]

@szuecs Could you describe the use case, or how this would be used in practice? I still don't understand why an extra key is necessary.

Having traffic weights in the spec is perfectly fine, having it in the annotation doesn't really get you any extra safety.

[aryszka]

@szuecs yes, something like that.

the possible types could be: shunt, loopback, service (default), external, and lb.

shunt would not require further fields, loopback neither. Service would be the default, but it would require the fields as you described above. The external would mean any free URL address (without path).

Shunt:

destinations:
- type: shunt

Loopback:

destinations:
- type: loopback

Service, explicit, no list required:

destinations:
- type: service
  serviceName: service-a
  port: 80

Service, default, no list required:

destinations:
- serviceName: service-a
  port: 80

Load Balanced (the above service still can be load balanced when applied, here just the user may explicitly set LB endpoints):

destinations:
- type: lb
  algorithm: consistent-hash
  endpoints:
    - address: 1.2.3.4:9000

External:

destinations:
- type: external
  address: https://foo.example.org

[aryszka]

see the above suggestion for defining the variants of a single destination. As we discussed since then, we will probably have to support multiple destinations for the traffic switching.

[mikkeloscar]

@aermakov-zalando

Having traffic weights in the spec is perfectly fine, having it in the annotation doesn't really get you any extra safety.

The only point against the spec would be if we want to support re-applying the RouteGroup without overwriting the traffic state. This would not be needed in a stackset case if we store the desired/actual traffic somewhere other than the RouteGroup.

[aermakov-zalando]

@mikkeloscar apply shouldn't touch any keys that it didn't create, so I don't think it'll be an issue.

[hjacobs]

Can we have some design proposal document (Markdown file in this repo?) to have a nice overview on how it will look like? It's really hard for me to follow this comment thread as I was not involved from the start.

[aryszka]

@hjacobs good idea, creating one. Just one last comment, to jot down what we just discussed IRL. The traffic weights, could also be part of the destination entries, s.g. like:

destinations:
  - type: service
    serviceName: service-a-1
    port: 80
  - type: service
    serviceName: service-a-2
    port: 80
    trafficWeight: 0.2

[szuecs]

@aermakov-zalando Use case for example admins have a different scope check than a client service for the same route. Different clients, filtered by Predicates could get different filter settings.

[aermakov-zalando]

@szuecs Couldn't they just create 2 path objects then? I'd rather reduce the noise in the most common use case (path object mapping to 1 route). If one object is supposed to generate multiple routes, then why is path not an array as well?

[szuecs]

@aermakov-zalando true it can be compiled into the list of paths. I am fine with that. I will soon post a proposal and link it

[szuecs]

Proposal: https://github.com/zalando/skipper/blob/feature/routegroup-crd/dataclients/kubernetes/crd.md

[szuecs]

There are some open questions raised by @aryszka:

• what should we by default for the path: Path() or PathSubtree()?
  
  
  
  • for human users, PathSubtree() is probably better
  
  
  • for OpenAPI users, Path() is better
  
  
• How to specify http/https for Kubernetes service backends (all others have to pass the URLs themselves).

My answers would be:

• Path()/PathSubtree() can be default to the one we choose from flags and overwrite via annotation
• http/https can be default to the one we choose from flags and overwrite via annotation, too. Or depends on servicePort settings in kubernetes. Every non kubernetes backend needs to pass the scheme of the URL anyways.

[aermakov-zalando]

@szuecs I'd make it controllable per-route (because why not), e.g. have path: / versus exactPath: /, or path: /, exactPath: true. No need to have it only globally.

[MarcusNoble]

I've only breifly looked through the proposal so far but I have a few thoughts:

• Will the CRD controller create an Ingress resource in kubernetes? If not, will there be any way to use RouteGroup in conjunction with external-dns?
• I'm wondering if a defaultBackends property is the way to go or if it might be better as a default bool property on the backend.
• In the complex example I don't see the value in having a shunt and loopback backend defined but I haven't a use case for these so may simply be lack of exposure.
• Will the path / pathSubtree support regex or wildcard paths?

[mikkeloscar]

Will the CRD controller create an Ingress resource in kubernetes? If not, will there be any way to use RouteGroup in conjunction with external-dns?

Good point. This should be addressed in the proposal.

[szuecs]

@MarcusNoble: thanks for your input!

• Yes external-dns is on my list to change to support RouteGroups. Also missing is kube-ingress-aws-controller for example to have feature parity with the current AWS ingress setup
• First, we had default as a property, but decided to put it into its own list of backendRef with the weights. This comment https://github.com/zalando/skipper/pull/1180#discussion_r322246510 was pointing out the reason. Basically now we can override in the routes more nicely and IMO it's easier to spot a traffic switch.
• shunt and loopback were only examples to show, you can use all skipper backend types. For a shunt use case, think about redirects, they need to be shunted in skipper.
• path and pathSubtree will not support regexps, because the routing tree does not allow this. I believe, that most users should not use regexp, because it's slow, consumes a lot of resources and regexps are not intuitive. We support pathRegexp so you can use just this without path or pathSubtree.

I will add a section about the components that need to be changed in order to make the RouteGroup useful to the proposal.

[szuecs]

FYI: I added https://github.com/zalando/skipper/blob/feature/routegroup-crd/dataclients/kubernetes/crd.md#external-components-that-need-to-be-changed to show what external components need to be changed and added https://github.com/zalando/skipper/blob/feature/routegroup-crd/dataclients/kubernetes/crd.md#why-not-adapt-other-solutions to show why we do not adapt SMI or other CRDs.

[szuecs]

https://github.com/zalando/skipper/pull/1180 is in progress

[szuecs]

This was done also for external-dns and kube-ingress-aws-controller and is in use by Zalando.