feat: add exception logic #5712

Eileen-Yu · 2022-12-16T22:34:31Z

Signed-off-by: Eileen Yu eileenylj@gmail.com

Explanation

Add policy exception logic to:

mutate
generate
imageVerify

Related issue

#2627

Milestone of this PR

What type of PR is this

/kind feature

Proposed Changes

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.
- 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

codecov · 2022-12-16T22:44:08Z

Codecov Report

Merging #5712 (bc2ec6b) into main (14445bf) will decrease coverage by 0.00%.
The diff coverage is 23.91%.

@@            Coverage Diff             @@
##             main    #5712      +/-   ##
==========================================
- Coverage   34.62%   34.62%   -0.01%     
==========================================
  Files         190      190              
  Lines       21080    21106      +26     
==========================================
+ Hits         7300     7307       +7     
- Misses      12970    12986      +16     
- Partials      810      813       +3

Impacted Files	Coverage Δ
pkg/engine/background.go	`0.00% <0.00%> (ø)`
pkg/engine/imageVerify.go	`69.52% <20.00%> (-0.56%)`	⬇️
pkg/engine/validation.go	`60.81% <25.00%> (-0.36%)`	⬇️
pkg/engine/mutation.go	`64.59% <50.00%> (-0.57%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

eddycharly · 2022-12-19T15:26:52Z

Looks good to me.
@JimBugwadia ?

JimBugwadia

looks good! a few minor comments for improvements.

JimBugwadia · 2022-12-20T17:20:28Z

pkg/engine/background.go

+	if err == nil && exception != nil {
+		key, err := cache.MetaNamespaceKeyFunc(exception)
+		logger := logging.WithName("exception")
+		// TODO: increase metrics


what does those comment mean? do we need to increment the rule metrics or something else?

This is recommended by @eddycharly .
I assume this is to instrument the triggered exception count.

cc: @eddycharly Pls fix me if I'm understanding wrong, or anything you'd like to point out.

JimBugwadia · 2022-12-20T17:22:00Z

pkg/engine/background.go

+	// check if there is a corresponding policy exception
+	exception, err := matchesException(policyContext, &rule)
+	// if we found an exception
+	if err == nil && exception != nil {


can we make the handling of the exception reusable for all the rule types? Currently we are duplicating the same 15-20 lines of code in each.

Yeah, that sounds good. So I extracted the logic to a separate function shouldRespondException. Will that be good?

JimBugwadia · 2022-12-20T17:22:29Z

pkg/engine/background.go

+		logger := logging.WithName("exception")
+		// TODO: increase metrics
+		if err != nil {
+			logger.Error(err, "failed to compute policy exception key", "namespace", exception.GetNamespace(), "name", exception.GetName())


We need to report a rule error in this case.

Sounds good! Updated with a RuleResponse returned( assume:Status: response.RuleStatusError). Will that be good?

JimBugwadia · 2022-12-20T17:24:24Z

pkg/engine/background.go

+			logger.V(3).Info("policy rule skipped due to policy exception", "exception", key)
+			return &response.RuleResponse{
+				Name:    rule.Name,
+				Message: "Rule skipped because of PolicyException" + key,


Can we make the text lower case? `rule skipped due to policy exception " + key

Yeah, sounds good!

JimBugwadia · 2022-12-20T20:09:54Z

pkg/engine/validation.go

