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

Add use of FALCO_DRIVER_CHOICE and FALCOCTL_ENABLED env vars #2773

Merged
merged 1 commit into from Dec 6, 2023

Conversation

vjjmiras
Copy link
Contributor

@vjjmiras vjjmiras commented Sep 5, 2023

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:

Which issue(s) this PR fixes:

Refs #2574

These new environment variables will be recognized when the user attempts the unattended installation of Falco.
They'll expand the customization options letting the user choose their preferred driver and the use of falcoctl. In other words, it's an alternative to the dialog input.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(scripts): add a way to enforce driver kind and falcoctl enablement when installing Falco from packages and dialog is not present.

Signed-off-by: Vicente J. Jiménez Miras <vjjmiras@gmail.com>
@Andreagit97
Copy link
Member

First of all, thank you for the proposed changes!
They are perfectly in line with what we do today with the FALCO_FRONTEND env variable. BTW in the future probably we would like to change the approach a little bit. See some pointers here: #2574 (comment)

So I'm not completely sure what we want do to here...I would avoid introducing new envs that will be deleted in the next release of Falco, but these are just my 2 cents. Any opinions here? @falcosecurity/falco-maintainers

@Andreagit97 Andreagit97 added this to the TBD milestone Sep 5, 2023
@incertum
Copy link
Contributor

@FedeDP since you work on env variables atm, what about this PR? Thank you!

@FedeDP
Copy link
Contributor

FedeDP commented Nov 27, 2023

I actually think this PR is useful on another level, ie: during rpm/deb installation for example in dockerfiles.
IMHO, even after #2413 , this would still be a good patch since users would be able to select a driver while installing a package without using the dialog input.
I'd wait for all the job on the new driver-loader before getting back at this, though, ie:

@FedeDP
Copy link
Contributor

FedeDP commented Nov 28, 2023

Adding a small info on top of this; al2023 does not ship any dialog package: https://docs.aws.amazon.com/linux/al2023/release-notes/removed-AL2023.2-AL2.html
Therefore our text-based dialog to choose the driver when installing a Falco rpm package won't be shown there.
I think this PR makes absolutely sense ;)
Will try to add this to the 0.37.0 milestone:
/milestone 0.37.0

@leogr
Copy link
Member

leogr commented Nov 28, 2023

Adding a small info on top of this; al2023 does not ship any dialog package: https://docs.aws.amazon.com/linux/al2023/release-notes/removed-AL2023.2-AL2.html Therefore our text-based dialog to choose the driver when installing a Falco rpm package won't be shown there. I think this PR makes absolutely sense ;) Will try to add this to the 0.37.0 milestone: /milestone 0.37.0

How can we ensure these $FALCO_DRIVER_CHOICE is in sync with falco.yaml (ie. engine.kind )?

@FedeDP
Copy link
Contributor

FedeDP commented Nov 28, 2023

How can we ensure these $FALCO_DRIVER_CHOICE is in sync with falco.yaml (ie. engine.kind )?

FALCO_DRIVER_CHOICE will be used during package installation, ie: sudo FALCO_DRIVER_CHOICE=ebpf rpm -i falco.rpm, not at Falco starts, and will only auto-select the ebpf driver for the driver-loader part of the Falco package installation.

@leogr
Copy link
Member

leogr commented Nov 29, 2023

How can we ensure these $FALCO_DRIVER_CHOICE is in sync with falco.yaml (ie. engine.kind )?

FALCO_DRIVER_CHOICE will be used during package installation, ie: sudo FALCO_DRIVER_CHOICE=ebpf rpm -i falco.rpm, not at Falco starts, and will only auto-select the ebpf driver for the driver-loader part of the Falco package installation.

I meant how we ensure it is in sync with Falco settings when Falco will start later. Anyway, I've likely found the answer:
FALCO_DRIVER_CHOICE affects the choice of the installed systemd service (ie kmod, bpf, etc), which in turn overrides the Falco config. For instance, sudo FALCO_DRIVER_CHOICE=ebpf rpm -i falco.rpm will have the following effects:

  • installing the ebpf driver
  • installing the falco-bpf.service which should override the default engine.kind by forcing the usage of epbf

Is my assumption correct? 🤔

If so, I guess we have to update systemd files according to the new driver selection mechanism. For example:
https://github.com/falcosecurity/falco/issues/new?permalink=https%3A%2F%2Fgithub.com%2Ffalcosecurity%2Ffalco%2Fblob%2Fc5364be191f580a7a1edce3314ea7ace0cff8a86%2Fscripts%2Fsystemd%2Ffalco-bpf.service%23L10-L11

Should be:

 ExecStart=/usr/bin/falco -o engine.kind=ebpf

@FedeDP
Copy link
Contributor

FedeDP commented Nov 29, 2023

Is my assumption correct? 🤔

yes

Should be:

This is done in #2905

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.

SGTM, considering #2773 (comment)

I just want to invite other @falcosecurity/falco-maintainers to take a look and consider if we want to support these env vars.

For me, they are ok. Maybe we can think about a better naming that lets users clearly understand their purpose and scope (ie they are for host installation use case only).

@FedeDP
Copy link
Contributor

FedeDP commented Nov 29, 2023

/milestone 0.37.0

@poiana poiana modified the milestones: TBD, 0.37.0 Nov 29, 2023
@leogr
Copy link
Member

leogr commented Dec 1, 2023

/assign
/assign @FedeDP

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 poiana added the lgtm label Dec 6, 2023
@poiana
Copy link

poiana commented Dec 6, 2023

LGTM label has been added.

Git tree hash: fd4473ff7b2711984f61c3bcc745bc10f4bfdda2

@poiana poiana added the approved label Dec 6, 2023
@@ -67,6 +67,25 @@ if [ "$1" = "configure" ]; then
esac
fi
clear
else
case $FALCO_DRIVER_CHOICE in
module | kmod )
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just enforce proper namings (the same names that we use for falco systemd units for example? Or the ones we use in Falco config)
But i can do that once this is merged, in #2905

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.

As discussed with @FedeDP, this PR is ok, and we just reserve to do minor changes or improvement (ie. naming) in a follow up PR (likely after we have tested Falco with all the recent changes merged in).

@poiana
Copy link

poiana commented Dec 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, leogr, vjjmiras

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:

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 13991f1 into falcosecurity:master Dec 6, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants