diff --git a/weed/s3api/iceberg/handlers_table.go b/weed/s3api/iceberg/handlers_table.go index f330618855..5901d17fa8 100644 --- a/weed/s3api/iceberg/handlers_table.go +++ b/weed/s3api/iceberg/handlers_table.go @@ -89,6 +89,34 @@ func (s *Server) handleListTables(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, result) } +// tablePathOccupied reports whether a filer entry already exists at the catalog +// name path. A leftover directory there (e.g. data kept when another table was +// renamed to this name) means a table created at the default location would +// overwrite it, so the caller routes the new table to a unique location. A +// lookup failure other than not-found is returned so the caller can fail the +// create rather than fall back to the default path on a transient filer error. +func (s *Server) tablePathOccupied(ctx context.Context, bucketName, tablePath string) (bool, error) { + full := path.Join(s3tables.TablesPath, bucketName, tablePath) + dir, name := path.Split(full) + dir = strings.TrimSuffix(dir, "/") + occupied := false + err := s.filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { + _, err := filer_pb.LookupEntry(ctx, client, &filer_pb.LookupDirectoryEntryRequest{ + Directory: dir, + Name: name, + }) + if errors.Is(err, filer_pb.ErrNotFound) { + return nil + } + if err != nil { + return err + } + occupied = true + return nil + }) + return occupied, err +} + // handleCreateTable creates a new table. func (s *Server) handleCreateTable(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) @@ -124,6 +152,17 @@ func (s *Server) handleCreateTable(w http.ResponseWriter, r *http.Request) { tablePath := path.Join(flattenNamespacePath(namespace), req.Name) location := strings.TrimSuffix(req.Location, "/") if location == "" { + // If a leftover directory already occupies the default location (data + // kept when another table was renamed to this name), route this table + // to a unique location so it cannot overwrite that table's files. + occupied, err := s.tablePathOccupied(r.Context(), bucketName, tablePath) + if err != nil { + writeError(w, http.StatusInternalServerError, "InternalServerError", "Failed to check table location: "+err.Error()) + return + } + if occupied { + tablePath = path.Join(flattenNamespacePath(namespace), req.Name+"-"+tableUUID.String()) + } if req.Properties != nil { if warehouse := strings.TrimSuffix(req.Properties["warehouse"], "/"); warehouse != "" { location = fmt.Sprintf("%s/%s", warehouse, tablePath) diff --git a/weed/s3api/s3tables/handler_delete_decouple_test.go b/weed/s3api/s3tables/handler_delete_decouple_test.go new file mode 100644 index 0000000000..966d9b12e0 --- /dev/null +++ b/weed/s3api/s3tables/handler_delete_decouple_test.go @@ -0,0 +1,94 @@ +package s3tables + +import ( + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func runDeleteTable(t *testing.T, m *Manager, fs *memFilerServer, namespace, name string) error { + t.Helper() + return m.Execute(context.Background(), NewManagerClient(fs.client), "DeleteTable", &DeleteTableRequest{ + TableBucketARN: mustBucketARN(t), + Namespace: []string{namespace}, + Name: name, + }, nil, "") +} + +// A table whose data was decoupled from its name (created over a leftover from a +// rename): catalog marker at ns/newt, data at ns/newt-x, and ns/newt still holds +// another table's leftover data. Dropping it must purge only its own data and +// clear the marker, never the data under the reused name path. +func TestDeleteTableDecoupledKeepsReusedNamePath(t *testing.T) { + fs, m := startRenameManager(t) + + newtMeta, _ := json.Marshal(tableMetadataInternal{ + Name: "newt", + Namespace: "ns", + Format: "ICEBERG", + OwnerAccountID: DefaultAccountID, + MetadataLocation: "s3://" + renameTestBucket + "/ns/newt-x/metadata/v1.metadata.json", + }) + markerKeys := []string{ExtendedKeyMetadata, ExtendedKeyMetadataVersion, ExtendedKeyPolicy, ExtendedKeyTags, ExtendedKeyEntryType} + fs.putEntry(GetNamespacePath(renameTestBucket, "ns"), "newt", map[string][]byte{ + ExtendedKeyMetadata: newtMeta, + ExtendedKeyMetadataVersion: []byte("v1"), + ExtendedKeyPolicy: []byte(`{"Version":"2012-10-17"}`), + ExtendedKeyTags: []byte(`{"k":"v"}`), + ExtendedKeyEntryType: []byte(EntryTypeTable), + }) + fs.putEntry(GetTablePath(renameTestBucket, "ns", "newt"), "leftover", nil) // another table's data under the name path + fs.putEntry(GetNamespacePath(renameTestBucket, "ns"), "newt-x", nil) // this table's own (decoupled) data + fs.putEntry(GetTablePath(renameTestBucket, "ns", "newt-x"), "metadata", nil) + + require.NoError(t, runDeleteTable(t, m, fs, "ns", "newt")) + + assert.Nil(t, fs.getEntry(GetNamespacePath(renameTestBucket, "ns"), "newt-x"), + "the table's own data location must be purged") + assert.NotNil(t, fs.getEntry(GetTablePath(renameTestBucket, "ns", "newt"), "leftover"), + "data under the reused name path must survive") + marker := fs.getEntry(GetNamespacePath(renameTestBucket, "ns"), "newt") + require.NotNil(t, marker) + for _, key := range markerKeys { + _, present := marker.Extended[key] + assert.Falsef(t, present, "catalog attribute %s must be cleared", key) + } +} + +// A table whose MetadataLocation resolves to an ancestor of its own name path +// (here the namespace root, e.g. from corrupt metadata) must not be deleted: a +// recursive purge of that data path would wipe sibling tables. The delete is +// refused and the namespace's other tables survive. +func TestDeleteTableRefusesAncestorDataPath(t *testing.T) { + fs, m := startRenameManager(t) + + badMeta, _ := json.Marshal(tableMetadataInternal{ + Name: "badt", + Namespace: "ns", + Format: "ICEBERG", + OwnerAccountID: DefaultAccountID, + MetadataLocation: "s3://" + renameTestBucket + "/ns/metadata/v1.metadata.json", + }) + fs.putEntry(GetNamespacePath(renameTestBucket, "ns"), "badt", map[string][]byte{ExtendedKeyMetadata: badMeta}) + + require.Error(t, runDeleteTable(t, m, fs, "ns", "badt")) + + // The sibling table seeded by startRenameManager and its data must survive. + assert.NotNil(t, fs.getEntry(GetNamespacePath(renameTestBucket, "ns"), "t"), + "sibling table marker must survive a refused delete") + assert.NotNil(t, fs.getEntry(GetTablePath(renameTestBucket, "ns", "t"), "data"), + "sibling table data must survive a refused delete") +} + +// A normal colocated table (data under its own name path) is removed wholesale. +func TestDeleteTableColocatedRemovesData(t *testing.T) { + fs, m := startRenameManager(t) + + require.NoError(t, runDeleteTable(t, m, fs, "ns", "t")) + + assert.Nil(t, fs.getEntry(GetNamespacePath(renameTestBucket, "ns"), "t"), "colocated table entry must be deleted") + assert.Nil(t, fs.getEntry(GetTablePath(renameTestBucket, "ns", "t"), "metadata"), "colocated table data must be deleted") +} diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index e2144a9b2e..1e23499375 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -1182,18 +1182,28 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque // Delete the table err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { - if err := h.deleteDirectory(r.Context(), client, tablePath); err != nil { - return err - } - // A renamed table keeps its data at the original location, so the catalog - // path no longer holds it; purge the data directory too when it differs. dataPath := tableDataDirFromMetadataLocation(metadata.MetadataLocation) if dataPath != "" && dataPath != tablePath && strings.HasPrefix(dataPath+"/", GetTableBucketPath(bucketName)+"/") { + // Refuse to purge a data path that is an ancestor of the table's own + // name path (e.g. corrupt metadata resolving to the bucket or + // namespace root): the bucket-scope check above still admits the + // bucket root, and a recursive delete there would take out unrelated + // tables. + if strings.HasPrefix(tablePath+"/", dataPath+"/") { + return fmt.Errorf("refusing to delete table %s: data path %q is an ancestor of catalog path %q", tableName, dataPath, tablePath) + } + // Decoupled table (renamed, or created over a leftover): its data + // lives elsewhere. Purge the data, then clear the catalog marker + // without deleting the name path -- it may still hold another + // table's data that was left when this name was reused. if err := h.deleteDirectory(r.Context(), client, dataPath); err != nil { return err } + return h.removeExtendedAttributes(r.Context(), client, tablePath, + ExtendedKeyMetadata, ExtendedKeyMetadataVersion, ExtendedKeyPolicy, ExtendedKeyTags, ExtendedKeyEntryType) } - return nil + // Colocated table: the name path holds the data. + return h.deleteDirectory(r.Context(), client, tablePath) }) if err != nil {