From 4a53633a1d84a7c6ec5fadcffa9a1b74281d0722 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 6 Apr 2021 16:33:47 -0700 Subject: [PATCH] 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 | 58 +++++++++++++-------- client/allocrunner/networking_cni_test.go | 63 +++++++++++++++++++++++ 2 files changed, 100 insertions(+), 21 deletions(-) create mode 100644 client/allocrunner/networking_cni_test.go diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index 746fd85ed..ce3a994d5 100644 --- a/client/allocrunner/networking_cni.go +++ b/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 != "" { + 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.Sandbox != "" && len(iface.IPConfigs) > 0 { + netStatus.Address = iface.IPConfigs[0].IP.String() + netStatus.InterfaceName = name break } } - 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 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) { diff --git a/client/allocrunner/networking_cni_test.go b/client/allocrunner/networking_cni_test.go new file mode 100644 index 000000000..84a02f404 --- /dev/null +++ b/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) +}