new(scap): extend scap machine info, new agent info (new metrics 1/n) #880

incertum · 2023-02-13T03:15:31Z

Signed-off-by: Melissa Kilby melissa.kilby.oss@gmail.com

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/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 libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

Migrating some lookups useful as identifiers and for new resource utilization metrics from Falco to libscap.

additional machine related info useful for:

host attributions
host kernel attributions
identifiers for new resource utilization metrics logs

scap_machine_info seems a suitable place for new lookups.

falcosecurity/falco#2333
falcosecurity/falco#2222

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(scap): extend scap machine info, new agent info

userspace/libscap/scap.c

jasondellaluce

Looks good so far! Left you some points.

jasondellaluce · 2023-02-13T12:01:08Z

userspace/libscap/scap.c

+{
+	gethostname(handle->m_machine_info.hostname, sizeof(handle->m_machine_info.hostname) / sizeof(handle->m_machine_info.hostname[0]));
+	char *env_hostname;
+	env_hostname = getenv("FALCO_HOSTNAME");


The env var must not be FALCO_ prefixed, but we'll need to make that a CMake overridable variable. Of course, for Falco as a consumer the name could be FALCO_HOSTNAME, but other libs consumers might want to have different namings.

Example of how we do this already for another env var:

libs/userspace/libscap/scap.c

Line 1316 in b9591d6

char* p = getenv(SCAP_HOST_ROOT_ENV_VAR_NAME);

libs/userspace/libscap/CMakeLists.txt

Line 44 in b9591d6

if(NOT DEFINED SCAP_HOST_ROOT_ENV_VAR_NAME)

jasondellaluce · 2023-02-13T12:06:06Z

userspace/libscap/scap.h

@@ -948,6 +948,24 @@ uint64_t scap_get_driver_api_version(scap_t* handle);
 */
 uint64_t scap_get_driver_schema_version(scap_t* handle);

+
+/**


is there a specific reason why are we exporting these functions in the libscap public API? Since we already have a getter to retrive machine info, and this info is populated in the handle at init time, I think these could just be internal helpers (scap_int.h maybe?). I would just avoid to further extend the API surface of libscap if not necessary, specially now that we're doing such a great job in its polishing process.

Agreed. Also, maybe it's a good time to factor out filling machine_info to a separate function (used for live/udig/nodriver engines)?

jasondellaluce · 2023-02-13T12:07:44Z

userspace/libscap/scap.h

-	uint64_t reserved2; ///< reserved for future use
-	uint64_t reserved3; ///< reserved for future use
-	uint64_t reserved4; ///< reserved for future use
+	uint64_t self_pid_start_ts; ///< Agent start ts in nanoseconds (epoch)


Sounds good, but I miss the context on why these were reserved. Maybe because of padding and backward compatibility with something?

My only concern, and broader question, is: will we be able to further grow this struct in the future, when we need more info?

AFAIK it's for future extensibility (so that we don't need another hack while reading scap files). Note that this patch breaks compatibility already (the old/new sizes are very much different) so we need changes to savefile code in this PR.

Personally, I'd rather:

add the string field after the currently reserved fields (let's keep the reserved ones for future integer vars)

use an uint64_t flags instead of a bool and #define a numeric constant for the one bit we need so far (BTW we can't use bool anyway since its size isn't guaranteed to be the same on all platforms and we save this struct directly to disk)

(deep in my patch queue there's another patch that adds stuff to machine_info; it needs just a few bits, like 3 or 4 I think, so maybe it could piggyback on the flags field without adding yet another one and using up a pretty scarce resource)

incertum · 2023-02-13T23:01:20Z

Thanks for the great initial feedback @gnosek and @jasondellaluce - I see the complications here, let's change it. I wasn't sure anyways where to put this data and how to best expose it with the bigger scap cleanup picture in mind. I like all of your suggestions, @gnosek I'll coordinate with you.

jasondellaluce

Left some more comments! Also, I'd put emphasis on the discussion at: https://github.com/falcosecurity/libs/pull/880/files#r1104380474

userspace/libscap/CMakeLists.txt

userspace/libscap/scap.c

incertum · 2023-03-25T23:35:47Z

Addressed reviewers comments including the request to cleanup machine_info retrieval via a clean function.

Would adding the new proposed scap_agent_info work? Could be that it may come in handy in the future for extensions, especially because of the limitations of scap_machine_info because of the scap files.

Plus because of the boot time hiccup decided to retrieve start_time from the agent stat file in order to base CPU usage calculations on that (meaning stat start_time will be used to calculate the elapsed time when referencing sysconf uptime) without needing to rely on procfs time stamps, but I kept that metric too so we can report a constant start timestamp of the agent as it should work on most systems.

userspace/libsinsp/sinsp.cpp

userspace/libscap/scap.h

jasondellaluce

Looks good Melissa, thanks for addressing all the concerns. I left few comments, but I think my only remaining big point is to avoid adding all those new functions in scap.h and just keep there const scap_agent_info* scap_get_agent_info(scap_t* handle);. You can either add those functions to scap-int.h or have them defined as static in scap.c, but my point is simply to avoid having those as part of the exported API unless we have a concrete use case.

userspace/libscap/scap.h

userspace/libsinsp/sinsp.cpp

jasondellaluce · 2023-03-28T10:33:12Z

userspace/libscap/scap.h

+ * where the hostname can be equivalent to the Kubernetes pod name.
+ * Customizable over cmake setup SCAP_HOSTNAME_ENV_VAR.
+ */
+void scap_gethostname(scap_t* handle);


I think this should go inside scap-int.h as well, since it's not really a getter (it contains the logic that sets the value). The hostname is available through the machine info struct anyways.

LGTM, this is my only remaining point.

Done, also had to rebase one more time.

additional machine related info useful for: * host attributions * host kernel attributions * identifiers for new resource utilization metrics logs Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

New uint64_t flag slated to be useful for future extensions, PPM_BPF_STATS_ENABLED as initial use case. Grzegorz also pointed out that a bool would not have been possible given the size isn't guaranteed to be the same on all platforms (scap file capture use case). Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Remove uname_r extraction and instead extract in Falco for resource utilization metrics logs. Minor additional cleanups and renaming. Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

jasondellaluce

/approve

poiana · 2023-03-28T15:26:06Z

LGTM label has been added.

Git tree hash: b36581820ae161c291a816f4a29e25ef1fa06605

Andreagit97

/approve

poiana · 2023-03-28T15:55:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, incertum, jasondellaluce

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:

~~OWNERS~~ [Andreagit97,incertum,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added release-note kind/cleanup dco-signoff: yes area/libscap labels Feb 13, 2023

poiana requested review from Andreagit97 and hbrueckner February 13, 2023 03:15

poiana added approved size/L labels Feb 13, 2023

incertum commented Feb 13, 2023

View reviewed changes

userspace/libscap/scap.c Outdated Show resolved Hide resolved

incertum mentioned this pull request Feb 13, 2023

new(libsinsp): introduce resource utilization snapshot metrics API (new metrics 2/n) #881

Merged

jasondellaluce reviewed Feb 13, 2023

View reviewed changes

FedeDP mentioned this pull request Feb 20, 2023

[NOMINATION] Falco contributor of the month - February falcosecurity/community#174

Closed

incertum force-pushed the scap-machine-info branch 2 times, most recently from 329cf89 to b4c1c49 Compare March 1, 2023 06:34

jasondellaluce reviewed Mar 6, 2023

View reviewed changes

userspace/libscap/CMakeLists.txt Outdated Show resolved Hide resolved

userspace/libscap/scap.c Outdated Show resolved Hide resolved

userspace/libscap/scap.c Outdated Show resolved Hide resolved

leogr added this to the 0.11.0 milestone Mar 22, 2023

incertum force-pushed the scap-machine-info branch from b4c1c49 to c654917 Compare March 25, 2023 23:29

Andreagit97 reviewed Mar 27, 2023

View reviewed changes

userspace/libsinsp/sinsp.cpp Show resolved Hide resolved

userspace/libscap/scap.h Outdated Show resolved Hide resolved

jasondellaluce reviewed Mar 27, 2023

View reviewed changes

userspace/libscap/scap.h Show resolved Hide resolved

userspace/libsinsp/sinsp.cpp Show resolved Hide resolved

poiana added size/XL and removed size/L labels Mar 27, 2023

incertum force-pushed the scap-machine-info branch 2 times, most recently from 537cb0b to e9d1d27 Compare March 27, 2023 23:17

jasondellaluce reviewed Mar 28, 2023

View reviewed changes

incertum and others added 4 commits March 28, 2023 15:10

cleanup(scap): extend scap machine info

c2227a3

additional machine related info useful for: * host attributions * host kernel attributions * identifiers for new resource utilization metrics logs Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libscap): add SCAP_HOSTNAME_ENV_VAR for hostname lookup

2da8212

Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libscap): do not add new char to scap_machine_info + cleanup

9ab7219

Remove uname_r extraction and instead extract in Falco for resource utilization metrics logs. Minor additional cleanups and renaming. Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum and others added 7 commits March 28, 2023 15:10

new(libsinsp, libscap): introduce scap_agent_info w/ initial metrics

ce27671

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libscap): address reviewers comments

acc6bc3

Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libscap): scap_retrieve_machine_info function

6ac1dc8

Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libscap): move new functions to scap-int

39adedc

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libsinsp): init m_agent_info

ed98eeb

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libscap): create dedicated scap_machine_agent

234dc6e

Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libscap): move scap_gethostname

f2ff449

Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum force-pushed the scap-machine-info branch from 9e50fae to f2ff449 Compare March 28, 2023 15:14

jasondellaluce approved these changes Mar 28, 2023

View reviewed changes

poiana assigned jasondellaluce Mar 28, 2023

poiana added the lgtm label Mar 28, 2023

incertum changed the title ~~cleanup(scap): extend scap machine info (new metrics 1/n)~~ new(scap): extend scap machine info, new agent info (new metrics 1/n) Mar 28, 2023

Andreagit97 approved these changes Mar 28, 2023

View reviewed changes

poiana assigned Andreagit97 Mar 28, 2023

poiana merged commit 2024af2 into falcosecurity:master Mar 28, 2023

