fix: minio init check (#38355)

Fix the buggy behavior introduced by "S3: log human readable error on connection failure (#26856)"

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
+	return wrapErr(err)
-}
-
-var getBucketVersioning = func(ctx context.Context, minioClient *minio.Client, bucket string) error {
-	_, err := minioClient.GetBucketVersioning(ctx, bucket)
-	return 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)
+		return nil, convertMinioErr(err, makeErrMsg("NewClient"))
-	}
-
-	// 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
-		}
 	}
 
 	// 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"))
 		}
 	}
 

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") == "" {
+	endpoint := test.ExternalServiceHTTP(t, "TEST_MINIO_ENDPOINT", "minio:9000")
-		t.Skip("S3Storage not present outside of CI")
-		return
-	}
 	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) {