Re: JMX exposing non-standard java classes, to fix requires a breaking change

[David Capwell <dc...@apple.com>]

Filed CASSANDRA-17580 and have a patch ready which will hide non-native java exceptions from leaking into JMX (Cassandra and third-party libraries), this is enabled by default but can be disabled via config or system property

Config:
jmx_hide_non_java_exceptions: false

System property

-Dcassandra.settings.jmx_hide_non_java_exceptions=false

Does anyone see any issues about this?

> On Apr 6, 2022, at 9:35 AM, David Capwell <dc...@gmail.com> wrote:
> 
> I noticed in DatabaseDescriptor there are setters that are leaking it to JMX since cassandra-3.0. I am not sure whether we can just change it to IllegalArgumentException which will be then also thrown on startup where they were ConfigurationExceptions before
> 
> My concern here is maintenance, you can fix it once but it will break again. I feel we need a more general solution, open to input here!
> 
> Looking into our own NodeTool we have two error codes - 1 that catches a few java and airlift exceptions and 2 for other errors. My guess is we aim to throw only exceptions that will lead to exit code 1, no? I am not sure.
> 
> Good point, nodetool is also a client to the JMX, but fully knows our API, whatever solution we do we need to be careful with nodetool's assumptions.  Looking at org.apache.cassandra.tools.NodeTool#execute it doesn't look like these errors are JMX related, but nodetool related (airlift is the CLI parser used in nodetool); but IllegalArgumentException and IllegalStateException could have been caused from a JMX call (RuntimeException thrown from Cassandra would result in status = 2)
> 
> In many setters we were actually not doing the same checks we do on Startup too… I consider this being a bug. 
> 
> Agree, if you can bypass our guards via JMX, that sounds like a bug to me
> 
> 
> On Wed, Apr 6, 2022 at 8:50 AM Ekaterina Dimitrova <e....@gmail.com> wrote:
> I would say keep the old ones and add new ones to be compatible in addition.
> Now your catch led me to more thinking and I think throwing ConfigurationException deserves more attention. I noticed in DatabaseDescriptor there are setters that are leaking it to JMX since cassandra-3.0. I am not sure whether we can just change it to IllegalArgumentException which will be then also thrown on startup where they were ConfigurationExceptions before. 
> Not sure what is the right course of action here to fix things without being considered breaking changes too as I see different people doing different things in our codebase.
> Looking into our own NodeTool we have two error codes - 1 that catches a few java and airlift exceptions and 2 for other errors. My guess is we aim to throw only exceptions that will lead to exit code 1, no? I am not sure.
> GetColumnIndexSize was added in trunk and I see it throws ConfigurationException, that was acknowledged in its test. Considering the setter was always doing it I guess this was just normal addition but It seems to me it is actually a bug… Now I can change it to throw IllegalArgumentException. Looking at the code I think different people probably have different understanding so I want to align us to ensure we don’t break tools by fixing or not stuff… 
> In many setters we were actually not doing the same checks we do on Startup too… I consider this being a bug. 
> 
> On Tue, 5 Apr 2022 at 18:21, David Capwell <dc...@gmail.com> wrote:
> There are 2 places that expose non-standard java classes, so JMX only
> works if and only if the JMX client also has Cassandra's jars, else
> they will fail; the 2 examples are
> 
> org.apache.cassandra.service.StorageServiceMBean#enableAuditLog throws
> org.apache.cassandra.exceptions.ConfigurationException, removing that
> exception does not break binary compatibility, but does break source
> as javac will say catching it isn't allowed as it doesn't throw.  If
> you call the method without Cassandra jars the method will work
> properly until a ConfigurationException is thrown, in which case a
> ClassNotFoundException will be thrown, hiding the error message.
> 
> org.apache.cassandra.db.ColumnFamilyStoreMBean#forceCompactionForTokenRange
> takes a collection of
> org.apache.cassandra.dht.Range<org.apache.cassandra.dht.Token>, since
> this is an operation the only people who could call it are those with
> the jars in the path.
> 
> Given that both are not properties and require setting values, jmx get
> object does not break, so the impact is limited to the callers of each
> method.
> 
> We have a few options to address:
> 
> 1) live with it.  Allow these cases to keep doing what they are doing,
> but block new cases from popping up
> 2) live with it, but expose new versions which do not break JMX
> 3) remove ConfigurationException from
> StorageServiceMBean#enableAuditLog and accept the source compatibility
> issue
> 4) remove or fix ColumnFamilyStoreMBean#forceCompactionForTokenRange
> 5) #3 and #4
> 
> What do others think?  In CASSANDRA-17527 I am adding a test to detect
> these cases and block them, I am also fixing the JMX issues on trunk
> that were not released, so this thread is only limited to the cases
> that were actually released
> 
> Thanks!