Gunnar

nordisk/skade

.nornir/serve-object-store-design.md

# `skade-katalog serve` over an object-store warehouse — design

Status: implemented (feature `object-store`, default OFF).
Scope: `serve/`, `rest/`, `grpc/`, plus one error-reporting fix in `skade/src/object_store.rs`.

## The problem

`skade::Warehouse` (the data plane) can put a warehouse's Iceberg blobs in a
pluggable object store — that is what `open_with_store` /
`open_with_object_store_config` exist for, and it is how a warehouse ends up on
a shared MinIO. `skade-katalog serve` (the REST/gRPC front) could not then serve
that warehouse, so an ecosystem engine — Doris, Trino, Spark, PyIceberg — had no
way to reach it over `iceberg.catalog.type = rest`.

Concretely, pointing `serve` at a redb written by the data plane against MinIO
gave a **half-working** catalog:

```
GET /v1/namespaces                          → {"namespaces":[["main"]]}      ✓
GET /v1/namespaces/main/tables              → {"identifiers":[{"name":…}]}   ✓
GET /v1/namespaces/main/tables/personer     → 404 "Failed to read file
    s3://…/metadata/00001-….metadata.json: No such file or directory"        ✗
```

### Why listing worked and loading did not

The two crates were never out of step on the **catalog**:

* both `RedbCatalogBuilder::load(…)` call sites — `Warehouse::open*` in
  `skade/src/warehouse.rs` and `open_catalog` in `serve/src/lib.rs` — pass the
  catalog name **`skade`**, and `serve --catalog` already defaults to `skade`;
* the redb keyspace is one implementation (`src/keys.rs`), shared by both;
* `list_namespaces` / `list_tables` key on `(catalog_name, ns)` only — the
  warehouse location plays no part in a listing, so a location mismatch cannot
  hide a namespace.

The mismatch was entirely on the **storage** side, in two places:

1. `serve::open_catalog` hardwired `LocalFsStorageFactory`. The catalog resolves
   a table to its recorded `metadata-location`, which for an object-store
   warehouse is `s3://bucket/prefix/…`. A local-fs `FileIO` cannot address that,
   so `Catalog::load_table` failed before the front ever saw a table.
2. Both fronts read the metadata document with `tokio::fs::read` after stripping
   `file://` (`rest/src/lib.rs::read_metadata_json`,
   `grpc/src/lib.rs::read_metadata_bytes`). Even with a correct catalog
   `FileIO`, the front would then have gone to the local filesystem for an
   `s3://…` path and failed.

`serve` also had no way to set the warehouse **location** to anything but
`file://<--warehouse>`, so a newly created table would have been given a
`file://` default location even on an S3 warehouse.

## The change

Three edges, all additive.

### 1. The fronts read metadata through the catalog's own `FileIO`

`read_metadata_json(file_io, loc)` / `read_metadata_bytes(file_io, loc)` now go
through `table.file_io()` — the storage the catalog is actually configured with.
For a `file://` warehouse that is `LocalFsStorageFactory`, i.e. exactly what the
fronts did before; for any other `StorageFactory` it is the one thing that lets
the metadata document be fetched at all.

The historical `tokio::fs` read is retained as a **fallback** on the error path,
so no arrangement that worked before can stop working. The reported error is the
`FileIO` one, because that is the storage the operator configured.

### 2. `ServeConfig` grows a location and a storage

```rust
pub warehouse_uri: Option<String>,   // None → the historical file://<warehouse>
pub storage: WarehouseStorage,       // default LocalFs → the historical factory
```

`WarehouseStorage::ObjectStore(skade::object_store::ObjectStoreConfig)` exists
only under the `object-store` feature. That config type is the **same** one
`Warehouse::open_with_object_store_config` takes, and `ObjectStoreFactory` is
the same `iceberg::io::StorageFactory` the data plane installs — so "serve the
warehouse the data plane wrote" is literally "hand `serve` the two values the
writer was opened with". REUSE, not a second S3 client.

`ServeConfig::warehouse_location()` returns `warehouse_uri` when set and
`file://<warehouse>` otherwise; `ServeConfig::new` sets both new fields to their
historical-behaviour defaults.

### 3. CLI

New flags, all optional:

```
--warehouse-uri URI       s3://bucket/prefix  (default: file://ROOT)
--s3-bucket NAME          turns on the S3 store
--s3-endpoint URL         MinIO endpoint; omit for real AWS
--s3-region NAME          default us-east-1
--s3-virtual-hosted       default is path-style (what MinIO needs)
```

`--warehouse ROOT` is still required in the local-fs shape and is now optional
**only** when `--warehouse-uri` is given, in which case `--db PATH` is required
(the redb file is then the only local artefact). Relaxing a requirement cannot
break an existing invocation.

Credentials are read from the **environment only** —
`SKADE_S3_ACCESS_KEY_ID | AWS_ACCESS_KEY_ID | MINIO_ACCESS_KEY` and
`SKADE_S3_SECRET_ACCESS_KEY | AWS_SECRET_ACCESS_KEY | MINIO_SECRET_KEY`. There is
deliberately no `--s3-access-key` flag: a flag lands in `ps` output, in shell
history, and in any systemd unit that carries the command line.

Without the feature the `--s3-*` flags parse and then fail with
"needs a build with the `object-store` feature", not "unknown argument".

Two pairings are checked at **startup**, because the failure they produce is the
half-working catalog this whole change exists to remove — listings fine,
`loadTable` broken — and that is far cheaper to diagnose as one line before the
socket binds than as a Doris error hours later:

* `--warehouse-uri s3://…` with **no** `--s3-bucket` (the catalog would come up
  on LocalFs while stamping `s3://` locations it cannot read);
* `--warehouse-uri s3://…` on a build **without** the feature (nothing linked
  into that binary can reach the scheme).

`--warehouse-uri file://…` stays legal in both shapes — relocating a local root
is not the same thing as needing a store. Both checks run *before* the
credential lookup, so the misconfiguration is reported on a box with no keys
set, and so they are unit-testable without touching the process environment
(`serve/src/main.rs`, `mod tests`).

### 4. Non-2xx S3 responses become errors (`skade/src/object_store.rs`)

`rust-s3` only fails a request itself under its `fail-on-err` feature, and skade
does not enable it (`default-features = false`, `["tokio-rustls-tls"]`). Without
it **every** `Bucket` method resolves `Ok` for a 4xx, with the XML error document
in the response body. `check_status` turns any non-2xx into

```
s3 get <key>: HTTP 403: <?xml …><Error><Code>InvalidAccessKeyId</Code>…
```

and it is applied across the whole surface, not just `get`, because each method
had its own way of lying:

| method | the lie |
|---|---|
| `get` / `get_range` | the XML error document returned as object bytes ⇒ `expected value at line 1 column 1` from the Iceberg JSON parser |
| `put` | **a write that never happened reported as success** — the commit that follows points at blobs that are not there. The worst of the family. |
| `delete` | a denied delete reported as done (404 stays a no-op: the trait documents a missing key as benign, and S3 answers 204 for it) |
| `exists` | `head_object` resolves `Ok` on a 404, so `Ok(_) => true` claimed **every** key exists |
| `size` | the 404/403 response's own `Content-Length` reported as the object's size, which `Storage::metadata` hands on as a Parquet footer offset |

`list` needs no check: rust-s3 does not hand the response back, but it
deserializes the body into a `ListBucketResult` whose `Name` element is
required, so an `<Error>` document already fails to parse. A denied listing
cannot read as "the warehouse is empty".

3xx counts as failure too. rust-s3 drives a bare `hyper::Client`, which does not
follow redirects, so the 301 AWS sends for a wrong-region bucket arrived as a
short HTML body masquerading as the object. Empty bodies are handled too — a
HEAD has none — so the message ends at the status instead of a dangling colon.
A `put` never carries a reason at all: rust-s3 fetches PUT responses in `etag`
mode, which replaces the body with the ETag header.

Error-path only: a 2xx GET is byte-for-byte what it was, and a ranged GET's
**206 Partial Content** is accepted (the obvious wrong fix, `status == 200`,
would break every Parquet read).

### 5. One-byte ranged reads no longer abort the process

Adjacent, and found while testing the above. `get_object_range` takes an
INCLUSIVE end and `assert!(start < end)` on it, so the honest conversion from
iceberg's exclusive range — `end - 1` — **panics** on a legitimate `5..6` read.
Parquet footer probing does walk single-byte ranges. `get_range` now asks for
one byte more in that one case and trims, still a single ranged GET.

## Why this is additive

* Feature `object-store` is **default OFF**. With it off, `serve` links no
  `skade`, no arrow, no parquet, no S3 client, and `open_catalog` builds the
  identical `LocalFsStorageFactory` catalog it always did.
* `ServeConfig::new` is unchanged in signature and yields the historical
  behaviour; the two new fields default to it. (Struct-literal construction of
  `ServeConfig` would need the two new fields — every call site in this repo and
  the documented path use `ServeConfig::new`.)
* No existing flag, endpoint, or wire shape changed.
* The fronts' metadata read keeps the old local-fs read as a fallback.

## What is verified

`serve/tests/data_plane_warehouse.rs`:

* `serves_a_local_data_plane_warehouse` (always on) — writes with
  `skade::Warehouse::open` + `Table::append`, drops it, then opens the same redb
  through `serve` and asserts `main` / `personer` / a real metadata document
  over REST. This is the test that pins the catalog-name and keyspace agreement
  between the data plane and the serve binary; a rename on either side goes red.
* `serves_an_object_store_data_plane_warehouse` (feature `object-store`) —
  writes with `open_with_object_store_config` under **`s3://testbucket/…`**
  locations backed by a `LocalFs` object store, then serves it and asserts
  `loadTable` returns metadata whose `metadata-location` and `manifest-list` are
  `s3://…`. Reverting either half of the fix turns this red (measured: with the
  fronts back on `tokio::fs`, `loadTable` → HTTP 500 "No such file or
  directory").

`skade/tests/s3_status.rs` (feature `s3`) — the non-2xx matrix, against a
one-connection HTTP server on `127.0.0.1` that plays the endpoint, so every
status code is reproducible and the suite stays offline. 15 tests covering
403/404/301, an empty error body, a 2xx body unchanged, 206 accepted, the
one-byte range, and the `put`/`delete`/`exists`/`size` half. Measured against
the pre-fix store: **11 of 15 red**. The 4 that pass both ways are the
"nothing that worked may stop working" guards (successful GET, 206 ranged GET,
404 delete as a no-op, denied listing).

`serve/src/main.rs` `mod tests` — the CLI pairings, both with and without the
feature: an `s3://` root with no bucket refused, a bucket with no root refused,
`--s3-*` without `--s3-bucket` refused, a stock build naming the feature rather
than "unknown argument", and the plain local shape unchanged.

`serve/examples/loadtable_bench.rs` — the A/B for the one thing here that could
cost an existing deployment: routing the metadata read through `FileIO` instead
of `tokio::fs`, on the hot path of every query plan. Measured on `dwarf`, same
file, 500 iterations: `tokio_fs` 18.4 µs/op, `file_io_localfs` 7.5 µs/op —
**0.41×, i.e. faster**, because iceberg's LocalFs storage reads directly while
`tokio::fs` hops through the blocking pool. The object-store arm
(`file_io_objectstore`, 7.5 µs/op) is level with it. End-to-end over REST the
read is noise: 215 µs local vs 221 µs object-store per `loadTable`.

`serve/examples/live_object_store.rs` — the live half, since none of the above
can exercise an endpoint URL, TLS, path-style addressing, SigV4 or a bucket
policy. Run against the `red` bench MinIO (`https://172.16.12.9:9000`, bucket
`red`, path-style, TLS via the red mock CA): wrote 256 rows through the data
plane, served that warehouse, and over REST got `[["main"]]`, `personer`, and a
`loadTable` whose `metadata-location` and `manifest-list` were both
`s3://red/…` — **PASS**, then cleaned up its 5 objects.

The same binary with deliberately wrong credentials, against the same live
MinIO, is the before/after for §4 in production:

```
pre-fix:  DataInvalid => Failed to parse json string,
          source: expected value at line 1 column 1
post-fix: s3 put skade-live-check/…/00000-….metadata.json: HTTP 403
```