From ac0b320cb4f48af941c7cd546ed20e782de43e73 Mon Sep 17 00:00:00 2001 From: Fred Dubois <169247+duboisf@users.noreply.github.com> Date: Thu, 21 Mar 2019 12:16:13 -0400 Subject: [PATCH] Fix bug with DockerExecutor's CopyFile 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 | 26 +++---- util/file/fileutil_test.go | 110 +++++++++++++++++++++++++++-- workflow/executor/docker/docker.go | 17 +++-- 3 files changed, 126 insertions(+), 27 deletions(-) diff --git a/util/file/fileutil.go b/util/file/fileutil.go index f99b2e5dea34..37f6a56179c2 100644 --- a/util/file/fileutil.go +++ b/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 { diff --git a/util/file/fileutil_test.go b/util/file/fileutil_test.go index c6a6ecc7b8d2..32379866afb2 100644 --- a/util/file/fileutil_test.go +++ b/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) + }) + } +} diff --git a/workflow/executor/docker/docker.go b/workflow/executor/docker/docker.go index 0d4084062f81..020ae938d1ea 100644 --- a/workflow/executor/docker/docker.go +++ b/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