skip rule application if referred path not exist (#1806)

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

pkg/engine/mutation.go

@@ -58,7 +58,7 @@ func Mutate(policyContext *PolicyContext) (resp *response.EngineResponse) {
 
 		// check if the resource satisfies the filter conditions defined in the rule
 		//TODO: this needs to be extracted, to filter the resource so that we can avoid passing resources that
-		// dont satisfy a policy rule resource description
+		// don't satisfy a policy rule resource description
 		excludeResource := []string{}
 		if len(policyContext.ExcludeGroupRole) > 0 {
 			excludeResource = policyContext.ExcludeGroupRole
@@ -95,11 +95,13 @@ func Mutate(policyContext *PolicyContext) (resp *response.EngineResponse) {
 				Name:    rule.Name,
 				Type:    utils.Validation.String(),
 				Message: fmt.Sprintf("variable substitution failed for rule %s: %s", rule.Name, err.Error()),
-				Success: false,
+				Success: true,
 			}
 
 			incrementAppliedCount(resp)
 			resp.PolicyResponse.Rules = append(resp.PolicyResponse.Rules, ruleResp)
+
+			logger.Error(err, "failed to substitute variables, skip current rule", "rule name", rule.Name)
 			continue
 		}
 

pkg/engine/mutation_test.go

@@ -160,7 +160,7 @@ func Test_variableSubstitutionPathNotExist(t *testing.T) {
 		JSONContext: ctx,
 		NewResource: *resourceUnstructured}
 	er := Mutate(policyContext)
-	expectedErrorStr := "variable substitution failed for rule test-path-not-exist: variable request.object.metadata.name1 not resolved at path /mutate/overlay/spec/name"
+	expectedErrorStr := "variable substitution failed for rule test-path-not-exist: NotFoundVariableErr, variable request.object.metadata.name1 not resolved at path /mutate/overlay/spec/name"
 	t.Log(er.PolicyResponse.Rules[0].Message)
 	assert.Equal(t, er.PolicyResponse.Rules[0].Message, expectedErrorStr)
 }

pkg/engine/validation.go

@@ -121,12 +121,13 @@ func validateResource(log logr.Logger, ctx *PolicyContext) *response.EngineRespo
 				Name:    rule.Name,
 				Type:    utils.Validation.String(),
 				Message: fmt.Sprintf("variable substitution failed for rule %s: %s", rule.Name, err.Error()),
-				Success: false,
+				Success: true,
 			}
 
 			incrementAppliedCount(resp)
 			resp.PolicyResponse.Rules = append(resp.PolicyResponse.Rules, ruleResp)
 
+			log.Error(err, "failed to substitute variables, skip current rule", "rule name", rule.Name)
 			continue
 		}
 

pkg/engine/validation_test.go

@@ -1319,9 +1319,9 @@ func Test_VariableSubstitutionPathNotExistInPattern(t *testing.T) {
 		JSONContext: ctx,
 		NewResource: *resourceUnstructured}
 	er := Validate(policyContext)
-	assert.Assert(t, !er.PolicyResponse.Rules[0].Success)
+	assert.Assert(t, er.PolicyResponse.Rules[0].Success)
 	assert.Equal(t, er.PolicyResponse.Rules[0].Message,
-		"variable substitution failed for rule test-path-not-exist: variable request.object.metadata.name1 not resolved at path /validate/pattern/spec/containers/0/name")
+		"variable substitution failed for rule test-path-not-exist: NotFoundVariableErr, variable request.object.metadata.name1 not resolved at path /validate/pattern/spec/containers/0/name")
 }
 
 func Test_VariableSubstitutionPathNotExistInAnyPattern_OnePatternStatisfiesButSubstitutionFails(t *testing.T) {
@@ -1411,8 +1411,8 @@ func Test_VariableSubstitutionPathNotExistInAnyPattern_OnePatternStatisfiesButSu
 		JSONContext: ctx,
 		NewResource: *resourceUnstructured}
 	er := Validate(policyContext)
