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

feat(falco): Provide a parameter for loading lua files from an alternate path #1419

Merged
merged 1 commit into from
Sep 29, 2020
Merged

feat(falco): Provide a parameter for loading lua files from an alternate path #1419

merged 1 commit into from
Sep 29, 2020

Conversation

admiral0
Copy link

@admiral0 admiral0 commented Sep 29, 2020 •
edited by leodido
Loading

This will be used by the static build to load lua files from
alternate directories that are not tied to the compile flags

What type of PR is this?

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

/kind feature

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:
This allows us to provide tarballs of static builds of falco and use --alternate-lua-dir $(pwd)/lua to specify where to look for lua
files.

Which issue(s) this PR fixes:

Special notes for your reviewer:
This will improve user experience for users of the static build, because currently that path is hardcoded at compile time.

Does this PR introduce a user-facing change?:

new: CLI flag `--alternate-lua-dir` to load Lua files from arbitrary paths

@poiana poiana requested review from krisnova and leodido September 29, 2020 11:46
@poiana poiana added the size/S label Sep 29, 2020
fntlnz
fntlnz reviewed Sep 29, 2020
View reviewed changes
Copy link
Contributor

@fntlnz fntlnz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for bringing this up @admiral0 - I also really needed this for some deployments I was doing this week - I like the general approach but had some comments.

userspace/falco/falco.cpp Outdated Show resolved Hide resolved
userspace/falco/falco.cpp Outdated Show resolved Hide resolved
userspace/falco/falco.cpp Outdated Show resolved Hide resolved
@fntlnz
Copy link
Contributor

fntlnz commented Sep 29, 2020

We probably need to add the trailing slash if the user didn't provide it:

This does not work

sudo ./userspace/falco/falco -r ../rules/falco_rules.yaml -u --alternate-lua-dir /tmp/lua 
alternate lua dir: /tmp/lua
Tue Sep 29 14:55:27 2020: Runtime error: Could not find Falco Lua entrypoint (tried /usr/share/falco/lua/rule_loader.lua, /tmp/luarule_loader.lua). Exiting.

This works

sudo ./userspace/falco/falco -r ../rules/falco_rules.yaml -u --alternate-lua-dir /tmp/lua/
alternate lua dir: /tmp/lua/
Tue Sep 29 14:56:19 2020: Falco version 0.26.0-2+ac84e3e (driver version 2aa88dcf6243982697811df4c1b484bcbe9488a2)
Tue Sep 29 14:56:19 2020: Falco initialized with configuration file /home/fntlnz/Projects/falcosecurity/falco/falco.yaml
Tue Sep 29 14:56:19 2020: Loading rules from file ../rules/falco_rules.yaml:

leodido
leodido reviewed Sep 29, 2020
View reviewed changes
Copy link
Member

@leodido leodido left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Radu for sending this PR!

This PR is in the right direction. Anyways, it is not completed yet IMHO.

For example in falco_outputs.cpp there is this line that needs to be updated too.

falco_common::init(m_lua_main_filename.c_str(), FALCO_SOURCE_LUA_DIR);

Note that the falco_common::init() is also called by the ctor of falco_engine that you updated.

@leodido
Copy link
Member

leodido commented Sep 29, 2020

/milestone 0.27.0

@poiana poiana added this to the 0.27.0 milestone Sep 29, 2020
@leodido
Copy link
Member

leodido commented Sep 29, 2020 •
edited
Loading

@admiral0 I've updated the release-note line, waiting for your new commits :)

@poiana poiana added size/M and removed size/S labels Sep 29, 2020
@admiral0
Copy link
Author

/assign @leodido

leogr
leogr reviewed Sep 29, 2020
View reviewed changes
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'd personally prefer to completely remove the need for external files (by embedding them into the binary, as proposed in #1367).

However, I can agree with this approach for now 👍

Just my 2 cents :)

leodido
leodido reviewed Sep 29, 2020
View reviewed changes
userspace/falco/falco.cpp Outdated Show resolved Hide resolved
feat(falco): Provide a parameter for loading lua files from an altern…
aac3ce2 
…ate path

This will be used by the static build to load lua files from
alternate directories that are not tied to the compile flags

Signed-off-by: Radu Andries <radu.andries@sysdig.com>
@poiana poiana added the lgtm label Sep 29, 2020
@poiana
Copy link
Contributor

poiana commented Sep 29, 2020

LGTM label has been added.

Git tree hash: a8605020170861c3cef418aee3d381290d6bcf97

@poiana
Copy link
Contributor

poiana commented Sep 29, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, leodido

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 bc1aeac into falcosecurity:master Sep 29, 2020
@admiral0 admiral0 deleted the alternate-lua-dir branch September 30, 2020 09:53
@leogr leogr modified the milestones: 0.27.0, 0.26.1 Sep 30, 2020
@admiral0 admiral0 mentioned this pull request Oct 1, 2020
Closed
@leodido leodido mentioned this pull request Oct 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leogr leogr leogr left review comments

@fntlnz fntlnz fntlnz approved these changes

@leodido leodido leodido approved these changes

@krisnova krisnova Awaiting requested review from krisnova

Assignees

@leodido leodido

@fntlnz fntlnz

Labels
approved area/engine dco-signoff: yes kind/feature lgtm release-note size/M
Projects
None yet
Milestone
0.26.1
Development

Successfully merging this pull request may close these issues.
5 participants
@admiral0 @fntlnz @leodido @poiana @leogr