[release-4.9] Bug 2014003: Fix gateway routers answer ARP/NDP requests for LoadBalancer/ExternalIP services by andreaskaris · Pull Request #952 · openshift/ovn-kubernetes

andreaskaris · 2022-02-11T11:32:20Z

- What this PR does and why is it needed
Backport of the first 2 out of this series of patches: https://github.com/openshift/ovn-kubernetes/pull/793/commits
See https://github.com/ovn-org/ovn-kubernetes/blob/master/docs/external-ip-and-loadbalancer-ingress.md

Commit Shared GW: Remove code duplication in updateServiceFlowCache was for readability only, to remove code duplication. It has no impact on the functionality and does not merge cleanly, thus I did not backport it.

- Special notes for reviewers

This PR backports 2 commits:
a) Output address resolution requests to LOCAL port (clean)
b) Neighbor solicitations and ARP requests used to hit all 3 OVN (conflict)

For a): Clean cherry-pick:

Output address resolution requests to LOCAL port

Currently address resolution requests (ARP/Neighbor solicitation) for
LoadBalancer/External IPs are answered by all of the gateway routers in the cluster.
By forwarding these requests to the local port, a network load balancer implementation
like MetalLB is able to be the only one replying to them - thus enabling it
to be the only one announcing a specific LoadBalancer service IP.

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
(cherry picked from commit https://github.com/openshift/ovn-kubernetes/commit/5d546a0498dd0a55b80c05cfbcd60bf761581cc7)

For b): Conflicting cherry-pick:

Neighbor solicitations and ARP requests used to hit all 3 OVN

load-balancers in addition to the node local IP for ExternalIP.
ARP requests or IPv6 NS would receive <node number + 1> replies.

This fix stops ARP requests and IPv6 NS for ExternalIPs from entering
the OVN dataplane. Only the node with the actual local IP will now
answer to the NS or ARP request.

Signed-off-by: Andreas Karis <ak.karis@gmail.com>
(cherry picked from commit https://github.com/openshift/ovn-kubernetes/commit/91d37a667d041574f58e27b4aebbc0258a627816)

Conflicts:
	go-controller/pkg/node/gateway_shared_intf.go

I think git went a bit crazy wrt the conflict, as it tried to pull unrelated stuff and failed there - see below for what it tried to do. So, when working around the conflict, I just removed the methods / functions that shouldn't have been there, in the first place:

[akaris@linux ovn-kubernetes (conflict)]$ git cherry-pick -x 91d37a667d041574f58e27b4aebbc0258a627816
Auto-merging go-controller/pkg/node/gateway_shared_intf.go
CONFLICT (content): Merge conflict in go-controller/pkg/node/gateway_shared_intf.go
error: could not apply 91d37a667... Neighbor solicitations and ARP requests used to hit all 3 OVN
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
[akaris@linux ovn-kubernetes (conflict|CHERRY-PICKING)]$ git add go-controller/pkg/node/gateway_shared_intf.go
[akaris@linux ovn-kubernetes (conflict|CHERRY-PICKING)]$ git commit
[conflict 7dadf064e] Neighbor solicitations and ARP requests used to hit all 3 OVN load-balancers in addition to the node local IP for ExternalIP. ARP requests or IPv6 NS would receive <node number + 1> replies.
 Author: Ori Braunshtein <obraunsh@redhat.com>
 Date: Wed Oct 13 12:35:33 2021 +0300
 3 files changed, 267 insertions(+), 14 deletions(-)
[akaris@linux ovn-kubernetes (conflict)]$ git show | cat
commit 7dadf064ea1ce2b3408191971d8e99451cc3359d
Author: Ori Braunshtein <obraunsh@redhat.com>
Date:   Wed Oct 13 12:35:33 2021 +0300

    Neighbor solicitations and ARP requests used to hit all 3 OVN
    load-balancers in addition to the node local IP for ExternalIP.
    ARP requests or IPv6 NS would receive <node number + 1> replies.
    
    This fix stops ARP requests and IPv6 NS for ExternalIPs from entering
    the OVN dataplane. Only the node with the actual local IP will now
    answer to the NS or ARP request.
    
    Signed-off-by: Andreas Karis <ak.karis@gmail.com>
    (cherry picked from commit 91d37a667d041574f58e27b4aebbc0258a627816)

diff --git a/go-controller/pkg/node/gateway_shared_intf.go b/go-controller/pkg/node/gateway_shared_intf.go
index 38f54602f..322a098d2 100644
--- a/go-controller/pkg/node/gateway_shared_intf.go
+++ b/go-controller/pkg/node/gateway_shared_intf.go
@@ -25,6 +25,8 @@ const (
 	// defaultOpenFlowCookie identifies default open flow rules added to the host OVS bridge.
 	// The hex number 0xdeff105, aka defflos, is meant to sound like default flows.
 	defaultOpenFlowCookie = "0xdeff105"
+	// ovsLocalPort is the name of the OVS bridge local port
+	ovsLocalPort = "LOCAL"
 )
 
 // nodePortWatcher manages OpenfLow and iptables rules
@@ -106,14 +108,10 @@ func (npw *nodePortWatcher) updateServiceFlowCache(service *kapi.Service, add bo
 			flowProtocol := protocol
 			nwDst := "nw_dst"
 			nwSrc := "nw_src"
-			addrResDst := nwDst
-			addrResProto := "arp"
 			if utilnet.IsIPv6String(ing.IP) {
 				flowProtocol = protocol + "6"
 				nwDst = "ipv6_dst"
 				nwSrc = "ipv6_src"
-				addrResDst = "nd_target"
-				addrResProto = "icmp6, icmp_type=135, icmp_code=0"
 			}
 			key = strings.Join([]string{"Ingress", service.Namespace, service.Name, ingIP.String(), fmt.Sprintf("%d", svcPort.Port)}, "_")
 			if !add {
@@ -126,9 +124,7 @@ func (npw *nodePortWatcher) updateServiceFlowCache(service *kapi.Service, add bo
 					fmt.Sprintf("cookie=%s, priority=110, in_port=%s, %s, %s=%s, tp_src=%d, "+
 						"actions=output:%s",
 						cookie, npw.ofportPatch, flowProtocol, nwSrc, ing.IP, svcPort.Port, npw.ofportPhys),
-					fmt.Sprintf("cookie=%s, priority=110, in_port=%s, %s, %s=%s, "+
-						"actions=output:%s",
-						cookie, npw.ofportPhys, addrResProto, addrResDst, ing.IP, ovsLocalPort)})
+					npw.generateArpBypassFlow(protocol, ing.IP, cookie)})
 			}
 		}
 
