Skip to content

Fix flex checksum validation cfg #2981

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

Merged
merged 10 commits into from
Jan 24, 2025
Merged

Conversation

wty-Bryant
Copy link
Contributor

@wty-Bryant wty-Bryant commented Jan 22, 2025

Operations supporting output checksum validation (e.g. s3.GetObject) would only return checksum if user enable that in request. This PR fixes flex checksum's validation cfg to enable validation by default

It has been tested that getting a s3 object uploaded with checksum will validate checksum from response by default, so there should be no warning like WARN Response has no supported checksum. Not validating response payload.. Sample test code is shown below:

import (
	"bytes"
	"context"
	"io"

	"github.com/aws/aws-sdk-go-v2/aws"
	"github.com/aws/aws-sdk-go-v2/config"
	"github.com/aws/aws-sdk-go-v2/service/s3"
)

func main() {
	cfg, err := config.LoadDefaultConfig(context.Background())
	if err != nil {
		panic(err)
	}

	svc := s3.NewFromConfig(cfg)
	_, err = svc.PutObject(context.Background(), &s3.PutObjectInput{ // should default to crc32
		Bucket: aws.String("your-bucket"),
		Key:    aws.String("sdk-upload-crc32"),
		Body:   bytes.NewReader([]byte("hello")),
	})
	if err != nil {
		panic(err)
	}

	out, err := svc.GetObject(context.Background(), &s3.GetObjectInput{ // should default to crc32
		Bucket: aws.String("your-bucket"),
		Key:    aws.String("sdk-upload-crc32"),
	})
	if err != nil {
		panic(err)
	}
	defer out.Body.Close()
	if _, err := io.Copy(io.Discard, out.Body); err != nil {
		panic(err)
	}
}

@wty-Bryant wty-Bryant requested a review from a team as a code owner January 22, 2025 19:45
Copy link
Contributor

@lucix-aws lucix-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include sample code that tests this and demonstrates the desired outcome in the PR description.

@Madrigal
Copy link
Contributor

to double check, if an object is uploaded with CRC64, enabling response validation should still succeed and we have a test for it, right?

{
"id": "df93fff8-f662-441f-a12d-ac87614a5064",
"type": "bugfix",
"description": "Add a setter to output checksum middleware cfg to enable request validation mode by default",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this setter something that customers can interact with? If not, I'd just leave it as "Enable request validation mode by default"

@lucix-aws
Copy link
Contributor

to double check, if an object is uploaded with CRC64, enabling response validation should still succeed and we have a test for it, right?

Seconded. In fact, we need integration tests for everything here. Please demonstrate all of the following in integration tests:

  • round-tripping an object succeeds and defaults to crc32
  • downloading an object that has a crc64 checksum succeeds, and logs skipped-validation warning
    • since we don't support it natively, you can do a crc64 checksum by precomputing one and just attaching it to the PutObject call
  • uploading an object with no checksum and then downloading it through the Go SDK succeeds

@lucix-aws
Copy link
Contributor

We may have tests already that cover some of those cases, just point to them it so

ChecksumCRC64NVME: aws.String("uxBNEklueLQ="),
},
expectPayload: []byte("Hello, precomputed checksum!"),
expectComputedChecksums: &s3.ComputedInputChecksumsMetadata{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is a bit of a misnomer since we don't really calculate anything, but ¯_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just got that checksum from previous old manual test :(

@wty-Bryant wty-Bryant merged commit cb98dee into main Jan 24, 2025
13 checks passed
@wty-Bryant wty-Bryant deleted the feat-fix-checksum-validation-cfg branch January 24, 2025 18:27
@RJKeevil
Copy link

Hello @wty-Bryant, this worked perfectly for me for single part gets, but i still get the warnings for multipart gets. With a debugger i can see that the CRC32 header is returned for small files but not for large/multipart files, triggering the warning. I am just using a library that calls _, err = downloader.Download(ctx, writer, input) and cannot see any missing options etc that would enable checksums in multipart, is it possible to add a larger file to your tests to confirm?

@anishsana
Copy link

anishsana commented Jan 27, 2025

@wty-Bryant This change is causing my application to constantly throw the following warning -

SDK 2025/01/27 20:27:25 WARN Response has no supported checksum. Not validating response payload.
SDK 2025/01/27 20:27:25 WARN Response has no supported checksum. Not validating response payload.
SDK 2025/01/27 20:27:25 WARN Response has no supported checksum. Not validating response payload.
SDK 2025/01/27 20:27:25 WARN Response has no supported checksum. Not validating response payload.
SDK 2025/01/27 20:27:25 WARN Response has no supported checksum. Not validating response payload.

Is there any way to disable this validation to prevent the warning from being thrown?

@RJKeevil
Copy link

@anishsana you probably need to set the env var AWS_RESPONSE_CHECKSUM_VALIDATION="WHEN_REQUIRED" in the meantime

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.

5 participants