[native] Add support for env vars providing property values

czentgr · ashkrisk · czentgr · commit 853778f471a0 · 2024-12-02T22:08:13.000-05:00
This allows a user to configure the value of an environment
variable to be used as value for a configuration property.
For example, the configuration file may contain keys that
would be hard coded. The values can now be replaced by
environment variables that are read at startup time and
the keys don't require hard coding in the configuration file.

Co-authored-by: ashkrisk &lt;ashwin.kumar6@ibm.com&gt;
diff --git a/presto-docs/src/main/sphinx/presto_cpp/properties.rst b/presto-docs/src/main/sphinx/presto_cpp/properties.rst
@@ -4,9 +4,9 @@ Presto C++ Configuration Properties
 
 This section describes Presto C++ configuration properties.
 
-The following is not a complete list of all configuration properties, 
-and does not include any connector-specific catalog configuration properties 
-or session properties. 
+The following is not a complete list of all configuration properties,
+and does not include any connector-specific catalog configuration properties
+or session properties.
 
 For information on catalog configuration properties, see :doc:`Connectors </connector/>`.
 
@@ -20,9 +20,9 @@ For information on Presto C++ session properties, see :doc:`properties-session`.
 Coordinator Properties
 ----------------------
 
-Set the following configuration properties for the Presto coordinator exactly 
-as they are shown in this code block to enable the Presto coordinator's use of 
-Presto C++ workers. 
+Set the following configuration properties for the Presto coordinator exactly
+as they are shown in this code block to enable the Presto coordinator's use of
+Presto C++ workers.
 
 .. code-block:: none
 
@@ -31,17 +31,17 @@ Presto C++ workers.
     regex-library=RE2J
     use-alternative-function-signatures=true
 
-These Presto coordinator configuration properties are described here, in 
-alphabetical order. 
+These Presto coordinator configuration properties are described here, in
+alphabetical order.
 
 ``driver.cancel-tasks-with-stuck-operators-threshold-ms``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 * **Type:** ``string``
 * **Default value:** ``240000`` (40 minutes)
 
-  Cancels any task when at least one operator has been stuck for at 
+  Cancels any task when at least one operator has been stuck for at
   least the time specified by this threshold.
-  
+
   Set this property to ``0`` to disable canceling.
 
 ``native-execution-enabled``
@@ -50,7 +50,7 @@ alphabetical order.
 * **Type:** ``boolean``
 * **Default value:** ``false``
 
-  This property is required when running Presto C++ workers because of 
+  This property is required when running Presto C++ workers because of
   underlying differences in behavior from Java workers.
 
 ``optimizer.optimize-hash-generation``
@@ -59,9 +59,9 @@ alphabetical order.
 * **Type:** ``boolean``
 * **Default value:** ``true``
 
-  Set this property to ``false`` when running Presto C++ workers.  
-  Velox does not support optimized hash generation, instead using a HashTable 
-  with adaptive runtime optimizations that does not use extra hash fields. 
+  Set this property to ``false`` when running Presto C++ workers.
+  Velox does not support optimized hash generation, instead using a HashTable
+  with adaptive runtime optimizations that does not use extra hash fields.
 
 ``regex-library``
 ^^^^^^^^^^^^^^^^^
@@ -78,17 +78,17 @@ alphabetical order.
 * **Type:** ``boolean``
 * **Default value:** ``false``
 
-  Some aggregation functions use generic intermediate types which are 
-  not compatible with Velox aggregation function intermediate types. One  
-  example function is ``approx_distinct``, whose intermediate type is 
-  ``VARBINARY``. 
-  This property provides function signatures for built-in aggregation 
+  Some aggregation functions use generic intermediate types which are
+  not compatible with Velox aggregation function intermediate types. One
+  example function is ``approx_distinct``, whose intermediate type is
+  ``VARBINARY``.
+  This property provides function signatures for built-in aggregation
   functions which are compatible with Velox.
 
 Worker Properties
 -----------------
 
-The configuration properties of Presto C++ workers are described here, in alphabetical order. 
+The configuration properties of Presto C++ workers are described here, in alphabetical order.
 
 ``async-cache-persistence-interval``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -292,8 +292,8 @@ The configuration properties of Presto C++ workers are described here, in alphab
 Memory Checker Properties
 -------------------------
 
