Set http.NoBody if contentLength is 0

[grodowski]

We've experienced a production bug using minio-go with minio:latest server and go version go1.8.3 linux/amd64.

A valid call to PutObject with a zero-length reader argument...

reader := bytes.NewReader(make([]byte, 0))
contentType := "application/octet-stream"
_, err = minioClient.PutObject(bucketName, objectName, reader, contentType)

...caused a server error:

You must provide the Content-Length HTTP header

We managed to pinpoint the problem to a go update (1.7 -> 1.8.3) we've done recently - it seems that an empty body is now treated differently by net/http. Check this issue: golang/go#20257.

This is a fix that works for our use case, but I am not sure about backwards compatibility with older Go versions. I'd also be happy to add unit tests for it, but I might need some guidance on that.

[vadmeste]

Thanks @grodowski for this PR.

I am not sure about backwards compatibility with older Go versions

Yeah, this change will break support of go 1.7.x. http.NoBody is known only in go 1.8

I'd also be happy to add unit tests for it, but I might need some guidance on that.

We already test putting zero size objects. We don't have to add any new tests.

Overall, I think that we need to see if we are obliged to move go 1.8. We can require 1.8 starting from the next release.

[grodowski]

It seems that http.NoBody which is not present in go < 1.8 could be replaced with nil. That should solve backwards compatibility 🎉

From net/http GoDoc

NoBody is an io.ReadCloser with no bytes. Read always returns EOF and Close always returns nil. It can be used in an outgoing client request to explicitly signal that a request has zero bytes. An alternative, however, is to simply set Request.Body to nil.

[grodowski]

