Services should not require a port

Fixes #3673

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
+// getAddress returns the IP and port to use for a service or check. If no port
-// is returned if an ip and port cannot be determined.
+// 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",
+					Name:     "httpd-http-check",
-					Type:      "http",
+					Type:     "http",
-					Path:      "/",
+					Path:     "/",
-					Protocol:  "http",
+					Protocol: "http",
-					PortLabel: "http",
+					Interval: 9000 * time.Hour,
-					Interval:  9000 * time.Hour,
+					Timeout:  1, // fail as fast as possible
-					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/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() {
+		if s.PortLabel == "" && c.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))
+			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{
+	getTask := func(s *Service) *Task {
-		Resources: &Resources{
+		return &Task{
-			Networks: []*NetworkResource{
+			Resources: &Resources{
-				{
+				Networks: []*NetworkResource{
-					DynamicPorts: []Port{
+					{
-						{
+						DynamicPorts: []Port{
-							Label: "http",
+							{
-							Value: 9999,
+								Label: "http",
+								Value: 9999,
+							},
 						},
 					},
 				},
 			},
-		},
+			Services: []*Service{s},
-		Services: []*Service{
+		}
-			{
+	}
+
+	cases := []struct {
+		Service     *Service
+		ErrContains string
+	}{
+		{
+			Service: &Service{
 				Name:        "invalid-driver",
 				PortLabel:   "80",
 				AddressMode: "host",
 			},
-			{
+			ErrContains: `port label "80" referenced`,
-				Name:        "http-driver",
+		},
+		{
+			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)
+			Service: &Service{
-	if err == nil {
+				Name: "empty-address-3673-passes-1",
-		t.Fatalf("expected errors but task validated successfully")
+				Checks: []*ServiceCheck{
-	}
+					{
-	errs := err.(*multierror.Error).Errors
+						Name:      "valid-port-label",
-	if expected := 4; len(errs) != expected {
+						Type:      "tcp",
-		for i, err := range errs {
+						PortLabel: "http",
-			t.Logf("errs[%d] -> %s", i, err)
+						Interval:  time.Second,
-		}
+						Timeout:   time.Second,
-		t.Fatalf("expected %d errors but found %d", expected, len(errs))
+					},
+					{
+						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`)
+	for _, tc := range cases {
-	assert.Contains(t, errs[1].Error(), `check "invalid-check-2" cannot use a numeric port`)
+		tc := tc
-	assert.Contains(t, errs[2].Error(), `port label "80" referenced`)
+		task := getTask(tc.Service)
-	assert.Contains(t, errs[3].Error(), `port label "missing-port-label" referenced`)
+		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/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
+  services which operate on multiple ports. `http` and `tcp` checks require a
-  ports by default. In Nomad 0.7.1 or later numeric ports may be used if
+  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