-
Notifications
You must be signed in to change notification settings - Fork 259
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
Include CommandLine in CreateProcess errors #1363
Conversation
92d99f3
to
31ed047
Compare
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, you'll need to run go mod vendor in /test
@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? |
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 |
Yup, we log the process parameters here: hcsshim/internal/vmcompute/vmcompute.go Lines 397 to 399 in 113a929
But its just the environment we remove |
Ok sweeeeeeet. I guess there is a go mod issue. I will fix that today. |
31ed047
to
16cd78e
Compare
Signed-off-by: Justin Terry <jlterry@amazon.com>
16cd78e
to
ce5cb3d
Compare
@dcantah - OH. The in |
Signed-off-by: Justin Terry <jlterry@amazon.com>
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:
With:
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