add new test; remove unnecessary anchors

[kacejot]

Signed-off-by: Maxim Goncharenko goncharenko.maxim@apriorit.com

Related issue

Closes #1916

What type of PR is this

/kind cleanup

Proposed Changes

There was no issue, we just needed to modify policy to work correct.
Added logic that removes unnecessary conditional anchors from lists.
Added test that covers #1916 case.
Note that I removed unnecessary conditional anchor from #1916 reference manifest. It breaks the policy. We don't support nested anchors.

Proof Manifests

Here is corrected policy:

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: set-runasnonroot-true
spec:
  validationFailureAction: audit
  rules:
    - name: set-runasnonroot-true
      match:
        resources:
          kinds:
            - Pod
      mutate:
        patchStrategicMerge:
          spec:
            securityContext:
              runAsNonRoot: true
            initContainers:
              - (name): '*'
                securityContext:
                  runAsNonRoot: true
            containers:
              - (name): '*'
                securityContext:
                  runAsNonRoot: true

Resource:

apiVersion: apps/v1
kind: Deployment
metadata: 
  labels: 
    app: foo
  name: foo
spec: 
  replicas: 1
  selector: 
    matchLabels: 
      app: foo
  template: 
    metadata: 
      labels: 
        app: foo
    spec: 
      affinity: 
        podAntiAffinity: 
          requiredDuringSchedulingIgnoredDuringExecution: 
            - 
              labelSelector: 
                matchExpressions: 
                  - 
                    key: app
                    operator: In
                    values: 
                      - foo
                      - bar
              topologyKey: kubernetes.io/hostname
      containers: 
        - 
          command: 
            - sleep
            - "9999"
          image: "busybox:1.28"
          name: busybox
      initContainers: 
        - 
          command: 
            - sleep
            - "9999"
          image: "busybox:1.28"
          name: initbusy

Checklist

• I have read the contributing guidelines.
• I have added tests that prove my fix is effective or that my feature works.
• My PR contains new or altered behavior to Kyverno and
  • I have added or changed the documentation myself in an existing PR and the link is:
  • I have raised an issue in kyverno/website to track the doc update and the link is:
  • I have read the PR documentation guide and followed the process including adding proof manifests to this PR.

[realshuting]

Note that I removed unnecessary conditional anchor from #1916 reference manifest. It breaks the policy. We don't support nested anchors.

Do we need to update doc accordingly?

[kacejot]

@realshuting, if we have such policy in the doc, then sure

[realshuting]

@kacejot - can you please attach proof results with / without initContainers?

[chipzoller]

Does this policy also work if the resource does not have initContainers defined at all? That was one of the issues to begin with.

[realshuting]

@kacejot - any update?

[realshuting]

Ping @kacejot.

[kacejot]

Hi @realshuting, sure. I plan to look it just after flux issue.

[kacejot]

Switched to this task

[kacejot]

@realshuting @chipzoller
So I tested this branch with next resources:

1

policy:

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: set-runasnonroot-true
spec:
  rules:
    - name: set-runasnonroot-true
      match:
        resources:
          kinds:
          - Pod
      mutate:
        patchStrategicMerge:
          spec:
            securityContext:
              runAsNonRoot: true
            initContainers:
              - (name): "*"
                securityContext:
                  runAsNonRoot: true
            containers:
              - (name): "*"
                securityContext:
                  runAsNonRoot: true

resource:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: foo
  labels:
    app: foo
spec:
  replicas: 1
  selector:
    matchLabels:
      app: foo
  template:
    metadata:
      labels:
        app: foo
    spec:
      # initContainers:
      #   - name: initbusy
      #     image: busybox:1.28
      #     command: ["sleep", "9999"]
      containers:
      - image: busybox:1.28
        name: busybox
        command: ["sleep", "9999"]
      affinity:
        podAntiAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
          - labelSelector:
              matchExpressions:
              - key: app
                operator: In
                values:
                - foo
                - bar
            topologyKey: kubernetes.io/hostname

result:

apiVersion: v1
items:
- apiVersion: v1
  kind: Pod
  metadata:
    creationTimestamp: "2021-08-26T11:15:13Z"
    generateName: foo-57c4999cd6-
    labels:
      app: foo
      pod-template-hash: 57c4999cd6
    name: foo-57c4999cd6-rpdsd
    namespace: default
    ownerReferences:
    - apiVersion: apps/v1
      blockOwnerDeletion: true
      controller: true
      kind: ReplicaSet
      name: foo-57c4999cd6
      uid: 57c5ce19-e839-4fdc-b0a6-a9f4f3511576
    resourceVersion: "2824"
    uid: b25ada91-9fce-4d2f-bd24-76d55255650c
  spec:
    affinity:
      podAntiAffinity:
        requiredDuringSchedulingIgnoredDuringExecution:
        - labelSelector:
            matchExpressions:
            - key: app
              operator: In
              values:
              - foo
              - bar
          topologyKey: kubernetes.io/hostname
    containers:
    - command:
      - sleep
      - "9999"
      image: busybox:1.28
      imagePullPolicy: IfNotPresent
      name: busybox
      resources: {}
      securityContext:
        runAsNonRoot: true
      terminationMessagePath: /dev/termination-log
      terminationMessagePolicy: File
      volumeMounts:
      - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
        name: default-token-kb2wm
        readOnly: true
    dnsPolicy: ClusterFirst
    enableServiceLinks: true
    nodeName: minikube
    preemptionPolicy: PreemptLowerPriority
    priority: 0
    restartPolicy: Always
    schedulerName: default-scheduler
    securityContext:
      runAsNonRoot: true
    serviceAccount: default
    serviceAccountName: default
    terminationGracePeriodSeconds: 30
    tolerations:
    - effect: NoExecute
      key: node.kubernetes.io/not-ready
      operator: Exists
      tolerationSeconds: 300
    - effect: NoExecute
      key: node.kubernetes.io/unreachable
      operator: Exists
      tolerationSeconds: 300
    volumes:
    - name: default-token-kb2wm
      secret:
        defaultMode: 420
        secretName: default-token-kb2wm