-	assert.Assert(t, !er.PolicyResponse.Rules[0].Success)
-	assert.Equal(t, er.PolicyResponse.Rules[0].Message, "variable substitution failed for rule test-path-not-exist: variable request.object.metadata.name1 not resolved at path /validate/anyPattern/0/spec/template/spec/containers/0/name")
+	assert.Assert(t, er.PolicyResponse.Rules[0].Success)
+	assert.Equal(t, er.PolicyResponse.Rules[0].Message, "variable substitution failed for rule test-path-not-exist: NotFoundVariableErr, variable request.object.metadata.name1 not resolved at path /validate/anyPattern/0/spec/template/spec/containers/0/name")
 }
 
 func Test_VariableSubstitution_NotOperatorWithStringVariable(t *testing.T) {
@@ -1561,8 +1561,8 @@ func Test_VariableSubstitutionPathNotExistInAnyPattern_AllPathNotPresent(t *test
 		JSONContext: ctx,
 		NewResource: *resourceUnstructured}
 	er := Validate(policyContext)
-	assert.Assert(t, !er.PolicyResponse.Rules[0].Success)
-	assert.Equal(t, er.PolicyResponse.Rules[0].Message, "variable substitution failed for rule test-path-not-exist: variable request.object.metadata.name1 not resolved at path /validate/anyPattern/0/spec/template/spec/containers/0/name")
+	assert.Assert(t, er.PolicyResponse.Rules[0].Success)
+	assert.Equal(t, er.PolicyResponse.Rules[0].Message, "variable substitution failed for rule test-path-not-exist: NotFoundVariableErr, variable request.object.metadata.name1 not resolved at path /validate/anyPattern/0/spec/template/spec/containers/0/name")
 }
 
 func Test_VariableSubstitutionPathNotExistInAnyPattern_AllPathPresent_NonePatternSatisfy(t *testing.T) {
@@ -1761,6 +1761,65 @@ func Test_VariableSubstitutionValidate_VariablesInMessageAreResolved(t *testing.
 	assert.Equal(t, er.PolicyResponse.Rules[0].Message, "The animal cow is not in the allowed list of animals.")
 }
 
+func Test_Flux_Kustomization_PathNotPresent(t *testing.T) {
+	tests := []struct {
+		name            string
+		policyRaw       []byte
+		resourceRaw     []byte
+		expectedResult  bool
+		expectedMessage string
+	}{
+		{
+			name:      "path-not-present",
+			policyRaw: []byte(`{"apiVersion":"kyverno.io/v1","kind":"ClusterPolicy","metadata":{"name":"flux-multi-tenancy"},"spec":{"validationFailureAction":"enforce","rules":[{"name":"serviceAccountName","exclude":{"resources":{"namespaces":["flux-system"]}},"match":{"resources":{"kinds":["Kustomization","HelmRelease"]}},"validate":{"message":".spec.serviceAccountName is required","pattern":{"spec":{"serviceAccountName":"?*"}}}},{"name":"sourceRefNamespace","exclude":{"resources":{"namespaces":["flux-system"]}},"match":{"resources":{"kinds":["Kustomization","HelmRelease"]}},"validate":{"message":"spec.sourceRef.namespace must be the same as metadata.namespace","deny":{"conditions":[{"key":"{{request.object.spec.sourceRef.namespace}}","operator":"NotEquals","value":"{{request.object.metadata.namespace}}"}]}}}]}}`),
+			// referred variable path not present
+			resourceRaw:     []byte(`{"apiVersion":"kustomize.toolkit.fluxcd.io/v1beta1","kind":"Kustomization","metadata":{"name":"dev-team","namespace":"apps"},"spec":{"serviceAccountName":"dev-team","interval":"5m","sourceRef":{"kind":"GitRepository","name":"dev-team"},"prune":true,"validation":"client"}}`),
+			expectedResult:  true,
+			expectedMessage: "variable substitution failed for rule sourceRefNamespace: NotFoundVariableErr, variable request.object.spec.sourceRef.namespace not resolved at path /validate/deny/conditions/0/key",
+		},
+		{
+			name:      "resource-with-violation",
+			policyRaw: []byte(`{"apiVersion":"kyverno.io/v1","kind":"ClusterPolicy","metadata":{"name":"flux-multi-tenancy"},"spec":{"validationFailureAction":"enforce","rules":[{"name":"serviceAccountName","exclude":{"resources":{"namespaces":["flux-system"]}},"match":{"resources":{"kinds":["Kustomization","HelmRelease"]}},"validate":{"message":".spec.serviceAccountName is required","pattern":{"spec":{"serviceAccountName":"?*"}}}},{"name":"sourceRefNamespace","exclude":{"resources":{"namespaces":["flux-system"]}},"match":{"resources":{"kinds":["Kustomization","HelmRelease"]}},"validate":{"message":"spec.sourceRef.namespace {{request.object.spec.sourceRef.namespace}} must be the same as metadata.namespace {{request.object.metadata.namespace}}","deny":{"conditions":[{"key":"{{request.object.spec.sourceRef.namespace}}","operator":"NotEquals","value":"{{request.object.metadata.namespace}}"}]}}}]}}`),
+			// referred variable path present with different value
+			resourceRaw:     []byte(`{"apiVersion":"kustomize.toolkit.fluxcd.io/v1beta1","kind":"Kustomization","metadata":{"name":"dev-team","namespace":"apps"},"spec":{"serviceAccountName":"dev-team","interval":"5m","sourceRef":{"kind":"GitRepository","name":"dev-team","namespace":"default"},"prune":true,"validation":"client"}}`),
+			expectedResult:  false,
+			expectedMessage: "spec.sourceRef.namespace default must be the same as metadata.namespace apps",
+		},
+		{
+			name:      "resource-comply",
+			policyRaw: []byte(`{"apiVersion":"kyverno.io/v1","kind":"ClusterPolicy","metadata":{"name":"flux-multi-tenancy"},"spec":{"validationFailureAction":"enforce","rules":[{"name":"serviceAccountName","exclude":{"resources":{"namespaces":["flux-system"]}},"match":{"resources":{"kinds":["Kustomization","HelmRelease"]}},"validate":{"message":".spec.serviceAccountName is required","pattern":{"spec":{"serviceAccountName":"?*"}}}},{"name":"sourceRefNamespace","exclude":{"resources":{"namespaces":["flux-system"]}},"match":{"resources":{"kinds":["Kustomization","HelmRelease"]}},"validate":{"message":"spec.sourceRef.namespace must be the same as metadata.namespace","deny":{"conditions":[{"key":"{{request.object.spec.sourceRef.namespace}}","operator":"NotEquals","value":"{{request.object.metadata.namespace}}"}]}}}]}}`),
+			// referred variable path present with same value - validate passes
+			resourceRaw:     []byte(`{"apiVersion":"kustomize.toolkit.fluxcd.io/v1beta1","kind":"Kustomization","metadata":{"name":"dev-team","namespace":"apps"},"spec":{"serviceAccountName":"dev-team","interval":"5m","sourceRef":{"kind":"GitRepository","name":"dev-team","namespace":"apps"},"prune":true,"validation":"client"}}`),
+			expectedResult:  true,
+			expectedMessage: "spec.sourceRef.namespace must be the same as metadata.namespace",
+		},
+	}
+
+	for _, test := range tests {
+		var policy kyverno.ClusterPolicy
+		assert.NilError(t, json.Unmarshal(test.policyRaw, &policy))
+		resourceUnstructured, err := utils.ConvertToUnstructured(test.resourceRaw)
+		assert.NilError(t, err)
+
+		ctx := context.NewContext()
+		err = ctx.AddResource(test.resourceRaw)
+		assert.NilError(t, err)
+
+		policyContext := &PolicyContext{
+			Policy:      policy,
+			JSONContext: ctx,
+			NewResource: *resourceUnstructured}
+		er := Validate(policyContext)
+
+		for i, rule := range er.PolicyResponse.Rules {
+			if rule.Name == "sourceRefNamespace" {
+				assert.Equal(t, er.PolicyResponse.Rules[i].Success, test.expectedResult)
+				assert.Equal(t, er.PolicyResponse.Rules[i].Message, test.expectedMessage, "\ntest %s failed\nexpected: %s\nactual: %s", test.name, test.expectedMessage, rule.Message)
+			}
+		}
+	}
+}
+
 type testCase struct {
 	description   string
 	policy        []byte

pkg/engine/variables/vars.go

@@ -119,7 +119,7 @@ type NotFoundVariableErr struct {
 }
 
 func (n NotFoundVariableErr) Error() string {
-	return fmt.Sprintf("variable %s not resolved at path %s", n.variable, n.path)
+	return fmt.Sprintf("NotFoundVariableErr, variable %s not resolved at path %s", n.variable, n.path)
 }
 
 // NotResolvedReferenceErr is returned when it is impossible to resolve the variable
@@ -129,7 +129,7 @@ type NotResolvedReferenceErr struct {
 }
 
 func (n NotResolvedReferenceErr) Error() string {
-	return fmt.Sprintf("reference %s not resolved at path %s", n.reference, n.path)
+	return fmt.Sprintf("NotResolvedReferenceErr,reference %s not resolved at path %s", n.reference, n.path)
 }
 
 func substituteReferencesIfAny(log logr.Logger) jsonUtils.Action {