You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2018/12/19 19:51:58 UTC
[GitHub] mikewalch closed pull request #841: Refactored SiteConfiguration
mikewalch closed pull request #841: Refactored SiteConfiguration
URL: https://github.com/apache/accumulo/pull/841
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientConfConverter.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientConfConverter.java
index bb55d09130..55372f3002 100644
--- a/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientConfConverter.java
+++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientConfConverter.java
@@ -166,19 +166,14 @@ public String get(Property property) {
if (property.isSensitive()) {
org.apache.hadoop.conf.Configuration hadoopConf = getHadoopConfiguration();
if (hadoopConf != null) {
- try {
- char[] value = CredentialProviderFactoryShim
- .getValueFromCredentialProvider(hadoopConf, key);
- if (value != null) {
- log.trace("Loaded sensitive value for {} from CredentialProvider", key);
- return new String(value);
- } else {
- log.trace("Tried to load sensitive value for {} from CredentialProvider, "
- + "but none was found", key);
- }
- } catch (IOException e) {
- log.warn("Failed to extract sensitive property ({}) from Hadoop CredentialProvider,"
- + " falling back to base AccumuloConfiguration", key, e);
+ char[] value = CredentialProviderFactoryShim.getValueFromCredentialProvider(hadoopConf,
+ key);
+ if (value != null) {
+ log.trace("Loaded sensitive value for {} from CredentialProvider", key);
+ return new String(value);
+ } else {
+ log.trace("Tried to load sensitive value for {} from CredentialProvider, "
+ + "but none was found", key);
}
}
}
diff --git a/core/src/main/java/org/apache/accumulo/core/conf/CredentialProviderFactoryShim.java b/core/src/main/java/org/apache/accumulo/core/conf/CredentialProviderFactoryShim.java
index c8ef29e8b6..33e9ab68ce 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/CredentialProviderFactoryShim.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/CredentialProviderFactoryShim.java
@@ -349,20 +349,15 @@ public static Configuration getConfiguration(Configuration conf, String credenti
* @param alias
* Name of CredentialEntry key
* @return The credential if found, null otherwise
- * @throws IOException
- * On errors reading a CredentialProvider
*/
- public static char[] getValueFromCredentialProvider(Configuration conf, String alias)
- throws IOException {
+ public static char[] getValueFromCredentialProvider(Configuration conf, String alias) {
requireNonNull(conf);
requireNonNull(alias);
-
if (isHadoopCredentialProviderAvailable()) {
log.trace("Hadoop CredentialProvider is available, attempting to extract value for {}",
alias);
return getFromHadoopCredentialProvider(conf, alias);
}
-
return null;
}
diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
index e20b7eff3a..dc759a3573 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
@@ -1186,18 +1186,6 @@ public static boolean isValidTablePropertyKey(String key) {
|| key.startsWith(TABLE_SCAN_DISPATCHER_OPTS.getKey())));
}
- /**
- * Properties we check the value of within the TabletServer request handling or maintenance
- * processing loops.
- */
- public static final EnumSet<Property> HOT_PATH_PROPERTIES = EnumSet.of(
- Property.TSERV_CLIENT_TIMEOUT, Property.TSERV_TOTAL_MUTATION_QUEUE_MAX,
- Property.GC_TRASH_IGNORE, Property.TSERV_MAJC_DELAY, Property.TABLE_MINC_LOGS_MAX,
- Property.TSERV_MAJC_MAXCONCURRENT, Property.REPLICATION_WORKER_THREADS,
- Property.TABLE_DURABILITY, Property.INSTANCE_ZK_TIMEOUT, Property.TABLE_CLASSPATH,
- Property.MASTER_METADATA_SUSPENDABLE, Property.TABLE_FAILURES_IGNORE,
- Property.TABLE_SCAN_MAXMEM, Property.INSTANCE_CRYPTO_SERVICE, Property.TABLE_DELETE_BEHAVIOR);
-
private static final EnumSet<Property> fixedProperties = EnumSet.of(Property.TSERV_CLIENTPORT,
Property.TSERV_NATIVEMAP_ENABLED, Property.TSERV_SCAN_MAX_OPENFILES,
Property.MASTER_CLIENTPORT, Property.GC_PORT);
diff --git a/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java b/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java
index df46dc2e28..96e6d856ba 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java
@@ -23,20 +23,19 @@
import java.net.URISyntaxException;
import java.net.URL;
import java.util.Collections;
+import java.util.HashMap;
import java.util.Map;
-import java.util.function.Function;
import java.util.function.Predicate;
-import java.util.stream.Collectors;
-import java.util.stream.StreamSupport;
import org.apache.accumulo.core.util.CachedConfiguration;
import org.apache.commons.configuration.CompositeConfiguration;
-import org.apache.commons.configuration.Configuration;
import org.apache.commons.configuration.ConfigurationException;
import org.apache.commons.configuration.PropertiesConfiguration;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.ImmutableMap;
+
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
/**
@@ -49,24 +48,24 @@
* property is not defined, it defaults to "accumulo.properties" and will look on classpath for
* file.
* <p>
- * This class is a singleton.
- * <p>
* <b>Note</b>: Client code should not use this class, and it may be deprecated in the future.
*/
public class SiteConfiguration extends AccumuloConfiguration {
+
private static final Logger log = LoggerFactory.getLogger(SiteConfiguration.class);
private static final AccumuloConfiguration parent = DefaultConfiguration.getInstance();
- private CompositeConfiguration internalConfig;
-
- private final Map<String,String> overrides;
- private final Map<String,String> staticConfigs;
+ private final ImmutableMap<String,String> config;
public SiteConfiguration() {
this(getAccumuloPropsLocation());
}
+ public SiteConfiguration(Map<String,String> overrides) {
+ this(getAccumuloPropsLocation(), overrides);
+ }
+
public SiteConfiguration(File accumuloPropsFile) {
this(accumuloPropsFile, Collections.emptyMap());
}
@@ -79,37 +78,52 @@ public SiteConfiguration(URL accumuloPropsLocation) {
this(accumuloPropsLocation, Collections.emptyMap());
}
- @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
- justification = "location of props is specified by an admin")
public SiteConfiguration(URL accumuloPropsLocation, Map<String,String> overrides) {
- this.overrides = overrides;
+ config = createMap(accumuloPropsLocation, overrides);
+ }
- init();
- PropertiesConfiguration config = new PropertiesConfiguration();
+ @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
+ justification = "location of props is specified by an admin")
+ private static ImmutableMap<String,String> createMap(URL accumuloPropsLocation,
+ Map<String,String> overrides) {
+ CompositeConfiguration config = new CompositeConfiguration();
+ config.setThrowExceptionOnMissing(false);
config.setDelimiterParsingDisabled(true);
+ PropertiesConfiguration propsConfig = new PropertiesConfiguration();
+ propsConfig.setDelimiterParsingDisabled(true);
if (accumuloPropsLocation != null) {
try {
- config.load(accumuloPropsLocation.openStream());
+ propsConfig.load(accumuloPropsLocation.openStream());
} catch (IOException | ConfigurationException e) {
throw new IllegalArgumentException(e);
}
}
- internalConfig.addConfiguration(config);
+ config.addConfiguration(propsConfig);
+
+ // Add all properties in config file
+ Map<String,String> result = new HashMap<>();
+ config.getKeys().forEachRemaining(key -> result.put(key, config.getString(key)));
- Map<String,String> temp = StreamSupport
- .stream(((Iterable<String>) internalConfig::getKeys).spliterator(), false)
- .collect(Collectors.toMap(Function.identity(), internalConfig::getString));
+ // Add all overrides
+ overrides.forEach(result::put);
- /*
- * If any of the configs used in hot codepaths are unset here, set a null so that we'll default
- * to the parent config without contending for the Hadoop Configuration object
- */
- for (Property hotConfig : Property.HOT_PATH_PROPERTIES) {
- if (!(temp.containsKey(hotConfig.getKey()))) {
- temp.put(hotConfig.getKey(), null);
+ // Add sensitive properties from credential provider (if set)
+ String credProvider = result.get(Property.GENERAL_SECURITY_CREDENTIAL_PROVIDER_PATHS.getKey());
+ if (credProvider != null) {
+ org.apache.hadoop.conf.Configuration hadoopConf = new org.apache.hadoop.conf.Configuration(
+ CachedConfiguration.getInstance());
+ hadoopConf.set(CredentialProviderFactoryShim.CREDENTIAL_PROVIDER_PATH, credProvider);
+ for (Property property : Property.values()) {
+ if (property.isSensitive()) {
+ char[] value = CredentialProviderFactoryShim.getValueFromCredentialProvider(hadoopConf,
+ property.getKey());
+ if (value != null) {
+ result.put(property.getKey(), new String(value));
+ }
+ }
}
}
- staticConfigs = Collections.unmodifiableMap(temp);
+ return ImmutableMap.copyOf(result);
}
private static URL toURL(File f) {
@@ -157,80 +171,22 @@ public static URL getAccumuloPropsLocation() {
}
}
- private void init() {
- internalConfig = new CompositeConfiguration();
- internalConfig.setThrowExceptionOnMissing(false);
- internalConfig.setDelimiterParsingDisabled(true);
- }
-
- private synchronized Configuration getConfiguration() {
- if (internalConfig == null) {
- init();
- }
- return internalConfig;
- }
-
@Override
public String get(Property property) {
- if (overrides.containsKey(property.getKey())) {
- return overrides.get(property.getKey());
- }
-
- String key = property.getKey();
- // If the property is sensitive, see if CredentialProvider was configured.
- if (property.isSensitive()) {
- String hadoopVal = getSensitiveFromHadoop(property);
- if (hadoopVal != null) {
- return hadoopVal;
- }
- }
-
- /*
- * Check the available-on-load configs and fall-back to the possibly-update Configuration
- * object.
- */
- String value = staticConfigs.containsKey(key) ? staticConfigs.get(key)
- : getConfiguration().getString(key);
-
+ String value = config.get(property.getKey());
if (value == null || !property.getType().isValidFormat(value)) {
- if (value != null)
- log.error("Using default value for {} due to improperly formatted {}: {}", key,
- property.getType(), value);
+ if (value != null) {
+ log.error("Using default value for {} due to improperly formatted {}: {}",
+ property.getKey(), property.getType(), value);
+ }
value = parent.get(property);
}
-
return value;
}
- private String getSensitiveFromHadoop(Property property) {
- org.apache.hadoop.conf.Configuration hadoopConf = getHadoopConfiguration();
- if (hadoopConf != null) {
- // Try to find the sensitive value from the CredentialProvider
- try {
- char[] value = CredentialProviderFactoryShim.getValueFromCredentialProvider(hadoopConf,
- property.getKey());
- if (value != null) {
- return new String(value);
- }
- } catch (IOException e) {
- log.warn("Failed to extract sensitive property (" + property.getKey()
- + ") from Hadoop CredentialProvider, falling back to accumulo.properties", e);
- }
- }
- return null;
- }
-
@Override
public boolean isPropertySet(Property prop, boolean cacheAndWatch) {
- if (prop.isSensitive()) {
- String hadoopVal = getSensitiveFromHadoop(prop);
- if (hadoopVal != null) {
- return true;
- }
- }
- return overrides.containsKey(prop.getKey()) || staticConfigs.containsKey(prop.getKey())
- || getConfiguration().containsKey(prop.getKey())
- || parent.isPropertySet(prop, cacheAndWatch);
+ return config.containsKey(prop.getKey()) || parent.isPropertySet(prop, cacheAndWatch);
}
@Override
@@ -243,78 +199,9 @@ public void getProperties(Map<String,String> props, Predicate<String> filter,
if (useDefaults) {
parent.getProperties(props, filter);
}
-
- StreamSupport.stream(((Iterable<String>) getConfiguration()::getKeys).spliterator(), false)
- .filter(filter).forEach(k -> props.put(k, getConfiguration().getString(k)));
-
- // CredentialProvider should take precedence over site
- org.apache.hadoop.conf.Configuration hadoopConf = getHadoopConfiguration();
- if (hadoopConf != null) {
- try {
- for (String key : CredentialProviderFactoryShim.getKeys(hadoopConf)) {
- if (!Property.isValidPropertyKey(key) || !Property.isSensitive(key)) {
- continue;
- }
-
- if (filter.test(key)) {
- char[] value = CredentialProviderFactoryShim.getValueFromCredentialProvider(hadoopConf,
- key);
- if (value != null) {
- props.put(key, new String(value));
- }
- }
- }
- } catch (IOException e) {
- log.warn("Failed to extract sensitive properties from Hadoop"
- + " CredentialProvider, falling back to accumulo.properties", e);
- }
- }
- if (overrides != null) {
- for (Map.Entry<String,String> entry : overrides.entrySet()) {
- if (filter.test(entry.getKey())) {
- props.put(entry.getKey(), entry.getValue());
- }
- }
- }
- }
-
- protected org.apache.hadoop.conf.Configuration getHadoopConfiguration() {
- String credProviderPathsKey = Property.GENERAL_SECURITY_CREDENTIAL_PROVIDER_PATHS.getKey();
- String credProviderPathsValue = getConfiguration().getString(credProviderPathsKey);
-
- if (credProviderPathsValue != null) {
- // We have configuration for a CredentialProvider
- // Try to pull the sensitive password from there
- org.apache.hadoop.conf.Configuration conf = new org.apache.hadoop.conf.Configuration(
- CachedConfiguration.getInstance());
- conf.set(CredentialProviderFactoryShim.CREDENTIAL_PROVIDER_PATH, credProviderPathsValue);
- return conf;
- }
-
- return null;
- }
-
- /**
- * Sets a property. This method supports testing and should not be called.
- *
- * @param property
- * property to set
- * @param value
- * property value
- */
- public void set(Property property, String value) {
- set(property.getKey(), value);
- }
-
- /**
- * Sets a property. This method supports testing and should not be called.
- *
- * @param key
- * key of property to set
- * @param value
- * property value
- */
- public void set(String key, String value) {
- getConfiguration().setProperty(key, value);
+ config.keySet().forEach(k -> {
+ if (filter.test(k))
+ props.put(k, config.get(k));
+ });
}
}
diff --git a/core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java b/core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java
index c017b373ce..ee412f59d6 100644
--- a/core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java
+++ b/core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java
@@ -24,10 +24,7 @@
import java.net.URL;
import java.util.HashMap;
import java.util.Map;
-import java.util.function.Predicate;
-import org.apache.hadoop.conf.Configuration;
-import org.easymock.EasyMock;
import org.junit.BeforeClass;
import org.junit.Test;
@@ -57,33 +54,43 @@ public void testOnlySensitivePropertiesExtractedFromCredentialProvider()
return;
}
- SiteConfiguration siteCfg = EasyMock.createMockBuilder(SiteConfiguration.class)
- .addMockedMethod("getHadoopConfiguration").createMock();
-
- siteCfg.set(Property.INSTANCE_SECRET, "ignored");
-
// site-cfg.jceks={'ignored.property'=>'ignored', 'instance.secret'=>'mysecret',
// 'general.rpc.timeout'=>'timeout'}
URL keystore = SiteConfigurationTest.class.getResource("/site-cfg.jceks");
assertNotNull(keystore);
- String keystorePath = new File(keystore.getFile()).getAbsolutePath();
-
- Configuration hadoopConf = new Configuration();
- hadoopConf.set(CredentialProviderFactoryShim.CREDENTIAL_PROVIDER_PATH,
- "jceks://file" + keystorePath);
+ String credProvPath = "jceks://file" + new File(keystore.getFile()).getAbsolutePath();
- EasyMock.expect(siteCfg.getHadoopConfiguration()).andReturn(hadoopConf).once();
+ SiteConfiguration config = new SiteConfiguration(ImmutableMap
+ .of(Property.GENERAL_SECURITY_CREDENTIAL_PROVIDER_PATHS.getKey(), credProvPath));
- EasyMock.replay(siteCfg);
+ assertEquals("mysecret", config.get(Property.INSTANCE_SECRET));
+ assertNull(config.get("ignored.property"));
+ assertEquals(Property.GENERAL_RPC_TIMEOUT.getDefaultValue(),
+ config.get(Property.GENERAL_RPC_TIMEOUT.getKey()));
+ }
- Map<String,String> props = new HashMap<>();
- Predicate<String> all = x -> true;
- siteCfg.getProperties(props, all);
+ @Test
+ public void testDefault() {
+ SiteConfiguration conf = new SiteConfiguration();
+ assertEquals("localhost:2181", conf.get(Property.INSTANCE_ZK_HOST));
+ assertEquals("DEFAULT", conf.get(Property.INSTANCE_SECRET));
+ assertEquals("", conf.get(Property.INSTANCE_VOLUMES));
+ assertEquals("120s", conf.get(Property.GENERAL_RPC_TIMEOUT));
+ assertEquals("1g", conf.get(Property.TSERV_WALOG_MAX_SIZE));
+ assertEquals("org.apache.accumulo.core.cryptoImpl.NoCryptoService",
+ conf.get(Property.INSTANCE_CRYPTO_SERVICE));
+ }
- assertEquals("mysecret", props.get(Property.INSTANCE_SECRET.getKey()));
- assertNull(props.get("ignored.property"));
- assertEquals(Property.GENERAL_RPC_TIMEOUT.getDefaultValue(),
- props.get(Property.GENERAL_RPC_TIMEOUT.getKey()));
+ @Test
+ public void testFile() {
+ URL propsUrl = getClass().getClassLoader().getResource("accumulo2.properties");
+ SiteConfiguration conf = new SiteConfiguration(propsUrl);
+ assertEquals("myhost123:2181", conf.get(Property.INSTANCE_ZK_HOST));
+ assertEquals("mysecret", conf.get(Property.INSTANCE_SECRET));
+ assertEquals("hdfs://localhost:8020/accumulo123", conf.get(Property.INSTANCE_VOLUMES));
+ assertEquals("123s", conf.get(Property.GENERAL_RPC_TIMEOUT));
+ assertEquals("256M", conf.get(Property.TSERV_WALOG_MAX_SIZE));
+ assertEquals("DummyService", conf.get(Property.INSTANCE_CRYPTO_SERVICE));
}
@Test
diff --git a/core/src/test/resources/accumulo2.properties b/core/src/test/resources/accumulo2.properties
new file mode 100644
index 0000000000..71de2bdbfa
--- /dev/null
+++ b/core/src/test/resources/accumulo2.properties
@@ -0,0 +1,23 @@
+# 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.
+
+general.rpc.timeout=123s
+instance.crypto.service=DummyService
+instance.secret=mysecret
+instance.volumes=hdfs://localhost:8020/accumulo123
+instance.zookeeper.host=myhost123:2181
+table.durability=flush
+tserver.memory.maps.native.enabled=false
+tserver.walog.max.size=256M
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services