[FIXED JENKINS-48561] Give precedence to proxy exclusion list system property over env var #243

etiennebec · 2017-12-14T14:58:13Z

https://issues.jenkins-ci.org/browse/JENKINS-48561

…property over env var

oleg-nenashev

Looks good so far. Could you please fix the locale thing?

oleg-nenashev · 2017-12-14T20:35:08Z

src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java

+                    StringTokenizer stringTokenizer = new StringTokenizer(nonProxyHosts, "|", false);
+                    try {
+                        while(stringTokenizer.hasMoreTokens()) {
+                            exclusionsPool.add(stringTokenizer.nextToken().toLowerCase(), Boolean.TRUE);


toLowerCase() is invoked without locale, it will cause FindBugs failures with #242 I'd guess

oleg-nenashev · 2017-12-14T20:35:55Z

src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java

+                            exclusionsPool.add(stringTokenizer.nextToken().toLowerCase(), Boolean.TRUE);
+                        }
+                    } catch(sun.misc.REException e) {
+                        System.err.println("Malformed exception list in http.nonProxyHosts system property: " + e.getMessage());


I will need to rework everything to loggers

oleg-nenashev · 2017-12-14T20:36:02Z

src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java

+                    } catch(sun.misc.REException e) {
+                        System.err.println("Malformed exception list in http.nonProxyHosts system property: " + e.getMessage());
+                    }
+                    if(exclusionsPool.match(host.toLowerCase()) != null) {


…esolver

etiennebec · 2017-12-15T09:14:30Z

Thanks for reviewing it. I pushed another two commits to take in account your comments.

On a side note, I noticed that some portions of code have been partially copy/pasted from hudson.remoting.Util to org.jenkinsci.remoting.engine.JnlpAgentEndpointResolver, mainly inNoProxyEnvVar() and openURLConnection(): 2cea38c

hudson.remoting.Launcher is still using the Util.openURLConnection, but JnlpAgentEndpointResolver.resolve uses the other one.

Maybe it is a way to keep compatibility with older releases while transitioning from hudson namespace to org.jenkinsci? IDK.

etiennebec · 2017-12-15T14:36:41Z

Noticed in mvn logs a warning regarding usage of Sun proprietary API (sun.misc.RegexpPool and sun.net.NetProperties). Should I try to replace them?

oleg-nenashev · 2017-12-15T14:56:18Z

On a side note, I noticed that some portions of code have been partially copy/pasted.... Maybe it is a way to keep compatibility with older releases while transitioning from hudson namespace to org.jenkinsci? IDK.

In this case it would be possible to have an utility class somewhere and to invoke its methods from other places. No need to invoke special magic, just keeping and probably deprecating the current methods, but moving the duplicated logic to another method

Noticed in mvn logs a warning regarding usage of Sun proprietary API (sun.misc.RegexpPool and sun.net.NetProperties). Should I try to replace them?

Would be great. I was also wondering about these packages, but I didn't check whether they are deprecated

oleg-nenashev · 2017-12-28T12:38:19Z

@etiennebec Are you still planning to change the classes?

etiennebec · 2017-12-29T10:41:05Z

@oleg-nenashev yes, probably by the end of next week.

…entry

oleg-nenashev · 2018-01-02T11:13:13Z

Jenkins project does not support Java 9 so far. Is it possible to have a Java8-compatible implementation without deprecated classes?

etiennebec · 2018-01-02T11:30:29Z

OpenJDK9 removed references to old internal Sun lib, including the ones used in DefaultProxySelector:

So I re-used the same logic which results in a highly similar code. I have two concerns:

diff-b72206907388927fa66809bdb849f86eR391 is almost a copy paste of 50a749f2cade#l1.91 but I'm not able to find another, simpler, implementation. Is this OK?
In the past, I've seen people using multiple wildcards in -DnonProxyHosts entries (e.g. 127.*.*.*). It was working since the entries were interpreted as regex, even though it was not explicitly documented: https://docs.oracle.com/javase/8/docs/api/java/net/doc-files/net-properties.html. This no longer work with the new implementation in Java 9 which only supports a wildcard at the beginning or the end of the hostname. I added a warning message which should be pretty obvious in case anyone get bite by this new behavior.

I will probably refactor and simplify a bit other proxy related methods, including the duplicate of inNoProxyEnvVar, but this will be the subject of another PR.

etiennebec · 2018-01-02T11:33:17Z

Didn't expect you to review my new commits so quickly. 👍

Jenkins project does not support Java 9 so far. Is it possible to have a Java8-compatible implementation without deprecated classes?

The code is actually Java8-compatible (I don't even have the JDK9 installed on my computer).

When I say in the comment that the implementation supports only Java9, I'm referring to the previous DefaultProxySelector implementation which was more permissive than what was described in the official documentation, and allowed the use of regex in nonProxyHosts entries.

As mentioned in my previous message Java9 restored the expected and correct behavior.

oleg-nenashev · 2018-01-30T17:35:53Z

Sorry, slipped down in my priority list

oleg-nenashev · 2018-01-30T17:39:25Z

I will try to play with that in order to deliver it in Remoting 3.18 (which will be likely an LTS candidate). Sorry for the review delay, JEP-200 consumed much my capacity

etiennebec · 2018-01-30T19:25:47Z

Thanks for all your effort on JEP-200. 👍

Don't worry, we're compiling our own version of jenkins/remoting/plugins in my company, so it's not an issue if it takes some time for this PR to be mainlined. Just trying to give back to the community!

oleg-nenashev

LGTM

oleg-nenashev · 2018-03-03T23:51:53Z

CC @rysteboe . I think we can land it

ghost

Argh, sorry, this slipped off my radar. Agreed, LGTM.

[FIXED JENKINS-48561] Give precedence to proxy exclusion list system …

eb2b2d9

…property over env var

oleg-nenashev self-requested a review December 14, 2017 20:33

oleg-nenashev reviewed Dec 14, 2017

View reviewed changes

etiennebec added 2 commits December 15, 2017 09:55

Use English locale when converting host to lower case

9d929e0

Replace System.err.println with LOGGER.log call in JnlpAgentEndpointR…

559f628

…esolver

oleg-nenashev added needs-review work-in-progress labels Dec 28, 2017

etiennebec added 3 commits January 2, 2018 11:41

Remove import of Sun internal lib and implement Java9 non proxy handling

24a0077

Add FIXME keyword on copy/pasted methods from hudson.remoting.Util

736678d

Add a warning when more than one wildcard is used in a nonProxyHosts …

8410222

…entry

oleg-nenashev removed the work-in-progress label Jan 30, 2018

oleg-nenashev requested review from oleg-nenashev and a user January 30, 2018 17:36

oleg-nenashev approved these changes Mar 3, 2018

View reviewed changes

oleg-nenashev added ready-to-merge and removed needs-review labels Mar 3, 2018

ghost approved these changes Mar 5, 2018

View reviewed changes

ghost merged commit dfb65c8 into jenkinsci:master Mar 5, 2018

jglick mentioned this pull request Oct 18, 2023

Send Node-Name HTTP header in JnlpAgentEndpointResolver #685

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED JENKINS-48561] Give precedence to proxy exclusion list system property over env var #243

[FIXED JENKINS-48561] Give precedence to proxy exclusion list system property over env var #243

etiennebec commented Dec 14, 2017

oleg-nenashev left a comment

oleg-nenashev Dec 14, 2017

oleg-nenashev Dec 14, 2017

oleg-nenashev Dec 14, 2017

etiennebec commented Dec 15, 2017 •

edited

etiennebec commented Dec 15, 2017

oleg-nenashev commented Dec 15, 2017 •

edited

oleg-nenashev commented Dec 28, 2017

etiennebec commented Dec 29, 2017

oleg-nenashev commented Jan 2, 2018

etiennebec commented Jan 2, 2018

etiennebec commented Jan 2, 2018 •

edited

oleg-nenashev commented Jan 30, 2018

oleg-nenashev commented Jan 30, 2018

etiennebec commented Jan 30, 2018

oleg-nenashev left a comment

oleg-nenashev commented Mar 3, 2018

ghost left a comment

[FIXED JENKINS-48561] Give precedence to proxy exclusion list system property over env var #243

[FIXED JENKINS-48561] Give precedence to proxy exclusion list system property over env var #243

Conversation

etiennebec commented Dec 14, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev Dec 14, 2017

Choose a reason for hiding this comment

oleg-nenashev Dec 14, 2017

Choose a reason for hiding this comment

oleg-nenashev Dec 14, 2017

Choose a reason for hiding this comment

etiennebec commented Dec 15, 2017 • edited

etiennebec commented Dec 15, 2017

oleg-nenashev commented Dec 15, 2017 • edited

oleg-nenashev commented Dec 28, 2017

etiennebec commented Dec 29, 2017

oleg-nenashev commented Jan 2, 2018

etiennebec commented Jan 2, 2018

etiennebec commented Jan 2, 2018 • edited

oleg-nenashev commented Jan 30, 2018

oleg-nenashev commented Jan 30, 2018

etiennebec commented Jan 30, 2018

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Mar 3, 2018

ghost left a comment

Choose a reason for hiding this comment

etiennebec commented Dec 15, 2017 •

edited

oleg-nenashev commented Dec 15, 2017 •

edited

etiennebec commented Jan 2, 2018 •

edited