Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(unit_test,userspace): allow env var to get expanded in yaml even when part of a string #2918

Merged
merged 4 commits into from
Dec 13, 2023
Merged

chore(unit_test,userspace): allow env var to get expanded in yaml even when part of a string #2918

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00343d4
chore(unit_test,userspace): allow env var to get expanded in yaml eve…
FedeDP Nov 20, 2023
a7f2693
fix(userspace,unit_tests): fixed bool parsing.
FedeDP Nov 20, 2023
0977f98
chore(unit_tests): added tests around empty config value resolving to…
FedeDP Nov 27, 2023
76274a3
new(unit_tests,userspace): properly support env var expansions in all…
FedeDP Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 78 additions & 20 deletions unit_tests/falco/test_configuration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,22 @@ TEST(Configuration, configuration_environment_variables)
// Set an environment variable for testing purposes
std::string env_var_value = "envVarValue";
std::string env_var_name = "ENV_VAR";
std::string default_value = "default";
SET_ENV_VAR(env_var_name.c_str(), env_var_value.c_str());
yaml_helper conf;

std::string sample_yaml =
std::string embedded_env_var_value = "${ENV_VAR}";
std::string embedded_env_var_name = "ENV_VAR_EMBEDDED";
SET_ENV_VAR(embedded_env_var_name.c_str(), embedded_env_var_value.c_str());

std::string bool_env_var_value = "true";
std::string bool_env_var_name = "ENV_VAR_BOOL";
SET_ENV_VAR(bool_env_var_name.c_str(), bool_env_var_value.c_str());

std::string int_env_var_value = "12";
std::string int_env_var_name = "ENV_VAR_INT";
SET_ENV_VAR(int_env_var_name.c_str(), int_env_var_value.c_str());

std::string default_value = "default";
std::string env_var_sample_yaml =
"base_value:\n"
" id: $ENV_VAR\n"
" name: '${ENV_VAR}'\n"
Expand All @@ -133,52 +144,99 @@ TEST(Configuration, configuration_environment_variables)
" sample_list:\n"
" - ${ENV_VAR}\n"
" - ' ${ENV_VAR}'\n"
" - $UNSED_XX_X_X_VAR\n";
conf.load_from_string(sample_yaml);
" - '${ENV_VAR} '\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests were expanded:

  • new case with whitespace AFTER the expanded string. Moreover, since we now support env variables embedded inside bigger strings, both ${ENV_VAR} and ${ENV_VAR} are now expanded
  • we do now tests that env variables found in the middle of strings are properly expanded (paths[0,1,2,3] cases)
  • paths[4] checks an even weirder case, ie: when the env variable gets expanded to another env variable. It must get resolved twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We check that plugins config (that is getted as a sequence) is still correctly expanded.

" - $UNSED_XX_X_X_VAR\n"
"paths:\n"
" - ${ENV_VAR}/foo\n"
" - $ENV_VAR/foo\n"
" - /foo/${ENV_VAR}/\n"
" - /${ENV_VAR}/${ENV_VAR}${ENV_VAR}/foo\n"
" - ${ENV_VAR_EMBEDDED}/foo\n"
"is_test: ${ENV_VAR_BOOL}\n"
"num_test: ${ENV_VAR_INT}\n";

yaml_helper conf;
conf.load_from_string(env_var_sample_yaml);

/* Check if the base values are defined */
ASSERT_TRUE(conf.is_defined("base_value"));
ASSERT_TRUE(conf.is_defined("base_value_2"));
ASSERT_TRUE(conf.is_defined("paths"));
ASSERT_FALSE(conf.is_defined("unknown_base_value"));

/* Test fetching of a regular string without any environment variable */
std::string base_value_string = conf.get_scalar<std::string>("base_value.string", default_value);
auto base_value_string = conf.get_scalar<std::string>("base_value.string", default_value);
ASSERT_EQ(base_value_string, "my_string");

/* Test fetching of escaped environment variable format. Should return the string as-is after stripping the leading `$` */
std::string base_value_invalid = conf.get_scalar<std::string>("base_value.invalid", default_value);
auto base_value_invalid = conf.get_scalar<std::string>("base_value.invalid", default_value);
ASSERT_EQ(base_value_invalid, "${ENV_VAR}");

/* Test fetching of invalid escaped environment variable format. Should return the string as-is */
std::string base_value_invalid_env = conf.get_scalar<std::string>("base_value.invalid_env", default_value);
/* Test fetching of invalid escaped environment variable format. Should return the string as-is */
auto base_value_invalid_env = conf.get_scalar<std::string>("base_value.invalid_env", default_value);
ASSERT_EQ(base_value_invalid_env, "$$ENV_VAR");

