From cde796162ceda93e270852328fd211c5c97de16a Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 20 Dec 2017 15:02:34 -0800 Subject: [PATCH 1/6] Always advertise driver IP when in driver mode Fixes #3681 When in drive address mode Nomad should always advertise the driver's IP in Consul even when no network exists. This matches the 0.6 behavior. When in host address mode Nomad advertises the alloc's network's IP if one exists. Otherwise it lets Consul determine the IP. I also added some much needed logging around Docker's network discovery. --- client/driver/docker.go | 10 ++++- command/agent/consul/client.go | 22 ++++++++--- command/agent/consul/unit_test.go | 64 +++++++++++++++++++------------ 3 files changed, 65 insertions(+), 31 deletions(-) diff --git a/client/driver/docker.go b/client/driver/docker.go index d2b3827c0647..81cffb448917 100644 --- a/client/driver/docker.go +++ b/client/driver/docker.go @@ -820,6 +820,9 @@ func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (*StartRespon // Detect container address ip, autoUse := d.detectIP(container) + if ip == "" { + d.logger.Printf("[DEBUG] driver.docker: task %s could not detect a container IP", d.taskName) + } // Create a response with the driver handle and container network metadata resp := &StartResponse{ @@ -860,13 +863,18 @@ func (d *DockerDriver) detectIP(c *docker.Container) (string, bool) { // Linux, nat on Windows) if name != "bridge" && name != "nat" { auto = true + d.logger.Printf("[INFO] driver.docker: task %s auto-advertising detected IP %s on network %q", + d.taskName, ip, name) + } else { + d.logger.Printf("[DEBUG] driver.docker task %s detect IP %s on network %q but not auto-advertising", + d.taskName, ip, name) } break } if n := len(c.NetworkSettings.Networks); n > 1 { - d.logger.Printf("[WARN] driver.docker: multiple (%d) Docker networks for container %q but Nomad only supports 1: choosing %q", n, c.ID, ipName) + d.logger.Printf("[WARN] driver.docker: task %s multiple (%d) Docker networks for container %q but Nomad only supports 1: choosing %q", d.taskName, n, c.ID, ipName) } return ip, auto diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go index b8db963aeb70..9955d7e99e1a 100644 --- a/command/agent/consul/client.go +++ b/command/agent/consul/client.go @@ -1098,11 +1098,6 @@ func isOldNomadService(id string) bool { // label is specified (an empty value), zero values are returned because no // address could be resolved. func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet *cstructs.DriverNetwork) (string, int, error) { - // No port label specified, no address can be assembled - if portLabel == "" { - return "", 0, nil - } - switch addrMode { case structs.AddressModeAuto: if driverNet.Advertise() { @@ -1112,6 +1107,18 @@ func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet } return getAddress(addrMode, portLabel, networks, driverNet) case structs.AddressModeHost: + if portLabel == "" { + if len(networks) != 1 { + // If no networks are specified return zero + // values. Consul will advertise the host IP + // with no port. This is the pre-0.7.1 behavior + // some people rely on. + return "", 0, nil + } + + return networks[0].IP, 0, nil + } + // Default path: use host ip:port ip, port := networks.Port(portLabel) if ip == "" && port <= 0 { @@ -1125,6 +1132,11 @@ func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet return "", 0, fmt.Errorf(`cannot use address_mode="driver": no driver network exists`) } + // If no port label is specified just return the IP + if portLabel == "" { + return driverNet.IP, 0, nil + } + // If the port is a label, use the driver's port (not the host's) if port, ok := driverNet.PortMap[portLabel]; ok { return driverNet.IP, port, nil diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index 4d8123b881ed..fa8b52ab4cd0 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -1464,9 +1464,9 @@ func TestGetAddress(t *testing.T) { Driver *cstructs.DriverNetwork // Results - IP string - Port int - ErrContains string + ExpectedIP string + ExpectedPort int + ExpectedErr string }{ { Name: "ExampleService", @@ -1477,8 +1477,8 @@ func TestGetAddress(t *testing.T) { PortMap: map[string]int{"db": 6379}, IP: "10.1.2.3", }, - IP: HostIP, - Port: 12435, + ExpectedIP: HostIP, + ExpectedPort: 12435, }, { Name: "Host", @@ -1489,8 +1489,8 @@ func TestGetAddress(t *testing.T) { PortMap: map[string]int{"db": 6379}, IP: "10.1.2.3", }, - IP: HostIP, - Port: 12345, + ExpectedIP: HostIP, + ExpectedPort: 12345, }, { Name: "Driver", @@ -1501,8 +1501,8 @@ func TestGetAddress(t *testing.T) { PortMap: map[string]int{"db": 6379}, IP: "10.1.2.3", }, - IP: "10.1.2.3", - Port: 6379, + ExpectedIP: "10.1.2.3", + ExpectedPort: 6379, }, { Name: "AutoDriver", @@ -1514,8 +1514,8 @@ func TestGetAddress(t *testing.T) { IP: "10.1.2.3", AutoAdvertise: true, }, - IP: "10.1.2.3", - Port: 6379, + ExpectedIP: "10.1.2.3", + ExpectedPort: 6379, }, { Name: "DriverCustomPort", @@ -1526,8 +1526,8 @@ func TestGetAddress(t *testing.T) { PortMap: map[string]int{"db": 6379}, IP: "10.1.2.3", }, - IP: "10.1.2.3", - Port: 7890, + ExpectedIP: "10.1.2.3", + ExpectedPort: 7890, }, { Name: "DriverWithoutNetwork", @@ -1535,7 +1535,7 @@ func TestGetAddress(t *testing.T) { PortLabel: "db", Host: map[string]int{"db": 12345}, Driver: nil, - ErrContains: "no driver network exists", + ExpectedErr: "no driver network exists", }, { Name: "DriverBadPort", @@ -1546,7 +1546,7 @@ func TestGetAddress(t *testing.T) { PortMap: map[string]int{"db": 6379}, IP: "10.1.2.3", }, - ErrContains: "invalid port", + ExpectedErr: "invalid port", }, { Name: "DriverZeroPort", @@ -1555,23 +1555,37 @@ func TestGetAddress(t *testing.T) { Driver: &cstructs.DriverNetwork{ IP: "10.1.2.3", }, - ErrContains: "invalid port", + ExpectedErr: "invalid port", }, { Name: "HostBadPort", Mode: structs.AddressModeHost, PortLabel: "bad-port-label", - ErrContains: "invalid port", + ExpectedErr: "invalid port", }, { Name: "InvalidMode", Mode: "invalid-mode", PortLabel: "80", - ErrContains: "invalid address mode", + ExpectedErr: "invalid address mode", }, { - Name: "EmptyIsOk", - Mode: structs.AddressModeHost, + Name: "NoPort_AutoMode", + Mode: structs.AddressModeHost, + ExpectedIP: HostIP, + }, + { + Name: "NoPort_HostMode", + Mode: structs.AddressModeHost, + ExpectedIP: HostIP, + }, + { + Name: "NoPort_DriverMode", + Mode: structs.AddressModeDriver, + Driver: &cstructs.DriverNetwork{ + IP: "10.1.2.3", + }, + ExpectedIP: "10.1.2.3", }, } @@ -1596,15 +1610,15 @@ func TestGetAddress(t *testing.T) { ip, port, err := getAddress(tc.Mode, tc.PortLabel, networks, tc.Driver) // Assert the results - assert.Equal(t, tc.IP, ip, "IP mismatch") - assert.Equal(t, tc.Port, port, "Port mismatch") - if tc.ErrContains == "" { + assert.Equal(t, tc.ExpectedIP, ip, "IP mismatch") + assert.Equal(t, tc.ExpectedPort, port, "Port mismatch") + if tc.ExpectedErr == "" { assert.Nil(t, err) } else { if err == nil { - t.Fatalf("expected error containing %q but err=nil", tc.ErrContains) + t.Fatalf("expected error containing %q but err=nil", tc.ExpectedErr) } else { - assert.Contains(t, err.Error(), tc.ErrContains) + assert.Contains(t, err.Error(), tc.ExpectedErr) } } }) From e0a3687faaccada308eb30dca9624d796fdfa978 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 20 Dec 2017 15:59:47 -0800 Subject: [PATCH 2/6] Fix test --- command/agent/consul/unit_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index fa8b52ab4cd0..05b1c0f8d313 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -1571,7 +1571,7 @@ func TestGetAddress(t *testing.T) { }, { Name: "NoPort_AutoMode", - Mode: structs.AddressModeHost, + Mode: structs.AddressModeAuto, ExpectedIP: HostIP, }, { From 6d77215f371569dce06196ba921656124df7a47e Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 20 Dec 2017 16:18:49 -0800 Subject: [PATCH 3/6] Improve driver network logging --- client/driver/docker.go | 8 -------- client/task_runner.go | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/client/driver/docker.go b/client/driver/docker.go index 81cffb448917..df6ee0fcacff 100644 --- a/client/driver/docker.go +++ b/client/driver/docker.go @@ -820,9 +820,6 @@ func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (*StartRespon // Detect container address ip, autoUse := d.detectIP(container) - if ip == "" { - d.logger.Printf("[DEBUG] driver.docker: task %s could not detect a container IP", d.taskName) - } // Create a response with the driver handle and container network metadata resp := &StartResponse{ @@ -863,11 +860,6 @@ func (d *DockerDriver) detectIP(c *docker.Container) (string, bool) { // Linux, nat on Windows) if name != "bridge" && name != "nat" { auto = true - d.logger.Printf("[INFO] driver.docker: task %s auto-advertising detected IP %s on network %q", - d.taskName, ip, name) - } else { - d.logger.Printf("[DEBUG] driver.docker task %s detect IP %s on network %q but not auto-advertising", - d.taskName, ip, name) } break diff --git a/client/task_runner.go b/client/task_runner.go index e01cc34cdd62..8befef0c3274 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -1440,6 +1440,21 @@ func (r *TaskRunner) startTask() error { } + // Log driver network information + if sresp.Network != nil && sresp.Network.IP != "" { + if sresp.Network.AutoAdvertise { + r.logger.Printf("[INFO] client: alloc %s task %s auto-advertising detected IP %s", + r.alloc.ID, r.task.Name, sresp.Network.IP) + } else { + r.logger.Printf("[DEBUG] client: alloc %s task %s detected IP %s but not auto-advertising", + r.alloc.ID, r.task.Name, sresp.Network.IP) + } + } + + if sresp.Network == nil || sresp.Network.IP == "" { + r.logger.Printf("[DEBUG] client: alloc %s task %s could not detect a driver IP", r.alloc.ID, r.task.Name) + } + // Update environment with the network defined by the driver's Start method. r.envBuilder.SetDriverNetwork(sresp.Network) From 7ae22915723db7ccd29bf0fc22bf93dd2f2b803a Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 12 Jan 2018 15:32:51 -0800 Subject: [PATCH 4/6] Improve invalid port error message for services Related to #3681 If a user specifies an invalid port *label* when using address_mode=driver they'll get an error message about the label being an invalid number which is very confusing. I also added a bunch of testing around Service.AddressMode validation since I was concerned by the linked issue that there were cases I was missing. Unfortunately when address_mode=driver is used there's only so much validation that can be done as structs/structs.go validation never peeks into the driver config which would be needed to verify the port labels/map. --- command/agent/consul/client.go | 7 +- command/agent/consul/unit_test.go | 3 + nomad/structs/structs_test.go | 116 ++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 2 deletions(-) diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go index 9955d7e99e1a..93547c26eea9 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 05b1c0f8d313..dd6b3477b853 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 c4e52038fe99..87d820ca332d 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{ From 7dd5dbdd56afad209f88806fecc1199ce47f33c8 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 16 Jan 2018 15:24:14 -0800 Subject: [PATCH 5/6] Drop log level to TRACE For people not using driver networks these log lines would just be confusing. --- client/task_runner.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/task_runner.go b/client/task_runner.go index 8befef0c3274..9d45d91f7f6a 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -1446,13 +1446,13 @@ func (r *TaskRunner) startTask() error { r.logger.Printf("[INFO] client: alloc %s task %s auto-advertising detected IP %s", r.alloc.ID, r.task.Name, sresp.Network.IP) } else { - r.logger.Printf("[DEBUG] client: alloc %s task %s detected IP %s but not auto-advertising", + r.logger.Printf("[TRACE] client: alloc %s task %s detected IP %s but not auto-advertising", r.alloc.ID, r.task.Name, sresp.Network.IP) } } if sresp.Network == nil || sresp.Network.IP == "" { - r.logger.Printf("[DEBUG] client: alloc %s task %s could not detect a driver IP", r.alloc.ID, r.task.Name) + r.logger.Printf("[TRACE] client: alloc %s task %s could not detect a driver IP", r.alloc.ID, r.task.Name) } // Update environment with the network defined by the driver's Start method. From c624e5b06b25b6b6d6f62f31d061d0ff0c6f1d5f Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 16 Jan 2018 15:26:10 -0800 Subject: [PATCH 6/6] Add changelog entry for #3682 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f9f7f562fe2..054c977336bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ BUG FIXES: * core: Fix an issue in which batch jobs with queued placements and lost allocations could result in improper placement counts [[GH-3717](https://github.com/hashicorp/nomad/issues/3717)] * client: Migrated ephemeral_disk's maintain directory permissions [[GH-3723](https://github.com/hashicorp/nomad/issues/3723)] + * client: Always advertise driver IP when in driver address mode [[GH-3682](https://github.com/hashicorp/nomad/issues/3682)] * client/vault: Recognize renewing non-renewable Vault lease as fatal [[GH-3727](https://github.com/hashicorp/nomad/issues/3727)] * config: Revert minimum CPU limit back to 20 from 100. * ui: Fix ui on non-leaders when ACLs are enabled [[GH-3722](https://github.com/hashicorp/nomad/issues/3722)]