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

feat(tests): new e2e tests [1/N] #1661

Merged
merged 22 commits into from
Feb 21, 2024
Merged

Conversation

therealbobo
Copy link
Contributor

@therealbobo therealbobo commented Jan 30, 2024

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 is the initial batch of new e2e tests.

Which issue(s) this PR fixes:

Part of #1650

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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.

Hey @therealbobo

Can you rebase this on the master?

This #1664 should fix the failing tests. 🙏

leogr
leogr previously approved these changes Feb 1, 2024
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.

🥳

@poiana poiana added the lgtm label Feb 1, 2024
@poiana
Copy link
Contributor

poiana commented Feb 1, 2024

LGTM label has been added.

Git tree hash: 472ea3c0f282d766bada55f6a7991b1df036dc27

@poiana poiana added the approved label Feb 1, 2024
@Andreagit97
Copy link
Member

/hold i would like to review this before merging it

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
test/libsinsp/CMakeLists.txt Outdated Show resolved Hide resolved
// Absolute path to the bpf probe .o file
#define LIBSINSP_TEST_BPF_PROBE_PATH "${CMAKE_BINARY_DIR}/driver/bpf/probe.o"

#define LIBSINSP_TEST_CAPTURES_PATH "${CMAKE_BINARY_DIR}/test/libsinsp/captures"
Copy link
Member

Choose a reason for hiding this comment

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

This seems relative to scap-files, but we already have a framework to support them https://github.com/falcosecurity/libs/tree/master/userspace/libsinsp/test/scap_files, maybe it could be enough WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LIBSINSP_TEST_CAPTURES_PATH has a different motivation: each test dumps the captured events in a scap file so that we can inspect them in a second moment. This is not meant to test scap file correctness. 😅

Comment on lines 221 to 229
std::string captures_dir = LIBSINSP_TEST_CAPTURES_PATH;

if(!std::filesystem::exists(captures_dir))
{
if (!std::filesystem::create_directory(captures_dir)) {
std::cerr << "Failed to create captures directory." << std::endl;;
return EXIT_FAILURE;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's see what we decide to do with scap-files

Comment on lines 156 to 162
/* Get current cwd as a base directory for the driver path */
char driver_path[FILENAME_MAX];
if(!getcwd(driver_path, FILENAME_MAX))
{
std::cerr << "Unable to get current dir" << std::endl;
return EXIT_FAILURE;
}
Copy link
Member

Choose a reason for hiding this comment

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

it seems we can remove this

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a README.md in this folder with instructions to run these tests?

Copy link
Member

Choose a reason for hiding this comment

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

More precisely I would try to follow the same structure we have in the other tests (e.g. the drivers one):

/libsinsp_e2e
├─ `helpers`
├─ `test_suites`
├─ `event_capture`
├─ `CMakeLists.txt`
├─ `README.md`
├─ ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, totally on board on this! On the other hand I'd like to do this kind on changes later on (I already have some PRs in the working). Are you ok with that?

Comment on lines 113 to 126
- name: Run e2e tests with modern bpf 🏎️
run: |
cd build/test/libsinsp/
sudo ./tests -m

- name: Run e2e tests with bpf 🏎️
run: |
cd build/test/libsinsp/
sudo ./tests -b

- name: Run e2e tests with kmod 🏎️
run: |
cd build/test/libsinsp/
sudo ./tests -k
Copy link
Member

Choose a reason for hiding this comment

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

On QEMU we should be able to inject eBPF drivers so I don't think we need to touch this job

Copy link
Contributor

Choose a reason for hiding this comment

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

Another suggestion I'd add here is that we may want to run the tests on different drivers by using a matrix without soft fail, so that we don't skip tests on a driver if we find errors in another.


using namespace std;

TEST_F(sys_call_test, stat)
Copy link
Member

Choose a reason for hiding this comment

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

Uhm, all the tests in this file seem to do the exact thing as drivers_test.
They run a syscall or a combination of syscalls and then assert that the event is here and something is happening in userspace... why don't we extend the class event_test to support some inspections in sinsp instead of introducing all these executables/classes + callbacks, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Although some tests are similar in this first batch, the whole contribution process of the test suite we foresee can potentially cover use cases that diverge with the goals of the driver-level tests. It's hard to predict it at the moment, and refactor everything right now may be outside of the engineering bandwidth we currently have available. Plus, having this suite being its own standalone thing will allow us to converge it with the drivers one once we have a more high-level view than now. If this was a library code contribution, I'd agree with you of this being a strong blocker. But since this is a test-based contribution, I think it can provide tons of value as-is and we can postpone cleaning it up deeply as a secondary priority. Moreover @therealbobo is also curating each test while contributing them.

@Andreagit97
Copy link
Member

I left some initial comments but my main point is this one #1661 (comment)

@LucaGuerra
Copy link
Contributor

When attempting to build the test suite I get

In file included from /usr/include/features.h:392,
                 from /usr/include/arpa/inet.h:21,
                 from /home/ubuntu/dev/falcosecurity/libs/test/libsinsp/test_helper.cpp:1:
/usr/include/features-time64.h:20:10: fatal error: bits/wordsize.h: No such file or directory
   20 | #include <bits/wordsize.h>
      |          ^~~~~~~~~~~~~~~~~
compilation terminated.

Am I missing some dependency?

@therealbobo
Copy link
Contributor Author

This is probably related to the build of the is needed gcc with multilib support: there are some tests that run also for x86.

@LucaGuerra
Copy link
Contributor

Yep, I was missing gcc-multilib and g++-multilib. I would suggest documenting how to run these tests in the libs README (we can mark them as experimental)

@therealbobo therealbobo force-pushed the new-e2e-tests branch 2 times, most recently from 7aa8ee4 to a242e36 Compare February 7, 2024 14:54
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Copy link
Contributor

@mrgian mrgian left a comment

Choose a reason for hiding this comment

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

Probably just a matter of setting the right directory

Hi @LucaGuerra
You are right, it's just a path issue caused by the new name of the test's directory.
I've tried running these tests with the correct paths and they are working 🎉

test/libsinsp_e2e/sys_call_test.cpp Outdated Show resolved Hide resolved
test/libsinsp_e2e/sys_call_test.cpp Outdated Show resolved Hide resolved
@LucaGuerra
Copy link
Contributor

I had tested this before and it seems like cmake had issues generating the binary, I'll clean and retry. Thank you for your efforts in making the e2e tests more compatible!

Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Copy link
Contributor

@LucaGuerra LucaGuerra left a comment

Choose a reason for hiding this comment

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

After the latest fixes I was able to run the tests as specified in the readme. Thanks!

@poiana
Copy link
Contributor

poiana commented Feb 21, 2024

LGTM label has been added.

Git tree hash: 7d47e4c299049d641e824eed88027c0d654f1548

@poiana
Copy link
Contributor

poiana commented Feb 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leogr, LucaGuerra, 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

@LucaGuerra
Copy link
Contributor

/unhold

I believe review comments have been addressed

@poiana poiana merged commit d8bfc3c into falcosecurity:master Feb 21, 2024
42 checks passed
@Andreagit97 Andreagit97 added this to the 0.15.0 milestone Feb 21, 2024
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.

7 participants