Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework Mesh*Route matching #10247

Open
lahabana opened this issue May 16, 2024 · 2 comments
Open

Rework Mesh*Route matching #10247

lahabana opened this issue May 16, 2024 · 2 comments
Labels
kind/feature New feature triage/accepted The issue was reviewed and is complete enough to start working on it

Comments

@lahabana
Copy link
Contributor

lahabana commented May 16, 2024

Description

With MeshService policy matching (#10152) and policy on namespace (#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":

func buildToList(p core_model.Resource, httpRoutes []core_model.Resource) ([]core_model.PolicyItem, error) {
policyWithTo, ok := p.GetSpec().(core_model.PolicyWithToList)
if !ok {
return nil, nil
}
var mhr *v1alpha1.MeshHTTPRouteResource
switch policyWithTo.GetTargetRef().Kind {
case common_api.MeshHTTPRoute:
for _, route := range httpRoutes {
if core_model.IsReferenced(p.GetMeta(), policyWithTo.GetTargetRef().Name, route.GetMeta()) {
if r, ok := route.(*v1alpha1.MeshHTTPRouteResource); ok {
mhr = r
}
}
}
if mhr == nil {
return nil, errors.New("can't resolve MeshHTTPRoute policy")
}
default:
return policyWithTo.GetToList(), nil
}
rv := []core_model.PolicyItem{}
for _, mhrRules := range mhr.Spec.To {
for _, mhrRule := range mhrRules.Rules {
matchesHash := v1alpha1.HashMatches(mhrRule.Matches)
for _, to := range policyWithTo.GetToList() {
var targetRef common_api.TargetRef
switch mhrRules.TargetRef.Kind {
case common_api.Mesh, common_api.MeshSubset:
targetRef = common_api.TargetRef{
Kind: common_api.MeshSubset,
Tags: map[string]string{
RuleMatchesHashTag: matchesHash,
},
}
default:
targetRef = common_api.TargetRef{
Kind: common_api.MeshServiceSubset,
Name: mhrRules.TargetRef.Name,
Tags: map[string]string{
RuleMatchesHashTag: matchesHash,
},
}
}
rv = append(rv, &artificialPolicyItem{
targetRef: targetRef,
conf: to.GetDefault(),
})
}
}
}
return rv, nil
}

suggested changes

  1. 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).
  2. Deprecate MeshService and MeshHTTPRoute as a toplevel targetRef (top level targetRef should identify entire proxies) 1
  3. 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

xref

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).

Footnotes

  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.

@lahabana lahabana added triage/pending This issue will be looked at on the next triage meeting kind/feature New feature labels May 16, 2024
@jakubdyszkiewicz
Copy link
Contributor

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
Copy link
Contributor Author

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.

@lahabana lahabana added triage/accepted The issue was reviewed and is complete enough to start working on it and removed triage/pending This issue will be looked at on the next triage meeting labels Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature triage/accepted The issue was reviewed and is complete enough to start working on it
Projects
None yet
Development

No branches or pull requests

2 participants