libs
libs copied to clipboard
falcosecurity
[WIP] update(libsinsp)!: merge all the opening methods into just one
What type of PR is this?
/kind cleanup
/kind design
/kind feature
Any specific area of the project related to this PR?
/area libscap-engine-bpf
/area libscap-engine-gvisor
/area libscap-engine-kmod
/area libscap-engine-modern-bpf
/area libscap-engine-nodriver
/area libscap-engine-noop
/area libscap-engine-source-plugin
/area libscap-engine-savefile
/area libscap-engine-udig
/area libscap
/area libsinsp
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
The rationale behind this PR is that we are adding a new engine, the modern_bpf_engine, so we need to provide our clients (e.g. Falco) with a new way to open this engine through sinsp. Working on it, I noticed a lot of duplicated code around the engine opening phase...We have 3 functions that perform almost the same stuff with small differences:
-
open_live_common -
open_nodriver -
open_int
So the idea here is to merge these 3 functions into just one (open_common) and use wrapper methods for every engine to customize the open phase. Please note we have already in place some wrapper open methods like open_gvisor or open_udig so also here we don't have a unified approach... The scope of this PR is to unify all these existing interfaces into just one: dedicated wrapper methods for every engine + open_common. This is the clearest way I found to address this issue by I'm all ears if there are suggestions on it :)
So please note, ALL libs consumers (@leogr @terylt @Molter73 ...) should now use these dedicated APIs (open_kmod, open_bpf, ...) to open the right engine, unfortunately, this is a breaking change in the libs interface.
In order to simplify the interface, I also refactored the scap_open_args to make them engine-specific. Now every engine will have a slightly different flavor of the scap_open_args. This work should also help @gnosek in his v-table refactor, now that we know which is the engine that we are trying to open, it should be quite easy to assign the correct v-table!
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
update(libsinsp): merge all the opening methods into just one
@Andreagit97: The label(s) area/libscap-engine-savefile cannot be applied, because the repository doesn't have them.
In response to this:
What type of PR is this?
/kind cleanup
/kind design
/kind feature
Any specific area of the project related to this PR?
/area libscap-engine-bpf
/area libscap-engine-gvisor
/area libscap-engine-kmod
/area libscap-engine-modern-bpf
/area libscap-engine-nodriver
/area libscap-engine-noop
/area libscap-engine-source-plugin
/area libscap-engine-savefile
/area libscap-engine-udig
/area libscap
/area libsinsp
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
The rationale behind this PR is that we are adding a new engine, the
modern_bpf_engine, so we need to provide our clients (e.g. Falco) with a new way to open this engine through sinsp. Working on it, I noticed a lot of duplicated code around the engine opening phase...We have 3 functions that perform almost the same stuff with small differences:
open_live_commonopen_nodriveropen_intSo the idea here is to merge these 3 functions into just one (
open_common) and use wrapper methods for every engine to customize the open phase. Please note we have already in place some wrapper open methods likeopen_gvisororopen_udigso also here we don't have a unified approach... The scope of this PR is to unify all these existing interfaces into just one: dedicated wrapper methods for every engine +open_common. This is the clearest way I found to address this issue by I'm all ears if there are suggestions on it :)So please note, ALL libs consumers (@leogr @terylt @Molter73 ...) should now use these dedicated APIs (
open_kmod,open_bpf, ...) to open the right engine, unfortunately, this is a breaking change in the libs interface.In order to simplify the interface, I also refactored the
scap_open_argsto make them engine-specific. Now every engine will have a slightly different flavor of thescap_open_args. This work should also help @gnosek in his v-table refactor, now that we know which is the engine that we are trying to open, it should be quite easy to assign the correct v-table!Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
update(libsinsp): merge all the opening methods into just one
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@Andreagit97: The label(s)
area/libscap-engine-savefilecannot be applied, because the repository doesn't have them.
I've added it @poiana :)
@jasondellaluce: You must be a member of the falcosecurity/libs-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your maintainers of falcosecurity/libs and have them propose you as an additional delegate for this responsibility.
In response to this:
/milestone 0.9.0
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
Hei @incertum these are all good ideas but probably I would open a dedicated PR for refactoring this example test, here we are just introducing the necessary flags, all other changes could be discussed in another PR, WDYT? :thinking: Moreover, this program is already used in some tests so it seems really the case to have a dedicated discussion/PR on it, but IMHO i agree with your points :)
@Andreagit97 this PR is super important, so yes we should do the refactor afterwards after those 2 PRs are merged in. Very happy that we can now use the sinsp example with bpf.
LGTM label has been added.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: Andreagit97, FedeDP
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [Andreagit97,FedeDP]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Left some minor nits; feel free to discard them (or fix them in a subsequent PR)!
@Andreagit97 Maybe it's too late to chime in (I noticed it when the latest pull I made from the libs upstream broke the SysFlow collector), but I would have suggested adding deprecation notices to the public APIs that were removed, and effectively removing them in a future release (e.g., 0.10.0). This is too important of a change to be done in a single shot, and it affects the stability of the public API of libsinsp.
