Validate minio.Object ETag between read requests #662

krisis · 2017-04-24T10:10:59Z

On each new request, we compare minio.Object's cached ETag with the current response ETag to error out if the object has changed since the last read.

Fixes #656

harshavardhana · 2017-04-25T03:13:21Z

api-get-object.go

+
+	// Check if etag from response matches Object's etag.
+	case o.etag != response.objectInfo.ETag:
+		return response, errors.New("object has been modified since last read")


You can use the ErrorResponse thingy here and return BadDigest instead.

@harshavardhana Since this error is returned by minio.Object's Stat, Read and ReadAt, I was thinking if syscall.EBADFD would be more appropriate?

You can use the ErrorResponse thingy here and return BadDigest instead.

@harshavardhana, The Content-Md5 you specified did not match what we received. can be misleading to someone receiving this error for Read or ReadAt on minio.Object. Further, the request headers sent during a GetObject were correct, at that point in time.Thoughts?

syscall.EBADFD is good would work with *os.File behavior.

krishnasrinivas · 2017-04-25T19:30:10Z

api-get-object.go

+
+	// Check if etag from response matches Object's etag.
+	case o.etag != response.objectInfo.ETag:
+		return response, syscall.EBADF


since we always return type ErrorResponse for error it's better to:
return ErrInvalidObjectName("ETag has changed")

instead of returning syscall.EBADF

But this sounds better:
return ErrInvalidObject("ETag has changed")

we could rename ErrInvalidObjectName() to ErrInvalidObject()

@krisis @harshavardhana mentioned that he discussed this with you. The reason ErrInvalidObject("ETag has changed") is better than syscall.EBADF is if application logs the error it will know exactly why error was returned (instead of a generic bad FD)

@krishnasrinivas, I agree that syscall.EBADF doesn't provide context to the user. How about "object has been modified since last read"? I find "ETag has changed" not entirely helpful. Thoughts?

It should be encapsulated inside ErrInvalidObjectNamet() @krisis which provides ErrorResponse compatible error. This way caller can look at the errResp.Code == NoSuchKey and can decide to re-open the connection to discard and start reading again.

@harshavardhana It doesn't make sense to return NoSuchKey. IMO InvalidObjectState[1] is a more apt error response code. thoughts?

[1] http://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#ErrorCodeList

NoSuchKey meaning we don't see the same object anymore i.e based on the ETag.

harshavardhana · 2017-04-26T06:33:40Z

Actually now that i think about it. This is not the right fix - we should have been using If-Match tag in GetObject header and let server return proper error.

http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectGET.html

deekoder · 2017-04-28T20:14:43Z

is this PR still valid? Please advise if it needs to be closed then?

harshavardhana · 2017-04-28T20:15:30Z

is this PR still valid? Please advise if it needs to be closed then?

It is valid it needs to be re-done since we merged #670

krisis · 2017-05-01T06:45:56Z

@harshavardhana @krishnasrinivas I have updated the PR to use recent changes in c.getObject method to include request headers.

harshavardhana · 2017-05-01T07:04:16Z

@harshavardhana @krishnasrinivas I have updated the PR to use recent changes in c.getObject method to include request headers.

Will test thanks @krisis

harshavardhana · 2017-05-01T07:05:54Z

api-get-object.go

