Add warn enforcement action #1107

sozercan · 2021-02-01T22:51:52Z

Signed-off-by: Sertac Ozercan sozercan@gmail.com

What this PR does / why we need it:
Creates a new enforcement action called warn that provides a warning during admission (only difference compared to dryrun). Requires Kubernetes v1.19+ clusters. For non-v1.19+ clusters, it behaves identical to dryrun.

$ kubectl apply pause.yaml
Warning: [denied by prod-repo-is-openpolicyagent] container <pause> has an invalid image repo <mcr.microsoft.com/oss/kubernetes/pause:1.4.0>, allowed repos are ["only-this-repo"]
pod/pause created

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Closes #701

Special notes for your reviewer:
This can also be merged into dryrun since functionality is very similar.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

codecov-io · 2021-02-01T22:54:19Z

Codecov Report

Merging #1107 (27cd3cf) into master (321d8a8) will decrease coverage by 0.07%.
The diff coverage is 12.19%.

@@            Coverage Diff             @@
##           master    #1107      +/-   ##
==========================================
- Coverage   49.17%   49.10%   -0.08%     
==========================================
  Files          63       63              
  Lines        4390     4423      +33     
==========================================
+ Hits         2159     2172      +13     
- Misses       1970     1993      +23     
+ Partials      261      258       -3

Flag	Coverage Δ
unittests	`49.10% <12.19%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/webhook/policy.go	`28.62% <11.11%> (-2.82%)`	⬇️
pkg/util/enforcement_action.go	`25.00% <100.00%> (ø)`
...onstrainttemplate/constrainttemplate_controller.go	`58.89% <0.00%> (+3.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 321d8a8...27cd3cf. Read the comment docs.

pkg/webhook/policy.go

maxsmythe · 2021-02-01T23:46:28Z

pkg/webhook/policy_test.go

+				resDeny,
+				resWarn,
+			},
+			ExpectedMsgCount: 2,


We should also test that the correct message is going to the correct response field, event type and log line.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

ritazh · 2021-02-04T02:36:34Z

At highlevel, we should decide on what is the expected user experience for admission response for each enforcementAction should be.
Here is a proposal:

dryrun: no warning, no deny message
warning: warning message, no deny message
deny: deny message, no warning

Both deny and dryrun should behave the same as they do today.

WDYT?

maxsmythe · 2021-02-04T04:47:06Z

SGTM

What about when there are multiple violations?

say: 1 deny, 1 dryrun, 1 warning

What gets returned?

sozercan · 2021-02-04T04:53:12Z

I am thinking:

no message for dryrun
"would have denied by" warning for warn
"denied by" for deny as the actual admission response

which will end as deny.

ritazh · 2021-02-04T06:07:54Z

What about when there are multiple violations?
say: 1 deny, 1 dryrun, 1 warning
What gets returned?

This would return: 1 warning and 1 deny

maxsmythe · 2021-02-04T06:52:54Z

SGTM

maxsmythe · 2021-02-04T06:53:13Z

What events and logs get written?

sozercan · 2021-02-04T19:39:12Z

For log-denies, we don't differentiate between dryrun and deny today, so warn should be same.

For events, we can make a new reason called WarningAdmission (+1 here) or WarningViolation (we are calling FailedAdmission for deny events and DryrunViolation for dryrun events).

Thoughts?

However, if we create a new reason, we won't be able to fallback to dryrun reason for pre-1.19 cluster.

maxsmythe · 2021-02-04T20:35:52Z

Per

gatekeeper/pkg/webhook/policy.go

