aws/request: Fix for Go 1.8 request incorrectly sent with body #991

jasdel · 2016-12-09T21:14:23Z

Go 1.8 tightened and clarified the rules code needs to use when building
requests with the http package. Go 1.8 removed the automatic detection
of if the Request.Body was empty, or actually had bytes in it. The SDK
always sets the Request.Body even if it is empty and should not actually
be sent. This is incorrect.

Go 1.8 did add a http.NoBody value that the SDK can use to tell the http
client that the request really should be sent without a body. The
Request.Body cannot be set to nil, which is preferable, because the
field is exported and could introduce nil pointer dereferences for users
of the SDK if they used that field.

This change also deprecates the aws.ReaderSeekerCloser type. This
type has a bug in its design that hides the fact the underlying reader
is not also a seeker. This obfication, leads to unexpected bugs. If using
this type for operations such as S3.PutObject it is suggested to use
s3manager.Upload instead. The s3manager.Upload takes an io.Reader
to make streaming to S3 easier.

Related golang/go#18257

Fix #984

Go 1.8 tightened and clarified the rules code needs to use when building requests with the http package. Go 1.8 removed the automatic detection of if the Request.Body was empty, or actually had bytes in it. The SDK always sets the Request.Body even if it is empty and should not actually be sent. This is incorrect. Go 1.8 did add a http.NoBody value that the SDK can use to tell the http client that the request really should be sent without a body. The Request.Body cannot be set to nil, which is preferable, because the field is exported and could introduce nil pointer dereferences for users of the SDK if they used that field. Related golang/go#18257 Fix aws#984

jasdel · 2016-12-10T01:03:34Z

aws/request/request_resetbody.go

+			r.Error = awserr.New("SerializationError", "failed to seek request body", err)
+			return
+		}
+		_, err = r.Body.Seek(r.BodyStart, io.SeekStart)


Remove copy/paste error handling?

I'm thinking adding a util function that does all this and returns the size of the body.

yeah included this in the refactor

jasdel · 2016-12-10T01:04:24Z

aws/request/request_resetbody_1.7_test.go

+	r.ResetBody()
+
+	if v, ok := r.HTTPRequest.Body.(*offsetReader); !ok || v == nil {
+		t.Errorf("expected request body to be set to reader, got %#v",


Update error messages to be more obvious what failed. This and other tests added.

xibz

Few comments

xibz · 2016-12-12T20:56:27Z

aws/request/request_resetbody.go

+func resetBody(r *Request) {
+	r.safeBody = newOffsetReader(r.Body, r.BodyStart)
+
+	seekable := false


seekable := true

xibz · 2016-12-12T20:56:36Z

aws/request/request_resetbody.go

+		seekable = v.IsSeeker()
+	case *aws.ReaderSeekerCloser:
+		seekable = v.IsSeeker()
+	default:


Can get rid of this

xibz · 2016-12-12T21:08:06Z

aws/request/request_resetbody.go

+			r.Error = awserr.New("SerializationError", "failed to seek request body", err)
+			return
+		}
+		_, err = r.Body.Seek(r.BodyStart, io.SeekStart)


I'm thinking adding a util function that does all this and returns the size of the body.

xibz · 2016-12-12T21:14:54Z

aws/request/request_resetbody.go

+// of the SDK if they used that field.
+//
+// Related golang/go#18257
+func resetBody(r *Request) {


How do we feel about returning an error? I feel like when we get an error in here, it'll at least clean it up some, rather than mutating the request's error.

Ended up refactoring this logic simplifying this

xibz · 2016-12-12T21:15:59Z

aws/request/request_resetbody_test.go

+	"testing"
+)
+
+func TestResetBody_WithBodyContents(t *testing.T) {


Do we have tests to test the non-seekable case?

pin · 2016-12-13T20:56:21Z

We have run a bunch of S3-related tests on 0a88972 with go1.8beta1 and it seems that it works correctly.

In current version we see issues you mentioned in #984 (comment): "0" symbol in the body where body is supposed to be empty (e.g. HEAD request) appears because net/http applies chunked encoding on []byte(""): zero is just a length of the first chunk.

jasdel · 2016-12-13T21:07:40Z

@pin thanks for testing against this change. I think you're correct. The SDK pre-sets the request to a io.ReadSeeker, and later populates this with connects. The SDK was relying on a undocumented feature in Go where Go's http client would figure out if the request should be sent with a request body or not based on the length of the body.

In the case of the SDK, AWS services do not seem to like GET, HEAD, or DELETE HTTP requests with chunked encoding bodies, even if those body chunks are 0 length. The request will fails with a timeout. I'm working on reaching out internally to see if that issue can be addressed in a broader way server side to ignore the body instead of timing out the socket. But, for now this change should fix the SDK's behavior.

xibz

Just the one comment

xibz · 2016-12-12T23:09:17Z

aws/request/request.go

+		return
+	}
+
+	if l == 0 {


👍 this looks a lot better

xibz · 2016-12-13T21:31:55Z

aws/signer/v4/v4.go

@@ -321,7 +331,7 @@ func (v4 Signer) signWithBody(r *http.Request, body io.ReadSeeker, service, regi
 	// If the request is not presigned the body should be attached to it. This
 	// prevents the confusion of wanting to send a signed request without
 	// the body the request was signed for attached.
-	if !ctx.isPresign {
+	if !v4.DisableRequestBodyOverwrite && !ctx.isPresign {


!(v4.DisableRequestBodyOverwrite || ctx.isPresign)

Don't know if the Go compiler does De Morgan's laws under the hood.

updated thanks

Adds resolver function for loading S3 client specific config

jasdel added 3 commits December 9, 2016 13:07

add docs

6b1f0ee

deprecate aws.ReaderSeekerCloser

bdda5b9

jasdel requested a review from xibz December 10, 2016 00:00

jasdel added the Review Needed label Dec 10, 2016

jasdel commented Dec 10, 2016

View reviewed changes

xibz suggested changes Dec 12, 2016

View reviewed changes

jasdel added 5 commits December 12, 2016 14:36

simplified implementation

e2d64a0

Fix usage of io.Seek constances not added until go 1.7

1ef9dd8

ensure the request body is in a good state after signing

0a88972

Add tests for signer option to not replace body

ebae57f

Add test for exlucde body by method

52df8ab

jasdel removed the Review Needed label Dec 13, 2016

Add lifecycle test

c9f262e

xibz suggested changes Dec 13, 2016

View reviewed changes

jasdel added 2 commits December 13, 2016 14:01

Add round trip test for each request method

d18e226

Fixup signer disable overwrite body condition

9113cb5

jasdel merged commit db7b699 into aws:master Dec 13, 2016

jasdel deleted the fix/requestBody branch December 13, 2016 23:40

jasdel mentioned this pull request Dec 13, 2016

service/route53: ListHostedZones hangs and then fails with go1.8 #984

Closed

awstools mentioned this pull request Dec 15, 2016

v1.6.3 #999

Merged

xibz mentioned this pull request Jan 26, 2017

S3 PUT Copy fails with go 1.8 #1058

Closed

titanous mentioned this pull request Aug 16, 2017

Update S3 client flynn/flynn#4213

Merged

jasdel mentioned this pull request Jun 24, 2019

SDK's http.Request Body contains a value when it should be nil aws/aws-sdk-go-v2#318

Closed

skotambkar added a commit to skotambkar/aws-sdk-go that referenced this pull request May 20, 2021

Merge pull request aws#991 from aws/s3-external-config

15e052d

Adds resolver function for loading S3 client specific config

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws/request: Fix for Go 1.8 request incorrectly sent with body #991

aws/request: Fix for Go 1.8 request incorrectly sent with body #991

jasdel commented Dec 9, 2016 •

edited

Loading

jasdel Dec 10, 2016

xibz Dec 12, 2016

jasdel Dec 12, 2016

jasdel Dec 10, 2016

jasdel Dec 12, 2016

xibz left a comment

xibz Dec 12, 2016

xibz Dec 12, 2016

xibz Dec 12, 2016

xibz Dec 12, 2016

jasdel Dec 12, 2016

xibz Dec 12, 2016

jasdel Dec 12, 2016

pin commented Dec 13, 2016

jasdel commented Dec 13, 2016

xibz left a comment

xibz Dec 12, 2016

xibz Dec 13, 2016

jasdel Dec 13, 2016

aws/request: Fix for Go 1.8 request incorrectly sent with body #991

aws/request: Fix for Go 1.8 request incorrectly sent with body #991

Conversation

jasdel commented Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xibz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pin commented Dec 13, 2016

jasdel commented Dec 13, 2016

xibz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasdel commented Dec 9, 2016 •

edited

Loading