From 9b09bab7a4a857b3c656b9bb1d86e906a24bd913 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 28 Mar 2023 16:57:34 +0800 Subject: [PATCH 1/7] fix --- modules/storage/minio.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/modules/storage/minio.go b/modules/storage/minio.go index 5c67dbf26a298..10713abd50c68 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -142,7 +142,14 @@ func (m *MinioStorage) Save(path string, r io.Reader, size int64) (int64, error) m.buildMinioPath(path), r, size, - minio.PutObjectOptions{ContentType: "application/octet-stream"}, + minio.PutObjectOptions{ + ContentType: "application/octet-stream", + // 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, so use legacy MD5 checksum + SendContentMd5: true, + }, ) if err != nil { return 0, convertMinioErr(err) From 0bcac8f37006eb91c52bf6feaf9a214c63d56674 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 28 Mar 2023 18:04:01 +0800 Subject: [PATCH 2/7] Update modules/storage/minio.go Co-authored-by: Yarden Shoham --- modules/storage/minio.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/storage/minio.go b/modules/storage/minio.go index 10713abd50c68..6e5bd2a3a8ff8 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -147,7 +147,7 @@ func (m *MinioStorage) Save(path string, r io.Reader, size int64) (int64, error) // 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, so use legacy MD5 checksum + // do not support "x-amz-checksum-algorithm" header, so use legacy MD5 checksum SendContentMd5: true, }, ) From 846284c34682575385a20ddca0dc0e448893de07 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 28 Mar 2023 18:28:31 +0800 Subject: [PATCH 3/7] introduce MINIO_CHECKSUM_ALGORITHM --- cmd/migrate_storage.go | 27 +++++++++++++------ custom/conf/app.example.ini | 9 ++++--- .../config-cheat-sheet.en-us.md | 1 + modules/setting/storage.go | 1 + modules/storage/minio.go | 14 +++++++--- 5 files changed, 38 insertions(+), 14 deletions(-) diff --git a/cmd/migrate_storage.go b/cmd/migrate_storage.go index dfa7212e5b0ca..9798913705f2d 100644 --- a/cmd/migrate_storage.go +++ b/cmd/migrate_storage.go @@ -72,12 +72,21 @@ var CmdMigrateStorage = cli.Command{ cli.StringFlag{ Name: "minio-base-path", Value: "", - Usage: "Minio storage basepath on the bucket", + Usage: "Minio storage base path on the bucket", }, cli.BoolFlag{ Name: "minio-use-ssl", Usage: "Enable SSL for minio", }, + cli.BoolFlag{ + Name: "minio-insecure-skip-verify", + Usage: "Skip SSL verification", + }, + cli.StringFlag{ + Name: "minio-checksum-algorithm", + Value: "", + Usage: "Minio checksum algorithm (default/md5)", + }, }, } @@ -168,13 +177,15 @@ func runMigrateStorage(ctx *cli.Context) error { dstStorage, err = storage.NewMinioStorage( stdCtx, storage.MinioStorageConfig{ - Endpoint: ctx.String("minio-endpoint"), - AccessKeyID: ctx.String("minio-access-key-id"), - SecretAccessKey: ctx.String("minio-secret-access-key"), - Bucket: ctx.String("minio-bucket"), - Location: ctx.String("minio-location"), - BasePath: ctx.String("minio-base-path"), - UseSSL: ctx.Bool("minio-use-ssl"), + Endpoint: ctx.String("minio-endpoint"), + AccessKeyID: ctx.String("minio-access-key-id"), + SecretAccessKey: ctx.String("minio-secret-access-key"), + Bucket: ctx.String("minio-bucket"), + Location: ctx.String("minio-location"), + BasePath: ctx.String("minio-base-path"), + UseSSL: ctx.Bool("minio-use-ssl"), + InsecureSkipVerify: ctx.Bool("minio-insecure-skip-verify"), + ChecksumAlgorithm: ctx.String("minio-checksum-algorithm"), }) default: return fmt.Errorf("unsupported storage type: %s", ctx.String("storage")) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index d1cfcd70e5850..a299255d17813 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -583,15 +583,15 @@ ROUTER = console ;; * In request Header: X-Request-ID: test-id-123 ;; * Configuration in app.ini: REQUEST_ID_HEADERS = X-Request-ID ;; * Print in log: 127.0.0.1:58384 - - [14/Feb/2023:16:33:51 +0800] "test-id-123" -;; -;; If you configure more than one in the .ini file, it will match in the order of configuration, +;; +;; If you configure more than one in the .ini file, it will match in the order of configuration, ;; and the first match will be finally printed in the log. ;; * E.g: ;; * In reuqest Header: X-Trace-ID: trace-id-1q2w3e4r ;; * Configuration in app.ini: REQUEST_ID_HEADERS = X-Request-ID, X-Trace-ID, X-Req-ID ;; * Print in log: 127.0.0.1:58384 - - [14/Feb/2023:16:33:51 +0800] "trace-id-1q2w3e4r" ;; -;; REQUEST_ID_HEADERS = +;; REQUEST_ID_HEADERS = ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; @@ -1886,6 +1886,9 @@ ROUTER = console ;; ;; Minio skip SSL verification available when STORAGE_TYPE is `minio` ;MINIO_INSECURE_SKIP_VERIFY = false +;; +;; Minio checksum algorithm: default (for MinIO or AWS S3) or md5 (for Cloudflare or Backblaze) +;MINIO_CHECKSUM_ALGORITHM = default ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/doc/administration/config-cheat-sheet.en-us.md b/docs/content/doc/administration/config-cheat-sheet.en-us.md index 7f31a27f15066..e12d96147fb78 100644 --- a/docs/content/doc/administration/config-cheat-sheet.en-us.md +++ b/docs/content/doc/administration/config-cheat-sheet.en-us.md @@ -855,6 +855,7 @@ Default templates for project boards: - `MINIO_BASE_PATH`: **attachments/**: Minio base path on the bucket only available when STORAGE_TYPE is `minio` - `MINIO_USE_SSL`: **false**: Minio enabled ssl only available when STORAGE_TYPE is `minio` - `MINIO_INSECURE_SKIP_VERIFY`: **false**: Minio skip SSL verification available when STORAGE_TYPE is `minio` +- `MINIO_CHECKSUM_ALGORITHM`: **default**: Minio checksum algorithm: `default` (for MinIO or AWS S3) or `md5` (for Cloudflare or Backblaze) ## Log (`log`) diff --git a/modules/setting/storage.go b/modules/setting/storage.go index 4d401614e4929..50d4c8439e2b1 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -42,6 +42,7 @@ func getStorage(rootCfg ConfigProvider, name, typ string, targetSec *ini.Section sec.Key("MINIO_LOCATION").MustString("us-east-1") sec.Key("MINIO_USE_SSL").MustBool(false) sec.Key("MINIO_INSECURE_SKIP_VERIFY").MustBool(false) + sec.Key("MINIO_CHECKSUM_ALGORITHM").MustString("default") if targetSec == nil { targetSec, _ = rootCfg.NewSection(name) diff --git a/modules/storage/minio.go b/modules/storage/minio.go index 6e5bd2a3a8ff8..250f17827ff5b 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -6,6 +6,7 @@ package storage import ( "context" "crypto/tls" + "fmt" "io" "net/http" "net/url" @@ -53,10 +54,12 @@ type MinioStorageConfig struct { BasePath string `ini:"MINIO_BASE_PATH"` UseSSL bool `ini:"MINIO_USE_SSL"` InsecureSkipVerify bool `ini:"MINIO_INSECURE_SKIP_VERIFY"` + ChecksumAlgorithm string `ini:"MINIO_CHECKSUM_ALGORITHM"` } // MinioStorage returns a minio bucket storage type MinioStorage struct { + cfg *MinioStorageConfig ctx context.Context client *minio.Client bucket string @@ -91,6 +94,10 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error } config := configInterface.(MinioStorageConfig) + if config.ChecksumAlgorithm != "" && config.ChecksumAlgorithm != "default" && config.ChecksumAlgorithm != "md5" { + return nil, fmt.Errorf("invalid minio checksum algorithm: %s", config.ChecksumAlgorithm) + } + log.Info("Creating Minio storage at %s:%s with base path %s", config.Endpoint, config.Bucket, config.BasePath) minioClient, err := minio.New(config.Endpoint, &minio.Options{ @@ -113,6 +120,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error } return &MinioStorage{ + cfg: &config, ctx: ctx, client: minioClient, bucket: config.Bucket, @@ -124,7 +132,7 @@ func (m *MinioStorage) buildMinioPath(p string) string { return util.PathJoinRelX(m.basePath, p) } -// Open open a file +// Open opens a file func (m *MinioStorage) Open(path string) (Object, error) { opts := minio.GetObjectOptions{} object, err := m.client.GetObject(m.ctx, m.bucket, m.buildMinioPath(path), opts) @@ -134,7 +142,7 @@ func (m *MinioStorage) Open(path string) (Object, error) { return &minioObject{object}, nil } -// Save save a file to minio +// Save saves a file to minio func (m *MinioStorage) Save(path string, r io.Reader, size int64) (int64, error) { uploadInfo, err := m.client.PutObject( m.ctx, @@ -148,7 +156,7 @@ func (m *MinioStorage) Save(path string, r io.Reader, size int64) (int64, error) // * https://developers.cloudflare.com/r2/api/s3/api/ // * https://www.backblaze.com/b2/docs/s3_compatible_api.html // do not support "x-amz-checksum-algorithm" header, so use legacy MD5 checksum - SendContentMd5: true, + SendContentMd5: m.cfg.ChecksumAlgorithm == "md5", }, ) if err != nil { From b6dd56779723dc334051931fe041c5c8dce55f39 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 28 Mar 2023 19:01:14 +0800 Subject: [PATCH 4/7] test ci --- tests/pgsql.ini.tmpl | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/pgsql.ini.tmpl b/tests/pgsql.ini.tmpl index fbfbae7c68130..15349aa4c217c 100644 --- a/tests/pgsql.ini.tmpl +++ b/tests/pgsql.ini.tmpl @@ -125,6 +125,7 @@ MINIO_SECRET_ACCESS_KEY = 12345678 MINIO_BUCKET = gitea MINIO_LOCATION = us-east-1 MINIO_USE_SSL = false +MINIO_CHECKSUM_ALGORITHM = md5 [packages] ENABLED = true From dc7fc8b1933817d118a868d237938f05b005b6ca Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 28 Mar 2023 20:19:01 +0800 Subject: [PATCH 5/7] fix Save bug caused by MinIO --- modules/lfs/content_store.go | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/modules/lfs/content_store.go b/modules/lfs/content_store.go index a4ae21bfd65c6..fbcbc71da3c14 100644 --- a/modules/lfs/content_store.go +++ b/modules/lfs/content_store.go @@ -60,6 +60,12 @@ func (s *ContentStore) Put(pointer Pointer, r io.Reader) error { return err } + // check again whether there is any error during the Save operation + // because some errors might be ignored by the Reader's caller + if wrappedRd.lastError != nil { + return wrappedRd.lastError + } + // This shouldn't happen but it is sensible to test if written != pointer.Size { if err := s.Delete(p); err != nil { @@ -109,6 +115,17 @@ type hashingReader struct { expectedSize int64 hash hash.Hash expectedHash string + lastError error +} + +// recordError records the last error during the Save operation +// Some callers of the Reader doesn't respect the returned "err" +// For example, MinIO's Put will ignore errors if the written size could equal to expected size +// So we must remember the error by ourselves, +// and later check again whether ErrSizeMismatch or ErrHashMismatch occurs during the Save operation +func (r *hashingReader) recordError(n int, err error) (int, error) { + r.lastError = err + return n, err } func (r *hashingReader) Read(b []byte) (int, error) { @@ -118,22 +135,22 @@ func (r *hashingReader) Read(b []byte) (int, error) { r.currentSize += int64(n) wn, werr := r.hash.Write(b[:n]) if wn != n || werr != nil { - return n, werr + return r.recordError(n, werr) } } - if err != nil && err == io.EOF { + if errors.Is(err, io.EOF) || r.currentSize >= r.expectedSize { if r.currentSize != r.expectedSize { - return n, ErrSizeMismatch + return r.recordError(n, ErrSizeMismatch) } shaStr := hex.EncodeToString(r.hash.Sum(nil)) if shaStr != r.expectedHash { - return n, ErrHashMismatch + return r.recordError(n, ErrHashMismatch) } } - return n, err + return r.recordError(n, err) } func newHashingReader(expectedSize int64, expectedHash string, reader io.Reader) *hashingReader { From 0ff0e7bed2d5133e3d3bd9415ca8ad67f16baa80 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 28 Mar 2023 20:53:41 +0800 Subject: [PATCH 6/7] fix error check --- modules/lfs/content_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lfs/content_store.go b/modules/lfs/content_store.go index fbcbc71da3c14..5bd9cdf32aa11 100644 --- a/modules/lfs/content_store.go +++ b/modules/lfs/content_store.go @@ -62,7 +62,7 @@ func (s *ContentStore) Put(pointer Pointer, r io.Reader) error { // check again whether there is any error during the Save operation // because some errors might be ignored by the Reader's caller - if wrappedRd.lastError != nil { + if wrappedRd.lastError != nil && !errors.Is(wrappedRd.lastError, io.EOF) { return wrappedRd.lastError } From 13bdd8ad3ec1a54d59885b7511774cb2f98bb7b9 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 28 Mar 2023 21:26:34 +0800 Subject: [PATCH 7/7] refactor recordError, delete failed files in Put --- modules/lfs/content_store.go | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/modules/lfs/content_store.go b/modules/lfs/content_store.go index 5bd9cdf32aa11..53fac4ab858bb 100644 --- a/modules/lfs/content_store.go +++ b/modules/lfs/content_store.go @@ -63,18 +63,19 @@ func (s *ContentStore) Put(pointer Pointer, r io.Reader) error { // check again whether there is any error during the Save operation // because some errors might be ignored by the Reader's caller if wrappedRd.lastError != nil && !errors.Is(wrappedRd.lastError, io.EOF) { - return wrappedRd.lastError + err = wrappedRd.lastError + } else if written != pointer.Size { + err = ErrSizeMismatch } - // This shouldn't happen but it is sensible to test - if written != pointer.Size { - if err := s.Delete(p); err != nil { - log.Error("Cleaning the LFS OID[%s] failed: %v", pointer.Oid, err) + // if the upload failed, try to delete the file + if err != nil { + if errDel := s.Delete(p); errDel != nil { + log.Error("Cleaning the LFS OID[%s] failed: %v", pointer.Oid, errDel) } - return ErrSizeMismatch } - return nil + return err } // Exists returns true if the object exists in the content store. @@ -123,9 +124,9 @@ type hashingReader struct { // For example, MinIO's Put will ignore errors if the written size could equal to expected size // So we must remember the error by ourselves, // and later check again whether ErrSizeMismatch or ErrHashMismatch occurs during the Save operation -func (r *hashingReader) recordError(n int, err error) (int, error) { +func (r *hashingReader) recordError(err error) error { r.lastError = err - return n, err + return err } func (r *hashingReader) Read(b []byte) (int, error) { @@ -135,22 +136,22 @@ func (r *hashingReader) Read(b []byte) (int, error) { r.currentSize += int64(n) wn, werr := r.hash.Write(b[:n]) if wn != n || werr != nil { - return r.recordError(n, werr) + return n, r.recordError(werr) } } if errors.Is(err, io.EOF) || r.currentSize >= r.expectedSize { if r.currentSize != r.expectedSize { - return r.recordError(n, ErrSizeMismatch) + return n, r.recordError(ErrSizeMismatch) } shaStr := hex.EncodeToString(r.hash.Sum(nil)) if shaStr != r.expectedHash { - return r.recordError(n, ErrHashMismatch) + return n, r.recordError(ErrHashMismatch) } } - return r.recordError(n, err) + return n, r.recordError(err) } func newHashingReader(expectedSize int64, expectedHash string, reader io.Reader) *hashingReader {