Merge pull request #1097 from hashicorp/service-no-port

Invalidating services when they contain check of type tcp and http bu…

nomad/structs/structs.go

@@ -1446,6 +1446,16 @@ func (sc *ServiceCheck) Validate() error {
 	return nil
 }
 
+// RequiresPort returns whether the service check requires the task has a port.
+func (sc *ServiceCheck) RequiresPort() bool {
+	switch sc.Type {
+	case ServiceCheckHTTP, ServiceCheckTCP:
+		return true
+	default:
+		return false
+	}
+}
+
 func (sc *ServiceCheck) Hash(serviceID string) string {
 	h := sha1.New()
 	io.WriteString(h, serviceID)
@@ -1527,6 +1537,10 @@ func (s *Service) Validate() error {
 	}
 
 	for _, c := range s.Checks {
+		if s.PortLabel == "" && c.RequiresPort() {
+			mErr.Errors = append(mErr.Errors, fmt.Errorf("check %q is not valid since service %q doesn't have port", c.Name, s.Name))
+			continue
+		}
 		if err := c.Validate(); err != nil {
 			mErr.Errors = append(mErr.Errors, err)
 		}
@@ -1722,10 +1736,9 @@ func (t *Task) Validate() error {
 		}
 	}
 
-	for _, service := range t.Services {
-		if err := service.Validate(); err != nil {
-			mErr.Errors = append(mErr.Errors, err)
-		}
+	// Validate Services
+	if err := validateServices(t); err != nil {
+		mErr.Errors = append(mErr.Errors, err)
 	}
 
 	if t.LogConfig != nil && t.Resources != nil {
@@ -1754,6 +1767,47 @@ func (t *Task) Validate() error {
 	return mErr.ErrorOrNil()
 }
 
+// validateServices takes a task and validates the services within it are valid
+// and reference ports that exist.
+func validateServices(t *Task) error {
+	var mErr multierror.Error
+
+	// Ensure that services don't ask for non-existent ports.
+	servicePorts := make(map[string][]string)
+	for i, service := range t.Services {
+		if err := service.Validate(); err != nil {
+			outer := fmt.Errorf("service %d validation failed: %s", i, err)
+			mErr.Errors = append(mErr.Errors, outer)
+		}
+
+		if service.PortLabel != "" {
+			servicePorts[service.PortLabel] = append(servicePorts[service.PortLabel], service.Name)
+		}
+	}
+
+	// Get the set of port labels.
+	portLabels := make(map[string]struct{})
+	if t.Resources != nil {
+		for _, network := range t.Resources.Networks {
+			ports := network.MapLabelToValues(nil)
+			for portLabel, _ := range ports {
+				portLabels[portLabel] = struct{}{}
+			}
+		}
+	}
+
+	// Ensure all ports referenced in services exist.
+	for servicePort, services := range servicePorts {
+		_, ok := portLabels[servicePort]
+		if !ok {
+			joined := strings.Join(services, ", ")
+			err := fmt.Errorf("port label %q referenced by services %v does not exist", servicePort, joined)
+			mErr.Errors = append(mErr.Errors, err)
+		}
+	}
+	return mErr.ErrorOrNil()
+}
+
 // Set of possible states for a task.
 const (
 	TaskStatePending = "pending" // The task is waiting to be run.

nomad/structs/structs_test.go

@@ -275,6 +275,38 @@ func TestTask_Validate(t *testing.T) {
 	}
 }
 
+func TestTask_Validate_Services(t *testing.T) {
+	s := &Service{
+		Name:      "service-name",
+		PortLabel: "bar",
+		Checks: []*ServiceCheck{
+			{
+				Name: "check-name",
+				Type: ServiceCheckTCP,
+			},
+		},
+	}
+
+	task := &Task{
+		Name:   "web",
+		Driver: "docker",
+		Resources: &Resources{
+			CPU:      100,
+			DiskMB:   200,
+			MemoryMB: 100,
+			IOPS:     10,
+		},
+		Services: []*Service{s},
+	}
+	err := task.Validate()
+	if err == nil {
+		t.Fatal("expected an error")
+	}
+	if !strings.Contains(err.Error(), "referenced by services service-name does not exist") {
+		t.Fatalf("err: %s", err)
+	}
+}
+
 func TestTask_Validate_LogConfig(t *testing.T) {
 	task := &Task{
 		LogConfig: DefaultLogConfig(),
@@ -518,7 +550,6 @@ func TestInvalidServiceCheck(t *testing.T) {
 		PortLabel: "bar",
 		Checks: []*ServiceCheck{
 			{
-
 				Name: "check-name",
 				Type: "lol",
 			},
@@ -551,6 +582,44 @@ func TestInvalidServiceCheck(t *testing.T) {
 	if err := s.Validate(); err == nil {
 		t.Fatalf("Service should be invalid (too long): %v", err)
 	}
+
+	s = Service{
+		Name: "service-name",
+		Checks: []*ServiceCheck{
+			{
+				Name:     "check-tcp",
+				Type:     ServiceCheckTCP,
+				Interval: 5 * time.Second,
+				Timeout:  2 * time.Second,
+			},
+			{
+				Name:     "check-http",
+				Type:     ServiceCheckHTTP,
+				Path:     "/foo",
+				Interval: 5 * time.Second,
+				Timeout:  2 * time.Second,
+			},
+		},
+	}
+	if err := s.Validate(); err == nil {
+		t.Fatalf("service should be invalid (tcp/http checks with no port): %v", err)
+	}
+
+	s = Service{
+		Name: "service-name",
+		Checks: []*ServiceCheck{
+			{
+				Name:     "check-script",
+				Type:     ServiceCheckScript,
+				Command:  "/bin/date",
+				Interval: 5 * time.Second,
+				Timeout:  2 * time.Second,
+			},
+		},
+	}
+	if err := s.Validate(); err != nil {
+		t.Fatalf("un-expected error: %v", err)
+	}
 }
 
 func TestDistinctCheckID(t *testing.T) {