Merge pull request #11830 from hashicorp/b-validate-reserved-ports

agent: validate reserved_ports are valid

.changelog/11830.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+agent: Validate reserved_ports are valid to prevent unschedulable nodes.
+```

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"
@@ -366,6 +367,27 @@ func (c *Command) isValidConfig(config, cmdConfig *Config) bool {
 		return false
 	}
 
+	if config.Client.Reserved == nil {
+		// Coding error; should always be set by DefaultConfig()
+		c.Ui.Error("client.reserved must be initialized. Please report a bug.")
+		return false
+	}
+
+	if ports := config.Client.Reserved.ReservedPorts; ports != "" {
+		if _, err := structs.ParsePortRanges(ports); err != nil {
+			c.Ui.Error(fmt.Sprintf("reserved.reserved_ports %q invalid: %v", ports, err))
+			return false
+		}
+	}
+
+	for _, hn := range config.Client.HostNetworks {
+		if _, err := structs.ParsePortRanges(hn.ReservedPorts); err != nil {
+			c.Ui.Error(fmt.Sprintf("host_network[%q].reserved_ports %q invalid: %v",
+				hn.Name, hn.ReservedPorts, err))
+			return false
+		}
+	}
+
 	if !config.DevMode {
 		// Ensure that we have the directories we need to run.
 		if config.Server.Enabled && config.DataDir == "" {

command/agent/command_test.go

@@ -8,7 +8,10 @@ import (
 	"testing"
 
 	"github.com/mitchellh/cli"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
+	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/version"
 )
 
@@ -241,3 +244,108 @@ 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: "BadReservedPorts",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled: true,
+					Reserved: &Resources{
+						ReservedPorts: "3-2147483647",
+					},
+				},
+			},
+			err: `reserved.reserved_ports "3-2147483647" invalid: port must be < 65536 but found 2147483647`,
+		},
+		{
+			name: "BadHostNetworkReservedPorts",
+			conf: Config{
+				Client: &ClientConfig{
+					Enabled: true,
+					HostNetworks: []*structs.ClientHostNetworkConfig{
+						&structs.ClientHostNetworkConfig{
+							Name:          "test",
+							ReservedPorts: "3-2147483647",
+						},
+					},
+				},
+			},
+			err: `host_network["test"].reserved_ports "3-2147483647" invalid: port must be < 65536 but found 2147483647`,
+		},
+	}
+
+	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/funcs.go

@@ -452,6 +452,12 @@ func ParsePortRanges(spec string) ([]uint64, error) {
 				return nil, fmt.Errorf("invalid range: starting value (%v) less than ending (%v) value", end, start)
 			}
 
+			// Full range validation is below but prevent creating
+			// arbitrarily large arrays here
+			if end > maxValidPort {
+				return nil, fmt.Errorf("port must be < %d but found %d", maxValidPort, end)
+			}
+
 			for i := start; i <= end; i++ {
 				ports[i] = struct{}{}
 			}
@@ -462,6 +468,12 @@ func ParsePortRanges(spec string) ([]uint64, error) {
 
 	var results []uint64
 	for port := range ports {
+		if port == 0 {
+			return nil, fmt.Errorf("port must be > 0")
+		}
+		if port > maxValidPort {
+			return nil, fmt.Errorf("port must be < %d but found %d", maxValidPort, port)
+		}
 		results = append(results, port)
 	}
 

nomad/structs/funcs_test.go

@@ -796,3 +796,42 @@ func TestMergeMultierrorWarnings(t *testing.T) {
 
 	require.Equal(t, "2 warning(s):\n\n* foo\n* bar", str)
 }
+
+// TestParsePortRanges asserts ParsePortRanges errors on invalid port ranges.
+func TestParsePortRanges(t *testing.T) {
+	cases := []struct {
+		name string
+		spec string
+		err  string
+	}{
+		{
+			name: "UnmatchedDash",
+			spec: "-1",
+			err:  `strconv.ParseUint: parsing "": invalid syntax`,
+		},
+		{
+			name: "Zero",
+			spec: "0",
+			err:  "port must be > 0",
+		},
+		{
+			name: "TooBig",
+			spec: fmt.Sprintf("1-%d", maxValidPort+1),
+			err:  "port must be < 65536 but found 65537",
+		},
+		{
+			name: "WayTooBig",           // would OOM if not caught early enough
+			spec: "9223372036854775807", // (2**63)-1
+			err:  "port must be < 65536 but found 9223372036854775807",
+		},
+	}
+
+	for i := range cases {
+		tc := cases[i]
+		t.Run(tc.name, func(t *testing.T) {
+			results, err := ParsePortRanges(tc.spec)
+			require.Nil(t, results)
+			require.EqualError(t, err, tc.err)
+		})
+	}
+}