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

@@ -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 {

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 {

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
+	// See https://github.com/hashicorp/consul/pull/3557 why we need this
-	// and why we should get rid of it.
+	// 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",

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"`

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)

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