added validation function for config keys #3004

h4l0gen · 2024-01-10T15:40:33Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:
This PR checks for existing configuration keys in falco.yaml.
Which issue(s) this PR fixes:
Emit a warning when trying to set a non-existing config key #2924

Fixes #2924

Special notes for your reviewer:
I'd like to discuss the available configuration options that I added in the fixed.yaml schema.
Does this PR introduce a user-facing change?:

NONE

poiana · 2024-01-10T15:40:43Z

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

Andreagit97 · 2024-01-10T16:23:55Z

ei @h4l0gen thank you for this! We will try to take a look ASAP! We are in the middle of Falco 0.37.0 release so we could have a little delay with the review of new PRs

leogr

Hey,

I quickly looked at this, and it is a great starting point! Thank you.

First thing that came to my mind is: Could we relay on some more formal validation mechanism (like https://json-schema.org/specification) instead of just having fixed.yaml with all default values? 🤔

IIRC, YAML can be validated using a json-schema (cc @jasondellaluce @LucaGuerra )

Also left a comment (see below)

leogr · 2024-01-12T12:15:42Z

userspace/falco/configuration.cpp

+}
+void falco_configuration::load_yaml(const std::string& config_name, const yaml_helper& config)
+{   
+    YAML::Node fixedSchema = YAML::LoadFile("userspace/falco/fixed.yaml");


This can work only when in development mode. A better solution would be to embed the file into the Falco binary so as not to load an external file at runtime.

hey @leogr,
Thanks for reviewing. I will embed file into Falco binary and along with that try to find other validation mechanisms too.

FedeDP · 2024-01-17T07:47:55Z

Could we relay on some more formal validation mechanism (like https://json-schema.org/specification) instead of just having fixed.yaml with all default values?

Fully agree with this one! We already have the valijson dep from libs: https://github.com/falcosecurity/libs/blob/master/cmake/modules/valijson.cmake and is being used by plugins to validate init config schema, if set by the plugin API: https://github.com/falcosecurity/libs/blob/48277b17e1cd906ec9faec7fa45bc9b795e66fc1/userspace/libsinsp/plugin.cpp#L678; i think we can do something similar?

FedeDP · 2024-01-17T07:50:31Z

valijson ships also a yaml-cpp adapter; here is an example: https://github.com/tristanpenman/valijson/blob/master/tests/test_yaml_cpp_adapter.cpp and here is the code: https://github.com/tristanpenman/valijson/blob/master/include/valijson/adapters/yaml_cpp_adapter.hpp

h4l0gen · 2024-01-18T03:55:12Z

Thank you for the suggestions, @leogr and @FedeDP . I am working on it.

h4l0gen · 2024-01-24T18:24:38Z

Hey @Andreagit97 @leogr @FedeDP, I have included Valijson and utilized the YAML-CPP adapter. Please take a look and provide any suggestions or corrections if I have made any mistakes. Thank you!!

leogr · 2024-01-25T09:40:27Z

userspace/falco/configuration.cpp

 #include <re2/re2.h>
-
+#include "valijson/adapters/yaml_cpp_adapters.hpp"
+#include "ValidationUtils.h"


Where does ValidationUtils.h come from? 🤔

Sorry for this, @leogr. I forgot to commit the ValidationUtils.h file 😅. This header file is being used for the validateKeysRecursive function.

Hey @leogr , could you please take a look at this PR so that we can finish our work on it?

h4l0gen · 2024-02-11T05:47:55Z

Hey @leogr, could you please review this PR again? It would be greatly appreciated, and we can then finish our work on this issue.

leogr

Hey @leogr, could you please review this PR again? It would be greatly appreciated, and we can finish our work on this issue.

The valijson version we are pulling via libs is v0.6 (which does not come with the yaml adapter). So, to make this work, we must first try to update valijson in libs (0.7 or later) (@LucaGuerra @FedeDP can you take a look at that?)

@h4l0gen, meanwhile, I suggest you clean up this PR by removing all now unnecessary stuff (e.g., userspace/falco/fixed.yaml and possibly other stuff). Also, rebase on top of the latest master. Thank you!

leogr · 2024-02-13T15:17:26Z

userspace/falco/configuration.cpp

@@ -183,6 +184,45 @@ void falco_configuration::load_engine_config(const std::string& config_name, con
 	m_changes_in_engine_config = true;
 }

+
+std::vector<std::string> fixedSchemaKeysVec = {


Since we are going to use valijson, do we still need this?

leogr · 2024-02-13T15:17:45Z

userspace/falco/fixed.yaml

@@ -0,0 +1,161 @@
+rules_file:


I guess this file is not needed anymore.

leogr · 2024-02-23T14:34:52Z

@h4l0gen

As an example, this is a schema file describing the rules YAML files format: https://github.com/falcosecurity/falco-playground/blob/main/src/components/Editor/falcoSchema.json

We basically need something similar for the falco.yaml config.

h4l0gen · 2024-02-23T17:26:54Z

Okay, thanks @leogr . I will implement this change.

Signed-off-by: h4l0gen <ks3913688@gmail.com>

This reverts commit 1373bb2. Signed-off-by: h4l0gen <ks3913688@gmail.com>

This reverts commit d809cbe. Signed-off-by: h4l0gen <ks3913688@gmail.com>

This reverts commit 9fec747. Signed-off-by: h4l0gen <ks3913688@gmail.com>

poiana · 2024-03-08T18:57:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: h4l0gen
Once this PR has been reviewed and has the lgtm label, please assign jasondellaluce for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

poiana · 2024-03-08T18:57:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: h4l0gen
Once this PR has been reviewed and has the lgtm label, please assign jasondellaluce for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

Andreagit97 · 2024-03-11T11:31:49Z

I added again the commits but it seems that now the CI is broken for another reason 😮‍💨 i will take a look ASAP!

FedeDP · 2024-05-15T16:05:33Z

/milestone 0.39.0

poiana added do-not-merge/work-in-progress release-note-none dco-signoff: yes kind/feature area/build labels Jan 10, 2024

poiana requested review from Andreagit97 and Kaizhe January 10, 2024 15:40

poiana added the size/L label Jan 10, 2024

leogr reviewed Jan 12, 2024

View reviewed changes

Andreagit97 added this to the 0.38.0 milestone Jan 17, 2024

poiana added dco-signoff: no and removed dco-signoff: yes labels Jan 24, 2024

h4l0gen force-pushed the kapil branch from f8eb97b to 2b93331 Compare January 24, 2024 18:18

poiana added dco-signoff: yes and removed dco-signoff: no labels Jan 24, 2024

leogr reviewed Jan 25, 2024

View reviewed changes

h4l0gen force-pushed the kapil branch 2 times, most recently from fce2570 to 96cc234 Compare January 25, 2024 18:14

leogr reviewed Feb 13, 2024

View reviewed changes

poiana added size/M and removed size/L labels Feb 25, 2024

h4l0gen force-pushed the kapil branch from 14dae96 to a5297c4 Compare March 7, 2024 05:55

poiana added dco-signoff: yes size/XS and removed dco-signoff: no size/XXL labels Mar 7, 2024

h4l0gen added 9 commits March 9, 2024 00:26

feat: added validation function for config keys

195fd38

Signed-off-by: h4l0gen <ks3913688@gmail.com>

Valijson mechanism updated

e8f4fe5

Signed-off-by: h4l0gen <ks3913688@gmail.com>

using valijson for validation process

a12cc4a

Signed-off-by: h4l0gen <ks3913688@gmail.com>

keys as a string instead of json file

907f18e

Signed-off-by: h4l0gen <ks3913688@gmail.com>

removing type constraint for keys

3f905b7

Signed-off-by: h4l0gen <ks3913688@gmail.com>

adding yaml-lint for falco.yaml

2e2a2e4

Signed-off-by: h4l0gen <ks3913688@gmail.com>

Revert "removing type constraint for keys"

a5f0813

This reverts commit 1373bb2. Signed-off-by: h4l0gen <ks3913688@gmail.com>

Revert "Revert "removing type constraint for keys""

926c35a

This reverts commit d809cbe. Signed-off-by: h4l0gen <ks3913688@gmail.com>

Revert "adding yaml-lint for falco.yaml"

bd38bfd

This reverts commit 9fec747. Signed-off-by: h4l0gen <ks3913688@gmail.com>

h4l0gen reopened this Mar 8, 2024

poiana added size/XL and removed size/XS labels Mar 8, 2024

Andreagit97 added 3 commits March 11, 2024 12:16

chore: bump to latest libs

d54e6c7

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

chore: include valijson also in minimal build

fb18964

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

new: use valijson adapters

e4876cc

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

h4l0gen changed the title ~~wip: added validation function for config keys~~ added validation function for config keys Apr 21, 2024

poiana removed the do-not-merge/work-in-progress label Apr 21, 2024

h4l0gen requested a review from Andreagit97 April 21, 2024 16:07

poiana modified the milestones: 0.38.0, 0.39.0 May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added validation function for config keys #3004

added validation function for config keys #3004

h4l0gen commented Jan 10, 2024 •

edited

poiana commented Jan 10, 2024

Andreagit97 commented Jan 10, 2024

leogr left a comment •

edited

leogr Jan 12, 2024

h4l0gen Jan 12, 2024

FedeDP commented Jan 17, 2024

FedeDP commented Jan 17, 2024

h4l0gen commented Jan 18, 2024

h4l0gen commented Jan 24, 2024 •

edited

leogr Jan 25, 2024 •

edited

h4l0gen Jan 25, 2024

h4l0gen Feb 5, 2024

h4l0gen commented Feb 11, 2024

leogr left a comment

leogr Feb 13, 2024

leogr Feb 13, 2024

leogr commented Feb 23, 2024

h4l0gen commented Feb 23, 2024

poiana commented Mar 8, 2024

poiana commented Mar 8, 2024

Andreagit97 commented Mar 11, 2024 •

edited

FedeDP commented May 15, 2024

added validation function for config keys #3004

Are you sure you want to change the base?

added validation function for config keys #3004

Conversation

h4l0gen commented Jan 10, 2024 • edited

poiana commented Jan 10, 2024

Andreagit97 commented Jan 10, 2024

leogr left a comment • edited

Choose a reason for hiding this comment

leogr Jan 12, 2024

Choose a reason for hiding this comment

h4l0gen Jan 12, 2024

Choose a reason for hiding this comment

FedeDP commented Jan 17, 2024

FedeDP commented Jan 17, 2024

h4l0gen commented Jan 18, 2024

h4l0gen commented Jan 24, 2024 • edited

leogr Jan 25, 2024 • edited

Choose a reason for hiding this comment

h4l0gen Jan 25, 2024

Choose a reason for hiding this comment

h4l0gen Feb 5, 2024

Choose a reason for hiding this comment

h4l0gen commented Feb 11, 2024

leogr left a comment

Choose a reason for hiding this comment

leogr Feb 13, 2024

Choose a reason for hiding this comment

leogr Feb 13, 2024

Choose a reason for hiding this comment

leogr commented Feb 23, 2024

h4l0gen commented Feb 23, 2024

poiana commented Mar 8, 2024

poiana commented Mar 8, 2024

Andreagit97 commented Mar 11, 2024 • edited

FedeDP commented May 15, 2024

h4l0gen commented Jan 10, 2024 •

edited

leogr left a comment •

edited

h4l0gen commented Jan 24, 2024 •

edited

leogr Jan 25, 2024 •

edited

Andreagit97 commented Mar 11, 2024 •

edited