Fix proxying of unannounced trailers #15

tcolgate · 2017-05-20T15:44:17Z

No description provided.

tcolgate · 2017-05-20T15:46:27Z

timoreimann

A few comments but mostly questions. :-)

timoreimann · 2017-05-24T06:25:57Z

forward/fwd.go

-		return
-	}
-
-	if written != 0 {


What's the reason we're removing this?

It's a set of a header after this initial WriteStatus, which isn't allowed. The only way to do this would be to buffer the entire response, write the content-length, then write the body out. You have to assume that the original handler has already set the content-length. It's unrelated to the actual fix to trailers, but it's definitely not a valid thing to do.

Okay, interesting, didn't know. For the sake of completeness, is this forbidden by Go (and possibly documented somewhere) or rather an underlying HTTP requirement?

I did the same a year ago in 1832129
But I had to rollback it to fix an empty response issue in traefik traefik/traefik#476
Any idea on this @tcolgate ?

timoreimann · 2017-05-24T13:58:29Z

forward/fwd.go

+
+	announcedTrailerKeyCount := len(response.Trailer)
+	if announcedTrailerKeyCount > 0 {
+		trailerKeys := make([]string, 0, len(response.Trailer))


Is it for performance reasons that we don't just append to a nil slice?

timoreimann · 2017-05-24T13:58:53Z

forward/fwd.go

-	written, err := io.Copy(newResponseFlusher(w, stream), response.Body)
+	_, err = io.Copy(newResponseFlusher(w, stream), response.Body)
+	if err != nil {
+		ctx.log.Errorf("Error copying upstream response Body: %v", err)


Nit: Body -> body

timoreimann · 2017-05-24T14:01:23Z

forward/fwd.go

+		return
+	}
+
+	response.Body.Close()


Keeping the defer might be preferable in order to allow future reads of the body inside this method without having to think about adjusting this line. WDYT?

You can't defer the Close() this is the crux of the problem. You must write trailers after the body is closed.

Let's leave a comment then so future developers know.

maybe we can use ioutils.NewReadCloserWrapper

oops it's a docker package...

👍 for comment here

timoreimann · 2017-05-24T19:52:41Z

I forgot: would it be possible to add a test as well? Thanks!

timoreimann · 2017-05-31T20:04:31Z

@tcolgate any updates on this one? do you think it would be possible to add a test and address the nit-picky comments? :)

timoreimann · 2017-05-31T20:07:12Z

For the sake of completion: vulcand/oxy also has an open PR on this issue: vulcand#27

Unfortunately, it never received maintainer attention.

tcolgate · 2017-05-31T20:48:12Z

It'll be a week or so before I get a chance. Re; oxy, they moved over to using httputil.ReverseProxy, which has the same problem, and should get the fix in Go 1.9 hopefully

…

On Wed, 31 May 2017, 21:07 Timo Reimann, ***@***.***> wrote: For the sake of completion: vulcand/oxy also has an open PR on this issue: vulcand/oxy#27 <vulcand#27> Unfortunately, it never received maintainer attention. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEo88VW7VAO4Xmcj_eMctB6LrDaEC1Bks5r_chygaJpZM4NhVUM> .

timoreimann · 2017-05-31T20:53:47Z

@tcolgate thanks for the heads up! whenever you have time.

btw, any chance you could assess if you fix might have a positive impact on traefik/traefik#1686?

tcolgate · 2017-06-01T05:02:05Z

Yes, probably. Though it would be good to know the motivation for the attempted Content-Length write.

…

On Wed, 31 May 2017, 21:53 Timo Reimann, ***@***.***> wrote: @tcolgate <https://github.com/tcolgate> thanks for the heads up! whenever you have time. btw, any chance you could assess if you fix might have a positive impact on traefik/traefik#1686 <traefik/traefik#1686>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEo8-YNgbYW71Nr629Ja3gyNIM0rhlpks5r_dNcgaJpZM4NhVUM> .

emilevauge · 2017-06-15T16:29:43Z

@tcolgate any news on this one ?

tcolgate · 2017-06-26T08:45:47Z

Sorry, not yet, tied up with other stuff at the moment.

juliens · 2017-06-23T14:18:23Z

forward/fwd.go

+		return
+	}
+
+	response.Body.Close()


👍 for comment here

juliens · 2017-06-28T08:47:08Z

forward/fwd.go

+	}
+
+	//No defer on body close in order to force announce trailer after body close
+	response.Body.Close()


