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

          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: