fix: handle http response code 307 - temporary redirect #571

balamurugana · 2017-05-05T16:53:48Z

Previously the SDK throws below exception when it gets response code 307.

java.io.EOFException: input contained no data
        at org.xmlpull.mxp1.MXParser.fillBuf(MXParser.java:3003)
        at org.xmlpull.mxp1.MXParser.more(MXParser.java:3046)
        at org.xmlpull.mxp1.MXParser.parseProlog(MXParser.java:1410)
        at org.xmlpull.mxp1.MXParser.nextImpl(MXParser.java:1395)
        at org.xmlpull.mxp1.MXParser.next(MXParser.java:1093)
        at com.google.api.client.xml.Xml.parseElementInternal(Xml.java:245)
        at com.google.api.client.xml.Xml.parseElement(Xml.java:222)
        at io.minio.messages.XmlEntity.parseXml(XmlEntity.java:75)
        at io.minio.messages.ErrorResponse.parseXml(ErrorResponse.java:159)
        at io.minio.messages.ErrorResponse.<init>(ErrorResponse.java:64)
        at io.minio.MinioClient.execute(MinioClient.java:970)
        at io.minio.MinioClient.executePut(MinioClient.java:1197)
        at io.minio.MinioClient.makeBucket(MinioClient.java:2597)
        at io.minio.MinioClient.makeBucket(MinioClient.java:2554)
        at FunctionalTest.makeBucket_test1(FunctionalTest.java:86)
        at FunctionalTest.runTests(FunctionalTest.java:1264)
        at FunctionalTest.main(FunctionalTest.java:1380)

This patch handles response code and returns Amazon S3 error.

vadmeste

LGTM

balamurugana · 2017-05-11T12:06:45Z

I would like get 307 is handled to redirect to received location or error out. Till then 👎

nitisht · 2017-05-13T08:23:50Z

api/src/main/java/io/minio/MinioClient.java

