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

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

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

Comments

@admtnnr
Copy link

@admtnnr 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

This comment has been minimized.

Copy link
Contributor

@bradfitz 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

This comment has been minimized.

Copy link
Contributor

@bradfitz 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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 4, 2016

Per offline discussion, I've retitled this bug.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 4, 2016

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 4, 2016

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

gopherbot pushed a commit that referenced this issue Feb 5, 2016
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
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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 5, 2016

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

@gopherbot gopherbot closed this in dc89b5b Feb 5, 2016
@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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.