-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
CI Results: |
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) |
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!
@@ -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 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?
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.
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?
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.
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
@@ -380,3 +385,164 @@ 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 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.
Build Results: |
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.
Looks reasonable to me. Could you please add "agent" to the title/description.
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>
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.
Looks good! Thanks for this! Feel free to tag me on the docs PR, too!
command/agent/exec/exec.go
Outdated
_ = s.childProcessStdout.Close() | ||
_ = s.childProcessStderr.Close() |
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.
Would this potentially close the global os.Stdout and os.Stderr? https://go.dev/play/p/ksHYajR4CVU
Perhaps you can check if s.childProcessStdout != os.Stdout { ... }
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.
Yes! why I had it in the first place.
will add back the bools
(think its messing with tests anyway)
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.
I like the if != os.Stdout approach a little more, if we're guarding against this -- the bools are a little confusing, I think
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.
^^ fair, will do that
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.
LGTM 👍
Adds ability for the user to send stderr and stdout of the child process to files