accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mwa...@apache.org
Subject [accumulo] branch master updated: Refactored SiteConfiguration (#841)
Date Wed, 19 Dec 2018 19:52:00 GMT
This is an automated email from the ASF dual-hosted git repository.

mwalch pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/accumulo.git


The following commit(s) were added to refs/heads/master by this push:
     new 90dfad3  Refactored SiteConfiguration (#841)
90dfad3 is described below

commit 90dfad3b341a5098113ea8fdce28a0bc4511d067
Author: Mike Walch <mwalch@apache.org>
AuthorDate: Wed Dec 19 14:51:56 2018 -0500

    Refactored SiteConfiguration (#841)
    
    * Constructor now creates ImmutableMap
    * Improved unit test
---
 .../core/clientImpl/ClientConfConverter.java       |  21 +-
 .../core/conf/CredentialProviderFactoryShim.java   |   7 +-
 .../org/apache/accumulo/core/conf/Property.java    |  12 --
 .../accumulo/core/conf/SiteConfiguration.java      | 217 +++++----------------
 .../accumulo/core/conf/SiteConfigurationTest.java  |  51 ++---
 core/src/test/resources/accumulo2.properties       |  23 +++
 6 files changed, 113 insertions(+), 218 deletions(-)

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 bb55d09..55372f3 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 class ClientConfConverter {
         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 c8ef29e..33e9ab6 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 class CredentialProviderFactoryShim {
    * @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 e20b7ef..dc759a3 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 enum Property {
             || 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 df46dc2..96e6d85 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.URI;
 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 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
  * 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 class SiteConfiguration extends AccumuloConfiguration {
     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 class SiteConfiguration extends AccumuloConfiguration {
     }
   }
 
-  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 class SiteConfiguration extends AccumuloConfiguration {
     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 c017b37..ee412f5 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.io.File;
 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 class SiteConfigurationTest {
       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 0000000..71de2bd
--- /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


Mime
View raw message