From e3b6f62198db9e3d10709c939b31ef19bf35c6b7 Mon Sep 17 00:00:00 2001 From: Aleksandr Zagaevskiy Date: Fri, 10 Sep 2021 11:52:47 +0300 Subject: [PATCH 1/8] Support configurable dynamic port range --- client/client.go | 12 ++++++++++++ client/config/config.go | 6 ++++++ command/agent/agent.go | 2 ++ command/agent/command.go | 6 ++++++ command/agent/config.go | 16 ++++++++++++++++ nomad/structs/network.go | 35 +++++++++++++++++++++-------------- nomad/structs/network_test.go | 12 ++++++------ nomad/structs/structs.go | 3 +++ 8 files changed, 72 insertions(+), 20 deletions(-) diff --git a/client/client.go b/client/client.go index 927fe4a0eb8..d3dd8aaaa39 100644 --- a/client/client.go +++ b/client/client.go @@ -643,6 +643,8 @@ func (c *Client) init() error { c.logger.Info("using alloc directory", "alloc_dir", c.config.AllocDir) + c.logger.Info("using dynamic ports", "min", c.config.MinDynamicPort, "max", c.config.MaxDynamicPort) + // Ensure cgroups are created on linux platform if runtime.GOOS == "linux" && c.cpusetManager != nil { err := c.cpusetManager.Init() @@ -1385,6 +1387,8 @@ func (c *Client) setupNode() error { } if node.NodeResources == nil { node.NodeResources = &structs.NodeResources{} + node.NodeResources.MinDynamicPort = c.config.MinDynamicPort + node.NodeResources.MaxDynamicPort = c.config.MaxDynamicPort } if node.ReservedResources == nil { node.ReservedResources = &structs.NodeReservedResources{} @@ -1496,6 +1500,14 @@ func (c *Client) updateNodeFromFingerprint(response *fingerprint.FingerprintResp c.config.Node.NodeResources.Merge(response.NodeResources) nodeHasChanged = true } + + response.NodeResources.MinDynamicPort = c.config.MinDynamicPort + response.NodeResources.MaxDynamicPort = c.config.MaxDynamicPort + if c.config.Node.NodeResources.MinDynamicPort != response.NodeResources.MinDynamicPort || + c.config.Node.NodeResources.MaxDynamicPort != response.NodeResources.MaxDynamicPort { + nodeHasChanged = true + } + } if nodeHasChanged { diff --git a/client/config/config.go b/client/config/config.go index e9c5e0fa562..13af0ca2d6c 100644 --- a/client/config/config.go +++ b/client/config/config.go @@ -137,6 +137,12 @@ type Config struct { // communicating with plugin subsystems over loopback ClientMinPort uint + // MaxDynamicPort is the largest dynamic port generated + MaxDynamicPort int + + // MinDynamicPort is the smallest dynamic port generated + MinDynamicPort int + // A mapping of directories on the host OS to attempt to embed inside each // task's chroot. ChrootEnv map[string]string diff --git a/command/agent/agent.go b/command/agent/agent.go index a500dea7f43..46de635b201 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -589,6 +589,8 @@ func convertClientConfig(agentConfig *Config) (*clientconfig.Config, error) { } conf.ClientMaxPort = uint(agentConfig.Client.ClientMaxPort) conf.ClientMinPort = uint(agentConfig.Client.ClientMinPort) + conf.MaxDynamicPort = agentConfig.Client.MaxDynamicPort + conf.MinDynamicPort = agentConfig.Client.MinDynamicPort conf.DisableRemoteExec = agentConfig.Client.DisableRemoteExec if agentConfig.Client.TemplateConfig.FunctionBlacklist != nil { conf.TemplateConfig.FunctionDenylist = agentConfig.Client.TemplateConfig.FunctionBlacklist diff --git a/command/agent/command.go b/command/agent/command.go index 1e753cf2723..0fbe1b95c1c 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -372,6 +372,12 @@ func (c *Command) isValidConfig(config, cmdConfig *Config) bool { return false } + if config.Client.MinDynamicPort > 0 && config.Client.MaxDynamicPort > 0 && + config.Client.MinDynamicPort >= config.Client.MaxDynamicPort { + c.Ui.Error("Invalid dynamic port range") + 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/config.go b/command/agent/config.go index 687b3f03748..0ec25534fc5 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -233,6 +233,14 @@ type ClientConfig struct { // communicating with plugin subsystems ClientMinPort int `hcl:"client_min_port"` + // MaxDynamicPort is the upper range of the dynamic ports that the client + // uses for allocations + MaxDynamicPort int `hcl:"max_dynamic_port"` + + // MinDynamicPort is the lower range of the dynamic ports that the client + // uses for allocations + MinDynamicPort int `hcl:"min_dynamic_port"` + // Reserved is used to reserve resources from being used by Nomad. This can // be used to target a certain utilization or to prevent Nomad from using a // particular set of ports. @@ -917,6 +925,8 @@ func DefaultConfig() *Config { MaxKillTimeout: "30s", ClientMinPort: 14000, ClientMaxPort: 14512, + MinDynamicPort: 20000, + MaxDynamicPort: 32000, Reserved: &Resources{}, GCInterval: 1 * time.Minute, GCParallelDestroys: 2, @@ -1598,6 +1608,12 @@ func (a *ClientConfig) Merge(b *ClientConfig) *ClientConfig { if b.ClientMinPort != 0 { result.ClientMinPort = b.ClientMinPort } + if b.MaxDynamicPort != 0 { + result.MaxDynamicPort = b.MaxDynamicPort + } + if b.MinDynamicPort != 0 { + result.MinDynamicPort = b.MinDynamicPort + } if result.Reserved == nil && b.Reserved != nil { reserved := *b.Reserved result.Reserved = &reserved diff --git a/nomad/structs/network.go b/nomad/structs/network.go index f7e69039db2..f597ff712aa 100644 --- a/nomad/structs/network.go +++ b/nomad/structs/network.go @@ -8,12 +8,6 @@ import ( ) const ( - // MinDynamicPort is the smallest dynamic port generated - MinDynamicPort = 20000 - - // MaxDynamicPort is the largest dynamic port generated - MaxDynamicPort = 32000 - // maxRandPortAttempts is the maximum number of attempt // to assign a random port maxRandPortAttempts = 20 @@ -39,6 +33,9 @@ type NetworkIndex struct { AvailBandwidth map[string]int // Bandwidth by device UsedPorts map[string]Bitmap // Ports by IP UsedBandwidth map[string]int // Bandwidth by device + + MinDynamicPort int // The smallest dynamic port generated + MaxDynamicPort int // The largest dynamic port generated } // NewNetworkIndex is used to construct a new network index @@ -48,6 +45,8 @@ func NewNetworkIndex() *NetworkIndex { AvailBandwidth: make(map[string]int), UsedPorts: make(map[string]Bitmap), UsedBandwidth: make(map[string]int), + MinDynamicPort: 20000, + MaxDynamicPort: 32000, } } @@ -136,6 +135,14 @@ func (idx *NetworkIndex) SetNode(node *Node) (collide bool) { } } + if node.NodeResources != nil && node.NodeResources.MinDynamicPort > 0 { + idx.MinDynamicPort = node.NodeResources.MinDynamicPort + } + + if node.NodeResources != nil && node.NodeResources.MaxDynamicPort > 0 { + idx.MaxDynamicPort = node.NodeResources.MaxDynamicPort + } + return } @@ -368,10 +375,10 @@ func (idx *NetworkIndex) AssignPorts(ask *NetworkResource) (AllocatedPorts, erro // lower memory usage. var dynPorts []int // TODO: its more efficient to find multiple dynamic ports at once - dynPorts, addrErr = getDynamicPortsStochastic(used, reservedIdx[port.HostNetwork], 1) + dynPorts, addrErr = getDynamicPortsStochastic(used, idx.MinDynamicPort, idx.MaxDynamicPort, reservedIdx[port.HostNetwork], 1) if addrErr != nil { // Fall back to the precise method if the random sampling failed. - dynPorts, addrErr = getDynamicPortsPrecise(used, reservedIdx[port.HostNetwork], 1) + dynPorts, addrErr = getDynamicPortsPrecise(used, idx.MinDynamicPort, idx.MaxDynamicPort, reservedIdx[port.HostNetwork], 1) if addrErr != nil { continue } @@ -450,13 +457,13 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour // lower memory usage. var dynPorts []int var dynErr error - dynPorts, dynErr = getDynamicPortsStochastic(used, ask.ReservedPorts, len(ask.DynamicPorts)) + dynPorts, dynErr = getDynamicPortsStochastic(used, idx.MinDynamicPort, idx.MaxDynamicPort, ask.ReservedPorts, len(ask.DynamicPorts)) if dynErr == nil { goto BUILD_OFFER } // Fall back to the precise method if the random sampling failed. - dynPorts, dynErr = getDynamicPortsPrecise(used, ask.ReservedPorts, len(ask.DynamicPorts)) + dynPorts, dynErr = getDynamicPortsPrecise(used, idx.MinDynamicPort, idx.MaxDynamicPort, ask.ReservedPorts, len(ask.DynamicPorts)) if dynErr != nil { err = dynErr return @@ -485,7 +492,7 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour // no ports have been allocated yet, the network ask and returns a set of unused // ports to fulfil the ask's DynamicPorts or an error if it failed. An error // means the ask can not be satisfied as the method does a precise search. -func getDynamicPortsPrecise(nodeUsed Bitmap, reserved []Port, numDyn int) ([]int, error) { +func getDynamicPortsPrecise(nodeUsed Bitmap, minDynamicPort, maxDynamicPort int, reserved []Port, numDyn int) ([]int, error) { // Create a copy of the used ports and apply the new reserves var usedSet Bitmap var err error @@ -506,7 +513,7 @@ func getDynamicPortsPrecise(nodeUsed Bitmap, reserved []Port, numDyn int) ([]int } // Get the indexes of the unset - availablePorts := usedSet.IndexesInRange(false, MinDynamicPort, MaxDynamicPort) + availablePorts := usedSet.IndexesInRange(false, uint(minDynamicPort), uint(maxDynamicPort)) // Randomize the amount we need if len(availablePorts) < numDyn { @@ -527,7 +534,7 @@ func getDynamicPortsPrecise(nodeUsed Bitmap, reserved []Port, numDyn int) ([]int // ports to fulfil the ask's DynamicPorts or an error if it failed. An error // does not mean the ask can not be satisfied as the method has a fixed amount // of random probes and if these fail, the search is aborted. -func getDynamicPortsStochastic(nodeUsed Bitmap, reservedPorts []Port, count int) ([]int, error) { +func getDynamicPortsStochastic(nodeUsed Bitmap, minDynamicPort, maxDynamicPort int, reservedPorts []Port, count int) ([]int, error) { var reserved, dynamic []int for _, port := range reservedPorts { reserved = append(reserved, port.Value) @@ -541,7 +548,7 @@ func getDynamicPortsStochastic(nodeUsed Bitmap, reservedPorts []Port, count int) return nil, fmt.Errorf("stochastic dynamic port selection failed") } - randPort := MinDynamicPort + rand.Intn(MaxDynamicPort-MinDynamicPort) + randPort := minDynamicPort + rand.Intn(maxDynamicPort-minDynamicPort) if nodeUsed != nil && nodeUsed.Check(uint(randPort)) { goto PICK } diff --git a/nomad/structs/network_test.go b/nomad/structs/network_test.go index ea80f116976..e7b0a52bedc 100644 --- a/nomad/structs/network_test.go +++ b/nomad/structs/network_test.go @@ -323,7 +323,7 @@ func TestNetworkIndex_AssignNetwork_Dynamic_Contention(t *testing.T) { }, ReservedResources: &NodeReservedResources{ Networks: NodeReservedNetworkResources{ - ReservedHostPorts: fmt.Sprintf("%d-%d", MinDynamicPort, MaxDynamicPort-1), + ReservedHostPorts: fmt.Sprintf("%d-%d", idx.MinDynamicPort, idx.MaxDynamicPort-1), }, }, } @@ -346,8 +346,8 @@ func TestNetworkIndex_AssignNetwork_Dynamic_Contention(t *testing.T) { if len(offer.DynamicPorts) != 1 { t.Fatalf("There should be one dynamic ports") } - if p := offer.DynamicPorts[0].Value; p != MaxDynamicPort { - t.Fatalf("Dynamic Port: should have been assigned %d; got %d", p, MaxDynamicPort) + if p := offer.DynamicPorts[0].Value; p != idx.MaxDynamicPort { + t.Fatalf("Dynamic Port: should have been assigned %d; got %d", p, idx.MaxDynamicPort) } } @@ -646,7 +646,7 @@ func TestNetworkIndex_AssignNetwork_Dynamic_Contention_Old(t *testing.T) { }, }, } - for i := MinDynamicPort; i < MaxDynamicPort; i++ { + for i := idx.MinDynamicPort; i < idx.MaxDynamicPort; i++ { n.Reserved.Networks[0].ReservedPorts = append(n.Reserved.Networks[0].ReservedPorts, Port{Value: i}) } @@ -669,8 +669,8 @@ func TestNetworkIndex_AssignNetwork_Dynamic_Contention_Old(t *testing.T) { if len(offer.DynamicPorts) != 1 { t.Fatalf("There should be three dynamic ports") } - if p := offer.DynamicPorts[0].Value; p != MaxDynamicPort { - t.Fatalf("Dynamic Port: should have been assigned %d; got %d", p, MaxDynamicPort) + if p := offer.DynamicPorts[0].Value; p != idx.MaxDynamicPort { + t.Fatalf("Dynamic Port: should have been assigned %d; got %d", p, idx.MaxDynamicPort) } } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index d0753cbe6d5..dbc258f86e2 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2801,6 +2801,9 @@ type NodeResources struct { Networks Networks NodeNetworks []*NodeNetworkResource Devices []*NodeDeviceResource + + MinDynamicPort int + MaxDynamicPort int } func (n *NodeResources) Copy() *NodeResources { From aff4b47f3c5802f953a20394669d727d79a75c27 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 30 Sep 2021 17:05:10 -0700 Subject: [PATCH 2/8] api: add Node.{Min,Max}DynamicPort --- api/nodes.go | 3 +++ api/nodes_test.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/api/nodes.go b/api/nodes.go index 5eac71a8185..070eae975d0 100644 --- a/api/nodes.go +++ b/api/nodes.go @@ -554,6 +554,9 @@ type NodeResources struct { Disk NodeDiskResources Networks []*NetworkResource Devices []*NodeDeviceResource + + MinDynamicPort int + MaxDynamicPort int } type NodeCpuResources struct { diff --git a/api/nodes_test.go b/api/nodes_test.go index 5dab49c8481..ebe85f14fc8 100644 --- a/api/nodes_test.go +++ b/api/nodes_test.go @@ -168,6 +168,9 @@ func TestNodes_Info(t *testing.T) { result.ID, result.Datacenter) } + require.Equal(t, 20000, result.NodeResources.MinDynamicPort) + require.Equal(t, 32000, result.NodeResources.MaxDynamicPort) + // Check that the StatusUpdatedAt field is being populated correctly if result.StatusUpdatedAt < startTime { t.Fatalf("start time: %v, status updated: %v", startTime, result.StatusUpdatedAt) From 2968c012951425eb573efb7e650ec75657aff25d Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 30 Sep 2021 17:05:46 -0700 Subject: [PATCH 3/8] client: output reserved ports with min/max ports Also add a little more min/max port testing and add the consts back that had been removed: but unexported and as defaults. --- client/client.go | 6 +++++- command/agent/config_test.go | 4 ++++ nomad/structs/network.go | 12 ++++++++++-- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/client/client.go b/client/client.go index d3dd8aaaa39..483ca642e6b 100644 --- a/client/client.go +++ b/client/client.go @@ -643,7 +643,11 @@ func (c *Client) init() error { c.logger.Info("using alloc directory", "alloc_dir", c.config.AllocDir) - c.logger.Info("using dynamic ports", "min", c.config.MinDynamicPort, "max", c.config.MaxDynamicPort) + c.logger.Info("using dynamic ports", + "min", c.config.MinDynamicPort, + "max", c.config.MaxDynamicPort, + "reserved", c.config.Node.ReservedResources.Networks.ReservedHostPorts, + ) // Ensure cgroups are created on linux platform if runtime.GOOS == "linux" && c.cpusetManager != nil { diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 0233a510644..8bb39a98fed 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -109,6 +109,8 @@ func TestConfig_Merge(t *testing.T) { }, NetworkSpeed: 100, CpuCompute: 100, + MinDynamicPort: 10001, + MaxDynamicPort: 10002, MemoryMB: 100, MaxKillTimeout: "20s", ClientMaxPort: 19996, @@ -292,6 +294,8 @@ func TestConfig_Merge(t *testing.T) { ClientMinPort: 22000, NetworkSpeed: 105, CpuCompute: 105, + MinDynamicPort: 10002, + MaxDynamicPort: 10003, MemoryMB: 105, MaxKillTimeout: "50s", DisableRemoteExec: false, diff --git a/nomad/structs/network.go b/nomad/structs/network.go index f597ff712aa..19f814be618 100644 --- a/nomad/structs/network.go +++ b/nomad/structs/network.go @@ -8,6 +8,14 @@ import ( ) const ( + // defaultMinDynamicPort is the smallest dynamic port generated by + // default + defaultMinDynamicPort = 20000 + + // defaultMaxDynamicPort is the largest dynamic port generated by + // default + defaultMaxDynamicPort = 32000 + // maxRandPortAttempts is the maximum number of attempt // to assign a random port maxRandPortAttempts = 20 @@ -45,8 +53,8 @@ func NewNetworkIndex() *NetworkIndex { AvailBandwidth: make(map[string]int), UsedPorts: make(map[string]Bitmap), UsedBandwidth: make(map[string]int), - MinDynamicPort: 20000, - MaxDynamicPort: 32000, + MinDynamicPort: defaultMinDynamicPort, + MaxDynamicPort: defaultMaxDynamicPort, } } From 73fa4754138ecf81509b1cbb4971b72c6e223fb8 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 30 Sep 2021 17:06:38 -0700 Subject: [PATCH 4/8] docs: add #11167 to changelog --- .changelog/11167.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/11167.md diff --git a/.changelog/11167.md b/.changelog/11167.md new file mode 100644 index 00000000000..a9521dabbaa --- /dev/null +++ b/.changelog/11167.md @@ -0,0 +1,3 @@ +```release-note:improvement +client: Allow configuring minimum and maximum host ports used for dynamic ports +``` From cc5c7443569192e42e18f564f399b7bae0382662 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 30 Sep 2021 17:10:28 -0700 Subject: [PATCH 5/8] docs: add new client.{min,max}_dynamic_port params --- website/content/docs/configuration/client.mdx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/website/content/docs/configuration/client.mdx b/website/content/docs/configuration/client.mdx index a7cb7a134e2..db8421ae997 100644 --- a/website/content/docs/configuration/client.mdx +++ b/website/content/docs/configuration/client.mdx @@ -65,6 +65,14 @@ client { - `memory_total_mb` `(int:0)` - Specifies an override for the total memory. If set, this value overrides any detected memory. +- `min_dynamic_port` `(int:20000)` - Specifies the minimum dynamic port to be + assigned. Individual ports and ranges of ports may be excluded from dynamic + port assignment via [`reserved`](#reserved-parameters) parameters. + +- `max_dynamic_port` `(int:32000)` - Specifies the maximum dynamic port to be + assigned. Individual ports and ranges of ports may be excluded from dynamic + port assignment via [`reserved`](#reserved-parameters) parameters. + - `node_class` `(string: "")` - Specifies an arbitrary string used to logically group client nodes by user-defined class. This can be used during job placement as a filter. From c615870911a0eb5929244efe406e160e7f7d662a Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 4 Oct 2021 15:43:35 -0700 Subject: [PATCH 6/8] client: defensively log reserved ports - Fix test broken due to being improperly setup. - Include min/max ports in default client config. --- client/client.go | 8 +++++++- client/client_test.go | 12 ++++++++---- client/config/config.go | 2 ++ nomad/structs/network.go | 12 ++++++------ 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/client/client.go b/client/client.go index 483ca642e6b..038ce31e677 100644 --- a/client/client.go +++ b/client/client.go @@ -643,10 +643,16 @@ func (c *Client) init() error { c.logger.Info("using alloc directory", "alloc_dir", c.config.AllocDir) + reserved := "" + if c.config.Node != nil && c.config.Node.ReservedResources != nil { + // Node should always be non-nil due to initialization in the + // agent package, but don't risk a panic just for a long line. + reserved = c.config.Node.ReservedResources.Networks.ReservedHostPorts + } c.logger.Info("using dynamic ports", "min", c.config.MinDynamicPort, "max", c.config.MaxDynamicPort, - "reserved", c.config.Node.ReservedResources.Networks.ReservedHostPorts, + "reserved", reserved, ) // Ensure cgroups are created on linux platform diff --git a/client/client_test.go b/client/client_test.go index 4c7c5688976..d27ab0ed061 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -737,11 +737,15 @@ func TestClient_Init(t *testing.T) { defer os.RemoveAll(dir) allocDir := filepath.Join(dir, "alloc") + config := config.DefaultConfig() + config.AllocDir = allocDir + config.StateDBFactory = cstate.GetStateDBFactory(true) + + // Node is always initialized in agent.go:convertClientConfig() + config.Node = mock.Node() + client := &Client{ - config: &config.Config{ - AllocDir: allocDir, - StateDBFactory: cstate.GetStateDBFactory(true), - }, + config: config, logger: testlog.HCLogger(t), } diff --git a/client/config/config.go b/client/config/config.go index 13af0ca2d6c..35d699988c9 100644 --- a/client/config/config.go +++ b/client/config/config.go @@ -343,6 +343,8 @@ func DefaultConfig() *Config { CNIInterfacePrefix: "eth", HostNetworks: map[string]*structs.ClientHostNetworkConfig{}, CgroupParent: cgutil.DefaultCgroupParent, + MaxDynamicPort: structs.DefaultMinDynamicPort, + MinDynamicPort: structs.DefaultMaxDynamicPort, } } diff --git a/nomad/structs/network.go b/nomad/structs/network.go index 19f814be618..3439c359ba5 100644 --- a/nomad/structs/network.go +++ b/nomad/structs/network.go @@ -8,13 +8,13 @@ import ( ) const ( - // defaultMinDynamicPort is the smallest dynamic port generated by + // DefaultMinDynamicPort is the smallest dynamic port generated by // default - defaultMinDynamicPort = 20000 + DefaultMinDynamicPort = 20000 - // defaultMaxDynamicPort is the largest dynamic port generated by + // DefaultMaxDynamicPort is the largest dynamic port generated by // default - defaultMaxDynamicPort = 32000 + DefaultMaxDynamicPort = 32000 // maxRandPortAttempts is the maximum number of attempt // to assign a random port @@ -53,8 +53,8 @@ func NewNetworkIndex() *NetworkIndex { AvailBandwidth: make(map[string]int), UsedPorts: make(map[string]Bitmap), UsedBandwidth: make(map[string]int), - MinDynamicPort: defaultMinDynamicPort, - MaxDynamicPort: defaultMaxDynamicPort, + MinDynamicPort: DefaultMinDynamicPort, + MaxDynamicPort: DefaultMaxDynamicPort, } } From 0620bb04a5d22cd1fc7de516eb0b5a06e27abfce Mon Sep 17 00:00:00 2001 From: Aleksandr Zagaevskiy Date: Mon, 11 Oct 2021 14:13:59 +0300 Subject: [PATCH 7/8] fixup! Support configurable dynamic port range --- command/agent/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/command.go b/command/agent/command.go index 0fbe1b95c1c..01f196706ba 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -372,7 +372,7 @@ func (c *Command) isValidConfig(config, cmdConfig *Config) bool { return false } - if config.Client.MinDynamicPort > 0 && config.Client.MaxDynamicPort > 0 && + if config.Client.MinDynamicPort < 0 || config.Client.MaxDynamicPort > 65535 || config.Client.MinDynamicPort >= config.Client.MaxDynamicPort { c.Ui.Error("Invalid dynamic port range") return false From fc89835dafec78a1369292ebb597548e4e206408 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 13 Oct 2021 15:50:46 -0700 Subject: [PATCH 8/8] client: improve errors & tests for dynamic ports --- command/agent/command.go | 14 +++- command/agent/command_test.go | 143 ++++++++++++++++++++++++++++++++++ nomad/structs/network.go | 20 ++--- 3 files changed, 164 insertions(+), 13 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index 01f196706ba..d540806f66f 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" @@ -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 } diff --git a/command/agent/command_test.go b/command/agent/command_test.go index 7be831ad430..6b2c80618e0 100644 --- a/command/agent/command_test.go +++ b/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()) + }) + } +} diff --git a/nomad/structs/network.go b/nomad/structs/network.go index 3439c359ba5..14dbfa9641d 100644 --- a/nomad/structs/network.go +++ b/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 }