diff --git a/.changelog/17766.txt b/.changelog/17766.txt new file mode 100644 index 000000000..7e722049b --- /dev/null +++ b/.changelog/17766.txt @@ -0,0 +1,3 @@ +```release-note:improvement +cni: Ensure to setup CNI addresses in deterministic order +``` diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index cec8330f7..5ab1c2b46 100644 --- a/client/allocrunner/networking_cni.go +++ b/client/allocrunner/networking_cni.go @@ -130,46 +130,52 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc // 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) { + if len(res.Interfaces) == 0 { + return nil, fmt.Errorf("failed to configure network: no interfaces found") + } + netStatus := new(structs.AllocNetworkStatus) - // Use the first sandbox interface with an IP address - if len(res.Interfaces) > 0 { - 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) - } + // Unfortunately the go-cni library returns interfaces in an unordered map meaning + // the results may be nondeterministic depending on CNI plugin output so make + // sure we sort them by interface name. + names := make([]string, 0, len(res.Interfaces)) + for k := range res.Interfaces { + names = append(names, k) + } + sort.Strings(names) - if iface.Sandbox != "" && len(iface.IPConfigs) > 0 { - netStatus.Address = iface.IPConfigs[0].IP.String() - netStatus.InterfaceName = name - break - } + // Use the first sandbox interface with an IP address + for _, name := range names { + iface := res.Interfaces[name] + 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) + continue + } + + 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 == "" { - var found bool - for name, iface := range res.Interfaces { + for _, name := range names { + iface := res.Interfaces[name] if len(iface.IPConfigs) > 0 { 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") - } } // If no IP address could be found, return an error diff --git a/client/allocrunner/networking_cni_test.go b/client/allocrunner/networking_cni_test.go index d50f8c839..f116a28ec 100644 --- a/client/allocrunner/networking_cni_test.go +++ b/client/allocrunner/networking_cni_test.go @@ -127,6 +127,22 @@ func TestCNI_forceCleanup(t *testing.T) { }) } +// TestCNI_cniToAllocNet_NoInterfaces asserts an error is returned if CNIResult +// contains no interfaces. +func TestCNI_cniToAllocNet_NoInterfaces(t *testing.T) { + ci.Parallel(t) + + cniResult := &cni.CNIResult{} + + // Only need a logger + c := &cniNetworkConfigurator{ + logger: testlog.HCLogger(t), + } + allocNet, err := c.cniToAllocNet(cniResult) + require.Error(t, err) + require.Nil(t, allocNet) +} + // 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) {