From 0acca44dc165395587becce1a656176c896eb2ca Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 24 Jun 2016 13:05:59 +0200 Subject: [PATCH] Error out on non-existing container in kubectl attach --- pkg/kubectl/cmd/attach.go | 33 +++++++++++------ pkg/kubectl/cmd/attach_test.go | 67 ++++++++++++++++++++++------------ pkg/kubectl/cmd/run.go | 8 +++- 3 files changed, 70 insertions(+), 38 deletions(-) diff --git a/pkg/kubectl/cmd/attach.go b/pkg/kubectl/cmd/attach.go index ed083ac2393..78748c01936 100644 --- a/pkg/kubectl/cmd/attach.go +++ b/pkg/kubectl/cmd/attach.go @@ -184,7 +184,10 @@ func (p *AttachOptions) Run() error { // check for TTY tty := p.TTY - containerToAttach := p.GetContainer(pod) + containerToAttach, err := p.containerToAttachTo(pod) + if err != nil { + return fmt.Errorf("cannot attach to the container: %v", err) + } if tty && !containerToAttach.TTY { tty = false fmt.Fprintf(p.Err, "Unable to use a TTY - container %s did not allocate one\n", containerToAttach.Name) @@ -229,26 +232,32 @@ func (p *AttachOptions) Run() error { return nil } -// GetContainer returns the container to attach to, with a fallback. -func (p *AttachOptions) GetContainer(pod *api.Pod) api.Container { +// containerToAttach returns a reference to the container to attach to, given +// by name or the first container if name is empty. +func (p *AttachOptions) containerToAttachTo(pod *api.Pod) (*api.Container, error) { if len(p.ContainerName) > 0 { - for _, container := range pod.Spec.Containers { - if container.Name == p.ContainerName { - return container + for i := range pod.Spec.Containers { + if pod.Spec.Containers[i].Name == p.ContainerName { + return &pod.Spec.Containers[i], nil } } - for _, container := range pod.Spec.InitContainers { - if container.Name == p.ContainerName { - return container + for i := range pod.Spec.InitContainers { + if pod.Spec.InitContainers[i].Name == p.ContainerName { + return &pod.Spec.InitContainers[i], nil } } + return nil, fmt.Errorf("container not found (%s)", p.ContainerName) } glog.V(4).Infof("defaulting container name to %s", pod.Spec.Containers[0].Name) - return pod.Spec.Containers[0] + return &pod.Spec.Containers[0], nil } // GetContainerName returns the name of the container to attach to, with a fallback. -func (p *AttachOptions) GetContainerName(pod *api.Pod) string { - return p.GetContainer(pod).Name +func (p *AttachOptions) GetContainerName(pod *api.Pod) (string, error) { + c, err := p.containerToAttachTo(pod) + if err != nil { + return "", err + } + return c.Name, nil } diff --git a/pkg/kubectl/cmd/attach_test.go b/pkg/kubectl/cmd/attach_test.go index e4af36582c7..eb67aaf9ba1 100644 --- a/pkg/kubectl/cmd/attach_test.go +++ b/pkg/kubectl/cmd/attach_test.go @@ -35,15 +35,15 @@ import ( ) type fakeRemoteAttach struct { - method string - url *url.URL - attachErr error + method string + url *url.URL + err error } func (f *fakeRemoteAttach) Attach(method string, url *url.URL, config *restclient.Config, stdin io.Reader, stdout, stderr io.Writer, tty bool) error { f.method = method f.url = url - return f.attachErr + return f.err } func TestPodAndContainerAttach(t *testing.T) { @@ -86,6 +86,12 @@ func TestPodAndContainerAttach(t *testing.T) { expectedContainer: "initfoo", name: "init container in flag", }, + { + p: &AttachOptions{ContainerName: "bar"}, + args: []string{"foo", "-c", "wrong"}, + expectError: true, + name: "non-existing container in flag", + }, } for _, test := range tests { @@ -123,7 +129,8 @@ func TestAttach(t *testing.T) { tests := []struct { name, version, podPath, attachPath, container string pod *api.Pod - attachErr bool + remoteAttachErr bool + exepctedErr string }{ { name: "pod attach", @@ -131,14 +138,26 @@ func TestAttach(t *testing.T) { podPath: "/api/" + version + "/namespaces/test/pods/foo", attachPath: "/api/" + version + "/namespaces/test/pods/foo/attach", pod: attachPod(), + container: "bar", }, { - name: "pod attach error", - version: version, - podPath: "/api/" + version + "/namespaces/test/pods/foo", - attachPath: "/api/" + version + "/namespaces/test/pods/foo/attach", - pod: attachPod(), - attachErr: true, + name: "pod attach error", + version: version, + podPath: "/api/" + version + "/namespaces/test/pods/foo", + attachPath: "/api/" + version + "/namespaces/test/pods/foo/attach", + pod: attachPod(), + remoteAttachErr: true, + container: "bar", + exepctedErr: "attach error", + }, + { + name: "container not found error", + version: version, + podPath: "/api/" + version + "/namespaces/test/pods/foo", + attachPath: "/api/" + version + "/namespaces/test/pods/foo/attach", + pod: attachPod(), + container: "foo", + exepctedErr: "cannot attach to the container: container not found (foo)", }, } for _, test := range tests { @@ -162,42 +181,42 @@ func TestAttach(t *testing.T) { bufOut := bytes.NewBuffer([]byte{}) bufErr := bytes.NewBuffer([]byte{}) bufIn := bytes.NewBuffer([]byte{}) - ex := &fakeRemoteAttach{} - if test.attachErr { - ex.attachErr = fmt.Errorf("attach error") + remoteAttach := &fakeRemoteAttach{} + if test.remoteAttachErr { + remoteAttach.err = fmt.Errorf("attach error") } params := &AttachOptions{ - ContainerName: "bar", + ContainerName: test.container, In: bufIn, Out: bufOut, Err: bufErr, - Attach: ex, + Attach: remoteAttach, } cmd := &cobra.Command{} if err := params.Complete(f, cmd, []string{"foo"}); err != nil { t.Fatal(err) } err := params.Run() - if test.attachErr && err != ex.attachErr { + if test.exepctedErr != "" && err.Error() != test.exepctedErr { t.Errorf("%s: Unexpected exec error: %v", test.name, err) continue } - if !test.attachErr && err != nil { + if test.exepctedErr == "" && err != nil { t.Errorf("%s: Unexpected error: %v", test.name, err) continue } - if test.attachErr { + if test.exepctedErr != "" { continue } - if ex.url.Path != test.attachPath { + if remoteAttach.url.Path != test.attachPath { t.Errorf("%s: Did not get expected path for exec request", test.name) continue } - if ex.method != "POST" { - t.Errorf("%s: Did not get method for attach request: %s", test.name, ex.method) + if remoteAttach.method != "POST" { + t.Errorf("%s: Did not get method for attach request: %s", test.name, remoteAttach.method) } - if ex.url.Query().Get("container") != "bar" { - t.Errorf("%s: Did not have query parameters: %s", test.name, ex.url.Query()) + if remoteAttach.url.Query().Get("container") != "bar" { + t.Errorf("%s: Did not have query parameters: %s", test.name, remoteAttach.url.Query()) } } } diff --git a/pkg/kubectl/cmd/run.go b/pkg/kubectl/cmd/run.go index 915c61d83f6..7e3c6f62309 100644 --- a/pkg/kubectl/cmd/run.go +++ b/pkg/kubectl/cmd/run.go @@ -332,8 +332,12 @@ func handleAttachPod(f *cmdutil.Factory, c *client.Client, pod *api.Pod, opts *A if err != nil { return err } + ctrName, err := opts.GetContainerName(pod) + if err != nil { + return err + } if status == api.PodSucceeded || status == api.PodFailed { - req, err := f.LogsForObject(pod, &api.PodLogOptions{Container: opts.GetContainerName(pod)}) + req, err := f.LogsForObject(pod, &api.PodLogOptions{Container: ctrName}) if err != nil { return err } @@ -350,7 +354,7 @@ func handleAttachPod(f *cmdutil.Factory, c *client.Client, pod *api.Pod, opts *A opts.Namespace = pod.Namespace if err := opts.Run(); err != nil { fmt.Fprintf(opts.Out, "Error attaching, falling back to logs: %v\n", err) - req, err := f.LogsForObject(pod, &api.PodLogOptions{Container: opts.GetContainerName(pod)}) + req, err := f.LogsForObject(pod, &api.PodLogOptions{Container: ctrName}) if err != nil { return err }