ar: refactor go-cni results processing & add test

The goal is to always find an interface with an address, preferring
sandbox interfaces, but falling back to the first address found.

A test was added against a known CNI plugin output that was not handled
correctly before.

client/allocrunner/networking_cni.go

@@ -117,45 +117,62 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc
 		c.logger.Debug("received result from CNI", "result", string(resultJSON))
 	}
 
+	return c.cniToAllocNet(res)
+
+}
+
+// cniToAllocNet converts a CNIResult to an AllocNetworkStatus or returns an
+// error. The first interface and IP with a sandbox and address set are
+// preferred. Failing that the first interface with an IP is selected.
+//
+// Unfortunately the go-cni library returns interfaces in an unordered map so
+// the results may be nondeterministic depending on CNI plugin output.
+func (c *cniNetworkConfigurator) cniToAllocNet(res *cni.CNIResult) (*structs.AllocNetworkStatus, error) {
 	netStatus := new(structs.AllocNetworkStatus)
 
+	// Use the first sandbox interface with an IP address
 	if len(res.Interfaces) > 0 {
-		// find an interface with Sandbox set, or any one of them if no
-		// interface has it set
-		var iface *cni.Config
-		var name string
-		for name, iface = range res.Interfaces {
-			if iface != nil && iface.Sandbox != "" {
-				break
+		for name, iface := range res.Interfaces {
+			if iface == nil {
+				// this should never happen but this value is coming from external
+				// plugins so we should guard against it
+				delete(res.Interfaces, name)
 			}
-		}
-		if iface == nil {
-			// this should never happen but this value is coming from external
-			// plugins so we should guard against it
-			return nil, fmt.Errorf("failed to configure network: no valid interface")
-		}
 
-		netStatus.InterfaceName = name
-		if len(iface.IPConfigs) > 0 {
-			netStatus.Address = iface.IPConfigs[0].IP.String()
+			if iface.Sandbox != "" && len(iface.IPConfigs) > 0 {
+				netStatus.Address = iface.IPConfigs[0].IP.String()
+				netStatus.InterfaceName = name
+				break
+			}
 		}
 	}
 
+	// If no IP address was found, use the first interface with an address
+	// found as a fallback
 	if netStatus.Address == "" {
-		c.logger.Debug("no address found for sandboxed interface from CNI result, using first available")
 		var found bool
-		for _, iface := range res.Interfaces {
+		for name, iface := range res.Interfaces {
 			if len(iface.IPConfigs) > 0 {
-				netStatus.Address = iface.IPConfigs[0].IP.String()
+				ip := iface.IPConfigs[0].IP.String()
+				c.logger.Debug("no sandbox interface with an address found CNI result, using first available", "interface", name, "ip", ip)
+				netStatus.Address = ip
+				netStatus.InterfaceName = name
 				found = true
 				break
 			}
 		}
 		if !found {
-			c.logger.Warn("no address could be found from CNI result", "allocID", alloc.ID)
+			c.logger.Warn("no address could be found from CNI result")
 		}
 	}
 
+	// If no IP address could be found, return an error
+	if netStatus.Address == "" {
+		return nil, fmt.Errorf("failed to configure network: no interface with an address")
+
+	}
+
+	// Use the first DNS results.
 	if len(res.DNS) > 0 {
 		netStatus.DNS = &structs.DNSConfig{
 			Servers:  res.DNS[0].Nameservers,
@@ -165,7 +182,6 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc
 	}
 
 	return netStatus, nil
-
 }
 
 func loadCNIConf(confDir, name string) ([]byte, error) {

client/allocrunner/networking_cni_test.go

@@ -0,0 +1,63 @@
+package allocrunner
+
+import (
+	"net"
+	"testing"
+
+	cni "github.com/containerd/go-cni"
+	"github.com/hashicorp/nomad/helper/testlog"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestCNI_cniToAllocNet_Fallback asserts if a CNI plugin result lacks an IP on
+// its sandbox interface, the first IP found is used.
+func TestCNI_cniToAllocNet_Fallback(t *testing.T) {
+	// Calico's CNI plugin v3.12.3 has been observed to return the
+	// following:
+	cniResult := &cni.CNIResult{
+		Interfaces: map[string]*cni.Config{
+			"cali39179aa3-74": &cni.Config{},
+			"eth0": &cni.Config{
+				IPConfigs: []*cni.IPConfig{
+					&cni.IPConfig{
+						IP: net.IPv4(192, 168, 135, 232),
+					},
+				},
+			},
+		},
+	}
+
+	// Only need a logger
+	c := &cniNetworkConfigurator{
+		logger: testlog.HCLogger(t),
+	}
+	allocNet, err := c.cniToAllocNet(cniResult)
+	require.NoError(t, err)
+	require.NotNil(t, allocNet)
+	assert.Equal(t, "192.168.135.232", allocNet.Address)
+	assert.Equal(t, "eth0", allocNet.InterfaceName)
+	assert.Nil(t, allocNet.DNS)
+}
+
+// TestCNI_cniToAllocNet_Invalid asserts an error is returned if a CNI plugin
+// result lacks any IP addresses. This has not been observed, but Nomad still
+// must guard against invalid results from external plugins.
+func TestCNI_cniToAllocNet_Invalid(t *testing.T) {
+	cniResult := &cni.CNIResult{
+		Interfaces: map[string]*cni.Config{
+			"eth0": &cni.Config{},
+			"veth1": &cni.Config{
+				IPConfigs: []*cni.IPConfig{},
+			},
+		},
+	}
+
+	// Only need a logger
+	c := &cniNetworkConfigurator{
+		logger: testlog.HCLogger(t),
+	}
+	allocNet, err := c.cniToAllocNet(cniResult)
+	require.Error(t, err)
+	require.Nil(t, allocNet)
+}