Setting validationFailureAction to enforce is going to enforce it for every Policy #601

realshuting · 2020-01-07T01:17:24Z

This PR fixes

[BUG] Setting validationFailureAction to enforce is going to enforce it for every Policy. #567, please refer to the issue for more information.
As we remove violations reported on resource owner, Policy violation on resource creation #535 is no longer applicable.
Report violation if referenced context is not present when substitute variables #568, report violation if referenced path not present. The violation is reported in separate places because we don't want to block resource creation if such an error exists in the policy that is not applicable to the resource.

…eneric call to create violation info

shivdudhani · 2020-01-07T01:30:40Z

but can we merge PR https://github.com/nirmata/kyverno/pull/594 and https://github.com/nirmata/kyverno/pull/587 first. As it uses reporting of PV in generate rules and will require changes to fix this PR.

shivdudhani · 2020-01-07T03:43:48Z

Question 1:
Why are we removing the logic of owners while building violations?

Question 2:
Why does policyviolation package need to know engine.EngineResponse, this package dependency is not needed.

Each package should process engineResponse individually and then reports PV.

If you notice the package history, this dependency was intentionally removed(this discussion has been done before). But this PR adds the dependency again.

Please revert the corresponding changes.

shivdudhani

Question 1:
Why are we removing the logic of owners while building violations?

Question 2:
Why does policyviolation package need to know engine.EngineResponse, this package dependency is not needed.

Each package should process engineResponse individually and then reports PV.

If you notice the package history, this dependency was intentionally removed(this discussion has been done before). But this PR adds the dependency again.

Please revert the corresponding changes.

realshuting · 2020-01-07T18:16:09Z

@shivdudhani
Q1: The violation created with owners was to inform the users if they have rule on Pod while deploying a pod controller. In this way we would have some sort of alarms and nothing go silent. After #518, we have an alternative solution to report such a scenario, please refer to comment.
Also it is natural to a Kubernetes user, for example, you would do a kubectl describe <podController> (event) or kubectl get -o yaml <podController> (status) to see why the pod does not get scheduled (when enforce).

Q2: If you notice, we create pvInfo in multiple components, with the change above, each of the function would have the same API, same logic. So it's better to use generic code to create this info instead of having it everywhere.

shivdudhani · 2020-01-07T18:25:59Z

Response to Q1: the whole idea of adding OwnerRef was to support any resources not just pod-controllers, as then we can report and this is how the feature was specified. With this we only support pod-controllers, so this will mean we are removing a feature. If this is the case, we can mention we have removed a feature.

Response to Q2: Yes, but it introduces package dependency and it was refactored to handle this. Generic code, it makes code easy but at the cost of increasing dependency. If the history of the package is looked at, it was refactored specifically for it. I'm not in favor of writing code that easy to write at the cost of code maintainability. The idea of policy violation is independent of a policy engine. We have had discussions to define standard design PV for any policy engine. But this adds the dependency.

realshuting · 2020-01-07T19:59:11Z

Q1: Let's move the discussion to the issue, it's easier to track.
Q2: I'm ok to remove the dependency from policyviolation pkg, it's just that having the same code in separate places is not scalable, it's better to have a standard API that we can directly make use of. In this case, to get pvInfo from engineReponse.

shivdudhani · 2020-01-07T20:06:59Z

PolicyViolation and EngineReponse are different resources, they don't need a direct dependency. You can define an interface that others implement to perform the conversion, but this again adds the dependency between the packages as the interface has to be defined in pkg engine or policy violation.

shivdudhani · 2020-01-07T20:13:37Z

@realshuting
As the above change is regarding re-factoring and is taking time, it can be completed later(refactoring issue for tracking) and can focus on other pending issues,

realshuting · 2020-01-07T20:22:19Z

As the above change is regarding re-factoring and is taking time, it can be completed later(refactoring issue for tracking) and can focus on other pending issues,

I'm not following, do you mean to keep the changes in PR or revert it back to the old way?

shivdudhani · 2020-01-07T21:37:22Z

@realshuting whichever is easier for you, if we have time to refactor and also complete the pending issues for release v1.1.0 then that's great. If not I would suggest prioritizing the issues then the refactoring.

shivdudhani · 2020-01-09T00:09:11Z

@realshuting as per the discussion the above behavior would change. Can you update this PR to include the reporting of PV if variable path is not found?

shivdudhani · 2020-01-09T01:02:05Z

Can use interface EvalInterface in pkg/engine/context/context.go to check if the variable is present in context or not . And use this to understand if the query was evaluated or not.

