rust-pcap / pcap

Rust language pcap library
Apache License 2.0
595 stars 138 forks source link

Initial Linux-only virtual interface testing #285

Closed robs-zeynet closed 1 year ago

robs-zeynet commented 1 year ago

This is an end-to-end test of rust pcap and the underlying libpcap and OS. Currently it only works on Linux (and maybe MacOS), must be run with root (at least CAP_NET_ADMIN) permissions.

The test creates a 'TAP' interface (https://en.wikipedia.org/wiki/TUN/TAP) which is a virtual NIC on one side and a file descriptor on the otherside (to fascilitate testing).

I've hidden it behind a 'tap-tests' cargo feature try to prevent people from accidentally compile it (which requires the 'tun-tap' and 'etherparse' additional dev dependencies) or running it without meeting the above requirements.

Current test just verifies that packets can be sent and received, e.g., do Capture::next_packet() and Capture::sendpacket() do the right things.

A fair bit of work is done to avoid race conditions which can be horrible for these types of tests.

[robs@laptop-o7rs71ij pcap]$ cargo test -F tap-tests --no-run --test
tap_tests | tail
    Finished test [unoptimized + debuginfo] target(s) in 0.05s
  Executable tests/tap_tests.rs
(target/debug/deps/tap_tests-232532a0859d5255)
[robs@laptop-o7rs71ij pcap]$ sudo
./target/debug/deps/tap_tests-232532a0859d5255

running 1 test
test tests::conntrack_tap_basic ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered
out; finished in 0.54s
Wojtek242 commented 1 year ago

Thanks for the PR @robs-zeynet! I think this is a really valuable addition to the repository! Sorry for the late reply.

I would like to merge this contribution straight away, but CI is stopping me. It appears that due the use of the --all-features flag it's catching your new feature flag and running it as well. This doesn't work in the CI since it doesn't have root and you use features to new for the oldest rust version we support. However, the test is still valuable to run manually and as a reference and example for further tests of this sort so I don't see that as a problem.

Having said that, I would like to continue using the --all-features flag in CI. Do you see any other way of excluding this test when --all-features is enabled?

robs-zeynet commented 1 year ago

Hello and thank you for the kind words.

One way to work around the --all-features issue - perhaps a bit hacky - would be to create a second feature (maybe called "all-features"?) and wrap the test in an:

#[cfg(all(feature = "tap-tests", not(feature = "all-features))]

that way the code would only trigger when 'tap-tests' was enabled and ''all-features" was not. Sound ok?

Just pushed a patch on top of the existing one to fix the build - let me know what you think.

Wojtek242 commented 1 year ago

Uff, this is a tricky one. Some of the new dev-dependencies don't work with out MSRV. I've played around on my machine and what I think should work is the following:

  1. Move etherparse and tun-tap to [dependencies] and mark them optional = true (you cannot mark `[dev-dependencies] optional).
  2. Make the tap-tests feature depend on etherparse and tun-tap.

It's a shame that this won't work with dev-dependencies. I'll think about a better long-term solution, or perhaps just wait until it's time to bump the MSRV.

stappersg commented 1 year ago

minimum supported Rust version (MSRV)

robs-zeynet commented 1 year ago

minimum supported Rust version (MSRV)

Admittedly I had to google it :-)

robs-zeynet commented 1 year ago

It's a shame that this won't work with dev-dependencies. I'll think about a better long-term solution, or perhaps just wait until it's time to bump the MSRV.

Agree that this isn't ideal (why can't you have optional dev dependencies!?) but your proposed workaround seems fine to me as both libraries are relatively small. Shall I just make the change or is there a reason to not consider your workaround?

Wojtek242 commented 1 year ago

It's a shame that this won't work with dev-dependencies. I'll think about a better long-term solution, or perhaps just wait until it's time to bump the MSRV.

Agree that this isn't ideal (why can't you have optional dev dependencies!?) but your proposed workaround seems fine to me as both libraries are relatively small. Shall I just make the change or is there a reason to not consider your workaround?

Yea, just go for it. We don't publish the crate automatically so there will be time to clean it up if there is a need to clean it up before publishing the next version.

robs-zeynet commented 1 year ago

Updated - feedback welcome!

Wojtek242 commented 1 year ago

Brilliant! Thanks, for the contribution!