From a0c26f687087e0d146d1298f51dc198d90946393 Mon Sep 17 00:00:00 2001
From: Earl Warren <contact@earl-warren.org>
Date: Thu, 31 Aug 2023 22:59:20 +0200
Subject: [PATCH] [GITEA] S3: log human readable error on connection failure

Should BucketExists (HeadBucket) fail because of an error related to
the connection rather than the existence of the bucket, no information
is available and the admin is left guessing.

https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html

> This action is useful to determine if a bucket exists and you have
> permission to access it. The action returns a 200 OK if the bucket
> exists and you have permission to access it.
>
> If the bucket does not exist or you do not have permission to access
> it, the HEAD request returns a generic 400 Bad Request, 403
> Forbidden or 404 Not Found code. A message body is not included, so
> you cannot determine the exception beyond these error codes.

GetBucketVersioning is used instead and exclusively dedicated to
asserting if using the connection does not return a BadRequest.
If it does the NewMinioStorage logs an error and returns. Otherwise
it keeps going knowing that BucketExists is not going to fail for
reasons unrelated to the existence of the bucket and the permissions
to access it.

(cherry picked from commit de599246059bf94c2a2ce89fdb753b6d105d83ea)
---
 modules/storage/minio.go      | 18 ++++++++++++++++
 modules/storage/minio_test.go | 39 +++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/modules/storage/minio.go b/modules/storage/minio.go
index 3246993bb1..26f947fdfe 100644
--- a/modules/storage/minio.go
+++ b/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,19 @@ func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage,
 		return nil, convertMinioErr(err)
 	}
 
+	// Check if the connection works
+	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 {
diff --git a/modules/storage/minio_test.go b/modules/storage/minio_test.go
index 8fdf31e6cf..bb1d0e954a 100644
--- a/modules/storage/minio_test.go
+++ b/modules/storage/minio_test.go
@@ -4,10 +4,17 @@
 package storage
 
 import (
+	"context"
+	"net/http"
 	"os"
 	"testing"
+	"time"
 
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
+
+	"github.com/minio/minio-go/v7"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestMinioStorageIterator(t *testing.T) {
@@ -25,3 +32,35 @@ func TestMinioStorageIterator(t *testing.T) {
 		},
 	})
 }
+
+func TestS3StorageBadRequest(t *testing.T) {
+	if os.Getenv("CI") == "" {
+		t.Skip("S3Storage not present outside of CI")
+		return
+	}
+	lc, cleanup := test.NewLogChecker("bad-request")
+	lc.StopMark("S3 storage connection failure")
+	defer cleanup()
+	cfg := &setting.Storage{
+		MinioConfig: setting.MinioStorageConfig{
+			Endpoint:        "minio:9000",
+			AccessKeyID:     "123456",
+			SecretAccessKey: "12345678",
+			Bucket:          "bucket",
+			Location:        "us-east-1",
+		},
+	}
+	message := "ERROR"
+	defer test.MockVariableValue(&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)
+
+	_, stopped := lc.Check(100 * time.Millisecond)
+	assert.False(t, stopped)
+}