Fixes agent error handling when check definition is invalid. Distingu… (#3560)

* Fixes agent error handling when check definition is invalid. Distinguishes between empty checks vs invalid checks

* Made CheckTypes return Checks from service definition struct rather than a new copy, and other changes from code review. This also errors when json payload contains empty structs

* Simplify and improve validate method, and make sure that CheckTypes always returns a new copy of validated check definitions

* Tweaks some small style things and error messages.

* Updates the change log.

CHANGELOG.md

@@ -124,6 +124,8 @@ BREAKING CHANGES:
     | consul.session_ttl |
     | consul.txn |
 
+* **Checks Validated On Agent Startup:** Consul agents now validate health check definitions in their configuration and will fail at startup if any checks are invalid. In previous versions of Consul, invalid health checks would get skipped. [[GH-3559](https://github.com/hashicorp/consul/issues/3559)]
+
 FEATURES:
 
 * **Support for HCL Config Files:** Consul now supports HashiCorp's [HCL](https://github.com/hashicorp/hcl#syntax) format for config files. This is easier to work with than JSON and supports comments. As part of this change, all config files will need to have either an `.hcl` or `.json` extension in order to specify their format. [[GH-3480](https://github.com/hashicorp/consul/issues/3480)]

agent/agent.go

@@ -1485,8 +1485,8 @@ func (a *Agent) AddService(service *structs.NodeService, chkTypes []*structs.Che
 		service.ID = service.Service
 	}
 	for _, check := range chkTypes {
-		if !check.Valid() {
-			return fmt.Errorf("Check type is not valid")
+		if err := check.Validate(); err != nil {
+			return fmt.Errorf("Check is not valid: %v", err)
 		}
 	}
 
@@ -1602,8 +1602,8 @@ func (a *Agent) AddCheck(check *structs.HealthCheck, chkType *structs.CheckType,
 	}
 
 	if chkType != nil {
-		if !chkType.Valid() {
-			return fmt.Errorf("Check type is not valid")
+		if err := chkType.Validate(); err != nil {
+			return fmt.Errorf("Check is not valid: %v", err)
 		}
 
 		if chkType.IsScript() && !a.config.EnableScriptChecks {
@@ -2041,9 +2041,12 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig) error {
 	// Register the services from config
 	for _, service := range conf.Services {
 		ns := service.NodeService()
-		chkTypes := service.CheckTypes()
+		chkTypes, err := service.CheckTypes()
+		if err != nil {
+			return fmt.Errorf("Failed to validate checks for service %q: %v", service.Name, err)
+		}
 		if err := a.AddService(ns, chkTypes, false, service.Token); err != nil {
-			return fmt.Errorf("Failed to register service '%s': %v", service.ID, err)
+			return fmt.Errorf("Failed to register service %q: %v", service.Name, err)
 		}
 	}
 

agent/agent_endpoint.go

@@ -309,8 +309,6 @@ func (s *HTTPServer) syncChanges() {
 	}
 }
 
-const invalidCheckMessage = "Must provide TTL or Script/DockerContainerID/HTTP/TCP and Interval"
-
 func (s *HTTPServer) AgentRegisterCheck(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	if req.Method != "PUT" {
 		return nil, MethodNotAllowedError{req.Method, []string{"PUT"}}
@@ -345,9 +343,10 @@ func (s *HTTPServer) AgentRegisterCheck(resp http.ResponseWriter, req *http.Requ
 
 	// Verify the check type.
 	chkType := args.CheckType()
-	if !chkType.Valid() {
+	err := chkType.Validate()
+	if err != nil {
 		resp.WriteHeader(http.StatusBadRequest)
-		fmt.Fprint(resp, invalidCheckMessage)
+		fmt.Fprint(resp, fmt.Errorf("Invalid check: %v", err))
 		return nil, nil
 	}
 
@@ -576,18 +575,18 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re
 	ns := args.NodeService()
 
 	// Verify the check type.
-	chkTypes := args.CheckTypes()
+	chkTypes, err := args.CheckTypes()
+	if err != nil {
+		resp.WriteHeader(http.StatusBadRequest)
+		fmt.Fprint(resp, fmt.Errorf("Invalid check: %v", err))
+		return nil, nil
+	}
 	for _, check := range chkTypes {
 		if check.Status != "" && !structs.ValidStatus(check.Status) {
 			resp.WriteHeader(http.StatusBadRequest)
 			fmt.Fprint(resp, "Status for checks must 'passing', 'warning', 'critical'")
 			return nil, nil
 		}
-		if !check.Valid() {
-			resp.WriteHeader(http.StatusBadRequest)
-			fmt.Fprint(resp, invalidCheckMessage)
-			return nil, nil
-		}
 	}
 
 	// Get the provided token, if any, and vet against any ACL policies.

agent/structs/check_type.go

@@ -1,6 +1,8 @@
 package structs
 
 import (
+	"fmt"
+	"reflect"
 	"time"
 
 	"github.com/hashicorp/consul/types"
@@ -43,9 +45,25 @@ type CheckType struct {
 }
 type CheckTypes []*CheckType
 
-// Valid checks if the CheckType is valid
-func (c *CheckType) Valid() bool {
-	return c.IsTTL() || c.IsMonitor() || c.IsHTTP() || c.IsTCP() || c.IsDocker()
+// Validate returns an error message if the check is invalid
+func (c *CheckType) Validate() error {
+	intervalCheck := c.IsScript() || c.HTTP != "" || c.TCP != ""
+
+	if c.Interval > 0 && c.TTL > 0 {
+		return fmt.Errorf("Interval and TTL cannot both be specified")
+	}
+	if intervalCheck && c.Interval <= 0 {
+		return fmt.Errorf("Interval must be > 0 for Script, HTTP, or TCP checks")
+	}
+	if !intervalCheck && c.TTL <= 0 {
+		return fmt.Errorf("TTL must be > 0 for TTL checks")
+	}
+	return nil
+}
+
+// Empty checks if the CheckType has no fields defined. Empty checks parsed from json configs are filtered out
+func (c *CheckType) Empty() bool {
+	return reflect.DeepEqual(c, &CheckType{})
 }
 
 // IsScript checks if this is a check that execs some kind of script.
@@ -55,25 +73,25 @@ func (c *CheckType) IsScript() bool {
 
 // IsTTL checks if this is a TTL type
 func (c *CheckType) IsTTL() bool {
-	return c.TTL != 0
+	return c.TTL > 0
 }
 
 // IsMonitor checks if this is a Monitor type
 func (c *CheckType) IsMonitor() bool {
-	return c.IsScript() && c.DockerContainerID == "" && c.Interval != 0
+	return c.IsScript() && c.DockerContainerID == "" && c.Interval > 0
 }
 
 // IsHTTP checks if this is a HTTP type
 func (c *CheckType) IsHTTP() bool {
-	return c.HTTP != "" && c.Interval != 0
+	return c.HTTP != "" && c.Interval > 0
 }
 
 // IsTCP checks if this is a TCP type
 func (c *CheckType) IsTCP() bool {
-	return c.TCP != "" && c.Interval != 0
+	return c.TCP != "" && c.Interval > 0
 }
 
 // IsDocker returns true when checking a docker container.
 func (c *CheckType) IsDocker() bool {
-	return c.IsScript() && c.DockerContainerID != "" && c.Interval != 0
+	return c.IsScript() && c.DockerContainerID != "" && c.Interval > 0
 }

agent/structs/service_definition.go

@@ -28,12 +28,19 @@ func (s *ServiceDefinition) NodeService() *NodeService {
 	return ns
 }
 
-func (s *ServiceDefinition) CheckTypes() (checks CheckTypes) {
-	s.Checks = append(s.Checks, &s.Check)
-	for _, check := range s.Checks {
-		if check.Valid() {
-			checks = append(checks, check)
+func (s *ServiceDefinition) CheckTypes() (checks CheckTypes, err error) {
+	if !s.Check.Empty() {
+		err := s.Check.Validate()
+		if err != nil {
+			return nil, err
 		}
+		checks = append(checks, &s.Check)
 	}
-	return
+	for _, check := range s.Checks {
+		if err := check.Validate(); err != nil {
+			return nil, err
+		}
+		checks = append(checks, check)
+	}
+	return checks, nil
 }

agent/structs/service_definition_test.go

@@ -1,8 +1,11 @@
 package structs
 
 import (
+	"fmt"
 	"testing"
 	"time"
+
+	"github.com/pascaldekloe/goe/verify"
 )
 
 func TestAgentStructs_CheckTypes(t *testing.T) {
@@ -32,10 +35,22 @@ func TestAgentStructs_CheckTypes(t *testing.T) {
 		TTL: 10 * time.Second,
 	})
 
-	// Does not return invalid checks
-	svc.Checks = append(svc.Checks, &CheckType{})
-
-	if len(svc.CheckTypes()) != 4 {
-		t.Fatalf("bad: %#v", svc)
+	// Validate checks
+	cases := []struct {
+		in   *CheckType
+		err  error
+		desc string
+	}{
+		{&CheckType{HTTP: "http://foo/baz"}, fmt.Errorf("Interval must be > 0 for Script, HTTP, or TCP checks"), "Missing interval"},
+		{&CheckType{TTL: -1}, fmt.Errorf("TTL must be > 0 for TTL checks"), "Negative TTL"},
+		{&CheckType{TTL: 20 * time.Second, Interval: 10 * time.Second}, fmt.Errorf("Interval and TTL cannot both be specified"), "Interval and TTL both set"},
+	}
+	for _, tc := range cases {
+		svc.Check = *tc.in
+		checks, err := svc.CheckTypes()
+		verify.Values(t, tc.desc, err.Error(), tc.err.Error())
+		if len(checks) != 0 {
+			t.Fatalf("bad: %#v", svc)
+		}
 	}
 }