kyverno / kyverno

Cloud Native Policy Management
https://kyverno.io
Apache License 2.0
5.74k stars 879 forks source link

Existence anchor for strategic merge patch #2371

Closed kacejot closed 2 months ago

kacejot commented 3 years ago

Is your feature request related to a problem? Please describe. I tried to make "add-safe-to-evict" policy work with global-anchor, but @realshuting found a case, when it doesn't work. Manifests:

Policy ```yaml apiVersion: kyverno.io/v1 kind: ClusterPolicy metadata: name: add-safe-to-evict annotations: policies.kyverno.io/category: Workload Management policies.kyverno.io/description: The Kubernetes cluster autoscaler does not evict pods that use hostPath or emptyDir volumes. To allow eviction of these pods, the annotation cluster-autoscaler.kubernetes.io/safe-to-evict=true must be added to the pods. spec: rules: - name: annotate-empty-dir match: resources: kinds: - Pod mutate: patchStrategicMerge: metadata: annotations: +(cluster-autoscaler.kubernetes.io/safe-to-evict): "true" spec: volumes: - <(emptyDir): {} - name: annotate-host-path match: resources: kinds: - Pod mutate: patchStrategicMerge: metadata: annotations: +(cluster-autoscaler.kubernetes.io/safe-to-evict): "true" spec: volumes: - hostPath: <(path): "*" ```
Resource ```yaml apiVersion: v1 kind: Pod metadata: name: static-web labels: role: myrole spec: containers: - name: web image: 1nginx ports: - name: web containerPort: 80 protocol: TCP volumeMounts: - mountPath: /cache name: cache-volume volumes: - emptyDir: {} name: cache-volume ```

As you can see, resource does have emptyDir volume named cache-volume, so the first rule from the policy should mutate it, because condition <(emptyDir): {} is satisfied. But when the pod is created, it also has another volume added by the cluster. Here is dumped resource JSON part:

...

"volumes":[
    {
        "emptyDir":{

        },
        "name":"cache-volume"
    },
    {
        "name":"default-token-6gplg",
        "secret":{
            "secretName":"default-token-6gplg"
        }
    }
]

...

In this case condition <(emptyDir): {} is not satisfied, because not all of the elements have emptyDir keyword inside.

I tried to fix it by making global anchor less strict - by skipping the elements in the resource that does not have keyword specified, but stop processing rule, if keyword exists and condition has failed. But in this case the first rule will be applied even if we don't have emptyDir at all. That is not how global anchor should work. I think current implementation matches the idea the most. For example, second rule is written in the correct way and does not have this problem.

Describe the solution you'd like Anyway we should check that emptyDir exists. I propose to add anchor for strategic merge patch. It will check that at least one array element has field present. If it fails in array element, all the policy is stopped being processed.

realshuting commented 3 years ago

So the global anchor validates all elements in the list and the rule will be applied only if all elements satisfy this rule?

kacejot commented 3 years ago

Yes, that is the idea of global anchor. If we want condition to use "any" logic and stop processing entire rule, we need some other anchor.

kacejot commented 3 years ago

Discussed with @realshuting. The behavior I have described in the issue body could be implemented by global anchor. I mean that global anchor with check if condition is satisfied for any list element, not for all. Working on it.

EDIT: anyway, don't close it. I will fix through the PR.