From 8ecdc93f8b04f2d2e2e26bedb25fde045f0aff64 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Wed, 13 Sep 2023 09:18:52 +0800
Subject: [PATCH] Fix object storage path handling (#27024)

Object storage path rules:

* No single `/` or `.`, use empty string for root path
* Need to use trailing `/` for a list prefix to distinguish a "dir/"
---
 modules/storage/minio.go      | 28 ++++++++++++++--------------
 modules/storage/minio_test.go | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/modules/storage/minio.go b/modules/storage/minio.go
index 714530ab50a7..b58ab67dc747 100644
--- a/modules/storage/minio.go
+++ b/modules/storage/minio.go
@@ -136,9 +136,18 @@ func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage,
 }
 
 func (m *MinioStorage) buildMinioPath(p string) string {
-	p = util.PathJoinRelX(m.basePath, p)
+	p = strings.TrimPrefix(util.PathJoinRelX(m.basePath, p), "/") // object store doesn't use slash for root path
 	if p == "." {
-		p = "" // minio doesn't use dot as relative path
+		p = "" // object store doesn't use dot as relative path
+	}
+	return p
+}
+
+func (m *MinioStorage) buildMinioDirPrefix(p string) string {
+	// ending slash is required for avoiding matching like "foo/" and "foobar/" with prefix "foo"
+	p = m.buildMinioPath(p) + "/"
+	if p == "/" {
+		p = "" // object store doesn't use slash for root path
 	}
 	return p
 }
@@ -237,20 +246,11 @@ func (m *MinioStorage) URL(path, name string) (*url.URL, error) {
 // IterateObjects iterates across the objects in the miniostorage
 func (m *MinioStorage) IterateObjects(dirName string, fn func(path string, obj Object) error) error {
 	opts := minio.GetObjectOptions{}
-	lobjectCtx, cancel := context.WithCancel(m.ctx)
-	defer cancel()
-
-	basePath := m.basePath
-	if dirName != "" {
-		// ending slash is required for avoiding matching like "foo/" and "foobar/" with prefix "foo"
-		basePath = m.buildMinioPath(dirName) + "/"
-	}
-
-	for mObjInfo := range m.client.ListObjects(lobjectCtx, m.bucket, minio.ListObjectsOptions{
-		Prefix:    basePath,
+	for mObjInfo := range m.client.ListObjects(m.ctx, m.bucket, minio.ListObjectsOptions{
+		Prefix:    m.buildMinioDirPrefix(dirName),
 		Recursive: true,
 	}) {
-		object, err := m.client.GetObject(lobjectCtx, m.bucket, mObjInfo.Key, opts)
+		object, err := m.client.GetObject(m.ctx, m.bucket, mObjInfo.Key, opts)
 		if err != nil {
 			return convertMinioErr(err)
 		}
diff --git a/modules/storage/minio_test.go b/modules/storage/minio_test.go
index 56dfd9100a40..c6fbb91ab472 100644
--- a/modules/storage/minio_test.go
+++ b/modules/storage/minio_test.go
@@ -31,6 +31,40 @@ func TestMinioStorageIterator(t *testing.T) {
 	})
 }
 
+func TestMinioStoragePath(t *testing.T) {
+	m := &MinioStorage{basePath: ""}
+	assert.Equal(t, "", m.buildMinioPath("/"))
+	assert.Equal(t, "", m.buildMinioPath("."))
+	assert.Equal(t, "a", m.buildMinioPath("/a"))
+	assert.Equal(t, "a/b", m.buildMinioPath("/a/b/"))
+	assert.Equal(t, "", m.buildMinioDirPrefix(""))
+	assert.Equal(t, "a/", m.buildMinioDirPrefix("/a/"))
+
+	m = &MinioStorage{basePath: "/"}
+	assert.Equal(t, "", m.buildMinioPath("/"))
+	assert.Equal(t, "", m.buildMinioPath("."))
+	assert.Equal(t, "a", m.buildMinioPath("/a"))
+	assert.Equal(t, "a/b", m.buildMinioPath("/a/b/"))
+	assert.Equal(t, "", m.buildMinioDirPrefix(""))
+	assert.Equal(t, "a/", m.buildMinioDirPrefix("/a/"))
+
+	m = &MinioStorage{basePath: "/base"}
+	assert.Equal(t, "base", m.buildMinioPath("/"))
+	assert.Equal(t, "base", m.buildMinioPath("."))
+	assert.Equal(t, "base/a", m.buildMinioPath("/a"))
+	assert.Equal(t, "base/a/b", m.buildMinioPath("/a/b/"))
+	assert.Equal(t, "base/", m.buildMinioDirPrefix(""))
+	assert.Equal(t, "base/a/", m.buildMinioDirPrefix("/a/"))
+
+	m = &MinioStorage{basePath: "/base/"}
+	assert.Equal(t, "base", m.buildMinioPath("/"))
+	assert.Equal(t, "base", m.buildMinioPath("."))
+	assert.Equal(t, "base/a", m.buildMinioPath("/a"))
+	assert.Equal(t, "base/a/b", m.buildMinioPath("/a/b/"))
+	assert.Equal(t, "base/", m.buildMinioDirPrefix(""))
+	assert.Equal(t, "base/a/", m.buildMinioDirPrefix("/a/"))
+}
+
 func TestS3StorageBadRequest(t *testing.T) {
 	if os.Getenv("CI") == "" {
 		t.Skip("S3Storage not present outside of CI")