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