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

New operators #2543

Merged
merged 10 commits into from Oct 27, 2021
Merged

New operators #2543

merged 10 commits into from Oct 27, 2021

Conversation

anushkamittal20
Copy link
Contributor

@anushkamittal20 anushkamittal20 commented Oct 14, 2021

Related issue

Support AnyIn, AllIn, AnyNotIn, AllNotIn operators #1837
Closes #2813

Milestone of this PR

/milestone 1.6.0

What type of PR is this

/kind feature

Proposed Changes

Added 4 new operators

Proof Manifests

I have added tests to check these in the evaluate_test.go

pod.yaml

apiVersion: v1
kind: Pod
metadata:
  name: test
spec:
  initContainers:
  - name: jimmy
    image: defdasdabian:923
    command: ["/bin/sh", "-c", "sleep infinity"]
    securityContext:
      capabilities:
        drop:
        - XXXNET_RAWYYY
        - SETUID
  containers:
  - name: test
    image: defdasdabian:923
    command: ["/bin/sh", "-c", "sleep infinity"]
    securityContext:
      capabilities:
        drop:
        - XXXNET_RAWYYY
        - SETUID
        - CAP_FOO_BAR
  - name: asdf
    image: defdasdabian:923
    command: ["/bin/sh", "-c", "sleep infinity"]
    securityContext:
      capabilities:
        drop:
        - CAP_SOMETHING

We can apply the following policies to test the new operators

pol-anyin.yaml

  apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: drop-cap-net-raw
spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: drop-cap-net-raw
    match:
      resources:
        kinds:
        - Pod
    validate:
      deny:
        conditions:
          any:
          - key: "{{ request.object.spec.[containers, initContainers][].securityContext.capabilities.drop[] }}"
            operator: AnyIn
            value: ["SETUID","CAP_FOO_BAR"]

The request is denied as some values in key are present in value

pol-allin.yaml

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: drop-cap-net-raw
spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: drop-cap-net-raw
    match:
      resources:
        kinds:
        - Pod
    validate:
      deny:
        conditions:
          any:
          - key: "{{ request.object.spec.[containers, initContainers][].securityContext.capabilities.drop[] }}"
            operator: AllIn
            value: ["SETUID","CAP_FOO_BAR","XXXNET_RAWYYY"]

The Pod is not denied as All values of key arent present in value

pol-anynotin.yaml

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: drop-cap-net-raw
spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: drop-cap-net-raw
    match:
      resources:
        kinds:
        - Pod
    validate:
      deny:
        conditions:
          any:
          - key: "{{ request.object.spec.[containers, initContainers][].securityContext.capabilities.drop[] }}"
            operator: AnyNotIn
            value: ["SETUID","CAP_FOO_BAR","XXXNET_RAWYYY"]

The condition returns true as CAP_SOMETHING is not present in value and so the Pod is blocked.

pol-anynotin.yaml

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: drop-cap-net-raw
spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: drop-cap-net-raw
    match:
      resources:
        kinds:
        - Pod
    validate:
      deny:
        conditions:
          any:
          - key: "{{ request.object.spec.[containers, initContainers][].securityContext.capabilities.drop[] }}"
            operator: AllNotIn
            value: ["SETUID","CAP_FOO_BAR","XXXNET_RAWYYY"]

The pod is not blocked as the condition in this case returns false. If the condition was

 - key: "{{ request.object.spec.[containers, initContainers][].securityContext.capabilities.drop[] }}"
            operator: AllNotIn
            value: ["AABBCC"]

then the condition would return true as no value from key is present in value.

Checklist

Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
@chipzoller chipzoller added this to the Kyverno Release 1.6.0 milestone Oct 14, 2021
@chipzoller
Copy link
Member

@anushkamittal20 This is great to see you're adding support for these. At some point, please provide proof manifests which demonstrate their operation so it's in accordance with the proposal here or if it deviates at all.

@anushkamittal20
Copy link
Contributor Author

anushkamittal20 commented Oct 14, 2021

Hey @chipzoller I have taken reference from your comment here to build these operators. I will try to add some proof manifest for the same, I have added some test cases too, I will add more from the examples you have provided.
Thank you very much.

Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
Copy link
Member

@JimBugwadia JimBugwadia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anushkamittal20 - we also need to update the OpenAPIv3 schema and generate CRDs for these changes:

https://github.com/kyverno/kyverno/blob/main/pkg/api/kyverno/v1/policy_types.go#L211

@anushkamittal20
Copy link
Contributor Author

Thank you @JimBugwadia I will make the suggested changes.

Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
Copy link
Member

@realshuting realshuting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anushkamittal20 - a few comments regarding this PR:

  • please check and address e2e test failure
  • looks like there are additional changes in the manifests, can you merge latest main to sync up?
  • It would be good to attach the results for real policy application in a live cluster

pkg/api/kyverno/v1/policy_types.go Show resolved Hide resolved
pkg/engine/variables/operator/allin.go Outdated Show resolved Hide resolved
pkg/engine/variables/operator/allin.go Outdated Show resolved Hide resolved
pkg/engine/variables/evaluate_test.go Outdated Show resolved Hide resolved
@realshuting realshuting self-assigned this Oct 21, 2021
@realshuting realshuting added the milestone 1.5.1 PRs for 1.5.1 release label Oct 21, 2021
@realshuting realshuting removed this from the Kyverno Release 1.6.0 milestone Oct 21, 2021
@realshuting
Copy link
Member

Please use the label to track the milestone for a PR if the fixing issue is already included in a specific milestone.

Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
@chipzoller
Copy link
Member

Is this planned for 1.5.1 now? If so, need to update corresponding docs PR and issue to match.

@JimBugwadia
Copy link
Member

Is this planned for 1.5.1 now? If so, need to update corresponding docs PR and issue to match.

Typically, enhancements should go to the next minor release (1.6) and critical fixes should be selected for the next patch release. This creates a bit more maintainer work for patch releases, but allows us to move faster on main for features, etc. In some cases if there are major structural changes, and if critical fixes are required in that area, we may not be able to use cherry-pick, etc. but will need to manually make fixes in a patch release. Ideally we can target one minor release per quarter and one patch release per month. May be good to discuss all of this in the next maintainers meeting and document.

@chipzoller
Copy link
Member

I agree, this seems more appropriate for 1.6.0.

@realshuting
Copy link
Member

Sure. I'm currently using label milestone 1.5.1 to track all PRs. We can update the label once finalize it.

@realshuting realshuting added the milestone 1.5.2 PRs for 1.5.2release label Oct 26, 2021
@chipzoller
Copy link
Member

@JimBugwadia

@JimBugwadia
Copy link
Member

@anushkamittal20 - the intent is to support multiple types, as documented in the code comment:

// anysetExistsInArray checks if any key is a subset of value
// The value can be a string, an array of strings, or a JSON format
// array of strings (e.g. ["val1", "val2", "val3"].
// notIn argument if set to true will check for NotIn
func anySetExistsInArray(key []string, value interface{}, log logr.Logger, anyNotIn bool) (invalidType bool, keyExists bool) {

   

The line you referenced seems to be trying to handle the JSON Map as a string case, and line 84 above is for the single string case. However, there is a potential bug here - I think we should be comparing to value.(string) and not valuesAvailable (which is the type):

https://github.com/kyverno/kyverno/blob/main/pkg/engine/variables/operator/anyin.go#L82

Please add a unit test case for this.

@anushkamittal20
Copy link
Contributor Author

- key: ["orange","green","blue"]
            operator: AllNotIn
            value:
            - green

Hey @chipzoller I was going through this and I think this behaviour is right. AllNotIn will return true if all the values in key are not present in value, if any of the keys are present in value it returns false and so we see the request not denied. In our case one value green is present in value and so to returns true.
Please correct me if I have understood this wrong

@chipzoller
Copy link
Member

- key: ["orange","green","blue"]
            operator: AllNotIn
            value:
            - green

Hey @chipzoller I was going through this and I think this behaviour is right. AllNotIn will return true if all the values in key are not present in value, if any of the keys are present in value it returns false and so we see the request not denied. In our case one value green is present in value and so to returns true. Please correct me if I have understood this wrong

Hi @anushkamittal20, the scenario you're describing would be of AnyNotIn. If you have the collection ["orange","green","blue"] compared to a value green the question represented by this operator is, "are any of these not in the value?" The answer is "yes" because orange and blue are not equal to green. If, in the case of AllNotIn, the question becomes, "are ALL of the collection, take as one, ["orange","green","blue"] not in the value of green?" the answer is also "yes" because All means the three together as one unit. Any means any one of the three.

So, basically, any time there is a key with more than one string being compared in AllNotIn to a value of a single string, the answer will always be true because there's no possible way three different things can be in one individual thing.

@anushkamittal20
Copy link
Contributor Author

@chipzoller understood, so I should add some sort of count to check all present.
So, basically, any time there is a key with more than one string being compared in AllNotIn to a value of a single string, the answer will always be true because there's no possible way three different things can be in one individual thing. this is not true when there is some sort of wildcard value in value....

[]interface{}{"1.1.1.1", "3.3.3.3", "5.5.5.5"}, Operator: kyverno.AllNotIn, Value: []interface{}{"2*"}

here
I will try to make some changes here

@chipzoller
Copy link
Member

@chipzoller understood, so I should add some sort of count to check all present. So, basically, any time there is a key with more than one string being compared in AllNotIn to a value of a single string, the answer will always be true because there's no possible way three different things can be in one individual thing. this is not true when there is some sort of wildcard value in value....

[]interface{}{"1.1.1.1", "3.3.3.3", "5.5.5.5"}, Operator: kyverno.AllNotIn, Value: []interface{}{"2*"}

here I will try to make some changes here

Yes, wildcard in value is a bit of a different situation. In those case it may be possible for all the strings in the key to be found in the value if the value contains a glob which is common to ALL the entries in the key. Like this:

Many-to-one

Source: ["gray","green","grass"]
Destination: "gr*"
Scenario: Is source AllNotIn destination?
Response: false

compare that to this next one where the glob is different

Source: ["gray","green","grass"]
Destination: "gra*"
Scenario: Is source AllNotIn destination?
Response: true

@anushkamittal20
Copy link
Contributor Author

Understood @chipzoller
Thank you very much!

@anushkamittal20 anushkamittal20 mentioned this pull request Nov 9, 2021
2 tasks
@chipzoller
Copy link
Member

chipzoller commented Nov 12, 2021

@anushkamittal20 based on the latest tag after #2704 was committed, I don't see the wildcard case working properly. I'm not sure if this was supposed to be part of that PR or not. EDIT: I see that's covered separately here.

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: operator-test
spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: operator-test
    match:
      resources:
        kinds:
        - Pod
    validate:
      deny:
        conditions:
          any:
          - key: ["gray","green","grass"]
            operator: AllNotIn
            value: "gr*"

Submitting a Pod (regardless of its contents) should result in a pass because the destination value is a wildcard and all entries in the key begin with gr and so the condition is false. It seems that the standard string comparison now works as intended.

@chipzoller
Copy link
Member

@anushkamittal20 I am seeing that in a foreach loop, the problem outlined in this comment is still present. Use the image tags from this comment.

Test policy:

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: check-cap-add
spec:
  validationFailureAction: enforce
  background: false
  rules:
  # Checks initContainers to ensure they don't add anything other than "something".
  - name: check-cap-add-initcontainers
    match:
      resources:
        kinds:
        - Pod
    preconditions:
      all:
      - key: "{{request.operation}}"
        operator: In
        value:
        - CREATE
        - UPDATE
      # Check if initContainers even exist in the Pod
      - key: "{{ request.object.spec.initContainers[] | length(@) }}"
        operator: GreaterThanOrEquals
        value: 1
    validate:
      message: The only capability that may be explicitly added is `something`.
      foreach:
      - list: "request.object.spec.initContainers[].securityContext.capabilities"
        deny:
          conditions:
            any:
            # Loop over the `add[]` array in each container and deny if anything other than the "something" capability has been added.
            - key: "{{ element.add }}"
              operator: AnyNotIn
              value: something
  # Checks containers to ensure they don't add anything other than "something".
  - name: check-cap-add-containers
    match:
      resources:
        kinds:
        - Pod
    preconditions:
      all:
      - key: "{{request.operation}}"
        operator: In
        value:
        - CREATE
        - UPDATE
    validate:
      message: The only capability that may be explicitly added is `something`.
      foreach:
      - list: "request.object.spec.containers[].securityContext.capabilities"
        deny:
          conditions:
            any:
            # Loop over the `add[]` array in each container and deny if anything other than the "something" capability has been added.
            - key: "{{ element.add }}"
              operator: AnyNotIn
              value: something

Pod which should be allowed:

add-cap-goodpod01.yaml

# This is good because it adds only the "something" capability.
apiVersion: v1
kind: Pod
metadata:
  name: goodpod01
spec:
  containers:
  - name: busybox
    image: something:tag
    securityContext:
      capabilities:
        add:
        - something

See this is blocked.

$ k apply -f add-cap-goodpod01.yaml 
Error from server: error when creating "add-cap-goodpod01.yaml": admission webhook "validate.kyverno.svc-fail" denied the request: 

resource Pod/default/goodpod01 was blocked due to the following policies

check-cap-add:
  check-cap-add-containers: validation failed in foreach rule for The only capability
    that may be explicitly added is `something`.
  check-cap-add-initcontainers: preconditions not met

Change value: something in the policy to value: ["something"] and see the pod is now allowed.

$ k apply -f add-cap-goodpod01.yaml 
pod/goodpod01 created

This is a one-to-one comparison in this case, but the same problem is exhibited where the value should be specifiable as both a string and array/string.

@chipzoller
Copy link
Member

chipzoller commented Dec 30, 2021

@anushkamittal20 and @JimBugwadia I am still seeing the same results here in a foreach declaration using the latest 1.6 dev image and the AnyNotIn operator. I don't know if Jim's PR #2891 impacts this at all.

This does not work.

      foreach:
        - list: "request.object.spec.rules"
          deny:
            conditions:
              all:
              - key: "{{ request.object.spec.tls[].hosts[] }}"
                operator: AnyNotIn
                value: "{{ element.host }}"

This works.

      foreach:
        - list: "request.object.spec.rules"
          deny:
            conditions:
              all:
              - key: "{{ request.object.spec.tls[].hosts[] }}"
                operator: AnyNotIn
                value:
                - "{{ element.host }}"

@realshuting
Copy link
Member

Does {{ element.host }} return a list or a single value?

@anushkamittal20 - do we accept the list only for value?

@Danny-Wei
Copy link
Contributor

Danny-Wei commented Feb 17, 2022

- key: ["orange","green","blue"]
            operator: AllNotIn
            value:
            - green

Hey @chipzoller I was going through this and I think this behaviour is right. AllNotIn will return true if all the values in key are not present in value, if any of the keys are present in value it returns false and so we see the request not denied. In our case one value green is present in value and so to returns true. Please correct me if I have understood this wrong

Hi @anushkamittal20, the scenario you're describing would be of AnyNotIn. If you have the collection ["orange","green","blue"] compared to a value green the question represented by this operator is, "are any of these not in the value?" The answer is "yes" because orange and blue are not equal to green. If, in the case of AllNotIn, the question becomes, "are ALL of the collection, take as one, ["orange","green","blue"] not in the value of green?" the answer is also "yes" because All means the three together as one unit. Any means any one of the three.

So, basically, any time there is a key with more than one string being compared in AllNotIn to a value of a single string, the answer will always be true because there's no possible way three different things can be in one individual thing.

I want to implement one-to-many, many-to-many, and many-to-one conditional judgments with these operators in some conditions. But I find the logic of AllNotIn doesn't match what the documentation describes and what I thought.

"If, in the case of AllNotIn, the question becomes, "are ALL of the collection, take as one, ["orange","green","blue"]not in the value of green?" the answer is also "yes" because All means the three together as one unit. Any means any one of the three."

I think the situation you described here should use the NotIn operator instead.

In my opinion, the prefix Any of AnyNotIn should probably be used to represent any one item in the list, and the prefix All of AllNotIn should probably be used to represent all of the items in the list. So that, we can use AllNotIn to compare ["orange","green","blue"] with green (equal to ['green']). In this case, the answer is NO because the green item of ["orange","green","blue"] is in ["green"].

@chipzoller
Copy link
Member

But that's exactly how this works @Danny-Wei :

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: anynotin
spec:
  validationFailureAction: enforce
  rules:
  - name: anynotintest
    match:
      any:
      - resources:
          kinds:
          - Pod
    validate:
      message: "Denied"
      deny:
        conditions:
          any:
          - key:
            - orange
            - green
            - blue
            operator: AllNotIn
            value: green
apiVersion: v1
kind: Pod
metadata:
  annotations:
    annotation.corp.com/restrict1: foo1
  labels:
    app: busybox
  name: mypod
spec:
  schedulerName: nothing
  containers:
  - name: busybox
    image: nothingtoseehere:latest
    args:
    - sleep
    - "9999"
$ k apply -f pod.yaml 
Error from server: error when creating "pod.yaml": admission webhook "validate.kyverno.svc-fail" denied the request: 

resource Pod/default/mypod was blocked due to the following policies

anynotin:
  anynotintest: Denied

@Danny-Wei
Copy link
Contributor

Danny-Wei commented Feb 17, 2022

If we define these operators as follows (Note: the key and value are treated as lists)

  • key In value
    the key is a subset of value, like the keyword in in Python

  • key NotIn value
    the key is not a subset of value, like the keywords not in in Python

  • key AnyIn value
    any item in the key is IN the value

  • key AnyNotIn value
    any item in the key is NOT IN the value

  • key AllIn value
    all items in the key are IN the value, same as In operator

  • key AllNotIn value
    all items in the key are NOT IN the value (no common elements in key and value)

, then we can achieve your use case in the following way

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: anynotin
spec:
  validationFailureAction: enforce
  rules:
  - name: anynotintest
    match:
      any:
      - resources:
          kinds:
          - Pod
    validate:
      message: "Denied"
      deny:
        conditions:
          any:
          - key:
            - orange
            - green
            - blue
            operator: NotIn
            value: green

@chipzoller
Copy link
Member

In and NotIn are deprecated and will be removed in a future version. Any of that (plus more) functionality is available in one of the other four operators.

@Danny-Wei
Copy link
Contributor

I see, I found the logic of AllNotIn doesn't match what the documentation describes and what I thought.
It seems like I can't use AllNotIn to check if key and value have common elements.

@chipzoller
Copy link
Member

I see what you mean. You're referring to this statement:

AllNotIn checks if all of the strings part of the key is not in the value set (i.e. no common elements in key and value)

The parenthetical clause isn't correct here. AllNotIn checks and ensures that ALL of the source set are not found in the destination. It is not a check to see that there are no common elements. That would be equivalent to AnyNotIn. I will correct the documentation now.

@Danny-Wei
Copy link
Contributor

It seems that I must split the condition if I want to check "all items in the key are NOT IN the value", just like as follows

deny:
  conditions:
    all:
    - key: NET_RAW
      operator: AnyNotIn
      value: "{{ element.securityContext.capabilities.drop || [''] }}"
    - key: net_raw
      operator: AnyNotIn
      value: "{{ element.securityContext.capabilities.drop || [''] }}"

@chipzoller
Copy link
Member

chipzoller commented Feb 17, 2022

- key: ["NET_RAW","net_raw"]
  operator: AnyNotIn
  value: "{{ element.securityContext.capabilities.drop || [''] }}"

@Danny-Wei
Copy link
Contributor

I already tried it but the result is not as expected. I'll look into it tomorrow.

@chipzoller
Copy link
Member

This is what you should use:

- key: NET_RAW
  operator: AnyNotIn
  value: "{{ element.securityContext.capabilities.drop[].to_upper(@) || [''] }}"

@Danny-Wei
Copy link
Contributor

Thanks. It works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Wildcards are ignored when occuring in lists/arrays
5 participants