If a path is not found a response will be nil interface. If there is any resource for which path exists but store nil value, the interface internal type will store the JSON type(mosty string) so will not be a nil interface.

realshuting · 2020-01-11T02:37:54Z

Merge to generate the release. If anything changes, would push a patch for that.

shivdudhani

Overall:

some previous comments not addresed. (package dependency between policyvioltaion pkg and engine.
Curious to know y we adding a new field just for this check. As other checks use ruleResponse failure. It seems inconsistent to add a field and helper functions just for this check.
the check is performed before the resource is matched.
Some minor checks

shivdudhani · 2020-01-13T17:48:59Z

pkg/engine/context/evaluate.go

-		glog.V(4).Infof("failed to unmarshall context")
-		return emptyResult, err
+		glog.V(4).Infof("failed to unmarshall context: %v", err)
+		return emptyResult, fmt.Errorf("failed to unmarshall context: %v", err)


spelling unmarshal

shivdudhani · 2020-01-13T17:52:40Z

pkg/engine/utils.go

+	var tempRule kyverno.Rule
+	var tempRulePattern interface{}
+
+	tempRule.MatchResources = rule.MatchResources


y use the tempRule variable here ?
As it as a copy of a copy of rule.

If you see the description of this function, it validates the general ruleInfo: matchedResource, excludeResource and condition, not a specific rule, so it extracts the general rule info as a copy.

shivdudhani · 2020-01-13T17:55:00Z

pkg/engine/generation.go

@@ -55,6 +61,13 @@ func filterRules(policy kyverno.ClusterPolicy, resource unstructured.Unstructure
 	}

 	for _, rule := range policy.Spec.Rules {
+		if paths := validateGeneralRuleInfoVariables(ctx, rule); len(paths) != 0 {


Y is this not placed inside filterRule function ? as this is being processed per rule and we already have a function filterRule which already has checks per rule.

shivdudhani · 2020-01-13T17:57:34Z

pkg/engine/mutation.go

@@ -55,6 +57,13 @@ func Mutate(policyContext PolicyContext) (resp response.EngineResponse) {
 			continue
 		}

+		if paths := validateGeneralRuleInfoVariables(ctx, rule); len(paths) != 0 {


Shouldnt this check be after MatchesResourceDescription ? Now the variables are checked even if the resource does not statify the resource match/exclude block

Potentially the user could substitute the variable in match/exclude block, especially for userInfo, so the check performs before MatchesResourceDescription.

shivdudhani · 2020-01-13T18:00:59Z

pkg/engine/response/response.go

@@ -65,6 +65,8 @@ type RuleResponse struct {
 	Success bool `json:"success"`
 	// statistics
 	RuleStats `json:",inline"`
+	// PathNotPresent indicates whether referenced path in variable substitution exist
+	PathNotPresent bool `json:"pathNotPresent"`


Curious why we introducing a check for a path not present, as this could rule that failed with an error message.
So now for some errors, we have rule failure and message, and some new field added to a struct. (seems inconsistent design)

From my understanding,

a rule failure refers to a rule application failure, while the path not present is much more a policy definition error

since later we will have policy error reported as new CRD, it's more clear to have that 2 logic decoupled for the change.

shivdudhani · 2020-01-13T18:03:49Z

pkg/engine/validate/common.go

@@ -7,6 +7,13 @@ import (
 	"github.com/nirmata/kyverno/pkg/engine/operator"
 )

+type ValidationFailureReason int


In reference to some previous comments, a new type is introduced but this doesn't really add much value.
As the enum is not PathNotFound and the other value is RuleFailure(which is already default behavior if the rule has failed).

shivdudhani · 2020-01-13T18:06:43Z

pkg/policyviolation/builder.go

 )

+func GeneratePVsFromEngineResponse(ers []response.EngineResponse) (pvInfos []Info) {


As discussed before, the policyviolation package is now dependent on engine (engine/response).
I believe the conclusion was to decouple the packages.

shivdudhani · 2020-01-13T18:09:00Z

pkg/policyviolation/builder.go

+			continue
+		}
+		// skip when response succeed AND referenced paths exist
+		if er.IsSuccesful() && !er.IsPathNotPresent() {


In reference to some above comments, the pathNotPresent is engineResponse error instead of adding a new filed and check specifically for it.
As this is one of the multiple check-in policy, others use a common field rule failure, which for this field we add a new field and new error checks.

Again, as we gonna report pathNotPresent as the policy error, it would be helpful to decouple this type of error from violation.

shivdudhani · 2020-01-13T18:09:38Z

pkg/policyviolation/builder.go

+	return info
+}
+
+func buildViolatedRules(er response.EngineResponse) []kyverno.ViolatedRule {


Same as the above comments adds package dependency.

realshuting added 4 commits January 3, 2020 15:19

update policymutation_test

f5411c1

change the policy action to operate on it's own validationFailureAction

77955ff

fix pod controller annotation to "none"

9194251

- remove policy violation created on owner and related logic; - use g…

ecbbd04

…eneric call to create violation info

realshuting mentioned this pull request Jan 7, 2020

[BUG] Setting validationFailureAction to enforce is going to enforce it for every Policy. #567

Closed

realshuting requested a review from shivdudhani January 7, 2020 01:31

realshuting added the do not merge label Jan 7, 2020

remove dclient from pvbuilder

dcc3179

shivdudhani suggested changes Jan 7, 2020

View reviewed changes

realshuting added 2 commits January 8, 2020 16:48

Merge branch 'master' into add_testscenario

e3123e9

fix build error

d0a1acb

realshuting added 6 commits January 9, 2020 12:23

validate paths in variable substitution is present

731fdb3

generate violation in mutation when substitute path not present

f78ca61

generate violation in validate when substitute path not present

5a44ab3

fix build error

2eb0e49

report violation in generate when path not present

434ed20

Merge branch 'master' into add_testscenario

ac0404b

realshuting removed the do not merge label Jan 10, 2020

realshuting requested a review from shivdudhani January 10, 2020 20:35

realshuting added 4 commits January 10, 2020 13:34

remove managedResource

eb0390d

add more unit tests

4c83ab8

pass in ctx to mutation and generation

f618bbc

fix build error

4eff0e9

realshuting merged commit 28ccff0 into master Jan 11, 2020

realshuting deleted the add_testscenario branch January 11, 2020 02:37

shivdudhani reviewed Jan 13, 2020

View reviewed changes

This was referenced Jan 13, 2020

Dependency: policyviolation is dependent on engine/response #620

Closed

Policy violation on resource creation #535

Closed

shivdudhani mentioned this pull request Jan 22, 2020

[BUG] Optimize the Variable helper functions #622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting validationFailureAction to enforce is going to enforce it for every Policy #601

Setting validationFailureAction to enforce is going to enforce it for every Policy #601

realshuting commented Jan 7, 2020 •

edited

shivdudhani commented Jan 7, 2020 •

edited

shivdudhani commented Jan 7, 2020

shivdudhani left a comment

realshuting commented Jan 7, 2020

shivdudhani commented Jan 7, 2020

realshuting commented Jan 7, 2020

shivdudhani commented Jan 7, 2020

shivdudhani commented Jan 7, 2020

realshuting commented Jan 7, 2020

shivdudhani commented Jan 7, 2020

shivdudhani commented Jan 9, 2020

shivdudhani commented Jan 9, 2020

realshuting commented Jan 11, 2020

shivdudhani left a comment

shivdudhani Jan 13, 2020

shivdudhani Jan 13, 2020

realshuting Jan 13, 2020

shivdudhani Jan 13, 2020

shivdudhani Jan 13, 2020

realshuting Jan 13, 2020

shivdudhani Jan 13, 2020

realshuting Jan 13, 2020

shivdudhani Jan 13, 2020

realshuting Jan 13, 2020

shivdudhani Jan 13, 2020

shivdudhani Jan 13, 2020

realshuting Jan 13, 2020

shivdudhani Jan 13, 2020

		)

		func GeneratePVsFromEngineResponse(ers []response.EngineResponse) (pvInfos []Info) {

Setting validationFailureAction to enforce is going to enforce it for every Policy #601

Setting validationFailureAction to enforce is going to enforce it for every Policy #601

Conversation

realshuting commented Jan 7, 2020 • edited

shivdudhani commented Jan 7, 2020 • edited

shivdudhani commented Jan 7, 2020

shivdudhani left a comment

Choose a reason for hiding this comment

realshuting commented Jan 7, 2020

shivdudhani commented Jan 7, 2020

realshuting commented Jan 7, 2020

shivdudhani commented Jan 7, 2020

shivdudhani commented Jan 7, 2020

realshuting commented Jan 7, 2020

shivdudhani commented Jan 7, 2020

shivdudhani commented Jan 9, 2020

shivdudhani commented Jan 9, 2020

realshuting commented Jan 11, 2020

shivdudhani 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

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

realshuting commented Jan 7, 2020 •

edited

shivdudhani commented Jan 7, 2020 •

edited