Traffic Route L7 proposal

[jakubdyszkiewicz]

Proposal on how we can introduce L7 HTTP routing to Kuma.

[subnetmarco]

Why do we need this match object?

[lobkovilya]

L7 traffic route finally gives an ability to define more envoy.Routes inside envoy.VirtualHost, that's why I have 2 comments:

1. There are policies (or will be in the future) like Retries, RateLimiting, TrafficMirror that could be configured per envoy.Route. If there was a way to match routes from TrafficRoute in the destination selectors then we'd solve the problem of multiple tags in the destination. Maybe we should let the user to name the routes he defined in TrafficRoute policy? Especially having in mind the fact envoy.Route has name docs. Speaking of TrafficMirror I think this is the only way to unblock it.
2. Do we want to support envoy.RouteActions? Probably features like host_rewrite or path_rewrite are pretty popular

[subnetmarco]

A few comments:

1. Why do we always have split? If the user doesn't want to split the traffic, no need to say split. ie:

conf:
  http:
    headers:
      canary: "^true$"
    method: "GET"
    destination:
      kuma.io/service: backend
      version: "1.1"

and

conf:
  http:
    headers:
      canary: "^true$"
    method: "GET"
    split:
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "1.1"
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "2.1"

2. If the user wants to split, they should always use a weight. If they don't want to split, they shouldn't use split in the first place but just point the destination directly.

3. Can we always use regular expressions instead of exact, prefix, etc to match the values?

4. I am okay with the catch-all since the most specific matcher always takes precedence anyways.

conf:
  - http:
      headers:
        canary: "^true$"
      method: "GET"
      split:
        - weight: 0.5
          destination:
            kuma.io/service: backend
            version: "1.1"
        - weight: 0.5
          destination:
            kuma.io/service: backend
            version: "2.1"
  - destination:
      kuma.io/service: backend
      version: "0.1"

Thoughts?

[jakubdyszkiewicz]

@subnetmarco

1 + 2 I like the idea. So there is either destination or split, right? Should we also expand this on L4? So the default policy is

type: TrafficRoute
name: route-all-default
mesh: default
sources:
  - match:
      kuma.io/service: '*'
destinations:
  - match:
      kuma.io/service: '*'
conf:
  loadBalancer:
    roundRobin: {}
  destination: # <- instead of split
    kuma.io/service: '*'

3. I was also thinking of this and I'm a bit hesitant to implement it. This will be executed on every single request. I can spend a day or two trying to benchmark exact vs regex, I'm worried this will have a performance penalty.

4. I'm a bit confused by this YAML. What is the schema with this? Conf is not a list. Conf right now has loadbalancer, split and http.

X. I think match is useful here. It's a clear indication that this specific setting is applied only when there is match on conditions presented in the match. But that's only my thoughts, would be useful to hear more from the rest of the team.

[sudeeptoroy]

For http, the destinations are chosen by certain condition. For example
path /backend1 --> kuma.io/service: backend1
path /backend2 --> kuma.io/service: backend2

So the initial 'destinations' seems extra for http workload and can be ignored.

destinations:

• match:
  kuma.io/service: '*'

Later point in time you may also have to extend them with timeouts, retry, rate limit, auth etc.
I see a parallel with kube ingress especially contour which is based on Envoy (- minus the virtual host config).

[jakubdyszkiewicz]

Thanks for the feedback @sudeeptoroy

So the initial 'destinations' seems extra for http workload and can be ignored.

do you mean top level? I don't think we can get rid of this. How would you then apply L4 rules? What about loadbalancing section?

Later point in time you may also have to extend them with timeouts, retry, rate limit, auth etc.

I'd very much like to keep this in a separate policy like we do this right now.

[jakubdyszkiewicz]

@lobkovilya

1. are you talking about something like this

---
type: TrafficRoute
name: route
mesh: default
sources:
- match:
    kuma.io/service: '*'
destinations:
- match:
    kuma.io/service: backend
conf:
  http:
  - name: offers # <-- specify the name of the route 
    match:
      path:
        prefix: "/offers"
    split:
      - destination:
          kuma.io/service: backend
