From 37759393032cdb0c4bbf1d05fcd2eb2f856c3d26 Mon Sep 17 00:00:00 2001 From: Bjorn Neergaard Date: Sat, 21 Jan 2023 16:04:20 -0700 Subject: [PATCH 1/3] libnetwork/netutils: refactor GenerateRandomName GenerateRandomName now uses length to represent the overall length of the string; this will help future users avoid creating interface names that are too long for the kernel to accept by mistake. The test coverage is increased and cleaned up using gotest.tools. Signed-off-by: Bjorn Neergaard --- libnetwork/drivers/bridge/bridge.go | 2 +- libnetwork/drivers/ipvlan/ipvlan.go | 2 +- libnetwork/drivers/macvlan/macvlan.go | 2 +- libnetwork/drivers/overlay/overlay.go | 2 +- libnetwork/netutils/utils.go | 19 ++++++--- libnetwork/netutils/utils_linux_test.go | 55 +++++++++++++++++++------ 6 files changed, 59 insertions(+), 23 deletions(-) diff --git a/libnetwork/drivers/bridge/bridge.go b/libnetwork/drivers/bridge/bridge.go index 7332cdf24b..2ef2040dc1 100644 --- a/libnetwork/drivers/bridge/bridge.go +++ b/libnetwork/drivers/bridge/bridge.go @@ -30,7 +30,7 @@ import ( const ( networkType = "bridge" vethPrefix = "veth" - vethLen = 7 + vethLen = len(vethPrefix) + 7 defaultContainerVethPrefix = "eth" maxAllocatePortAttempts = 10 ) diff --git a/libnetwork/drivers/ipvlan/ipvlan.go b/libnetwork/drivers/ipvlan/ipvlan.go index c0e3964c2d..89091b9cba 100644 --- a/libnetwork/drivers/ipvlan/ipvlan.go +++ b/libnetwork/drivers/ipvlan/ipvlan.go @@ -14,9 +14,9 @@ import ( ) const ( - vethLen = 7 containerVethPrefix = "eth" vethPrefix = "veth" + vethLen = len(vethPrefix) + 7 driverName = "ipvlan" // driver type name parentOpt = "parent" // parent interface -o parent diff --git a/libnetwork/drivers/macvlan/macvlan.go b/libnetwork/drivers/macvlan/macvlan.go index d455affa56..eae5bb8176 100644 --- a/libnetwork/drivers/macvlan/macvlan.go +++ b/libnetwork/drivers/macvlan/macvlan.go @@ -14,9 +14,9 @@ import ( ) const ( - vethLen = 7 containerVethPrefix = "eth" vethPrefix = "veth" + vethLen = len(vethPrefix) + 7 driverName = "macvlan" // driver type name modePrivate = "private" // macvlan mode private modeVepa = "vepa" // macvlan mode vepa diff --git a/libnetwork/drivers/overlay/overlay.go b/libnetwork/drivers/overlay/overlay.go index 07084317b7..5daf05e0b3 100644 --- a/libnetwork/drivers/overlay/overlay.go +++ b/libnetwork/drivers/overlay/overlay.go @@ -25,7 +25,7 @@ import ( const ( networkType = "overlay" vethPrefix = "veth" - vethLen = 7 + vethLen = len(vethPrefix) + 7 vxlanIDStart = 256 vxlanIDEnd = (1 << 24) - 1 vxlanEncap = 50 diff --git a/libnetwork/netutils/utils.go b/libnetwork/netutils/utils.go index 76b2478cb7..49f3c39611 100644 --- a/libnetwork/netutils/utils.go +++ b/libnetwork/netutils/utils.go @@ -121,14 +121,21 @@ func GenerateMACFromIP(ip net.IP) net.HardwareAddr { return genMAC(ip) } -// GenerateRandomName returns a new name joined with a prefix. This size -// specified is used to truncate the randomly generated value -func GenerateRandomName(prefix string, size int) (string, error) { - id := make([]byte, 32) - if _, err := io.ReadFull(rand.Reader, id); err != nil { +// GenerateRandomName returns a string of the specified length, created by joining the prefix to random hex characters. +// The length must be strictly larger than len(prefix), or an error will be returned. +func GenerateRandomName(prefix string, length int) (string, error) { + if length <= len(prefix) { + return "", fmt.Errorf("invalid length %d for prefix %s", length, prefix) + } + + // We add 1 here as integer division will round down, and we want to round up. + b := make([]byte, (length-len(prefix)+1)/2) + if _, err := io.ReadFull(rand.Reader, b); err != nil { return "", err } - return prefix + hex.EncodeToString(id)[:size], nil + + // By taking a slice here, we ensure that the string is always the correct length. + return (prefix + hex.EncodeToString(b))[:length], nil } // ReverseIP accepts a V4 or V6 IP string in the canonical form and returns a reversed IP in diff --git a/libnetwork/netutils/utils_linux_test.go b/libnetwork/netutils/utils_linux_test.go index ae9c42bc8a..34f1ae6779 100644 --- a/libnetwork/netutils/utils_linux_test.go +++ b/libnetwork/netutils/utils_linux_test.go @@ -2,14 +2,18 @@ package netutils import ( "bytes" + "fmt" "net" "sort" + "strings" "testing" "github.com/docker/docker/libnetwork/ipamutils" "github.com/docker/docker/libnetwork/testutils" "github.com/docker/docker/libnetwork/types" "github.com/vishvananda/netlink" + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) func TestNonOverlappingNameservers(t *testing.T) { @@ -186,21 +190,46 @@ func TestNetworkRange(t *testing.T) { // Test veth name generation "veth"+rand (e.g.veth0f60e2c) func TestGenerateRandomName(t *testing.T) { - name1, err := GenerateRandomName("veth", 7) - if err != nil { - t.Fatal(err) + const vethPrefix = "veth" + const vethLen = len(vethPrefix) + 7 + + testCases := []struct { + prefix string + length int + error bool + }{ + {vethPrefix, -1, true}, + {vethPrefix, 0, true}, + {vethPrefix, len(vethPrefix) - 1, true}, + {vethPrefix, len(vethPrefix), true}, + {vethPrefix, len(vethPrefix) + 1, false}, + {vethPrefix, 255, false}, } - // veth plus generated append equals a len of 11 - if len(name1) != 11 { - t.Fatalf("Expected 11 characters, instead received %d characters", len(name1)) + for _, tc := range testCases { + t.Run(fmt.Sprintf("prefix=%s/length=%d", tc.prefix, tc.length), func(t *testing.T) { + name, err := GenerateRandomName(tc.prefix, tc.length) + if tc.error { + assert.Check(t, is.ErrorContains(err, "invalid length")) + } else { + assert.NilError(t, err) + assert.Check(t, strings.HasPrefix(name, tc.prefix), "Expected name to start with %s", tc.prefix) + assert.Check(t, is.Equal(len(name), tc.length), "Expected %d characters, instead received %d characters", tc.length, len(name)) + } + }) } - name2, err := GenerateRandomName("veth", 7) - if err != nil { - t.Fatal(err) - } - // Fail if the random generated names equal one another - if name1 == name2 { - t.Fatalf("Expected differing values but received %s and %s", name1, name2) + + var randomNames [16]string + for i := range randomNames { + randomName, err := GenerateRandomName(vethPrefix, vethLen) + assert.NilError(t, err) + + for _, oldName := range randomNames { + if randomName == oldName { + t.Fatalf("Duplicate random name generated: %s", randomName) + } + } + + randomNames[i] = randomName } } From b3e6aa9316188fc539e3be9ee8be37acf88db92c Mon Sep 17 00:00:00 2001 From: Bjorn Neergaard Date: Sat, 21 Jan 2023 16:23:07 -0700 Subject: [PATCH 2/3] libnetwork/netutils: clean up GenerateIfaceName netlink offers the netlink.LinkNotFoundError type, which we can use with errors.As() to detect a unused link name. Additionally, early return if GenerateRandomName fails, as reading random bytes should be a highly reliable operation, and otherwise the error would be swallowed by the fall-through return. Signed-off-by: Bjorn Neergaard --- libnetwork/netutils/utils_linux.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libnetwork/netutils/utils_linux.go b/libnetwork/netutils/utils_linux.go index 89aec1ac2a..761cd433a7 100644 --- a/libnetwork/netutils/utils_linux.go +++ b/libnetwork/netutils/utils_linux.go @@ -8,7 +8,6 @@ package netutils import ( "net" "os" - "strings" "github.com/docker/docker/libnetwork/ipamutils" "github.com/docker/docker/libnetwork/ns" @@ -49,11 +48,11 @@ func GenerateIfaceName(nlh *netlink.Handle, prefix string, len int) (string, err for i := 0; i < 3; i++ { name, err := GenerateRandomName(prefix, len) if err != nil { - continue + return "", err } _, err = linkByName(name) if err != nil { - if strings.Contains(err.Error(), "not found") { + if errors.As(err, &netlink.LinkNotFoundError{}) { return name, nil } return "", err From 390532cbc677b2796c8aeeff74b1a17a3ba97c31 Mon Sep 17 00:00:00 2001 From: Bjorn Neergaard Date: Sat, 21 Jan 2023 16:25:14 -0700 Subject: [PATCH 3/3] libnetwork/windows/overlay: drop unused variables These package-level variables were copied over from the Linux implementation; drop them for clarity's sake. Signed-off-by: Bjorn Neergaard --- libnetwork/drivers/windows/overlay/overlay_windows.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/libnetwork/drivers/windows/overlay/overlay_windows.go b/libnetwork/drivers/windows/overlay/overlay_windows.go index d31370246c..727a3f1db6 100644 --- a/libnetwork/drivers/windows/overlay/overlay_windows.go +++ b/libnetwork/drivers/windows/overlay/overlay_windows.go @@ -17,10 +17,7 @@ import ( ) const ( - networkType = "overlay" - vethPrefix = "veth" - vethLen = 7 - secureOption = "encrypted" + networkType = "overlay" ) type driver struct {