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

@@ -386,6 +386,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

@@ -12,6 +12,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/version"
 )
 
@@ -363,6 +364,33 @@ func TestIsValidConfig(t *testing.T) {
 				},
 			},
 		},
+		{
+			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 {

nomad/structs/funcs.go

@@ -531,6 +531,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{}{}
 			}
@@ -541,6 +547,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

@@ -880,3 +880,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)
+		})
+	}
+}