Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
68 changes: 54 additions & 14 deletions agent/registration/host_registrar.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,58 @@ func GetOSFamily(lookPath func(string) (string, error)) string {
return ""
}

// netInterface decouples GetNetworkStatus from net.Interface so tests can
// supply fake interfaces without a real OS routing table.
type netInterface struct {
Name string
Flags net.Flags
MACAddr string
Addrs []net.Addr
}

func systemInterfaces() ([]netInterface, error) {
ifaces, err := net.Interfaces()
if err != nil {
return nil, err
}
result := make([]netInterface, 0, len(ifaces))
for _, iface := range ifaces {
addrs, err := iface.Addrs()
if err != nil {
continue
}
result = append(result, netInterface{
Name: iface.Name,
Flags: iface.Flags,
MACAddr: iface.HardwareAddr.String(),
Addrs: addrs,
})
}
return result, nil
}

// GetNetworkStatus returns the network interface(s) status for the host
func (hr *HostRegistrar) GetNetworkStatus() []infrastructurev1beta1.NetworkStatus {
return hr.getNetworkStatus(gateway.DiscoverInterface, gateway.DiscoverInterfaceIPv6, systemInterfaces)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like an anti pattern. I think GetNetowrkStatus should accept an interface (Go, not network interface). And the implementations of the interface are something like:

  • RealNet (the existing methods that are being passed here instead)
  • FakeNet (the fake methods in the test file)

}