@@ -136,14 +132,10 @@ func (npw *nodePortWatcher) updateServiceFlowCache(service *kapi.Service, add bo
 			flowProtocol := protocol
 			nwDst := "nw_dst"
 			nwSrc := "nw_src"
-			addrResDst := nwDst
-			addrResProto := "arp"
 			if utilnet.IsIPv6String(externalIP) {
 				flowProtocol = protocol + "6"
 				nwDst = "ipv6_dst"
 				nwSrc = "ipv6_src"
-				addrResDst = "nd_target"
-				addrResProto = "icmp6, icmp_type=135, icmp_code=0"
 			}
 			cookie, err = svcToCookie(service.Namespace, service.Name, externalIP, svcPort.Port)
 			if err != nil {
@@ -162,14 +154,154 @@ func (npw *nodePortWatcher) updateServiceFlowCache(service *kapi.Service, add bo
 					fmt.Sprintf("cookie=%s, priority=110, in_port=%s, %s, %s=%s, tp_src=%d, "+
 						"actions=output:%s",
 						cookie, npw.ofportPatch, flowProtocol, nwSrc, externalIP, svcPort.Port, npw.ofportPhys),
-					fmt.Sprintf("cookie=%s, priority=110, in_port=%s, %s, %s=%s, "+
-						"actions=output:%s",
-						cookie, npw.ofportPhys, addrResProto, addrResDst, externalIP, ovsLocalPort)})
+					npw.generateArpBypassFlow(protocol, externalIP, cookie)})
+			}
+		}
+	}
+}
+
+<<<<<<< HEAD
+=======
+// generate ARP/NS bypass flow which will send the ARP/NS request everywhere *but* to OVN
+// OpenFlow will not do hairpin switching, so we can safely add the origin port to the list of ports, too
+func (npw *nodePortWatcher) generateArpBypassFlow(protocol string, ipAddr string, cookie string) string {
+	addrResDst := "arp_tpa"
+	addrResProto := "arp, arp_op=1"
+	if utilnet.IsIPv6String(ipAddr) {
+		addrResDst = "nd_target"
+		addrResProto = "icmp6, icmp_type=135, icmp_code=0"
+	}
+
+	var arpFlow string
+	var arpPortsFiltered []string
+	arpPorts, err := util.GetOpenFlowPorts(npw.gwBridge, false)
+	if err != nil {
+		// in the odd case that getting all ports from the bridge should not work,
+		// simply output to LOCAL (this should work well in the vast majority of cases, anyway)
+		klog.Warningf("Unable to get port list from bridge. Using ovsLocalPort as output only: error: %v",
+			err)
+		arpFlow = fmt.Sprintf("cookie=%s, priority=110, in_port=%s, %s, %s=%s, "+
+			"actions=output:%s",
+			cookie, npw.ofportPhys, addrResProto, addrResDst, ipAddr, ovsLocalPort)
+	} else {
+		// cover the case where breth0 has more than 3 ports, e.g. if an admin adds a 4th port
+		// and the ExternalIP would be on that port
+		// Use all ports except for ofPortPhys and the ofportPatch
+		// Filtering ofPortPhys is for consistency / readability only, OpenFlow will not send
+		// out the in_port normally (see man 7 ovs-actions)
+		for _, port := range arpPorts {
+			if port == npw.ofportPatch || port == npw.ofportPhys {
+				continue
 			}
+			arpPortsFiltered = append(arpPortsFiltered, port)
 		}
+		arpFlow = fmt.Sprintf("cookie=%s, priority=110, in_port=%s, %s, %s=%s, "+
+			"actions=output:%s",
+			cookie, npw.ofportPhys, addrResProto, addrResDst, ipAddr, strings.Join(arpPortsFiltered, ","))
+	}
+
+	return arpFlow
+}
+
+// getAndDeleteServiceInfo returns the serviceConfig for a service and if it exists and then deletes the entry
+func (npw *nodePortWatcher) getAndDeleteServiceInfo(index ktypes.NamespacedName) (out *serviceConfig, exists bool) {
+	npw.serviceInfoLock.Lock()
+	defer npw.serviceInfoLock.Unlock()
+	out, exists = npw.serviceInfo[index]
+	delete(npw.serviceInfo, index)
+	return out, exists
+}
+
+// getServiceInfo returns the serviceConfig for a service and if it exists
+func (npw *nodePortWatcher) getServiceInfo(index ktypes.NamespacedName) (out *serviceConfig, exists bool) {
+	npw.serviceInfoLock.Lock()
+	defer npw.serviceInfoLock.Unlock()
+	out, exists = npw.serviceInfo[index]
+	return out, exists
+}
+
+// getAndSetServiceInfo creates and sets the serviceConfig, returns if it existed and whatever was there
+func (npw *nodePortWatcher) getAndSetServiceInfo(index ktypes.NamespacedName, service *kapi.Service, etpHostRules bool) (old *serviceConfig, exists bool) {
+	npw.serviceInfoLock.Lock()
+	defer npw.serviceInfoLock.Unlock()
+
+	old, exists = npw.serviceInfo[index]
+	npw.serviceInfo[index] = &serviceConfig{service: service, etpHostRules: etpHostRules}
+	return old, exists
+}
+
+// addOrSetServiceInfo creates and sets the serviceConfig if it doesn't exist
+func (npw *nodePortWatcher) addOrSetServiceInfo(index ktypes.NamespacedName, service *kapi.Service, etpHostRules bool) (exists bool) {
+	npw.serviceInfoLock.Lock()
+	defer npw.serviceInfoLock.Unlock()
+
+	if _, exists := npw.serviceInfo[index]; !exists {
+		// Only set this if it doesn't exist
+		npw.serviceInfo[index] = &serviceConfig{service: service, etpHostRules: etpHostRules}
+		return false
 	}
+	return true
+
+}
+
+// updateServiceInfo sets the serviceConfig for a service and returns the existing serviceConfig, if inputs are nil
+// do not update those fields, if it does not exist return nil.
+func (npw *nodePortWatcher) updateServiceInfo(index ktypes.NamespacedName, service *kapi.Service, etpHostRules *bool) (old *serviceConfig, exists bool) {
+
+	npw.serviceInfoLock.Lock()
+	defer npw.serviceInfoLock.Unlock()
+
+	if old, exists = npw.serviceInfo[index]; !exists {
+		klog.V(5).Infof("No serviceConfig found for service %s in namespace %s", index.Name, index.Namespace)
+		return nil, exists
+	}
+
+	if service != nil {
+		npw.serviceInfo[index].service = service
+	}
+
+	if etpHostRules != nil {
+		npw.serviceInfo[index].etpHostRules = *etpHostRules
+	}
+
+	return old, exists
+}
+
+// addServiceRules ensures the correct iptables rules and OpenFlow physical
+// flows are programmed for a given service and hostNetwork endpoint configuration
+func addServiceRules(service *kapi.Service, hasHostNet bool, npw *nodePortWatcher) {
+	if util.ServiceExternalTrafficPolicyLocal(service) && hasHostNet {
+		klog.V(5).Infof("Adding externalTrafficPolicy:local and hostNetworked rules for %v", service)
+		npw.updateServiceFlowCache(service, true, true)
+		npw.ofm.requestFlowSync()
+		addSharedGatewayIptRules(service, true)
+	} else {
+		npw.updateServiceFlowCache(service, true, false)
+		npw.ofm.requestFlowSync()
+		addSharedGatewayIptRules(service, false)
+	}
+}
+
+// delServiceRules deletes all possible iptables rules and OpenFlow physical
+// flows for a service
+func delServiceRules(service *kapi.Service, npw *nodePortWatcher) {
+	npw.updateServiceFlowCache(service, false, false)
+	npw.ofm.requestFlowSync()
+	// Always try and delete all rules here
+	delSharedGatewayIptRules(service, true)
+	delSharedGatewayIptRules(service, false)
+}
+
+func serviceUpdateNeeded(old, new *kapi.Service) bool {
+	return reflect.DeepEqual(new.Spec.Ports, old.Spec.Ports) &&
+		reflect.DeepEqual(new.Spec.ExternalIPs, old.Spec.ExternalIPs) &&
+		reflect.DeepEqual(new.Spec.ClusterIP, old.Spec.ClusterIP) &&
+		reflect.DeepEqual(new.Spec.Type, old.Spec.Type) &&
+		reflect.DeepEqual(new.Status.LoadBalancer.Ingress, old.Status.LoadBalancer.Ingress) &&
+		reflect.DeepEqual(new.Spec.ExternalTrafficPolicy, old.Spec.ExternalTrafficPolicy)
 }
 
