TrafficPermissions can shadow one another if the match the same destination #2417

lahabana · 2021-07-22T13:19:09Z

@cpu601 pointed out that:

with 3 service a, b and c.
an existing policy:

apiVersion: kuma.io/v1alpha1
kind: TrafficPermission
mesh: default
metadata:
  name: allow-b-to-a
spec:
  sources:
    - match:
        kuma.io/service: b
  destinations:
    - match:
        kuma.io/service: a

add a policy:

apiVersion: kuma.io/v1alpha1
kind: TrafficPermission
mesh: default
metadata:
  name: allow-c-to-a
spec:
  sources:
    - match:
        kuma.io/service: c
  destinations:
    - match:
        kuma.io/service: a

The second will take over the first one resulting in service b not able to communicate with service a anymore.

This has 2 issues:

There's a massive risk of users shooting themselves in the foot (create a new policy that breaks existing connections when they seem unrelated)
Shouldn't it be possible to have multiple policies for the same service?

If the answer to 2 is no and we decide that only 1 destination matcher set (.i.e: the map at spec.destinations.match) should be possible we should introduce validation at creation/modification time to not process this policy at all and avoid the shadowing explained here.

The text was updated successfully, but these errors were encountered:

jpeach · 2021-07-22T22:05:48Z

An inbound gets exactly 1 best-match TrafficPermission policy applied to it. So you can't apply both of these policies to same destination service at once (since it's inbound can only have 1 policy).

lahabana · 2021-07-23T08:39:30Z

Right, just to reword my question and the idea behind this ticket:

Is really a limitation we want to keep?
If yes shouldn't the oldest permission have priority (rather than the newest like it is now) in order to avoid shooting yourself in the foot?
Could we add validation to reject policies that already have the same exact destination matcher?

I'm not opposed to the single destination restriction, it just seems we might be able to improve the UX

lobkovilya · 2021-07-23T09:17:50Z

Will this work?

apiVersion: kuma.io/v1alpha1
kind: TrafficPermission
mesh: default
metadata:
  name: allow-b-c-to-a
spec:
  sources:
    - match:
        kuma.io/service: b
    - match:
        kuma.io/service: c
  destinations:
    - match:
        kuma.io/service: a

Anyway, I think validation is a good idea. We have to provide more visibility of what policies were applied between Service A and Service B. Ideally it should be kumactl inspect --src=service-a --dst=service-b --policy=TrafficPermissionor something like this, but that's for the future

lahabana · 2021-07-29T12:13:28Z

This does work but the user that hit did point out that:

a) Creation of a new permission broke something existing
b) It looked like it worked but it didn't which can be confusing from the user

I think @bdecoste was especially worried about users breaking everything by accident.

jpeach · 2021-08-02T06:36:42Z

This does work but the user that hit did point out that:

a) Creation of a new permission broke something existing
b) It looked like it worked but it didn't which can be confusing from the user

Policies just float around in the API until they match something, and they can be pre-empted at anytime. Within that model it's hard for users to predict what will happen, so maybe we can give better tooling. For example

Given a YAML policy, show what objects it will affect before I create it
Posting a YAML resource (in a disabled state), then show what it matches before enabling

lahabana · 2021-12-14T09:38:39Z

#3330 will fix this

github-actions · 2022-01-14T08:01:10Z