Skip to content
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

VAULT-19681 allow users to specify files for agent child process stdout/stderr #22812

Merged
merged 22 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/22812.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
agent: allow users to specify files for child process stdout/stderr
```
7 changes: 6 additions & 1 deletion command/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -729,13 +729,17 @@ func (c *AgentCommand) Run(args []string) int {
ExitAfterAuth: config.ExitAfterAuth,
})

es := exec.NewServer(&exec.ServerConfig{
es, err := exec.NewServer(&exec.ServerConfig{
AgentConfig: c.config,
Namespace: templateNamespace,
Logger: c.logger.Named("exec.server"),
LogLevel: c.logger.GetLevel(),
LogWriter: c.logWriter,
})
if err != nil {
c.logger.Error("could not create exec server", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

return 1
}

g.Add(func() error {
return ah.Run(ctx, method)
Expand Down Expand Up @@ -800,6 +804,7 @@ func (c *AgentCommand) Run(args []string) int {
leaseCache.SetShuttingDown(true)
}
cancelFunc()
es.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need an if es != nil or would that be overly defensive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think it would be overly defensive, es would only be nil if there was an error opening one of the log files and in that case would exit early

or should we check anyway b/c its in a closure?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it up to you -- I think it wouldn't hurt, but like you said, I can't really see how we'd get to that point

})

}
Expand Down
2 changes: 2 additions & 0 deletions command/agent/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ type ExecConfig struct {
Command []string `hcl:"command,attr" mapstructure:"command"`
RestartOnSecretChanges string `hcl:"restart_on_secret_changes,optional" mapstructure:"restart_on_secret_changes"`
RestartStopSignal os.Signal `hcl:"-" mapstructure:"restart_stop_signal"`
ChildProcessStdout string `mapstructure:"child_process_stdout"`
ChildProcessStderr string `mapstructure:"child_process_stderr"`
averche marked this conversation as resolved.
Show resolved Hide resolved
}

func NewConfig() *Config {
Expand Down
55 changes: 48 additions & 7 deletions command/agent/exec/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@ type Server struct {

logger hclog.Logger

childProcess *child.Child
childProcessState childProcessState
childProcessLock sync.Mutex
childProcess *child.Child
childProcessState childProcessState
childProcessLock sync.Mutex
childProcessStdout io.WriteCloser
childProcessStderr io.WriteCloser

// exit channel of the child process
childProcessExitCh chan int
Expand All @@ -85,15 +87,38 @@ func (e *ProcessExitError) Error() string {
return fmt.Sprintf("process exited with %d", e.ExitCode)
}

func NewServer(cfg *ServerConfig) *Server {
func NewServer(cfg *ServerConfig) (*Server, error) {
var err error

childProcessStdout := os.Stdout
childProcessStderr := os.Stderr

if cfg.AgentConfig.Exec != nil {
if cfg.AgentConfig.Exec.ChildProcessStdout != "" {
childProcessStdout, err = os.OpenFile(cfg.AgentConfig.Exec.ChildProcessStdout, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
if err != nil {
return nil, fmt.Errorf("could not open %q, %w", cfg.AgentConfig.Exec.ChildProcessStdout, err)
}
}

if cfg.AgentConfig.Exec.ChildProcessStderr != "" {
childProcessStderr, err = os.OpenFile(cfg.AgentConfig.Exec.ChildProcessStderr, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
if err != nil {
return nil, fmt.Errorf("could not open %q, %w", cfg.AgentConfig.Exec.ChildProcessStdout, err)
}
}
}

server := Server{
logger: cfg.Logger,
config: cfg,
childProcessState: childProcessStateNotStarted,
childProcessExitCh: make(chan int),
childProcessStdout: childProcessStdout,
childProcessStderr: childProcessStderr,
}

return &server
return &server, nil
}

func (s *Server) Run(ctx context.Context, incomingVaultToken chan string) error {
Expand Down Expand Up @@ -152,6 +177,7 @@ func (s *Server) Run(ctx context.Context, incomingVaultToken chan string) error
s.childProcess.Stop()
}
s.childProcessState = childProcessStateStopped
s.close()
s.childProcessLock.Unlock()
return nil

Expand Down Expand Up @@ -291,8 +317,8 @@ func (s *Server) restartChildProcess(newEnvVars []string) error {

childInput := &child.NewInput{
Stdin: os.Stdin,
Stdout: os.Stdout,
Stderr: os.Stderr,
Stdout: s.childProcessStdout,
Stderr: s.childProcessStderr,
Command: args[0],
Args: args[1:],
Timeout: 0, // let it run forever
Expand Down Expand Up @@ -333,3 +359,18 @@ func (s *Server) restartChildProcess(newEnvVars []string) error {

return nil
}

func (s *Server) Close() {
s.childProcessLock.Lock()
defer s.childProcessLock.Unlock()
s.close()
}

func (s *Server) close() {
if s.childProcessStdout != os.Stdout {
_ = s.childProcessStdout.Close()
}
if s.childProcessStderr != os.Stderr {
_ = s.childProcessStderr.Close()
}
}
189 changes: 188 additions & 1 deletion command/agent/exec/exec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func TestExecServer_Run(t *testing.T) {
strconv.Itoa(testCase.testAppPort),
}

execServer := NewServer(&ServerConfig{
execServer, err := NewServer(&ServerConfig{
Logger: logging.NewVaultLogger(hclog.Trace),
AgentConfig: &config.Config{
Vault: &config.Vault{
Expand All @@ -280,6 +280,9 @@ func TestExecServer_Run(t *testing.T) {
LogLevel: hclog.Trace,
LogWriter: hclog.DefaultOutput,
})
if err != nil {
t.Fatalf("could not create exec server: %q", err)
}

// start the exec server
var (
Expand Down Expand Up @@ -380,3 +383,187 @@ func TestExecServer_Run(t *testing.T) {
})
}
}

func TestExecServer_LogFiles(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, this only tests the stderr case -- we should add a test for stdout too.

goBinary, err := exec.LookPath("go")
if err != nil {
t.Fatalf("could not find go binary on path: %s", err)
}

testAppBinary := filepath.Join(os.TempDir(), "test-app")

if err := exec.Command(goBinary, "build", "-o", testAppBinary, "./test-app").Run(); err != nil {
t.Fatalf("could not build the test application: %s", err)
}
t.Cleanup(func() {
if err := os.Remove(testAppBinary); err != nil {
t.Fatalf("could not remove %q test application: %s", testAppBinary, err)
}
})

testCases := map[string]struct {
testAppPort int
testAppArgs []string
stderrFile string
stdoutFile string

expectedError error
}{
"can_log_stderr_to_file": {
testAppPort: 34001,
stderrFile: "vault-exec-test.stderr.log",
},
"can_log_stdout_to_file": {
testAppPort: 34002,
stdoutFile: "vault-exec-test.stdout.log",
testAppArgs: []string{"-log-to-stdout"},
},
"cant_open_file": {
testAppPort: 34003,
stderrFile: "/file/does/not/exist",
expectedError: os.ErrNotExist,
VioletHynes marked this conversation as resolved.
Show resolved Hide resolved
},
}

for tcName, testCase := range testCases {
t.Run(tcName, func(t *testing.T) {
fakeVault := fakeVaultServer(t)
defer fakeVault.Close()

testAppCommand := []string{
testAppBinary,
"--port",
strconv.Itoa(testCase.testAppPort),
"--stop-after",
"60s",
}

execConfig := &config.ExecConfig{
RestartOnSecretChanges: "always",
Command: append(testAppCommand, testCase.testAppArgs...),
}

if testCase.stdoutFile != "" {
execConfig.ChildProcessStdout = filepath.Join(os.TempDir(), "vault-agent-exec.stdout.log")
t.Cleanup(func() {
_ = os.Remove(execConfig.ChildProcessStdout)
})
}

if testCase.stderrFile != "" {
execConfig.ChildProcessStderr = filepath.Join(os.TempDir(), "vault-agent-exec.stderr.log")
t.Cleanup(func() {
_ = os.Remove(execConfig.ChildProcessStderr)
})
}

execServer, err := NewServer(&ServerConfig{
Logger: logging.NewVaultLogger(hclog.Trace),
AgentConfig: &config.Config{
Vault: &config.Vault{
Address: fakeVault.URL,
Retry: &config.Retry{
NumRetries: 3,
},
},
Exec: execConfig,
EnvTemplates: []*ctconfig.TemplateConfig{{
Contents: pointerutil.StringPtr(`{{ with secret "kv/my-app/creds" }}{{ .Data.data.user }}{{ end }}`),
MapToEnvironmentVariable: pointerutil.StringPtr("MY_USER"),
}},
TemplateConfig: &config.TemplateConfig{
ExitOnRetryFailure: true,
StaticSecretRenderInt: 5 * time.Second,
},
},
LogLevel: hclog.Trace,
LogWriter: hclog.DefaultOutput,
})
if err != nil {
if testCase.expectedError != nil {
if errors.Is(err, testCase.expectedError) {
t.Log("test passes! caught expected err")
return
} else {
t.Fatalf("caught error %q did not match expected error %q", err, testCase.expectedError)
}
}
t.Fatalf("could not create exec server: %q", err)
}

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

// start the exec server
var (
execServerErrCh = make(chan error)
execServerTokenCh = make(chan string, 1)
)
go func() {
execServerErrCh <- execServer.Run(ctx, execServerTokenCh)
}()

// send a dummy token to kick off the server
execServerTokenCh <- "my-token"

// ensure the test app is running after 500ms
var (
testAppAddr = fmt.Sprintf("http://localhost:%d", testCase.testAppPort)
testAppStartedCh = make(chan error)
)
time.AfterFunc(500*time.Millisecond, func() {
_, err := retryablehttp.Head(testAppAddr)
testAppStartedCh <- err
})

select {
case <-ctx.Done():
t.Fatal("timeout reached before templates were rendered")

case err := <-execServerErrCh:
if testCase.expectedError == nil && err != nil {
t.Fatalf("exec server did not expect an error, got: %v", err)
}

if errors.Is(err, testCase.expectedError) {
t.Fatalf("exec server expected error %v; got %v", testCase.expectedError, err)
}

t.Log("exec server exited without an error")

return

case <-testAppStartedCh:
t.Log("test app started successfully")
}

// let the app run a bit
time.Sleep(5 * time.Second)
// stop the app
cancel()
// wait for app to stop
time.Sleep(5 * time.Second)

// check if the files have content
if testCase.stdoutFile != "" {
stdoutInfo, err := os.Stat(execConfig.ChildProcessStdout)
if err != nil {
t.Fatalf("error calling stat on stdout file: %q", err)
}
if stdoutInfo.Size() == 0 {
t.Fatalf("stdout log file does not have any data!")
}
}

if testCase.stderrFile != "" {
stderrInfo, err := os.Stat(execConfig.ChildProcessStderr)
if err != nil {
t.Fatalf("error calling stat on stderr file: %q", err)
}
if stderrInfo.Size() == 0 {
t.Fatalf("stderr log file does not have any data!")
}
}
})
}
}
14 changes: 10 additions & 4 deletions command/agent/exec/test-app/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ import (

var (
port uint
ignoreStopSignal bool
sleepAfterStopSignal time.Duration
useSigusr1StopSignal bool
stopAfter time.Duration
exitCode int
logToStdout bool
)

func init() {
Expand All @@ -42,6 +42,7 @@ func init() {
flag.BoolVar(&useSigusr1StopSignal, "use-sigusr1", false, "use SIGUSR1 as the stop signal, instead of the default SIGTERM")
flag.DurationVar(&stopAfter, "stop-after", 0, "stop the process after duration (overrides all other flags if set)")
flag.IntVar(&exitCode, "exit-code", 0, "exit code to return when this script exits")
flag.BoolVar(&logToStdout, "log-to-stdout", false, "log to stdout instead of stderr")
}

type Response struct {
Expand Down Expand Up @@ -78,8 +79,15 @@ func handler(w http.ResponseWriter, r *http.Request) {
}

func main() {
logger := log.New(os.Stderr, "test-app: ", log.LstdFlags)
flag.Parse()

logOut := os.Stderr
if logToStdout {
logOut = os.Stdout
}
logger := log.New(logOut, "test-app: ", log.LstdFlags)

logger.Printf("running on port %d", port)
if err := run(logger); err != nil {
log.Fatalf("error: %v\n", err)
}
Expand All @@ -96,8 +104,6 @@ func run(logger *log.Logger) error {
ctx, cancelContextFunc := context.WithTimeout(context.Background(), 60*time.Second)
defer cancelContextFunc()

flag.Parse()

server := http.Server{
Addr: fmt.Sprintf(":%d", port),
Handler: http.HandlerFunc(handler),
Expand Down