fix(s3): remove ChecksumAlgorithm

[michael-todorovic]

Fixes #602 by checking ChecksumAlgorithmSha256 support on the s3 bucket
The check is made only on datastore startup

[LucasMrqes]

Looks like it was introduced in #588, thanks for catching all bugs related to on-prem S3 ! We might add some test suites with several S3 backends to catch this kind of issues

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 44.92%. Comparing base (6ebc0ea) to head (07c1f38).

Files with missing lines	Patch %	Lines
internal/datastore/storage/s3/s3.go	0.00%	3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #603   +/-   ##
=======================================
  Coverage   44.91%   44.92%           
=======================================
  Files          79       79           
  Lines        5762     5761    -1     
=======================================
  Hits         2588     2588           
+ Misses       2959     2958    -1     
  Partials      215      215           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[michael-todorovic]

You're welcome :) I was thinking about adding some unit tests around this but I have nothing easy right now 😄

To me, a mock would unlikely fit in such case.
I can see there are several tools to mimic CSP buckets that could be tried. For on prem, I'd say an ephemeral container for ceph+minio (major on-prem providers) would be good enough.

Wdyt? If that sounds like a plan, I can try to build the tests at some point 😄

[corrieriluca]

Indeed, this bug was introduced with #588.

To be honest I don't think the right fix is to check is the SHA256 algorithm is present with a fake object Put/Delete at each startup of the datastore...

I checked in the code, turns out we don't really use the checksum of object storage backends in any way.

In #588 I added the below lines because I wanted to make S3 on pair with GCS and Azure backends (which automatically provide MD5 checksums of objects, in my understanding):

burrito/internal/datastore/storage/s3/s3.go

Lines 91 to 96 in 765acea

	// S3 returns a checksum only if the object was uploaded with one
	if result.ChecksumSHA256 == nil {
	return make([]byte, 0), nil
	}
	return []byte(*result.ChecksumSHA256), nil

But since in the datastore client we don't read this value and only return a boolean, I think we should remove completely the usage of the ChecksumAlgorithm parameter in the PutObjectInput structure:

burrito/internal/datastore/storage/s3/s3.go

Line 104 in 765acea

ChecksumAlgorithm: types.ChecksumAlgorithmSha256,

This will ease the implementation of the S3 backend and make it (I hope) universally compliant to multiple S3 implementations.

WDYT @michael-todorovic @LucasMrqes ?

I agree on the need to test Ceph and MinIO in ephemeral containers though 😄

[michael-todorovic]

With Terraform 1.11.2, they bumped the aws sdk which has the same exact feat and it broke with our ceph with the same log 😄
I don't know why aws added it but maybe it's worth checking why.
If the sha256 thing is the future at aws, I think it could be kept in burrito and controlled through a config instead of the automated check :)

[michael-todorovic]

It seems like they're not planning to change anything anytime soon. I'll update this PR then :)