From 966e19fe50c63f570cd75e9fd3c8f869e1c2ceb2 Mon Sep 17 00:00:00 2001 From: Nick Ethier Date: Tue, 26 Jan 2021 10:49:29 -0500 Subject: [PATCH 1/4] ar: try to find CNI addr if not returned with interface --- client/allocrunner/networking_cni.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index 6306882dc..a6412f509 100644 --- a/client/allocrunner/networking_cni.go +++ b/client/allocrunner/networking_cni.go @@ -6,6 +6,7 @@ package allocrunner import ( "context" + "encoding/json" "fmt" "math/rand" "os" @@ -111,6 +112,11 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc break } + if c.logger.IsDebug() { + resultJSON, _ := json.Marshal(res) + c.logger.Debug("received result from CNI", "result", resultJSON) + } + netStatus := new(structs.AllocNetworkStatus) if len(res.Interfaces) > 0 { @@ -134,6 +140,18 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc netStatus.Address = iface.IPConfigs[0].IP.String() } } + + if netStatus.Address == "" { + c.logger.Debug("no address found for sandboxed interface from CNI result, using first available") + for _, iface := range res.Interfaces { + if len(iface.IPConfigs) > 0 { + netStatus.Address = iface.IPConfigs[0].IP.String() + break + } + } + c.logger.Warn("no address could be found from CNI result", "allocID", alloc.ID) + } + if len(res.DNS) > 0 { netStatus.DNS = &structs.DNSConfig{ Servers: res.DNS[0].Nameservers, From 6e8419c7d34bad4decbf292d58f90d9ed5b3c6ce Mon Sep 17 00:00:00 2001 From: Nick Ethier Date: Tue, 26 Jan 2021 11:58:52 -0500 Subject: [PATCH 2/4] ar: only log warning if no addr in found --- client/allocrunner/networking_cni.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index a6412f509..8739d362c 100644 --- a/client/allocrunner/networking_cni.go +++ b/client/allocrunner/networking_cni.go @@ -143,13 +143,17 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc 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 { if len(iface.IPConfigs) > 0 { netStatus.Address = iface.IPConfigs[0].IP.String() + found = true break } } - c.logger.Warn("no address could be found from CNI result", "allocID", alloc.ID) + if !found { + c.logger.Warn("no address could be found from CNI result", "allocID", alloc.ID) + } } if len(res.DNS) > 0 { From 5aed5b7cd48f19fdef9d2980bdef130cb3669bcc Mon Sep 17 00:00:00 2001 From: Nick Ethier Date: Mon, 5 Apr 2021 12:35:34 -0400 Subject: [PATCH 3/4] ar: stringify CNI result debug message --- client/allocrunner/networking_cni.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index 8739d362c..746fd85ed 100644 --- a/client/allocrunner/networking_cni.go +++ b/client/allocrunner/networking_cni.go @@ -114,7 +114,7 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc if c.logger.IsDebug() { resultJSON, _ := json.Marshal(res) - c.logger.Debug("received result from CNI", "result", resultJSON) + c.logger.Debug("received result from CNI", "result", string(resultJSON)) } netStatus := new(structs.AllocNetworkStatus) From 4a53633a1d84a7c6ec5fadcffa9a1b74281d0722 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 6 Apr 2021 16:33:47 -0700 Subject: [PATCH 4/4] 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) +}