Skip to content

Conversation

@hiifong
Copy link
Member

@hiifong hiifong commented Dec 30, 2024

Use UNSUPPORTED response code instead of InternalServerError response code.

Ref: https://github.com/distribution/distribution/blob/main/docs/content/spec/api.md#errors-2

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 30, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Dec 30, 2024
@wxiaoguang
Copy link
Contributor

I do not think it is right, it seems to be an abuse of "UNSUPPORTED"

@hiifong
Copy link
Member Author

hiifong commented Dec 30, 2024

I do not think it is right, it seems to be an abuse of "UNSUPPORTED"

Currently I think UNSUPPORTED will have detailed errors, at least better than InternalServerError.

There is no InternalServerError error for docker registry.

log:

ERROR: failed commit on ref "manifest-sha256:33896528d2ebc8dd157f0f4e6e1a8d28e2580e1fde51ab5655a89158d143a73a": unexpected status from PUT request to https://demo.gitea.com/v2/skydust2b/multiarchtest/manifests/latest: 500 Internal Server Error

@wxiaoguang
Copy link
Contributor

I do not think it is right, it seems to be an abuse of "UNSUPPORTED"

Currently I think UNSUPPORTED will have detailed errors, at least better than InternalServerError.

  1. These errors shouldn't happen in daily usage, if it happens, end users still could do nothing.
  2. It depends on what kind of error you'd like to show to, it is intentionally "not show every internal error to end users" because some errors might contain sensitive server side messages (eg: filesystem path, database IP, etc). If you'd like to show errors to end users, it's better to clearly define what end users would see.

@wxiaoguang
Copy link
Contributor

Even you do not care about server-side sensitive information, always returning "unsupported' is definitely an abuse to the docker status code.

  1. you are reporting internal errors, it doesn't mean it is not supported
  2. if you read docker's document, you could see that "unsupported" is not defined for every API response, if you'd like to strictly follow docker's document, many API should not respond "unsupported".

So I'd like to suggest do not spend more time on this, unless you have a clear plan to use it in some real cases.

@hiifong
Copy link
Member Author

hiifong commented Dec 30, 2024

I do not think it is right, it seems to be an abuse of "UNSUPPORTED"

Currently I think UNSUPPORTED will have detailed errors, at least better than InternalServerError.

  1. These errors shouldn't happen in daily usage, if it happens, end users still could do nothing.
  2. It depends on what kind of error you'd like to show to, it is intentionally "not show every internal error to end users" because some errors might contain sensitive server side messages (eg: filesystem path, database IP, etc). If you'd like to show errors to end users, it's better to clearly define what end users would see.

But there was an error message returned before.
image
Maybe we should desensitize misinformation.

@wxiaoguang
Copy link
Contributor

But there was an error message returned before.

No, they don't.

image

Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not return unsupported for internal errors (ex when the database could be unavailable for a moment etc), also returning internal error message to client is no go from security perspective

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 30, 2024
@hiifong hiifong marked this pull request as draft December 30, 2024 15:10
@wxiaoguang
Copy link
Contributor

So I'd like to suggest do not spend more time on this, unless you have a clear plan to use it in some real cases.

Again, do not spend time on it unless you have a clear picture about it.

@hiifong hiifong closed this Dec 30, 2024
@hiifong hiifong deleted the optimization/docker/registry/response branch December 30, 2024 15:32
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 31, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants