Skip to content

Commit

Permalink
client: improve errors & tests for dynamic ports
Browse files Browse the repository at this point in the history
  • Loading branch information
@schmichael
schmichael committed Oct 13, 2021
1 parent 0620bb0 commit 3821260
Show file tree
Hide file tree
Showing 3 changed files with 164 additions and 13 deletions.
14 changes: 11 additions & 3 deletions command/agent/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down
143 changes: 143 additions & 0 deletions command/agent/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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.MaxInt,
},
},
err: "min_dynamic_port",
},
{
name: "BigMaxDynamicPort",
conf: Config{
Client: &ClientConfig{
Enabled: true,
MaxDynamicPort: math.MaxInt,
},
},
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())
})
}
}
20 changes: 10 additions & 10 deletions nomad/structs/network.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)) {
Expand All @@ -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)) {
Expand Down Expand Up @@ -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)) {
Expand All @@ -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)) {
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down

0 comments on commit 3821260

Please sign in to comment.