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.

Sign up for GitHub

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

[FIXED JENKINS-32326] Support no_proxy environment variable #84

Merged
merged 1 commit into from
Jun 9, 2016
Merged

[FIXED JENKINS-32326] Support no_proxy environment variable #84

merged 1 commit into from
Jun 9, 2016

Conversation

etiennebec
Copy link
Contributor

@etiennebec
[FIXED JENKINS-32326] Support no_proxy environment variable
72431ab 
Change-Id: I894e7831677ee39c37020b0fab0e6db5b290f9ca
oleg-nenashev
oleg-nenashev reviewed Jun 1, 2016
View reviewed changes
src/main/java/hudson/remoting/Util.java
@@ -108,6 +108,55 @@ static String indent(String s) {
}

/**
* Check if given URL is in the exclusion list defined by the no_proxy environment variable.
* On most *NIX system wildcards are not supported but if one top domain is added, all related subdomains will also
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not help much with redirects, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean HTTP 301/302 redirection? The goal of this function is only to determine whether the slave needs to use a proxy or not to reach its master. The actual selection of proxy address, redirection/connection to the proxy will be handled by other parts: Util.openURLConnection() relies on java.net.URL.openConnection() to do so, and the Engine does it manually: https://github.com/jenkinsci/remoting/blob/remoting-2.59/src/main/java/hudson/remoting/Engine.java#L411

I tried to mimic as much as possible the behaviour of well known tools as curl or wget which supports the no_proxy environment variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it should work

@oleg-nenashev
Copy link
Member

In Jenkins project we commonly use System properties. IMHO it would be useful to support both options. System property could have a higher priority in this case. On the other hand, such approach may lead to security concerns

@etiennebec
Copy link
Contributor Author

etiennebec commented Jun 1, 2016 •
edited
Loading

I would also prefer to mostly rely on system properties. But previous PR #27 introduced the use of http_proxy environment variable without supporting no_proxy exception list (equivalent of http.nonProxyHosts but less powerful).

So since #27 was merged, every remoting release includes a bug which prevent a slave to connect to its master when http_proxy was set. Because even if the system properties have precedence over this variable (see Util.openURLConnection() and Util.getResolvedHttpProxyAddress()), http_proxy was used as a fallback when they were not defined.

To sum up if you must use a proxy to access internet and has configured no_proxy so apt/curl/wget for instance could download files, your slave cannot currently contact its master which is on the same network.

Please have a look to the last comments of JENKINS-28289 and JENKINS-32326 for further details.

@oleg-nenashev
Copy link
Member

LGTM 👍
Needs some streamlining for System Properties, but it's a follow-up task IMHO

@oleg-nenashev oleg-nenashev merged commit a1bdc23 into jenkinsci:master Jun 9, 2016
oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Jun 10, 2016
@oleg-nenashev
Update remoting to 2.60
d9f12b0 
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) - 
Make the channel reader tolerant against Socket timeouts. 
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) - 
Support no_proxy environment variable. 
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  - 
Do not invoke PingFailureAnalyzer for agent=>master ping failures. 
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) - 
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection. 
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) - 
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class). 
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>. 
(jenkinsci/remoting#80)
@oleg-nenashev oleg-nenashev mentioned this pull request Jun 10, 2016
Merged
oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Jun 11, 2016
@oleg-nenashev
Update remoting to 2.60 (#2403)
c718516 
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) - 
Make the channel reader tolerant against Socket timeouts. 
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) - 
Support no_proxy environment variable. 
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  - 
Do not invoke PingFailureAnalyzer for agent=>master ping failures. 
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) - 
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection. 
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) - 
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class). 
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>. 
(jenkinsci/remoting#80)
olivergondza pushed a commit to jenkinsci/jenkins that referenced this pull request Jul 20, 2016
@oleg-nenashev @olivergondza
Update remoting to 2.60 (#2403)
3da25a0 
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) -
Make the channel reader tolerant against Socket timeouts.
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) -
Support no_proxy environment variable.
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  -
Do not invoke PingFailureAnalyzer for agent=>master ping failures.
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) -
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection.
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) -
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class).
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>.
(jenkinsci/remoting#80)
(cherry picked from commit c718516)
samatdav pushed a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016
@oleg-nenashev @samatdav
Update remoting to 2.60 (jenkinsci#2403)
de0ccc5 
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) - 
Make the channel reader tolerant against Socket timeouts. 
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) - 
Support no_proxy environment variable. 
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  - 
Do not invoke PingFailureAnalyzer for agent=>master ping failures. 
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) - 
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection. 
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) - 
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class). 
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>. 
(jenkinsci/remoting#80)
samatdav pushed a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016
@oleg-nenashev @samatdav
Update remoting to 2.60 (jenkinsci#2403)
46e5d60 
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) - 
Make the channel reader tolerant against Socket timeouts. 
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) - 
Support no_proxy environment variable. 
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  - 
Do not invoke PingFailureAnalyzer for agent=>master ping failures. 
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) - 
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection. 
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) - 
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class). 
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>. 
(jenkinsci/remoting#80)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@etiennebec @oleg-nenashev