Add CRC-64/NVME transactional checksum support for StageBlock#2644
Add CRC-64/NVME transactional checksum support for StageBlock#2644mcroomp wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds CRC-64/ECMA-182 transactional checksum support for the Blob StageBlock operation, enabling request validation via x-ms-content-crc64 and echoing the validated checksum back in the response.
Changes:
- Added CRC64 utilities (
getCRC64FromString,getCRC64FromStream) plus a combinedcomputeTransactionalChecksumshelper. - Updated
BlockBlobHandler.stageBlockto validatetransactionalContentCrc64and returnx-ms-content-crc64when provided. - Added unit/integration tests covering CRC64 correctness and StageBlock success/failure behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/blob/utils.test.ts | Adds CRC64 unit tests (known-check value, stream parity, chunk-boundary invariance). |
| tests/blob/apis/blockblob.test.ts | Adds StageBlock tests for CRC64 success, mismatch failure, and absence of CRC64 response header. |
| src/common/utils/utils.ts | Introduces CRC-64/ECMA-182 implementation and a combined MD5+CRC64 streaming helper. |
| src/blob/middlewares/StrictModelMiddlewareFactory.ts | Allows x-ms-content-crc64 header by removing it from the strict-model unsupported list. |
| src/blob/handlers/BlockBlobHandler.ts | Validates transactional CRC64 for StageBlock and conditionally returns x-ms-content-crc64 in the response. |
Comments suppressed due to low confidence (1)
src/blob/handlers/BlockBlobHandler.ts:7
- There are now two separate imports from the same module (
../../common/utils/utils): one forconvertRawHeadersToMetadataand another for the rest. Consider consolidating into a single import to avoid duplication and keep imports easier to maintain.
import { convertRawHeadersToMetadata } from "../../common/utils/utils";
import {
computeTransactionalChecksums,
getMD5FromStream,
getMD5FromString,
newEtag
} from "../../common/utils/utils";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mcroomp Is StageBlock the first step of CRC64 support rollout, or is there a service-compatibility reason this checksum should only be supported on Put Block and not on other upload APIs yet? |
…r. Also added logic to crc PutBlock and AppendBlock
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/blob/middlewares/StrictModelMiddlewareFactory.ts:27
- Removing
x-ms-content-crc64from the global unsupported-header blocklist means requests can now include this header on operations that still ignore it (e.g., PageBlobUploadPagesincludestransactionalContentCrc64in the generated spec, butPageBlobHandlerhas no CRC64/MD5 transactional validation). This will cause Azurite to acceptx-ms-content-crc64without validating it, which violates the Azure REST contract and can give callers a false sense of integrity checking. Consider either (1) implementing CRC64 validation for the other operations that accept this header (at leastPageBlobUploadPages), or (2) making unsupported-header validation operation-aware sox-ms-content-crc64is only allowed where it is actually implemented.
const UnsupportedHeaderKeys = [
HeaderConstants.X_MS_RANGE_GET_CONTENT_CRC64,
HeaderConstants.X_MS_ENCRYPTION_KEY,
HeaderConstants.X_MS_ENCRYPTION_KEY_SHA256,
HeaderConstants.X_MS_ENCRYPTION_ALGORITHM
];
Ok I implemented PutBlock and AppendBlock as well. I also added an env variable so we can run the test cases against real Azure to make sure the behavior is the same. I identified a couple gaps from this in the md5/crc behavior. |
Verified against real Azure: a 4-byte Content-MD5 header is rejected with InvalidMd5 (not Md5Mismatch). The new test guards stageBlock against silently regressing this code, complementing the existing wrong-value test that asserts Md5Mismatch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Live probe showed real Azure returns InvalidMd5 (not InvalidHeaderValue) for wrong-length x-ms-blob-content-md5 on Put Blob. Drop Azurite's explicit InvalidHeaderValue path and let the unified transactional validator handle all three MD5 sources (Content-MD5, transactionalContentMD5, x-ms-blob-content-md5) the same way. Also adds a CRC64 wrong-length test (pins the existing InvalidHeaderValue path) and a Put Blob wrong-length-blob-content-md5 test (pins the new InvalidMd5 path). Both verified live. Clarifies tests/testutils.ts JSDoc to acknowledge http:// connection strings instead of claiming HTTPS is always used in live mode. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
One small change since the review due to copilot comment, added the InvalidMd5 behavior checks to all cases and ensured we are consistent with live azure. Closed the rest of the copilot low confidence comments that werent helpful. |
…vely against live azure to ensure identical behavior
Test improvements