Merge pull request #2006 from flavio-fernandes/eip_fix_excluding_matc…

…hes-release-4.12

[release-4.12] OCPBUGS-26243: Fix Egress IP Deletion Handler to Prevent OVN Policy Leaks

go-controller/pkg/ovn/egressip.go

@@ -504,8 +504,10 @@ func (oc *Controller) reconcileEgressIPPod(old, new *v1.Pod) (err error) {
 			// match all pods in the namespace.
 			podSelector, _ := metav1.LabelSelectorAsSelector(&egressIP.Spec.PodSelector)
 			if !podSelector.Empty() {
-				newMatches := podSelector.Matches(newPodLabels)
-				oldMatches := podSelector.Matches(oldPodLabels)
+				// Use "new" and "old" instead of "newPod" and "oldPod" to determine whether
+				// pods was created or is being deleted.
+				newMatches := new != nil && podSelector.Matches(newPodLabels)
+				oldMatches := old != nil && podSelector.Matches(oldPodLabels)
 				// If the podSelector doesn't match the pod, then continue
 				// because this EgressIP intends to match other pods in that
 				// namespace and not this one. Other EgressIP objects might
@@ -514,8 +516,7 @@ func (oc *Controller) reconcileEgressIPPod(old, new *v1.Pod) (err error) {
 					continue
 				}
 				// Check if the pod stopped matching. If the pod was deleted,
-				// "new" will be nil and newPodLabels will not match, so this is
-				// should cover that case.
+				// "new" will be nil, so this must account for that case.
 				if !newMatches && oldMatches {
 					if err := oc.deletePodEgressIPAssignments(egressIP.Name, egressIP.Status.Items, oldPod); err != nil {
 						return err

go-controller/pkg/ovn/egressip_test.go

@@ -3392,7 +3392,7 @@ var _ = ginkgo.Describe("OVN master EgressIP Operations", func() {
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 		})
 
-		ginkgo.It("should delete and re-create", func() {
+		ginkgo.It("should delete and re-create and delete", func() {
 			app.Action = func(ctx *cli.Context) error {
 
 				egressIP := net.ParseIP("0:0:0:0:0:feff:c0a8:8e0d")
@@ -3522,12 +3522,25 @@ var _ = ginkgo.Describe("OVN master EgressIP Operations", func() {
 				eIPUpdate, err := fakeOvn.fakeClient.EgressIPClient.K8sV1().EgressIPs().Get(context.TODO(), eIP.Name, metav1.GetOptions{})
 				gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
+				// For pod selector, let's test a use case where the selector only looks for keys that are not
+				// present in the pod. That should be properly handled in deletion cases, where the match is
+				// true, but the "new" pod object is nil.
 				eIPUpdate.Spec = egressipv1.EgressIPSpec{
 					EgressIPs: []string{
 						updatedEgressIP.String(),
 					},
 					PodSelector: metav1.LabelSelector{
-						MatchLabels: egressPodLabel,
+						MatchExpressions: []metav1.LabelSelectorRequirement{
+							{
+								Key:      "someNonExistingKey",
+								Operator: metav1.LabelSelectorOpDoesNotExist,
+							},
+							{
+								Key:      "egress",
+								Operator: metav1.LabelSelectorOpNotIn,
+								Values:   []string{"not_needed", "needle", "haystack"},
+							},
+						},
 					},
 					NamespaceSelector: metav1.LabelSelector{
 						MatchLabels: egressPodLabel,
@@ -3544,6 +3557,34 @@ var _ = ginkgo.Describe("OVN master EgressIP Operations", func() {
 				}).Should(gomega.ContainElement(updatedEgressIP.String()))
 
 				gomega.Expect(nodes[0]).To(gomega.Equal(node2.name))
+
+				// Remove pod and ensure LRP and SNAT are removed as well
+				ginkgo.By("Deleting the completed pod should update EIPs SNATs")
+				err = fakeOvn.fakeClient.KubeClient.CoreV1().Pods(namespace).Delete(context.TODO(), podName, metav1.DeleteOptions{})
+				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+				expectedDatabaseState2 := []libovsdbtest.TestData{
+					&nbdb.LogicalRouter{
+						Name:     ovntypes.OVNClusterRouter,
+						UUID:     ovntypes.OVNClusterRouter + "-UUID",
+						Policies: []string{},
+					},
+					&nbdb.LogicalRouterPort{
+						UUID:     ovntypes.GWRouterToJoinSwitchPrefix + ovntypes.GWRouterPrefix + node2.name + "-UUID",
+						Name:     ovntypes.GWRouterToJoinSwitchPrefix + ovntypes.GWRouterPrefix + node2.name,
+						Networks: []string{nodeLogicalRouterIfAddrV6},
+					},
+					&nbdb.LogicalRouter{
+						Name: ovntypes.GWRouterPrefix + node1.name,
+						UUID: ovntypes.GWRouterPrefix + node1.name + "-UUID",
+					},
+					&nbdb.LogicalRouter{
+						Name: ovntypes.GWRouterPrefix + node2.name,
+						UUID: ovntypes.GWRouterPrefix + node2.name + "-UUID",
+						Nat:  []string{},
+					},
+				}
+				gomega.Eventually(fakeOvn.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState2))
 				return nil
 			}