diff --git a/modules/storage/minio.go b/modules/storage/minio.go index 565d298616..310dd6da5e 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -6,6 +6,7 @@ package storage import ( "context" "crypto/tls" + "errors" "fmt" "io" "net/http" @@ -47,40 +48,42 @@ type MinioStorage struct { basePath string } -func convertMinioErr(err error) error { +func convertMinioErr(err error, optMsg ...string) error { if err == nil { return nil } - errResp, ok := err.(minio.ErrorResponse) + + wrapErr := func(err error) error { + if len(optMsg) == 0 { + return err + } + return fmt.Errorf("%s: %w", optMsg[0], err) + } + + errResp, ok := errors.AsType[minio.ErrorResponse](err) if !ok { - return err + return wrapErr(err) } // Convert two responses to standard analogues switch errResp.Code { case "NoSuchKey": - return os.ErrNotExist + return wrapErr(os.ErrNotExist) case "AccessDenied": - return os.ErrPermission + return wrapErr(os.ErrPermission) } - return err -} - -var getBucketVersioning = func(ctx context.Context, minioClient *minio.Client, bucket string) error { - _, err := minioClient.GetBucketVersioning(ctx, bucket) - return err + return wrapErr(err) } // NewMinioStorage returns a minio storage func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage, error) { config := cfg.MinioConfig + log.Info("Creating minio storage at %s:%s with base path %s", config.Endpoint, config.Bucket, config.BasePath) 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) - var lookup minio.BucketLookupType switch config.BucketLookUpType { case "auto", "": @@ -93,6 +96,13 @@ func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage, return nil, fmt.Errorf("invalid minio bucket lookup type: %s", config.BucketLookUpType) } + // The request error message is something like: + // * "The request signature we calculated does not match the signature you provided. Check your key and signing method." + // It doesn't contain useful information to site admin, so here we wrap the error with our error message + // to tell the site admin what is the problem. + makeErrMsg := func(hint string) string { + return fmt.Sprintf("ObjectStorage.%s: endpoint=%s, location=%s, bucket=%s", hint, config.Endpoint, config.Location, config.Bucket) + } minioClient, err := minio.New(config.Endpoint, &minio.Options{ Creds: buildMinioCredentials(config), Secure: config.UseSSL, @@ -101,37 +111,21 @@ func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage, BucketLookup: lookup, }) if err != nil { - return nil, convertMinioErr(err) - } - - // 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. - err = getBucketVersioning(ctx, minioClient, config.Bucket) - if err != nil { - errResp, ok := err.(minio.ErrorResponse) - if !ok { - return nil, err - } - if errResp.StatusCode == http.StatusBadRequest { - log.Error("S3 storage connection failure at %s:%s with base path %s and region: %s", config.Endpoint, config.Bucket, config.Location, errResp.Message) - return nil, err - } + return nil, convertMinioErr(err, makeErrMsg("NewClient")) } // Check to see if we already own this bucket exists, err := minioClient.BucketExists(ctx, config.Bucket) if err != nil { - return nil, convertMinioErr(err) + return nil, convertMinioErr(err, makeErrMsg("BucketExists")) } + // If the bucket doesn't exist, try to create one if !exists { if err := minioClient.MakeBucket(ctx, config.Bucket, minio.MakeBucketOptions{ Region: config.Location, }); err != nil { - return nil, convertMinioErr(err) + return nil, convertMinioErr(err, makeErrMsg("MakeBucket")) } } diff --git a/modules/storage/minio_test.go b/modules/storage/minio_test.go index 1bb772c8d3..67c4d18933 100644 --- a/modules/storage/minio_test.go +++ b/modules/storage/minio_test.go @@ -4,16 +4,13 @@ package storage import ( - "context" "net/http" "net/http/httptest" - "os" "testing" "gitea.dev/modules/setting" "gitea.dev/modules/test" - "github.com/minio/minio-go/v7" "github.com/stretchr/testify/assert" ) @@ -84,31 +81,18 @@ func TestMinioStoragePath(t *testing.T) { } func TestS3StorageBadRequest(t *testing.T) { - if os.Getenv("CI") == "" { - t.Skip("S3Storage not present outside of CI") - return - } + endpoint := test.ExternalServiceHTTP(t, "TEST_MINIO_ENDPOINT", "minio:9000") cfg := &setting.Storage{ MinioConfig: setting.MinioStorageConfig{ - Endpoint: "minio:9000", + Endpoint: endpoint, AccessKeyID: "123456", - SecretAccessKey: "12345678", + SecretAccessKey: "invalid-secret", Bucket: "bucket", Location: "us-east-1", }, } - message := "ERROR" - old := getBucketVersioning - defer func() { getBucketVersioning = old }() - getBucketVersioning = func(ctx context.Context, minioClient *minio.Client, bucket string) error { - return minio.ErrorResponse{ - StatusCode: http.StatusBadRequest, - Code: "FixtureError", - Message: message, - } - } _, err := NewStorage(setting.MinioStorageType, cfg) - assert.ErrorContains(t, err, message) + assert.ErrorContains(t, err, "ObjectStorage.BucketExists: endpoint="+endpoint) } func TestMinioCredentials(t *testing.T) {