-
Notifications
You must be signed in to change notification settings - Fork 182
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
Add unit test to not log stdout or stderr fields on create task request #782
Add unit test to not log stdout or stderr fields on create task request #782
Conversation
db20dea
to
8ec2104
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.
Is there a mechanism for not redacting the info? (e.g. if a debug flag is set) If so, we might want to make sure that it gets tested as well.
testLogger.Level = logrus.DebugLevel | ||
|
||
vmIsReady := make(chan struct{}) | ||
close(vmIsReady) |
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.
defer close(vmisReady)
?
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.
In this case, the channel is closed after the CreateVM
API call is made. So mocking here the VM is ready.
Not that I have found, but open to ideas there. |
8ec2104
to
cb3b0a8
Compare
// (*service).Create will fail on (Dir).CreateBundleLink after the log we want to validate. | ||
_, _ = uut.Create(ctx, createTaskRequest) | ||
|
||
for _, entry := range hook.AllEntries() { |
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.
For sanity, can you assert that your configured logger is actually logging? Like, check that it logs the task ID.
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 call, added lines 355 and 358 for this.
cb3b0a8
to
5f9ef42
Compare
In the shim logger use case, the stdout or stderr fields for create task can be the full binary URI and contain sensitive information via environment variables or parameters. Since this is not a clean solution to redact this information, it is best to not log them to disk. Signed-off-by: Austin Vazquez <[email protected]>
5f9ef42
to
135e85a
Compare
Issue #, if available:
Validates #781
Description of changes:
This change adds a unit test to validate when launching a container with the firecracker runtime the shim implementation will not log the stdout/stderr shim logger binary URI to disk.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.