code review 57370043: httputil: in ReverseProxy, strip hop-by-hop headers and...

net/http/httputil: strip hop-by-hop headers in ReverseProxy

Fixes issue 5967.

[fredr, 2014-01-27 19:34:00]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-01-27 19:36:52]

https://codereview.appspot.com/57370043/diff/40001/src/pkg/net/http/httputil/...
File src/pkg/net/http/httputil/reverseproxy.go (right):

https://codereview.appspot.com/57370043/diff/40001/src/pkg/net/http/httputil/...
src/pkg/net/http/httputil/reverseproxy.go:79: dst.Set(k, v)
Why? Seems wrong. This will lose multi-valued headers.

[fredr, 2014-01-27 19:54:23]

On 2014/01/27 19:36:52, bradfitz wrote:
>
https://codereview.appspot.com/57370043/diff/40001/src/pkg/net/http/httputil/...
> File src/pkg/net/http/httputil/reverseproxy.go (right):
> 
>
https://codereview.appspot.com/57370043/diff/40001/src/pkg/net/http/httputil/...
> src/pkg/net/http/httputil/reverseproxy.go:79: dst.Set(k, v)
> Why? Seems wrong. This will lose multi-valued headers.

No, it will actually preserve multi-valued headers, but if both the backend and
the proxy sets the header, it will only be the backends headers that will be
preserved.
This makes sense in our projects, but if you don't agree, I'll remove it.

The reason why it works is that http.Header is actually a map[string][]string so
in our loop v will be the array of all values

If you want it to stay, I can add a test for it, otherwise I'll remove it.

[bradfitz, 2014-01-27 19:59:06]

On Mon, Jan 27, 2014 at 11:54 AM,
<fredrik.enestad@soundtrackyourbrand.com>wrote:

> On 2014/01/27 19:36:52, bradfitz wrote:
>
> https://codereview.appspot.com/57370043/diff/40001/src/
> pkg/net/http/httputil/reverseproxy.go
>
>> File src/pkg/net/http/httputil/reverseproxy.go (right):
>>
>
>
> https://codereview.appspot.com/57370043/diff/40001/src/
> pkg/net/http/httputil/reverseproxy.go#newcode79
>
>> src/pkg/net/http/httputil/reverseproxy.go:79: dst.Set(k, v)
>> Why? Seems wrong. This will lose multi-valued headers.
>>
>
> No, it will actually preserve multi-valued headers,


I don't see how.


> but if both the
> backend and the proxy sets the header, it will only be the backends
> headers that will be preserved.
> This makes sense in our projects, but if you don't agree, I'll remove
> it.
>

This CL should only do one thing: remove hop-by-hop headers.  Don't change
the behavior of unrelated stuff.


> The reason why it works is that http.Header is actually a
> map[string][]string so in our loop v will be the array of all values

If you want it to stay, I can add a test for it, otherwise I'll remove
> it.
>

Yes, please add a test and restore the behavior.

It should be possible for a backend to send:

Foo: bar1
Foo: bar2

[fredr, 2014-01-27 20:34:02]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[fredr, 2014-01-27 20:34:20]

On 2014/01/27 19:59:06, bradfitz wrote:
> On Mon, Jan 27, 2014 at 11:54 AM,
> <mailto:fredrik.enestad@soundtrackyourbrand.com>wrote:
> 
> > On 2014/01/27 19:36:52, bradfitz wrote:
> >
> > https://codereview.appspot.com/57370043/diff/40001/src/
> > pkg/net/http/httputil/reverseproxy.go
> >
> >> File src/pkg/net/http/httputil/reverseproxy.go (right):
> >>
> >
> >
> > https://codereview.appspot.com/57370043/diff/40001/src/
> > pkg/net/http/httputil/reverseproxy.go#newcode79
> >
> >> src/pkg/net/http/httputil/reverseproxy.go:79: dst.Set(k, v)
> >> Why? Seems wrong. This will lose multi-valued headers.
> >>
> >
> > No, it will actually preserve multi-valued headers,
> 
> 
> I don't see how.

Your right

> 
> 
> > but if both the
> > backend and the proxy sets the header, it will only be the backends
> > headers that will be preserved.
> > This makes sense in our projects, but if you don't agree, I'll remove
> > it.
> >
> 
> This CL should only do one thing: remove hop-by-hop headers.  Don't change
> the behavior of unrelated stuff.

