diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go index 9955d7e99..93547c26e 100644 --- a/command/agent/consul/client.go +++ b/command/agent/consul/client.go @@ -1145,10 +1145,13 @@ func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet // If port isn't a label, try to parse it as a literal port number port, err := strconv.Atoi(portLabel) if err != nil { - return "", 0, fmt.Errorf("invalid port %q: %v", portLabel, err) + // Don't include Atoi error message as user likely + // never intended it to be a numeric and it creates a + // confusing error message + return "", 0, fmt.Errorf("invalid port label %q: port labels in driver address_mode must be numeric or in the driver's port map", portLabel) } if port <= 0 { - return "", 0, fmt.Errorf("invalid port: %q: port 0 is invalid", portLabel) + return "", 0, fmt.Errorf("invalid port: %q: port must be >0", portLabel) } return driverNet.IP, port, nil diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index 05b1c0f8d..dd6b3477b 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -1468,6 +1468,7 @@ func TestGetAddress(t *testing.T) { ExpectedPort int ExpectedErr string }{ + // Valid Configurations { Name: "ExampleService", Mode: structs.AddressModeAuto, @@ -1529,6 +1530,8 @@ func TestGetAddress(t *testing.T) { ExpectedIP: "10.1.2.3", ExpectedPort: 7890, }, + + // Invalid Configurations { Name: "DriverWithoutNetwork", Mode: structs.AddressModeDriver, diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index c4e52038f..87d820ca3 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -1178,6 +1178,122 @@ func TestTask_Validate_Services(t *testing.T) { } } +func TestTask_Validate_Service_AddressMode_Ok(t *testing.T) { + ephemeralDisk := DefaultEphemeralDisk() + getTask := func(s *Service) *Task { + task := &Task{ + Name: "web", + Driver: "docker", + Resources: DefaultResources(), + Services: []*Service{s}, + LogConfig: DefaultLogConfig(), + } + task.Resources.Networks = []*NetworkResource{ + { + MBits: 10, + DynamicPorts: []Port{ + { + Label: "http", + Value: 80, + }, + }, + }, + } + return task + } + + cases := []*Service{ + { + // https://github.com/hashicorp/nomad/issues/3681#issuecomment-357274177 + Name: "DriverModeWithLabel", + PortLabel: "http", + AddressMode: AddressModeDriver, + }, + { + Name: "DriverModeWithPort", + PortLabel: "80", + AddressMode: AddressModeDriver, + }, + { + Name: "HostModeWithLabel", + PortLabel: "http", + AddressMode: AddressModeHost, + }, + { + Name: "HostModeWithoutLabel", + AddressMode: AddressModeHost, + }, + { + Name: "DriverModeWithoutLabel", + AddressMode: AddressModeDriver, + }, + } + + for _, service := range cases { + task := getTask(service) + t.Run(service.Name, func(t *testing.T) { + if err := task.Validate(ephemeralDisk); err != nil { + t.Fatalf("unexpected err: %v", err) + } + }) + } +} + +func TestTask_Validate_Service_AddressMode_Bad(t *testing.T) { + ephemeralDisk := DefaultEphemeralDisk() + getTask := func(s *Service) *Task { + task := &Task{ + Name: "web", + Driver: "docker", + Resources: DefaultResources(), + Services: []*Service{s}, + LogConfig: DefaultLogConfig(), + } + task.Resources.Networks = []*NetworkResource{ + { + MBits: 10, + DynamicPorts: []Port{ + { + Label: "http", + Value: 80, + }, + }, + }, + } + return task + } + + cases := []*Service{ + { + // https://github.com/hashicorp/nomad/issues/3681#issuecomment-357274177 + Name: "DriverModeWithLabel", + PortLabel: "asdf", + AddressMode: AddressModeDriver, + }, + { + Name: "HostModeWithLabel", + PortLabel: "asdf", + AddressMode: AddressModeHost, + }, + { + Name: "HostModeWithPort", + PortLabel: "80", + AddressMode: AddressModeHost, + }, + } + + for _, service := range cases { + task := getTask(service) + t.Run(service.Name, func(t *testing.T) { + err := task.Validate(ephemeralDisk) + if err == nil { + t.Fatalf("expected an error") + } + //t.Logf("err: %v", err) + }) + } +} + func TestTask_Validate_Service_Check(t *testing.T) { invalidCheck := ServiceCheck{