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.

Sign up for GitHub

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

Jump to bottom

storage/minio: Add options to disable signature and multipart for Minio Client #21780

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

storage/minio: Add options to disable signature and multipart for Minio Client #21780

wants to merge 12 commits into from

Conversation

stevefan1999-personal
Copy link

@stevefan1999-personal stevefan1999-personal commented Nov 11, 2022 •
edited
Loading

Fixes #21778, hopefully

This may also help rclone/rclone#6461 in the future, I don't think gofakes3 would have those two features implemented well either.

The Docker package is available at https://github.com/users/stevefan1999-personal/packages/container/package/gitea, feel free to checkout and test.

@KN4CK3R
Copy link
Member

KN4CK3R commented Nov 11, 2022 •
edited
Loading

Are you able to test it? From looking at the minio code it may be enough to set DisableMultipart. But then all storage.Save calls need to provide the size of the content. Otherwise the upload just fails:
https://github.com/minio/minio-go/blob/7aa4b0e0d1a9fdb4a99f50df715c21ec21d91753/api-put-object.go#L236
Currently we do not provide a correct size everywhere:
https://github.com/go-gitea/gitea/pull/15264/files

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 11, 2022
@stevefan1999-personal
Copy link
Author

stevefan1999-personal commented Nov 11, 2022 •
edited
Loading

@KN4CK3R that's exactly the reason I put this on draft

Indeed, I tracked some storage.Save call, it seems like some of them passes -1 which indicates it is a stream or some sort. This would be quite tricky

@stevefan1999-personal
Copy link
Author

stevefan1999-personal commented Nov 11, 2022 •
edited
Loading

I can confirm that by disabling signature in Minio client, Docker registry works (I have both pushed an image, pulled it back, and ran the image). I'm not sure about others though.

@codecov-commenter

This comment was marked as duplicate.

Sign in to view
@lunny lunny added the type/enhancement An improvement of existing functionality label Nov 12, 2022
@lunny lunny added this to the 1.19.0 milestone Nov 12, 2022
@wizpresso-steve-cy-fan wizpresso-steve-cy-fan mentioned this pull request Nov 23, 2022
Closed
@lunny lunny modified the milestones: 1.19.0, 1.20.0 Jan 31, 2023
@stevefan1999-personal stevefan1999-personal marked this pull request as ready for review March 22, 2023 04:47
@stevefan1999-personal
Copy link
Author

I have ran this internally in my company for the last few month and it pretty much works. I'm looking to get it merged ASAP

yardenshoham
yardenshoham suggested changes Mar 22, 2023
View reviewed changes
Copy link
Member

@yardenshoham yardenshoham left a comment

Choose a reason for hiding this comment

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

Missing documentation changes, see #23166 for example

techknowlogick
techknowlogick approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! Could you resolve conflicts?

@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 May 23, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 23, 2023
@yardenshoham yardenshoham self-requested a review May 23, 2023 20:35
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels May 23, 2023
@yardenshoham
Copy link
Member

Still missing documentation

@wxiaoguang
Copy link
Contributor

I agree with KN4CK3R here. After reading the code again, I think the callers to Save should provide the size/length.

Especially the doArchive in archiver.go, if the Save reads all the stream content to memory, it would take GB memory?

@lunny
Copy link
Member

lunny commented May 25, 2023

I agree with KN4CK3R here. After reading the code again, I think the callers to Save should provide the size/length.

Especially the doArchive in archiver.go, if the Save reads all the stream content to memory, it would take GB memory?

I can confirm the size parameter was added to fix the memory problem.

@stevefan1999-personal
Copy link
Author

Gimme a moment, I was on a trip and didn't catch up so soon :)

@stevefan1999-personal
Copy link
Author

stevefan1999-personal commented May 27, 2023 •
edited
Loading

I have another idea about multi-part upload, and that is to use mmap. If we can lock the file beforehand, using a memory map will not only solve the potential DoS problem, but that it will also drastically speed it up. I will see if I can implement it soon

@delvh delvh removed this from the 1.20.0 milestone Jun 7, 2023
@lunny
Copy link
Member

lunny commented Aug 22, 2023

Please resolve the conflicts

@yardenshoham
Copy link
Member

Still missing documentation

@lunny lunny added this to the 1.21.0 milestone Aug 22, 2023
lunny
lunny reviewed Aug 22, 2023
View reviewed changes
modules/storage/minio.go
disableSignature, disableMultipart = m.cfg.DisableSignature, m.cfg.DisableMultipart
}

if disableMultipart && size < 0 {
Copy link
Member

Choose a reason for hiding this comment

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

So how to handle when size == 0?

@lunny lunny modified the milestones: 1.21.0, 1.22.0 Sep 21, 2023
@StarAurryon
Copy link

Hi there, do you have any news.

I am having the same issue. What were the remaining points to get this merged ?

I can do another PR if needed.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 7, 2024
@lunny lunny modified the milestones: 1.22.0, 1.23.0 Mar 29, 2024
@lunny lunny modified the milestones: 1.23.0, 1.24.0 Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@techknowlogick techknowlogick techknowlogick approved these changes

@lunny lunny lunny left review comments

@yardenshoham yardenshoham Awaiting requested review from yardenshoham

Assignees
No one assigned
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.

Some S3 compatible services does not have chunked upload properly implemented
10 participants
@stevefan1999-personal @KN4CK3R @codecov-commenter @yardenshoham @wxiaoguang @lunny @StarAurryon @techknowlogick @GiteaBot @delvh