You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/05/13 21:31:37 UTC
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1607: CASSANDRA-17580: Clients using JMX are unable to handle non-standard java types but we leak this into our Exceptions
ekaterinadimitrova2 commented on code in PR #1607:
URL: https://github.com/apache/cassandra/pull/1607#discussion_r871730654
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -853,6 +853,8 @@ public static void setClientMode(boolean clientMode)
public volatile DurationSpec repair_state_expires = DurationSpec.inDays(3);
public volatile int repair_state_size = 100_000;
+ public boolean jmx_hide_non_java_exceptions = true;
Review Comment:
Let's make this one `false` by default as we spoke.
In that sense I suggest on this branch (https://github.com/ekaterinadimitrova2/cassandra/commits/17580-review) a few commits to change the config to throw `IllegalArgumentException` as we actually make `loadConfig` to throw ConfigurationException.
I also corrected a few setters in the DB descriptor which were not released yet
All in all there I see only three problematic now which were already released.
Wondering whether we should add new versions that throw `IllegalArgumentException` and deprecate the old ones.
`setRepairSessionSpaceInMiB, setMaxConcurrentAutoUpgradeTasks `
`checkValidForByteConversion` is used both on startup and in a few setters.
##########
src/java/org/apache/cassandra/service/CassandraDaemon.java:
##########
@@ -147,6 +147,8 @@ private void maybeInitJmx()
{
logger.warn("JMX settings in cassandra-env.sh have been bypassed as the JMX connector server is " +
"already initialized. Please refer to cassandra-env.(sh|ps1) for JMX configuration info");
+
+ JMXServerUtils.hackPatchGlobalMBeanServerToFixExceptions();
Review Comment:
I guess this was WIP name :D How about `patchGlobalMBeanServerToFixExceptions`?
##########
test/unit/org/apache/cassandra/utils/JMXServerUtilsTest.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.utils;
+
+import java.io.IOException;
+import java.net.InetAddress;
+import javax.management.AttributeNotFoundException;
+import javax.management.InstanceNotFoundException;
+import javax.management.MBeanException;
+import javax.management.MBeanServerConnection;
+import javax.management.MalformedObjectNameException;
+import javax.management.ObjectName;
+import javax.management.ReflectionException;
+import javax.management.remote.JMXConnector;
+import javax.management.remote.JMXConnectorFactory;
+import javax.management.remote.JMXConnectorServer;
+import javax.management.remote.JMXServiceURL;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.assertj.core.api.ThrowableAssert;
+
+import static org.apache.cassandra.config.CassandraRelevantProperties.IS_DISABLED_MBEAN_REGISTRATION;
+
+public class JMXServerUtilsTest
+{
+ @BeforeClass
+ public static void setup()
+ {
+ IS_DISABLED_MBEAN_REGISTRATION.setBoolean(false);
+ }
+
+ @Test
+ public void test() throws IOException, MalformedObjectNameException, InstanceNotFoundException, AttributeNotFoundException, ReflectionException, MBeanException
+ {
+ InetAddress loopback = InetAddress.getLoopbackAddress();
+ String host = loopback.getHostAddress();
+ int port = CQLTester.getAutomaticallyAllocatedPort(loopback);
+
+ JMXConnectorServer server = JMXServerUtils.createJMXServer(port, host, true);
+ try
+ {
+ // register an MBean that will fail
+ ObjectName mbeanName = new ObjectName("org.apache.cassandra.test:type=Broken");
+ MBeanWrapper.instance.registerMBean(new Broken(), mbeanName);
+
+ String url = "service:jmx:rmi:///jndi/rmi://" + host + ":" + port + "/jmxrmi";
+ try (JMXConnector jmxc = JMXConnectorFactory.connect(new JMXServiceURL(url), null))
+ {
+ MBeanServerConnection mbsc = jmxc.getMBeanServerConnection();
+
+ assertThatThrownByIsAllowed(() -> mbsc.getAttribute(mbeanName, "Value"));
+ assertThatThrownByIsAllowed(() -> mbsc.getAttribute(mbeanName, "ValueNested"));
+ }
+ }
+ finally
+ {
+ server.stop();
+ }
+ }
+
+ private static void assertThatThrownByIsAllowed(ThrowableAssert.ThrowingCallable fn)
+ {
+ try
+ {
+ fn.call();
+ }
+ catch (Throwable throwable)
+ {
+ if (!JMXServerUtils.isAllowed(throwable))
+ throw new AssertionError("Non-java exception found", throwable);
+ }
+ }
+
+ public interface BrokenMBean
+ {
+ String getValue();
+
+ String getValueNested();
+ }
+
+ public static class Broken implements BrokenMBean
+ {
+ @Override
+ public String getValue()
+ {
+ throw new ConfigurationException("Not allowed");
+ }
+
+ @Override
+ public String getValueNested()
+ {
+ int levels = 10;
+ RuntimeException e = new ConfigurationException("bottom level");
+ for (int i = 0; i < levels; i++)
+ e = new RuntimeException("level " + i, e);
+ throw e;
+ }
+ }
+}
Review Comment:
new line at the end
##########
src/java/org/apache/cassandra/utils/JMXServerUtils.java:
##########
@@ -143,6 +161,148 @@ public static JMXConnectorServer createJMXServer(int port, String hostname, bool
return jmxServer;
}
+ public static void hackPatchGlobalMBeanServerToFixExceptions()
+ {
+ if (!DatabaseDescriptor.getJmxHideNonJavaExceptions())
+ return;
+ try
+ {
+ Field field = JmxMBeanServer.class.getDeclaredField("interceptorsEnabled");
+ field.setAccessible(true);
+ for (MBeanServer m : MBeanServerFactory.findMBeanServer(null))
+ {
+ if (m instanceof JmxMBeanServer)
+ {
+ JmxMBeanServer server = (JmxMBeanServer) m;
+ try
+ {
+ field.set(server, true);
+ server.setMBeanServerInterceptor(JMXServerUtils.fixExceptions(server.getMBeanServerInterceptor()));
+ }
+ catch (Throwable t)
+ {
+ logger.warn("Unable to intersept JMX", t);
+ }
+ }
+ }
+ }
+ catch (Throwable t)
+ {
+ logger.warn("Unable to intersept JMX", t);
Review Comment:
typo: intercept
##########
src/java/org/apache/cassandra/utils/ForwardingMBeanServer.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.utils;
+
+import java.io.ObjectInputStream;
+import java.util.Objects;
+import java.util.Set;
+import javax.management.Attribute;
+import javax.management.AttributeList;
+import javax.management.AttributeNotFoundException;
+import javax.management.InstanceAlreadyExistsException;
+import javax.management.InstanceNotFoundException;
+import javax.management.IntrospectionException;
+import javax.management.InvalidAttributeValueException;
+import javax.management.ListenerNotFoundException;
+import javax.management.MBeanException;
+import javax.management.MBeanInfo;
+import javax.management.MBeanRegistrationException;
+import javax.management.MBeanServer;
+import javax.management.NotCompliantMBeanException;
+import javax.management.NotificationFilter;
+import javax.management.NotificationListener;
+import javax.management.ObjectInstance;
+import javax.management.ObjectName;
+import javax.management.OperationsException;
+import javax.management.QueryExp;
+import javax.management.ReflectionException;
+import javax.management.loading.ClassLoaderRepository;
+
+public class ForwardingMBeanServer implements MBeanServer
+{
+ private final MBeanServer delegate;
+
+ public ForwardingMBeanServer(MBeanServer mbeanServer)
+ {
+ this.delegate = Objects.requireNonNull(mbeanServer);
+ }
+
+ protected MBeanServer delegate()
+ {
+ return delegate;
+ }
+
+ @Override
+ public ObjectInstance createMBean(String className, ObjectName name) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException
+ {
+ return delegate().createMBean(className, name);
+ }
+
+ @Override
+ public ObjectInstance createMBean(String className, ObjectName name, ObjectName loaderName) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException, InstanceNotFoundException
+ {
+ return delegate().createMBean(className, name, loaderName);
+ }
+
+ @Override
+ public ObjectInstance createMBean(String className, ObjectName name, Object[] params, String[] signature) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException
+ {
+ return delegate().createMBean(className, name, params, signature);
+ }
+
+ @Override
+ public ObjectInstance createMBean(String className, ObjectName name, ObjectName loaderName, Object[] params, String[] signature) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException, InstanceNotFoundException
+ {
+ return delegate().createMBean(className, name, loaderName, params, signature);
+ }
+
+ @Override
+ public ObjectInstance registerMBean(Object object, ObjectName name) throws InstanceAlreadyExistsException, MBeanRegistrationException, NotCompliantMBeanException
+ {
+ return delegate().registerMBean(object, name);
+ }
+
+ @Override
+ public void unregisterMBean(ObjectName name) throws InstanceNotFoundException, MBeanRegistrationException
+ {
+ delegate().unregisterMBean(name);
+ }
+
+ @Override
+ public ObjectInstance getObjectInstance(ObjectName name) throws InstanceNotFoundException
+ {
+ return delegate().getObjectInstance(name);
+ }
+
+ @Override
+ public Set<ObjectInstance> queryMBeans(ObjectName name, QueryExp query)
+ {
+ return delegate().queryMBeans(name, query);
+ }
+
+ @Override
+ public Set<ObjectName> queryNames(ObjectName name, QueryExp query)
+ {
+ return delegate().queryNames(name, query);
+ }
+
+ @Override
+ public boolean isRegistered(ObjectName name)
+ {
+ return delegate().isRegistered(name);
+ }
+
+ @Override
+ public Integer getMBeanCount()
+ {
+ return delegate().getMBeanCount();
+ }
+
+ @Override
+ public Object getAttribute(ObjectName name, String attribute) throws MBeanException, AttributeNotFoundException, InstanceNotFoundException, ReflectionException
+ {
+ return delegate().getAttribute(name, attribute);
+ }
+
+ @Override
+ public AttributeList getAttributes(ObjectName name, String[] attributes) throws InstanceNotFoundException, ReflectionException
+ {
+ return delegate().getAttributes(name, attributes);
+ }
+
+ @Override
+ public void setAttribute(ObjectName name, Attribute attribute) throws InstanceNotFoundException, AttributeNotFoundException, InvalidAttributeValueException, MBeanException, ReflectionException
+ {
+ delegate().setAttribute(name, attribute);
+ }
+
+ @Override
+ public AttributeList setAttributes(ObjectName name, AttributeList attributes) throws InstanceNotFoundException, ReflectionException
+ {
+ return delegate().setAttributes(name, attributes);
+ }
+
+ @Override
+ public Object invoke(ObjectName name, String operationName, Object[] params, String[] signature) throws InstanceNotFoundException, MBeanException, ReflectionException
+ {
+ return delegate().invoke(name, operationName, params, signature);
+ }
+
+ @Override
+ public String getDefaultDomain()
+ {
+ return delegate().getDefaultDomain();
+ }
+
+ @Override
+ public String[] getDomains()
+ {
+ return delegate().getDomains();
+ }
+
+ @Override
+ public void addNotificationListener(ObjectName name, NotificationListener listener, NotificationFilter filter, Object handback) throws InstanceNotFoundException
+ {
+ delegate().addNotificationListener(name, listener, filter, handback);
+ }
+
+ @Override
+ public void addNotificationListener(ObjectName name, ObjectName listener, NotificationFilter filter, Object handback) throws InstanceNotFoundException
+ {
+ delegate().addNotificationListener(name, listener, filter, handback);
+ }
+
+ @Override
+ public void removeNotificationListener(ObjectName name, ObjectName listener) throws InstanceNotFoundException, ListenerNotFoundException
+ {
+ delegate().removeNotificationListener(name, listener);
+ }
+
+ @Override
+ public void removeNotificationListener(ObjectName name, ObjectName listener, NotificationFilter filter, Object handback) throws InstanceNotFoundException, ListenerNotFoundException
+ {
+ delegate().removeNotificationListener(name, listener, filter, handback);
+ }
+
+ @Override
+ public void removeNotificationListener(ObjectName name, NotificationListener listener) throws InstanceNotFoundException, ListenerNotFoundException
+ {
+ delegate().removeNotificationListener(name, listener);
+ }
+
+ @Override
+ public void removeNotificationListener(ObjectName name, NotificationListener listener, NotificationFilter filter, Object handback) throws InstanceNotFoundException, ListenerNotFoundException
+ {
+ delegate().removeNotificationListener(name, listener, filter, handback);
+ }
+
+ @Override
+ public MBeanInfo getMBeanInfo(ObjectName name) throws InstanceNotFoundException, IntrospectionException, ReflectionException
+ {
+ return delegate().getMBeanInfo(name);
+ }
+
+ @Override
+ public boolean isInstanceOf(ObjectName name, String className) throws InstanceNotFoundException
+ {
+ return delegate().isInstanceOf(name, className);
+ }
+
+ @Override
+ public Object instantiate(String className) throws ReflectionException, MBeanException
+ {
+ return delegate().instantiate(className);
+ }
+
+ @Override
+ public Object instantiate(String className, ObjectName loaderName) throws ReflectionException, MBeanException, InstanceNotFoundException
+ {
+ return delegate().instantiate(className, loaderName);
+ }
+
+ @Override
+ public Object instantiate(String className, Object[] params, String[] signature) throws ReflectionException, MBeanException
+ {
+ return delegate().instantiate(className, params, signature);
+ }
+
+ @Override
+ public Object instantiate(String className, ObjectName loaderName, Object[] params, String[] signature) throws ReflectionException, MBeanException, InstanceNotFoundException
+ {
+ return delegate().instantiate(className, loaderName, params, signature);
+ }
+
+ @Override
+ public ObjectInputStream deserialize(ObjectName name, byte[] data) throws InstanceNotFoundException, OperationsException
+ {
+ return delegate().deserialize(name, data);
Review Comment:
Seems those `deserialize` methods are deprecated in favor of `getClassLoader`, did you consider that?
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -853,6 +853,8 @@ public static void setClientMode(boolean clientMode)
public volatile DurationSpec repair_state_expires = DurationSpec.inDays(3);
public volatile int repair_state_size = 100_000;
+ public boolean jmx_hide_non_java_exceptions = true;
Review Comment:
Oh also in my branch I removed the validation of the config file as it is not needed anymore, the two problematic properties were set with `0ms` default value. Not being null anymore doesn't allow to have the problem that the validation was trying to prevent. Let me know if it can be part of this patch or you want me to pull it. Just int doesn't make sense to read one more time the config file when not actually needed anymore.
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -853,6 +853,8 @@ public static void setClientMode(boolean clientMode)
public volatile DurationSpec repair_state_expires = DurationSpec.inDays(3);
public volatile int repair_state_size = 100_000;
+ public boolean jmx_hide_non_java_exceptions = true;
Review Comment:
Oh I have bad sad news. I pushed my commit and then got an email from Circle that Java11 build failed:
```
_build_java:
[echo] Compiling for Java 11...
[javac] Compiling 2137 source files to /home/cassandra/cassandra/build/classes/main
[javac] Note: Processing compiler hints annotations
[javac] /home/cassandra/cassandra/src/java/org/apache/cassandra/utils/JMXServerUtils.java:67: error: package com.sun.jmx.mbeanserver is not visible
[javac] import com.sun.jmx.mbeanserver.JmxMBeanServer;
[javac] ^
[javac] (package com.sun.jmx.mbeanserver is declared in module java.management, which does not export it)
[javac] Note: Processing compiler hints annotations
[javac] Note: Writing compiler command file at META-INF/hotspot_compiler
[javac] Note: Done processing compiler hints annotations
[javac] 1 error
```
I feel the community won't be happy to open more internals for this :(
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -853,6 +853,8 @@ public static void setClientMode(boolean clientMode)
public volatile DurationSpec repair_state_expires = DurationSpec.inDays(3);
public volatile int repair_state_size = 100_000;
+ public boolean jmx_hide_non_java_exceptions = true;
Review Comment:
Also, we need NEWS.txt entry
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -853,6 +853,8 @@ public static void setClientMode(boolean clientMode)
public volatile DurationSpec repair_state_expires = DurationSpec.inDays(3);
public volatile int repair_state_size = 100_000;
+ public boolean jmx_hide_non_java_exceptions = true;
Review Comment:
Maybe also a bit of JavaDoc will be nice
##########
src/java/org/apache/cassandra/utils/ForwardingMBeanServer.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.utils;
+
+import java.io.ObjectInputStream;
+import java.util.Objects;
+import java.util.Set;
+import javax.management.Attribute;
+import javax.management.AttributeList;
+import javax.management.AttributeNotFoundException;
+import javax.management.InstanceAlreadyExistsException;
+import javax.management.InstanceNotFoundException;
+import javax.management.IntrospectionException;
+import javax.management.InvalidAttributeValueException;
+import javax.management.ListenerNotFoundException;
+import javax.management.MBeanException;
+import javax.management.MBeanInfo;
+import javax.management.MBeanRegistrationException;
+import javax.management.MBeanServer;
+import javax.management.NotCompliantMBeanException;
+import javax.management.NotificationFilter;
+import javax.management.NotificationListener;
+import javax.management.ObjectInstance;
+import javax.management.ObjectName;
+import javax.management.OperationsException;
+import javax.management.QueryExp;
+import javax.management.ReflectionException;
+import javax.management.loading.ClassLoaderRepository;
+
+public class ForwardingMBeanServer implements MBeanServer
+{
+ private final MBeanServer delegate;
+
+ public ForwardingMBeanServer(MBeanServer mbeanServer)
+ {
+ this.delegate = Objects.requireNonNull(mbeanServer);
+ }
+
+ protected MBeanServer delegate()
+ {
+ return delegate;
+ }
+
+ @Override
+ public ObjectInstance createMBean(String className, ObjectName name) throws ReflectionException, InstanceAlreadyExistsException, MBeanRegistrationException, MBeanException, NotCompliantMBeanException
Review Comment:
In theory I guess we can get rid of `MBeanException` here, but I see that `MBeanServer#createMBean` keeps it so I guess I am fine with it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org