daemon/server/httpstatus: add mappings for missing errdefs types#52530
Open
htoyoda18 wants to merge 1 commit into
Open
daemon/server/httpstatus: add mappings for missing errdefs types#52530htoyoda18 wants to merge 1 commit into
htoyoda18 wants to merge 1 commit into
Conversation
Add HTTP status mappings for five errdefs types that were previously unmapped, causing them to incorrectly return 500 Internal Server Error: - IsAlreadyExists → 409 Conflict - IsFailedPrecondition → 400 Bad Request - IsOutOfRange → 400 Bad Request - IsAborted → 409 Conflict - IsResourceExhausted → 429 Too Many Requests Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>
Contributor
Author
|
@thaJeztah Hi! Could you please take a look at this PR when you have a chance? Thank you! |
thaJeztah
reviewed
May 13, 2026
Comment on lines
28
to
55
| case cerrdefs.IsNotFound(rerr): | ||
| return http.StatusNotFound | ||
| case cerrdefs.IsInvalidArgument(rerr): | ||
| return http.StatusBadRequest | ||
| case cerrdefs.IsConflict(rerr): | ||
| return http.StatusConflict | ||
| case cerrdefs.IsAlreadyExists(rerr): | ||
| return http.StatusConflict | ||
| case cerrdefs.IsFailedPrecondition(rerr): | ||
| return http.StatusBadRequest | ||
| case cerrdefs.IsOutOfRange(rerr): | ||
| return http.StatusBadRequest | ||
| case cerrdefs.IsAborted(rerr): | ||
| return http.StatusConflict | ||
| case cerrdefs.IsResourceExhausted(rerr): | ||
| return http.StatusTooManyRequests | ||
| case cerrdefs.IsUnauthorized(rerr): | ||
| return http.StatusUnauthorized | ||
| case cerrdefs.IsUnavailable(rerr): | ||
| return http.StatusServiceUnavailable | ||
| case cerrdefs.IsPermissionDenied(rerr): | ||
| return http.StatusForbidden | ||
| case cerrdefs.IsNotModified(rerr): | ||
| return http.StatusNotModified | ||
| case cerrdefs.IsNotImplemented(rerr): | ||
| return http.StatusNotImplemented | ||
| case cerrdefs.IsInternal(rerr) || cerrdefs.IsDataLoss(rerr) || cerrdefs.IsDeadlineExceeded(rerr) || cerrdefs.IsCanceled(rerr): | ||
| return http.StatusInternalServerError |
Member
There was a problem hiding this comment.
Actually wondering now why we're not using the utility that was added in containerd/errdefs; we probably should check if there's any differences in mapping, and we could still keep a wrapper around it, but I think most (if not all) of these should also be handled there; https://github.com/containerd/errdefs/blob/4817405e4a3caeb7aee9dac68ed55339c59cb635/pkg/errhttp/http.go#L32-L66
thaJeztah
reviewed
May 13, 2026
Comment on lines
57
to
59
| if statusCode := statusCodeFromGRPCError(err); statusCode != http.StatusInternalServerError { | ||
| return statusCode | ||
| } |
Member
There was a problem hiding this comment.
There's also a utility for gRPC errors (but I haven't looked yet how that maps); https://github.com/containerd/errdefs/blob/4817405e4a3caeb7aee9dac68ed55339c59cb635/pkg/errgrpc/grpc.go#L205-L211
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
- What I did
Added HTTP status code mappings for five containerd errdefs error types that were previously unmapped in the
FromErrorfunction. Without these mappings, these errors incorrectly returned500 Internal Server Errorinstead of appropriate status codes.- How I did it
Added case statements in
FromError()for:IsAlreadyExists→ 409 ConflictIsFailedPrecondition→ 400 Bad RequestIsOutOfRange→ 400 Bad RequestIsAborted→ 409 ConflictIsResourceExhausted→ 429 Too Many RequestsThese mappings align with the existing gRPC error code mappings in
statusCodeFromGRPCError.Added
TestAllErrdefsTypesAreMappedto ensure all 16 containerd errdefs types are explicitly handled, preventing future regressions when new error types are added.- How to verify it
Run the new test:
go test -v ./daemon/server/httpstatus -run TestAllErrdefsTypesAreMappedAll 16 errdefs types should be tested and pass.
- Human readable description for the release notes
- Related issues
This addresses the FIXME comment at #48359 (comment) by ensuring more error types are properly mapped, reducing cases where the "Got an API for which error does not match any expected type" debug log appears.