Fix issue saving outputs which overlap paths with inputs (argoproj#1567)

test/e2e/functional/artifact-input-output-samedir.yaml

@@ -0,0 +1,101 @@
+# Tests ability to capture output directories and files when it overlaps with inputs
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: artifact-input-output-samedir-
+spec:
+ entrypoint: artifact-input-output-samedir
+ templates:
+ - name: artifact-input-output-samedir
+ steps:
+ - - name: generate
+ template: generate
+ - - name: collect
+ template: collect
+ arguments:
+ artifacts:
+ - name: samedir
+ from: "{{steps.generate.outputs.artifacts.dir}}"
+ - name: hello
+ from: "{{steps.generate.outputs.artifacts.hello}}"
+ - - name: verify
+ template: verify
+ arguments:
+ artifacts:
+ - name: outer
+ from: "{{steps.collect.outputs.artifacts.outer}}"
+ - name: inner
+ from: "{{steps.collect.outputs.artifacts.inner}}"
+ - name: hello
+ from: "{{steps.collect.outputs.artifacts.hello}}"
+ - name: coincidental-prefix
+ from: "{{steps.collect.outputs.artifacts.coincidental-prefix}}"
+
+ # generate a folder structure with directories
+ - name: generate
+ script:
+ image: docker/whalesay:latest
+ command: [sh, -xe]
+ source: |
+ sleep 1
+ mkdir -p /outer/inner
+ cowsay outer | tee /outer/outer.txt
+ cowsay inner | tee /outer/inner/inner.txt
+ cowsay hello | tee /hello.txt
+ outputs:
+ artifacts:
+ - name: dir
+ path: /outer
+ - name: hello
+ path: /hello.txt
+
+ # test collection of artifacts where:
+ # 1) input and output mapped to same directory
+ # 2) collecting an output directory of a subdir
+ # 3) output happens to have same prefix of an input, but is unrelated
+ - name: collect
+ inputs:
+ artifacts:
+ - name: samedir
+ path: /outer
+ - name: hello
+ path: /hello.txt
+ container:
+ image: docker/whalesay:latest
+ command: [sh, -c]
+ args: ["
+ sleep 1 &&
+ cowsay coincidental-prefix | tee /hello.txt-COINCIDENCAL.txt
+ "]
+ outputs:
+ artifacts:
+ - name: outer
+ path: /outer
+ - name: inner
+ path: /outer/inner
+ - name: hello
+ path: /hello.txt
+ - name: coincidental-prefix
+ path: /hello.txt-COINCIDENCAL.txt
+
+ # verifies collection was successful
+ - name: verify
+ inputs:
+ artifacts:
+ - name: outer
+ path: /outer
+ - name: inner
+ path: /inner
+ - name: hello
+ path: /hello.txt
+ - name: coincidental-prefix
+ path: /coincidental-prefix.txt
+ script:
+ image: alpine:3.8
+ command: [sh, -xe]
+ source: |
+ cat /outer/outer.txt
+ cat /outer/inner/inner.txt
+ cat /inner/inner.txt
+ cat /hello.txt
+ cat /coincidental-prefix.txt

util/archive/archive.go

@@ -49,7 +49,8 @@ func TarGzToWriter(sourcePath string, w io.Writer) error {
 
 func tarDir(sourcePath string, tw *tar.Writer) error {
  baseName := filepath.Base(sourcePath)
- return filepath.Walk(sourcePath, func(fpath string, info os.FileInfo, err error) error {
+ count := 0
+ err := filepath.Walk(sourcePath, func(fpath string, info os.FileInfo, err error) error {
  if err != nil {
  return errors.InternalWrapError(err)
  }
@@ -59,7 +60,8 @@ func tarDir(sourcePath string, tw *tar.Writer) error {
  return errors.InternalWrapError(err)
  }
  nameInArchive = filepath.Join(baseName, nameInArchive)
- log.Infof("writing %s", nameInArchive)
+ log.Debugf("writing %s", nameInArchive)
+ count++
 
  var header *tar.Header
  if (info.Mode() & os.ModeSymlink) != 0 {
@@ -102,6 +104,8 @@ func tarDir(sourcePath string, tw *tar.Writer) error {
  }
  return nil
  })
+ log.Infof("archived %d files/dirs in %s", count, sourcePath)
+ return err
 }
 
 func tarFile(sourcePath string, tw *tar.Writer) error {

workflow/common/util.go

@@ -34,13 +34,18 @@ import (
 // user specified volumeMounts in the template, and returns the deepest volumeMount
 // (if any). A return value of nil indicates the path is not under any volumeMount.
 func FindOverlappingVolume(tmpl *wfv1.Template, path string) *apiv1.VolumeMount {
- if tmpl.Container == nil {
+ var volMounts []apiv1.VolumeMount
+ if tmpl.Container != nil {
+ volMounts = tmpl.Container.VolumeMounts
+ } else if tmpl.Script != nil {
+ volMounts = tmpl.Script.VolumeMounts
+ } else {
  return nil
  }
  var volMnt *apiv1.VolumeMount
  deepestLen := 0
- for _, mnt := range tmpl.Container.VolumeMounts {
- if !strings.HasPrefix(path, mnt.MountPath) {
+ for _, mnt := range volMounts {
+ if path != mnt.MountPath && !strings.HasPrefix(path, mnt.MountPath+"/") {
  continue
  }
  if len(mnt.MountPath) > deepestLen {

workflow/common/util_test.go

@@ -0,0 +1,26 @@
+package common
+
+import (
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ corev1 "k8s.io/api/core/v1"
+
+ wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
+)
+
+// TestFindOverlappingVolume tests logic of TestFindOverlappingVolume
+func TestFindOverlappingVolume(t *testing.T) {
+ volMnt := corev1.VolumeMount{
+ Name: "workdir",
+ MountPath: "/user-mount",
+ }
+ templateWithVolMount := &wfv1.Template{
+ Container: &corev1.Container{
+ VolumeMounts: []corev1.VolumeMount{volMnt},
+ },
+ }
+ assert.Equal(t, &volMnt, FindOverlappingVolume(templateWithVolMount, "/user-mount"))
+ assert.Equal(t, &volMnt, FindOverlappingVolume(templateWithVolMount, "/user-mount/subdir"))
+ assert.Nil(t, FindOverlappingVolume(templateWithVolMount, "/user-mount-coincidental-prefix"))
+}

workflow/executor/executor.go

@@ -386,8 +386,23 @@ func (we *WorkflowExecutor) stageArchiveFile(mainCtrID string, art *wfv1.Artifac
  return fileName, localArtPath, nil
 }
 
+// isBaseImagePath checks if the given artifact path resides in the base image layer of the container
+// versus a shared volume mount between the wait and main container
 func (we *WorkflowExecutor) isBaseImagePath(path string) bool {
- return common.FindOverlappingVolume(&we.Template, path) == nil
+ // first check if path overlaps with a user-specified volumeMount
+ if common.FindOverlappingVolume(&we.Template, path) != nil {
+ return false
+ }
+ // next check if path overlaps with a shared input-artifact emptyDir mounted by argo
+ for _, inArt := range we.Template.Inputs.Artifacts {
+ if path == inArt.Path {
+ return false
+ }
+ if strings.HasPrefix(path, inArt.Path+"/") {
+ return false
+ }
+ }
+ return true
 }
 
 // SaveParameters will save the content in the specified file path as output parameter value

workflow/executor/executor_test.go

@@ -3,10 +3,12 @@ package executor
 import (
  "testing"
 
- wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
- "github.com/argoproj/argo/workflow/executor/mocks"
  "github.com/stretchr/testify/assert"
+ corev1 "k8s.io/api/core/v1"
  "k8s.io/client-go/kubernetes/fake"
+
+ wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
+ "github.com/argoproj/argo/workflow/executor/mocks"
 )
 
 const (
@@ -46,3 +48,59 @@ func TestSaveParameters(t *testing.T) {
  assert.Nil(t, err)
  assert.Equal(t, *we.Template.Outputs.Parameters[0].Value, "has a newline")
 }
+
+// TestIsBaseImagePath tests logic of isBaseImagePath which determines if a path is coming from a
+// base image layer versus a shared volumeMount.
+func TestIsBaseImagePath(t *testing.T) {
+ templateWithSameDir := wfv1.Template{
+ Inputs: wfv1.Inputs{
+ Artifacts: []wfv1.Artifact{
+ {
+ Name: "samedir",
+ Path: "/samedir",
+ },
+ },
+ },
+ Container: &corev1.Container{},
+ Outputs: wfv1.Outputs{
+ Artifacts: []wfv1.Artifact{
+ {
+ Name: "samedir",
+ Path: "/samedir",
+ },
+ },
+ },
+ }
+
+ we := WorkflowExecutor{
+ Template: templateWithSameDir,
+ }
+ // 1. unrelated dir/file should be captured from base image layer
+ assert.True(t, we.isBaseImagePath("/foo"))
+
+ // 2. when input and output directory is same, it should be captured from shared emptyDir
+ assert.False(t, we.isBaseImagePath("/samedir"))
+
+ // 3. when output is a sub path of input dir, it should be captured from shared emptyDir
+ we.Template.Outputs.Artifacts[0].Path = "/samedir/inner"
+ assert.False(t, we.isBaseImagePath("/samedir/inner"))
+
+ // 4. when output happens to overlap with input (in name only), it should be captured from base image layer
+ we.Template.Inputs.Artifacts[0].Path = "/hello.txt"
+ we.Template.Outputs.Artifacts[0].Path = "/hello.txt-COINCIDENCE"
+ assert.True(t, we.isBaseImagePath("/hello.txt-COINCIDENCE"))
+
+ // 5. when output is under a user specified volumeMount, it should be captured from shared mount
+ we.Template.Inputs.Artifacts = nil
+ we.Template.Container.VolumeMounts = []corev1.VolumeMount{
+ {
+ Name: "workdir",
+ MountPath: "/user-mount",
+ },
+ }
+ we.Template.Outputs.Artifacts[0].Path = "/user-mount/some-path"
+ assert.False(t, we.isBaseImagePath("/user-mount"))
+ assert.False(t, we.isBaseImagePath("/user-mount/some-path"))
+ assert.False(t, we.isBaseImagePath("/user-mount/some-path/foo"))
+ assert.True(t, we.isBaseImagePath("/user-mount-coincidence"))
+}

workflow/validate/validate.go

@@ -705,7 +705,7 @@ func validateOutputs(scope map[string]interface{}, tmpl *wfv1.Template) error {
  return nil
 }
 
-// validateBaseImageOutputs detects if the template contains an output from
+// validateBaseImageOutputs detects if the template contains an valid output from base image layer
 func (ctx *templateValidationCtx) validateBaseImageOutputs(tmpl *wfv1.Template) error {
  switch ctx.ContainerRuntimeExecutor {
  case "", common.ContainerRuntimeExecutorDocker:
@@ -725,7 +725,7 @@ func (ctx *templateValidationCtx) validateBaseImageOutputs(tmpl *wfv1.Template)
 
  }
  if tmpl.Script != nil {
- for _, volMnt := range tmpl.Container.VolumeMounts {
+ for _, volMnt := range tmpl.Script.VolumeMounts {
  if strings.HasPrefix(volMnt.MountPath, out.Path+"/") {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s.outputs.artifacts.%s: %s", tmpl.Name, out.Name, errMsg)
  }