Exec should be easier to reuse as a command

We have a few use cases where we want to have shortcut commands
for common behavior. In these cases, we want to default commands
with a simpler syntax, so being able to reuse Exec from code but
not share the same arguments is useful.

In this case, we're introducing 'rsh' which tries to run exec -itp --
bash.

pkg/kubectl/cmd/exec.go

@@ -17,6 +17,7 @@ limitations under the License.
 package cmd
 
 import (
+	"fmt"
 	"io"
 	"os"
 	"os/signal"
@@ -44,96 +45,144 @@ $ kubectl exec 123456-7890 -c ruby-container -i -t -- bash -il`
 )
 
 func NewCmdExec(f *cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer) *cobra.Command {
-	params := &execParams{}
+	options := &ExecOptions{
+		In:  cmdIn,
+		Out: cmdOut,
+		Err: cmdErr,
+
+		Executor: &DefaultRemoteExecutor{},
+	}
 	cmd := &cobra.Command{
 		Use:     "exec POD -c CONTAINER -- COMMAND [args...]",
 		Short:   "Execute a command in a container.",
 		Long:    "Execute a command in a container.",
 		Example: exec_example,
 		Run: func(cmd *cobra.Command, args []string) {
-			err := RunExec(f, cmd, cmdIn, cmdOut, cmdErr, params, args, &defaultRemoteExecutor{})
+			cmdutil.CheckErr(options.Complete(f, cmd, args))
-			cmdutil.CheckErr(err)
+			cmdutil.CheckErr(options.Validate())
+			cmdutil.CheckErr(options.Run())
 		},
 	}
-	cmd.Flags().StringVarP(&params.podName, "pod", "p", "", "Pod name")
+	cmd.Flags().StringVarP(&options.PodName, "pod", "p", "", "Pod name")
 	// TODO support UID
-	cmd.Flags().StringVarP(&params.containerName, "container", "c", "", "Container name")
+	cmd.Flags().StringVarP(&options.ContainerName, "container", "c", "", "Container name")
-	cmd.Flags().BoolVarP(&params.stdin, "stdin", "i", false, "Pass stdin to the container")
+	cmd.Flags().BoolVarP(&options.Stdin, "stdin", "i", false, "Pass stdin to the container")
-	cmd.Flags().BoolVarP(&params.tty, "tty", "t", false, "Stdin is a TTY")
+	cmd.Flags().BoolVarP(&options.TTY, "tty", "t", false, "Stdin is a TTY")
 	return cmd
 }
 
-type remoteExecutor interface {
+// RemoteExecutor defines the interface accepted by the Exec command - provided for test stubbing
+type RemoteExecutor interface {
 	Execute(req *client.Request, config *client.Config, command []string, stdin io.Reader, stdout, stderr io.Writer, tty bool) error
 }
 
-type defaultRemoteExecutor struct{}
+// DefaultRemoteExecutor is the standard implementation of remote command execution
+type DefaultRemoteExecutor struct{}
 
-func (*defaultRemoteExecutor) Execute(req *client.Request, config *client.Config, command []string, stdin io.Reader, stdout, stderr io.Writer, tty bool) error {
+func (*DefaultRemoteExecutor) Execute(req *client.Request, config *client.Config, command []string, stdin io.Reader, stdout, stderr io.Writer, tty bool) error {
 	executor := remotecommand.New(req, config, command, stdin, stdout, stderr, tty)
 	return executor.Execute()
 }
 
-type execParams struct {
+// ExecOptions declare the arguments accepted by the Exec command
-	podName       string
+type ExecOptions struct {
-	containerName string
+	Namespace     string
-	stdin         bool
+	PodName       string
-	tty           bool
+	ContainerName string
+	Stdin         bool
+	TTY           bool
+	Command       []string
+
+	In  io.Reader
+	Out io.Writer
+	Err io.Writer
+
+	Executor RemoteExecutor
+	Client   *client.Client
+	Config   *client.Config
 }
 
-func extractPodAndContainer(cmd *cobra.Command, argsIn []string, p *execParams) (podName string, containerName string, args []string, err error) {
+// Complete verifies command line arguments and loads data from the command environment
-	if len(p.podName) == 0 && len(argsIn) == 0 {
+func (p *ExecOptions) Complete(f *cmdutil.Factory, cmd *cobra.Command, argsIn []string) error {
-		return "", "", nil, cmdutil.UsageError(cmd, "POD is required for exec")
+	if len(p.PodName) == 0 && len(argsIn) == 0 {
+		return cmdutil.UsageError(cmd, "POD is required for exec")
 	}
-	if len(p.podName) != 0 {
+	if len(p.PodName) != 0 {
 		printDeprecationWarning("exec POD", "-p POD")
-		podName = p.podName
 		if len(argsIn) < 1 {
-			return "", "", nil, cmdutil.UsageError(cmd, "COMMAND is required for exec")
+			return cmdutil.UsageError(cmd, "COMMAND is required for exec")
 		}
-		args = argsIn
+		p.Command = argsIn
 	} else {
-		podName = argsIn[0]
+		p.PodName = argsIn[0]
-		args = argsIn[1:]
+		p.Command = argsIn[1:]
-		if len(args) < 1 {
+		if len(p.Command) < 1 {
-			return "", "", nil, cmdutil.UsageError(cmd, "COMMAND is required for exec")
+			return cmdutil.UsageError(cmd, "COMMAND is required for exec")
 		}
 	}
-	return podName, p.containerName, args, nil
-}
 
-func RunExec(f *cmdutil.Factory, cmd *cobra.Command, cmdIn io.Reader, cmdOut, cmdErr io.Writer, p *execParams, argsIn []string, re remoteExecutor) error {
-	podName, containerName, args, err := extractPodAndContainer(cmd, argsIn, p)
 	namespace, _, err := f.DefaultNamespace()
 	if err != nil {
 		return err
 	}
+	p.Namespace = namespace
+
+	config, err := f.ClientConfig()
+	if err != nil {
+		return err
+	}
+	p.Config = config
 
 	client, err := f.Client()
 	if err != nil {
 		return err
 	}
+	p.Client = client
 
-	pod, err := client.Pods(namespace).Get(podName)
+	return nil
+}
+
+// Validate checks that the provided exec options are specified.
+func (p *ExecOptions) Validate() error {
+	if len(p.PodName) == 0 {
+		return fmt.Errorf("pod name must be specified")
+	}
+	if len(p.Command) == 0 {
+		return fmt.Errorf("you must specify at least one command for the container")
+	}
+	if p.Out == nil || p.Err == nil {
+		return fmt.Errorf("both output and error output must be provided")
+	}
+	if p.Executor == nil || p.Client == nil || p.Config == nil {
+		return fmt.Errorf("client, client config, and executor must be provided")
+	}
+	return nil
+}
+
+// Run executes a validated remote execution against a pod.
+func (p *ExecOptions) Run() error {
+	pod, err := p.Client.Pods(p.Namespace).Get(p.PodName)
 	if err != nil {
 		return err
 	}
 
 	if pod.Status.Phase != api.PodRunning {
-		glog.Fatalf("Unable to execute command because pod %s is not running. Current status=%v", podName, pod.Status.Phase)
+		return fmt.Errorf("pod %s is not running and cannot execute commands; current phase is %s", p.PodName, pod.Status.Phase)
 	}
 
+	containerName := p.ContainerName
 	if len(containerName) == 0 {
 		glog.V(4).Infof("defaulting container name to %s", pod.Spec.Containers[0].Name)
 		containerName = pod.Spec.Containers[0].Name
 	}
 
+	// TODO: refactor with terminal helpers from the edit utility once that is merged
 	var stdin io.Reader
-	tty := p.tty
+	tty := p.TTY
-	if p.stdin {
+	if p.Stdin {
-		stdin = cmdIn
+		stdin = p.In
 		if tty {
-			if file, ok := cmdIn.(*os.File); ok {
+			if file, ok := stdin.(*os.File); ok {
 				inFd := file.Fd()
 				if term.IsTerminal(inFd) {
 					oldState, err := term.SetRawTerminal(inFd)
@@ -155,26 +204,22 @@ func RunExec(f *cmdutil.Factory, cmd *cobra.Command, cmdIn io.Reader, cmdOut, cm
 						os.Exit(0)
 					}()
 				} else {
-					glog.Warning("Stdin is not a terminal")
+					fmt.Fprintln(p.Err, "STDIN is not a terminal")
 				}
 			} else {
 				tty = false
-				glog.Warning("Unable to use a TTY")
+				fmt.Fprintln(p.Err, "Unable to use a TTY - input is not the right kind of file")
 			}
 		}
 	}
 
-	config, err := f.ClientConfig()
+	// TODO: consider abstracting into a client invocation or client helper
-	if err != nil {
+	req := p.Client.RESTClient.Post().
-		return err
-	}
-
-	req := client.RESTClient.Post().
 		Resource("pods").
 		Name(pod.Name).
-		Namespace(namespace).
+		Namespace(pod.Namespace).
 		SubResource("exec").
 		Param("container", containerName)
 
-	return re.Execute(req, config, args, stdin, cmdOut, cmdErr, tty)
+	return p.Executor.Execute(req, p.Config, p.Command, stdin, p.Out, p.Err, tty)
 }

pkg/kubectl/cmd/exec_test.go

@@ -44,7 +44,7 @@ func (f *fakeRemoteExecutor) Execute(req *client.Request, config *client.Config,
 func TestPodAndContainer(t *testing.T) {
 	tests := []struct {
 		args              []string
-		p                 *execParams
+		p                 *ExecOptions
 		name              string
 		expectError       bool
 		expectedPod       string
@@ -52,42 +52,42 @@ func TestPodAndContainer(t *testing.T) {
 		expectedArgs      []string
 	}{
 		{
-			p:           &execParams{},
+			p:           &ExecOptions{},
 			expectError: true,
 			name:        "empty",
 		},
 		{
-			p:           &execParams{podName: "foo"},
+			p:           &ExecOptions{PodName: "foo"},
 			expectError: true,
 			name:        "no cmd",
 		},
 		{
-			p:           &execParams{podName: "foo", containerName: "bar"},
+			p:           &ExecOptions{PodName: "foo", ContainerName: "bar"},
 			expectError: true,
 			name:        "no cmd, w/ container",
 		},
 		{
-			p:            &execParams{podName: "foo"},
+			p:            &ExecOptions{PodName: "foo"},
 			args:         []string{"cmd"},
 			expectedPod:  "foo",
 			expectedArgs: []string{"cmd"},
 			name:         "pod in flags",
 		},
 		{
-			p:           &execParams{},
+			p:           &ExecOptions{},
 			args:        []string{"foo"},
 			expectError: true,
 			name:        "no cmd, w/o flags",
 		},
 		{
-			p:            &execParams{},
+			p:            &ExecOptions{},
 			args:         []string{"foo", "cmd"},
 			expectedPod:  "foo",
 			expectedArgs: []string{"cmd"},
 			name:         "cmd, w/o flags",
 		},
 		{
-			p:                 &execParams{containerName: "bar"},
+			p:                 &ExecOptions{ContainerName: "bar"},
 			args:              []string{"foo", "cmd"},
 			expectedPod:       "foo",
 			expectedContainer: "bar",
@@ -96,22 +96,34 @@ func TestPodAndContainer(t *testing.T) {
 		},
 	}
 	for _, test := range tests {
+		f, tf, codec := NewAPIFactory()
+		tf.Client = &client.FakeRESTClient{
+			Codec:  codec,
+			Client: client.HTTPClientFunc(func(req *http.Request) (*http.Response, error) { return nil, nil }),
+		}
+		tf.Namespace = "test"
+		tf.ClientConfig = &client.Config{}
+
 		cmd := &cobra.Command{}
-		podName, containerName, args, err := extractPodAndContainer(cmd, test.args, test.p)
+		options := test.p
-		if podName != test.expectedPod {
+		err := options.Complete(f, cmd, test.args)
-			t.Errorf("expected: %s, got: %s (%s)", test.expectedPod, podName, test.name)
-		}
-		if containerName != test.expectedContainer {
-			t.Errorf("expected: %s, got: %s (%s)", test.expectedContainer, containerName, test.name)
-		}
 		if test.expectError && err == nil {
 			t.Errorf("unexpected non-error (%s)", test.name)
 		}
 		if !test.expectError && err != nil {
 			t.Errorf("unexpected error: %v (%s)", err, test.name)
 		}
-		if !reflect.DeepEqual(test.expectedArgs, args) {
+		if err != nil {
-			t.Errorf("expected: %v, got %v (%s)", test.expectedArgs, args, test.name)
+			continue
+		}
+		if options.PodName != test.expectedPod {
+			t.Errorf("expected: %s, got: %s (%s)", test.expectedPod, options.PodName, test.name)
+		}
+		if options.ContainerName != test.expectedContainer {
+			t.Errorf("expected: %s, got: %s (%s)", test.expectedContainer, options.ContainerName, test.name)
+		}
+		if !reflect.DeepEqual(test.expectedArgs, options.Command) {
+			t.Errorf("expected: %v, got %v (%s)", test.expectedArgs, options.Command, test.name)
 		}
 	}
 }
@@ -150,8 +162,8 @@ func TestExec(t *testing.T) {
 					return &http.Response{StatusCode: 200, Body: body}, nil
 				default:
 					// Ensures no GET is performed when deleting by name
-					t.Errorf("%s: unexpected request: %#v\n%#v", test.name, req.URL, req)
+					t.Errorf("%s: unexpected request: %s %#v\n%#v", test.name, req.Method, req.URL, req)
-					return nil, nil
+					return nil, fmt.Errorf("unexpected request")
 				}
 			}),
 		}
@@ -164,20 +176,30 @@ func TestExec(t *testing.T) {
 		if test.execErr {
 			ex.execErr = fmt.Errorf("exec error")
 		}
-		params := &execParams{
+		params := &ExecOptions{
-			podName:       "foo",
+			PodName:       "foo",
-			containerName: "bar",
+			ContainerName: "bar",
+			In:            bufIn,
+			Out:           bufOut,
+			Err:           bufErr,
+			Executor:      ex,
 		}
 		cmd := &cobra.Command{}
-		err := RunExec(f, cmd, bufIn, bufOut, bufErr, params, []string{"test", "command"}, ex)
+		if err := params.Complete(f, cmd, []string{"test", "command"}); err != nil {
+			t.Fatal(err)
+		}
+		err := params.Run()
 		if test.execErr && err != ex.execErr {
 			t.Errorf("%s: Unexpected exec error: %v", test.name, err)
-		}
+			continue
-		if !test.execErr && ex.req.URL().Path != test.execPath {
-			t.Errorf("%s: Did not get expected path for exec request", test.name)
 		}
 		if !test.execErr && err != nil {
 			t.Errorf("%s: Unexpected error: %v", test.name, err)
+			continue
+		}
+		if !test.execErr && ex.req.URL().Path != test.execPath {
+			t.Errorf("%s: Did not get expected path for exec request", test.name)
+			continue
 		}
 	}
 }