jmx-dev RFR 6425769: jmx remote bind address

Mon Nov 9 09:32:05 UTC 2015

On Wed, 2015-11-04 at 11:54 +0100, Severin Gehwolf wrote:
> Hi,
> 
> Updated webrev with jtreg test in Java:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/02/
> bug: https://bugs.openjdk.java.net/browse/JDK-6425769
> 
> I believe this updated webrev addresses all concerns and incorporates
> suggestions brought up by Jaroslav and Daniel.
> 
> I'm still looking for a sponsor and a hotspot/servicability-dev
> reviewer. Could somebody maintaining javax.rmi.ssl have a look at
> this
> as well? Thank you!

Ping? Friendly reminder that I still need reviewers and a sponsor for
this.

Thanks,
Severin

> Cheers,
> Severin
> 
> On Tue, 2015-11-03 at 15:45 +0100, Jaroslav Bachorik wrote:
> > On 2.11.2015 19:06, Severin Gehwolf wrote:
> > > Hi,
> > > 
> > > Thanks Jaroslav and Daniel for the reviews! Comments inline.
> > > 
> > > On Mon, 2015-11-02 at 16:54 +0100, Jaroslav Bachorik wrote:
> > > > Hi,
> > > > 
> > > > On 2.11.2015 16:19, Daniel Fuchs wrote:
> > > > > Hi Severin,
> > > > > 
> > > > > Adding serviceability-dev at openjdk.java.net into the loop -
> > > > > that's
> > > > > a better alias than hotspot-dev for this kind of changes -
> > > > > maybe
> > > > > someone from serviceability-dev will offer to sponsor :-)
> > > > > 
> > > > > I will let serviceability team members comment on the hotspot
> > > > > changes.
> > > > > 
> > > > > ConnectorBootstrap.java
> > > > > 
> > > > > I have one suggestion and one concern:
> > > > > 
> > > > > Suggestion: I would suggest to reuse 'csf' (Client Socket
> > > > > Factory)
> > > > > and
> > > > > ssf  (Server Socket Factory) variables rather than introduce
> > > > > the
> > > > > two
> > > > > new variables rmiServerSocketFactory and
> > > > > rmiClientSocketFactory.
> > > > > You might want to create a new boolean 'useSocketFactory'
> > > > > variable,
> > > > > if that makes the code more readable.
> > > > > 
> > > > > Concern: If I understand correctly how RMI socket factories
> > > > > work,
> > > > > the client socket factory will be serialized and sent to the
> > > > > client
> > > > > side. This is problematic for interoperability, as the class
> > > > > may
> > > > > not
> > > > > present in the remote client - if the remote client is e.g.
> > > > > jdk
> > > > > 8.
> > > > > 
> > > > > As far as I can see, your new DefaultClientSocketFactory
> > > > > doesn't do
> > > > > anything useful - so I would suggest to simply get rid of it,
> > > > > and
> > > > > only
> > > > > set the Server Socket Factory when SSL is not involved.
> > > 
> > > Thanks. Fixed in updated webrev.
> > > 
> > > > > Tests:
> > > > > 
> > > > > Concerning the tests - we're trying to get rid of shell
> > > > > scripts
> > > > > rather than introducing new ones :-)
> > > > > Could the test be rewritten in pure java using the Process
> > > > > API?
> > > > > 
> > > > > I believe there's even a test library that will let you do
> > > > > that
> > > > > easily jdk/test/lib/testlibrary/jdk/testlibrary/
> > > > > (see ProcessTools.java)
> > > 
> > > It'll take me a bit to rewrite the test in pure Java, but should
> > > be
> > > fine. This is not yet fixed in the updated webrev.
> > > 
> > > > > Other:
> > > > > 
> > > > > Also - I believe the new option should be documented in:
> > > > > src/java.management/share/conf/management.properties
> > > 
> > > Docs have been updated
> > > in src/java.management/share/conf/management.properties.
> > > 
> > > > I share Daniel's concerns. Also, the part of the changeset is
> > > > related
> > > > to javax.rmi.ssl - someone maintaining this library should also
> > > > comment here.
> > > > 
> > > > Also, the change is introducing new API (system property) and
> > > > changing the existing one (adding SslRmiServerSocketFactory
> > > > public
> > > > constructors) so compatibility review process will have to be
> > > > involved.
> > > 
> > > OK. What exactly is there for me to do? I'm not familiar with
> > > this
> > > process. Note that the intent would be to get this backported to
> > > JDK 8.
> > Not much for you. But for the potential Oracle sponsor this means
> > she
> > will have to remember to go through some extra hoops before
> > integrating your patch.
> 
> I see. Thanks for clarifying it.
> 
> > -JB-
> > 
> > > 
> > > New webrev at:
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/01/
> > > 
> > > Thanks,
> > > Severin
> > > 
> > > > -JB-
> > > > > 
> > > > > best regards,
> > > > > 
> > > > > -- daniel
> > > > > 
> > > > > On 02/11/15 11:38, Severin Gehwolf wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Here is a patch addressing JDK-6425769. The issue is that
> > > > > > the
> > > > > > JMX
> > > > > > agent
> > > > > > binds to the wildcard address by default, preventing users
> > > > > > to
> > > > > > use
> > > > > > system properties for JMX agents on multi-homed hosts.
> > > > > > Given
> > > > > > a
> > > > > > host
> > > > > > with local network interfaces, 192.168.0.1 and 192.168.0.2
> > > > > > say,
> > > > > > it's
> > > > > > impossible to start two JMX agents binding to fixed ports
> > > > > > but
> > > > > > to
> > > > > > different network interfaces, 192.168.0.1:{9111,9112} and
> > > > > > 192.168.0.2:{9111,9112} say.
> > > > > > 
> > > > > > The JDK would bind to all local interfaces by default. In
> > > > > > the
> > > > > > above
> > > > > > example to 192.168.0.1 *and* 192.168.0.2. The effect is
> > > > > > that
> > > > > > the
> > > > > > second
> > > > > > Java process would get a "Connection refused" error because
> > > > > > another
> > > > > > process has already been bound to the specified JMX/RMI
> > > > > > port
> > > > > > pairs.
> > > > > > 
> > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-6425769
> > > > > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-64
> > > > > > 25
> > > > > > 769/
> > > > > > 00/
> > > > > > 
> > > > > > Testing done:
> > > > > > jdk_jmx and jdk_management tests all pass after this change
> > > > > > (no
> > > > > > regressions). There is also a new JTREG test which fails
> > > > > > prior
> > > > > > this
> > > > > > change and passes after. Updates to the diagnostic command
> > > > > > have
> > > > > > been
> > > > > > tested manually.
> > > > > > 
> > > > > > I understand that, if approved, the JDK and hotspot changes
> > > > > > should get
> > > > > > pushed together? Hotspot changes are fairly trivial since
> > > > > > it's
> > > > > > only a
> > > > > > doc-update for the new JDK property in the relevant
> > > > > > diagnostic
> > > > > > command.
> > > > > > 
> > > > > > Could someone please review and sponsor this change? Please
> > > > > > let
> > > > > > me know
> > > > > > if there are questions.
> > > > > > 
> > > > > > Thanks,
> > > > > > Severin
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 