Merge pull request #36615 from timstclair/cri-attach-tty

pkg/kubelet/api/v1alpha1/runtime/api.proto

@@ -719,7 +719,7 @@ message ExecRequest {
     optional string container_id = 1;
     // Command to execute.
     repeated string cmd = 2;
-    // Whether use tty.
+    // Whether to exec the command in a TTY.
     optional bool tty = 3;
     // Whether to stream stdin.
     optional bool stdin = 4;
@@ -735,6 +735,9 @@ message AttachRequest {
     optional string container_id = 1;
     // Whether to stream stdin.
     optional bool stdin = 2;
+    // Whether the process being attached is running in a TTY.
+    // This must match the TTY setting in the ContainerConfig.
+    optional bool tty = 3;
 }
 
 message AttachResponse {

pkg/kubelet/container/helpers.go

@@ -246,3 +246,18 @@ func (r *containerCommandRunnerWrapper) RunInContainer(id ContainerID, cmd []str
 	// error.
 	return buffer.Bytes(), err
 }
+
+// GetContainerSpec gets the container spec by containerName.
+func GetContainerSpec(pod *v1.Pod, containerName string) *v1.Container {
+	for i, c := range pod.Spec.Containers {
+		if containerName == c.Name {
+			return &pod.Spec.Containers[i]
+		}
+	}
+	for i, c := range pod.Spec.InitContainers {
+		if containerName == c.Name {
+			return &pod.Spec.InitContainers[i]
+		}
+	}
+	return nil
+}

pkg/kubelet/container/runtime.go

@@ -140,7 +140,7 @@ type DirectStreamingRuntime interface {
 // the runtime server.
 type IndirectStreamingRuntime interface {
 	GetExec(id ContainerID, cmd []string, stdin, stdout, stderr, tty bool) (*url.URL, error)
-	GetAttach(id ContainerID, stdin, stdout, stderr bool) (*url.URL, error)
+	GetAttach(id ContainerID, stdin, stdout, stderr, tty bool) (*url.URL, error)
 	GetPortForward(podName, podNamespace string, podUID types.UID) (*url.URL, error)
 }
 

pkg/kubelet/container/testing/fake_runtime.go

@@ -78,12 +78,16 @@ type FakeDirectStreamingRuntime struct {
 	}
 }
 
+var _ DirectStreamingRuntime = &FakeDirectStreamingRuntime{}
+
 const FakeHost = "localhost:12345"
 
 type FakeIndirectStreamingRuntime struct {
 	*FakeRuntime
 }
 
+var _ IndirectStreamingRuntime = &FakeIndirectStreamingRuntime{}
+
 // FakeRuntime should implement Runtime.
 var _ Runtime = &FakeRuntime{}
 
@@ -459,7 +463,7 @@ func (f *FakeIndirectStreamingRuntime) GetExec(id ContainerID, cmd []string, std
 	return &url.URL{Host: FakeHost}, f.Err
 }
 
-func (f *FakeIndirectStreamingRuntime) GetAttach(id ContainerID, stdin, stdout, stderr bool) (*url.URL, error) {
+func (f *FakeIndirectStreamingRuntime) GetAttach(id ContainerID, stdin, stdout, stderr, tty bool) (*url.URL, error) {
 	f.Lock()
 	defer f.Unlock()
 

pkg/kubelet/dockershim/docker_streaming.go

@@ -51,13 +51,12 @@ func (r *streamingRuntime) exec(containerID string, cmd []string, in io.Reader,
 	return r.execHandler.ExecInContainer(r.client, container, cmd, in, out, errw, tty, resize, timeout)
 }
 
-func (r *streamingRuntime) Attach(containerID string, in io.Reader, out, errw io.WriteCloser, resize <-chan term.Size) error {
-	container, err := checkContainerStatus(r.client, containerID)
+func (r *streamingRuntime) Attach(containerID string, in io.Reader, out, errw io.WriteCloser, tty bool, resize <-chan term.Size) error {
+	_, err := checkContainerStatus(r.client, containerID)
 	if err != nil {
 		return err
 	}
 
-	tty := container.Config.Tty
 	return dockertools.AttachContainer(r.client, containerID, in, out, errw, tty, resize)
 }
 
@@ -99,12 +98,11 @@ func (ds *dockerService) Attach(req *runtimeapi.AttachRequest) (*runtimeapi.Atta
 	if ds.streamingServer == nil {
 		return nil, streaming.ErrorStreamingDisabled("attach")
 	}
-	container, err := checkContainerStatus(ds.client, req.GetContainerId())
+	_, err := checkContainerStatus(ds.client, req.GetContainerId())
 	if err != nil {
 		return nil, err
 	}
-	tty := container.Config.Tty
-	return ds.streamingServer.GetAttach(req, tty)
+	return ds.streamingServer.GetAttach(req)
 }
 
 // PortForward prepares a streaming endpoint to forward ports from a PodSandbox, and returns the address.

pkg/kubelet/kubelet_pods.go

@@ -1350,10 +1350,23 @@ func (kl *Kubelet) GetAttach(podFullName string, podUID types.UID, containerName
 			return nil, err
 		}
 		if container == nil {
-			return nil, fmt.Errorf("container not found (%q)", containerName)
+			return nil, fmt.Errorf("container %s not found in pod %s", containerName, podFullName)
+		}
+
+		// The TTY setting for attach must match the TTY setting in the initial container configuration,
+		// since whether the process is running in a TTY cannot be changed after it has started.  We
+		// need the api.Pod to get the TTY status.
+		pod, found := kl.GetPodByFullName(podFullName)
+		if !found || pod.UID != podUID {
+			return nil, fmt.Errorf("pod %s not found", podFullName)
+		}
+		containerSpec := kubecontainer.GetContainerSpec(pod, containerName)
+		if containerSpec == nil {
+			return nil, fmt.Errorf("container %s not found in pod %s", containerName, podFullName)
 		}
+		tty := containerSpec.TTY
 
-		return streamingRuntime.GetAttach(container.ID, streamOpts.Stdin, streamOpts.Stdout, streamOpts.Stderr)
+		return streamingRuntime.GetAttach(container.ID, streamOpts.Stdin, streamOpts.Stdout, streamOpts.Stderr, tty)
 	default:
 		return nil, fmt.Errorf("container runtime does not support attach")
 	}

pkg/kubelet/kuberuntime/helpers.go

@@ -130,22 +130,6 @@ func (m *kubeGenericRuntimeManager) sandboxToKubeContainer(s *runtimeapi.PodSand
 	}, nil
 }
 
-// getContainerSpec gets the container spec by containerName.
-func getContainerSpec(pod *v1.Pod, containerName string) *v1.Container {
-	for i, c := range pod.Spec.Containers {
-		if containerName == c.Name {
-			return &pod.Spec.Containers[i]
-		}
-	}
-	for i, c := range pod.Spec.InitContainers {
-		if containerName == c.Name {
-			return &pod.Spec.InitContainers[i]
-		}
-	}
-
-	return nil
-}
-
 // getImageUser gets uid or user name that will run the command(s) from image. The function
 // guarantees that only one of them is set.
 func (m *kubeGenericRuntimeManager) getImageUser(image string) (*int64, *string, error) {

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -490,7 +490,7 @@ func (m *kubeGenericRuntimeManager) restoreSpecsFromContainerLabels(containerID
 func (m *kubeGenericRuntimeManager) killContainer(pod *v1.Pod, containerID kubecontainer.ContainerID, containerName string, reason string, gracePeriodOverride *int64) error {
 	var containerSpec *v1.Container
 	if pod != nil {
-		containerSpec = getContainerSpec(pod, containerName)
+		containerSpec = kubecontainer.GetContainerSpec(pod, containerName)
 	} else {
 		// Restore necessary information if one of the specs is nil.
 		restoredPod, restoredContainer, err := m.restoreSpecsFromContainerLabels(containerID)
@@ -684,10 +684,11 @@ func (m *kubeGenericRuntimeManager) GetExec(id kubecontainer.ContainerID, cmd []
 }
 
 // GetAttach gets the endpoint the runtime will serve the attach request from.
-func (m *kubeGenericRuntimeManager) GetAttach(id kubecontainer.ContainerID, stdin, stdout, stderr bool) (*url.URL, error) {
+func (m *kubeGenericRuntimeManager) GetAttach(id kubecontainer.ContainerID, stdin, stdout, stderr, tty bool) (*url.URL, error) {
 	req := &runtimeapi.AttachRequest{
 		ContainerId: &id.ID,
 		Stdin:       &stdin,
+		Tty:         &tty,
 	}
 	resp, err := m.runtimeService.Attach(req)
 	if err != nil {

pkg/kubelet/server/streaming/server.go

@@ -42,7 +42,7 @@ type Server interface {
 	// Get the serving URL for the requests.
 	// Requests must not be nil. Responses may be nil iff an error is returned.
 	GetExec(*runtimeapi.ExecRequest) (*runtimeapi.ExecResponse, error)
-	GetAttach(req *runtimeapi.AttachRequest, tty bool) (*runtimeapi.AttachResponse, error)
+	GetAttach(req *runtimeapi.AttachRequest) (*runtimeapi.AttachResponse, error)
 	GetPortForward(*runtimeapi.PortForwardRequest) (*runtimeapi.PortForwardResponse, error)
 
 	// Start the server.
@@ -58,7 +58,7 @@ type Server interface {
 // The interface to execute the commands and provide the streams.
 type Runtime interface {
 	Exec(containerID string, cmd []string, in io.Reader, out, err io.WriteCloser, tty bool, resize <-chan term.Size) error
-	Attach(containerID string, in io.Reader, out, err io.WriteCloser, resize <-chan term.Size) error
+	Attach(containerID string, in io.Reader, out, err io.WriteCloser, tty bool, resize <-chan term.Size) error
 	PortForward(podSandboxID string, port int32, stream io.ReadWriteCloser) error
 }
 
@@ -154,12 +154,12 @@ func (s *server) GetExec(req *runtimeapi.ExecRequest) (*runtimeapi.ExecResponse,
 	}, nil
 }
 
-func (s *server) GetAttach(req *runtimeapi.AttachRequest, tty bool) (*runtimeapi.AttachResponse, error) {
+func (s *server) GetAttach(req *runtimeapi.AttachRequest) (*runtimeapi.AttachResponse, error) {
 	url := s.buildURL("attach", req.GetContainerId(), streamOpts{
 		stdin:  req.GetStdin(),
 		stdout: true,
-		stderr: !tty, // For TTY connections, both stderr is combined with stdout.
-		tty:    tty,
+		stderr: !req.GetTty(), // For TTY connections, both stderr is combined with stdout.
+		tty:    req.GetTty(),
 	})
 	return &runtimeapi.AttachResponse{
 		Url: &url,
@@ -314,7 +314,7 @@ func (a *criAdapter) ExecInContainer(podName string, podUID types.UID, container
 }
 
 func (a *criAdapter) AttachContainer(podName string, podUID types.UID, container string, in io.Reader, out, err io.WriteCloser, tty bool, resize <-chan term.Size) error {
-	return a.Attach(container, in, out, err, resize)
+	return a.Attach(container, in, out, err, tty, resize)
 }
 
 func (a *criAdapter) PortForward(podName string, podUID types.UID, port uint16, stream io.ReadWriteCloser) error {

pkg/kubelet/server/streaming/server_test.go

@@ -134,15 +134,16 @@ func TestGetAttach(t *testing.T) {
 		request := &runtimeapi.AttachRequest{
 			ContainerId: &containerID,
 			Stdin:       &test.stdin,
+			Tty:         &test.tty,
 		}
 		// Non-TLS
-		resp, err := server.GetAttach(request, test.tty)
+		resp, err := server.GetAttach(request)
 		assert.NoError(t, err, "testcase=%+v", test)
 		expectedURL := "http://" + testAddr + "/attach/" + testContainerID + test.expectedQuery
 		assert.Equal(t, expectedURL, resp.GetUrl(), "testcase=%+v", test)
 
 		// TLS
-		resp, err = tlsServer.GetAttach(request, test.tty)
+		resp, err = tlsServer.GetAttach(request)
 		assert.NoError(t, err, "testcase=%+v", test)
 		expectedURL = "https://" + testAddr + "/attach/" + testContainerID + test.expectedQuery
 		assert.Equal(t, expectedURL, resp.GetUrl(), "testcase=%+v", test)
@@ -299,7 +300,7 @@ func (f *fakeRuntime) Exec(containerID string, cmd []string, stdin io.Reader, st
 	return nil
 }
 
-func (f *fakeRuntime) Attach(containerID string, stdin io.Reader, stdout, stderr io.WriteCloser, resize <-chan term.Size) error {
+func (f *fakeRuntime) Attach(containerID string, stdin io.Reader, stdout, stderr io.WriteCloser, tty bool, resize <-chan term.Size) error {
 	assert.Equal(f.t, testContainerID, containerID)
 	doServerStreams(f.t, "attach", stdin, stdout, stderr)
 	return nil