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

add tcpdump wrapper #873

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

add tcpdump wrapper #873

wants to merge 11 commits into from

Conversation

gytsto
Copy link
Contributor

@gytsto gytsto commented Oct 15, 2024 •
edited
Loading

Description

  • add multi-OS tcpdump wrapper
  • add DO_NOT_KILL prefix to process kill_id, so it won't be killed during cleanup before each test
  • make dns-server pcaps session wide
  • unify GITLAB_CI usage between code, previously was GITLAB_CI and CUSTOM_ENV_GITLAB_CI
  • add Connection name to Process, so errors would be more clear

☑️ Definition of Done checklist

  • Commit history is clean (requirements)
  • README.md is updated
  • Functionality is covered by unit or integration tests

@gytsto gytsto requested a review from a team as a code owner October 15, 2024 13:51
@gytsto gytsto force-pushed the LLT-5690_natlab_tcpdump_wrapper branch from 7e73f4b to 79a26af Compare November 28, 2024 07:54
@gytsto gytsto force-pushed the LLT-5690_natlab_tcpdump_wrapper branch from 79a26af to 5c23d78 Compare December 3, 2024 07:57
@gytsto gytsto force-pushed the LLT-5690_natlab_tcpdump_wrapper branch from 5c23d78 to 7eca67a Compare December 3, 2024 08:09
@gytsto gytsto force-pushed the LLT-5690_natlab_tcpdump_wrapper branch from 7eca67a to 6a1e004 Compare December 3, 2024 08:10
@gytsto gytsto changed the title add tcpdump wrapper skeleton add tcpdump wrapper Dec 3, 2024
@gytsto gytsto force-pushed the LLT-5690_natlab_tcpdump_wrapper branch from ee1b2cb to 4763432 Compare December 4, 2024 11:22
@gytsto gytsto force-pushed the LLT-5690_natlab_tcpdump_wrapper branch from 58972bf to 6c67199 Compare December 10, 2024 10:04
@gytsto gytsto force-pushed the LLT-5690_natlab_tcpdump_wrapper branch from 6874c3b to e973bdf Compare December 10, 2024 14:41
@gytsto gytsto force-pushed the LLT-5690_natlab_tcpdump_wrapper branch from 0582d52 to f9eb323 Compare December 11, 2024 14:38
Jauler
Jauler reviewed Dec 17, 2024
View reviewed changes
nat-lab/bin/cleanup_natlab_processes
@@ -4,12 +4,16 @@ echo "Executing natlab process cleanup script"

for pid in $(ps -e -o pid=); do
# Skip non-testing processes
if ! grep --null-data --text KILL_ID /proc/${pid}/environ; then
if ! grep --null-data --text "KILL_ID" /proc/${pid}/environ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Maybe it would be worth simply skipping KILL_ID instead of adding KILL_ID and then separately adding DO_NOT_KILL?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note this is rather a nitpick, so it is up to you to decide whether this is relevant enough to fix.

Jauler
Jauler reviewed Dec 17, 2024
View reviewed changes
nat-lab/tests/conftest.py
if not isinstance(connections[0], DockerConnection):
raise Exception("Not docker connection")

async with make_tcpdump(connections):
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking, maybe it would make sense to combine packet capture started in setup_check_interderp() with the general one started in pytest_sessionstart? Or are those non-overlapping in time? E.g. by adding DERP servers to the list here: https://github.com/NordSecurity/libtelio/pull/873/files#diff-0433c1e25b1d0e1b02e84b811ebf3db74a6d26d59f6439e210b242377d92836aR421

Copy link
Contributor

Choose a reason for hiding this comment

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

Note this is rather a nitpick, so it is up to you to decide whether this is relevant enough to fix.

Jauler
Jauler approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@Jauler Jauler left a comment

Choose a reason for hiding this comment

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

+1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jauler Jauler Jauler approved these changes

@jjanowsk jjanowsk Awaiting requested review from jjanowsk jjanowsk is a code owner

@packgron packgron Awaiting requested review from packgron packgron is a code owner

@tomaszklak tomaszklak Awaiting requested review from tomaszklak tomaszklak is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gytsto @Jauler