feat: support parsing of system environment variables in yaml #2562

therealdwright · 2023-05-22T07:01:11Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area engine
/area tests

What this PR does / why we need it:

In order to allow the user to supply environment variables in standard
ways performed in other applications the get_scalar function has been
extended to support defining an environment variable in the format
${FOO}. Environment variables can be escaped via defining as $${FOO}.
As this handles some additional complexity, a unit test has been added
to cover this new functionality

Which issue(s) this PR fixes:
Fixes #2561

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

feat: support parsing of system environment variables in yaml

poiana · 2023-05-22T07:01:19Z

Welcome @therealdwright! It looks like this is your first PR to falcosecurity/falco 🎉

jasondellaluce · 2023-05-22T07:25:27Z

This is interesting! Thanks a lot. I'm gonna set this to the current release milestone just to not lose track of it, however keep in mind that we're very close to a Falco release and getting this into it will require a decision among the Falco maintainers and the release managers.

/milestone 0.35.0

cc @falcosecurity/falco-maintainers

FedeDP · 2023-05-22T07:30:57Z

I love this! 👍 from me for the 0.35 milestone.

FedeDP

/approve

poiana · 2023-05-22T13:46:49Z

LGTM label has been added.

Git tree hash: db4fea09ce034163dfa7815ec098655c97a36b58

leogr

I love this feature. However, I would like more time for testing and thinking about corner cases. For example:

How can I escape from it if I wanted the literal $SOMETHING as a string (instead of the env var value)? 🤔
How this feature interact with the program_output which is a shell code? 🤔
Are any other possible side effects? 🤔

For the reasons above and since we are a few days from releasing Falco 0.35, I propose pushing this to Falco 0.36. Let's discuss it!

therealdwright · 2023-05-23T08:53:23Z

How can I escape from it if I wanted the literal $SOMETHING as a string (instead of the env var value)? 🤔

In most other applications, you would quote the string e.g. "$SOMETHING" . However, This has not something I have come across in practice. This implementation looks for a { at position 1, and a } at position n - 1, so it only handles unquoted environment strings, consistent with other OSS implementations (e.g. filebeat).

How this feature interact with the program_output which is a shell code? 🤔

The above response covers this somewhat, but I also tested the original behaviour and ran regression tests while developing the change. Does the regression suite cover such cases?

In terms of timeline, I'll leave it to the maintainers, I selfishly want this sooner rather than later as not being able to define environment variables leads to one of two consequences for my organisation.

We have to encrypt the entire configuration file with something like sops if we want to use the current implementation as is.
We must write some custom tooling to extend the upstream Falco image to iterate through the YAML and eval all environment variables.

Neither of these are the direction we want to take, but if we have to, so be it.

Edit: please let me know if you want additional unit testing to cover such cases, I would be happy to do so.

leogr · 2023-05-23T09:31:52Z

How can I escape from it if I wanted the literal $SOMETHING as a string (instead of the env var value)? thinking

In most other applications, you would quote the string e.g. "$SOMETHING" . However, This has not something I have come across in practice. This implementation looks for a { at position 1, and a } at position n - 1, so it only handles unquoted environment strings, consistent with other OSS implementations (e.g. filebeat).

How this feature interact with the program_output which is a shell code? thinking

The above response covers this somewhat, but I also tested the original behaviour and ran regression tests while developing the change. Does the regression suite cover such cases?

AFAIK, no.

The filebeat example is interesting. I have never used it so, but their docs report:

If you need to use a special character in your configuration file, use $ to escape the expansion. For example, you can escape ${ or } with $${ or $}.

Anyway, I'm not saying this is necessarily an issue - I just don't know yet and I couldn't find enough time to iterate on that 😅

In terms of timeline, I'll leave it to the maintainers, I selfishly want this sooner rather than later as not being able to define environment variables leads to one of two consequences for my organisation.

👍 I'm open to any decision, just the timeline is unfortunate. Let's see other maintainers' opinions.

We have to encrypt the entire configuration file with something like sops if we want to use the current implementation as is.

We must write some custom tooling to extend the upstream Falco image to iterate through the YAML and eval all environment variables.

Neither of these are the direction we want to take, but if we have to, so be it.

Out of curiosity: have you ever tried using --option as a workaround? For example:

