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(falco.yaml): rename none in nodriver #3012

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

Andreagit97
Copy link
Member

@Andreagit97 Andreagit97 commented Jan 15, 2024

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

This PR renames the none engine config into state_only. The idea is that none should mean no engine enabled, but this is not the case. Even though the nodriver is a particular engine, it opens an inspector without generating events. So the idea is to rename the none into state_only and leave the none config for future use cases.

For example none should be used when we want to run a source plugin like k8saudit without opening a syscall engine. Today we need to do something like this:

falco -c /etc/falco.yaml -r k8saudit.rules --disable-source=syscall

In the future, if we add the config none we could simply specify none in the config file and we have done

falco -c /etc/falco.yaml -r k8saudit.rules

I renamed the config into state_only because, in the end, this is what the nodriver does, it opens an inspector and the thread table allowing plugins with syscall source to run.

Special notes for your reviewer:

I would like to have it in this release because if we do it now this is not a breaking change, if we wait Falco 0.38.0 it will become a breaking change

Does this PR introduce a user-facing change?:

NONE

@leogr
Copy link
Member

leogr commented Jan 16, 2024

The idea is that none should mean no engine enabled, but this is not the case
I agree with this, stating the syscall source is still enabled.

Even though the nodriver is a particular engine, it opens an inspector without generating events
I also agree with this statement. Indeed, nodriver is really an engine, which however, does not connect o any driver so it can't produce any syscall event. However, other than just the state, other events might come into the flow (from container engine? plugins? I can't think of others, but there may be).

In other words, the nodriver implementation is kind of Null Object Pattern and it has been introduced mainly for testing and CI purposes, AFAIK.

So the idea is to rename the none into state_only and leave the none config for future use cases.'
Although I understand none config should be reserved for the no syscall data source at all use case, state_only seems too specific to me for the nodriver case.

Thus, I'd suggest:

  • just use nodriver. I know we had an initial discussion regarding this that went in a different direction, but since the config node is now called engine.kind, nodriver seems appropriate. Basically, no driver would mean no kernel instrumentation thus the engine will produce no syscall event. That's consistent.
  • nulldriver, driverless, etc.. can be valid alternatives.

I've highlighted this because it would be nice to choose a name without risking the need to change it again.

I would like to have it in this release because if we do it now this is not a breaking change, if we wait Falco 0.38.0 it will become a breaking change

+1 better to change it now.

@Andreagit97
Copy link
Member Author

Andreagit97 commented Jan 16, 2024

so you are suggesting renaming the actual none into nodriver since it means that there is an engine but not a driver, and in the future use none (or something similar) to indicate that there is no engine at all, is this true? If yes I agree with this idea!

@FedeDP
Copy link
Contributor

FedeDP commented Jan 16, 2024

just use nodriver. I know we had an initial discussion regarding this that went in a different direction, but since the config node is now called engine.kind, nodriver seems appropriate. Basically, no driver would mean no kernel instrumentation thus the engine will produce no syscall event. That's consistent.

Agree, nodriver is far better than state_only and 1:1 matches the libscap engine name.

Signed-off-by: Andrea Terzolo <[email protected]>
Co-authored-by: Leonardo Grasso <[email protected]>
@Andreagit97
Copy link
Member Author

done!

@Andreagit97 Andreagit97 changed the title [WIP] [PROPOSAL] cleanup(falco.yaml): rename none in state_only cleanup(falco.yaml): rename none in state_only Jan 16, 2024
@leogr leogr changed the title cleanup(falco.yaml): rename none in state_only cleanup(falco.yaml): rename none in nodriver Jan 16, 2024
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.

+1

@poiana
Copy link
Contributor

poiana commented Jan 17, 2024

LGTM label has been added.

Git tree hash: 5f1b4188178c7c0a43d4c673c281d76d105564ff

Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Jan 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, 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 [Andreagit97,FedeDP,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 ae9ffe4 into falcosecurity:master Jan 17, 2024
28 checks passed
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