Handle duplicate archive and package specs; handle multifile archives better (#1018)

Archives with multiple files should have all files in the inputs.
Also clean up some inconsistent variable naming -- archive name,
archive file path, and archive input files were all stored in
incorrectly-named variables.

common.go

@@ -28,12 +28,10 @@ import (
 	"strings"
 	"syscall"
 
-	log "github.com/sirupsen/logrus"
-
 	"github.com/gorilla/handlers"
 	"github.com/imdario/mergo"
 	"github.com/mholt/archiver"
-	uuid "github.com/satori/go.uuid"
+	"github.com/satori/go.uuid"
 	apiv1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -144,18 +142,27 @@ func GetTempDir() (string, error) {
 	return dir, err
 }
 
-func MakeArchive(targetName string, globs ...string) (string, error) {
+// FindAllGlobs returns a list of globs of input list.
+func FindAllGlobs(inputList []string) ([]string, error) {
 	files := make([]string, 0)
-	for _, glob := range globs {
+	for _, glob := range inputList {
 		f, err := filepath.Glob(glob)
 		if err != nil {
-			log.Warn(fmt.Sprintf("Invalid glob %v: %v", glob, err))
-			return "", err
+			return nil, fmt.Errorf("Invalid glob %v: %v", glob, err)
 		}
 		files = append(files, f...)
 	}
+	return files, nil
+}
+
+func MakeArchive(targetName string, globs ...string) (string, error) {
+	files, err := FindAllGlobs(globs)
+	if err != nil {
+		return "", err
+	}
+
 	// zip up the file list
-	err := archiver.Zip.Make(targetName, files)
+	err = archiver.Zip.Make(targetName, files)
 	if err != nil {
 		return "", err
 	}

fission/function.go

@@ -193,6 +193,7 @@ func fnCreate(c *cli.Context) error {
 		spec = true
 		specFile = fmt.Sprintf("function-%v.yaml", fnName)
 	}
+	specDir := getSpecDir(c)
 
 	// check for unique function names within a namespace
 	fnList, err := client.FunctionList(fnNamespace)
@@ -252,25 +253,25 @@ func fnCreate(c *cli.Context) error {
 			}
 		}
 
-		srcArchiveName := c.StringSlice("src")
-		var deployArchiveName []string
-		codeFlag := false
+		srcArchiveFiles := c.StringSlice("src")
+		var deployArchiveFiles []string
+		noZip := false
 		code := c.String("code")
 		if len(code) == 0 {
-			deployArchiveName = c.StringSlice("deploy")
+			deployArchiveFiles = c.StringSlice("deploy")
 		} else {
-			deployArchiveName = append(deployArchiveName, c.String("code"))
-			codeFlag = true
+			deployArchiveFiles = append(deployArchiveFiles, c.String("code"))
+			noZip = true
 		}
 		// fatal when both src & deploy archive are empty
-		if len(srcArchiveName) == 0 && len(deployArchiveName) == 0 {
+		if len(srcArchiveFiles) == 0 && len(deployArchiveFiles) == 0 {
 			log.Fatal("Need --deploy or --src argument.")
 		}
 
 		buildcmd := c.String("buildcmd")
 
 		// create new package in the same namespace as the function.
-		pkgMetadata = createPackage(client, fnNamespace, envName, envNamespace, srcArchiveName, deployArchiveName, buildcmd, specFile, codeFlag)
+		pkgMetadata = createPackage(client, fnNamespace, envName, envNamespace, srcArchiveFiles, deployArchiveFiles, buildcmd, specDir, specFile, noZip)
 
 		fmt.Printf("package '%v' created\n", pkgMetadata.Name)
 	}
@@ -471,17 +472,17 @@ func fnUpdate(c *cli.Context) error {
 		envNamespace = ""
 	}
 
-	var deployArchiveName []string
+	var deployArchiveFiles []string
 	codeFlag := false
 	code := c.String("code")
 	if len(code) == 0 {
-		deployArchiveName = c.StringSlice("deploy")
+		deployArchiveFiles = c.StringSlice("deploy")
 	} else {
-		deployArchiveName = append(deployArchiveName, c.String("code"))
+		deployArchiveFiles = append(deployArchiveFiles, c.String("code"))
 		codeFlag = true
 	}
 
-	srcArchiveName := c.StringSlice("src")
+	srcArchiveFiles := c.StringSlice("src")
 	pkgName := c.String("pkg")
 	entrypoint := c.String("entrypoint")
 	buildcmd := c.String("buildcmd")
@@ -490,7 +491,7 @@ func fnUpdate(c *cli.Context) error {
 	secretName := c.String("secret")
 	cfgMapName := c.String("configmap")
 
-	if len(srcArchiveName) > 0 && len(deployArchiveName) > 0 {
+	if len(srcArchiveFiles) > 0 && len(deployArchiveFiles) > 0 {
 		log.Fatal("Need either of --src or --deploy and not both arguments.")
 	}
 
@@ -566,7 +567,7 @@ func fnUpdate(c *cli.Context) error {
 
 	pkgMetadata := &pkg.Metadata
 
-	if len(deployArchiveName) != 0 || len(srcArchiveName) != 0 || len(buildcmd) != 0 || len(envName) != 0 || len(envNamespace) != 0 {
+	if len(deployArchiveFiles) != 0 || len(srcArchiveFiles) != 0 || len(buildcmd) != 0 || len(envName) != 0 || len(envNamespace) != 0 {
 		fnList, err := getFunctionsByPackage(client, pkg.Metadata.Name, pkg.Metadata.Namespace)
 		util.CheckErr(err, "get function list")
 
@@ -574,7 +575,7 @@ func fnUpdate(c *cli.Context) error {
 			log.Fatal("Package is used by multiple functions, use --force to force update")
 		}
 
-		pkgMetadata, err = updatePackage(client, pkg, envName, envNamespace, srcArchiveName, deployArchiveName, buildcmd, false, codeFlag)
+		pkgMetadata, err = updatePackage(client, pkg, envName, envNamespace, srcArchiveFiles, deployArchiveFiles, buildcmd, false, codeFlag)
 		util.CheckErr(err, fmt.Sprintf("update package '%v'", pkgName))
 
 		fmt.Printf("package '%v' updated\n", pkgMetadata.GetName())

fission/main.go

@@ -100,7 +100,7 @@ func newCliApp() *cli.App {
 	fnEnvNameFlag := cli.StringFlag{Name: "env", Usage: "environment name for function"}
 	fnCodeFlag := cli.StringFlag{Name: "code", Usage: "local path or URL for source code"}
 	fnDeployArchiveFlag := cli.StringSliceFlag{Name: "deployarchive, deploy", Usage: "local path or URL for deployment archive"}
-	fnSrcArchiveFlag := cli.StringSliceFlag{Name: "sourcearchive, src", Usage: "local path or URL for source archive"}
+	fnSrcArchiveFlag := cli.StringSliceFlag{Name: "sourcearchive, src, source", Usage: "local path or URL for source archive"}
 	fnPkgNameFlag := cli.StringFlag{Name: "pkgname, pkg", Usage: "Name of the existing package (--deploy and --src and --env will be ignored), should be in the same namespace as the function"}
 	fnPodFlag := cli.StringFlag{Name: "pod", Usage: "function pod name, optional (use latest if unspecified)"}
 	fnFollowFlag := cli.BoolFlag{Name: "follow, f", Usage: "specify if the logs should be streamed"}

fission/package.go

@@ -30,7 +30,6 @@ import (
 	"path/filepath"
 	"strings"
 	"text/tabwriter"
-	"time"
 
 	"github.com/dchest/uniuri"
 	"github.com/fission/fission/fission/util"
@@ -84,16 +83,15 @@ func pkgCreate(c *cli.Context) error {
 		log.Fatal("Need --env argument.")
 	}
 	envNamespace := c.String("envNamespace")
-	srcArchive := c.StringSlice("src")
-	deployArchive := c.StringSlice("deploy")
+	srcArchiveFiles := c.StringSlice("src")
+	deployArchiveFiles := c.StringSlice("deploy")
 	buildcmd := c.String("buildcmd")
 
-	if len(srcArchive) == 0 && len(deployArchive) == 0 {
+	if len(srcArchiveFiles) == 0 && len(deployArchiveFiles) == 0 {
 		log.Fatal("Need --src to specify source archive, or use --deploy to specify deployment archive.")
 	}
 
-	meta := createPackage(client, pkgNamespace, envName, envNamespace, srcArchive, deployArchive, buildcmd, "", false)
-	fmt.Printf("Package '%v' created\n", meta.GetName())
+	createPackage(client, pkgNamespace, envName, envNamespace, srcArchiveFiles, deployArchiveFiles, buildcmd, "", "", false)
 
 	return nil
 }
@@ -110,15 +108,15 @@ func pkgUpdate(c *cli.Context) error {
 	force := c.Bool("f")
 	envName := c.String("env")
 	envNamespace := c.String("envNamespace")
-	srcArchive := c.StringSlice("src")
-	deployArchive := c.StringSlice("deploy")
+	srcArchiveFiles := c.StringSlice("src")
+	deployArchiveFiles := c.StringSlice("deploy")
 	buildcmd := c.String("buildcmd")
 
-	if len(srcArchive) > 0 && len(deployArchive) > 0 {
+	if len(srcArchiveFiles) > 0 && len(deployArchiveFiles) > 0 {
 		log.Fatal("Need either of --src or --deploy and not both arguments.")
 	}
 
-	if len(srcArchive) == 0 && len(deployArchive) == 0 &&
+	if len(srcArchiveFiles) == 0 && len(deployArchiveFiles) == 0 &&
 		len(envName) == 0 && len(buildcmd) == 0 {
 		log.Fatal("Need --env or --src or --deploy or --buildcmd argument.")
 	}
@@ -148,7 +146,7 @@ func pkgUpdate(c *cli.Context) error {
 	}
 
 	newPkgMeta, err := updatePackage(client, pkg,
-		envName, envNamespace, srcArchive, deployArchive, buildcmd, false, false)
+		envName, envNamespace, srcArchiveFiles, deployArchiveFiles, buildcmd, false, false)
 	if err != nil {
 		util.CheckErr(err, "update package")
 	}
@@ -166,7 +164,7 @@ func pkgUpdate(c *cli.Context) error {
 }
 
 func updatePackage(client *client.Client, pkg *crd.Package, envName, envNamespace string,
-	srcArchive []string, deployArchive []string, buildcmd string, forceRebuild bool, codeFlag bool) (*metav1.ObjectMeta, error) {
+	srcArchiveFiles []string, deployArchiveFiles []string, buildcmd string, forceRebuild bool, noZip bool) (*metav1.ObjectMeta, error) {
 
 	var srcArchiveMetadata, deployArchiveMetadata *fission.Archive
 	needToBuild := false
@@ -186,21 +184,14 @@ func updatePackage(client *client.Client, pkg *crd.Package, envName, envNamespac
 		needToBuild = true
 	}
 
-	if len(srcArchive) > 0 {
-		srcArchiveName := archiveParser(srcArchive, envName)
-		srcArchiveMetadata = createArchive(client, srcArchiveName, "")
+	if len(srcArchiveFiles) > 0 {
+		srcArchiveMetadata = createArchive(client, srcArchiveFiles, false, "", "")
 		pkg.Spec.Source = *srcArchiveMetadata
 		needToBuild = true
 	}
 
-	if len(deployArchive) > 0 {
-		var deployArchiveName string
-		if codeFlag {
-			deployArchiveName = deployArchive[0]
-		} else {
-			deployArchiveName = archiveParser(deployArchive, envName)
-		}
-		deployArchiveMetadata = createArchive(client, deployArchiveName, "")
+	if len(deployArchiveFiles) > 0 {
+		deployArchiveMetadata = createArchive(client, deployArchiveFiles, noZip, "", "")
 		pkg.Spec.Deployment = *deployArchiveMetadata
 		// Users may update the env, envNS and deploy archive at the same time,
 		// but without the source archive. In this case, we should set needToBuild to false
@@ -481,25 +472,31 @@ func fileChecksum(fileName string) (*fission.Checksum, error) {
 	}, nil
 }
 
-// upload a file and return a fission.Archive
-func createArchive(client *client.Client, fileName string, specFile string) *fission.Archive {
-	var archive fission.Archive
-
-	// fetch archive from arbitrary url if fileName is a url
-	if strings.HasPrefix(fileName, "http://") || strings.HasPrefix(fileName, "https://") {
-		fileName = downloadToTempFile(fileName)
-	}
-
+// Return a fission.Archive made from an archive .  If specFile, then
+// create an archive upload spec in the specs directory; otherwise
+// upload the archive using client.  noZip avoids zipping the
+// includeFiles, but is ignored if there's more than one includeFile.
+func createArchive(client *client.Client, includeFiles []string, noZip bool, specDir string, specFile string) *fission.Archive {
 	if len(specFile) > 0 {
 		// create an ArchiveUploadSpec and reference it from the archive
 		aus := &ArchiveUploadSpec{
-			Name:         util.KubifyName(path.Base(fileName)),
-			IncludeGlobs: []string{fileName},
+			Name:         archiveName("", includeFiles),
+			IncludeGlobs: includeFiles,
 		}
-		// save the uploadspec
-		err := specSave(*aus, specFile)
-		util.CheckErr(err, fmt.Sprintf("write spec file %v", specFile))
-		// create the archive
+
+		// check if this AUS exists in the specs; if so, don't create a new one
+		fr, err := readSpecs(specDir)
+		util.CheckErr(err, "read specs")
+		if m := fr.specExists(aus, false, true); m != nil {
+			fmt.Printf("Re-using previously created archive %v\n", m.Name)
+			aus.Name = m.Name
+		} else {
+			// save the uploadspec
+			err := specSave(*aus, specFile)
+			util.CheckErr(err, fmt.Sprintf("write spec file %v", specFile))
+		}
+
+		// create the archive object
 		ar := &fission.Archive{
 			Type: fission.ArchiveTypeUrl,
 			URL:  fmt.Sprintf("%v%v", ARCHIVE_URL_PREFIX, aus.Name),
@@ -507,10 +504,22 @@ func createArchive(client *client.Client, fileName string, specFile string) *fis
 		return ar
 	}
 
+	archivePath := makeArchiveFileIfNeeded("", includeFiles, noZip)
+
+	return uploadArchive(client, archivePath)
+}
+
+func uploadArchive(client *client.Client, fileName string) *fission.Archive {
+	var archive fission.Archive
+
+	// If filename is a URL, download it first
+	if strings.HasPrefix(fileName, "http://") || strings.HasPrefix(fileName, "https://") {
+		fileName = downloadToTempFile(fileName)
+	}
+
 	if fileSize(fileName) < fission.ArchiveLiteralSizeLimit {
-		contents := getContents(fileName)
 		archive.Type = fission.ArchiveTypeLiteral
-		archive.Literal = contents
+		archive.Literal = getContents(fileName)
 	} else {
 		u := strings.TrimSuffix(client.Url, "/") + "/proxy/storage"
 		ssClient := storageSvcClient.MakeClient(u)
@@ -539,7 +548,7 @@ func createArchive(client *client.Client, fileName string, specFile string) *fis
 	return &archive
 }
 
-func createPackage(client *client.Client, pkgNamespace string, envName string, envNamespace string, srcArchive []string, deployArchive []string, buildcmd string, specFile string, codeFlag bool) *metav1.ObjectMeta {
+func createPackage(client *client.Client, pkgNamespace string, envName string, envNamespace string, srcArchiveFiles []string, deployArchiveFiles []string, buildcmd string, specDir string, specFile string, noZip bool) *metav1.ObjectMeta {
 	pkgSpec := fission.PackageSpec{
 		Environment: fission.EnvironmentReference{
 			Namespace: envNamespace,
@@ -549,25 +558,17 @@ func createPackage(client *client.Client, pkgNamespace string, envName string, e
 	var pkgStatus fission.BuildStatus = fission.BuildStatusSucceeded
 
 	var pkgName string
-	if len(deployArchive) > 0 {
+	if len(deployArchiveFiles) > 0 {
 		if len(specFile) > 0 { // we should do this in all cases, i think
 			pkgStatus = fission.BuildStatusNone
 		}
-		var deployArchiveName string
-		if codeFlag {
-			deployArchiveName = deployArchive[0]
-		} else {
-			deployArchiveName = archiveParser(deployArchive, envName)
-		}
-		pkgSpec.Deployment = *createArchive(client, deployArchiveName, specFile)
-		pkgName = util.KubifyName(fmt.Sprintf("%v-%v", path.Base(deployArchiveName), uniuri.NewLen(4)))
+		pkgSpec.Deployment = *createArchive(client, deployArchiveFiles, noZip, specDir, specFile)
+		pkgName = util.KubifyName(fmt.Sprintf("%v-%v", path.Base(deployArchiveFiles[0]), uniuri.NewLen(4)))
 	}
-	if len(srcArchive) > 0 {
-		srcArchiveName := archiveParser(srcArchive, envName)
-		pkgSpec.Source = *createArchive(client, srcArchiveName, specFile)
-		// set pending status to package
-		pkgStatus = fission.BuildStatusPending
-		pkgName = util.KubifyName(fmt.Sprintf("%v-%v", path.Base(srcArchiveName), uniuri.NewLen(4)))
+	if len(srcArchiveFiles) > 0 {
+		pkgSpec.Source = *createArchive(client, srcArchiveFiles, false, specDir, specFile)
+		pkgStatus = fission.BuildStatusPending // set package build status to pending
+		pkgName = util.KubifyName(fmt.Sprintf("%v-%v", path.Base(srcArchiveFiles[0]), uniuri.NewLen(4)))
 	}
 
 	if len(buildcmd) > 0 {
@@ -589,12 +590,21 @@ func createPackage(client *client.Client, pkgNamespace string, envName string, e
 	}
 
 	if len(specFile) > 0 {
-		err := specSave(*pkg, specFile)
+		// if a package sith the same spec exists, don't create a new spec file
+		fr, err := readSpecs(getSpecDir(nil))
+		util.CheckErr(err, "read specs")
+		if m := fr.specExists(pkg, false, true); m != nil {
+			fmt.Printf("Re-using previously created package %v\n", m.Name)
+			return m
+		}
+
+		err = specSave(*pkg, specFile)
 		util.CheckErr(err, "save package spec")
 		return &pkg.Metadata
 	} else {
 		pkgMetadata, err := client.PackageCreate(pkg)
 		util.CheckErr(err, "create package")
+		fmt.Printf("Package '%v' created\n", pkgMetadata.GetName())
 		return pkgMetadata
 	}
 }
@@ -669,22 +679,63 @@ func downloadURL(fileUrl string) (io.ReadCloser, error) {
 	return resp.Body, nil
 }
 
-func archiveParser(archiveInput []string, envName string) string {
-	var archiveName = ""
+// Create an archive from the given list of input files, unless that
+// list has only one item and that item is either a zip file or a URL.
+//
+// If the inputs have only one file and noZip is true, the file is
+// returned as-is with no zipping.  (This is used for compatibility
+// with v1 envs.)  noZip is IGNORED if there is more than one input
+// file.
+func makeArchiveFileIfNeeded(archiveNameHint string, archiveInput []string, noZip bool) string {
 
-	if (len(archiveInput) == 1 && archiver.Zip.Match(archiveInput[0])) ||
-		(len(archiveInput) == 1 && (strings.HasPrefix(archiveInput[0], "http://") || strings.HasPrefix(archiveInput[0], "https://"))) {
-		return archiveInput[0]
+	// Unique name for the archive
+	archiveName := archiveName(archiveNameHint, archiveInput)
+
+	// Get files from inputs as number of files decide next steps
+	files, err := fission.FindAllGlobs(archiveInput)
+	if err != nil {
+		util.CheckErr(err, "finding all globs")
 	}
 
+	// We have one file; if it's a zip file or a URL, no need to archive it
+	if len(files) == 1 {
+		// make sure it exists
+		if _, err := os.Stat(files[0]); err != nil {
+			util.CheckErr(err, fmt.Sprintf("open input file %v", files[0]))
+		}
+
+		// if it's an existing zip file OR we're not supposed to zip it, don't do anything
+		if archiver.Zip.Match(files[0]) || noZip {
+			return files[0]
+		}
+
+		// if it's an HTTP URL, just use the URL.
+		if strings.HasPrefix(files[0], "http://") || strings.HasPrefix(files[0], "https://") {
+			return files[0]
+		}
+	}
+
+	// For anything else, create a new archive
 	tmpDir, err := fission.GetTempDir()
 	if err != nil {
-		util.CheckErr(err, "create archive file")
+		util.CheckErr(err, "create temporary archive directory")
 	}
-	archiveName, err = fission.MakeArchive(filepath.Join(tmpDir, fmt.Sprintf("%v-%v", envName, time.Now().Unix())), archiveInput...)
+
+	archivePath, err := fission.MakeArchive(filepath.Join(tmpDir, archiveName), archiveInput...)
 	if err != nil {
 		util.CheckErr(err, "create archive file")
 	}
 
-	return archiveName
+	return archivePath
+}
+
+// Name an archive
+func archiveName(givenNameHint string, includedFiles []string) string {
+	if len(givenNameHint) > 0 {
+		return fmt.Sprintf("%v-%v", givenNameHint, uniuri.NewLen(4))
+	}
+	if len(includedFiles) == 0 {
+		return uniuri.NewLen(8)
+	}
+	return fmt.Sprintf("%v-%v", util.KubifyName(includedFiles[0]), uniuri.NewLen(4))
 }

fission/spec.go

@@ -123,7 +123,10 @@ type (
 )
 
 func getSpecDir(c *cli.Context) string {
-	specDir := c.String("specdir")
+	specDir := ""
+	if c != nil {
+		specDir = c.String("specdir")
+	}
 	if len(specDir) == 0 {
 		specDir = "specs"
 	}
@@ -854,7 +857,8 @@ func applyArchives(fclient *client.Client, specDir string, fr *FissionResources)
 		} else {
 			// doesn't exist, upload
 			fmt.Printf("uploading archive %v\n", name)
-			uploadedAr := createArchive(fclient, ar.URL, "")
+			// ar.URL is actually a local filename at this stage
+			uploadedAr := uploadArchive(fclient, ar.URL)
 			archiveFiles[name] = *uploadedAr
 		}
 	}
@@ -1779,3 +1783,40 @@ func specSave(resource interface{}, specFile string) error {
 	}
 	return nil
 }
+
+// Returns metadata if the given resource exists in the specs, nil
+// otherwise.  compareMetadata and compareSpec control how the
+// equality check is performed.
+func (fr *FissionResources) specExists(resource interface{}, compareMetadata bool, compareSpec bool) *metav1.ObjectMeta {
+	switch typedres := resource.(type) {
+	case *ArchiveUploadSpec:
+		for _, aus := range fr.archiveUploadSpecs {
+			if compareMetadata && aus.Name != typedres.Name {
+				continue
+			}
+			if compareSpec &&
+				!(reflect.DeepEqual(aus.RootDir, typedres.RootDir) &&
+					reflect.DeepEqual(aus.IncludeGlobs, typedres.IncludeGlobs) &&
+					reflect.DeepEqual(aus.ExcludeGlobs, typedres.ExcludeGlobs)) {
+				continue
+			}
+			return &metav1.ObjectMeta{Name: aus.Name}
+		}
+		return nil
+	case *crd.Package:
+		for _, p := range fr.packages {
+			if compareMetadata && !reflect.DeepEqual(p.Metadata, typedres.Metadata) {
+				continue
+			}
+			if compareSpec && !reflect.DeepEqual(p.Spec, typedres.Spec) {
+				continue
+			}
+			return &p.Metadata
+		}
+		return nil
+
+	default:
+		// XXX not implemented
+		return nil
+	}
+}

fission/upgrade.go

@@ -293,7 +293,7 @@ func upgradeRestoreState(c *cli.Context) error {
 		tmpfile.Close()
 
 		// upload
-		archive := createArchive(client, tmpfile.Name(), "")
+		archive := uploadArchive(client, tmpfile.Name())
 		os.Remove(tmpfile.Name())
 
 		// create pkg