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

Rule merging alghoritm is not sorting lexographically on MeshTrafficPermission #8484

Closed
samm-git opened this issue Nov 28, 2023 · 6 comments · Fixed by #8705
Closed

Rule merging alghoritm is not sorting lexographically on MeshTrafficPermission #8484

samm-git opened this issue Nov 28, 2023 · 6 comments · Fixed by #8705
Assignees
Labels
kind/bug A bug triage/accepted The issue was reviewed and is complete enough to start working on it
Milestone

Comments

@samm-git
Copy link

What happened?

My intention is to create a MeshTrafficPermission set of rules that will allow connectivity from some services and ShadowDeny (or Deny) everything else.

To achieve that i did a 2 MeshTrafficPermission objects, both on MeshSubset targetRef level.

apiVersion: kuma.io/v1alpha1
kind: MeshTrafficPermission
metadata:
  labels:
    kuma.io/mesh: default
  name: default-demo-app
  namespace: kuma-system
spec:
  targetRef:
    kind: MeshSubset
    tags:
      k8s.kuma.io/service-name: demo-app
  from:
    - default:
        action: Allow
      targetRef:
        kind: MeshSubset
        tags:
          app.kubernetes.io/name: ui
    - default:
        action: Allow
      targetRef:
        kind: MeshSubset
        tags:
          app.kubernetes.io/name: service-order
    - default:
        action: Allow
      targetRef:
        kind: MeshSubset
        tags:
          app.kubernetes.io/name: service-payment
---

apiVersion: kuma.io/v1alpha1
kind: MeshTrafficPermission
metadata:
  labels:
    kuma.io/mesh: default
  name: default-demo-app-b
  namespace: kuma-system
spec:
  targetRef:
    kind: MeshSubset
    tags:
      k8s.kuma.io/service-name: demo-app
  from:
    - default:
        action: Deny
      targetRef:
        kind: Mesh

This setup works fine if naming is like myrule for allow set and myrule-a for denyset. I see in UI that rules are applied in correct order:
Screenshot 2023-11-28 at 15 54 03

My tests also shows that traffic works as expected

However, when I try to use names like rule-a and rule-b order is opposite to expected and tests also failing.

Screenshot 2023-11-28 at 15 56 25

I am not sure how sorting is working, but looks like that not according to doc which state that 2. At the same level we use lexicographic order on name.

@samm-git samm-git added kind/bug A bug triage/pending This issue will be looked at on the next triage meeting labels Nov 28, 2023
@lahabana
Copy link
Contributor

I indeed see the same problem by writing a simple test.

I you want I made it work this way with a single policy:

# The first entry in from is then overridden by the following ones
type: MeshTrafficPermission
mesh: default
name: default-demo-app
spec:
  targetRef:
    kind: MeshSubset
    tags:
      k8s.kuma.io/service-name: demo-app
  from:
    - default:
        action: Deny
      targetRef:
        kind: Mesh
    - default:
        action: Allow
      targetRef:
        kind: MeshSubset
        tags:
          app.kubernetes.io/name: ui
    - default:
        action: Allow
      targetRef:
        kind: MeshSubset
        tags:
          app.kubernetes.io/name: service-order
    - default:
        action: Allow
      targetRef:
        kind: MeshSubset
        tags:
          app.kubernetes.io/name: service-payment

Give me the rules:

Rules:
  127.0.0.1:80:
    - Conf:
        action: Allow
      Origin:
        - mesh: default
          name: default-demo-app
          type: MeshTrafficPermission
      Subset:
        - Key: app.kubernetes.io/name
          Not: false
          Value: ui
    - Conf:
        action: Allow
      Origin:
        - mesh: default
          name: default-demo-app
          type: MeshTrafficPermission
      Subset:
        - Key: app.kubernetes.io/name
          Not: false
          Value: service-payment
    - Conf:
        action: Allow
      Origin:
        - mesh: default
          name: default-demo-app
          type: MeshTrafficPermission
      Subset:
        - Key: app.kubernetes.io/name
          Not: false
          Value: service-order
    - Conf:
        action: Deny
      Origin:
        - mesh: default
          name: default-demo-app
          type: MeshTrafficPermission
      Subset:
        - Key: app.kubernetes.io/name
          Not: true
          Value: service-order
        - Key: app.kubernetes.io/name
          Not: true
          Value: service-payment
        - Key: app.kubernetes.io/name
          Not: true
          Value: ui

I think this is what you are trying to do: "Deny everyone except these 3 apps"

lahabana added a commit to lahabana/kuma that referenced this issue Nov 29, 2023
Add some tests including one as pending which is the repro

Fix kumahq#8484

Signed-off-by: Charly Molter <[email protected]>
@lobkovilya
Copy link
Contributor

I see what's the problem here https://go.dev/play/p/RvxA2xLrXo6, we have to check the order without .kuma-system suffixes

@samm-git
Copy link
Author

@lahabana thank you for the tip and solution, it works well. I feel that there is some inconsistency in the doc (or I just misunderstood something). I was thinking that rules inside one MeshTrafficPermission are applied one by one, and in the example you set, I see that the first rule is later partially overridden by the next rules.

Probably documentation is not entirely correct or I am looking in the wrong place. Could you please tell me where exactly logic is described?

@lahabana
Copy link
Contributor

It's possible that the docs are not super clear. The main docs for targetRef policies is: https://kuma.io/docs/2.5.x/policies/targetref/#merging-configuration

Do you think an example to explain what happens when there's multiple rules in a policy would help?

@samm-git
Copy link
Author

@lahabana yes, I think multiple rules in a policy would make it clearer. Also now I understand that rules inside the object are merged from first to last, and the resulting set is applied to traffic, but it was not that obvious from the beginning.

@lahabana
Copy link
Contributor

I see what's the problem here https://go.dev/play/p/RvxA2xLrXo6, we have to check the order without .kuma-system suffixes

Do you have any idea how we can work around this? Seems like the labels we've been talking about could be helpful here no?

lahabana added a commit that referenced this issue Nov 30, 2023
- Add some tests to the policy matching algo
- Add possibility to add a test as pending in test.EntriesForFolder

Ref #8484
@jakubdyszkiewicz jakubdyszkiewicz 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 Dec 4, 2023
@lahabana lahabana added this to the 2.6.x milestone Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug triage/accepted The issue was reviewed and is complete enough to start working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants