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

chore(docker/falco): add back some deps to falco docker image. #2932

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Nov 29, 2023

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:

The PR adds back some deps that were removed in c2b940f from the Falco "fat" image.

Those deps are needed to build drivers on some distros/kernel configs.
I added back the ones that are provided by the falco-driver-loader-legacy image.
See relevant slack thread: https://kubernetes.slack.com/archives/CMWH3EH32/p1701249107807099

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 29, 2023

/cc @LucaGuerra
/milestone 0.37.0

@poiana poiana requested a review from LucaGuerra November 29, 2023 10:34
@poiana poiana added this to the 0.37.0 milestone Nov 29, 2023
@poiana poiana added the size/S label Nov 29, 2023
gcc \
gcc-11 \
gnupg2 \
jq \
libelf1 \
libc6-dev \
libelf-dev \
Copy link
Contributor

Choose a reason for hiding this comment

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

I know libelf-dev is useful for OpenShift compatibility but can we understand why the others? This is because we may be tempted to remove them in the future and it'd be good to have any kind of "trail" we can follow to understand why something was there in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were used in the -legacy image.
My opinion is: using the ones used by -legacy image we are sure that we are not breaking an existing behavior; perhaps we miss some more, but at least the 2 images are equal now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me take a bit of a closer look. I don't think we want a bloated image but at the same time we want to increase compatibility.

@LucaGuerra
Copy link
Contributor

LucaGuerra commented Dec 4, 2023

Hi! I'm back here. I analyzed two things about what we are attempting to do:

  1. The size difference of the two images
  2. The potential impact of the vulnerabilities that we may see in scanners due to these added packages (because I had a quick discussion with @leogr in the past about this)

The size of the compressed image is not a lot, about 23 MBs, I think we can live with that.
Regarding the vulnerabilities, I won't bore you with how I conducted the analysis but I noticed which packages, dependencies and "source packages" (a dpkg concept) are introduced with this change. Out of those, how often vulnerabilities are detected and if it's likely that they would very negatively impact our scanning results.

The result is that they won't. libbpf had two vulnerabilities per year recently and bison had a couple in 2020, the rest are in the security DBs even less frequently. They are massively outweighted by the many, many vulnerabilities (technically false positives but I digress) that are detected due to the dependencies of LLVM and the base image. Conclusion: I think we can add all of those!

Thank you Fede.

@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 4, 2023

Thanks to you Luca for your great analysis!

@poiana
Copy link
Contributor

poiana commented Dec 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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 0ba0dd8 into master Dec 5, 2023
20 checks passed
@poiana poiana deleted the chore/driver-loader-deps branch December 5, 2023 17:34
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