code review 102770046: go.tools/{cmd/present,playground/socket}: add orighost ...

go.tools/{cmd/present,playground/socket}: add orighost flag to handle the web origin more flexible

Also fixes the following nits;
- literal IPv6 address handling
- URL host component handling in the case of a wildcard listen
- URL port component handling in the case of no port component in origin

Fixes issue 8096.

[mikio, 2014-05-28 03:03:32]

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

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

[kortschak, 2014-05-28 04:14:19]

On 2014/05/28 03:03:32, mikio wrote:
> Hello mailto:adg@golang.org, mailto:golang-codereviews@googlegroups.com (cc:
> mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go.tools

This does not fix 8096, at least not as I intended it to be understood.

There still needs to be logic in go.tool/platground/socket to allow accepted
hosts to connect - this works when a single known client connects, but not for
an arbitrary set of clients. When NaCl support comes in that is a reasonable
expectation.

[mikio, 2014-05-28 04:25:00]

On Wed, May 28, 2014 at 1:14 PM,  <dan.kortschak@adelaide.edu.au> wrote:

> This does not fix 8096, at least not as I intended it to be understood.

ouch.

> There still needs to be logic in go.tool/platground/socket to allow
> accepted hosts to connect - this works when a single known client
> connects, but not for an arbitrary set of clients. When NaCl support
> comes in that is a reasonable expectation.

seems i'm missing something. could you please elaborate on the use
case, especially why "present -http=:5963 -urlhost=www.example.com"
not work for you. thanks.

[kortschak, 2014-05-28 04:37:46]

On 2014/05/28 04:25:00, mikio wrote:
> On Wed, May 28, 2014 at 1:14 PM,  <mailto:dan.kortschak@adelaide.edu.au>
wrote:
> 
> > This does not fix 8096, at least not as I intended it to be understood.
> 
> ouch.
> 
> > There still needs to be logic in http://go.tool/platground/socket to allow
> > accepted hosts to connect - this works when a single known client
> > connects, but not for an arbitrary set of clients. When NaCl support
> > comes in that is a reasonable expectation.
> 
> seems i'm missing something. could you please elaborate on the use
> case, especially why "present -http=:5963 -urlhost=http://www.example.com"
> not work for you. thanks.

Sure. The use cases for me are 1) a simple situation where I host a presentation
on my workstation and present it from an institutional client whose address or
hostname I may not be able to know before hand or even at all depending on its
setup, and 2) I host a presentation on my workstation or server using the NaCl
environment, allowing students/workshop attendees to play with examples given in
the presentation.

[kortschak, 2014-05-28 05:24:23]

LGTM

I have tried with another client and I can get this to work now - there were
issues with iPad connection. Apologies for the confusion.

[mikio, 2014-05-28 05:59:14]

Hello adg@golang.org, golang-codereviews@googlegroups.com,
dan.kortschak@adelaide.edu.au (cc: golang-codereviews@googlegroups.com),

Please take another look.

[mikio, 2014-05-28 06:15:13]

On Wed, May 28, 2014 at 1:37 PM,  <dan.kortschak@adelaide.edu.au> wrote:

> Sure. The use cases for me are 1) a simple situation where I host a
> presentation on my workstation and present it from an institutional
> client whose address or hostname I may not be able to know before hand
> or even at all depending on its setup, and 2) I host a presentation on
> my workstation or server using the NaCl environment, allowing
> students/workshop attendees to play with examples given in the
> presentation.

looks like you need a pretty pertty service discovery function. let me
say; for on-link clients, you can use dns-based, mdns, upnp or other,
discovery or proprietary functions. either is an all-in-one solution.
for off-link clients, a bit complicated, advertising your
service+location on the net using dyndns+dns-sd would be a solution.
also, you might be needed to make sure connectivity to your present
instance if your present server stays behind nat or other translators.
in that case using pubic stun servers or proprietary
nat/xlat-traversal function would be a solution.

[mikio, 2014-05-28 06:16:51]

On Wed, May 28, 2014 at 2:24 PM,  <dan.kortschak@adelaide.edu.au> wrote:

> I have tried with another client and I can get this to work now - there
> were issues with iPad connection. Apologies for the confusion.

glad to hear that.

[mikio, 2014-05-29 02:33:28]

adg?

[adg, 2014-05-29 04:05:47]

https://codereview.appspot.com/102770046/diff/120001/cmd/present/local.go
File cmd/present/local.go (right):

https://codereview.appspot.com/102770046/diff/120001/cmd/present/local.go#new...
cmd/present/local.go:28: originHost   string
why make this a global var?

https://codereview.appspot.com/102770046/diff/120001/cmd/present/local.go#new...
cmd/present/local.go:65: origin.Host = ln.Addr().String()
what does this set origin.Host to if you write -http=:3999 ?

[mikio, 2014-05-29 04:22:50]

ptal

https://codereview.appspot.com/102770046/diff/120001/cmd/present/local.go
File cmd/present/local.go (right):

