Conversation
siminsavani-msft
requested review from
gapra-msft,
jhendrixMSFT,
souravgupta-msft and
tasherif-msft
as code owners
ghost
added
the
jhendrixMSFT
left a comment
jhendrixMSFT
left a comment
There was a problem hiding this comment.
Might be worth doing a scrub to see if there are other places where we missed propagating option values.
|
|
||
| md5sum, resp, err := performUploadWithMD5Check(s.T(), _require, s.T().Name(), fileSize) | ||
|
|
||
| _require.ErrorContains(err, "400 The MD5 value specified in the request did not match with the MD5 value calculated by the server.") |
There was a problem hiding this comment.
Why are we getting this error for large file uploads?
There was a problem hiding this comment.
For larger files, the service won't be able to calculate the MD5 and this error is expected as the user provided MD5 and the response MD5 won't match. We don't get this error as of now because the user provided MD5 doesn't get passed at all.
There was a problem hiding this comment.
a transactional md5 will always be calculated by the service. If the service is failing the transactional md5 check it's because the check is actually failing. I imagine this is because you've configured the transactional md5 on put blocklist. A transactional md5 for that API is literally checking the content integrity of the blocklist itself (it's one of the only places .NET SDK currently doesn't calculate one).
Transactional md5 validates individual requests and nothing more, meaning it would need to validate individual blocks. If one large md5 is being passed in for a partitioned upload, the client should fail fast since the checksum cannot be split.
| Tags: o.Tags, | ||
| Metadata: o.Metadata, | ||
| Tier: o.AccessTier, | ||
| TransactionalContentMD5: o.TransactionalContentMD5, |
There was a problem hiding this comment.
There are two features, one is to just set the MD5 sum in the blob and other one is transactional MD5. For transactional options crc64 is also supported. Transactional fields are only for validation and will not set any properties in the blob. Just double check that we are good with all these combinations as I do not see transactional CRC here, just had doubts on we are still missing some things.
There was a problem hiding this comment.
We need to be careful about the way we chose to expose transactional MD-5 and CRC-64 to the customer. We will be implement end-to-end content validation in the Go SDK this fall, after the FE has implement trailing CRC-64 header.
This work will look something like this - Azure/azure-sdk-for-net#29778
We do not need to calculate CRC-64 or MD5 within the Go SDK at this time, but we need to design our public interfaces in a way that will support this option in the future.
There was a problem hiding this comment.
This is how UploadOptions looks in .NET - https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/storage/Azure.Storage.Blobs/src/Models/BlobUploadOptions.cs
It has UploadTransferValidationOptions as a property - https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/storage/Azure.Storage.Common/src/UploadTransferValidationOptions.cs
I recommend we do something similar in Go to future proof out public interfaces.
There was a problem hiding this comment.
Agree with Sean. In .NET we had to retire our byte[] contentHash arguments and replace them with a whole configuration setup. It would be best to avoid writing "instant legacy" APIs and this probably merits further offline discussion of how to balance immediate needs with medium-term plans for transactional content integrity.
There was a problem hiding this comment.
In .NET, if the customer populates TransferValidationOptions.PrecalculatedChecksum and TransferValidationOptions.ChecksumAlgorithm, the SDK uses their pre-calculated checksum. If the customer only populates TransferValidationOptions.ChecksumAlgorithm, the SDK calculates the checksum for the user.
We don't want to implement the SDK calculating the checksum for the user until ~6 months from now when the service supports trailing checksum footers, but we need to design our public interface to be compatible with this functionality.
There was a problem hiding this comment.
@jaschrep-msft, what should we do in in the case where the customer provides a precalculated checksum for a multi-part upload, before we implement calculating checksums in the SDK?
There was a problem hiding this comment.
We should fail fast. If a customer provides a transactional checksum that the SDK cannot make use of, we shouldn't pretend an integrity check has occurred when it actually hasn't.
In .NET, before we had SDK calculation on caller behalf, we didn't accept the parameter on any API that had the possibility of splitting an upload, as we didn't want customers to write code that suddenly broke once they passed a file size threshold they were not aware of. Customers had to use direct to REST apis if they wanted this feature.
6 participants
Fixes #20092
Adds MD5 from UploadFileOptions to UploadOptions and CommitBlockListOptions.