Migrate Azure blob storage to azure_storage_blob 1.0.0 - #6693
siva-abstract-security wants to merge 14 commits into
Conversation
Move the workspace off the legacy Azure SDK and onto the 1.0 line: azure_core 1.1, azure_identity 1.0 and azure_storage_blob 1.0. `azure_storage` is dropped outright. The rewritten SDK has no successor for `StorageCredentials`, `CloudLocation` or `ConnectionString`, so the concepts it provided have to be rebuilt on top of the pipeline instead of renamed. Feature names changed with the rewrite: `enable_reqwest_rustls` is now `reqwest_rustls`, and the `azurite_workaround` features no longer exist, so they leave `integration-testsuite`. `hmac_rust` survives in `azure_core` 1.1, which matters because a shared key signing policy needs it. The new SDK resolves to a smaller graph: `Cargo.lock` loses 296 lines net. This commit only moves the dependencies. `quickwit-storage` does not build against them yet.
…election Two capabilities the rewritten Azure SDK no longer provides, added ahead of porting the blob storage backend itself. `azure_shared_key` signs requests with the storage account key. The 1.0 SDK authenticates with Entra ID tokens only, and the SDK team has said shared key support will not return (Azure/azure-sdk-for-rust#2975). Quickwit documents `azure.access_key` as a supported credential, and Azurite accepts shared key only, so the signing `azure_storage` 0.21 used to provide lives here now. The policy runs per retry, because the service rejects an `x-ms-date` that has drifted more than fifteen minutes and a retried request would otherwise carry a stale timestamp. `azure_credentials` replaces `azure_identity::create_credential()`, which no longer exists: the 1.0 line removed `DefaultAzureCredential` along with it, and the remaining `DeveloperToolsCredential` chains the two CLIs only. Workload identity is chosen when all three variables the webhook injects are present, managed identity otherwise, and `AZURE_CREDENTIAL_KIND` still pins the choice explicitly. The container client is built here too, since 1.0 clients take a container URL rather than an account name plus a cloud location, which removes the special case a sovereign endpoint used to need. Both modules compile and carry unit tests. The backend in `azure_blob_storage.rs` is not ported yet, so the crate still does not build.
Rewrites `AzureBlobStorage` against the 1.0 clients and deletes the last references to the legacy SDK, so `quickwit-storage` builds again. The client mapping is mostly mechanical: `ContainerClient` becomes `BlobContainerClient`, `put_block_blob` becomes `BlockBlobClient::upload`, `put_block` and `put_block_list` become `stage_block` and `commit_block_list`, and `list_blobs` yields a `Pager` rather than a `Pageable`. Two places needed more thought. Downloads no longer walk a page of chunk responses. `BlobClient::download` returns one result whose `body` is a stream, so `copy_to` and `get_slice_stream` share a single `get_to_reader` helper that pulls the first chunk before returning. That keeps an error arriving with the response headers inside the retry rather than handing it to a caller with no way to retry. `BlockBlobClient` is not `Clone` in 1.0, so each part of a multipart upload builds its own client from the container client. Construction is local: the pipeline is behind an `Arc` and only the URL differs. Two behaviour notes. Single part upload sets `blob_content_md5` rather than a transactional checksum, because the partitioned upload path does not expose one, so the digest is stored with the blob instead of verified per request. Multipart still checks per block via `stage_block`. `ClientBuilder::emulator()` is gone, and the SDK could not have kept it, since the emulator authenticates with a shared key. The integration test now asks this crate to create and delete its container so the signing policy stays internal. 85 unit tests pass, clippy is clean, and `quickwit-cli` builds under `release-feature-set`.
Verified against Azurite, which rejected `commit_block_list` with `AuthorizationFailure` while every other operation authorized fine. The generated operations are inconsistent about `Content-Length`. `stage_block` inserts the header itself, so signing saw it. `commit_block_list` leaves it to the transport, so signing saw nothing and covered an empty length while the wire carried the real one, and shared key rejects that mismatch. The response says only that the signature is malformed, so the operation-specific nature of the failure is invisible from the error. Set the header before signing when the body length is known and the header is absent, which makes the signature and the wire agree whatever the operation did. A zero length body still signs an empty slot, as the specification requires from API version 2015-02-21 onwards. Two tests cover it through `Policy::send` with a capturing terminal policy, one for the header being added and one for an existing value being left alone. Removing the fix fails the first and nothing else. Azurite needed two changes to run the suite at all. The pinned 3.24.0 predates the API version `azure_storage_blob` 1.0 sends, and so does 3.36.0, the newest released, so the emulator now runs with `--skipApiVersionCheck`. Skipping the check keeps the emulated client sending the same request as production, which is what the signing needs to be tested against, rather than pinning an older API version only the tests would use.
siva-abstract-security
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f8944b283
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`BlockBlobClient::upload()` only exposes `blob_content_md5`, which the service stores as a property without checking it against the body, so the port had quietly dropped the integrity check `put_block_blob(..).hash(..)` used to give us. Splits are immutable and never re-verified, so a corrupted upload would have been permanent and silent. Stage a single block and commit it instead. `stage_block` takes a transactional checksum, so the service rejects a payload that does not match on arrival. The cost is one extra request per object below the multipart threshold, which is the cheaper side of this trade. Also pins the path encoding in the canonicalized resource, which a review question prompted me to check. The path is signed exactly as the URI carries it, escapes and all, while query parameters are decoded. That asymmetry is specified: "any portion of the CanonicalizedResource string that is derived from the resource's URI should be encoded exactly as it is in the URI", and the query steps separately say to URL-decode each name and value. Decoding the path instead is rejected with `AuthorizationFailure`, confirmed against Azurite. Two tests now hold that shape in place so it does not get tidied away later.
Two credential regressions from replacing `azure_identity::create_credential()`, both raised in review. The old chain tried an environment credential before managed identity, reading `AZURE_TENANT_ID`, `AZURE_CLIENT_ID` and `AZURE_CLIENT_SECRET`. The replacement recognized only workload identity and fell through to managed identity, so a deployment authenticating with a service principal had its secret ignored and its request sent to IMDS. Those deployments would have lost Azure access on upgrade, with an error naming neither the secret nor the reason. Separately, `AZURE_CLIENT_ID` on its own names a user-assigned managed identity. Passing no options asks IMDS for the system-assigned identity, which is either absent or, on a host carrying both, the wrong principal. The client id now becomes `UserAssignedId::ClientId`. Selection moved into `select_token_credential_kind`, which takes a lookup function rather than reading the process environment, so precedence is covered by ordinary tests. Mutating environment variables inside a test is not safe while other tests run, and precedence between overlapping variable sets is exactly what needs pinning: a secret beats a federated token file, a partial set of either falls back rather than half-configuring a credential, and a blank value counts as unset. `TokenCredentialKind` carries the secret, so its `Debug` is hand written to redact it. A derived one would print the secret, and this type appears in test failure output.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/quickwit-oss/quickwit/blob/abb142913b283f85fdb5b46d04eb77bf8281a6a0/quickwit-storage/src/object_storage/azure_blob_storage.rs#L349
When two small put calls target the same blob concurrently, both stage their payload under the fixed block:00000 ID. Because uncommitted blocks are keyed by blob and block ID, the later stage replaces the earlier payload; the earlier commit can then report success after publishing the other writer's bytes, while the remaining commit may fail because its uncommitted block was consumed. The previous single-request upload path did not introduce this cross-write race, so generate a unique block ID once per upload, outside the retry closure.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b4467799c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let content: RequestContent<Bytes, NoFormat> = data.into(); | ||
| block_blob_client | ||
| .stage_block( | ||
| SINGLE_PART_BLOCK_ID, |
There was a problem hiding this comment.
When two callers upload to the same blob concurrently, both stage data under SINGLE_PART_BLOCK_ID; Azure replaces an uncommitted block when the same ID is staged again, so one caller can commit the other caller's payload and still report success. The storage wrapper does not serialize puts, and the previous single-request upload did not have this cross-request collision, so generate a per-upload fixed-length block ID and commit that ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is likely not an issue for quickwit since split names are unique and retries are sequential but it's a one line fix that ensures the last writer wins so I think its worth adopting anyway
There was a problem hiding this comment.
fixed in cfee6c7 - block ids are {upload ULID}:{part:05} now, minted before the retry loop, and multipart got the same treatment since it had the same shape and would have ended up a different id length otherwise.
| // An empty or unrecognized value falls through to detection. | ||
| _ => select_token_credential_kind(|name| env::var(name).ok()), |
There was a problem hiding this comment.
When an existing environment sets AZURE_CREDENTIAL_KIND=azurecli, which the previous azure_identity::create_credential() path supported, this wildcard now treats it as unrecognized and selects a credential from the other environment variables—typically managed identity. Developer and CI machines authenticated through az login then contact IMDS instead and lose Blob access; handle the Azure CLI/developer-tools credential explicitly.
Useful? React with 👍 / 👎.
| let content_length = response | ||
| .content_length() | ||
| .map_err(AzureErrorWrapper::from)? | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
When a successful Blob Properties response lacks Content-Length—for example from a faulty proxy or Azure-compatible endpoint—this reports the object as zero bytes instead of rejecting the invalid response. Callers use file_num_bytes to size split and source downloads, so the fallback can turn a malformed response into incorrect processing; convert None into a storage error rather than 0.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
dayaffe
left a comment
dayaffe
left a comment
There was a problem hiding this comment.
Thanks for this contribution!
I tested it against a real Azure account with 800k documents using both access-key and service-principal auth, and everything worked well.
I also confirmed the fix for #6672 directly. I ran one credential for the process lifetime, sped up the clock so the 24h token expiry hit in minutes, and verified that the refresh re-read the token file and carried on. The same setup on the old SDK fails with AADSTS700024, matching the issue.
I found one problem: test_exists fails against real Azure because Put Block rejects empty blocks with 400 InvalidHeaderValue. Azurite accepts them, so the suite still passes locally. This fixed it for me in put_single_part:
if data.is_empty() {
let block_list_content = RequestContent::try_from(BlockLookupList::default())
.map_err(AzureErrorWrapper::from)?;
block_blob_client
.commit_block_list(block_list_content, None)
.await?;
return Result::<(), AzureErrorWrapper>::Ok(());
}Everything else looked good from my testing.
Raised by review on the single part path, which the port moved from an atomic
`put_block_blob` to a stage-then-commit pair under a constant block id.
Uncommitted blocks are keyed by blob name and block id, so two concurrent
uploads of the same object shared a block. The second stage replaced the first
upload's bytes, then the first commit published them under its own name and
consumed the block the second commit was still waiting to name, so one caller
returned success over the other caller's payload and the other failed with an
invalid block list. The atomic upload it replaced could not do this, so it came
in with the port.
The multipart path had the same shape already, from `block:{num:05}` alone, and
gets the same treatment. Leaving it would also have split the two paths apart on
id length, which matters: Azure requires one length across the block ids of a
single blob, and an uncommitted block outlives the upload that staged it by a
week, so a single part upload that stages and never commits would poison a later
multipart upload to the same name.
Ids are now `{upload ULID}:{part:05}`. Fixed length, unique per upload, and
still sortable by part within an upload, which is what the block list needs. The
single part id is minted before the retry loop rather than inside it, so a retry
re-stages over its own block instead of leaving one behind.
Found in review against a real Azure account, where `test_exists` failed with `400 InvalidHeaderValue`. `Put Block` will not take a zero length block, so the port broke empty objects when it moved the single part path off the atomic `put_block_blob`, which accepted them. Azurite takes the empty block, so the suite stayed green locally and the failure only shows against the service. An empty payload now skips staging and commits an empty block list, which is how you name a zero length blob. Verified against Azurite that the blob is created, reads back empty and reports zero bytes, so the branch is exercised by `test_exists` on both. No new test: Azurite accepted the broken form, so a test written here cannot fail on the case it is meant to catch. What would catch it is the suite running against a real account, which is the gap this bug came through.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 288d8b9c4f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "xml", | ||
| ] } | ||
| azure_identity = { version = "1", default-features = false } | ||
| azure_storage_blob = { version = "1", default-features = false } |
There was a problem hiding this comment.
Adding azure_storage_blob also introduces azure_core_macros and the typespec* crates, but LICENSE-3rdparty.csv was not regenerated: it still lists the removed azure_storage, azure_storage_blobs, and azure_svc_blobstorage packages and omits the new dependencies. This leaves the shipped third-party attribution incomplete; run make update-licenses and commit the generated inventory.
AGENTS.md reference: AGENTS.md:L104-L107
Useful? React with 👍 / 👎.
Trims the comments the two previous commits added, and drops a claim that was wrong: unique block ids do not make concurrent uploads of the same blob independent. `commit_block_list` garbage collects every uncommitted block on the blob, so the losing upload still fails with `InvalidBlockList`, which is not retryable. What the ids fix is the winner publishing someone else's bytes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4947cd4f32
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // An empty or unrecognized value falls through to detection. | ||
| _ => select_token_credential_kind(|name| env::var(name).ok()), |
There was a problem hiding this comment.
When AZURE_CREDENTIAL_KIND contains a typo or unsupported nonempty value, this wildcard silently ignores the explicit selector and falls back to environment detection, which can authenticate with a different service principal or managed identity instead of reporting the invalid configuration. Distinguish unset/empty values from unknown values and return InvalidConfig for the latter.
AGENTS.md reference: AGENTS.md:L19-L22
Useful? React with 👍 / 👎.
| /// The 1.0 SDK has no `emulator()` helper, and could not offer one: the emulator | ||
| /// authenticates with a shared key, which the SDK no longer signs. The well-known | ||
| /// account and key are documented at | ||
| /// <https://learn.microsoft.com/azure/storage/common/storage-use-azurite>. |
There was a problem hiding this comment.
| /// The 1.0 SDK has no `emulator()` helper, and could not offer one: the emulator | |
| /// authenticates with a shared key, which the SDK no longer signs. The well-known | |
| /// account and key are documented at | |
| /// <https://learn.microsoft.com/azure/storage/common/storage-use-azurite>. | |
| /// The well-known account and key are documented at: | |
| /// <https://learn.microsoft.com/azure/storage/common/storage-use-azurite>. |
| .put_block_blob(data) | ||
| .hash(hash) | ||
| .into_future() | ||
| let digest = md5::compute(&data[..]); |
There was a problem hiding this comment.
Any chance we can do CRC64-NVME?
There was a problem hiding this comment.
yep - done in 008f624, both stage_block sites. no new crate either, crc-fast was already in the tree via aws-smithy-checksums. shared key signing needed nothing since canonicalized_headers already picks up x-ms-* generically.
two things worth flagging - the 1.0 sdk's own block blob tests pin the right vector (V0JSBnCFdzM= for hello) but label it ECMA-182, which is a different polynomial. it's actually CRC-64/NVME little endian, so i pinned that in a unit test.
and azurite doesn't validate x-ms-content-crc64 at all - i swapped the checksum for 8 zero bytes and the suite still passed, where the old md5 path got rejected with Provided contentMD5 doesn't match. so the integration suite no longer covers this end to end, only the unit vector does. still on the "never run against a real azure account" pile.
| /// environment credential came before managed identity. Dropping that ordering silently | ||
| /// breaks every deployment that authenticates with a service principal, because the client | ||
| /// secret is ignored and IMDS is contacted instead. | ||
| fn select_token_credential_kind(var: impl Fn(&str) -> Option<String>) -> TokenCredentialKind { |
There was a problem hiding this comment.
| fn select_token_credential_kind(var: impl Fn(&str) -> Option<String>) -> TokenCredentialKind { | |
| fn select_token_credential_kind(var_fn: impl Fn(&str) -> Option<String>) -> TokenCredentialKind { |
| /// secret is ignored and IMDS is contacted instead. | ||
| fn select_token_credential_kind(var: impl Fn(&str) -> Option<String>) -> TokenCredentialKind { | ||
| let non_empty = |name: &str| match var(name) { | ||
| Some(value) if !value.trim().is_empty() => Some(value), |
There was a problem hiding this comment.
Don't we want to use the trimmed value?
There was a problem hiding this comment.
yeah good catch - we checked trim().is_empty() but stored the untrimmed value, so padding would go to Entra as part of the tenant/client id. fixed in 29cce17 plus a test. also unified the explicit managedidentity branch which wasn't trimming at all.
Values were checked for emptiness after trimming but stored untrimmed, so whitespace around AZURE_TENANT_ID, AZURE_CLIENT_ID or AZURE_CLIENT_SECRET reached Entra as part of the credential. The explicit managedidentity branch skipped trimming entirely; both paths now agree.
stage_block carried a transactional MD5. CRC64 is the stronger and cheaper check, and crc-fast is already in the tree via aws-smithy-checksums, so this adds no new third-party crate. x-ms-content-crc64 is CRC-64/NVME little endian. The 1.0 SDK's own block blob tests pin the right vector but label it ECMA-182, a different polynomial, so a unit test pins the algorithm against that server-confirmed vector. Azurite does not validate x-ms-content-crc64 (an all-zero checksum is accepted), unlike Content-MD5 which it does check, so the integration suite no longer covers the checksum end to end.
Still listed azure_storage, azure_storage_blobs and azure_svc_blobstorage and omitted azure_core_macros and the typespec crates.
3 participants
migrates azure blob storage to the rewritten sdk - azure_core 1.1, azure_identity 1.0, azure_storage_blob 1.0.
azure_storageis gone entirely and nothing replaces it. closes #6672.the upgrade fixes the bug on its own -
azure_identity1.0 re-reads the federated token file once the cached copy is >600s old, so the assertion at T+24h is fresh and the indexer stops dying. no workaround needed.two things 1.0 dropped that i had to rebuild:
1 - shared key signing. 1.0 does entra tokens only and MS says it's not coming back (Azure/azure-sdk-for-rust#2975). we document
access_keyand azurite speaks shared key only, soazure_shared_keydoes whatazure_storage0.21 used to.2 - credential selection.
create_credential()andDefaultAzureCredentialare both gone, soazure_credentialspicks workload vs managed identity from the env explicitly.one thing worth your attention - the generated ops disagree about
Content-Length.stage_blocksets the header,commit_block_listleaves it to the transport, so signing covered an empty length while the wire carried a real one and shared key rejected it asAuthorizationFailurewith nothing pointing at why. only azurite caught this - unit tests over the string construction didn't.azurite also needs
--skipApiVersionChecknow. 3.24.0 and even 3.36.0 (newest released) both predate the api version 1.0 sends. docker-compose carries that plus the image bump.green: 80 unit tests, the full azurite integration suite, clippy, and
quickwit-cliunderrelease-feature-set.what i don't know:
1 - haven't run this against a real azure account, only azurite. so managed identity and workload identity never actually executed, including the 24h refresh this closes.
2 - single part upload sets
blob_content_md5instead of a transactional checksum, since the partitioned upload path doesn't expose one - stored with the blob rather than checked per request. multipart still checks per block viastage_block.3 - whether
--skipApiVersionCheckis hiding a real incompatibility. azurite took every op the suite runs, but that flag stops it telling us what it doesn't implement.written with claude opus 5.