+      // Handle 307 (temporary redirect) specially.
+      String location = response.headers().get("Location");
+      if (location == null || location.isEmpty()) {
+        throw new InternalException("unexpected empty value in location header");


IMO, this is a server issue, not internal to the SDK. Is InternalException justified here?

IMO, this is a server issue, not internal to the SDK. Is InternalException justified here?

@balamurugana your views on this?

nitisht · 2017-05-13T08:29:55Z

api/src/main/java/io/minio/MinioClient.java

+    Response response = null;
+
+    // Loop to retry with new location of 307.
+    for (int i = 0; i < TEMP_REDIRECT_TRY_COUNT; i++) {


Instead of doing all the process in the loop, may be it is better to get in the loop only when 307 is received? IMO that will be more readable.

The loop is continued only for 307. What is all the process in the loop?

My suggestion to 307 is that SDK should error out instead of retrying its own. Auto-retry doesn't solve real problem ie put object doesn't work (not only for minio-java).

Minio server returns 307 only for http to https. https would invite other problems like ignoring self signed certificate.

I agree, I assumed request could be made outside the for loop but that is not right. This is fine.

harshavardhana · 2017-05-16T03:07:49Z

@nitisht anything pending here for you review?

nitisht · 2017-05-16T05:20:17Z

@harshavardhana just one comment, other than that LGTM

balamurugana · 2017-05-16T06:28:58Z

@harshavardhana there are issues handling 307 for all the cases (in all the libraries). Amazon AWS S3 sends 307 and new location for temporary resource change, however in minio server, it is used as http to https redirect. However put/post ReST calls in all SDKs do not work in minio server.

Assume the case of http to https redirection using 307 where https uses self signed certificate, automatic retry on 307 would lead to certificate error. This may be a surprise error for the consumer as he was trying with http.

This needs to be discussed and taken forward accordingly.

harshavardhana · 2017-05-16T06:31:56Z

Assume the case of http to https redirection using 307 where https uses self signed certificate, automatic retry on 307 would lead to certificate error. This may be a surprise error for the consumer as he was trying with http.

mc ls works fine btw.

mc config host add play-http http://play.minio.io:9000 Q3AM3UQ867SPQQA43P2F zuf+tfteSlswRu7BJ86wekitnifILbZam1KYY3TG
mc ls play-http

Here is how the redirection happens

$ mc ls --debug play-http
mc: <DEBUG> GET / HTTP/1.1
Host: play.minio.io:9000
User-Agent: Minio (linux; amd64) minio-go/2.0.4 mc/2017-05-14T07:14:06Z
Authorization: AWS4-HMAC-SHA256 Credential=**REDACTED**/20170516/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
X-Amz-Content-Sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
X-Amz-Date: 20170516T063123Z
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 307 Temporary Redirect
Content-Length: 63
Content-Type: text/html; charset=utf-8
Date: Tue, 16 May 2017 06:31:33 GMT
Location: https://play.minio.io:9000/

<a href="https://play.minio.io:9000/">Temporary Redirect</a>.

mc: <DEBUG> Response Time:  47.945222ms

mc: <DEBUG> GET / HTTP/1.1
Host: play.minio.io:9000
User-Agent: Minio (linux; amd64) minio-go/2.0.4 mc/2017-05-14T07:14:06Z
Authorization: AWS4-HMAC-SHA256 Credential=**REDACTED**/20170516/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
Referer: http://play.minio.io:9000/
X-Amz-Content-Sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
X-Amz-Date: 20170516T063123Z
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 200 OK
Transfer-Encoding: chunked
Accept-Ranges: bytes
Content-Type: application/xml
Date: Tue, 16 May 2017 06:31:33 GMT
Server: Minio/DEVELOPMENT.2017-05-05T03-22-47Z (linux; amd64)
Vary: Origin
X-Amz-Bucket-Region: us-east-1
X-Amz-Request-Id: 14BF0272ED46B4F8

mc: <DEBUG> Response Time:  250.239354ms

mc: <DEBUG> GET / HTTP/1.1
Host: play.minio.io:9000
User-Agent: Minio (linux; amd64) minio-go/2.0.4 mc/2017-05-14T07:14:06Z
Authorization: AWS4-HMAC-SHA256 Credential=**REDACTED**/20170516/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
X-Amz-Content-Sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
X-Amz-Date: 20170516T063124Z
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 307 Temporary Redirect
Content-Length: 63
Content-Type: text/html; charset=utf-8
Date: Tue, 16 May 2017 06:31:33 GMT
Location: https://play.minio.io:9000/

<a href="https://play.minio.io:9000/">Temporary Redirect</a>.

mc: <DEBUG> Response Time:  13.791017ms

mc: <DEBUG> GET / HTTP/1.1
Host: play.minio.io:9000
User-Agent: Minio (linux; amd64) minio-go/2.0.4 mc/2017-05-14T07:14:06Z
Authorization: AWS4-HMAC-SHA256 Credential=**REDACTED**/20170516/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
Referer: http://play.minio.io:9000/
X-Amz-Content-Sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
X-Amz-Date: 20170516T063124Z
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 200 OK
Transfer-Encoding: chunked
Accept-Ranges: bytes
Content-Type: application/xml
Date: Tue, 16 May 2017 06:31:34 GMT
Server: Minio/DEVELOPMENT.2017-05-05T03-22-47Z (linux; amd64)
Vary: Origin
X-Amz-Bucket-Region: us-east-1
X-Amz-Request-Id: 14BF0272F0E3BA00

mc: <DEBUG> Response Time:  26.234454ms

Are you saying something else?

harshavardhana · 2017-05-16T06:33:53Z

You are right mc cp fails. We need to fix this, there i think is a known Go bug in not having PUT redirects, PUT is automatically converted to GET after a 307 because of HTTP RFC. Need to read on why this happens.

// cc @vadmeste can you comment?

vadmeste · 2017-05-16T10:36:55Z

i think is a known Go bug in not having PUT redirects, PUT is automatically converted to GET after a 307 because of HTTP RFC

Yes, right. But this bug is already fixed in go1.8. I already compiled mc with go1.8 and it works fine.

@balamurugana, did you test enabling automatic redirect in the http library ?

balamurugana · 2017-05-16T10:40:35Z

@vadmeste

@balamurugana, did you test enabling automatic redirect in the http library ?

Do you mean golang http library or okhttp?

vadmeste · 2017-05-16T10:45:37Z

Do you mean golang http library or okhttp?

okhttp.

I mean I wonder what's the default behavior of okhttp when it receives 307 redirect.

balamurugana · 2017-05-16T10:47:20Z

I mean I wonder what's the default behavior of okhttp when it receives 307 redirect.

307 is not handled in OkHttp. Its consumer's responsibility.

harshavardhana · 2017-05-31T03:13:15Z

@balamurugana this requires a rebase..

nitisht · 2017-05-31T03:13:53Z

api/src/main/java/io/minio/MinioClient.java

+      // Handle 307 (temporary redirect) specially.
+      String location = response.headers().get("Location");
+      if (location == null || location.isEmpty()) {
+        throw new InternalException("unexpected empty value in location header");


IMO, this is a server issue, not internal to the SDK. Is InternalException justified here?

@balamurugana your views on this?

harshavardhana · 2017-06-02T03:07:00Z

@balamurugana can you rebase the PR?

Previously the SDK throws below exception ``` java.io.EOFException: input contained no data at org.xmlpull.mxp1.MXParser.fillBuf(MXParser.java:3003) at org.xmlpull.mxp1.MXParser.more(MXParser.java:3046) at org.xmlpull.mxp1.MXParser.parseProlog(MXParser.java:1410) at org.xmlpull.mxp1.MXParser.nextImpl(MXParser.java:1395) at org.xmlpull.mxp1.MXParser.next(MXParser.java:1093) at com.google.api.client.xml.Xml.parseElementInternal(Xml.java:245) at com.google.api.client.xml.Xml.parseElement(Xml.java:222) at io.minio.messages.XmlEntity.parseXml(XmlEntity.java:75) at io.minio.messages.ErrorResponse.parseXml(ErrorResponse.java:159) at io.minio.messages.ErrorResponse.<init>(ErrorResponse.java:64) at io.minio.MinioClient.execute(MinioClient.java:970) at io.minio.MinioClient.executePut(MinioClient.java:1197) at io.minio.MinioClient.makeBucket(MinioClient.java:2597) at io.minio.MinioClient.makeBucket(MinioClient.java:2554) at FunctionalTest.makeBucket_test1(FunctionalTest.java:86) at FunctionalTest.runTests(FunctionalTest.java:1264) at FunctionalTest.main(FunctionalTest.java:1380) ``` when it gets response code 307. This patch handles followup location provided in server response upto 21 times like Firefox, Chrome and OkHttp.

Previously the SDK throws below exception when it gets response code 307. ``` java.io.EOFException: input contained no data at org.xmlpull.mxp1.MXParser.fillBuf(MXParser.java:3003) at org.xmlpull.mxp1.MXParser.more(MXParser.java:3046) at org.xmlpull.mxp1.MXParser.parseProlog(MXParser.java:1410) at org.xmlpull.mxp1.MXParser.nextImpl(MXParser.java:1395) at org.xmlpull.mxp1.MXParser.next(MXParser.java:1093) at com.google.api.client.xml.Xml.parseElementInternal(Xml.java:245) at com.google.api.client.xml.Xml.parseElement(Xml.java:222) at io.minio.messages.XmlEntity.parseXml(XmlEntity.java:75) at io.minio.messages.ErrorResponse.parseXml(ErrorResponse.java:159) at io.minio.messages.ErrorResponse.<init>(ErrorResponse.java:64) at io.minio.MinioClient.execute(MinioClient.java:970) at io.minio.MinioClient.executePut(MinioClient.java:1197) at io.minio.MinioClient.makeBucket(MinioClient.java:2597) at io.minio.MinioClient.makeBucket(MinioClient.java:2554) at FunctionalTest.makeBucket_test1(FunctionalTest.java:86) at FunctionalTest.runTests(FunctionalTest.java:1264) at FunctionalTest.main(FunctionalTest.java:1380) ``` This patch handles response code and returns Amazon S3 error.

balamurugana · 2017-06-04T02:40:04Z

Please note that this PR returns "Redirect" amazon S3 error, not following new location automatically.

harshavardhana · 2017-06-25T00:16:12Z

What is going on with this PR? @nitisht @balamurugana @vadmeste ?

balamurugana · 2017-06-25T01:41:50Z

@harshavardhana waiting on @nitisht's review

nitisht

I understand it was decided to just respond with ErrorCode.REDIRECT instead of actually redirecting. LGTM in that case

harshavardhana requested review from vadmeste and nitisht May 11, 2017 03:23

vadmeste approved these changes May 11, 2017

View reviewed changes

nitisht reviewed May 13, 2017

View reviewed changes

nitisht suggested changes May 31, 2017

View reviewed changes

balamurugana mentioned this pull request May 31, 2017

server: Redirection should use globalMinioPort with host without port. minio/minio#4445

Merged

8 tasks

balamurugana added 2 commits June 4, 2017 07:52

balamurugana force-pushed the handle-307-code branch from c7204fb to 7a74518 Compare June 4, 2017 02:35

nitisht approved these changes Jun 25, 2017

View reviewed changes

harshavardhana merged commit 423621d into minio:master Jun 25, 2017

balamurugana deleted the handle-307-code branch June 25, 2017 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle http response code 307 - temporary redirect #571

fix: handle http response code 307 - temporary redirect #571

balamurugana commented May 5, 2017 •

edited

vadmeste left a comment

balamurugana commented May 11, 2017 •

edited

nitisht May 13, 2017

nitisht May 31, 2017 •

edited

nitisht May 13, 2017

balamurugana May 13, 2017

nitisht May 16, 2017

harshavardhana commented May 16, 2017

nitisht commented May 16, 2017

balamurugana commented May 16, 2017

harshavardhana commented May 16, 2017

harshavardhana commented May 16, 2017

vadmeste commented May 16, 2017

balamurugana commented May 16, 2017

vadmeste commented May 16, 2017

balamurugana commented May 16, 2017

harshavardhana commented May 31, 2017

nitisht May 31, 2017 •

edited

harshavardhana commented Jun 2, 2017

balamurugana commented Jun 4, 2017

harshavardhana commented Jun 25, 2017

balamurugana commented Jun 25, 2017

nitisht left a comment

fix: handle http response code 307 - temporary redirect #571

fix: handle http response code 307 - temporary redirect #571

Conversation

balamurugana commented May 5, 2017 • edited

vadmeste left a comment

Choose a reason for hiding this comment

balamurugana commented May 11, 2017 • edited

nitisht May 13, 2017

Choose a reason for hiding this comment

nitisht May 31, 2017 • edited

Choose a reason for hiding this comment

nitisht May 13, 2017

Choose a reason for hiding this comment

balamurugana May 13, 2017

Choose a reason for hiding this comment

nitisht May 16, 2017

Choose a reason for hiding this comment

harshavardhana commented May 16, 2017

nitisht commented May 16, 2017

balamurugana commented May 16, 2017

harshavardhana commented May 16, 2017

harshavardhana commented May 16, 2017

vadmeste commented May 16, 2017

balamurugana commented May 16, 2017

vadmeste commented May 16, 2017

balamurugana commented May 16, 2017

harshavardhana commented May 31, 2017

nitisht May 31, 2017 • edited

Choose a reason for hiding this comment

harshavardhana commented Jun 2, 2017

balamurugana commented Jun 4, 2017

harshavardhana commented Jun 25, 2017

balamurugana commented Jun 25, 2017

nitisht left a comment

Choose a reason for hiding this comment

balamurugana commented May 5, 2017 •

edited

balamurugana commented May 11, 2017 •

edited

nitisht May 31, 2017 •

edited

nitisht May 31, 2017 •

edited