code review 4248075: http: move RemoteAddr & UsingTLS from ResponseWriter to...

http: move RemoteAddr & UsingTLS from ResponseWriter to Request

ResponseWriter.RemoteAddr() string -> Request.RemoteAddr string
ResponseWriter.UsingTLS() bool -> Request.TLS *tls.ConnectionState

[bradfitz, 2011-03-09 19:44:54]

Hello rsc (cc: gburd, golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-03-09 19:46:25]

Russ,

I left the docs XXXed out.  You want to wordsmith?

On Wed, Mar 9, 2011 at 11:44 AM, <bradfitz@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: gburd, golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: move RemoteAddr & UsingTLS from ResponseWriter to Request
>
> ResponseWriter.RemoteAddr() string -> Request.RemoteAddr string
> ResponseWriter.UsingTLS() bool -> Request.TLS *tls.ConnectionState
>
> Please review this at http://codereview.appspot.com/4248075/
>
> Affected files:
>  M src/cmd/godoc/main.go
>  M src/pkg/http/cgi/host.go
>  M src/pkg/http/cgi/host_test.go
>  M src/pkg/http/httptest/recorder.go
>  M src/pkg/http/request.go
>  M src/pkg/http/serve_test.go
>  M src/pkg/http/server.go
>  M src/pkg/rpc/server.go
>  M src/pkg/websocket/server.go
>
>
>

[bradfitzgoog, 2011-03-10 04:40:19]

Ping.  Not sure if this was missed or whether we're not ready for it yet
(still debating design?).

I'd like to add []*tls.PeerCertificates to Request too if that's where we're
moving client details.  (unless we want to hang that off
tls.ConnectionState, Adam?)

(background for others:
https://docs.google.com/document/d/1e7xPAIlJqMm50n2nCndxbIJwup2ES7x5ukg8VmT-S...
 )

On Wed, Mar 9, 2011 at 11:44 AM, <bradfitz@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: gburd, golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: move RemoteAddr & UsingTLS from ResponseWriter to Request
>
> ResponseWriter.RemoteAddr() string -> Request.RemoteAddr string
> ResponseWriter.UsingTLS() bool -> Request.TLS *tls.ConnectionState
>
> Please review this at http://codereview.appspot.com/4248075/
>
> Affected files:
>  M src/cmd/godoc/main.go
>  M src/pkg/http/cgi/host.go
>  M src/pkg/http/cgi/host_test.go
>  M src/pkg/http/httptest/recorder.go
>  M src/pkg/http/request.go
>  M src/pkg/http/serve_test.go
>  M src/pkg/http/server.go
>  M src/pkg/rpc/server.go
>  M src/pkg/websocket/server.go
>
>
>

[rsc, 2011-03-10 04:52:27]

I was hoping to move it into tls.ConnectionState.

[bradfitzgoog, 2011-03-10 06:10:01]

Looks easy to do if Adam doesn't object.  Just copy the peer list slice into
a new field on the returned ConnectionState copy.  The peer list looks
immutable.  Would that weird having two paths to the certificates from the
tls packge, though?

Separate CL after this one or include it here?

On Wed, Mar 9, 2011 at 8:52 PM, Russ Cox <rsc@golang.org> wrote:

> I was hoping to move it into tls.ConnectionState.
>

[bradfitz, 2011-03-10 15:27:47]

Hello rsc, bradfitzwork (cc: gburd, golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-03-10 15:29:15]

I've filled in docs now.  PTAL.

Doc tweaks welcome, though.

On Thu, Mar 10, 2011 at 7:27 AM, <bradfitz@golang.org> wrote:

