client: improve errors & tests for dynamic ports

command/agent/command.go

@@ -30,6 +30,7 @@ import (
 	gatedwriter "github.com/hashicorp/nomad/helper/gated-writer"
 	"github.com/hashicorp/nomad/helper/logging"
 	"github.com/hashicorp/nomad/helper/winsvc"
+	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/nomad/structs/config"
 	"github.com/hashicorp/nomad/version"
 	"github.com/mitchellh/cli"
@@ -372,9 +373,16 @@ func (c *Command) isValidConfig(config, cmdConfig *Config) bool {
 		return false
 	}
 
-	if config.Client.MinDynamicPort < 0 || config.Client.MaxDynamicPort > 65535 ||
-		config.Client.MinDynamicPort >= config.Client.MaxDynamicPort {
-		c.Ui.Error("Invalid dynamic port range")
+	if config.Client.MinDynamicPort < 0 || config.Client.MinDynamicPort > structs.MaxValidPort {
+		c.Ui.Error(fmt.Sprintf("Invalid dynamic port range: min_dynamic_port=%d", config.Client.MinDynamicPort))
+		return false
+	}
+	if config.Client.MaxDynamicPort < 0 || config.Client.MaxDynamicPort > structs.MaxValidPort {
+		c.Ui.Error(fmt.Sprintf("Invalid dynamic port range: max_dynamic_port=%d", config.Client.MaxDynamicPort))
+		return false
+	}
+	if config.Client.MinDynamicPort > config.Client.MaxDynamicPort {
+		c.Ui.Error(fmt.Sprintf("Invalid dynamic port range: min_dynamic_port=%d and max_dynamic_port=%d", config.Client.MinDynamicPort, config.Client.MaxDynamicPort))
 		return false
 	}
 

command/agent/command_test.go

@@ -2,12 +2,15 @@ package agent
 
 import (
 	"io/ioutil"
+	"math"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/mitchellh/cli"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/hashicorp/nomad/version"
 )
@@ -241,3 +244,143 @@ func TestCommand_NullCharInRegion(t *testing.T) {
 		}
 	}
 }
+
+// TestIsValidConfig asserts that invalid configurations return false.
+func TestIsValidConfig(t *testing.T) {
+
+	cases := []struct {
+		name string
+		conf Config // merged into DefaultConfig()
+
+		// err should appear in error output; success expected if err
+		// is empty
+		err string
+	}{
+		{
+			name: "Default",
+			conf: Config{
+				DataDir: "/tmp",
+				Client:  &ClientConfig{Enabled: true},
+			},
+		},
+		{
+			name: "NoMode",
+			conf: Config{
+				Client: &ClientConfig{Enabled: false},
+				Server: &ServerConfig{Enabled: false},
+			},
+			err: "Must specify either",
+		},
+		{
+			name: "InvalidRegion",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled: true,
+				},
+				Region: "Hello\000World",
+			},
+			err: "Region contains",
+		},
+		{
+			name: "InvalidDatacenter",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled: true,
+				},
+				Datacenter: "Hello\000World",
+			},
+			err: "Datacenter contains",
+		},
+		{
+			name: "RelativeDir",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled: true,
+				},
+				DataDir: "foo/bar",
+			},
+			err: "must be given as an absolute",
+		},
+		{
+			name: "NegativeMinDynamicPort",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled:        true,
+					MinDynamicPort: -1,
+				},
+			},
+			err: "min_dynamic_port",
+		},
+		{
+			name: "NegativeMaxDynamicPort",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled:        true,
+					MaxDynamicPort: -1,
+				},
+			},
+			err: "max_dynamic_port",
+		},
+		{
+			name: "BigMinDynamicPort",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled:        true,
+					MinDynamicPort: math.MaxInt32,
+				},
+			},
+			err: "min_dynamic_port",
+		},
+		{
+			name: "BigMaxDynamicPort",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled:        true,
+					MaxDynamicPort: math.MaxInt32,
+				},
+			},
+			err: "max_dynamic_port",
+		},
+		{
+			name: "MinMaxDynamicPortSwitched",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled:        true,
+					MinDynamicPort: 5000,
+					MaxDynamicPort: 4000,
+				},
+			},
+			err: "and max",
+		},
+		{
+			name: "DynamicPortOk",
+			conf: Config{
+				DataDir: "/tmp",
+				Client: &ClientConfig{
+					Enabled:        true,
+					MinDynamicPort: 4000,
+					MaxDynamicPort: 5000,
+				},
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			mui := cli.NewMockUi()
+			cmd := &Command{Ui: mui}
+			config := DefaultConfig().Merge(&tc.conf)
+			result := cmd.isValidConfig(config, DefaultConfig())
+			if tc.err == "" {
+				// No error expected
+				assert.True(t, result, mui.ErrorWriter.String())
+				return
+			}
+
+			// Error expected
+			assert.False(t, result)
+			require.Contains(t, mui.ErrorWriter.String(), tc.err)
+			t.Logf("%s returned: %s", tc.name, mui.ErrorWriter.String())
+		})
+	}
+}

nomad/structs/network.go

@@ -20,8 +20,8 @@ const (
 	// to assign a random port
 	maxRandPortAttempts = 20
 
-	// maxValidPort is the max valid port number
-	maxValidPort = 65536
+	// MaxValidPort is the max valid port number
+	MaxValidPort = 65536
 )
 
 var (
@@ -67,7 +67,7 @@ func (idx *NetworkIndex) getUsedPortsFor(ip string) Bitmap {
 			used = raw.(Bitmap)
 			used.Clear()
 		} else {
-			used, _ = NewBitmap(maxValidPort)
+			used, _ = NewBitmap(MaxValidPort)
 		}
 		idx.UsedPorts[ip] = used
 	}
@@ -215,7 +215,7 @@ func (idx *NetworkIndex) AddReserved(n *NetworkResource) (collide bool) {
 	for _, ports := range [][]Port{n.ReservedPorts, n.DynamicPorts} {
 		for _, port := range ports {
 			// Guard against invalid port
-			if port.Value < 0 || port.Value >= maxValidPort {
+			if port.Value < 0 || port.Value >= MaxValidPort {
 				return true
 			}
 			if used.Check(uint(port.Value)) {
@@ -234,7 +234,7 @@ func (idx *NetworkIndex) AddReserved(n *NetworkResource) (collide bool) {
 func (idx *NetworkIndex) AddReservedPorts(ports AllocatedPorts) (collide bool) {
 	for _, port := range ports {
 		used := idx.getUsedPortsFor(port.HostIP)
-		if port.Value < 0 || port.Value >= maxValidPort {
+		if port.Value < 0 || port.Value >= MaxValidPort {
 			return true
 		}
 		if used.Check(uint(port.Value)) {
@@ -265,7 +265,7 @@ func (idx *NetworkIndex) AddReservedPortRange(ports string) (collide bool) {
 	for _, used := range idx.UsedPorts {
 		for _, port := range resPorts {
 			// Guard against invalid port
-			if port >= maxValidPort {
+			if port >= MaxValidPort {
 				return true
 			}
 			if used.Check(uint(port)) {
@@ -291,7 +291,7 @@ func (idx *NetworkIndex) AddReservedPortsForIP(ports string, ip string) (collide
 	used := idx.getUsedPortsFor(ip)
 	for _, port := range resPorts {
 		// Guard against invalid port
-		if port >= maxValidPort {
+		if port >= MaxValidPort {
 			return true
 		}
 		if used.Check(uint(port)) {
@@ -345,7 +345,7 @@ func (idx *NetworkIndex) AssignPorts(ask *NetworkResource) (AllocatedPorts, erro
 		for _, addr := range idx.AvailAddresses[port.HostNetwork] {
 			used := idx.getUsedPortsFor(addr.Address)
 			// Guard against invalid port
-			if port.Value < 0 || port.Value >= maxValidPort {
+			if port.Value < 0 || port.Value >= MaxValidPort {
 				return nil, fmt.Errorf("invalid port %d (out of range)", port.Value)
 			}
 
@@ -438,7 +438,7 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour
 		// Check if any of the reserved ports are in use
 		for _, port := range ask.ReservedPorts {
 			// Guard against invalid port
-			if port.Value < 0 || port.Value >= maxValidPort {
+			if port.Value < 0 || port.Value >= MaxValidPort {
 				err = fmt.Errorf("invalid port %d (out of range)", port.Value)
 				return
 			}
@@ -510,7 +510,7 @@ func getDynamicPortsPrecise(nodeUsed Bitmap, minDynamicPort, maxDynamicPort int,
 			return nil, err
 		}
 	} else {
-		usedSet, err = NewBitmap(maxValidPort)
+		usedSet, err = NewBitmap(MaxValidPort)
 		if err != nil {
 			return nil, err
 		}