@@ -105,7 +105,7 @@ func (c Client) GetObject(bucketName, objectName string) (*Object, error) {
 							// Do not set objectInfo from the first readAt request because it will not get
 							// the whole object.
 							reqHeaders.SetRange(req.Offset, req.Offset+int64(len(req.Buffer))-1)
-							httpReader, _, err = c.getObject(bucketName, objectName, reqHeaders)
+							httpReader, objectInfo, err = c.getObject(bucketName, objectName, reqHeaders)


Replacing objectInfo for each ReadAt changes the Size() of the object.. Is the Size() of the object expected to change . Stat() method is supposed to return the actual object size.

Can you test this with all other conditions such as a combination of ReadAt() and then Read()/Seek() ?

Replacing objectInfo for each ReadAt changes the Size() of the object.. Is the Size() of the object expected to change . Stat() method is supposed to return the actual object size

objectInfo (i.e, minio.Object.objectInfo) is not being replaced on each ReadAt. if !o.objectInfoSet && !req.isReadAt check in doGetRequest ensures that objectInfo is set only the first time and never from a ReadAt call.

The problem is that Size returned is lesser than the original Size of the object. So it means that if the Read() is ensued after ReadAt() - would result in an EOF without reading the whole object.

Can you test this with all other conditions such as a combination of ReadAt() and then Read()/Seek() ?

I have added a functional test that makes a ReadAt, Stat and ReadAt call on minio.Object in that order, in api_functional_v4_test.go. Between the two ReadAt calls the object is modified in the store. I shall see how I can add tests that will cover other sequences of minio.Object methods, especially Read, ReadAt and Stat.

You need something like a user did following sequence of events tested.

ReadAt() to read some bytes at an offset.

Then proceeds to start Read() instead of ReadAt().

Or

ReadAt() to read some bytes at offset.

Seek() try to seek past the offset+length used in ReadAt().

The reason previously objectInfo was not updated in ReadAt() was to ensure that it is updated only when a Read() or a Seek() is called. I am pretty sure setting objectInfo would cause some unknown situations during ReadAt().

I see now there is an additional check.. to ignore objectInfo sent from ReadAt..

The reason previously objectInfo was not updated in ReadAt() was to ensure that it is updated only when a Read() or a Seek() is called. I am pretty sure setting objectInfo would cause some unknown situations during ReadAt().

There is a check in doGetRequest which ensures objectInfo is not updated. See https://github.com/minio/minio-go/pull/662/files#diff-0afcc28c2029fa14fe9a221e90b524e5R307.

harshavardhana · 2017-05-01T07:32:03Z

api_functional_v4_test.go

+
+	// Read again only to find object contents have been modified since last read.
+	_, err = reader.ReadAt(b, int64(n))
+	if err.Error() != s3ErrorResponseMap["PreconditionFailed"] {


Make sure that err is not nil. Since you need to fail for that as well.

In this case ReadAt call should fail. If it returned nil it implies that this test failed. I can add err != nil for readability sake.

harshavardhana · 2017-05-01T07:32:27Z

api_functional_v4_test.go

+	}
+
+	// Confirm that a Stat() call in between doesn't change the Object's cached etag.
+	_, err = reader.Stat()


Shouldn't you be reading the cached value and validating it as well?

In this sequence of method calls, namely PutObject, ReadAt, PutObject, Stat ..., we can check for object size to be len(newContent). If Read call replaced the (first) ReadAt, then the objectInfo.Size would be len(content). In summary, Stat method may return stale objectInfo while Read and ReadAt methods will return error if the object is modified in the object store.

This brings us to if we should return error on Stat if the object is modified since the call to GetObject.

harshavardhana · 2017-05-01T07:32:53Z

api_functional_v4_test.go

+
+	defer c.RemoveObject(bucketName, objectName)
+
+	reader, err := c.GetObject(bucketName, objectName)


Close the reader for posterity.

harshavardhana · 2017-05-01T07:33:04Z

api_functional_v4_test.go

@@ -2422,3 +2423,75 @@ func TestFunctional(t *testing.T) {
 		t.Fatal("Error: ", err)
 	}
 }
+


A comment..

krishnasrinivas

