Fix bug with DockerExecutor's CopyFile (#1275)

The check to see if the source path was in the tgz archive was wrong
when source path was a folder, the arguments to strings.Contains were
inverted.

util/file/fileutil.go

@@ -7,37 +7,27 @@ import (
 	"encoding/base64"
 	"io"
 	"io/ioutil"
-	"os"
 	"strings"
 
 	log "github.com/sirupsen/logrus"
 )
 
-// IsFileOrDirExistInGZip return true if file or directory exists in GZip file
-func IsFileOrDirExistInGZip(sourcePath string, gzipFilePath string) bool {
-
-	fi, err := os.Open(gzipFilePath)
-
-	if os.IsNotExist(err) {
-		return false
-	}
-	defer close(fi)
+type TarReader interface {
+	Next() (*tar.Header, error)
+}
 
-	fz, err := gzip.NewReader(fi)
-	if err != nil {
-		return false
-	}
-	tr := tar.NewReader(fz)
+// ExistsInTar return true if file or directory exists in tar
+func ExistsInTar(sourcePath string, tarReader TarReader) bool {
+	sourcePath = strings.Trim(sourcePath, "/")
 	for {
-		hdr, err := tr.Next()
+		hdr, err := tarReader.Next()
 		if err == io.EOF {
 			break
 		}
 		if err != nil {
-
 			return false
 		}
-		if hdr.FileInfo().IsDir() && strings.Contains(strings.Trim(hdr.Name, "/"), strings.Trim(sourcePath, "/")) {
+		if hdr.FileInfo().IsDir() && strings.Contains(sourcePath, strings.Trim(hdr.Name, "/")) {
 			return true
 		}
 		if strings.Contains(sourcePath, hdr.Name) && hdr.Size > 0 {

util/file/fileutil_test.go

@@ -1,9 +1,12 @@
-package file
+package file_test
 
 import (
-	"testing"
-
+	"archive/tar"
+	"bytes"
+	"github.com/argoproj/argo/util/file"
 	"github.com/stretchr/testify/assert"
+	"os"
+	"testing"
 )
 
 // TestResubmitWorkflowWithOnExit ensures we do not carry over the onExit node even if successful
@@ -13,9 +16,106 @@ func TestCompressContentString(t *testing.T) {
 		"\"Succeeded\",\"boundaryID\":\"pod-limits-rrdm8\",\"startedAt\":\"2019-03-07T19:14:50Z\",\"finishedAt\":" +
 		"\"2019-03-07T19:14:55Z\"}}"
 
-	compString := CompressEncodeString(content)
+	compString := file.CompressEncodeString(content)
 
-	resultString, _ := DecodeDecompressString(compString)
+	resultString, _ := file.DecodeDecompressString(compString)
 
 	assert.Equal(t, content, resultString)
 }
+
+func TestExistsInTar(t *testing.T) {
+	type fakeFile struct {
+		name, body string
+		isDir      bool
+	}
+
+	newTarReader := func(t *testing.T, files []fakeFile) *tar.Reader {
+		var buf bytes.Buffer
+		writer := tar.NewWriter(&buf)
+		for _, f := range files {
+			mode := os.FileMode(0600)
+			if f.isDir {
+				mode |= os.ModeDir
+			}
+			hdr := tar.Header{Name: f.name, Mode: int64(mode), Size: int64(len(f.body))}
+			err := writer.WriteHeader(&hdr)
+			assert.Nil(t, err)
+			_, err = writer.Write([]byte(f.body))
+			assert.Nil(t, err)
+		}
+		err := writer.Close()
+		assert.Nil(t, err)
+		return tar.NewReader(&buf)
+	}
+
+	type TestCase struct {
+		sourcePath string
+		expected   bool
+		files      []fakeFile
+	}
+
+	tests := []TestCase{
+		{
+			sourcePath: "/root.txt", expected: true,
+			files: []fakeFile{{name: "root.txt", body: "file in the root"}},
+		},
+		{
+			sourcePath: "/tmp/file/in/subfolder.txt", expected: true,
+			files: []fakeFile{{name: "subfolder.txt", body: "a file in a subfolder"}},
+		},
+		{
+			sourcePath: "/root", expected: true,
+			files: []fakeFile{
+				{name: "root/", isDir: true},
+				{name: "root/a.txt", body: "a"},
+				{name: "root/b.txt", body: "b"},
+			},
+		},
+		{
+			sourcePath: "/tmp/subfolder", expected: true,
+			files: []fakeFile{
+				{name: "subfolder/", isDir: true},
+				{name: "subfolder/a.txt", body: "a"},
+				{name: "subfolder/b.txt", body: "b"},
+			},
+		},
+		{
+			// should an empty tar return true??
+			sourcePath: "/tmp/empty", expected: true,
+			files: []fakeFile{
+				{name: "empty/", isDir: true},
+			},
+		},
+		{
+			sourcePath: "/tmp/folder/that", expected: false,
+			files: []fakeFile{
+				{name: "this/", isDir: true},
+				{name: "this/a.txt", body: "a"},
+				{name: "this/b.txt", body: "b"},
+			},
+		},
+		{
+			sourcePath: "/empty.txt", expected: false,
+			files: []fakeFile{
+				// fails because empty.txt is empty
+				{name: "empty.txt", body: ""},
+			},
+		},
+		{
+			sourcePath: "/tmp/empty.txt", expected: false,
+			files: []fakeFile{
+				// fails because empty.txt is empty
+				{name: "empty.txt", body: ""},
+			},
+		},
+	}
+	for _, tc := range tests {
+		tc := tc
+		t.Run("source path "+tc.sourcePath, func(t *testing.T) {
+			t.Parallel()
+			tarReader := newTarReader(t, tc.files)
+			actual := file.ExistsInTar(tc.sourcePath, tarReader)
+			assert.Equalf(t, tc.expected, actual, "sourcePath %s not found", tc.sourcePath)
+		})
+	}
+}

workflow/executor/docker/docker.go

@@ -1,6 +1,8 @@
 package docker
 
 import (
+	"archive/tar"
+	"compress/gzip"
 	"fmt"
 	"os"
 	"os/exec"
@@ -53,10 +55,17 @@ func (d *DockerExecutor) CopyFile(containerID string, sourcePath string, destPat
 	if err != nil {
 		return err
 	}
-	if !file.IsFileOrDirExistInGZip(sourcePath, destPath) {
-		errMsg := fmt.Sprintf("File or Artifact does not exist. %s", sourcePath)
-		log.Warn(errMsg)
-		return errors.InternalError(errMsg)
+	copiedFile, err := os.Open(destPath)
+	if err != nil {
+		return err
+	}
+	defer util.Close(copiedFile)
+	gzipReader, err := gzip.NewReader(copiedFile)
+	if err != nil {
+		return err
+	}
+	if !file.ExistsInTar(sourcePath, tar.NewReader(gzipReader)) {
+		return errors.InternalErrorf("path %s does not exist (or %s is empty) in archive %s", sourcePath, sourcePath, destPath)
 	}
 	log.Infof("Archiving completed")
 	return nil