From 11c8816648ac0ce6c6ef3694b5488e72d0d9dab6 Mon Sep 17 00:00:00 2001 From: Fang-Pen Lin Date: Fri, 16 Jun 2023 14:32:02 -0700 Subject: [PATCH] Added option struct for run hook functions, also added workdir option Signed-off-by: Fang-Pen Lin --- pkg/hooks/exec/exec.go | 44 +++++++++++++++++++--- pkg/hooks/exec/exec_test.go | 34 ++++++++++++++--- pkg/hooks/exec/runtimeconfigfilter.go | 37 +++++++++++++++--- pkg/hooks/exec/runtimeconfigfilter_test.go | 2 +- 4 files changed, 99 insertions(+), 18 deletions(-) diff --git a/pkg/hooks/exec/exec.go b/pkg/hooks/exec/exec.go index bc639245f..2bd3a2adf 100644 --- a/pkg/hooks/exec/exec.go +++ b/pkg/hooks/exec/exec.go @@ -16,16 +16,50 @@ import ( // DefaultPostKillTimeout is the recommended default post-kill timeout. const DefaultPostKillTimeout = time.Duration(10) * time.Second +type RunOptions struct { + // The hook to run + Hook *rspec.Hook + // The workdir to change when invoking the hook + Dir string + // The container state data to pass into the hook process + State []byte + // Stdout from the hook process + Stdout io.Writer + // Stderr from the hook process + Stderr io.Writer + // Timeout for waiting process killed + PostKillTimeout time.Duration +} + // Run executes the hook and waits for it to complete or for the // context or hook-specified timeout to expire. +// +// Deprecated: Too many arguments, has been refactored and replaced by RunWithOptions instead func Run(ctx context.Context, hook *rspec.Hook, state []byte, stdout io.Writer, stderr io.Writer, postKillTimeout time.Duration) (hookErr, err error) { + return RunWithOptions( + ctx, + RunOptions{ + Hook: hook, + State: state, + Stdout: stdout, + Stderr: stderr, + PostKillTimeout: postKillTimeout, + }, + ) +} + +// RunWithOptions executes the hook and waits for it to complete or for the +// context or hook-specified timeout to expire. +func RunWithOptions(ctx context.Context, options RunOptions) (hookErr, err error) { + hook := options.Hook cmd := osexec.Cmd{ Path: hook.Path, Args: hook.Args, Env: hook.Env, - Stdin: bytes.NewReader(state), - Stdout: stdout, - Stderr: stderr, + Dir: options.Dir, + Stdin: bytes.NewReader(options.State), + Stdout: options.Stdout, + Stderr: options.Stderr, } if cmd.Env == nil { cmd.Env = []string{} @@ -57,11 +91,11 @@ func Run(ctx context.Context, hook *rspec.Hook, state []byte, stdout io.Writer, if err := cmd.Process.Kill(); err != nil { logrus.Errorf("Failed to kill pid %v", cmd.Process) } - timer := time.NewTimer(postKillTimeout) + timer := time.NewTimer(options.PostKillTimeout) defer timer.Stop() select { case <-timer.C: - err = fmt.Errorf("failed to reap process within %s of the kill signal", postKillTimeout) + err = fmt.Errorf("failed to reap process within %s of the kill signal", options.PostKillTimeout) case err = <-exit: } return err, ctx.Err() diff --git a/pkg/hooks/exec/exec_test.go b/pkg/hooks/exec/exec_test.go index 9b8883f03..eea3fe71c 100644 --- a/pkg/hooks/exec/exec_test.go +++ b/pkg/hooks/exec/exec_test.go @@ -29,7 +29,7 @@ func TestRun(t *testing.T) { Args: []string{"sh", "-c", "cat"}, } var stderr, stdout bytes.Buffer - hookErr, err := Run(ctx, hook, []byte("{}"), &stdout, &stderr, DefaultPostKillTimeout) + hookErr, err := RunWithOptions(ctx, RunOptions{Hook: hook, State: []byte("{}"), Stdout: &stdout, Stderr: &stderr, PostKillTimeout: DefaultPostKillTimeout}) if err != nil { t.Fatal(err) } @@ -46,7 +46,7 @@ func TestRunIgnoreOutput(t *testing.T) { Path: path, Args: []string{"sh", "-c", "cat"}, } - hookErr, err := Run(ctx, hook, []byte("{}"), nil, nil, DefaultPostKillTimeout) + hookErr, err := RunWithOptions(ctx, RunOptions{Hook: hook, State: []byte("{}"), PostKillTimeout: DefaultPostKillTimeout}) if err != nil { t.Fatal(err) } @@ -60,7 +60,7 @@ func TestRunFailedStart(t *testing.T) { hook := &rspec.Hook{ Path: "/does/not/exist", } - hookErr, err := Run(ctx, hook, []byte("{}"), nil, nil, DefaultPostKillTimeout) + hookErr, err := RunWithOptions(ctx, RunOptions{Hook: hook, State: []byte("{}"), PostKillTimeout: DefaultPostKillTimeout}) if err == nil { t.Fatal("unexpected success") } @@ -125,7 +125,7 @@ func TestRunEnvironment(t *testing.T) { 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) + hookErr, err := RunWithOptions(ctx, RunOptions{Hook: hook, State: []byte("{}"), Stdout: &stdout, Stderr: &stderr, PostKillTimeout: DefaultPostKillTimeout}) if err != nil { t.Fatal(err) } @@ -143,6 +143,28 @@ func TestRunEnvironment(t *testing.T) { } } +func TestRunCwd(t *testing.T) { + ctx := context.Background() + hook := &rspec.Hook{ + Path: path, + Args: []string{"sh", "-c", "pwd"}, + } + cwd, err := os.MkdirTemp("", "userdata") + if err != nil { + t.Fatal(err) + } + var stderr, stdout bytes.Buffer + hookErr, err := RunWithOptions(ctx, RunOptions{Hook: hook, Dir: cwd, State: []byte("{}"), Stdout: &stdout, Stderr: &stderr, PostKillTimeout: DefaultPostKillTimeout}) + if err != nil { + t.Fatal(err) + } + if hookErr != nil { + t.Fatal(hookErr) + } + assert.Equal(t, "", stderr.String()) + assert.Equal(t, strings.TrimSuffix(stdout.String(), "\n"), cwd) +} + func TestRunCancel(t *testing.T) { hook := &rspec.Hook{ Path: path, @@ -186,7 +208,7 @@ func TestRunCancel(t *testing.T) { defer cancel() } hook.Timeout = test.hookTimeout - hookErr, err := Run(ctx, hook, []byte("{}"), &stdout, &stderr, DefaultPostKillTimeout) + hookErr, err := RunWithOptions(ctx, RunOptions{Hook: hook, State: []byte("{}"), Stdout: &stdout, Stderr: &stderr, PostKillTimeout: DefaultPostKillTimeout}) assert.Equal(t, test.expectedRunError, err) if test.expectedHookError == "" { if hookErr != nil { @@ -208,7 +230,7 @@ func TestRunKillTimeout(t *testing.T) { Path: path, Args: []string{"sh", "-c", "sleep 1"}, } - hookErr, err := Run(ctx, hook, []byte("{}"), nil, nil, time.Duration(0)) + hookErr, err := RunWithOptions(ctx, RunOptions{Hook: hook, State: []byte("{}"), PostKillTimeout: time.Duration(0)}) assert.Equal(t, context.DeadlineExceeded, err) assert.Regexp(t, "^(failed to reap process within 0s of the kill signal|executing \\[sh -c sleep 1]: signal: killed)$", hookErr) } diff --git a/pkg/hooks/exec/runtimeconfigfilter.go b/pkg/hooks/exec/runtimeconfigfilter.go index 72d4b8979..ac17ac64d 100644 --- a/pkg/hooks/exec/runtimeconfigfilter.go +++ b/pkg/hooks/exec/runtimeconfigfilter.go @@ -21,19 +21,44 @@ var spewConfig = spew.ConfigState{ SortKeys: true, } +type RuntimeConfigFilterOptions struct { + // The hooks to run + Hooks []spec.Hook + // The workdir to change when invoking the hook + Dir string + // The container config spec to pass into the hook processes and potentially get modified by them + Config *spec.Spec + // Timeout for waiting process killed + PostKillTimeout time.Duration +} + // RuntimeConfigFilter calls a series of hooks. But instead of // passing container state on their standard input, // RuntimeConfigFilter passes the proposed runtime configuration (and // reads back a possibly-altered form from their standard output). +// +// Deprecated: Too many arguments, has been refactored and replaced by RuntimeConfigFilterWithOptions instead func RuntimeConfigFilter(ctx context.Context, hooks []spec.Hook, config *spec.Spec, postKillTimeout time.Duration) (hookErr, err error) { - data, err := json.Marshal(config) + return RuntimeConfigFilterWithOptions(ctx, RuntimeConfigFilterOptions{ + Hooks: hooks, + Config: config, + PostKillTimeout: postKillTimeout, + }) +} + +// RuntimeConfigFilterWithOptions calls a series of hooks. But instead of +// passing container state on their standard input, +// RuntimeConfigFilterWithOptions passes the proposed runtime configuration (and +// reads back a possibly-altered form from their standard output). +func RuntimeConfigFilterWithOptions(ctx context.Context, options RuntimeConfigFilterOptions) (hookErr, err error) { + data, err := json.Marshal(options.Config) if err != nil { return nil, err } - for i, hook := range hooks { + for i, hook := range options.Hooks { hook := hook var stdout bytes.Buffer - hookErr, err = Run(ctx, &hook, data, &stdout, nil, postKillTimeout) + hookErr, err = RunWithOptions(ctx, RunOptions{Hook: &hook, Dir: options.Dir, State: data, Stdout: &stdout, PostKillTimeout: options.PostKillTimeout}) if err != nil { return hookErr, err } @@ -46,8 +71,8 @@ func RuntimeConfigFilter(ctx context.Context, hooks []spec.Hook, config *spec.Sp return nil, fmt.Errorf("unmarshal output from config-filter hook %d: %w", i, err) } - if !reflect.DeepEqual(config, &newConfig) { - oldConfig := spewConfig.Sdump(config) + if !reflect.DeepEqual(options.Config, &newConfig) { + oldConfig := spewConfig.Sdump(options.Config) newConfig := spewConfig.Sdump(&newConfig) diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ A: difflib.SplitLines(oldConfig), @@ -65,7 +90,7 @@ func RuntimeConfigFilter(ctx context.Context, hooks []spec.Hook, config *spec.Sp } } - *config = newConfig + *options.Config = newConfig } return nil, nil diff --git a/pkg/hooks/exec/runtimeconfigfilter_test.go b/pkg/hooks/exec/runtimeconfigfilter_test.go index 0e7901bc5..0553672ac 100644 --- a/pkg/hooks/exec/runtimeconfigfilter_test.go +++ b/pkg/hooks/exec/runtimeconfigfilter_test.go @@ -243,7 +243,7 @@ func TestRuntimeConfigFilter(t *testing.T) { ctx, cancel = context.WithTimeout(ctx, test.contextTimeout) defer cancel() } - hookErr, err := RuntimeConfigFilter(ctx, test.hooks, test.input, DefaultPostKillTimeout) + hookErr, err := RuntimeConfigFilterWithOptions(ctx, RuntimeConfigFilterOptions{Hooks: test.hooks, Config: test.input, PostKillTimeout: DefaultPostKillTimeout}) if test.expectedRunErrorString != "" { // We have to compare the error strings in that case because // errors.Is works differently.