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

Inconsistent gitea behaviors and definition about config SERVE_DIRECT #16711

Closed
2 of 6 tasks
ABNER-1 opened this issue Aug 17, 2021 · 15 comments
Closed
2 of 6 tasks

Inconsistent gitea behaviors and definition about config SERVE_DIRECT #16711

ABNER-1 opened this issue Aug 17, 2021 · 15 comments
Labels
type/enhancement An improvement of existing functionality

Comments

@ABNER-1
Copy link
Contributor

ABNER-1 commented Aug 17, 2021

  • Gitea version (or commit ref): gitea docker latest

  • Git version: git version 2.30.1

  • Operating system: ubuntu18.04

  • Database (use [x]):

    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:

    • Yes (provide example URL)
    • No
  • Log gist:

Description

I try the config field 'SERVE_DIRECT' which mentioned in gitea doc as Allows the storage driver to redirect to authenticated URLs to serve files directly. Currently, only Minio/S3 is supported via signed URLs, local does nothing.
My config related is below:

[lfs]
STORAGE_TYPE    = my_minio

[storage.my_minio]
STORAGE_TYPE            = minio
SERVE_DIRECT            = true
MINIO_ENDPOINT          = ***:9000
MINIO_ACCESS_KEY_ID     = ***
MINIO_SECRET_ACCESS_KEY = ***
MINIO_BUCKET            = test
MINIO_LOCATION          = us-east-1
MINIO_USE_SSL           = false

However, I found nothing changes when I open this config. All lfs actions including add/download/clone are also add the I/O NET of gitea container.
My docker stats i/o summary is blow:

CONTAINER ID        NAME                CPU %               MEM USAGE / LIMIT     MEM %               NET I/O             BLOCK I/O           PIDS
08f9524c06a9        test_gitea_1        0.01%               109.7MiB / 3.842GiB   2.79%               4.37GB / 4.36GB     91.2MB / 85.1MB     16
ab7d361bc42c        test_giteadb_1      0.03%               74.61MiB / 3.842GiB   1.90%               822kB / 2.25MB      24.7MB / 246kB      8
8b55a751fc54        blissful_taussig    0.01%               70.05MiB / 3.842GiB   1.78%               3.23GB / 4.37GB     1.9GB / 2.55GB      11

Is my understanding of 'SERVE_DIRECT' config wrong? Or it is impossibe that my lfs file upload/download request can be served by minio directly with currently version gitea.

Thanks for any related answers or suggestions.

Screenshots

@ABNER-1 ABNER-1 changed the title SERVE_DIRECT mismatch gitea actions about config SERVE_DIRECT Aug 17, 2021
@ABNER-1 ABNER-1 changed the title mismatch gitea actions about config SERVE_DIRECT Inconsistent gitea behaviors and definition about config SERVE_DIRECT Aug 17, 2021
@lunny lunny added the issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change label Aug 17, 2021
@lunny
Copy link
Member

lunny commented Aug 17, 2021

I have checked that avatars/attahcments/archives have support that, but we may never support it for LFS.

@ABNER-1
Copy link
Contributor Author

ABNER-1 commented Aug 17, 2021

Thanks for your reply.
It's consistent with my experiment result.

By the way, why didn't gitea implement with it? Is there some security tricks in it?
I found github use this option so that uploading a lfs object will be redirected to a s3 link.

Thanks for your reply again.

@lunny
Copy link
Member

lunny commented Aug 17, 2021

The s3 or minio will create a secret link for the download objects and the link will be expired about some days defaultly.

@ABNER-1
Copy link
Contributor Author

ABNER-1 commented Aug 18, 2021

I got it. I want to implement this task and contribute to gitea.
I found the archive doing that by allocating a url by PresignedGetObject.
I try to add codes when lfs info link: info/lfs/objects/{oid} handler DownloadHandler and it work.

I think I need to add some codes about PresignedPutObject url at the upload handler.
Or I need to add some verify logic in BatchHandler like download action which was often verified in UploadHandler.

By the way, should I add this feature in lfs file downloading on the web page?

@lunny
Copy link
Member

lunny commented Aug 18, 2021

Before you do that, do you know if git client or lfs will follow the redirection URL?