Lines 214 to 223 in db5a580

    
           if r.EnforcementAction == "deny" || r.EnforcementAction == "dryrun" { 
        
           	if *logDenies { 
        
           		log.WithValues( 
        
           			logging.Process, "admission", 
        
           			logging.EventType, "violation", 
        
           			logging.ConstraintName, r.Constraint.GetName(), 
        
           			logging.ConstraintGroup, r.Constraint.GroupVersionKind().Group, 
        
           			logging.ConstraintAPIVersion, r.Constraint.GroupVersionKind().Version, 
        
           			logging.ConstraintKind, r.Constraint.GetKind(), 
        
           			logging.ConstraintAction, r.EnforcementAction,

--log-denies reports the enforcement action on the constraint, we should continue with that. IMO we can also drop the

if r.EnforcementAction == "deny" || r.EnforcementAction == "dryrun" {

line, and just report any violation we see as we see it.

Per

gatekeeper/pkg/webhook/policy.go

Line 240 in db5a580

logging.ConstraintAction: r.EnforcementAction,

events are currently emitted with the enforcement action as-written written as an annotation. IMO that works as-is.

WRT event reasons...

I don't think we are coupled to the K8s admission webhook here, so there is no risk in adding a new reason for a new violation type. Users who don't use warning will not see any new behavior. Users who do will see the appropriate event reason in their cluster regardless of K8s version.

Another question is... what purpose do we want Event reason to serve? Is it a proxy for the enforcement action, which is how it's currently written? Is it more about categorizing the events between "those that cause rejections and those that dont", and users have the ability to dig deeper by looking at the event annotations?

If the later, we should change the text for reason. We have wriggle room to do so b/c events are still alpha.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan · 2021-03-01T23:22:13Z

@maxsmythe @ritazh updated this per the discussion above. let me know what you think

added would have denied by text to warning
multiple violations with different enforcement actions will return both warning and deny. In case of deny, resource will get denied.
created a new event type called WarningAdmission

maxsmythe

Looking close! A couple of nits and a question about word choice for "warning" events.

pkg/webhook/policy.go

maxsmythe · 2021-03-02T02:56:28Z

pkg/webhook/policy.go

 					eventMsg = "Dryrun violation"
 					reason = "DryrunViolation"
+				case string(util.Warn):
+					eventMsg = "Admission webhook \"validation.gatekeeper.sh\" would have denied the request"


"would have denied the request" seems more appropriate for dryrun, where we are presumably dry-running against a future deny. The request "would have been rejected" if this had not been a dry run.

"raised a warning for this request" maybe?

updated eventMsg. should we change warning message to include raised a warning for this request too?

so it's either
Warning: [would have denied by cm-must-have-gk] you must provide labels: {"gatekeeper"}

or

Warning: [cm-must-have-gk raised a warning for this request] you must provide labels: {"gatekeeper"}

Why not just:

Warning: [cm-must-have-gk] you must provide labels: {"gatekeeper"}

?

It sounds like kubectl is responsible for providing the context about what is a warning vs. not?

Ah, I see we're adding "denied by" by default.

We could use [warning from cm-must-have-gk] .....

Warning: in the beginning is automatically added, so that'll be

Warning: [warning from cm-must-have-gk] you must provide labels: {"gatekeeper"}

Remove both denied by ... and would have denied by ...?

for 1 warning, 1 deny:

Warning: [cm-must-have-gk] you must provide labels: {"gatekeeper"} Error from server ([deny-all-deny] DATA: {}): error when creating "test/bats/tests/bad/bad_cm.yaml": admission webhook "validation.gatekeeper.sh" denied the request: [deny-all-deny] DATA: {}

I like that approach, personally.

Wouldn't this change existing behavior/expectation though? especially if users have been parsing that string

@ritazh wdyt?

It would change existing behavior. I suppose whether that matters depends on whether we view the string format as part of our API. I'd consider these intended to be human-readable, and therefore not part of the API.

HTTP status codes and the API as defined by K8s should be what machines rely on.

maxsmythe · 2021-03-02T03:00:44Z

Also, it looks like hte unit tests are failing?

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

pkg/webhook/policy.go

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

pkg/webhook/policy.go

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

maxsmythe

LGTM!

add warn enforcement action

922f69f

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan requested review from maxsmythe and ritazh February 1, 2021 22:52

maxsmythe requested changes Feb 1, 2021

View reviewed changes

fix enforcementAction

a133e63

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan added 2 commits March 1, 2021 15:02

update

d141d92

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Merge branch 'master' into warn-ea

cc93ef6

sozercan force-pushed the warn-ea branch from b011520 to cc93ef6 Compare March 1, 2021 23:18

maxsmythe reviewed Mar 2, 2021

View reviewed changes

address comments

dd4744d

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan force-pushed the warn-ea branch from 23e4558 to dd4744d Compare March 2, 2021 20:52

Merge branch 'master' into warn-ea

27cd3cf

shomron reviewed Mar 3, 2021

View reviewed changes

pkg/webhook/policy.go Outdated Show resolved Hide resolved

Merge branch 'master' into warn-ea

25dc79e

sozercan force-pushed the warn-ea branch from 23d7cf6 to 1621e19 Compare March 3, 2021 17:48

add const

dce6c42

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan force-pushed the warn-ea branch from 1621e19 to dce6c42 Compare March 3, 2021 17:48

maxsmythe reviewed Mar 4, 2021

View reviewed changes

pkg/webhook/policy.go Show resolved Hide resolved

update messages

d295651

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan added 2 commits March 5, 2021 13:47

Merge branch 'master' into warn-ea

286e0cd

add test label

4f7bd02

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan requested a review from maxsmythe March 8, 2021 21:18

maxsmythe approved these changes Mar 8, 2021

View reviewed changes

sozercan merged commit 426949f into open-policy-agent:master Mar 9, 2021

sozercan deleted the warn-ea branch March 9, 2021 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warn enforcement action #1107

Add warn enforcement action #1107

sozercan commented Feb 1, 2021

codecov-io commented Feb 1, 2021 •

edited

maxsmythe Feb 1, 2021

ritazh commented Feb 4, 2021 •

edited

maxsmythe commented Feb 4, 2021

sozercan commented Feb 4, 2021

ritazh commented Feb 4, 2021

maxsmythe commented Feb 4, 2021

maxsmythe commented Feb 4, 2021

sozercan commented Feb 4, 2021 •

edited

maxsmythe commented Feb 4, 2021

sozercan commented Mar 1, 2021 •

edited

maxsmythe left a comment

maxsmythe Mar 2, 2021

sozercan Mar 2, 2021

maxsmythe Mar 3, 2021

maxsmythe Mar 3, 2021

sozercan Mar 3, 2021

sozercan Mar 4, 2021

maxsmythe Mar 4, 2021

sozercan Mar 4, 2021

maxsmythe Mar 4, 2021

sozercan Mar 5, 2021

maxsmythe commented Mar 2, 2021

maxsmythe left a comment

Add warn enforcement action #1107

Add warn enforcement action #1107

Conversation

sozercan commented Feb 1, 2021

codecov-io commented Feb 1, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

ritazh commented Feb 4, 2021 • edited

maxsmythe commented Feb 4, 2021

sozercan commented Feb 4, 2021

ritazh commented Feb 4, 2021

maxsmythe commented Feb 4, 2021

maxsmythe commented Feb 4, 2021

sozercan commented Feb 4, 2021 • edited

maxsmythe commented Feb 4, 2021

sozercan commented Mar 1, 2021 • edited

maxsmythe 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

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

maxsmythe commented Mar 2, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 1, 2021 •

edited

ritazh commented Feb 4, 2021 •

edited

sozercan commented Feb 4, 2021 •

edited

sozercan commented Mar 1, 2021 •

edited