Remove the pod reference in v1beta1 AddressGroup (#1586)

To be backwards compatible, AddressGroup v1beta1 must not include
PodReferences in its members, otherwise the agents that use v1beta1 will
think the members have changed and will add new IPs to the openflow
entries of relevant NetworkPolicies and delete old IPs from them, while
the old IPs and new IPs are actually the same. In current
implementation, deleting old IPs is executed after adding new IPs, so it
leads to some IPs missing in the openflow entries.

Fixes #1587

.github/workflows/kind_upgrade.yml

@@ -73,7 +73,18 @@ jobs:
         sudo mv kind /usr/local/bin
     - name: Run test
       run: |
-        ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 1
+        mkdir log
+        ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 1
+    - name: Tar log files
+      if: ${{ failure() }}
+      run: tar -czf log.tar.gz log
+    - name: Upload test log
+      uses: actions/upload-artifact@v2
+      if: ${{ failure() }}
+      with:
+        name: upgrade-from-antrea-version-n-1.tar.gz
+        path: log.tar.gz
+        retention-days: 30
 
   from-N-2:
     name: Upgrade from Antrea version N-2
@@ -102,7 +113,18 @@ jobs:
         sudo mv kind /usr/local/bin
     - name: Run test
       run: |
-        ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 2
+        mkdir log
+        ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 2
+    - name: Tar log files
+      if: ${{ failure() }}
+      run: tar -czf log.tar.gz log
+    - name: Upload test log
+      uses: actions/upload-artifact@v2
+      if: ${{ failure() }}
+      with:
+        name: upgrade-from-antrea-version-n-2.tar.gz
+        path: log.tar.gz
+        retention-days: 30
 
   compatible-N-1:
     name: API compatible with client version N-1
@@ -131,7 +153,18 @@ jobs:
           sudo mv kind /usr/local/bin
       - name: Run test
         run: |
-          ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 1 --controller-only
+          mkdir log
+          ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 1 --controller-only
+      - name: Tar log files
+        if: ${{ failure() }}
+        run: tar -czf log.tar.gz log
+      - name: Upload test log
+        uses: actions/upload-artifact@v2
+        if: ${{ failure() }}
+        with:
+          name: api-compatible-with-client-version-n-1.tar.gz
+          path: log.tar.gz
+          retention-days: 30
 
   compatible-N-2:
     name: API compatible with client version N-2
@@ -160,7 +193,18 @@ jobs:
           sudo mv kind /usr/local/bin
       - name: Run test
         run: |
-          ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 2 --controller-only
+          mkdir log
+          ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 2 --controller-only
+      - name: Tar log files
+        if: ${{ failure() }}
+        run: tar -czf log.tar.gz log
+      - name: Upload test log
+        uses: actions/upload-artifact@v2
+        if: ${{ failure() }}
+        with:
+          name: api-compatible-with-client-version-n-2.tar.gz
+          path: log.tar.gz
+          retention-days: 30
 
   # Runs after all other jobs in the workflow and deletes Antrea Docker images uploaded as temporary
   # artifacts. It uses a third-party, MIT-licensed action (geekyeggo/delete-artifact). While Github

ci/kind/test-upgrade-antrea.sh

@@ -158,7 +158,7 @@ popd
 rm -rf $TMP_DIR
 
 rc=0
-go test -v -run=TestUpgrade github.com/vmware-tanzu/antrea/test/e2e -provider=kind -upgrade.toYML=antrea-new.yml --upgrade.controllerOnly=$CONTROLLER_ONLY || rc=$?
+go test -v -run=TestUpgrade github.com/vmware-tanzu/antrea/test/e2e -provider=kind -upgrade.toYML=antrea-new.yml --upgrade.controllerOnly=$CONTROLLER_ONLY --logs-export-dir=$ANTREA_LOG_DIR || rc=$?
 
 $THIS_DIR/kind-setup.sh destroy kind
 

pkg/apis/controlplane/v1beta1/conversion.go

@@ -109,9 +109,11 @@ func Convert_controlplane_GroupMember_To_v1beta1_GroupMember(in *controlplane.Gr
 }
 
 func Convert_v1beta1_GroupMemberPod_To_controlplane_GroupMember(in *GroupMemberPod, out *controlplane.GroupMember, s conversion.Scope) error {
-	out.Pod = &controlplane.PodReference{
-		Name:      in.Pod.Name,
-		Namespace: in.Pod.Namespace,
+	if in.Pod != nil {
+		out.Pod = &controlplane.PodReference{
+			Name:      in.Pod.Name,
+			Namespace: in.Pod.Namespace,
+		}
 	}
 	out.IPs = []controlplane.IPAddress{controlplane.IPAddress(in.IP)}
 	ports := make([]controlplane.NamedPort, len(in.Ports))
@@ -124,13 +126,20 @@ func Convert_v1beta1_GroupMemberPod_To_controlplane_GroupMember(in *GroupMemberP
 	return nil
 }
 
-func Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(in *controlplane.GroupMember, out *GroupMemberPod, s conversion.Scope) error {
+// Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod converts controlplane GroupMember to v1beta1 GroupMember
+// based on whether it's required to include Pod reference in the result. We must not include Pod reference when the
+// conversion is called for an AddressGroup as agents don't expect it in v1beta1 version.
+// This function doesn't match the pattern of conversion function which requires the last parameter to be
+// conversion.Scope so won't be registered to schema.
+func Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(in *controlplane.GroupMember, out *GroupMemberPod, includePodRef bool) error {
 	if in.Pod == nil || len(in.IPs) > 1 {
 		return fmt.Errorf("cannot convert ExternalEntity or dual stack Pod into GroupMemberPod")
 	}
-	out.Pod = &PodReference{
-		Name:      in.Pod.Name,
-		Namespace: in.Pod.Namespace,
+	if includePodRef {
+		out.Pod = &PodReference{
+			Name:      in.Pod.Name,
+			Namespace: in.Pod.Namespace,
+		}
 	}
 	if len(in.IPs) > 0 {
 		out.IP = IPAddress(in.IPs[0])
@@ -172,7 +181,7 @@ func Convert_controlplane_AddressGroup_To_v1beta1_AddressGroup(in *controlplane.
 		m := in.GroupMembers[i]
 		if m.Pod != nil {
 			var pod GroupMemberPod
-			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
+			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, false); err != nil {
 				return err
 			}
 			pods = append(pods, pod)
@@ -227,7 +236,7 @@ func Convert_controlplane_AddressGroupPatch_To_v1beta1_AddressGroupPatch(in *con
 		m := in.AddedGroupMembers[i]
 		if m.Pod != nil {
 			var pod GroupMemberPod
-			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
+			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, false); err != nil {
 				return err
 			}
 			addedPods = append(addedPods, pod)
@@ -241,7 +250,7 @@ func Convert_controlplane_AddressGroupPatch_To_v1beta1_AddressGroupPatch(in *con
 		m := in.RemovedGroupMembers[i]
 		if m.Pod != nil {
 			var pod GroupMemberPod
-			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
+			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, false); err != nil {
 				return err
 			}
 			removedPods = append(removedPods, pod)
@@ -285,7 +294,7 @@ func Convert_controlplane_AppliedToGroup_To_v1beta1_AppliedToGroup(in *controlpl
 		m := in.GroupMembers[i]
 		if m.Pod != nil {
 			var pod GroupMemberPod
-			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
+			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, true); err != nil {
 				return err
 			}
 			pods = append(pods, pod)
@@ -341,7 +350,7 @@ func Convert_controlplane_AppliedToGroupPatch_To_v1beta1_AppliedToGroupPatch(in
 		m := in.AddedGroupMembers[i]
 		if m.Pod != nil {
 			var pod GroupMemberPod
-			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
+			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, true); err != nil {
 				return err
 			}
 			addedPods = append(addedPods, pod)
@@ -355,7 +364,7 @@ func Convert_controlplane_AppliedToGroupPatch_To_v1beta1_AppliedToGroupPatch(in
 		m := in.RemovedGroupMembers[i]
 		if m.Pod != nil {
 			var pod GroupMemberPod
-			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
+			if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, true); err != nil {
 				return err
 			}
 			removedPods = append(removedPods, pod)

pkg/apis/controlplane/v1beta1/conversion_test.go

@@ -146,11 +146,17 @@ func TestConvertBetweenV1beta1GroupMemberPodAndControlplaneGroupMember(t *testin
 
 	var convertedCPGroupMember controlplane.GroupMember
 	var convertedV1B1GroupMemberPod GroupMemberPod
-	err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpGroupMember, &convertedV1B1GroupMemberPod, nil)
+	err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpGroupMember, &convertedV1B1GroupMemberPod, true)
 	require.Errorf(t, err, "should not be able to convert group member with multiple IPs to GroupMemberPod")
 	require.NoError(t,
-		Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpPodGroupMember, &convertedV1B1GroupMemberPod, nil))
+		Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpPodGroupMember, &convertedV1B1GroupMemberPod, true))
 	assert.Equal(t, v1b1GroupMemberPod, convertedV1B1GroupMemberPod, "controlplane.GroupMember -> v1beta1.GroupMemberPod")
+	var convertedV1B1GroupMemberPodWithoutPodRef GroupMemberPod
+	require.NoError(t,
+		Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpPodGroupMember, &convertedV1B1GroupMemberPodWithoutPodRef, false))
+	expectedV1b1GroupMemberPodWithoutPodRef := *v1b1GroupMemberPod.DeepCopy()
+	expectedV1b1GroupMemberPodWithoutPodRef.Pod = nil
+	assert.Equal(t, expectedV1b1GroupMemberPodWithoutPodRef, convertedV1B1GroupMemberPodWithoutPodRef, "controlplane.GroupMember -> v1beta1.GroupMemberPod")
 	require.NoError(t,
 		Convert_v1beta1_GroupMemberPod_To_controlplane_GroupMember(&v1b1GroupMemberPod, &convertedCPGroupMember, nil))
 	assert.Equal(t, cpPodGroupMember, convertedCPGroupMember, "v1beta1.GroupMemberPod -> controlplane.GroupMember")
@@ -169,9 +175,11 @@ func TestConvertBetweenV1beta1AndControlplaneAddressGroup(t *testing.T) {
 	}
 	var convertedCPAddressGroup controlplane.AddressGroup
 	var convertedV1B1AddressGroup AddressGroup
+	expectedV1B1AddressGroup := v1b1AddressGroup.DeepCopy()
+	expectedV1B1AddressGroup.Pods[0].Pod = nil
 	require.NoError(t,
 		Convert_controlplane_AddressGroup_To_v1beta1_AddressGroup(&cpAddressGroup, &convertedV1B1AddressGroup, nil))
-	assert.Equal(t, v1b1AddressGroup, convertedV1B1AddressGroup)
+	assert.Equal(t, *expectedV1B1AddressGroup, convertedV1B1AddressGroup)
 	require.NoError(t,
 		Convert_v1beta1_AddressGroup_To_controlplane_AddressGroup(&v1b1AddressGroup, &convertedCPAddressGroup, nil))
 	assert.Equal(t, cpAddressGroup, convertedCPAddressGroup)
@@ -193,9 +201,12 @@ func TestConvertBetweenV1beta1AndControlplaneAddressGroupPatch(t *testing.T) {
 	}
 	var convertedCPPatch controlplane.AddressGroupPatch
 	var convertedV1B1Patch AddressGroupPatch
+	expectedV1B1AddressGroupPatch := v1b1AddressGroupPatch.DeepCopy()
+	expectedV1B1AddressGroupPatch.AddedPods[0].Pod = nil
+	expectedV1B1AddressGroupPatch.RemovedPods[0].Pod = nil
 	require.NoError(t,
 		Convert_controlplane_AddressGroupPatch_To_v1beta1_AddressGroupPatch(&cpAddressGroupPatch, &convertedV1B1Patch, nil))
-	assert.Equal(t, v1b1AddressGroupPatch, convertedV1B1Patch)
+	assert.Equal(t, *expectedV1B1AddressGroupPatch, convertedV1B1Patch)
 	require.NoError(t,
 		Convert_v1beta1_AddressGroupPatch_To_controlplane_AddressGroupPatch(&v1b1AddressGroupPatch, &convertedCPPatch, nil))
 	assert.Equal(t, cpAddressGroupPatch, convertedCPPatch)

test/e2e/networkpolicy_test.go

@@ -157,27 +157,43 @@ func TestDifferentNamedPorts(t *testing.T) {
 		t.Fatalf("Error when setting up test: %v", err)
 	}
 	defer teardownTest(t, data)
-	data.testDifferentNamedPorts(t)
+	checkFn, cleanupFn := data.setupDifferentNamedPorts(t)
+	defer cleanupFn()
+	checkFn()
 }
 
-func (data *TestData) testDifferentNamedPorts(t *testing.T) {
+func (data *TestData) setupDifferentNamedPorts(t *testing.T) (checkFn func(), cleanupFn func()) {
+	var success bool
+	var cleanupFuncs []func()
+	cleanupFn = func() {
+		for i := len(cleanupFuncs) - 1; i >= 0; i-- {
+			cleanupFuncs[i]()
+		}
+	}
+	// Call cleanupFn only if the function fails. In case of success, we will call cleanupFn in callers.
+	defer func() {
+		if !success {
+			cleanupFn()
+		}
+	}()
+
 	server0Port := 80
-	_, server0IPs, cleanupFunc := createAndWaitForPod(t, data, func(name string, nodeName string) error {
+	server0Name, server0IPs, cleanupFunc := createAndWaitForPod(t, data, func(name string, nodeName string) error {
 		return data.createServerPod(name, "http", server0Port, false)
 	}, "test-server-", "")
-	defer cleanupFunc()
+	cleanupFuncs = append(cleanupFuncs, cleanupFunc)
 
 	server1Port := 8080
-	_, server1IPs, cleanupFunc := createAndWaitForPod(t, data, func(name string, nodeName string) error {
+	server1Name, server1IPs, cleanupFunc := createAndWaitForPod(t, data, func(name string, nodeName string) error {
 		return data.createServerPod(name, "http", server1Port, false)
 	}, "test-server-", "")
-	defer cleanupFunc()
+	cleanupFuncs = append(cleanupFuncs, cleanupFunc)
 
 	client0Name, _, cleanupFunc := createAndWaitForPod(t, data, data.createBusyboxPodOnNode, "test-client-", "")
-	defer cleanupFunc()
+	cleanupFuncs = append(cleanupFuncs, cleanupFunc)
 
 	client1Name, _, cleanupFunc := createAndWaitForPod(t, data, data.createBusyboxPodOnNode, "test-client-", "")
-	defer cleanupFunc()
+	cleanupFuncs = append(cleanupFuncs, cleanupFunc)
 
 	preCheckFunc := func(server0IP, server1IP string) {
 		// Both clients can connect to both servers.
@@ -201,8 +217,14 @@ func (data *TestData) testDifferentNamedPorts(t *testing.T) {
 
 	// Create NetworkPolicy rule.
 	spec := &networkingv1.NetworkPolicySpec{
-		// Apply to all Pods.
-		PodSelector: metav1.LabelSelector{},
+		// Apply to two server Pods.
+		PodSelector: metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{
+			{
+				Key:      "antrea-e2e",
+				Operator: metav1.LabelSelectorOpIn,
+				Values:   []string{server0Name, server1Name},
+			},
+		}},
 		// Allow client0 to access named port: "http".
 		Ingress: []networkingv1.NetworkPolicyIngressRule{{
 			Ports: []networkingv1.NetworkPolicyPort{{
@@ -217,15 +239,16 @@ func (data *TestData) testDifferentNamedPorts(t *testing.T) {
 			},
 		}},
 	}
-	np, err := data.createNetworkPolicy("test-networkpolicy-allow-client0-to-http", spec)
+	np, err := data.createNetworkPolicy(randName("test-networkpolicy-allow-client0-to-http"), spec)
 	if err != nil {
 		t.Fatalf("Error when creating network policy: %v", err)
 	}
-	defer func() {
+	cleanupFuncs = append(cleanupFuncs, func() {
 		if err = data.deleteNetworkpolicy(np); err != nil {
 			t.Fatalf("Error when deleting network policy: %v", err)
 		}
-	}()
+	})
+	time.Sleep(networkPolicyDelay)
 
 	npCheck := func(server0IP, server1IP string) {
 		// client0 can connect to both servers.
@@ -244,15 +267,18 @@ func (data *TestData) testDifferentNamedPorts(t *testing.T) {
 		}
 	}
 
-	// NetworkPolicy check.
-	if clusterInfo.podV4NetworkCIDR != "" {
-		npCheck(server0IPs.ipv4.String(), server1IPs.ipv4.String())
-	}
+	checkFn = func() {
+		// NetworkPolicy check.
+		if clusterInfo.podV4NetworkCIDR != "" {
+			npCheck(server0IPs.ipv4.String(), server1IPs.ipv4.String())
+		}
 
-	if clusterInfo.podV6NetworkCIDR != "" {
-		npCheck(server0IPs.ipv6.String(), server1IPs.ipv6.String())
+		if clusterInfo.podV6NetworkCIDR != "" {
+			npCheck(server0IPs.ipv6.String(), server1IPs.ipv6.String())
+		}
 	}
-
+	success = true
+	return
 }
 
 func TestDefaultDenyEgressPolicy(t *testing.T) {

test/e2e/upgrade_test.go

@@ -70,7 +70,12 @@ func TestUpgrade(t *testing.T) {
 
 	data.testPodConnectivitySameNode(t)
 	data.testPodConnectivityDifferentNodes(t)
-	data.testDifferentNamedPorts(t)
+	// We test NetworkPolicy with 2 scenarios:
+	// 1. The NetworkPolicy is created before upgrading controller.
+	// 2. The NetworkPolicy is created after upgrading controller.
+	checkFn, cleanupFn := data.setupDifferentNamedPorts(t)
+	defer cleanupFn()
+	checkFn()
 
 	if t.Failed() {
 		t.FailNow()
@@ -97,7 +102,13 @@ func TestUpgrade(t *testing.T) {
 
 	data.testPodConnectivitySameNode(t)
 	data.testPodConnectivityDifferentNodes(t)
-	data.testDifferentNamedPorts(t)
+	// Verify that the NetworkPolicy created before upgrading still works.
+	checkFn()
+	// Verify that the NetworkPolicy created after upgrading works.
+	// random resource names are used in the test so it's OK to call setupDifferentNamedPorts the second time.
+	checkFn, cleanupFn = data.setupDifferentNamedPorts(t)
+	defer cleanupFn()
+	checkFn()
 
 	t.Logf("Deleting namespace '%s'", namespace)
 	if err := data.deleteNamespace(namespace, defaultTimeout); err != nil {