Merge pull request #43922 from cezarsa/spdy-fix

Automatic merge from submit-queue

prevent corrupted spdy stream after hijacking connection

This PR fixes corner case in spdy stream code where some bytes would never arrive at the server.

Reading directly from a hijacked connection isn't safe because some data may have already been read by the server before `Hijack` was called. To ensure all data will be received it's safer to read from the returned `bufio.Reader`. This problem seem to happen more frequently when using Go 1.8.
This is described in https://golang.org/pkg/net/http/#Hijacker:

> // The returned bufio.Reader may contain unprocessed buffered
   // data from the client.

I came across this while debugging a flaky test that used code from the `k8s.io/apimachinery/pkg/util/httpstream/spdy` package. After filling the code with debug logs and long hours running the tests in loop in the hope of catching the error I finally caught something weird.

The first word on the first spdy frame [read by the server here](b625085230/vendor/github.com/docker/spdystream/spdy/read.go (L148)) had the value `0x03000100`. See, the first frame to arrive on the server was supposed to be a control frame indicating the creation of a new stream, but all control frames need the high-order bit set to 1, which was not the case here, so the saver mistakenly assumed this was a data frame and the stream would never be created. The correct value for the first word of a SYN_STREAM frame was supposed to be `0x80030001` and this lead me on the path of finding who had consumed the first 1 byte prior to the frame reader being called and finally finding the problem with the Hijack call.

I added a new test to try stressing this condition and ensuring that this bug doesn't happen anymore. However, it's quite ugly as it loops 1000 times creating streams on servers to increase the chances of this bug happening. So, I'm not sure whether it's worth it to keep this test or if I should remove it from the PR. Please let me know what you guys think and I'll be happy to update this.

Fixes #45093 #45089 #45078 #45075 #45072 #45066 #45023

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/upgrade.go

@@ -17,9 +17,13 @@ limitations under the License.
 package spdy
 
 import (
+	"bufio"
 	"fmt"
+	"io"
+	"net"
 	"net/http"
 	"strings"
+	"sync/atomic"
 
 	"k8s.io/apimachinery/pkg/util/httpstream"
 	"k8s.io/apimachinery/pkg/util/runtime"
@@ -32,6 +36,30 @@ const HeaderSpdy31 = "SPDY/3.1"
 type responseUpgrader struct {
 }
 
+// connWrapper is used to wrap a hijacked connection and its bufio.Reader. All
+// calls will be handled directly by the underlying net.Conn with the exception
+// of Read and Close calls, which will consider data in the bufio.Reader. This
+// ensures that data already inside the used bufio.Reader instance is also
+// read.
+type connWrapper struct {
+	net.Conn
+	closed    int32
+	bufReader *bufio.Reader
+}
+
+func (w *connWrapper) Read(b []byte) (n int, err error) {
+	if atomic.LoadInt32(&w.closed) == 1 {
+		return 0, io.EOF
+	}
+	return w.bufReader.Read(b)
+}
+
+func (w *connWrapper) Close() error {
+	err := w.Conn.Close()
+	atomic.StoreInt32(&w.closed, 1)
+	return err
+}
+
 // NewResponseUpgrader returns a new httpstream.ResponseUpgrader that is
 // capable of upgrading HTTP responses using SPDY/3.1 via the
 // spdystream package.
@@ -62,13 +90,14 @@ func (u responseUpgrader) UpgradeResponse(w http.ResponseWriter, req *http.Reque
 	w.Header().Add(httpstream.HeaderUpgrade, HeaderSpdy31)
 	w.WriteHeader(http.StatusSwitchingProtocols)
 
-	conn, _, err := hijacker.Hijack()
+	conn, bufrw, err := hijacker.Hijack()
 	if err != nil {
 		runtime.HandleError(fmt.Errorf("unable to upgrade: error hijacking response: %v", err))
 		return nil
 	}
 
-	spdyConn, err := NewServerConnection(conn, newStreamHandler)
+	connWithBuf := &connWrapper{Conn: conn, bufReader: bufrw.Reader}
+	spdyConn, err := NewServerConnection(connWithBuf, newStreamHandler)
 	if err != nil {
 		runtime.HandleError(fmt.Errorf("unable to upgrade: error creating SPDY server connection: %v", err))
 		return nil