diff --git a/.changelog/11830.txt b/.changelog/11830.txt new file mode 100644 index 000000000000..29eb29a9a391 --- /dev/null +++ b/.changelog/11830.txt @@ -0,0 +1,3 @@ +```release-note:bug +agent: Validate reserved_ports are valid to prevent unschedulable nodes. +``` diff --git a/command/agent/command.go b/command/agent/command.go index 7489c67bb34c..3d8ee901f109 100644 --- a/command/agent/command.go +++ b/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 == "" { diff --git a/command/agent/command_test.go b/command/agent/command_test.go index 7be831ad430d..a063f88a57ed 100644 --- a/command/agent/command_test.go +++ b/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()) + }) + } +} diff --git a/nomad/structs/funcs.go b/nomad/structs/funcs.go index 2756b3945aa6..7907b027b0f6 100644 --- a/nomad/structs/funcs.go +++ b/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) } diff --git a/nomad/structs/funcs_test.go b/nomad/structs/funcs_test.go index 2893c5264fcf..5c8ba1cfea57 100644 --- a/nomad/structs/funcs_test.go +++ b/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) + }) + } +}