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

@@ -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

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()