From 53f67c3993f3080228c1b673e5f189a7df3782b4 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 18 Oct 2017 11:28:39 -0700 Subject: [PATCH] Fixes API client for ScriptArgs and updates documentation. (#3589) * Updates the API client to support the current `ScriptArgs` parameter for checks. * Updates docs for checks to explain the `ScriptArgs` parameter issue. * Adds mappings for "args" and "script-args" to give th API parity with config. * Adds checks on return codes. * Removes debug logging that shows empty when args are used. --- agent/agent_endpoint_test.go | 98 ++++++++++++++++++++++++-- agent/check.go | 2 - agent/config.go | 8 ++- api/agent.go | 3 +- api/agent_test.go | 63 +++++++++++++++++ website/source/api/agent/check.html.md | 5 ++ 6 files changed, 170 insertions(+), 9 deletions(-) diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 0ebfdc204e..7068eab2df 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -667,7 +667,6 @@ func TestAgent_RegisterCheck(t *testing.T) { a := NewTestAgent(t.Name(), "") defer a.Shutdown() - // Register node args := &structs.CheckDefinition{ Name: "test", TTL: 15 * time.Second, @@ -703,12 +702,105 @@ func TestAgent_RegisterCheck(t *testing.T) { } } +// This verifies all the forms of the new args-style check that we need to +// support as a result of https://github.com/hashicorp/consul/issues/3587. +func TestAgent_RegisterCheck_Scripts(t *testing.T) { + t.Parallel() + a := NewTestAgent(t.Name(), ` + enable_script_checks = true +`) + defer a.Shutdown() + + tests := []struct { + name string + check map[string]interface{} + }{ + { + "< Consul 1.0.0", + map[string]interface{}{ + "Name": "test", + "Interval": "2s", + "Script": "true", + }, + }, + { + "== Consul 1.0.0", + map[string]interface{}{ + "Name": "test", + "Interval": "2s", + "ScriptArgs": []string{"true"}, + }, + }, + { + "> Consul 1.0.0 (fixup)", + map[string]interface{}{ + "Name": "test", + "Interval": "2s", + "script_args": []string{"true"}, + }, + }, + { + "> Consul 1.0.0", + map[string]interface{}{ + "Name": "test", + "Interval": "2s", + "Args": []string{"true"}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name+" as node check", func(t *testing.T) { + req, _ := http.NewRequest("PUT", "/v1/agent/check/register", jsonReader(tt.check)) + resp := httptest.NewRecorder() + if _, err := a.srv.AgentRegisterCheck(resp, req); err != nil { + t.Fatalf("err: %v", err) + } + if resp.Code != http.StatusOK { + t.Fatalf("bad: %d", resp.Code) + } + }) + + t.Run(tt.name+" as top-level service check", func(t *testing.T) { + args := map[string]interface{}{ + "Name": "a", + "Port": 1234, + "Check": tt.check, + } + + req, _ := http.NewRequest("PUT", "/v1/agent/service/register", jsonReader(args)) + resp := httptest.NewRecorder() + if _, err := a.srv.AgentRegisterService(resp, req); err != nil { + t.Fatalf("err: %v", err) + } + if resp.Code != http.StatusOK { + t.Fatalf("bad: %d", resp.Code) + } + }) + + t.Run(tt.name+" as slice-based service check", func(t *testing.T) { + args := map[string]interface{}{ + "Name": "a", + "Port": 1234, + "Checks": []map[string]interface{}{tt.check}, + } + + req, _ := http.NewRequest("PUT", "/v1/agent/service/register", jsonReader(args)) + resp := httptest.NewRecorder() + if _, err := a.srv.AgentRegisterService(resp, req); err != nil { + t.Fatalf("err: %v", err) + } + if resp.Code != http.StatusOK { + t.Fatalf("bad: %d", resp.Code) + } + }) + } +} + func TestAgent_RegisterCheck_Passing(t *testing.T) { t.Parallel() a := NewTestAgent(t.Name(), "") defer a.Shutdown() - // Register node args := &structs.CheckDefinition{ Name: "test", TTL: 15 * time.Second, @@ -744,7 +836,6 @@ func TestAgent_RegisterCheck_BadStatus(t *testing.T) { a := NewTestAgent(t.Name(), "") defer a.Shutdown() - // Register node args := &structs.CheckDefinition{ Name: "test", TTL: 15 * time.Second, @@ -795,7 +886,6 @@ func TestAgent_DeregisterCheck(t *testing.T) { t.Fatalf("err: %v", err) } - // Register node req, _ := http.NewRequest("PUT", "/v1/agent/check/deregister/test", nil) obj, err := a.srv.AgentDeregisterCheck(nil, req) if err != nil { diff --git a/agent/check.go b/agent/check.go index 4386429160..440fc36aef 100644 --- a/agent/check.go +++ b/agent/check.go @@ -86,7 +86,6 @@ func (c *CheckMonitor) Stop() { func (c *CheckMonitor) run() { // Get the randomized initial pause time initialPauseTime := lib.RandomStagger(c.Interval) - c.Logger.Printf("[DEBUG] agent: pausing %v before first invocation of %s", initialPauseTime, c.Script) next := time.After(initialPauseTime) for { select { @@ -594,7 +593,6 @@ func (c *CheckDocker) Stop() { func (c *CheckDocker) run() { firstWait := lib.RandomStagger(c.Interval) - c.Logger.Printf("[DEBUG] agent: pausing %v before first invocation of %s -c %s in container %s", firstWait, c.Shell, c.Script, c.DockerContainerID) next := time.After(firstWait) for { select { diff --git a/agent/config.go b/agent/config.go index 84052986b1..bb7d8a2dce 100644 --- a/agent/config.go +++ b/agent/config.go @@ -17,9 +17,13 @@ func FixupCheckType(raw interface{}) error { return nil } - // see https://github.com/hashicorp/consul/pull/3557 why we need this - // and why we should get rid of it. + // See https://github.com/hashicorp/consul/pull/3557 why we need this + // and why we should get rid of it. In Consul 1.0 we also didn't map + // Args correctly, so we ended up exposing (and need to carry forward) + // ScriptArgs, see https://github.com/hashicorp/consul/issues/3587. config.TranslateKeys(rawMap, map[string]string{ + "args": "ScriptArgs", + "script_args": "ScriptArgs", "deregister_critical_service_after": "DeregisterCriticalServiceAfter", "docker_container_id": "DockerContainerID", "tls_skip_verify": "TLSSkipVerify", diff --git a/api/agent.go b/api/agent.go index ac57415c15..533b245578 100644 --- a/api/agent.go +++ b/api/agent.go @@ -80,7 +80,8 @@ type AgentCheckRegistration struct { // AgentServiceCheck is used to define a node or service level check type AgentServiceCheck struct { - Script string `json:",omitempty"` + Args []string `json:"ScriptArgs,omitempty"` + Script string `json:",omitempty"` // Deprecated, use Args. DockerContainerID string `json:",omitempty"` Shell string `json:",omitempty"` // Only supported for Docker. Interval string `json:",omitempty"` diff --git a/api/agent_test.go b/api/agent_test.go index 11666f71e3..89c2283a86 100644 --- a/api/agent_test.go +++ b/api/agent_test.go @@ -498,6 +498,69 @@ func TestAPI_AgentChecks(t *testing.T) { } } +func TestAPI_AgentScriptCheck(t *testing.T) { + t.Parallel() + c, s := makeClientWithConfig(t, nil, func(c *testutil.TestServerConfig) { + c.EnableScriptChecks = true + }) + defer s.Stop() + + agent := c.Agent() + + t.Run("node script check", func(t *testing.T) { + reg := &AgentCheckRegistration{ + Name: "foo", + AgentServiceCheck: AgentServiceCheck{ + Interval: "10s", + Args: []string{"sh", "-c", "false"}, + }, + } + if err := agent.CheckRegister(reg); err != nil { + t.Fatalf("err: %v", err) + } + + checks, err := agent.Checks() + if err != nil { + t.Fatalf("err: %v", err) + } + if _, ok := checks["foo"]; !ok { + t.Fatalf("missing check: %v", checks) + } + }) + + t.Run("service script check", func(t *testing.T) { + reg := &AgentServiceRegistration{ + Name: "bar", + Port: 1234, + Checks: AgentServiceChecks{ + &AgentServiceCheck{ + Interval: "10s", + Args: []string{"sh", "-c", "false"}, + }, + }, + } + if err := agent.ServiceRegister(reg); err != nil { + t.Fatalf("err: %v", err) + } + + services, err := agent.Services() + if err != nil { + t.Fatalf("err: %v", err) + } + if _, ok := services["bar"]; !ok { + t.Fatalf("missing service: %v", services) + } + + checks, err := agent.Checks() + if err != nil { + t.Fatalf("err: %v", err) + } + if _, ok := checks["service:bar"]; !ok { + t.Fatalf("missing check: %v", checks) + } + }) +} + func TestAPI_AgentCheckStartPassing(t *testing.T) { t.Parallel() c, s := makeClient(t) diff --git a/website/source/api/agent/check.html.md b/website/source/api/agent/check.html.md index c51b8fc637..da64214c70 100644 --- a/website/source/api/agent/check.html.md +++ b/website/source/api/agent/check.html.md @@ -110,6 +110,11 @@ The table below shows this endpoint's support for `Script` field is deprecated, and you should include the shell in the `Args` to run under a shell, eg. `"args": ["sh", "-c", "..."]`. + -> **Note:** Consul 1.0 shipped with an issue where `Args` was erroneously named + `ScriptArgs` in this API. Please use `ScriptArgs` with Consul 1.0 (that will + continue to be accepted in future versions of Consul), and `Args` in Consul + 1.0.1 and later. + - `DockerContainerID` `(string: "")` - Specifies that the check is a Docker check, and Consul will evaluate the script every `Interval` in the given container using the specified `Shell`. Note that `Shell` is currently only