Skip to content

Commit

Permalink
Clean up subprocess handling and make shell use optional (#3509)
Browse files Browse the repository at this point in the history 
* Clean up handling of subprocesses and make using a shell optional

* Update docs for subprocess changes

* Fix tests for new subprocess behavior

* More cleanup of subprocesses

* Minor adjustments and cleanup for subprocess logic

* Makes the watch handler reload test use the new path.

* Adds check tests for new args path, and updates existing tests to use new path.

* Adds support for script args in Docker checks.

* Fixes the sanitize unit test.

* Adds panic for unknown watch type, and reverts back to Run().

* Adds shell option back to consul lock command.

* Adds shell option back to consul exec command.

* Adds shell back into consul watch command.

* Refactors signal forwarding and makes Windows-friendly.

* Adds a clarifying comment.

* Changes error wording to a warning.

* Scopes signals to interrupt and kill.

This avoids us trying to send SIGCHILD to the dead process.

* Adds an error for shell=false for consul exec.

* Adds notes about the deprecated script and handler fields.

* De-nests an if statement.
  • Loading branch information
@kyhavlov @slackpad
kyhavlov authored and slackpad committed Oct 4, 2017
1 parent a61929a commit 198ed60
Show file tree
Hide file tree
Showing 25 changed files with 472 additions and 167 deletions.
63 changes: 50 additions & 13 deletions agent/agent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -535,16 +535,40 @@ func (a *Agent) reloadWatches(cfg *config.RuntimeConfig) error {
var watchPlans []*watch.Plan
for _, params := range cfg.Watches {
// Parse the watches, excluding the handler
wp, err := watch.ParseExempt(params, []string{"handler"})
wp, err := watch.ParseExempt(params, []string{"handler", "args"})
if err != nil {
return fmt.Errorf("Failed to parse watch (%#v): %v", params, err)
}

// Get the handler
h := wp.Exempt["handler"]
if _, ok := h.(string); h == nil || !ok {
// Get the handler and subprocess arguments
handler, hasHandler := wp.Exempt["handler"]
args, hasArgs := wp.Exempt["args"]
if hasHandler {
a.logger.Printf("[WARN] agent: The 'handler' field in watches has been deprecated " +
"and replaced with the 'args' field. See https://www.consul.io/docs/agent/watches.html")
}
if _, ok := handler.(string); hasHandler && !ok {
return fmt.Errorf("Watch handler must be a string")
}
if raw, ok := args.([]interface{}); hasArgs && ok {
var parsed []string
for _, arg := range raw {
if v, ok := arg.(string); !ok {
return fmt.Errorf("Watch args must be a list of strings")
} else {
parsed = append(parsed, v)
}
}
wp.Exempt["args"] = parsed
} else if hasArgs && !ok {
return fmt.Errorf("Watch args must be a list of strings")
}
if hasHandler && hasArgs {
return fmt.Errorf("Cannot define both watch handler and args")
}
if !hasHandler && !hasArgs {
return fmt.Errorf("Must define either watch handler or args")
}

// Store the watch plan
watchPlans = append(watchPlans, wp)
Expand All @@ -566,7 +590,13 @@ func (a *Agent) reloadWatches(cfg *config.RuntimeConfig) error {
for _, wp := range watchPlans {
a.watchPlans = append(a.watchPlans, wp)
go func(wp *watch.Plan) {
wp.Handler = makeWatchHandler(a.LogOutput, wp.Exempt["handler"])
var handler interface{}
if h, ok := wp.Exempt["handler"]; ok {
handler = h
} else {
handler = wp.Exempt["args"]
}
wp.Handler = makeWatchHandler(a.LogOutput, handler)
wp.LogOutput = a.LogOutput
if err := wp.Run(addr); err != nil {
a.logger.Printf("[ERR] Failed to run watch: %v", err)
Expand Down Expand Up @@ -1684,6 +1714,7 @@ func (a *Agent) AddCheck(check *structs.HealthCheck, chkType *structs.CheckType,
DockerContainerID: chkType.DockerContainerID,
Shell: chkType.Shell,
Script: chkType.Script,
ScriptArgs: chkType.ScriptArgs,
Interval: chkType.Interval,
Logger: a.logger,
client: a.dockerClient,
Expand All @@ -1697,18 +1728,24 @@ func (a *Agent) AddCheck(check *structs.HealthCheck, chkType *structs.CheckType,
delete(a.checkMonitors, check.CheckID)
}
if chkType.Interval < MinInterval {
a.logger.Println(fmt.Sprintf("[WARN] agent: check '%s' has interval below minimum of %v",
check.CheckID, MinInterval))
a.logger.Printf("[WARN] agent: check '%s' has interval below minimum of %v",
check.CheckID, MinInterval)
chkType.Interval = MinInterval
}
if chkType.Script != "" {
a.logger.Printf("[WARN] agent: check %q has the 'script' field, which has been deprecated "+
"and replaced with the 'args' field. See https://www.consul.io/docs/agent/checks.html",
check.CheckID)
}

monitor := &CheckMonitor{
Notify: a.state,
CheckID: check.CheckID,
Script: chkType.Script,
Interval: chkType.Interval,
Timeout: chkType.Timeout,
Logger: a.logger,
Notify: a.state,
CheckID: check.CheckID,
Script: chkType.Script,
ScriptArgs: chkType.ScriptArgs,
Interval: chkType.Interval,
Timeout: chkType.Timeout,
Logger: a.logger,
}
monitor.Start()
a.checkMonitors[check.CheckID] = monitor
Expand Down
18 changes: 9 additions & 9 deletions agent/agent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1928,9 +1928,9 @@ func TestAgent_reloadWatches(t *testing.T) {
newConf := *a.config
newConf.Watches = []map[string]interface{}{
{
"type": "key",
"key": "asdf",
"handler": "ls",
"type": "key",
"key": "asdf",
"args": []interface{}{"ls"},
},
}
if err := a.reloadWatches(&newConf); err != nil {
Expand All @@ -1942,9 +1942,9 @@ func TestAgent_reloadWatches(t *testing.T) {
newConf.HTTPAddrs = make([]net.Addr, 0)
newConf.Watches = []map[string]interface{}{
{
"type": "key",
"key": "asdf",
"handler": "ls",
"type": "key",
"key": "asdf",
"args": []interface{}{"ls"},
},
}
if err := a.reloadWatches(&newConf); err != nil {
Expand All @@ -1955,9 +1955,9 @@ func TestAgent_reloadWatches(t *testing.T) {
newConf.HTTPSAddrs = make([]net.Addr, 0)
newConf.Watches = []map[string]interface{}{
{
"type": "key",
"key": "asdf",
"handler": "ls",
"type": "key",
"key": "asdf",
"args": []interface{}{"ls"},
},
}
if err := a.reloadWatches(&newConf); err == nil || !strings.Contains(err.Error(), "watch plans require an HTTP or HTTPS endpoint") {
Expand Down
30 changes: 22 additions & 8 deletions agent/check.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,13 @@ type CheckNotifier interface {
// determine the health of a given check. It is compatible with
// nagios plugins and expects the output in the same format.
type CheckMonitor struct {
Notify CheckNotifier
CheckID types.CheckID
Script string
Interval time.Duration
Timeout time.Duration
Logger *log.Logger
Notify CheckNotifier
CheckID types.CheckID
Script string
ScriptArgs []string
Interval time.Duration
Timeout time.Duration
Logger *log.Logger

stop bool
stopCh chan struct{}
Expand Down Expand Up @@ -101,7 +102,13 @@ func (c *CheckMonitor) run() {
// check is invoked periodically to perform the script check
func (c *CheckMonitor) check() {
// Create the command
cmd, err := ExecScript(c.Script)
var cmd *exec.Cmd
var err error
if len(c.ScriptArgs) > 0 {
cmd, err = ExecSubprocess(c.ScriptArgs)
} else {
cmd, err = ExecScript(c.Script)
}
if err != nil {
c.Logger.Printf("[ERR] agent: failed to setup invoke '%s': %s", c.Script, err)
c.Notify.UpdateCheck(c.CheckID, api.HealthCritical, err.Error())
Expand Down Expand Up @@ -524,6 +531,7 @@ type CheckDocker struct {
Notify CheckNotifier
CheckID types.CheckID
Script string
ScriptArgs []string
DockerContainerID string
Shell string
Interval time.Duration
Expand Down Expand Up @@ -599,7 +607,13 @@ func (c *CheckDocker) check() {
}

func (c *CheckDocker) doCheck() (string, *circbuf.Buffer, error) {
cmd := []string{c.Shell, "-c", c.Script}
var cmd []string
if len(c.ScriptArgs) > 0 {
cmd = c.ScriptArgs
} else {
cmd = []string{c.Shell, "-c", c.Script}
}

execID, err := c.client.CreateExec(c.DockerContainerID, cmd)
if err != nil {
return api.HealthCritical, nil, err
Expand Down
71 changes: 53 additions & 18 deletions agent/check_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"github.com/hashicorp/consul/types"
)

func TestCheckMonitor(t *testing.T) {
func TestCheckMonitor_Script(t *testing.T) {
tests := []struct {
script, status string
}{
Expand Down Expand Up @@ -54,16 +54,51 @@ func TestCheckMonitor(t *testing.T) {
}
}

func TestCheckMonitor_Args(t *testing.T) {
tests := []struct {
args []string
status string
}{
{[]string{"sh", "-c", "exit 0"}, "passing"},
{[]string{"sh", "-c", "exit 1"}, "warning"},
{[]string{"sh", "-c", "exit 2"}, "critical"},
{[]string{"foobarbaz"}, "critical"},
}

for _, tt := range tests {
t.Run(tt.status, func(t *testing.T) {
notif := mock.NewNotify()
check := &CheckMonitor{
Notify: notif,
CheckID: types.CheckID("foo"),
ScriptArgs: tt.args,
Interval: 25 * time.Millisecond,
Logger: log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
}
check.Start()
defer check.Stop()
retry.Run(t, func(r *retry.R) {
if got, want := notif.Updates("foo"), 2; got < want {
r.Fatalf("got %d updates want at least %d", got, want)
}
if got, want := notif.State("foo"), tt.status; got != want {
r.Fatalf("got state %q want %q", got, want)
}
})
})
}
}

func TestCheckMonitor_Timeout(t *testing.T) {
// t.Parallel() // timing test. no parallel
notif := mock.NewNotify()
check := &CheckMonitor{
Notify: notif,
CheckID: types.CheckID("foo"),
Script: "sleep 1 && exit 0",
Interval: 50 * time.Millisecond,
Timeout: 25 * time.Millisecond,
Logger: log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
Notify: notif,
CheckID: types.CheckID("foo"),
ScriptArgs: []string{"sh", "-c", "sleep 1 && exit 0"},
Interval: 50 * time.Millisecond,
Timeout: 25 * time.Millisecond,
Logger: log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
}
check.Start()
defer check.Stop()
Expand All @@ -83,11 +118,11 @@ func TestCheckMonitor_RandomStagger(t *testing.T) {
// t.Parallel() // timing test. no parallel
notif := mock.NewNotify()
check := &CheckMonitor{
Notify: notif,
CheckID: types.CheckID("foo"),
Script: "exit 0",
Interval: 25 * time.Millisecond,
Logger: log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
Notify: notif,
CheckID: types.CheckID("foo"),
ScriptArgs: []string{"sh", "-c", "exit 0"},
Interval: 25 * time.Millisecond,
Logger: log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
}
check.Start()
defer check.Stop()
Expand All @@ -108,11 +143,11 @@ func TestCheckMonitor_LimitOutput(t *testing.T) {
t.Parallel()
notif := mock.NewNotify()
check := &CheckMonitor{
Notify: notif,
CheckID: types.CheckID("foo"),
Script: "od -N 81920 /dev/urandom",
Interval: 25 * time.Millisecond,
Logger: log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
Notify: notif,
CheckID: types.CheckID("foo"),
ScriptArgs: []string{"od", "-N", "81920", "/dev/urandom"},
Interval: 25 * time.Millisecond,
Logger: log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
}
check.Start()
defer check.Stop()
Expand Down Expand Up @@ -775,7 +810,7 @@ func TestCheck_Docker(t *testing.T) {
check := &CheckDocker{
Notify: notif,
CheckID: id,
Script: "/health.sh",
ScriptArgs: []string{"/health.sh"},
DockerContainerID: "123",
Interval: 25 * time.Millisecond,
client: c,
Expand Down
1 change: 1 addition & 0 deletions agent/config/builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,7 @@ func (b *Builder) checkVal(v *CheckDefinition) *structs.CheckDefinition {
Token: b.stringVal(v.Token),
Status: b.stringVal(v.Status),
Script: b.stringVal(v.Script),
ScriptArgs: v.ScriptArgs,
HTTP: b.stringVal(v.HTTP),
Header: v.Header,
Method: b.stringVal(v.Method),
Expand Down
1 change: 1 addition & 0 deletions agent/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ type CheckDefinition struct {
Token *string `json:"token,omitempty" hcl:"token" mapstructure:"token"`
Status *string `json:"status,omitempty" hcl:"status" mapstructure:"status"`
Script *string `json:"script,omitempty" hcl:"script" mapstructure:"script"`
ScriptArgs []string `json:"args,omitempty" hcl:"args" mapstructure:"args"`
HTTP *string `json:"http,omitempty" hcl:"http" mapstructure:"http"`
Header map[string][]string `json:"header,omitempty" hcl:"header" mapstructure:"header"`
Method *string `json:"method,omitempty" hcl:"method" mapstructure:"method"`
Expand Down
Loading

0 comments on commit 198ed60

Please sign in to comment.