s3vectors: support TagResource, UntagResource and ListTagsForResource (#12335)

* s3vectors: tag vector buckets and indexes TagResource, UntagResource and ListTagsForResource answer on /tags/{resourceArn} (POST, DELETE, GET) and by name. Each authorizes its own s3vectors action on the bucket or index ARN, then reads or writes the catalog record's tags. A resource holds at most 50 tags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3vectors: apply the tags passed to CreateVectorBucket and CreateIndex The tags field was accepted and dropped. It is now stored on the new bucket or index, and creating with tags needs s3vectors:TagResource in addition to the create action. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test/s3vectors: cover resource tagging end to end Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3tables: keep tag operations to a bound table bucket TagResource, UntagResource and ListTagsForResource run through Execute with WithTableBucketCreatedAt now answer not found when the bucket name holds another bucket, as the bound creates and deletes already do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3vectors: bind tagging to the authorized bucket and to vector indexes The catalog tag calls carry the creation time of the bucket authorizeVector approved, so a bucket created again under the name is not touched. An index ARN must name a Lance table: another table in the vector namespace of a format-less bucket is refused, as DeleteIndex does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3vectors: match a TagResource grant scoped to the bucket being created Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3tables: merge tag writes into the tags the record holds at the write TagResource and UntagResource merged into the tags they read before the compare-and-swap and wrote that map on every attempt, so a tag write that landed in between was undone. The merge now runs inside the mutation. WithMaxTags lets an in-process caller cap the merged set there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3vectors: check the 50-tag limit inside the catalog's tag write Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3tables: check a tag operation's bound bucket after its reads Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3vectors: validate each tag key UntagResource removes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3vectors: validate untag keys one at a time UntagResource sized a map by the number of submitted tag keys before validating them, so a large body of repeated keys allocated far more than the request itself, before authorization ran. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3vectors: authorize UntagResource before validating its keys Validating a near-32 MiB body of tag keys costs about 13x its JSON decode, so an unauthorized caller could make the gateway spend that work on a request it then refuses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-11-03 18:47:41 +01:00 · 2026-11-02 10:33:26 -08:00
1 parent 690aabda0e
commit ec968aa016
@@ -0,0 +1,96 @@
+package s3tables
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Tag operations bound to a table bucket answer not found once its name
+// holds another bucket, for the bucket and for a table in it, and leave the
+// tags as they were.
+func TestTagOperationsKeepToTheBoundBucket(t *testing.T) {
+	fs, m := startRenameManager(t)
+	client := NewManagerClient(fs.Client)
+	tableARN, err := BuildTableARN(DefaultRegion, DefaultAccountID, renameTestBucket, "ns", "t")
+	require.NoError(t, err)
+	var bucket GetTableBucketResponse
+	require.NoError(t, m.Execute(context.Background(), client, "GetTableBucket",
+		&GetTableBucketRequest{TableBucketARN: mustBucketARN(t)}, &bucket, ""))
+	bound := WithTableBucketCreatedAt(context.Background(), bucket.CreatedAt)
+	stale := WithTableBucketCreatedAt(context.Background(), bucket.CreatedAt.Add(-time.Hour))
+
+	for _, arn := range []string{mustBucketARN(t), tableARN} {
+		require.NoError(t, m.Execute(bound, client, "TagResource",
+			&TagResourceRequest{ResourceARN: arn, Tags: map[string]string{"k": "v"}}, nil, ""), arn)
+		for operation, req := range map[string]interface{}{
+			"TagResource":         &TagResourceRequest{ResourceARN: arn, Tags: map[string]string{"k": "other"}},
+			"UntagResource":       &UntagResourceRequest{ResourceARN: arn, TagKeys: []string{"k"}},
+			"ListTagsForResource": &ListTagsForResourceRequest{ResourceARN: arn},
+		} {
+			err := m.Execute(stale, client, operation, req, nil, "")
+			var apiErr *S3TablesError
+			require.ErrorAs(t, err, &apiErr, "%s on %s", operation, arn)
+			assert.Contains(t, []string{ErrCodeNoSuchBucket, ErrCodeNoSuchTable}, apiErr.Type, "%s on %s", operation, arn)
+		}
+		var tags ListTagsForResourceResponse
+		require.NoError(t, m.Execute(bound, client, "ListTagsForResource",
+			&ListTagsForResourceRequest{ResourceARN: arn}, &tags, ""), arn)
+		assert.Equal(t, map[string]string{"k": "v"}, tags.Tags, arn)
+	}
+}
+
+// A tag write merges into the tags the record holds when it is written, so a
+// tag write that lands between its read and its write is kept.
+func TestTagWritesKeepAConcurrentTagWrite(t *testing.T) {
+	for _, tc := range []struct {
+		operation string
+		input     func(t *testing.T) interface{}
+		want      map[string]string
+	}{
+		{"TagResource", func(t *testing.T) interface{} {
+			return &TagResourceRequest{ResourceARN: mustBucketARN(t), Tags: map[string]string{"k": "v"}}
+		}, map[string]string{"keep": "1", "other": "1", "k": "v"}},
+		{"UntagResource", func(t *testing.T) interface{} {
+			return &UntagResourceRequest{ResourceARN: mustBucketARN(t), TagKeys: []string{"keep"}}
+		}, map[string]string{"other": "1"}},
+	} {
+		t.Run(tc.operation, func(t *testing.T) {
+			fs, m := startEmptyBucket(t)
+			extended := extendedOf(t, fs, TablesPath, renameTestBucket)
+			extended[ExtendedKeyTags] = []byte(`{"keep":"1"}`)
+			fs.Put(TablesPath, renameTestBucket, extended)
+			fs.BeforeUpdate = func() {
+				current := extendedOf(t, fs, TablesPath, renameTestBucket)
+				current[ExtendedKeyTags] = []byte(`{"keep":"1","other":"1"}`)
+				fs.Put(TablesPath, renameTestBucket, current)
+			}
+			code, resp := runHandler(t, m, fs.Client, tc.operation, tc.input(t))
+			require.Equal(t, http.StatusOK, code, resp.Message)
+			var got map[string]string
+			require.NoError(t, json.Unmarshal(extendedOf(t, fs, TablesPath, renameTestBucket)[ExtendedKeyTags], &got))
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+func TestTagResourceKeepsToItsLimit(t *testing.T) {
+	fs, m := startEmptyBucket(t)
+	client := NewManagerClient(fs.Client)
+	ctx := WithMaxTags(context.Background(), 2)
+	tag := func(tags map[string]string) error {
+		return m.Execute(ctx, client, "TagResource", &TagResourceRequest{ResourceARN: mustBucketARN(t), Tags: tags}, nil, "")
+	}
+	require.NoError(t, tag(map[string]string{"a": "1", "b": "1"}))
+	require.NoError(t, tag(map[string]string{"a": "2"}))
+	err := tag(map[string]string{"c": "1"})
+	var apiErr *S3TablesError
+	require.ErrorAs(t, err, &apiErr)
+	assert.Equal(t, ErrCodeInvalidRequest, apiErr.Type)
+	assert.JSONEq(t, `{"a":"2","b":"1"}`, string(extendedOf(t, fs, TablesPath, renameTestBucket)[ExtendedKeyTags]))
+}
@@ -2,6 +2,7 @@ package s3tables

 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -43,6 +44,65 @@ func (h *S3TablesHandler) extractResourceOwnerAndBucket(
 	return ownerAccountID, bucketName, createdAt, nil
 }

+type maxTagsKey struct{}
+
+var errTooManyTags = errors.New("too many tags")
+
+// WithMaxTags caps how many tags a TagResource run through Execute may leave
+// on its resource. In-process only, like WithTableBucketCreatedAt.
+func WithMaxTags(ctx context.Context, n int) context.Context {
+	return context.WithValue(ctx, maxTagsKey{}, n)
+}
+
+// updateTags applies set and remove to raw, the tags the record holds at the
+// write, so concurrent tag writes do not undo each other.
+func updateTags(raw []byte, set map[string]string, remove []string) (map[string]string, error) {
+	tags := make(map[string]string)
+	if len(raw) > 0 {
+		if err := json.Unmarshal(raw, &tags); err != nil {
+			return nil, err
+		}
+	}
+	for k, v := range set {
+		tags[k] = v
+	}
+	for _, k := range remove {
+		delete(tags, k)
+	}
+	return tags, nil
+}
+
+func writeTags(extended map[string][]byte, key string, tags map[string]string) error {
+	data, err := json.Marshal(tags)
+	if err != nil {
+		return err
+	}
+	extended[key] = data
+	return nil
+}
+
+// requireBoundTableBucket answers not found when ctx binds the request to a
+// table bucket other than the one now named bucketName.
+func (h *S3TablesHandler) requireBoundTableBucket(ctx context.Context, client filer_pb.SeaweedFilerClient, bucketName string) error {
+	if _, bound := ctx.Value(tableBucketCreatedAtKey{}).(time.Time); !bound {
+		return nil
+	}
+	data, err := h.getExtendedAttribute(ctx, client, GetTableBucketPath(bucketName), ExtendedKeyMetadata)
+	if errors.Is(err, ErrAttributeNotFound) {
+		return filer_pb.ErrNotFound
+	} else if err != nil {
+		return err
+	}
+	var meta tableBucketMetadata
+	if err := json.Unmarshal(data, &meta); err != nil {
+		return err
+	}
+	if otherTableBucket(ctx, meta.CreatedAt) {
+		return filer_pb.ErrNotFound
+	}
+	return nil
+}
+
 // handlePutTableBucketPolicy puts a policy on a table bucket
 func (h *S3TablesHandler) handlePutTableBucketPolicy(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error {

@@ -751,6 +811,10 @@ func (h *S3TablesHandler) handleTagResource(w http.ResponseWriter, r *http.Reque
 			return err
 		}

+		// Checked after the reads, so they all came from the bound bucket.
+		if err := h.requireBoundTableBucket(r.Context(), client, bucketName); err != nil {
+			return err
+		}
 		var resourceARN string
 		if rType == ResourceTypeTable {
 			resourceARN = h.generateTableARN(ownerAccountID, bucketName, tableNamespace+"/"+tableName)
@@ -794,28 +858,24 @@ func (h *S3TablesHandler) handleTagResource(w http.ResponseWriter, r *http.Reque
 		return err
 	}

-	// Merge new tags
-	for k, v := range req.Tags {
-		existingTags[k] = v
-	}
-
-	// Write merged tags
-	tagsBytes, err := json.Marshal(existingTags)
-	if err != nil {
-		h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, "failed to marshal tags")
-		return fmt.Errorf("failed to marshal tags: %w", err)
-	}
 	sameResource := requireTableCreatedAt
 	if rType != ResourceTypeTable {
 		sameResource = requireTableBucketCreatedAt
 	}
+	maxTags, limited := r.Context().Value(maxTagsKey{}).(int)
 	err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
 		return h.mutateCatalogRecord(r.Context(), client, resourcePath, func(extended map[string][]byte) error {
 			if err := sameResource(extended, resourcePath, createdAt); err != nil {
 				return err
 			}
-			extended[extendedKey] = tagsBytes
-			return nil
+			tags, err := updateTags(extended[extendedKey], req.Tags, nil)
+			if err != nil {
+				return err
+			}
+			if limited && len(tags) > maxTags {
+				return errTooManyTags
+			}
+			return writeTags(extended, extendedKey, tags)
 		})
 	})

@@ -828,6 +888,11 @@ func (h *S3TablesHandler) handleTagResource(w http.ResponseWriter, r *http.Reque
 			h.writeError(w, http.StatusNotFound, errorCode, "resource not found")
 			return err
 		}
+		if errors.Is(err, errTooManyTags) {
+			h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest,
+				fmt.Sprintf("a resource can have at most %d tags", maxTags))
+			return err
+		}
 		if errors.Is(err, ErrConcurrentUpdate) {
 			h.writeError(w, http.StatusConflict, ErrCodeConflict, "resource tags changed concurrently, retry the request")
 			return err
@@ -909,6 +974,10 @@ func (h *S3TablesHandler) handleListTagsForResource(w http.ResponseWriter, r *ht
 			return err
 		}

+		// Checked after the reads, so they all came from the bound bucket.
+		if err := h.requireBoundTableBucket(r.Context(), client, bucketName); err != nil {
+			return err
+		}
 		var resourceARN string
 		if rType == ResourceTypeTable {
 			resourceARN = h.generateTableARN(ownerAccountID, bucketName, tableNamespace+"/"+tableName)
@@ -1034,6 +1103,10 @@ func (h *S3TablesHandler) handleUntagResource(w http.ResponseWriter, r *http.Req
 			return err
 		}

+		// Checked after the reads, so they all came from the bound bucket.
+		if err := h.requireBoundTableBucket(r.Context(), client, bucketName); err != nil {
+			return err
+		}
 		var resourceARN string
 		if rType == ResourceTypeTable {
 			resourceARN = h.generateTableARN(ownerAccountID, bucketName, tableNamespace+"/"+tableName)
@@ -1076,17 +1149,6 @@ func (h *S3TablesHandler) handleUntagResource(w http.ResponseWriter, r *http.Req
 		return err
 	}

-	// Remove specified tags
-	for _, key := range req.TagKeys {
-		delete(tags, key)
-	}
-
-	// Write updated tags
-	tagsBytes, err := json.Marshal(tags)
-	if err != nil {
-		h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, "failed to marshal tags")
-		return fmt.Errorf("failed to marshal tags: %w", err)
-	}
 	sameResource := requireTableCreatedAt
 	if rType != ResourceTypeTable {
 		sameResource = requireTableBucketCreatedAt
@@ -1096,8 +1158,11 @@ func (h *S3TablesHandler) handleUntagResource(w http.ResponseWriter, r *http.Req
 			if err := sameResource(extended, resourcePath, createdAt); err != nil {
 				return err
 			}
-			extended[extendedKey] = tagsBytes
-			return nil
+			tags, err := updateTags(extended[extendedKey], nil, req.TagKeys)
+			if err != nil {
+				return err
+			}
+			return writeTags(extended, extendedKey, tags)
 		})
 	})

@@ -80,10 +80,10 @@ func (m *Manager) PolicyContext(r *http.Request, bucket string) *PolicyContext {
 type tableBucketCreatedAtKey struct{}

 // WithTableBucketCreatedAt binds a DeleteTableBucket, CreateNamespace,
-// 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.
+// DeleteNamespace, CreateTable, DeleteTable or tag operation 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)
 }