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: provide ProxyFromEnvironment functionality without requiring environment #22079

Closed
rogpeppe opened this issue Sep 28, 2017 · 11 comments
Closed
Labels
Milestone

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Sep 28, 2017

It is not possible to use ProxyFromEnvironment if the proxy settings may ever change, because it uses once-only-cached values from the environment variables.

For example, when running a server that reads its proxy settings from a config that may be updated at runtime, it would be useful to be able to create a new Transport.Proxy value that uses the new settings.

It is non-trivial, so just reimplementing it outside the standard library is undesirable.

I propose an addition to net/http:

// ProxyConfig holds configuration for HTTP proxy settings.
type ProxyConfig struct {
	RequestMethod string
	HTTPProxy     string
	HTTPSProxy    string
	NoProxy       string
}

// ProxyForRequest determines the URL to use for the given request.
// This method may be used as a value for Transport.Proxy.
func (cfg *ProxyConfig) ProxyForRequest(req *http.Request) (*url.URL, error)

// ProxyConfigFromEnvironment returns a ProxyConfig
// instance populated from environment variables.
// ProxyFromEnvironment is equivalent to ProxyConfigFromEnvironment().Proxy
// except that it only reads the environment variables once,
// the first time it is called.
func ProxyConfigFromEnvironment() *ProxyConfig {
	return &ProxyConfig{
		RequestMethod: os.Getenv("REQUEST_METHOD"),
		HTTPProxy: os.Getenv("HTTP_PROXY"),
		HTTPSProxy: os.Getenv("HTTPS_PROXY"),
		NoProxy: os.Getenv("NO_PROXY"),
	}
}
@rogpeppe rogpeppe added the Proposal label Sep 28, 2017
@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

@rogpeppe rogpeppe commented Oct 3, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 3, 2017

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Oct 4, 2017

This sounds entirely reasonable to me. For reference, "NoProxy" is the non-trivial part of the implementation. @rogpeppe, want to send a CL?

For completeness, another option is to add http.ReloadProxyConfigFromEnvironment() to reload the environment variables. This is not a good idea because it will be hard to write that function in such a way that it won't race with concurrent calls to RoundTrip (which may be calling ProxyFromEnvironment).

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

@rogpeppe rogpeppe commented Oct 4, 2017

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Oct 4, 2017

@rogpeppe, can you flesh out how you're going to use this feature? Specifically, why does ProxyConfigFromEnvironment() return the current values of the environment variables, rather than read the environment once and cache the result?

I did not read your proposed definition of ProxyConfigFromEnvironment carefully enough the first time I read your proposal. I'm not sure how you will update ProxyConfig safely when there are concurrent RoundTrip calls.

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

@rogpeppe rogpeppe commented Oct 4, 2017

To guard against races, it's not hard to do something like:

transport.Proxy = func(req *http.Request) (*url.URL, error) {
	myType.mu.Lock()
	defer myType.mu.Unlock()
	return myType.proxyConfig.ProxyForRequest(req)
}

An alternative might be to put a mutex into ProxyConfig, unexport the fields, and provide a way of updating it. That provides a more complex API though, and I'm not sure it's worth it - there are plenty of exported fields in net/http that are vulnerable to concurrency issues if modified without care.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Oct 4, 2017

I agree adding a mutex to ProxyConfig is not worth it. The part that confused me is this:

// ProxyFromEnvironment is equivalent to ProxyConfigFromEnvironment().Proxy
// except that it only reads the environment variables once,
// the first time it is called.

Why does ProxyFromEnvironment need to re-read the environment variables each time it is called? It seems simpler to assume that the environment variables are fixed at startup. If the config needs to change, the caller can change the struct using a lock as in your above example.

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

@rogpeppe rogpeppe commented Oct 4, 2017

Why does ProxyFromEnvironment need to re-read the environment variables each time it is called? It seems simpler to assume that the environment variables are fixed at startup. If the config needs to change, the caller can change the struct using a lock as in your above example.

It doesn't. I think my explanation was confusing. I've updated the explanation and added a note about the need for a mutex. PTAL.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 8, 2017

Change https://golang.org/cl/68091 mentions this issue: net/http: add ProxyConfig and NewProxyConfigFromEnvironment

@ianlancetaylor ianlancetaylor added NeedsFix and removed Proposal labels Mar 29, 2018
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 29, 2018
@andrewrynhard

This comment has been minimized.

Copy link

@andrewrynhard andrewrynhard commented Dec 20, 2019

I see this is unplanned. We are hitting this problem, what is the recommended workaround, and will this PR be closed eventually? It would be nice to have this in the standard library.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 21, 2019

@andrewrynhard, actually according to the comments above it looks like this happened:

https://godoc.org/golang.org/x/net/http/httpproxy

So closing this as done. Let me know if I missed something.

@bradfitz bradfitz closed this Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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