From 87039c07f45d4adcb2687887a6a12a006c348c89 Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Sat, 21 Nov 2020 03:01:27 +0800 Subject: [PATCH] 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 | 52 +++++++++++++-- ci/kind/test-upgrade-antrea.sh | 2 +- pkg/apis/controlplane/v1beta1/conversion.go | 35 ++++++---- .../controlplane/v1beta1/conversion_test.go | 19 ++++-- .../v1beta1/zz_generated.conversion.go | 5 -- test/e2e/networkpolicy_test.go | 66 +++++++++++++------ test/e2e/upgrade_test.go | 15 ++++- 7 files changed, 145 insertions(+), 49 deletions(-) diff --git a/.github/workflows/kind_upgrade.yml b/.github/workflows/kind_upgrade.yml index ad808146a43..fab48021baf 100644 --- a/.github/workflows/kind_upgrade.yml +++ b/.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 diff --git a/ci/kind/test-upgrade-antrea.sh b/ci/kind/test-upgrade-antrea.sh index ff44ae1de4c..bf9149f1e1a 100755 --- a/ci/kind/test-upgrade-antrea.sh +++ b/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 diff --git a/pkg/apis/controlplane/v1beta1/conversion.go b/pkg/apis/controlplane/v1beta1/conversion.go index b79e6bed75c..70a14b3e4a4 100644 --- a/pkg/apis/controlplane/v1beta1/conversion.go +++ b/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) diff --git a/pkg/apis/controlplane/v1beta1/conversion_test.go b/pkg/apis/controlplane/v1beta1/conversion_test.go index df28d6b1347..376b6330660 100644 --- a/pkg/apis/controlplane/v1beta1/conversion_test.go +++ b/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) diff --git a/pkg/apis/controlplane/v1beta1/zz_generated.conversion.go b/pkg/apis/controlplane/v1beta1/zz_generated.conversion.go index b77390b7059..72e43831b8e 100644 --- a/pkg/apis/controlplane/v1beta1/zz_generated.conversion.go +++ b/pkg/apis/controlplane/v1beta1/zz_generated.conversion.go @@ -211,11 +211,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddConversionFunc((*controlplane.GroupMember)(nil), (*GroupMemberPod)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(a.(*controlplane.GroupMember), b.(*GroupMemberPod), scope) - }); err != nil { - return err - } if err := s.AddConversionFunc((*AddressGroupPatch)(nil), (*controlplane.AddressGroupPatch)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_AddressGroupPatch_To_controlplane_AddressGroupPatch(a.(*AddressGroupPatch), b.(*controlplane.AddressGroupPatch), scope) }); err != nil { diff --git a/test/e2e/networkpolicy_test.go b/test/e2e/networkpolicy_test.go index 91d74151b9e..e615f7690fd 100644 --- a/test/e2e/networkpolicy_test.go +++ b/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) { diff --git a/test/e2e/upgrade_test.go b/test/e2e/upgrade_test.go index 330648975fc..4e01f6b330d 100644 --- a/test/e2e/upgrade_test.go +++ b/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 {