fix(s3tables): let a table renamed across namespaces keep committing (#12515)

* s3tables: let a table renamed across namespaces keep committing

* s3tables: keep the same-directory exception off a directory another table shares

A record written before metadata locations were confined to their namespace
(6f93f874) and claimed (#12473) can keep its files in another table's
directory, in another namespace. The exception that lets a table renamed
across namespaces keep committing to the directory it left behind accepted
such a record's commits too, where master refused them.

The namespace check now runs first, as on master. Only a location it refuses
is matched against the record's current directory, and the exception holds
only when no other record keeps its files there. Where other records' entries
sit is ignored, so a table created under the renamed table's old name, whose
files go to a directory of its own, does not take the exception away. The
scan runs only on that path, so commits inside the namespace cost what they
did.

Co-Authored-By: Claude <noreply@anthropic.com>

* s3tables: seed the table at the renamed table's old name

A rename may keep the old name taken, so creating a table there can be
refused; seeding the record keeps the test on the update path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>

weed/s3api/s3tables/filer_ops.go

@@ -1091,6 +1091,24 @@ func RootClaimed(roots map[string][]string, selfPath, root string) bool {
 	return false
 }
 
+// rootFilesShared reports whether a record other than selfPath keeps its files
+// at root, above it or under it. Unlike RootClaimed it ignores where the other
+// records themselves sit: a table created under a renamed table's old name has
+// its entry on that directory and its files elsewhere.
+func rootFilesShared(roots map[string][]string, selfPath, root string) bool {
+	for recordPath, others := range roots {
+		if recordPath == selfPath {
+			continue
+		}
+		for _, other := range others {
+			if PathsOverlap(other, root) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // checkLocationUnclaimed returns ErrLocationInUse when root, the one the record
 // at recordPath would have, is claimed by another record.
 func checkLocationUnclaimed(ctx context.Context, client filer_pb.SeaweedFilerClient, bucketName, recordPath, root string) error {
@@ -1174,6 +1192,27 @@ func (h *S3TablesHandler) refuseClaimedLocation(w http.ResponseWriter, r *http.R
 	return err
 }
 
+// refuseUpdatedMetadataLocation answers an update whose metadata location the
+// record at recordPath, whose stored metadata is current, may not take; see
+// validateUpdatedMetadataLocation.
+func (h *S3TablesHandler) refuseUpdatedMetadataLocation(w http.ResponseWriter, r *http.Request, filerClient FilerClient, bucketName, namespace, recordPath string, current *tableMetadataInternal, metadataLocation string) error {
+	var checkErr error
+	err := validateUpdatedMetadataLocation(metadataLocation, bucketName, namespace, recordPath, current, func(root string) (shared bool, err error) {
+		checkErr = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
+			roots, err := CatalogRecordRoots(r.Context(), client, bucketName)
+			shared = err == nil && rootFilesShared(roots, recordPath, root)
+			return err
+		})
+		return shared, checkErr
+	})
+	if checkErr != nil {
+		h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, "failed to check location")
+	} else if err != nil {
+		h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error())
+	}
+	return err
+}
+
 // claimTreeDirectory writes a fresh delete claim onto the directory at dirPath
 // while it still carries expected, and returns the claim with the attributes it
 // left. A claim seen in expected, one a delete that died left, is replaced like

weed/s3api/s3tables/handler_table.go

@@ -2004,7 +2004,9 @@ func (h *S3TablesHandler) authorizeUpdateTable(w http.ResponseWriter, r *http.Re
 		return nil, err
 	}
 
-	if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil {
+	// Where the location may point depends on the stored record, so only its
+	// shape is checked here; the rest waits for authorization below.
+	if _, err := validateMetadataLocationShape(req.MetadataLocation, bucketName); err != nil {
 		h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error())
 		return nil, err
 	}
@@ -2107,6 +2109,12 @@ func (h *S3TablesHandler) authorizeUpdateTable(w http.ResponseWriter, r *http.Re
 		return nil, NewAuthError("UpdateTable", principal, "not authorized to update table")
 	}
 
+	// After authorization, so a caller who may not update the table learns
+	// nothing about where its files live.
+	if err := h.refuseUpdatedMetadataLocation(w, r, filerClient, bucketName, namespaceName, tablePath, &metadata, req.MetadataLocation); err != nil {
+		return nil, err
+	}
+
 	return &updateTableTarget{
 		bucketName:     bucketName,
 		namespaceName:  namespaceName,

weed/s3api/s3tables/handler_update_after_rename_test.go

@@ -0,0 +1,248 @@
+package s3tables
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest"
+)
+
+// A rename is catalog-only: a table or view renamed into another namespace keeps
+// its files, and so the directory its metadata location names, under the
+// namespace it came from. Its next update writes there and must be accepted,
+// while an update that moves it elsewhere stays confined to its new namespace.
+
+func addRenameNamespace(t *testing.T, fs *s3tablestest.MemFiler, namespace string) {
+	t.Helper()
+	nsMeta, err := json.Marshal(namespaceMetadata{Namespace: []string{namespace}, OwnerAccountID: DefaultAccountID})
+	require.NoError(t, err)
+	fs.Put(GetTableBucketPath(renameTestBucket), namespace, map[string][]byte{ExtendedKeyMetadata: nsMeta})
+}
+
+// renamedAcrossNamespaces seeds ns/t, whose files sit under ns/t, and renames it
+// to ns2/t.
+func renamedAcrossNamespaces(t *testing.T) (*s3tablestest.MemFiler, *Manager) {
+	t.Helper()
+	fs, m := startRenameManager(t)
+	addRenameNamespace(t, fs, "ns2")
+	require.NoError(t, runRename(t, m, fs, &RenameTableRequest{TableBucketARN: mustBucketARN(t),
+		SourceNamespace: []string{"ns"}, SourceName: "t", DestNamespace: []string{"ns2"}, DestName: "t"}))
+	return fs, m
+}
+
+func seedRenameTable(t *testing.T, fs *s3tablestest.MemFiler, namespace, name string) {
+	t.Helper()
+	meta, err := json.Marshal(tableMetadataInternal{Name: name, Namespace: namespace, Format: "ICEBERG", OwnerAccountID: DefaultAccountID,
+		MetadataVersion: 1, MetadataLocation: "s3://" + renameTestBucket + "/" + namespace + "/" + name + "/metadata/v1.metadata.json"})
+	require.NoError(t, err)
+	fs.Put(GetNamespacePath(renameTestBucket, namespace), name, map[string][]byte{ExtendedKeyMetadata: meta})
+}
+
+func updateTableAt(ctx context.Context, t *testing.T, m *Manager, fs *s3tablestest.MemFiler, namespace, location string) error {
+	return m.Execute(ctx, NewManagerClient(fs.Client), "UpdateTable", &UpdateTableRequest{TableBucketARN: mustBucketARN(t),
+		Namespace: []string{namespace}, Name: "t", MetadataVersion: 4, MetadataLocation: location}, nil, "")
+}
+
+func s3TablesErrorText(err error) string {
+	var tableErr *S3TablesError
+	if errors.As(err, &tableErr) {
+		return tableErr.Type + ": " + tableErr.Message
+	}
+	if err != nil {
+		return err.Error()
+	}
+	return ""
+}
+
+func TestUpdateAfterRenameAcrossNamespacesKeepsItsDirectory(t *testing.T) {
+	fs, m := renamedAcrossNamespaces(t)
+	location := "s3://" + renameTestBucket + "/ns/t/metadata/v4.metadata.json"
+	require.NoError(t, updateTableAt(context.Background(), t, m, fs, "ns2", location))
+
+	got, err := runGetTable(t, m, fs, "ns2", "t")
+	require.NoError(t, err)
+	require.Equal(t, location, got.MetadataLocation)
+}
+
+func TestUpdateAfterRenameAcrossNamespacesRefusesOtherDirectoriesOfTheOldNamespace(t *testing.T) {
+	fs, m := renamedAcrossNamespaces(t)
+	err := updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns/elsewhere/metadata/v4.metadata.json")
+	require.Error(t, err)
+	require.Contains(t, s3TablesErrorText(err), "must be within namespace ns2")
+}
+
+func TestUpdateAfterRenameAcrossNamespacesRefusesAnotherTablesDirectory(t *testing.T) {
+	// One the old namespace holds: not the table's own directory, and outside
+	// its current namespace.
+	fs, m := renamedAcrossNamespaces(t)
+	seedRenameTable(t, fs, "ns", "u")
+	err := updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns/u/metadata/v9.metadata.json")
+	require.Error(t, err)
+	require.Contains(t, s3TablesErrorText(err), "must be within namespace ns2")
+
+	// One the current namespace holds: inside the namespace, but claimed.
+	fs, m = renamedAcrossNamespaces(t)
+	seedRenameTable(t, fs, "ns2", "w")
+	err = updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns2/w/metadata/v9.metadata.json")
+	require.Error(t, err)
+	require.Contains(t, s3TablesErrorText(err), ErrCodeConflict+": location is in use")
+}
+
+// A table created under the renamed table's old name has its entry on that
+// directory and its files in a directory of its own, so the renamed table still
+// keeps its files there alone. The record is seeded rather than created, since
+// a rename may keep the old name taken.
+func TestUpdateAfterRenameAcrossNamespacesKeepsItsDirectoryUnderANewTableOfTheOldName(t *testing.T) {
+	ctx := context.Background()
+	fs, m := renamedAcrossNamespaces(t)
+	meta, err := json.Marshal(tableMetadataInternal{Name: "t", Namespace: "ns", Format: "ICEBERG", OwnerAccountID: DefaultAccountID,
+		MetadataVersion: 1, WarehouseLocation: "s3://" + renameTestBucket + "/ns/t-fresh"})
+	require.NoError(t, err)
+	fs.Put(GetNamespacePath(renameTestBucket, "ns"), "t", map[string][]byte{ExtendedKeyMetadata: meta})
+
+	location := "s3://" + renameTestBucket + "/ns/t/metadata/v4.metadata.json"
+	require.NoError(t, updateTableAt(ctx, t, m, fs, "ns2", location))
+	got, err := runGetTable(t, m, fs, "ns2", "t")
+	require.NoError(t, err)
+	require.Equal(t, location, got.MetadataLocation)
+}
+
+// A record written before locations were confined to their namespace and
+// claimed can keep its files in another table's directory. It gets no
+// exception: it stays held to its namespace, as before.
+func TestUpdateRefusesADirectoryOutsideTheNamespaceThatAnotherTableShares(t *testing.T) {
+	fs, m := startRenameManager(t)
+	addRenameNamespace(t, fs, "ns2")
+	meta, err := json.Marshal(tableMetadataInternal{Name: "t", Namespace: "ns2", Format: "ICEBERG", OwnerAccountID: DefaultAccountID,
+		MetadataVersion: 3, MetadataLocation: "s3://" + renameTestBucket + "/ns/t/metadata/v3.metadata.json"})
+	require.NoError(t, err)
+	fs.Put(GetNamespacePath(renameTestBucket, "ns2"), "t", map[string][]byte{ExtendedKeyMetadata: meta})
+
+	err = updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns/t/metadata/v4.metadata.json")
+	require.Error(t, err)
+	require.Contains(t, s3TablesErrorText(err), "must be within namespace ns2")
+}
+
+func TestUpdateAfterRenameAcrossNamespacesMayMoveIntoItsNamespace(t *testing.T) {
+	fs, m := renamedAcrossNamespaces(t)
+	require.NoError(t, updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns2/t-moved/metadata/v4.metadata.json"))
+}
+
+func TestUpdateStillConfinesAMoveToTheTablesNamespace(t *testing.T) {
+	fs, m := startRenameManager(t)
+	addRenameNamespace(t, fs, "ns2")
+	err := updateTableAt(context.Background(), t, m, fs, "ns", "s3://"+renameTestBucket+"/ns2/elsewhere/metadata/v4.metadata.json")
+	require.Error(t, err)
+	require.Contains(t, s3TablesErrorText(err), "must be within namespace ns")
+}
+
+func TestUpdateLocationIsCheckedAfterAuthorization(t *testing.T) {
+	fs, m := renamedAcrossNamespaces(t)
+	m.SetTrusted(false)
+	m.SetDefaultAllow(false)
+	stranger := &testIdentity{Name: "stranger", Account: &testIdentityAccount{Id: "stranger"}}
+	ctx := s3_constants.SetIdentityInContext(context.Background(), stranger)
+
+	err := updateTableAt(ctx, t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns/elsewhere/metadata/v4.metadata.json")
+	require.Error(t, err)
+	require.NotContains(t, s3TablesErrorText(err), "must be within namespace",
+		"a caller who may not update the table must not learn where its files may go")
+}
+
+func renameViewAcrossNamespaces(t *testing.T) (*s3tablestest.MemFiler, *Manager) {
+	t.Helper()
+	fs, m := startRenameManager(t)
+	seedView(t, fs, "v")
+	addRenameNamespace(t, fs, "ns2")
+	require.NoError(t, m.Execute(context.Background(), NewManagerClient(fs.Client), "RenameView", &RenameTableRequest{
+		TableBucketARN: mustBucketARN(t), SourceNamespace: []string{"ns"}, SourceName: "v", DestNamespace: []string{"ns2"}, DestName: "v",
+	}, nil, ""))
+	return fs, m
+}
+
+func updateViewAt(t *testing.T, m *Manager, fs *s3tablestest.MemFiler, location string) error {
+	return m.Execute(context.Background(), NewManagerClient(fs.Client), "UpdateView", &UpdateViewRequest{TableBucketARN: mustBucketARN(t),
+		Namespace: []string{"ns2"}, Name: "v", MetadataVersion: 2, MetadataLocation: location}, nil, "")
+}
+
+func TestUpdateViewAfterRenameAcrossNamespacesKeepsItsDirectory(t *testing.T) {
+	fs, m := renameViewAcrossNamespaces(t)
+	location := "s3://" + renameTestBucket + "/ns/v/metadata/v2.metadata.json"
+	require.NoError(t, updateViewAt(t, m, fs, location))
+
+	entry := fs.Get(GetNamespacePath(renameTestBucket, "ns2"), "v")
+	require.NotNil(t, entry)
+	var stored tableMetadataInternal
+	require.NoError(t, json.Unmarshal(entry.Extended[ExtendedKeyMetadata], &stored))
+	require.Equal(t, location, stored.MetadataLocation)
+}
+
+func TestUpdateViewAfterRenameAcrossNamespacesRefusesOtherDirectories(t *testing.T) {
+	fs, m := renameViewAcrossNamespaces(t)
+	err := updateViewAt(t, m, fs, "s3://"+renameTestBucket+"/ns/elsewhere/metadata/v2.metadata.json")
+	require.Error(t, err)
+	require.Contains(t, s3TablesErrorText(err), "must be within namespace ns2")
+}
+
+// The same-directory exception compares the directory the location names. A
+// location whose directory is the bucket root (s3://bucket/metadata/...) names
+// no table directory, so it must not match the root a record falls back to and
+// must meet the namespace check, as it did before the exception existed.
+func TestUpdateRefusesAMetadataDirectoryAtTheBucketRoot(t *testing.T) {
+	bucketRoot := "s3://" + renameTestBucket + "/metadata/v9.metadata.json"
+	ctx := context.Background()
+
+	t.Run("table created without a location", func(t *testing.T) {
+		fs, m := startRenameManager(t)
+		require.NoError(t, m.Execute(ctx, NewManagerClient(fs.Client), "CreateTable", &CreateTableRequest{TableBucketARN: mustBucketARN(t),
+			Namespace: []string{"ns"}, Name: "fresh", Format: "ICEBERG"}, nil, ""))
+		err := m.Execute(ctx, NewManagerClient(fs.Client), "UpdateTable", &UpdateTableRequest{TableBucketARN: mustBucketARN(t),
+			Namespace: []string{"ns"}, Name: "fresh", MetadataVersion: 2, MetadataLocation: bucketRoot}, nil, "")
+		require.Error(t, err)
+		require.Contains(t, s3TablesErrorText(err), "must be within namespace ns")
+	})
+	t.Run("table stored at its name path", func(t *testing.T) {
+		fs, m := startRenameManager(t)
+		err := updateTableAt(ctx, t, m, fs, "ns", bucketRoot)
+		require.Error(t, err)
+		require.Contains(t, s3TablesErrorText(err), "must be within namespace ns")
+	})
+	t.Run("view", func(t *testing.T) {
+		fs, m := startRenameManager(t)
+		seedView(t, fs, "v")
+		err := m.Execute(ctx, NewManagerClient(fs.Client), "UpdateView", &UpdateViewRequest{TableBucketARN: mustBucketARN(t),
+			Namespace: []string{"ns"}, Name: "v", MetadataVersion: 2, MetadataLocation: bucketRoot}, nil, "")
+		require.Error(t, err)
+		require.Contains(t, s3TablesErrorText(err), "must be within namespace ns")
+	})
+}
+
+// A table created without a location keeps its files in the warehouse directory
+// it was given; its first commit writes there, also after a rename across
+// namespaces.
+func TestUpdateOfATableCreatedWithoutALocationWritesItsWarehouse(t *testing.T) {
+	ctx := context.Background()
+	fs, m := startRenameManager(t)
+	addRenameNamespace(t, fs, "ns2")
+	client := NewManagerClient(fs.Client)
+	require.NoError(t, m.Execute(ctx, client, "CreateTable", &CreateTableRequest{TableBucketARN: mustBucketARN(t),
+		Namespace: []string{"ns"}, Name: "fresh", Format: "ICEBERG"}, nil, ""))
+	created, err := runGetTable(t, m, fs, "ns", "fresh")
+	require.NoError(t, err)
+	require.NotEmpty(t, created.WarehouseLocation)
+
+	require.NoError(t, runRename(t, m, fs, &RenameTableRequest{TableBucketARN: mustBucketARN(t),
+		SourceNamespace: []string{"ns"}, SourceName: "fresh", DestNamespace: []string{"ns2"}, DestName: "fresh"}))
+	location := created.WarehouseLocation + "/metadata/v1.metadata.json"
+	require.NoError(t, m.Execute(ctx, client, "UpdateTable", &UpdateTableRequest{TableBucketARN: mustBucketARN(t),
+		Namespace: []string{"ns2"}, Name: "fresh", MetadataVersion: 1, MetadataLocation: location}, nil, ""))
+
+	got, err := runGetTable(t, m, fs, "ns2", "fresh")
+	require.NoError(t, err)
+	require.Equal(t, location, got.MetadataLocation)
+}

weed/s3api/s3tables/handler_view.go

@@ -467,7 +467,8 @@ func (h *S3TablesHandler) authorizeUpdateView(w http.ResponseWriter, r *http.Req
 		return nil, err
 	}
 
-	if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil {
+	// As for tables: the shape now, where it may point once authorized.
+	if _, err := validateMetadataLocationShape(req.MetadataLocation, bucketName); err != nil {
 		h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error())
 		return nil, err
 	}
@@ -511,6 +512,10 @@ func (h *S3TablesHandler) authorizeUpdateView(w http.ResponseWriter, r *http.Req
 		return nil, NewAuthError("UpdateView", accountID, "not authorized to update view")
 	}
 
+	if err := h.refuseUpdatedMetadataLocation(w, r, filerClient, bucketName, namespaceName, viewPath, &metadata, req.MetadataLocation); err != nil {
+		return nil, err
+	}
+
 	return &updateViewTarget{
 		bucketName:     bucketName,
 		namespaceName:  namespaceName,

weed/s3api/s3tables/utils.go

@@ -139,12 +139,63 @@ func ValidateMetadataLocation(metadataLocation, bucketName, namespace string) er
 	if metadataLocation == "" {
 		return nil
 	}
-	bucket, tablePath, err := parseS3Location(metadataLocation)
+	tablePath, err := validateMetadataLocationShape(metadataLocation, bucketName)
 	if err != nil {
 		return err
 	}
+	if !strings.HasPrefix(tablePath, namespace+"/") {
+		return fmt.Errorf("metadata location must be within namespace %s", namespace)
+	}
+	return nil
+}
+
+// validateUpdatedMetadataLocation is ValidateMetadataLocation for an update of
+// the record at recordPath, whose stored metadata is current. A location
+// outside the record's namespace is accepted only when it keeps the record in
+// the directory it already holds its files under and rootShared reports that
+// no other record keeps its files there: a rename across namespaces moves the
+// record and leaves its files behind, while a record written before locations
+// were confined and claimed may share another table's directory. A location
+// that moves the record to another directory must lie inside the namespace,
+// and the update still refuses one another record claims.
+func validateUpdatedMetadataLocation(metadataLocation, bucketName, namespace, recordPath string, current *tableMetadataInternal, rootShared func(root string) (bool, error)) error {
+	err := ValidateMetadataLocation(metadataLocation, bucketName, namespace)
+	if err == nil {
+		return nil
+	}
+	// A malformed location is refused whatever directory it cleans up to.
+	if _, shapeErr := validateMetadataLocationShape(metadataLocation, bucketName); shapeErr != nil {
+		return shapeErr
+	}
+	// The directory the location itself names, with no fallback: CatalogRecordRoot
+	// replaces a directory outside the bucket subtree (s3://bucket/metadata/...)
+	// with the record's warehouse or name path, which would match the current
+	// root and let a location no load can resolve skip the namespace check.
+	dir := TableDataDirFromMetadataLocation(metadataLocation)
+	if !strings.HasPrefix(dir, GetTableBucketPath(bucketName)+"/") || dir != current.root(bucketName, recordPath) {
+		return err
+	}
+	if shared, sharedErr := rootShared(dir); sharedErr != nil {
+		return sharedErr
+	} else if shared {
+		return err
+	}
+	return nil
+}
+
+// validateMetadataLocationShape checks what a metadata location must satisfy
+// whatever record it belongs to: it names the table bucket and a table path of
+// safe segments. It returns that path. An empty location passes.
+func validateMetadataLocationShape(metadataLocation, bucketName string) (string, error) {
+	if metadataLocation == "" {
+		return "", nil
+	}
+	bucket, tablePath, err := parseS3Location(metadataLocation)
+	if err != nil {
+		return "", err
+	}
 	if bucket != bucketName {
-		return fmt.Errorf("metadata location must be within bucket %s", bucketName)
+		return "", fmt.Errorf("metadata location must be within bucket %s", bucketName)
 	}
 	hasSegment := false
 	for _, segment := range strings.Split(tablePath, "/") {
@@ -152,17 +203,14 @@ func ValidateMetadataLocation(metadataLocation, bucketName, namespace string) er
 			continue
 		}
 		if segment == "." || segment == ".." || strings.ContainsAny(segment, "\\\x00") {
-			return fmt.Errorf("invalid metadata location path")
+			return "", fmt.Errorf("invalid metadata location path")
 		}
 		hasSegment = true
 	}
 	if !hasSegment {
-		return fmt.Errorf("metadata location must include a table path")
+		return "", fmt.Errorf("metadata location must include a table path")
 	}
-	if !strings.HasPrefix(tablePath, namespace+"/") {
-		return fmt.Errorf("metadata location must be within namespace %s", namespace)
-	}
-	return nil
+	return tablePath, nil
 }
 
 // ValidateEncryptionConfiguration admits only the AES256 declaration, under the