Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/http: k8s spdy TestUpgradeResponse broken in Go 1.11 #26161

Closed
rsc opened this issue Jun 30, 2018 · 5 comments
Closed

net/http: k8s spdy TestUpgradeResponse broken in Go 1.11 #26161

rsc opened this issue Jun 30, 2018 · 5 comments
Labels
FrozenDueToAge release-blocker
Milestone
Go1.11

Comments

@rsc
Copy link
Contributor

rsc commented Jun 30, 2018

Trying "make test" in Kubernetes with Go 1.11, and ran into a problem in one test.
(All the other unit tests pass, so that's something!)

Note that the test doesn't use hardly any Kubernetes stuff (this is a very low-level package in the Kubernetes graph), so it can be run standalone on Mac etc., using plain go tools:

Using Go 1.10 everything is OK:

$ go get k8s.io/apimachinery/pkg/util/httpstream/spdy
$ go1.10 test k8s.io/apimachinery/pkg/util/httpstream/spdy -v
=== RUN   TestConnectionCloseIsImmediateThroughAProxy
--- PASS: TestConnectionCloseIsImmediateThroughAProxy (0.00s)
=== RUN   TestRoundTripAndNewConnection
=== RUN   TestRoundTripAndNewConnection/redirect_=_false
2018/06/30 11:40:26 http: TLS handshake error from 127.0.0.1:54578: remote error: tls: bad certificate
2018/06/30 11:40:26 http: TLS handshake error from 127.0.0.1:54601: remote error: tls: bad certificate
2018/06/30 11:40:26 http: TLS handshake error from 127.0.0.1:54614: remote error: tls: bad certificate
2018/06/30 11:40:26 [001] WARN: Error copying to client: read tcp 127.0.0.1:54605->127.0.0.1:54602: read: connection reset by peer
2018/06/30 11:40:26 [001] WARN: Error copying to client: read tcp 127.0.0.1:54592->127.0.0.1:54589: read: connection reset by peer
=== RUN   TestRoundTripAndNewConnection/redirect_=_true
2018/06/30 11:40:26 [001] WARN: Error copying to client: read tcp 127.0.0.1:54568->127.0.0.1:54565: read: connection reset by peer
2018/06/30 11:40:26 [001] WARN: Error copying to client: read tcp 127.0.0.1:54564->127.0.0.1:54561: read: connection reset by peer
2018/06/30 11:40:26 http: TLS handshake error from 127.0.0.1:54622: remote error: tls: bad certificate
2018/06/30 11:40:26 http: TLS handshake error from 127.0.0.1:54625: remote error: tls: bad certificate
2018/06/30 11:40:26 http: TLS handshake error from 127.0.0.1:54652: remote error: tls: bad certificate
2018/06/30 11:40:26 [001] WARN: Error copying to client: write tcp 127.0.0.1:54664->127.0.0.1:54665: write: broken pipe
2018/06/30 11:40:26 [001] WARN: Error copying to client: read tcp 127.0.0.1:54662->127.0.0.1:54659: read: connection reset by peer
2018/06/30 11:40:26 [001] WARN: Error copying to client: readfrom tcp 127.0.0.1:54639->127.0.0.1:54640: read tcp 127.0.0.1:54641->127.0.0.1:54638: read: connection reset by peer
--- PASS: TestRoundTripAndNewConnection (0.20s)
    --- PASS: TestRoundTripAndNewConnection/redirect_=_false (0.17s)
    --- PASS: TestRoundTripAndNewConnection/redirect_=_true (0.04s)
=== RUN   TestRoundTripRedirects
2018/06/30 11:40:26 [001] WARN: Error copying to client: write tcp 127.0.0.1:54627->127.0.0.1:54628: write: broken pipe
=== RUN   TestRoundTripRedirects/with_0_redirects
=== RUN   TestRoundTripRedirects/with_1_redirects
=== RUN   TestRoundTripRedirects/with_10_redirects
=== RUN   TestRoundTripRedirects/with_11_redirects
--- PASS: TestRoundTripRedirects (0.01s)
    --- PASS: TestRoundTripRedirects/with_0_redirects (0.00s)
    --- PASS: TestRoundTripRedirects/with_1_redirects (0.00s)
    --- PASS: TestRoundTripRedirects/with_10_redirects (0.00s)
    --- PASS: TestRoundTripRedirects/with_11_redirects (0.00s)
=== RUN   TestUpgradeResponse
--- PASS: TestUpgradeResponse (0.00s)
PASS
ok  	k8s.io/apimachinery/pkg/util/httpstream/spdy	0.271s
$

Using recent master (257d6c4, Thu Jun 28):

$ go test k8s.io/apimachinery/pkg/util/httpstream/spdy -v
=== RUN   TestConnectionCloseIsImmediateThroughAProxy
--- PASS: TestConnectionCloseIsImmediateThroughAProxy (0.00s)
=== RUN   TestRoundTripAndNewConnection
=== RUN   TestRoundTripAndNewConnection/redirect_=_false
2018/06/30 11:42:21 http: TLS handshake error from 127.0.0.1:54784: remote error: tls: bad certificate
2018/06/30 11:42:21 http: TLS handshake error from 127.0.0.1:54792: remote error: tls: bad certificate
2018/06/30 11:42:21 http: TLS handshake error from 127.0.0.1:54801: remote error: tls: bad certificate
2018/06/30 11:42:21 [001] WARN: Error copying to client: write tcp 127.0.0.1:54813->127.0.0.1:54814: write: broken pipe
2018/06/30 11:42:21 [001] WARN: Error copying to client: read tcp 127.0.0.1:54777->127.0.0.1:54774: read: connection reset by peer
=== RUN   TestRoundTripAndNewConnection/redirect_=_true
2018/06/30 11:42:21 http: TLS handshake error from 127.0.0.1:54828: remote error: tls: bad certificate
2018/06/30 11:42:21 http: TLS handshake error from 127.0.0.1:54831: remote error: tls: bad certificate
2018/06/30 11:42:21 http: TLS handshake error from 127.0.0.1:54857: remote error: tls: bad certificate
2018/06/30 11:42:21 [001] WARN: Error copying to client: write tcp 127.0.0.1:54859->127.0.0.1:54860: write: broken pipe
--- PASS: TestRoundTripAndNewConnection (0.24s)
    --- PASS: TestRoundTripAndNewConnection/redirect_=_false (0.20s)
    --- PASS: TestRoundTripAndNewConnection/redirect_=_true (0.04s)
=== RUN   TestRoundTripRedirects
=== RUN   TestRoundTripRedirects/with_0_redirects
=== RUN   TestRoundTripRedirects/with_1_redirects
=== RUN   TestRoundTripRedirects/with_10_redirects
=== RUN   TestRoundTripRedirects/with_11_redirects
--- PASS: TestRoundTripRedirects (0.03s)
    --- PASS: TestRoundTripRedirects/with_0_redirects (0.00s)
    --- PASS: TestRoundTripRedirects/with_1_redirects (0.00s)
    --- PASS: TestRoundTripRedirects/with_10_redirects (0.00s)
    --- PASS: TestRoundTripRedirects/with_11_redirects (0.02s)
=== RUN   TestUpgradeResponse
--- FAIL: TestUpgradeResponse (0.00s)
    upgrade_test.go:82: 3: unexpected non-nil err from client.Do: Get http://127.0.0.1:54909: net/http: HTTP/1.x transport connection broken: malformed HTTP response "\x80\x03\x00\a\x00\x00\x00\b\x00\x00\x00\x00\x00\x00\x00\x00"
FAIL
FAIL	k8s.io/apimachinery/pkg/util/httpstream/spdy	0.326s
$

That test is sending an HTTP request header that includes:

Connection: Upgrade
Upgrade: SPDY/3.1

The server is sending back a header with WriteHeader and then hijacks the connection.

The test expects the client to get back a 101 (switching protocols) response.
Instead it gets this error.

Any ideas?
@rsc rsc added this to the Go1.11 milestone Jun 30, 2018
@meirf
Copy link
Contributor

meirf commented Jun 30, 2018 •
edited

Introduced by d88b137 "net/http, net/http/httptrace: make Transport support 1xx responses properly". (There have been a couple 1xx issues created recently; I haven't checked yet if they are related to this.)

@meirf
Copy link
Contributor

meirf commented Jun 30, 2018 •
edited

transport.go used to ReadResponse after receiving 100. Now it does so for any 1xx.

The server in the test returns only:

HTTP/1.1 101 Switching Protocols
Connection: Upgrade
Upgrade: SPDY/3.1

In particular, it does not satisfy the GET request.

The spec says:

immediately after sending the 101 (Switching Protocols) response, the server is expected to continue responding to the original request as if it had received its equivalent within the new protocol
...
For example, if the Upgrade header field is received in a GET request and the server decides to switch protocols, it first responds with a 101 (Switching Protocols) message in HTTP/1.1 and then immediately follows that with the new protocol's equivalent of a response to a GET on the target resource.

So the test server is not following the spec and d88b137 is doing the right thing according to the spec, but causes breakage to things that rely on the old incorrect behavior.

(If we don’t revert, some coordination between the libraries may be required. I'm going to look at the k8s test a bit more to see if I can get it to work with and without d88b137.)

@bradfitz
Copy link
Contributor

bradfitz commented Jul 1, 2018

I think we can just treat 101 as a terminal status and fix this regression issue while also still fixing the original issue.

@gopherbot
Copy link

Change https://golang.org/cl/121860 mentions this issue: net/http: make Transport treat 101 as a terminal status

@bradfitz
Copy link
Contributor

bradfitz commented Jul 3, 2018

I verified it passes again.

Except now it fails with a vet error, but it passes with go test -vet=off.

@golang golang locked and limited conversation to collaborators Jul 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
4 participants
@bradfitz @rsc @meirf @gopherbot