From ee92110e961709278bb915473c2a3909ee573780 Mon Sep 17 00:00:00 2001 From: Jacob Tanenbaum Date: Tue, 21 Dec 2021 11:00:57 -0500 Subject: [PATCH 1/3] fixes flakes with networkPolicy egress upstream tests attempts to add NetworkPolicy Egress tests in origin PR https://github.com/openshift/origin/pull/26647 are flaky because of assumptions made about when it is safe to unisolate pods new pods that existing networkPolicies apply to Becuase the syncFlows is rate limited it is not safe to assume that all flows are synced when returning from np.refreshPodNetworkPolicies(). This PR changes how the flows that restrict new pods are removed by removing the isolating flows as part of syncFlows if a recalulation is required. --- pkg/network/node/networkpolicy.go | 37 ++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/pkg/network/node/networkpolicy.go b/pkg/network/node/networkpolicy.go index e0b23c587..073b0a6b2 100644 --- a/pkg/network/node/networkpolicy.go +++ b/pkg/network/node/networkpolicy.go @@ -48,6 +48,10 @@ type networkPolicyPlugin struct { warnedPolicies map[ktypes.UID]string skippedPolicies map[ktypes.UID]string + + // ips includes the ips of pods that have been created and are affected by an + // existing network policy + ips []string } // npNamespace tracks NetworkPolicy-related data for a Namespace @@ -109,6 +113,8 @@ func NewNetworkPolicyPlugin() osdnPolicy { warnedPolicies: make(map[ktypes.UID]string), skippedPolicies: make(map[ktypes.UID]string), + + ips: []string{}, } } @@ -330,6 +336,15 @@ func (np *networkPolicyPlugin) syncFlows() { npns.mustSync = false } } + + for _, ip := range np.ips { + // remove network isolation from pods that have been added and + // caused a flow recalculation + otx.DeleteFlows("table=27, cookie=1/-1, ip, nw_src=%s", ip) + otx.DeleteFlows("table=80, cookie=1/-1, ip, nw_src=%s", ip) + } + // reset the list of pods started since the last sync + np.ips = []string{} if err := otx.Commit(); err != nil { klog.Errorf("Error syncing OVS flows: %v", err) } @@ -932,14 +947,7 @@ func (np *networkPolicyPlugin) handleAddOrUpdatePod(obj, old interface{}, eventT } np.lock.Lock() - defer func() { - otx := np.node.oc.NewTransaction() - otx.DeleteFlows("table=27, cookie=1/-1, ip, nw_src=%s", pod.Status.PodIP) - otx.DeleteFlows("table=80, cookie=1/-1, ip, nw_src=%s", pod.Status.PodIP) - otx.Commit() - - np.lock.Unlock() - }() + defer np.lock.Unlock() np.refreshPodNetworkPolicies(pod) } @@ -1019,10 +1027,15 @@ func (np *networkPolicyPlugin) refreshNamespaceNetworkPolicies() { } func (np *networkPolicyPlugin) refreshPodNetworkPolicies(pod *corev1.Pod) { + podNeedsSync := false podNs := np.namespacesByName[pod.Namespace] for _, npns := range np.namespaces { for _, npp := range npns.policies { if (npp.watchesOwnPods && npns == podNs) || npp.watchesAllPods { + if !podNeedsSync { + np.ips = append(np.ips, pod.Status.PodIP) + podNeedsSync = true + } npns.mustRecalculate = true } } @@ -1030,6 +1043,14 @@ func (np *networkPolicyPlugin) refreshPodNetworkPolicies(pod *corev1.Pod) { np.syncNamespace(npns) } } + if !podNeedsSync && len(pod.Status.PodIP) > 0 { + otx := np.node.oc.NewTransaction() + otx.DeleteFlows("table=27, cookie=1/-1, ip, nw_src=%s", pod.Status.PodIP) + otx.DeleteFlows("table=80, cookie=1/-1, ip, nw_src=%s", pod.Status.PodIP) + if err := otx.Commit(); err != nil { + klog.Errorf("Error syncing OVS flows to remove isolation from pod %s with ip %s (%v)", pod.Name, pod.Status.PodIP, err) + } + } } func getPodFullName(pod *corev1.Pod) string { From a4ff864ac6803a06647f8dfe53e4757ac94d4028 Mon Sep 17 00:00:00 2001 From: Jacob Tanenbaum Date: Wed, 22 Dec 2021 14:07:46 -0500 Subject: [PATCH 2/3] checking this --- pkg/network/node/networkpolicy.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/network/node/networkpolicy.go b/pkg/network/node/networkpolicy.go index 073b0a6b2..11a6a54d6 100644 --- a/pkg/network/node/networkpolicy.go +++ b/pkg/network/node/networkpolicy.go @@ -340,6 +340,7 @@ func (np *networkPolicyPlugin) syncFlows() { for _, ip := range np.ips { // remove network isolation from pods that have been added and // caused a flow recalculation + klog.Errorf("KEYWORD: why is maybe this invalid (%s)", ip) otx.DeleteFlows("table=27, cookie=1/-1, ip, nw_src=%s", ip) otx.DeleteFlows("table=80, cookie=1/-1, ip, nw_src=%s", ip) } From 7dd0bed7e3c151233731b995ab7558fec3dbdaea Mon Sep 17 00:00:00 2001 From: Jacob Tanenbaum Date: Wed, 22 Dec 2021 14:09:25 -0500 Subject: [PATCH 3/3] this may fix it --- pkg/network/node/networkpolicy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/network/node/networkpolicy.go b/pkg/network/node/networkpolicy.go index 11a6a54d6..d60dbeb03 100644 --- a/pkg/network/node/networkpolicy.go +++ b/pkg/network/node/networkpolicy.go @@ -1033,7 +1033,7 @@ func (np *networkPolicyPlugin) refreshPodNetworkPolicies(pod *corev1.Pod) { for _, npns := range np.namespaces { for _, npp := range npns.policies { if (npp.watchesOwnPods && npns == podNs) || npp.watchesAllPods { - if !podNeedsSync { + if !podNeedsSync && len(pod.Status.PodIP) > 0 { np.ips = append(np.ips, pod.Status.PodIP) podNeedsSync = true }