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.

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

cleanup(config): improve falco.yaml config #2571

Merged
merged 5 commits into from
May 29, 2023

Conversation

incertum
Copy link
Contributor

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:

Make it easier for adopters to understand and navigate Falco configs to achieve optimal deployment settings.

  • re-arrange falco.yaml configs in logical categories
    • add an index for logical categories
    • move configs around without changing description content,
      solely add a uniform header to each config
    • indicate "Stable" or "Experimental" for most configs
      to indicate current stability or maturity
  • improve config descriptions for the basic config options

Was planning to touch up the remaining configs, but perhaps in a follow up PR depending on your preferences.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

cleanup(config): improve falco config

* add an index for logical categories
* move configs around without changing description content,
  solely add a uniform header to each config
* indicate "Stable" or "Experimental" for most configs
  to indicate current stability or maturity

Signed-off-by: Melissa Kilby <[email protected]>
@leogr
Copy link
Member

leogr commented May 24, 2023

/milestone 0.35.0

@poiana poiana added this to the 0.35.0 milestone May 24, 2023
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

I really like this! 🤩 Thank you! 🙏

I agree it's needed since we added a lot of options but never reorganized this file.

I've left some suggestions, mainly regarding the placement of the options. Apart from that, it seems already good for me!

falco.yaml Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
falco.yaml Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
falco.yaml Outdated Show resolved Hide resolved
* incorporate reviewers suggestions re ordering and phrasing
* minor additional cleanups

Co-authored-by: Leonardo Grasso <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
@incertum
Copy link
Contributor Author

@leogr thanks for the amazing suggestions, incorporated them all. Please let me know if we first want to stop here now and move further language adjustments and improvements to a new PR? Would recommend not overloading this PR as we have been moving around things a lot.

Loaded the yaml and printed all primary keys to ensure we didn't lose one and the ordering is correct, please also verify.

…d more information

* rephrase descriptions for numerous config options
  without changing the original content, meaning changes
  reflect language improvements and minor extensions
  (such as adding justifications or what it is) only
* add Falco environment variables section
* add Guidance for Kubernetes container engine command-line args settings
* general rewrap formatting w/ IDE
* minor additional re-ordering of configs
* minor general language adjustments

Signed-off-by: Melissa Kilby <[email protected]>
@incertum
Copy link
Contributor Author

incertum commented May 24, 2023

@leogr as discussed added all planned changes in this PR. From my perspective this PR can be considered complete.
Re metadata_download unsure if I got it right, would you mind helping me with this config?

Looking forward to the next round of feedback in order to polish everything.

@jasondellaluce
Copy link
Contributor

Love it Melissa! My only suggestion is to move the plugin configs up in the file, right below the loaded rules files. Alongside rules, plugins are the biggest customization point for Falco's behavior and use cases, and I think this will be come even more true now that we can develop plugins that work with the syscall event stream.

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

I just left a comment for metadata_download. Otherwise, SGTM.

And Melissa, thank you again for this PR 🙏

falco.yaml Outdated Show resolved Hide resolved
* place falco plugins after falco rules config
* change metadata_download description
* minor formatting

Co-authored-by: Jason Dellaluce <[email protected]>
Co-authored-by: Leonardo Grasso <[email protected]>

Signed-off-by: Melissa Kilby <[email protected]>
@incertum
Copy link
Contributor Author

Addressed feedback, thank you!

@poiana
Copy link
Contributor

poiana commented May 25, 2023

LGTM label has been added.

Git tree hash: 4ae5b547efe51529b677854a985d21bcf3921e45

@poiana
Copy link
Contributor

poiana commented May 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [jasondellaluce,leogr]

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

@poiana poiana merged commit e9402b7 into falcosecurity:master May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants