From 07cf3448473487c5e306a23eb71f3a0f76f16277 Mon Sep 17 00:00:00 2001 From: Diptanu Choudhury Date: Fri, 15 Apr 2016 12:50:55 +0400 Subject: [PATCH 1/2] Invalidating services when they contain check of type tcp and http but no ports --- nomad/structs/structs.go | 4 ++++ nomad/structs/structs_test.go | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 380bdeaefa86..c3ab3a056b4e 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1527,6 +1527,10 @@ func (s *Service) Validate() error { } for _, c := range s.Checks { + if s.PortLabel == "" && (c.Type == ServiceCheckTCP || c.Type == ServiceCheckHTTP) { + 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) } diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 04f96635d515..72e7145cefce 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -551,6 +551,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) { From fc8a6891880a544ed27db4d6976ead465dc1529c Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Mon, 18 Apr 2016 19:38:47 -0700 Subject: [PATCH 2/2] Ensure the label exists on checks and small enhancements --- nomad/structs/structs.go | 60 ++++++++++++++++++++++++++++++++--- nomad/structs/structs_test.go | 33 ++++++++++++++++++- 2 files changed, 87 insertions(+), 6 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index c3ab3a056b4e..f461c9b052df 100644 --- a/nomad/structs/structs.go +++ b/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,7 +1537,7 @@ func (s *Service) Validate() error { } for _, c := range s.Checks { - if s.PortLabel == "" && (c.Type == ServiceCheckTCP || c.Type == ServiceCheckHTTP) { + 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 } @@ -1726,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 { @@ -1758,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. diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 72e7145cefce..fbf3ffd5bd56 100644 --- a/nomad/structs/structs_test.go +++ b/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", },