iceberg: let clients select their table bucket as the catalog warehouse (#10549)

* iceberg: accept bare bucket names and ARNs as the catalog warehouse

Only s3://<bucket>/ was recognized. A warehouse spelled as a bare table
bucket name or as the s3tables bucket ARN -- the two forms users reach for
first, the latter being what AWS S3 Tables itself takes -- was silently
dropped, so every call landed on the default "warehouse" bucket and failed
with "table bucket warehouse not found".

* iceberg: report a missing table bucket as 404, not 500

Pointing a client at a table bucket that does not exist -- which every
client with no warehouse set does, since the default bucket "warehouse"
rarely exists -- returned InternalServerError with a message naming a
bucket the client never asked for. Answer 404 and say how to select one.

* admin: show the warehouse in the PyIceberg example

The example connected without one, so it always resolved to the default
table bucket and every client that copied it failed on the first call.

* test: pin bearer auth against a table bucket that exists

The subtest called the catalog with no warehouse and accepted 500 as proof
that auth had passed, since the default bucket does not exist. A missing
table bucket now answers 404, which the test read as an auth failure. Give
it a real table bucket so only 200 passes.

* test: assert the missing-bucket guidance reaches the client

The status and error type were checked but not the message, which is the
part of the mapping that tells a user how to select a table bucket.

* test: encode the warehouse query value

The ARN case pasted raw colons and slashes into the query string. Go's
parser tolerates them, so the test passed without modelling how a client
actually sends the request.
This commit is contained in:
Chris Lu
2026-08-03 13:25:37 -07:00
committed by GitHub
parent 63a180ef75
commit c191b2fe01
11 changed files with 189 additions and 38 deletions
+8 -9
View File
@@ -216,8 +216,12 @@ func TestOAuthTokenEndpoint(t *testing.T) {
t.Run("bearer token auth on catalog endpoint", func(t *testing.T) {
token := requestOAuthToken(t, env, env.accessKey, env.secretKey)
// Use the token to call the catalog
req, err := http.NewRequest(http.MethodGet, env.icebergURL()+"/v1/namespaces", nil)
// Point the call at a table bucket that exists, so anything but 200 is an
// auth or routing fault rather than the catalog reporting a missing bucket.
bucketName := "oauth-bearer-" + randomSuffix()
createTableBucketViaShell(t, env, bucketName)
req, err := http.NewRequest(http.MethodGet, env.icebergURL()+"/v1/"+bucketName+"/namespaces", nil)
if err != nil {
t.Fatalf("create request: %v", err)
}
@@ -225,17 +229,12 @@ func TestOAuthTokenEndpoint(t *testing.T) {
resp, err := http.DefaultClient.Do(req)
if err != nil {
t.Fatalf("GET /v1/namespaces with Bearer: %v", err)
t.Fatalf("GET /v1/%s/namespaces with Bearer: %v", bucketName, err)
}
defer resp.Body.Close()
// Auth should pass. We accept 200 (success) or 500 (missing warehouse bucket
// is an internal error, not an auth error). Reject 401/403/404/405.
body, _ := io.ReadAll(resp.Body)
switch resp.StatusCode {
case http.StatusOK, http.StatusInternalServerError:
t.Logf("Bearer token auth succeeded, status=%d", resp.StatusCode)
default:
if resp.StatusCode != http.StatusOK {
t.Fatalf("Bearer auth failed unexpectedly: status=%d body=%s", resp.StatusCode, body)
}
})
@@ -234,6 +234,7 @@ catalog = load_catalog(
**{
"type": "rest",
"uri": "http://localhost:` + fmt.Sprintf("%d", data.IcebergPort) + `",
"warehouse": "s3://my-table-bucket/",
}
)
+2 -1
View File
@@ -302,13 +302,14 @@ catalog = load_catalog(
**{
"type": "rest",
"uri": "http://localhost:` + fmt.Sprintf("%d", data.IcebergPort) + `",
"warehouse": "s3://my-table-bucket/",
}
)
# List namespaces
namespaces = catalog.list_namespaces()`)
if templ_7745c5c3_Err != nil {
return templ.Error{Err: templ_7745c5c3_Err, FileName: `view/app/iceberg_catalog.templ`, Line: 241, Col: 39}
return templ.Error{Err: templ_7745c5c3_Err, FileName: `view/app/iceberg_catalog.templ`, Line: 242, Col: 39}
}
_, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var17))
if templ_7745c5c3_Err != nil {
@@ -235,6 +235,7 @@ catalog = load_catalog(
**{
"type": "rest",
"uri": "http://localhost:` + fmt.Sprintf("%d", data.IcebergPort) + `",
"warehouse": "s3://my-table-bucket/",
}
)
@@ -361,12 +361,13 @@ catalog = load_catalog(
**{
"type": "rest",
"uri": "http://localhost:` + fmt.Sprintf("%d", data.IcebergPort) + `",
"warehouse": "s3://my-table-bucket/",
}
)
namespaces = catalog.list_namespaces()`)
if templ_7745c5c3_Err != nil {
return templ.Error{Err: templ_7745c5c3_Err, FileName: `view/app/s3tables_buckets.templ`, Line: 241, Col: 39}
return templ.Error{Err: templ_7745c5c3_Err, FileName: `view/app/s3tables_buckets.templ`, Line: 242, Col: 39}
}
_, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var20))
if templ_7745c5c3_Err != nil {
+11 -9
View File
@@ -16,12 +16,12 @@ import (
// handleConfig returns catalog configuration.
//
// When a client passes ?warehouse=s3://<bucket>/, the Iceberg REST spec
// expects the server to echo a catalog identifier back as overrides.prefix
// so subsequent calls use /v1/{prefix}/... and land on the right table
// bucket. Without this, clients like DuckDB's ATTACH flow fall back to an
// unprefixed path that resolves to the wrong bucket and report phantom
// "schema does not exist" errors. See issue #9103.
// When a client passes ?warehouse=, the Iceberg REST spec expects the server
// to echo a catalog identifier back as overrides.prefix so subsequent calls
// use /v1/{prefix}/... and land on the right table bucket. Without this,
// clients like DuckDB's ATTACH flow fall back to an unprefixed path that
// resolves to the wrong bucket and report phantom "schema does not exist"
// errors.
func (s *Server) handleConfig(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
config := CatalogConfig{
@@ -29,13 +29,15 @@ func (s *Server) handleConfig(w http.ResponseWriter, r *http.Request) {
Overrides: map[string]string{},
}
if warehouse := strings.TrimSpace(r.URL.Query().Get("warehouse")); warehouse != "" {
// Only the bucket portion of the warehouse URL is meaningful today —
// Only the bucket portion of the warehouse is meaningful today —
// SeaweedFS table-bucket routing is bucket-scoped, so any sub-path
// (e.g. s3://bucket/prefix/) is ignored here and clients that try to
// scope a catalog under a sub-prefix will still land on the bucket.
if bucket, _, err := parseS3Location(warehouse); err == nil && bucket != "" {
if bucket := resolveWarehouseBucket(warehouse); bucket != "" {
config.Overrides["prefix"] = bucket
config.Defaults["warehouse"] = warehouse
// Echo the location form regardless of how the client spelled the
// warehouse: clients derive default table locations from it.
config.Defaults["warehouse"] = "s3://" + bucket
}
}
if err := json.NewEncoder(w).Encode(config); err != nil {
@@ -5,9 +5,11 @@ import (
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/apache/iceberg-go"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3tables"
)
func TestNameValidationError(t *testing.T) {
@@ -37,15 +39,26 @@ func TestNameValidationError(t *testing.T) {
func TestWriteManagerError(t *testing.T) {
cases := []struct {
name string
err error
wantCode int
wantType string
name string
err error
wantCode int
wantType string
wantMessage []string
}{
{"invalid name is a client error", fmt.Errorf("invalid namespace name: only 'a-z', '0-9', and '_' are allowed"), http.StatusBadRequest, "BadRequestException"},
{"rejected schema is a client error", fmt.Errorf("%w: for v2: variant is not supported until v3", iceberg.ErrInvalidSchema), http.StatusBadRequest, "BadRequestException"},
{"bad format version is a client error", fmt.Errorf("%w: 4", iceberg.ErrInvalidFormatVersion), http.StatusBadRequest, "BadRequestException"},
{"everything else is a server fault", fmt.Errorf("all filers failed, last error: connection refused"), http.StatusInternalServerError, "InternalServerError"},
{name: "invalid name is a client error", err: fmt.Errorf("invalid namespace name: only 'a-z', '0-9', and '_' are allowed"), wantCode: http.StatusBadRequest, wantType: "BadRequestException"},
{name: "rejected schema is a client error", err: fmt.Errorf("%w: for v2: variant is not supported until v3", iceberg.ErrInvalidSchema), wantCode: http.StatusBadRequest, wantType: "BadRequestException"},
{name: "bad format version is a client error", err: fmt.Errorf("%w: 4", iceberg.ErrInvalidFormatVersion), wantCode: http.StatusBadRequest, wantType: "BadRequestException"},
{
name: "missing table bucket is a client error",
err: fmt.Errorf("all filers failed, last error: %w", &s3tables.S3TablesError{Type: s3tables.ErrCodeNoSuchBucket, Message: "table bucket warehouse not found"}),
wantCode: http.StatusNotFound,
wantType: "NoSuchNamespaceException",
// The guidance is the point of the mapping: the bucket the request
// resolved to is one the client never named, so the response has to
// say how to name a real one.
wantMessage: []string{"table bucket warehouse not found", "warehouse=s3://<table-bucket>/", "/v1/<table-bucket>/"},
},
{name: "everything else is a server fault", err: fmt.Errorf("all filers failed, last error: connection refused"), wantCode: http.StatusInternalServerError, wantType: "InternalServerError"},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
@@ -64,6 +77,11 @@ func TestWriteManagerError(t *testing.T) {
if resp.Error.Code != c.wantCode {
t.Fatalf("error code = %d, want %d", resp.Error.Code, c.wantCode)
}
for _, want := range c.wantMessage {
if !strings.Contains(resp.Error.Message, want) {
t.Errorf("message %q does not contain %q", resp.Error.Message, want)
}
}
})
}
}
+11 -1
View File
@@ -22,8 +22,18 @@ func TestGetBucketFromPrefix_WarehouseQueryFallback(t *testing.T) {
want: "another",
},
{
name: "malformed warehouse value falls through to default",
name: "bare bucket name is taken as the table bucket",
url: "/v1/namespaces?warehouse=not-a-url",
want: "not-a-url",
},
{
name: "table bucket ARN routes to its bucket",
url: "/v1/namespaces?warehouse=arn%3Aaws%3As3tables%3Aus-east-1%3Aadmin%3Abucket%2Fseaweed-iceberg",
want: "seaweed-iceberg",
},
{
name: "unusable warehouse value falls through to default",
url: "/v1/namespaces?warehouse=file%3A%2F%2F%2Ftmp%2Fwh",
want: "warehouse",
},
{
@@ -0,0 +1,71 @@
package iceberg
import (
"encoding/json"
"net/http/httptest"
"net/url"
"testing"
)
// The admin console tells users that every table bucket is its own Iceberg
// catalog, so the /v1/config handshake must turn whichever spelling of that
// bucket the client sends into overrides.prefix. PyIceberg only sends the
// warehouse on this one call, so a dropped value leaves it talking to the
// default bucket for the rest of the session.
func TestHandleConfigWarehouseSpellings(t *testing.T) {
tests := []struct {
name string
warehouse string
wantPrefix string
wantWarehouse string
}{
{
name: "s3 location",
warehouse: "s3://seaweed-iceberg/",
wantPrefix: "seaweed-iceberg",
wantWarehouse: "s3://seaweed-iceberg",
},
{
name: "bare bucket name",
warehouse: "seaweed-iceberg",
wantPrefix: "seaweed-iceberg",
wantWarehouse: "s3://seaweed-iceberg",
},
{
name: "table bucket ARN",
warehouse: "arn:aws:s3tables:us-east-1:admin:bucket/seaweed-iceberg",
wantPrefix: "seaweed-iceberg",
wantWarehouse: "s3://seaweed-iceberg",
},
{
name: "no warehouse",
warehouse: "",
},
{
name: "unusable value",
warehouse: "file:///tmp/wh",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
target := "/v1/config"
if tc.warehouse != "" {
target += "?" + url.Values{"warehouse": {tc.warehouse}}.Encode()
}
r := httptest.NewRequest("GET", target, nil)
rec := httptest.NewRecorder()
(&Server{}).handleConfig(rec, r)
var config CatalogConfig
if err := json.Unmarshal(rec.Body.Bytes(), &config); err != nil {
t.Fatalf("decode config: %v", err)
}
if got := config.Overrides["prefix"]; got != tc.wantPrefix {
t.Errorf("overrides.prefix = %q, want %q", got, tc.wantPrefix)
}
if got := config.Defaults["warehouse"]; got != tc.wantWarehouse {
t.Errorf("defaults.warehouse = %q, want %q", got, tc.wantWarehouse)
}
})
}
}
+51 -9
View File
@@ -141,27 +141,69 @@ func writeManagerError(w http.ResponseWriter, err error) {
writeError(w, http.StatusBadRequest, "BadRequestException", err.Error())
return
}
// A missing table bucket means the catalog the client selected does not
// exist, not a server fault. The storage-layer message names the resolved
// bucket, which for a client that sent no warehouse at all is the default
// one it never asked for, so say how to select a real table bucket.
var tableErr *s3tables.S3TablesError
if errors.As(err, &tableErr) && tableErr.Type == s3tables.ErrCodeNoSuchBucket {
writeError(w, http.StatusNotFound, "NoSuchNamespaceException",
fmt.Sprintf("%s: each table bucket is a separate catalog, select one with warehouse=s3://<table-bucket>/ or /v1/<table-bucket>/", tableErr.Message))
return
}
writeError(w, http.StatusInternalServerError, "InternalServerError", err.Error())
}
// resolveWarehouseBucket maps a client-supplied warehouse value to a table
// bucket name. Clients spell the warehouse three ways: the s3://<bucket>/
// location this catalog advertises, the table bucket ARN that AWS S3 Tables
// uses, and the bare bucket name. Accepting only the first sends every other
// spelling to the default bucket, where the request fails naming a bucket the
// client never asked for. Returns "" when the value names no usable bucket so
// the caller keeps its own default.
func resolveWarehouseBucket(warehouse string) string {
warehouse = strings.TrimSpace(warehouse)
var bucket string
switch {
case warehouse == "":
return ""
case strings.HasPrefix(warehouse, "s3://"):
parsed, _, err := parseS3Location(warehouse)
if err != nil {
return ""
}
bucket = parsed
case strings.HasPrefix(warehouse, "arn:"):
parsed, err := s3tables.ParseBucketNameFromARN(warehouse)
if err != nil {
return ""
}
bucket = parsed
default:
// Bare name, possibly with a sub-path that bucket-scoped routing ignores.
bucket, _, _ = strings.Cut(strings.TrimSuffix(warehouse, "/"), "/")
}
if !s3tables.IsValidBucketName(bucket) {
return ""
}
return bucket
}
// getBucketFromPrefix extracts table bucket name from prefix parameter.
// For now, we use the prefix as the table bucket name.
//
// The Iceberg REST spec lets clients identify a catalog either by embedding
// its prefix in the URL (/v1/{prefix}/...) or by passing ?warehouse=s3://
// <bucket>/ as a query parameter. Clients that skip the /v1/config handshake
// (or ignore its overrides) still routinely send the warehouse parameter on
// every request, so honor it as a fallback before the env-var default.
// See issue #9103.
// its prefix in the URL (/v1/{prefix}/...) or by passing ?warehouse= as a
// query parameter. Clients that skip the /v1/config handshake (or ignore its
// overrides) still routinely send the warehouse parameter on every request,
// so honor it as a fallback before the env-var default.
func getBucketFromPrefix(r *http.Request) string {
vars := mux.Vars(r)
if prefix := vars["prefix"]; prefix != "" {
return prefix
}
if warehouse := strings.TrimSpace(r.URL.Query().Get("warehouse")); warehouse != "" {
if bucket, _, err := parseS3Location(warehouse); err == nil && bucket != "" {
return bucket
}
if bucket := resolveWarehouseBucket(r.URL.Query().Get("warehouse")); bucket != "" {
return bucket
}
if bucket := os.Getenv("S3TABLES_DEFAULT_BUCKET"); bucket != "" {
return bucket
+5
View File
@@ -50,6 +50,11 @@ func ParseBucketNameFromARN(arn string) (string, error) {
return parseBucketNameFromARN(arn)
}
// IsValidBucketName is a wrapper to validate a table bucket name for other packages.
func IsValidBucketName(name string) bool {
return isValidBucketName(name)
}
// parseTableFromARN extracts bucket name, namespace, and table name from ARN
// ARN format: arn:aws:s3tables:{region}:{account}:bucket/{bucket-name}/table/{namespace}/{table-name}
func parseTableFromARN(arn string) (bucketName, namespace, tableName string, err error) {