[PING] RFR 6425769: jmx remote bind address

Tue Dec 1 10:17:09 UTC 2015

On Mon, 2015-11-09 at 10:32 +0100, Severin Gehwolf wrote:
> 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.

Anyone?

> 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
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 