[
https://issues.apache.org/jira/browse/CASSANDRA-11695?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15456190#comment-15456190
]
Sam Tunnicliffe commented on CASSANDRA-11695:
---------------------------------------------
[~zzheng] thanks for the patch, I think this could be pretty useful.
Sorry it's taken so long to get to review. There have been some changes since
you posted the patch (principally CASSANDRA-12109), so I've rebased against
trunk and pushed the result
[here|https://github.com/beobal/cassandra/tree/11695-trunk].
On the patch itself, I have a couple of remarks (& I've pushed some commits to
my branch to illustrate):
I agree that it would be best to reuse {{SSLFactory}} to obtain an
{{SSLContext}} and let that handle the keystore and truststore configuration.
To that end, I've refactored {{JMXServerOptions}} a little and added a new
subclass of {{EncryptionOptions}} which we can use to obtain the context.
The patch broke {{JMXAuthTest}} as its setup was driven by setting system
properties. Adding the dependency on {{DatabaseDescriptor}} also makes the
whole thing harder to test in general. So, I've modified
{{JMXServerUtils::createJMXServer}} to take an options object as an argument.
I don't think that having the option to override settings in the yaml with
system properties is a great idea, as it's likely to make debugging issues
harder. So, I've removed that functionality in favour of having everything
configured only from yaml options. This will make upgrades more onerous, but a
decent explanation and clear instructions in NEWS.txt should mitigate that. (I
haven't added that to NEWS.txt yet, btw).
One exception to that last point re: system properties concerns the location of
a JAAS config file. As far as I know, the only portable way to specify that
location is to use the {{java.security.auth.login.config}} property. As with
the keystore settings, spreading this config across multiple places makes for a
bad experience and is somewhat brittle, so I prefer the approach in your patch,
where the value of the yaml setting is used to set the system property.
However, there is a caveat. It's possible that operators may need to set that
property for other reasons - i.e. if they have any custom components (triggers,
indexes, authenticator, role manager) which uses login configuration when
connecting to some external system, we shouldn't stomp over their existing
setting. So in {{JMXServerUtil::configureJmxAuthentication}} I've changed it so
that we set the system property to the value from the yaml only if it isn't set
already.
[~zzheng]what do you think of the additional changes I've outlined?
Also, [~mshuler] - last time we changed stuff in this area, we ended up
breaking some test infrastructure which was depending on using system
properties (CASSANDRA-11725). Is it possible to verify that this doesn't cause
any problems? (Sorry, I forgot exactly what it was that broke last time).
One final thing to note - this will require a change to dtests, as there are
tests for JMX
[auth|https://github.com/riptano/cassandra-dtest/blob/master/jmx_auth_test.py]
and
[SSL|https://github.com/riptano/cassandra-dtest/blob/master/jmx_test.py#L194].
This would be an improvement though (and a pretty minor modification) as it
would remove the need to munge the cassandra-env files. I can do this once we
agree on the approach here. See:
[here|https://github.com/riptano/cassandra-dtest/blob/master/tools/jmxutils.py#L57-L126]
and
[here:|https://github.com/riptano/cassandra-dtest/blob/master/tools/jmxutils.py#L129-L152]
> Move JMX connection config to cassandra.yaml
> --------------------------------------------
>
> Key: CASSANDRA-11695
> URL: https://issues.apache.org/jira/browse/CASSANDRA-11695
> Project: Cassandra
> Issue Type: Improvement
> Components: Configuration
> Reporter: Sam Tunnicliffe
> Priority: Minor
> Labels: lhf
> Fix For: 3.x
>
>
> Since CASSANDRA-10091, we always construct the JMX connector server
> programatically, so we could move its configuration from cassandra-env to
> yaml.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
