fix(s3tables): route REST RenameTable and refuse unrouted S3 Tables requests (#12446)

* s3tables: route REST RenameTable and refuse unrouted S3 Tables requests

PUT /tables/<arn>/<ns>/<name>/rename had no route, so it fell through to the
/{bucket} routers and ran as a PutObject into a bucket named "tables",
answering 2xx while the table kept its name.

- Answer any s3tables-signed request no S3 Tables route takes with 404
  UnknownOperationException, before the health, domain and bucket routers.
- Route the AWS RenameTable REST call (newNamespaceName, newName, both
  optional; versionToken).
- Honor versionToken on RenameTable, checked against the snapshot the claim
  asserts so a commit between the two reads cannot slip a stale token through.

* s3tables: match the credential scope's service, not a region named s3tables

The unrouted-request fallback widens what a false match costs: an S3
client whose region is named s3tables had every request answered as an
unknown S3 Tables operation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: refuse an empty newName or newNamespaceName on REST rename

An empty field was read as omitted, so {"newNamespaceName":"other",
"newName":""} moved the table under its old name instead of failing
validation the way AWS does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: check the rename version token after the snapshot comparison

Once the separately read metadata is known to equal the claimed snapshot,
the token compares against the parsed metadata; no second decode of the
snapshot bytes is needed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables test: cover policy and tag changes in the rename snapshot compare

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables test: give each REST rename subtest its own source table

The token subtests renamed the table the first subtest produced, so run
alone they failed on a missing table instead of testing the token.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
authored and GitHub committed 2026-11-06 09:32:13 -08:00
1 parent c84c43f3da
commit f71c0fe523
8 files changed
+1027 -6

No files matched your search

+42
View File
@@ -7,6 +7,7 @@ import (
"errors"
"fmt"
"io"
"maps"
"net/http"
"path"
"strconv"
@@ -1395,6 +1396,24 @@ func (h *S3TablesHandler) handleRenameView(w http.ResponseWriter, r *http.Reques
return h.renameCatalogEntry(w, r, filerClient, viewEntryKind)
}
// renameReadsMatchSnapshot reports whether the source attributes a rename read
// separately (reads, plus the parsed tags) are the ones in snapshot, the entry
// its claim asserts. An absent attribute matches an empty read.
func renameReadsMatchSnapshot(snapshot map[string][]byte, tags map[string]string, reads map[string][]byte) bool {
for key, value := range reads {
if !bytes.Equal(snapshot[key], value) {
return false
}
}
var snapshotTags map[string]string
if raw := snapshot[ExtendedKeyTags]; len(raw) > 0 {
if err := json.Unmarshal(raw, &snapshotTags); err != nil {
return false
}
}
return maps.Equal(snapshotTags, tags)
}
// markSourceGone tags a missing source path so the rename reports it under the
// source name instead of the closed-parent mapping naming the destination.
func markSourceGone(err error) error {
@@ -1447,6 +1466,7 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ
destPath := GetTablePath(bucketName, destNamespace, destName)
var metadata tableMetadataInternal
var metadataXattr []byte
var metadataVersionXattr []byte
var maintenanceXattr []byte
var maintenanceStatusXattr []byte
@@ -1461,6 +1481,7 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ
if err != nil {
return err
}
metadataXattr = data
if err := json.Unmarshal(data, &metadata); err != nil {
return fmt.Errorf("failed to unmarshal table metadata: %w", err)
}
@@ -1636,6 +1657,27 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ
return NewAuthError(kind.renameOp, principal, "not authorized to create "+kind.noun+" in the destination namespace")
}
// The destination is built from attributes read one lookup at a time, but
// the claim and the source clear assert srcExtended. A write landing between
// those reads would let the claim succeed while the destination publishes
// the older values, dropping the write; refuse it so the caller retries.
// Past this check metadata is the claimed snapshot, so the token compares
// against it.
if !renameReadsMatchSnapshot(srcExtended, tableTags, map[string][]byte{
ExtendedKeyMetadata: metadataXattr,
ExtendedKeyMetadataVersion: metadataVersionXattr,
ExtendedKeyPolicy: []byte(tablePolicy),
ExtendedKeyMaintenance: maintenanceXattr,
ExtendedKeyMaintenanceStatus: maintenanceStatusXattr,
}) {
h.writeError(w, http.StatusConflict, ErrCodeConflict, kind.noun+" changed during rename, retry the request")
return ErrConcurrentUpdate
}
if req.VersionToken != "" && metadata.VersionToken != req.VersionToken {
h.writeError(w, http.StatusConflict, ErrCodeConflict, "version token mismatch")
return ErrVersionTokenMismatch
}
if destExists {
h.writeError(w, http.StatusConflict, kind.existsCode, fmt.Sprintf("%s %s already exists", kind.noun, destName))
return ErrTableAlreadyExists