From e6eff95769156701bf26c6d81504c214903d6ad1 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 12 Jan 2022 12:04:31 -0800 Subject: [PATCH 1/2] agent: validate reserved_ports are valid Goal is to fix at least one of the causes that can cause a node to be ineligible to receive work: https://github.com/hashicorp/nomad/issues/9506#issuecomment-1002880600 --- command/agent/command.go | 21 +++++++++++++++++++ command/agent/command_test.go | 28 +++++++++++++++++++++++++ nomad/structs/funcs.go | 12 +++++++++++ nomad/structs/funcs_test.go | 39 +++++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+) diff --git a/command/agent/command.go b/command/agent/command.go index 0cdfc76aa..2966bd49d 100644 --- a/command/agent/command.go +++ b/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 == "" { diff --git a/command/agent/command_test.go b/command/agent/command_test.go index 6b2c80618..3d51eab00 100644 --- a/command/agent/command_test.go +++ b/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 { diff --git a/nomad/structs/funcs.go b/nomad/structs/funcs.go index 1820443a7..b77e3cb26 100644 --- a/nomad/structs/funcs.go +++ b/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) } diff --git a/nomad/structs/funcs_test.go b/nomad/structs/funcs_test.go index 988e5527b..24211c67a 100644 --- a/nomad/structs/funcs_test.go +++ b/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) + }) + } +} From ebadaabc718cc4e9316b530729db84cf026ad0ae Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 12 Jan 2022 12:28:38 -0800 Subject: [PATCH 2/2] doc: add changelog for #11830 --- .changelog/11830.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/11830.txt diff --git a/.changelog/11830.txt b/.changelog/11830.txt new file mode 100644 index 000000000..29eb29a9a --- /dev/null +++ b/.changelog/11830.txt @@ -0,0 +1,3 @@ +```release-note:bug +agent: Validate reserved_ports are valid to prevent unschedulable nodes. +```