Skip to content

Capture StdErr in command execution error#35

Open
mayankdaruka-msft wants to merge 3 commits intomainfrom
dev/mayankdaruka/add-stderr-to-execute-err
Open

Capture StdErr in command execution error#35
mayankdaruka-msft wants to merge 3 commits intomainfrom
dev/mayankdaruka/add-stderr-to-execute-err

Conversation

@mayankdaruka-msft
Copy link

Currently, StdOut and StdErr are not captured in Kusto logs or failures for VMApps extension operations (executing install/update/remove commands for VMApps). They are however logged to the in-VM log folders.

The change here captures StdErr as part of the overall execution error for a command which is then returned to the caller of the Execute command, providing them with a more descriptive error.

In the case of the VMApps extension, it will then be able to log this error within the extension events that are collected by the Guest Agent and are displayed in Kusto. This error would also be used within the status reported by the extension for the specific application for which the command failed.

@mayankdaruka-msft mayankdaruka-msft changed the title Capture StdErr in ExtensionEventManager logs Capture StdErr in command execution error May 23, 2024
el.Info("stderr: %s", stdErrContents)

if execErr != nil {
return exitCode, errors.Wrap(execErr, string(stdErrContents))
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably shouldn't fail the command if we can't read the standard error, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I didn't change the behavior with regards to that.
If the command execution results in an error, and we are able to read the standard error, then we return a combined error. Otherwise, we just return the original error at the end of the function.

if err3 == nil {
el.InfoFromStream("stderr:", stdErrFile)
stdErrFile.Close()
el.Info("stderr: %s", stdErrContents)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, my fear here is that we've dodged a security issue with a bug that causes these events to not reach Kusto. There's a high likelihood that these events will contain security credentials.

Please check with Norberto and Simon on whether there's any plan to scrub these generically for creds. Otherwise, I fear we may have to replicate the logic we have for the BI data to scrub them here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants