Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add checksum validation on artifact upload #1063

Merged
merged 12 commits into from May 19, 2022
Merged

Conversation

robherley
Copy link
Member

@robherley robherley commented Apr 26, 2022

Relevant Issues:

tl;dr adds base64 encodings of the CRC64 and MD5 to the headers of a chunk upload:

x-actions-results-crc64
x-actions-results-md5

Recently we've been running into issues when artifacts are getting corrupted during the upload process. This is extremely rare (seen < 1% during my tests) but happens nonetheless. There still needs to be more investigation as to why these are being corrupted, but we've narrowed the problem areas to be between the upload and file container service handler after isolated testing with md5 checksum headers.

This PR adds a CRC64 and MD5 checksum to the header. Since NodeJS's crypto library (openssl bindings) doesn't have CRC64, I added a simple implementation based on Go's hash/crc64 pkg. Also this is tailored to the CRC polynomial used by azure storage. We're already using this same polynomial places deeper in the stack.

I ran some benchmarks and this CRC64 implementation is a bit faster than sha256/md5:

crc64 x 145,627 ops/sec ±8.33% (79 runs sampled)
md5 x 76,392 ops/sec ±12.44% (69 runs sampled)
sha256 x 65,982 ops/sec ±14.55% (62 runs sampled)
Fastest is crc64

@robherley robherley force-pushed the robherley/artifact-digest branch from eee8fbe to d5c547c Compare Apr 27, 2022
@robherley robherley marked this pull request as ready for review May 19, 2022
@robherley robherley requested a review from as a code owner May 19, 2022
Copy link
Contributor

@yacaovsnc yacaovsnc left a comment

:shipit:

@robherley robherley merged commit a708045 into main May 19, 2022
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants