future-proof validateImageParts. Add custom error types.

\nSigned-off-by: davis-haba <davishaba@google.com>

pkg/mutation/mutators/assignimage/assignimage_mutator.go

@@ -112,23 +112,23 @@ func (m *Mutator) String() string {
 	return fmt.Sprintf("%s/%s/%s:%d", m.id.Kind, m.id.Namespace, m.id.Name, m.assignImage.GetGeneration())
 }
 
-// MutatorForAssign returns an mutator built from
+// MutatorForAssignImage returns a mutator built from
 // the given assignImage instance.
 func MutatorForAssignImage(assignImage *mutationsunversioned.AssignImage) (*Mutator, error) {
 	// This is not always set by the kubernetes API server
 	assignImage.SetGroupVersionKind(runtimeschema.GroupVersionKind{Group: mutationsv1beta1.GroupVersion.Group, Kind: "AssignImage"})
 
 	path, err := parser.Parse(assignImage.Spec.Location)
 	if err != nil {
-		return nil, errors.Wrapf(err, "invalid location format `%s` for Assign %s", assignImage.Spec.Location, assignImage.GetName())
+		return nil, errors.Wrapf(err, "invalid location format `%s` for assignImage %s", assignImage.Spec.Location, assignImage.GetName())
 	}
 
 	if core.HasMetadataRoot(path) {
-		return nil, fmt.Errorf("assignImage %s can't change metadata", assignImage.GetName())
+		return nil, newMetadataRootError(assignImage.GetName())
 	}
 
 	if hasListTerminal(path) {
-		return nil, fmt.Errorf("assignImage %s cannot mutate list-type fields", assignImage.GetName())
+		return nil, newListTerminalError(assignImage.GetName())
 	}
 
 	err = core.CheckKeyNotChanged(path)

pkg/mutation/mutators/assignimage/assignimage_mutator_test.go

@@ -1,7 +1,9 @@
 package assignimage
 
 import (
+	"errors"
 	"fmt"
+	"strings"
 	"testing"
 
 	mutationsunversioned "github.com/open-policy-agent/gatekeeper/apis/mutations/unversioned"
@@ -262,9 +264,9 @@ func TestMutate(t *testing.T) {
 
 func TestMutatorForAssignImage(t *testing.T) {
 	tests := []struct {
-		name    string
-		cfg     *aiTestConfig
-		wantErr bool
+		name  string
+		cfg   *aiTestConfig
+		errFn func(error) bool
 	}{
 		{
 			name: "valid assignImage",
@@ -285,7 +287,10 @@ func TestMutatorForAssignImage(t *testing.T) {
 				location: "metadata.labels.image",
 				applyTo:  []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Foo"}}},
 			},
-			wantErr: true,
+			errFn: func(err error) bool {
+				_, ok := err.(metadataRootError)
+				return ok
+			},
 		},
 		{
 			name: "terminal list returns err",
@@ -296,7 +301,10 @@ func TestMutatorForAssignImage(t *testing.T) {
 				location: "spec.containers[name:foo]",
 				applyTo:  []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Foo"}}},
 			},
-			wantErr: true,
+			errFn: func(err error) bool {
+				_, ok := err.(listTerminalError)
+				return ok
+			},
 		},
 		{
 			name: "syntactically invalid location returns err",
@@ -307,7 +315,9 @@ func TestMutatorForAssignImage(t *testing.T) {
 				location: "/x/y/zx[)",
 				applyTo:  []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Foo"}}},
 			},
-			wantErr: true,
+			errFn: func(err error) bool {
+				return strings.Contains(err.Error(), "invalid location format")
+			},
 		},
 		{
 			name: "bad assigns return err",
@@ -318,18 +328,10 @@ func TestMutatorForAssignImage(t *testing.T) {
 				location: "spec.containers[name:foo].image",
 				applyTo:  []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Foo"}}},
 			},
-			wantErr: true,
-		},
-		{
-			name: "metadata root returns err",
-			cfg: &aiTestConfig{
-				domain:   "a.b.c",
-				path:     "new/app",
-				tag:      ":latest",
-				location: "metadata.labels.image",
-				applyTo:  []match.ApplyTo{{Groups: []string{""}, Versions: []string{"v1"}, Kinds: []string{"Foo"}}},
+			errFn: func(err error) bool {
+				_, ok := errors.Unwrap(err).(domainLikePathError)
+				return ok
 			},
-			wantErr: true,
 		},
 	}
 
@@ -339,11 +341,12 @@ func TestMutatorForAssignImage(t *testing.T) {
 			if err != nil && mut != nil {
 				t.Errorf("returned non-nil mutator but got err: %s", err)
 			}
-			if !tc.wantErr && err != nil {
-				t.Errorf("did not want error but got: %v", err)
-			}
-			if tc.wantErr && err == nil {
-				t.Error("wanted error but got nil")
+			if tc.errFn != nil {
+				if err == nil {
+					t.Errorf("wanted err but  got nil")
+				} else if !tc.errFn(err) {
+					t.Errorf("got error of unexpected type: %s", err)
+				}
 			}
 		})
 	}

pkg/mutation/mutators/assignimage/errors.go

@@ -0,0 +1,81 @@
+package assignimage
+
+import "fmt"
+
+type baseError struct {
+	s string
+}
+
+func (e baseError) Error() string {
+	return e.s
+}
+
+// Component field (domain|path|tag) errors.
+type invalidDomainError struct{ baseError }
+
+type (
+	invalidPathError       struct{ baseError }
+	invalidTagError        struct{ baseError }
+	missingComponentsError struct{ baseError }
+	domainLikePathError    struct{ baseError }
+)
+
+// Location field errors.
+type listTerminalError struct{ baseError }
+type metadataRootError struct{ baseError }
+
+func newInvalidDomainError(domain string) invalidDomainError {
+	return invalidDomainError{
+		baseError{
+			fmt.Sprintf("assignDomain %q does not match pattern %s", domain, domainRegexp.String()),
+		},
+	}
+}
+
+func newInvalidPathError(path string) invalidPathError {
+	return invalidPathError{
+		baseError{
+			fmt.Sprintf("assignPath %q does not match pattern %s", path, pathRegexp.String()),
+		},
+	}
+}
+
+func newInvalidTagError(tag string) invalidTagError {
+	return invalidTagError{
+		baseError{
+			fmt.Sprintf("assignTag %q does not match pattern %s", tag, tagRegexp.String()),
+		},
+	}
+}
+
+func newMissingComponentsError() missingComponentsError {
+	return missingComponentsError{
+		baseError{
+			"at least one of [assignDomain, assignPath, assignTag] must be set",
+		},
+	}
+}
+
+func newDomainLikePathError(path string) domainLikePathError {
+	return domainLikePathError{
+		baseError{
+			fmt.Sprintf("assignDomain must be set if the first part of assignPath %q can be interpretted as part of a domain", path),
+		},
+	}
+}
+
+func newListTerminalError(name string) listTerminalError {
+	return listTerminalError{
+		baseError{
+			fmt.Sprintf("assignImage %s cannot mutate list-type fields", name),
+		},
+	}
+}
+
+func newMetadataRootError(name string) metadataRootError {
+	return metadataRootError{
+		baseError{
+			fmt.Sprintf("assignImage %s can't change metadata", name),
+		},
+	}
+}

pkg/mutation/mutators/assignimage/imageparser.go

@@ -13,7 +13,8 @@ var (
 	// [@:/], into components that would cause that component to be split the next
 	// time the mutation is applied and "leak" to its neighbor. Some validation is
 	// done as regex on individual components, and other validation which looks at
-	// multiple components together is done in code.
+	// multiple components together is done in code. All validation for domain and
+	// tag must be put in validateDomain and validateTag respectively.
 
 	// domainRegexp defines a schema for a domain component.
 	domainRegexp = regexp.MustCompile(`(^\w[\w\-_]*\.[\w\-_\.]*[\w](:\d+)?$)|(^localhost(:\d+)?$)`)
@@ -43,16 +44,22 @@ func mutateImage(domain, path, tag, mutableImgRef string) string {
 
 func newImage(imageRef string) image {
 	domain, remainder := splitDomain(imageRef)
-	var pt string
+	path, tag := splitTag(remainder)
+	return image{domain: domain, path: path, tag: tag}
+}
+
+// splitTag separates the path and tag components from a string.
+func splitTag(remainder string) (string, string) {
+	var path string
 	tag := ""
 	if tagSep := strings.IndexAny(remainder, ":@"); tagSep > -1 {
-		pt = remainder[:tagSep]
+		path = remainder[:tagSep]
 		tag = remainder[tagSep:]
 	} else {
-		pt = remainder
+		path = remainder
 	}
 
-	return image{domain: domain, path: pt, tag: tag}
+	return path, tag
 }
 
 func (img image) newMutatedImage(domain, path, tag string) image {
@@ -93,29 +100,50 @@ func validateDomain(domain string) error {
 	}
 
 	if !domainRegexp.MatchString(domain) {
-		return fmt.Errorf("assignDomain %q does not match pattern %s", domain, domainRegexp.String())
+		return newInvalidDomainError(domain)
 	}
 
+	// The error below should theoretically be unreachable, as the regex
+	// validation should preclude this from happening. This check is included
+	// anyway to prevent code drift, and ensure that if a domain is validated
+	// it can also be recognized as a domain.
 	if d, r := splitDomain(domain + "/"); d != domain || r != "" {
-		return fmt.Errorf("domain %q could not be regognized as a valid domain", domain)
+		return fmt.Errorf("domain %q could not be recognized as a valid domain", domain)
+	}
+
+	return nil
+}
+
+func validateTag(tag string) error {
+	if tag == "" {
+		return nil
+	}
+
+	if !tagRegexp.MatchString(tag) {
+		return newInvalidTagError(tag)
+	}
+
+	// This error should never happen, but the check is included to prevent drift.
+	if _, t := splitTag(tag); t != tag {
+		return fmt.Errorf("tag %q could not be recognized as a valid tag or digest", tag)
 	}
 
 	return nil
 }
 
 func validateImageParts(domain, path, tag string) error {
 	if domain == "" && path == "" && tag == "" {
-		return fmt.Errorf("at least one of [assignDomain, assignPath, assignTag] must be set")
+		return newMissingComponentsError()
 	}
 	if err := validateDomain(domain); err != nil {
 		return err
 	}
 	// match the whole string for path (anchoring with `$` is tricky here)
 	if path != "" && path != pathRegexp.FindString(path) {
-		return fmt.Errorf("assignPath %q does not match pattern %s", path, pathRegexp.String())
+		return newInvalidPathError(path)
 	}
-	if tag != "" && !tagRegexp.MatchString(tag) {
-		return fmt.Errorf("assignTag %q does not match pattern %s", tag, tagRegexp.String())
+	if err := validateTag(tag); err != nil {
+		return err
 	}
 
 	// Check if the path looks like a domain string, and the domain is not set.
@@ -127,7 +155,7 @@ func validateImageParts(domain, path, tag string) error {
 	// the domain component, so the result would be "gcr.io/gcr.io/repo" and so on.
 	if domain == "" {
 		if d, _ := splitDomain(path); d != "" {
-			return fmt.Errorf("assignDomain must be set if the first part of assignPath %q can be interpretted as part of a domain", path)
+			return newDomainLikePathError(path)
 		}
 	}
 

pkg/mutation/mutators/assignimage/imageparser_test.go

@@ -1,7 +1,6 @@
 package assignimage
 
 import (
-	"fmt"
 	"strings"
 	"testing"
 )
@@ -161,31 +160,36 @@ func TestNewImage(t *testing.T) {
 
 func isDomainError(domain string) func(error) bool {
 	return func(err error) bool {
-		return strings.Contains(err.Error(), fmt.Sprintf("assignDomain %q does not match pattern", domain))
+		_, ok := err.(invalidDomainError)
+		return ok && strings.Contains(err.Error(), domain)
 	}
 }
 
 func isPathError(path string) func(error) bool {
 	return func(err error) bool {
-		return strings.Contains(err.Error(), fmt.Sprintf("assignPath %q does not match pattern", path))
+		_, ok := err.(invalidPathError)
+		return ok && strings.Contains(err.Error(), path)
 	}
 }
 
 func isTagError(tag string) func(error) bool {
 	return func(err error) bool {
-		return strings.Contains(err.Error(), fmt.Sprintf("assignTag %q does not match pattern", tag))
+		_, ok := err.(invalidTagError)
+		return ok && strings.Contains(err.Error(), tag)
 	}
 }
 
 func isEmptyArgsError() func(error) bool {
 	return func(err error) bool {
-		return strings.Contains(err.Error(), "at least one of")
+		_, ok := err.(missingComponentsError)
+		return ok
 	}
 }
 
 func isPathlikeDomainError() func(error) bool {
 	return func(err error) bool {
-		return strings.Contains(err.Error(), "assignDomain must be set if")
+		_, ok := err.(domainLikePathError)
+		return ok
 	}
 }