[jira] [Commented] (CASSANDRA-11695) Move JMX connection config to cassandra.yaml

["Sam Tunnicliffe (JIRA)" <ji...@apache.org>]

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