From f515bc5dbf153e15a812ab2c6a3ef522fca1b3ec Mon Sep 17 00:00:00 2001 From: shuting Date: Thu, 15 Apr 2021 17:33:34 -0700 Subject: [PATCH] skip rule application if referred path not exist (#1806) Signed-off-by: Shuting Zhao --- pkg/engine/mutation.go | 6 ++- pkg/engine/mutation_test.go | 2 +- pkg/engine/validation.go | 3 +- pkg/engine/validation_test.go | 71 ++++++++++++++++++++++++++++++++--- pkg/engine/variables/vars.go | 4 +- 5 files changed, 74 insertions(+), 12 deletions(-) diff --git a/pkg/engine/mutation.go b/pkg/engine/mutation.go index 9e84f7552c..6b44cf130e 100644 --- a/pkg/engine/mutation.go +++ b/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 } diff --git a/pkg/engine/mutation_test.go b/pkg/engine/mutation_test.go index 8bd55a3103..6d7f5bdb66 100644 --- a/pkg/engine/mutation_test.go +++ b/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) } diff --git a/pkg/engine/validation.go b/pkg/engine/validation.go index 17ad9d5a36..353d9e4e59 100644 --- a/pkg/engine/validation.go +++ b/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 } diff --git a/pkg/engine/validation_test.go b/pkg/engine/validation_test.go index 1b09f3aa85..5fd4850eab 100644 --- a/pkg/engine/validation_test.go +++ b/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 diff --git a/pkg/engine/variables/vars.go b/pkg/engine/variables/vars.go index c8a2d005b4..11943e7404 100644 --- a/pkg/engine/variables/vars.go +++ b/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 {