From 3ceb9a13c9b9a35474c0111e4671d7b982728a17 Mon Sep 17 00:00:00 2001 From: davis-haba Date: Wed, 21 Dec 2022 20:44:39 -0800 Subject: [PATCH] future-proof validateImageParts. Add custom error types. \nSigned-off-by: davis-haba --- .../assignimage/assignimage_mutator.go | 8 +- .../assignimage/assignimage_mutator_test.go | 47 ++++++----- pkg/mutation/mutators/assignimage/errors.go | 81 +++++++++++++++++++ .../mutators/assignimage/imageparser.go | 52 +++++++++--- .../mutators/assignimage/imageparser_test.go | 16 ++-- 5 files changed, 160 insertions(+), 44 deletions(-) create mode 100644 pkg/mutation/mutators/assignimage/errors.go diff --git a/pkg/mutation/mutators/assignimage/assignimage_mutator.go b/pkg/mutation/mutators/assignimage/assignimage_mutator.go index 037800d1c14..b96b9f640d3 100644 --- a/pkg/mutation/mutators/assignimage/assignimage_mutator.go +++ b/pkg/mutation/mutators/assignimage/assignimage_mutator.go @@ -112,7 +112,7 @@ 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 @@ -120,15 +120,15 @@ func MutatorForAssignImage(assignImage *mutationsunversioned.AssignImage) (*Muta 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) diff --git a/pkg/mutation/mutators/assignimage/assignimage_mutator_test.go b/pkg/mutation/mutators/assignimage/assignimage_mutator_test.go index ca838d22c83..97b93d7303a 100644 --- a/pkg/mutation/mutators/assignimage/assignimage_mutator_test.go +++ b/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) + } } }) } diff --git a/pkg/mutation/mutators/assignimage/errors.go b/pkg/mutation/mutators/assignimage/errors.go new file mode 100644 index 00000000000..c54906f765c --- /dev/null +++ b/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), + }, + } +} diff --git a/pkg/mutation/mutators/assignimage/imageparser.go b/pkg/mutation/mutators/assignimage/imageparser.go index 42a39f3817b..26b56f06659 100644 --- a/pkg/mutation/mutators/assignimage/imageparser.go +++ b/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,11 +100,32 @@ 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 @@ -105,17 +133,17 @@ func validateDomain(domain string) error { 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) } } diff --git a/pkg/mutation/mutators/assignimage/imageparser_test.go b/pkg/mutation/mutators/assignimage/imageparser_test.go index ee6b7a191b4..bff316d8249 100644 --- a/pkg/mutation/mutators/assignimage/imageparser_test.go +++ b/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 } }