HTTP proxy improvments (http_proxy et al)

[trentm]

Some comments on https://github.com/restify/node-restify/blob/master/lib/clients/http_client.js#L366-L378

Some issues here:

• parsing the envvar blows up if just given, e.g. "1.2.3.4:5678" without
  the leading 'http://'. This works with curl. It should with restify
  clients as well.
• if options.proxy is given, it should override the env
• options.proxy shouldn't have to be parsed already. This should allow
  a URL string (arguably it should prefer that form). Parsing the URL
  is implementation guts.
• the isProxyForURL|NO_PROXY handling should be overrideable via
  options as well. I'm undecided, but leaning toward thinking that
  NO_PROXY should be ignore if options.proxy is given. I.e. if
  the programmer passed in options.proxy, they are intentionally
  overriding the environment. If they want the env, they could easily
  also pull and pass in options.no_proxy.

Thoughts? /cc yunong and others.

If it seems reasonable to others I could work on a patch.

[trentm]

Some trace logging from proxy and no_proxy would help, as well, IMO.

[yunong]

Options should override env vars. @trentm Please feel free to work on a patch, that would be awesome. Btw, we've split out the client into its own repo now, https://github.com/restify/clients so feel free to send a PR there.

[trentm]

• Also there is the subtlety that if you pass in agent:false (a common usage pattern I've seen to avoid keep-alive), proxy settings are ignored.

[trentm]

TIL about https://github.com/restify/clients That's great.

[yunong]

It's also an NPM module.

On 13 August 2015 at 15:25, Trent Mick notifications@github.com wrote:

TIL about https://github.com/restify/clients That's great.

—
Reply to this email directly or view it on GitHub
#878 (comment)
.

[trentm]

TritonDataCenter@afdfb77 is a second commit related to this. This is currently on the v2.8 branch of the fork of node-restify. Eventually I need to (a) get these on a "v2.8" branch of restify/node-restify and (b) port these to restify/clients.

[trentm]

Some notes on proxy-related envvars in some tools:

• curl and wget strictly separate http_proxy for HTTP and https_proxy
  for HTTPS.
• curl and wget require that http_proxy is lowercase (i.e. HTTP_PROXY is
  ignored).
• curl allows HTTPS_PROXY uppercase or lowercase (and documents it uppercase,
  https://curl.haxx.se/docs/manpage.html). wget only supports lowercase
  https_proxy.
• curl allows NO_PROXY (documented uppercase) and no_proxy.
• wget documents no_proxy
  (https://www.gnu.org/software/wget/manual/wget.html#Proxies), but my build
  from brew on MacOS appears to not support no_proxy. For example, the
  following did use the proxy:
  no_proxy=* https_proxy=http://127.0.0.1:8213 wget https://www.google.com
• curl supports additional ALL_PROXY and [protocol]_PROXY envvars.
• The "request" module allows http_proxy as a fallback for HTTPS. It
  does not support ALL_PROXY.

Suggestions for restify:

• The impl. should change to prefer opts.proxy if given, over the *_proxy
  envvars.

• To keep backward compat in restify 4.x and restify-clients 1.x, the updated
  implementation should not change envvar interpretation (i.e. https_proxy
  envvar wins for HTTP and HTTPS. http_proxy then considered for HTTP and
  HTTPS). The implementation should add support for the uppercase versions
  of those envvars, even though that differs from curl: it is more typical of other environment variable usage; as well FWIW golang seems to support both upper and lower https://golang.org/pkg/net/http/#ProxyFromEnvironment

• For restify 5.x and restify-clients 2.x we can consider changing to one of the
  following:

  1. Don't change. https_proxy envvar wins for HTTP and HTTPS. http_proxy
     then considered for HTTP and HTTPS.
  2. Change to be close to curl behaviour: HTTPS_PROXY and https_proxy
     work for HTTPS. HTTP_PROXY and http_proxy work for HTTP. (The
     diff from curl is that curl doesn't support uppercase HTTP_PROXY.)
  3. Change to be close to request (and go-lang) behaviour: HTTP_PROXY and http_proxy
     work for HTTP. HTTPS_PROXY, https_proxy, HTTP_PROXY, and
     http_proxy (in that order) work for HTTPS.

  My preference is Add an arbitrary User-Defined object to newError #3. I used to prefer Separate URI params from query/body params #2, but seeing that the request
  module and go-lang support style Add an arbitrary User-Defined object to newError #3, I'm more willing to just consider curl
  and wget behaviour a weird edge case.

• Don't bother with ALL_PROXY, that seems to be a curl-only-ism.

• The impl. should add support for opts.noProxy that, if specified, overrides
  any NO_PROXY in the environment.

[trentm]

@yunong @DonutEspresso Finally have a restify-clients 1.x PR for this here: restify/clients#85
It would be great if you had a change to take a look. I'd like to do a restify-clients@1.4.0 with this.

[trentm]

restify-clients@1.4.0 published with this.

@yunong @DonutEspresso What's a policy, if any, for getting clients changes into node-restify/lib/clients? Are we just EOL'ing the clients there?

[DonutEspresso]

Yep, barring critical bugs or security issues, we haven't been back porting any changes into the node-restify repo.

[trentm]

Cool, thanks. I think this can be closed then. Restify 5.x doesn't have the clients... and restify-clients doesn't have a 2.x stream yet.