falco ... --option someopt=$SOMEVAR

N.B. I'm not saying this is a clean solution.

therealdwright · 2023-05-23T09:53:30Z

Let me know if you would like this PR extended to handle the special case scenario similarly to filebeat. I'm happy to update the PR.

Out of curiosity: have you ever tried using --option as a workaround? For example:

I wasn't aware this was an option; I'll take a look tomorrow and give it a try.

therealdwright · 2023-05-25T02:59:43Z

Unfortunately, moving forward with http_output configured as an option did the job of getting the config in but resulted in a new bug which has been raised as #2579

FedeDP · 2023-05-29T09:08:52Z

I'd move this to 0.36.0 since we have not reach an agreement for this to get merged in time for 0.35.
I am sorry! We will review it asap right after 0.36.0

/milestone 0.36.0

FedeDP · 2023-05-30T09:44:01Z

/hold

therealdwright · 2023-09-05T12:04:17Z

OK @Andreagit97 @leogr and @FedeDP

This has taken me a bit to get around as I have not thought about or touched this codebase for three months, and each piece of feedback in isolation was reasonable, but there were some conflicts to resolve to appease it all. I think I have struck a good balance here.

Only one format of an environment variable is now supported, being ${ENV_VAR}
YAML values following a $ENV_VAR format will be processed as a regular string
YAML Values following a $${ENV_VAR} format will be processed as a regular string with the first $ stripped
YAML values following a $$ENV_VAR format will be processed as a regular string

I think I have adequately covered the various edge cases in the unit test, as I hope you can appreciate there are a lot of edge cases to check for here. Please re-review and let me know what you think. I would appreciate it if the team could come back to me soon as it's very hard to step away from this project and C++ for so long and be productive.

If you would like me to write some user-facing docs, LMK, I'd be happy to do so.

Andreagit97

Amazing work thank you! left just a minor comment :)

userspace/falco/yaml_helper.h

unit_tests/falco/test_configuration.cpp

Andreagit97 · 2023-09-05T14:50:01Z

Only one format of an environment variable is now supported, being ${ENV_VAR}

YAML values following a $ENV_VAR format will be processed as a regular string

YAML Values following a $${ENV_VAR} format will be processed as a regular string with the first $ stripped

YAML values following a $$ENV_VAR format will be processed as a regular string

Agree with the design, thanks!

FedeDP

LGTM! Agree with @Andreagit97 comments, then we are good to go!
Thanks for the effort @therealdwright !

leogr · 2023-09-05T15:13:11Z

LGTM! Agree with @Andreagit97 comments, then we are good to go!
Thanks for the effort @therealdwright !

+1 for me too! Thank you!

FedeDP

/approve

poiana · 2023-09-05T20:26:44Z

LGTM label has been added.

Git tree hash: 304a49cfa14a173eb0e94edcbd448caf86039aae

In order to allow the user to supply environment variables in standard ways performed in other applications the get_scalar function has been extended to support defining an environment variable in the format `${FOO}`. Environment variables can be escaped via defining as `$${FOO}`. As this handles some additional complexity, a unit test has been added to cover this new functionality Signed-off-by: Daniel Wright <[email protected]>

therealdwright · 2023-09-05T20:32:05Z

For posterity, PR description and commit message body have been updated to reflect the new state of the change.

Andreagit97

Thanks!

/approve

poiana · 2023-09-06T08:37:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, therealdwright

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Andreagit97,FedeDP]

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

FedeDP · 2023-09-06T09:44:14Z

/unhold

FedeDP · 2023-09-06T09:44:39Z

Let's go with this one! Again, thank you very much for your effort 🚀

poiana added release-note kind/feature dco-signoff: no area/engine area/tests labels May 22, 2023

poiana requested review from jasondellaluce and Kaizhe May 22, 2023 07:01

poiana added the size/L label May 22, 2023

therealdwright force-pushed the falco-config-yaml-env-var-support branch from fafb60f to 78b2efd Compare May 22, 2023 07:02

poiana added dco-signoff: yes and removed dco-signoff: no labels May 22, 2023

poiana added this to the 0.35.0 milestone May 22, 2023

FedeDP previously approved these changes May 22, 2023

View reviewed changes

poiana assigned FedeDP May 22, 2023

poiana added the lgtm label May 22, 2023

