From 0202e269a3deb7ff96d3b9e3fce767c4b37a766e Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 13 Nov 2018 10:57:33 -0500 Subject: [PATCH] Fix sending oversided data frames to spdy stream --- pkg/client/tests/remotecommand_test.go | 21 +++++++++- .../client-go/tools/remotecommand/BUILD | 1 + .../client-go/tools/remotecommand/reader.go | 41 +++++++++++++++++++ .../client-go/tools/remotecommand/v1.go | 2 +- .../client-go/tools/remotecommand/v2.go | 2 +- 5 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 staging/src/k8s.io/client-go/tools/remotecommand/reader.go diff --git a/pkg/client/tests/remotecommand_test.go b/pkg/client/tests/remotecommand_test.go index 143ee2e7d5..010cdff92d 100644 --- a/pkg/client/tests/remotecommand_test.go +++ b/pkg/client/tests/remotecommand_test.go @@ -108,7 +108,7 @@ func (ex *fakeExecutor) run(name string, uid types.UID, container string, cmd [] return nil } -func fakeServer(t *testing.T, testName string, exec bool, stdinData, stdoutData, stderrData, errorData string, tty bool, messageCount int, serverProtocols []string) http.HandlerFunc { +func fakeServer(t *testing.T, requestReceived chan struct{}, testName string, exec bool, stdinData, stdoutData, stderrData, errorData string, tty bool, messageCount int, serverProtocols []string) http.HandlerFunc { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { executor := &fakeExecutor{ t: t, @@ -134,6 +134,7 @@ func fakeServer(t *testing.T, testName string, exec bool, stdinData, stdoutData, if e, a := strings.Repeat(stdinData, messageCount), executor.stdinReceived.String(); e != a { t.Errorf("%s: stdin: expected %q, got %q", testName, e, a) } + close(requestReceived) }) } @@ -165,6 +166,15 @@ func TestStream(t *testing.T) { ClientProtocols: []string{remotecommandconsts.StreamProtocolV2Name}, ServerProtocols: []string{remotecommandconsts.StreamProtocolV2Name}, }, + { + TestName: "oversized stdin", + Stdin: strings.Repeat("a", 20*1024*1024), + Stdout: "b", + Stderr: "", + MessageCount: 1, + ClientProtocols: []string{remotecommandconsts.StreamProtocolV2Name}, + ServerProtocols: []string{remotecommandconsts.StreamProtocolV2Name}, + }, { TestName: "in/out/tty", Stdin: "a", @@ -218,7 +228,8 @@ func TestStream(t *testing.T) { localOut := &bytes.Buffer{} localErr := &bytes.Buffer{} - server := httptest.NewServer(fakeServer(t, name, exec, testCase.Stdin, testCase.Stdout, testCase.Stderr, testCase.Error, testCase.Tty, testCase.MessageCount, testCase.ServerProtocols)) + requestReceived := make(chan struct{}) + server := httptest.NewServer(fakeServer(t, requestReceived, name, exec, testCase.Stdin, testCase.Stdout, testCase.Stderr, testCase.Error, testCase.Tty, testCase.MessageCount, testCase.ServerProtocols)) url, _ := url.ParseRequestURI(server.URL) config := restclient.ContentConfig{ @@ -305,6 +316,12 @@ func TestStream(t *testing.T) { } } + select { + case <-requestReceived: + case <-time.After(time.Minute): + t.Errorf("%s: expected fakeServerInstance to receive request", name) + } + server.Close() } } diff --git a/staging/src/k8s.io/client-go/tools/remotecommand/BUILD b/staging/src/k8s.io/client-go/tools/remotecommand/BUILD index efeffeab72..043217a3ce 100644 --- a/staging/src/k8s.io/client-go/tools/remotecommand/BUILD +++ b/staging/src/k8s.io/client-go/tools/remotecommand/BUILD @@ -25,6 +25,7 @@ go_library( srcs = [ "doc.go", "errorstream.go", + "reader.go", "remotecommand.go", "resize.go", "v1.go", diff --git a/staging/src/k8s.io/client-go/tools/remotecommand/reader.go b/staging/src/k8s.io/client-go/tools/remotecommand/reader.go new file mode 100644 index 0000000000..d1f1be34c9 --- /dev/null +++ b/staging/src/k8s.io/client-go/tools/remotecommand/reader.go @@ -0,0 +1,41 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package remotecommand + +import ( + "io" +) + +// readerWrapper delegates to an io.Reader so that only the io.Reader interface is implemented, +// to keep io.Copy from doing things we don't want when copying from the reader to the data stream. +// +// If the Stdin io.Reader provided to remotecommand implements a WriteTo function (like bytes.Buffer does[1]), +// io.Copy calls that method[2] to attempt to write the entire buffer to the stream in one call. +// That results in an oversized call to spdystream.Stream#Write [3], +// which results in a single oversized data frame[4] that is too large. +// +// [1] https://golang.org/pkg/bytes/#Buffer.WriteTo +// [2] https://golang.org/pkg/io/#Copy +// [3] https://github.com/kubernetes/kubernetes/blob/90295640ef87db9daa0144c5617afe889e7992b2/vendor/github.com/docker/spdystream/stream.go#L66-L73 +// [4] https://github.com/kubernetes/kubernetes/blob/90295640ef87db9daa0144c5617afe889e7992b2/vendor/github.com/docker/spdystream/spdy/write.go#L302-L304 +type readerWrapper struct { + reader io.Reader +} + +func (r readerWrapper) Read(p []byte) (int, error) { + return r.reader.Read(p) +} diff --git a/staging/src/k8s.io/client-go/tools/remotecommand/v1.go b/staging/src/k8s.io/client-go/tools/remotecommand/v1.go index 81cc93ec05..4120f1f5f3 100644 --- a/staging/src/k8s.io/client-go/tools/remotecommand/v1.go +++ b/staging/src/k8s.io/client-go/tools/remotecommand/v1.go @@ -127,7 +127,7 @@ func (p *streamProtocolV1) stream(conn streamCreator) error { // because stdin is not closed until the process exits. If we try to call // stdin.Close(), it returns no error but doesn't unblock the copy. It will // exit when the process exits, instead. - go cp(v1.StreamTypeStdin, p.remoteStdin, p.Stdin) + go cp(v1.StreamTypeStdin, p.remoteStdin, readerWrapper{p.Stdin}) } waitCount := 0 diff --git a/staging/src/k8s.io/client-go/tools/remotecommand/v2.go b/staging/src/k8s.io/client-go/tools/remotecommand/v2.go index b74ae8de22..4b0001502a 100644 --- a/staging/src/k8s.io/client-go/tools/remotecommand/v2.go +++ b/staging/src/k8s.io/client-go/tools/remotecommand/v2.go @@ -101,7 +101,7 @@ func (p *streamProtocolV2) copyStdin() { // the executed command will remain running. defer once.Do(func() { p.remoteStdin.Close() }) - if _, err := io.Copy(p.remoteStdin, p.Stdin); err != nil { + if _, err := io.Copy(p.remoteStdin, readerWrapper{p.Stdin}); err != nil { runtime.HandleError(err) } }()