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

Jump to bottom

Emit a warning when trying to set a non-existing config key #2924

Open
leogr opened this issue Nov 23, 2023 · 7 comments · May be fixed by #3004
Open

Emit a warning when trying to set a non-existing config key #2924

leogr opened this issue Nov 23, 2023 · 7 comments · May be fixed by #3004
Assignees
@h4l0gen
Labels
kind/feature
Milestone
TBD

Comments

@leogr
Copy link
Member

leogr commented Nov 23, 2023 •
edited

Motivation

Silently ignoring a non-existing config key in falco.yaml can hide problems and mislead users. A simple typo can make Falco completely ignore the value, which may be hard to spot.

For example:

stdout_output:
  enabled: false

vs.

stduot_output:
  enabled: true

(N.B. the typo in stduot_output)

Currently, Falco silently ignores the latter case.

Feature

Falco should emit a warning when the user sets a non-existent config key.

Alternatives

An alternative can be to emit an error and stop the Falco execution. Would this alternative be better? 🤔
On the other hand, a simple warning could help spot misconfiguration when upgrading to a new version without blocking the deployment.

Additional context

This idea came up while reviewing this PR #2413
@Andreagit97 Andreagit97 added this to the TBD milestone Nov 28, 2023
@h4l0gen
Copy link

h4l0gen commented Dec 3, 2023

hey @leogr , i want to contribute on this issue, can you guide me to what changes required and how can i proceed in this. Thanks

@leogr
Copy link
Member Author

leogr commented Dec 5, 2023

hey @leogr , i want to contribute on this issue, can you guide me to what changes required and how can i proceed in this. Thanks

Hey @h4l0gen
Great! 🤩

I haven't looked into code details, but I guess that we should look for the existence of an option while parsing the YAML here 👇
https://github.com/falcosecurity/falco/blob/master/userspace/falco/configuration.cpp#L186

An alternative approach would be to create a YAML schema and validate the file before loading it (I wonder if it is worth keeping this approach).

@jasondellaluce any suggestion in this regard?

@h4l0gen let me know if this is ok for you. In case you plan to work on this, please /assign this issue to yourself, thank you!

@h4l0gen
Copy link

h4l0gen commented Dec 6, 2023

hey @leogr ,
it would be really great if you assign me this issue. am starting looking on code details and soon come up with required changes.

@h4l0gen
Copy link

h4l0gen commented Dec 6, 2023

/assign

@leogr
Copy link
Member Author

leogr commented Dec 11, 2023

Hey @h4l0gen

How is it going? :)

@h4l0gen h4l0gen linked a pull request Jan 10, 2024 that will close this issue
Open
@poiana
Copy link

poiana commented Mar 10, 2024

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@Andreagit97
Copy link
Member

/remove-lifecycle stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@h4l0gen h4l0gen

Labels
kind/feature
Projects
None yet
Milestone
TBD
Development

Successfully merging a pull request may close this issue.

added validation function for config keys h4l0gen/falco
4 participants
@leogr @poiana @Andreagit97 @h4l0gen