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

Break apart logic in falco-driver-loader or rename the script #1193

Closed
krisnova opened this issue May 6, 2020 · 5 comments · Fixed by #1200
Closed

Break apart logic in falco-driver-loader or rename the script #1193

krisnova opened this issue May 6, 2020 · 5 comments · Fixed by #1200
Assignees

Comments

@krisnova
Copy link
Contributor

krisnova commented May 6, 2020

Motivation

Right now our documentation and our calls use the term loader to define the behavior of this script.

I think that the name could be improved to clarify exactly what the script is doing.

I also believe that the script should be dynamic and configurable to perform single tasks without making assumptions on behalf of the user.

Feature

Right now the script performs multiple phases of logic within monolithic functions that perform more than one task.

For instance load_kernel_module does more than just load something. It also tries to download a kernel module, it also tries to compile a kernel module.

I think we should take one of 2 paths

  1. Rename the script to something that describes what it does.
./falcodriver-loader-downloader-compiler
  1. Fix the script so it is modular and can be used as a single use case
# This will try to download first, and then attempt to compile if the download fails
./falcodriver module --download --compile 

# This will never download, and will only try to compile
./falcodriver bpf --compile

# This will first try to use DKMS to load the kernel module if it exists on the system, then download, then compile
./falcodriver module --load --download --compile

# This will do exactly what it does now and try to detect and succeed at all costs
./falcodriver # no arguments

Alternatives

Additional context

@leodido
Copy link
Member

leodido commented May 6, 2020

I think I like this proposal Kreees. :))

BTW, that script already has the bpf argument (ie., - ./falco-driver-loader bpf already exists and works).

@krisnova
Copy link
Contributor Author

krisnova commented May 6, 2020

Thank you for reading it! I was thinking about this on the call but didn't want to pollute the conversation with yet another thing to talk about. So I wrote it down instead for us.

@leogr
Copy link
Member

leogr commented May 7, 2020

I like the proposal to modularize the script, as already said here. 🥳

Just a thought: I don't see the need for renaming it because loading the driver should still be the primary responsibility of the script. Skipping the loading seems to me like a "dry-run", or is there a specific use case for that? I would be okay with the current name and just add arguments and flags to modify its default behavior.

So I'm for path 2.

Thank you for having written it!

@fntlnz
Copy link
Contributor

fntlnz commented May 8, 2020 via email

@leogr
Copy link
Member

leogr commented May 8, 2020

I can work on that
/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants