From 7d8b72c2b4aba705f68af922628a8bed2d1f2b61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20L=E1=BB=99c=20Ph=C3=BAc?= <112325531+omninguyen@users.noreply.github.com> Date: Sat, 31 Oct 2026 09:53:57 +0700 Subject: [PATCH] fix(s3tables): answer 404 for a table name that was renamed away (#12258) * s3tables: answer 404 for a table name that was renamed away A rename leaves the source directory in place to hold the data under it, with its catalog attributes cleared. UpdateTable, DeleteTable, the table policy calls and the table maintenance calls read the metadata attribute of that directory and reported its absence as an internal error, so they answered 500, and GetTablePolicy answered NoSuchPolicy. An absent metadata attribute now reads as a missing entry. tableMetadata reports it as both filer_pb.ErrNotFound and the attribute error, so callers that test for either keep working. The policy and maintenance handlers read the attribute through the new readTableMetadataAttr, which reports filer_pb.ErrNotFound, so each call answers NoSuchTable. * s3tables: a view record without its metadata reads absent too * s3tables: refuse a catalog write that lands on a metadata-less record --------- Co-authored-by: user.email --- weed/s3api/s3tables/filer_ops.go | 13 ++ weed/s3api/s3tables/handler_maintenance.go | 2 +- weed/s3api/s3tables/handler_policy.go | 6 +- .../s3tables/handler_renamed_source_test.go | 180 ++++++++++++++++++ weed/s3api/s3tables/handler_table.go | 7 +- weed/s3api/s3tables/handler_view.go | 6 +- 6 files changed, 205 insertions(+), 9 deletions(-) create mode 100644 weed/s3api/s3tables/handler_renamed_source_test.go diff --git a/weed/s3api/s3tables/filer_ops.go b/weed/s3api/s3tables/filer_ops.go index ecc4e1410d..4434df6704 100644 --- a/weed/s3api/s3tables/filer_ops.go +++ b/weed/s3api/s3tables/filer_ops.go @@ -378,6 +378,16 @@ func (h *S3TablesHandler) loadNamespaceMetadata(ctx context.Context, filerClient return &metadata, nil } +// readTableMetadataAttr returns the table-metadata xattr at tablePath. A +// directory without it is not a table, so its absence reads as a missing entry. +func (h *S3TablesHandler) readTableMetadataAttr(ctx context.Context, client filer_pb.SeaweedFilerClient, tablePath string) ([]byte, error) { + data, err := h.getExtendedAttribute(ctx, client, tablePath, ExtendedKeyMetadata) + if errors.Is(err, ErrAttributeNotFound) { + return nil, fmt.Errorf("%w: %s", filer_pb.ErrNotFound, tablePath) + } + return data, err +} + // lookupEntry returns the filer entry at the given path. func (h *S3TablesHandler) lookupEntry(ctx context.Context, client filer_pb.SeaweedFilerClient, path string) (*filer_pb.Entry, error) { dir, name := splitPath(path) @@ -551,6 +561,9 @@ func (h *S3TablesHandler) mutateCatalogRecord( ) error { return h.mutateEntryExtended(ctx, client, path, func(extended map[string][]byte) error { if _, claimed := extended[ExtendedKeyDeleting]; !claimed { + if _, ok := extended[ExtendedKeyMetadata]; !ok { + return filer_pb.ErrNotFound + } return mutate(extended) } if isUnfinishedCreate(extended) { diff --git a/weed/s3api/s3tables/handler_maintenance.go b/weed/s3api/s3tables/handler_maintenance.go index 97890d3742..02a2764a25 100644 --- a/weed/s3api/s3tables/handler_maintenance.go +++ b/weed/s3api/s3tables/handler_maintenance.go @@ -371,7 +371,7 @@ func (h *S3TablesHandler) authorizeMaintenanceTable(r *http.Request, filerClient var bucketPolicy, tablePolicy string var bucketTags, tableTags map[string]string err := filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { - data, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyMetadata) + data, err := h.readTableMetadataAttr(r.Context(), client, tablePath) if err != nil { return err } diff --git a/weed/s3api/s3tables/handler_policy.go b/weed/s3api/s3tables/handler_policy.go index 947fa1fef1..f6c89f6061 100644 --- a/weed/s3api/s3tables/handler_policy.go +++ b/weed/s3api/s3tables/handler_policy.go @@ -349,7 +349,7 @@ func (h *S3TablesHandler) handlePutTablePolicy(w http.ResponseWriter, r *http.Re var bucketPolicy, tablePolicy string var bucketTags, tableTags map[string]string err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { - data, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyMetadata) + data, err := h.readTableMetadataAttr(r.Context(), client, tablePath) if err != nil { return err } @@ -464,7 +464,7 @@ func (h *S3TablesHandler) handleGetTablePolicy(w http.ResponseWriter, r *http.Re err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { // Get metadata for ownership check - data, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyMetadata) + data, err := h.readTableMetadataAttr(r.Context(), client, tablePath) if err != nil { return err } @@ -568,7 +568,7 @@ func (h *S3TablesHandler) handleDeleteTablePolicy(w http.ResponseWriter, r *http var bucketPolicy, tablePolicy string var bucketTags, tableTags map[string]string err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { - data, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyMetadata) + data, err := h.readTableMetadataAttr(r.Context(), client, tablePath) if err != nil { return err } diff --git a/weed/s3api/s3tables/handler_renamed_source_test.go b/weed/s3api/s3tables/handler_renamed_source_test.go new file mode 100644 index 0000000000..c4f81121e2 --- /dev/null +++ b/weed/s3api/s3tables/handler_renamed_source_test.go @@ -0,0 +1,180 @@ +package s3tables + +import ( + "context" + "net/http" + "path" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// extendedOf copies the attributes held on an entry, so a later write to the +// entry shows up as a difference. +func extendedOf(t *testing.T, fs *s3tablestest.MemFiler, dir, name string) map[string][]byte { + t.Helper() + entry := fs.Get(dir, name) + require.NotNil(t, entry) + copied := make(map[string][]byte, len(entry.Extended)) + for key, value := range entry.Extended { + copied[key] = append([]byte(nil), value...) + } + return copied +} + +// A rename leaves the source directory in place to hold the data under it, with +// its catalog attributes cleared. That name is no longer a table, so every table +// operation on it answers NoSuchTable and writes nothing to the directory. +func TestRenamedAwayNameIsNoSuchTable(t *testing.T) { + fs, m := startRenameManager(t) + requireOK(t, m, fs, "CreateNamespace", &CreateNamespaceRequest{ + TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns2"}, + }) + requireOK(t, m, fs, "RenameTable", &RenameTableRequest{ + TableBucketARN: mustBucketARN(t), + SourceNamespace: []string{"ns"}, + SourceName: "t", + DestNamespace: []string{"ns2"}, + DestName: "t", + }) + nsPath := GetNamespacePath(renameTestBucket, "ns") + before := extendedOf(t, fs, nsPath, "t") + + bucketARN := mustBucketARN(t) + ops := []struct { + operation string + input interface{} + }{ + {"UpdateTable", &UpdateTableRequest{ + TableBucketARN: bucketARN, + Namespace: []string{"ns"}, + Name: "t", + MetadataVersion: 4, + MetadataLocation: "s3://" + renameTestBucket + "/ns/t/metadata/v4.metadata.json", + }}, + {"DeleteTable", deleteTableRequest(t, "t")}, + {"PutTablePolicy", &PutTablePolicyRequest{ + TableBucketARN: bucketARN, + Namespace: []string{"ns"}, + Name: "t", + ResourcePolicy: `{"Statement":[{"Effect":"Allow","Principal":"writer","Action":"s3tables:*","Resource":"*"}]}`, + }}, + {"GetTablePolicy", &GetTablePolicyRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "t"}}, + {"DeleteTablePolicy", &DeleteTablePolicyRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "t"}}, + {"PutTableMaintenanceConfiguration", &PutTableMaintenanceConfigurationRequest{ + TableBucketARN: bucketARN, + Namespace: []string{"ns"}, + Name: "t", + Type: MaintenanceTypeIcebergCompaction, + Value: &MaintenanceConfigurationValue{Status: MaintenanceStatusEnabled}, + }}, + {"GetTableMaintenanceConfiguration", &GetTableMaintenanceConfigurationRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "t"}}, + {"GetTableMaintenanceJobStatus", &GetTableMaintenanceJobStatusRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "t"}}, + } + for _, op := range ops { + t.Run(op.operation, func(t *testing.T) { + status, resp := runHandler(t, m, fs.Client, op.operation, op.input) + assert.Equal(t, http.StatusNotFound, status, "%+v", resp) + assert.Equal(t, ErrCodeNoSuchTable, resp.Type, "%+v", resp) + }) + } + + assert.Equal(t, before, extendedOf(t, fs, nsPath, "t"), "the renamed-away directory must not be written to") + status, resp := runHandler(t, m, fs.Client, "GetTable", &GetTableRequest{ + TableBucketARN: bucketARN, + Namespace: []string{"ns2"}, + Name: "t", + }) + assert.Equal(t, http.StatusOK, status, "the renamed table must still resolve: %+v", resp) +} + +// Views share the renamer, so a renamed-away view name answers NoSuchView the +// same way, and the leftover view record still takes no writes. +func TestRenamedAwayViewNameIsNoSuchView(t *testing.T) { + fs, m := startRenameManager(t) + seedView(t, fs, "v") + requireOK(t, m, fs, "RenameView", &RenameTableRequest{ + TableBucketARN: mustBucketARN(t), + SourceNamespace: []string{"ns"}, + SourceName: "v", + DestNamespace: []string{"ns"}, + DestName: "v2", + }) + nsPath := GetNamespacePath(renameTestBucket, "ns") + before := extendedOf(t, fs, nsPath, "v") + + bucketARN := mustBucketARN(t) + ops := []struct { + operation string + input interface{} + }{ + {"GetView", &GetViewRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "v"}}, + {"UpdateView", &UpdateViewRequest{ + TableBucketARN: bucketARN, + Namespace: []string{"ns"}, + Name: "v", + MetadataLocation: "s3://" + renameTestBucket + "/ns/v/metadata/v2.metadata.json", + }}, + {"DeleteView", &DeleteViewRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "v"}}, + } + for _, op := range ops { + t.Run(op.operation, func(t *testing.T) { + status, resp := runHandler(t, m, fs.Client, op.operation, op.input) + assert.Equal(t, http.StatusNotFound, status, "%+v", resp) + assert.Equal(t, ErrCodeNoSuchView, resp.Type, "%+v", resp) + }) + } + + assert.Equal(t, before, extendedOf(t, fs, nsPath, "v"), "the renamed-away directory must not be written to") +} + +// A record that keeps its view marker but lost the metadata — the state a +// rename leaves behind while its clear is in flight — is not a view to its +// readers either. +func TestViewRecordWithoutMetadataIsNoSuchView(t *testing.T) { + fs, m := startRenameManager(t) + nsPath := GetNamespacePath(renameTestBucket, "ns") + fs.Put(nsPath, "half", map[string][]byte{ExtendedKeyEntryType: []byte(EntryTypeView)}) + + bucketARN := mustBucketARN(t) + ops := []struct { + operation string + input interface{} + }{ + {"GetView", &GetViewRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "half"}}, + {"UpdateView", &UpdateViewRequest{ + TableBucketARN: bucketARN, + Namespace: []string{"ns"}, + Name: "half", + MetadataLocation: "s3://" + renameTestBucket + "/ns/half/metadata/v2.metadata.json", + }}, + {"DeleteView", &DeleteViewRequest{TableBucketARN: bucketARN, Namespace: []string{"ns"}, Name: "half"}}, + } + for _, op := range ops { + t.Run(op.operation, func(t *testing.T) { + status, resp := runHandler(t, m, fs.Client, op.operation, op.input) + assert.Equal(t, http.StatusNotFound, status, "%+v", resp) + assert.Equal(t, ErrCodeNoSuchView, resp.Type, "%+v", resp) + }) + } +} + +// A write landing after the record lost its metadata — a rename cleared it +// between the handler's read and the mutation — must not write the leftover +// directory. +func TestMutateCatalogRecordRefusesClearedRecord(t *testing.T) { + fs, m := startRenameManager(t) + nsPath := GetNamespacePath(renameTestBucket, "ns") + fs.Put(nsPath, "gone", map[string][]byte{}) + + err := m.handler.mutateCatalogRecord(context.Background(), fs.Client, path.Join(nsPath, "gone"), func(extended map[string][]byte) error { + extended[ExtendedKeyPolicy] = []byte("x") + return nil + }) + assert.ErrorIs(t, err, filer_pb.ErrNotFound) + assert.Empty(t, fs.Get(nsPath, "gone").Extended) +} diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index 97859fefdc..8e6ce93c96 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -1298,14 +1298,17 @@ func (h *S3TablesHandler) readTableMetadata(ctx context.Context, client filer_pb // tableMetadata extracts the table-metadata xattr from an entry. A view stored // at the same name is reported absent, the way GetTable and the renamer's // entry-type check already treat kind mismatches, and so is a record its -// create has not finished writing. +// create has not finished writing. A directory without the record is no table +// either, and reports the same absence. func tableMetadata(entry *filer_pb.Entry) ([]byte, error) { if EntryType(entry.Extended) == EntryTypeView || isUnfinishedCreate(entry.Extended) { return nil, filer_pb.ErrNotFound } data, ok := entry.Extended[ExtendedKeyMetadata] if !ok { - return nil, fmt.Errorf("%w: %s", ErrAttributeNotFound, ExtendedKeyMetadata) + // A directory without the record is not a table (e.g. a renamed-away + // source), so it reads as a missing entry. + return nil, fmt.Errorf("%w: %w: %s", filer_pb.ErrNotFound, ErrAttributeNotFound, ExtendedKeyMetadata) } return data, nil } diff --git a/weed/s3api/s3tables/handler_view.go b/weed/s3api/s3tables/handler_view.go index 2c2ef5591e..cb63a3b97c 100644 --- a/weed/s3api/s3tables/handler_view.go +++ b/weed/s3api/s3tables/handler_view.go @@ -224,7 +224,7 @@ func (h *S3TablesHandler) handleGetView(w http.ResponseWriter, r *http.Request, } data, ok := entry.Extended[ExtendedKeyMetadata] if !ok { - return fmt.Errorf("%w: %s", ErrAttributeNotFound, ExtendedKeyMetadata) + return fmt.Errorf("%w: %w: %s", filer_pb.ErrNotFound, ErrAttributeNotFound, ExtendedKeyMetadata) } return json.Unmarshal(data, &metadata) }) @@ -470,7 +470,7 @@ func (h *S3TablesHandler) authorizeUpdateView(w http.ResponseWriter, r *http.Req } data, ok := entry.Extended[ExtendedKeyMetadata] if !ok { - return fmt.Errorf("%w: %s", ErrAttributeNotFound, ExtendedKeyMetadata) + return fmt.Errorf("%w: %w: %s", filer_pb.ErrNotFound, ErrAttributeNotFound, ExtendedKeyMetadata) } return json.Unmarshal(data, &metadata) }) @@ -594,7 +594,7 @@ func completedViewMetadata(entry *filer_pb.Entry, token string) (tableMetadataIn } data, ok := entry.Extended[ExtendedKeyMetadata] if !ok { - return metadata, fmt.Errorf("%w: %s", ErrAttributeNotFound, ExtendedKeyMetadata) + return metadata, fmt.Errorf("%w: %w: %s", filer_pb.ErrNotFound, ErrAttributeNotFound, ExtendedKeyMetadata) } if err := json.Unmarshal(data, &metadata); err != nil { return metadata, err