Ok, I did take it up in the discussion, but I should probably add a separate
ticket for that then

> 
> 
> > The reason why it works is that http.Header is actually a
> > map[string][]string so in our loop v will be the array of all values
> 
> If you want it to stay, I can add a test for it, otherwise I'll remove
> > it.
> >
> 
> Yes, please add a test and restore the behavior.
> 
> It should be possible for a backend to send:
> 
> Foo: bar1
> Foo: bar2

Done!

[bradfitz, 2014-01-27 22:23:41]

https://codereview.appspot.com/57370043/diff/80001/src/pkg/net/http/httputil/...
File src/pkg/net/http/httputil/reverseproxy_test.go (right):

https://codereview.appspot.com/57370043/diff/80001/src/pkg/net/http/httputil/...
src/pkg/net/http/httputil/reverseproxy_test.go:40:
w.Header().Set("Transfer-Encoding", "chunked")
This is causing noise in the tests, with a warning from net/http:

=== RUN TestReverseProxy
2014/01/27 14:20:18 http: WriteHeader called with both Transfer-Encoding of
"chunked" and a Content-Length of 16
--- PASS: TestReverseProxy (0.00 seconds)

Because the net/http package itself owns setting Content-Type and
Transfer-Encoding, and went to set its own Content-Length but noticed this
confusing header set.

How about this:

At the top of this test, do:

const fakeHopHeader = "X-Fake-Hop-Header-For-Test"

func init() {
    hopHeaders = append(hopHeaders, fakeHopHeader)
}

And then use fakeHopHeader instead of "Transfer-Encoding".  (And you can set the
value to "foo" instead of "chunked".)

[fredr, 2014-01-27 22:48:05]

On 2014/01/27 22:23:41, bradfitz wrote:
>
https://codereview.appspot.com/57370043/diff/80001/src/pkg/net/http/httputil/...
> File src/pkg/net/http/httputil/reverseproxy_test.go (right):
> 
>
https://codereview.appspot.com/57370043/diff/80001/src/pkg/net/http/httputil/...
> src/pkg/net/http/httputil/reverseproxy_test.go:40:
> w.Header().Set("Transfer-Encoding", "chunked")
> This is causing noise in the tests, with a warning from net/http:
> 
> === RUN TestReverseProxy
> 2014/01/27 14:20:18 http: WriteHeader called with both Transfer-Encoding of
> "chunked" and a Content-Length of 16
> --- PASS: TestReverseProxy (0.00 seconds)
> 
> Because the net/http package itself owns setting Content-Type and
> Transfer-Encoding, and went to set its own Content-Length but noticed this
> confusing header set.
> 
> How about this:
> 
> At the top of this test, do:
> 
> const fakeHopHeader = "X-Fake-Hop-Header-For-Test"
> 
> func init() {
>     hopHeaders = append(hopHeaders, fakeHopHeader)
> }
> 
> And then use fakeHopHeader instead of "Transfer-Encoding".  (And you can set
the
> value to "foo" instead of "chunked".)

Magic! I'v updated the code

[bradfitz, 2014-01-27 23:23:50]

LGTM



On Mon, Jan 27, 2014 at 2:48 PM,
<fredrik.enestad@soundtrackyourbrand.com>wrote:

> On 2014/01/27 22:23:41, bradfitz wrote:
>
> https://codereview.appspot.com/57370043/diff/80001/src/
> pkg/net/http/httputil/reverseproxy_test.go
>
>> File src/pkg/net/http/httputil/reverseproxy_test.go (right):
>>
>
>
> https://codereview.appspot.com/57370043/diff/80001/src/
> pkg/net/http/httputil/reverseproxy_test.go#newcode40
>
>> src/pkg/net/http/httputil/reverseproxy_test.go:40:
>> w.Header().Set("Transfer-Encoding", "chunked")
>> This is causing noise in the tests, with a warning from net/http:
>>
>
>  === RUN TestReverseProxy
>> 2014/01/27 14:20:18 http: WriteHeader called with both
>>
> Transfer-Encoding of
>
>> "chunked" and a Content-Length of 16
>> --- PASS: TestReverseProxy (0.00 seconds)
>>
>
>  Because the net/http package itself owns setting Content-Type and
>> Transfer-Encoding, and went to set its own Content-Length but noticed
>>
> this
>
>> confusing header set.
>>
>
>  How about this:
>>
>
>  At the top of this test, do:
>>
>
>  const fakeHopHeader = "X-Fake-Hop-Header-For-Test"
>>
>
>  func init() {
>>      hopHeaders = append(hopHeaders, fakeHopHeader)
>> }
>>
>
>  And then use fakeHopHeader instead of "Transfer-Encoding".  (And you
>>
> can set the
>
>> value to "foo" instead of "chunked".)
>>
>
> Magic! I'v updated the code
>
> https://codereview.appspot.com/57370043/
>

