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

Support no_proxy environment variable. #140 #145

Merged
merged 2 commits into from Aug 28, 2015
Merged

Conversation

@hgiddens
Copy link
Contributor

hgiddens commented Aug 7, 2015

Sorry if I've done any dumb stuff here. I tried to find a spec, and the nearest I could find was some ancient info from W3C. I've thus implemented a subset of what other popular libraries support.

no_proxy contains a list of domain suffixes, separated by commas. The domain suffixes may optionally have leading periods; the commas may optionally have trailing spaces.

For example, if no_proxy=internal.company.com, .other.company.com, requests to internal.company.com, foo.internal.company.com, and other.company.com will avoid the proxy. Requests to notactuallyinternal.company.com will still go via the proxy.

Differences to other implementations:
Domain suffix vs. string suffix

wget and Python's urllib3 avoid the proxy if one of the no_proxy hosts is a string suffix of the request host, rather than a domain suffix (with special casing so no_proxy=.foo.example avoids the proxy for requests to foo.example).

Ignoring case/spaces

Some implementations ignore all spaces, rather than just 0x20s following a comma. Some implementations also use case-sensitive domain handling.

Port-specific rules

Ruby's stdlib implementation can be made to apply only to specific ports.

Avoiding the proxy for CADR ranges

Python's urllib3 allows you to specify e.g. no_proxy=internal.company.com,192.168.0.0/16.

Avoiding the proxy for all requests

Ruby and cURL avoid the proxy for all requests if no_proxy=* is set.

@snoyberg

This comment has been minimized.

Copy link
Owner

snoyberg commented Aug 7, 2015

Unless I'm misreading this, this code will require parsing the no_proxy environment variable from scratch on each request. It would be better to explicitly parse it immediately into some intermediate data structure, and then perform lookups on that.

@hgiddens

This comment has been minimized.

Copy link
Contributor Author

hgiddens commented Aug 11, 2015

Yeah, you're quite right. Would newManager be an appropriate place for the parsing step?

@snoyberg

This comment has been minimized.

Copy link
Owner

snoyberg commented Aug 11, 2015

It's fine to do it in this function. Just do it outside of the inner function (in the same part of the code where getEnvironment is called).

@hgiddens

This comment has been minimized.

Copy link
Contributor Author

hgiddens commented Aug 28, 2015

Is there anything else you'd like done on this one?

@snoyberg

This comment has been minimized.

Copy link
Owner

snoyberg commented Aug 28, 2015

I'll check now. For future reference: Github doesn't send notifications on new commits to a PR, so it's good to add a comment when making such a change.

snoyberg added a commit that referenced this pull request Aug 28, 2015
Support no_proxy environment variable. #140
@snoyberg snoyberg merged commit 815b960 into snoyberg:master Aug 28, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snoyberg

This comment has been minimized.

Copy link
Owner

snoyberg commented Aug 28, 2015

Thanks!

snoyberg added a commit that referenced this pull request Aug 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.