From 0fb6815f08f39c8af814fa733d5235681a124b81 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Fri, 21 Aug 2020 19:24:19 +0200 Subject: [PATCH] Use separate pathSpec for local and remote to properly handle cleaning paths --- staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go | 159 +++++--------- .../src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go | 201 +++++++++++------- .../src/k8s.io/kubectl/pkg/cmd/cp/filespec.go | 161 ++++++++++++++ 3 files changed, 341 insertions(+), 180 deletions(-) create mode 100644 staging/src/k8s.io/kubectl/pkg/cmd/cp/filespec.go diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go b/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go index 578700d228b..5daf863f46a 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go @@ -24,11 +24,8 @@ import ( "io" "io/ioutil" "os" - "path" - "path/filepath" "strings" - "github.com/lithammer/dedent" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" @@ -67,12 +64,6 @@ var ( # Copy /tmp/foo from a remote pod to /tmp/bar locally kubectl cp /:/tmp/foo /tmp/bar`)) - - cpUsageStr = dedent.Dedent(` - expected 'cp [-c container]'. - is: - [namespace/]pod-name:/file/path for a remote file - /file/path for a local file`) ) // CopyOptions have the data required to perform the copy operation @@ -158,6 +149,7 @@ func NewCmdCp(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.C }, Run: func(cmd *cobra.Command, args []string) { cmdutil.CheckErr(o.Complete(f, cmd)) + cmdutil.CheckErr(o.Validate(cmd, args)) cmdutil.CheckErr(o.Run(args)) }, } @@ -167,27 +159,22 @@ func NewCmdCp(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.C return cmd } -type fileSpec struct { - PodNamespace string - PodName string - File string -} - var ( errFileSpecDoesntMatchFormat = errors.New("filespec must match the canonical format: [[namespace/]pod:]file/path") - errFileCannotBeEmpty = errors.New("filepath can not be empty") ) func extractFileSpec(arg string) (fileSpec, error) { i := strings.Index(arg, ":") - if i == -1 { - return fileSpec{File: arg}, nil - } // filespec starting with a semicolon is invalid if i == 0 { return fileSpec{}, errFileSpecDoesntMatchFormat } + if i == -1 { + return fileSpec{ + File: newLocalPath(arg), + }, nil + } pod, file := arg[:i], arg[i+1:] pieces := strings.Split(pod, "/") @@ -195,13 +182,13 @@ func extractFileSpec(arg string) (fileSpec, error) { case 1: return fileSpec{ PodName: pieces[0], - File: file, + File: newRemotePath(file), }, nil case 2: return fileSpec{ PodNamespace: pieces[0], PodName: pieces[1], - File: file, + File: newRemotePath(file), }, nil default: return fileSpec{}, errFileSpecDoesntMatchFormat @@ -235,16 +222,13 @@ func (o *CopyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error { // Validate makes sure provided values for CopyOptions are valid func (o *CopyOptions) Validate(cmd *cobra.Command, args []string) error { if len(args) != 2 { - return cmdutil.UsageErrorf(cmd, cpUsageStr) + return fmt.Errorf("source and destination are required") } return nil } // Run performs the execution func (o *CopyOptions) Run(args []string) error { - if len(args) < 2 { - return fmt.Errorf("source and destination are required") - } srcSpec, err := extractFileSpec(args[0]) if err != nil { return err @@ -257,6 +241,9 @@ func (o *CopyOptions) Run(args []string) error { if len(srcSpec.PodName) != 0 && len(destSpec.PodName) != 0 { return fmt.Errorf("one of src or dest must be a local file specification") } + if len(srcSpec.File.String()) == 0 || len(destSpec.File.String()) == 0 { + return errors.New("filepath can not be empty") + } if len(srcSpec.PodName) != 0 { return o.copyFromPod(srcSpec, destSpec) @@ -283,7 +270,7 @@ func (o *CopyOptions) checkDestinationIsDir(dest fileSpec) error { PodName: dest.PodName, }, - Command: []string{"test", "-d", dest.File}, + Command: []string{"test", "-d", dest.File.String()}, Executor: &exec.DefaultRemoteExecutor{}, } @@ -291,29 +278,24 @@ func (o *CopyOptions) checkDestinationIsDir(dest fileSpec) error { } func (o *CopyOptions) copyToPod(src, dest fileSpec, options *exec.ExecOptions) error { - if len(src.File) == 0 || len(dest.File) == 0 { - return errFileCannotBeEmpty - } - if _, err := os.Stat(src.File); err != nil { + if _, err := os.Stat(src.File.String()); err != nil { return fmt.Errorf("%s doesn't exist in local filesystem", src.File) } reader, writer := io.Pipe() - // strip trailing slash (if any) - if dest.File != "/" && strings.HasSuffix(string(dest.File[len(dest.File)-1]), "/") { - dest.File = dest.File[:len(dest.File)-1] - } + srcFile := src.File.(localPath) + destFile := dest.File.(remotePath) if err := o.checkDestinationIsDir(dest); err == nil { // If no error, dest.File was found to be a directory. // Copy specified src into it - dest.File = dest.File + "/" + path.Base(src.File) + destFile = destFile.Join(srcFile.Base()) } - go func() { + go func(src localPath, dest remotePath, writer io.WriteCloser) { defer writer.Close() - cmdutil.CheckErr(makeTar(src.File, dest.File, writer)) - }() + cmdutil.CheckErr(makeTar(src, dest, writer)) + }(srcFile, destFile, writer) var cmdArr []string // TODO: Improve error messages by first testing if 'tar' is present in the container? @@ -322,9 +304,9 @@ func (o *CopyOptions) copyToPod(src, dest fileSpec, options *exec.ExecOptions) e } else { cmdArr = []string{"tar", "-xmf", "-"} } - destDir := path.Dir(dest.File) - if len(destDir) > 0 { - cmdArr = append(cmdArr, "-C", destDir) + destFileDir := destFile.Dir().String() + if len(destFileDir) > 0 { + cmdArr = append(cmdArr, "-C", destFileDir) } options.StreamOptions = exec.StreamOptions{ @@ -345,10 +327,6 @@ func (o *CopyOptions) copyToPod(src, dest fileSpec, options *exec.ExecOptions) e } func (o *CopyOptions) copyFromPod(src, dest fileSpec) error { - if len(src.File) == 0 || len(dest.File) == 0 { - return errFileCannotBeEmpty - } - reader, outStream := io.Pipe() options := &exec.ExecOptions{ StreamOptions: exec.StreamOptions{ @@ -363,7 +341,7 @@ func (o *CopyOptions) copyFromPod(src, dest fileSpec) error { }, // TODO: Improve error messages by first testing if 'tar' is present in the container? - Command: []string{"tar", "cf", "-", src.File}, + Command: []string{"tar", "cf", "-", src.File.String()}, Executor: &exec.DefaultRemoteExecutor{}, } @@ -371,49 +349,28 @@ func (o *CopyOptions) copyFromPod(src, dest fileSpec) error { defer outStream.Close() cmdutil.CheckErr(o.execute(options)) }() - prefix := getPrefix(src.File) - prefix = path.Clean(prefix) + + srcFile := src.File.(remotePath) + destFile := dest.File.(localPath) + // remove extraneous path shortcuts - these could occur if a path contained extra "../" // and attempted to navigate beyond "/" in a remote filesystem - prefix = stripPathShortcuts(prefix) - return o.untarAll(src, reader, dest.File, prefix) + prefix := stripPathShortcuts(srcFile.StripSlashes().Clean().String()) + return o.untarAll(src.PodNamespace, src.PodName, prefix, srcFile, destFile, reader) } -// stripPathShortcuts removes any leading or trailing "../" from a given path -func stripPathShortcuts(p string) string { - newPath := path.Clean(p) - trimmed := strings.TrimPrefix(newPath, "../") - - for trimmed != newPath { - newPath = trimmed - trimmed = strings.TrimPrefix(newPath, "../") - } - - // trim leftover {".", ".."} - if newPath == "." || newPath == ".." { - newPath = "" - } - - if len(newPath) > 0 && string(newPath[0]) == "/" { - return newPath[1:] - } - - return newPath -} - -func makeTar(srcPath, destPath string, writer io.Writer) error { +func makeTar(src localPath, dest remotePath, writer io.Writer) error { // TODO: use compression here? tarWriter := tar.NewWriter(writer) defer tarWriter.Close() - srcPath = path.Clean(srcPath) - destPath = path.Clean(destPath) - return recursiveTar(path.Dir(srcPath), path.Base(srcPath), path.Dir(destPath), path.Base(destPath), tarWriter) + srcPath := src.Clean() + destPath := dest.Clean() + return recursiveTar(srcPath.Dir(), srcPath.Base(), destPath.Dir(), destPath.Base(), tarWriter) } -func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) error { - srcPath := path.Join(srcBase, srcFile) - matchedPaths, err := filepath.Glob(srcPath) +func recursiveTar(srcDir, srcFile localPath, destDir, destFile remotePath, tw *tar.Writer) error { + matchedPaths, err := srcDir.Join(srcFile).Glob() if err != nil { return err } @@ -430,13 +387,14 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e if len(files) == 0 { //case empty directory hdr, _ := tar.FileInfoHeader(stat, fpath) - hdr.Name = destFile + hdr.Name = destFile.String() if err := tw.WriteHeader(hdr); err != nil { return err } } for _, f := range files { - if err := recursiveTar(srcBase, path.Join(srcFile, f.Name()), destBase, path.Join(destFile, f.Name()), tw); err != nil { + if err := recursiveTar(srcDir, srcFile.Join(newLocalPath(f.Name())), + destDir, destFile.Join(newRemotePath(f.Name())), tw); err != nil { return err } } @@ -450,7 +408,7 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e } hdr.Linkname = target - hdr.Name = destFile + hdr.Name = destFile.String() if err := tw.WriteHeader(hdr); err != nil { return err } @@ -460,7 +418,7 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e if err != nil { return err } - hdr.Name = destFile + hdr.Name = destFile.String() if err := tw.WriteHeader(hdr); err != nil { return err @@ -481,7 +439,7 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e return nil } -func (o *CopyOptions) untarAll(src fileSpec, reader io.Reader, destDir, prefix string) error { +func (o *CopyOptions) untarAll(ns, pod string, prefix string, src remotePath, dest localPath, reader io.Reader) error { symlinkWarningPrinted := false // TODO: use compression here? tarReader := tar.NewReader(reader) @@ -505,19 +463,21 @@ func (o *CopyOptions) untarAll(src fileSpec, reader io.Reader, destDir, prefix s // basic file information mode := header.FileInfo().Mode() - destFileName := filepath.Join(destDir, header.Name[len(prefix):]) + // header.Name is a name of the REMOTE file, so we need to create + // a remotePath so that it goes through appropriate processing related + // with cleaning remote paths + destFileName := dest.Join(newRemotePath(header.Name[len(prefix):])) - if !isDestRelative(destDir, destFileName) { + if !isRelative(dest, destFileName) { fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is outside target destination, skipping\n", destFileName) continue } - baseName := filepath.Dir(destFileName) - if err := os.MkdirAll(baseName, 0755); err != nil { + if err := os.MkdirAll(destFileName.Dir().String(), 0755); err != nil { return err } if header.FileInfo().IsDir() { - if err := os.MkdirAll(destFileName, 0755); err != nil { + if err := os.MkdirAll(destFileName.String(), 0755); err != nil { return err } continue @@ -525,14 +485,16 @@ func (o *CopyOptions) untarAll(src fileSpec, reader io.Reader, destDir, prefix s if mode&os.ModeSymlink != 0 { if !symlinkWarningPrinted && len(o.ExecParentCmdName) > 0 { - fmt.Fprintf(o.IOStreams.ErrOut, "warning: skipping symlink: %q -> %q (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, header.Linkname, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File) + fmt.Fprintf(o.IOStreams.ErrOut, + "warning: skipping symlink: %q -> %q (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", + destFileName, header.Linkname, o.ExecParentCmdName, ns, pod, src) symlinkWarningPrinted = true continue } fmt.Fprintf(o.IOStreams.ErrOut, "warning: skipping symlink: %q -> %q\n", destFileName, header.Linkname) continue } - outFile, err := os.Create(destFileName) + outFile, err := os.Create(destFileName.String()) if err != nil { return err } @@ -548,21 +510,6 @@ func (o *CopyOptions) untarAll(src fileSpec, reader io.Reader, destDir, prefix s return nil } -// isDestRelative returns true if dest is pointing outside the base directory, -// false otherwise. -func isDestRelative(base, dest string) bool { - relative, err := filepath.Rel(base, dest) - if err != nil { - return false - } - return relative == "." || relative == stripPathShortcuts(relative) -} - -func getPrefix(file string) string { - // tar strips the leading '/' if it's there, so we will too - return strings.TrimLeft(file, "/") -} - func (o *CopyOptions) execute(options *exec.ExecOptions) error { if len(options.Namespace) == 0 { options.Namespace = o.Namespace diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go index 9af5c92e73f..10a1428ca9c 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go @@ -24,7 +24,6 @@ import ( "io/ioutil" "net/http" "os" - "path" "path/filepath" "reflect" "strings" @@ -105,30 +104,42 @@ func TestExtractFileSpec(t *testing.T) { if spec.PodNamespace != test.expectedNamespace { t.Errorf("expected: %s, saw: %s", test.expectedNamespace, spec.PodNamespace) } - if spec.File != test.expectedFile { - t.Errorf("expected: %s, saw: %s", test.expectedFile, spec.File) + specFile := "" + if spec.File != nil { + specFile = spec.File.String() + } + if specFile != test.expectedFile { + t.Errorf("expected: %s, saw: %s", test.expectedFile, specFile) } } } func TestGetPrefix(t *testing.T) { + remoteSeparator := '/' + osSeparator := os.PathSeparator tests := []struct { input string expected string }{ { - input: "/foo/bar", - expected: "foo/bar", + input: "%[1]cfoo%[1]cbar", + expected: "foo%[1]cbar", }, { - input: "foo/bar", - expected: "foo/bar", + input: "foo%[1]cbar", + expected: "foo%[1]cbar", }, } for _, test := range tests { - out := getPrefix(test.input) - if out != test.expected { - t.Errorf("expected: %s, saw: %s", test.expected, out) + outRemote := newRemotePath(fmt.Sprintf(test.input, remoteSeparator)).StripSlashes() + expectedRemote := fmt.Sprintf(test.expected, remoteSeparator) + if outRemote.String() != expectedRemote { + t.Errorf("remote expected: %s, saw: %s", expectedRemote, outRemote.String()) + } + outLocal := newLocalPath(fmt.Sprintf(test.input, osSeparator)).StripSlashes() + expectedLocal := fmt.Sprintf(test.expected, osSeparator) + if outLocal.String() != expectedLocal { + t.Errorf("local expected: %s, saw: %s", expectedLocal, outLocal.String()) } } } @@ -144,36 +155,71 @@ func TestStripPathShortcuts(t *testing.T) { input: "../foo/bar", expected: "foo/bar", }, + { + name: "test single path shortcut prefix", + input: `..\foo\bar`, + expected: "foo/bar", + }, { name: "test multiple path shortcuts", input: "../../foo/bar", expected: "foo/bar", }, + { + name: "test multiple path shortcuts", + input: `..\..\foo\bar`, + expected: "foo/bar", + }, { name: "test multiple path shortcuts with absolute path", input: "/tmp/one/two/../../foo/bar", expected: "tmp/foo/bar", }, + { + name: "test multiple path shortcuts with absolute path", + input: `\tmp\one\two\..\..\foo\bar`, + expected: "tmp/foo/bar", + }, { name: "test multiple path shortcuts with no named directory", input: "../../", expected: "", }, + { + name: "test multiple path shortcuts with no named directory", + input: `..\..\`, + expected: "", + }, { name: "test multiple path shortcuts with no named directory and no trailing slash", input: "../..", expected: "", }, + { + name: "test multiple path shortcuts with no named directory and no trailing slash", + input: `..\..`, + expected: "", + }, { name: "test multiple path shortcuts with absolute path and filename containing leading dots", input: "/tmp/one/two/../../foo/..bar", expected: "tmp/foo/..bar", }, + { + name: "test multiple path shortcuts with absolute path and filename containing leading dots", + input: `\tmp\one\two\..\..\foo\..bar`, + expected: "tmp/foo/..bar", + }, { name: "test multiple path shortcuts with no named directory and filename containing leading dots", input: "../...foo", expected: "...foo", }, + { + name: "test multiple path shortcuts with no named directory and filename containing leading dots", + input: `..\...foo`, + expected: "...foo", + }, { name: "test filename containing leading dots", input: "...foo", @@ -184,12 +230,17 @@ func TestStripPathShortcuts(t *testing.T) { input: "/", expected: "", }, + { + name: "test root directory", + input: `\`, + expected: "", + }, } - for _, test := range tests { - out := stripPathShortcuts(test.input) - if out != test.expected { - t.Errorf("expected: %s, saw: %s", test.expected, out) + for i, test := range tests { + out := newRemotePath(test.input).StripShortcuts() + if out.String() != test.expected { + t.Errorf("expected[%d]: %s, saw: %s", i, test.expected, out) } } } @@ -243,7 +294,7 @@ func TestIsDestRelative(t *testing.T) { for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - if test.relative != isDestRelative(test.base, test.dest) { + if test.relative != isRelative(newLocalPath(test.base), newLocalPath(test.dest)) { t.Errorf("unexpected result for: base %q, dest %q", test.base, test.dest) } }) @@ -260,12 +311,15 @@ func checkErr(t *testing.T, err error) { func TestTarUntar(t *testing.T) { dir, err := ioutil.TempDir("", "input") checkErr(t, err) + dir = dir + "/" + dir2, err := ioutil.TempDir("", "output") checkErr(t, err) + dir2 = dir2 + "/" + dir3, err := ioutil.TempDir("", "dir") checkErr(t, err) - dir = dir + "/" defer func() { os.RemoveAll(dir) os.RemoveAll(dir2) @@ -290,7 +344,7 @@ func TestTarUntar(t *testing.T) { fileType: RegularFile, }, { - name: "some/other/directory/", + name: "some/other/directory", data: "with more data here", fileType: RegularFile, }, @@ -307,13 +361,13 @@ func TestTarUntar(t *testing.T) { }, { name: "relative_to_dest", - data: path.Join(dir2, "foo"), + data: dir2 + "/foo", omitted: true, fileType: SymLink, }, { name: "tricky_relative", - data: path.Join(dir3, "xyz"), + data: dir3 + "/xyz", omitted: true, fileType: SymLink, }, @@ -332,20 +386,20 @@ func TestTarUntar(t *testing.T) { } for _, file := range files { - filepath := path.Join(dir, file.name) - if err := os.MkdirAll(path.Dir(filepath), 0755); err != nil { + completePath := dir + file.name + if err := os.MkdirAll(filepath.Dir(completePath), 0755); err != nil { t.Fatalf("unexpected error: %v", err) } if file.fileType == RegularFile { - createTmpFile(t, filepath, file.data) + createTmpFile(t, completePath, file.data) } else if file.fileType == SymLink { - err := os.Symlink(file.data, filepath) + err := os.Symlink(file.data, completePath) if err != nil { t.Fatalf("unexpected error: %v", err) } } else if file.fileType == RegexFile { for _, fileName := range file.nameList { - createTmpFile(t, path.Join(dir, fileName), file.data) + createTmpFile(t, dir+fileName, file.data) } } else { t.Fatalf("unexpected file type: %v", file) @@ -355,18 +409,18 @@ func TestTarUntar(t *testing.T) { opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard()) writer := &bytes.Buffer{} - if err := makeTar(dir, dir, writer); err != nil { + if err := makeTar(newLocalPath(dir), newRemotePath(dir), writer); err != nil { t.Fatalf("unexpected error: %v", err) } reader := bytes.NewBuffer(writer.Bytes()) - if err := opts.untarAll(fileSpec{}, reader, dir2, ""); err != nil { + if err := opts.untarAll("", "", "", remotePath{}, newLocalPath(dir2), reader); err != nil { t.Fatalf("unexpected error: %v", err) } for _, file := range files { - absPath := filepath.Join(dir2, strings.TrimPrefix(dir, os.TempDir())) - filePath := filepath.Join(absPath, file.name) + absPath := dir2 + strings.TrimPrefix(dir, os.TempDir()) + filePath := absPath + file.name if file.fileType == RegularFile { cmpFileData(t, filePath, file.data) @@ -387,7 +441,7 @@ func TestTarUntar(t *testing.T) { } } else if file.fileType == RegexFile { for _, fileName := range file.nameList { - cmpFileData(t, path.Join(dir, fileName), file.data) + cmpFileData(t, dir+fileName, file.data) } } else { t.Fatalf("unexpected file type: %v", file) @@ -398,30 +452,31 @@ func TestTarUntar(t *testing.T) { func TestTarUntarWrongPrefix(t *testing.T) { dir, err := ioutil.TempDir("", "input") checkErr(t, err) + dir = dir + "/" + dir2, err := ioutil.TempDir("", "output") checkErr(t, err) - dir = dir + "/" defer func() { os.RemoveAll(dir) os.RemoveAll(dir2) }() - filepath := path.Join(dir, "foo") - if err := os.MkdirAll(path.Dir(filepath), 0755); err != nil { + completePath := dir + "foo" + if err := os.MkdirAll(filepath.Dir(completePath), 0755); err != nil { t.Fatalf("unexpected error: %v", err) } - createTmpFile(t, filepath, "sample data") + createTmpFile(t, completePath, "sample data") opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard()) writer := &bytes.Buffer{} - if err := makeTar(dir, dir, writer); err != nil { + if err := makeTar(newLocalPath(dir), newRemotePath(dir), writer); err != nil { t.Fatalf("unexpected error: %v", err) } reader := bytes.NewBuffer(writer.Bytes()) - err = opts.untarAll(fileSpec{}, reader, dir2, "verylongprefix-showing-the-tar-was-tempered-with") + err = opts.untarAll("", "", "verylongprefix-showing-the-tar-was-tempered-with", remotePath{}, newLocalPath(dir2), reader) if err == nil || !strings.Contains(err.Error(), "tar contents corrupted") { t.Fatalf("unexpected error: %v", err) } @@ -467,17 +522,17 @@ func TestTarDestinationName(t *testing.T) { // ensure files exist on disk for _, file := range files { - filepath := path.Join(dir, file.name) - if err := os.MkdirAll(path.Dir(filepath), 0755); err != nil { + completePath := dir + "/" + file.name + if err := os.MkdirAll(filepath.Dir(completePath), 0755); err != nil { t.Errorf("unexpected error: %v", err) t.FailNow() } - createTmpFile(t, filepath, file.data) + createTmpFile(t, completePath, file.data) } reader, writer := io.Pipe() go func() { - if err := makeTar(dir, dir2, writer); err != nil { + if err := makeTar(newLocalPath(dir), newRemotePath(dir2), writer); err != nil { t.Errorf("unexpected error: %v", err) } }() @@ -492,8 +547,8 @@ func TestTarDestinationName(t *testing.T) { t.FailNow() } - if !strings.HasPrefix(hdr.Name, path.Base(dir2)) { - t.Errorf("expected %q as destination filename prefix, saw: %q", path.Base(dir2), hdr.Name) + if !strings.HasPrefix(hdr.Name, filepath.Base(dir2)) { + t.Errorf("expected %q as destination filename prefix, saw: %q", filepath.Base(dir2), hdr.Name) } } } @@ -536,13 +591,13 @@ func TestBadTar(t *testing.T) { } opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard()) - if err := opts.untarAll(fileSpec{}, &buf, dir, "/prefix"); err != nil { + if err := opts.untarAll("", "", "/prefix", remotePath{}, newLocalPath(dir), &buf); err != nil { t.Errorf("unexpected error: %v ", err) t.FailNow() } for _, file := range files { - _, err := os.Stat(path.Join(dir, path.Clean(file.name[len("/prefix"):]))) + _, err := os.Stat(dir + filepath.Clean(file.name[len("/prefix"):])) if err != nil { t.Errorf("Error finding file: %v", err) } @@ -596,7 +651,7 @@ func TestCopyToPod(t *testing.T) { expectedErr: true, }, "copy unexisting file": { - src: path.Join(srcFile, "nope"), + src: filepath.Join(srcFile, "nope"), dest: "/tmp", expectedErr: true, }, @@ -604,17 +659,9 @@ func TestCopyToPod(t *testing.T) { for name, test := range tests { opts := NewCopyOptions(ioStreams) - src := fileSpec{ - File: test.src, - } - dest := fileSpec{ - PodNamespace: "pod-ns", - PodName: "pod-name", - File: test.dest, - } opts.Complete(tf, cmd) t.Run(name, func(t *testing.T) { - err = opts.copyToPod(src, dest, &kexec.ExecOptions{}) + err = opts.Run([]string{test.src, fmt.Sprintf("pod-ns/pod-name:%s", test.dest)}) //If error is NotFound error , it indicates that the //request has been sent correctly. //Treat this as no error. @@ -669,12 +716,12 @@ func TestCopyToPodNoPreserve(t *testing.T) { } opts := NewCopyOptions(ioStreams) src := fileSpec{ - File: srcFile, + File: newLocalPath(srcFile), } dest := fileSpec{ PodNamespace: "pod-ns", PodName: "pod-name", - File: "foo", + File: newRemotePath("foo"), } opts.Complete(tf, cmd) @@ -728,7 +775,7 @@ func TestUntar(t *testing.T) { defer os.RemoveAll(testdir) t.Logf("Test base: %s", testdir) - basedir := filepath.Join(testdir, "base") + basedir := testdir + "/" + "base" type file struct { path string @@ -737,29 +784,35 @@ func TestUntar(t *testing.T) { } files := []file{{ // Absolute file within dest - path: filepath.Join(basedir, "abs"), - expected: filepath.Join(basedir, basedir, "abs"), + path: basedir + "/" + "abs", + expected: basedir + basedir + "/" + "abs", }, { // Absolute file outside dest - path: filepath.Join(testdir, "abs-out"), - expected: filepath.Join(basedir, testdir, "abs-out"), + path: testdir + "/" + "abs-out", + expected: basedir + testdir + "/" + "abs-out", }, { // Absolute nested file within dest - path: filepath.Join(basedir, "nested/nest-abs"), - expected: filepath.Join(basedir, basedir, "nested/nest-abs"), + path: basedir + "/" + "nested/nest-abs", + expected: basedir + basedir + "/" + "nested/nest-abs", }, { // Absolute nested file outside dest - path: filepath.Join(basedir, "nested/../../nest-abs-out"), - expected: filepath.Join(basedir, testdir, "nest-abs-out"), + path: basedir + "/" + "nested/../../nest-abs-out", + expected: basedir + testdir + "/" + "nest-abs-out", }, { // Relative file inside dest path: "relative", - expected: filepath.Join(basedir, "relative"), + expected: basedir + "/" + "relative", }, { // Relative file outside dest path: "../unrelative", expected: "", + }, { // Relative file outside dest (windows) + path: `..\unrelative-windows`, + expected: "", }, { // Nested relative file inside dest path: "nested/nest-rel", - expected: filepath.Join(basedir, "nested/nest-rel"), + expected: basedir + "/" + "nested/nest-rel", }, { // Nested relative file outside dest path: "nested/../../nest-unrelative", expected: "", + }, { // Nested relative file outside dest (windows) + path: `nested\..\..\nest-unrelative`, + expected: "", }} links := []file{} @@ -770,15 +823,15 @@ func TestUntar(t *testing.T) { expected: "", }, file{ path: f.path + "-innerlink-abs", - linkTarget: filepath.Join(basedir, "link-target"), + linkTarget: basedir + "/" + "link-target", expected: "", }, file{ path: f.path + "-backlink", - linkTarget: filepath.Join("..", "link-target"), + linkTarget: ".." + "/" + "link-target", expected: "", }, file{ path: f.path + "-outerlink-abs", - linkTarget: filepath.Join(testdir, "link-target"), + linkTarget: testdir + "/" + "link-target", expected: "", }) @@ -787,7 +840,7 @@ func TestUntar(t *testing.T) { outerlink, _ := filepath.Rel(f.expected, testdir) links = append(links, file{ path: f.path + "outerlink", - linkTarget: filepath.Join(outerlink, "link-target"), + linkTarget: outerlink + "/" + "link-target", expected: "", }) } @@ -803,7 +856,7 @@ func TestUntar(t *testing.T) { }, file{ path: "nested/again/back-link/../../../back-link-file", - expected: filepath.Join(basedir, "back-link-file"), + expected: basedir + "/" + "back-link-file", }) // Test chaining back-tick symlinks. @@ -859,7 +912,7 @@ func TestUntar(t *testing.T) { output := (*testWriter)(t) opts := NewCopyOptions(genericclioptions.IOStreams{In: &bytes.Buffer{}, Out: output, ErrOut: output}) - require.NoError(t, opts.untarAll(fileSpec{}, buf, filepath.Join(basedir), "")) + require.NoError(t, opts.untarAll("", "", "", remotePath{}, newLocalPath(basedir), buf)) filepath.Walk(testdir, func(path string, info os.FileInfo, err error) error { if err != nil { @@ -887,7 +940,7 @@ func TestUntar_SingleFile(t *testing.T) { require.NoError(t, err) defer os.RemoveAll(testdir) - dest := filepath.Join(testdir, "target") + dest := testdir + "/" + "target" buf := &bytes.Buffer{} tw := tar.NewWriter(buf) @@ -910,7 +963,7 @@ func TestUntar_SingleFile(t *testing.T) { output := (*testWriter)(t) opts := NewCopyOptions(genericclioptions.IOStreams{In: &bytes.Buffer{}, Out: output, ErrOut: output}) - require.NoError(t, opts.untarAll(fileSpec{}, buf, filepath.Join(dest), srcName)) + require.NoError(t, opts.untarAll("", "", srcName, remotePath{}, newLocalPath(dest), buf)) cmpFileData(t, dest, content) } diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/cp/filespec.go b/staging/src/k8s.io/kubectl/pkg/cmd/cp/filespec.go new file mode 100644 index 00000000000..199bbeeed74 --- /dev/null +++ b/staging/src/k8s.io/kubectl/pkg/cmd/cp/filespec.go @@ -0,0 +1,161 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cp + +import ( + "path" + "path/filepath" + "strings" +) + +type fileSpec struct { + PodName string + PodNamespace string + File pathSpec +} + +type pathSpec interface { + String() string +} + +// localPath represents a client-native path, which will differ based +// on the client OS, its methods will use path/filepath package which +// is OS dependant +type localPath struct { + file string +} + +func newLocalPath(fileName string) localPath { + file := stripTrailingSlash(fileName) + return localPath{file: file} +} + +func (p localPath) String() string { + return p.file +} + +func (p localPath) Dir() localPath { + return newLocalPath(filepath.Dir(p.file)) +} + +func (p localPath) Base() localPath { + return newLocalPath(filepath.Base(p.file)) +} + +func (p localPath) Clean() localPath { + return newLocalPath(filepath.Clean(p.file)) +} + +func (p localPath) Join(elem pathSpec) localPath { + return newLocalPath(filepath.Join(p.file, elem.String())) +} + +func (p localPath) Glob() (matches []string, err error) { + return filepath.Glob(p.file) +} + +func (p localPath) StripSlashes() localPath { + return newLocalPath(stripLeadingSlash(p.file)) +} + +func isRelative(base, target localPath) bool { + relative, err := filepath.Rel(base.String(), target.String()) + if err != nil { + return false + } + return relative == "." || relative == stripPathShortcuts(relative) +} + +// remotePath represents always UNIX path, its methods will use path +// package which is always using `/` +type remotePath struct { + file string +} + +func newRemotePath(fileName string) remotePath { + // we assume remote file is a linux container but we need to convert + // windows path separators to unix style for consistent processing + file := strings.ReplaceAll(stripTrailingSlash(fileName), `\`, "/") + return remotePath{file: file} +} + +func (p remotePath) String() string { + return p.file +} + +func (p remotePath) Dir() remotePath { + return newRemotePath(path.Dir(p.file)) +} + +func (p remotePath) Base() remotePath { + return newRemotePath(path.Base(p.file)) +} + +func (p remotePath) Clean() remotePath { + return newRemotePath(path.Clean(p.file)) +} + +func (p remotePath) Join(elem pathSpec) remotePath { + return newRemotePath(path.Join(p.file, elem.String())) +} + +func (p remotePath) StripShortcuts() remotePath { + p = p.Clean() + return newRemotePath(stripPathShortcuts(p.file)) +} + +func (p remotePath) StripSlashes() remotePath { + return newRemotePath(stripLeadingSlash(p.file)) +} + +// strips trailing slash (if any) both unix and windows style +func stripTrailingSlash(file string) string { + if len(file) == 0 { + return file + } + if file != "/" && strings.HasSuffix(string(file[len(file)-1]), "/") { + return file[:len(file)-1] + } + return file +} + +func stripLeadingSlash(file string) string { + // tar strips the leading '/' and '\' if it's there, so we will too + return strings.TrimLeft(file, `/\`) +} + +// stripPathShortcuts removes any leading or trailing "../" from a given path +func stripPathShortcuts(p string) string { + newPath := p + trimmed := strings.TrimPrefix(newPath, "../") + + for trimmed != newPath { + newPath = trimmed + trimmed = strings.TrimPrefix(newPath, "../") + } + + // trim leftover {".", ".."} + if newPath == "." || newPath == ".." { + newPath = "" + } + + if len(newPath) > 0 && string(newPath[0]) == "/" { + return newPath[1:] + } + + return newPath +}