poiana added the approved label May 22, 2023

leogr reviewed May 23, 2023

View reviewed changes

therealdwright mentioned this pull request May 25, 2023

http_output results in libcurl error: URL using bad/illegal format or missing URL #2579

Closed

poiana modified the milestones: 0.35.0, 0.36.0 May 29, 2023

therealdwright force-pushed the falco-config-yaml-env-var-support branch 5 times, most recently from c62db2f to 13ab729 Compare September 5, 2023 11:57

therealdwright force-pushed the falco-config-yaml-env-var-support branch 2 times, most recently from 33e7c6f to 4f2babc Compare September 5, 2023 12:20

Andreagit97 reviewed Sep 5, 2023

View reviewed changes

userspace/falco/yaml_helper.h Outdated Show resolved Hide resolved

unit_tests/falco/test_configuration.cpp Outdated Show resolved Hide resolved

FedeDP reviewed Sep 5, 2023

View reviewed changes

therealdwright force-pushed the falco-config-yaml-env-var-support branch from 4f2babc to 396c53d Compare September 5, 2023 20:24

FedeDP approved these changes Sep 5, 2023

View reviewed changes

poiana added the lgtm label Sep 5, 2023

therealdwright force-pushed the falco-config-yaml-env-var-support branch from 396c53d to 53d71a6 Compare September 5, 2023 20:30

therealdwright force-pushed the falco-config-yaml-env-var-support branch from 53d71a6 to 62a664b Compare September 5, 2023 20:31

Andreagit97 approved these changes Sep 6, 2023

View reviewed changes

poiana assigned Andreagit97 Sep 6, 2023

poiana removed the do-not-merge/hold label Sep 6, 2023

poiana merged commit 513f122 into falcosecurity:master Sep 6, 2023

therealdwright deleted the falco-config-yaml-env-var-support branch September 6, 2023 10:29

FedeDP mentioned this pull request Nov 20, 2023

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

Merged

FedeDP mentioned this pull request Dec 13, 2023

[TRACKING] Breaking changes in Falco 0.37.0 #2763

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support parsing of system environment variables in yaml #2562

feat: support parsing of system environment variables in yaml #2562

therealdwright commented May 22, 2023 •

edited

Loading

poiana commented May 22, 2023

jasondellaluce commented May 22, 2023

FedeDP commented May 22, 2023

FedeDP left a comment

poiana commented May 22, 2023

leogr left a comment

therealdwright commented May 23, 2023 •

edited

Loading

leogr commented May 23, 2023 •

edited

Loading

therealdwright commented May 23, 2023

therealdwright commented May 25, 2023

FedeDP commented May 29, 2023

FedeDP commented May 30, 2023

therealdwright commented Sep 5, 2023

Andreagit97 left a comment

Andreagit97 commented Sep 5, 2023

FedeDP left a comment

leogr commented Sep 5, 2023

FedeDP left a comment

poiana commented Sep 5, 2023

therealdwright commented Sep 5, 2023

Andreagit97 left a comment

poiana commented Sep 6, 2023

FedeDP commented Sep 6, 2023

FedeDP commented Sep 6, 2023

feat: support parsing of system environment variables in yaml #2562

feat: support parsing of system environment variables in yaml #2562

Conversation

therealdwright commented May 22, 2023 • edited Loading

poiana commented May 22, 2023

jasondellaluce commented May 22, 2023

FedeDP commented May 22, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented May 22, 2023

leogr left a comment

Choose a reason for hiding this comment

therealdwright commented May 23, 2023 • edited Loading

leogr commented May 23, 2023 • edited Loading

therealdwright commented May 23, 2023

therealdwright commented May 25, 2023

FedeDP commented May 29, 2023

FedeDP commented May 30, 2023

therealdwright commented Sep 5, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

Andreagit97 commented Sep 5, 2023

FedeDP left a comment

Choose a reason for hiding this comment

leogr commented Sep 5, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Sep 5, 2023

therealdwright commented Sep 5, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Sep 6, 2023

FedeDP commented Sep 6, 2023

FedeDP commented Sep 6, 2023

therealdwright commented May 22, 2023 •

edited

Loading

therealdwright commented May 23, 2023 •

edited

Loading

leogr commented May 23, 2023 •

edited

Loading