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

fix(userspace/libsinsp): not report container id as host on failed lookups #1604

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

jasondellaluce
Copy link
Contributor

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area libsinsp

Does this PR require a change in the driver versions?

What this PR does / why we need it:

Which issue(s) this PR fixes:

On failed container info lookups, the container ID stored in each thread info can potentially be marked as empty. However, the container.id field currently reports empty container IDs as host value, thus making impossible to distinguish actual threads running on the host from threads running in containers for which the metadata lookup failed.

This attempts mitigating the issue by also checking that the thread is running outside of a namespace for container.id. As such, failed container ID lookups will return an empty string instead of host.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(userspace/libsinsp): not report container id as host on failed lookups

…okups

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@jasondellaluce
Copy link
Contributor Author

cc @Andreagit97

@poiana poiana added the size/XS label Dec 22, 2023
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 for the fix! the ideal fix would be to extract the container-id directly from the cgroups even when no container engines are configured, but not sure if this is really feasible, for sure it deserves some investigation!

@poiana
Copy link
Contributor

poiana commented Dec 22, 2023

LGTM label has been added.

Git tree hash: e9478f953ee656983b6baed5860a8dfade666687

@Andreagit97 Andreagit97 added this to the 0.14.1 milestone Dec 22, 2023
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.

/approve

@poiana
Copy link
Contributor

poiana commented Dec 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, 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,FedeDP,jasondellaluce]

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 2d6ccce into master Dec 22, 2023
@poiana poiana deleted the fix/container-id-wrong-host branch December 22, 2023 14:35
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.

4 participants