S3: log human readable error on connection failure

modules/storage/minio.go

@@ -71,6 +71,11 @@ func convertMinioErr(err error) error {
 	return 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
@@ -90,6 +95,23 @@ func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage,
 		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
+		}
+	}
+
 	// Check to see if we already own this bucket
 	exists, err := minioClient.BucketExists(ctx, config.Bucket)
 	if err != nil {

modules/storage/minio_test.go

@@ -4,10 +4,15 @@
 package storage
 
 import (
+	"context"
+	"net/http"
 	"os"
 	"testing"
 
 	"code.gitea.io/gitea/modules/setting"
+
+	"github.com/minio/minio-go/v7"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestMinioStorageIterator(t *testing.T) {
@@ -25,3 +30,31 @@ func TestMinioStorageIterator(t *testing.T) {
 		},
 	})
 }
+
+func TestS3StorageBadRequest(t *testing.T) {
+	if os.Getenv("CI") == "" {
+		t.Skip("S3Storage not present outside of CI")
+		return
+	}
+	cfg := &setting.Storage{
+		MinioConfig: setting.MinioStorageConfig{
+			Endpoint:        "minio:9000",
+			AccessKeyID:     "123456",
+			SecretAccessKey: "12345678",
+			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)
+}