search

falcosecurity / falco

Cloud Native Runtime Security
https://falco.org
Apache License 2.0
7.29k stars 897 forks source link

fix(build): metrics compilation after refactoring into classes #3265

Closed Molter73 closed 1 month ago

Molter73 commented 3 months ago

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it: Once falcosecurity/libs#1920 is merged, compilation of falco will be broken due to the new_metric method changing the class it lives in. This PR makes falco compatible with this change.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer: The commit that changes the libs cmake files will be dropped and replaced with a commit that properly updates the libs commit and SHA once falcosecurity/libs#1920 is merged, please disregard these changes until that merge occurs.

Does this PR introduce a user-facing change?:

NONE
Molter73 commented 3 months ago

/hold

Molter73 commented 3 months ago

/cc @FedeDP @incertum

FedeDP commented 3 months ago

Thanks Mauro for taking care of this!

Molter73 commented 3 months ago

I'm a bit confused as to why those driver tests are failing, these PRs don't do any changes to driver code or its corresponding userspace components. Any ideas?

FedeDP commented 3 months ago

It happens sometimes; probably mismatch between kernel headers package and runner kernel (or bugged headers package); rnothing to worry about for you! Restarted :)

Molter73 commented 3 months ago

Kmod failed again, but I need to fix the cmake files after I merge the libs PR anyways. @FedeDP, what do you think? Am I OK to unhold the libs PR?

FedeDP commented 3 months ago

Yes no problem, the issue is not dependent on your changes!

Molter73 commented 3 months ago

/unhold

incertum commented 3 months ago

@Molter73 we usually have the habit to bump both libs and driver during the dev phase. Few times we didn't do it and it caused compatibility issues. If you agree we can just continue bumping both together?

FedeDP commented 3 months ago

/milestone 0.39.0

Molter73 commented 3 months ago

Sorry it took a bit, I've bumped the driver version so it matches libs.

Molter73 commented 3 months ago

I'm guessing the error I'm getting is related to this commit: https://github.com/falcosecurity/libs/commit/fac0ae442474bc1265d2ddc54a3f12557337a821

How do we want to handle it?

FedeDP commented 3 months ago

Yep! I already tagged Gerald in that PR to ask him to double check ;) Not a big deal, we can wait and then bump libs and driver here once the fix PR is merged.

Molter73 commented 3 months ago

One of these days I'll get this PR to pass CI...

incertum commented 3 months ago

/hold for possible patches https://github.com/falcosecurity/falco/issues/3271#issuecomment-2206918760. Do not merge prior to having made a decision.

poiana commented 1 month ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Molter73 Once this PR has been reviewed and has the lgtm label, please assign mstemm for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/falcosecurity/falco/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
Molter73 commented 1 month ago

I'm guessing the 3 issues are due to some change that bled in with the latest libs bump, I was having some issues linking unit tests locally, I'll look into them when I get a minute.

Molter73 commented 1 month ago

@jasondellaluce looks like the unit test errors I got are due to some warning messages being changed in falcosecurity/libs#1831, could you double check I got those right in the last commit?

Molter73 commented 1 month ago

Now it looks like ASAN is not very happy, tried to run the tests locally with a debug build and the errors seem related to falcosecurity/libs#1992, would appreciate some confirmation.

LucaGuerra commented 1 month ago

@Molter73 AFAIK the ASan failures are fixed by https://github.com/falcosecurity/libs/pull/1992 , I'm writing some tests for those and will merge the PR soon. I'm not sure why the asan crashes started appearing now.

Molter73 commented 1 month ago

No worries, I'll update this PR after you merge!

Molter73 commented 1 month ago

I'm gonna need some help understanding the last errors I'm getting, seems something has been broken with the legacy rules?

FedeDP commented 1 month ago

This is the same error we are gettng on #3283 . Needs further investigation... Also this and #3283 are basically the same, i'd close one of them (well this is older and we should keep this but the other is on an upstream branch that is much easier to work with to fix the test issues; let me know if you wish to keep this one, i'll close the other!)

Molter73 commented 1 month ago

Closing in favor of #3283, let me know if any help is needed on that PR!