[Bug] Enabling many-to-one comparisons for `AnyNotIn` operator #9462

1Shubham7 · 2024-01-20T14:26:44Z

Explanation

The AnyNotIn operator can't evaluate many-to-one comparisons (e.g. where key is array of int and value is int) for any datatype. This causes the rule in the webhook work differently from the CLI.

Related issue

Fixes #9319

Milestone of this PR

Documentation (optional)

My PR contains new or altered behavior to Kyverno.

I have sent the draft PR to add or update the documentation and the link is:

What type of PR is this

/kind bug

Proposed Changes

I have added support for these many-to-one comparisons for all datatypes.

Proof Manifests

Before

Step 1. The example Policy below uses AnyNotIn operator and has a key of [0,1,2] and value is 0, so obviously it should evaluate to true, but since AnyNotIn as of now doesn't support comparison when key is array of int and partially in value, the default implementation is false.

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: test-policy
spec:
  validationFailureAction: Enforce
  background: false
  rules:
    - name: test-rule
      match:
        any:
        - resources:
            kinds:
              - ConfigMap
            names:
              - kyvernotest
            operations:
              - CREATE
      validate:
        message: Must have all.
        deny:
          conditions:
            all:
            - key:
              - 0
              - 1
              - 2
              operator: AnyNotIn
              value: 0

Step 2. So after applying that policy, when we create this cm

kubectl create cm kyvernotest --from-literal=foo=bar

Step 3. It allows the Configmap to be created.

configmap/kyvernotest created

After

After implementing these changes, AnyNotIn checks if the key of type []int is not included in value which is true and the cm is blocked. So when we now apply the policy and create the configmap

kubectl create cm kyvernotest --from-literal=foo=bar

It blocks the cm, and the result is

error: failed to create configmap: admission webhook "validate.kyverno.svc-fail" denied the request: 

resource ConfigMap/default/kyvernotest was blocked due to the following policies 

test-policy:
  test-rule: Must have all.

Checklist

I have read the contributing guidelines.
I have read the PR documentation guide and followed the process including adding proof manifests to this PR.
This is a bug fix and I have added unit tests that prove my fix is effective.
This is a feature and I have added CLI tests that are applicable.
My PR needs to be cherry picked to a specific release branch which is .
My PR contains new or altered behavior to Kyverno and
- CLI support should be added and my PR doesn't contain that functionality.

Further Comments

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

codecov · 2024-01-20T14:31:35Z

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 10.12%. Comparing base (022fc4f) to head (62c6b18).

Files	Patch %	Lines
pkg/engine/variables/operator/anyin.go	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9462      +/-   ##
==========================================
+ Coverage   10.11%   10.12%   +0.01%     
==========================================
  Files        1030     1030              
  Lines       91757    91763       +6     
==========================================
+ Hits         9285     9295      +10     
+ Misses      81455    81452       -3     
+ Partials     1017     1016       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chipzoller · 2024-01-27T12:48:45Z

Please add unit tests which cover any missing scenarios for this operator in your PR.

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

1Shubham7 · 2024-01-27T14:02:51Z

@chipzoller, I have added tests for scenarios like:

key is float
key is bool
key is array of int
key is array of float
key is array of bool
this PR deals with last three scenarios.

chipzoller · 2024-01-27T14:12:52Z

Please run go fmt

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

1Shubham7 · 2024-01-29T08:02:25Z

@chipzoller, please have a look.

chipzoller · 2024-01-29T13:56:18Z

You don't need to keep mentioning users upon every comment. This triggers a push notification every time is makes unnecessary noise when we already subscribe to these PRs/issues.

chipzoller · 2024-01-29T13:57:16Z

@eddycharly or @realshuting: can you please ensure this is correct and we have 100% coverage on unit tests for all combinations and all types of supported values?

realshuting · 2024-02-07T15:50:56Z

Please explain the changes in full so the reviewer understands what has been fixed.

The unit tests need to be fixed.

chipzoller · 2024-02-07T15:53:03Z

PR template is also not complete.

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

1Shubham7 · 2024-02-16T13:17:12Z

PR template is also not complete.

sorry for that, now its done.

chipzoller

Will this also cover the same behavior in the value as well as the key? All of these operators need to work a similar way for both key and value fields.

1Shubham7 · 2024-04-08T11:23:15Z

Will this also cover the same behavior in the value as well as the key? All of these operators need to work a similar way for both key and value fields.

no, that will defy the current logic i.e. "AnyNotIn checks that ANY of the keys are NOT found in the values." . See if key = [0,1,2] and value = 0 then since 1,2 are not in value, AnyNotIn is true. but when key = 0 and value =[0,1,2] now 0 is in value therefore false.

