From 5b486a18492be048bfaf5cae9b559462b697186a Mon Sep 17 00:00:00 2001 From: Zachary Wasserman Date: Tue, 23 Apr 2019 15:58:45 -0700 Subject: [PATCH] Update logic for setting primary NIC (#2032) - The most active NIC will be picked even if a formerly more active interface still exists (previously, a NIC would stay primary as long as it existed). - Ignore link-local and loopback interfaces when choosing the primary. - Fix bugs in which update status of the primary interface could be reported incorrectly. Fixes #2020 --- server/kolide/hosts.go | 73 +++++++++---------- server/kolide/hosts_test.go | 138 ++++++++++++++++++++++++++++-------- 2 files changed, 147 insertions(+), 64 deletions(-) diff --git a/server/kolide/hosts.go b/server/kolide/hosts.go index 1e214de5d2..cb7952ad88 100644 --- a/server/kolide/hosts.go +++ b/server/kolide/hosts.go @@ -5,7 +5,6 @@ import ( "crypto/rand" "encoding/base64" "net" - "strings" "time" ) @@ -120,48 +119,50 @@ type HostSummary struct { NewCount uint `json:"new_count"` } -// ResetPrimaryNetwork will determine if the PrimaryNetworkInterfaceID -// needs to change. If it has not been set, it will default to the interface -// with the most IO. If it doesn't match an existing nic (as in the nic got changed) -// is will be reset. If there are not any nics, it will be set to nil. In any -// case if it changes, this function will return true, indicating that the -// change should be written back to the database +// ResetPrimaryNetwork determines the primary network interface by picking the +// first non-loopback/link-local interface in the network interfaces list. +// These networks should be ordered by I/O activity (before calling this +// function), so it will effectively pick the most active interface. If no +// interface exists, the ID will be set to nil. If only loopback or link-local +// interfaces exist, the most active of those will be set. The function returns +// a boolean indicating whether the primary interface was changed. func (h *Host) ResetPrimaryNetwork() bool { - if h.PrimaryNetworkInterfaceID != nil { - // This *may* happen if other details of the host are fetched before network_interfaces - if len(h.NetworkInterfaces) == 0 { - h.PrimaryNetworkInterfaceID = nil - return true - } - for _, nic := range h.NetworkInterfaces { - if *h.PrimaryNetworkInterfaceID == nic.ID { - return false - } - } + oldID := h.PrimaryNetworkInterfaceID + + h.resetPrimaryNetwork() + + if h.PrimaryNetworkInterfaceID != nil && oldID != nil { + return *h.PrimaryNetworkInterfaceID != *oldID + } else { + return h.PrimaryNetworkInterfaceID != oldID + } +} + +func (h *Host) resetPrimaryNetwork() { + if len(h.NetworkInterfaces) == 0 { h.PrimaryNetworkInterfaceID = nil + return } - // nics are in descending order of IO - // so we default to the most active nic - if len(h.NetworkInterfaces) > 0 { - // Check IPv4 address - for _, nic := range h.NetworkInterfaces { - if strings.Index(nic.IPAddress, "127.") == 0 { - continue - } - var isIpAddress = net.ParseIP(nic.IPAddress) - if isIpAddress.To4() != nil { - h.PrimaryNetworkInterfaceID = &nic.ID - return true - } + for _, nic := range h.NetworkInterfaces { + ip := net.ParseIP(nic.IPAddress) + if ip == nil { + continue } - // return IPv6 or other nic in place of IPv4 - h.PrimaryNetworkInterfaceID = &h.NetworkInterfaces[0].ID - return true + + // Skip link-local and loopback interfaces + if ip.IsLinkLocalUnicast() || ip.IsLoopback() { + continue + } + + // Choose first allowed interface + h.PrimaryNetworkInterfaceID = &nic.ID + return } - return false - + // If no interfaces qualify, still pick the first interface so that we + // show something. + h.PrimaryNetworkInterfaceID = &h.NetworkInterfaces[0].ID } // RandomText returns a stdEncoded string of diff --git a/server/kolide/hosts_test.go b/server/kolide/hosts_test.go index ea2647353d..bab4319e2c 100644 --- a/server/kolide/hosts_test.go +++ b/server/kolide/hosts_test.go @@ -8,42 +8,124 @@ import ( "github.com/stretchr/testify/assert" ) -func TestResetHosts(t *testing.T) { +func TestResetPrimaryNetworkNoInterfaces(t *testing.T) { host := Host{} result := host.ResetPrimaryNetwork() assert.False(t, result) + assert.Nil(t, host.PrimaryNetworkInterfaceID) +} - host.NetworkInterfaces = []*NetworkInterface{ - &NetworkInterface{ - ID: 1, - IPAddress: "192.168.1.2", - }, - &NetworkInterface{ - ID: 2, - IPAddress: "192.168.1.3", - }, - } - - result = host.ResetPrimaryNetwork() - assert.True(t, result) - assert.Equal(t, uint(1), *host.PrimaryNetworkInterfaceID) - - host.PrimaryNetworkInterfaceID = &host.NetworkInterfaces[1].ID - result = host.ResetPrimaryNetwork() - assert.False(t, result) - assert.Equal(t, uint(2), *host.PrimaryNetworkInterfaceID) - - host.NetworkInterfaces = host.NetworkInterfaces[:1] - result = host.ResetPrimaryNetwork() - assert.True(t, result) - assert.Equal(t, uint(1), *host.PrimaryNetworkInterfaceID) - - host.NetworkInterfaces = []*NetworkInterface{} - result = host.ResetPrimaryNetwork() +func TestResetPrimaryNetworkInterfaceRemoved(t *testing.T) { + // Start with interface set, but then all interfaces are removed + id := uint(1) + host := Host{PrimaryNetworkInterfaceID: &id} + result := host.ResetPrimaryNetwork() assert.True(t, result) assert.Nil(t, host.PrimaryNetworkInterfaceID) } +func TestResetPrimaryNetworkInterfaceNew(t *testing.T) { + host := Host{ + NetworkInterfaces: []*NetworkInterface{ + &NetworkInterface{ + ID: 1, + IPAddress: "192.168.1.2", + }, + &NetworkInterface{ + ID: 2, + IPAddress: "192.168.1.3", + }, + }, + } + result := host.ResetPrimaryNetwork() + assert.True(t, result) + assert.Equal(t, uint(1), *host.PrimaryNetworkInterfaceID) +} + +func TestResetPrimaryNetworkInterfaceUnchanged(t *testing.T) { + id := uint(1) + host := Host{ + NetworkInterfaces: []*NetworkInterface{ + &NetworkInterface{ + ID: 1, + IPAddress: "192.168.1.2", + }, + &NetworkInterface{ + ID: 2, + IPAddress: "192.168.1.3", + }, + }, + PrimaryNetworkInterfaceID: &id, + } + result := host.ResetPrimaryNetwork() + assert.False(t, result) + assert.Equal(t, uint(1), *host.PrimaryNetworkInterfaceID) +} + +func TestResetPrimaryNetworkInterfaceChanged(t *testing.T) { + id := uint(1) + host := Host{ + NetworkInterfaces: []*NetworkInterface{ + // 2 appears before 1 now (meaning more traffic to that + // interface) + &NetworkInterface{ + ID: 2, + IPAddress: "192.168.1.3", + }, + &NetworkInterface{ + ID: 1, + IPAddress: "192.168.1.2", + }, + }, + PrimaryNetworkInterfaceID: &id, + } + result := host.ResetPrimaryNetwork() + assert.True(t, result) + assert.Equal(t, uint(2), *host.PrimaryNetworkInterfaceID) +} + +func TestResetPrimaryNetworkLinkLocal(t *testing.T) { + id := uint(1) + host := Host{ + NetworkInterfaces: []*NetworkInterface{ + &NetworkInterface{ + ID: 1, + // Link-local IP + IPAddress: "169.254.10.12", + }, + &NetworkInterface{ + ID: 2, + IPAddress: "192.168.1.2", + }, + }, + PrimaryNetworkInterfaceID: &id, + } + result := host.ResetPrimaryNetwork() + assert.True(t, result) + assert.Equal(t, uint(2), *host.PrimaryNetworkInterfaceID) +} + +func TestResetPrimaryNetworkLoopback(t *testing.T) { + id := uint(1) + host := Host{ + NetworkInterfaces: []*NetworkInterface{ + &NetworkInterface{ + ID: 1, + // Loopback IP + IPAddress: "127.0.0.1", + }, + &NetworkInterface{ + ID: 2, + IPAddress: "192.168.1.2", + }, + }, + PrimaryNetworkInterfaceID: &id, + } + result := host.ResetPrimaryNetwork() + assert.True(t, result) + assert.Equal(t, uint(2), *host.PrimaryNetworkInterfaceID) +} + func TestHostStatus(t *testing.T) { mockClock := clock.NewMockClock()