-The LinuxMemoryChecker extends from PeriodicMemoryChecker and is used for Linux systems only. 
-The LinuxMemoryChecker can be enabled by setting the CMake flag ``PRESTO_MEMORY_CHECKER_TYPE=LINUX_MEMORY_CHECKER``. 
+The LinuxMemoryChecker extends from PeriodicMemoryChecker and is used for Linux systems only.
+The LinuxMemoryChecker can be enabled by setting the CMake flag ``PRESTO_MEMORY_CHECKER_TYPE=LINUX_MEMORY_CHECKER``.
 The following properties for PeriodicMemoryChecker are as follows:
 
 ``system-mem-pushback-enabled``
@@ -312,7 +312,7 @@ server is under low memory pressure.
 * **Default value:** ``55``
 
 Specifies the system memory limit that triggers the memory pushback or heap dump if
-the server memory usage is beyond this limit. A value of zero means no limit is set. 
+the server memory usage is beyond this limit. A value of zero means no limit is set.
 This only applies if ``system-mem-pushback-enabled`` is ``true``.
 
 ``system-mem-shrink-gb``
@@ -323,3 +323,34 @@ This only applies if ``system-mem-pushback-enabled`` is ``true``.
 
 Specifies the amount of memory to shrink when the memory pushback is
 triggered. This only applies if ``system-mem-pushback-enabled`` is ``true``.
+
+Environment Variables As Values For Worker Properties
+-----------------------------------------------------
+
+This section applies to worker configurations in the ``config.properties`` file
+and catalog property files only.
+
+The value in a key-value pair can reference an environment variable by using
+a leading `$` followed by enclosing the environment variable name in brackets (`{}`).
+
+``key=${ENV_VAR_NAME}``
+
+The environment variable name must match exactly with the defined variable.
+
+This allows a worker to read sensitive data such as access keys from an
+environment variable rather than having the actual value hard coded in a configuration
+file on disk, improving the security of deployments.
+
+For example, consider the hive connector's ``hive.s3.aws-access-key`` property.
+This is sensitive data and can be stored in an environment variable such as
+``AWS_S3_ACCESS_KEY`` which is set to the actual access key value.
+
+One mechanism is to create a preload library that is injected at the time
+presto_server is started that decrypts encrypted secrets and sets environment
+variables specific to the presto_server process. These can then be referenced
+in the properties.
+
+Once decrypted the preloaded library sets the ``AWS_S3_ACCESS_KEY``
+environment variable which then can be accessed by providing it in the catalog properties:
+
+``hive.s3.aws-access-key=${AWS_S3_ACCESS_KEY}``
diff --git a/presto-native-execution/presto_cpp/main/common/ConfigReader.cpp b/presto-native-execution/presto_cpp/main/common/ConfigReader.cpp
@@ -19,6 +19,27 @@
 #include "velox/common/config/Config.h"
 
 namespace facebook::presto::util {
+
+namespace {
+// Replaces strings of the form "${VAR}"
+// with the value of the environment variable "VAR" (if it exists).
+// Does nothing if the input doesn't look like "${...}".
+void extractValueIfEnvironmentVariable(std::string& value) {
+  if (value.size() > 3 && value.substr(0, 2) == "${" && value.back() == '}') {
+    auto envName = value.substr(2, value.size() - 3);
+
+    const char* envVal = std::getenv(envName.c_str());
+    if (envVal != nullptr) {
+      if (strlen(envVal) == 0) {
+        LOG(WARNING) << fmt::format(
+            "Config environment variable {} is empty.", envName);
+      }
+      value = std::string(envVal);
+    }
+  }
+}
+} // namespace
+
 std::unordered_map<std::string, std::string> readConfig(
     const std::string& filePath) {
   // https://teradata.github.io/presto/docs/141t/configuration/configuration.html
@@ -44,7 +65,8 @@ std::unordered_map<std::string, std::string> readConfig(
         line);
     const auto name = line.substr(0, delimiterPos);
     VELOX_CHECK(!name.empty(), "property pair '{}' has empty key", line);
-    const auto value = line.substr(delimiterPos + 1);
+    auto value = line.substr(delimiterPos + 1);
+    extractValueIfEnvironmentVariable(value);
     properties.emplace(name, value);
   }
 
diff --git a/presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp b/presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp
@@ -25,23 +25,36 @@ using namespace velox;
 
 class ConfigTest : public testing::Test {
  protected:
-  void setUpConfigFilePath() {
+  void SetUp() override {
     velox::filesystems::registerLocalFileSystem();
+    setUpConfigFilePath();
+  }
+
+  void TearDown() override {
+    cleanupConfigFilePath();
+  }
 
+  void setUpConfigFilePath() {
     char path[] = "/tmp/velox_system_config_test_XXXXXX";
     const char* tempDirectoryPath = mkdtemp(path);
     if (tempDirectoryPath == nullptr) {
       throw std::logic_error("Cannot open temp directory");
     }
-    configFilePath = tempDirectoryPath;
-    configFilePath += "/config.properties";
+    configFilePath_ = tempDirectoryPath;
+    configFilePath_ += "/config.properties";
+  }
+
+  void cleanupConfigFilePath() {
+    auto fileSystem = filesystems::getFileSystem(configFilePath_, nullptr);
+    auto dirPath = std::filesystem::path(configFilePath_).parent_path();
+    fileSystem->rmdir(dirPath.string());
   }
 
   void writeDefaultConfigFile(bool isMutable) {
-    auto fileSystem = filesystems::getFileSystem(configFilePath, nullptr);
-    auto sysConfigFile = fileSystem->openFileForWrite(configFilePath);
+    auto fileSystem = filesystems::getFileSystem(configFilePath_, nullptr);
+    auto sysConfigFile = fileSystem->openFileForWrite(configFilePath_);
     sysConfigFile->append(
-        fmt::format("{}={}\n", SystemConfig::kPrestoVersion, prestoVersion));
+        fmt::format("{}={}\n", SystemConfig::kPrestoVersion, kPrestoVersion_));
     sysConfigFile->append(
         fmt::format("{}=11kB\n", SystemConfig::kQueryMaxMemoryPerNode));
     if (isMutable) {
@@ -52,8 +65,8 @@ class ConfigTest : public testing::Test {
   }
 
   void writeConfigFile(const std::string& config) {
-    auto fileSystem = filesystems::getFileSystem(configFilePath, nullptr);
-    auto sysConfigFile = fileSystem->openFileForWrite(configFilePath);
+    auto fileSystem = filesystems::getFileSystem(configFilePath_, nullptr);
+    auto sysConfigFile = fileSystem->openFileForWrite(configFilePath_);
     sysConfigFile->append(config);
     sysConfigFile->close();
   }
@@ -65,40 +78,41 @@ class ConfigTest : public testing::Test {
         std::make_unique<config::ConfigBase>(std::move(properties)));
   }
 
-  std::string configFilePath;
-  const std::string prestoVersion{"SystemConfigTest1"};
-  const std::string prestoVersion2{"SystemConfigTest2"};
+  std::string configFilePath_;
+  const std::string_view kPrestoVersion_{"SystemConfigTest1"};
+  const std::string_view kPrestoVersion2_{"SystemConfigTest2"};
 };
 
 TEST_F(ConfigTest, defaultSystemConfig) {
-  setUpConfigFilePath();
   writeDefaultConfigFile(false);
   auto systemConfig = SystemConfig::instance();
-  systemConfig->initialize(configFilePath);
+  systemConfig->initialize(configFilePath_);
 
   ASSERT_FALSE(systemConfig->mutableConfig());
-  ASSERT_EQ(prestoVersion, systemConfig->prestoVersion());
+  ASSERT_EQ(kPrestoVersion_, systemConfig->prestoVersion());
   ASSERT_EQ(11 << 10, systemConfig->queryMaxMemoryPerNode());
   ASSERT_THROW(
       systemConfig->setValue(
-          std::string(SystemConfig::kPrestoVersion), prestoVersion2),
+          std::string(SystemConfig::kPrestoVersion),
+          std::string(kPrestoVersion2_)),
       VeloxException);
 }
 
 TEST_F(ConfigTest, mutableSystemConfig) {
-  setUpConfigFilePath();
   writeDefaultConfigFile(true);
   auto systemConfig = SystemConfig::instance();
-  systemConfig->initialize(configFilePath);
+  systemConfig->initialize(configFilePath_);
 
   ASSERT_TRUE(systemConfig->mutableConfig());
-  ASSERT_EQ(prestoVersion, systemConfig->prestoVersion());
+  ASSERT_EQ(kPrestoVersion_, systemConfig->prestoVersion());
   ASSERT_EQ(
-      prestoVersion,
+      kPrestoVersion_,
       systemConfig
-          ->setValue(std::string(SystemConfig::kPrestoVersion), prestoVersion2)
+          ->setValue(
+              std::string(SystemConfig::kPrestoVersion),
+              std::string(kPrestoVersion2_))
           .value());
-  ASSERT_EQ(prestoVersion2, systemConfig->prestoVersion());
+  ASSERT_EQ(kPrestoVersion2_, systemConfig->prestoVersion());
   ASSERT_EQ(
       "11kB",
       systemConfig
@@ -206,7 +220,6 @@ TEST_F(ConfigTest, remoteFunctionServer) {
 }
 
 TEST_F(ConfigTest, parseValid) {
-  setUpConfigFilePath();
   writeConfigFile(
       "#comment\n"
       "#a comment with space\n"
@@ -220,7 +233,7 @@ TEST_F(ConfigTest, parseValid) {
       "key1= value with space\n"
       "key2=value=with=key=word\n"
       "emptyvaluekey=");
-  auto configMap = presto::util::readConfig(configFilePath);
+  auto configMap = presto::util::readConfig(configFilePath_);
   ASSERT_EQ(configMap.size(), 6);
 
   std::unordered_map<std::string, std::string> expected{
@@ -237,10 +250,11 @@ TEST_F(ConfigTest, parseInvalid) {
   auto testInvalid = [this](
                          const std::string& fileContent,
                          const std::string& expectedErrorMsg) {
+    cleanupConfigFilePath();
     setUpConfigFilePath();
     writeConfigFile(fileContent);
     VELOX_ASSERT_THROW(
-        presto::util::readConfig(configFilePath), expectedErrorMsg);
+        presto::util::readConfig(configFilePath_), expectedErrorMsg);
   };
   testInvalid(
       "noequalsign\n", "No '=' sign found for property pair 'noequalsign'");
@@ -255,4 +269,45 @@ TEST_F(ConfigTest, optionalNodeId) {
   EXPECT_EQ(nodeId, config.nodeId());
 }
 
+TEST_F(ConfigTest, readConfigEnvVarTest) {
+  const std::string kEnvVarName = "PRESTO_READ_CONFIG_TEST_VAR";
+  const std::string kEmptyEnvVarName = "PRESTO_READ_CONFIG_TEST_EMPTY_VAR";
+
+  const std::string kPlainTextKey = "plain-text";
+  const std::string kPlainTextValue = "plain-text-value";
+
+  const std::string kEnvVarKey = "env-var";
+  const std::string kEnvVarValue = "env-var-value";
+
+  // Keys to test invalid environment variable values.
+  const std::string kEnvVarKey2 = "env-var2";
+  const std::string kEnvVarKey3 = "env-var3";
+  const std::string kNoEnvVarKey = "no-env-var";
+  const std::string kEmptyEnvVarKey = "empty-env-var";
+
+  writeConfigFile(
+      fmt::format("{}={}\n", kPlainTextKey, kPlainTextValue) +
+      fmt::format("{}=${{{}}}\n", kEnvVarKey, kEnvVarName) +
+      fmt::format("{}=${{{}\n", kEnvVarKey2, kEnvVarName) +
+      fmt::format("{}={}}}\n", kEnvVarKey3, kEnvVarName) +
+      fmt::format("{}=${{}}\n", kNoEnvVarKey) +
+      fmt::format("{}=${{{}}}\n", kEmptyEnvVarKey, kEmptyEnvVarName));
+
+  setenv(kEnvVarName.c_str(), kEnvVarValue.c_str(), 1);
+  setenv(kEmptyEnvVarName.c_str(), "", 1);
+
+  auto properties = presto::util::readConfig(configFilePath_);
+  std::unordered_map<std::string, std::string> expected{
+      {kPlainTextKey, kPlainTextValue},
+      {kEnvVarKey, kEnvVarValue},
+      {kEnvVarKey2, "${PRESTO_READ_CONFIG_TEST_VAR"},
+      {kEnvVarKey3, "PRESTO_READ_CONFIG_TEST_VAR}"},
+      {kNoEnvVarKey, "${}"},
+      {kEmptyEnvVarKey, ""}};
+  ASSERT_EQ(properties, expected);
+
+  unsetenv(kEnvVarName.c_str());
+  unsetenv(kEmptyEnvVarName.c_str());
+}
+
 } // namespace facebook::presto::test
