Object.Read doesn't return error on truncated response

[pracucci]

Thanos and Cortex are using the Minio client as S3 client. From time to time we've some reports of bugs that looks like if we get a partial/truncated response from the server which is not treated as an error from our application.

I've tried to simulate a scenario (which I don't know how much realistic could be) where the server returns a body response smaller than the Content-Length. The Minio Object.Read() returns no error once all response has been consumed while, as comparison, the Google GCS client does return io.ErrUnexpectedEOF.

I've added a test in Thanos to show it:
thanos-io/thanos#3795

Questions:

• Why the Minio client doesn't check if the received response size matches the Content-Length?
• The scenario described shouldn't be reported as an error by the Minio client?

[pracucci]

As comparison I tried to run the same test with the official AWS SDK and I get io.ErrUnexpectedEOF:

func TestAWSSDK_ShouldReturnErrorIfServerTruncateResponse(t *testing.T) {
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("Last-Modified", "Wed, 21 Oct 2015 07:28:00 GMT")
		w.Header().Set("Content-Length", "100")

		// Write less bytes than the content length.
		_, err := w.Write([]byte("12345"))
		testutil.Ok(t, err)
	}))
	defer srv.Close()

	cfg := &aws.Config{
		Endpoint:         aws.String(srv.Listener.Addr().String()),
		Region:           aws.String("dummy"),
		DisableSSL:       aws.Bool(true),
		S3ForcePathStyle: aws.Bool(true),
		Credentials:      credentials.NewStaticCredentials("test", "test", ""),
	}

	sess, err := session.NewSession(cfg)
	require.NoError(t, err)

	s3Client := s3.New(sess)
	out, err := s3Client.GetObject(&s3.GetObjectInput{
		Bucket: aws.String("test-bucket"),
		Key:    aws.String("test-key"),
	})
	require.NoError(t, err)

	// We expect an error when reading back.
	_, err = ioutil.ReadAll(out.Body)
	testutil.Equals(t, io.ErrUnexpectedEOF, err)
}

[harshavardhana]

Will take a look @pracucci

[harshavardhana]

The main reason is that io.ErrUnexpectedEOF is treated as a legitimate error because of our readFull() usage @pracucci - NOTE: shorter reads are allowed in io.Reader implementation so it is the caller that needs to know

The actual expected size returned after a fully io.Copy() is n != expectedN

[pracucci]

Is it a nice way to say it's by design? :)

[harshavardhana]

s3Client := s3.New(sess)
out, err := s3Client.GetObject(&s3.GetObjectInput{
Bucket: aws.String("test-bucket"),
Key: aws.String("test-key"),
})
require.NoError(t, err)

// We expect an error when reading back.
_, err = ioutil.ReadAll(out.Body)
testutil.Equals(t, io.ErrUnexpectedEOF, err)

This is a very low-level implementation - you would see the same behavior in this SDK as well if you used core.GetObject()

the top-level implementation i.e api.GetObject() is io.ReadSeeker and io.ReaderAt compatible so the behavior is trying to be io.Reader compatible.

So Read() will not return io.ErrUnexpectedEOF

[harshavardhana]

Let's see if it's possible to add it @pracucci and let me do some experimentation to see if it possible without breaking io.Reader compatibility.

[harshavardhana]

Let's see if it's possible to add it @pracucci and let me do some experimentation to see if it possible without breaking io.Reader compatibility.

Actually no its not possible since Read() semantics allows shorter reads i.e Read(buf(n)) is allowed to be not equal to n1 returned value

If you look at the io.Copy() implementation in Go you can see the value of n is verified properly - so it the callers responsibility here to check for exact content-length - meaning don't blindly do io.ReadAll() and expect that returned buffer is exactly what you want.

Add further validation for it such that look at len(buf) returned.

[pracucci]

We can also try to mitigated it on our side. I'm just wondering if the Minio behaviour is expected or not because may potentially lead to issues hard to debug (we're hearing such issue since a long time and I figured it out just today).

[harshavardhana]

We can also try to mitigated it on our side. I'm just wondering if the Minio behaviour is expected or not because may potentially lead to issues hard to debug (we're hearing such issue since a long time and I figured it out just today).

The problem is perhaps our readFull() implementation is hiding the real io.ErrUnexpectedEOF v/s io.ErrUnexpectedEOF because of the buffer is bigger than the underlying stream.

Let me think more.

[harshavardhana]

So I tested with a test code @pracucci I see that io.ErrUnexpectedEOF is correctly returned

package minio

import (
        "context"
        "io"
        "io/ioutil"
        "net/http"
        "net/http/httptest"
        "testing"
)

func TestGetObjectReturnErrorIfServerTruncatesResponse(t *testing.T) {
        srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                w.Header().Set("Last-Modified", "Wed, 21 Oct 2015 07:28:00 GMT")
                w.Header().Set("Content-Length", "100")

                // Write less bytes than the content length.
                w.Write([]byte("12345"))
        }))
        defer srv.Close()

        // New - instantiate minio client with options
        clnt, err := New(srv.Listener.Addr().String(), &Options{})

        obj, err := clnt.GetObject(context.Background(), "bucketName", "objectName", GetObjectOptions{})
        if err != nil {
                t.Fatal(err)
        }

        // We expect an error when reading back.
        if _, err = ioutil.ReadAll(obj); err != io.ErrUnexpectedEOF {
                t.Fatalf("Expected %v, got %v", io.ErrUnexpectedEOF, err)
        }
}

~  go test -v -run TestGetObjectReturnErrorIfServerTruncatesResponse
=== RUN   TestGetObjectReturnErrorIfServerTruncatesResponse
--- PASS: TestGetObjectReturnErrorIfServerTruncatesResponse (0.00s)
PASS
ok      github.com/minio/minio-go/v7    0.004s

[harshavardhana]

Oh wait there is an issue in the test my bad!

[harshavardhana]

Yep reproduced it.

[harshavardhana]

PR #1452 should fix this properly once and for all.

[pracucci]

Thanks a lot @harshavardhana !

[harshavardhana]

@pracucci this should fix it #1453

[bwplotka]

Thanks a lot for quick turnaround 🤗