-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
30ec449
4f5f7df
4627a00
b7f8508
e1a7af0
13e8715
791ca7c
31b9165
b01f929
e24d1ef
ff73f76
6d4e92d
16c12b2
3ba1c92
8f9d2c2
ae81328
fa1863a
db6c588
7e93456
84083d0
24c85de
ab3a406
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,3 @@ | ||
```release-note:improvement | ||
agent: allow users to specify files for child process stdout/stderr | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
return 1 | ||
} | ||
|
||
g.Add(func() error { | ||
return ah.Run(ctx, method) | ||
|
@@ -800,6 +804,7 @@ func (c *AgentCommand) Run(args []string) int { | |
leaseCache.SetShuttingDown(true) | ||
} | ||
cancelFunc() | ||
es.Close() | ||
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. Does this need an 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. think it would be overly defensive, or should we check anyway b/c its in a closure? 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'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 |
||
}) | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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{ | ||
|
@@ -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 ( | ||
|
@@ -380,3 +383,187 @@ func TestExecServer_Run(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func TestExecServer_LogFiles(t *testing.T) { | ||
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. From what I can tell, this only tests the |
||
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!") | ||
} | ||
} | ||
}) | ||
} | ||
} |
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.
good catch!