This is an automated email from the ASF dual-hosted git repository.
dlmarion pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new 293e304945 Validate that classloader context is valid when property is
set (#3678)
293e304945 is described below
commit 293e30494548d6eca247df9b68cef3c613cd8066
Author: Dave Marion <dlmar...@apache.org>
AuthorDate: Thu Aug 10 10:13:54 2023 -0400
Validate that classloader context is valid when property is set (#3678)
Added method to ContextClassLoaderFactory that is called when
the table.context.class.loader property is set to validate that the
context name is valid. Default implementation returns true if the
implementation returns a non-null ClassLoader and does not throw
an exception. The new method is also called from
ClassLoaderUtil.getClassLoader(String) and will return the non-context
aware Accumulo ClassLoader if isValid returns false.
Co-authored-by: Ed Coleman <edcole...@apache.org>
---
.../accumulo/core/classloader/ClassLoaderUtil.java | 18 +++++++--
.../DefaultContextClassLoaderFactory.java | 19 ++++++++++
.../core/spi/common/ContextClassLoaderFactory.java | 26 +++++++++++++
.../core/classloader/URLClassLoaderFactory.java | 44 +++++++++++++++++-----
.../org/apache/accumulo/server/util/PropUtil.java | 15 +++++---
.../accumulo/server/util/SystemPropUtil.java | 19 ++++++----
.../classloader/vfs/AccumuloVFSClassLoader.java | 2 +-
.../start/classloader/vfs/ContextManager.java | 10 ++++-
.../apache/accumulo/test/shell/ShellServerIT.java | 25 ++++++++++++
9 files changed, 152 insertions(+), 26 deletions(-)
diff --git
a/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java
b/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java
index 3a5d0ebda3..8fc7a85cc3 100644
---
a/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java
+++
b/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java
@@ -75,13 +75,25 @@ public class ClassLoaderUtil {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- if (context != null && !context.isEmpty()) {
- return FACTORY.getClassLoader(context);
- } else {
+ try {
+ if (FACTORY.isValid(context)) {
+ return FACTORY.getClassLoader(context);
+ } else {
+ return
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
+ }
+ } catch (RuntimeException e) {
return
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
}
}
+ public static boolean isValidContext(String context) {
+ try {
+ return FACTORY.isValid(context);
+ } catch (RuntimeException e) {
+ return false;
+ }
+ }
+
public static <U> Class<? extends U> loadClass(String context, String
className,
Class<U> extension) throws ClassNotFoundException {
return getClassLoader(context).loadClass(className).asSubclass(extension);
diff --git
a/core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java
b/core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java
index 984e86d0b8..695aa3e473 100644
---
a/core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java
+++
b/core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java
@@ -20,6 +20,7 @@ package org.apache.accumulo.core.classloader;
import static java.util.concurrent.TimeUnit.MINUTES;
+import java.io.IOException;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ScheduledFuture;
@@ -95,8 +96,26 @@ public class DefaultContextClassLoaderFactory implements
ContextClassLoaderFacto
@SuppressWarnings("deprecation")
@Override
public ClassLoader getClassLoader(String contextName) {
+ if (contextName == null || contextName.isEmpty()) {
+ throw new IllegalArgumentException("contextName must have a value");
+ }
return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader
.getContextClassLoader(contextName);
}
+ @SuppressWarnings("deprecation")
+ @Override
+ public boolean isValid(String contextName) {
+ if (contextName == null || contextName.isEmpty()) {
+ return false;
+ }
+ try {
+ return
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getContextManager()
+ .isKnownContext(contextName);
+ } catch (IOException e) {
+ LOG.error("Error checking that context is valid, context: " +
contextName, e);
+ return false;
+ }
+ }
+
}
diff --git
a/core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java
b/core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java
index 7f423f3ead..c713653670 100644
---
a/core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java
+++
b/core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java
@@ -18,6 +18,8 @@
*/
package org.apache.accumulo.core.spi.common;
+import org.slf4j.LoggerFactory;
+
/**
* The ContextClassLoaderFactory provides a mechanism for various Accumulo
components to use a
* custom ClassLoader for specific contexts, such as loading table iterators.
This factory is
@@ -65,4 +67,28 @@ public interface ContextClassLoaderFactory {
* @return the class loader for the given contextName
*/
ClassLoader getClassLoader(String contextName);
+
+ /**
+ * Validate that the contextName is supported by the
ContextClassLoaderFactory implementation
+ *
+ * @param contextName the name of the context that represents a class loader
that is managed by
+ * this factory (can be null)
+ * @return true if valid, false otherwise
+ * @since 2.1.2
+ */
+ default boolean isValid(String contextName) {
+ try {
+ ClassLoader cl = getClassLoader(contextName);
+ if (cl == null) {
+ LoggerFactory.getLogger(ContextClassLoaderFactory.class)
+ .info("Null ClassLoader returned for context: {}", contextName);
+ return false;
+ }
+ return true;
+ } catch (RuntimeException e) {
+ LoggerFactory.getLogger(ContextClassLoaderFactory.class).info(
+ "Error thrown getting ClassLoader for context: {}, msg: {}",
contextName, e.getMessage());
+ return false;
+ }
+ }
}
diff --git
a/core/src/test/java/org/apache/accumulo/core/classloader/URLClassLoaderFactory.java
b/core/src/test/java/org/apache/accumulo/core/classloader/URLClassLoaderFactory.java
index c97a7d231e..07ebfc5dfe 100644
---
a/core/src/test/java/org/apache/accumulo/core/classloader/URLClassLoaderFactory.java
+++
b/core/src/test/java/org/apache/accumulo/core/classloader/URLClassLoaderFactory.java
@@ -21,26 +21,52 @@ package org.apache.accumulo.core.classloader;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;
-import java.util.stream.Stream;
import org.apache.accumulo.core.spi.common.ContextClassLoaderFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
// test implementation
public class URLClassLoaderFactory implements ContextClassLoaderFactory {
+ private static final Logger LOG =
LoggerFactory.getLogger(URLClassLoaderFactory.class);
private static final String COMMA = ",";
+ private URL[] parseURLsFromContextName(String contextName) throws
MalformedURLException {
+ String[] parts = contextName.split(COMMA);
+ URL[] urls = new URL[parts.length];
+ for (int i = 0; i < parts.length; i++) {
+ urls[i] = new URL(parts[i]);
+ }
+ return urls;
+ }
+
@Override
public ClassLoader getClassLoader(String contextName) {
+ if (contextName == null || contextName.isEmpty()) {
+ throw new IllegalArgumentException("contextName must have a value");
+ }
// The context name is the classpath.
- URL[] urls = Stream.of(contextName.split(COMMA)).map(p -> {
- try {
- return new URL(p);
- } catch (MalformedURLException e) {
- throw new IllegalArgumentException("Error creating URL from classpath
segment: " + p, e);
- }
- }).toArray(URL[]::new);
- return URLClassLoader.newInstance(urls);
+ try {
+ URL[] urls = parseURLsFromContextName(contextName);
+ return URLClassLoader.newInstance(urls);
+ } catch (MalformedURLException e) {
+ throw new IllegalArgumentException("Error creating URL from contextName:
" + contextName, e);
+ }
+ }
+
+ @Override
+ public boolean isValid(String contextName) {
+ if (contextName == null || contextName.isEmpty()) {
+ return false;
+ }
+ try {
+ parseURLsFromContextName(contextName);
+ return true;
+ } catch (MalformedURLException e) {
+ LOG.error("Error creating URL from contextName: " + contextName, e);
+ return false;
+ }
}
}
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/util/PropUtil.java
b/server/base/src/main/java/org/apache/accumulo/server/util/PropUtil.java
index ae20ac67c4..85e03ea1c2 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/util/PropUtil.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/util/PropUtil.java
@@ -21,6 +21,7 @@ package org.apache.accumulo.server.util;
import java.util.Collection;
import java.util.Map;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.server.ServerContext;
import org.apache.accumulo.server.conf.store.PropStoreKey;
@@ -38,7 +39,7 @@ public final class PropUtil {
*/
public static void setProperties(final ServerContext context, final
PropStoreKey<?> propStoreKey,
final Map<String,String> properties) throws IllegalArgumentException {
- PropUtil.validateProperties(context, propStoreKey, properties);
+ PropUtil.validateProperties(propStoreKey, properties);
context.getPropStore().putAll(propStoreKey, properties);
}
@@ -50,13 +51,12 @@ public final class PropUtil {
public static void replaceProperties(final ServerContext context,
final PropStoreKey<?> propStoreKey, final long version, final
Map<String,String> properties)
throws IllegalArgumentException {
- PropUtil.validateProperties(context, propStoreKey, properties);
+ PropUtil.validateProperties(propStoreKey, properties);
context.getPropStore().replaceAll(propStoreKey, version, properties);
}
- protected static void validateProperties(final ServerContext context,
- final PropStoreKey<?> propStoreKey, final Map<String,String> properties)
- throws IllegalArgumentException {
+ protected static void validateProperties(final PropStoreKey<?> propStoreKey,
+ final Map<String,String> properties) throws IllegalArgumentException {
for (Map.Entry<String,String> prop : properties.entrySet()) {
if (!Property.isValidProperty(prop.getKey(), prop.getValue())) {
String exceptionMessage = "Invalid property for : ";
@@ -65,6 +65,11 @@ public final class PropUtil {
}
throw new IllegalArgumentException(exceptionMessage + propStoreKey + "
name: "
+ prop.getKey() + ", value: " + prop.getValue());
+ } else if
(prop.getKey().equals(Property.TABLE_CLASSLOADER_CONTEXT.getKey())
+ &&
!Property.TABLE_CLASSLOADER_CONTEXT.getDefaultValue().equals(prop.getValue())
+ && !ClassLoaderUtil.isValidContext(prop.getValue())) {
+ throw new IllegalArgumentException(
+ "Unable to resolve classloader for context: " + prop.getValue());
}
}
}
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java
b/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java
index 219a6cee03..0bb2e8a8e3 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java
@@ -36,17 +36,19 @@ public class SystemPropUtil {
public static void setSystemProperty(ServerContext context, String property,
String value)
throws IllegalArgumentException {
- context.getPropStore().putAll(SystemPropKey.of(context),
- Map.of(validateSystemProperty(property, value), value));
+ final SystemPropKey key = SystemPropKey.of(context);
+ context.getPropStore().putAll(key, Map.of(validateSystemProperty(key,
property, value), value));
}
public static void modifyProperties(ServerContext context, long version,
Map<String,String> properties) throws IllegalArgumentException {
+ final SystemPropKey key = SystemPropKey.of(context);
final Map<String,
- String> checkedProperties = properties.entrySet().stream().collect(
- Collectors.toMap(entry -> validateSystemProperty(entry.getKey(),
entry.getValue()),
+ String> checkedProperties = properties.entrySet().stream()
+ .collect(Collectors.toMap(
+ entry -> validateSystemProperty(key, entry.getKey(),
entry.getValue()),
Map.Entry::getValue));
- context.getPropStore().replaceAll(SystemPropKey.of(context), version,
checkedProperties);
+ context.getPropStore().replaceAll(key, version, checkedProperties);
}
public static void removeSystemProperty(ServerContext context, String
property) {
@@ -64,8 +66,8 @@ public class SystemPropUtil {
context.getPropStore().removeProperties(SystemPropKey.of(context),
List.of(property));
}
- private static String validateSystemProperty(String property, final String
value)
- throws IllegalArgumentException {
+ private static String validateSystemProperty(SystemPropKey key, String
property,
+ final String value) throws IllegalArgumentException {
// Retrieve the replacement name for this property, if there is one.
// Do this before we check if the name is a valid zookeeper name.
final var original = property;
@@ -85,6 +87,9 @@ public class SystemPropUtil {
log.trace("Encountered error setting zookeeper property", iae);
throw iae;
}
+ if (Property.isValidTablePropertyKey(property)) {
+ PropUtil.validateProperties(key, Map.of(property, value));
+ }
// Find the property taking prefix into account
Property foundProp = null;
diff --git
a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
index 55aa86c554..d7626025e7 100644
---
a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
+++
b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
@@ -424,7 +424,7 @@ public class AccumuloVFSClassLoader {
}
}
- private static synchronized ContextManager getContextManager() throws
IOException {
+ public static synchronized ContextManager getContextManager() throws
IOException {
if (contextManager == null) {
getClassLoader();
contextManager = new ContextManager(generateVfs(),
AccumuloVFSClassLoader::getClassLoader);
diff --git
a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java
b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java
index c57c38c4c0..d571dba6c6 100644
---
a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java
+++
b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java
@@ -31,7 +31,7 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Deprecated
-class ContextManager {
+public class ContextManager {
private static final Logger log =
LoggerFactory.getLogger(ContextManager.class);
@@ -158,6 +158,14 @@ class ContextManager {
this.config = config;
}
+ public boolean isKnownContext(String contextName) {
+ ContextConfig cconfig = config.getContextConfig(contextName);
+ if (cconfig == null) {
+ return false;
+ }
+ return true;
+ }
+
public ClassLoader getClassLoader(String contextName) throws
FileSystemException {
ContextConfig cconfig = config.getContextConfig(contextName);
diff --git
a/test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java
b/test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java
index fae2e6531f..1122b13338 100644
--- a/test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java
+++ b/test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java
@@ -50,6 +50,7 @@ import java.util.regex.Pattern;
import org.apache.accumulo.core.Constants;
import org.apache.accumulo.core.client.Accumulo;
import org.apache.accumulo.core.client.AccumuloClient;
+import org.apache.accumulo.core.client.AccumuloException;
import org.apache.accumulo.core.client.IteratorSetting;
import org.apache.accumulo.core.client.Scanner;
import org.apache.accumulo.core.client.sample.RowColumnSampler;
@@ -2257,4 +2258,28 @@ public class ShellServerIT extends SharedMiniClusterBase
{
System.setProperty("accumulo.properties", orgProps);
}
}
+
+ @Test
+ public void failOnInvalidClassloaderContestTest() throws Exception {
+
+ final String[] names = getUniqueNames(3);
+ final String table1 = names[0];
+ final String namespace1 = names[1];
+ final String table2 = namespace1 + "." + names[2];
+
+ ts.exec("createtable " + table1, true);
+ ts.exec("createnamespace " + namespace1, true);
+ ts.exec("createtable " + table2, true);
+
+ ts.exec("config -s table.class.loader.context=invalid", false,
+ AccumuloException.class.getName(), true);
+ ts.exec("config -s table.class.loader.context=invalid -ns " + namespace1,
false,
+ AccumuloException.class.getName(), true);
+ ts.exec("config -s table.class.loader.context=invalid -t " + table1, false,
+ AccumuloException.class.getName(), true);
+ ts.exec("config -s table.class.loader.context=invalid -t " + table2, false,
+ AccumuloException.class.getName(), true);
+
+ }
+
}
