Skip to content

Commit

Permalink
fix(cp): ensure kubectl fails if source file doesnt exist in pod
Browse files Browse the repository at this point in the history 
Signed-off-by: Jian Zeng <zengjian.zj@bytedance.com>
  • Loading branch information
@knight42
knight42 committed May 15, 2021
1 parent 8c8f79c commit 033d546
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 26 deletions.
48 changes: 30 additions & 18 deletions staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go
View file
Expand Up @@ -207,7 +207,7 @@ func (o *CopyOptions) Run(args []string) error {
}

if len(srcSpec.PodName) != 0 {
return o.copyFromPod(srcSpec, destSpec)
return o.copyFromPod(srcSpec, destSpec, &exec.ExecOptions{})
}
if len(destSpec.PodName) != 0 {
return o.copyToPod(srcSpec, destSpec, &exec.ExecOptions{})
Expand Down Expand Up @@ -292,39 +292,46 @@ func (o *CopyOptions) copyToPod(src, dest fileSpec, options *exec.ExecOptions) e
return o.execute(options)
}

func (o *CopyOptions) copyFromPod(src, dest fileSpec) error {
func (o *CopyOptions) copyFromPod(src, dest fileSpec, options *exec.ExecOptions) error {
if len(src.File) == 0 || len(dest.File) == 0 {
return errFileCannotBeEmpty
}

reader, outStream := io.Pipe()
options := &exec.ExecOptions{
StreamOptions: exec.StreamOptions{
IOStreams: genericclioptions.IOStreams{
In: nil,
Out: outStream,
ErrOut: o.Out,
},

Namespace: src.PodNamespace,
PodName: src.PodName,
options.StreamOptions = exec.StreamOptions{
IOStreams: genericclioptions.IOStreams{
In: nil,
Out: outStream,
ErrOut: o.Out,
},

// TODO: Improve error messages by first testing if 'tar' is present in the container?
Command: []string{"tar", "cf", "-", src.File},
Executor: &exec.DefaultRemoteExecutor{},
Namespace: src.PodNamespace,
PodName: src.PodName,
}
// TODO: Improve error messages by first testing if 'tar' is present in the container?
options.Command = []string{"tar", "cf", "-", src.File}

errCh := make(chan error)
go func() {
defer outStream.Close()
cmdutil.CheckErr(o.execute(options))
defer close(errCh)
err := o.execute(options)
// `outStream` must be Close immediately after `o.execute` returns to unblock tarReader
outStream.Close()
if err != nil {
errCh <- err
}
}()
prefix := getPrefix(src.File)
prefix = path.Clean(prefix)
// 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)
err := o.untarAll(src, reader, dest.File, prefix)
if err != nil {
return err
}
return <-errCh
}

// stripPathShortcuts removes any leading or trailing "../" from a given path
Expand Down Expand Up @@ -429,7 +436,8 @@ 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(src fileSpec, reader io.ReadCloser, destDir, prefix string) error {
defer reader.Close()
symlinkWarningPrinted := false
// TODO: use compression here?
tarReader := tar.NewReader(reader)
Expand Down Expand Up @@ -520,6 +528,10 @@ func (o *CopyOptions) execute(options *exec.ExecOptions) error {
options.ContainerName = o.Container
}

if options.Executor == nil {
options.Executor = &exec.DefaultRemoteExecutor{}
}

options.Config = o.ClientConfig
options.PodClient = o.Clientset.CoreV1()

Expand Down
91 changes: 83 additions & 8 deletions staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go
View file
Expand Up @@ -23,6 +23,7 @@ import (
"io"
"io/ioutil"
"net/http"
"net/url"
"os"
"path"
"path/filepath"
Expand All @@ -33,12 +34,14 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/rest"
"k8s.io/client-go/rest/fake"
"k8s.io/client-go/tools/remotecommand"
kexec "k8s.io/kubectl/pkg/cmd/exec"
cmdtesting "k8s.io/kubectl/pkg/cmd/testing"
"k8s.io/kubectl/pkg/scheme"
Expand All @@ -52,6 +55,14 @@ const (
RegexFile FileType = 2
)

type fakeRemoteExecutor struct {
execErr error
}

func (f *fakeRemoteExecutor) Execute(method string, url *url.URL, config *rest.Config, stdin io.Reader, stdout, stderr io.Writer, tty bool, terminalSizeQueue remotecommand.TerminalSizeQueue) error {
return f.execErr
}

func TestExtractFileSpec(t *testing.T) {
tests := []struct {
spec string
Expand Down Expand Up @@ -251,9 +262,9 @@ func TestIsDestRelative(t *testing.T) {
}

func checkErr(t *testing.T, err error) {
t.Helper()
if err != nil {
t.Errorf("unexpected error: %v", err)
t.FailNow()
t.Fatalf("unexpected error: %v", err)
}
}

Expand Down Expand Up @@ -360,7 +371,7 @@ func TestTarUntar(t *testing.T) {
}

reader := bytes.NewBuffer(writer.Bytes())
if err := opts.untarAll(fileSpec{}, reader, dir2, ""); err != nil {
if err := opts.untarAll(fileSpec{}, ioutil.NopCloser(reader), dir2, ""); err != nil {
t.Fatalf("unexpected error: %v", err)
}

Expand Down Expand Up @@ -421,7 +432,7 @@ func TestTarUntarWrongPrefix(t *testing.T) {
}

reader := bytes.NewBuffer(writer.Bytes())
err = opts.untarAll(fileSpec{}, reader, dir2, "verylongprefix-showing-the-tar-was-tempered-with")
err = opts.untarAll(fileSpec{}, ioutil.NopCloser(reader), dir2, "verylongprefix-showing-the-tar-was-tempered-with")
if err == nil || !strings.Contains(err.Error(), "tar contents corrupted") {
t.Fatalf("unexpected error: %v", err)
}
Expand Down Expand Up @@ -536,7 +547,7 @@ func TestBadTar(t *testing.T) {
}

opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard())
if err := opts.untarAll(fileSpec{}, &buf, dir, "/prefix"); err != nil {
if err := opts.untarAll(fileSpec{}, ioutil.NopCloser(&buf), dir, "/prefix"); err != nil {
t.Errorf("unexpected error: %v ", err)
t.FailNow()
}
Expand All @@ -549,6 +560,70 @@ func TestBadTar(t *testing.T) {
}
}

func TestCopyFromPod(t *testing.T) {
tf := cmdtesting.NewTestFactory().WithNamespace("test")
defer tf.Cleanup()
ns := scheme.Codecs.WithoutConversion()
codec := scheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...)
pod := &v1.Pod{Spec: v1.PodSpec{Containers: []v1.Container{{Name: "test"}}}}
data := []byte(runtime.EncodeOrDie(codec, pod))

tf.Client = &fake.RESTClient{
GroupVersion: schema.GroupVersion{Version: "v1"},
NegotiatedSerializer: ns,
Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
resp := &http.Response{
StatusCode: http.StatusOK,
Header: cmdtesting.DefaultHeader(),
Body: ioutil.NopCloser(bytes.NewBuffer(data)),
}
return resp, nil
}),
}
tf.ClientConfigVal = cmdtesting.DefaultClientConfig()
ioStreams, _, _, _ := genericclioptions.NewTestIOStreams()

cmd := NewCmdCp(tf, ioStreams)
src := fileSpec{
PodNamespace: "test",
PodName: "pod-name",
File: "/tmp/foo",
}
dest := fileSpec{
File: "/tmp/foo",
}

testCases := []struct {
name string
expectErr error
}{
{
name: "copy nonexistent file from pod should throw error",
expectErr: fmt.Errorf("tar: Cannot stat: No such file or directory"),
},
{
name: "successfully copy from pod",
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
opts := NewCopyOptions(ioStreams)
err := opts.Complete(tf, cmd)
checkErr(t, err)
options := &kexec.ExecOptions{Executor: &fakeRemoteExecutor{execErr: testCase.expectErr}}
err = opts.copyFromPod(src, dest, options)
if err != nil {
if testCase.expectErr != nil {
return
}
t.Errorf("Unexpected error: %v", err)
} else if testCase.expectErr != nil {
t.Errorf("Unexpected success, expect error: %v", testCase.expectErr)
}
})
}
}

func TestCopyToPod(t *testing.T) {
tf := cmdtesting.NewTestFactory().WithNamespace("test")
ns := scheme.Codecs.WithoutConversion()
Expand Down Expand Up @@ -859,7 +934,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(fileSpec{}, ioutil.NopCloser(buf), filepath.Join(basedir), ""))

filepath.Walk(testdir, func(path string, info os.FileInfo, err error) error {
if err != nil {
Expand Down Expand Up @@ -910,7 +985,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(fileSpec{}, ioutil.NopCloser(buf), filepath.Join(dest), srcName))
cmpFileData(t, dest, content)
}

Expand Down

0 comments on commit 033d546

Please sign in to comment.