OCPBUGS-43352: calculate outgoing interface for arping

openshift · Jan 6, 2025 · 258fdb0 · 258fdb0
1 parent 8d7bc11
commit 258fdb0
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 50 deletions.
diff --git a/src/connectivity_check/connectivity_check_test.go b/src/connectivity_check/connectivity_check_test.go
@@ -218,7 +218,7 @@ var _ = Describe("check host parallel validation", func() {
 		It(t.name, func() {
 			d := setupDispather(t.simulateL2IPConflict, t.success, t.hosts.Nics)
 			ret, err := d.Run(models.ConnectivityCheckParams{t.hosts}, funk.Map(t.nics, func(s string) OutgoingNic {
-				return OutgoingNic{Name: s, HasIpv4Addresses: true, HasIpv6Addresses: true}
+				return OutgoingNic{Name: s, HasIpv4Addresses: true, HasIpv6Addresses: true, Addresses: getAddr("192.168.1.133", 24)}
 			}).([]OutgoingNic))
 			Expect(err).ToNot(HaveOccurred())
 			Expect(ret.RemoteHosts).To(HaveLen(1))
@@ -346,3 +346,13 @@ func TestConnectivityCheck(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "connectivity check tests")
 }
+
+func getAddr(addr string, mask int) []net.Addr {
+	localIP := net.ParseIP(addr)
+	localMask := net.CIDRMask(mask, 32)
+
+	return []net.Addr{&net.IPNet{
+		IP:   localIP,
+		Mask: localMask,
+	}}
+}
diff --git a/src/connectivity_check/ipv4_arping_checker.go b/src/connectivity_check/ipv4_arping_checker.go
@@ -1,6 +1,7 @@
 package connectivity_check
 
 import (
+	"net"
 	"os/exec"
 	"regexp"
 	"sort"
@@ -24,61 +25,79 @@ func (p *arpingChecker) Check(attributes Attributes) ResultReporter {
 		return nil
 	}
 
-	result, err := p.executer.Execute("arping", "-c", "10", "-w", "5", "-I", attributes.OutgoingNIC.Name, attributes.RemoteIPAddress)
-	if err != nil {
-		// Ignore exit code of 1; only 2 or -1 are actual errors
-		if exitErr, ok := err.(*exec.ExitError); !ok || exitErr.ExitCode() != 1 {
-			log.WithError(err).Error("Error while processing 'arping' command")
+	for _, addr := range attributes.OutgoingNIC.Addresses {
+		_, nicNetwork, err := net.ParseCIDR(addr.String())
+		if err != nil {
+			log.WithError(err).Errorf("Cannot Parse CIDR: %s", addr.String())
 			return nil
 		}
-	}
-	lines := strings.Split(result, "\n")
-	if len(lines) == 0 {
-		log.Warn("Missing output for arping")
-		return nil
-	}
 
-	hRegex := regexp.MustCompile("^ARPING ([^ ]+) from ([^ ]+) ([^ ]+)$")
-	parts := hRegex.FindStringSubmatch(lines[0])
-	if len(parts) != 4 {
-		log.Warnf("Wrong format for header line: %s", lines[0])
-		return nil
-	}
-
-	outgoingIpAddress := parts[2]
-	rRegexp := regexp.MustCompile(`^Unicast reply from ([^ ]+) \[([^]]+)\]  [^ ]+$`)
-
-	// We use a map to de-duplicate arping responses from the same mac. They are redundant
-	// because the reason we're reporting more than one line of arping in the first place is to allow
-	// the service to also detect devices in the network that have IP conflict with cluster hosts
-	returnValues := make(map[string]*models.L2Connectivity)
-	for _, line := range lines[1:] {
-		parts = rRegexp.FindStringSubmatch(line)
-		if len(parts) != 3 {
+		remoteIP := net.ParseIP(attributes.RemoteIPAddress)
+		if !nicNetwork.Contains(remoteIP) {
 			continue
 		}
-		remoteMAC := strings.ToLower(parts[2])
-		successful := macInDstMacs(remoteMAC, attributes.RemoteMACAddresses)
-		if !successful {
-			log.Warnf("Unexpected mac address for arping %s on nic %s: %s", attributes.RemoteIPAddress, attributes.OutgoingNIC.Name, remoteMAC)
+
+		result, err := p.executer.Execute("arping", "-c", "10", "-w", "5", "-I", attributes.OutgoingNIC.Name, attributes.RemoteIPAddress)
+		if err != nil {
+			if err.(*exec.ExitError).ExitCode() == 1 {
+				// The return code of 1 will be returned when there are some missing replies (possibly all of them)
+				// as we are looking for "duplicate" replies and are not concerned with missing replies, we may ignore this.
+				log.Warn("ARPPing exited with error code 1, ignoring")
+			} else {
+				log.WithError(err).Errorf("Error while processing 'arping' command. remote address: %v, Outgioing NIC: %v", attributes.RemoteIPAddress, attributes.OutgoingNIC.Name)
+				return nil
+			}
+		}
+		lines := strings.Split(result, "\n")
+		if len(lines) == 0 {
+			log.Warn("Missing output for arping")
+			return nil
 		}
-		if strings.ToLower(attributes.RemoteMACAddress) != remoteMAC {
-			log.Infof("Received remote mac %s different then expected mac %s", remoteMAC, attributes.RemoteMACAddress)
+
+		hRegex := regexp.MustCompile("^ARPING ([^ ]+) from ([^ ]+) ([^ ]+)$")
+		parts := hRegex.FindStringSubmatch(lines[0])
+		if len(parts) != 4 {
+			log.Warnf("Wrong format for header line: %s", lines[0])
+			return nil
 		}
 
-		returnValues[remoteMAC] = &models.L2Connectivity{
-			OutgoingIPAddress: outgoingIpAddress,
-			OutgoingNic:       attributes.OutgoingNIC.Name,
-			RemoteIPAddress:   attributes.RemoteIPAddress,
-			RemoteMac:         remoteMAC,
-			Successful:        successful,
+		outgoingIpAddress := parts[2]
+		rRegexp := regexp.MustCompile(`^Unicast reply from ([^ ]+) \[([^]]+)\]  [^ ]+$`)
+
+		// We use a map to de-duplicate arping responses from the same mac. They are redundant
+		// because the reason we're reporting more than one line of arping in the first place is to allow
+		// the service to also detect devices in the network that have IP conflict with cluster hosts
+		returnValues := make(map[string]*models.L2Connectivity)
+		for _, line := range lines[1:] {
+			parts = rRegexp.FindStringSubmatch(line)
+			if len(parts) != 3 {
+				continue
+			}
+			remoteMAC := strings.ToLower(parts[2])
+			successful := macInDstMacs(remoteMAC, attributes.RemoteMACAddresses)
+			if !successful {
+				log.Warnf("Unexpected mac address for arping %s on nic %s: %s", attributes.RemoteIPAddress, attributes.OutgoingNIC.Name, remoteMAC)
+			}
+			if strings.ToLower(attributes.RemoteMACAddress) != remoteMAC {
+				log.Infof("Received remote mac %s different then expected mac %s", remoteMAC, attributes.RemoteMACAddress)
+			}
+
+			returnValues[remoteMAC] = &models.L2Connectivity{
+				OutgoingIPAddress: outgoingIpAddress,
+				OutgoingNic:       attributes.OutgoingNIC.Name,
+				RemoteIPAddress:   attributes.RemoteIPAddress,
+				RemoteMac:         remoteMAC,
+				Successful:        successful,
+			}
 		}
+		var l2Connectivity []*models.L2Connectivity
+		for _, v := range returnValues {
+			l2Connectivity = append(l2Connectivity, v)
+		}
+		return newArpingResultReporter(l2Connectivity)
 	}
-	var l2Connectivity []*models.L2Connectivity
-	for _, v := range returnValues {
-		l2Connectivity = append(l2Connectivity, v)
-	}
-	return newArpingResultReporter(l2Connectivity)
+
+	return nil
 }
 
 func (p *arpingChecker) Finalize(resultingHost *models.ConnectivityRemoteHost) {

diff --git a/src/connectivity_check/ipv4_arping_checker_test.go b/src/connectivity_check/ipv4_arping_checker_test.go
@@ -55,7 +55,7 @@ Received 0 response(s)
 		attributes := Attributes{
 			RemoteIPAddress:    remoteIPAddress,
 			RemoteMACAddress:   remoteMACAddress,
-			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true},
+			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true, Addresses: getAddr(outgoingIPAddress, 24)},
 			RemoteMACAddresses: remoteMACAddresses,
 		}
 		mockFullReply(remoteIPAddress, remoteMACAddress)
@@ -84,7 +84,7 @@ Received 0 response(s)
 		attributes := Attributes{
 			RemoteIPAddress:    remoteIPAddress,
 			RemoteMACAddress:   remoteMACAddress,
-			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true},
+			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true, Addresses: getAddr(outgoingIPAddress, 24)},
 			RemoteMACAddresses: remoteMACAddresses,
 		}
 		mockFullReply(remoteIPAddress, secondaryMACAddress)
@@ -103,7 +103,7 @@ Received 0 response(s)
 		attributes := Attributes{
 			RemoteIPAddress:    remoteIPAddress,
 			RemoteMACAddress:   remoteMACAddress,
-			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true},
+			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true, Addresses: getAddr(outgoingIPAddress, 24)},
 			RemoteMACAddresses: remoteMACAddresses,
 		}
 		mockFullReply(remoteIPAddress, additionalMACAddress)
@@ -122,7 +122,7 @@ Received 0 response(s)
 		attributes := Attributes{
 			RemoteIPAddress:    remoteIPAddress,
 			RemoteMACAddress:   remoteMACAddress,
-			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true},
+			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true, Addresses: getAddr(outgoingIPAddress, 24)},
 			RemoteMACAddresses: remoteMACAddresses,
 		}
 		mockEmptyReply(remoteIPAddress)
@@ -132,4 +132,14 @@ Received 0 response(s)
 		Expect(reporter.Report(&resultingHost)).ToNot(HaveOccurred())
 		Expect(resultingHost.L2Connectivity).To(HaveLen(0))
 	})
+	It("outgoing interface in different subnet", func() {
+		attributes := Attributes{
+			RemoteIPAddress:    remoteIPAddress,
+			RemoteMACAddress:   remoteMACAddress,
+			OutgoingNIC:        OutgoingNic{Name: outgoingNIC, HasIpv4Addresses: true, Addresses: getAddr("2.2.3.4", 24)},
+			RemoteMACAddresses: remoteMACAddresses,
+		}
+		reporter := checker.Check(attributes)
+		Expect(reporter).To(BeNil(), fmt.Sprintf("report: %s", reporter))
+	})
 })