+>>>>>>> 91d37a667 (Neighbor solicitations and ARP requests used to hit all 3 OVN)
 // AddService handles configuring shared gateway bridge flows to steer External IP, Node Port, Ingress LB traffic into OVN
 func (npw *nodePortWatcher) AddService(service *kapi.Service) {

You can compare:
1813ea5
and
91d37a6
to see that they are identical.

- How to verify it

- Description for the changelog

Currently address resolution requests (ARP/Neighbor solicitation) for LoadBalancer/External IPs are answered by all of the gateway routers in the cluster. By forwarding these requests to the local port, a network load balancer implementation like MetalLB is able to be the only one replying to them - thus enabling it to be the only one announcing a specific LoadBalancer service IP. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com> (cherry picked from commit 5d546a0)

load-balancers in addition to the node local IP for ExternalIP. ARP requests or IPv6 NS would receive <node number + 1> replies. This fix stops ARP requests and IPv6 NS for ExternalIPs from entering the OVN dataplane. Only the node with the actual local IP will now answer to the NS or ARP request. Signed-off-by: Andreas Karis <ak.karis@gmail.com> (cherry picked from commit 91d37a6) Conflicts: go-controller/pkg/node/gateway_shared_intf.go

openshift-ci · 2022-02-11T11:32:29Z

@andreaskaris: This pull request references Bugzilla bug 2014003, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 1987445 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 1987445 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0
bug has dependents

Requesting review from QA contact:
/cc @anuragthehatter

Details

In response to this:

[release-4.9] Bug 2014003: Fix gateway routers answer ARP/NDP requests for LoadBalancer/ExternalIP services

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andreaskaris · 2022-02-11T16:55:16Z

/retest

andreaskaris · 2022-02-14T10:59:12Z

/retest

andreaskaris · 2022-02-14T12:44:48Z

Verification instructions for QE:

verification.txt

openshift-ci · 2022-02-14T13:52:45Z

@andreaskaris: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-e2e-gcp-ovn	`1813ea5`	link	false	`/test okd-e2e-gcp-ovn`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci · 2022-02-14T15:51:19Z

@andreaskaris: This pull request references Bugzilla bug 2014003, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 1987445 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 1987445 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0
bug has dependents

Requesting review from QA contact:
/cc @anuragthehatter

Details

In response to this:

[release-4.9] Bug 2014003: Fix gateway routers answer ARP/NDP requests for LoadBalancer/ExternalIP services

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2022-02-14T15:53:41Z

@andreaskaris: This pull request references Bugzilla bug 2014003, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 1987445 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 1987445 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0
bug has dependents

Requesting review from QA contact:
/cc @anuragthehatter

Details

In response to this:

[release-4.9] Bug 2014003: Fix gateway routers answer ARP/NDP requests for LoadBalancer/ExternalIP services

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tssurya · 2022-02-14T21:22:38Z

/lgtm
I compared the conflicted commit (2nd one) with the original one: 91d37a6 and did not find any variations.

tssurya · 2022-02-14T21:23:00Z

/cc @oribon

trozet · 2022-02-17T18:32:35Z

/approve
/label backport-risk-assessed

openshift-ci · 2022-02-17T18:33:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreaskaris, trozet, tssurya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [trozet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2022-02-17T18:46:01Z

/retest-required