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

fix: deferred loader panic when mutate and generate policies are applied #9935

Merged
merged 14 commits into from
Mar 29, 2024

Conversation

vishal-chdhry
Copy link
Member

@vishal-chdhry vishal-chdhry commented Mar 23, 2024

Explanation

Previously mutate and generate rules were sharing the same policy context and were operating concurrently. This causes a race condition problem where if a checkpoint is created by either of the handlers too soon then a deferred loader creates a loader entry at the wrong level. This causes the deferred loader being deleted too soon and causes a panic in some scenario.

Related issue

Closes #9413

Milestone of this PR

Documentation (required for features)

My PR contains new or altered behavior to Kyverno.

What type of PR is this

Proposed Changes

Deep copy the policy context before passing it to generate and mutate handlers

Proof Manifests

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: Vishal Choudhary <vishal.choudhary@nirmata.com>
Copy link

codecov bot commented Mar 23, 2024

Codecov Report

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

Project coverage is 33.65%. Comparing base (1a19540) to head (17ad807).

❗ Current head 17ad807 differs from pull request most recent head 08b588a. Consider uploading reports for the commit 08b588a to get more accurate results

Files Patch % Lines
pkg/engine/context/mock_deferred.go 66.66% 10 Missing and 4 partials ⚠️
pkg/webhooks/resource/updaterequest.go 55.00% 5 Missing and 4 partials ⚠️
pkg/webhooks/resource/handlers.go 69.23% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9935      +/-   ##
==========================================
+ Coverage   33.58%   33.65%   +0.06%     
==========================================
  Files         346      347       +1     
  Lines       23682    23740      +58     
==========================================
+ Hits         7954     7990      +36     
- Misses      14853    14868      +15     
- Partials      875      882       +7     

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

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
pkg/engine/context/context.go Outdated Show resolved Hide resolved
pkg/engine/context/context.go Outdated Show resolved Hide resolved
realshuting
realshuting previously approved these changes Mar 27, 2024
@realshuting realshuting enabled auto-merge (squash) March 27, 2024 10:07
@realshuting
Copy link
Member

/cherry-pick release-1.12

@github-actions github-actions bot added the lgtm label Mar 27, 2024
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.

I am thinking it may be best to pass in the context factory here for the mutate and generate rule invocations:

go h.handleBackgroundApplies(ctx, logger, request.AdmissionRequest, policyContext, generatePolicies, mutatePolicies, startTime)

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
realshuting
realshuting previously approved these changes Mar 28, 2024
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.

👍

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
JimBugwadia
JimBugwadia previously approved these changes Mar 29, 2024
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
@realshuting realshuting merged commit 93eac3f into kyverno:main Mar 29, 2024
246 of 247 checks passed
gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Mar 29, 2024
…ied (#9935)

* fix: deferred loader panic when mutate and generate policies are applied

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

* fix: tests

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

* fix: update policies

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

* remove clusterrolebinding

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

* fix: copy only json context

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

* fix: polctx

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

---------

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
@realshuting realshuting added cherry-pick-required cherry-pick-completed The PR was cherry-picked (or merged) to required release branches labels Mar 29, 2024
realshuting added a commit that referenced this pull request Mar 29, 2024
…ied (#9935) (#9968)

* fix: deferred loader panic when mutate and generate policies are applied



* fix: tests



* fix: update policies



* remove clusterrolebinding



* fix: copy only json context



* fix: polctx



---------

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-completed The PR was cherry-picked (or merged) to required release branches cherry-pick-required lgtm milestone 1.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] panic: runtime error using precondition on mutate and generate rules at the same time
3 participants