+			log.Error(err, "failed to compute policy exception key", "namespace", exception.GetNamespace(), "name", exception.GetName())
+			return &response.RuleResponse{
+				Name:    rule.Name,
+				Message: "failed to find matched exception" + key,


needs a space after "...exception" before we + key

Thx for letting me know 👍

JimBugwadia · 2022-12-20T20:10:16Z

pkg/engine/validation.go

+	// if we found an exception
+	if err == nil && exception != nil {
+		key, err := cache.MetaNamespaceKeyFunc(exception)
+		// TODO: increase metrics


remove as we are returning an error which counts in the metrics

JimBugwadia · 2022-12-20T20:10:33Z

pkg/engine/validation.go

@@ -800,3 +788,29 @@ func matchesException(policyContext *PolicyContext, rule *kyvernov1.Rule) (*kyve
 	}
 	return nil, nil
 }
+
+func shouldRespondException(ctx *PolicyContext, rule *kyvernov1.Rule, log logr.Logger) *response.RuleResponse {


rename to hasPolicyExceptions

JimBugwadia · 2022-12-20T20:11:07Z

pkg/engine/validation.go

+		log.V(3).Info("policy rule skipped due to policy exception", "exception", key)
+		return &response.RuleResponse{
+			Name:    rule.Name,
+			Message: "rule skipped due to policy exception" + key,


missing space after "....exception"

JimBugwadia · 2022-12-20T20:12:41Z

pkg/engine/validation.go

@@ -800,3 +788,29 @@ func matchesException(policyContext *PolicyContext, rule *kyvernov1.Rule) (*kyve
 	}
 	return nil, nil
 }
+
+func shouldRespondException(ctx *PolicyContext, rule *kyvernov1.Rule, log logr.Logger) *response.RuleResponse {


change return to (bool, *response.RuleResponse)

If it returns true then we process the response. Otherwise, we expect a nil response.

As discussed in channel, comment has been added to make it clear.

JimBugwadia

minor comments, please check!

Signed-off-by: Eileen Yu <eileenylj@gmail.com>

Signed-off-by: Eileen Yu <eileenylj@gmail.com> Signed-off-by: Eileen Yu <eileenylj@gmail.com> Co-authored-by: Jim Bugwadia <jim@nirmata.com> Signed-off-by: Md Sahil <Mohdssahil1@gmail.com>

Signed-off-by: Eileen Yu <eileenylj@gmail.com> Signed-off-by: Eileen Yu <eileenylj@gmail.com> Co-authored-by: Jim Bugwadia <jim@nirmata.com> Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>

Eileen-Yu requested a review from prateekpandey14 as a code owner December 16, 2022 22:34

Eileen-Yu mentioned this pull request Dec 19, 2022

feat: add new api PolicyException #5054

Closed

20 tasks

eddycharly added the milestone 1.9.0 label Dec 20, 2022

eddycharly added this to the Kyverno Release 1.9.0 milestone Dec 20, 2022

JimBugwadia requested changes Dec 20, 2022

View reviewed changes

Eileen-Yu force-pushed the feat/apply-exception-logic branch from d760469 to 60c244a Compare December 20, 2022 19:42

JimBugwadia approved these changes Dec 20, 2022

View reviewed changes

JimBugwadia requested changes Dec 20, 2022

View reviewed changes

feat: add exception logic

c90823b

Signed-off-by: Eileen Yu <eileenylj@gmail.com>

Eileen-Yu force-pushed the feat/apply-exception-logic branch from 60c244a to c90823b Compare December 21, 2022 03:46

JimBugwadia approved these changes Dec 21, 2022

View reviewed changes

Merge branch 'main' into feat/apply-exception-logic

bc2ec6b

JimBugwadia enabled auto-merge (squash) December 21, 2022 04:01

JimBugwadia merged commit baa41bc into kyverno:main Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add exception logic #5712

feat: add exception logic #5712

Eileen-Yu commented Dec 16, 2022

codecov bot commented Dec 16, 2022 •

edited

eddycharly commented Dec 19, 2022

JimBugwadia left a comment

JimBugwadia Dec 20, 2022

Eileen-Yu Dec 20, 2022

JimBugwadia Dec 20, 2022

Eileen-Yu Dec 20, 2022 •

edited

JimBugwadia Dec 20, 2022

Eileen-Yu Dec 20, 2022

JimBugwadia Dec 20, 2022

Eileen-Yu Dec 20, 2022

JimBugwadia Dec 20, 2022

Eileen-Yu Dec 21, 2022

JimBugwadia Dec 20, 2022

JimBugwadia Dec 20, 2022

JimBugwadia Dec 20, 2022

JimBugwadia Dec 20, 2022

JimBugwadia Dec 20, 2022

Eileen-Yu Dec 21, 2022

JimBugwadia left a comment

feat: add exception logic #5712

feat: add exception logic #5712

Conversation

Eileen-Yu commented Dec 16, 2022

Explanation

Related issue

Milestone of this PR

What type of PR is this

Proposed Changes

Proof Manifests

Checklist

Further Comments

codecov bot commented Dec 16, 2022 • edited

Codecov Report

eddycharly commented Dec 19, 2022

JimBugwadia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eileen-Yu Dec 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBugwadia left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 16, 2022 •

edited

Eileen-Yu Dec 20, 2022 •

edited