> Hello rsc, bradfitzwork (cc: gburd, golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
> http://codereview.appspot.com/4248075/
>

[rsc, 2011-03-10 15:40:46]

LGTM

ResponseWriter is starting to look good!

http://codereview.appspot.com/4248075/diff/2003/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4248075/diff/2003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:67: // In HTTP Handlers, Request also contains
additional information about
delete
(might apply to other contexts too, leave for below)

http://codereview.appspot.com/4248075/diff/2003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:144: // RemoteAddr is the remote user's address for
logging
// RemoteAddr allows HTTP servers and other software to record
// the network address that sent the request, usually for logging.
// This field is not filled in by ReadRequest and has no defined format.
// The HTTP server in this package sets RemoteAddr to an IP address
// before invoking a handler.

http://codereview.appspot.com/4248075/diff/2003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:149: // TLS contains the TLS ConnectionState, if the
request being
// TLS allows HTTP servers and other software to record information about
// the TLS connection on which the request was received.
// This field is not filled in by ReadRequest.
// The HTTP server in this package sets the field for TLS-enabled
// connections before invoking a handler; otherwise it leaves the field nil.

[bradfitz, 2011-03-10 15:47:26]

Cool.  Changed this sentence slightly:

// The HTTP server in this package

// sets RemoteAddr to an "IP:port" address before invoking a

// handler.

... to show that the port is included.

On Thu, Mar 10, 2011 at 7:40 AM, <rsc@golang.org> wrote:

> LGTM
>
> ResponseWriter is starting to look good!
>
>
>
> http://codereview.appspot.com/4248075/diff/2003/src/pkg/http/request.go
> File src/pkg/http/request.go (right):
>
>
>
http://codereview.appspot.com/4248075/diff/2003/src/pkg/http/request.go#newco...
> src/pkg/http/request.go:67: // In HTTP Handlers, Request also contains
> additional information about
> delete
> (might apply to other contexts too, leave for below)
>
>
>
http://codereview.appspot.com/4248075/diff/2003/src/pkg/http/request.go#newco...
> src/pkg/http/request.go:144: // RemoteAddr is the remote user's address
> for logging
> // RemoteAddr allows HTTP servers and other software to record
> // the network address that sent the request, usually for logging.
> // This field is not filled in by ReadRequest and has no defined format.
> // The HTTP server in this package sets RemoteAddr to an IP address
> // before invoking a handler.
>
>
>
http://codereview.appspot.com/4248075/diff/2003/src/pkg/http/request.go#newco...
> src/pkg/http/request.go:149: // TLS contains the TLS ConnectionState, if
> the request being
> // TLS allows HTTP servers and other software to record information
> about
> // the TLS connection on which the request was received.
> // This field is not filled in by ReadRequest.
> // The HTTP server in this package sets the field for TLS-enabled
> // connections before invoking a handler; otherwise it leaves the field
> nil.
>
>
> http://codereview.appspot.com/4248075/
>

[rsc, 2011-03-10 15:50:17]

LGTM

I think maybe we should drop the port but I will do that
separately once net.SplitHostPort is exported.

[bradfitz, 2011-03-10 16:04:26]

We could split it off from RemoteAddr if we stick it elsewhere (another
field in Request?), but it has enough random uses that I'd rather keep it
somewhere.

On Thu, Mar 10, 2011 at 7:50 AM, Russ Cox <rsc@golang.org> wrote:

> LGTM
>
> I think maybe we should drop the port but I will do that
> separately once net.SplitHostPort is exported.
>

[rsc, 2011-03-10 16:09:42]

On Thu, Mar 10, 2011 at 11:04, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> We could split it off from RemoteAddr if we stick it elsewhere (another
> field in Request?), but it has enough random uses that I'd rather keep it
> somewhere.

Okay, then we can leave it as is.

[bradfitz, 2011-03-10 16:17:29]

*** Submitted as http://code.google.com/p/go/source/detail?r=0e09be41e7a5 ***

http: move RemoteAddr & UsingTLS from ResponseWriter to Request

ResponseWriter.RemoteAddr() string -> Request.RemoteAddr string
ResponseWriter.UsingTLS() bool -> Request.TLS *tls.ConnectionState

R=rsc, bradfitzwork
CC=gburd, golang-dev
http://codereview.appspot.com/4248075