Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Silence missing BPF program error #1393

Merged
merged 1 commit into from
Feb 27, 2024
Merged

Silence missing BPF program error #1393

merged 1 commit into from
Feb 27, 2024

Conversation

erthalion
Copy link
Contributor

@erthalion erthalion commented Oct 6, 2023

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:

PR #943 has introduced ability to exclude a set of tail-called programs from loading. Yet current such excluded program will generate an error message, which could be misleading. Downgrage the error message to the debug level.

Which issue(s) this PR fixes:

Fixes #943

Special notes for your reviewer:

PR also extends the logging functionality a bit to be able to specify the logging level.

Does this PR introduce a user-facing change?:

NONE

@Andreagit97
Copy link
Member

Ideally we would like to keep such message, but there is no logger there in libpman. I'm going to work on a follow-up PR to introduce logging capabilities, consistent with other libraries, then reintroduce this message at the debug level.

Maybe we could replace this message when we will have the logging capability in place, in this way, we don't lose this info for an undefined period of time...WDYT? BTW thank you for working on this, a logging framework is something we really need in scap/libpman!

Just a minor note: in theory, we could remove libpman now, if it requires extra work in this logging task. libpman was used as an helper library in the initial modern bpf testing framework but now that we have the v-table and the engine architecture, this library is no longer necessary

@Andreagit97 Andreagit97 added this to the TBD milestone Oct 6, 2023
@erthalion
Copy link
Contributor Author

Ideally we would like to keep such message, but there is no logger there in libpman. I'm going to work on a follow-up PR to introduce logging capabilities, consistent with other libraries, then reintroduce this message at the debug level.

Maybe we could replace this message when we will have the logging capability in place, in this way, we don't lose this info for an undefined period of time...WDYT? BTW thank you for working on this, a logging framework is something we really need in scap/libpman!

My intention here is to optimize the state of things short-term as well. The logging message already produces confusion among users, while bringing value in form of debugging information. It would take some time to introduce a proper logging, and while the downsides are outweighing the advantages, it makes sense to me to turn it off for now. Does it sound reasonable for you, folks?

Just a minor note: in theory, we could remove libpman now, if it requires extra work in this logging task. libpman was used as an helper library in the initial modern bpf testing framework but now that we have the v-table and the engine architecture, this library is no longer necessary

Sounds interesting, but to be honest I haven't followed the recent development so closely, and need to think about this possibility. Generally speaking less moving parts the better of course.

@FedeDP
Copy link
Contributor

FedeDP commented Oct 12, 2023

Hey, it seems like someone heard your prayers 😆 #1395

Feel free to leave your review over there!

@erthalion
Copy link
Contributor Author

Hey, it seems like someone heard your prayers 😆 #1395

Prayers is a strong word, but it's nice to see this development :)

@FedeDP
Copy link
Contributor

FedeDP commented Oct 24, 2023

I think this can now be improved to log a debug message since #1395 was merged :)

@poiana
Copy link
Contributor

poiana commented Jan 22, 2024

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@poiana
Copy link
Contributor

poiana commented Feb 21, 2024

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

@FedeDP
Copy link
Contributor

FedeDP commented Feb 21, 2024

/remove-lifecycle rotten

@erthalion news? :)

@erthalion
Copy link
Contributor Author

@erthalion news? :)

Lots of news, which one you would like to hear? :)

Haven't had a chance yet to rework this one, give me a couple of days.

@FedeDP
Copy link
Contributor

FedeDP commented Feb 21, 2024

Lots of news, which one you would like to hear? :)

Ahahaha hope they are good news :D

Haven't had a chance yet to rework this one, give me a couple of days.

No problem!

@poiana poiana added size/S and removed size/XS labels Feb 26, 2024
@erthalion
Copy link
Contributor Author

@FedeDP this should look better now. Not sure, is there anything needed to trigger the tests?

@FedeDP
Copy link
Contributor

FedeDP commented Feb 27, 2024

Not sure, is there anything needed to trigger the tests?

Just me pushing the button heheh

@@ -46,7 +46,23 @@ static void log_error(const char* fmt, ...)
va_end(args);
}

static void log_error(const char* fmt, ...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop this one now ;)

@@ -25,7 +25,7 @@ limitations under the License.

struct internal_state g_state = {};

static void log_error(const char* fmt, ...)
static void log_msg(enum falcosecurity_log_severity level, const char* fmt, ...)
Copy link
Member

Choose a reason for hiding this comment

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

we need to use the level inside this function, atm it's not used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/me facepalming

Sure, give me a moment.

PR #943 has introduced ability to exclude a set of
tail-called programs from loading. Yet current such excluded program
will generate an error message, which could be misleading. Extend the
logging infrastructure to be able to use a specified log level and
downgrade the message to the debug level.

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

I like this!
/approve

@poiana
Copy link
Contributor

poiana commented Feb 27, 2024

LGTM label has been added.

Git tree hash: 4c7cedd301e0f4073123e462fa6d7b3cbf3f286f

@FedeDP
Copy link
Contributor

FedeDP commented Feb 27, 2024

/milestone 0.15.0

@poiana poiana modified the milestones: TBD, 0.15.0 Feb 27, 2024
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve
Thank you!

@poiana
Copy link
Contributor

poiana commented Feb 27, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, erthalion, FedeDP

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:

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

@poiana poiana merged commit 102bc0f into falcosecurity:master Feb 27, 2024
41 checks passed
@erthalion
Copy link
Contributor Author

Thanks, @FedeDP @Andreagit97 !

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

Successfully merging this pull request may close these issues.

4 participants