Skip to content

tests/gnrc_sock_dns: port to scapy#10898

Merged
miri64 merged 3 commits intoRIOT-OS:masterfrom
miri64:tests/enh/gnrc_sock_dns
Mar 27, 2019
Merged

tests/gnrc_sock_dns: port to scapy#10898
miri64 merged 3 commits intoRIOT-OS:masterfrom
miri64:tests/enh/gnrc_sock_dns

Conversation

@miri64
Copy link
Member

@miri64 miri64 commented Jan 29, 2019

Contribution description

This

Testing procedure

Build and flash the application to a board of your choice (must support ethos or netdev_tap). Create a TAP interface and run

make test

The tests should succeed.

Issues/PRs references

Adds tests for #10739
Requires #10896 (merged) for tests to finish successfully.

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: tests Area: tests and testing framework labels Jan 29, 2019
@miri64 miri64 added this to the Release 2019.04 milestone Jan 29, 2019
@miri64 miri64 requested a review from kaspar030 January 29, 2019 21:39

$ sudo dnsmasq -d -2 -z -i tap0 -q --no-resolv \
--dhcp-range=::1,constructor:tap0,ra-only \
--listen-address 2001:db8::1 \
Copy link
Member Author

Choose a reason for hiding this comment

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

I mentioned this in the commit that does this change: this bind causes (at least my version of dnsmasq) to send RAs from that address. However, sending RAs from a non-link-local address is invalid.

@miri64
Copy link
Member Author

miri64 commented Jan 29, 2019

Rebased to current master to include fixes from #10896.

@miri64 miri64 force-pushed the tests/enh/gnrc_sock_dns branch from 20a23c2 to 3c29512 Compare January 29, 2019 22:46
@aabadie
Copy link
Contributor

aabadie commented Jan 30, 2019

sudo make test

I have to say that I don't like this. The files generated by the build (if the application was not built already) will be owned by root and it will create a mess with file permissions in the riot directory. For me this is also a big problem with automated tests but also for developers without root access on their machine. The same issue is already in master with 2 other gnrc test applications.
The fact that it passes on Murdock is probably because the tests are run within a container environment, using a root user. This doesn't reflect the way a developer is supposed to use the build system to test something.
Running tests as root could cause many troubles, just imagine a line that just do os.system('rm -rf /').

If there is a way to use scapy without the need to be root, that would be more useful and it should also be documented.

The purpose of tests is to be runnable by any developer, without strong knowledge of all the corner cases and setup details. It should also be easy to understand and help understand the code being tested.

@miri64
Copy link
Member Author

miri64 commented Jan 30, 2019

The files generated by the build

make test isn't supposed to build just execute a test script.

For the most part I agree with you (the tests are currently not run on Murdock btw for exactly that reason). Sadly, there is no way to run without root, since the scapy tests

  • needs to be able to send Ethernet or at least raw IPv6 packets
  • need to be able to sniff the tap interface (even wireshark also effectively needs root permissions for that).

To execute them without sudo, scapy (and thus python) would require certain permissions, like e. g. wireshark gets when the user is added to the wireshark group. Since we are talking raw networking capabilities for the python executionable here I rather like to not have that on a system like Murdock (unless we can ensure better isolation there).

[edit: darn mobile interface]

@miri64 miri64 closed this Jan 30, 2019
@miri64 miri64 reopened this Jan 30, 2019
@miri64
Copy link
Member Author

miri64 commented Jan 30, 2019

If and only if I find a way to send scapy-cooked packets over UDP sockets, this test at least could be ported to UDP sockets, since it is testing DNS, an application layer protocol. I will try that. However, this is not possible with the other two tests (gnrc_ipv6_ext and gnrc_rpl_srh), since they test parts of the IPv6 implementation and are thus required to at least be able to send hand-cooked IPv6 datagrams.

@miri64
Copy link
Member Author

miri64 commented Jan 30, 2019

The purpose of tests is to be runnable by any developer, without strong knowledge of all the corner cases and setup details. It should also be easy to understand and help understand the code being tested.

Apart for developers without administrative access to the machine scapy allows for exactly that. As a bonus, the script even provides you a bold red warning if you try to run it without root permissions ;)

@miri64
Copy link
Member Author

miri64 commented Jan 30, 2019

scapy allows for exactly that.

To elaborate on that: the dependency-less alternative to scapy would be either the struct module or raw byte strings + sockets but I think those solutions are neither readable nor maintainable for the uninformed or informed developer ;)

@miri64
Copy link
Member Author

miri64 commented Jan 30, 2019

If and only if I find a way to send scapy-cooked packets over UDP sockets, this test at least could be ported to UDP sockets, since it is testing DNS, an application layer protocol. I will try that. However, this is not possible with the other two tests (gnrc_ipv6_ext and gnrc_rpl_srh), since they test parts of the IPv6 implementation and are thus required to at least be able to send hand-cooked IPv6 datagrams.

Worked. The tests are now even simpler than with pure scapy :-).

@miri64
Copy link
Member Author

miri64 commented Jan 30, 2019

They still require a TAP interface though, so no way to run them with Murdock at the moment :(

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

I looked at the changes. I really like the moving to shell tests and implementing logic in python. The fact that @aabadie comments regarding sudo have been addressed means it doesn't cost anything.

Though I am not a networking guy I was able to run the tests and they seem much more in depth than the test that is replaced.

I would be ok to move this forward if nobody else... @kaspar030... has an opinion as it appears to be an improvement to me.

@MrKevinWeiss
Copy link
Contributor

Also rebase seems needed.

@MrKevinWeiss MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Mar 26, 2019
@miri64 miri64 force-pushed the tests/enh/gnrc_sock_dns branch from c6e658f to 0337291 Compare March 26, 2019 18:52
@miri64
Copy link
Member Author

miri64 commented Mar 26, 2019

Rebased and adapted for master

@MrKevinWeiss
Copy link
Contributor

Well since no response, I trust @miri64 that is testing the correct thing.

Please squash!

miri64 added 3 commits March 27, 2019 11:35
Also remove binding of the DNS server to 2001:db8::1. It causes
`dnsmasq` to send router advertisements from that address, which is
not a [valid source for RAs][RFC 4861], so a default route is never
configured on the RIOT to reach the DNS server.

[RFC 4861]: https://tools.ietf.org/html/rfc4861#section-6.1.2
@miri64 miri64 force-pushed the tests/enh/gnrc_sock_dns branch from b85b0ec to 3bfaded Compare March 27, 2019 10:35
@miri64
Copy link
Member Author

miri64 commented Mar 27, 2019

Squashed

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

ACK. We can wait for the CI.

@miri64 miri64 merged commit e5c5bea into RIOT-OS:master Mar 27, 2019
@miri64 miri64 deleted the tests/enh/gnrc_sock_dns branch March 27, 2019 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants