Remove underscores from object metadata keys - #2354
Conversation
nginx sets `underscores_in_headers off` by default, so an S3 gateway behind nginx drops a request header whose name contains an underscore. Icechunk signs its metadata headers, so the gateway then saw a signed header that was absent from the request and answered: AccessDenied: There were headers present in the request which were not signed The message points at the wrong thing. Nothing extra arrived; something signed went missing. Tigris serves t3.storage.dev from a mixed fleet, so this hit only the nginx nodes and looked like a random flake: whole CI jobs failed or passed together, across every client version and environment. Reads were never affected, because only writes carry metadata. Keys are now alphanumeric, which also satisfies Azure's C# identifier rule that forbids `-`. A test in each crate locks the invariant, and scripts/gateway-metadata-header-probe.sh reproduces the gateway behaviour without credentials. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2354 +/- ##
=======================================
Coverage 84.95% 84.96%
=======================================
Files 88 88
Lines 39594 39611 +17
=======================================
+ Hits 33638 33655 +17
Misses 5956 5956
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| pub const ICECHUNK_COMPRESSION_OFFSET: usize = ICECHUNK_FILE_TYPE_OFFSET + 1; | ||
| pub const ICECHUNK_FILE_HEADER_LEN: usize = ICECHUNK_COMPRESSION_OFFSET + 1; | ||
|
|
||
| pub const LATEST_ICECHUNK_FORMAT_VERSION_METADATA_KEY: &str = "ic_spec_ver"; |
There was a problem hiding this comment.
can we also keep constants for the old names, maybe with suffix _DEPRACATED or something? In case we ever need to write some scripts that use them with old repos
| - Deleting a chunk key that cannot exist (coordinates outside the chunk grid, missing node, or a group path) is now a no-op instead of raising, matching zarr-python's stores. Writing a chunk outside the grid is still rejected ([#2312](https://github.com/earth-mover/icechunk/pull/2312)). | ||
| - `to_icechunk` no longer passes `synchronizer` and `zarr_version` to xarray's `ZarrStore.open_group`; xarray removed both parameters and passing them made `to_icechunk` fail with a `TypeError` on xarray development versions ([#2312](https://github.com/earth-mover/icechunk/pull/2312)). | ||
| - Writing a chunk with length 0 is now rejected instead of being committed. A chunk must decode to the full chunk shape, so no valid chunk is ever zero bytes long, and such a chunk could only fail once it was read back — long after the commit that introduced it. This is how a sparse GeoTIFF's unstored tiles (`offset = 0, byteCount = 0`) used to reach a repository. Applies to inline, virtual and materialized chunks alike, which means Icechunk deliberately rejects an empty write at a chunk key where a plain key-value store would accept it, in the same way it already rejects invalid zarr keys and invalid metadata. To record that a chunk is not stored at all, delete it rather than writing a zero-length one; it then reads back as the array's fill value ([#2328](https://github.com/earth-mover/icechunk/issues/2328)). | ||
| - Object metadata keys no longer contain `_`. Icechunk stamps its own metadata on every file it writes, and stamps a write-id on conditional PUTs. nginx sets `underscores_in_headers off` by default, so an S3 gateway fronted by nginx silently dropped those headers; because they were also signed, the gateway then rejected the request with `AccessDenied: There were headers present in the request which were not signed`. The message is misleading, since the problem is a signed header going missing, not an extra one arriving. Tigris serves `t3.storage.dev` from a mixed fleet, so writes failed only on the nginx nodes and looked intermittent. The keys are now `icspecver`, `icclient`, `icfiletype`, `iccompalg` and `icechunkwriteid`; they stay alphanumeric so they also satisfy Azure's C# identifier rule, which forbids `-`. Use `scripts/gateway-metadata-header-probe.sh` to test a gateway ([#2354](https://github.com/earth-mover/icechunk/pull/2354)). |
There was a problem hiding this comment.
Let's put less detail, just what happened not why
Scripts that read repositories written before the rename still need the old names. Also trims the changelog entry to what changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The probe diagnoses a third-party gateway and is not part of the library, so it belongs in the pull request discussion instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
paraseba
left a comment
There was a problem hiding this comment.
Let's wait on Luiz's +1 too, to have two people thinking if we depend on this anywhere.
li-em
left a comment
There was a problem hiding this comment.
Unfortunate to drop _ and - because it is harder to read, but it is the right solution. Thanks!
What
Renames the five object-metadata keys Icechunk stamps on writes so they contain letters only:
ic_spec_vericspecveric_clienticclientic_file_typeicfiletypeic_comp_algiccompalgicechunk_write_idicechunkwriteidThe old names stay available as
*_DEPRECATEDconstants, for scripts that read repositories written before this change.Why
nginx sets
underscores_in_headers offby default, so an S3 gateway behind nginx silently drops a request header whose name contains an underscore. Icechunk signs its metadata headers, so the gateway then sees a header inSignedHeadersthat is not in the request. MinIO-derived gateways answer:The message points at the wrong thing. Nothing extra arrived, something signed went missing (
extractSignedHeaders,cmd/signature-v4-utils.go).Tigris serves
t3.storage.devfrom a mixed fleet, so only some nodes are affected. That made it look like a random flake in Arraylake's integration tests: a whole job failed or passed together, across every client version and environment, and only ever on writes, because reads carry no metadata.66.93.0.25runs nginx,66.93.0.22does not. Same request, same second, 10 runs each:Script that produces that table (needs no credentials)
MinIO-derived code validates
SignedHeadersbefore it looks up the access key, so a fake key still reaches the branch under test.Why not hyphens
Azure requires metadata names to be valid C# identifiers, which forbids
-. That is why the keys used_in the first place (design doc 017). Letters and digits are the only characters both back ends accept, so the new names carry no separator at all. Design doc 017 is updated to record both constraints.Compatibility
The four format keys have no reader in this repo; they are informational.
icechunkwriteidis read back, but only to recognise a write the same process just issued, so it is always written and read with the new name. Objects written by older versions keep the old keys and stay readable; they simply read as "not ours" in the readback path, which is already the correct answer for another writer's object.Testing
cargo test -p icechunk-format -p icechunk-storagepasses. A test in each crate asserts the keys stay ASCII alphanumeric so this cannot regress.cargo fmt --check,cargo clippy --all-targetsandcargo check --workspace --all-targetsare clean.Related: earth-mover/arraylake#7414.
Draft on purpose. Samantha, per
AI_USAGE_POLICY.mdthe description should be in your own words along with your review attestation, so please rewrite this before marking it ready.[This is Claude Code on behalf of Samantha Hughes]