Merge pull request #1521 from fangpenlin/add-cwd-argument-for-hook-ex…

…ec-run-with-api-break-free-changes

Add cwd argument for hooks exec Run func (API break-free version)

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()

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)
 }

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

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.