Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-28289

JNLP slave should exclude non proxy hosts when connecting via HTTP proxy

      https://issues.jenkins-ci.org/browse/JENKINS-6167
      The fix submitted for this issue does not take into account the excluded proxy hosts unlike the hudson.remoting.Util.openURLConnection method which implicitly excludes hosts.

      A fix using the same method as sun.net.www.protocol.http.HttpURLConnection is available here :
      https://github.com/hypnoce/remoting/commit/67dbd966e628c42a0aa4d3cd91a56a00b7f95030

      Thanks

          [JENKINS-28289] JNLP slave should exclude non proxy hosts when connecting via HTTP proxy

          Francis JAC added a comment -

          Hi,

          any news on that point ? Is the fix provided suitable for a pull request ?

          Thanks

          François

          Francis JAC added a comment - Hi, any news on that point ? Is the fix provided suitable for a pull request ? Thanks François

          Daniel Beck added a comment -

          Could you submit your fix as a pull request to remoting?

          Daniel Beck added a comment - Could you submit your fix as a pull request to remoting?

          Francis JAC added a comment -

          Francis JAC added a comment - Hello, done : https://github.com/jenkinsci/remoting/pull/55 Thanks.

          Evan Wee added a comment -

          Would you mind resolving the conflict and resubmitting the pull request?

          Evan Wee added a comment - Would you mind resolving the conflict and resubmitting the pull request?

          Evan Wee added a comment -

          I currently do not have a workaround for this.

          Evan Wee added a comment - I currently do not have a workaround for this.

          Code changed in jenkins
          User: Oleg Nenashev
          Path:
          src/main/java/hudson/remoting/Engine.java
          src/main/java/hudson/remoting/Util.java
          http://jenkins-ci.org/commit/remoting/a4cc02d33f35bf7ba04b58a36fc3a683097b7a9e
          Log:
          Merge pull request #55 from hypnoce/master

          JENKINS-28289 Ensure proxy exclusion list is used while getting proxy for JNLP clients

          Compare: https://github.com/jenkinsci/remoting/compare/bfbcfb3282d9...a4cc02d33f35

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Oleg Nenashev Path: src/main/java/hudson/remoting/Engine.java src/main/java/hudson/remoting/Util.java http://jenkins-ci.org/commit/remoting/a4cc02d33f35bf7ba04b58a36fc3a683097b7a9e Log: Merge pull request #55 from hypnoce/master JENKINS-28289 Ensure proxy exclusion list is used while getting proxy for JNLP clients Compare: https://github.com/jenkinsci/remoting/compare/bfbcfb3282d9...a4cc02d33f35

          Geoffroy Jabouley added a comment - - edited

          Hello
          could this fix be included in the incoming LTS version?
          Regads

          Geoffroy Jabouley added a comment - - edited Hello could this fix be included in the incoming LTS version? Regads

          Daniel Beck added a comment -

          There has not been a release of the library yet, and that release first needs to be in mainline Jenkins to soak before it can be included in LTS.

          As the release candidate for 1.625.3 is next Wednesday, this change is ineligible for inclusion in that release unless someone expedites it by pointing to some urgency.

          Maybe towards the next LTS base line for release in January.

          Daniel Beck added a comment - There has not been a release of the library yet, and that release first needs to be in mainline Jenkins to soak before it can be included in LTS. As the release candidate for 1.625.3 is next Wednesday, this change is ineligible for inclusion in that release unless someone expedites it by pointing to some urgency. Maybe towards the next LTS base line for release in January.

          Daniel Beck added a comment -

          oleg_nenashev Does the merged PR resolve this issue?

          Daniel Beck added a comment - oleg_nenashev Does the merged PR resolve this issue?

          Oleg Nenashev added a comment -

          I think so

          Oleg Nenashev added a comment - I think so

          Daniel Beck added a comment -

          Assuming this is resolved and just an oversight in the commit message.

          Daniel Beck added a comment - Assuming this is resolved and just an oversight in the commit message.

          Oliver Gondža added a comment - - edited

          I am rejecting this is a candidate against 1.625.3 as #55 was not released (jenkinsci/remoting) and therefore not in Jenkins core. #62 was not merged yet.

          danielbeck, Is it really resolved if none of the changes are in core until today?

          Oliver Gondža added a comment - - edited I am rejecting this is a candidate against 1.625.3 as #55 was not released (jenkinsci/remoting) and therefore not in Jenkins core. #62 was not merged yet. danielbeck , Is it really resolved if none of the changes are in core until today?

          Daniel Beck added a comment -

          Good question. In general, resolved does not mean released as it's the commit that matters. Of course with library/framework changes, this means it may never make it into the software as the component version also needs to be increased.

          Daniel Beck added a comment - Good question. In general, resolved does not mean released as it's the commit that matters. Of course with library/framework changes, this means it may never make it into the software as the component version also needs to be increased.

          Evan Wee added a comment -

          Hi, please ignore #62 as #55 has been redone and merged.

          Would appreciate if jenkinsci/remoting could release a new version.

          Evan Wee added a comment - Hi, please ignore #62 as #55 has been redone and merged. Would appreciate if jenkinsci/remoting could release a new version.

          Evan Wee added a comment -

          danielbeck Any prospects for this fix to be released via a new jenkinsci/remoting module?

          Evan Wee added a comment - danielbeck Any prospects for this fix to be released via a new jenkinsci/remoting module?

          Daniel Beck added a comment -

          Sending a homing pidgeon to oleg_nenashev so he can answer this.

          Daniel Beck added a comment - Sending a homing pidgeon to oleg_nenashev so he can answer this.

          Release still pending.

          Oliver Gondža added a comment - Release still pending.

          Just for clarification:
          how do we set the proxy exclusion list with this patch? Is the environment variable "no_proxy" enought, or do we have to provide a "-Dhttp.nonProxyHosts" java system property when starting the jenkins slave?
          Looking forward a new release in a LTS version

          Geoffroy Jabouley added a comment - Just for clarification: how do we set the proxy exclusion list with this patch? Is the environment variable "no_proxy" enought, or do we have to provide a "-Dhttp.nonProxyHosts" java system property when starting the jenkins slave? Looking forward a new release in a LTS version

          geoffroyjabouley, I hope I have not confused you. This was rejected for LTS inclusion again as the component was not released and integrated in Jenkins mainline.

          Oliver Gondža added a comment - geoffroyjabouley , I hope I have not confused you. This was rejected for LTS inclusion again as the component was not released and integrated in Jenkins mainline.

          thanks for clarification. The sooner the better

          Geoffroy Jabouley added a comment - thanks for clarification. The sooner the better

          remoting 2.54 including the required fix was released 3 days ago but not yet merged into master
          PR to upgrade remoting in master: https://github.com/jenkinsci/jenkins/pull/2054

          Arnaud Héritier added a comment - remoting 2.54 including the required fix was released 3 days ago but not yet merged into master PR to upgrade remoting in master: https://github.com/jenkinsci/jenkins/pull/2054

          Another attempt to get this into core https://github.com/jenkinsci/jenkins/pull/2074

          Oliver Gondža added a comment - Another attempt to get this into core https://github.com/jenkinsci/jenkins/pull/2074

          Hello
          do you think this fix will make its way to a jenkins 1.X LTS release before jenkins 2.0?

          Geoffroy Jabouley added a comment - Hello do you think this fix will make its way to a jenkins 1.X LTS release before jenkins 2.0?

          Daniel Beck added a comment -

          geoffroyjabouley Not automatically, as it was only merged into 1.653, and the next LTS line is based on 1.651. But now at least there's a chance.

          Daniel Beck added a comment - geoffroyjabouley Not automatically, as it was only merged into 1.653, and the next LTS line is based on 1.651. But now at least there's a chance.

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java
          core/src/main/java/jenkins/slaves/JnlpAgentReceiver.java
          core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java
          core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol2.java
          core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol3.java
          core/src/main/java/jenkins/slaves/JnlpSlaveHandshake.java
          pom.xml
          http://jenkins-ci.org/commit/jenkins/af1a53d91c4863f27e4fad295911f131beb64b9a
          Log:
          Merge pull request #2010 from jenkinsci/jnlp3

          [FIXED JENKINS-26580][FIXED JENKINS-28289] Activate JNLP3 support
          (cherry picked from commit 6d3e05439643097d2f172761ea82a32a857d058a)

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java core/src/main/java/jenkins/slaves/JnlpAgentReceiver.java core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol2.java core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol3.java core/src/main/java/jenkins/slaves/JnlpSlaveHandshake.java pom.xml http://jenkins-ci.org/commit/jenkins/af1a53d91c4863f27e4fad295911f131beb64b9a Log: Merge pull request #2010 from jenkinsci/jnlp3 [FIXED JENKINS-26580] [FIXED JENKINS-28289] Activate JNLP3 support (cherry picked from commit 6d3e05439643097d2f172761ea82a32a857d058a)

          Fixing this on master caused JENKINS-33886, not backporting this into LTS.

          Oliver Gondža added a comment - Fixing this on master caused JENKINS-33886 , not backporting this into LTS.

          What was at the beginning a simple patch supposed to fix a regression introduced in march 2015 (it's been a year now!) still cannot be mainlined because someone has decided instead to include it in a much wider story/task of implementing JNLP3 support.

          Why not keeping the scope of this fix simple?

          Geoffroy Jabouley added a comment - What was at the beginning a simple patch supposed to fix a regression introduced in march 2015 (it's been a year now!) still cannot be mainlined because someone has decided instead to include it in a much wider story/task of implementing JNLP3 support. Why not keeping the scope of this fix simple?

          Etienne Bec added a comment - - edited

          Hi,

          We've upgraded to the latest LTS 1.651.2 and still experiencing issue with our proxy. The JNLP agent doesn't take in account the environment variable no_proxy nor the system proprerty http.nonProxyHosts. When http_proxy or http.proxyHost are defined it always use the proxy connection.

          To my understanding the bug was introduced as a side-effect by PR27 on remoting which proxy implementation was incomplete (it doesn't support exclusions). It actually added proxy to two methods: Launcher:parseJnlpArguments #1 and Engine:run #2.

          PR55 only fixed Engine:run, it didn't modify the Util:openURLConnection method which is now called in Launcher:parseJnlpArguments #3 (instead of URL:openConnection). As this method is immediately called after the agent is started when used with system property jnlpUrl, it also immediately failed when the connection timeouts, throws an Exception and exits at #4.

          Moreover PR55 fix only works if you use http.proxyHost property: Java reads for you http.nonProxyHosts and will update the list of proxies returned by ProxySelector.getDefault().select(<URI>). But if you use the environment variable, no_proxy is never used.

          1. https://github.com/shirosaki/remoting/blob/8c875cbc81d22df898a57ef86e833b41c50470cb/src/main/java/hudson/remoting/Launcher.java#L247
          2. https://github.com/shirosaki/remoting/blob/8c875cbc81d22df898a57ef86e833b41c50470cb/src/main/java/hudson/remoting/Engine.java#L169
          3. https://github.com/jenkinsci/remoting/pull/55/files?diff=unified
          4. https://github.com/jenkinsci/remoting/blob/master/src/main/java/hudson/remoting/Launcher.java#L275

          Etienne Bec added a comment - - edited Hi, We've upgraded to the latest LTS 1.651.2 and still experiencing issue with our proxy. The JNLP agent doesn't take in account the environment variable no_proxy nor the system proprerty http.nonProxyHosts . When http_proxy or http.proxyHost are defined it always use the proxy connection. To my understanding the bug was introduced as a side-effect by PR27 on remoting which proxy implementation was incomplete (it doesn't support exclusions). It actually added proxy to two methods: Launcher:parseJnlpArguments #1 and Engine:run #2 . PR55 only fixed Engine:run , it didn't modify the Util:openURLConnection method which is now called in Launcher:parseJnlpArguments #3 (instead of URL:openConnection ). As this method is immediately called after the agent is started when used with system property jnlpUrl , it also immediately failed when the connection timeouts, throws an Exception and exits at #4 . Moreover PR55 fix only works if you use http.proxyHost property: Java reads for you http.nonProxyHosts and will update the list of proxies returned by ProxySelector.getDefault().select(<URI>). But if you use the environment variable, no_proxy is never used. 1. https://github.com/shirosaki/remoting/blob/8c875cbc81d22df898a57ef86e833b41c50470cb/src/main/java/hudson/remoting/Launcher.java#L247 2. https://github.com/shirosaki/remoting/blob/8c875cbc81d22df898a57ef86e833b41c50470cb/src/main/java/hudson/remoting/Engine.java#L169 3. https://github.com/jenkinsci/remoting/pull/55/files?diff=unified 4. https://github.com/jenkinsci/remoting/blob/master/src/main/java/hudson/remoting/Launcher.java#L275

          Etienne Bec added a comment -

          We're still experiencing the issue. See my last comment.

          Etienne Bec added a comment - We're still experiencing the issue. See my last comment.

          Daniel Beck added a comment -

          etiennebec To clarify, this issue is fixed as described, but you're using yet another environment variable, and that one isn't used?

          Daniel Beck added a comment - etiennebec To clarify, this issue is fixed as described, but you're using yet another environment variable, and that one isn't used?

          Etienne Bec added a comment - - edited

          I will give you some background. Our network is behind a corporate proxy and thus we need to go through it to access to Internet. That's why we have defined the two standard environment variables, http_proxy and no_proxy, to configure several tools (curl for instance), to use the proxy. The slaves and the master are on the same network so they can communicate directly, that's why we didn't set the system properties http.proxyHost and so on.

          We were previously running Jenkins 1.596.3 which uses remoting 2.47, and this version doesn't include the PR27. And with this PR the trouble begins because the slave started to take in account only the first env var I mentioned: http_proxy. no_proxy is never read. And the PR55 which is related to this issue doesn't fix this either. It just introduced the use of ProxySelector which automatically take in account the exception list ... defined by the system property http.nonProxyHosts. And as in the PR27 it reads the http_proxy env var as a fallback, but doesn't use no_proxy at all!

          To be more clear, we don't use the proxy features on our side. It's just that the slave started with the PR27 to read some env var it should not (because that's not how the JVM should be configured). So either we need to take in account no_proxy or we remove from the two methods the part which use http_proxy: https://github.com/jenkinsci/remoting/blob/master/src/main/java/hudson/remoting/Util.java#L114 & https://github.com/jenkinsci/remoting/blob/master/src/main/java/hudson/remoting/Util.java#L114.

          I've reopened this issue because to my understanding the goal of this issue wasn't just to add the support for http.nonProxyHosts but to correct PR27. Should we create a new issue?

          Etienne Bec added a comment - - edited I will give you some background. Our network is behind a corporate proxy and thus we need to go through it to access to Internet. That's why we have defined the two standard environment variables, http_proxy and no_proxy , to configure several tools (curl for instance), to use the proxy. The slaves and the master are on the same network so they can communicate directly, that's why we didn't set the system properties http.proxyHost and so on. We were previously running Jenkins 1.596.3 which uses remoting 2.47, and this version doesn't include the PR27. And with this PR the trouble begins because the slave started to take in account only the first env var I mentioned: http_proxy . no_proxy is never read. And the PR55 which is related to this issue doesn't fix this either. It just introduced the use of ProxySelector which automatically take in account the exception list ... defined by the system property http.nonProxyHosts . And as in the PR27 it reads the http_proxy env var as a fallback, but doesn't use no_proxy at all! To be more clear, we don't use the proxy features on our side. It's just that the slave started with the PR27 to read some env var it should not (because that's not how the JVM should be configured). So either we need to take in account no_proxy or we remove from the two methods the part which use http_proxy : https://github.com/jenkinsci/remoting/blob/master/src/main/java/hudson/remoting/Util.java#L114 & https://github.com/jenkinsci/remoting/blob/master/src/main/java/hudson/remoting/Util.java#L114 . I've reopened this issue because to my understanding the goal of this issue wasn't just to add the support for http.nonProxyHosts but to correct PR27. Should we create a new issue?

          Daniel Beck added a comment -

          Probably best as a separate issue. This one has too much history IMO.

          Daniel Beck added a comment - Probably best as a separate issue. This one has too much history IMO.

          Etienne Bec added a comment - - edited

          Looks like it has already been reported: https://issues.jenkins-ci.org/browse/JENKINS-32326

          I will mark this issue as resolved.

          Etienne Bec added a comment - - edited Looks like it has already been reported: https://issues.jenkins-ci.org/browse/JENKINS-32326 I will mark this issue as resolved.

            Unassigned Unassigned
            hypnoce Francis JAC
            Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

              Created:
              Updated:
              Resolved: