diff --git a/pkg/kubectl/cmd/exec.go b/pkg/kubectl/cmd/exec.go index 88a7f3e547..d66d0cbf61 100644 --- a/pkg/kubectl/cmd/exec.go +++ b/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(err) + cmdutil.CheckErr(options.Complete(f, cmd, args)) + cmdutil.CheckErr(options.Validate()) + cmdutil.CheckErr(options.Run()) }, } - cmd.Flags().StringVarP(¶ms.podName, "pod", "p", "", "Pod name") + cmd.Flags().StringVarP(&options.PodName, "pod", "p", "", "Pod name") // TODO support UID - cmd.Flags().StringVarP(¶ms.containerName, "container", "c", "", "Container name") - cmd.Flags().BoolVarP(¶ms.stdin, "stdin", "i", false, "Pass stdin to the container") - cmd.Flags().BoolVarP(¶ms.tty, "tty", "t", false, "Stdin is a TTY") + cmd.Flags().StringVarP(&options.ContainerName, "container", "c", "", "Container name") + cmd.Flags().BoolVarP(&options.Stdin, "stdin", "i", false, "Pass stdin to the container") + 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 { - podName string - containerName string - stdin bool - tty bool +// ExecOptions declare the arguments accepted by the Exec command +type ExecOptions struct { + Namespace string + PodName string + 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) { - if len(p.podName) == 0 && len(argsIn) == 0 { - return "", "", nil, cmdutil.UsageError(cmd, "POD is required for exec") +// Complete verifies command line arguments and loads data from the command environment +func (p *ExecOptions) Complete(f *cmdutil.Factory, cmd *cobra.Command, argsIn []string) error { + 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] - args = argsIn[1:] - if len(args) < 1 { - return "", "", nil, cmdutil.UsageError(cmd, "COMMAND is required for exec") + p.PodName = argsIn[0] + p.Command = argsIn[1:] + if len(p.Command) < 1 { + 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 - if p.stdin { - stdin = cmdIn + tty := p.TTY + if p.Stdin { + 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() - if err != nil { - return err - } - - req := client.RESTClient.Post(). + // TODO: consider abstracting into a client invocation or client helper + req := p.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) } diff --git a/pkg/kubectl/cmd/exec_test.go b/pkg/kubectl/cmd/exec_test.go index 64ef45b71a..67d6b41f9f 100644 --- a/pkg/kubectl/cmd/exec_test.go +++ b/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) - if podName != test.expectedPod { - 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) - } + options := test.p + err := options.Complete(f, cmd, test.args) 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) { - t.Errorf("expected: %v, got %v (%s)", test.expectedArgs, args, test.name) + if err != nil { + 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) - return nil, nil + t.Errorf("%s: unexpected request: %s %#v\n%#v", test.name, req.Method, req.URL, req) + 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{ - podName: "foo", - containerName: "bar", + params := &ExecOptions{ + PodName: "foo", + 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) - } - if !test.execErr && ex.req.URL().Path != test.execPath { - t.Errorf("%s: Did not get expected path for exec request", test.name) + continue } 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 } } }