// GetObject - returns an seekable, readable object.
func (c Client) GetObject(bucketName, objectName string) (*Object, error) {
        // Input validation.
        if err := isValidBucketName(bucketName); err != nil {
                return nil, err
        }
        if err := isValidObjectName(objectName); err != nil {
                return nil, err
        }

        var httpReader io.ReadCloser
        var objectInfo ObjectInfo

ETag information is already available in the objectInfo, so you could use it instead of maintaining it in getRequest

krisis · 2017-05-02T01:47:02Z

@krishnasrinivas, you are right, the way we use objectInfo is not straightforward. If you see how doGetRequest uses objectInfo you will see why we need ETag saved separately. We can simplify the code structure in a different PR.

krishnasrinivas · 2017-05-02T02:39:10Z

@krisis no, see this is all we need: krishnasrinivas@fa0e640

krisis · 2017-05-02T02:43:17Z

@krishnasrinivas, could you test your changes with Get object followed by ReadAt on the reader, putobject modifying the object and another ReadAt? Meanwhile I shall check how it is different from what is present in this PR.

krisis · 2017-05-02T04:05:26Z

@krisis no, see this is all we need: krishnasrinivas/minio-go@fa0e640

This fix looks good to me. I will make a minor change where var etag string will be moved into the go-routine. I shall revert my changes and include this.

@krishnasrinivas

Thanks @krishnasrinivas for the simple approach. - Add a functional test case to confirm the fix.

krisis force-pushed the issue/656 branch from 643f0c1 to a4aae42 Compare April 24, 2017 16:17

harshavardhana reviewed Apr 25, 2017

View reviewed changes

harshavardhana requested a review from krishnasrinivas April 25, 2017 07:47

krisis force-pushed the issue/656 branch from a4aae42 to 36c0afb Compare April 25, 2017 07:51

krishnasrinivas reviewed Apr 25, 2017

View reviewed changes

krisis force-pushed the issue/656 branch 2 times, most recently from 1ceb3bd to 75f0a3a Compare April 26, 2017 05:43

krisis force-pushed the issue/656 branch from 75f0a3a to 25b57a1 Compare May 1, 2017 06:44

harshavardhana reviewed May 1, 2017

View reviewed changes

harshavardhana requested changes May 1, 2017

View reviewed changes

krisis force-pushed the issue/656 branch from 25b57a1 to 1a769d7 Compare May 1, 2017 09:15

harshavardhana previously approved these changes May 1, 2017

View reviewed changes

krishnasrinivas reviewed May 1, 2017

View reviewed changes

Verify etag for minio.Object methods

75b4db0

Thanks @krishnasrinivas for the simple approach. - Add a functional test case to confirm the fix.

krisis dismissed harshavardhana’s stale review via 75b4db0 May 2, 2017 04:43

krisis force-pushed the issue/656 branch from 1a769d7 to 75b4db0 Compare May 2, 2017 04:43

krishnasrinivas approved these changes May 2, 2017

View reviewed changes

harshavardhana approved these changes May 2, 2017

View reviewed changes

harshavardhana merged commit e461683 into minio:master May 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate minio.Object ETag between read requests #662

Validate minio.Object ETag between read requests #662

krisis commented Apr 24, 2017

harshavardhana Apr 25, 2017

krisis Apr 25, 2017

krisis Apr 25, 2017

harshavardhana Apr 25, 2017

krishnasrinivas Apr 25, 2017

krishnasrinivas Apr 25, 2017

krisis Apr 26, 2017

harshavardhana Apr 26, 2017

krisis Apr 26, 2017

harshavardhana Apr 26, 2017

harshavardhana commented Apr 26, 2017

deekoder commented Apr 28, 2017

harshavardhana commented Apr 28, 2017

krisis commented May 1, 2017

harshavardhana commented May 1, 2017

harshavardhana May 1, 2017

harshavardhana May 1, 2017

krisis May 1, 2017

harshavardhana May 1, 2017

krisis May 1, 2017

harshavardhana May 1, 2017 •

edited

Loading

harshavardhana May 1, 2017

krisis May 1, 2017

harshavardhana May 1, 2017

krisis May 1, 2017

harshavardhana May 1, 2017

krisis May 1, 2017

harshavardhana May 1, 2017

harshavardhana May 1, 2017

krishnasrinivas left a comment

krisis commented May 2, 2017

krishnasrinivas commented May 2, 2017

krisis commented May 2, 2017

krisis commented May 2, 2017


		defer c.RemoveObject(bucketName, objectName)

		reader, err := c.GetObject(bucketName, objectName)

Validate minio.Object ETag between read requests #662

Validate minio.Object ETag between read requests #662

Conversation

krisis commented Apr 24, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana commented Apr 26, 2017

deekoder commented Apr 28, 2017

harshavardhana commented Apr 28, 2017

krisis commented May 1, 2017

harshavardhana commented May 1, 2017

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

harshavardhana May 1, 2017 • 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

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

krishnasrinivas left a comment

Choose a reason for hiding this comment

krisis commented May 2, 2017

krishnasrinivas commented May 2, 2017

krisis commented May 2, 2017

krisis commented May 2, 2017

harshavardhana May 1, 2017 •

edited

Loading