Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: 308 response missing Location header #17773

Closed
bradfitz opened this issue Nov 3, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@bradfitz
Copy link
Member

commented Nov 3, 2016

I just tried to upload a binary to Google Cloud Storage using golang.org/x/build/cmd/upload and got:

2016/11/03 17:20:16 Write error: Post https://www.googleapis.com/upload/storage/v1/b/go-builder-data/o?alt=json&projection=full&uploadType=resumable&upload_id=xxxxx: 308 response missing Location header

I suspect this is due to the recent 307/308 support in 7db996e (https://go-review.googlesource.com/29852).

/cc @odeke-em @jba

@bradfitz bradfitz added this to the Go1.8 milestone Nov 3, 2016

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2016

Um, Google is actually replying like that:

2016/11/03 17:26:58 http2: Framer 0xc4201dd380: read HEADERS flags=END_STREAM|END_HEADERS stream=5 len=65
2016/11/03 17:26:58 http2: decoded hpack field header field ":status" = "308"  
2016/11/03 17:26:58 http2: decoded hpack field header field "x-guploader-uploadid" = "AEnB2Urzk0RnIpfPa3ajMQ9S6djxJLJ7W2-xxxx-xxxxx"
2016/11/03 17:26:58 http2: decoded hpack field header field "range" = "bytes=0-8388607"
2016/11/03 17:26:58 http2: decoded hpack field header field "x-range-md5" = "e48f3fcb9ae099cfec5f7fd65d9f8c94"
2016/11/03 17:26:58 http2: decoded hpack field header field "content-length" = "0"
2016/11/03 17:26:58 http2: decoded hpack field header field "date" = "Thu, 03 Nov 2016 17:26:58 GMT"
2016/11/03 17:26:58 http2: decoded hpack field header field "server" = "UploadServer"
2016/11/03 17:26:58 http2: decoded hpack field header field "content-type" = "text/html; charset=UTF-8"
2016/11/03 17:26:58 http2: decoded hpack field header field "alt-svc" = "quic=\":443\"; ma=2592000; v=\"36,35,34\""
2016/11/03 17:26:58 http2: Transport received HEADERS flags=END_STREAM|END_HEADERS stream=5 len=65
2016/11/03 17:26:58 http2: Framer 0xc4201dd380: read PING len=8 ping="\x00\x00\x00\x00\x00\x00\x00\x04"
2016/11/03 17:26:58 http2: Transport received PING len=8 ping="\x00\x00\x00\x00\x00\x00\x00\x04"
2016/11/03 17:26:58 http2: Framer 0xc4201dd380: wrote PING flags=ACK len=8 ping="\x00\x00\x00\x00\x00\x00\x00\x04"  
2016/11/03 17:26:58 Write error: Post https://www.googleapis.com/upload/storage/v1/b/go-builder-data/o?alt=json&projection=full&uploadType=resumable&upload_id=xxx: 308 response missing Location header

And this was the request:

2016/11/03 17:26:58 http2: Transport encoding header ":authority" = "www.googleapis.com"
2016/11/03 17:26:58 http2: Transport encoding header ":method" = "POST"
2016/11/03 17:26:58 http2: Transport encoding header ":path" = "/upload/storage/v1/b/go-builder-data/o?alt=json&projection=full&uploadType=resumable&upload_id=xxxxx-xxxxxx"
2016/11/03 17:26:58 http2: Transport encoding header ":scheme" = "https"
2016/11/03 17:26:58 http2: Transport encoding header "authorization" = "Bearer ya29.xxxxxxxxxx"
2016/11/03 17:26:58 http2: Transport encoding header "content-range" = "bytes 0-8388607/*"
2016/11/03 17:26:58 http2: Transport encoding header "content-type" = "application/octet-stream"
2016/11/03 17:26:58 http2: Transport encoding header "user-agent" = "google-api-go-client/0.5"
2016/11/03 17:26:58 http2: Transport encoding header "content-length" = "8388608"
2016/11/03 17:26:58 http2: Transport encoding header "accept-encoding" = "gzip"
2016/11/03 17:26:58 http2: Framer 0xc4201dd380: wrote HEADERS flags=END_HEADERS stream=5 len=199  
@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2016

Hey @Capstan, can you shed any light on what we're supposed to be doing here? I've never seen a 308 without a Location header. Is that intentional from GCS's side?

@Capstan

This comment has been minimized.

Copy link

commented Nov 3, 2016

I would have thought the UploadServer would always yield the location header. I can get this into the hands of that team. The response type is described at 308 Resume Incomplete. The Location header in this case would just be the same as the header to the method, since you'd need to continue the upload. This cannot be handled with a raw redirect-follow.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2016

Ugh, right. Thanks for the reminder about GCS's 308.

Yeah, we support GCS resumable uploads and such. But previous Go would ignore 308 responses. As of Go 1.8, Go will have support for 307 and 308s (including replaying the request body), which meant Go started handling UploadServer's 308 response as a redirect, instead of the "308 Resume Incomplete" semantics that GCS apparently invented as an alternate meaning for 308.

I think I'll just change the Go storage client to use http.Transport.RoundTrip instead of http.Client.Do, the only major difference being who handles 3xx.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2016

But maybe we should also modify the Go http client to just stop and return the 308 response without an error if it lacks the Location header. That would at least match the behavior in previous Go releases.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 3, 2016

CL https://golang.org/cl/32595 mentions this issue.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2016

Two separate fixes, both of which solve the problem on their own:

The second fix (now submitted) is more specific. The first one is a nice safety net, and will help people who update Go but not their google-api-go-client code.

gopherbot pushed a commit that referenced this issue Nov 4, 2016

net/http: tweak the new Client 307/308 redirect behavior a bit
This CL tweaks the new (unreleased) 307/308 support added in
https://golang.org/cl/29852 for #10767.

Change 1: if a 307/308 response doesn't have a Location header in its
response (as observed in the wild in #17773), just do what we used to
do in Go 1.7 and earlier, and don't try to follow that redirect.

Change 2: don't follow a 307/308 if we sent a body on the first
request and the caller's Request.GetBody func is nil so we can't
"rewind" the body to send it again.

Updates #17773 (will be fixed more elsewhere)

Change-Id: I183570f7346917828a4b6f7f1773094122a30406
Reviewed-on: https://go-review.googlesource.com/32595
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators Nov 3, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.