---
apiVersion: kuma.io/v1alpha1
kind: Retry
mesh: default
metadata:
  name: web-to-backend-retry-policy
spec:
  sources:
  - match:
      kuma.io/service: web
  destinations:
  - match:
      kuma.io/service: backend
    http: # <-- new matcher that works in parallel with match
      routeName: offers # <-- refer to the routes
  conf:
    http:
      numRetries: 3

In this case, do we even need TrafficRoute, we could explicitly generate it when something like this is specified

apiVersion: kuma.io/v1alpha1
kind: Retry
mesh: default
metadata:
  name: web-to-backend-retry-policy
spec:
  sources:
  - match:
      kuma.io/service: web
  destinations:
  - match:
      kuma.io/service: backend
    http: # <-- new matcher that works in parallel with match
      path:
        prefix: "/offers"
  conf:
    http:
      numRetries: 3

2. Good point, I don't know if we can squeeze this into first iteration but we should definitely make a room for it
   How about this?

---
type: TrafficRoute
name: route
mesh: default
sources:
- match:
    kuma.io/service: '*'
destinations:
- match:
    kuma.io/service: backend
conf:
  http:
  - match:
      path:
        prefix: "/offers"
    modify:
      path: "/not-offers"
      host: not-offers
      headers:
        add:
        - name: x-custom-header
          value: xyz
          append: true
        remove:
        - name: x-something
    destination:
      kuma.io/service: offers

@subnetmarco that's why I'd like to have match section. For me it's much easier to read that there is a separation between fields that are for matching and fields that are an action to do on matched route. I'd consider even pushing destination into modify. What do you think?

[sudeeptoroy]

Hi @jakubdyszkiewicz

I can see why getting rid of top level destinations is tough as all the other spec is based on source and destination pair. For TCP traffic, it makes a lot of sense - one to one. However http has one to many relation, depending on match criteria. I find the present spec a bit confusing, select a top source and destination and later select another destination using matching rules.

May be you should introduce a 'selector' as you have for Proxytemplate?
Other policies like retry, fault can be use the selector and destination pair to apply the config.

ex:

---
type: TrafficRoute
name: route
mesh: default
spec:
  selectors:
  - match:
      kuma.io/service: 'web'
conf:
  http:
  - match:
      path:
        prefix: "/offers"
    modify:
      path: "/not-offers"
      host: not-offers
      headers:
        add:
        - name: x-custom-header
          value: xyz
          append: true
        remove:
        - name: x-something
    destination:
      kuma.io/service: offers

---
apiVersion: kuma.io/v1alpha1
kind: Retry
mesh: default
metadata:
  name: web-to-offer-retry-policy
spec:
  sources:
  - match:
      kuma.io/service: web
  destinations:
  - match:
      kuma.io/service: offer
  conf:
    http:
      numRetries: 3

finer policies like per route retry will need many attributes creation.

what does * signify in case of http? in what scenario you would want a * for source.

type: TrafficRoute
name: route
mesh: default
sources:
- match:
    kuma.io/service: '*'
destinations:
- match:
    kuma.io/service: backend

[subnetmarco]

@sudeeptoroy in your example:

selectors:
  - match:
      kuma.io/service: 'web'

is essentially:

sources:
- match:
    kuma.io/service: 'web'
destinations:
- match:
    kuma.io/service: '*'

Making things explicit has been usually reduced the risk of errors, from our experience. Is the goal that you are trying to achieve a less verbose syntax?

[subnetmarco]

@jakubdyszkiewicz

• Let's implement my suggestion (1) and (2) in the spec.
• Regarding (3) I am okay either way.
• Regarding (4) below you find the correct example (this is what I meant originally):

conf:
  loadBalancer:
    roundRobin: {}
  http:
    headers:
      canary: "^true$"
    method: "GET"
    split:
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "1.1"
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "2.1"
  destination:
    kuma.io/service: backend
    version: "0.1"

Basically, the final catch-all implements suggestion (1) and turns into the above. If they do want to split, then it becomes:

conf:
  loadBalancer:
    roundRobin: {}
  http:
    headers:
      canary: "^true$"
    method: "GET"
    split:
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "1.1"
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "2.1"
  split:
    - weight: 100
        destination:
          kuma.io/service: backend
          version: "0.1"

[subnetmarco]

More comments:

1. http is the first L7 protocol we will implement, grpc, kafka, redis and more will show up afterwards.
2. What if the user doesn't want a catch-all? ie:

loadBalancer:
    roundRobin: {}
  http:
    headers:
      canary: "^true$"
    method: "GET"
    split:
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "1.1"
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "2.1"

What happens then? Should we return 404 by default? Just close the request?

3. When applying http on services, are we validating that the services also have the proper protocol in place? How is the routing going to be reconciled with the protocol that the user can specify on the services?

[sudeeptoroy]

Making things explicit has been usually reduced the risk of errors, from our experience. Is the goal that you are trying to achieve a less verbose syntax?

Hi @subnetmarco, I would suggest taking a few more opinion before concluding the top part of the spec. I find * in the destinations not explicit nor very intuitive.

[subnetmarco]

@sudeeptoroy perhaps there are two different topics:

1. The current PR is about implementing L7 support in Kuma, leveraging the existing structure that we are also using for other policies, and particularly the TrafficRoute policy (this PR is an extension of an existing policy).
2. You are suggesting that - in addition to implementing L7 support in the TrafficRoute policy - we should also reconsider the way sources and destinations are currently working to match our policies. I would say that this is a more holistic change that would affect every policy, not just the current L7 extension only (which is the scope for this proposal). I wouldn't want to end up in a situation where the TrafficRoute policy is a one-off when it comes to how selectors are implemented.

So I think that we can still implement the L7 functionality while at the same time open up a conversation to support both sources and destinations + the individual selectors element, when applicable. I don't how that will look like, but I welcome the conversation.

[sudeeptoroy]

@subnetmarco thank-you for the explanation. Kindly relook at this when you feel it's appropriate.

[jakubdyszkiewicz]

conf:
  loadBalancer:
    roundRobin: {}
  http:
    headers:
      canary: "^true$"
    method: "GET"
    split:
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "1.1"
      - weight: 0.5
        destination:
          kuma.io/service: backend
          version: "2.1"
  destination:
    kuma.io/service: backend
    version: "0.1"

@subnetmarco

1. http should be a list
2. I still feel that match in http is more explicit. Especially when you consider modify section, for headers and path rewrite.
3. Are you saying that we then use split in both conf and http? Or just destination in conf and split in http?

That's a good point about "catch-all". I checked that Envoy returns 404 if it cannot find a route. I think we should stick to this behavior

Protocols

I don't think it is a good idea to add validation on kuma.io/protocol: http in destination.

An example. Let's say we've got header modification in place (discussed in the comments above). We have a default policy but with modified loadBalancer

kind: TrafficRoute
mesh: default
metadata:
  name: route-all-default
spec:
  sources:
    - match:
        kuma.io/service: '*'
  destinations:
    - match:
        kuma.io/service: '*'
  conf:
    loadBalancer:
      leastRequest:
        choiceCount: 1
    split:
      - weight: 100
        destination:
          kuma.io/service: '*'

now, if we want to add a capability to remove headers we can do this

kind: TrafficRoute
mesh: default
metadata:
  name: route-all-default
spec:
  sources:
    - match:
        kuma.io/service: '*'
  destinations:
    - match:
        kuma.io/service: '*'
  conf:
    loadBalancer:
      leastRequest:
        choiceCount: 1
    http:
    - match:
        path:
          prefix: "/"
      modify:
        headers:
        - remove:
            name: x-custom-header
    split:
      - weight: 100
        destination:
          kuma.io/service: '*'

If we had validation that in order to use http, you must have kuma.io/protocol: http in destination. Then the user would have to split the policies into two and keep loadBalancer in sync.

This will also be "symmetric" with Healthcheck that has two sections in one policy.

Additionally, this will be difficult to implement since we only allow kuma.io/service in the destination.

[bartsmykla]

I agree with @jakubdyszkiewicz that "match" section inside "http" looks kinda easier to read and understand what's going on

[subnetmarco]

Okay for (1) and (2). For (3) I suggest that we show both split and destination in both http and in the catch-all.

This in addition to the other comments that we agreed on.

[jakubdyszkiewicz]

Updated the proposal with discussed changes

[jakubdyszkiewicz]

@sudeeptoroy

With this example

type: TrafficRoute
name: route
mesh: default
spec:
  selectors:
  - match:
      kuma.io/service: 'web'
conf:
  http:
  - match:
      path:
        prefix: "/offers"
    modify:
      path: "/not-offers"
      host: not-offers
      headers:
        add:
        - name: x-custom-header
          value: xyz
          append: true
        remove:
        - name: x-something
    destination:
      kuma.io/service: offers

selectors is usually used when you apply rules on a given dataplane.

With your example, does it mean that

1. When web tries to consume anything on /offers then the traffic goes to offers ?
2. When there is incoming traffic to the web on /offers then we somehow redirect it to offers?

You raised good point that the policy is getting more complicated, but keep in mind that we are trying to support many different use-cases.

[sudeeptoroy]

Hi @jakubdyszkiewicz, I mean (1)
however (2) is also very desirable on gateway dataplane. It will act as ingress and can spray traffic to many backends on matching criteria.

[jpeach]

Suggest a match-type field:

path:
    type: exact
    value: /users/

This is consistent with Kubernetes API conventions and allows valid match types to be automatically validated by API annotations. It also makes validation check unambigious.

[jakubdyszkiewicz]

However, there is a feature of protobuf called "oneof" and providing a couple of options and picking one of them is a convention of Envoy. We use this already in many places. Even loadBalancer in TrafficRoute

[jakubdyszkiewicz]

Were they also migrating from "oneof" convention?
Validation on API annotations? I don't think I'm familiar with this, can you tell me more? We have our own unified validators for both Kubernetes and Universal.

[jpeach]

The kubernetes CRD generator support validation that will propagate checks into the openapi spec (see kubebuilder docs).

I'm confide about the protobuf reference. Is this a protobuf API? If so, shouldn't it be in proto3 rather than in yaml?

[jakubdyszkiewicz]

Kuma API is described in protobuf and then translated between JSON / YAML (but also sent between control planes in multizone deployment as serialized protobuf). In proposals, we tend to use YAML examples so it's easier to read, but you might be right that maybe we should also include proto schemas.

Protobuf also has a builtin validation functionality but we found this to not be flexible enough (descriptive messages) for our needs

[jpeach]

Ah I see. I'd probably design a YAML-oriented API differently than a protobuf-oriented API :)

[jpeach]

FWIW, Kubernetes CRD validation doesn't support unions, or one-of. There are generator annotations for it, but they don't do anything 😂

[jpeach]

Gateway API just moved away from this pattern, to a style that is more consistent with Kubernetes API guidelines:

headers:
- name: some-header
  value: some-value
  matchType: exact

Similar reasons as for path matches. This structure is also consistent with the structure you have in the modify section.

[jpeach]

Here's the kubernetes rationale for avoiding maps, https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#lists-of-named-subobjects-preferred-over-maps

[jpeach]

Assume that "HTTP" in this context means, http, https, http/2 and so on?

[jakubdyszkiewicz]

https yes, but only when mTLS is provided by Kuma. Otherwise, we cannot decrypt the traffic

[jpeach]

Ok you confused me with mtls :) why is mtls required here?

[jakubdyszkiewicz]

Imagine there is a client and server with sidecars

A <-> A's sidecar <-> B's sidecar <-> B

If A itself does not encrypt the traffic then even with mTLS A's sidecar can redirect the traffic because it can read the request metadata (it happens before encrypting the traffic)

However, if A encrypts the traffic with TLS itself (for example old service when migrating to mesh), then A's sidecar cannot decrypt the traffic and we cannot route it by matching path, headers, etc.