@ABNER-1
Copy link
Contributor Author

ABNER-1 commented Aug 19, 2021

I found I have a mistake in the previous response.
It should be "I try to add codes in the BatchHandler and return a response including download action href of s3 object and it work."

@ABNER-1
Copy link
Contributor Author

ABNER-1 commented Aug 19, 2021

When I implemented the redirection logic in the BatchHandler, it download directly from my S3 URL.

@ABNER-1
Copy link
Contributor Author

ABNER-1 commented Aug 19, 2021

And In the upload phase, I prefer to choose to make it following the download phase -- just return the PresignedPutObject url in the BatchHandler. Github does this.

@lunny
Copy link
Member

lunny commented Aug 19, 2021

Upload could be another PR, all of avatars/archives/attahcments need the feature.

@ABNER-1
Copy link
Contributor Author

ABNER-1 commented Aug 19, 2021

OK. I will requet a PR about download.

I have also tried to change the upload phase.
But I failed because lfs meta is nil in the Verify action which was created in the upload handler.
I am confused that if need to add some codes about lfs file upload in the verify handler.
Or are some changes needed in other handler?

@KN4CK3R
Copy link
Member

KN4CK3R commented Aug 19, 2021

The upload may be tricky because currently we verify the size and hash of the uploaded data with the LFS metadata.

@ABNER-1
Copy link
Contributor Author

ABNER-1 commented Aug 20, 2021

I have some questions about currently upload workflow.

  1. Is it necessary to verify the hash of file user upload if user upload directly to s3?
    I read all functions of minio sdk. There is no method getting the sha256 (md5 is possible)of file. So, if we want to verify the sha256 hash, it is neceassary that download file from minio.

  2. If solve the hash verify question, is it possible that create lfs meta object in the verify handler when verify ok with object information getting from minio/s3?

@KN4CK3R
Copy link
Member

KN4CK3R commented Aug 20, 2021

  1. Yes, it is necessary to verify the upload. If you download the file again (when would you do that? you don't know when the upload is finished) you have won nothing.
    Here is why it's possible in Github:
"upload": {
  "href": "https://github-cloud.s3.amazonaws.com/alambic/media/325942445/fb/8f/fb8f7d8435968c4f82a726a92395be4d16f2f63116caf36c8ad35c60831ab042?actor_id=1666336&key_id=0&repo_id=330759100",
  "header": {
    "Authorization": "AWS4-HMAC-SHA256 Credential=xxxxxxxxx/20210820/us-east-1/s3/aws4_request,SignedHeaders=host;x-amz-content-sha256;x-amz-date,Signature=ad139477146c04b7b512ee9118093a63729cd2a6377a010279a8ecb03b55deab",
    "x-amz-content-sha256": "fb8f7d8435968c4f82a726a92395be4d16f2f63116caf36c8ad35c60831ab042",
    "x-amz-date": "20210820T164935Z"
  },
  "expires_at": "2021-08-20T17:04:35Z",
  "expires_in": 900
}

The client must provide the x-amz-content-sha256 header with the hash of the file and S3 checks against this. For min.io there is content-md5 but you don't know the md5 hash of the file.

  1. Verify is optional and may not be called at all.

@ABNER-1
Copy link
Contributor Author

ABNER-1 commented Aug 23, 2021

Thanks for your great answer.

As your answer says, we can't add more necessary actions in verify handler because it may not be called.
On my opinion, is it the only choice that depending on the content header of s3 presigned url to verify the file user upload like github?
And if so, can we add metadata in the batch handler and make other verify work done by s3.

I will read more aws s3 docs for some details about the x-amz-content-sha256.

I can't ensure my thought true, and I am not good at English.
If I am wrong in words and thought, please make me know it. Thanks for your detailed reply again.

@lunny lunny added type/enhancement An improvement of existing functionality and removed issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change labels Aug 23, 2021
@lunny
Copy link
Member

lunny commented Aug 23, 2021

I think this has been resolved via the title. For upload things, we can open another issue.

@lunny lunny closed this as completed Aug 23, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/enhancement An improvement of existing functionality
Projects
None yet
Development

No branches or pull requests

3 participants