From f6066f8305cf9e68cce453326e2e0373ef56d583 Mon Sep 17 00:00:00 2001 From: preetapan Date: Tue, 10 Oct 2017 18:54:06 -0500 Subject: [PATCH] =?UTF-8?q?Fixes=20agent=20error=20handling=20when=20check?= =?UTF-8?q?=20definition=20is=20invalid.=20Distingu=E2=80=A6=20(#3560)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 2 ++ agent/agent.go | 15 ++++++----- agent/agent_endpoint.go | 19 +++++++------ agent/structs/check_type.go | 34 ++++++++++++++++++------ agent/structs/service_definition.go | 19 ++++++++----- agent/structs/service_definition_test.go | 25 +++++++++++++---- 6 files changed, 79 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c2a12dd6..df97d856e 100644 --- a/CHANGELOG.md +++ b/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)] diff --git a/agent/agent.go b/agent/agent.go index 2d232fe55..b9479c0e4 100644 --- a/agent/agent.go +++ b/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) } } diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index f9358477c..271f53da6 100644 --- a/agent/agent_endpoint.go +++ b/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. diff --git a/agent/structs/check_type.go b/agent/structs/check_type.go index 2f4c8a369..f6cd55053 100644 --- a/agent/structs/check_type.go +++ b/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 } diff --git a/agent/structs/service_definition.go b/agent/structs/service_definition.go index 01975ef08..71a7ea0df 100644 --- a/agent/structs/service_definition.go +++ b/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 } diff --git a/agent/structs/service_definition_test.go b/agent/structs/service_definition_test.go index 3e44bb2de..12a9ef99a 100644 --- a/agent/structs/service_definition_test.go +++ b/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) + } } }