Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

S3: log human readable error on connection failure #26856

Merged
merged 3 commits into from Sep 12, 2023

Conversation

earl-warren
Copy link
Contributor

Should BucketExists (HeadBucket) fail because of an error related to
the connection rather than the existence of the bucket, no information
is available and the admin is left guessing.

https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html

This action is useful to determine if a bucket exists and you have
permission to access it. The action returns a 200 OK if the bucket
exists and you have permission to access it.

If the bucket does not exist or you do not have permission to access
it, the HEAD request returns a generic 400 Bad Request, 403
Forbidden or 404 Not Found code. A message body is not included, so
you cannot determine the exception beyond these error codes.

GetBucketVersioning is used instead and exclusively dedicated to
asserting if using the connection does not return a BadRequest.
If it does the NewMinioStorage logs an error and returns. Otherwise
it keeps going knowing that BucketExists is not going to fail for
reasons unrelated to the existence of the bucket and the permissions
to access it.

(cherry picked from commit d1df4b3bc62e5e61893a923f1c4b58f084eb03af)

Refs: https://codeberg.org/forgejo/forgejo/issues/1338

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 1, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 1, 2023
@puni9869
Copy link
Member

puni9869 commented Sep 1, 2023

Some linter errors.

@wxiaoguang
Copy link
Contributor

Instead of introducing mockvariable, actually it should do this:

-> Move web/api context related testing function into a separate package #26859

See the TODO comment of the MockContext:

// TODO: move this function to other packages, because it depends on "models" package

@earl-warren
Copy link
Contributor Author

The commit related to MockVariableValue was removed, it does not belong here.

@wxiaoguang
Copy link
Contributor

I am not sure whether it is right. According to https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketVersioning.html

Returns the versioning state of a bucket. To retrieve the versioning state of a bucket, you must be the bucket owner.

Then, are you sure that the non-owner but writer users could still run Gitea correctly?

@earl-warren
Copy link
Contributor Author

GetBucketVersioning is used instead and exclusively dedicated to asserting if using the connection does not return a BadRequest.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 4, 2023

GetBucketVersioning is used instead and exclusively dedicated to asserting if using the connection does not return a BadRequest.

That's quite tricky. Could there be some comments to explain the design and behavior? eg:

// The GetBucketVersioning is only used for checking whether the Object Storage parameters are generally good. It doesn't need to succeed.
// The assumption is that if the API returns the HTTP code 400, then the parameters could be incorrect.
// Otherwise even if the request itself fails (403, 404, etc), the code should still continue because the parameters seem "good" enough.
// Keep in mind that GetBucketVersioning requires "owner" to really succeed, so it can't be used to check the existence.
// Not using "BucketExists (HeadBucket)" because it doesn't include detailed failure reasons.

@wxiaoguang wxiaoguang added this to the 1.21.0 milestone Sep 4, 2023
@wxiaoguang wxiaoguang added the type/enhancement An improvement of existing functionality label Sep 4, 2023
Should BucketExists (HeadBucket) fail because of an error related to
the connection rather than the existence of the bucket, no information
is available and the admin is left guessing.

https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html

> This action is useful to determine if a bucket exists and you have
> permission to access it. The action returns a 200 OK if the bucket
> exists and you have permission to access it.
>
> If the bucket does not exist or you do not have permission to access
> it, the HEAD request returns a generic 400 Bad Request, 403
> Forbidden or 404 Not Found code. A message body is not included, so
> you cannot determine the exception beyond these error codes.

GetBucketVersioning is used instead and exclusively dedicated to
asserting if using the connection does not return a BadRequest.
If it does the NewMinioStorage logs an error and returns. Otherwise
it keeps going knowing that BucketExists is not going to fail for
reasons unrelated to the existence of the bucket and the permissions
to access it.

(cherry picked from commit d1df4b3bc62e5e61893a923f1c4b58f084eb03af)

Refs: https://codeberg.org/forgejo/forgejo/issues/1338
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 11, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 11, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 11, 2023
@lunny lunny merged commit 7818121 into go-gitea:main Sep 12, 2023
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 12, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 12, 2023
* upstream/main:
  Add more package registry paths to the labeler (go-gitea#27032)
  Extract auth middleware from service (go-gitea#27028)
  S3: log human readable error on connection failure (go-gitea#26856)
  [skip ci] Updated translations via Crowdin
  Fix "delete" modal dialog for issue/PR (go-gitea#27015)
  Fix context cache bug & enable context cache for dashabord commits' authors (go-gitea#26991)
  fix: typo (go-gitea#27009)
  Use secure cookie for HTTPS sites (go-gitea#26999)
  Add fetch wrappers, ignore network errors in actions view (go-gitea#26985)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants