Merge #152475

152475: backup: normalize subdir format and index paths r=msbutler a=kev-cao

This commit ensures that when creating a backup job, the `subdir` is always normalized to start with a leading '/' and to not end with a trailing '/'. As a corollary, the backup index is also updated to store the absolute path from the collection URI root for every backup in the same way.

Epic: None

Release note: None

Co-authored-by: Kevin Cao <[email protected]>

Author: craig[bot]

pkg/backup/backup_job.go

@@ -9,8 +9,6 @@ import (
 	"context"
 	"fmt"
 	"math"
-	"net/url"
-	"path"
 	"reflect"
 	"sort"
 	"strconv"
@@ -842,16 +840,10 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error {
 	// potentially expensive listing of a giant backup collection to find the most
 	// recent completed entry.
 	if backupManifest.StartTime.IsEmpty() && details.CollectionURI != "" {
-		backupURI, err := url.Parse(details.URI)
+		suffix, err := backuputils.AbsoluteBackupPathInCollectionURI(details.CollectionURI, details.URI)
 		if err != nil {
 			return err
 		}
-		collectionURI, err := url.Parse(details.CollectionURI)
-		if err != nil {
-			return err
-		}
-
-		suffix := strings.TrimPrefix(path.Clean(backupURI.Path), path.Clean(collectionURI.Path))
 
 		c, err := p.ExecCfg().DistSQLSrv.ExternalStorageFromURI(ctx, details.CollectionURI, p.User())
 		if err != nil {

pkg/backup/backup_planning.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/backup/backupbase"
 	"github.com/cockroachdb/cockroach/pkg/backup/backupresolver"
+	"github.com/cockroachdb/cockroach/pkg/backup/backuputils"
 	"github.com/cockroachdb/cockroach/pkg/cloud"
 	"github.com/cockroachdb/cockroach/pkg/featureflag"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
@@ -636,7 +637,11 @@ func backupPlanHook(
 			initialDetails.Destination.Subdir = backupbase.LatestFileName
 			initialDetails.Destination.Exists = true
 		} else if subdir != "" {
-			initialDetails.Destination.Subdir = "/" + strings.TrimPrefix(subdir, "/")
+			normalizedSubdir, err := backuputils.NormalizeSubdir(subdir)
+			if err != nil {
+				return err
+			}
+			initialDetails.Destination.Subdir = normalizedSubdir
 			initialDetails.Destination.Exists = true
 		} else {
 			initialDetails.Destination.Subdir = endTime.GoTime().Format(backupbase.DateBasedIntoFolderName)

pkg/backup/backup_test.go

@@ -726,8 +726,15 @@ func TestBackupAndRestoreJobDescription(t *testing.T) {
 		"BACKUP INTO LATEST IN $4 WITH incremental_location=($1, $2, $3)",
 		append(incrementals, collections[0])...)
 
-	sqlDB.ExpectErr(t, "No full backup exists in \"/subdir\" to append an incremental backup to. To take a full backup, remove the subdirectory from the backup command",
-		"BACKUP INTO $4 IN ($1, $2, $3)", append(collections, "subdir")...)
+	nonExistentFullDir := time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC).Format(backupbase.DateBasedIntoFolderName)
+	sqlDB.ExpectErr(
+		t,
+		fmt.Sprintf(
+			`No full backup exists in "%s" to append an incremental backup to. To take a full backup, remove the subdirectory from the backup command`,
+			nonExistentFullDir,
+		),
+		"BACKUP INTO $4 IN ($1, $2, $3)", append(collections, nonExistentFullDir)...,
+	)
 
 	time.Sleep(time.Second + 2)
 	sqlDB.Exec(t, "BACKUP INTO ($1, $2, $3) AS OF SYSTEM TIME '-1s'", collections...)
@@ -757,7 +764,7 @@ func TestBackupAndRestoreJobDescription(t *testing.T) {
 		},
 	)
 	sqlDB.CheckQueryResults(t, "SELECT description FROM crdb_internal.jobs WHERE job_type = 'BACKUP' AND status = 'failed'",
-		[][]string{{fmt.Sprintf("BACKUP INTO '%s' IN ('%s', '%s', '%s')", "/subdir", collections[0],
+		[][]string{{fmt.Sprintf("BACKUP INTO '%s' IN ('%s', '%s', '%s')", nonExistentFullDir, collections[0],
 			collections[1], collections[2])}})
 
 	sqlDB.Exec(t, "DROP DATABASE data CASCADE")

pkg/backup/backupdest/backup_index.go

@@ -62,22 +62,7 @@ func WriteBackupIndexMetadata(
 		return errors.AssertionFailedf("incremental backup details missing a start time")
 	}
 
-	var backupCollectionURI string
-	// Find the root of the collection URI that the backup is being written to so
-	// that we can determine the relative path of the backup.
-	if details.StartTime.IsEmpty() {
-		backupCollectionURI = details.CollectionURI
-	} else {
-		var err error
-		backupCollectionURI, err = ResolveDefaultBaseIncrementalStorageLocation(
-			details.Destination.To, details.Destination.IncrementalStorage,
-		)
-		if err != nil {
-			return errors.Wrapf(err, "get incremental backup collection URI")
-		}
-	}
-
-	path, err := backuputils.RelativeBackupPathInCollectionURI(backupCollectionURI, details.URI)
+	path, err := backuputils.AbsoluteBackupPathInCollectionURI(details.CollectionURI, details.URI)
 	if err != nil {
 		return errors.Wrapf(err, "get relative backup path")
 	}

pkg/backup/backupdest/backup_index_test.go

@@ -215,11 +215,6 @@ func TestWriteBackupIndexMetadataWithLocalityAwareBackups(t *testing.T) {
 
 	require.True(t, fullIndex.StartTime.IsEmpty())
 	require.False(t, incrIndex.StartTime.IsEmpty())
-
-	// Ensure that the incremental path index starts immediately with the full
-	// index path, implying that its path starts from the root of the incremental
-	// storage directory.
-	require.True(t, strings.HasPrefix(incrIndex.Path, fullIndex.Path))
 }
 
 func TestWriteBackupindexMetadataWithSpecifiedIncrementalLocation(t *testing.T) {

pkg/backup/backuputils/BUILD.bazel

@@ -21,7 +21,10 @@ go_library(
 
 go_test(
     name = "backuputils_test",
-    srcs = ["memory_backed_quota_pool_test.go"],
+    srcs = [
+        "memory_backed_quota_pool_test.go",
+        "utils_test.go",
+    ],
     embed = [":backuputils"],
     deps = [
         "//pkg/settings/cluster",

pkg/backup/backuputils/utils.go

@@ -9,11 +9,13 @@ import (
 	"encoding/hex"
 	"net/url"
 	"path"
+	"regexp"
 	"strings"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/cloud"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
+	"github.com/cockroachdb/errors"
 )
 
 // URLSeparator represents the standard separator used in backup URLs.
@@ -100,16 +102,18 @@ func EncodeDescendingTS(ts time.Time) string {
 	return hex.EncodeToString(buffer)
 }
 
-// RelativeBackupPathInCollectionURI returns the relative path of a backup
-// within a collection URI. Backup URI represents the URI that points to the
-// directory containing the backup manifest of the backup.
+// AbsoluteBackupPathInCollectionURI returns the absolute path of a backup
+// assuming the root is the collection URI. Backup URI represents the URI that
+// points to the directory containing the backup manifest of the backup. Since
+// this is an absolute path, it always starts with `/`. Any trailing slash is
+// also removed.
 //
 // Example:
 //
 //	collectionURI: "nodelocal://1/collection"
-//	backupURI: "nodelocal://1/collection/backup1/"
-//	returns: "backup1/"
-func RelativeBackupPathInCollectionURI(collectionURI string, backupURI string) (string, error) {
+//	backupURI: "nodelocal://1/collection/path/to/backup"
+//	returns: "/path/to/backup"
+func AbsoluteBackupPathInCollectionURI(collectionURI string, backupURI string) (string, error) {
 	backupURL, err := url.Parse(backupURI)
 	if err != nil {
 		return "", err
@@ -119,6 +123,44 @@ func RelativeBackupPathInCollectionURI(collectionURI string, backupURI string) (
 		return "", err
 	}
 
-	relPath := strings.TrimPrefix(path.Clean(backupURL.Path), path.Clean(collectionURL.Path))
+	if backupURL.Scheme != collectionURL.Scheme || backupURL.Host != collectionURL.Host {
+		return "", errors.New("backup URI does not share the same scheme and host as collection URI")
+	}
+
+	collectionPath := path.Clean(collectionURL.Path)
+	if collectionPath == "." {
+		collectionPath = ""
+	}
+
+	backupPath := path.Clean(backupURL.Path)
+	if backupPath == "." {
+		backupPath = ""
+	}
+
+	relPath, found := strings.CutPrefix(backupPath, collectionPath)
+	if !found {
+		return "", errors.New("backup URI not contained within collection URI")
+	}
+
+	relPath = strings.TrimSuffix(relPath, string(URLSeparator))
+	if len(relPath) == 0 || relPath[0] != URLSeparator {
+		relPath = string(URLSeparator) + relPath
+	}
 	return relPath, nil
 }
+
+// NormalizeSubdir takes a provided full backup subdirectory and normalizes it
+// to the form /YYYY/MM/DD-HHMMSS.SS with a leading slash and no trailing slash.
+func NormalizeSubdir(subdir string) (string, error) {
+	subdirPattern := regexp.MustCompile(`\/?\d{4}\/\d{2}\/\d{2}-\d{6}\.\d{2}\/?`)
+	if !subdirPattern.Match([]byte(subdir)) {
+		return "", errors.Newf(
+			`provided subdir "%s" does not match expected format YYYY/MM/DD-HHMMSS.SS`, subdir,
+		)
+	}
+	normalized := strings.TrimSuffix(subdir, string(URLSeparator))
+	if normalized[0] != URLSeparator {
+		normalized = string(URLSeparator) + normalized
+	}
+	return normalized, nil
+}

pkg/backup/backuputils/utils_test.go

@@ -0,0 +1,127 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package backuputils
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestAbsoluteBackupPathInCollectionURI(t *testing.T) {
+	testcases := []struct {
+		name                     string
+		collectionURI, backupURI string
+		expected                 string
+		error                    string
+	}{
+		{
+			name:          "collection URI rooted at /",
+			collectionURI: "nodelocal://1/",
+			backupURI:     "nodelocal://1/foo/bar/baz",
+			expected:      "/foo/bar/baz",
+		},
+		{
+			name:          "backup URI contains trailing slash",
+			collectionURI: "nodelocal://1/",
+			backupURI:     "nodelocal://1/foo/bar/baz/",
+			expected:      "/foo/bar/baz",
+		},
+		{
+			name:          "collection URI contains non-empty path",
+			collectionURI: "s3://backup/foo/bar",
+			backupURI:     "s3://backup/foo/bar/baz",
+			expected:      "/baz",
+		},
+		{
+			name:          "collection and backup URI ends in trailing slash",
+			collectionURI: "external://backup/foo/bar/",
+			backupURI:     "external://backup/foo/bar/baz/qux/",
+			expected:      "/baz/qux",
+		},
+		{
+			name:          "backupURI is the same as collection URI",
+			collectionURI: "gs://backup/foo/bar",
+			backupURI:     "gs://backup/foo/bar",
+			expected:      "/",
+		},
+		{
+			name:          "backup URI not in collection URI",
+			collectionURI: "gs://backup/foo/bar",
+			backupURI:     "gs://backup/baz",
+			error:         "backup URI not contained within collection URI",
+		},
+		{
+			name:          "backup URI does not share same scheme or host as collection URI",
+			collectionURI: "nodelocal://1/foo/bar",
+			backupURI:     "gs://backup/foo/bar/baz",
+			error:         "backup URI does not share the same scheme and host as collection URI",
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			result, err := AbsoluteBackupPathInCollectionURI(tc.collectionURI, tc.backupURI)
+			if tc.error != "" {
+				require.Error(t, err)
+				require.ErrorContains(t, err, tc.error)
+				return
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, tc.expected, result)
+		})
+	}
+}
+
+func TestNormalizeSubdir(t *testing.T) {
+	testcases := []struct {
+		name       string
+		input      string
+		normalized string
+		error      string
+	}{
+		{
+			name:       "subdir without leading slash",
+			input:      "2025/08/26-112233.44",
+			normalized: "/2025/08/26-112233.44",
+		},
+		{
+			name:       "subdir with leading and trailing slashes",
+			input:      "/2025/08/26-112233.44/",
+			normalized: "/2025/08/26-112233.44",
+		},
+		{
+			name:       "subdir with only trailing slash",
+			input:      "2025/08/26-112233.44/",
+			normalized: "/2025/08/26-112233.44",
+		},
+		{
+			name:       "subdir already normalized",
+			input:      "/2025/08/26-112233.44",
+			normalized: "/2025/08/26-112233.44",
+		},
+		{
+			name:  "invalid subdir format",
+			input: "2023-10-05_123456.78",
+			error: "does not match expected format YYYY/MM/DD-HHMMSS.SS",
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			result, err := NormalizeSubdir(tc.input)
+			if tc.error != "" {
+				require.Error(t, err)
+				require.ErrorContains(t, err, tc.error)
+				return
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, tc.normalized, result)
+		})
+	}
+}