Hi Xiang, Here are comments and suggestions for the set of assertions in the iptun test suite. I must say, this is an excellent set of assertions.
* admin-dladm/create-iptun/test01 There is no "-e <el>" option to create-iptun, so there is no value in testing that this option fails. I would restrict this assertion to simply verifying that IPv4 tunnel creation works. A separate assertion should verify that one cannot set the encaplimit property on an IPv4 tunnel. I'd suggest having a new assertion section (admin-dladm/linkprop perhaps?) for link property testing. * admin-dladm/create-iptun/test04 To keep the assertions simple (and ease debugging of test failures), I would suggest separating out the <tdst> test to a separate assertion. I would remove the "-e <el>" check since that option doesn't exist. I would instead move the encaplimit property check to its own assertion. Perhaps an assertion that states "cannot set the encaplimit property on ipv4 or 6to4 tunnels", and you can combine this with the check from test01. * admin-dladm/create-iptun/test06 The "-T type" argument to create-iptun is now always mandatory, in all cases. This assertion should probably simply state, the "-T <type>" argument cannot be omitted when creating a tunnel. * admin-dladm/create-iptun/test07 There's no "tsrc" subcommand for create-iptun. In any case, this assertion can probably be combined with test06, as there's nothiing 6to4-specific about creating an IP tunnel and only specifying the source at creation time. The destination for an ipv4 tunnel can be specified later with modify-iptun. * admin-dladm/create-iptun/test11 Since the type is mandatory, you can remove the ", and the type is specified" from this assertion, it's now redundant. * admin-dladm/create-iptun/test14 This assertion is now obsolete as the type is mandatory. * delete-iptun/test04 A test04 should be added to verify that delete-iptun fails if an IP interface is plumbed on the tunnel. * admin-dladm/modify-iptun/test01 It's unclear to me what is being tested and verified in this assertion. This same comment also applies to test02. The assertion should probably be specific to modifying the source and destination addresses of the tunnel. Also, show-linkprop doesn't display anything that was modified using modify-iptun. There should be separate assertions that verify the modification of the hoplimit and encaplimit properties. Perhaps a separate "admin-dladm/linkprop" section would be useful to contain all tests related to the "hoplimit" and "encaplimit" link properties. * admin-dladm/modify-iptun/test02 For this test, it's important to verify that after modifying a tunnel's source or destination address, that packets transmitted over the tunnel have the correct source or destination in their outer IP headers. The code makes this work is quite complex, and I've run into some bugs during development that cause the "modify-iptun" to work (show-iptun shows the correct updated values), but transmitted packets still have the old IP addresses in their headers. There should also be similar sanity checking of packets when modifying the hoplimit or encaplimit properties in the assertions that verify the modification of these properties. * admin-dladm/modify-iptun/test04 The specific parameters need to be mentioned. The strategy should mention what kinds of invalid attributes are tested. This assertion should probably focus on tunnel source and destination, while link properties should go in separate assertions. * admin-dladm/show-iptun/test03 The show-linkprop part of this test should go into a separate assertion, perhaps in the suggested new "linkprop" section. * admin-dladm/ip_interface/test04 There should be a test that makes sure that one cannot plumb an IPv4 IP interface on a 6to4 tunnel. * admin-ifconfig/test06 Typo - there's a stray "2.6" below the assertion ID. * admin-ifconfig/test08 I'm not convinced that this is the correct behavior. I know that this assertion currently fails, but I'm not sure that fixing it is the correct thing to do. If there's an old /etc/hostname.<tunnel> file with stale configuration, overriding that configuration with dladm masks the fact that there is stale configuration around that may unexpectedly take effect if the "dladm" tunnel link is ever deleted or renamed. I'm tempted to flip this one on its head and make the design (and this assertion) adhere to the current implementation. * dladm-misc/test02 Since show-dev no longer exists (removed by Crossbow), this assertion is stale. It should instead use show-phys. * dladm-misc/test05 This assertion (and test06) should mention which link properties it is operating on. * autopush/test03 This assertion doesn't belong in this test suite. The interaction between the autopush property and traditional /etc/iu.ap probably belongs in the GLDv3 test suite, but there's certainly nothing specific to IP tunneling in this assertion. * functional/test02 I'm glad this assertion is here. MTU calculation is by far the most complicated aspect of IP tunneling. In general, there also seem to be MTU-related tests further down in this category (test07 and test08). It might make sense to create a subsection named "functional/mtu" and place these three tests in there. * functional/test03 I'd suggest breaking down the functional category further, placing 6to4 functional tests into their own subcategory (functional/6to4). There are a lot of potential tests related to 6to4 that could be added, and they could clutter the functional category. For example, here are a few security-related tests that would be very valuable: - When receiving a tunneled packet, if the IPv6 destination isn't in the site, then the packet is dropped. - When receiving a tunneled packet, if the IPv4 source and embedded IPv4 address in the 6to4 IPv6 source don't match, then the packet is dropped. - Only accept packets from a relay router (IPv6 source isn't 6to4) if we've configured a relay router. - Don't tunnel packets with non-6to4 source addresses (we don't implement relay router functionality). - Don't tunnel a packet if the embedded IPv4 address in the IPv6 source doesn't match the IPv4 tunnel address. - Nothing bad happens (the packet is dropped) when sending to a global destination and a relay router hasn't been configured. * functional/test07 The strategy should say that Host A's routing table results in packets being transmitted to Host B via tun1, and that Host B's routing table results in packets being transmitted to Host A via tun2. In other words, packets that Host A sends go through tun1, and Host B attempts to forward them through tun2, but can't because tun2's MTU is too small (correct?). The verification step should ensure that the MTU value in the ICMP packet-too-bit message is correct. This test has four variants: ipv4-over ipv6, ipv6-over-ipv6, ipv4-over-ipv4, and ipv6-over-ipv4. Each of these cases needs to be tested. In the ipv6-over-<foo> cases, you're looking at ICMPv6 packet-too-big messages. In the ipv4-over-<foo> cases, you're looking at ICMPv4 fragmentation-needed messages. * functional/test08 The "and MTU of the tunnel will be recalculated" statement can't be verified since 6to4 tunnels don't have meaningful MTU's (they're point-to-multipoint). The important part is that ICMPv6 packet-too-big messages being returned from a 6to4 tunnel interface have the correct MTU value. -Seb
