Rework Mesh*Route matching

lahabana commented 2 weeks ago

Description

With MeshService policy matching (https://github.com/kumahq/kuma/pull/10152) and policy on namespace (https://github.com/kumahq/kuma/pull/10148) we're getting a good story for producer/consumer policies and how to "attach a policy to a service"

The choice was made for spec.to[].targetRef.kind: MeshService this goes a little against how MeshHTTPRoute matches which was decided in MADR-028. The strong argument for going with top level targetRef in MADR-28 was matching dataplanes that don't have the route. In this case it's simple enough we just ignore it.

This current implementation has made things a little tricky because policy matching is convoluted where the top level MeshHTTPRoute gets converted to a "fake to":

https://github.com/kumahq/kuma/blob/55fa983fc12bdd1a5ea7c1cfe75901320870372b/pkg/plugins/policies/core/rules/rules.go#L346-L401

suggested changes

Allow spec.to[].targetRef.kind: Mesh*Route working like MeshService (If the service has this route then we apply the matcher on it). It's also great because we can add validation like (disallow TCP parameters on MeshHTTPRoute matches).
Deprecate MeshService and MeshHTTPRoute as a toplevel targetRef (top level targetRef should identify entire proxies) [^1]
Make topLevel targetRef optional (in practice people will most often use targetRef: kind: Mesh). Having it optional creates less confusion to users about "where they should match stuff".

benefits

top level targetRef is something most people can ignore so this makes policies easier to use (this was already discussed in MADR-005 but I think experience has changed our mind).
topLevel targetRef is complex (it depends on whether the policy is created on the zone, in the namespace of the service, another one...)
The policy matching algorithm and rules api becomes simpler and there are no indirections anymore top level targetRef identifies proxies that are matched and then to and from identify parts of the config (namely: inbounds, outbounds and routes).

open questions

How does merging work? (#9151 was already asking questions there)
What happens when we have a timeout on a service and on a route? (multiple filters?, routes is always more specific to service?)

xref

https://github.com/kumahq/kuma/issues/9589
https://github.com/kumahq/kuma/issues/8681
https://github.com/kumahq/kuma/issues/9151
https://github.com/kumahq/kuma/issues/6645 (would heavily influence this change)

examples

A producer attached policy (all consumers everywhere use this):

kind: MeshTimeout
metadata:
  name: timeout-for-service
spec:
  to:
    - targetRef:
         kind: MeshHTTPRoute
         name: my-cool-route-in-the-same-namespace
       default:
         http:
            requestTimeout: 5s

A consumer policy:

kind: MeshTimeout
metadata:
  name: timeout-for-service
spec:
  to:
    - targetRef:
         kind: MeshHTTPRoute
         labels: 
           kuma.io/display-name: my-cool-route-in-the-same-namespace
           k8s.kuma.io/namespace: some-ns
           kuma.io/zone: zone-a
       default:
         http:
            requestTimeout: 5s

A producer policy applying to just some proxies:

kind: MeshTimeout
metadata:
  name: timeout-for-service
spec:
  targetRef:
    kind: MeshSubset
    tags:
       type: "canaries"
  to:
    - targetRef:
         kind: MeshHTTPRoute
         name: my-cool-route-in-the-same-namespace
       default:
         http:
            requestTimeout: 5s

(the consumer ones work the same).

[^1]: Mesh, MeshSubset and MeshGateway would be the only topLevel targetRef allowed. We talked about remove MeshSubset but that's a parallel discussion. MeshGateway can identify a specific listener but in practice Each Gateway listener is very similar to being its own proxy so I'm not concerned here.

jakubdyszkiewicz commented 2 weeks ago

I like this change. I still hold my opinion to treat MeshHTTPRoute just like MeshService, so to forbid it in top-level target ref.

lahabana commented 2 weeks ago

I like this change. I still hold my opinion to treat MeshHTTPRoute just like MeshService, so to forbid it in top-level target ref.

Agreed which is why I suggest starting by deprecating it.

kumahq / kuma