Merge pull request #3674 from hashicorp/b-3673-accept-empty-port

Services should not require a port

command/agent/consul/client.go

@@ -663,7 +663,7 @@ func (c *ServiceClient) checkRegs(ops *operations, allocID, serviceID string, se
 
 		ip, port, err := getAddress(addrMode, portLabel, task.Resources.Networks, net)
 		if err != nil {
-			return nil, fmt.Errorf("unable to get address for check %q: %v", check.Name, err)
+			return nil, fmt.Errorf("error getting address for check %q: %v", check.Name, err)
 		}
 
 		checkReg, err := createCheckReg(serviceID, checkID, check, ip, port)
@@ -1036,6 +1036,11 @@ func createCheckReg(serviceID, checkID string, check *structs.ServiceCheck, host
 	chkReg.Timeout = check.Timeout.String()
 	chkReg.Interval = check.Interval.String()
 
+	// Require an address for http or tcp checks
+	if port == 0 && check.RequiresPort() {
+		return nil, fmt.Errorf("%s checks require an address", check.Type)
+	}
+
 	switch check.Type {
 	case structs.ServiceCheckHTTP:
 		proto := check.Protocol
@@ -1089,9 +1094,15 @@ func isOldNomadService(id string) bool {
 	return strings.HasPrefix(id, prefix)
 }
 
-// getAddress returns the ip and port to use for a service or check. An error
-// is returned if an ip and port cannot be determined.
+// getAddress returns the IP and port to use for a service or check. If no port
+// 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() {

command/agent/consul/int_test.go

@@ -87,8 +87,17 @@ func TestConsul_Integration(t *testing.T) {
 	task.Config = map[string]interface{}{
 		"run_for": "1h",
 	}
+
 	// Choose a port that shouldn't be in use
-	task.Resources.Networks[0].ReservedPorts = []structs.Port{{Label: "http", Value: 3}}
+	netResource := &structs.NetworkResource{
+		Device:        "eth0",
+		IP:            "127.0.0.1",
+		MBits:         50,
+		ReservedPorts: []structs.Port{{Label: "http", Value: 3}},
+	}
+	alloc.Resources.Networks[0] = netResource
+	alloc.TaskResources["web"].Networks[0] = netResource
+	task.Resources.Networks[0] = netResource
 	task.Services = []*structs.Service{
 		{
 			Name:      "httpd",
@@ -96,13 +105,12 @@ func TestConsul_Integration(t *testing.T) {
 			Tags:      []string{"nomad", "test", "http"},
 			Checks: []*structs.ServiceCheck{
 				{
-					Name:      "httpd-http-check",
-					Type:      "http",
-					Path:      "/",
-					Protocol:  "http",
-					PortLabel: "http",
-					Interval:  9000 * time.Hour,
-					Timeout:   1, // fail as fast as possible
+					Name:     "httpd-http-check",
+					Type:     "http",
+					Path:     "/",
+					Protocol: "http",
+					Interval: 9000 * time.Hour,
+					Timeout:  1, // fail as fast as possible
 				},
 				{
 					Name:     "httpd-script-check",

command/agent/consul/unit_test.go

@@ -1566,8 +1566,13 @@ func TestGetAddress(t *testing.T) {
 		{
 			Name:        "InvalidMode",
 			Mode:        "invalid-mode",
+			PortLabel:   "80",
 			ErrContains: "invalid address mode",
 		},
+		{
+			Name: "EmptyIsOk",
+			Mode: structs.AddressModeHost,
+		},
 	}
 
 	for _, tc := range cases {

nomad/fsm_test.go

@@ -944,9 +944,14 @@ func TestFSM_UpsertAllocs_StrippedResources(t *testing.T) {
 	fsm := testFSM(t)
 
 	alloc := mock.Alloc()
+
+	// Need to remove mock dynamic port from alloc as it won't be computed
+	// in this test
+	alloc.TaskResources["web"].Networks[0].DynamicPorts[0].Value = 0
+
 	fsm.State().UpsertJobSummary(1, mock.JobSummary(alloc.JobID))
 	job := alloc.Job
-	resources := alloc.Resources
+	origResources := alloc.Resources
 	alloc.Resources = nil
 	req := structs.AllocUpdateRequest{
 		Job:   job,
@@ -973,10 +978,10 @@ func TestFSM_UpsertAllocs_StrippedResources(t *testing.T) {
 	alloc.AllocModifyIndex = out.AllocModifyIndex
 
 	// Resources should be recomputed
-	resources.DiskMB = alloc.Job.TaskGroups[0].EphemeralDisk.SizeMB
-	alloc.Resources = resources
+	origResources.DiskMB = alloc.Job.TaskGroups[0].EphemeralDisk.SizeMB
+	alloc.Resources = origResources
 	if !reflect.DeepEqual(alloc, out) {
-		t.Fatalf("bad: %#v %#v", alloc, out)
+		t.Fatalf("not equal: % #v", pretty.Diff(alloc, out))
 	}
 }
 

nomad/mock/mock.go

@@ -292,7 +292,7 @@ func Alloc() *structs.Allocation {
 						IP:            "192.168.0.100",
 						ReservedPorts: []structs.Port{{Label: "admin", Value: 5000}},
 						MBits:         50,
-						DynamicPorts:  []structs.Port{{Label: "http"}},
+						DynamicPorts:  []structs.Port{{Label: "http", Value: 9876}},
 					},
 				},
 			},

nomad/structs/structs.go

@@ -3131,8 +3131,8 @@ func (s *Service) Validate() error {
 	}
 
 	for _, c := range s.Checks {
-		if s.PortLabel == "" && c.RequiresPort() {
-			mErr.Errors = append(mErr.Errors, fmt.Errorf("check %s invalid: check requires a port but the service %+q has no port", c.Name, s.Name))
+		if s.PortLabel == "" && c.PortLabel == "" && c.RequiresPort() {
+			mErr.Errors = append(mErr.Errors, fmt.Errorf("check %s invalid: check requires a port but neither check nor service %+q have a port", c.Name, s.Name))
 			continue
 		}
 
@@ -3577,8 +3577,16 @@ func validateServices(t *Task) error {
 		}
 	}
 
+	// Iterate over a sorted list of keys to make error listings stable
+	keys := make([]string, 0, len(servicePorts))
+	for p := range servicePorts {
+		keys = append(keys, p)
+	}
+	sort.Strings(keys)
+
 	// Ensure all ports referenced in services exist.
-	for servicePort, services := range servicePorts {
+	for _, servicePort := range keys {
+		services := servicePorts[servicePort]
 		_, ok := portLabels[servicePort]
 		if !ok {
 			names := make([]string, 0, len(services))

nomad/structs/structs_test.go

@@ -1235,55 +1235,93 @@ func TestTask_Validate_Service_Check(t *testing.T) {
 // TestTask_Validate_Service_Check_AddressMode asserts that checks do not
 // inherit address mode but do inherit ports.
 func TestTask_Validate_Service_Check_AddressMode(t *testing.T) {
-	task := &Task{
-		Resources: &Resources{
-			Networks: []*NetworkResource{
-				{
-					DynamicPorts: []Port{
-						{
-							Label: "http",
-							Value: 9999,
+	getTask := func(s *Service) *Task {
+		return &Task{
+			Resources: &Resources{
+				Networks: []*NetworkResource{
+					{
+						DynamicPorts: []Port{
+							{
+								Label: "http",
+								Value: 9999,
+							},
 						},
 					},
 				},
 			},
-		},
-		Services: []*Service{
-			{
+			Services: []*Service{s},
+		}
+	}
+
+	cases := []struct {
+		Service     *Service
+		ErrContains string
+	}{
+		{
+			Service: &Service{
 				Name:        "invalid-driver",
 				PortLabel:   "80",
 				AddressMode: "host",
 			},
-			{
-				Name:        "http-driver",
+			ErrContains: `port label "80" referenced`,
+		},
+		{
+			Service: &Service{
+				Name:        "http-driver-fail-1",
 				PortLabel:   "80",
 				AddressMode: "driver",
 				Checks: []*ServiceCheck{
 					{
-						// Should fail
 						Name:     "invalid-check-1",
 						Type:     "tcp",
 						Interval: time.Second,
 						Timeout:  time.Second,
 					},
+				},
+			},
+			ErrContains: `check "invalid-check-1" cannot use a numeric port`,
+		},
+		{
+			Service: &Service{
+				Name:        "http-driver-fail-2",
+				PortLabel:   "80",
+				AddressMode: "driver",
+				Checks: []*ServiceCheck{
 					{
-						// Should fail
 						Name:      "invalid-check-2",
 						Type:      "tcp",
 						PortLabel: "80",
 						Interval:  time.Second,
 						Timeout:   time.Second,
 					},
+				},
+			},
+			ErrContains: `check "invalid-check-2" cannot use a numeric port`,
+		},
+		{
+			Service: &Service{
+				Name:        "http-driver-fail-3",
+				PortLabel:   "80",
+				AddressMode: "driver",
+				Checks: []*ServiceCheck{
 					{
-						// Should fail
 						Name:      "invalid-check-3",
 						Type:      "tcp",
 						PortLabel: "missing-port-label",
 						Interval:  time.Second,
 						Timeout:   time.Second,
 					},
+				},
+			},
+			ErrContains: `port label "missing-port-label" referenced`,
+		},
+		{
+			Service: &Service{
+				Name:        "http-driver-passes",
+				PortLabel:   "80",
+				AddressMode: "driver",
+				Checks: []*ServiceCheck{
 					{
-						// Should pass
 						Name:     "valid-script-check",
 						Type:     "script",
 						Command:  "ok",
@@ -1291,7 +1329,6 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) {
 						Timeout:  time.Second,
 					},
 					{
-						// Should pass
 						Name:      "valid-host-check",
 						Type:      "tcp",
 						PortLabel: "http",
@@ -1299,7 +1336,6 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) {
 						Timeout:   time.Second,
 					},
 					{
-						// Should pass
 						Name:        "valid-driver-check",
 						Type:        "tcp",
 						AddressMode: "driver",
@@ -1309,23 +1345,65 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) {
 				},
 			},
 		},
-	}
-	err := validateServices(task)
-	if err == nil {
-		t.Fatalf("expected errors but task validated successfully")
-	}
-	errs := err.(*multierror.Error).Errors
-	if expected := 4; len(errs) != expected {
-		for i, err := range errs {
-			t.Logf("errs[%d] -> %s", i, err)
-		}
-		t.Fatalf("expected %d errors but found %d", expected, len(errs))
+		{
+			Service: &Service{
+				Name: "empty-address-3673-passes-1",
+				Checks: []*ServiceCheck{
+					{
+						Name:      "valid-port-label",
+						Type:      "tcp",
+						PortLabel: "http",
+						Interval:  time.Second,
+						Timeout:   time.Second,
+					},
+					{
+						Name:     "empty-is-ok",
+						Type:     "script",
+						Command:  "ok",
+						Interval: time.Second,
+						Timeout:  time.Second,
+					},
+				},
+			},
+		},
+		{
+			Service: &Service{
+				Name: "empty-address-3673-passes-2",
+			},
+		},
+		{
+			Service: &Service{
+				Name: "empty-address-3673-fails",
+				Checks: []*ServiceCheck{
+					{
+						Name:     "empty-is-not-ok",
+						Type:     "tcp",
+						Interval: time.Second,
+						Timeout:  time.Second,
+					},
+				},
+			},
+			ErrContains: `invalid: check requires a port but neither check nor service`,
+		},
 	}
 
-	assert.Contains(t, errs[0].Error(), `check "invalid-check-1" cannot use a numeric port`)
-	assert.Contains(t, errs[1].Error(), `check "invalid-check-2" cannot use a numeric port`)
-	assert.Contains(t, errs[2].Error(), `port label "80" referenced`)
-	assert.Contains(t, errs[3].Error(), `port label "missing-port-label" referenced`)
+	for _, tc := range cases {
+		tc := tc
+		task := getTask(tc.Service)
+		t.Run(tc.Service.Name, func(t *testing.T) {
+			err := validateServices(task)
+			if err == nil && tc.ErrContains == "" {
+				// Ok!
+				return
+			}
+			if err == nil {
+				t.Fatalf("no error returned. expected: %s", tc.ErrContains)
+			}
+			if !strings.Contains(err.Error(), tc.ErrContains) {
+				t.Fatalf("expected %q but found: %v", tc.ErrContains, err)
+			}
+		})
+	}
 }
 
 func TestTask_Validate_Service_Check_CheckRestart(t *testing.T) {

website/source/api/json-jobs.html.md

@@ -300,13 +300,17 @@ The `Task` object supports the following keys:
     }
     ```
 
+- `KillSignal` - Specifies a configurable kill signal for a task, where the
+  default is SIGINT. Note that this is only supported for drivers which accept
+  sending signals (currently `docker`, `exec`, `raw_exec`, and `java` drivers).
+
 - `KillTimeout` - `KillTimeout` is a time duration in nanoseconds. It can be
   used to configure the time between signaling a task it will be killed and
   actually killing it. Drivers first sends a task the `SIGINT` signal and then
   sends `SIGTERM` if the task doesn't die after the `KillTimeout` duration has
   elapsed. The default `KillTimeout` is 5 seconds.
 
-- `leader` - Specifies whether the task is the leader task of the task group. If
+- `Leader` - Specifies whether the task is the leader task of the task group. If
   set to true, when the leader task completes, all other tasks within the task
   group will be gracefully shutdown.
 
@@ -346,6 +350,23 @@ The `Task` object supports the following keys:
        defined in the resources block.  This could be a label of either a
        dynamic or a static port.
 
+     - `AddressMode`: Specifies what address (host or driver-specific) this
+       service should advertise.  This setting is supported in Docker since
+       Nomad 0.6 and rkt since Nomad 0.7. Valid options are:
+
+       - `auto` - Allows the driver to determine whether the host or driver
+         address should be used. Defaults to `host` and only implemented by
+	 Docker. If you use a Docker network plugin such as weave, Docker will
+         automatically use its address.
+
+       - `driver` - Use the IP specified by the driver, and the port specified
+         in a port map. A numeric port may be specified since port maps aren't
+	 required by all network plugins. Useful for advertising SDN and
+         overlay network addresses. Task will fail if driver network cannot be
+         determined. Only implemented for Docker and rkt.
+
+       - `host` - Use the host IP and port.
+
      - `Checks`: `Checks` is an array of check objects. A check object defines a
        health check associated with the service. Nomad supports the `script`,
        `http` and `tcp` Consul Checks. Script checks are not supported for the
@@ -357,6 +378,25 @@ The `Task` object supports the following keys:
 
          - `Name`: The name of the health check.
 
+	 - `AddressMode`: Same as `AddressMode` on `Service`.  Unlike services,
+	   checks do not have an `auto` address mode as there's no way for
+	   Nomad to know which is the best address to use for checks. Consul
+	   needs access to the address for any HTTP or TCP checks. Added in
+	   Nomad 0.7.1. Unlike `PortLabel`, this setting is *not* inherited
+           from the `Service`.
+
+	 - `PortLabel`: Specifies the label of the port on which the check will
+	   be performed. Note this is the _label_ of the port and not the port
+	   number unless `AddressMode: "driver"`. The port label must match one
+	   defined in the Network stanza. If a port value was declared on the
+	   `Service`, this will inherit from that value if not supplied. If
+	   supplied, this value takes precedence over the `Service.PortLabel`
+	   value. This is useful for services which operate on multiple ports.
+	  `http` and `tcp` checks require a port while `script` checks do not.
+	  Checks will use the host IP and ports by default. In Nomad 0.7.1 or
+	  later numeric ports may be used if `AddressMode: "driver"` is set on
+          the check.
+
 	 - `Header`: Headers for HTTP checks. Should be an object where the
 	   values are an array of values. Headers will be written once for each
            value.

website/source/docs/job-specification/service.html.md

@@ -96,7 +96,7 @@ does not automatically enable service discovery.
     interpolated and revalidated. This can cause certain service names to pass validation at submit time but fail 
     at runtime.
     
-- `port` `(string: <required>)` - Specifies the label of the port on which this
+- `port` `(string: <optional>)` - Specifies the label of the port on which this
   service is running. Note this is the _label_ of the port and not the port
   number unless `address_mode = driver`. The port label must match one defined
    in the [`network`][network] stanza unless you're also using
@@ -174,14 +174,15 @@ scripts.
   add the IP of the service and the port, so this is just the relative URL to
   the health check endpoint. This is required for http-based health checks.
 
-- `port` `(string: <required>)` - Specifies the label of the port on which the
+- `port` `(string: <varies>)` - Specifies the label of the port on which the
   check will be performed. Note this is the _label_ of the port and not the port
   number unless `address_mode = driver`. The port label must match one defined
   in the [`network`][network] stanza. If a port value was declared on the
   `service`, this will inherit from that value if not supplied. If supplied,
   this value takes precedence over the `service.port` value. This is useful for
-  services which operate on multiple ports. Checks will use the host IP and
-  ports by default. In Nomad 0.7.1 or later numeric ports may be used if
+  services which operate on multiple ports. `http` and `tcp` checks require a
+  port while `script` checks do not. Checks will use the host IP and ports by
+  default. In Nomad 0.7.1 or later numeric ports may be used if
   `address_mode="driver"` is set on the check.
 
 - `protocol` `(string: "http")` - Specifies the protocol for the http-based

website/source/docs/job-specification/task.html.md

@@ -56,8 +56,7 @@ job "docs" {
 
 - `kill_signal` `(string)` - Specifies a configurable kill signal for a task,
   where the default is SIGINT. Note that this is only supported for drivers
-  which accept sending signals (currently Docker, exec, raw_exec, and Java
-  drivers).
+  sending signals (currently `docker`, `exec`, `raw_exec`, and `java` drivers).
 
 - `leader` `(bool: false)` - Specifies whether the task is the leader task of
   the task group. If set to true, when the leader task completes, all other