Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NO-ISSUE: OCPBUGS-43352 calculate outgoing interface for arping #877

Merged
merged 1 commit into from
Jan 7, 2025
Merged

NO-ISSUE: OCPBUGS-43352 calculate outgoing interface for arping #877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/connectivity_check/connectivity_check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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,
}}
}
109 changes: 64 additions & 45 deletions src/connectivity_check/ipv4_arping_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package connectivity_check

import (
"net"
"os/exec"
"regexp"
"sort"
Expand All @@ -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")
eifrach marked this conversation as resolved.
Show resolved Hide resolved
} 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) {
Expand Down
18 changes: 14 additions & 4 deletions src/connectivity_check/ipv4_arping_checker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)},
eifrach marked this conversation as resolved.
Show resolved Hide resolved
RemoteMACAddresses: remoteMACAddresses,
}
mockEmptyReply(remoteIPAddress)
Expand All @@ -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))
})
})