[IPv6] Extend e2e tests for dual-stack

[wenyingd]

1. Add e2e cases for IPAM allocation in dual-stack.
2. Add e2e cases for connectivity, including same Node and different Node cases.

[antrea-bot]

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

• /test-e2e: to trigger e2e tests.
• /skip-e2e: to skip e2e tests.
• /test-conformance: to trigger conformance tests.
• /skip-conformance: to skip conformance tests.
• /test-whole-conformance: to trigger all conformance tests on linux.
• /skip-whole-conformance: to skip all conformance tests on linux.
• /test-networkpolicy: to trigger networkpolicy tests.
• /skip-networkpolicy: to skip networkpolicy tests.
• /test-windows-conformance: to trigger windows conformance tests.
• /skip-windows-conformance: to skip windows conformance tests.
• /test-windows-networkpolicy: to trigger windows networkpolicy tests.
• /skip-windows-networkpolicy: to skip windows networkpolicy tests.
• /test-hw-offload: to trigger ovs hardware offload test.
• /skip-hw-offload: to skip ovs hardware offload test.
• /test-all: to trigger all tests (except whole conformance).
• /skip-all: to skip all tests (except whole conformance).

[codecov-commenter]

Codecov Report

Merging #1192 into ipv6 will decrease coverage by 1.29%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             ipv6    #1192      +/-   ##
==========================================
- Coverage   41.88%   40.58%   -1.30%     
==========================================
  Files          76       76              
  Lines       11291     9823    -1468     
==========================================
- Hits         4729     3987     -742     
+ Misses       6178     5475     -703     
+ Partials      384      361      -23     

Flag	Coverage Δ
#unit-tests	40.58% <ø> (-1.30%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/controller/networkpolicy/store/util.go	63.63% <0.00%> (-5.60%)	⬇️
pkg/antctl/command_list.go	32.69% <0.00%> (-4.81%)	⬇️
pkg/agent/openflow/cookie/allocator.go	61.29% <0.00%> (-4.57%)	⬇️
pkg/util/ip/ip.go	66.66% <0.00%> (-3.04%)	⬇️
pkg/antctl/command_definition.go	42.40% <0.00%> (-3.03%)	⬇️
pkg/ovs/openflow/logs.go	9.52% <0.00%> (-2.98%)	⬇️
...ntroller/networkpolicy/networkpolicy_controller.go	55.73% <0.00%> (-2.81%)	⬇️
...kg/controller/networkpolicy/store/networkpolicy.go	74.13% <0.00%> (-2.79%)	⬇️
pkg/agent/proxy/endpoints.go	81.42% <0.00%> (-2.67%)	⬇️
pkg/controller/networkpolicy/store/addressgroup.go	85.24% <0.00%> (-2.60%)	⬇️
... and 66 more

[wenyingd]

/test-ipv6-e2e
/test-all

[lzhecheng]

/test-ipv6-e2e

[antoninbas]

I think there is a lot of potential for code unification with the existing tests. It seems that these existing tests just need to be extended to support multiple IPs. Then we can avoid duplicating tests for single stack & dual stack.

For example, the existing podWaitForIP function could be renamed to podWaitForIPs and updated to return a slice of IPs. Tests which depend on that function (e.g. connectivity tests) could be updated to support a slice of IPs. In the single stack case, the slice would include a single IP and the tests would be equivalent to their current version. In the dual stack case, we would check connectivity for both address families.

[wenyingd]

I think there is a lot of potential for code unification with the existing tests. It seems that these existing tests just need to be extended to support multiple IPs. Then we can avoid duplicating tests for single stack & dual stack.

For example, the existing podWaitForIP function could be renamed to podWaitForIPs and updated to return a slice of IPs. Tests which depend on that function (e.g. connectivity tests) could be updated to support a slice of IPs. In the single stack case, the slice would include a single IP and the tests would be equivalent to their current version. In the dual stack case, we would check connectivity for both address families.

Actually, I don't object to unify some generic functions, but I would prefer to have specific tests for dual-stack and single-stack. It is because dual-stack cases should have some additional precondition requirement on the cluster like if multiple CIDRs are set. As for the connectivity verification in dual-stack cluster, we should check the address family and find the corresponding address, this make the test more complicated than a single-stack case. If we keep the code unique, such check in a single-stack cluster test should not be necessary.

[antoninbas]

@wenyingd

It is because dual-stack cases should have some additional precondition requirement on the cluster like if multiple CIDRs are set

It seems to me that this is not something that we need to explicitly verify in the tests. For example:

1. TestPodAssignIPs:

The same test can be used for single-stack and dual-stack. The logic of the test can be as follows:

• if podV4NetworkCIDR is set, make sure the Pod was assigned an IPv4 address
• if podV6NetworkCIDR is set, make sure the Pod was assigned an IPv6 address

2. Connectivity tests. The following code you wrote should work well in both single-stack and dual-stack cases:

t.Logf("Ping mesh test between all Pods")
	for _, podName1 := range podNames {
		for _, podName2 := range podNames {
			if podName1 == podName2 {
				continue
			}
			for _, podIP := range podIPs[podName2] {
				if err := data.runPingCommandFromTestPod(podName1, podIP, pingCount); err != nil {
					t.Errorf("Ping '%s' -> '%s': ERROR (%v)", podName1, podName2, err)
				} else {
					t.Logf("Ping '%s' -> '%s': OK", podName1, podName2)
				}
			}
		}
	}

I agree that it makes the test code a bit more complicated if you look at the single-stack case only. But overall, you avoid having duplicate test cases which do pretty much the same thing. Please let me know if I'm missing something.

The generic podWaitForIPs function can make sure that we have at most one IP from each family.

[mengdie-song]

I have no strong preference on whether we add new connection test case for dual stack or we choose to modify existing connection tests. But I do think we need a new separate test case for dual stack IPAM IP allocation.

[wenyingd]

I agree that it makes the test code a bit more complicated if you look at the single-stack case only. But overall, you avoid having duplicate test cases which do pretty much the same thing. Please let me know if I'm missing something.

The generic podWaitForIPs function can make sure that we have at most one IP from each family.

@antoninbas It should make sense that using generic test code could reduce the duplications. My only concern is that it might break the existing cases if we extend them but not write new test cases. But that risk should be able to avoid if we could control the code quality. Anyway, I would try to extend existing cases to support dual-stack cases.

Another detail I want to discuss is, I would prefer to define a new struct as podIPs in e2e (might be used in the return value of podWaitForIPs ), and one IPv4 address and one IPv6 address are included in this struct. Comparing to a slice for all IPs in a Pod, such a struct should be more convenient, then we don't need to loop the slice and check the address family for each address. What do you think?

[antoninbas]

@wenyingd I'm open to the struct idea. But sometimes a slice is convenient, e.g. in the connectivity tests it may make more sense to loop over the IPs and ensure that we have connectivity to each one.

Since these are tests, it's also possible to define a struct like this one:

type podIPs struct {
    IPStrings []string
    IPv4 *net.IPv4
    IPv6 *net.IPv6
}

The struct is created by podWaitForIPs and each test consumes the information as appropriate (using the slice and / or the individual addresses).

[wenyingd]

Hi @ksamoray , currently I skip these e2e tests for Antrea Proxy in IPv6 cluster (still runs in IPv4 cluster). After your changes for Service is ready, please remember to change the related to code to ensure the test cases could cover your code.

[wenyingd]

/test-all
/test-ipv6-e2e

[wenyingd]

/test-all