https://codereview.appspot.com/102770046/diff/120001/cmd/present/local.go#new...
cmd/present/local.go:28: originHost   string
On 2014/05/29 04:05:46, adg wrote:
> why make this a global var?

moved

https://codereview.appspot.com/102770046/diff/120001/cmd/present/local.go#new...
cmd/present/local.go:65: origin.Host = ln.Addr().String()
On 2014/05/29 04:05:46, adg wrote:
> what does this set origin.Host to if you write -http=:3999 ?

for now we don't have any good uniform to represent an unspecified IPv4 or IPv6
address that indicates it's booked as a wildcard address". so, it represents
[::]:3999 or 0.0.0.0:3999.  please let me know if you prefer a blank host
component or a hostname. fwiw, almost all the platforms support routing
host-orignated packets toward unspecified address to the loopback address, as a
conventional way, except windows (see golang.org/issue/6290).

[adg, 2014-05-29 04:25:04]

https://codereview.appspot.com/102770046/diff/120001/cmd/present/local.go
File cmd/present/local.go (right):

https://codereview.appspot.com/102770046/diff/120001/cmd/present/local.go#new...
cmd/present/local.go:65: origin.Host = ln.Addr().String()
On 2014/05/29 04:22:50, mikio wrote:
> On 2014/05/29 04:05:46, adg wrote:
> > what does this set origin.Host to if you write -http=:3999 ?
> 
> for now we don't have any good uniform to represent an unspecified IPv4 or
IPv6
> address that indicates it's booked as a wildcard address". so, it represents
> [::]:3999 or 0.0.0.0:3999.  please let me know if you prefer a blank host
> component or a hostname. fwiw, almost all the platforms support routing
> host-orignated packets toward unspecified address to the loopback address, as
a
> conventional way, except windows (see golang.org/issue/6290).

So this means the websocket connection will not work if a user specifies
-http=:3999.

What I would prefer is to simply disallow omitting the hostname or IP address.
Users should have to explicitly state which interface they want to listen on,
unless -play=false. This would remove the need for the -originhost flag.

Does that make sense to you?

[mikio, 2014-05-29 04:45:51]

On Thu, May 29, 2014 at 1:25 PM,  <adg@golang.org> wrote:

> So this means the websocket connection will not work if a user specifies
> -http=:3999.

unless you give -orighost=<reachable fqdn or hostname>. when we
specify -orighost like "present -http=:3999 -orighost=www.example.com"
any clients that access to www.example.com can use websocket.

> What I would prefer is to simply disallow omitting the hostname or IP
> address. Users should have to explicitly state which interface they want
> to listen on, unless -play=false. This would remove the need for the
> -originhost flag.
>
> Does that make sense to you?

what if we deploy a present instance behind nat or other ip address
translators?

[adg, 2014-05-29 04:47:55]

On 29 May 2014 14:45, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> On Thu, May 29, 2014 at 1:25 PM,  <adg@golang.org> wrote:
>
> > So this means the websocket connection will not work if a user specifies
> > -http=:3999.
>
> unless you give -orighost=<reachable fqdn or hostname>. when we
> specify -orighost like "present -http=:3999 -orighost=www.example.com"
> any clients that access to www.example.com can use websocket.
>

Well, can we have -orighost default to the value given to -http ?
I'd just prefer not to have to specify multiple flags to do the simple
thing.

> What I would prefer is to simply disallow omitting the hostname or IP
> > address. Users should have to explicitly state which interface they want
> > to listen on, unless -play=false. This would remove the need for the
> > -originhost flag.
> >
> > Does that make sense to you?
>
> what if we deploy a present instance behind nat or other ip address
> translators?
>

That's a good point, I suppose.

[kortschak, 2014-05-29 04:56:43]

On 2014/05/29 04:47:55, adg wrote:
> > what if we deploy a present instance behind nat or other ip address
> > translators?
> >
> 
> That's a good point, I suppose.

Also -nacl=true.

[adg, 2014-05-29 04:57:59]

On 29 May 2014 14:56, <dan.kortschak@adelaide.edu.au> wrote:

> Also -nacl=true.


What do you mean?

[kortschak, 2014-05-29 05:02:30]

On 2014/05/29 04:57:59, adg wrote:
> On 29 May 2014 14:56, <mailto:dan.kortschak@adelaide.edu.au> wrote:
> 
> > Also -nacl=true.
> 
> 
> What do you mean?

I'm thinking when -nacl=true the behaviour should be pretty close to -play=false
as far as strictness goes.

[adg, 2014-05-29 05:04:33]

On 29 May 2014 15:02, <dan.kortschak@adelaide.edu.au> wrote:

> I'm thinking when -nacl=true the behaviour should be pretty close to
> -play=false as far as strictness goes.
>

Oh, right, yes.

[mikio, 2014-05-29 05:15:06]

On Thu, May 29, 2014 at 1:47 PM, Andrew Gerrand <adg@golang.org> wrote:

> Well, can we have -orighost default to the value given to -http ?

