move image verification logic into preppers. replace string->prep map with array

Author: nkubala

cmd/analyze.go

@@ -33,7 +33,7 @@ var analyzeCmd = &cobra.Command{
 	Short: "Analyzes an image: [image]",
 	Long:  `Analyzes an image using the specifed analyzers as indicated via flags (see documentation for available ones).`,
 	Args: func(cmd *cobra.Command, args []string) error {
-		if err := validateArgs(args, checkAnalyzeArgNum, checkArgType); err != nil {
+		if err := validateArgs(args, checkAnalyzeArgNum); err != nil {
 			return errors.New(err.Error())
 		}
 		if err := checkIfValidAnalyzer(types); err != nil {

cmd/diff.go

@@ -34,7 +34,7 @@ var diffCmd = &cobra.Command{
 	Short: "Compare two images: [image1] [image2]",
 	Long:  `Compares two images using the specifed analyzers as indicated via flags (see documentation for available ones).`,
 	Args: func(cmd *cobra.Command, args []string) error {
-		if err := validateArgs(args, checkDiffArgNum, checkArgType); err != nil {
+		if err := validateArgs(args, checkDiffArgNum); err != nil {
 			return errors.New(err.Error())
 		}
 		if err := checkIfValidAnalyzer(types); err != nil {

cmd/root.go

@@ -24,7 +24,6 @@ import (
 	"strings"
 
 	"github.com/GoogleCloudPlatform/container-diff/differs"
-	pkgutil "github.com/GoogleCloudPlatform/container-diff/pkg/util"
 	"github.com/GoogleCloudPlatform/container-diff/util"
 	"github.com/docker/docker/client"
 	"github.com/golang/glog"
@@ -92,24 +91,6 @@ func validateArgs(args []string, validatefxns ...validatefxn) error {
 	return nil
 }
 
-func checkImage(arg string) bool {
-	if !pkgutil.CheckImageID(arg) && !pkgutil.CheckImageURL(arg) && !pkgutil.CheckTar(arg) {
-		return false
-	}
-	return true
-}
-
-func checkArgType(args []string) error {
-	for _, arg := range args {
-		if !checkImage(arg) {
-			errMessage := fmt.Sprintf("Argument %s is not an image ID, URL, or tar\n", args[0])
-			glog.Errorf(errMessage)
-			return errors.New(errMessage)
-		}
-	}
-	return nil
-}
-
 func checkIfValidAnalyzer(flagtypes string) error {
 	if flagtypes == "" {
 		return nil

cmd/root_test.go

@@ -16,32 +16,7 @@ limitations under the License.
 
 package cmd
 
-import (
-	"testing"
-)
-
 type testpair struct {
 	input           []string
 	expected_output bool
 }
-
-var argTypeTests = []testpair{
-	{[]string{"badID", "badID"}, false},
-	{[]string{"123456789012", "badID"}, false},
-	{[]string{"123456789012", "123456789012"}, true},
-	{[]string{"?!badDiffer71", "123456789012"}, false},
-	{[]string{"123456789012", "gcr.io/repo/image"}, true},
-}
-
-func TestArgType(t *testing.T) {
-	for _, test := range argTypeTests {
-		err := checkArgType(test.input)
-		if (err == nil) != test.expected_output {
-			if test.expected_output {
-				t.Errorf("Got unexpected error: %s", err)
-			} else {
-				t.Errorf("Expected error but got none")
-			}
-		}
-	}
-}

pkg/util/cloud_prepper.go

@@ -17,6 +17,8 @@ limitations under the License.
 package util
 
 import (
+	"regexp"
+
 	"github.com/containers/image/docker"
 )
 
@@ -25,7 +27,24 @@ type CloudPrepper struct {
 	ImagePrepper
 }
 
-func (p CloudPrepper) getFileSystem() (string, error) {
+func (p CloudPrepper) Name() string {
+	return "Cloud Registry"
+}
+
+func (p CloudPrepper) GetSource() string {
+	return p.ImagePrepper.Source
+}
+
+func (p CloudPrepper) SupportsImage() bool {
+	pattern := regexp.MustCompile("^.+/.+(:.+){0,1}$")
+	image := p.ImagePrepper.Source
+	if exp := pattern.FindString(image); exp != image || CheckTar(image) {
+		return false
+	}
+	return true
+}
+
+func (p CloudPrepper) GetFileSystem() (string, error) {
 	ref, err := docker.ParseReference("//" + p.Source)
 	if err != nil {
 		return "", err
@@ -34,7 +53,7 @@ func (p CloudPrepper) getFileSystem() (string, error) {
 	return getFileSystemFromReference(ref, p.Source)
 }
 
-func (p CloudPrepper) getConfig() (ConfigSchema, error) {
+func (p CloudPrepper) GetConfig() (ConfigSchema, error) {
 	ref, err := docker.ParseReference("//" + p.Source)
 	if err != nil {
 		return ConfigSchema{}, err

pkg/util/daemon_prepper.go

@@ -19,6 +19,7 @@ package util
 import (
 	"context"
 	"os"
+	"regexp"
 
 	"github.com/golang/glog"
 )
@@ -27,7 +28,23 @@ type DaemonPrepper struct {
 	ImagePrepper
 }
 
-func (p DaemonPrepper) getFileSystem() (string, error) {
+func (p DaemonPrepper) Name() string {
+	return "Local Daemon"
+}
+
+func (p DaemonPrepper) GetSource() string {
+	return p.ImagePrepper.Source
+}
+
+func (p DaemonPrepper) SupportsImage() bool {
+	pattern := regexp.MustCompile("[a-z|0-9]{12}")
+	if exp := pattern.FindString(p.ImagePrepper.Source); exp != p.ImagePrepper.Source {
+		return false
+	}
+	return true
+}
+
+func (p DaemonPrepper) GetFileSystem() (string, error) {
 	tarPath, err := saveImageToTar(p.Client, p.Source, p.Source)
 	if err != nil {
 		return "", err
@@ -37,7 +54,7 @@ func (p DaemonPrepper) getFileSystem() (string, error) {
 	return getImageFromTar(tarPath)
 }
 
-func (p DaemonPrepper) getConfig() (ConfigSchema, error) {
+func (p DaemonPrepper) GetConfig() (ConfigSchema, error) {
 	inspect, _, err := p.Client.ImageInspectWithRaw(context.Background(), p.Source)
 	if err != nil {
 		return ConfigSchema{}, err
@@ -46,14 +63,14 @@ func (p DaemonPrepper) getConfig() (ConfigSchema, error) {
 	config := ConfigObject{
 		Env: inspect.Config.Env,
 	}
-	history := p.getHistory()
+	history := p.GetHistory()
 	return ConfigSchema{
 		Config:  config,
 		History: history,
 	}, nil
 }
 
-func (p DaemonPrepper) getHistory() []ImageHistoryItem {
+func (p DaemonPrepper) GetHistory() []ImageHistoryItem {
 	history, err := p.Client.ImageHistory(context.Background(), p.Source)
 	if err != nil {
 		glog.Error("Could not obtain image history for %s: %s", p.Source, err)

pkg/util/image_prep_utils.go

@@ -30,16 +30,10 @@ import (
 	"github.com/golang/glog"
 )
 
-var sourceToPrepMap = map[string]func(ip ImagePrepper) Prepper{
-	"ID":  func(ip ImagePrepper) Prepper { return DaemonPrepper{ImagePrepper: ip} },
-	"URL": func(ip ImagePrepper) Prepper { return CloudPrepper{ImagePrepper: ip} },
-	"tar": func(ip ImagePrepper) Prepper { return TarPrepper{ImagePrepper: ip} },
-}
-
-var sourceCheckMap = map[string]func(string) bool{
-	"ID":  CheckImageID,
-	"URL": CheckImageURL,
-	"tar": CheckTar,
+var orderedPreppers = []func(ip ImagePrepper) Prepper{
+	func(ip ImagePrepper) Prepper { return DaemonPrepper{ImagePrepper: ip} },
+	func(ip ImagePrepper) Prepper { return CloudPrepper{ImagePrepper: ip} },
+	func(ip ImagePrepper) Prepper { return TarPrepper{ImagePrepper: ip} },
 }
 
 type Image struct {

pkg/util/image_prepper.go

@@ -29,31 +29,36 @@ type ImagePrepper struct {
 }
 
 type Prepper interface {
-	getFileSystem() (string, error)
-	getConfig() (ConfigSchema, error)
+	Name() string
+	GetSource() string
+	GetFileSystem() (string, error)
+	GetConfig() (ConfigSchema, error)
+	SupportsImage() bool
 }
 
 func (p ImagePrepper) GetImage() (Image, error) {
 	glog.Infof("Starting prep for image %s", p.Source)
 	img := p.Source
 
 	var prepper Prepper
-	for source, check := range sourceCheckMap {
-		if check(img) {
-			prepper = sourceToPrepMap[source](p)
+
+	for _, prepperConstructor := range orderedPreppers {
+		prepper = prepperConstructor(p)
+		if prepper.SupportsImage() {
 			break
 		}
 	}
+
 	if prepper == nil {
 		return Image{}, errors.New("Could not retrieve image from source")
 	}
 
-	imgPath, err := prepper.getFileSystem()
+	imgPath, err := prepper.GetFileSystem()
 	if err != nil {
 		return Image{}, err
 	}
 
-	config, err := prepper.getConfig()
+	config, err := prepper.GetConfig()
 	if err != nil {
 		glog.Error("Error retrieving History: ", err)
 	}

pkg/util/image_utils.go

@@ -21,7 +21,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"regexp"
 
 	"github.com/docker/docker/pkg/system"
 	"github.com/golang/glog"
@@ -42,22 +41,6 @@ func GetImageLayers(pathToImage string) []string {
 	return layers
 }
 
-func CheckImageID(image string) bool {
-	pattern := regexp.MustCompile("[a-z|0-9]{12}")
-	if exp := pattern.FindString(image); exp != image {
-		return false
-	}
-	return true
-}
-
-func CheckImageURL(image string) bool {
-	pattern := regexp.MustCompile("^.+/.+(:.+){0,1}$")
-	if exp := pattern.FindString(image); exp != image || CheckTar(image) {
-		return false
-	}
-	return true
-}
-
 // copyToFile writes the content of the reader to the specified file
 func copyToFile(outfile string, r io.Reader) error {
 	// We use sequential file access here to avoid depleting the standby list

pkg/util/tar_prepper.go

@@ -32,11 +32,23 @@ type TarPrepper struct {
 	ImagePrepper
 }
 
-func (p TarPrepper) getFileSystem() (string, error) {
+func (p TarPrepper) Name() string {
+	return "Tar Archive"
+}
+
+func (p TarPrepper) GetSource() string {
+	return p.ImagePrepper.Source
+}
+
+func (p TarPrepper) SupportsImage() bool {
+	return IsTar(p.ImagePrepper.Source)
+}
+
+func (p TarPrepper) GetFileSystem() (string, error) {
 	return getImageFromTar(p.Source)
 }
 
-func (p TarPrepper) getConfig() (ConfigSchema, error) {
+func (p TarPrepper) GetConfig() (ConfigSchema, error) {
 	tempDir := strings.TrimSuffix(p.Source, filepath.Ext(p.Source)) + "-config"
 	defer os.RemoveAll(tempDir)
 	err := UnTar(p.Source, tempDir)

util/image_utils_test.go

@@ -33,7 +33,12 @@ func TestCheckImageID(t *testing.T) {
 		{input: "gcr.io/repo/image", expectedOutput: false},
 		{input: "testTars/la-croix1.tar", expectedOutput: false},
 	} {
-		output := pkgutil.CheckImageID(test.input)
+		prepper := pkgutil.DaemonPrepper{
+			pkgutil.ImagePrepper{
+				Source: test.input,
+			},
+		}
+		output := prepper.SupportsImage()
 		if output != test.expectedOutput {
 			if test.expectedOutput {
 				t.Errorf("Expected input to be image ID but %s tested false", test.input)
@@ -67,7 +72,12 @@ func TestCheckImageURL(t *testing.T) {
 		{input: "gcr.io/repo/image", expectedOutput: true},
 		{input: "testTars/la-croix1.tar", expectedOutput: false},
 	} {
-		output := pkgutil.CheckImageURL(test.input)
+		prepper := pkgutil.CloudPrepper{
+			pkgutil.ImagePrepper{
+				Source: test.input,
+			},
+		}
+		output := prepper.SupportsImage()
 		if output != test.expectedOutput {
 			if test.expectedOutput {
 				t.Errorf("Expected input to be a tar file but %s tested false", test.input)