On 04/15/09 01:34, Sebastien Roy wrote:
> 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.
>
Thank you very much for your feedback on the assertions, you suggestion
can make the test suite cover more aspect of iptun, and make the test
suite have a better stucture.
> * 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.
>
Fine.
> * 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.
>
Agree, new test is separated for <tdst> test in admin-dladm/create-iptun.
> 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.
>
Fine, I added one assertion: "dladm set-linkprop" fails to set
encaplimit to v4 and 6to4 tunnels.
ID: dladm-misc/test07
DESCRIPTION:
"dladm set-linkprop" fails to set encaplimit to v4 and 6to4 tunnels.
STRATEGY:
- create an ipv4 tunnel using "dladm create-iptun".
- try to set encaplimit to the tunnel by "dladm set-linkprop", expect
failure.
- delete the tunnel.
- for 6to4 tunnels, do similar things.
> * 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.
>
The assertion have been changed like this:
ID:
DESCRIPTION:
the "-T <type>" argument cannot be omitted when creating a tunnel.
STRATEGY:
- create a configured ipv4 tunnel using
dladm create-iptun -s <v4_addr1> -d <v4_addr2> <name>
expect failure.
- create a configured ipv6 tunnel using
dladm create-iptun -s <v6_addr1> -d <v6_addr2> <name>
expect failure.
- create a configured tunnel using
dladm create-iptun -s <v4_addr1> <name>
expect failure.
> * 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.
>
Fine, the assertion was merged to admin-dladm/create-iptun/test06.
> * 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.
>
OK.
> * admin-dladm/create-iptun/test14
>
> This assertion is now obsolete as the type is mandatory.
>
Right, but I'd like to keep it for creating iptun using hostnames.
> * delete-iptun/test04
>
> A test04 should be added to verify that delete-iptun fails if an IP
> interface is plumbed on the tunnel.
>
Plumbed iptun deletion failure is verified in
admin-dladm/delete-iptun/test03.
> * 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.
>
Yes, it should only verify src and dst addresses of the tunnels.
> 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.
>
Fine, the splited test for hoplimit and encaplimit modification is
dladm-misc/test05.
> * 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.
>
Fine, I added 2 assertions in function/basic test category.
> 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.
>
>
Right, verification of these properties will be also added.
> * 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.
>
Right, I only verify the misconfig for src and dst address of iptun in
this assertion. And verify misconfig for link properties in
dladm-misc/test06.
> * 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.
>
I think the show-linkprop part have been contained in dladm-misc/test04.
So I just need to remove the part from this assertion.
> * 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.
>
Fine, new assertion should be like this:
DESCRIPTION:
IPv4 interfaces can not be plumbed on a 6to4 tunnel.
STRATEGY:
- create a 6to4 tunnel using "dladm create-iptun" named tun1
- observe the following operations are failed:
- ifconfig tun1 plumb
- ifconfig tun1 plumb up
- ifconfig tun1 plumb <v4_addr1> <v4_addr2>
- ifconfig tun1 plumb <v4_addr1> <v4_addr2> up
- ifconfig tun1 plumb <v6_addr1> <v6_addr2>
- ifconfig tun1 plumb <v6_addr1> <v6_addr2> up
- delete tun1
> * 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.
>
You mean this assertion should be described like this: "The permenant
configuration (i.e. dladm(1M) command line parameters) *doesn't*
overrides contents in /etc/hostname*.ip*.tun*."? Is current
implementation ok when iptun integrated?
> * dladm-misc/test02
>
> Since show-dev no longer exists (removed by Crossbow), this assertion is
> stale. It should instead use show-phys.
>
OK.
> * dladm-misc/test05
>
> This assertion (and test06) should mention which link properties it is
> operating on.
>
Yes, the two assertion verify hoplimit and encaplimit of iptun.
> * 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.
>
Well, I broke functional tests into 3 categories: basic, 6to4 and mtu.
> * 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.
>
I'd like to know how to verify the packet is dropped? From user space
point of view, ping will give "100% loss" at stdout when sending icmp
request, would it be ok to verify that 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.
>
Fine, I am considering the strategy on how to implement these
assertions. I will add these 6to4 tunnel test cases.
>
> * 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?).
>
Right, but we need only configure route table on Host A, for Host B will
auto-forward the packet though tun2 because creation of tun2 will auto
add the entry to route table.
> 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.
>
Make sense. I'll apply the test to ipv[4|6]-over-ipv[4|6] tunnels.
>
> * 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.
>
OK.
>
> -Seb
>
Thanks,
--
Best Regards,
Xiang
