diff --git a/test/s3tables/catalog/duckdb_oauth_test.go b/test/s3tables/catalog/duckdb_oauth_test.go index 6019f107a..afbb4886f 100644 --- a/test/s3tables/catalog/duckdb_oauth_test.go +++ b/test/s3tables/catalog/duckdb_oauth_test.go @@ -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) } }) diff --git a/weed/admin/view/app/iceberg_catalog.templ b/weed/admin/view/app/iceberg_catalog.templ index 04dd13c60..16dbb305f 100644 --- a/weed/admin/view/app/iceberg_catalog.templ +++ b/weed/admin/view/app/iceberg_catalog.templ @@ -234,6 +234,7 @@ catalog = load_catalog( **{ "type": "rest", "uri": "http://localhost:` + fmt.Sprintf("%d", data.IcebergPort) + `", + "warehouse": "s3://my-table-bucket/", } ) diff --git a/weed/admin/view/app/iceberg_catalog_templ.go b/weed/admin/view/app/iceberg_catalog_templ.go index a6f9a6d45..3070b11d6 100644 --- a/weed/admin/view/app/iceberg_catalog_templ.go +++ b/weed/admin/view/app/iceberg_catalog_templ.go @@ -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 { diff --git a/weed/admin/view/app/s3tables_buckets.templ b/weed/admin/view/app/s3tables_buckets.templ index 4d366c47d..f0e6c090e 100644 --- a/weed/admin/view/app/s3tables_buckets.templ +++ b/weed/admin/view/app/s3tables_buckets.templ @@ -235,6 +235,7 @@ catalog = load_catalog( **{ "type": "rest", "uri": "http://localhost:` + fmt.Sprintf("%d", data.IcebergPort) + `", + "warehouse": "s3://my-table-bucket/", } ) diff --git a/weed/admin/view/app/s3tables_buckets_templ.go b/weed/admin/view/app/s3tables_buckets_templ.go index 99dd7daad..f57dab566 100644 --- a/weed/admin/view/app/s3tables_buckets_templ.go +++ b/weed/admin/view/app/s3tables_buckets_templ.go @@ -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 { diff --git a/weed/s3api/iceberg/handlers_namespace.go b/weed/s3api/iceberg/handlers_namespace.go index 0a6ff7cde..1cfdf856d 100644 --- a/weed/s3api/iceberg/handlers_namespace.go +++ b/weed/s3api/iceberg/handlers_namespace.go @@ -16,12 +16,12 @@ import ( // handleConfig returns catalog configuration. // -// When a client passes ?warehouse=s3:///, 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 { diff --git a/weed/s3api/iceberg/iceberg_invalid_name_test.go b/weed/s3api/iceberg/iceberg_invalid_name_test.go index bab683af8..b56bdecb7 100644 --- a/weed/s3api/iceberg/iceberg_invalid_name_test.go +++ b/weed/s3api/iceberg/iceberg_invalid_name_test.go @@ -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:///", "/v1//"}, + }, + {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) + } + } }) } } diff --git a/weed/s3api/iceberg/iceberg_issue_9103_test.go b/weed/s3api/iceberg/iceberg_issue_9103_test.go index a7877ca27..691779148 100644 --- a/weed/s3api/iceberg/iceberg_issue_9103_test.go +++ b/weed/s3api/iceberg/iceberg_issue_9103_test.go @@ -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", }, { diff --git a/weed/s3api/iceberg/iceberg_warehouse_test.go b/weed/s3api/iceberg/iceberg_warehouse_test.go new file mode 100644 index 000000000..2be505f95 --- /dev/null +++ b/weed/s3api/iceberg/iceberg_warehouse_test.go @@ -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) + } + }) + } +} diff --git a/weed/s3api/iceberg/utils.go b/weed/s3api/iceberg/utils.go index 94277a2f5..52f4aa6c7 100644 --- a/weed/s3api/iceberg/utils.go +++ b/weed/s3api/iceberg/utils.go @@ -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:/// or /v1//", 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:/// +// 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:// -// / 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 diff --git a/weed/s3api/s3tables/utils.go b/weed/s3api/s3tables/utils.go index f04b31e6a..d65c02de7 100644 --- a/weed/s3api/s3tables/utils.go +++ b/weed/s3api/s3tables/utils.go @@ -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) {