@harshavardhana, using >= as suggested in the comment still breaks my test case (https://gist.github.com/grodowski/41a0c4f4db6ad18718e5725f655389b6), because contentLength is actually 0 and contentBody is an empty tmpfile rather than nil. What do you think?

[harshavardhana]

This fix is good. Will test it with GCS.

[harshavardhana]

There is a bug which failed one of the functional tests.

This is the change which should work for all versions and functionality.

$ git diff
diff --git a/api.go b/api.go
index f68a2aa..c1e7f14 100644
--- a/api.go
+++ b/api.go
@@ -616,17 +616,8 @@ func (c Client) newRequest(method string, metadata requestMetadata) (req *http.R
                return nil, err
        }
 
-       // Go net/http notoriously closes the request body.
-       // - The request Body, if non-nil, will be closed by the underlying Transport, even on errors.
-       // This can cause underlying *os.File seekers to fail, avoid that
-       // by making sure to wrap the closer as a nop.
-       var body io.ReadCloser
-       if metadata.contentBody != nil && metadata.contentLength > 0 {
-               body = ioutil.NopCloser(metadata.contentBody)
-       }
-
        // Initialize a new HTTP request for the method.
-       req, err = http.NewRequest(method, targetURL.String(), body)
+       req, err = http.NewRequest(method, targetURL.String(), nil)
        if err != nil {
                return nil, err
        }
@@ -678,6 +669,16 @@ func (c Client) newRequest(method string, metadata requestMetadata) (req *http.R
                req.Header.Set(k, v[0])
        }
 
+       // Go net/http notoriously closes the request body.
+       // - The request Body, if non-nil, will be closed by the underlying Transport, even on errors.
+       // This can cause underlying *os.File seekers to fail, avoid that
+       // by making sure to wrap the closer as a nop.
+       if metadata.contentLength == 0 {
+               req.Body = noBody{}
+       } else {
+               req.Body = ioutil.NopCloser(metadata.contentBody)
+       }
+
        // Set incoming content-length.
        req.ContentLength = metadata.contentLength
        if req.ContentLength <= -1 {
diff --git a/tempfile.go b/tempfile.go
index 65c7b0d..d908e05 100644
--- a/tempfile.go
+++ b/tempfile.go
@@ -17,6 +17,7 @@
 package minio
 
 import (
+       "io"
        "io/ioutil"
        "os"
        "sync"
@@ -58,3 +59,13 @@ func (t *tempFile) Close() error {
        }
        return nil
 }
+
+// noBody is an io.ReadCloser with no bytes. Read always returns EOF
+// and Close always returns nil. It can be used in an outgoing client
+// request to explicitly signal that a request has zero bytes.
+// An alternative, however, is to simply set Request.Body to nil.
+type noBody struct{}
+
+func (noBody) Read([]byte) (int, error)         { return 0, io.EOF }
+func (noBody) Close() error                     { return nil }
+func (noBody) WriteTo(io.Writer) (int64, error) { return 0, nil }

[grodowski]

@harshavardhana I have applied your patch and unfortunately my test program still fails.

if metadata.contentLength == 0 {
	req.Body = noBody{} // it works if only I changed `noBody{}` back to `http.NoBody` 🤔
} else {
	req.Body = ioutil.NopCloser(metadata.contentBody)
}

The problem could be that net/http performs explicit comparisons with == http.NoBody (a var) somewhere under the hood, maybe.

Meanwhile I've discovered how to run functional tests against a local minio instance, which is great.

SERVER_ENDPOINT="localhost:9000" ACCESS_KEY="bacon" SECRET_KEY="bacon123" ENABLE_HTTPS=false go test -race ./...

And I can see that the problem can in fact be reproduced using these tests. This run used go1.8.3 and minio:latest after applying the patch.

...
...
	api_functional_v4_test.go:728: Error: PutObject shouldn't fail. You must provide the Content-Length HTTP header.
	api_functional_v4_test.go:728: Error: PutObject shouldn't fail. You must provide the Content-Length HTTP header.
	api_functional_v4_test.go:728: Error: PutObject shouldn't fail. You must provide the Content-Length HTTP header.
--- FAIL: TestPutObjectUploadSeekedObject (0.04s)
	api_functional_v4_test.go:2372: Error: You must provide the Content-Length HTTP header.

If I change api.go to my previous version:

if metadata.contentLength == 0 {
		req.Body = nil // or http.NoBody
} else {
	req.Body = ioutil.NopCloser(metadata.contentBody)
}

These tests fail with a nil pointer panic in pkg/s3signer/request-signature-streaming.go:251, which is probably the error you meant earlier.

[harshavardhana]

if metadata.contentLength == 0 {
req.Body = nil // or http.NoBody
} else {
req.Body = ioutil.NopCloser(metadata.contentBody)
}

What if you do

req.Body = ioutil.NopCloser(bytes.NewReader(""))

[grodowski]

Then we're back to api_functional_v4_test.go:728: Error: PutObject shouldn't fail. You must provide the Content-Length HTTP header.

[harshavardhana]

Then we're back to api_functional_v4_test.go:728: Error: PutObject shouldn't fail. You must provide the Content-Length HTTP header.

There is a fix but its not clean. @grodowski testing right now.

[harshavardhana]

Try this

$ git diff
diff --git a/api.go b/api.go
index f68a2aa..67b8b51 100644
--- a/api.go
+++ b/api.go
@@ -616,17 +616,8 @@ func (c Client) newRequest(method string, metadata requestMetadata) (req *http.R
                return nil, err
        }
 
-       // Go net/http notoriously closes the request body.
-       // - The request Body, if non-nil, will be closed by the underlying Transport, even on errors.
-       // This can cause underlying *os.File seekers to fail, avoid that
-       // by making sure to wrap the closer as a nop.
-       var body io.ReadCloser
-       if metadata.contentBody != nil && metadata.contentLength > 0 {
-               body = ioutil.NopCloser(metadata.contentBody)
-       }
-
        // Initialize a new HTTP request for the method.
-       req, err = http.NewRequest(method, targetURL.String(), body)
+       req, err = http.NewRequest(method, targetURL.String(), nil)
        if err != nil {
                return nil, err
        }
@@ -678,6 +669,16 @@ func (c Client) newRequest(method string, metadata requestMetadata) (req *http.R
                req.Header.Set(k, v[0])
        }
 
+       // Go net/http notoriously closes the request body.
+       // - The request Body, if non-nil, will be closed by the underlying Transport, even on errors.
+       // This can cause underlying *os.File seekers to fail, avoid that
+       // by making sure to wrap the closer as a nop.
+       if metadata.contentLength == 0 {
+               req.Body = nil
+       } else {
+               req.Body = ioutil.NopCloser(metadata.contentBody)
+       }
+
        // Set incoming content-length.
        req.ContentLength = metadata.contentLength
        if req.ContentLength <= -1 {
diff --git a/pkg/s3signer/request-signature-streaming.go b/pkg/s3signer/request-signature-streaming.go
index c2f0bae..04b29a8 100644
--- a/pkg/s3signer/request-signature-streaming.go
+++ b/pkg/s3signer/request-signature-streaming.go
@@ -205,6 +205,10 @@ func StreamingSignV4(req *http.Request, accessKeyID, secretAccessKey, sessionTok
        // Set headers needed for streaming signature.
        prepareStreamingRequest(req, sessionToken, dataLen, reqTime)
 
+       if req.Body == nil {
+               req.Body = ioutil.NopCloser(bytes.NewReader([]byte("")))
+       }
+
        stReader := &StreamingReader{
                baseReadCloser:  req.Body,
                accessKeyID:     accessKeyID,

[grodowski]

That fixes the panic indeed, but all req.Body lengths still fail with a signature mismatch. I can look into it soon.

[harshavardhana]

@grodowski that is not because of this fix. There was bug in previous release and Minio master is already fixed.

Remove the TODO.

[vadmeste]

LGTM & tested with go 1.7.6 and 1.8.3