Merge pull request #28012 from sttts/sttts-kubectl-attach-err-non-existing-ctr

Automatic merge from submit-queue

kubectl attach: error out for non-existing containers

Currently, kubectl attach falls back to the first container which is pretty confusing.

Based on https://github.com/kubernetes/kubernetes/pull/27541.

pkg/kubectl/cmd/attach.go

@@ -186,7 +186,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)
@@ -231,26 +234,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
 }

pkg/kubectl/cmd/attach_test.go

@@ -37,13 +37,13 @@ import (
 type fakeRemoteAttach struct {
 	method string
 	url    *url.URL
-	attachErr error
+	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,6 +138,7 @@ 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",
@@ -138,7 +146,18 @@ func TestAttach(t *testing.T) {
 			podPath:         "/api/" + version + "/namespaces/test/pods/foo",
 			attachPath:      "/api/" + version + "/namespaces/test/pods/foo/attach",
 			pod:             attachPod(),
-			attachErr:  true,
+			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())
 		}
 	}
 }

pkg/kubectl/cmd/run.go

@@ -335,8 +335,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
 		}
@@ -353,7 +357,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
 		}