From 81d6f082f36940f72c8a1961ac1150d7e63e415f Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 30 May 2018 16:53:10 -0700 Subject: pkg/hooks/exec: Add a new package for local hook execution This wraps os/exec to: * Clear the environment when the hook doesn't set 'env'. The runtime spec has [1]: > * env (array of strings, OPTIONAL) with the same semantics as IEEE > Std 1003.1-2008's environ. And running execle or similar with NULL env results in an empty environment: $ cat test.c #include int main() { return execle("/usr/bin/env", "env", NULL, NULL); } $ cc -o test test.c $ ./test ...no output... Go's Cmd.Env, on the other hand, has [2]: > If Env is nil, the new process uses the current process's environment. This commit works around that by setting []string{} in those cases to avoid leaking the runtime environment into the hooks. * Roll the 'timeout' value (if set) into the passed context. There's no need for two separate ways to cancel hook execution. * Add a configurable timeout on abandoning a post-kill wait. The waiting goroutine will continue and eventually reap the process, but this avoids blocking the Run() call when that takes inordinately long (for example, if a GPU cleanup hook is stuck in I/O sleep [3]). The 'env' output format is specified in POSIX [4]. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.1/config.md#posix-platform-hooks [2]: https://golang.org/pkg/os/exec/#Cmd [3]: https://github.com/projectatomic/libpod/pull/857#discussion_r192191002 [4]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/env.html Signed-off-by: W. Trevor King Closes: #857 Approved by: mheon --- pkg/hooks/exec/exec.go | 62 +++++++++++++ pkg/hooks/exec/exec_test.go | 220 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 282 insertions(+) create mode 100644 pkg/hooks/exec/exec.go create mode 100644 pkg/hooks/exec/exec_test.go (limited to 'pkg/hooks') diff --git a/pkg/hooks/exec/exec.go b/pkg/hooks/exec/exec.go new file mode 100644 index 000000000..94469b1d2 --- /dev/null +++ b/pkg/hooks/exec/exec.go @@ -0,0 +1,62 @@ +// Package exec provides utilities for executing Open Container Initative runtime hooks. +package exec + +import ( + "bytes" + "context" + "fmt" + "io" + osexec "os/exec" + "time" + + rspec "github.com/opencontainers/runtime-spec/specs-go" +) + +// DefaultPostKillTimeout is the recommended default post-kill timeout. +const DefaultPostKillTimeout = time.Duration(10) * time.Second + +// Run executes the hook and waits for it to complete or for the +// context or hook-specified timeout to expire. +func Run(ctx context.Context, hook *rspec.Hook, state []byte, stdout io.Writer, stderr io.Writer, postKillTimeout time.Duration) (hookErr, err error) { + cmd := osexec.Cmd{ + Path: hook.Path, + Args: hook.Args, + Env: hook.Env, + Stdin: bytes.NewReader(state), + Stdout: stdout, + Stderr: stderr, + } + if cmd.Env == nil { + cmd.Env = []string{} + } + + if hook.Timeout != nil { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, time.Duration(*hook.Timeout)*time.Second) + defer cancel() + } + + err = cmd.Start() + if err != nil { + return err, err + } + exit := make(chan error, 1) + go func() { + exit <- cmd.Wait() + }() + + select { + case err = <-exit: + return err, err + case <-ctx.Done(): + cmd.Process.Kill() + timer := time.NewTimer(postKillTimeout) + defer timer.Stop() + select { + case <-timer.C: + err = fmt.Errorf("failed to reap process within %s of the kill signal", postKillTimeout) + case err = <-exit: + } + return err, ctx.Err() + } +} diff --git a/pkg/hooks/exec/exec_test.go b/pkg/hooks/exec/exec_test.go new file mode 100644 index 000000000..433f953df --- /dev/null +++ b/pkg/hooks/exec/exec_test.go @@ -0,0 +1,220 @@ +package exec + +import ( + "bytes" + "context" + "fmt" + "os" + "runtime" + "strings" + "testing" + "time" + + rspec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/stretchr/testify/assert" +) + +// path is the path to an example hook executable. +var path string + +// unavoidableEnvironmentKeys may be injected even if the hook +// executable is executed with a requested empty environment. +var unavoidableEnvironmentKeys []string + +func TestRun(t *testing.T) { + ctx := context.Background() + hook := &rspec.Hook{ + Path: path, + Args: []string{"sh", "-c", "cat"}, + } + var stderr, stdout bytes.Buffer + hookErr, err := Run(ctx, hook, []byte("{}"), &stdout, &stderr, DefaultPostKillTimeout) + if err != nil { + t.Fatal(err) + } + if hookErr != nil { + t.Fatal(hookErr) + } + assert.Equal(t, "{}", stdout.String()) + assert.Equal(t, "", stderr.String()) +} + +func TestRunIgnoreOutput(t *testing.T) { + ctx := context.Background() + hook := &rspec.Hook{ + Path: path, + Args: []string{"sh", "-c", "cat"}, + } + hookErr, err := Run(ctx, hook, []byte("{}"), nil, nil, DefaultPostKillTimeout) + if err != nil { + t.Fatal(err) + } + if hookErr != nil { + t.Fatal(hookErr) + } +} + +func TestRunFailedStart(t *testing.T) { + ctx := context.Background() + hook := &rspec.Hook{ + Path: "/does/not/exist", + } + hookErr, err := Run(ctx, hook, []byte("{}"), nil, nil, DefaultPostKillTimeout) + if err == nil { + t.Fatal("unexpected success") + } + if !os.IsNotExist(err) { + t.Fatal(err) + } + assert.Equal(t, err, hookErr) +} + +func parseEnvironment(input string) (env map[string]string, err error) { + env = map[string]string{} + lines := strings.Split(input, "\n") + for i, line := range lines { + if line == "" && i == len(lines)-1 { + continue // no content after the terminal newline + } + keyValue := strings.SplitN(line, "=", 2) + if len(keyValue) < 2 { + return env, fmt.Errorf("no = in environment line: %q", line) + } + env[keyValue[0]] = keyValue[1] + } + for _, key := range unavoidableEnvironmentKeys { + delete(env, key) + } + return env, nil +} + +func TestRunEnvironment(t *testing.T) { + ctx := context.Background() + hook := &rspec.Hook{ + Path: path, + Args: []string{"sh", "-c", "env"}, + } + for _, test := range []struct { + name string + env []string + expected map[string]string + }{ + { + name: "unset", + expected: map[string]string{}, + }, + { + name: "set empty", + env: []string{}, + expected: map[string]string{}, + }, + { + name: "set", + env: []string{ + "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + "TERM=xterm", + }, + expected: map[string]string{ + "PATH": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + "TERM": "xterm", + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + var stderr, stdout bytes.Buffer + hook.Env = test.env + hookErr, err := Run(ctx, hook, []byte("{}"), &stdout, &stderr, DefaultPostKillTimeout) + if err != nil { + t.Fatal(err) + } + if hookErr != nil { + t.Fatal(hookErr) + } + assert.Equal(t, "", stderr.String()) + + env, err := parseEnvironment(stdout.String()) + if err != nil { + t.Fatal(err) + } + assert.Equal(t, test.expected, env) + }) + } +} + +func TestRunCancel(t *testing.T) { + hook := &rspec.Hook{ + Path: path, + Args: []string{"sh", "-c", "echo waiting; sleep 2; echo done"}, + } + one := 1 + for _, test := range []struct { + name string + contextTimeout time.Duration + hookTimeout *int + expectedHookError string + expectedRunError error + expectedStdout string + }{ + { + name: "no timeouts", + expectedStdout: "waiting\ndone\n", + }, + { + name: "context timeout", + contextTimeout: time.Duration(1) * time.Second, + expectedStdout: "waiting\n", + expectedHookError: "^signal: killed$", + expectedRunError: context.DeadlineExceeded, + }, + { + name: "hook timeout", + hookTimeout: &one, + expectedStdout: "waiting\n", + expectedHookError: "^signal: killed$", + expectedRunError: context.DeadlineExceeded, + }, + } { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + var stderr, stdout bytes.Buffer + if test.contextTimeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, test.contextTimeout) + defer cancel() + } + hook.Timeout = test.hookTimeout + hookErr, err := Run(ctx, hook, []byte("{}"), &stdout, &stderr, DefaultPostKillTimeout) + assert.Equal(t, test.expectedRunError, err) + if test.expectedHookError == "" { + if hookErr != nil { + t.Fatal(hookErr) + } + } else { + assert.Regexp(t, test.expectedHookError, hookErr.Error()) + } + assert.Equal(t, "", stderr.String()) + assert.Equal(t, test.expectedStdout, stdout.String()) + }) + } +} + +func TestRunKillTimeout(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(500)*time.Millisecond) + defer cancel() + hook := &rspec.Hook{ + Path: path, + Args: []string{"sh", "-c", "sleep 1"}, + } + hookErr, err := Run(ctx, hook, []byte("{}"), nil, nil, time.Duration(0)) + assert.Equal(t, context.DeadlineExceeded, err) + assert.Regexp(t, "^failed to reap process within 0s of the kill signal$", hookErr) +} + +func init() { + if runtime.GOOS != "windows" { + path = "/bin/sh" + unavoidableEnvironmentKeys = []string{"PWD", "SHLVL", "_"} + } else { + panic("we need a reliable executable path on Windows") + } +} -- cgit v1.2.3-54-g00ecf