Add integration tests #4

siegfried · 2020-01-30T18:00:32Z

The tests require a tun0 device with address 10.10.10.1/24 in the OS. On Debian, you can set it up like this:

# ip tuntap add dev tun0 mode tun
# ip address add 10.10.10.1/24 dev tun0
# ip link set tun0 up

Please let me know if this is acceptable.

Currently the two tests can only pass one because the Iface doesn't close the device file. Could it be a bug?

* Test payload * Improve iface.send()

siegfried · 2020-01-31T11:58:36Z

@vorner Do you think it's better to configure the tun device in the test? I didn't do that because those commands should be run as root. And do you know how to configure the device in the CI environment?

vorner · 2020-01-31T13:05:25Z

Sorry, I don't have the time to look at it right now. Hopefully over the weekend I might find some.

siegfried · 2020-01-31T13:13:29Z

@vorner No problem. Take your time. Thanks.

Make the line shorter.

vorner

You're right, the missing Drop implementation closing the file descriptor is a bug. Do you want to fix it, or should I?

As for the commands, I'd probably say that the test should do them and start without a pre-created tun. The Iface constructor creates a new tuntap device if it doesn't exist ‒ see the examples. Furthermore, you might want to allow the kernel to include a sequence number in the name (eg. testtun%d), so the tests running in parallel won't disrupt each other.

I think the CI will have to be configured to allow to run with privileged docker or in non-docker environment. I think I've seen some documentation in travis that allows such thing.

A little style thing: as these are more of integration tests than unit tests, would you move them into a separate file in the tests directory?

As for the etherparse crate, thanks for bringing it to my attention. I've usually used pnet-packet for such things, but this one seems more lightweight. I'll definitely want to have a look at it.

Cargo.toml

siegfried · 2020-02-02T20:59:50Z

src/lib.rs

@@ -221,3 +221,10 @@ impl IntoRawFd for Iface {
        self.fd.into_raw_fd()
    }
 }
+
+impl Drop for Iface {


@vorner I tried to fixed it. But the compiler says:

error[E0509]: cannot move out of type `Iface`, which implements the `Drop` trait --> src/lib.rs:221:9 | 221 | self.fd.into_raw_fd() | ^^^^^^^ | | | cannot move out of here | move occurs because `self.fd` has type `std::fs::File`, which does not implement the `Copy` trait

siegfried · 2020-02-02T21:21:52Z

I think the CI will have to be configured to allow to run with privileged docker or in non-docker environment. I think I've seen some documentation in travis that allows such thing.

I couldn't create tuntap device in Docker. I'm running this on a VM.

siegfried · 2020-02-02T22:22:28Z

As for the commands, I'd probably say that the test should do them and start without a pre-created tun. The Iface constructor creates a new tuntap device if it doesn't exist ‒ see the examples. Furthermore, you might want to allow the kernel to include a sequence number in the name (eg. testtun%d), so the tests running in parallel won't disrupt each other.

To create a TunTap device with Iface might not be a common operation. It assumes to be run as root, which could lead to more security issues. I think a better practice is to let root user assign the devices like ip tuntap add dev ${interface name} mode ${mode} user ${user} group ${group} to the users.
Normally you cannot test a device when you don't have it on your machine. I think this is the case.
Also, the iproute2 command line tools cannot work on other systems.

siegfried · 2020-02-02T23:05:14Z

You're right, the missing Drop implementation closing the file descriptor is a bug. Do you want to fix it, or should I?

Sorry, it's not a bug. You just reminded me that the tests were run in parallels. With --test-threads=1 all tests passed. Might be better to avoid parallel tests on functions with side effects. And this crate won't be big anyway.

vorner · 2020-02-07T21:23:14Z

Sorry, it's not a bug. You just reminded me that the tests were run in parallels.

Ah, I see. The fd is not int, but File, because I wanted to the File to take care of closing.

For the test, I agree that it makes sense to not make it work in parallel and run them sequentially. It would be nice, though, to be able to just do cargo test when developing (possibly by root) and not remember to write the parameter. I usually add a global mutex and the test lock it while running.

The same reason (running on random developer's computer) would still apply to generating the name from within the test, somehow dynamically as opposed to using hardcoded tun0 (and stealing some VPN running there by accident). Or at least use something else, like tun-test-1. Furthermore, it seems most programs that use some tun device set it up automatically ‒ eg. OpenVPN never asked me to set up the tun device for it. Some programs start as root and then drop the privileges.

I do find an argument that in practice you might want to use a pre-created one, nevertheless, these are tests and convenience of running (and cleanup) probably wins over security.

The tun in docker ‒ it should be possible, but you need to run the docker in privileged mode.

(Sorry for somewhat chaotic answer, I still didn't find enough time to think about it thoroughly)

siegfried · 2020-02-07T23:13:14Z

For the test, I agree that it makes sense to not make it work in parallel and run them sequentially. It would be nice, though, to be able to just do cargo test when developing (possibly by root) and not remember to write the parameter. I usually add a global mutex and the test lock it while running.

How did you do that? Is a crate required? rust-lang/rust#43155

vorner · 2020-02-08T06:59:02Z

How did you do that? Is a crate required? rust-lang/rust#43155

Yes, something like once-cell or lazy_static (I personally prefer the first, it has nicer API).

vorner

Sorry for nitpicking that much and that it takes such a long time, but I kind of can't help it 😇. I'm starting to more or less like the current version, there are still some details.

Some slightly bigger topics:

I'm still not entirely happy about the manual setup of the tun, and that it's tun0 which might be actually used on a developer computer. I won't insist on tests setting it themselves, but at least:
- Use some different name, please, one that's less likely to be taken. Maybe tun-test-0.
- It would be nice if someone run this on their own computer (not on CI), they would get a hint/the right error message. As a suggestion, I'd say add another test case called system_setup_correctly that would check the correct tun exists and if not, it would error out with explaining message and hint what to do (or maybe at least reference a comment in the code how to set it up). I wouldn't fancy issues opened stating tests fail only to find out someone didn't set up environment for the test.
We'll need to somehow clean up the git history before merging. Let's leave it with adding commints during the review, but some kind of rewrite/rebase will have to happen. Will you want to do it yourself, or will you leave it up to me?

vorner · 2020-02-16T10:03:23Z

Cargo.toml

@@ -31,3 +31,4 @@ tokio-core = { version = "~0.1", optional = true }
 [dev-dependencies]
 version-sync = "~0.5"
 etherparse = "0.9.0"
+serial_test = "0.3.2"


Hey, this is cool :-). Didn't know about that one.

I've noticed version 0.4 came out yesterday, maybe this could be used.

Anyway, looking at the dependencies, I know this is only a style point (valid for the etherparse above too):

I prefer to prefix them with the tilde (~). This is the same as no prefix, but it is cleaner in stating the version required is not exact, but only semver-compatible.

I omit the last digit that's not needed, to give the resolver as much freedom as possible. For etherparse, the 0 at the end is not needed for sure. Do you need some specific changes/bugfixes in serial_test 0.3.2, or any 0.3 would do? (or 0.4)?

vorner · 2020-02-16T10:06:36Z

tests/integration_test.rs

+        assert_eq!(_ip_header.source, [10, 10, 10, 1]);
+        assert_eq!(_ip_header.destination, [10, 10, 10, 2]);
+        assert_eq!(_udp_header.source_port, 2424);
+        assert_eq!(_udp_header.destination_port, 4242);


Looking at this again, why are the variable names with _? Using them in assert counts as use, so you shouldn't get an unused variable lint, and especially in tests, checking them is the intended use.

I got compiler warnings for having variable names like ip_header. _ prefix was the compiler proposed. After upgraded to 1.40 the warnings were gone.

siegfried · 2020-02-16T22:20:38Z

Sorry, I'm a little busy recently. I will make the changes when I'm available. Thanks for reviewing.

siegfried · 2020-02-16T22:28:50Z

And I'm glad you are picky because I'm new in Rust, so don't worry about it.

And add the shell script to create and to delete the device.

siegfried · 2020-02-24T04:50:36Z

Some slightly bigger topics:

I'm still not entirely happy about the manual setup of the tun, and that it's tun0 which might be actually used on a developer computer. I won't insist on tests setting it themselves, but at least:

Use some different name, please, one that's less likely to be taken. Maybe tun-test-0.

It would be nice if someone run this on their own computer (not on CI), they would get a hint/the right error message. As a suggestion, I'd say add another test case called system_setup_correctly that would check the correct tun exists and if not, it would error out with explaining message and hint what to do (or maybe at least reference a comment in the code how to set it up). I wouldn't fancy issues opened stating tests fail only to find out someone didn't set up environment for the test.

Come to think about it again, I still don't feel running sudo cargo test on the developing machine every time is a good idea (at least I don't want to do it on my machine), so I created two shell scripts to create and to destroy the test device, and add some descriptions in README. Please let me know how you think of it. I can also make this more automatic by wrapping cargo test in a Makefile.

We'll need to somehow clean up the git history before merging. Let's leave it with adding commints during the review, but some kind of rewrite/rebase will have to happen. Will you want to do it yourself, or will you leave it up to me?

Is it possible to squash all the commits into one when you merge it? (I thought I saw this feature somewhere) Otherwise I can rebase it if you are not available.

vorner

Is it possible to squash all the commits into one when you merge it?

Yes, that's a possibility. It couldn't be used if you wanted to preserve multiple commits with related changes but somewhat separate, but I think in this case one commit is good.

Should I use the commit message of the first one?

Anyway, I'm happy with the current version. There's this one language correction which I might as well do myself when merging it. So, I'll just wait for you to confirm that you're OK with the current state too and then go ahead.

vorner · 2020-02-29T12:36:57Z

tests/setup.sh

+
+sudo ip tuntap add dev tun10 mode tun user $(whoami)
+sudo ip address add 10.10.10.1/24 dev tun10
+sudo ip link set tun10 up


This requires sudo to be available, as opposed to some other ways of setting this up (eg. running it directly as root). But I guess it's OK to live with it this way; people with really unusual systems might at least use the script as an inspiration.

README.md

siegfried · 2020-03-02T10:44:29Z

I think it's ok to use the message of the first commit. Thanks.

vorner · 2020-03-03T19:13:40Z

Thank you. And sorry it took so long.

siegfried added 2 commits January 31, 2020 00:11

Add integration tests

00d2a2e

Improve tests

735f321

* Test payload * Improve iface.send()

siegfried requested a review from vorner January 31, 2020 11:46

Rename variables

15907a9

Reformat

1e67caa

Make the line shorter.

siegfried force-pushed the add-tests branch from 9efc6de to 1e67caa Compare January 31, 2020 13:14

vorner requested changes Feb 2, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Create tun0 device in CI

b67e4e7

siegfried force-pushed the add-tests branch from bf3b50e to b67e4e7 Compare February 2, 2020 20:18

siegfried added 3 commits February 3, 2020 04:41

Move tests to integration_test.rs

4e94246

Finish up the file with a newline

abce763

Implement Drop for Iface

885da67

siegfried commented Feb 2, 2020

View reviewed changes

Remove Drop implement

2bf861e

siegfried added 2 commits February 3, 2020 07:10

Try to fix travis CI

f4cd4f4

Apply sudo

656b757

siegfried force-pushed the add-tests branch from b477120 to 656b757 Compare February 3, 2020 21:07

siegfried requested a review from vorner February 3, 2020 21:10

Remove self::

946f52a

siegfried changed the title ~~[WIP] Add integration tests~~ Add integration tests Feb 3, 2020

Use serial_test

eff3a32

vorner requested changes Feb 16, 2020

View reviewed changes

siegfried added 5 commits February 24, 2020 11:15

Use ~ for dependencies

2038f5c

Upgrade serial_test to 0.4

d67bcda

Remove the _ prefixes

d47761c

Change the test tun to tun10

3c73f0b

And add the shell script to create and to delete the device.

Fix .travis.yml

fac91e3

siegfried requested a review from vorner February 24, 2020 04:53

Show the commands

c0a0344

vorner approved these changes Feb 29, 2020

View reviewed changes

Fix the language mistake

4714e0e

vorner merged commit e150911 into vorner:master Mar 3, 2020

Add integration tests #4

Add integration tests #4

Uh oh!

Conversation

siegfried commented Jan 30, 2020

Uh oh!

siegfried commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vorner commented Jan 31, 2020

Uh oh!

siegfried commented Jan 31, 2020

Uh oh!

vorner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

siegfried Feb 2, 2020

Choose a reason for hiding this comment

Uh oh!

siegfried commented Feb 2, 2020

Uh oh!

siegfried commented Feb 2, 2020

Uh oh!

siegfried commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vorner commented Feb 7, 2020

Uh oh!

siegfried commented Feb 7, 2020

Uh oh!

vorner commented Feb 8, 2020

Uh oh!

vorner left a comment

Choose a reason for hiding this comment

Uh oh!

vorner Feb 16, 2020

Choose a reason for hiding this comment

Uh oh!

vorner Feb 16, 2020

Choose a reason for hiding this comment

Uh oh!

siegfried Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

siegfried commented Feb 16, 2020

Uh oh!

siegfried commented Feb 16, 2020

Uh oh!

siegfried commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vorner left a comment

Choose a reason for hiding this comment

Uh oh!

vorner Feb 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

siegfried commented Mar 2, 2020

Uh oh!

vorner commented Mar 3, 2020

Uh oh!

Uh oh!

siegfried commented Jan 31, 2020 •

edited

Loading

siegfried commented Feb 2, 2020 •

edited

Loading

siegfried commented Feb 24, 2020 •

edited

Loading