realshuting · 2024-04-17T08:51:02Z

Do we need to clarify this new behavior in our doc?

pkg/engine/variables/operator/anyin.go

1Shubham7 · 2024-04-17T10:12:28Z

Do we need to clarify this new behavior in our doc?

no, this is not new behavior, it is just a bug that defies existing behavior. Docs already says this - AnyNotIn: checks that ANY of the keys are NOT found in the values. link

realshuting

/lgtm
Thanks @1Shubham7.

MariamFahmy98 · 2024-04-22T11:43:10Z

@1Shubham7 - Could you please resolve all the comments? So that we can merge it.

1Shubham7 · 2024-04-22T11:51:31Z

@1Shubham7 - Could you please resolve all the comments? So that we can merge it.

Hey Mariam, all the comments are already resolved.

…no#9462) * added cases for int, float Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com> * added bool as well Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com> * added tests Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com> * some more tests Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com> * go fmt Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com> * fixed the failing test cases Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com> --------- Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com> Co-authored-by: shuting <shuting@nirmata.com>

added cases for int, float

01a4059

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

1Shubham7 added 2 commits January 22, 2024 19:46

Merge branch 'kyverno:main' into AnyNotIn

997faf8

Merge branch 'kyverno:main' into AnyNotIn

dfb2e57

1Shubham7 marked this pull request as ready for review January 27, 2024 12:44

1Shubham7 requested review from MarcelMue, realshuting and MariamFahmy98 as code owners January 27, 2024 12:44

Merge branch 'main' into AnyNotIn

5ade256

1Shubham7 added 2 commits January 27, 2024 19:13

added bool as well

8a2e744

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

added tests

2825f6e

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

1Shubham7 changed the title ~~[Bug] Enabling many-to-one comparisons in AnyNotIn (and other operators)~~ [Bug] Enabling many-to-one comparisons for AnyNotIn operator Jan 27, 2024

1Shubham7 added 2 commits January 27, 2024 19:26

some more tests

353469e

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

Merge branch 'main' into AnyNotIn

06cb7a7

1Shubham7 added 2 commits January 27, 2024 19:50

go fmt

07189ba

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

Merge branch 'main' into AnyNotIn

513e4c7

chipzoller requested a review from eddycharly January 29, 2024 13:56

realshuting added the milestone 1.13.0 label Feb 1, 2024

1Shubham7 added 4 commits February 14, 2024 22:20

Merge branch 'main' into AnyNotIn

330f6e7

fixed the failing test cases

0de9d8d

Signed-off-by: Shubham Singh <shubhammahar1306@gmail.com>

Merge branch 'main' into AnyNotIn

815e0c2

Merge branch 'main' into AnyNotIn

0895245

1Shubham7 changed the title ~~[Bug] Enabling many-to-one comparisons for AnyNotIn operator~~ [Bug] Enabling many-to-one comparisons for AnyNotIn operator Feb 19, 2024

Merge branch 'main' into AnyNotIn

751b978

1Shubham7 requested a review from KhaledEmaraDev as a code owner April 6, 2024 06:41

chipzoller reviewed Apr 6, 2024

View reviewed changes

realshuting reviewed Apr 17, 2024

View reviewed changes

pkg/engine/variables/operator/anyin.go Show resolved Hide resolved

Merge branch 'main' into AnyNotIn

5275b71

realshuting approved these changes Apr 18, 2024

View reviewed changes

realshuting enabled auto-merge (squash) April 18, 2024 10:02

MariamFahmy98 added this to the Kyverno Release 1.13.0 milestone Apr 22, 2024

Merge branch 'main' into AnyNotIn

62c6b18

realshuting merged commit dbc12ac into kyverno:main Apr 22, 2024
248 of 250 checks passed

1Shubham7 mentioned this pull request Apr 30, 2024

adding myself (Shubham Singh) to CONTRIBUTORS.md #10149

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Enabling many-to-one comparisons for `AnyNotIn` operator #9462

[Bug] Enabling many-to-one comparisons for `AnyNotIn` operator #9462

[Bug] Enabling many-to-one comparisons for AnyNotIn operator #9462

[Bug] Enabling many-to-one comparisons for AnyNotIn operator #9462

Conversation

Explanation

Related issue

Milestone of this PR

Documentation (optional)

What type of PR is this

Proposed Changes

Proof Manifests

Before

After

Checklist

Further Comments

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Bug] Enabling many-to-one comparisons for `AnyNotIn` operator #9462

[Bug] Enabling many-to-one comparisons for `AnyNotIn` operator #9462