Fix Windows terminal handling

Fix some issues with Windows terminal handling with respect to TTYs that came up as part of the
code that adds support for terminal resizing.

cmd/hyperkube/kubectl.go

@@ -17,15 +17,14 @@ limitations under the License.
 package main
 
 import (
-	"github.com/docker/docker/pkg/term"
+	"os"
+
 	"k8s.io/kubernetes/pkg/kubectl/cmd"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 )
 
 func NewKubectlServer() *Server {
-	// need to use term.StdStreams to get the right IO refs on Windows
+	cmd := cmd.NewKubectlCommand(cmdutil.NewFactory(nil), os.Stdin, os.Stdout, os.Stderr)
-	stdin, stdout, stderr := term.StdStreams()
-	cmd := cmd.NewKubectlCommand(cmdutil.NewFactory(nil), stdin, stdout, stderr)
 	localFlags := cmd.LocalFlags()
 	localFlags.SetInterspersed(false)
 

cmd/kubectl/app/kubectl.go

@@ -17,7 +17,7 @@ limitations under the License.
 package app
 
 import (
-	"github.com/docker/docker/pkg/term"
+	"os"
 
 	"k8s.io/kubernetes/pkg/kubectl/cmd"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
@@ -28,8 +28,6 @@ WARNING: this logic is duplicated, with minor changes, in cmd/hyperkube/kubectl.
 Any salient changes here will need to be manually reflected in that file.
 */
 func Run() error {
-	// need to use term.StdStreams to get the right IO refs on Windows
+	cmd := cmd.NewKubectlCommand(cmdutil.NewFactory(nil), os.Stdin, os.Stdout, os.Stderr)
-	stdin, stdout, stderr := term.StdStreams()
-	cmd := cmd.NewKubectlCommand(cmdutil.NewFactory(nil), stdin, stdout, stderr)
 	return cmd.Execute()
 }

pkg/kubectl/cmd/attach.go

@@ -32,7 +32,6 @@ import (
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	remotecommandserver "k8s.io/kubernetes/pkg/kubelet/server/remotecommand"
 	utilerrors "k8s.io/kubernetes/pkg/util/errors"
-	"k8s.io/kubernetes/pkg/util/interrupt"
 	"k8s.io/kubernetes/pkg/util/term"
 )
 
@@ -51,9 +50,11 @@ var (
 
 func NewCmdAttach(f *cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer) *cobra.Command {
 	options := &AttachOptions{
+		StreamOptions: StreamOptions{
 			In:  cmdIn,
 			Out: cmdOut,
 			Err: cmdErr,
+		},
 
 		Attach: &DefaultRemoteAttach{},
 	}
@@ -100,20 +101,10 @@ func (*DefaultRemoteAttach) Attach(method string, url *url.URL, config *restclie
 
 // AttachOptions declare the arguments accepted by the Exec command
 type AttachOptions struct {
-	Namespace     string
+	StreamOptions
-	PodName       string
+
-	ContainerName string
-	Stdin         bool
-	TTY           bool
 	CommandName string
 
-	// InterruptParent, if set, is used to handle interrupts while attached
-	InterruptParent *interrupt.Handler
-
-	In  io.Reader
-	Out io.Writer
-	Err io.Writer
-
 	Pod *api.Pod
 
 	Attach RemoteAttach
@@ -188,38 +179,30 @@ func (p *AttachOptions) Run() error {
 	}
 	pod := p.Pod
 
-	// ensure we can recover the terminal while attached
-	t := term.TTY{
-		Parent: p.InterruptParent,
-		Out:    p.Out,
-	}
-
 	// check for TTY
-	tty := p.TTY
 	containerToAttach, err := p.containerToAttachTo(pod)
 	if err != nil {
 		return fmt.Errorf("cannot attach to the container: %v", err)
 	}
-	if tty && !containerToAttach.TTY {
+	if p.TTY && !containerToAttach.TTY {
-		tty = false
+		p.TTY = false
+		if p.Err != nil {
 			fmt.Fprintf(p.Err, "Unable to use a TTY - container %s did not allocate one\n", containerToAttach.Name)
 		}
-	if p.Stdin {
+	} else if !p.TTY && containerToAttach.TTY {
-		t.In = p.In
+		// the container was launched with a TTY, so we have to force a TTY here, otherwise you'll get
-		if tty && !t.IsTerminalIn() {
+		// an error "Unrecognized input header"
-			tty = false
+		p.TTY = true
-			fmt.Fprintln(p.Err, "Unable to use a TTY - input is not a terminal or the right kind of file")
 	}
-	} else {
+
-		p.In = nil
+	// ensure we can recover the terminal while attached
-	}
+	t := p.setupTTY()
-	t.Raw = tty
 
 	// save p.Err so we can print the command prompt message below
 	stderr := p.Err
 
 	var sizeQueue term.TerminalSizeQueue
-	if tty {
+	if t.Raw {
 		if size := t.GetSize(); size != nil {
 			// fake resizing +1 and then back to normal so that attach-detach-reattach will result in the
 			// screen being redrawn
@@ -252,17 +235,17 @@ func (p *AttachOptions) Run() error {
 			Stdin:     p.Stdin,
 			Stdout:    p.Out != nil,
 			Stderr:    p.Err != nil,
-			TTY:       tty,
+			TTY:       t.Raw,
 		}, api.ParameterCodec)
 
-		return p.Attach.Attach("POST", req.URL(), p.Config, p.In, p.Out, p.Err, tty, sizeQueue)
+		return p.Attach.Attach("POST", req.URL(), p.Config, p.In, p.Out, p.Err, t.Raw, sizeQueue)
 	}
 
 	if err := t.Safe(fn); err != nil {
 		return err
 	}
 
-	if p.Stdin && tty && pod.Spec.RestartPolicy == api.RestartPolicyAlways {
+	if p.Stdin && t.Raw && pod.Spec.RestartPolicy == api.RestartPolicyAlways {
 		fmt.Fprintf(p.Out, "Session ended, resume using '%s %s -c %s -i -t' command when the pod is running\n", p.CommandName, pod.Name, containerToAttach.Name)
 	}
 	return nil

pkg/kubectl/cmd/attach_test.go

@@ -74,21 +74,21 @@ func TestPodAndContainerAttach(t *testing.T) {
 			name:        "no container, no flags",
 		},
 		{
-			p:                 &AttachOptions{ContainerName: "bar"},
+			p:                 &AttachOptions{StreamOptions: StreamOptions{ContainerName: "bar"}},
 			args:              []string{"foo"},
 			expectedPod:       "foo",
 			expectedContainer: "bar",
 			name:              "container in flag",
 		},
 		{
-			p:                 &AttachOptions{ContainerName: "initfoo"},
+			p:                 &AttachOptions{StreamOptions: StreamOptions{ContainerName: "initfoo"}},
 			args:              []string{"foo"},
 			expectedPod:       "foo",
 			expectedContainer: "initfoo",
 			name:              "init container in flag",
 		},
 		{
-			p:           &AttachOptions{ContainerName: "bar"},
+			p:           &AttachOptions{StreamOptions: StreamOptions{ContainerName: "bar"}},
 			args:        []string{"foo", "-c", "wrong"},
 			expectError: true,
 			name:        "non-existing container in flag",
@@ -187,10 +187,12 @@ func TestAttach(t *testing.T) {
 			remoteAttach.err = fmt.Errorf("attach error")
 		}
 		params := &AttachOptions{
+			StreamOptions: StreamOptions{
 				ContainerName: test.container,
 				In:            bufIn,
 				Out:           bufOut,
 				Err:           bufErr,
+			},
 			Attach: remoteAttach,
 		}
 		cmd := &cobra.Command{}
@@ -261,12 +263,14 @@ func TestAttachWarnings(t *testing.T) {
 		bufIn := bytes.NewBuffer([]byte{})
 		ex := &fakeRemoteAttach{}
 		params := &AttachOptions{
+			StreamOptions: StreamOptions{
 				ContainerName: test.container,
 				In:            bufIn,
 				Out:           bufOut,
 				Err:           bufErr,
 				Stdin:         test.stdin,
 				TTY:           test.tty,
+			},
 			Attach: ex,
 		}
 		cmd := &cobra.Command{}

pkg/kubectl/cmd/exec.go

@@ -21,6 +21,7 @@ import (
 	"io"
 	"net/url"
 
+	dockerterm "github.com/docker/docker/pkg/term"
 	"github.com/golang/glog"
 	"github.com/renstrom/dedent"
 	"github.com/spf13/cobra"
@@ -49,9 +50,11 @@ var (
 
 func NewCmdExec(f *cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer) *cobra.Command {
 	options := &ExecOptions{
+		StreamOptions: StreamOptions{
 			In:  cmdIn,
 			Out: cmdOut,
 			Err: cmdErr,
+		},
 
 		Executor: &DefaultRemoteExecutor{},
 	}
@@ -98,22 +101,29 @@ func (*DefaultRemoteExecutor) Execute(method string, url *url.URL, config *restc
 	})
 }
 
-// ExecOptions declare the arguments accepted by the Exec command
+type StreamOptions struct {
-type ExecOptions struct {
 	Namespace     string
 	PodName       string
 	ContainerName string
 	Stdin         bool
 	TTY           bool
-	Command       []string
-
 	// InterruptParent, if set, is used to handle interrupts while attached
 	InterruptParent *interrupt.Handler
-
 	In              io.Reader
 	Out             io.Writer
 	Err             io.Writer
 
+	// for testing
+	overrideStreams func() (io.ReadCloser, io.Writer, io.Writer)
+	isTerminalIn    func(t term.TTY) bool
+}
+
+// ExecOptions declare the arguments accepted by the Exec command
+type ExecOptions struct {
+	StreamOptions
+
+	Command []string
+
 	Executor RemoteExecutor
 	Client   *client.Client
 	Config   *restclient.Config
@@ -177,6 +187,58 @@ func (p *ExecOptions) Validate() error {
 	return nil
 }
 
+func (o *StreamOptions) setupTTY() term.TTY {
+	t := term.TTY{
+		Parent: o.InterruptParent,
+		Out:    o.Out,
+	}
+
+	if !o.Stdin {
+		// need to nil out o.In to make sure we don't create a stream for stdin
+		o.In = nil
+		o.TTY = false
+		return t
+	}
+
+	t.In = o.In
+	if !o.TTY {
+		return t
+	}
+
+	if o.isTerminalIn == nil {
+		o.isTerminalIn = func(tty term.TTY) bool {
+			return tty.IsTerminalIn()
+		}
+	}
+	if !o.isTerminalIn(t) {
+		o.TTY = false
+
+		if o.Err != nil {
+			fmt.Fprintln(o.Err, "Unable to use a TTY - input is not a terminal or the right kind of file")
+		}
+
+		return t
+	}
+
+	// if we get to here, the user wants to attach stdin, wants a TTY, and o.In is a terminal, so we
+	// can safely set t.Raw to true
+	t.Raw = true
+
+	if o.overrideStreams == nil {
+		// use dockerterm.StdStreams() to get the right I/O handles on Windows
+		o.overrideStreams = dockerterm.StdStreams
+	}
+	stdin, stdout, _ := o.overrideStreams()
+	o.In = stdin
+	t.In = stdin
+	if o.Out != nil {
+		o.Out = stdout
+		t.Out = stdout
+	}
+
+	return t
+}
+
 // Run executes a validated remote execution against a pod.
 func (p *ExecOptions) Run() error {
 	pod, err := p.Client.Pods(p.Namespace).Get(p.PodName)
@@ -195,26 +257,10 @@ func (p *ExecOptions) Run() error {
 	}
 
 	// ensure we can recover the terminal while attached
-	t := term.TTY{
+	t := p.setupTTY()
-		Parent: p.InterruptParent,
-		Out:    p.Out,
-	}
-
-	// check for TTY
-	tty := p.TTY
-	if p.Stdin {
-		t.In = p.In
-		if tty && !t.IsTerminalIn() {
-			tty = false
-			fmt.Fprintln(p.Err, "Unable to use a TTY - input is not a terminal or the right kind of file")
-		}
-	} else {
-		p.In = nil
-	}
-	t.Raw = tty
 
 	var sizeQueue term.TerminalSizeQueue
-	if tty {
+	if t.Raw {
 		// this call spawns a goroutine to monitor/update the terminal size
 		sizeQueue = t.MonitorSize(t.GetSize())
 
@@ -237,10 +283,10 @@ func (p *ExecOptions) Run() error {
 			Stdin:     p.Stdin,
 			Stdout:    p.Out != nil,
 			Stderr:    p.Err != nil,
-			TTY:       tty,
+			TTY:       t.Raw,
 		}, api.ParameterCodec)
 
-		return p.Executor.Execute("POST", req.URL(), p.Config, p.In, p.Out, p.Err, tty, sizeQueue)
+		return p.Executor.Execute("POST", req.URL(), p.Config, p.In, p.Out, p.Err, t.Raw, sizeQueue)
 	}
 
 	if err := t.Safe(fn); err != nil {

pkg/kubectl/cmd/exec_test.go

@@ -20,9 +20,11 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net/http"
 	"net/url"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -65,19 +67,19 @@ func TestPodAndContainer(t *testing.T) {
 			name:          "empty",
 		},
 		{
-			p:             &ExecOptions{PodName: "foo"},
+			p:             &ExecOptions{StreamOptions: StreamOptions{PodName: "foo"}},
 			argsLenAtDash: -1,
 			expectError:   true,
 			name:          "no cmd",
 		},
 		{
-			p:             &ExecOptions{PodName: "foo", ContainerName: "bar"},
+			p:             &ExecOptions{StreamOptions: StreamOptions{PodName: "foo", ContainerName: "bar"}},
 			argsLenAtDash: -1,
 			expectError:   true,
 			name:          "no cmd, w/ container",
 		},
 		{
-			p:             &ExecOptions{PodName: "foo"},
+			p:             &ExecOptions{StreamOptions: StreamOptions{PodName: "foo"}},
 			args:          []string{"cmd"},
 			argsLenAtDash: -1,
 			expectedPod:   "foo",
@@ -115,7 +117,7 @@ func TestPodAndContainer(t *testing.T) {
 			name:          "cmd, cmd is behind dash",
 		},
 		{
-			p:                 &ExecOptions{ContainerName: "bar"},
+			p:                 &ExecOptions{StreamOptions: StreamOptions{ContainerName: "bar"}},
 			args:              []string{"foo", "cmd"},
 			argsLenAtDash:     -1,
 			expectedPod:       "foo",
@@ -206,11 +208,13 @@ func TestExec(t *testing.T) {
 			ex.execErr = fmt.Errorf("exec error")
 		}
 		params := &ExecOptions{
+			StreamOptions: StreamOptions{
 				PodName:       "foo",
 				ContainerName: "bar",
 				In:            bufIn,
 				Out:           bufOut,
 				Err:           bufErr,
+			},
 			Executor: ex,
 		}
 		cmd := &cobra.Command{}
@@ -257,3 +261,124 @@ func execPod() *api.Pod {
 		},
 	}
 }
+
+func TestSetupTTY(t *testing.T) {
+	stderr := &bytes.Buffer{}
+
+	// test 1 - don't attach stdin
+	o := &StreamOptions{
+		// InterruptParent: ,
+		Stdin: false,
+		In:    &bytes.Buffer{},
+		Out:   &bytes.Buffer{},
+		Err:   stderr,
+		TTY:   true,
+	}
+
+	tty := o.setupTTY()
+
+	if o.In != nil {
+		t.Errorf("don't attach stdin: o.In should be nil")
+	}
+	if tty.In != nil {
+		t.Errorf("don't attach stdin: tty.In should be nil")
+	}
+	if o.TTY {
+		t.Errorf("don't attach stdin: o.TTY should be false")
+	}
+	if tty.Raw {
+		t.Errorf("don't attach stdin: tty.Raw should be false")
+	}
+	if len(stderr.String()) > 0 {
+		t.Errorf("don't attach stdin: stderr wasn't empty: %s", stderr.String())
+	}
+
+	// tests from here on attach stdin
+	// test 2 - don't request a TTY
+	o.Stdin = true
+	o.In = &bytes.Buffer{}
+	o.TTY = false
+
+	tty = o.setupTTY()
+
+	if o.In == nil {
+		t.Errorf("attach stdin, no TTY: o.In should not be nil")
+	}
+	if tty.In != o.In {
+		t.Errorf("attach stdin, no TTY: tty.In should equal o.In")
+	}
+	if o.TTY {
+		t.Errorf("attach stdin, no TTY: o.TTY should be false")
+	}
+	if tty.Raw {
+		t.Errorf("attach stdin, no TTY: tty.Raw should be false")
+	}
+	if len(stderr.String()) > 0 {
+		t.Errorf("attach stdin, no TTY: stderr wasn't empty: %s", stderr.String())
+	}
+
+	// test 3 - request a TTY, but stdin is not a terminal
+	o.Stdin = true
+	o.In = &bytes.Buffer{}
+	o.Err = stderr
+	o.TTY = true
+
+	tty = o.setupTTY()
+
+	if o.In == nil {
+		t.Errorf("attach stdin, TTY, not a terminal: o.In should not be nil")
+	}
+	if tty.In != o.In {
+		t.Errorf("attach stdin, TTY, not a terminal: tty.In should equal o.In")
+	}
+	if o.TTY {
+		t.Errorf("attach stdin, TTY, not a terminal: o.TTY should be false")
+	}
+	if tty.Raw {
+		t.Errorf("attach stdin, TTY, not a terminal: tty.Raw should be false")
+	}
+	if !strings.Contains(stderr.String(), "input is not a terminal") {
+		t.Errorf("attach stdin, TTY, not a terminal: expected 'input is not a terminal' to stderr")
+	}
+
+	// test 4 - request a TTY, stdin is a terminal
+	o.Stdin = true
+	o.In = &bytes.Buffer{}
+	stderr.Reset()
+	o.TTY = true
+
+	overrideStdin := ioutil.NopCloser(&bytes.Buffer{})
+	overrideStdout := &bytes.Buffer{}
+	overrideStderr := &bytes.Buffer{}
+	o.overrideStreams = func() (io.ReadCloser, io.Writer, io.Writer) {
+		return overrideStdin, overrideStdout, overrideStderr
+	}
+
+	o.isTerminalIn = func(tty term.TTY) bool {
+		return true
+	}
+
+	tty = o.setupTTY()
+
+	if o.In != overrideStdin {
+		t.Errorf("attach stdin, TTY, is a terminal: o.In should equal overrideStdin")
+	}
+	if tty.In != o.In {
+		t.Errorf("attach stdin, TTY, is a terminal: tty.In should equal o.In")
+	}
+	if !o.TTY {
+		t.Errorf("attach stdin, TTY, is a terminal: o.TTY should be true")
+	}
+	if !tty.Raw {
+		t.Errorf("attach stdin, TTY, is a terminal: tty.Raw should be true")
+	}
+	if len(stderr.String()) > 0 {
+		t.Errorf("attach stdin, TTY, is a terminal: stderr wasn't empty: %s", stderr.String())
+	}
+	if o.Out != overrideStdout {
+		t.Errorf("attach stdin, TTY, is a terminal: o.Out should equal overrideStdout")
+	}
+	if tty.Out != o.Out {
+		t.Errorf("attach stdin, TTY, is a terminal: tty.Out should equal o.Out")
+	}
+}

pkg/kubectl/cmd/run.go

@@ -226,11 +226,13 @@ func Run(f *cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer, cmd *cob
 
 	if attach {
 		opts := &AttachOptions{
+			StreamOptions: StreamOptions{
 				In:    cmdIn,
 				Out:   cmdOut,
 				Err:   cmdErr,
 				Stdin: interactive,
 				TTY:   tty,
+			},
 
 			CommandName: cmd.Parent().CommandPath() + " attach",
 

pkg/util/term/resize.go

@@ -32,10 +32,11 @@ type Size struct {
 // GetSize returns the current size of the user's terminal. If it isn't a terminal,
 // nil is returned.
 func (t TTY) GetSize() *Size {
-	if !t.IsTerminalOut() {
+	outFd, isTerminal := term.GetFdInfo(t.Out)
+	if !isTerminal {
 		return nil
 	}
-	return GetSize(t.Out.(fd).Fd())
+	return GetSize(outFd)
 }
 
 // GetSize returns the current size of the terminal associated with fd.
@@ -57,7 +58,8 @@ func SetSize(fd uintptr, size Size) error {
 // MonitorSize monitors the terminal's size. It returns a TerminalSizeQueue primed with
 // initialSizes, or nil if there's no TTY present.
 func (t *TTY) MonitorSize(initialSizes ...*Size) TerminalSizeQueue {
-	if !t.IsTerminalOut() {
+	outFd, isTerminal := term.GetFdInfo(t.Out)
+	if !isTerminal {
 		return nil
 	}
 
@@ -69,7 +71,7 @@ func (t *TTY) MonitorSize(initialSizes ...*Size) TerminalSizeQueue {
 		stopResizing: make(chan struct{}),
 	}
 
-	t.sizeQueue.monitorSize(initialSizes...)
+	t.sizeQueue.monitorSize(outFd, initialSizes...)
 
 	return t.sizeQueue
 }
@@ -94,7 +96,7 @@ var _ TerminalSizeQueue = &sizeQueue{}
 
 // monitorSize primes resizeChan with initialSizes and then monitors for resize events. With each
 // new event, it sends the current terminal size to resizeChan.
-func (s *sizeQueue) monitorSize(initialSizes ...*Size) {
+func (s *sizeQueue) monitorSize(outFd uintptr, initialSizes ...*Size) {
 	// send the initial sizes
 	for i := range initialSizes {
 		if initialSizes[i] != nil {
@@ -104,7 +106,7 @@ func (s *sizeQueue) monitorSize(initialSizes ...*Size) {
 
 	resizeEvents := make(chan Size, 1)
 
-	monitorResizeEvents(s.t.Out.(fd).Fd(), resizeEvents, s.stopResizing)
+	monitorResizeEvents(outFd, resizeEvents, s.stopResizing)
 
 	// listen for resize events in the background
 	go func() {

pkg/util/term/resizeevents_windows.go

@@ -29,7 +29,11 @@ func monitorResizeEvents(fd uintptr, resizeEvents chan<- Size, stop chan struct{
 	go func() {
 		defer runtime.HandleCrash()
 
-		var lastSize Size
+		size := GetSize(fd)
+		if size == nil {
+			return
+		}
+		lastSize := *size
 
 		for {
 			// see if we need to stop running

pkg/util/term/term.go

@@ -52,11 +52,6 @@ type TTY struct {
 	sizeQueue *sizeQueue
 }
 
-// fd returns a file descriptor for a given object.
-type fd interface {
-	Fd() uintptr
-}
-
 // IsTerminalIn returns true if t.In is a terminal. Does not check /dev/tty
 // even if TryDev is set.
 func (t TTY) IsTerminalIn() bool {
@@ -71,8 +66,8 @@ func (t TTY) IsTerminalOut() bool {
 
 // IsTerminal returns whether the passed object is a terminal or not
 func IsTerminal(i interface{}) bool {
-	file, ok := i.(fd)
+	_, terminal := term.GetFdInfo(i)
-	return ok && term.IsTerminal(file.Fd())
+	return terminal
 }
 
 // Safe invokes the provided function and will attempt to ensure that when the
@@ -82,22 +77,16 @@ func IsTerminal(i interface{}) bool {
 // If the input file descriptor is not a TTY and TryDev is true, the /dev/tty file
 // will be opened (if available).
 func (t TTY) Safe(fn SafeFunc) error {
-	in := t.In
+	inFd, isTerminal := term.GetFdInfo(t.In)
 
-	var hasFd bool
+	if !isTerminal && t.TryDev {
-	var inFd uintptr
-	if desc, ok := in.(fd); ok && in != nil {
-		inFd = desc.Fd()
-		hasFd = true
-	}
-	if t.TryDev && (!hasFd || !term.IsTerminal(inFd)) {
 		if f, err := os.Open("/dev/tty"); err == nil {
 			defer f.Close()
 			inFd = f.Fd()
-			hasFd = true
+			isTerminal = term.IsTerminal(inFd)
 		}
 	}
-	if !hasFd || !term.IsTerminal(inFd) {
+	if !isTerminal {
 		return fn()
 	}