docker: return most relevant error from docker resolution #8861
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Justin Chadwell <me@jedevc.com>
|
Hi @jedevc. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
Member
|
/ok-to-test |
dmcgowan
reviewed
Jul 25, 2023
Previously, we would return the first non-404 error from a host. This is logical, however, it can result in confusing errors to the user: - e.g. we have an HTTP host, and an HTTPS host. If the image does not exist, we return "http: server gave HTTP response to HTTPS client". This is technically correct, however, the user is easily confused - the most relevant error in this case is the 404 error. - e.g. we have a broken HTTP host that returns 5XX errors, and a HTTP host with authentication. On the request for an image, we return the 5XX error directly. However, we have a host later on which returned an authentication error which is now hidden from the user. Note: this *can* be resolved by changing the order of hosts passed in, however this requires 1. knowing ahead of time which hosts are going to return certain errors and 2. this is often not desirable, we'd prefer to use HTTPS if it's available, and only then fallback to HTTP. To resolve this, we assign each possible error during resolution a "priority" that marks how far through the image resolution process a host/path combo got. Then we return the error with the highest priority, which is much more likely to be the most relevant error to the user. The ranking of priority then is (from lowest to highest): - Underlying transport errors (TLS, TCP, etc) - 404 errors - Other 4XX/5XX errors - Manifest rejection (due to max size exceeded) Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
7827b43 to
a1cdf60
Compare
dmcgowan
approved these changes
Oct 3, 2023
Member
|
This one looks good to go! Sorry for the late response |
estesp
approved these changes
Oct 3, 2023
kiashok
added a commit
to kiashok/containerd
that referenced
this pull request
Oct 23, 2024
…h upstream containerd/main update fork-external/main branch to upstream containerd/main at commit f90f80d Related work items: containerd#8736, containerd#8861, containerd#8924, containerd#8934, containerd#9027, containerd#9076, containerd#9104, containerd#9118, containerd#9141, containerd#9155, containerd#9177, containerd#9183, containerd#9184, containerd#9186, containerd#9187, containerd#9191, containerd#9200, containerd#9205, containerd#9211, containerd#9214, containerd#9215, containerd#9221, containerd#9223, containerd#9228, containerd#9231, containerd#9234, containerd#9242, containerd#9246, containerd#9247, containerd#9251, containerd#9253, containerd#9254, containerd#9255, containerd#9268
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follows up from #3400, #3868, #5275, and possibly more that I'm missing.
Previously, we would return the first non-404 error from a host.
This is logical, however, it can result in confusing errors to the user:
e.g. we have an HTTP host, and an HTTPS host.
If the image does not exist, we return "http: server gave HTTP response to HTTPS client". This is technically correct, however, the user is easily confused - the most relevant error in this case is the 404 error.
e.g. we have a broken HTTP host that returns 5XX errors, and a HTTP host with authentication.
On the request for an image, we return the 5XX error directly. However, we have a host later on which returned an authentication error which is now hidden from the user.
Note: this can sometimes be resolved by changing the order of hosts passed in, however this requires 1. knowing ahead of time which hosts are going to return certain errors and 2. this is often not desirable, we'd prefer to use HTTPS if it's available, and only then fallback to HTTP.
To resolve this, we assign each possible error during resolution a "priority" that marks how far through the image resolution process a host/path combo got. Then we return the error with the highest priority, which is much more likely to be the most relevant error to the user.
The ranking of priority then is (from lowest to highest):
We've encountered this in BuildKit (moby/buildkit#3382 (comment), and maybe in docker/buildx#1642), I've reproduced the scenario we hit issues in and created a test case for it as well.