if we do that deploying a present instance on multihomed node would be
tough. let me say, www.example.com has ip addresses 1.2.3.4 and
5.6.6.8 and the latter is used for some internal purposes, how could
we know that information? I think only thing we can do for now is
providing an individual control knob for each namespace; IP address
namespace for routing, transporting packets and web namespace for
locating resources.

[adg, 2014-05-29 05:17:13]

On 29 May 2014 15:15, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> if we do that deploying a present instance on multihomed node would be
> tough. let me say, www.example.com has ip addresses 1.2.3.4 and
> 5.6.6.8 and the latter is used for some internal purposes, how could
> we know that information? I think only thing we can do for now is
> providing an individual control knob for each namespace; IP address
> namespace for routing, transporting packets and web namespace for
> locating resources.
>

I don't mean removing the -orighost flag, just making its default value sane

[mikio, 2014-05-29 05:18:49]

On Thu, May 29, 2014 at 2:16 PM, Andrew Gerrand <adg@golang.org> wrote:

> I don't mean removing the -orighost flag, just making its default value sane

got it. plus removing -nacl flag, right?

[adg, 2014-05-29 05:27:14]

On 29 May 2014 15:18, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> got it. plus removing -nacl flag, right?


Why so?

[mikio, 2014-05-29 05:31:31]

On Thu, May 29, 2014 at 2:26 PM, Andrew Gerrand <adg@golang.org> wrote:

>> got it. plus removing -nacl flag, right?
>
> Why so?

no, i misunderstood, never mind.

[mikio, 2014-05-29 05:58:30]

Hello adg@golang.org, golang-codereviews@googlegroups.com,
dan.kortschak@adelaide.edu.au (cc: golang-codereviews@googlegroups.com),

Please take another look.

[adg, 2014-05-30 01:07:53]

Functionally looks good, a couple of questions.

https://codereview.appspot.com/102770046/diff/160001/cmd/present/local.go
File cmd/present/local.go (right):

https://codereview.appspot.com/102770046/diff/160001/cmd/present/local.go#new...
cmd/present/local.go:61: origin.Host = ln.Addr().String()
I'm not sure why we would want to use ln.Addr().String() if it's unspecified.
What does this mean?

https://codereview.appspot.com/102770046/diff/160001/cmd/present/local.go#new...
cmd/present/local.go:90: !strings.HasPrefix(*httpAddr, "localhost") &&
is this test necessary anymore?

[mikio, 2014-05-30 02:24:48]

https://codereview.appspot.com/102770046/diff/160001/cmd/present/local.go
File cmd/present/local.go (right):

https://codereview.appspot.com/102770046/diff/160001/cmd/present/local.go#new...
cmd/present/local.go:61: origin.Host = ln.Addr().String()
On 2014/05/30 01:07:53, adg wrote:
> I'm not sure why we would want to use ln.Addr().String() if it's unspecified.
> What does this mean?

simply make the url conform to rfc 6454 and 3986, but as i asked above if you
prefer a blank or local hostname that's also fine. which do you favor?

https://codereview.appspot.com/102770046/diff/160001/cmd/present/local.go#new...
cmd/present/local.go:90: !strings.HasPrefix(*httpAddr, "localhost") &&
On 2014/05/30 01:07:53, adg wrote:
> is this test necessary anymore?

nice catch!

[adg, 2014-05-30 02:36:52]

https://codereview.appspot.com/102770046/diff/160001/cmd/present/local.go
File cmd/present/local.go (right):

https://codereview.appspot.com/102770046/diff/160001/cmd/present/local.go#new...
cmd/present/local.go:61: origin.Host = ln.Addr().String()
On 2014/05/30 02:24:48, mikio wrote:
> On 2014/05/30 01:07:53, adg wrote:
> > I'm not sure why we would want to use ln.Addr().String() if it's
unspecified.
> > What does this mean?
> 
> simply make the url conform to rfc 6454 and 3986, but as i asked above if you
> prefer a blank or local hostname that's also fine. which do you favor?

I think we should go with localhost. Better to err on the restrictive side.

[mikio, 2014-05-30 03:04:42]

Hello adg@golang.org, golang-codereviews@googlegroups.com,
dan.kortschak@adelaide.edu.au (cc: golang-codereviews@googlegroups.com),

Please take another look.

[adg, 2014-06-02 06:56:46]

LGTM

[mikio, 2014-06-02 12:39:40]

*** Submitted as
https://code.google.com/p/go/source/detail?r=7b4a1fea7b95&repo=tools ***

go.tools/{cmd/present,playground/socket}: add orighost flag to handle the web
origin more flexible

Also fixes the following nits;
- literal IPv6 address handling
- URL host component handling in the case of a wildcard listen
- URL port component handling in the case of no port component in origin

Fixes issue 8096.

LGTM=dan.kortschak, adg
R=adg, golang-codereviews, dan.kortschak
CC=golang-codereviews
https://codereview.appspot.com/102770046

[kortschak, 2014-06-02 21:02:24]

Thank you for doing this.