Skip to content

Commit a121095

Browse files
skotambkarskotambkar
authored
service/s3: fix behavior for 200 OK responses with no body (#549)
1 parent 241222e commit a121095

File tree

6 files changed

+203
-17
lines changed

6 files changed

+203
-17
lines changed

‎CHANGELOG_PENDING.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,4 @@ SDK Enhancements
99

1010
SDK Bugs
1111
---
12+
`service/s3`: Fix S3 client behavior wrt 200 OK response with empty payload

‎service/s3/s3manager/upload_internal_test.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,14 @@ import (
2020
"github.com/aws/aws-sdk-go-v2/service/s3/internal/s3testing"
2121
)
2222

23+
const respBody = `<?xml version="1.0" encoding="UTF-8"?>
24+
<CompleteMultipartUploadOutput>
25+
<Location>mockValue</Location>
26+
<Bucket>mockValue</Bucket>
27+
<Key>mockValue</Key>
28+
<ETag>mockValue</ETag>
29+
</CompleteMultipartUploadOutput>`
30+
2331
type testReader struct {
2432
br *bytes.Reader
2533
m sync.Mutex
@@ -81,7 +89,7 @@ func TestUploadByteSlicePool(t *testing.T) {
8189

8290
r.HTTPResponse = &http.Response{
8391
StatusCode: 200,
84-
Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
92+
Body: ioutil.NopCloser(bytes.NewReader([]byte(respBody))),
8593
}
8694

8795
switch data := r.Data.(type) {
@@ -177,7 +185,7 @@ func TestUploadByteSlicePool_Failures(t *testing.T) {
177185
r.Error = fmt.Errorf("request error")
178186
r.HTTPResponse = &http.Response{
179187
StatusCode: 400,
180-
Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
188+
Body: ioutil.NopCloser(bytes.NewReader([]byte(respBody))),
181189
}
182190
return
183191
}
@@ -252,7 +260,7 @@ func TestUploadByteSlicePoolConcurrentMultiPartSize(t *testing.T) {
252260

253261
r.HTTPResponse = &http.Response{
254262
StatusCode: 200,
255-
Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
263+
Body: ioutil.NopCloser(bytes.NewReader([]byte(respBody))),
256264
}
257265

258266
switch data := r.Data.(type) {
@@ -363,7 +371,7 @@ func BenchmarkPools(b *testing.B) {
363371

364372
r.HTTPResponse = &http.Response{
365373
StatusCode: 200,
366-
Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
374+
Body: ioutil.NopCloser(bytes.NewReader([]byte(respBody))),
367375
}
368376

369377
switch data := r.Data.(type) {

‎service/s3/s3manager/upload_test.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,14 @@ import (
2626
"github.com/aws/aws-sdk-go-v2/service/s3/s3manager"
2727
)
2828

29+
const respBody = `<?xml version="1.0" encoding="UTF-8"?>
30+
<CompleteMultipartUploadOutput>
31+
<Location>mockValue</Location>
32+
<Bucket>mockValue</Bucket>
33+
<Key>mockValue</Key>
34+
<ETag>mockValue</ETag>
35+
</CompleteMultipartUploadOutput>`
36+
2937
var emptyList []string
3038

3139
func val(i interface{}, s string) interface{} {
@@ -74,7 +82,7 @@ func loggingSvc(ignoreOps []string) (*s3.Client, *[]string, *[]interface{}) {
7482

7583
r.HTTPResponse = &http.Response{
7684
StatusCode: 200,
77-
Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
85+
Body: ioutil.NopCloser(bytes.NewReader([]byte(respBody))),
7886
}
7987

8088
switch data := r.Data.(type) {
@@ -952,7 +960,7 @@ func TestReaderAt(t *testing.T) {
952960
contentLen = r.HTTPRequest.Header.Get("Content-Length")
953961
r.HTTPResponse = &http.Response{
954962
StatusCode: 200,
955-
Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
963+
Body: ioutil.NopCloser(bytes.NewReader([]byte(respBody))),
956964
}
957965
})
958966

@@ -990,7 +998,7 @@ func TestSSE(t *testing.T) {
990998
defer mutex.Unlock()
991999
r.HTTPResponse = &http.Response{
9921000
StatusCode: 200,
993-
Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
1001+
Body: ioutil.NopCloser(bytes.NewReader([]byte(respBody))),
9941002
}
9951003
switch data := r.Data.(type) {
9961004
case *s3.CreateMultipartUploadOutput:
@@ -1137,7 +1145,7 @@ func TestUploadBufferStrategy(t *testing.T) {
11371145

11381146
r.HTTPResponse = &http.Response{
11391147
StatusCode: 200,
1140-
Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
1148+
Body: ioutil.NopCloser(bytes.NewReader([]byte(respBody))),
11411149
}
11421150

11431151
switch data := r.Data.(type) {

‎service/s3/statusok_error.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"io"
66
"io/ioutil"
77
"net/http"
8+
"strings"
89

910
request "github.com/aws/aws-sdk-go-v2/aws"
1011
"github.com/aws/aws-sdk-go-v2/aws/awserr"
@@ -20,17 +21,18 @@ func copyMultipartStatusOKUnmarhsalError(r *request.Request) {
2021
r.HTTPResponse.Body = ioutil.NopCloser(body)
2122
defer body.Seek(0, io.SeekStart)
2223

23-
if body.Len() == 0 {
24-
// If there is no body don't attempt to parse the body.
25-
return
26-
}
27-
2824
unmarshalError(r)
2925
if err, ok := r.Error.(awserr.Error); ok && err != nil {
30-
if err.Code() == "SerializationError" {
26+
if err.Code() == "SerializationError" &&
27+
!strings.Contains(err.Error(), io.EOF.Error()) {
3128
r.Error = nil
3229
return
3330
}
34-
r.HTTPResponse.StatusCode = http.StatusServiceUnavailable
31+
// if empty payload
32+
if strings.Contains(err.Error(), io.EOF.Error()) {
33+
r.HTTPResponse.StatusCode = http.StatusInternalServerError
34+
} else {
35+
r.HTTPResponse.StatusCode = http.StatusServiceUnavailable
36+
}
3537
}
3638
}

‎service/s3/statusok_error_test.go‎

Lines changed: 162 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import (
55
"fmt"
66
"net/http"
77
"net/http/httptest"
8+
"strconv"
9+
"strings"
810
"testing"
911
"time"
1012

@@ -14,7 +16,10 @@ import (
1416
"github.com/aws/aws-sdk-go-v2/service/s3"
1517
)
1618

17-
const errMsg = `<Error><Code>ErrorCode</Code><Message>message body</Message><RequestId>requestID</RequestId><HostId>hostID=</HostId></Error>`
19+
const (
20+
errMsg = `<Error><Code>ErrorCode</Code><Message>message body</Message><RequestId>requestID</RequestId><HostId>hostID=</HostId></Error>`
21+
xmlPreambleMsg = `<?xml version="1.0" encoding="UTF-8"?>`
22+
)
1823

1924
var lastModifiedTime = time.Date(2009, 11, 23, 0, 0, 0, 0, time.UTC)
2025

@@ -173,3 +178,159 @@ func newCopyTestSvc(errMsg string) *s3.Client {
173178

174179
return svc
175180
}
181+
182+
func TestStatusOKPayloadHandling(t *testing.T) {
183+
cases := map[string]struct {
184+
Header http.Header
185+
Payloads [][]byte
186+
OpCall func(api *s3.Client) error
187+
Err string
188+
}{
189+
"200 error": {
190+
Header: http.Header{
191+
"Content-Length": []string{strconv.Itoa(len(errMsg))},
192+
},
193+
Payloads: [][]byte{[]byte(errMsg)},
194+
OpCall: func(c *s3.Client) error {
195+
req := c.CopyObjectRequest(&s3.CopyObjectInput{
196+
Bucket: aws.String("bucketname"),
197+
CopySource: aws.String("bucketname/doesnotexist.txt"),
198+
Key: aws.String("destination.txt"),
199+
})
200+
_, err := req.Send(context.Background())
201+
return err
202+
},
203+
Err: "ErrorCode: message body",
204+
},
205+
"200 error partial response": {
206+
Header: http.Header{
207+
"Content-Length": []string{strconv.Itoa(len(errMsg))},
208+
},
209+
Payloads: [][]byte{
210+
[]byte(errMsg[:20]),
211+
},
212+
OpCall: func(c *s3.Client) error {
213+
req := c.CopyObjectRequest(&s3.CopyObjectInput{
214+
Bucket: aws.String("bucketname"),
215+
CopySource: aws.String("bucketname/doesnotexist.txt"),
216+
Key: aws.String("destination.txt"),
217+
})
218+
_, err := req.Send(context.Background())
219+
return err
220+
},
221+
Err: "unexpected EOF",
222+
},
223+
"200 error multipart": {
224+
Header: http.Header{
225+
"Transfer-Encoding": []string{"chunked"},
226+
},
227+
Payloads: [][]byte{
228+
[]byte(errMsg[:20]),
229+
[]byte(errMsg[20:]),
230+
},
231+
OpCall: func(c *s3.Client) error {
232+
req := c.CopyObjectRequest(&s3.CopyObjectInput{
233+
Bucket: aws.String("bucketname"),
234+
CopySource: aws.String("bucketname/doesnotexist.txt"),
235+
Key: aws.String("destination.txt"),
236+
})
237+
_, err := req.Send(context.Background())
238+
return err
239+
},
240+
Err: "ErrorCode: message body",
241+
},
242+
"200 error multipart partial response": {
243+
Header: http.Header{
244+
"Transfer-Encoding": []string{"chunked"},
245+
},
246+
Payloads: [][]byte{
247+
[]byte(errMsg[:20]),
248+
},
249+
OpCall: func(c *s3.Client) error {
250+
req := c.CopyObjectRequest(&s3.CopyObjectInput{
251+
Bucket: aws.String("bucketname"),
252+
CopySource: aws.String("bucketname/doesnotexist.txt"),
253+
Key: aws.String("destination.txt"),
254+
})
255+
_, err := req.Send(context.Background())
256+
return err
257+
},
258+
Err: "XML syntax error",
259+
},
260+
"200 error multipart no payload": {
261+
Header: http.Header{
262+
"Transfer-Encoding": []string{"chunked"},
263+
},
264+
Payloads: [][]byte{},
265+
OpCall: func(c *s3.Client) error {
266+
req := c.CopyObjectRequest(&s3.CopyObjectInput{
267+
Bucket: aws.String("bucketname"),
268+
CopySource: aws.String("bucketname/doesnotexist.txt"),
269+
Key: aws.String("destination.txt"),
270+
})
271+
_, err := req.Send(context.Background())
272+
return err
273+
},
274+
Err: "empty response payload",
275+
},
276+
"response with only xml preamble": {
277+
Header: http.Header{
278+
"Transfer-Encoding": []string{"chunked"},
279+
},
280+
Payloads: [][]byte{
281+
[]byte(xmlPreambleMsg),
282+
},
283+
OpCall: func(c *s3.Client) error {
284+
req := c.CopyObjectRequest(&s3.CopyObjectInput{
285+
Bucket: aws.String("bucketname"),
286+
CopySource: aws.String("bucketname/doesnotexist.txt"),
287+
Key: aws.String("destination.txt"),
288+
})
289+
_, err := req.Send(context.Background())
290+
return err
291+
},
292+
Err: "empty response payload",
293+
},
294+
}
295+
296+
for name, c := range cases {
297+
t.Run(name, func(t *testing.T) {
298+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
299+
ww := w.(interface {
300+
http.ResponseWriter
301+
http.Flusher
302+
})
303+
304+
for k, vs := range c.Header {
305+
for _, v := range vs {
306+
ww.Header().Add(k, v)
307+
}
308+
}
309+
ww.WriteHeader(http.StatusOK)
310+
ww.Flush()
311+
312+
for _, p := range c.Payloads {
313+
ww.Write(p)
314+
ww.Flush()
315+
}
316+
}))
317+
defer srv.Close()
318+
319+
cfg := unit.Config()
320+
cfg.EndpointResolver = aws.ResolveWithEndpointURL(srv.URL)
321+
svc := s3.New(cfg)
322+
svc.ForcePathStyle = true
323+
err := c.OpCall(svc)
324+
if len(c.Err) != 0 {
325+
if err == nil {
326+
t.Fatalf("expect error, got none")
327+
}
328+
if e, a := c.Err, err.Error(); !strings.Contains(a, e) {
329+
t.Fatalf("expect %v error in, %v", e, a)
330+
}
331+
} else if err != nil {
332+
t.Fatalf("expect no error, got %v", err)
333+
}
334+
})
335+
}
336+
}

‎service/s3/unmarshal_error.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,13 @@ func unmarshalError(r *request.Request) {
4747
// Attempt to parse error from body if it is known
4848
resp := &xmlErrorResponse{}
4949
err := xml.NewDecoder(r.HTTPResponse.Body).Decode(resp)
50-
if err != nil && err != io.EOF {
50+
51+
// if 200 OK response payload with EOF error
52+
if r.HTTPResponse.StatusCode >= 200 && r.HTTPResponse.StatusCode < 300 &&
53+
err == io.EOF {
54+
errCode = "SerializationError"
55+
errMsg = "empty response payload"
56+
} else if err != nil && err != io.EOF {
5157
errCode = "SerializationError"
5258
errMsg = "failed to decode S3 XML error response"
5359
} else {

0 commit comments

Comments
 (0)