As you can see, both runAsNonRoot fields are present

2

same policy
resource:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: foo
  labels:
    app: foo
spec:
  replicas: 1
  selector:
    matchLabels:
      app: foo
  template:
    metadata:
      labels:
        app: foo
    spec:
      initContainers:
      - name: initbusy
        image: busybox:1.28
        command: ["sleep", "9999"]
      containers:
      - image: busybox:1.28
        name: busybox
        command: ["sleep", "9999"]
      affinity:
        podAntiAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
          - labelSelector:
              matchExpressions:
              - key: app
                operator: In
                values:
                - foo
                - bar
            topologyKey: kubernetes.io/hostname

result:

...
   containers:
    - command:
      - sleep
      - "9999"
      image: busybox:1.28
      imagePullPolicy: IfNotPresent
      name: busybox
      resources: {}
      securityContext:
        runAsNonRoot: true
      terminationMessagePath: /dev/termination-log
      terminationMessagePolicy: File
      volumeMounts:
      - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
        name: default-token-kb2wm
        readOnly: true
    dnsPolicy: ClusterFirst
    enableServiceLinks: true
    initContainers:
    - command:
      - sleep
      - "9999"
      image: busybox:1.28
      imagePullPolicy: IfNotPresent
      name: initbusy
      resources: {}
      securityContext:
        runAsNonRoot: true
      terminationMessagePath: /dev/termination-log
      terminationMessagePolicy: File
      volumeMounts:
      - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
        name: default-token-kb2wm
        readOnly: true
    nodeName: minikube
    preemptionPolicy: PreemptLowerPriority
    priority: 0
    restartPolicy: Always
    schedulerName: default-scheduler
    securityContext:
      runAsNonRoot: true
    serviceAccount: default
...

Here we also have all mutations applied.

[chipzoller]

Ok, Max, looks good. So this clearly looks like user error on my part, but when you say

We don't support nested anchors.

what does that mean? Where in the original policy in #1916 were there "nested anchors"? I think my confusion came from the fact that in a validate policy, a user would have to wrap initContainers in a conditional or else validation would always fail if there weren't any. But in this mutate policy, that's not needed and the policy will still succeed if there aren't any initContainers.

And how can we turn these learnings into documentation? Maybe just use this as an example?

[kacejot]

@chipzoller, for now conditions in mutate and validate work the same way. We do not support nested anchors, because it is written so in the doc: https://kyverno.io/docs/writing-policies/mutate/#conditional-anchor (the last sentence from the chapter).

Nested anchors are present in #1916 description:

          spec:
            securityContext:
              runAsNonRoot: true
            # (initContainers):
            #   - (name): "*"
            #     securityContext:
            #       runAsNonRoot: true

As you can see (initContainers) has (name) condition nested inside itself.

I think my confusion came from the fact that in a validate policy, a user would have to wrap initContainers in a conditional or else validation would always fail if there weren't any.

Why there should be any fail? For example we have:

          spec:
            securityContext:
              runAsNonRoot: true
            initContainers:
            - (name): "*"
              securityContext:
                runAsNonRoot: true

You can read this as "For any initContainers element that has any name, set securityContext.runAsNonRoot to true"

[chipzoller]

There wouldn't be a failure in a mutation policy, but there would in a validate policy.

          spec:
            securityContext:
              runAsNonRoot: true
            initContainers:
            - (name): "*"
              securityContext:
                runAsNonRoot: true

If the match selects a resource and it doesn't have even one initContainer, validation will fail. That's why the validate rules need nested anchors so you can state, "IF there are any initContainers and IF any of those initContainers have XYZ...". I'm just saying that this is different behavior compared with a mutate policy.

[kacejot]

I think we could support nested anchors. In this case we would read this:

          spec:
            securityContext:
              runAsNonRoot: true
            (initContainers):
            - (name): "*"
              securityContext:
                runAsNonRoot: true

as:

For each element in `initContainers`:
  If element has any name than:
    Validate it with pattern
    If validation fails, then consider `(initContainers)` condition failed and skip the policy

The pattern from the algorithm:

            securityContext:
              runAsNonRoot: true

We have validation with the pattern here, because all the part:

            - (name): "*"
              securityContext:
                runAsNonRoot: true

is a value of (initContainers). And from the docs:
"Conditional () Use the tag and value as an “if” condition"

[kacejot]

@chipzoller, I see. So condition anchor still has different functionality in mutation and validation.
Condition anchor in validation still has two jobs to do: check existence and "if-then" statement

[kacejot]

I looked at the code and maybe it is already works as I described here:
#2217 (comment)

[chipzoller]

I looked at the code and maybe it is already works as I described here:
#2217 (comment)

If that's so, wouldn't this whole thing be a non-issue then?

[kacejot]

I think yes, but we need to check first.

[realshuting]

Can we add both test cases (as you attached in this comment)?

[realshuting]

I looked at the code and maybe it is already works as I described here:
#2217 (comment)

I think I'm lost here, is the conditional anchor works the same in mutate and validate rules?

[kacejot]

@realshuting, yes, they work the same. The only difference is if we have next pattern:

validate:
  parent:
    (condition): ''123*"

Then validation will fail if we don't have parent key in the resource.

In mutation the same policy:

strategicMergePatch:
  parent:
    (condition):"123*"

It will not add the parent, if condition failed.