From 7298b4ed37c887dcb43ba9ccbb12e386beb6cc5f Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 2 Nov 2026 09:55:09 -0800 Subject: [PATCH] fix(s3tables,s3vectors): bind DeleteNamespace and the vector bucket cleanup to the authorized records (#12331) * s3tables: bind DeleteNamespace to the authorized namespace The delete claimed whatever finished namespace held the name at claim time, so one deleted and created again by another owner after the authorization read was removed under the first one's authorization. The claim now requires the creation time that was authorized. Co-Authored-By: Claude Opus 5.5 * s3vectors: bind DeleteVectorBucket's namespace cleanup to the authorized bucket After the bound DeleteTableBucket reported the bucket not empty, the index namespace cleanup ran unbound, so a bucket deleted and created again in between lost its empty index namespace under the first bucket's authorization. DeleteNamespace now honors the bucket binding, checking the bucket after reading the namespace, and the cleanup carries it. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- .../s3tables/handler_bind_authorized_test.go | 25 ++++++++++++++ weed/s3api/s3tables/handler_namespace.go | 14 +++++++- weed/s3api/s3tables/manager.go | 16 ++++++--- weed/s3api/s3vectors/bind_bucket_test.go | 34 +++++++++++++++++++ weed/s3api/s3vectors/handlers.go | 2 +- 5 files changed, 84 insertions(+), 7 deletions(-) diff --git a/weed/s3api/s3tables/handler_bind_authorized_test.go b/weed/s3api/s3tables/handler_bind_authorized_test.go index ce48bdf3e1..0544ec5406 100644 --- a/weed/s3api/s3tables/handler_bind_authorized_test.go +++ b/weed/s3api/s3tables/handler_bind_authorized_test.go @@ -583,3 +583,28 @@ func TestUpdateNamespaceSparesARecreatedNamespace(t *testing.T) { assert.Equal(t, http.StatusConflict, code, "%+v", resp) assert.Equal(t, recreated, fs.Get(bucketPath, "ns").Extended, "the new namespace is untouched") } + +// DeleteNamespace removes only the namespace it was authorized against: one +// deleted and created again before the claim reads as the first one gone. +func TestDeleteNamespaceSparesARecreatedNamespace(t *testing.T) { + fs, m := startEmptyBucket(t) + requireOK(t, m, fs, "CreateNamespace", createNamespaceNsRequest(t)) + bucketPath := GetTableBucketPath(renameTestBucket) + recreated := recreatedNamespace(t, "ns") + fs.OnUpdate = func(dir, name string) { + if dir != bucketPath || name != "ns" { + return + } + fs.OnUpdate = nil + fs.Put(bucketPath, "ns", recreated) + } + + code, resp := runHandler(t, m, fs.Client, "DeleteNamespace", &DeleteNamespaceRequest{ + TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns"}, + }) + + assert.Equal(t, http.StatusNotFound, code, "%+v", resp) + require.NotNil(t, fs.Get(bucketPath, "ns"), "the new namespace survives") + assert.Equal(t, recreated, fs.Get(bucketPath, "ns").Extended, "the new namespace is untouched") +} diff --git a/weed/s3api/s3tables/handler_namespace.go b/weed/s3api/s3tables/handler_namespace.go index 4327fc2f56..f693ce833e 100644 --- a/weed/s3api/s3tables/handler_namespace.go +++ b/weed/s3api/s3tables/handler_namespace.go @@ -757,6 +757,17 @@ func (h *S3TablesHandler) handleDeleteNamespace(w http.ResponseWriter, r *http.R if err := json.Unmarshal(data, &metadata); err != nil { return fmt.Errorf("failed to unmarshal metadata: %w", err) } + // Checked after the namespace read: a bucket created again holds only + // namespaces of its own, and the claim below is bound to this one. + if createdAt, bound := boundTableBucket(r.Context()); bound { + bucket, err := h.lookupEntry(r.Context(), client, bucketPath) + if err != nil { + return err + } + if !isTableBucketCreatedAt(createdAt)(bucket) { + return filer_pb.ErrNotFound + } + } // Fetch bucket policy if it exists policyData, err := h.getExtendedAttribute(r.Context(), client, bucketPath, ExtendedKeyPolicy) @@ -802,11 +813,12 @@ func (h *S3TablesHandler) handleDeleteNamespace(w http.ResponseWriter, r *http.R // A namespace whose create has not confirmed it is absent, and claiming it // would replace the create's mark: a refused delete then lifts the claim and // leaves the namespace looking confirmed before its create checked the bucket. + // One created again since it was authorized is absent too. var claim string var claimed map[string][]byte err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { var claimErr error - claim, claimed, claimErr = h.claimForDelete(r.Context(), client, namespacePath, isFinishedNamespaceEntry) + claim, claimed, claimErr = h.claimForDelete(r.Context(), client, namespacePath, isNamespaceCreatedAt(metadata.CreatedAt)) return claimErr }) if err != nil { diff --git a/weed/s3api/s3tables/manager.go b/weed/s3api/s3tables/manager.go index 0334f80fec..e3be966476 100644 --- a/weed/s3api/s3tables/manager.go +++ b/weed/s3api/s3tables/manager.go @@ -80,18 +80,24 @@ func (m *Manager) PolicyContext(r *http.Request, bucket string) *PolicyContext { type tableBucketCreatedAtKey struct{} // WithTableBucketCreatedAt binds a DeleteTableBucket, CreateNamespace, -// CreateTable or DeleteTable run through Execute to the bucket created at -// createdAt, so a -// caller that authorized the bucket itself never writes into one created -// again under its name. In-process only: no wire request can carry it. +// DeleteNamespace, CreateTable or DeleteTable run through Execute to the +// bucket created at createdAt, so a caller that authorized the bucket itself +// never writes into one created again under its name. In-process only: no +// wire request can carry it. func WithTableBucketCreatedAt(ctx context.Context, createdAt time.Time) context.Context { return context.WithValue(ctx, tableBucketCreatedAtKey{}, createdAt) } +// boundTableBucket returns the bucket creation time ctx binds the request to. +func boundTableBucket(ctx context.Context) (time.Time, bool) { + createdAt, ok := ctx.Value(tableBucketCreatedAtKey{}).(time.Time) + return createdAt, ok +} + // otherTableBucket reports whether ctx binds the request to a bucket other // than the one created at createdAt. func otherTableBucket(ctx context.Context, createdAt time.Time) bool { - want, ok := ctx.Value(tableBucketCreatedAtKey{}).(time.Time) + want, ok := boundTableBucket(ctx) return ok && !createdAt.Equal(want) } diff --git a/weed/s3api/s3vectors/bind_bucket_test.go b/weed/s3api/s3vectors/bind_bucket_test.go index 3a73326874..b70a0cba36 100644 --- a/weed/s3api/s3vectors/bind_bucket_test.go +++ b/weed/s3api/s3vectors/bind_bucket_test.go @@ -207,3 +207,37 @@ func TestIndexWritesKeepToAuthorizedBucket(t *testing.T) { } }) } + +// DeleteVectorBucket drops the empty index namespace only from the bucket it +// authorized: a bucket created again before that cleanup keeps its own. +func TestDeleteVectorBucketCleanupKeepsReplacingBucket(t *testing.T) { + fs := s3tablestest.Start(t) + s := NewServer(s3tables.NewManagerClient(fs.Client), allowAllAuth{}, newFakeVectors(0)) + if w := postJSONAs(t, "", s.handleCreateVectorBucket, &CreateVectorBucketRequest{VectorBucketName: "vec-bucket"}); w.Code != http.StatusOK { + t.Fatalf("create bucket: %d %s", w.Code, w.Body.String()) + } + if w := postJSONAs(t, "", s.handleCreateIndex, &CreateIndexRequest{ + VectorBucketName: "vec-bucket", IndexName: "idx", DataType: "float32", Dimension: 3, DistanceMetric: "euclidean", + }); w.Code != http.StatusOK { + t.Fatalf("create index: %d %s", w.Code, w.Body.String()) + } + if w := postJSONAs(t, "", s.handleDeleteIndex, &DeleteIndexRequest{VectorBucketName: "vec-bucket", IndexName: "idx"}); w.Code != http.StatusOK { + t.Fatalf("delete index: %d %s", w.Code, w.Body.String()) + } + + bucketPath := s3tables.GetTableBucketPath("vec-bucket") + fs.OnLookup = func(d, n string) { + if d == bucketPath && n == indexNamespace { + fs.OnLookup = nil + replaceBucket(t, fs, "vec-bucket") + } + } + w := postJSONAs(t, "", s.handleDeleteVectorBucket, &DeleteVectorBucketRequest{VectorBucketName: "vec-bucket"}) + fs.OnLookup = nil + if w.Code == http.StatusOK { + t.Fatalf("delete over a replaced bucket succeeded: %s", w.Body.String()) + } + if fs.Get(bucketPath, indexNamespace) == nil { + t.Fatal("DeleteVectorBucket removed the index namespace of the bucket that replaced the authorized one") + } +} diff --git a/weed/s3api/s3vectors/handlers.go b/weed/s3api/s3vectors/handlers.go index af762d7097..9f9be68b51 100644 --- a/weed/s3api/s3vectors/handlers.go +++ b/weed/s3api/s3vectors/handlers.go @@ -421,7 +421,7 @@ func (s *Server) handleDeleteVectorBucket(w http.ResponseWriter, r *http.Request // holds no indexes; drop it under the delete's own authorization and // retry once. A namespace still holding indexes keeps the conflict. var nsResp map[string]interface{} - nsErr := s.internalCatalog(context.WithoutCancel(r.Context()), "DeleteNamespace", + nsErr := s.internalCatalog(context.WithoutCancel(ctx), "DeleteNamespace", &s3tables.DeleteNamespaceRequest{ TableBucketARN: s.s3tablesArn(arns.bucket), Namespace: []string{indexNamespace},