-
Notifications
You must be signed in to change notification settings - Fork 2
[WIP -- looking for feedback] Unified Exec Wrapper #445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
995d851
2dc5e04
f3bf489
0ab0b1a
8672cdb
018e6ed
f59a43e
0601540
31f5594
6a31401
bc9aa3d
86f5abc
eb1fa05
fce36ca
5c4dbaa
12a67af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,349 @@ | ||
// Copyright 2023 The Authors (see AUTHORS file) | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
// Package run provides enhanced functionality for executing external commands, | ||
// offering control over environment variables, process attributes, I/O, | ||
// timeouts, and integrated logging, using a functional options pattern. | ||
package run | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
"errors" | ||
"fmt" | ||
"io" | ||
"os" | ||
"os/exec" | ||
"path/filepath" | ||
"strings" | ||
"time" | ||
|
||
"github.com/abcxyz/pkg/logging" // Import the specific logging package | ||
) | ||
|
||
// DefaultRunTimeout is how long commands wait if the context doesn't have a deadline. | ||
const DefaultRunTimeout = time.Minute | ||
|
||
// DefaultWaitDelay is how long after context cancellation processes are actually killed | ||
// if another value isn't set. See exec.Cmd.WaitDelay for more information. | ||
const DefaultWaitDelay = time.Second | ||
|
||
// Simple is a wrapper around [Run] that captures stdout and stderr as strings. | ||
// This is intended to be used for commands that run non-interactively then | ||
// exit. | ||
// | ||
// If the command exits with a nonzero status code, an *exec.ExitError will be | ||
// returned. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd also clarify that it's only good for commands that produce minimal output that can be represented as a string (since we're serializing everything to a string). If there's a command that outputs bytes, this won't work and would need a more complex version. We could fix half that problem by returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't you just cast the string back to a []byte if that was an issue? My understanding is go strings are just []byte under the hood, and are not required to hold valid utf-8 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Casting copies the data... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about this:
I feel like in either case (binary or string output) if its large enough to care about copies, probably good idea to stream output anyways. |
||
// | ||
// If the command fails, the error message will include the contents of stdout | ||
// and stderr. This saves boilerplate in the caller. | ||
func Simple(ctx context.Context, args ...string) (stdout, stderr string, _ error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these return values just named for the purposes of docs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, not sure if there is a better way to do this |
||
var stdoutBuf, stderrBuf bytes.Buffer | ||
opts := []*Option{ | ||
WithStdout(&stdoutBuf), | ||
WithStderr(&stderrBuf), | ||
} | ||
|
||
_, err := Run(ctx, opts, args...) | ||
|
||
return stdoutBuf.String(), stderrBuf.String(), err | ||
} | ||
|
||
// Run executes the command specified by args, applying configurations from opts. | ||
// | ||
// By default, a non-zero exit code results in an error (*exec.ExitError), | ||
// unless the AllowNonzeroExit option is used. | ||
// The error message includes stdout/stderr content if they are captured to buffers. | ||
// | ||
// If the context doesn't have a deadline, DefaultRunTimeout is applied. | ||
// | ||
// The input args must have len>=1. opts may be nil if no special options are | ||
// needed. | ||
// | ||
// This doesn't execute a shell (unless of course args[0] is the name of a shell | ||
// binary). | ||
func Run(ctx context.Context, opts []*Option, args ...string) (exitCode int, _ error) { | ||
logger := logging.FromContext(ctx) | ||
|
||
if len(args) == 0 { | ||
logger.DebugContext(ctx, "run called with no arguments") | ||
return -1, errors.New("run: must provide at least one argument (the command)") | ||
} | ||
|
||
// Apply default timeout if none exists on the context. | ||
if _, ok := ctx.Deadline(); !ok { | ||
var cancel context.CancelFunc | ||
ctx, cancel = context.WithTimeout(ctx, DefaultRunTimeout) | ||
defer cancel() // Ensure the derived context is cancelled | ||
} | ||
|
||
compiledOpts := compileOpts(opts) | ||
|
||
// #nosec G204 -- Execution of external commands is the purpose of this package. Inputs must be trusted by the caller. | ||
cmd := exec.CommandContext(ctx, args[0], args[1:]...) | ||
|
||
cmd.Dir = compiledOpts.cwd | ||
cmd.Stdin = compiledOpts.stdin | ||
// Handle stdout/stderr, capturing output if writers are bytes.Buffer for error reporting. | ||
var stdoutBuf, stderrBuf *bytes.Buffer | ||
if compiledOpts.stdout != nil { | ||
cmd.Stdout = compiledOpts.stdout | ||
if bb, ok := compiledOpts.stdout.(*bytes.Buffer); ok { | ||
stdoutBuf = bb | ||
} | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
cmd.Stdout = os.Stdout | ||
} | ||
if compiledOpts.stderr != nil { | ||
cmd.Stderr = compiledOpts.stderr | ||
if bb, ok := compiledOpts.stderr.(*bytes.Buffer); ok { | ||
stderrBuf = bb | ||
} | ||
} else { | ||
cmd.Stderr = os.Stderr | ||
} | ||
|
||
useCustomEnv := len(compiledOpts.allowedEnvKeys) > 0 || len(compiledOpts.deniedEnvKeys) > 0 || len(compiledOpts.additionalEnv) > 0 | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if useCustomEnv { | ||
currentEnv := os.Environ() | ||
logger.DebugContext(ctx, "calculating custom environment", | ||
"inherited_count", len(currentEnv), | ||
"allowed_patterns", compiledOpts.allowedEnvKeys, | ||
"denied_patterns", compiledOpts.deniedEnvKeys, | ||
"additional_vars_count", len(compiledOpts.additionalEnv)) | ||
env := environ(currentEnv, compiledOpts.allowedEnvKeys, compiledOpts.deniedEnvKeys, compiledOpts.additionalEnv) | ||
logger.DebugContext(ctx, "computed environment", "env", env) | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cmd.Env = env | ||
} else { | ||
logger.DebugContext(ctx, "using inherited environment") | ||
} | ||
|
||
if compiledOpts.waitDelay != nil { | ||
cmd.WaitDelay = *compiledOpts.waitDelay | ||
} else { | ||
cmd.WaitDelay = DefaultWaitDelay | ||
} | ||
logger.DebugContext(ctx, "set wait delay", "delay", cmd.WaitDelay) | ||
|
||
logger.DebugContext(ctx, "starting command", | ||
"args", args, | ||
"options", compiledOpts, | ||
) | ||
err := cmd.Run() // This blocks until the command exits or context is cancelled | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is the right pattern. There's no way to interrupt or signal the command like this, and it blocks all execution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because its using a commandcontext, canceling the context will send a stop signal. As far as blocking goes, I think that should be fine, if someone really cares they can always wrap in a goroutine. I could see an argument if you feel like there will likely need to be other signals sent to the command, in which case it would be nice to get the PID back somehow. |
||
|
||
// Exit code -1 if ProcessState is nil (e.g., start failed) | ||
exitCode = -1 | ||
sethvargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if cmd.ProcessState != nil { | ||
exitCode = cmd.ProcessState.ExitCode() | ||
} | ||
|
||
if err != nil { | ||
var exitErr *exec.ExitError | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
isExitError := errors.As(err, &exitErr) | ||
|
||
if isExitError && compiledOpts.allowNonZeroExit { | ||
logger.DebugContext(ctx, "command exited non-zero, but allowed by option", "exit_code", exitCode) | ||
err = nil | ||
} else if isExitError { | ||
stdoutContent := "[stdout not captured]" | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if stdoutBuf != nil { | ||
stdoutContent = stdoutBuf.String() | ||
} | ||
stderrContent := "[stderr not captured]" | ||
if stderrBuf != nil { | ||
stderrContent = stderrBuf.String() | ||
} | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
err = fmt.Errorf("command %v exited non-zero (%d): %w (context error: %w)\nstdout:\n%s\nstderr:\n%s", | ||
args, exitCode, err, ctx.Err(), stdoutContent, stderrContent) | ||
logger.DebugContext(ctx, "command exited non-zero", | ||
"exit_code", exitCode, | ||
"error", err, | ||
) | ||
} else { | ||
// It's an actual execution error (e.g., command not found, context cancelled early) | ||
stdoutContent := "[stdout not captured]" | ||
if stdoutBuf != nil { | ||
stdoutContent = stdoutBuf.String() | ||
} | ||
stderrContent := "[stderr not captured]" | ||
if stderrBuf != nil { | ||
stderrContent = stderrBuf.String() | ||
} | ||
err = fmt.Errorf("command %v failed: %w (context error: %w)\nstdout:\n%s\nstderr:\n%s", | ||
args, err, ctx.Err(), stdoutContent, stderrContent) | ||
logger.DebugContext(ctx, "command failed with execution error", | ||
"exit_code", exitCode, | ||
"error", err, | ||
) | ||
} | ||
} else { | ||
// Command ran successfully (exit code 0) | ||
logger.DebugContext(ctx, "command finished successfully", "exit_code", exitCode) | ||
} | ||
|
||
return exitCode, err | ||
} | ||
|
||
// Option implements the functional options pattern for [Run]. | ||
// It holds all configurable settings for executing a command. | ||
type Option struct { | ||
// Basic execution control | ||
allowNonZeroExit bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be more generic with "allowed exit codes" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, that seems fine, but runs to the question: what if you want to allow all exit codes? Do we need a separate flag? Do we accept a magic value that means all? Do we accept a slice of strings so we can have some wildcard? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I typed a reply to this, but idk where it went... I think we can accept a slice of allowed exit codes. If the slice is empty, we expect 0. Otherwise we only accept the codes in the list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about int[]{-1} representing "accept any exit code" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's any use case for "allow any exit code"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/abcxyz/abc/blob/main/templates/common/run/diff.go#L85 Its being used by abc currently to simplify error handling (you don't need to cast the error to check if its exit code vs some other type) |
||
cwd string | ||
stdin io.Reader | ||
stdout io.Writer | ||
stderr io.Writer | ||
waitDelay *time.Duration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be a pointer (it's an int64) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its a pointer as its an optional value where the default value has semantic meaning (Wait forever) different from the default behaivor we want (set to DefaultWaitDelay const) That is assuming we want DefaultWaitDelay as a default, rather than cmd's normal behavior There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default value for an int (and time.Duration) is 0, so it's functionally equivalent? Or are you trying to differentiate between "0" and "unset"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Default value for int* is nil, which will cause 0 is already well defiend by cmd.Run:
which isn't the default behavior in guardian's library, assumed that behavior was wanted: https://github.com/abcxyz/guardian/blob/main/pkg/child/child.go#L93 |
||
|
||
allowedEnvKeys []string | ||
deniedEnvKeys []string | ||
additionalEnv []string | ||
} | ||
|
||
// AllowNonzeroExit prevents Run from returning an error | ||
// when the command exits with a non-zero status code. | ||
func AllowNonzeroExit() *Option { | ||
return &Option{allowNonZeroExit: true} | ||
} | ||
|
||
// WithStdin provides the given reader as the command's | ||
// standard input. | ||
func WithStdin(stdin io.Reader) *Option { | ||
return &Option{stdin: stdin} | ||
} | ||
|
||
// WithStdinStr is a convenience option that uses the given string as the | ||
// command's standard input. | ||
func WithStdinStr(stdin string) *Option { | ||
return WithStdin(bytes.NewBufferString(stdin)) | ||
} | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// WithStdout directs the command's standard output | ||
// to the given writer. | ||
func WithStdout(stdout io.Writer) *Option { | ||
return &Option{stdout: stdout} | ||
} | ||
|
||
// WithStderr directs the command's standard error | ||
// to the given writer. | ||
func WithStderr(stderr io.Writer) *Option { | ||
return &Option{stderr: stderr} | ||
} | ||
|
||
// WithCwd runs the command in the specified working directory. | ||
func WithCwd(cwd string) *Option { | ||
return &Option{cwd: cwd} | ||
} | ||
|
||
// WithFilteredEnv filters the inherited environment variables. | ||
// allowed is a list of glob patterns for keys to keep (empty means allow all initially). | ||
// denied is a list of glob patterns for keys to remove (takes precedence). | ||
func WithFilteredEnv(allowed, denied []string) *Option { | ||
return &Option{allowedEnvKeys: allowed, deniedEnvKeys: denied} | ||
} | ||
|
||
// WithAdditionalEnv returns an option that adds or overrides environment variables. | ||
// vars is a slice of strings in "KEY=VALUE" format. Can be added multiple times. | ||
func WithAdditionalEnv(vars []string) *Option { | ||
return &Option{additionalEnv: vars} | ||
} | ||
|
||
// WithWaitDelay sets a grace period for the command to exit after the context | ||
// is cancelled before being forcefully killed. | ||
// Default: DefautWaitDelay. | ||
// See exec.Cmd.WaitDelay for more information. | ||
func WithWaitDelay(d time.Duration) *Option { | ||
return &Option{waitDelay: &d} | ||
} | ||
|
||
// TODO: handle process group and cancel semantics. | ||
// WithProcessGroup returns an option that attempts to run the command in a new | ||
// process group (primarily effective on Unix-like systems). | ||
//func WithProcessGroup(set bool) *Option { | ||
// return &Option{setProcessGroup: set} | ||
//} | ||
|
||
// The last option specified for most fields takes precedence. | ||
// additionalEnv is appended across options. | ||
func compileOpts(opts []*Option) *Option { | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
out := Option{} | ||
|
||
for _, opt := range opts { | ||
if opt.allowNonZeroExit { | ||
out.allowNonZeroExit = true | ||
} | ||
if opt.cwd != "" { | ||
out.cwd = opt.cwd | ||
} | ||
if opt.stdin != nil { | ||
out.stdin = opt.stdin | ||
} | ||
if opt.stdout != nil { | ||
out.stdout = opt.stdout | ||
} | ||
if opt.stderr != nil { | ||
out.stderr = opt.stderr | ||
} | ||
if opt.waitDelay != nil { | ||
out.waitDelay = opt.waitDelay | ||
} | ||
if opt.allowedEnvKeys != nil { | ||
out.allowedEnvKeys = opt.allowedEnvKeys | ||
} | ||
if opt.deniedEnvKeys != nil { | ||
out.deniedEnvKeys = opt.deniedEnvKeys | ||
} | ||
if len(opt.additionalEnv) > 0 { | ||
out.additionalEnv = append(out.additionalEnv, opt.additionalEnv...) | ||
} | ||
} | ||
return &out | ||
} | ||
|
||
// anyGlobMatch checks if string s matches any of the glob patterns. | ||
func anyGlobMatch(s string, patterns []string) bool { | ||
for _, p := range patterns { | ||
if p == "*" || p == ".*" { // Common fast-path wildcards | ||
return true | ||
} | ||
} | ||
for _, p := range patterns { | ||
if ok, _ := filepath.Match(p, s); ok { // Ignore Match errors | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
|
||
// environ compiles the appropriate environment to pass to the child process. | ||
// The overridden environment is always added, even if not explicitly | ||
// allowed/denied. | ||
func environ(osEnv, allowedKeys, deniedKeys, overrideEnv []string) []string { | ||
finalEnv := make([]string, 0, len(osEnv)+len(overrideEnv)) | ||
|
||
// Select keys that match the allow filter (if given) but not the deny filter. | ||
for _, v := range osEnv { | ||
k := strings.SplitN(v, "=", 2)[0] | ||
if (len(allowedKeys) == 0 || anyGlobMatch(k, allowedKeys)) && | ||
!anyGlobMatch(k, deniedKeys) { | ||
finalEnv = append(finalEnv, v) | ||
} | ||
} | ||
|
||
// Add overrides at the end, after any filtering. os/exec has "last wins" | ||
// semantics, so appending is an overriding action. | ||
finalEnv = append(finalEnv, overrideEnv...) | ||
|
||
return finalEnv | ||
} | ||
pdewilde marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RunTimeout
andDefaultWaitDelay
have different units, do you think it might be more user friendly to change this to60 * time.Second
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The are both time.Duration which under the hood is in nanoseconds.
They are actually just
IDK if 60 * time.Second is easier to grok than time.Mintue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are all int64 under the hood so it doesn't matter. I do wonder why we're setting such a long default? It seems like most commands should finish in like < 5s, or manually have no timeout. We also want the ability to disable the timeout entirely, since we rely on the subcommand to handle the timeouts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/abcxyz/abc/blob/49babab96513de693386e38b4ca1ff6a335aeeb1/templates/common/run/run.go#L30
I kept the default from
abc
. I assume the thinking there was it likely would be calling a long-running process (compilation or something similar).Maybe for a general library that should be shorter though