[native] Add support for env vars providing property values #23880

czentgr · 2024-10-24T02:37:47Z

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.

Description

This change allows reading of a config value that references an environment variable that contains the value as opposed to providing the value directly in the config file.

Motivation and Context

This can be used if certain values should not be hard coded, e.g. keys in configuration files.

Impact

User can provide config values using environment variables and refer to the environment variable name as the value in the configuration file.

Test Plan

Added unit test.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Prestissimo
* Add utilizing environment variables as stand in for configuration values. The environment variable value is retrieved and used for the configuration option. :pr:`23880`

czentgr · 2024-10-24T02:43:04Z

TODo add documentation in the PrestoC++ section.

czentgr · 2024-11-08T20:10:52Z

@majetideepak FYI

czentgr · 2024-11-26T03:38:58Z

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp

+  std::optional<std::string> value{std::nullopt};
+  if (str.size() >= 3 && str.substr(0, 2) == "${" &&
+      str[str.size() - 1] == '}') {
+    auto env_name = std::string(str.substr(2, str.size() - 3));


@majetideepak we discussed using string_view here but we need a null terminated char* so we need to construct something from the substring string_view to pass to getenv.

Thanks for clarifying. Maybe add a comment for this to ensure someone else does not make this change.

majetideepak · 2024-11-26T11:33:20Z

presto-docs/src/main/sphinx/presto_cpp/properties.rst

@@ -333,3 +333,31 @@ 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


Title should be camel case.

majetideepak · 2024-11-26T11:33:59Z

presto-docs/src/main/sphinx/presto_cpp/properties.rst

+The environment variable name must match exactly with the defined variable.
+
+This allows a worker to read sensitive data e.g. for access keys from an
+environment variable rather than having the actual value hard coded in a comnfiguration


typo: configuration

majetideepak · 2024-11-26T11:34:31Z

presto-docs/src/main/sphinx/presto_cpp/properties.rst

+
+This allows a worker to read sensitive data e.g. for access keys from an
+environment variable rather than having the actual value hard coded in a comnfiguration
+file on disk enhancing the security stance of deployments.


nit: remove stance

majetideepak · 2024-11-26T11:36:31Z

presto-docs/src/main/sphinx/presto_cpp/properties.rst

+file on disk enhancing the security stance 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, for example,


May also add to this the real usage of how these key values can be stored encrypted on disk and are decrypted during deployment as passed as environment variables.

majetideepak · 2024-11-26T11:38:23Z

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp

+std::optional<std::string> extractValueIfEnvironmentVariable(
+    std::string_view str) {
+  std::optional<std::string> value{std::nullopt};
+  if (str.size() >= 3 && str.substr(0, 2) == "${" &&


nit: str.size() > 3. Let's skip empty ${} as well.

majetideepak · 2024-11-26T11:45:42Z

presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp

+  std::string ENV_VAR = "PRESTO_READ_CONFIG_TEST_VAR";
+
+  writeConfigFile(
+      fmt::format("{}={}\n", "plain-text", "plain-text-value") +


use constants for "plain-text" and other keys to be consistent when reading them below.

majetideepak · 2024-11-26T11:46:43Z

presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp

+      fmt::format("{}={}}}\n", "env-var3", ENV_VAR) +
+      fmt::format("{}=${{}}\n", "no-env-var"));
+
+  setenv(ENV_VAR.c_str(), "env-var-value", 1);


Add a test for empty value. It should be enough to check that it does not introduce a crash.

majetideepak · 2024-11-26T11:47:55Z

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp

+  std::optional<std::string> value{std::nullopt};
+  if (str.size() >= 3 && str.substr(0, 2) == "${" &&
+      str[str.size() - 1] == '}') {
+    auto env_name = std::string(str.substr(2, str.size() - 3));


Thanks for clarifying. Maybe add a comment for this to ensure someone else does not make this change.

majetideepak · 2024-11-26T11:49:59Z

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp

+      str[str.size() - 1] == '}') {
+    auto env_name = std::string(str.substr(2, str.size() - 3));
+
+    char* envval = std::getenv(env_name.c_str());


nit: const char*

majetideepak · 2024-11-26T11:56:41Z

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp

@@ -45,7 +70,9 @@ std::unordered_map<std::string, std::string> readConfig(
    const auto name = line.substr(0, delimiterPos);
    VELOX_CHECK(!name.empty(), "property pair '{}' has empty key", line);
    const auto value = line.substr(delimiterPos + 1);
-    properties.emplace(name, value);
+    const auto adjustedValue = extractValueIfEnvironmentVariable(value);
+    adjustedValue.has_value() ? properties.emplace(name, adjustedValue.value())


During our sync, I misunderstood that we are redundantly updating non-env values.
How about we use void extractValueIfEnvironmentVariable(std::string& value)?
We can then use

extractValueIfEnvironmentVariable(value); properties.emplace(name, value);

Yes. We used to copy it around if the value was a real value and not envvar.
This approach also works and might be a bit cleaner.

majetideepak · 2024-11-27T16:41:46Z

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp

+// 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 env_name = value.substr(2, value.size() - 3);


majetideepak · 2024-11-27T16:41:59Z

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp

+  if (value.size() > 3 && value.substr(0, 2) == "${" && value.back() == '}') {
+    auto env_name = value.substr(2, value.size() - 3);
+
+    const char* envval = std::getenv(env_name.c_str());


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 <[email protected]>

czentgr force-pushed the cz_read_env_var_config_values branch from 4a0e096 to 18373b8 Compare October 24, 2024 02:46

czentgr marked this pull request as ready for review October 31, 2024 20:49

czentgr requested a review from a team as a code owner October 31, 2024 20:49

czentgr requested a review from majetideepak November 8, 2024 20:10

czentgr force-pushed the cz_read_env_var_config_values branch from 18373b8 to d7d834c Compare November 14, 2024 23:34

czentgr force-pushed the cz_read_env_var_config_values branch from d7d834c to 9fc8fbb Compare November 26, 2024 03:37

czentgr requested review from steveburnett, elharo and a team as code owners November 26, 2024 03:37

czentgr requested a review from presto-oss November 26, 2024 03:37

czentgr commented Nov 26, 2024

View reviewed changes

majetideepak reviewed Nov 26, 2024

View reviewed changes

czentgr force-pushed the cz_read_env_var_config_values branch 2 times, most recently from cfed7b5 to aa87ffd Compare November 27, 2024 00:36

majetideepak reviewed Nov 27, 2024

View reviewed changes

czentgr force-pushed the cz_read_env_var_config_values branch from aa87ffd to fbeee81 Compare November 27, 2024 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Add support for env vars providing property values #23880

[native] Add support for env vars providing property values #23880

czentgr commented Oct 24, 2024 •

edited

Loading

czentgr commented Oct 24, 2024

czentgr commented Nov 8, 2024

czentgr Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

majetideepak Nov 26, 2024

czentgr Nov 26, 2024

majetideepak Nov 27, 2024

czentgr Nov 27, 2024

majetideepak Nov 27, 2024

[native] Add support for env vars providing property values #23880

Are you sure you want to change the base?

[native] Add support for env vars providing property values #23880

Conversation

czentgr commented Oct 24, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

czentgr commented Oct 24, 2024

czentgr commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr commented Oct 24, 2024 •

edited

Loading