chore(unit_test,userspace): allow env var to get expanded in yaml even when part of a string by FedeDP · Pull Request #2918 · falcosecurity/falco

FedeDP · 2023-11-20T13:44:05Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

Moreover, support env variable embedding another env variable.

Basically, this PR would allow us to use eg:

engine:
  ebpf:
    probe: /${HOME}/.falco/falco-bpf.o

in #2413.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(userspace): support env variable expansion in all yaml, even inside strings.

…n when part of a string. Moreover, support env variable embedding another env variable. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP · 2023-11-20T13:44:35Z

Expands #2562.

FedeDP · 2023-11-20T13:47:29Z

unit_tests/falco/test_configuration.cpp

        "        - ' ${ENV_VAR}'\n"
-        "        - $UNSED_XX_X_X_VAR\n";
-    conf.load_from_string(sample_yaml);
+	"        - '${ENV_VAR} '\n"


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.

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

FedeDP · 2023-11-20T13:47:49Z

userspace/falco/yaml_helper.h


 			// 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())


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

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

Will double check and eventually add one!

FedeDP · 2023-11-20T13:48:40Z

userspace/falco/yaml_helper.h

+
+				if constexpr (std::is_same_v<T, std::string>)
+				{
+					return str;


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.

FedeDP · 2023-11-20T14:31:19Z

userspace/falco/yaml_helper.h

 				std::stringstream ss(str);
 				T result;
-				if (ss >> result) return result;
+				if (ss >> std::boolalpha >> result) return result;


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

Moreover, added some more tests around env vars. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP · 2023-11-21T09:10:16Z

/cc @jasondellaluce

I am not sure whether we want this for 0.37.0, leave it to other maintainers to decide ;)
Also cc @therealdwright who first opened the original PR!

incertum

/approve

incertum · 2023-11-27T04:09:07Z

unit_tests/falco/test_configuration.cpp

+    auto integer = conf.get_scalar<int32_t>("num_test", -1);
+    ASSERT_EQ(integer, 12);
+
+    /* Clear the set environment variables after testing */


Or just unsetenv?

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

unit_tests/falco/test_configuration.cpp

incertum · 2023-11-27T05:20:10Z

userspace/falco/yaml_helper.h


 			// 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())


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

… default. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP · 2023-11-27T09:37:32Z

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

Added in latest commit, thank you!

incertum

/approve

FedeDP · 2023-11-28T10:10:01Z

Trying to put this in the 0.37.0 milestone; let's see if we can make this happen :D
/milestone 0.37.0

FedeDP · 2023-12-04T12:38:49Z

/hold

FedeDP · 2023-12-05T08:15:58Z

userspace/falco/yaml_helper.h


