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

new(plugins/k8smeta): update k8smeta plugin to require plugin API version 3.9.0. #625

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Feb 13, 2025

What type of PR is this?

/kind cleanup
/kind feature

Any specific area of the project related to this PR?

/area plugins

What this PR does / why we need it:

Implemented the "suggested output fields feature", by suggesting k8smeta.pod.name and k8smeta.ns.name as output fields.

Also, entirely avoid the proc scan, instead relying on the listening CAPability to initially loop over the thread table to attach pod_uid to threads.
Following the above, hostProc initConfig key is now deprecated and unused.

Moved the plugin to 0.3.0 too (given the minimum plugin API version changes).

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@poiana
Copy link
Contributor

poiana commented Feb 13, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: FedeDP
Once this PR has been reviewed and has the lgtm label, please assign jasondellaluce 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:

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

@poiana poiana added the size/XL label Feb 13, 2025
@@ -6,7 +6,7 @@ message(
FetchContent_Declare(
plugin-sdk-cpp
GIT_REPOSITORY https://github.com/falcosecurity/plugin-sdk-cpp.git
GIT_TAG 2097bdb5a5d77f3f38162da1f438382912465340)
GIT_TAG 0.2.0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use tagged sdk-cpp version.

@@ -138,8 +138,8 @@ falcosecurity::init_schema my_plugin::get_init_schema()
},
"hostProc": {
"type": "string",
"title": "Path to reach the '/proc' folder we want to scan.",
"description": "The plugin needs to scan the '/proc' of the host on which is running. In Falco usually we put the host '/proc' folder under '/host/proc' so the the default for this config is '/host'. The path used here must not have a final '/'."
"title": "[DEPRECATED] Path to reach the '/proc' folder we want to scan.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecated initConfig key, unused now.

Copy link
Member

Choose a reason for hiding this comment

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

Do you confirm that this setting won't produce any effect anymore?

If so, we should add a notice in addition to the deprecation. Make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -428,11 +295,20 @@ bool my_plugin::init(falcosecurity::init_input& in)
try
{
m_thread_table = t.get_table(THREAD_TABLE_NAME, st::SS_PLUGIN_ST_INT64);

// get the 'cgroups' field accessor from the thread table
m_thread_field_cgroups = m_thread_table.get_field(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need these to be able to correctly fetch cgroups string from the thread entry in the capture_open() listen CAP callback.

SPDLOG_DEBUG("enriching initial thread table entries");
auto& tr = in.get_table_reader();
auto& tw = in.get_table_writer();
m_thread_table.iterate_entries(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We iterate over all thread table entries, and for each, we iterate over its cgroups vector, taking the ->second (cgroups are a vector of std::pair and second hosts the cgroup path, while first hosts the cgroup controller); for each cgroup path we try to extract the pod_uid and, if successful, we break the loop for the current thread entry.

@@ -520,7 +438,7 @@ std::vector<falcosecurity::field_info> my_plugin::get_fields()
// Use an array to perform a static_assert one the size.
const falcosecurity::field_info fields[] = {
{ft::FTYPE_STRING, "k8smeta.pod.name", "Pod Name",
"Kubernetes pod name."},
"Kubernetes pod name.", {}, false, {}, true}, // use as suggested output format
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mark this field as suggested output format.

@@ -535,7 +453,7 @@ std::vector<falcosecurity::field_info> my_plugin::get_fields()
{ft::FTYPE_STRING, "k8smeta.pod.ip", "Pod Ip", "Kubernetes pod ip"},

{ft::FTYPE_STRING, "k8smeta.ns.name", "Namespace Name",
"Kubernetes namespace name."},
"Kubernetes namespace name.", {}, false, {}, true}, // use as suggested output format
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mark this field as suggested output format.

…sion 3.9.0.

Implement the suggested output fields feature,
by suggesting `k8smeta.pod.name` and `k8smeta.ns.name` as output fields.

Also, entirely avoid the proc scan, instead relying on the listening CAPability to
initially loop over the thread table to attach pod_uid to threads.

`hostProc` initConfig key is now deprecated and unused.

Moved the plugin to 0.3.0 too.

Signed-off-by: Federico Di Pierro <[email protected]>
@FedeDP FedeDP force-pushed the new/k8smeta_update branch from e7b1571 to f8c60d8 Compare February 13, 2025 09:59
@@ -282,3 +286,4 @@ FALCOSECURITY_PLUGIN(my_plugin);
FALCOSECURITY_PLUGIN_FIELD_EXTRACTION(my_plugin);
FALCOSECURITY_PLUGIN_ASYNC_EVENTS(my_plugin);
FALCOSECURITY_PLUGIN_EVENT_PARSING(my_plugin);
FALCOSECURITY_PLUGIN_CAPTURE_LISTENING(my_plugin);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New capability.

@@ -245,7 +250,7 @@ class my_plugin
private:
// Async thread
std::thread m_async_thread;
std::atomic<bool> m_async_thread_quit;
std::atomic<bool> m_async_thread_quit = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compiler warning.

Copy link

Rules files suggestions

Copy link

Rules files suggestions

Copy link

Rules files suggestions

1 similar comment
Copy link

Rules files suggestions

@FedeDP FedeDP force-pushed the new/k8smeta_update branch from e90d163 to d8815ba Compare February 13, 2025 13:45
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 13, 2025

/hold

Copy link

Rules files suggestions

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Nice!

Just left two suggestions. I will do a second pass later.

Thank you

@@ -138,8 +138,8 @@ falcosecurity::init_schema my_plugin::get_init_schema()
},
"hostProc": {
"type": "string",
"title": "Path to reach the '/proc' folder we want to scan.",
"description": "The plugin needs to scan the '/proc' of the host on which is running. In Falco usually we put the host '/proc' folder under '/host/proc' so the the default for this config is '/host'. The path used here must not have a final '/'."
"title": "[DEPRECATED] Path to reach the '/proc' folder we want to scan.",
Copy link
Member

Choose a reason for hiding this comment

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

Do you confirm that this setting won't produce any effect anymore?

If so, we should add a notice in addition to the deprecation. Make sense?

Moreover, dropped proc-scan related tests.

Signed-off-by: Federico Di Pierro <[email protected]>
@FedeDP FedeDP force-pushed the new/k8smeta_update branch from d8815ba to fcba3e4 Compare February 13, 2025 14:50
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 13, 2025

@leogr pushed changes to address your review! Thanks!

Copy link

Rules files suggestions

@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 28, 2025

Also, we might want to use the provided logger by the sdk, like i did here for the container plugin: FedeDP/container_plugin#26

This would allow use to drop the spdlog dep, to have better unified logging, and to drop the verbosity init config key, since we would then use the verbosity used for libs logger (https://github.com/falcosecurity/libs/blob/3d1d4a930d3a16f3e72ab62876ba0837c02d2ec5/userspace/libsinsp/plugin.cpp#L79).

I will make the change in its own commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants