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

x/net/http2: should interpret Connection: close request header as Request.Close = true #14227

Closed
admtnnr opened this issue Feb 4, 2016 · 6 comments
Closed

x/net/http2: should interpret Connection: close request header as Request.Close = true #14227

admtnnr opened this issue Feb 4, 2016 · 6 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@admtnnr
Copy link

admtnnr commented Feb 4, 2016

Related to #14214, it doesn't seem possible to explicitly set a protocol version when sending via an http.Transport and this breaks when sending requests with certain headers to servers supporting HTTP2. As an example, the following encounters the same 400 error from the GFE as we were previously seeing with the Proxy-Connection header:

package main

import (
    "net/http"
    "net/url"
    "os"
)

func main() {
    req := &http.Request{
        Method: "GET",
        URL: &url.URL{
            Scheme: "https",
            Host:   "www.google.co.uk",
            Path:   "/",
        },
        ProtoMajor: 1,
        ProtoMinor: 1,
        Proto:      "HTTP/1.1",
        Host:       "www.google.co.uk",
        Header: http.Header{
            "Connection": []string{"close"},
        },
    }
    req.Write(os.Stdout)

    // Uncomment to Nerf HTTP2 support and the request succeeds.
    // http.DefaultTransport.(*http.Transport).TLSNextProto = make(map[string]func(string, *tls.Conn) http.RoundTripper)

    res, err := http.DefaultTransport.RoundTrip(req)
    if err != nil {
        panic(err)
    }
    res.Write(os.Stdout)
}

// Example:
// GET / HTTP/1.1
// Host: www.google.co.uk
// User-Agent: Go-http-client/1.1
// Connection: close
//
// HTTP/2.0 400 Bad Request
// Content-Length: 54
// Content-Type: text/html; charset=UTF-8
// Date: Thu, 04 Feb 2016 14:37:38 GMT
// Server: GFE/2.0
//
// <html><title>Error 400 (Bad Request)!!1</title></html>

For non-HTTP2 requests the Connection header is perfectly valid, but when upgraded by http.Transport to HTTP2 they become invalid and break.
@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2016

You're not allowed to set the protocol version for outgoing requests. The docs say:

https://tip.golang.org/pkg/net/http/#Request

    // The protocol version for incoming server requests.
    //
    // For client requests these fields are ignored. The HTTP
    // client code always uses either HTTP/1.1 or HTTP/2.
    // See the docs on Transport for details.
    Proto      string // "HTTP/1.0"
    ProtoMajor int    // 1
    ProtoMinor int    // 0

As for your,

        Header: http.Header{
            "Connection": []string{"close"},
        },

That's not really a header that Go code is supposed to set manually. That's one of the ones managed for you. Note the Request.Close boolean, which is how you're supposed to control that.

Did this break real code or was this hypothetical, an experiment after seeing the previous commits and bugs about similar things?

We can update docs, but unless this broke real code, we'd rather not change any code behavior after we did the rc2 release.

@bradfitz bradfitz self-assigned this Feb 4, 2016
@bradfitz bradfitz added this to the Go1.7 milestone Feb 4, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2016

To be clear, there's stuff to change at some point. I'm just trying to determine when.

For example, the Transport code should remove that Connection: "close" header (so it's not sent) and treat it as if you set Request.Close.

@bradfitz bradfitz changed the title x/net/http2: automatic upgrade to h2 in Transport breaks valid HTTP/1.* requests x/net/http2: should interpret Connection: close request header as Request.Close = true Feb 4, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2016

Per offline discussion, I've retitled this bug.

@gopherbot
Copy link

CL https://golang.org/cl/19222 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/19223 mentions this issue.

gopherbot pushed a commit that referenced this issue Feb 5, 2016
@bradfitz
net/http: document Request.Header and Request.Close more
107a6ef 
Updates #14227

Change-Id: If39f11471ecd307c9483f64e73f9c89fe906ae71
Reviewed-on: https://go-review.googlesource.com/19222
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Feb 5, 2016
@bradfitz
http2: don't send Connection-level headers in Transport
493a262 
Accept common things that users might try to do to be helpful (managed
by net/http anyway, and previously legal or at best ignored), like

  Connection: close
  Connection: keep-alive
  Transfer-Encoding: chunked

But reject all other connection-level headers, per http2 spec. The
Google GFE enforces this, so we need to filter these before sending,
and give users a better error message for the ones we can't safely
filter. That is, reject any connection-level header that we don't know
the meaning of.

This CL also makes "Connection: close" mean the same as Request.Close,
and respects that as well, which was previously ignored in http2.

Mostly tests.

Updates golang/go#14227

Change-Id: I06e20286f71e8416149588e2c6274a3fce68033b
Reviewed-on: https://go-review.googlesource.com/19223
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

CL https://golang.org/cl/19250 mentions this issue.

@golang golang locked and limited conversation to collaborators Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
3 participants
@bradfitz @admtnnr @gopherbot