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.

Sign up for GitHub

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

Jump to bottom

feat(tests): new e2e tests [3/N] #1720

Merged
merged 12 commits into from
Mar 22, 2024
Merged

feat(tests): new e2e tests [3/N] #1720

merged 12 commits into from
Mar 22, 2024

Conversation

therealbobo
Copy link
Contributor

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:

This add e2e tests related to paths.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@therealbobo therealbobo changed the title feat(tests): new e2e tests [3/N] [WIP] feat(tests): new e2e tests [3/N] Mar 1, 2024
@poiana poiana added the size/XXL label Mar 1, 2024
@poiana poiana requested review from hbrueckner and incertum March 1, 2024 10:42
incertum
incertum reviewed Mar 1, 2024
View reviewed changes
test/libsinsp_e2e/CMakeLists.txt Outdated Show resolved Hide resolved
test/libsinsp_e2e/container_cgroup.cpp Outdated

using namespace libsinsp::runc;

constexpr const cgroup_layout CRI_CGROUP_LAYOUT[] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is re-defining those in the test setup not defying the purpose of e2e tests? We should import the definitions from libs instead.

test/libsinsp_e2e/container_image_splitting.cpp Outdated Show resolved Hide resolved
test/libsinsp_e2e/thread_state.cpp Outdated
// This just verifies that the mechanism of wait_for with a
// timeout actually works in the face of a thread that never
// stops
void loop_almost_forever()
Copy link
Contributor

Choose a reason for hiding this comment

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

😄

test/libsinsp_e2e/docker_utils.cpp Outdated

using namespace std;

bool dutils_check_docker()
Copy link
Contributor

Choose a reason for hiding this comment

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

[Outlook]: I believe we could easily add similar tests for containerd and cri-o in follow up PRs, and simulate running pod sand boxes using crictl https://github.com/kubernetes-sigs/cri-tools/blob/master/docs/crictl.md -> It never fails me for quick testing and is so much easier than requiring an entire Kubernetes setup.

test/libsinsp_e2e/resources/docker/large_container_dockerfiles/Dockerfile.long_labels Outdated Show resolved Hide resolved
test/libsinsp_e2e/container.cpp Outdated Show resolved Hide resolved
test/libsinsp_e2e/container.cpp Outdated
before_close_t cleanup = [&](sinsp* inspector)
{ inspector->set_docker_socket_path("/var/run/docker.sock"); };

ASSERT_NO_FATAL_FAILURE({ event_capture::run(test, callback, filter, setup, cleanup); });
Copy link
Contributor

Choose a reason for hiding this comment

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

Also in this test container_docker_bad_socket could we at least assert that the socket is bad and what else would make sense to assert (for example assert that container_info->m_image is empty etc)? It seems like the exact same assertions as for the successful docker socket, which is what we want to assert 100% as well.

@incertum incertum mentioned this pull request Mar 1, 2024
Merged
@Andreagit97 Andreagit97 added this to the TBD milestone Mar 11, 2024
@therealbobo therealbobo changed the title [WIP] feat(tests): new e2e tests [3/N] feat(tests): new e2e tests [3/N] Mar 13, 2024
@therealbobo therealbobo marked this pull request as ready for review March 13, 2024 14:40
@poiana poiana requested a review from Molter73 March 13, 2024 14:40
@therealbobo therealbobo force-pushed the new-e2e-tests-3 branch 13 times, most recently from 5d60c0b to d05d092 Compare March 20, 2024 12:20
FedeDP
FedeDP approved these changes Mar 22, 2024
View reviewed changes
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 poiana added the lgtm label Mar 22, 2024
@poiana
Copy link
Contributor

poiana commented Mar 22, 2024

LGTM label has been added.

Git tree hash: c8a2b6571169d5eabd3345a0662bdaea5e1a07c8

@FedeDP
Copy link
Contributor

FedeDP commented Mar 22, 2024

/milestone 0.16.0

@poiana poiana modified the milestones: TBD, 0.16.0 Mar 22, 2024
Andreagit97
Andreagit97 reviewed Mar 22, 2024
View reviewed changes
userspace/libsinsp/container.cpp
Comment on lines +274 to +275
uint32_t json_len = json.length() + 1;
size_t totlen = sizeof(scap_evt) + sizeof(uint32_t) + json_len;
Copy link
Member

Choose a reason for hiding this comment

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

is there are reason behind this change?

Copy link
Contributor Author

@therealbobo therealbobo Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That fixes a misaligned access (discovery provided by ASAN :) )

Copy link
Member

Choose a reason for hiding this comment

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

uhm got it!

test/libsinsp_e2e/container/container.cpp Show resolved Hide resolved
Andreagit97
Andreagit97 approved these changes Mar 22, 2024
View reviewed changes
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

@poiana
Copy link
Contributor

poiana commented Mar 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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 4686acb into falcosecurity:master Mar 22, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@incertum incertum incertum left review comments

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@FedeDP FedeDP FedeDP approved these changes

@hbrueckner hbrueckner Awaiting requested review from hbrueckner

@Molter73 Molter73 Awaiting requested review from Molter73

Assignees

@FedeDP FedeDP

@Andreagit97 Andreagit97

Labels
approved area/tests dco-signoff: yes kind/feature New feature or request lgtm release-note-none size/XXL
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.
5 participants
@therealbobo @poiana @FedeDP @incertum @Andreagit97