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

10-icmpv6-error: Add ICMPv6 error message release specs #88

Merged
merged 1 commit into from
Jan 17, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 23, 2018

This just adds the test descriptions for testing ICMPv6 error message behavior. The tests are based on the testing procedures I proposed for RIOT-OS/RIOT#8594, but re-ordered to reflect the order they have in RFC 4443 and not for incremental configuration as it was in RIOT-OS/RIOT#8594. I will provide a test script soon as a separate PR, based on #79 and the test scripts I provided in RIOT-OS/RIOT#8594.

@miri64
Copy link
Member Author

miri64 commented Dec 13, 2018

I added another test-case which isn't really ICMPv6 error triggering but definitely goes into the direction "weirdly formatted packets send to the stack" which can be implemented with scapy and native as well:

ip = IPv6(src=src_ipv6, dst=dst_ipv6)
i = 0
while (((i + 1) * 40) + 8) < 1500:
    ips = ip
    for _ in range(i):
        ips /= ip
    sendp(Ether(dst=ether_dst) / ips / ICMPv6EchoRequest(seq=i), iface="tapbr0")
    i += 1
    print(i - 1)
    time.sleep(0.001)

Here you can find a fully executable version of that script

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

@aabadie ping?

@aabadie
Copy link
Contributor

aabadie commented Jan 16, 2019

I think it makes sense to add these test cases to the release specs, but I also think that, as is, it is difficult to understand how setup everything to correctly test this.
After reading the testing procedure in RIOT-OS/RIOT#8594, I think I'm starting to understand things. Why not having everything in the same place ?
I always thought that those release specs description were too high level and that it would be great to provide some associated setup/commands. Otherwise, only a very few people know how to run them (think of the bus factor here).

@aabadie
Copy link
Contributor

aabadie commented Jan 16, 2019

Maybe adding a Procedure section between Description and Result would improve the situation ?

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

After reading the testing procedure in RIOT-OS/RIOT#8594, I think I'm starting to understand things. Why not having everything in the same place ?

For a third time (see #88 (comment)):

I intentionally wanted to keep these specs implementation independent and then provide the actual code in a testing script (as I hinted at in OP).

@aabadie
Copy link
Contributor

aabadie commented Jan 16, 2019

I intentionally wanted to keep these specs implementation independent and then provide the actual code in a testing script (as I hinted at in OP).

That doesn't answer what I'm looking for. As is, if I just read this README, I have no idea how to test it.

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

I always thought that those release specs description were too high level and that it would be great to provide some associated setup/commands.

IMHO the point of a specification is to be high level. Everything else goes already more into the direction of test implementation.

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

I intentionally wanted to keep these specs implementation independent and then provide the actual code in a testing script (as I hinted at in OP).

That doesn't answer what I'm looking for. As is, if I just read this README, I have no idea how to test it.

Then please provide some directions how I might help with that without restricting these specs to a specific implementation.

@aabadie
Copy link
Contributor

aabadie commented Jan 16, 2019

Then please provide some directions how I might help with that without restricting these specs to a specific implementation.

See #88 (comment) above. Might be a good starting point. We already provide this in PRs why not backport the testing procedure to release specs ?

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

Then please provide some directions how I might help with that without restricting these specs to a specific implementation.

See #88 (comment) above. Might be a good starting point. We already provide this in PRs why not backport the testing procedure to release specs ?

So something like (for task 1)

### Procedure
1. Compile `examples/gnrc_networking` for `native`
2. Create a TAP interface (if not already done).
   E.g. `./dist/tools/tapsetup/tapsetup` (creates a TAP bridge as well)
3. Execute `examples/gnrc_networking`
4. Assign `beef::1/64` to the TAP bridge / TAP interface.
   E.g. `ip addr add beef::1/64 dev tapbr0`
5. Configure a route to `affe::/64` via the TAP bridge / TAP interface to the
   `example/gnrc_networking` instance.
   E.g. `ip route add affe::/64 via <link-local address of native instance> dev tapbr0`
6. Use your favorite method for sending UDP packets from the Linux host to
   `[affe::1]:48879` with an empty payload.
   E.g. `echo "" | nc -6 -u affe::1 48879`

?

@aabadie
Copy link
Contributor

aabadie commented Jan 16, 2019

So something like (for task 1)

Not sure this should be copied in all sub-task but it could be in a common setup section at the beginning of the file and an internal link added to each sub-task.

I also like what's done in #77 where the task scripts are also provided with some notes on how to play the task.

Of course it would be better to automatize all that (using pytest, which is my highly preferred solution compared to others proposed) but that doesn't prevent to provide a manual procedure.

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

Of course it would be better to automatize all that (using pytest, which is my highly preferred solution compared to others proposed) but that doesn't prevent to provide a manual procedure.

I think the discussion regarding framework setup is still ongoing in RIOT-OS/RIOT#10241 (yet another reason why I didn't want to provide an implementation yet) and I know you have a strong opinion against RFW (as I did but that changed). I think your comments there regarding your test framework of choice would be very welcome there.

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

Of course it would be better to automatize all that (using pytest, which is my highly preferred solution compared to others proposed) but that doesn't prevent to provide a manual procedure.

Again, these are specifications. That means apart from giving us (the contributors) an agreed upon way to test the release they also provide a user with the information "This is what has been tested for the release". I'm a bit worried that extensive testing descriptions (especially if implementation specific) might confuse more in the latter case than be helpful.

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

(+ too detailed stuff might lead to testers doing the same predescribed thing over and over again without even thinking about it.... in the past we found bugs during the release testing because testers did or tried something different then other testers before them).

@aabadie
Copy link
Contributor

aabadie commented Jan 16, 2019

in the past we found bugs during the release testing because testers did or tried something different than other testers before them

Providing or suggesting a testing procedure would increase the number of potential testers. Like it is now, with almost no guide and specifications that are becoming more and more vague/complex to setup and test, it ends up in this to be only testable by the person who initially wrote the specification (or maybe the same 2 or 3 people with enough knowledge), because he/she knows how to do it. This breaks your argument since one will very quickly fall into "things being tried always the same way".
#85 is also going in the direction of helping testers starting with those non straight-forward (for uninitiated) specifications.

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

because he/she knows how to do it. This breaks your argument since one will very quickly fall into "things being tried always the same way".

I will provide some documentation, but I at least expect people to read the scapy doc to a point were they are able to communicate with the native instance. Otherwise, it will be as I worry about.

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

Done

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Thanks for updating @miri64, it's clearer now.

Could the Send the UDP packet as specified. be associated with an example command based on you python script (the one you put in the gist) or something ?

10-icmpv6-error/10-icmpv6-error.md Show resolved Hide resolved
10-icmpv6-error/10-icmpv6-error.md Outdated Show resolved Hide resolved
using link-layer frames with `sendp()`)*

```py
sendp(Ether(dst=DST_HWADDR) / IPv6(src=SRC_IPV6, dst=DST_IPV6) / UDP(dport=DST_PORT),
Copy link
Contributor

@aabadie aabadie Jan 17, 2019

Choose a reason for hiding this comment

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

This is much better. Stupid (or lazy guy) question: how to set a payload of 1452B as required here ?

Would the following work ?

sendp(Ether(dst=DST_HWADDR) / IPv6(src=SRC_IPV6, dst=DST_IPV6) / UDP(dport=DST_PORT) / b"x" * 1452 ,iface="tapbr0", timeout=0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yepp

Copy link
Member Author

Choose a reason for hiding this comment

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

Added something like that to the doc ;-).

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK, please squash!

@miri64 miri64 force-pushed the icmpv6-error/feat/initial branch from 332d700 to ce830d7 Compare January 17, 2019 15:35
@miri64
Copy link
Member Author

miri64 commented Jan 17, 2019

Since I gave this the number 10, but #77 is not merged yet, how do we handle this? Should I renumber, should we wait for #77, or just merge and accept that there will be a hole for a while?

@aabadie
Copy link
Contributor

aabadie commented Jan 17, 2019

I'd like to also merge 09-coap for this release but I want to test it before.

@miri64
Copy link
Member Author

miri64 commented Jan 17, 2019

Please also consider #90 for the release. It took me quite some elbow grease to get those bugs fixed and I'd hate a regression to be introduced unnoticed ;-).

@miri64 miri64 merged commit 34780fb into RIOT-OS:master Jan 17, 2019
@miri64 miri64 deleted the icmpv6-error/feat/initial branch January 17, 2019 16:22
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.

2 participants