[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*

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

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);


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

majetideepak

@steveburnett Can you please take another look at the doc changes?
@czentgr this looks great. I like that the new API leaves the existing value as is if the env var is missing.

majetideepak · 2024-12-02T14:30:10Z

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

+    if (envVal != nullptr) {
+      if (strlen(envVal) == 0) {
+        LOG(WARNING) << fmt::format(
+            "Config environment variable {} was empty.", envVal);


This should be envName.

steveburnett

Thank you for the doc! Some nits of formatting and some suggested rephrasings for you to consider.

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

steveburnett · 2024-12-02T15:09:31Z

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

+One mechanism is to create a preload library that is injected at the time
+presto_server is started. It decrypts encrypted secrets and sets environment
+variables specific to the presto_server process. These can then be referenced
+in the properies.


Suggested change

in the properies.

in the properties.

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

steveburnett

LGTM! (docs)

Thanks for the quick revision! Pull updated branch, new local doc build, reviewed new build, looks good.

majetideepak

Thanks, @czentgr

majetideepak · 2024-12-02T18:02:02Z

The presto_serve_test failure could be due to the image update. I will take a look.

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

majetideepak · 2024-12-02T18:25:40Z

The presto_serve_test failure could be due to the image update. I will take a look.

Likely non-deterministic. Can you rerun the test?

steveburnett

LGTM! (docs)

Pulled updated branch, new local doc build, reviewed new build, looks good.

majetideepak

@czentgr please squash the commits.

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 <ashwin.kumar6@ibm.com>

czentgr · 2024-12-03T03:09:45Z

@czentgr please squash the commits.

Done.

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

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp Outdated Show resolved Hide resolved

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

majetideepak reviewed Dec 2, 2024

View reviewed changes

steveburnett requested changes Dec 2, 2024

View reviewed changes

czentgr force-pushed the cz_read_env_var_config_values branch from fbeee81 to 74e6c34 Compare December 2, 2024 16:55

czentgr requested review from steveburnett and majetideepak December 2, 2024 16:56

steveburnett previously approved these changes Dec 2, 2024

View reviewed changes

majetideepak previously approved these changes Dec 2, 2024

View reviewed changes

majetideepak reviewed Dec 2, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/common/ConfigReader.cpp Outdated Show resolved Hide resolved

czentgr dismissed stale reviews from majetideepak and steveburnett via 76a6e2f December 2, 2024 20:40

majetideepak approved these changes Dec 2, 2024

View reviewed changes

steveburnett approved these changes Dec 2, 2024

View reviewed changes

majetideepak requested changes Dec 2, 2024

View reviewed changes

czentgr force-pushed the cz_read_env_var_config_values branch from 76a6e2f to 853778f Compare December 3, 2024 03:08

czentgr requested a review from majetideepak December 3, 2024 03:09

majetideepak approved these changes Dec 3, 2024

View reviewed changes

majetideepak merged commit ca26526 into prestodb:master Dec 3, 2024
61 checks passed

This was referenced Jan 28, 2025

Add release notes for 0.291 #24445

Merged

Add release notes for 0.291 #24448

Closed

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 27, 2024

czentgr Nov 27, 2024

majetideepak left a comment

majetideepak Dec 2, 2024

steveburnett left a comment

steveburnett Dec 2, 2024

steveburnett left a comment

majetideepak left a comment

majetideepak commented Dec 2, 2024

majetideepak commented Dec 2, 2024

steveburnett left a comment

majetideepak left a comment

czentgr commented Dec 3, 2024

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

[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

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

majetideepak commented Dec 2, 2024

majetideepak commented Dec 2, 2024

steveburnett left a comment

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

czentgr commented Dec 3, 2024

czentgr commented Oct 24, 2024 •

edited

Loading