incertum deleted the scap-machine-info branch December 8, 2023 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(scap): extend scap machine info, new agent info (new metrics 1/n) #880

new(scap): extend scap machine info, new agent info (new metrics 1/n) #880

incertum commented Feb 13, 2023 •

edited

Loading

jasondellaluce left a comment

jasondellaluce Feb 13, 2023

jasondellaluce Feb 13, 2023

gnosek Feb 13, 2023

jasondellaluce Feb 13, 2023

gnosek Feb 13, 2023

incertum commented Feb 13, 2023

jasondellaluce left a comment

incertum commented Mar 25, 2023 •

edited

Loading

jasondellaluce left a comment

jasondellaluce Mar 28, 2023

incertum Mar 28, 2023

jasondellaluce left a comment

poiana commented Mar 28, 2023

Andreagit97 left a comment

poiana commented Mar 28, 2023

new(scap): extend scap machine info, new agent info (new metrics 1/n) #880

new(scap): extend scap machine info, new agent info (new metrics 1/n) #880

Conversation

incertum commented Feb 13, 2023 • edited Loading

jasondellaluce left a comment

Choose a reason for hiding this comment

jasondellaluce Feb 13, 2023

Choose a reason for hiding this comment

jasondellaluce Feb 13, 2023

Choose a reason for hiding this comment

gnosek Feb 13, 2023

Choose a reason for hiding this comment

jasondellaluce Feb 13, 2023

Choose a reason for hiding this comment

gnosek Feb 13, 2023

Choose a reason for hiding this comment

incertum commented Feb 13, 2023

jasondellaluce left a comment

Choose a reason for hiding this comment

incertum commented Mar 25, 2023 • edited Loading

jasondellaluce left a comment

Choose a reason for hiding this comment

jasondellaluce Mar 28, 2023

Choose a reason for hiding this comment

incertum Mar 28, 2023

Choose a reason for hiding this comment

jasondellaluce left a comment

Choose a reason for hiding this comment

poiana commented Mar 28, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Mar 28, 2023

incertum commented Feb 13, 2023 •

edited

Loading

incertum commented Mar 25, 2023 •

edited

Loading