From 863dcc55905656f187b7ef7e497e6455ace7785b Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Thu, 18 May 2023 00:05:08 +0800 Subject: [PATCH 1/2] Use LOCAL instead of CONTROLLER as the in_port of packet-out message CONTROLLER cannot be used as the in_port due to a bug in Windows ovsext driver, otherwise the Windows OS would crash. See https://github.com/openvswitch/ovs-issues/issues/280. Signed-off-by: Quan Tian --- pkg/agent/proxy/proxier.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/agent/proxy/proxier.go b/pkg/agent/proxy/proxier.go index bd67f59788c..3a8fc1f3a0a 100644 --- a/pkg/agent/proxy/proxier.go +++ b/pkg/agent/proxy/proxier.go @@ -23,6 +23,7 @@ import ( "sync" "time" + "antrea.io/libOpenflow/openflow15" "antrea.io/libOpenflow/protocol" "antrea.io/ofnet/ofctrl" corev1 "k8s.io/api/core/v1" @@ -1034,12 +1035,15 @@ func (p *proxier) HandlePacketIn(pktIn *ofctrl.PacketIn) error { return fmt.Errorf("error when getting match field inPort") } outPort := inPortField.GetValue().(uint32) + // It cannot use CONTROLLER (the default value when inPort is 0) as the inPort due to a bug in Windows ovsext + // driver, otherwise the Windows OS would crash. See https://github.com/openvswitch/ovs-issues/issues/280. + inPort := uint32(openflow15.P_LOCAL) return openflow.SendRejectPacketOut(p.ofClient, srcMAC, dstMAC, srcIP, dstIP, - 0, + inPort, outPort, isIPv6, ethernetPkt, From 16807592a615b256e020fed99ad5ce29c8f3d19e Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Wed, 17 May 2023 11:26:51 +0800 Subject: [PATCH 2/2] Fix policy not being cleaned if a policy with the same name exist We need to check if the UID matches when checking if a policy exists, because it's possible another policy is created with the same name after the policy is deleted. Otherwise the deleted policy would still be enforced. Signed-off-by: Quan Tian --- .../networkpolicy/networkpolicy_controller.go | 9 +- .../networkpolicy_controller_test.go | 86 +++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/pkg/controller/networkpolicy/networkpolicy_controller.go b/pkg/controller/networkpolicy/networkpolicy_controller.go index 629016f3aed..374aef56493 100644 --- a/pkg/controller/networkpolicy/networkpolicy_controller.go +++ b/pkg/controller/networkpolicy/networkpolicy_controller.go @@ -1425,21 +1425,24 @@ func (n *NetworkPolicyController) syncInternalNetworkPolicy(key *controlplane.Ne switch key.Type { case controlplane.AntreaClusterNetworkPolicy: cnp, err := n.cnpLister.Get(key.Name) - if err != nil { + // We need to check if the UID matches because it's possible another policy is created with the same name after + // the policy is deleted. It's safe to just delete the internal NetworkPolicy associated with the old policy as + // the two policies are different items in the workqueue and internalNetworkPolicyStore due to different UIDs. + if err != nil || cnp.UID != key.UID { n.deleteInternalNetworkPolicy(internalNetworkPolicyName) return nil } newInternalNetworkPolicy, newAppliedToGroups, newAddressGroups = n.processClusterNetworkPolicy(cnp) case controlplane.AntreaNetworkPolicy: anp, err := n.anpLister.NetworkPolicies(key.Namespace).Get(key.Name) - if err != nil { + if err != nil || anp.UID != key.UID { n.deleteInternalNetworkPolicy(internalNetworkPolicyName) return nil } newInternalNetworkPolicy, newAppliedToGroups, newAddressGroups = n.processAntreaNetworkPolicy(anp) case controlplane.K8sNetworkPolicy: knp, err := n.networkPolicyLister.NetworkPolicies(key.Namespace).Get(key.Name) - if err != nil { + if err != nil || knp.UID != key.UID { n.deleteInternalNetworkPolicy(internalNetworkPolicyName) return nil } diff --git a/pkg/controller/networkpolicy/networkpolicy_controller_test.go b/pkg/controller/networkpolicy/networkpolicy_controller_test.go index a5ac1b77c4c..3667b9737d0 100644 --- a/pkg/controller/networkpolicy/networkpolicy_controller_test.go +++ b/pkg/controller/networkpolicy/networkpolicy_controller_test.go @@ -3044,6 +3044,92 @@ func TestSyncInternalNetworkPolicy(t *testing.T) { checkGroupItemExistence(t, c.appliedToGroupStore) } +// TestSyncInternalNetworkPolicyWithSameName verifies SyncInternalNetworkPolicy can work correctly when processing +// multiple NetworkPolicies that have the same name. +func TestSyncInternalNetworkPolicyWithSameName(t *testing.T) { + // policyA and policyB have the same name but different UIDs. + policyA := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "foo", UID: "uidA"}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: selectorA, + }, + } + policyB := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "foo", UID: "uidB"}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: selectorB, + }, + } + + selectorAGroup := getNormalizedUID(antreatypes.NewGroupSelector("default", &selectorA, nil, nil, nil).NormalizedName) + selectorBGroup := getNormalizedUID(antreatypes.NewGroupSelector("default", &selectorB, nil, nil, nil).NormalizedName) + expectedPolicyA := &antreatypes.NetworkPolicy{ + UID: "uidA", + Name: "uidA", + SpanMeta: antreatypes.SpanMeta{NodeNames: sets.NewString()}, + SourceRef: &controlplane.NetworkPolicyReference{ + Type: controlplane.K8sNetworkPolicy, + Namespace: "default", + Name: "foo", + UID: "uidA", + }, + AppliedToGroups: []string{selectorAGroup}, + Rules: []controlplane.NetworkPolicyRule{}, + } + expectedPolicyB := &antreatypes.NetworkPolicy{ + UID: "uidB", + Name: "uidB", + SpanMeta: antreatypes.SpanMeta{NodeNames: sets.NewString()}, + SourceRef: &controlplane.NetworkPolicyReference{ + Type: controlplane.K8sNetworkPolicy, + Namespace: "default", + Name: "foo", + UID: "uidB", + }, + AppliedToGroups: []string{selectorBGroup}, + Rules: []controlplane.NetworkPolicyRule{}, + } + + // Add and sync policyA first, it should create an AppliedToGroup. + _, c := newController() + c.networkPolicyStore.Add(policyA) + networkPolicyRefA := getKNPReference(policyA) + assert.NoError(t, c.syncInternalNetworkPolicy(networkPolicyRefA)) + obj, exists, _ := c.internalNetworkPolicyStore.Get(expectedPolicyA.Name) + require.True(t, exists) + assert.Equal(t, expectedPolicyA, obj.(*antreatypes.NetworkPolicy)) + checkGroupItemExistence(t, c.appliedToGroupStore, selectorAGroup) + + // Delete policyA and add policyB, then sync them concurrently, the resources associated with policyA should be deleted. + c.networkPolicyStore.Delete(policyA) + c.networkPolicyStore.Add(policyB) + networkPolicyRefB := getKNPReference(policyB) + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + assert.NoError(t, c.syncInternalNetworkPolicy(networkPolicyRefA)) + }() + go func() { + defer wg.Done() + assert.NoError(t, c.syncInternalNetworkPolicy(networkPolicyRefB)) + }() + wg.Wait() + _, exists, _ = c.internalNetworkPolicyStore.Get(expectedPolicyA.Name) + require.False(t, exists) + obj, exists, _ = c.internalNetworkPolicyStore.Get(expectedPolicyB.Name) + require.True(t, exists) + assert.Equal(t, expectedPolicyB, obj.(*antreatypes.NetworkPolicy)) + checkGroupItemExistence(t, c.appliedToGroupStore, selectorBGroup) + + // Delete policyB and sync it, the resources associated with policyB should be deleted. + c.networkPolicyStore.Delete(policyB) + assert.NoError(t, c.syncInternalNetworkPolicy(networkPolicyRefB)) + _, exists, _ = c.internalNetworkPolicyStore.Get(expectedPolicyB.Name) + require.False(t, exists) + checkGroupItemExistence(t, c.appliedToGroupStore) +} + // TestSyncInternalNetworkPolicyConcurrently verifies SyncInternalNetworkPolicy can create and delete AppliedToGroups // and AddressGroups correctly when concurrently processing multiple NetworkPolicies that refer to the same groups. func TestSyncInternalNetworkPolicyConcurrently(t *testing.T) {