From 731ffde0e7ad485e0197c50f08da820ac98e1c79 Mon Sep 17 00:00:00 2001 From: Jim Bugwadia Date: Sun, 3 Oct 2021 03:15:22 -0700 Subject: [PATCH] fix messages and tests Signed-off-by: Jim Bugwadia --- pkg/engine/anchor/anchor.go | 4 +-- .../mutate/strategicPreprocessing_test.go | 2 +- pkg/engine/response/response.go | 2 +- pkg/engine/validate/validate.go | 6 ++--- pkg/engine/validate/validate_test.go | 14 +++++----- pkg/engine/validation.go | 8 +++++- pkg/engine/variables/vars.go | 4 +++ pkg/generate/validate.go | 6 ++--- pkg/policy/common/validate_pattern.go | 2 +- pkg/policy/validate/validate_test.go | 26 +++++++++---------- test/e2e/mutate/mutate_test.go | 2 +- 11 files changed, 43 insertions(+), 33 deletions(-) diff --git a/pkg/engine/anchor/anchor.go b/pkg/engine/anchor/anchor.go index c57a2be5fc..b1127ad4e0 100644 --- a/pkg/engine/anchor/anchor.go +++ b/pkg/engine/anchor/anchor.go @@ -58,7 +58,7 @@ func (nh NegationHandler) Handle(handler resourceElementHandler, resourceMap map // if anchor is present in the resource then fail if _, ok := resourceMap[anchorKey]; ok { // no need to process elements in value as key cannot be present in resource - return currentPath, fmt.Errorf("Validation rule failed at %s, field %s is disallowed", currentPath, anchorKey) + return currentPath, fmt.Errorf("%s/%s is not allowed", currentPath, anchorKey) } // key is not defined in the resource return "", nil @@ -118,7 +118,7 @@ func (dh DefaultHandler) Handle(handler resourceElementHandler, resourceMap map[ if dh.pattern == "*" && resourceMap[dh.element] != nil { return "", nil } else if dh.pattern == "*" && resourceMap[dh.element] == nil { - return dh.path, fmt.Errorf("Validation rule failed at %s, Field %s is not present", dh.path, dh.element) + return dh.path, fmt.Errorf("%s/%s not found", dh.path, dh.element) } else { path, err := handler(log.Log, resourceMap[dh.element], dh.pattern, originPattern, currentPath, ac) if err != nil { diff --git a/pkg/engine/mutate/strategicPreprocessing_test.go b/pkg/engine/mutate/strategicPreprocessing_test.go index 28905473cf..fc2a1af8ef 100644 --- a/pkg/engine/mutate/strategicPreprocessing_test.go +++ b/pkg/engine/mutate/strategicPreprocessing_test.go @@ -924,7 +924,7 @@ func Test_CheckConditionAnchor_DoesNotMatch(t *testing.T) { resource := yaml.MustParse(string(resourceRaw)) err := checkCondition(log.Log, pattern, resource) - assert.Error(t, err, "Validation rule failed at '/key1/' to validate value 'sample' with pattern 'value*'") + assert.Error(t, err, "resource value 'sample' does not match 'value*' at path /key1/") } func Test_ValidateConditions_MapWithOneCondition_Matches(t *testing.T) { diff --git a/pkg/engine/response/response.go b/pkg/engine/response/response.go index 2a50d1e62e..1ef04f8a79 100644 --- a/pkg/engine/response/response.go +++ b/pkg/engine/response/response.go @@ -106,7 +106,7 @@ type RuleStats struct { //IsSuccessful checks if any rule has failed or not func (er EngineResponse) IsSuccessful() bool { for _, r := range er.PolicyResponse.Rules { - if r.Status == RuleStatusFail { + if r.Status == RuleStatusFail || r.Status == RuleStatusError { return false } } diff --git a/pkg/engine/validate/validate.go b/pkg/engine/validate/validate.go index 3ab332a5ee..e11d239539 100644 --- a/pkg/engine/validate/validate.go +++ b/pkg/engine/validate/validate.go @@ -82,19 +82,19 @@ func validateResourceElement(log logr.Logger, resourceElement, patternElement, o case []interface{}: for _, res := range resource { if !ValidateValueWithPattern(log, res, patternElement) { - return path, fmt.Errorf("Validation rule failed at '%s' to validate value '%v' with pattern '%v'", path, resourceElement, patternElement) + return path, fmt.Errorf("resource value '%v' does not match '%v' at path %s", resourceElement, patternElement, path) } } return "", nil default: if !ValidateValueWithPattern(log, resourceElement, patternElement) { - return path, fmt.Errorf("Validation rule failed at '%s' to validate value '%v' with pattern '%v'", path, resourceElement, patternElement) + return path, fmt.Errorf("resource value '%v' does not match '%v' at path %s", resourceElement, patternElement, path) } } default: log.V(4).Info("Pattern contains unknown type", "path", path, "current", fmt.Sprintf("%T", patternElement)) - return path, fmt.Errorf("Validation rule failed at '%s', pattern contains unknown type", path) + return path, fmt.Errorf("failed at '%s', pattern contains unknown type", path) } return "", nil } diff --git a/pkg/engine/validate/validate_test.go b/pkg/engine/validate/validate_test.go index 65787e8bef..0f46a73833 100644 --- a/pkg/engine/validate/validate_test.go +++ b/pkg/engine/validate/validate_test.go @@ -1507,7 +1507,7 @@ func TestConditionalAnchorWithMultiplePatterns(t *testing.T) { name: "test-23", pattern: []byte(`{"spec": {"containers": [{"name": "*","<(image)": "*:latest","imagePullPolicy": "!Always"}]}}`), resource: []byte(`{"spec": {"containers": [{"name": "nginx","image": "nginx", "imagePullPolicy": "Always"}]}}`), - nilErr: true, + nilErr: false, }, { name: "test-24", @@ -1519,7 +1519,7 @@ func TestConditionalAnchorWithMultiplePatterns(t *testing.T) { name: "test-25", pattern: []byte(`{"spec": {"containers": [{"name": "*","<(image)": "nginx", "env": [{"<(name)": "foo", "<(value)": "bar" }],"imagePullPolicy": "!Always"}]}}`), resource: []byte(`{"spec": {"containers": [{"name": "nginx","image": "nginx", "env": [{"name": "foo1", "value": "bar" }],"imagePullPolicy": "Always"}]}}`), - nilErr: true, + nilErr: false, }, { name: "test-26", @@ -1531,7 +1531,7 @@ func TestConditionalAnchorWithMultiplePatterns(t *testing.T) { name: "test-27", pattern: []byte(`{"spec": {"containers": [{"name": "*", "env": [{"<(name)": "foo", "<(value)": "bar" }],"imagePullPolicy": "!Always"}]}}`), resource: []byte(`{"spec": {"containers": [{"name": "nginx","image": "nginx", "env": [{"name": "foo1", "value": "bar" }],"imagePullPolicy": "Always"}]}}`), - nilErr: true, + nilErr: false, }, { name: "test-28", @@ -1549,7 +1549,7 @@ func TestConditionalAnchorWithMultiplePatterns(t *testing.T) { name: "test-30", pattern: []byte(`{"metadata": {"<(name)": "nginx"},"spec": {"imagePullSecrets": [{"name": "regcred"}]}}`), resource: []byte(`{"metadata": {"name": "somename"},"spec": {"containers": [{"name": "nginx","image": "nginx:latest"}], "imagePullSecrets": [{"name": "cred"}]}}`), - nilErr: true, + nilErr: false, }, { name: "test-31", @@ -1579,7 +1579,7 @@ func TestConditionalAnchorWithMultiplePatterns(t *testing.T) { name: "test-35", pattern: []byte(`{"spec": {"containers": [{"name": "*","<(image)": "nginx"}],"imagePullSecrets": [{"name": "my-registry-secret"}]}}`), resource: []byte(`{"spec": {"containers": [{"name": "nginx","image": "somepod"}], "imagePullSecrets": [{"name": "cred"}]}}`), - nilErr: true, + nilErr: false, }, { name: "test-36", @@ -1605,7 +1605,7 @@ func Test_global_anchor(t *testing.T) { name: "check global anchor_skip", pattern: []byte(`{"spec": {"containers": [{"name": "*","<(image)": "*:latest","imagePullPolicy": "!Always"}]}}`), resource: []byte(`{"spec": {"containers": [{"name": "nginx","image": "nginx:v1", "imagePullPolicy": "Always"}]}}`), - nilErr: true, + nilErr: false, }, { name: "check global anchor_apply", @@ -1631,7 +1631,7 @@ func testMatchPattern(t *testing.T, testCase struct { err = json.Unmarshal(testCase.resource, &resource) assert.NilError(t, err) - err, _ = MatchPattern(log.Log, resource, pattern) + err = MatchPattern(log.Log, resource, pattern) if testCase.nilErr { assert.NilError(t, err, fmt.Sprintf("\ntest: %s\npattern: %s\nresource: %s\n", testCase.name, pattern, resource)) } else { diff --git a/pkg/engine/validation.go b/pkg/engine/validation.go index 0304f45f71..844acb72a6 100644 --- a/pkg/engine/validation.go +++ b/pkg/engine/validation.go @@ -458,14 +458,20 @@ func isSameRuleResponse(r1 *response.RuleResponse, r2 *response.RuleResponse) bo func (v *validator) validatePatterns(resource unstructured.Unstructured) *response.RuleResponse { if v.pattern != nil { if err := validate.MatchPattern(v.log, resource.Object, v.pattern); err != nil { - if pe, ok := err.(*validate.PatternError); ok { v.log.V(3).Info("validation error", "path", pe.Path, "error", err.Error()) + + if pe.Skip { + return ruleResponse(v.rule, pe.Error(), response.RuleStatusSkip) + } + if pe.Path == "" { return ruleResponse(v.rule, v.buildErrorMessage(err, ""), response.RuleStatusError) } return ruleResponse(v.rule, v.buildErrorMessage(err, pe.Path), response.RuleStatusFail) + } else { + return ruleResponse(v.rule, v.buildErrorMessage(err, pe.Path), response.RuleStatusError) } } diff --git a/pkg/engine/variables/vars.go b/pkg/engine/variables/vars.go index ab019ff951..65d4b07ea9 100644 --- a/pkg/engine/variables/vars.go +++ b/pkg/engine/variables/vars.go @@ -320,6 +320,10 @@ func substituteVariablesIfAny(log logr.Logger, ctx context.EvalInterface, vr Var } func isDeleteRequest(ctx context.EvalInterface) bool { + if ctx == nil { + return false + } + operation, err := ctx.Query("request.operation") if err == nil && operation == "DELETE" { return true diff --git a/pkg/generate/validate.go b/pkg/generate/validate.go index 0bc4345f22..6047e814c3 100644 --- a/pkg/generate/validate.go +++ b/pkg/generate/validate.go @@ -55,12 +55,12 @@ func validateResourceElement(log logr.Logger, resourceElement, patternElement, o // elementary values case string, float64, int, int64, bool, nil: if !validate.ValidateValueWithPattern(log, resourceElement, patternElement) { - return path, fmt.Errorf("Validation rule failed at '%s' to validate value '%v' with pattern '%v'", path, resourceElement, patternElement) + return path, fmt.Errorf("value '%v' does not match '%v' at path %s", resourceElement, patternElement, path) } default: log.V(4).Info("Pattern contains unknown type", "path", path, "current", fmt.Sprintf("%T", patternElement)) - return path, fmt.Errorf("Validation rule failed at '%s', pattern contains unknown type", path) + return path, fmt.Errorf("failed at path '%s', pattern contains unknown type", path) } return "", nil } @@ -145,7 +145,7 @@ func (dh Handler) Handle(handler resourceElementHandler, resourceMap map[string] if dh.pattern == "*" && resourceMap[dh.element] != nil { return "", nil } else if dh.pattern == "*" && resourceMap[dh.element] == nil { - return dh.path, fmt.Errorf("Validation rule failed at %s, Field %s is not present", dh.path, dh.element) + return dh.path, fmt.Errorf("failed at path %s, field %s is not present", dh.path, dh.element) } else { path, err := handler(log.Log, resourceMap[dh.element], dh.pattern, originPattern, currentPath) if err != nil { diff --git a/pkg/policy/common/validate_pattern.go b/pkg/policy/common/validate_pattern.go index d14f77e67a..737d7d65d0 100644 --- a/pkg/policy/common/validate_pattern.go +++ b/pkg/policy/common/validate_pattern.go @@ -19,7 +19,7 @@ func ValidatePattern(patternElement interface{}, path string, supportedAnchors [ //TODO? check operator return "", nil default: - return path, fmt.Errorf("Validation rule failed at '%s', pattern contains unknown type", path) + return path, fmt.Errorf("error at '%s', pattern contains unknown type", path) } } func validateMap(patternMap map[string]interface{}, path string, supportedAnchors []commonAnchors.IsAnchor) (string, error) { diff --git a/pkg/policy/validate/validate_test.go b/pkg/policy/validate/validate_test.go index 17ce74590e..99b7b3349b 100644 --- a/pkg/policy/validate/validate_test.go +++ b/pkg/policy/validate/validate_test.go @@ -16,7 +16,7 @@ func Test_Validate_OverlayPattern_Empty(t *testing.T) { err := json.Unmarshal(rawValidation, &validation) assert.NilError(t, err) - checker := NewValidateFactory(validation) + checker := NewValidateFactory(&validation) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -30,7 +30,7 @@ func Test_Validate_OverlayPattern_Nil_PatternAnypattern(t *testing.T) { var validation kyverno.Validation err := json.Unmarshal(rawValidation, &validation) assert.NilError(t, err) - checker := NewValidateFactory(validation) + checker := NewValidateFactory(&validation) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -68,7 +68,7 @@ func Test_Validate_OverlayPattern_Exist_PatternAnypattern(t *testing.T) { var validation kyverno.Validation err := json.Unmarshal(rawValidation, &validation) assert.NilError(t, err) - checker := NewValidateFactory(validation) + checker := NewValidateFactory(&validation) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -106,7 +106,7 @@ func Test_Validate_OverlayPattern_Valid(t *testing.T) { var validation kyverno.Validation err := json.Unmarshal(rawValidation, &validation) assert.NilError(t, err) - checker := NewValidateFactory(validation) + checker := NewValidateFactory(&validation) if _, err := checker.Validate(); err != nil { assert.NilError(t, err) } @@ -139,7 +139,7 @@ func Test_Validate_ExistingAnchor_AnchorOnMap(t *testing.T) { var validation kyverno.Validation err := json.Unmarshal(rawValidation, &validation) assert.NilError(t, err) - checker := NewValidateFactory(validation) + checker := NewValidateFactory(&validation) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -169,7 +169,7 @@ func Test_Validate_ExistingAnchor_AnchorOnString(t *testing.T) { var validation kyverno.Validation err := json.Unmarshal(rawValidation, &validation) assert.NilError(t, err) - checker := NewValidateFactory(validation) + checker := NewValidateFactory(&validation) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -202,7 +202,7 @@ func Test_Validate_ExistingAnchor_Valid(t *testing.T) { err = json.Unmarshal(rawValidation, &validation) assert.NilError(t, err) - checker := NewValidateFactory(validation) + checker := NewValidateFactory(&validation) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -227,7 +227,7 @@ func Test_Validate_ExistingAnchor_Valid(t *testing.T) { } `) err = json.Unmarshal(rawValidation, &validation) assert.NilError(t, err) - checker = NewValidateFactory(validation) + checker = NewValidateFactory(&validation) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -268,7 +268,7 @@ func Test_Validate_Validate_ValidAnchor(t *testing.T) { err = json.Unmarshal(rawValidate, &validate) assert.NilError(t, err) - checker := NewValidateFactory(validate) + checker := NewValidateFactory(&validate) if _, err := checker.Validate(); err != nil { assert.NilError(t, err) } @@ -290,7 +290,7 @@ func Test_Validate_Validate_ValidAnchor(t *testing.T) { err = json.Unmarshal(rawValidate, &validate) assert.NilError(t, err) - checker = NewValidateFactory(validate) + checker = NewValidateFactory(&validate) if _, err := checker.Validate(); err != nil { assert.NilError(t, err) } @@ -317,7 +317,7 @@ func Test_Validate_Validate_Mismatched(t *testing.T) { var validate kyverno.Validation err := json.Unmarshal(rawValidate, &validate) assert.NilError(t, err) - checker := NewValidateFactory(validate) + checker := NewValidateFactory(&validate) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -347,7 +347,7 @@ func Test_Validate_Validate_Unsupported(t *testing.T) { err = json.Unmarshal(rawValidate, &validate) assert.NilError(t, err) - checker := NewValidateFactory(validate) + checker := NewValidateFactory(&validate) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } @@ -373,7 +373,7 @@ func Test_Validate_Validate_Unsupported(t *testing.T) { err = json.Unmarshal(rawValidate, &validate) assert.NilError(t, err) - checker = NewValidateFactory(validate) + checker = NewValidateFactory(&validate) if _, err := checker.Validate(); err != nil { assert.Assert(t, err != nil) } diff --git a/test/e2e/mutate/mutate_test.go b/test/e2e/mutate/mutate_test.go index 621445a2cd..ff79bfdd1e 100644 --- a/test/e2e/mutate/mutate_test.go +++ b/test/e2e/mutate/mutate_test.go @@ -216,7 +216,7 @@ func Test_Mutate(t *testing.T) { Expect(err).NotTo(HaveOccurred()) By("Validating created resource with the expected pattern...") - err, _ = validate.MatchPattern(log.Log, actual, expected) + err = validate.MatchPattern(log.Log, actual, expected) Expect(err).NotTo(HaveOccurred()) By("Deleting Cluster Policies...")