[bradfitz, 2014-01-27 23:26:01]

Please change CL description first line to read: "net/http/httputil: strip
hop-by-hop headers in ReverseProxy"



On Mon, Jan 27, 2014 at 3:23 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
>
>
> On Mon, Jan 27, 2014 at 2:48 PM,
<fredrik.enestad@soundtrackyourbrand.com>wrote:
>
>> On 2014/01/27 22:23:41, bradfitz wrote:
>>
>> https://codereview.appspot.com/57370043/diff/80001/src/
>> pkg/net/http/httputil/reverseproxy_test.go
>>
>>> File src/pkg/net/http/httputil/reverseproxy_test.go (right):
>>>
>>
>>
>> https://codereview.appspot.com/57370043/diff/80001/src/
>> pkg/net/http/httputil/reverseproxy_test.go#newcode40
>>
>>> src/pkg/net/http/httputil/reverseproxy_test.go:40:
>>> w.Header().Set("Transfer-Encoding", "chunked")
>>> This is causing noise in the tests, with a warning from net/http:
>>>
>>
>>  === RUN TestReverseProxy
>>> 2014/01/27 14:20:18 http: WriteHeader called with both
>>>
>> Transfer-Encoding of
>>
>>> "chunked" and a Content-Length of 16
>>> --- PASS: TestReverseProxy (0.00 seconds)
>>>
>>
>>  Because the net/http package itself owns setting Content-Type and
>>> Transfer-Encoding, and went to set its own Content-Length but noticed
>>>
>> this
>>
>>> confusing header set.
>>>
>>
>>  How about this:
>>>
>>
>>  At the top of this test, do:
>>>
>>
>>  const fakeHopHeader = "X-Fake-Hop-Header-For-Test"
>>>
>>
>>  func init() {
>>>      hopHeaders = append(hopHeaders, fakeHopHeader)
>>> }
>>>
>>
>>  And then use fakeHopHeader instead of "Transfer-Encoding".  (And you
>>>
>> can set the
>>
>>> value to "foo" instead of "chunked".)
>>>
>>
>> Magic! I'v updated the code
>>
>> https://codereview.appspot.com/57370043/
>>
>
>

[fredr, 2014-01-27 23:29:21]

On 2014/01/27 23:26:01, bradfitz wrote:
> Please change CL description first line to read: "net/http/httputil: strip
> hop-by-hop headers in ReverseProxy"
> 
> 
> 
> On Mon, Jan 27, 2014 at 3:23 PM, Brad Fitzpatrick
<mailto:bradfitz@golang.org>wrote:
> 
> > LGTM
> >
> >
> >
> > On Mon, Jan 27, 2014 at 2:48 PM,
> <mailto:fredrik.enestad@soundtrackyourbrand.com>wrote:
> >
> >> On 2014/01/27 22:23:41, bradfitz wrote:
> >>
> >> https://codereview.appspot.com/57370043/diff/80001/src/
> >> pkg/net/http/httputil/reverseproxy_test.go
> >>
> >>> File src/pkg/net/http/httputil/reverseproxy_test.go (right):
> >>>
> >>
> >>
> >> https://codereview.appspot.com/57370043/diff/80001/src/
> >> pkg/net/http/httputil/reverseproxy_test.go#newcode40
> >>
> >>> src/pkg/net/http/httputil/reverseproxy_test.go:40:
> >>> w.Header().Set("Transfer-Encoding", "chunked")
> >>> This is causing noise in the tests, with a warning from net/http:
> >>>
> >>
> >>  === RUN TestReverseProxy
> >>> 2014/01/27 14:20:18 http: WriteHeader called with both
> >>>
> >> Transfer-Encoding of
> >>
> >>> "chunked" and a Content-Length of 16
> >>> --- PASS: TestReverseProxy (0.00 seconds)
> >>>
> >>
> >>  Because the net/http package itself owns setting Content-Type and
> >>> Transfer-Encoding, and went to set its own Content-Length but noticed
> >>>
> >> this
> >>
> >>> confusing header set.
> >>>
> >>
> >>  How about this:
> >>>
> >>
> >>  At the top of this test, do:
> >>>
> >>
> >>  const fakeHopHeader = "X-Fake-Hop-Header-For-Test"
> >>>
> >>
> >>  func init() {
> >>>      hopHeaders = append(hopHeaders, fakeHopHeader)
> >>> }
> >>>
> >>
> >>  And then use fakeHopHeader instead of "Transfer-Encoding".  (And you
> >>>
> >> can set the
> >>
> >>> value to "foo" instead of "chunked".)
> >>>
> >>
> >> Magic! I'v updated the code
> >>
> >> https://codereview.appspot.com/57370043/
> >>
> >
> >

