Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 126 additions & 14 deletions cmd/podman/cp.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strconv"
"strings"

"github.com/containers/buildah"
"github.com/containers/buildah/pkg/chrootuser"
"github.com/containers/buildah/util"
"github.com/containers/libpod/cmd/podman/cliconfig"
Expand All @@ -19,6 +20,7 @@ import (
"github.com/containers/storage/pkg/idtools"
digest "github.com/opencontainers/go-digest"
specs "github.com/opencontainers/runtime-spec/specs-go"
"github.com/openshift/imagebuilder"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -53,6 +55,12 @@ func init() {
rootCmd.AddCommand(cpCommand.Command)
}

type copyOptions struct {
contextDir string
extract bool
excludes []buildah.DockerIgnore
}

func cpCmd(c *cliconfig.CpValues) error {
args := c.InputArgs
if len(args) != 2 {
Expand All @@ -68,11 +76,13 @@ func cpCmd(c *cliconfig.CpValues) error {
}
defer runtime.Shutdown(false)

extract := c.Flag("extract").Changed
return copyBetweenHostAndContainer(runtime, args[0], args[1], extract)
copyOpts := copyOptions{
extract: c.Extract,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we only filling one field in here?

If we're not using all of copyOptions, just pass in Extract to copyBetweenHostAndContainer and make a copyOptions in there

}
return copyBetweenHostAndContainer(runtime, args[0], args[1], copyOpts)
}

func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest string, extract bool) error {
func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest string, copyOpts copyOptions) error {

srcCtr, srcPath := parsePath(runtime, src)
destCtr, destPath := parsePath(runtime, dest)
Expand Down Expand Up @@ -164,17 +174,23 @@ func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest strin
if len(glob) == 0 {
glob = append(glob, srcPath)
}

dir, err := os.Getwd()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually how Docker find .dockerignore? I would assume they would start at the source directory and look for .dockerignore in the source directory and its parents?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's for Dockerfiles. We don't have a context directory in podman copy, really. We have a source and a destination we're copying.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking for documentation here on how docker cp actually handles this and not finding much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Docker even do this? I can't find any documentation for this actually working in Docker. I don't really see why we would want to implement it if they don't?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find the documentation, either. But just found the cp command was influenced if there is a .dockerignore file in the context directory

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm...maybe we don't need this.

if err != nil {
return errors.Wrapf(err, "err getting current working directory")
}
if !filepath.IsAbs(destPath) {
dir, err := os.Getwd()
if err != nil {
return errors.Wrapf(err, "err getting current working directory")
}
destPath = filepath.Join(dir, destPath)
}

excludeLines, err := imagebuilder.ParseDockerignore(dir)
if err != nil {
return errors.Wrapf(err, "error reading .dockerignore file")
}
copyOpts.excludes = buildah.DockerIgnoreHelper(excludeLines, dir)
copyOpts.contextDir = dir
var lastError error
for _, src := range glob {
err := copy(src, destPath, dest, idMappingOpts, &containerOwner, extract)
err := copy(src, destPath, dest, idMappingOpts, &containerOwner, copyOpts)
if lastError != nil {
logrus.Error(lastError)
}
Expand Down Expand Up @@ -227,7 +243,7 @@ func getPathInfo(path string) (string, os.FileInfo, error) {
return path, srcfi, nil
}

func copy(src, destPath, dest string, idMappingOpts storage.IDMappingOptions, chownOpts *idtools.IDPair, extract bool) error {
func copy(src, destPath, dest string, idMappingOpts storage.IDMappingOptions, chownOpts *idtools.IDPair, copyOpts copyOptions) error {
srcPath, err := filepath.EvalSymlinks(src)
if err != nil {
return errors.Wrapf(err, "error evaluating symlinks %q", srcPath)
Expand All @@ -244,20 +260,87 @@ func copy(src, destPath, dest string, idMappingOpts storage.IDMappingOptions, ch
if err = os.MkdirAll(destdir, 0755); err != nil {
return errors.Wrapf(err, "error creating directory %q", destdir)
}

srcAbsPath, err := filepath.Abs(srcPath)
if err != nil {
return errors.Wrapf(err, "error getting abusolute path of source path %s", srcPath)
}
// return functions for copying items
copyFileWithTar := chrootarchive.CopyFileWithTarAndChown(chownOpts, digest.Canonical.Digester().Hash(), idMappingOpts.UIDMap, idMappingOpts.GIDMap)
copyWithTar := chrootarchive.CopyWithTarAndChown(chownOpts, digest.Canonical.Digester().Hash(), idMappingOpts.UIDMap, idMappingOpts.GIDMap)
untarPath := chrootarchive.UntarPathAndChown(chownOpts, digest.Canonical.Digester().Hash(), idMappingOpts.UIDMap, idMappingOpts.GIDMap)

visitedDir, err := getVisitedDir(copyOpts.contextDir, copyOpts.excludes)
if err != nil {
return errors.Wrapf(err, "error checking directories in .dokcerignore")
}

if srcfi.IsDir() {

logrus.Debugf("copying %q to %q", srcPath+string(os.PathSeparator)+"*", dest+string(os.PathSeparator)+"*")
if err = copyWithTar(srcPath, destPath); err != nil {
return errors.Wrapf(err, "error copying %q to %q", srcPath, dest)
if len(copyOpts.excludes) == 0 {
if err = copyWithTar(srcPath, destPath); err != nil {
return errors.Wrapf(err, "error copying %q to %q", srcPath, dest)
}
return nil
}

err = filepath.Walk(srcAbsPath, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if a directory is marked to be excluded?

For example, if I've:

mkdir -p a/b
touch a/b/c

echo a/b > .dockerignore

Shouldn't we skip also the file c from being copied?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}

for _, exclude := range copyOpts.excludes {
match, err := filepath.Match(filepath.Clean(exclude.ExcludePath), filepath.Clean(path))
if err != nil {
return err
}
prefix, exist := visitedDir[exclude.ExcludePath]
hasPrefix := false
if exist {
hasPrefix = filepath.HasPrefix(path, prefix)
}
if !(match || hasPrefix) {
continue
}
if (hasPrefix && exclude.IsExcluded) || (match && exclude.IsExcluded) {
return nil
}
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why break here? Don't we need to check all the exclude rules?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's not match, I have continue in line 295

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we consider the last match though?

Something like:

a/b
!a/b/c

Wouldn't we skip a/b/c in this way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, @giuseppe , needs review 🕵️‍♂️

}
// combine the filename with the dest directory
fpath := strings.TrimPrefix(path, srcAbsPath)
if err = copyFileWithTar(path, filepath.Join(destPath, fpath)); err != nil {
return errors.Wrapf(err, "error copying %q to %q", path, dest)
}
return nil
})
if err != nil {
return err
}
return nil
}

for _, exclude := range copyOpts.excludes {
match, err := filepath.Match(filepath.Clean(exclude.ExcludePath), srcAbsPath)
if err != nil {
return err
}
prefix, exist := visitedDir[exclude.ExcludePath]
hasPrefix := false
if exist {
hasPrefix = filepath.HasPrefix(srcAbsPath, prefix)
}
if !(match || hasPrefix) {
continue
}
if (hasPrefix && exclude.IsExcluded) || (match && exclude.IsExcluded) {
return nil
}
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, don't we need to check all the excludes?

}
if !archive.IsArchivePath(srcPath) {
// This srcPath is a file, and either it's not an
// archive, or we don't care whether or not it's an
Expand All @@ -273,7 +356,7 @@ func copy(src, destPath, dest string, idMappingOpts storage.IDMappingOptions, ch
}
}

if extract {
if copyOpts.extract {
// We're extracting an archive into the destination directory.
logrus.Debugf("extracting contents of %q into %q", srcPath, destPath)
if err = untarPath(srcPath, destPath); err != nil {
Expand All @@ -300,3 +383,32 @@ func convertIDMap(idMaps []idtools.IDMap) (convertedIDMap []specs.LinuxIDMapping
}
return convertedIDMap
}

func getVisitedDir(srcAbsPath string, excludes []buildah.DockerIgnore) (map[string]string, error) {
visitedDir := make(map[string]string)
err := filepath.Walk(srcAbsPath, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
for _, exclude := range excludes {
match, err := filepath.Match(filepath.Clean(exclude.ExcludePath), filepath.Clean(path))
if err != nil {
return err
}
if !match {
continue
}
if _, exist := visitedDir[exclude.ExcludePath]; exist {
continue
}
visitedDir[exclude.ExcludePath] = path
}
}
return nil
})
if err != nil {
return visitedDir, err
}
return visitedDir, nil
}
53 changes: 53 additions & 0 deletions test/e2e/cp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,57 @@ var _ = Describe("Podman cp", func() {
}
Expect(string(output)).To(Equal("copy from host to container directory"))
})

It("podman cp with .dockerignore", func() {
path, err := os.Getwd()
if err != nil {
os.Exit(1)
}

testDirPath := filepath.Join(path, "TestDir")
err = os.Mkdir(testDirPath, 0777)
if err != nil {
os.Exit(1)
}

testSubDirPath := filepath.Join(testDirPath, "SubTestDir")
err = os.Mkdir(testSubDirPath, 0777)
if err != nil {
os.Exit(1)
}

filePath := filepath.Join(testSubDirPath, "subtest1.txt")
ignoreme := []byte("test1 fail")
err = ioutil.WriteFile(filePath, ignoreme, 0644)
if err != nil {
os.Exit(1)
}

filePath = filepath.Join(testSubDirPath, "subtest2.txt")
notignoreme := []byte("test2 fail")
err = ioutil.WriteFile(filePath, notignoreme, 0644)
if err != nil {
os.Exit(1)
}

filePath = filepath.Join(path, ".dockerignore")
dockerignore := []byte("*/SubT*\n!*/*/subtest2*")
err = ioutil.WriteFile(filePath, dockerignore, 0644)
if err != nil {
os.Exit(1)
}

session := podmanTest.Podman([]string{"create", ALPINE, "ls", "foodir/SubTestDir/"})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))
name := session.OutputToString()

session = podmanTest.Podman([]string{"cp", testDirPath, name + ":foodir/"})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))
session = podmanTest.Podman([]string{"start", "-a", name})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))
Expect(session.OutputToString()).To(Equal("subtest2.txt"))
})
})