func (hr *HostRegistrar) getNetworkStatus(
discoverIPv4 func() (net.IP, error),
discoverIPv6 func() (net.IP, error),
interfaces func() ([]netInterface, error),
) []infrastructurev1beta1.NetworkStatus {

Network := make([]infrastructurev1beta1.NetworkStatus, 0)

defaultIP, err := gateway.DiscoverInterface()
if err != nil {
return Network
var defaultIPs []net.IP
if ip, err := discoverIPv4(); err == nil {
defaultIPs = append(defaultIPs, ip)
}
if ip, err := discoverIPv6(); err == nil {
defaultIPs = append(defaultIPs, ip)
}

ifaces, err := net.Interfaces()
ifaces, err := interfaces()
if err != nil {
return Network
}
Expand All @@ -139,24 +181,22 @@ func (hr *HostRegistrar) GetNetworkStatus() []infrastructurev1beta1.NetworkStatu
netStatus.Connected = true
}

netStatus.MACAddr = iface.HardwareAddr.String()
addrs, err := iface.Addrs()
if err != nil {
continue
}

netStatus.MACAddr = iface.MACAddr
netStatus.NetworkInterfaceName = iface.Name
for _, addr := range addrs {
for _, addr := range iface.Addrs {
var ip net.IP
switch v := addr.(type) {
case *net.IPNet:
ip = v.IP
case *net.IPAddr:
ip = v.IP
}
if ip.String() == defaultIP.String() {
netStatus.IsDefault = true
hr.ByoHostInfo.DefaultNetworkInterfaceName = netStatus.NetworkInterfaceName
for _, defaultIP := range defaultIPs {
if ip.Equal(defaultIP) {
netStatus.IsDefault = true
hr.ByoHostInfo.DefaultNetworkInterfaceName = netStatus.NetworkInterfaceName
break
}
}
netStatus.IPAddrs = append(netStatus.IPAddrs, addr.String())
}
Expand Down
85 changes: 85 additions & 0 deletions agent/registration/host_registrar_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package registration

import (
"fmt"
"net"
"os"

. "github.com/onsi/ginkgo/v2"
Expand All @@ -14,6 +15,26 @@ import (
infrastructurev1beta1 "github.com/vmware-tanzu/cluster-api-provider-bringyourownhost/apis/infrastructure/v1beta1"
)

func noDefaultRoute() (net.IP, error) {
return nil, fmt.Errorf("no default route")
}

func fixedDefaultRoute(ip string) func() (net.IP, error) {
return func() (net.IP, error) { return net.ParseIP(ip), nil }
}

func fakeInterfaces(ifaces []netInterface, err error) func() ([]netInterface, error) {
return func() ([]netInterface, error) { return ifaces, err }
}

func addrOf(ip string) net.Addr {
parsed := net.ParseIP(ip)
if parsed.To4() != nil {
return &net.IPNet{IP: parsed, Mask: net.CIDRMask(24, 32)}
}
return &net.IPNet{IP: parsed, Mask: net.CIDRMask(64, 128)}
}

func getMockFile(targetOs string) ([]byte, error) {
out := fmt.Sprintf(`NAME="Ubuntu"
VERSION="20.04.4 LTS (Focal Fossa)"
Expand Down Expand Up @@ -107,6 +128,70 @@ var _ = Describe("Host Registrar Tests", func() {
})
})

Context("When computing network status", func() {
It("Should mark the default interface via an IPv4 default route", func() {
hr := HostRegistrar{}
ifaces := []netInterface{
{Name: "eth0", Flags: net.FlagUp, MACAddr: "aa:bb:cc:dd:ee:ff", Addrs: []net.Addr{addrOf("192.168.1.10")}},
}
status := hr.getNetworkStatus(fixedDefaultRoute("192.168.1.10"), noDefaultRoute, fakeInterfaces(ifaces, nil))

Expect(status).To(HaveLen(1))
Expect(status[0].IsDefault).To(BeTrue())
Expect(hr.ByoHostInfo.DefaultNetworkInterfaceName).To(Equal("eth0"))
})

It("Should mark the default interface via an IPv6-only default route", func() {
hr := HostRegistrar{}
ifaces := []netInterface{
{Name: "eth0", Flags: net.FlagUp, MACAddr: "aa:bb:cc:dd:ee:ff", Addrs: []net.Addr{addrOf("2001:db8::10")}},
}
status := hr.getNetworkStatus(noDefaultRoute, fixedDefaultRoute("2001:db8::10"), fakeInterfaces(ifaces, nil))

Expect(status).To(HaveLen(1))
Expect(status[0].IsDefault).To(BeTrue())
Expect(hr.ByoHostInfo.DefaultNetworkInterfaceName).To(Equal("eth0"))
})

It("Should mark the default interface on a dual-stack host via either family's route", func() {
hr := HostRegistrar{}
ifaces := []netInterface{
{
Name: "eth0",
Flags: net.FlagUp,
MACAddr: "aa:bb:cc:dd:ee:ff",
Addrs: []net.Addr{addrOf("192.168.1.10"), addrOf("2001:db8::10")},
},
}
status := hr.getNetworkStatus(fixedDefaultRoute("192.168.1.10"), fixedDefaultRoute("2001:db8::10"), fakeInterfaces(ifaces, nil))

Expect(status).To(HaveLen(1))
Expect(status[0].IsDefault).To(BeTrue())
Expect(status[0].IPAddrs).To(ConsistOf("192.168.1.10/24", "2001:db8::10/64"))
Expect(hr.ByoHostInfo.DefaultNetworkInterfaceName).To(Equal("eth0"))
})

It("Should still report interfaces when neither family has a default route", func() {
hr := HostRegistrar{}
ifaces := []netInterface{
{Name: "eth0", Flags: net.FlagUp, MACAddr: "aa:bb:cc:dd:ee:ff", Addrs: []net.Addr{addrOf("2001:db8::10")}},
}
status := hr.getNetworkStatus(noDefaultRoute, noDefaultRoute, fakeInterfaces(ifaces, nil))

Expect(status).To(HaveLen(1))
Expect(status[0].IsDefault).To(BeFalse())
Expect(status[0].IPAddrs).To(ConsistOf("2001:db8::10/64"))
Expect(hr.ByoHostInfo.DefaultNetworkInterfaceName).To(BeEmpty())
})

It("Should return an empty status when interface enumeration itself fails", func() {
hr := HostRegistrar{}
status := hr.getNetworkStatus(fixedDefaultRoute("192.168.1.10"), noDefaultRoute, fakeInterfaces(nil, fmt.Errorf("boom")))

Expect(status).To(BeEmpty())
})
})

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please rewrite this as table driven non-ginkgo unit tests? I find this format time consuming in terms of evaluating all the test cases.


Context("When the OS family is detected", func() {
It("Should return debian when dpkg is on PATH", func() {
family := GetOSFamily(func(file string) (string, error) {
Expand Down
Loading