In fact, we don't need the response to be closed, we just need that the body was read until EOF

https://github.com/golang/go/blob/master/src/net/http/response.go#L96

juliens · 2017-06-28T09:26:07Z

@tcolgate WDYT about this changes ?

tcolgate · 2017-06-28T09:46:25Z

The writeheader after data has been written is invalid unfortunately (headers are , by definition, only valid at the start of the response). The empty response is likely cause by no write ever having been done on a response before a close in some other path (so you never actually write a status).

ldez

Big improvement!

LGTM

timoreimann

One nit-pick left, but I'll already give my LGTM. :-)

timoreimann · 2017-06-28T13:41:28Z

forward/fwd.go

@@ -178,6 +190,20 @@ func (f *httpForwarder) serveHTTP(w http.ResponseWriter, req *http.Request, ctx
 		}
 	}
 	written, err := io.Copy(newResponseFlusher(w, stream), response.Body)
+	if err != nil {
+		ctx.log.Errorf("Error copying upstream response Body: %v", err)


Still nit-pick, but should still be decapitalized to body. :-)

emilevauge

Thanks @tcolgate & @juliens 👏
@tcolgate we will manage the ContentLength issue separately #19 :)
LGTM

Fix proxying of unannounced trailers

35154ef

ldez self-requested a review May 23, 2017 11:09

timoreimann reviewed May 24, 2017

View reviewed changes

This was referenced May 31, 2017

Traefik prevents media streaming traefik/traefik#1184

Closed

http: multiple response.WriteHeader calls traefik/traefik#1686

Closed

emilevauge added the WIP label Jun 15, 2017

Add comment on body.Close()

f523556

juliens approved these changes Jun 27, 2017

View reviewed changes

forward/fwd.go Outdated

return

}

response.Body.Close()

Copy link

Member

juliens Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for comment here

juliens reviewed Jun 28, 2017

View reviewed changes

juliens added 3 commits June 28, 2017 11:09

Unannounced trailers tests

052869e

Keep defer body close

db789b9

Rollback on Content-Length Header

8ce11ac

juliens added 2 commits June 28, 2017 14:56

Wrong imports order

17624a6

Reuse precalculated len

83b0768

ldez added enhancement and removed WIP labels Jun 28, 2017

ldez approved these changes Jun 28, 2017

View reviewed changes

timoreimann approved these changes Jun 28, 2017

View reviewed changes

fix nit-pick

e38434b

emilevauge approved these changes Jun 28, 2017

View reviewed changes

ldez merged commit 7da864c into containous:master Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix proxying of unannounced trailers #15

Fix proxying of unannounced trailers #15

tcolgate commented May 20, 2017

tcolgate commented May 20, 2017 •

edited

timoreimann left a comment

timoreimann May 24, 2017

tcolgate May 24, 2017

timoreimann May 24, 2017 •

edited

emilevauge Jun 27, 2017

timoreimann May 24, 2017

timoreimann May 24, 2017

timoreimann May 24, 2017

tcolgate May 24, 2017

timoreimann May 24, 2017

ldez May 24, 2017

ldez May 24, 2017

juliens Jun 23, 2017

timoreimann commented May 24, 2017

timoreimann commented May 31, 2017

timoreimann commented May 31, 2017

tcolgate commented May 31, 2017 via email

timoreimann commented May 31, 2017

tcolgate commented Jun 1, 2017 via email

emilevauge commented Jun 15, 2017

tcolgate commented Jun 26, 2017

juliens Jun 23, 2017

juliens Jun 28, 2017

juliens commented Jun 28, 2017

tcolgate commented Jun 28, 2017

ldez left a comment

timoreimann left a comment

timoreimann Jun 28, 2017

emilevauge left a comment

Fix proxying of unannounced trailers #15

Fix proxying of unannounced trailers #15

Conversation

tcolgate commented May 20, 2017

tcolgate commented May 20, 2017 • edited

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann May 24, 2017 • edited

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

timoreimann commented May 24, 2017

timoreimann commented May 31, 2017

timoreimann commented May 31, 2017

tcolgate commented May 31, 2017 via email

timoreimann commented May 31, 2017

tcolgate commented Jun 1, 2017 via email

emilevauge commented Jun 15, 2017

tcolgate commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliens commented Jun 28, 2017

tcolgate commented Jun 28, 2017

ldez left a comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

tcolgate commented May 20, 2017 •

edited

timoreimann May 24, 2017 •

edited