Make minio package support legacy MD5 checksum #23768

wxiaoguang · 2023-03-28T08:59:18Z

A feedback from discord: https://discord.com/channels/322538954119184384/561007778139734027/1090185427115319386

Some storages like:

They do not support "x-amz-checksum-algorithm" header

But minio recently uses that header with CRC32C by default. So we have to tell minio to use legacy MD5 checksum.

I guess this needs to be backported because IIRC we 1.19 and 1.20 are using similar minio package.

The minio package code for SendContentMD5 looks like this:

modules/storage/minio.go

KN4CK3R · 2023-03-28T09:39:47Z

Should we make that configurable? That setting feels like "legacy" and may prevent working with newer services.

wxiaoguang · 2023-03-28T10:02:42Z

Should we make that configurable? That setting feels like "legacy" and may prevent working with newer services.

I feel that it won't break newer services because it seems that newer services should have the ability to handle such behavior.

And I haven't got a clear idea about how to configure the minio behavior clearly, for example, there are a lot of options.

If you really prefer to introduce a new option, what's the possible/suitable name for it in your mind?

Co-authored-by: Yarden Shoham <[email protected]>

wxiaoguang · 2023-03-28T12:21:06Z

@KN4CK3R I found a low-level bug in MinIO package, it ignores some errors if the written side could match the expected size. The bug only happens when SendContentMD5=true

So I have to fix the bug on our side , see dc7fc8b , recordError

lunny · 2023-03-28T12:36:17Z

@KN4CK3R I found a low-level bug in MinIO package, it ignores some errors if the written side could match the expected size. The bug only happens when SendContentMD5=true

So I have to fix the bug on our side , see dc7fc8b , recordError

Could we upgrade our minio sdk to test if it's fixed?

wxiaoguang · 2023-03-28T12:39:54Z

@KN4CK3R I found a low-level bug in MinIO package, it ignores some errors if the written side could match the expected size. The bug only happens when SendContentMD5=true
So I have to fix the bug on our side , see dc7fc8b , recordError

Could we upgrade our minio sdk to test if it's fixed?

Even if MinIO package gets fixed, it doesn't guarantee that other packages never have similar behaviors.

The root problem is that Gitea's hashingReader tries to hijack the "Read" procedure, but there is no clearly defined behavior for it, caller could do anything they like. So, we should always do the extra check, to make sure it works correctly for all cases.

modules/lfs/content_store.go

KN4CK3R · 2023-03-28T13:20:09Z

Here is the code which drops the error btw: https://github.com/minio/minio-go/blob/f54fa21d32c040f6b33ac949b72c58bcea5d0669/utils.go#L389-L422

codecov-commenter · 2023-03-28T13:20:26Z

Codecov Report

Merging #23768 (13bdd8a) into main (f521e88) will decrease coverage by 0.03%.
The diff coverage is 36.88%.

❗ Current head 13bdd8a differs from pull request most recent head 6b6eac4. Consider uploading reports for the commit 6b6eac4 to get more accurate results

@@            Coverage Diff             @@
##             main   #23768      +/-   ##
==========================================
- Coverage   47.14%   47.11%   -0.03%     
==========================================
  Files        1149     1155       +6     
  Lines      151446   152517    +1071     
==========================================
+ Hits        71397    71863     +466     
- Misses      71611    72178     +567     
- Partials     8438     8476      +38

Impacted Files	Coverage Δ
cmd/dump.go	`0.66% <0.00%> (-0.01%)`	⬇️
cmd/migrate_storage.go	`5.76% <0.00%> (-0.12%)`	⬇️
cmd/web.go	`0.00% <0.00%> (ø)`
models/actions/run.go	`1.64% <0.00%> (-0.08%)`	⬇️
models/actions/runner.go	`1.44% <ø> (ø)`
models/packages/package.go	`45.45% <0.00%> (-1.13%)`	⬇️
models/user/search.go	`77.50% <0.00%> (-6.29%)`	⬇️
modules/actions/workflows.go	`0.00% <0.00%> (ø)`
modules/context/context.go	`64.54% <0.00%> (-3.53%)`	⬇️
modules/doctor/storage.go	`30.65% <0.00%> (-1.29%)`	⬇️
... and 41 more

... and 48 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

wxiaoguang · 2023-03-28T13:28:53Z

@KN4CK3R All done in 13bdd8a

douglasparker · 2023-03-28T14:43:37Z

Thank you so much! This was the missing piece for me to migrate my projects to Gitea! 🥳

A feedback from discord: https://discord.com/channels/322538954119184384/561007778139734027/1090185427115319386 Some storages like: * https://developers.cloudflare.com/r2/api/s3/api/ * https://www.backblaze.com/b2/docs/s3_compatible_api.html They do not support "x-amz-checksum-algorithm" header But minio recently uses that header with CRC32C by default. So we have to tell minio to use legacy MD5 checksum. I guess this needs to be backported because IIRC we 1.19 and 1.20 are using similar minio package. The minio package code for SendContentMD5 looks like this: <details> <img width="755" alt="image" src="https://user-images.githubusercontent.com/2114189/228186768-4f2f6f67-62b9-4aee-9251-5af714ad9674.png"> </details> # Conflicts: # custom/conf/app.example.ini # tests/pgsql.ini.tmpl

Backport #23768 (no source code conflict, only some unrelated docs/test-ini conflicts) Some storages like: * https://developers.cloudflare.com/r2/api/s3/api/ * https://www.backblaze.com/b2/docs/s3_compatible_api.html They do not support "x-amz-checksum-algorithm" header But minio recently uses that header with CRC32C by default. So we have to tell minio to use legacy MD5 checksum.

* upstream/main: Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" (go-gitea#23687) Add CSS rules for basic colored labels (go-gitea#23774) Add meilisearch support (go-gitea#23136) Add missing translation for `actions.runners.reset_registration_token_success` (go-gitea#23732) [skip ci] Updated translations via Crowdin Implement Issue Config (go-gitea#20956) Set repository link based on the url in package.json for npm packages (go-gitea#20379) Add API to manage issue dependencies (go-gitea#17935) Add creation time in tag list page (go-gitea#23693) Make minio package support legacy MD5 checksum (go-gitea#23768) Yarden Shoham has a new email address (go-gitea#23767) fix br display for packages curls (go-gitea#23737)

fix

9b09bab

yardenshoham reviewed Mar 28, 2023

View reviewed changes

modules/storage/minio.go Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 28, 2023

wxiaoguang and others added 2 commits March 28, 2023 18:04

Update modules/storage/minio.go

0bcac8f

Co-authored-by: Yarden Shoham <[email protected]>

introduce MINIO_CHECKSUM_ALGORITHM

846284c

wxiaoguang force-pushed the fix-minio-checksum branch from eac88bc to 846284c Compare March 28, 2023 10:31

wxiaoguang added 2 commits March 28, 2023 19:01

test ci

b6dd567

fix Save bug caused by MinIO

dc7fc8b

lunny approved these changes Mar 28, 2023

View reviewed changes

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 Mar 28, 2023

lunny added the type/enhancement An improvement of existing functionality label Mar 28, 2023

lunny added this to the 1.20.0 milestone Mar 28, 2023

fix error check

0ff0e7b

wolfogre approved these changes Mar 28, 2023

View reviewed changes

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 Mar 28, 2023

KN4CK3R requested changes Mar 28, 2023

View reviewed changes

KN4CK3R reviewed Mar 28, 2023

View reviewed changes

modules/lfs/content_store.go Outdated Show resolved Hide resolved

refactor recordError, delete failed files in Put

13bdd8a

KN4CK3R approved these changes Mar 28, 2023

View reviewed changes

wxiaoguang changed the title ~~Make minio package use legacy MD5 checksum~~ Make minio package support legacy MD5 checksum Mar 28, 2023

Merge branch 'main' into fix-minio-checksum

6b6eac4

jolheiser enabled auto-merge (squash) March 28, 2023 14:45

jolheiser merged commit 5727056 into go-gitea:main Mar 28, 2023

wxiaoguang deleted the fix-minio-checksum branch March 28, 2023 15:14

wxiaoguang mentioned this pull request Mar 28, 2023

Make minio package support legacy MD5 checksum (#23768) #23770

Merged

wxiaoguang mentioned this pull request Mar 31, 2023

Support MINIO_CHECKSUM_ALGORITHM on lfs #23837

Closed

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make minio package support legacy MD5 checksum #23768

Make minio package support legacy MD5 checksum #23768

wxiaoguang commented Mar 28, 2023 •

edited

Loading

KN4CK3R commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023 •

edited

Loading

wxiaoguang commented Mar 28, 2023 •

edited

Loading

lunny commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023

KN4CK3R commented Mar 28, 2023

codecov-commenter commented Mar 28, 2023 •

edited

Loading

wxiaoguang commented Mar 28, 2023

douglasparker commented Mar 28, 2023

Make minio package support legacy MD5 checksum #23768

Make minio package support legacy MD5 checksum #23768

Conversation

wxiaoguang commented Mar 28, 2023 • edited Loading

KN4CK3R commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023 • edited Loading

wxiaoguang commented Mar 28, 2023 • edited Loading

lunny commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023

KN4CK3R commented Mar 28, 2023

codecov-commenter commented Mar 28, 2023 • edited Loading

Codecov Report

wxiaoguang commented Mar 28, 2023

douglasparker commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023 •

edited

Loading

wxiaoguang commented Mar 28, 2023 •

edited

Loading

wxiaoguang commented Mar 28, 2023 •

edited

Loading

codecov-commenter commented Mar 28, 2023 •

edited

Loading