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

Include CommandLine in CreateProcess errors #1363

Merged

Conversation

jterry75
Copy link
Contributor

Hey team,

It can be horribly annoying without verbose logs for a user to figure out what exactly was being executed when running a command in a container. How do we feel about including the CommandLine in the main error?

Without:

ctr run --rm mcr.microsoft.com/windows/nanoserver:ltsc2022 test powershell -Command { Write-Host Here }
ctr: hcs::System::CreateProcess test: The system cannot find the file specified.: unknown

With:

ctr run --rm mcr.microsoft.com/windows/nanoserver:ltsc2022 test powershell -Command { Write-Host Here }
ctr: hcs::System::CreateProcess: powershell -Command { Write-Host Here } test: The system cannot find the file specified.: unknown

The second example more clearly illustrates to the caller that "powershell" was the file that was not found.

Signed-off-by: Justin Terry jlterry@amazon.com

@jterry75 jterry75 requested a review from a team as a code owner April 20, 2022 17:30
@jterry75 jterry75 force-pushed the jterry75/commandline_in_createprocess_error branch from 92d99f3 to 31ed047 Compare April 20, 2022 17:33
Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

lgtm, you'll need to run go mod vendor in /test

@kevpar
Copy link
Member

kevpar commented Apr 21, 2022

@helsaawy I think you looked at some cases where confidential info like creds might be included in the container command line by the user. Do you expect that to be a problem here?

@helsaawy
Copy link
Contributor

we decided not to scrub the command line, so even if we dont accept this, they will be leaked by containerd, HCS, and elsewhere in the shim

@jterry75
Copy link
Contributor Author

I was worried about the same @kevpar. Based on @helsaawy's comment it seems safe to take?

@helsaawy
Copy link
Contributor

I was worried about the same @kevpar. Based on @helsaawy's comment it seems safe to take?

Yup, we log the process parameters here:

if s, err := log.ScrubProcessParameters(processParameters); err == nil {
span.AddAttributes(trace.StringAttribute("processParameters", s))
}

But its just the environment we remove

@jterry75
Copy link
Contributor Author

Ok sweeeeeeet. I guess there is a go mod issue. I will fix that today.

@jterry75 jterry75 force-pushed the jterry75/commandline_in_createprocess_error branch from 31ed047 to 16cd78e Compare April 26, 2022 21:08
Signed-off-by: Justin Terry <jlterry@amazon.com>
@jterry75 jterry75 force-pushed the jterry75/commandline_in_createprocess_error branch from 16cd78e to ce5cb3d Compare April 26, 2022 21:15
@ghost
Copy link

ghost commented Apr 26, 2022

CLA assistant check
All CLA requirements met.

@jterry75
Copy link
Contributor Author

@dcantah - OH. The in /test part matters a lot to the context :)

@dcantah dcantah merged commit f6694ad into microsoft:master Apr 26, 2022
@jterry75 jterry75 deleted the jterry75/commandline_in_createprocess_error branch April 28, 2022 17:07
anmaxvl added a commit that referenced this pull request Feb 7, 2023
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
Signed-off-by: Justin Terry <jlterry@amazon.com>
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.

4 participants