Skip to content

fix: update KeysAreMissing() to ignore negations in resource #8953

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 22, 2023
Merged

fix: update KeysAreMissing() to ignore negations in resource #8953

merged 5 commits into from
Nov 22, 2023

Conversation

vishal-chdhry
Copy link
Member

@vishal-chdhry vishal-chdhry commented Nov 19, 2023 •
edited
Loading

Explanation

The resource is not blocked when setting the failurePolicy to Ignore for a policy that was set to Enforce .

When testing using the provided proof manifest I found out that ac.KeysAreMissing() returns true here which results in the rule returning an error and not a failure. Due to this failurePolicy=Ignore consumes the error response and the resource is not blocked.

The resource should be blocked and ac.KeysAreMissing() should not return an error when it does not encounters an negation anchor key in resource.

Related issue

Closes: #8916

Milestone of this PR

/milestone 1.11.1

What type of PR is this

/kind bug

Proposed Changes

Proof Manifests

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: disallow-annotations
spec:
  background: true
  failurePolicy: Ignore
  rules:
  - match:
      all:
      - resources:
          kinds:
          - Pod
    name: disallow-annotations
    validate:
      message: One or more annotations is not allowed per the policies disallowed
        values list.
      pattern:
        metadata:
          =(annotations):
            =(kyverno-policies-test/key): '!disallowed'
            X(kyverno-policies-test/disallowed): ?*
  validationFailureAction: Enforce
  webhookTimeoutSeconds: 30

---
apiVersion: v1
kind: Pod
metadata:
  name: disallow-annotations-example
  namespace: default
  annotations:
    kyverno-policies-test/key: disallowed
spec:
  containers:
  - name: example
    image: busybox
    args: ["sleep", "infinity"]

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 release 1.11.1
  • My PR contains new or altered behavior to Kyverno and
    • CLI support should be added and my PR doesn't contain that functionality.
    • 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 documentation update and the link is:

Further Comments

@vishal-chdhry
fix: update KeysAreMissing() to ignore negations in resource
83d53ed 
KeysAreMissing() checks if a key is missing in a resource, since a negation should not be present in the resource, it should not count as a missing key

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
@vishal-chdhry
feat: add tests
3e3feac 
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
@vishal-chdhry vishal-chdhry added this to the Kyverno Release 1.11.1 milestone Nov 19, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 19, 2023 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f4b18b6) 33.60% compared to head (f9da9d7) 33.60%.

Files Patch % Lines
pkg/engine/anchor/anchormap.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8953      +/-   ##
==========================================
- Coverage   33.60%   33.60%   -0.01%     
==========================================
  Files         315      315              
  Lines       24951    24953       +2     
==========================================
  Hits         8386     8386              
- Misses      15768    15769       +1     
- Partials      797      798       +1

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

@vishal-chdhry
fix: pod is supposed to fail
9521b5b 
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
@realshuting realshuting enabled auto-merge (squash) November 21, 2023 09:29
@realshuting
Copy link
Member

/cherry-pick release-1.11

realshuting
realshuting approved these changes Nov 22, 2023
View reviewed changes
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.

May not be related directly to this fix, do we need a doc issue to clarify behaviors for the policy failure vs error ?

@realshuting
Copy link
Member

/cherry-pick release-1.11

@realshuting realshuting merged commit 72524c7 into kyverno:main Nov 22, 2023
gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Nov 22, 2023
@vishal-chdhry
fix: update KeysAreMissing() to ignore negations in resource (#8953)
d6719c9 
* fix: update KeysAreMissing() to ignore negations in resource

KeysAreMissing() checks if a key is missing in a resource, since a negation should not be present in the resource, it should not count as a missing key

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

* feat: add tests

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

* fix: pod is supposed to fail

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

---------

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
@gcp-cherry-pick-bot gcp-cherry-pick-bot bot mentioned this pull request Nov 22, 2023
Merged
@realshuting realshuting added the cherry-pick-completed The PR was cherry-picked (or merged) to required release branches label Nov 22, 2023
realshuting pushed a commit that referenced this pull request Nov 22, 2023
@gcp-cherry-pick-bot @vishal-chdhry
fix: update KeysAreMissing() to ignore negations in resource (#8953) (#…
c86039d 
…8982)

* fix: update KeysAreMissing() to ignore negations in resource

KeysAreMissing() checks if a key is missing in a resource, since a negation should not be present in the resource, it should not count as a missing key



* feat: add tests



* fix: pod is supposed to fail



---------

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
vishal-chdhry added a commit to vishal-chdhry/kyverno that referenced this pull request Jan 5, 2024
@vishal-chdhry
fix: update KeysAreMissing() to ignore negations in resource (kyverno…
52354b9 
…#8953)

* fix: update KeysAreMissing() to ignore negations in resource

KeysAreMissing() checks if a key is missing in a resource, since a negation should not be present in the resource, it should not count as a missing key

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

* feat: add tests

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

* fix: pod is supposed to fail

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

---------

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
vishal-chdhry added a commit to vishal-chdhry/kyverno that referenced this pull request Jan 5, 2024
@vishal-chdhry
fix: update KeysAreMissing() to ignore negations in resource (kyverno…
2489eb2 
…#8953)

* fix: update KeysAreMissing() to ignore negations in resource

KeysAreMissing() checks if a key is missing in a resource, since a negation should not be present in the resource, it should not count as a missing key

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

* feat: add tests

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

* fix: pod is supposed to fail

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

---------

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
vishal-chdhry added a commit to vishal-chdhry/kyverno that referenced this pull request Jan 6, 2024
@vishal-chdhry
fix: update KeysAreMissing() to ignore negations in resource (kyverno…
f9c7103 
…#8953)

* fix: update KeysAreMissing() to ignore negations in resource

KeysAreMissing() checks if a key is missing in a resource, since a negation should not be present in the resource, it should not count as a missing key

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

* feat: add tests

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

* fix: pod is supposed to fail

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>

---------

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@realshuting realshuting realshuting approved these changes

@chipzoller chipzoller Awaiting requested review from chipzoller

@eddycharly eddycharly Awaiting requested review from eddycharly eddycharly is a code owner

@MariamFahmy98 MariamFahmy98 Awaiting requested review from MariamFahmy98 MariamFahmy98 is a code owner

@MarcelMue MarcelMue Awaiting requested review from MarcelMue

Assignees
No one assigned
Labels
cherry-pick-completed The PR was cherry-picked (or merged) to required release branches cherry-pick-required milestone 1.12.0
Projects
None yet
Milestone
Kyverno Release 1.11.1
Development

Successfully merging this pull request may close these issues.

[Bug] failurePolicy of Ignore causes some invalid resources to be admitted
2 participants
@vishal-chdhry @realshuting