+class yaml_helper;
+
+class yaml_visitor {


yaml_visitor is a small visitor API for yaml files, to call a callback function on each scalar value.
It is private to yaml_helper since it receives non-constant YAML::Nodes (thus it can modify the parsed yaml structure).

FedeDP · 2023-12-05T08:16:20Z

userspace/falco/yaml_helper.h

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


No more custom logic needed.

BREAKING CHANGE: in #2562 env var that resolved to empty string (both non-set env vars and env vars set to "") returned default value.
This was in constrast with everything else in our yaml impl (ie: we only returned default value if node is not defined).
I reset to the same behavior of all other variables.

BREAKING CHANGE: node.as<T>(default_value); will return default_value when it is not able to parse the node to T.
It makes sense IMHO.

cc @jasondellaluce @leogr

… scalar values of yaml file. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP · 2023-12-05T08:20:00Z

/unhold

Now we support env vars expansion in all getters, since we pre-process the yaml root to expand any scalar value as soon as the document is loaded.

leogr · 2023-12-05T17:41:28Z

/assign

I'll take a look as soon as I can

leogr

As discussed with @FedeDP this PR introduces a minor behavior change that needs to be signaled and documented.

Anyway, the feature is great and the PR LGTM.

@incertum PTAL again

incertum

/approve

poiana · 2023-12-13T16:03:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,incertum,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chore(unit_test,userspace): allow env var to get expanded in yaml eve…

00343d4

…n when part of a string. Moreover, support env variable embedding another env variable. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

poiana added release-note kind/feature dco-signoff: yes area/engine labels Nov 20, 2023

poiana requested review from Kaizhe and LucaGuerra November 20, 2023 13:44

poiana added approved size/L labels Nov 20, 2023

FedeDP commented Nov 20, 2023

View reviewed changes

FedeDP changed the title ~~chore(unit_test,userspace): allow env var to get expanded in yaml even when part of a string~~ wip: chore(unit_test,userspace): allow env var to get expanded in yaml even when part of a string Nov 20, 2023

poiana added the do-not-merge/work-in-progress label Nov 20, 2023

FedeDP commented Nov 20, 2023

View reviewed changes

fix(userspace,unit_tests): fixed bool parsing.

a7f2693

Moreover, added some more tests around env vars. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP force-pushed the new/env_var_expansion branch from 4add0ee to a7f2693 Compare November 20, 2023 14:33

FedeDP changed the title ~~wip: chore(unit_test,userspace): allow env var to get expanded in yaml even when part of a string~~ chore(unit_test,userspace): allow env var to get expanded in yaml even when part of a string Nov 21, 2023

poiana removed the do-not-merge/work-in-progress label Nov 21, 2023

poiana requested a review from jasondellaluce November 21, 2023 09:10

leogr mentioned this pull request Nov 23, 2023

new: driver selection in falco.yaml #2413

Merged

leogr added this to the 0.37.0 milestone Nov 24, 2023

incertum previously approved these changes Nov 27, 2023

View reviewed changes

poiana assigned incertum Nov 27, 2023

poiana added the lgtm label Nov 27, 2023

chore(unit_tests): added tests around empty config value resolving to…

0977f98

… default. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP dismissed incertum’s stale review via 0977f98 November 27, 2023 09:37

incertum previously approved these changes Nov 27, 2023

View reviewed changes

poiana added the do-not-merge/hold label Dec 4, 2023

FedeDP dismissed incertum’s stale review via 8cb1a26 December 5, 2023 08:11

FedeDP force-pushed the new/env_var_expansion branch from 8cb1a26 to cf3bb89 Compare December 5, 2023 08:14

FedeDP commented Dec 5, 2023

View reviewed changes

new(unit_tests,userspace): properly support env var expansions in all…

76274a3

… scalar values of yaml file. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP force-pushed the new/env_var_expansion branch from cf3bb89 to 76274a3 Compare December 5, 2023 08:19

poiana removed the do-not-merge/hold label Dec 5, 2023

poiana assigned leogr Dec 5, 2023

FedeDP mentioned this pull request Dec 13, 2023

[TRACKING] Breaking changes in Falco 0.37.0 #2763

Closed

6 tasks

leogr approved these changes Dec 13, 2023

View reviewed changes

incertum approved these changes Dec 13, 2023

View reviewed changes

poiana merged commit cbbcb61 into master Dec 13, 2023

poiana deleted the new/env_var_expansion branch December 13, 2023 16:03

FedeDP mentioned this pull request Dec 13, 2023

chore(falco.yaml): use HOME env var for ebpf probe path. #2971

Merged


		class yaml_helper;

		class yaml_visitor {

Conversation

FedeDP commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FedeDP commented Nov 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Nov 21, 2023

Uh oh!

incertum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Nov 27, 2023

Uh oh!

incertum left a comment

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Nov 28, 2023

Uh oh!

FedeDP commented Dec 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leogr commented Dec 5, 2023

Uh oh!

leogr left a comment

Choose a reason for hiding this comment

Uh oh!

incertum left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FedeDP commented Nov 20, 2023 •

edited

Loading

FedeDP Nov 20, 2023 •

edited

Loading

FedeDP Dec 5, 2023 •

edited

Loading

FedeDP Dec 5, 2023 •

edited

Loading

FedeDP commented Dec 5, 2023 •

edited

Loading