Run gator test 2 (#1519)

* start refactoring per suggestions

Signed-off-by: Will Beason <[email protected]>

* fix object parameter usage

Signed-off-by: Will Beason <[email protected]>

* Implement violations

Users may now specify:

- that an object is expected to produce violations
- the expected number of violations
- expected violation messages

This deviates from the design by making Violations an object rather than
a list - making it an array was awkward to work with. I've tried to
leave room so that it is reasonably extensible. I think we'll have a
better idea of the ideal structure when we actually get more assertions
we want to allow users to make.

Signed-off-by: Will Beason <[email protected]>

* Add example violations to ReadSuite tests

Note that users can specify simply that there are violations by setting
"violations: {}" and leaving it as an empty object, and they don't have
to specify anything further.

Signed-off-by: Will Beason <[email protected]>

* Run golangci-lint run --fix

Signed-off-by: Will Beason <[email protected]>

* Responds to nits

Also refactor runner to reduce duplicate code and reduce responsibility
of individual functions.

Signed-off-by: Will Beason <[email protected]>

* Implement reviewer-suggested changes

This more closely follows the original design, with the explicit changes
discussed in the gatekeeper weekly meeting.

Signed-off-by: Will Beason <[email protected]>

* Add comprehensive unit tests for asssertions

This covers a reasonable variety of use cases for Assertions.

Signed-off-by: Will Beason <[email protected]>

* Clarify Assertions field default behavior.

Signed-off-by: Will Beason <[email protected]>

* Use k8s IntOrStr

Since IntOrStr doesn't actually support YAML, we have to make a pass
through JSON in order for this to work.

Signed-off-by: Will Beason <[email protected]>

* Add test case for when string value is invalid

Signed-off-by: Will Beason <[email protected]>

* Improve Assertions field comments

Signed-off-by: Will Beason <[email protected]>

* Resolve review comments

- Rename tests
- Refactor error messages to reduce variety of test failure messages

Signed-off-by: Will Beason <[email protected]>

* Fix lint errors

Signed-off-by: Will Beason <[email protected]>

pkg/gktest/assertion.go

@@ -0,0 +1,140 @@
+package gktest
+
+import (
+	"fmt"
+	"regexp"
+	"sync"
+
+	"github.com/open-policy-agent/frameworks/constraint/pkg/types"
+	"k8s.io/apimachinery/pkg/util/intstr"
+)
+
+// An Assertion is a declaration about the data returned by running an object
+// against a Constraint.
+type Assertion struct {
+	// Violations, if set, indicates either whether there are violations, or how
+	// many violations match this assertion.
+	//
+	// The value may be either an integer, of a string. If an integer, exactly
+	// this number of violations must otherwise match this Assertion. If a string,
+	// must be either "yes" or "no". If "yes" at least one violation must match
+	// the Assertion to be satisfied. If "no", there must be zero violations
+	// matching the Assertion to be satisfied.
+	//
+	// Defaults to "yes".
+	Violations *intstr.IntOrString `json:"violations,omitempty"`
+
+	// Message is a regular expression which matches the Msg field of individual
+	// violations.
+	//
+	// If unset, has no effect and all violations match this Assertion.
+	Message *string `json:"message,omitempty"`
+
+	onceMsgRegex sync.Once
+	msgRegex     *regexp.Regexp
+}
+
+func (a *Assertion) Run(results []*types.Result) error {
+	matching := int32(0)
+	var messages []string
+
+	for _, r := range results {
+		messages = append(messages, r.Msg)
+
+		matches, err := a.matches(r)
+		if err != nil {
+			return err
+		}
+
+		if matches {
+			matching++
+		}
+	}
+
+	// Default to assuming the object fails validation.
+	if a.Violations == nil {
+		a.Violations = intStrFromStr("yes")
+	}
+
+	err := a.matchesCount(matching)
+	if err != nil {
+		return fmt.Errorf("%w: got messages %v", err, messages)
+	}
+
+	return nil
+}
+
+func (a *Assertion) matchesCount(matching int32) error {
+	switch a.Violations.Type {
+	case intstr.Int:
+		return a.matchesCountInt(matching)
+	case intstr.String:
+		return a.matchesCountStr(matching)
+	default:
+		// Requires a bug in intstr unmarshalling code, or a misuse of the IntOrStr
+		// type in Go code.
+		return fmt.Errorf("%w: assertion.violations improperly parsed to type %d",
+			ErrInvalidYAML, a.Violations.Type)
+	}
+}
+
+func (a *Assertion) matchesCountInt(matching int32) error {
+	wantMatching := a.Violations.IntVal
+	if wantMatching != matching {
+		return fmt.Errorf("%w: got %d violations but want exactly %d",
+			ErrNumViolations, matching, wantMatching)
+	}
+
+	return nil
+}
+
+func (a *Assertion) matchesCountStr(matching int32) error {
+	switch a.Violations.StrVal {
+	case "yes":
+		if matching == 0 {
+			return fmt.Errorf("%w: got %d violations but want at least %d",
+				ErrNumViolations, matching, 1)
+		}
+
+		return nil
+	case "no":
+		if matching > 0 {
+			return fmt.Errorf("%w: got %d violations but want none",
+				ErrNumViolations, matching)
+		}
+
+		return nil
+	default:
+		return fmt.Errorf(`%w: assertion.violation, if set, must be an integer, "yes", or "no"`,
+			ErrInvalidYAML)
+	}
+}
+
+func (a *Assertion) matches(result *types.Result) (bool, error) {
+	r, err := a.getMsgRegex()
+	if err != nil {
+		return false, err
+	}
+
+	if r != nil {
+		return r.MatchString(result.Msg), nil
+	}
+
+	return true, nil
+}
+
+func (a *Assertion) getMsgRegex() (*regexp.Regexp, error) {
+	if a.Message == nil {
+		return nil, nil
+	}
+
+	var err error
+	a.onceMsgRegex.Do(func() {
+		a.msgRegex, err = regexp.Compile(*a.Message)
+	})
+	if err != nil {
+		return nil, fmt.Errorf("%w: %v", ErrInvalidRegex, err)
+	}
+
+	return a.msgRegex, nil
+}

pkg/gktest/errors.go

@@ -20,10 +20,10 @@ var (
 	ErrCreatingClient = errors.New("creating client")
 	// ErrInvalidCase indicates a Case cannot be run due to not being configured properly.
 	ErrInvalidCase = errors.New("invalid Case")
-	// ErrUnexpectedAllow indicates a Case failed because it was expected to get
-	// violations, but did not get any.
-	ErrUnexpectedAllow = errors.New("got no violations")
-	// ErrUnexpectedDeny indicates a Case failed because it got violations, but
-	// was not expected to get any.
-	ErrUnexpectedDeny = errors.New("got violations")
+	// ErrNumViolations indicates an Object did not get the expected number of
+	// violations.
+	ErrNumViolations = errors.New("unexpected number of violations")
+	// ErrInvalidRegex indicates a Case specified a Violation regex that could not
+	// be compiled.
+	ErrInvalidRegex = errors.New("message contains invalid regular expression")
 )

pkg/gktest/intstr.go

@@ -0,0 +1,13 @@
+package gktest
+
+import "k8s.io/apimachinery/pkg/util/intstr"
+
+func intStrFromInt(val int) *intstr.IntOrString {
+	result := intstr.FromInt(val)
+	return &result
+}
+
+func intStrFromStr(val string) *intstr.IntOrString {
+	result := intstr.FromString(val)
+	return &result
+}

pkg/gktest/read_suites.go

@@ -1,12 +1,13 @@
 package gktest
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io/fs"
 	"path/filepath"
 
-	"gopkg.in/yaml.v2"
+	"gopkg.in/yaml.v3"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -167,10 +168,16 @@ func readSuite(f fs.FS, path string) (*Suite, error) {
 		return nil, nil
 	}
 
+	// We must pass through JSON as IntOrStr does not unmarshal from YAML properly.
+	jsn, err := json.Marshal(u.Object)
+	if err != nil {
+		return nil, fmt.Errorf("%w: marshaling yaml to json: %v", ErrInvalidYAML, err)
+	}
+
 	suite := Suite{}
-	err = yaml.Unmarshal(bytes, &suite)
+	err = json.Unmarshal(jsn, &suite)
 	if err != nil {
-		return nil, fmt.Errorf("parsing Test %q into Suite: %w", path, err)
+		return nil, fmt.Errorf("%w: parsing Test %q into Suite: %v", ErrInvalidYAML, path, err)
 	}
 	return &suite, nil
 }

pkg/gktest/read_suites_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"k8s.io/utils/pointer"
 )
 
 func TestReadSuites(t *testing.T) {
@@ -264,8 +265,11 @@ tests:
 - template: template.yaml
   constraint: constraint.yaml
   cases:
-  - allow: allow.yaml
-  - deny: deny.yaml
+  - object: allow.yaml
+  - object: deny.yaml
+    assertions:
+    - violations: 2
+      message: "some message"
 `),
 				},
 			},
@@ -274,9 +278,49 @@ tests:
 					Template:   "template.yaml",
 					Constraint: "constraint.yaml",
 					Cases: []Case{{
-						Allow: "allow.yaml",
+						Object: "allow.yaml",
 					}, {
-						Deny: "deny.yaml",
+						Object: "deny.yaml",
+						Assertions: []Assertion{{
+							Violations: intStrFromInt(2),
+							Message:    pointer.StringPtr("some message"),
+						}},
+					}},
+				}},
+			}},
+			wantErr: nil,
+		},
+		{
+			name:      "suite with empty assertions",
+			target:    "test.yaml",
+			recursive: false,
+			fileSystem: fstest.MapFS{
+				"test.yaml": &fstest.MapFile{
+					Data: []byte(`
+kind: Suite
+apiVersion: test.gatekeeper.sh/v1alpha1
+tests:
+- template: template.yaml
+  constraint: constraint.yaml
+  cases:
+  - object: allow.yaml
+  - object: deny.yaml
+    assertions:
+    - violations: "yes"
+`),
+				},
+			},
+			want: map[string]*Suite{"test.yaml": {
+				Tests: []Test{{
+					Template:   "template.yaml",
+					Constraint: "constraint.yaml",
+					Cases: []Case{{
+						Object: "allow.yaml",
+					}, {
+						Object: "deny.yaml",
+						Assertions: []Assertion{{
+							Violations: intStrFromStr("yes"),
+						}},
 					}},
 				}},
 			}},
@@ -305,17 +349,32 @@ tests:
 			}},
 			wantErr: nil,
 		},
+		{
+			name:      "invalid suite",
+			target:    "test.yaml",
+			recursive: false,
+			fileSystem: fstest.MapFS{
+				"test.yaml": &fstest.MapFile{
+					Data: []byte(`
+kind: Suite
+apiVersion: test.gatekeeper.sh/v1alpha1
+tests: {}
+`),
+				},
+			},
+			wantErr: ErrInvalidYAML,
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			got, gotErr := ReadSuites(tc.fileSystem, tc.target, tc.recursive)
 			if !errors.Is(gotErr, tc.wantErr) {
-				t.Errorf("got error %v, want error %v",
+				t.Fatalf("got error %v, want error %v",
 					gotErr, tc.wantErr)
 			}
 
-			if diff := cmp.Diff(tc.want, got, cmpopts.EquateEmpty()); diff != "" {
+			if diff := cmp.Diff(tc.want, got, cmpopts.EquateEmpty(), cmpopts.IgnoreUnexported(Assertion{})); diff != "" {
 				t.Error(diff)
 			}
 		})