Policy generated wrong rules

[Icarus9913]

What happened?

Kuma Version

2.9.2

What happened?

When I went through the BuildRules function, I found the generated rules might be wrong.

How to reproduce?

1. Create single zone with kuma 2.9.2
2. Enable mTLS

apiVersion: kuma.io/v1alpha1
kind: Mesh
metadata:
  name: default
spec:
  mtls:
    enabledBackend: ca-1
    backends:
    - name: ca-1
      type: builtin

4. Create 3 applications with sidecar injected:

apiVersion: v1
kind: Namespace
metadata:
  labels:
    kuma.io/sidecar-injection: enabled
  name: kuma-demo
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx-1
  namespace: kuma-demo
spec:
  replicas: 1
  selector:
    matchLabels:
      app: nginx-1
  template:
    metadata:
      labels:
        app: nginx-1
    spec:
      containers:
      - name: nginx-1
        image: nginx:stable
        ports:
          - containerPort: 80
            name: http-web-svc
---
apiVersion: v1
kind: Service
metadata:
  name: nginx-service-1
  namespace: kuma-demo
spec:
  selector:
    app: nginx-1
  ports:
  - name: name-of-service-port
    appProtocol: http
    protocol: TCP
    port: 80
    targetPort: http-web-svc
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx-2
  namespace: kuma-demo
spec:
  replicas: 1
  selector:
    matchLabels:
      app: nginx-2
  template:
    metadata:
      labels:
        app: nginx-2
    spec:
      containers:
      - name: nginx-2
        image: nginx:stable
        ports:
          - containerPort: 80
            name: http-web-svc
---
apiVersion: v1
kind: Service
metadata:
  name: nginx-service-2
  namespace: kuma-demo
spec:
  selector:
    app: nginx-2
  ports:
  - name: name-of-service-port
    appProtocol: http
    protocol: TCP
    port: 80
    targetPort: http-web-svc
---
apiVersion: v1
kind: Service
metadata:
  name: httpbin-1
  namespace: kuma-demo
  labels:
    app: httpbin-1
    service: httpbin-1
spec:
  ports:
  - name: http
    port: 8000
    targetPort: 8080
    protocol: TCP
    appProtocol: http
  selector:
    app: httpbin-1
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: httpbin-1
  namespace: kuma-demo
spec:
  replicas: 1
  selector:
    matchLabels:
      app: httpbin-1
      version: v1
  template:
    metadata:
      labels:
        app: httpbin-1
        version: v1
    spec:
      containers:
      - image: docker.io/kong/httpbin:0.1.0
        imagePullPolicy: IfNotPresent
        name: httpbin
        command:
        - gunicorn
        - -b
        - 0.0.0.0:8080
        - httpbin:app
        - -k
        - gevent
        env:
        - name: WORKON_HOME
          value: /tmp
        ports:
        - containerPort: 8080

5. Create MeshTrafficPermission (This is the only one MTP policy)

apiVersion: kuma.io/v1alpha1
kind: MeshTrafficPermission
metadata:
  name: mtp
  namespace: kuma-system
  labels:
    kuma.io/mesh: default
spec:
  targetRef:
    kind: Mesh
  from:
  - targetRef:
      kind: MeshSubset
      tags:
        k8s.kuma.io/service-name: nginx-service-1
    default:
      action: Deny
  - targetRef:
      kind: Mesh
    default:
      action: Allow

6. Then you can find both nginx-1 and nginx-2 can request httpbin-1_kuma-demo_svc_8000.mesh

Additional Context

I tried to debug the BuildRules function and found the above MeshTrafficPermission policy would generate the following rules:

    - BackendRefOriginIndex: {}
      Conf:
        action: Allow
      Subset:
        - Key: k8s.kuma.io/service-name
          Not: false
          Value: nginx-service-1
    - BackendRefOriginIndex: {}
      Conf:
        action: Allow
      Subset:
        - Key: k8s.kuma.io/service-name
          Not: true
          Value: nginx-service-1

Actually, I think the first Subset {Key: k8s.kuma.io/service-name, Not: false, Value: nginx-service-1 } should own the action Deny right?

[Icarus9913]

An easy way to reproduce it, we can use the unit test

kuma/pkg/plugins/policies/core/rules/rules_test.go

Line 306 in 8659306

Describe("BuildRules", func() {

And create a input.yaml file in /kuma/pkg/plugins/policies/core/rules/testdata/rules/from/ folder with the following content:

type: MeshTrafficPermission
mesh: default
name: mtp-mix
spec:
  targetRef:
    kind: Mesh
  from:
    - targetRef:
        kind: MeshSubset
        tags:
          k8s.kuma.io/service-name: nginx-service-1
      default:
        action: Deny
    - targetRef:
        kind: Mesh
      default:
        action: Allow

[lobkovilya]

It's actually by design, that the order of items in from matters and from[1] has more priority than from[0]. We're going to change that with the new design, but for now you can flip the order and it should work as you expect:

type: MeshTrafficPermission
mesh: default
name: mtp-mix
spec:
  targetRef:
    kind: Mesh
  from:
    - targetRef:
        kind: Mesh
      default:
        action: Allow
    - targetRef:
        kind: MeshSubset
        tags:
          k8s.kuma.io/service-name: nginx-service-1
      default:
        action: Deny

[Icarus9913]

We define a total order of policy priority:

MeshServiceSubset > MeshService > MeshSubset > Mesh (the more a targetRef is focused the higher priority it has)
If levels are equal the lexicographic order of policy names is used

From the doc, isn't that the MeshSubset has higher priority than Mesh?

[Icarus9913]

If we can use new design to replace it, that would be good.

[lobkovilya]

We define a total order of policy priority:

MeshServiceSubset > MeshService > MeshSubset > Mesh (the more a targetRef is focused the higher priority it has)
If levels are equal the lexicographic order of policy names is used

From the doc, isn't that the MeshSubset has higher priority than Mesh?

It applies to the order that's used when multiple policies are merged, i.e. if you'd have mtp-1, mtp-2, etc. then we order them based on spec.targetRef according to the MeshServiceSubset > MeshService > MeshSubset > Mesh

[Icarus9913]

Close it as it's designed