/* Test fetching of strings that contain environment variables */
std::string base_value_id = conf.get_scalar<std::string>("base_value.id", default_value);
auto base_value_id = conf.get_scalar<std::string>("base_value.id", default_value);
ASSERT_EQ(base_value_id, "$ENV_VAR"); // Does not follow the `${VAR}` format, so it should be treated as a regular string

std::string base_value_name = conf.get_scalar<std::string>("base_value.name", default_value);
auto base_value_name = conf.get_scalar<std::string>("base_value.name", default_value);
ASSERT_EQ(base_value_name, env_var_value); // Proper environment variable format

std::string base_value_escaped = conf.get_scalar<std::string>("base_value.escaped", default_value);
auto base_value_escaped = conf.get_scalar<std::string>("base_value.escaped", default_value);
ASSERT_EQ(base_value_escaped, env_var_value); // Environment variable within quotes

/* Test fetching of an undefined environment variable. Expected to return the default value.*/
std::string unknown_boolean = conf.get_scalar<std::string>("base_value.subvalue.subvalue2.boolean", default_value);
auto unknown_boolean = conf.get_scalar<std::string>("base_value.subvalue.subvalue2.boolean", default_value);
ASSERT_EQ(unknown_boolean, default_value);

/* Test fetching of environment variables from a list */
std::string base_value_2_list_0 = conf.get_scalar<std::string>("base_value_2.sample_list[0]", default_value);
auto base_value_2_list_0 = conf.get_scalar<std::string>("base_value_2.sample_list[0]", default_value);
ASSERT_EQ(base_value_2_list_0, env_var_value); // Proper environment variable format

std::string base_value_2_list_1 = conf.get_scalar<std::string>("base_value_2.sample_list[1]", default_value);
ASSERT_EQ(base_value_2_list_1, " ${ENV_VAR}"); // Environment variable preceded by a space, hence treated as a regular string
auto base_value_2_list_1 = conf.get_scalar<std::string>("base_value_2.sample_list[1]", default_value);
ASSERT_EQ(base_value_2_list_1, " " + env_var_value); // Environment variable preceded by a space, still extracted env var with leading space

std::string base_value_2_list_2 = conf.get_scalar<std::string>("base_value_2.sample_list[2]", default_value);
ASSERT_EQ(base_value_2_list_2, "$UNSED_XX_X_X_VAR"); // Does not follow the `${VAR}` format, so should be treated as a regular string
auto base_value_2_list_2 = conf.get_scalar<std::string>("base_value_2.sample_list[2]", default_value);
ASSERT_EQ(base_value_2_list_2, env_var_value + " "); // Environment variable proceeded by a space, still extracted env var with trailing space
FedeDP marked this conversation as resolved.
Show resolved Hide resolved

/* Clear the set environment variable after testing */
SET_ENV_VAR(env_var_name.c_str(), env_var_value.c_str());
auto base_value_2_list_3 = conf.get_scalar<std::string>("base_value_2.sample_list[3]", default_value);
ASSERT_EQ(base_value_2_list_3, "$UNSED_XX_X_X_VAR"); // Does not follow the `${VAR}` format, so should be treated as a regular string

/* Test expansion of environment variables within strings */
auto path_list_0 = conf.get_scalar<std::string>("paths[0]", default_value);
ASSERT_EQ(path_list_0, env_var_value + "/foo"); // Even if env var is part of bigger string, it gets expanded

auto path_list_1 = conf.get_scalar<std::string>("paths[1]", default_value);
ASSERT_EQ(path_list_1, "$ENV_VAR/foo"); // Does not follow the `${VAR}` format, so should be treated as a regular string

auto path_list_2 = conf.get_scalar<std::string>("paths[2]", default_value);
ASSERT_EQ(path_list_2, "/foo/" + env_var_value + "/"); // Even when env var is in the middle of a string. it gets expanded

auto path_list_3 = conf.get_scalar<std::string>("paths[3]", default_value);
ASSERT_EQ(path_list_3, "/" + env_var_value + "/" + env_var_value + env_var_value + "/foo"); // Even when the string contains multiple env vars they are correctly expanded

auto path_list_4 = conf.get_scalar<std::string>("paths[4]", default_value);
ASSERT_EQ(path_list_4, env_var_value + "/foo"); // Even when the env var contains another env var, it gets correctly double-expanded

/* Check that variable expansion is type-aware */
auto boolean = conf.get_scalar<bool>("is_test", false);
ASSERT_EQ(boolean, true); // `true` can be parsed to bool.

auto boolean_as_str = conf.get_scalar<std::string>("is_test", "false");
ASSERT_EQ(boolean_as_str, "true"); // `true` can be parsed to string.

auto boolean_as_int = conf.get_scalar<int32_t>("is_test", 0);
ASSERT_EQ(boolean_as_int, 0); // `true` cannot be parsed to integer.

auto integer = conf.get_scalar<int32_t>("num_test", -1);
ASSERT_EQ(integer, 12);

/* Clear the set environment variables after testing */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just unsetenv?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On windows we should map unsetenv to _putenv(string,"") anyway, so i decided to avoid adding a new macro and just use what i got :)

SET_ENV_VAR(env_var_name.c_str(), "");
SET_ENV_VAR(embedded_env_var_name.c_str(), "");
SET_ENV_VAR(bool_env_var_name.c_str(), "");
SET_ENV_VAR(int_env_var_name.c_str(), "");
}

TEST(Configuration, configuration_webserver_ip)
Expand Down
84 changes: 60 additions & 24 deletions userspace/falco/yaml_helper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ limitations under the License.
#include <set>
#include <iostream>
#include <fstream>
#include <type_traits>

#include "config_falco.h"

Expand Down Expand Up @@ -77,42 +78,77 @@ class yaml_helper
get_node(node, key);
if(node.IsDefined())
{
std::string value = node.as<std::string>();
auto value = node.as<std::string>();

// Helper function to convert string to the desired type T
auto convert_str_to_t = [&default_value](const std::string& str) -> T {
if (str.empty())
Copy link
Contributor Author

@FedeDP FedeDP Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short-circuit, when empty value is passed, just return default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for it? I may have overlooked it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will double check and eventually add one!

{
return default_value;
}

if constexpr (std::is_same_v<T, std::string>)
{
return str;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For strings, just return it.
This workarounds the fact that stringstream splits values by whitespaces, and this is something we don't want to be done when converting string->string.

}
std::stringstream ss(str);
T result;
if (ss >> result) return result;
if (ss >> std::boolalpha >> result) return result;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enforce std::boolalpha so that true and false are correctly parsed to booleans.

return default_value;
};

// If the value starts with `$$`, check for a subsequent `{...}`
if (value.size() >= 3 && value[0] == '$' && value[1] == '$')
auto start_pos = value.find('$');
while (start_pos != std::string::npos)
{
// If after stripping the first `$`, the string format is like `${VAR}`, treat it as a plain string and don't resolve.
if (value[2] == '{' && value[value.size() - 1] == '}')
auto substr = value.substr(start_pos);
// Case 1 -> ${}
if (substr.rfind("${", 0) == 0)
{
value = value.substr(1);
return convert_str_to_t(value);
auto end_pos = substr.find('}');
if (end_pos != std::string::npos)
{
// Eat "${" and "}" when getting the env var name
auto env_str = substr.substr(2, end_pos - 2);
const char* env_value = std::getenv(env_str.c_str()); // Get the environment variable value
if(env_value)
{
// env variable name + "${}"
value.replace(start_pos, env_str.length() + 3, env_value);
}
else
{
value.erase(start_pos, env_str.length() + 3);
}
}
else
{
// There are no "}" chars anymore; just break leaving rest of value untouched.
break;
}
}
else return convert_str_to_t(value);
}

// Check if the value is an environment variable reference
if(value.size() >= 2 && value[0] == '$' && value[1] == '{' && value[value.size() - 1] == '}')
{
// Format: ${ENV_VAR_NAME}
std::string env_var = value.substr(2, value.size() - 3);

const char* env_value = std::getenv(env_var.c_str()); // Get the environment variable value
if(env_value) return convert_str_to_t(env_value);

return default_value;
// Case 2 -> $${}
else if (substr.rfind("$${", 0) == 0)
{
auto end_pos = substr.find('}');
if (end_pos != std::string::npos)
{
// Consume first "$" token
value.erase(start_pos, 1);
}
else
{
// There are no "}" chars anymore; just break leaving rest of value untouched.
break;
}
start_pos++; // consume the second '$' token
}
else
{
start_pos += substr.length();
}
start_pos = value.find("$", start_pos);
}

// If it's not an environment variable reference, return the value as is
return node.as<T>();
return convert_str_to_t(value);
}

return default_value;
Expand Down
Loading