Done!

[bradfitz, 2014-01-27 23:32:14]

Whoops. The submit went through (halfway) before I noticed the commit
description.

Submitted in https://code.google.com/p/go/source/detail?r=cd8d13f7fcd67



On Mon, Jan 27, 2014 at 3:29 PM,
<fredrik.enestad@soundtrackyourbrand.com>wrote:

> On 2014/01/27 23:26:01, bradfitz wrote:
>
>> Please change CL description first line to read: "net/http/httputil:
>>
> strip
>
>> hop-by-hop headers in ReverseProxy"
>>
>
>
>
>  On Mon, Jan 27, 2014 at 3:23 PM, Brad Fitzpatrick
>>
> <mailto:bradfitz@golang.org>wrote:
>
>  > LGTM
>> >
>> >
>> >
>> > On Mon, Jan 27, 2014 at 2:48 PM,
>> <mailto:fredrik.enestad@soundtrackyourbrand.com>wrote:
>>
>> >
>> >> On 2014/01/27 22:23:41, bradfitz wrote:
>> >>
>> >> https://codereview.appspot.com/57370043/diff/80001/src/
>> >> pkg/net/http/httputil/reverseproxy_test.go
>> >>
>> >>> File src/pkg/net/http/httputil/reverseproxy_test.go (right):
>> >>>
>> >>
>> >>
>> >> https://codereview.appspot.com/57370043/diff/80001/src/
>> >> pkg/net/http/httputil/reverseproxy_test.go#newcode40
>> >>
>> >>> src/pkg/net/http/httputil/reverseproxy_test.go:40:
>> >>> w.Header().Set("Transfer-Encoding", "chunked")
>> >>> This is causing noise in the tests, with a warning from net/http:
>> >>>
>> >>
>> >>  === RUN TestReverseProxy
>> >>> 2014/01/27 14:20:18 http: WriteHeader called with both
>> >>>
>> >> Transfer-Encoding of
>> >>
>> >>> "chunked" and a Content-Length of 16
>> >>> --- PASS: TestReverseProxy (0.00 seconds)
>> >>>
>> >>
>> >>  Because the net/http package itself owns setting Content-Type and
>> >>> Transfer-Encoding, and went to set its own Content-Length but
>>
> noticed
>
>> >>>
>> >> this
>> >>
>> >>> confusing header set.
>> >>>
>> >>
>> >>  How about this:
>> >>>
>> >>
>> >>  At the top of this test, do:
>> >>>
>> >>
>> >>  const fakeHopHeader = "X-Fake-Hop-Header-For-Test"
>> >>>
>> >>
>> >>  func init() {
>> >>>      hopHeaders = append(hopHeaders, fakeHopHeader)
>> >>> }
>> >>>
>> >>
>> >>  And then use fakeHopHeader instead of "Transfer-Encoding".  (And
>>
> you
>
>> >>>
>> >> can set the
>> >>
>> >>> value to "foo" instead of "chunked".)
>> >>>
>> >>
>> >> Magic! I'v updated the code
>> >>
>> >> https://codereview.appspot.com/57370043/
>> >>
>> >
>> >
>>
>
> Done!
>
> https://codereview.appspot.com/57370043/
>

[rsc, 2014-03-05 20:03:25]

R=close