From d6a01ad4796b831c0e486cd5f18721b57d05d554 Mon Sep 17 00:00:00 2001 From: Hiroki Okui Date: Sat, 21 Sep 2024 15:30:49 +0900 Subject: [PATCH 1/2] fix: Adjust testResult structs for better result report --- internal/tester/result.go | 142 +++++++++++++++++++++++++++++++------- internal/tester/tester.go | 4 +- 2 files changed, 118 insertions(+), 28 deletions(-) diff --git a/internal/tester/result.go b/internal/tester/result.go index 4498115..0517df5 100644 --- a/internal/tester/result.go +++ b/internal/tester/result.go @@ -103,6 +103,67 @@ func (r *policyEvalResult) String(verbose bool) string { return strings.Join(out, "\n") } +type policyNotFoundResult struct { + Policy string +} + +var _ testResult = &policyNotFoundResult{} + +func newPolicyNotFoundResult(policy string) *policyNotFoundResult { + return &policyNotFoundResult{ + Policy: policy, + } +} + +func (r *policyNotFoundResult) Pass() bool { + return false +} + +func (r *policyNotFoundResult) String(verbose bool) string { + return fmt.Sprintf("FAIL: %s ==> POLICY NOT FOUND", r.Policy) +} + +type setupErrorResult struct { + Policy string + TestCase TestCase + Errors []error +} + +var _ testResult = &setupErrorResult{} + +func newSetupErrorResult(policy string, tc TestCase, errs []error) *setupErrorResult { + return &setupErrorResult{ + Policy: policy, + TestCase: tc, + Errors: errs, + } +} + +func (r *setupErrorResult) Pass() bool { + return false +} + +func (r *setupErrorResult) String(verbose bool) string { + summary := fmt.Sprintf("FAIL: %s", r.Policy) + if r.TestCase.Object.IsValid() && r.TestCase.OldObject.IsValid() { //nolint:gocritic + summary += fmt.Sprintf(" - %s -> %s ", r.TestCase.Object.String(), r.TestCase.OldObject.NamespacedName.String()) + } else if r.TestCase.Object.IsValid() { + summary += fmt.Sprintf(" - %s", r.TestCase.Object.String()) + } else if r.TestCase.OldObject.IsValid() { + summary += fmt.Sprintf(" - %s", r.TestCase.OldObject.String()) + } + if r.TestCase.Param.IsValid() { + summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) + } + summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "SETUP ERROR") + + out := []string{summary} + for _, err := range r.Errors { + out = append(out, fmt.Sprintf("--- ERROR: %v", err)) + } + return strings.Join(out, "\n") +} + type policyNotMatchConditionResult struct { Policy string TestCase TestCase @@ -142,71 +203,100 @@ func (r *policyNotMatchConditionResult) String(verbose bool) string { if r.TestCase.Param.IsValid() { summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) } - if r.Pass() { - summary += fmt.Sprintf(" - %s ==> %s", "SKIP", "SKIP") - } else { - summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "NOT MATCH") - } + summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "SKIP") out := []string{summary} - out = append(out, fmt.Sprintf("--- NOT MATCH: condition-name %q", r.FailedConditionName)) + if !r.Pass() || verbose { + out = append(out, fmt.Sprintf("--- NOT MATCH: condition-name %q", r.FailedConditionName)) + } return strings.Join(out, "\n") } -type policyNotFoundResult struct { - Policy string +type policyEvalErrorResult struct { + Policy string + TestCase TestCase + Errors []error } -var _ testResult = &policyNotFoundResult{} +var _ testResult = &policyEvalErrorResult{} -func newPolicyNotFoundResult(policy string) *policyNotFoundResult { - return &policyNotFoundResult{ - Policy: policy, +func newPolicyEvalErrorResult(policy string, tc TestCase, errs []error) *policyEvalErrorResult { + return &policyEvalErrorResult{ + Policy: policy, + TestCase: tc, + Errors: errs, } } -func (r *policyNotFoundResult) Pass() bool { - return false +func (r *policyEvalErrorResult) Pass() bool { + return r.TestCase.Expect == Error } -func (r *policyNotFoundResult) String(verbose bool) string { - return fmt.Sprintf("FAIL: %s ==> POLICY NOT FOUND", r.Policy) +func (r *policyEvalErrorResult) String(verbose bool) string { + var summary string + if r.Pass() { + summary = "PASS" + } else { + summary = "FAIL" + } + + summary += fmt.Sprintf(": %s", r.Policy) + if r.TestCase.Object.IsValid() && r.TestCase.OldObject.IsValid() { //nolint:gocritic + summary += fmt.Sprintf(" - (UPDATE) %s -> %s", r.TestCase.OldObject.String(), r.TestCase.Object.NamespacedName.String()) + } else if r.TestCase.Object.IsValid() { + summary += fmt.Sprintf(" - (CREATE) %s", r.TestCase.Object.String()) + } else if r.TestCase.OldObject.IsValid() { + summary += fmt.Sprintf(" - (DELETE) %s", r.TestCase.OldObject.String()) + } + if r.TestCase.Param.IsValid() { + summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) + } + summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "ERROR") + + out := []string{summary} + if !r.Pass() || verbose { + for _, err := range r.Errors { + out = append(out, fmt.Sprintf("--- ERROR: %v", err)) + } + } + + return strings.Join(out, "\n") } -type policyEvalErrorResult struct { +type policyEvalFatalErrorResult struct { Policy string TestCase TestCase Errors []error } -var _ testResult = &policyEvalErrorResult{} +var _ testResult = &policyEvalFatalErrorResult{} -func newPolicyEvalErrorResult(policy string, tc TestCase, errs []error) *policyEvalErrorResult { - return &policyEvalErrorResult{ +func newPolicyEvalFatalErrorResult(policy string, tc TestCase, errs []error) *policyEvalFatalErrorResult { + return &policyEvalFatalErrorResult{ Policy: policy, TestCase: tc, Errors: errs, } } -func (r *policyEvalErrorResult) Pass() bool { +func (r *policyEvalFatalErrorResult) Pass() bool { return false } -func (r *policyEvalErrorResult) String(verbose bool) string { +func (r *policyEvalFatalErrorResult) String(verbose bool) string { summary := fmt.Sprintf("FAIL: %s", r.Policy) if r.TestCase.Object.IsValid() && r.TestCase.OldObject.IsValid() { //nolint:gocritic - summary += fmt.Sprintf(" - %s -> %s ", r.TestCase.Object.String(), r.TestCase.OldObject.NamespacedName.String()) + summary += fmt.Sprintf(" - (UPDATE) %s -> %s", r.TestCase.OldObject.String(), r.TestCase.Object.NamespacedName.String()) } else if r.TestCase.Object.IsValid() { - summary += fmt.Sprintf(" - %s", r.TestCase.Object.String()) + summary += fmt.Sprintf(" - (CREATE) %s", r.TestCase.Object.String()) } else if r.TestCase.OldObject.IsValid() { - summary += fmt.Sprintf(" - %s", r.TestCase.OldObject.String()) + summary += fmt.Sprintf(" - (DELETE) %s", r.TestCase.OldObject.String()) } if r.TestCase.Param.IsValid() { summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) } - summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "SETUP ERROR") + summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "FATAL ERROR") out := []string{summary} for _, err := range r.Errors { diff --git a/internal/tester/tester.go b/internal/tester/tester.go index 7d70709..26d092d 100644 --- a/internal/tester/tester.go +++ b/internal/tester/tester.go @@ -125,7 +125,7 @@ func runEach(cfg CmdConfig, manifestPath string) testResultSummary { // Setup params for validation given, errs := newValidationParams(vap, tc, loader) if len(errs) > 0 { - results = append(results, newPolicyEvalErrorResult(tt.Policy, tc, errs)) + results = append(results, newSetupErrorResult(tt.Policy, tc, errs)) continue } @@ -145,7 +145,7 @@ func runEach(cfg CmdConfig, manifestPath string) testResultSummary { slog.Debug("RUN: ", "policy", tt.Policy, "expect", tc.Expect, "object", tc.Object.String(), "oldObject", tc.OldObject.String(), "param", tc.Param.String()) validationResult, err := validator.Validate(given) if err != nil { - results = append(results, newPolicyEvalErrorResult(tt.Policy, tc, []error{err})) + results = append(results, newPolicyEvalFatalErrorResult(tt.Policy, tc, []error{err})) continue } From 1d3cee81fe1724648bc5a6e2895915e1818aec09 Mon Sep 17 00:00:00 2001 From: Hiroki Okui Date: Sat, 21 Sep 2024 15:45:38 +0900 Subject: [PATCH 2/2] refactor: Extract summaryLine func --- internal/tester/result.go | 141 +++++++++++--------------------------- 1 file changed, 41 insertions(+), 100 deletions(-) diff --git a/internal/tester/result.go b/internal/tester/result.go index 0517df5..7d6450e 100644 --- a/internal/tester/result.go +++ b/internal/tester/result.go @@ -31,7 +31,28 @@ type testResult interface { String(verbose bool) string } -var _ testResult = &policyEvalResult{} +func summaryLine(pass bool, policy string, testCase TestCase, result string) string { + var summary string + if pass { + summary = "PASS" + } else { + summary = "FAIL" + } + + summary += fmt.Sprintf(": %s", policy) + if testCase.Object.IsValid() && testCase.OldObject.IsValid() { //nolint:gocritic + summary += fmt.Sprintf(" - (UPDATE) %s -> %s", testCase.OldObject.String(), testCase.Object.NamespacedName.String()) + } else if testCase.Object.IsValid() { + summary += fmt.Sprintf(" - (CREATE) %s", testCase.Object.String()) + } else if testCase.OldObject.IsValid() { + summary += fmt.Sprintf(" - (DELETE) %s", testCase.OldObject.String()) + } + if testCase.Param.IsValid() { + summary += fmt.Sprintf(" (Param: %s)", testCase.Param.String()) + } + summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(testCase.Expect)), result) + return summary +} type policyEvalResult struct { Policy string @@ -40,6 +61,8 @@ type policyEvalResult struct { Result validating.PolicyDecisionEvaluation } +var _ testResult = &policyEvalResult{} + func newPolicyEvalResult(policy string, tc TestCase, decisions []validating.PolicyDecision) *policyEvalResult { result := validating.EvalAdmit for _, d := range decisions { @@ -64,40 +87,20 @@ func (r *policyEvalResult) Pass() bool { } func (r *policyEvalResult) String(verbose bool) string { - var summary string - if r.Pass() { - summary = "PASS" - } else { - summary = "FAIL" - } - - summary += fmt.Sprintf(": %s", r.Policy) - if r.TestCase.Object.IsValid() && r.TestCase.OldObject.IsValid() { //nolint:gocritic - summary += fmt.Sprintf(" - (UPDATE) %s -> %s", r.TestCase.OldObject.String(), r.TestCase.Object.NamespacedName.String()) - } else if r.TestCase.Object.IsValid() { - summary += fmt.Sprintf(" - (CREATE) %s", r.TestCase.Object.String()) - } else if r.TestCase.OldObject.IsValid() { - summary += fmt.Sprintf(" - (DELETE) %s", r.TestCase.OldObject.String()) - } - if r.TestCase.Param.IsValid() { - summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) - } - summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), strings.ToUpper(string(r.Result))) - + summary := summaryLine(r.Pass(), r.Policy, r.TestCase, string(r.Result)) out := []string{summary} - for _, d := range r.Decisions { - if r.Pass() && !verbose { - continue - } - // Workaround to handle the case where the evaluation is not set - // TODO remove this workaround after htcps://github.com/kubernetes/kubernetes/pull/126867 is released - if d.Evaluation == "" { - d.Evaluation = validating.EvalDeny - } - if d.Evaluation == validating.EvalDeny { - out = append(out, fmt.Sprintf("--- DENY: reason %q, message %q", d.Reason, d.Message)) - } else if d.Evaluation == validating.EvalError { - out = append(out, fmt.Sprintf("--- ERROR: reason %q, message %q", d.Reason, d.Message)) + if !r.Pass() || verbose { + for _, d := range r.Decisions { + // Workaround to handle the case where the evaluation is not set + // TODO remove this workaround after htcps://github.com/kubernetes/kubernetes/pull/126867 is released + if d.Evaluation == "" { + d.Evaluation = validating.EvalDeny + } + if d.Evaluation == validating.EvalDeny { + out = append(out, fmt.Sprintf("--- DENY: reason %q, message %q", d.Reason, d.Message)) + } else if d.Evaluation == validating.EvalError { + out = append(out, fmt.Sprintf("--- ERROR: reason %q, message %q", d.Reason, d.Message)) + } } } return strings.Join(out, "\n") @@ -144,19 +147,7 @@ func (r *setupErrorResult) Pass() bool { } func (r *setupErrorResult) String(verbose bool) string { - summary := fmt.Sprintf("FAIL: %s", r.Policy) - if r.TestCase.Object.IsValid() && r.TestCase.OldObject.IsValid() { //nolint:gocritic - summary += fmt.Sprintf(" - %s -> %s ", r.TestCase.Object.String(), r.TestCase.OldObject.NamespacedName.String()) - } else if r.TestCase.Object.IsValid() { - summary += fmt.Sprintf(" - %s", r.TestCase.Object.String()) - } else if r.TestCase.OldObject.IsValid() { - summary += fmt.Sprintf(" - %s", r.TestCase.OldObject.String()) - } - if r.TestCase.Param.IsValid() { - summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) - } - summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "SETUP ERROR") - + summary := summaryLine(r.Pass(), r.Policy, r.TestCase, "SETUP ERROR") out := []string{summary} for _, err := range r.Errors { out = append(out, fmt.Sprintf("--- ERROR: %v", err)) @@ -185,26 +176,7 @@ func (r *policyNotMatchConditionResult) Pass() bool { } func (r *policyNotMatchConditionResult) String(verbose bool) string { - var summary string - if r.Pass() { - summary = "PASS" - } else { - summary = "FAIL" - } - - summary += fmt.Sprintf(": %s", r.Policy) - if r.TestCase.Object.IsValid() && r.TestCase.OldObject.IsValid() { //nolint:gocritic - summary += fmt.Sprintf(" - (UPDATE) %s -> %s", r.TestCase.OldObject.String(), r.TestCase.Object.NamespacedName.String()) - } else if r.TestCase.Object.IsValid() { - summary += fmt.Sprintf(" - (CREATE) %s", r.TestCase.Object.String()) - } else if r.TestCase.OldObject.IsValid() { - summary += fmt.Sprintf(" - (DELETE) %s", r.TestCase.OldObject.String()) - } - if r.TestCase.Param.IsValid() { - summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) - } - summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "SKIP") - + summary := summaryLine(r.Pass(), r.Policy, r.TestCase, "SKIP") out := []string{summary} if !r.Pass() || verbose { out = append(out, fmt.Sprintf("--- NOT MATCH: condition-name %q", r.FailedConditionName)) @@ -234,26 +206,7 @@ func (r *policyEvalErrorResult) Pass() bool { } func (r *policyEvalErrorResult) String(verbose bool) string { - var summary string - if r.Pass() { - summary = "PASS" - } else { - summary = "FAIL" - } - - summary += fmt.Sprintf(": %s", r.Policy) - if r.TestCase.Object.IsValid() && r.TestCase.OldObject.IsValid() { //nolint:gocritic - summary += fmt.Sprintf(" - (UPDATE) %s -> %s", r.TestCase.OldObject.String(), r.TestCase.Object.NamespacedName.String()) - } else if r.TestCase.Object.IsValid() { - summary += fmt.Sprintf(" - (CREATE) %s", r.TestCase.Object.String()) - } else if r.TestCase.OldObject.IsValid() { - summary += fmt.Sprintf(" - (DELETE) %s", r.TestCase.OldObject.String()) - } - if r.TestCase.Param.IsValid() { - summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) - } - summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "ERROR") - + summary := summaryLine(r.Pass(), r.Policy, r.TestCase, "ERROR") out := []string{summary} if !r.Pass() || verbose { for _, err := range r.Errors { @@ -285,19 +238,7 @@ func (r *policyEvalFatalErrorResult) Pass() bool { } func (r *policyEvalFatalErrorResult) String(verbose bool) string { - summary := fmt.Sprintf("FAIL: %s", r.Policy) - if r.TestCase.Object.IsValid() && r.TestCase.OldObject.IsValid() { //nolint:gocritic - summary += fmt.Sprintf(" - (UPDATE) %s -> %s", r.TestCase.OldObject.String(), r.TestCase.Object.NamespacedName.String()) - } else if r.TestCase.Object.IsValid() { - summary += fmt.Sprintf(" - (CREATE) %s", r.TestCase.Object.String()) - } else if r.TestCase.OldObject.IsValid() { - summary += fmt.Sprintf(" - (DELETE) %s", r.TestCase.OldObject.String()) - } - if r.TestCase.Param.IsValid() { - summary += fmt.Sprintf(" (Param: %s)", r.TestCase.Param.String()) - } - summary += fmt.Sprintf(" - %s ==> %s", strings.ToUpper(string(r.TestCase.Expect)), "FATAL ERROR") - + summary := summaryLine(r.Pass(), r.Policy, r.TestCase, "FATAL ERROR") out := []string{summary} for _, err := range r.Errors { out = append(out, fmt.Sprintf("--- ERROR: %v", err))