[release/1.5 backport] pushWriter: correctly propagate errors#7998
Conversation
In the refactor from 926b9c7, the error handling was substantially reworked, and changed the types of errors returned. Notably, in the case of a network error, instead of propogating the error through to return from pushWriter.Write (as previously), it would be propagated through to pushWriter.Commit - however, this is too late, since we've already closed the io.Pipe by the time we would have reached this function. Therefore, we get the generic error message "io: read/write on closed pipe" for *every network error*. This patch corrects this behavior to ensure that the correct error object is always returned as early as possible, by checking the error result after writing and detecting a closed pipe. Additionally, we do some additional hardening - specifically we prevent falling through when resetting the content or detecting errors, and update the tests to explicitly check for the ErrReset message. Signed-off-by: Justin Chadwell <[email protected]> (cherry picked from commit 9f6058d) Signed-off-by: Sebastiaan van Stijn <[email protected]>
| if _, err := pw.pipe.Write([]byte{}); err != nil && !errors.Is(err, io.ErrClosedPipe) { | ||
| return errors.Wrap(err, "pipe error before commit") | ||
| } |
There was a problem hiding this comment.
Minor conflict in adjacent line here, as release/1.5 is still using pkg/errors here
|
I missed that the regression was also backported to 1.5, so opening a PR for this branch as well (if there's gonna be another 1.5 release) /cc @jedevc @tonistiigi |
|
@thaJeztah: GitHub didn't allow me to request PR reviews from the following users: jedevc. Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
4 participants
ctr images pushwhile pushing large images #7972In the refactor from 926b9c7, the error handling was substantially reworked, and changed the types of errors returned.
Notably, in the case of a network error, instead of propogating the error through to return from pushWriter.Write (as previously), it would be propagated through to pushWriter.Commit - however, this is too late, since we've already closed the io.Pipe by the time we would have reached this function. Therefore, we get the generic error message "io: read/write on closed pipe" for every network error.
This patch corrects this behavior to ensure that the correct error object is always returned as early as possible, by checking the error result after writing and detecting a closed pipe.
Additionally, we do some additional hardening - specifically we prevent falling through when resetting the content or detecting errors, and update the tests to explicitly check for the ErrReset message.
(cherry picked from commit 9f6058d)