update(libsinsp)!: merge all the opening methods into just one#540
update(libsinsp)!: merge all the opening methods into just one#540poiana merged 22 commits intofalcosecurity:masterfrom
Conversation
poiana
added
release-note
kind/cleanup
dco-signoff: yes
kind/design
kind/feature
|
@Andreagit97: The label(s) DetailsIn response to this:
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
changed the title
I've added it @poiana :) |
| const char *scap_get_bpf_probe_from_env() | ||
| { | ||
| return getenv(SCAP_BPF_PROBE_ENV_VAR_NAME); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't know why we use this approach to open the bpf probe, but I had to change it since in this way we discover only during the scap_open phase if we are using the bpf engine or not and this is really strange in my opinion, and btw this method cannot work with the new approach proposed int his PR where the client must know which engine it will use! I think this is the behavior we want but I'm all ears if you other suggestions
| struct bpf_map_def def; | ||
| }; | ||
|
|
||
| static const char* resolve_bpf_probe(const char *bpf_probe, char *buf) |
There was a problem hiding this comment.
We receive the bpf path directly from the client, so we don't need anymore this :)
There was a problem hiding this comment.
The comment just below describes why we haven't done that in the first place :)
Your change forces every consumer to do BPF probe discovery on their own. On the other hand, it might actually be good anyway ;) BTW, does this let us drop the env var #define (I don't remember the name) and SCAP_BPF_PROBE_FILEPATH? If so, let's kill them both with fire ;)
There was a problem hiding this comment.
Yeah, this probably was born as a last attempt to open something, the problem is that some consumers like Falco use this workaround as the only way to open the probe and this is really strange IMHO 🤔 On the other side with this new approach is the consumer that decides the engine and which params it must have so we cannot rely anymore on this kind of approach :/
And yes, definitely i forgot them, thank you!
There was a problem hiding this comment.
Well, this is the reason I added the match method to the vtable (each engine can look into the scap_open_args and decide if it can support them). The bpf engine tries to resolve the probe in its match method to support the current API (I'm not touching the sinsp<->scap API in my series even though I'd love to).
The method isn't used yet (in the submitted/merged PRs) but the end result would be:
struct scap_vtable* find_engine(struct scap_open_args *args)
{
for(i=0; i<ARRAY_SIZE(known_engines); ++i) {
if(known_engines[i]->match(args)) {
return known_engines[i];
}
}
return NULL; // no suitable engine found
}
and the rest of libscap should be completely engine-agnostic (modulo the known_engines array which is a weak point of the design)
There was a problem hiding this comment.
Uhm I understood, that seems a smart approach but since we are changing the sinsp interface we can solve the problem at the root. This seems the right time to perform this change since we are quite far from the libs tag and the consumers can have the right time to change the interface 🤔
There was a problem hiding this comment.
Sure. I'm not touching sinsp (my PRs are big enough as is) but if you're willing to open that can of worms, go ahead :)
But my request if you do change the sinsp API would be to submit a PR to Falco at a similar time, so that consumers have at least something to base their changes on.
There was a problem hiding this comment.
ahahah yeah for sure, if we agree on that I can open a PR in Falco even before this one is merged :)
| @@ -0,0 +1,206 @@ | |||
| /* | |||
There was a problem hiding this comment.
Now the modern probe should also work with the sinsp-example, but let's do further tests before adding it to the CI :)
| @@ -23,9 +23,7 @@ limitations under the License. | |||
|
|
|||
There was a problem hiding this comment.
refactor the scap-open to use the new engine enum
userspace/libscap/scap.c
Outdated
| /* probably at the end of the v-table work we can use just one function | ||
| * with an internal switch that selects the right vtable! For the moment | ||
| * let's keep different functions. |
There was a problem hiding this comment.
@gnosek As I said in the PR comment this refactor should help us to unify all the engines under just one open method, which will assign the correct v-table according to the engine type, in this way we can remove a lot of duplicated code. Hoping this refactor doesn't bother your parallel v-table work, if this is the case i can revert something if you need it :)
| #ifdef HAS_ENGINE_MODERN_BPF | ||
| \brief Scap possible modes | ||
| */ | ||
| typedef enum | ||
| { | ||
| /*! | ||
| * Default value that mostly exists so that sinsp can have a valid value | ||
| * before it is initialized. | ||
| */ | ||
| SCAP_MODE_NONE = 0, | ||
| /*! | ||
| * Read system call data from a capture file. | ||
| */ | ||
| SCAP_MODE_CAPTURE, | ||
| /*! | ||
| * Read system call data from the underlying operating system. | ||
| */ | ||
| SCAP_MODE_LIVE, | ||
| /*! | ||
| * Do not read system call data. If next is called, a dummy event is | ||
| * returned. | ||
| */ | ||
| SCAP_MODE_NODRIVER, | ||
| /*! | ||
| * Do not read system call data. Events come from the configured input plugin. | ||
| */ | ||
| SCAP_MODE_PLUGIN, | ||
| } scap_mode_t; | ||
|
|
||
| /*! | ||
| * Read system call data from the underlying operating system using a modern | ||
| * bpf probe. | ||
| * \brief Argument for scap_open | ||
| * Set any PPM_SC syscall idx to true to enable its tracing at driver level, | ||
| * otherwise syscalls are not traced (so called "uninteresting syscalls"). | ||
| */ | ||
| SCAP_MODE_MODERN_BPF, | ||
| #endif |
There was a problem hiding this comment.
Please note, now also the modern_bpf is under the SCAP_MODE_LIVE so I've removed the dedicated scap mode SCAP_MODE_MODERN_BPF
Andreagit97
changed the title
|
/hold |
| } | ||
|
|
||
| static bool gvisor_match(scap_open_args* open_args) | ||
| static bool gvisor_match(scap_open_args* oargs) |
There was a problem hiding this comment.
Do we really need such a match API when we can just strncmp ?
Perhaps it is better to change the match API to just engine_name and let scap call
strcmp(oargs->engine_name, handle->engine_name()) == 0
?
| #pragma once | ||
|
|
||
| #define NODRIVER_ENGINE "nodriver" | ||
| #define NODRIVER_ENGINE_LEN 9 |
There was a problem hiding this comment.
Also, specifying the length to just do a strncmp(x, X_ENGINE, X_ENGINE_LEN) is exactly equal to just call strcmp(x, X_ENGINE); i think we can save us from defining the len everywhere and just use plain old strcmp.
| /*! | ||
| * Do not read system call data. Events come from the configured input plugin. | ||
| */ | ||
| SCAP_MODE_PLUGIN, |
There was a problem hiding this comment.
Isn't this now a case of SCAP_MODE_LIVE with scap->engine == sourceplugin (or whatever it is called :D )
|
|
||
| scap_test_input_data* test_input_data; ///< only used for testing scap consumers by supplying arbitrary test data | ||
| }scap_open_args; | ||
| typedef struct |
There was a problem hiding this comment.
Can't these structs be defined in various engines publich headers? Like
<engine/bpf/bpf_public.h>
FedeDP
left a comment
FedeDP
left a comment
There was a problem hiding this comment.
Very cool PR!! We definitely needed a refactor in this direction, thank you!
/approve
|
LGTM label has been added. DetailsGit tree hash: fa63ced892b651157cbebe62572682ded7ff9013 |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Left some minor nits; feel free to discard them (or fix them in a subsequent PR)! |
|
/unhold |
Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
|
@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. |
…y#540. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
…y#540. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
…y#540. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
…y#540. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
9 participants
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?: