Merge pull request #3682 from hashicorp/b-3681-always-set-driver-ip

Always advertise driver IP when in driver mode

CHANGELOG.md

@@ -17,6 +17,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)]

client/driver/docker.go

@@ -866,7 +866,7 @@ func (d *DockerDriver) detectIP(c *docker.Container) (string, bool) {
 	}
 
 	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

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("[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("[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.
 	r.envBuilder.SetDriverNetwork(sresp.Network)
 

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
@@ -1133,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

command/agent/consul/unit_test.go

@@ -1464,10 +1464,11 @@ func TestGetAddress(t *testing.T) {
 		Driver    *cstructs.DriverNetwork
 
 		// Results
-		IP          string
-		Port        int
-		ErrContains string
+		ExpectedIP   string
+		ExpectedPort int
+		ExpectedErr  string
 	}{
+		// Valid Configurations
 		{
 			Name:      "ExampleService",
 			Mode:      structs.AddressModeAuto,
@@ -1477,8 +1478,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 +1490,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 +1502,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 +1515,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,16 +1527,18 @@ 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,
 		},
+
+		// Invalid Configurations
 		{
 			Name:        "DriverWithoutNetwork",
 			Mode:        structs.AddressModeDriver,
 			PortLabel:   "db",
 			Host:        map[string]int{"db": 12345},
 			Driver:      nil,
-			ErrContains: "no driver network exists",
+			ExpectedErr: "no driver network exists",
 		},
 		{
 			Name:      "DriverBadPort",
@@ -1546,7 +1549,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 +1558,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:       "NoPort_AutoMode",
+			Mode:       structs.AddressModeAuto,
+			ExpectedIP: HostIP,
+		},
+		{
+			Name:       "NoPort_HostMode",
+			Mode:       structs.AddressModeHost,
+			ExpectedIP: HostIP,
 		},
 		{
-			Name: "EmptyIsOk",
-			Mode: structs.AddressModeHost,
+			Name: "NoPort_DriverMode",
+			Mode: structs.AddressModeDriver,
+			Driver: &cstructs.DriverNetwork{
+				IP: "10.1.2.3",
+			},
+			ExpectedIP: "10.1.2.3",
 		},
 	}
 
@@ -1596,15 +1613,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)
 				}
 			}
 		})

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{