diff --git a/pkg/cmd/attach/attach.go b/pkg/cmd/attach/attach.go index af25e0729..c1e8d83dc 100644 --- a/pkg/cmd/attach/attach.go +++ b/pkg/cmd/attach/attach.go @@ -158,23 +158,10 @@ type DefaultRemoteAttach struct{} // Attach executes attach to a running container func (*DefaultRemoteAttach) Attach(url *url.URL, config *restclient.Config, stdin io.Reader, stdout, stderr io.Writer, tty bool, terminalSizeQueue remotecommand.TerminalSizeQueue) error { - // Legacy SPDY executor is default. If feature gate enabled, fallback - // executor attempts websockets first--then SPDY. - exec, err := remotecommand.NewSPDYExecutor(config, "POST", url) + exec, err := createExecutor(url, config) if err != nil { return err } - if cmdutil.RemoteCommandWebsockets.IsEnabled() { - // WebSocketExecutor must be "GET" method as described in RFC 6455 Sec. 4.1 (page 17). - websocketExec, err := remotecommand.NewWebSocketExecutor(config, "GET", url.String()) - if err != nil { - return err - } - exec, err = remotecommand.NewFallbackExecutor(websocketExec, exec, httpstream.IsUpgradeFailure) - if err != nil { - return err - } - } return exec.StreamWithContext(context.Background(), remotecommand.StreamOptions{ Stdin: stdin, Stdout: stdout, @@ -184,6 +171,27 @@ func (*DefaultRemoteAttach) Attach(url *url.URL, config *restclient.Config, stdi }) } +// createExecutor returns the Executor or an error if one occurred. +func createExecutor(url *url.URL, config *restclient.Config) (remotecommand.Executor, error) { + exec, err := remotecommand.NewSPDYExecutor(config, "POST", url) + if err != nil { + return nil, err + } + // Fallback executor is default, unless feature flag is explicitly disabled. + if !cmdutil.RemoteCommandWebsockets.IsDisabled() { + // WebSocketExecutor must be "GET" method as described in RFC 6455 Sec. 4.1 (page 17). + websocketExec, err := remotecommand.NewWebSocketExecutor(config, "GET", url.String()) + if err != nil { + return nil, err + } + exec, err = remotecommand.NewFallbackExecutor(websocketExec, exec, httpstream.IsUpgradeFailure) + if err != nil { + return nil, err + } + } + return exec, nil +} + // Complete verifies command line arguments and loads data from the command environment func (o *AttachOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []string) error { var err error diff --git a/pkg/cmd/attach/attach_test.go b/pkg/cmd/attach/attach_test.go index 6d491323e..ac217e08c 100644 --- a/pkg/cmd/attach/attach_test.go +++ b/pkg/cmd/attach/attach_test.go @@ -37,6 +37,7 @@ import ( "k8s.io/client-go/tools/remotecommand" "k8s.io/kubectl/pkg/cmd/exec" cmdtesting "k8s.io/kubectl/pkg/cmd/testing" + cmdutil "k8s.io/kubectl/pkg/cmd/util" "k8s.io/kubectl/pkg/cmd/util/podcmd" "k8s.io/kubectl/pkg/polymorphichelpers" "k8s.io/kubectl/pkg/scheme" @@ -553,3 +554,37 @@ func TestReattachMessage(t *testing.T) { }) } } + +func TestCreateExecutor(t *testing.T) { + url, err := url.Parse("http://localhost:8080/index.html") + if err != nil { + t.Fatalf("unable to parse test url: %v", err) + } + config := cmdtesting.DefaultClientConfig() + // First, ensure that no environment variable creates the fallback executor. + executor, err := createExecutor(url, config) + if err != nil { + t.Fatalf("unable to create executor: %v", err) + } + if _, isFallback := executor.(*remotecommand.FallbackExecutor); !isFallback { + t.Errorf("expected fallback executor, got %#v", executor) + } + // Next, check turning on feature flag explicitly also creates fallback executor. + t.Setenv(string(cmdutil.RemoteCommandWebsockets), "true") + executor, err = createExecutor(url, config) + if err != nil { + t.Fatalf("unable to create executor: %v", err) + } + if _, isFallback := executor.(*remotecommand.FallbackExecutor); !isFallback { + t.Errorf("expected fallback executor, got %#v", executor) + } + // Finally, check explicit disabling does NOT create the fallback executor. + t.Setenv(string(cmdutil.RemoteCommandWebsockets), "false") + executor, err = createExecutor(url, config) + if err != nil { + t.Fatalf("unable to create executor: %v", err) + } + if _, isFallback := executor.(*remotecommand.FallbackExecutor); isFallback { + t.Errorf("expected fallback executor, got %#v", executor) + } +} diff --git a/pkg/cmd/exec/exec.go b/pkg/cmd/exec/exec.go index 36d43bece..405807664 100644 --- a/pkg/cmd/exec/exec.go +++ b/pkg/cmd/exec/exec.go @@ -121,23 +121,10 @@ type RemoteExecutor interface { type DefaultRemoteExecutor struct{} func (*DefaultRemoteExecutor) Execute(url *url.URL, config *restclient.Config, stdin io.Reader, stdout, stderr io.Writer, tty bool, terminalSizeQueue remotecommand.TerminalSizeQueue) error { - // Legacy SPDY executor is default. If feature gate enabled, fallback - // executor attempts websockets first--then SPDY. - exec, err := remotecommand.NewSPDYExecutor(config, "POST", url) + exec, err := createExecutor(url, config) if err != nil { return err } - if cmdutil.RemoteCommandWebsockets.IsEnabled() { - // WebSocketExecutor must be "GET" method as described in RFC 6455 Sec. 4.1 (page 17). - websocketExec, err := remotecommand.NewWebSocketExecutor(config, "GET", url.String()) - if err != nil { - return err - } - exec, err = remotecommand.NewFallbackExecutor(websocketExec, exec, httpstream.IsUpgradeFailure) - if err != nil { - return err - } - } return exec.StreamWithContext(context.Background(), remotecommand.StreamOptions{ Stdin: stdin, Stdout: stdout, @@ -147,6 +134,27 @@ func (*DefaultRemoteExecutor) Execute(url *url.URL, config *restclient.Config, s }) } +// createExecutor returns the Executor or an error if one occurred. +func createExecutor(url *url.URL, config *restclient.Config) (remotecommand.Executor, error) { + exec, err := remotecommand.NewSPDYExecutor(config, "POST", url) + if err != nil { + return nil, err + } + // Fallback executor is default, unless feature flag is explicitly disabled. + if !cmdutil.RemoteCommandWebsockets.IsDisabled() { + // WebSocketExecutor must be "GET" method as described in RFC 6455 Sec. 4.1 (page 17). + websocketExec, err := remotecommand.NewWebSocketExecutor(config, "GET", url.String()) + if err != nil { + return nil, err + } + exec, err = remotecommand.NewFallbackExecutor(websocketExec, exec, httpstream.IsUpgradeFailure) + if err != nil { + return nil, err + } + } + return exec, nil +} + type StreamOptions struct { Namespace string PodName string diff --git a/pkg/cmd/exec/exec_test.go b/pkg/cmd/exec/exec_test.go index 7305231f1..af8b9099d 100644 --- a/pkg/cmd/exec/exec_test.go +++ b/pkg/cmd/exec/exec_test.go @@ -33,8 +33,8 @@ import ( restclient "k8s.io/client-go/rest" "k8s.io/client-go/rest/fake" "k8s.io/client-go/tools/remotecommand" - cmdtesting "k8s.io/kubectl/pkg/cmd/testing" + cmdutil "k8s.io/kubectl/pkg/cmd/util" "k8s.io/kubectl/pkg/scheme" "k8s.io/kubectl/pkg/util/term" ) @@ -402,3 +402,37 @@ func TestSetupTTY(t *testing.T) { t.Errorf("attach stdin, TTY, is a terminal: tty.Out should equal o.Out") } } + +func TestCreateExecutor(t *testing.T) { + url, err := url.Parse("http://localhost:8080/index.html") + if err != nil { + t.Fatalf("unable to parse test url: %v", err) + } + config := cmdtesting.DefaultClientConfig() + // First, ensure that no environment variable creates the fallback executor. + executor, err := createExecutor(url, config) + if err != nil { + t.Fatalf("unable to create executor: %v", err) + } + if _, isFallback := executor.(*remotecommand.FallbackExecutor); !isFallback { + t.Errorf("expected fallback executor, got %#v", executor) + } + // Next, check turning on feature flag explicitly also creates fallback executor. + t.Setenv(string(cmdutil.RemoteCommandWebsockets), "true") + executor, err = createExecutor(url, config) + if err != nil { + t.Fatalf("unable to create executor: %v", err) + } + if _, isFallback := executor.(*remotecommand.FallbackExecutor); !isFallback { + t.Errorf("expected fallback executor, got %#v", executor) + } + // Finally, check explicit disabling does NOT create the fallback executor. + t.Setenv(string(cmdutil.RemoteCommandWebsockets), "false") + executor, err = createExecutor(url, config) + if err != nil { + t.Fatalf("unable to create executor: %v", err) + } + if _, isFallback := executor.(*remotecommand.FallbackExecutor); isFallback { + t.Errorf("expected fallback executor, got %#v", executor) + } +}