Fix tainted stderr on docker exec #5852
Conversation
Signed-off-by: Fabio Pugliese Ornellas <fabio.ornellas@gmail.com>
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #5852 +/- ##
==========================================
+ Coverage 58.86% 58.98% +0.12%
==========================================
Files 349 343 -6
Lines 29562 29303 -259
==========================================
- Hits 17401 17285 -116
+ Misses 11180 11043 -137
+ Partials 981 975 -6 |
There was a problem hiding this comment.
Discussed in the maintainer's call; this behavior is unintended and was flagged as a regression after merging the last PR. The implementation here is a bit messy (the StatusError/exitCode abstraction is leaking badly), but it's no worse than it was before @thaJeztah attempted to refactor here.
Let's take this now to fix the obvious change in behavior, and flag this area of the code for another round of clean-up later.
| @@ -43,7 +43,10 @@ func main() { | |||
| } | |||
|
|
|||
| if err != nil && !errdefs.IsCancelled(err) { | |||
| _, _ = fmt.Fprintln(os.Stderr, err) | |||
| var stErr cli.StatusError | |||
| if errors.As(err, &stErr) && stErr.Status != "" { | |||
There was a problem hiding this comment.
This is filtering all non-StatusErrors now 😅 I think it needs to be something like this:
| if errors.As(err, &stErr) && stErr.Status != "" { | |
| if !errors.As(err, &stErr) || stErr.Status != "" { |
There was a problem hiding this comment.
Good catch; this looked plausible at first glance as it was identical to the removed code, but at this spot in the code we need to adjust the conditional.
There was a problem hiding this comment.
Looks like this isn't quite it, as we seem to have actually added the exit status as expected to some tests. I'll probably have to spend a bit of time figuring out how much of that is intended/what the expected behavior is here.
I suspect it is to print 'exit status Y' for CLI plugins, but not for attach/exec; however that might require a new type as both current are a StatusError...
There was a problem hiding this comment.
I'll take a look at it soon.
After a quick look, we shouldn't update the code here. We'll break too many other things in the process.
There was a problem hiding this comment.
@neersighted please see this PR #5854
|
Thanks @fornellas ! We merged a fix in #5854, but added you as "co-author" on the commit; thank you so much for contributing! |
- What I did
#5229 (cc @thaJeztah) seems to have introduced a bug, that makes
docker exectaint stderr, by printingexit status Xwhen a command is run. This was not the case before, and introduces issues with anything that depends on using docker exec to run commands and process the output (something I do a lot when running tests). So, this new behavior breaks this previous contract, and goes against how analog *nix commands work (eg:sh -c false).Here's an example. Given we have a container started with
docker run -ti --rm --name ubuntu ubuntu, before we could do:and it'd output correct stdout & stderr and exit 2 (coming from
ls).After #5229, things changed to:
so there's an extra line on stderr
exit status 2.- How I did it
This PR restores the previous functional behavior, by adding an extra if to handle this case, exactly it was handled before, so I'd not expect it to have any unintended side effects.
- How to verify it
I've bisected the change to the bad commit #5229, validated the before / after with the commands above, and that this PR fixes the issue.
- Human readable description for the release notes