Merge pull request #3619 from hashicorp/f-3380-custom-ports

Allow custom ports for services and checks when using driver address_mode

CHANGELOG.md

@@ -1,6 +1,9 @@
 ## 0.7.1 (Unreleased)
 
 __BACKWARDS INCOMPATIBILITIES:__
+ * client: The format of service IDs in Consul has changed. If you rely upon
+   Nomad's service IDs (*not* service names; those are stable), you will need
+   to update your code.  [GH-3632]
  * config: Nomad no longer parses Atlas configuration stanzas. Atlas has been
    deprecated since earlier this year. If you have an Atlas stanza in your
    config file it will have to be removed.
@@ -22,10 +25,13 @@ IMPROVEMENTS:
  * api: Environment variables are ignored during service name validation [GH-3532]
  * cli: Allocation create and modify times are displayed in a human readable
    relative format like `6 h ago` [GH-3449]
+ * client: Support `address_mode` on checks [GH-3619]
  * client: Sticky volume migrations are now atomic. [GH-3563]
  * client: Added metrics to track state transitions of allocations [GH-3061]
  * client: When `network_interface` is unspecified use interface attached to
    default route [GH-3546]
+ * client: Support numeric ports on services and checks when
+   `address_mode="driver"` [GH-3619]
  * driver/docker: Detect OOM kill event [GH-3459]
  * driver/docker: Adds support for adding host device to container via
    `--device` [GH-2938]
@@ -54,9 +60,13 @@ BUG FIXES:
    explicitly [GH-3520]
  * cli: Fix passing Consul address via flags [GH-3504]
  * cli: Fix panic when running `keyring` commands [GH-3509]
+ * client: Fix advertising services with tags that require URL escaping
+   [GH-3632]
  * client: Fix a panic when restoring an allocation with a dead leader task
    [GH-3502]
  * client: Fix crash when following logs from a Windows node [GH-3608]
+ * client: Fix service/check updating when just interpolated variables change
+   [GH-3619]
  * client: Fix allocation accounting in GC and trigger GCs on allocation
    updates [GH-3445]
  * driver/rkt: Remove pods on shutdown [GH-3562]

api/tasks.go

@@ -154,6 +154,7 @@ type ServiceCheck struct {
 	Path          string
 	Protocol      string
 	PortLabel     string `mapstructure:"port"`
+	AddressMode   string `mapstructure:"address_mode"`
 	Interval      time.Duration
 	Timeout       time.Duration
 	InitialStatus string `mapstructure:"initial_status"`

client/driver/driver_test.go

@@ -209,12 +209,12 @@ func setupTaskEnv(t *testing.T, driver string) (*allocdir.TaskDir, map[string]st
 		"NOMAD_PORT_two":                "443",
 		"NOMAD_HOST_PORT_two":           "443",
 		"NOMAD_ADDR_admin":              "1.2.3.4:8081",
-		"NOMAD_ADDR_web_main":           "192.168.0.100:5000",
+		"NOMAD_ADDR_web_admin":          "192.168.0.100:5000",
 		"NOMAD_ADDR_web_http":           "192.168.0.100:2000",
-		"NOMAD_IP_web_main":             "192.168.0.100",
+		"NOMAD_IP_web_admin":            "192.168.0.100",
 		"NOMAD_IP_web_http":             "192.168.0.100",
 		"NOMAD_PORT_web_http":           "2000",
-		"NOMAD_PORT_web_main":           "5000",
+		"NOMAD_PORT_web_admin":          "5000",
 		"NOMAD_IP_admin":                "1.2.3.4",
 		"NOMAD_PORT_admin":              "8081",
 		"NOMAD_HOST_PORT_admin":         "8081",

client/driver/mock_driver.go

@@ -10,6 +10,7 @@ import (
 	"log"
 	"os"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/mitchellh/mapstructure"
@@ -61,6 +62,17 @@ type MockDriverConfig struct {
 
 	// SignalErr is the error message that the task returns if signalled
 	SignalErr string `mapstructure:"signal_error"`
+
+	// DriverIP will be returned as the DriverNetwork.IP from Start()
+	DriverIP string `mapstructure:"driver_ip"`
+
+	// DriverAdvertise will be returned as DriverNetwork.AutoAdvertise from
+	// Start().
+	DriverAdvertise bool `mapstructure:"driver_advertise"`
+
+	// DriverPortMap will parse a label:number pair and return it in
+	// DriverNetwork.PortMap from Start().
+	DriverPortMap string `mapstructure:"driver_port_map"`
 }
 
 // MockDriver is a driver which is used for testing purposes
@@ -114,6 +126,23 @@ func (m *MockDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse
 		return nil, structs.NewRecoverableError(errors.New(driverConfig.StartErr), driverConfig.StartErrRecoverable)
 	}
 
+	// Create the driver network
+	net := &cstructs.DriverNetwork{
+		IP:            driverConfig.DriverIP,
+		AutoAdvertise: driverConfig.DriverAdvertise,
+	}
+	if raw := driverConfig.DriverPortMap; len(raw) > 0 {
+		parts := strings.Split(raw, ":")
+		if len(parts) != 2 {
+			return nil, fmt.Errorf("malformed port map: %q", raw)
+		}
+		port, err := strconv.Atoi(parts[1])
+		if err != nil {
+			return nil, fmt.Errorf("malformed port map: %q -- error: %v", raw, err)
+		}
+		net.PortMap = map[string]int{parts[0]: port}
+	}
+
 	h := mockDriverHandle{
 		taskName:    task.Name,
 		runFor:      driverConfig.RunFor,
@@ -133,7 +162,8 @@ func (m *MockDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse
 	}
 	m.logger.Printf("[DEBUG] driver.mock: starting task %q", task.Name)
 	go h.run()
-	return &StartResponse{Handle: &h}, nil
+
+	return &StartResponse{Handle: &h, Network: net}, nil
 }
 
 // Cleanup deletes all keys except for Config.Options["cleanup_fail_on"] for
@@ -265,10 +295,20 @@ func (h *mockDriverHandle) Kill() error {
 	select {
 	case <-h.doneCh:
 	case <-time.After(h.killAfter):
-		close(h.doneCh)
+		select {
+		case <-h.doneCh:
+			// already closed
+		default:
+			close(h.doneCh)
+		}
 	case <-time.After(h.killTimeout):
 		h.logger.Printf("[DEBUG] driver.mock: terminating task %q", h.taskName)
-		close(h.doneCh)
+		select {
+		case <-h.doneCh:
+			// already closed
+		default:
+			close(h.doneCh)
+		}
 	}
 	return nil
 }
@@ -286,7 +326,12 @@ func (h *mockDriverHandle) run() {
 	for {
 		select {
 		case <-timer.C:
-			close(h.doneCh)
+			select {
+			case <-h.doneCh:
+				// already closed
+			default:
+				close(h.doneCh)
+			}
 		case <-h.doneCh:
 			h.logger.Printf("[DEBUG] driver.mock: finished running task %q", h.taskName)
 			h.waitCh <- dstructs.NewWaitResult(h.exitCode, h.exitSignal, h.exitErr)

client/task_runner.go

@@ -1631,7 +1631,12 @@ func (r *TaskRunner) handleUpdate(update *structs.Allocation) error {
 	// Merge in the task resources
 	updatedTask.Resources = update.TaskResources[updatedTask.Name]
 
-	// Update the task's environment for interpolating in services/checks
+	// Interpolate the old task with the old env before updating the env as
+	// updating services in Consul need both the old and new interpolations
+	// to find differences.
+	oldInterpolatedTask := interpolateServices(r.envBuilder.Build(), r.task)
+
+	// Now it's safe to update the environment
 	r.envBuilder.UpdateTask(update, updatedTask)
 
 	var mErr multierror.Error
@@ -1650,7 +1655,8 @@ func (r *TaskRunner) handleUpdate(update *structs.Allocation) error {
 		}
 
 		// Update services in Consul
-		if err := r.updateServices(drv, r.handle, r.task, updatedTask); err != nil {
+		newInterpolatedTask := interpolateServices(r.envBuilder.Build(), updatedTask)
+		if err := r.updateServices(drv, r.handle, oldInterpolatedTask, newInterpolatedTask); err != nil {
 			mErr.Errors = append(mErr.Errors, fmt.Errorf("error updating services and checks in Consul: %v", err))
 		}
 	}
@@ -1667,19 +1673,17 @@ func (r *TaskRunner) handleUpdate(update *structs.Allocation) error {
 	return mErr.ErrorOrNil()
 }
 
-// updateServices and checks with Consul.
-func (r *TaskRunner) updateServices(d driver.Driver, h driver.ScriptExecutor, old, new *structs.Task) error {
+// updateServices and checks with Consul. Tasks must be interpolated!
+func (r *TaskRunner) updateServices(d driver.Driver, h driver.ScriptExecutor, oldTask, newTask *structs.Task) error {
 	var exec driver.ScriptExecutor
 	if d.Abilities().Exec {
 		// Allow set the script executor if the driver supports it
 		exec = h
 	}
-	newInterpolatedTask := interpolateServices(r.envBuilder.Build(), new)
-	oldInterpolatedTask := interpolateServices(r.envBuilder.Build(), old)
 	r.driverNetLock.Lock()
 	net := r.driverNet.Copy()
 	r.driverNetLock.Unlock()
-	return r.consul.UpdateTask(r.alloc.ID, oldInterpolatedTask, newInterpolatedTask, r, exec, net)
+	return r.consul.UpdateTask(r.alloc.ID, oldTask, newTask, r, exec, net)
 }
 
 // handleDestroy kills the task handle. In the case that killing fails,