environs/ec2: authorize internal traffic

environs/ec2: authorize internal traffic

Also add a live test that relies on internal traffic
to pass (it deploys a charm live and waits for the
new unit's machine agent to arrive).

Oh yes, turn on the debug flag in live machines too.
It's too much hassle to remember to add it every time,
and at this stage in development, realistically we always
want it on IMHO.

https://code.launchpad.net/~rogpeppe/juju-core/051-authorize-internal-traffic/+merge/123105

Requires: https://code.launchpad.net/~rogpeppe/juju-core/052-testing-charm-suite/+merge/122864

(do not edit description out of merge proposal)

[rog, 2012-09-06 15:20:20]

Please take a look.

[dave_cheney.net, 2012-09-11 04:53:50]

LGTM.

https://codereview.appspot.com/6500089/diff/1030/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:122: // TODO: disable debug mode by default when
the system is stable.
i'd just leave debug on.

https://codereview.appspot.com/6500089/diff/1030/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/ec2/ec2.go#newcode664
environs/ec2/ec2.go:664: })
suggestion: add a comment to point out these are only external traffic
permissions and the internal perms are being handled in function foo.

https://codereview.appspot.com/6500089/diff/1030/environs/ec2/ec2.go#newcode668
environs/ec2/ec2.go:668: jujuMachineGroup, err :=
e.ensureGroup(e.machineGroupName(machineId), false, nil)
This func signature is getting a bit big, could I interest you in
ensurePublicGroup and ensurePrivateGroup as wrappers ?

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:177: w0, tools0 := t.machineAgentTools(c, m0)
nice, i think this should be promoted out of the test.

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:186: svc, err := conn.AddService("", sch)
Adding a service with an empty name should not succeed.  I know it does, but it
shouldn't

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:194: mid, err := unit.AssignedMachineId()
I don't think this will work. There is no guarantee that the machine id will be
set.

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:216: assertInstanceId(c, m1, nil)
Should there also be a test for trying to remove the machine before all units
have been removed ?

[rog, 2012-09-11 15:33:19]

Please take a look.

https://codereview.appspot.com/6500089/diff/1030/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:122: // TODO: disable debug mode by default when
the system is stable.
On 2012/09/11 04:53:50, dfc wrote:
> i'd just leave debug on.

if debug is always on, why bother with a debug mode?

https://codereview.appspot.com/6500089/diff/1030/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/ec2/ec2.go#newcode668
environs/ec2/ec2.go:668: jujuMachineGroup, err :=
e.ensureGroup(e.machineGroupName(machineId), false, nil)
On 2012/09/11 04:53:50, dfc wrote:
> This func signature is getting a bit big, could I interest you in
> ensurePublicGroup and ensurePrivateGroup as wrappers ?

after a little ec2 experimentation, i think we can do better than this.

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:177: w0, tools0 := t.machineAgentTools(c, m0)
On 2012/09/11 04:53:50, dfc wrote:
> nice, i think this should be promoted out of the test. 

where else might we use it?

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:186: svc, err := conn.AddService("", sch)
On 2012/09/11 04:53:50, dfc wrote:
> Adding a service with an empty name should not succeed.  I know it does, but
it
> shouldn't

the empty name specifies the default service name for the charm. perhaps that's
an unwise thing to do and we should make the service name mandatory, but it was
a deliberate choice and i don't think it's a bug per se.

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:194: mid, err := unit.AssignedMachineId()
On 2012/09/11 04:53:50, dfc wrote:
> I don't think this will work. There is no guarantee that the machine id will
be
> set.

conn.AddUnits is documented to add the units and assign them to a new machine,
so i think this should be fine.

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:216: assertInstanceId(c, m1, nil)
On 2012/09/11 04:53:50, dfc wrote:
> Should there also be a test for trying to remove the machine before all units
> have been removed ?

i'm not sure. that functionality should be well tested in state, and i'm not
sure that doing it live tests anything more in particular. but perhaps you're
thinking of something different to me?

[niemeyer, 2012-09-11 21:10:53]

LGTM. Very happy to see that test working, thanks!

https://codereview.appspot.com/6500089/diff/1030/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:122: // TODO: disable debug mode by default when
the system is stable.
On 2012/09/11 15:33:19, rog wrote:
> On 2012/09/11 04:53:50, dfc wrote:
> > i'd just leave debug on.
> 
> if debug is always on, why bother with a debug mode?

Agreed.

I think eventually we'll want juju set-env debug=on or something similar.

https://codereview.appspot.com/6500089/diff/1030/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/ec2/ec2.go#newcode680
environs/ec2/ec2.go:680: // If it exists, its permissions are set to perms.
// The authSelf parameter ...

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6500089/diff/1030/environs/jujutest/livetests....
environs/jujutest/livetests.go:186: svc, err := conn.AddService("", sch)
On 2012/09/11 15:33:19, rog wrote:
> On 2012/09/11 04:53:50, dfc wrote:
> > Adding a service with an empty name should not succeed.  I know it does, but
> it
> > shouldn't
> 
> the empty name specifies the default service name for the charm. perhaps
that's
> an unwise thing to do and we should make the service name mandatory, but it
was
> a deliberate choice and i don't think it's a bug per se.

It also reflects the command line behavior, FWIW. That's likely where it comes
from.

[rog, 2012-09-14 07:06:59]

This proposal has been submitted as
https://code.launchpad.net/~rogpeppe/juju-core/059-authorize-internal-traffic...
due to a prereq change.