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: DefaultTransport documentation should describe HTTPS_PROXY #32649

Closed
HouzuoGuo opened this issue Jun 17, 2019 · 6 comments
Closed

net/http: DefaultTransport documentation should describe HTTPS_PROXY #32649

HouzuoGuo opened this issue Jun 17, 2019 · 6 comments
Labels
Documentation FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@HouzuoGuo
Copy link

What version of Go are you using (go version)?

go version go1.12.4 linux/amd64

Does this issue reproduce with the latest release?

Affirmative.

What operating system and processor architecture are you using (go env)?

go env Output
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/howard/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/howard/gopath"
GOPROXY=""
GORACE=""
GOROOT="/home/howard/go"
GOTMPDIR=""
GOTOOLDIR="/home/howard/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build043222332=/tmp/go-build -gno-record-gcc-switches"

What did you do?

In https://golang.org/pkg/net/http/#RoundTripper, the online manual explains the following for RoundTripper:

DefaultTransport is the default implementation of Transport and is used by DefaultClient. It establishes network connections as needed and caches them for reuse by subsequent calls. It uses HTTP proxies as directed by the $HTTP_PROXY and $NO_PROXY (or $http_proxy and $no_proxy) environment variables.

Although DefaultTransport uses proxy settings from system environment via ProxyFromEnvironment, which not even recognises HTTP proxy, but HTTPS proxy also:

ProxyFromEnvironment returns the URL of the proxy to use for a given request, as indicated by the environment variables HTTP_PROXY, HTTPS_PROXY and NO_PROXY (or the lowercase versions thereof). HTTPS_PROXY takes precedence over HTTP_PROXY for https requests.

DefaultTransport indeed behaves correctly and recognises both HTTPS and HTTP proxy settings from system environment. Therefore, please elaborate about the support of HTTPS proxy in the documentation for DefaultTransport.

Thanks!
@katiehockman katiehockman changed the title http: minor issue with documentation of DefaultTransport for HTTP client net/http: minor issue with documentation of DefaultTransport for HTTP client Jun 17, 2019
@katiehockman katiehockman changed the title net/http: minor issue with documentation of DefaultTransport for HTTP client net/http: DefaultTransport documentation should describe HTTPS_PROXY Jun 17, 2019
@katiehockman
Copy link
Contributor

Please let me know if I am understanding this issue. To summarize:
The documentation for DefaultTransport says that it supports HTTP_PROXY and NO_PROXY, but does not mention that it supports HTTPS_PROXY, and your suggestion is that it should describe this additional environment variable. Correct?

@katiehockman
Copy link
Contributor

/cc @rsc (since Brad is on leave)

@katiehockman katiehockman added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 17, 2019
@HouzuoGuo
Copy link
Author

Thanks @katiehockman and yes indeed.

@seankhliao seankhliao added this to the Unplanned milestone Aug 27, 2022
narqo added a commit to narqo/go that referenced this issue Sep 10, 2022
@narqo
net/http: make DefaultTransport docs about HTTP proxy consistent
2fc7519 
The changes improve the documentation for DefaultTransport, making
the style with how the HTTP proxy environment variables are being
referred to, consistent with the rest of the project's
documentation.

Also mention HTTPS_PROXY environment variables, as suggested in golang#32649.
@narqo narqo mentioned this issue Sep 10, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/430135 mentions this issue: net/http: make DefaultTransport docs about HTTP proxy consistent

gopherbot pushed a commit that referenced this issue Sep 13, 2022
@narqo @gopherbot
net/http: make DefaultTransport docs about HTTP proxy consistent
39b31a3 
The changes improve the documentation for DefaultTransport, making
the style with how the HTTP proxy environment variables are being
referred to, consistent with the rest of the project's
documentation.

Also mention HTTPS_PROXY environment variables, as suggested in #32649.

Change-Id: I4e6b49881d7b30b5a0d4699531fa7c2929fc49f7
GitHub-Last-Rev: 2fc7519
GitHub-Pull-Request: #54996
Reviewed-on: https://go-review.googlesource.com/c/go/+/430135
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Jenny Rakoczy <jenny@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Jenny Rakoczy <jenny@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@narqo
Copy link
Contributor

narqo commented Sep 14, 2022

@HouzuoGuo with the changes from https://go.dev/cl/430135 is there anything to improve for the issue?

See the tip's documentation for the reference https://pkg.go.dev/net/http@master#DefaultTransport

@HouzuoGuo
Copy link
Author

That's amazing - thank you!

@golang golang locked and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@narqo @HouzuoGuo @gopherbot @seankhliao @katiehockman