Add an Azure Blob Storage backend - #38
Open
AlvinStanescu wants to merge 15 commits into
Open
Conversation
Adds `StoreBackend::Azure`, `AzureConfig` (account / endpoint / credential) and `AzureCredentialKind` (developer_tools | client_secret | managed_identity | workload_identity, default developer_tools), plus `StoreConfig.azure` and the `[store.azure]` block in walgit.example.toml. Config only — no store implementation yet. `account` is the storage account and `bucket` stays the blob container; `endpoint = ""` means https://<account>.blob.core.windows.net, overridden for Azurite and sovereign clouds. `apply_env` is generic over the serialized doc, so WALGIT__STORE__AZURE__* overrides work without extra wiring. `open_store` grows a bailing `Azure` arm: the match is exhaustive, so the new variant would otherwise break the workspace build (and `just clippy`) before the backend lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the `azure` feature (in the default set) with the Azure Blob SDK
dependencies, and an `AzureStore` skeleton that `open_store` now
dispatches `StoreBackend::Azure` to, replacing the temporary bail.
Construction is real: `store.azure.account` is required and fails closed
with the config key named, the endpoint resolves to
`https://{account}.blob.core.windows.net` when unset, and the credential
kind selects an `azure_identity` credential whose secrets come only from
the environment. Every `ObjectStore` operation is a stub returning
`InvalidArgument`; the following tasks fill them in.
`azure_core` is a direct dependency so the transport is rustls (never
openssl) and `hmac_rust`/`xml` are available for the SAS-signing and
list/commit payload work ahead.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements the non-chunked data plane on top of the Task 2 skeleton: - `get`: `download` with `parallel: 1` and half-open→HttpRange conversion; `ObjectMeta::size` is the whole-object size (parsed from `Content-Range`, which the SDK's partitioned download always requests). A conditional GET's 304 arrives as an `Err`, so it is caught before classification and returned as `GetResult::NotModified`, taking the version from the ETag the SDK keeps in the error's raw response (HEAD as a fallback). - `head`: `get_properties`, 404 → `Ok(None)`. - `put`: `PutBody::Bytes`, small `Stream` (collected) and `File` (read whole) in one `Put Blob`; `Create` → `If-None-Match: *`, `Update(v)` → `If-Match`, with `current` back-filled by HEAD on a CAS failure. Bodies at or above `multipart_threshold` route to `chunked_put`, still a stub. - `delete`: Azure has a *native* conditional delete, so the HEAD + compare + DELETE emulation s3.rs needs (and its check-then-act race) does not apply. Absent key: `Ok(())` unconditional, `NotFound` conditional. - Error mapping: 404 → NotFound, 409|412 → PreconditionFailed, 429|5xx → Retryable, else Other. Errors name the key, never header contents. ETag quoting is centralized in `strip_etag`/`to_wire_etag` (walgit stores the bare value; Azure wants it quoted on the wire), so the whole round trip is one edit if the live service disagrees. 17 hermetic unit tests cover the ETag round trip, classification (real `azure_core::Error`s — `ErrorKind::HttpResponse` is publicly constructible), 304 detection, ETag extraction from an error response, range conversion, Content-Range parsing, and hierarchical-key URL encoding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…place
Review fixes.
1. Transport/IO failures were classified `Other`, so a connection reset, TLS
error, timeout or DNS/connect failure was fatal where the same failure on
S3 is retryable. `coord::cas_update` — the read-modify-write loop behind
every manifest and lease — retries only `Retryable` and `PreconditionFailed`
and returns anything else at once, and the server maps a retryable store
error to a 503 the client can retry rather than a 500, so a blip could fail
a push outright. Status-less `ErrorKind::Io` and `ErrorKind::Connection` are
now `Retryable`; `Credential`/`DataConversion`/`Other` stay `Other`.
`Connection` is included alongside `Io` because that is where the SDK's
reqwest transport files a refused connect / DNS failure (`is_connect()`),
and the SDK's own retry policy retries exactly `Io | Connection`. Mapping
only `Io` would have left the DNS case fatal.
2. `version_from_error` inlined a third quote-stripping site, contradicting the
module doc's claim that `strip_etag`/`to_wire_etag` are the only two places
that know about ETag quoting. It now routes through `version_from_etag`, so
the claim holds again — `trim_matches('"')` appears in those two helpers and
nowhere else.
Tests: the status-less pinning test is split — `Io`/`Connection` assert
retryable, `Credential`/`DataConversion`/`Other` assert Other (18 azure tests).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`list` rides the SDK's own `Pager`: `ItemIterator` is already a `Stream` of items across pages, threading the marker itself, so azure needs no unfold buffer of the kind `s3.rs` has to keep. Azure has no server-side start-after (`startFrom` is a hierarchical-namespace parameter, and inclusive), so that cut is made client-side and strictly greater, per the store contract. `list_prefixes` cannot use the SDK at all: `azure_storage_blob` 1.1.0-beta.2 exposes no `delimiter` parameter anywhere, and a delimited listing is the only way to walk "directories" without paging every blob under them. It issues the `List Blobs` REST call directly, with an Entra token from the same credential the SDK uses, `x-ms-version` held equal to the SDK's own `DEFAULT_VERSION` (2026-04-06), a URL built with the SDK's `UrlExt::query_builder` so both paths percent-encode identically, and the response parsed with `azure_core`'s quick-xml re-export — no new dependency, one XML stack in the tree. The token is marked sensitive on its header and never enters a log line or an error string; the REST error path goes through the same numeric status mapping `classify` uses, now extracted into `from_status` so there is one copy of it. Unit tests are hermetic and fixture-driven: the XML parse (prefixes + marker, empty/absent marker, a page with only blobs, malformed input), URL construction (prefix and marker percent-encoding, dropped empty prefix), the start_after predicate, and the BlobItem -> ObjectMeta mapping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bodies at or above `multipart_threshold` are staged as `multipart_part_size` blocks (sequential, no fan-out) and published with one `Put Block List`. The conditional headers ride on that commit, so — unlike `s3.rs`, whose `CreateMultipartUpload` cannot carry one and is therefore `Overwrite`-only — every `PutMode` chunks. The mapping is extracted into `put_conditions` and shared with the single-shot upload so the two paths cannot drift, as is the CAS-failure HEAD (`put_error`). Block ids are zero-padded 16-digit ASCII: uniform length is an Azure requirement and the padding keeps byte order in step with staging order. They are passed raw — the SDK base64-encodes them for the `blockid` query parameter (generated/clients/block_blob_client.rs:344) and for the commit body (`BlockLookupList::latest` via `models_serde::option_vec_encoded_bytes_std`). Blocks staged for a commit that never lands belong to no blob and Azure collects them after seven days, so there is no abort path. Tests first: block-id uniformity and ordering, the pure chunk-split arithmetic at its boundaries, the mode->condition mapping, stream regrouping, the zero-copy slice path, and a short body rejected before anything is committed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`chunked_put` cut a `PutBody::File` at the raw `multipart_part_size`
while the block-count guard and `chunk_sizes` both normalized through
`effective_part`. Nothing validates `multipart_part_size`, so a
configured 0 made `util::file_stream` compute `want = 0` and end the
stream at once, while the split still expected one whole-body chunk —
`next_chunk` then failed the put with a spurious "body ended short of
its declared length" where the design calls for a single-block success.
The body -> stream selection moves into `body_stream`, which normalizes
through `effective_part` like the other two sites, so the guard, the
split and the reader now share one definition and the tests can drive
the real seam instead of a copy of it.
Regression test drives every `PutBody` shape through the production
guard + `body_stream` + `chunk_sizes` + `next_chunk` at part sizes 0,
100, 250 and 4096, asserting each requested chunk arrives in full, the
pieces reassemble the file, and the split's chunk count matches what the
guard counted. Verified to fail before the fix ("chunk of 250 at part 0:
body ended 250 bytes short of its declared length") and pass after.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mints a read-only, time-boxed SAS URL signed with a user delegation key the account issues to this identity — never a shared account key, which walgit has nowhere to store and never holds. The string-to-sign is the 24-field layout the "Create a user delegation SAS" reference prescribes for sv >= 2020-12-06 (the 23-field one predates signedEncryptionScope), with sv taken from the key's own skv. The separators are positional, so every unused field is present and empty; `sas_string_to_sign_layout` asserts all 24 positions, and `sas_signature_is_deterministic` pins a fixed vector recomputed independently of this code. `SasFields` carries both halves of the token — what gets hashed and what goes on the URL — so a parameter cannot be signed but not sent, or sent but not signed. Either is an opaque AuthenticationFailed from the service. Two things the SDK forces, both commented at the site: `UserDelegationKey` lives in azure_storage_common (not re-exported by azure_storage_blob, and not named here at all — `into_model()` infers it), and its `value` arrives already base64-*decoded*, so it is re-encoded for `hmac_sha256`, which decodes it again itself. The key is cached for a day behind a Mutex, refreshed when it can no longer cover the URL being minted plus a clock-skew margin. That is a signing-key cache, not an access-token cache: Azure enforces revocation server-side, so holding one cannot extend revoked access, and per-URL exposure is bounded by the caller's ttl either way. A ttl no key can cover is refused rather than shortened — accepting it would re-fetch on every call and still sign a URL the service rejects. The result is a credential: it is never logged, traced, or put in an error message, and no test writes a whole signed URL down — assertions run on parsed query pairs, with `sig` checked only for shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`azure_contract` runs the shared contract suite against a real storage
account when `WALGIT_TEST_AZURE_ACCOUNT` is set (container from
`WALGIT_TEST_AZURE_CONTAINER`, default `walgit-test`), and skips with a
line on stderr when it is not — the same shape `gcs_contract` uses:
unique `contract-test-{uuid}` prefix, cleanup by listing and deleting
everything under it.
Two details the wrapper cannot inherit:
- 5 MiB `multipart_threshold`/`multipart_part_size`, exactly as
`s3_contract` sets them. At the default threshold the 6 MiB multipart
case and the 8 MiB streamed roundtrip both fit one upload and the
staged-block path (stage_block + commit_block_list) would never run
against the service.
- A SAS probe: put a small blob, mint its user-delegation URL, fetch it
with a bare reqwest client and no Authorization header. The URL is a
credential, so nothing prints it — not on success, and not on failure,
where a reqwest error's own Display would carry the URL. Failures name
the key, the HTTP status and Azure's `x-ms-error-code` header only.
`just test-azure <account> [container]` runs it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The user-delegation SAS declared `sv = skv`, on the assumption that signing
under one service version and declaring another is AuthenticationFailed. The
live service disproves it: `sv` and `skv` are independent — `skv` states which
version issued the delegation key, `sv` which version's signing rules the URL
was built under — and a SAS whose `sv` is older than its `skv` is accepted.
Tying them together was fatal here because `skv` is whatever version the SDK
negotiated, currently 2026-04-06, and that version does not use the documented
24-field string-to-sign this code builds.
Probed against the live account (walgitplay*, westeurope), one blob, read-only
SAS, delegation key issued at x-ms-version 2026-04-06:
sv = skv = 2026-04-06, 24 fields signed -> 403 AuthenticationFailed
(service's own string-to-sign: 28 fields)
sv = skv = 2026-04-06, 25/26/27/28/29/30 trailing-empty fields
-> 403 AuthenticationFailed (28)
sv = 2020-12-06, skv left 2026-04-06 -> 200 OK
sv = 2022-11-02, skv left 2026-04-06 -> 200 OK
sv = skv = 2022-11-02 -> 200 OK
The four extra fields at 2026-04-06 are interleaved, not appended — padding the
layout out to the service's own field count still fails to match — and that
preview version's field order is undocumented, so it cannot be guessed. `sv` is
therefore pinned to 2020-12-06, the version that introduced the 24-field layout
(it added signedEncryptionScope) and the oldest one this code signs correctly.
`skv` continues to travel verbatim from the key, into both the string-to-sign
and the query string.
The fixed signature vector moves with it: field 15 of the signed string is a
different value now, so every byte after it hashes differently. The new
expectation was recomputed independently of this code, from the REST
reference's field list.
azure_contract's SAS probe — a plain reqwest GET carrying no Authorization
header — now returns the blob body.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Eleven steps share one #[tokio::test], so a green run against a real service said only `ok` — no evidence that a particular guarantee held, and no way to see which step got slow. Each step now prints its name and elapsed time on the way out; a failure still names itself through the panic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enumeration-level only; the backend itself landed in the preceding commits and changes no protocol. - README: the quickstart bucket line, the intro sentence, the stores feature row (Entra credentials, user-delegation SAS) and the code map (`backends s3, gcs, azure, memory`). - docs/CONTRACT.md: `AzureStore::new` alongside `S3Store`/`GcsStore` in the walgit-store backends block, and the contract-suite env var (`WALGIT_TEST_AZURE_ACCOUNT`, container `WALGIT_TEST_AZURE_CONTAINER`). - AGENTS.md: §2.1 store list, D3 (`gcs/s3/azure/memory`, plus the two optional capabilities azure declines — `compose` and `accel_target`, so the byte-path fallbacks are what callers get), and the §5 first-class rule (azure runs the same suite via `just test-azure <account>`). - docs/ROUNDTRIPS.md: unchanged — it is a per-operation budget table with no backend enumeration, and this backend adds no protocol change. Also: tighten the `sas_string_to_sign_layout` doc comment — the 24-field layout is the one for exactly `sv` 2020-12-06 (the version SAS_VERSION pins), not "2020-12-06 and newer"; the live run proved newer versions sign more fields (28 at 2026-04-06). Matches the correction already made on `SasFields::string_to_sign`. And one rustfmt reflow in tests/contract.rs left behind by the previous commit, so `cargo fmt --check` is clean on the branch again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ents Three review findings, no behaviour change. MSRV. The workspace declares rust-version = "1.90", but the azure test code reached for `Duration::from_mins` / `from_hours`, both stabilized in 1.91. Rewritten as `from_secs` with the same values (5 min, 1 h, 24 h). Clippy was the reason they were written that way: `[workspace.package] rust-version` is not inherited by the member crates, so clippy assumed the current toolchain and `duration_suboptimal_units` actively suggested the 1.91 APIs — under a `-D warnings` gate, that is a lint pushing the tree past its own MSRV. Pinning `msrv = "1.90"` in clippy.toml settles it; 38 such suggestions across the workspace go quiet with it. Doc comments. `AzureStore::credential` still described "the accel/SAS paths to come" — SAS has landed and signs with the delegation key `service` fetches, not with a bearer token; the credential's one remaining job is the by-hand `Authorization` header on the delimited listing. `AzureStore::http` claimed "streaming GETs via SAS URLs" — the store hands signed URLs to the caller and never fetches one; that client serves the same delimited listing. Both now say what they do. `endpoint`. The config and example both offered the override "for Azurite/sovereign clouds", but the SDK rejects a non-https URL whenever a credential is attached and this backend always attaches one, so an http emulator cannot be reached. Both now claim only sovereign clouds and custom domains, and say the endpoint must be https. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Azure's uncommitted-block namespace is keyed by blob NAME, shared by every writer of that key, and `Put Block List` commits the most recently staged version of each id no matter who staged it. Block ids were bare zero-padded indices, so two concurrent `chunked_put`s of one key overwrote each other's staged blocks — and the winner's commit, with its CAS satisfied (neither writer had published anything), could publish the loser's bytes. Silent corruption on the flagship pack path, reachable whenever two writers race a >=64 MiB Create at different part sizes, as a rolled-out config change makes them; `publish.rs` already documents pack Create races as expected. `chunked_put` now draws one random nonce per upload and prefixes every block id with it: 16 lowercase hex chars (8 random bytes) plus the same 16-digit index, so ids stay uniform width — Azure's requirement — at 32 ASCII chars, well inside the 64-byte pre-base64 limit. Commit-list order still defines byte order, so nothing about the layout moves. The SDK's own managed uploader takes the same precaution with a UUID per block (`clients/block_blob_client.rs:274`). The module and `chunked_put` doc comments claimed more than the service gives: commit-time atomicity governs VISIBILITY only. Both now say that, and name the per-upload id uniqueness as the only thing isolating one upload's staged bytes from another's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… time out the REST client Three smaller azure-backend findings. `classify` mapped every 409 to `PreconditionFailed` without looking at the service's error code. That is right for the one 409 walgit races for — a lost `If-None-Match: *` create, `BlobAlreadyExists` — but wrong for the rest: `ContainerBeingDeleted` is transient container state, and a lease/snapshot/append conflict is a real fault. Filed as `PreconditionFailed` they made `coord::cas_update` spin on a race that never happened. 409 now consults `x-ms-error-code`: `ContainerBeingDeleted` is `Retryable`, any other named code is `Other`. A 409 with `BlobAlreadyExists` — or with no code at all, which means a body the SDK could not parse — keeps the live-verified CAS verdict, so the hot path is unchanged. `from_status` stays the coarse status table: its only caller is the delimited listing, which sends no conditional header and so can never be answered with a CAS-shaped 409. `put` chunked at `len >= multipart_threshold` while `s3.rs` chunks at `len >` and both modules' docs say "above"; a body of exactly the threshold took the staged-block path on Azure and the single PUT on S3. Now `>` on both, and the two Azure doc comments say "above". The supplemental `reqwest` client behind the `list_prefixes` REST loop was built with no timeouts, so a black-holed connection wedged the caller forever — it rides no SDK pipeline and inherits none. Added a 10s connect timeout and a 60s request timeout; both surface as `reqwest` errors, which that path already maps to `Retryable`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AlvinStanescu
force-pushed
the
feat/azure-blob-store
branch
from
September 1, 2026 13:42
0c2af7f to
e28b3a7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds
backend = "azure"next to s3 and gcs, using the officialazure_storage_blobcrate with Entra auth only (dev tool credentials, client secret, managed identity, or workload identity).CAS maps over cleanly: versions are ETags, creates use
If-None-Match: *(Azure answers a lost create race with 409 instead of 412, so that's normalized), updates useIf-Match. Large objects stage blocks and do the CAS check at commit; block ids carry a per-upload random prefix because Azure shares the uncommitted-block namespace between concurrent writers on the same blob name. LFS can also hand out user-delegation SAS URLs (serve_via = "signed_url") so big files bypass the server entirely.Two quirks worth knowing: the SDK has no delimiter listing yet, so
list_prefixesmakes one supplemental REST call with the same credential, and SAS signing pinssv=2020-12-06since newer service versions changed the string-to-sign layout.Testing done:
just test-azure <account> [container]runs the live suite if you have credentials; everything else is unit-tested so that the CI does not need an Azure connection.