libs
libs copied to clipboard
falcosecurity
feat(libsinsp/container_info): change default / init lookup state to `FAILED`
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
/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:
While working on https://github.com/falcosecurity/libs/pull/1595 it became evident that the init state is set wrongly. Curious if something breaks, if it does we likely will have found more improvement areas in regards to the container engine in general.
Opened it again against the libs repo directly to allow for easier testing.
CC @therealbobo @leogr @deepskyblue86
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
NONE
Opened it again against the libs repo directly to allow for easier testing.
It seems at least 3 tests are failing:
[ FAILED ] sinsp_with_test_input.event_sources
[ FAILED ] sinsp_with_test_input.container_parser_cri_containerd
[ FAILED ] sinsp_with_test_input.container_parser_cri_crio
Still trying to figure out where the problem is: the change or the tests? :thinking:
We first wanted to get the other container PRs in. They are all merged now. Let me rebase today and check.
Unit tests just needed to be updated w/ set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL).
LGTM label has been added.
Wasn't aware we were gonna move already forward with this PR. In turns out that all the non primary container engines never explicitly set the status (see last commit) ... probably why the default came from. I believe this is a great first step towards more transparent lookup status handling and please keep in mind that there is an open ticket for a more comprehensive future refactor #1708.
This PR hadn't a milestone, but I guess it should be for the 0.16 (since it's too late for 0.15, further testing and another round of maintainers' opinion is likely needed cc @falcosecurity/libs-maintainers)
/milestone 0.16
Let me know if you disagree.
@leogr: The provided milestone is not valid for this repository. Milestones in this repository: [0.15.0, 0.16.0, TBD, next-driver]
Use /milestone clear to clear the milestone.
In response to this:
Wasn't aware we were gonna move already forward with this PR. In turns out that all the non primary container engines never explicitly set the status (see last commit) ... probably why the default came from. I believe this is a great first step towards more transparent lookup status handling and please keep in mind that there is an open ticket for a more comprehensive future refactor #1708.
This PR hadn't a milestone, but I guess it should be for the 0.16 (since it's too late for 0.15, further testing and another round of maintainers' opinion is likely needed cc @falcosecurity/libs-maintainers)
/milestone 0.16
Let me know if you disagree.
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.
@leogr: The provided milestone is not valid for this repository. Milestones in this repository: [
0.15.0,0.16.0,TBD,next-driver]Use
/milestone clearto clear the milestone.
sorry
/milestone 0.16.0
Exactly @leogr there was never a milestone since it's not particularly urgent from a functionality point of view, but it's one step towards making sure we don't break the container engine every time we try to touch it. It also aligns with https://github.com/falcosecurity/libs/issues/1747 (better documentation, more transparency). Furthermore, it's the first step towards improving the container information lookup functionality, see https://github.com/falcosecurity/libs/issues/1708, more to follow.
I manually tested these changes for docker and CRI ...
LGTM label has been added.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: FedeDP, incertum, LucaGuerra
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [FedeDP,LucaGuerra,incertum]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment