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

net/http: add Transport.GetProxyConnectHeader #41048

Closed
bradfitz opened this issue Aug 26, 2020 · 7 comments
Closed

net/http: add Transport.GetProxyConnectHeader #41048

bradfitz opened this issue Aug 26, 2020 · 7 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 26, 2020

net/http.Transport currently has:

https://golang.org/pkg/net/http/#Transport.ProxyConnectHeader

    // ProxyConnectHeader optionally specifies headers to send to
    // proxies during CONNECT requests.
    ProxyConnectHeader Header // Go 1.8

... but it's static for the life of the Transport.

Some authentication modes like NTLM/Kerberos/Negotiate require a dynamic header value, which currently requires making a new Transport for each request going through a proxy. That's heavy, as the Transport is the unit of connection reuse.

Proposal:

    // GetProxyConnectHeader is called to return headers to send to
    // proxuURL during a CONNECT request to the target ip:port. If it returns an error, the Transport
    // RoundTrip fails with that error. It can return (nil, nil) to not add headers.
    // If GetProxyConnectHeader is non-nil, ProxyConnectHeader is ignored.
    GetProxyConnectHeader func(ctx context.Context, proxyURL *url.URL, target string) (Header, error)

    // ProxyConnectHeader optionally specifies headers to send to
    // proxies during CONNECT requests. For dynamic use, see GetProxyConnectHeader.
    ProxyConnectHeader Header // Go 1.8
@gopherbot gopherbot added this to the Proposal milestone Aug 26, 2020
@gopherbot gopherbot added the Proposal label Aug 26, 2020
bradfitz added a commit to tailscale/go that referenced this issue Aug 27, 2020
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Aug 27, 2020

Potential implementation: tailscale@6db6825 (licensed+CLA'd for use by Go)

(Lacking tests for now.)

@rsc
Copy link
Contributor

@rsc rsc commented Sep 16, 2020

The Get is a bit non-standard but it does follow our pattern of having GetFoo func() T override Foo T in a struct when we realize too late that Foo T might not be initializable from the start. (For example, Request.GetBody vs Request.Body.)

Does anyone object to this?

@rsc
Copy link
Contributor

@rsc rsc commented Sep 23, 2020

Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Sep 23, 2020
@networkimprov
Copy link

@networkimprov networkimprov commented Sep 23, 2020

cc @fraenkel @odeke-em for any input...

@rsc
Copy link
Contributor

@rsc rsc commented Sep 30, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 30, 2020
@rsc rsc modified the milestones: Proposal, Backlog Sep 30, 2020
@rsc rsc changed the title proposal: net/http: add Transport.GetProxyConnectHeader net/http: add Transport.GetProxyConnectHeader Sep 30, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 6, 2020

Change https://golang.org/cl/259917 mentions this issue: net/http: add Transport.GetProxyConnectHeader

@gopherbot gopherbot closed this in 930fa89 Oct 6, 2020
@dmitshur dmitshur mentioned this issue Dec 28, 2020
469 of 481 tasks complete
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 22, 2021

Change https://golang.org/cl/285594 mentions this issue: doc/go1.16: mention net/http.Transport.GetProxyConnectHeader

gopherbot pushed a commit that referenced this issue Jan 22, 2021
For #40700
For #41048

Change-Id: Ida6bcaaf5edaa2bba9ba2b8e02ec9959481f8302
Reviewed-on: https://go-review.googlesource.com/c/go/+/285594
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants