Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Support NoProxy setting as CIDR internally #7885
Conversation
wupeka
added some commits
Sep 12, 2017
|
Can one of the admins verify this patch? |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
jujubot
merged commit bea19b0
into
juju:develop
Sep 27, 2017
1 check passed
jameinel
reviewed
Oct 2, 2017
I'm concerned that by doing this Juju will operate differently than every other client that uses NOPROXY settings, and people will be confused when they think something works with Juju, but doesn't end up working for LXD/curl/wget/other applications that they deploy.
I suppose the other alternative is that getting it supported starts somewhere?
Note that setting NOPROXY in this way will also get it set in things like /etc/apt/* stuff where at best it will just get ignored.
| @@ -115,6 +114,9 @@ func (pc *ProxyConfig) useProxy(addr string) bool { | ||
| // no_proxy "foo.com" matches "bar.foo.com" | ||
| return false | ||
| } | ||
| + if _, net, err := net.ParseCIDR(p); ip != nil && err == nil && net.Contains(ip) { |
jameinel
Oct 2, 2017
Owner
shouldn't you check ip != nil before you bother to ParseCIDR ? Given that if the original ip is nil, then the net.Contains doesn't make any sense. Maybe something like:
if ip != nil {
// this is an IP, check to see if we were given a CIDR instead of just an exact IP match
}
|
From my perspective we should reference other software and the fact that we are not really in control here more clearly. https://curl.haxx.se/libcurl/c/CURLOPT_PROXY.html https://www.gnu.org/software/wget/manual/html_node/Proxies.html There is also a problem which has to be noted when a workaround such as the following is used: printf -v no_proxy '%s,' 10.0.{1..255}.{1..255}; echo $(( We simply hit a kernel limit on a command line argument size (or execve arg) with full /16 networks (or even /14) which is (PAGE_SIZE * 32) = 4096 * 32 = 131072 bytes http://elixir.free-electrons.com/linux/v4.13.4/source/include/uapi/linux/binfmts.h#L14 I think it might be useful for Juju to get an option which would say: "for any address of any juju-allocated machine use noproxy unless stated otherwise". From my perspective, this would certainly help as the main use-case is to prevent proxy usage with addresses known to Juju - doesn't have to be the whole /16 subnet or /8 subnet. It will largely help on the practical side given that no generic solution can be provided as we cannot rewrite everything to uniformly support noproxy. |
|
So the main problem for saying "addresses allocated by Juju" is that the
thing that needs noproxy is often very deep (what are the no proxy settings
for the application that the charm had deployed), and getting the settings
to the charm and triggering updates and restarts is painful. Adding a new
machine shouldn't cause every application that you have deployed to be
restarted in case it was depending on proxy settings.
It is reasonably easy for Juju itself to have a good feel for things, and
would be pretty easy to start tacking this sort of information onto spaces.
(should this space use the proxy for other entities inside the
space/outside the space or not).
And spaces are reasonably slow to change, so also more practical.
John
=:->
…
|
wupeka commentedSep 27, 2017
Description of change
Internal HTTP client now supports no_proxy setting as CIDR, eg 10.0.0.0/24
QA steps
juju bootstrap --config
'proxy="some_working_proxy_that_does_not_allow_connections_back_to_controller" no_proxy="cidr_in_which_controller_resides"'
Documentation changes
None