ec2: avoid deleting security groups if we can.

It takes ages to delete a security group if an
instance has been using it, so this change
adjusts the old group to have the required
permissions instead.

https://code.launchpad.net/~rogpeppe/juju/go-ec2-check-group-perms/+merge/93628

(do not edit description out of merge proposal)

[rog, 2012-02-17 18:32:29]

Please take a look.

[rog, 2012-02-21 15:56:18]

Please take a look.

[rog, 2012-02-21 16:11:37]

Please take a look.

[rog, 2012-02-21 16:12:01]

Please take a look.

[rog, 2012-02-21 16:14:22]

Please take a look.

[rog, 2012-02-21 16:18:19]

Please take a look.

[fwereade, 2012-02-21 16:55:08]

LGTM.

https://codereview.appspot.com/5671086/diff/10001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/5671086/diff/10001/environs/ec2/ec2.go#newcode310
environs/ec2/ec2.go:310: var zg ec2.SecurityGroup
At first glance, this looks to me like a verbose way of saying "nil". Presumably
it has some deeper purpose I haven't seen yet, but it could use a comment.

https://codereview.appspot.com/5671086/diff/10001/environs/ec2/ec2.go#newcode314
environs/ec2/ec2.go:314: // will be changed accordingly. If the group does not
exist, it will be created
Nice :D.

https://codereview.appspot.com/5671086/diff/10001/environs/ec2/ec2.go#newcode331
environs/ec2/ec2.go:331: // TODO do we mind if the old group has the wrong
description?
I think we probably do -- it's there, after all, and I would prefer that it not
be wrong -- but we could get around this by setting every description to
"managed by juju" or something (because while it is there, and should not be
*wrong*, I don't think there's any requirement that it be *useful*).

[rog, 2012-02-22 12:01:31]

Please take a look.

[rog, 2012-02-22 13:21:36]

Please take a look.

[rog, 2012-02-22 13:22:35]

Please take a look.

[rog, 2012-02-22 16:06:59]

Please take a look.

[niemeyer, 2012-02-22 20:12:08]

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode306
environs/ec2/ec2.go:306: descr := fmt.Sprintf("juju group for %s machine %d",
e.name, machineId)
"... machine %d of environment %q", otherwise we may get things like "... for
testing machine N.".

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode315
environs/ec2/ec2.go:315: var zg ec2.SecurityGroup
s/zg/zeroGroup/

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode320
environs/ec2/ec2.go:320: // with the given description. It returns the group.
// ensureGroup returns the security group with name and perms.
// If a group with name does not exist, one will be created.
// If it exists, its permissions are set to perms.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode321
environs/ec2/ec2.go:321: func (e *environ) ensureGroup(name, descr string, perms
[]ec2.IPPerm) (g ec2.SecurityGroup, err error) {
Let's please drop the descr parameter, and set it internally to something like
William suggested ("managed by juju"?). Simplifies the interface, the
documentation, and avoids having wrong information in there.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode336
environs/ec2/ec2.go:336: // TODO do we mind if the old group has the wrong
description?
The comment may be dropped with the changes above.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode347
environs/ec2/ec2.go:347: _, err := e.ec2.RevokeSecurityGroup(g,
revoke.ipPerms())
Nice. Very readable logic.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode360
environs/ec2/ec2.go:360: _, err := e.ec2.AuthorizeSecurityGroup(g,
add.ipPerms())
Feels like these methods should really be taking a pointer to the group. Perhaps
something to fix in the future.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode371
environs/ec2/ec2.go:371: type ipPerm struct {
s/ipPerm/permKey/, so we can refer to IPPerm and ipPerm unambiguously.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode387
environs/ec2/ec2.go:387: ipp := ipPerm{
s/ipp/pk/

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode405
environs/ec2/ec2.go:405: // ipPerms returns the given set of permissions
// ipPerms returns m as a slice of permissions usable
// with the ec2 package.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:77: // Add one permission the same as will be needed
This is reading poorly.

[rog, 2012-02-23 09:41:36]

Please take a look.

[rog, 2012-02-23 09:45:45]

PTAL

https://codereview.appspot.com/5671086/diff/10001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/5671086/diff/10001/environs/ec2/ec2.go#newcode310
environs/ec2/ec2.go:310: var zg ec2.SecurityGroup
On 2012/02/21 16:55:08, fwereade wrote:
> At first glance, this looks to me like a verbose way of saying "nil".
Presumably
> it has some deeper purpose I haven't seen yet, but it could use a comment.

will add comment.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode306
environs/ec2/ec2.go:306: descr := fmt.Sprintf("juju group for %s machine %d",
e.name, machineId)
On 2012/02/22 20:12:08, niemeyer wrote:
> "... machine %d of environment %q", otherwise we may get things like "... for
> testing machine N.".

deleted (following comment below)

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode315
environs/ec2/ec2.go:315: var zg ec2.SecurityGroup
On 2012/02/22 20:12:08, niemeyer wrote:
> s/zg/zeroGroup/

done. but let me register a note of reluctance. it's a stand-in for nil so it's
nice if it's short. also, there is precedent:
http://weekly.golang.org/pkg/image/#Point

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode320
environs/ec2/ec2.go:320: // with the given description. It returns the group.
On 2012/02/22 20:12:08, niemeyer wrote:
> // ensureGroup returns the security group with name and perms.
> // If a group with name does not exist, one will be created.
> // If it exists, its permissions are set to perms.

done. that's better, thanks.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode321
environs/ec2/ec2.go:321: func (e *environ) ensureGroup(name, descr string, perms
[]ec2.IPPerm) (g ec2.SecurityGroup, err error) {
On 2012/02/22 20:12:08, niemeyer wrote:
> Let's please drop the descr parameter, and set it internally to something like
> William suggested ("managed by juju"?). Simplifies the interface, the
> documentation, and avoids having wrong information in there.

i don't think it avoids having wrong information there. the group can still end
up with the wrong description if something other than juju creates it.

if juju has created it, the description for a given group name will always be
the same, so it won't be wrong.

still, i buy the simplified interface argument, so done.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode336
environs/ec2/ec2.go:336: // TODO do we mind if the old group has the wrong
description?
On 2012/02/22 20:12:08, niemeyer wrote:
> The comment may be dropped with the changes above.

it is still relevant (see above). i'll change the comment.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode360
environs/ec2/ec2.go:360: _, err := e.ec2.AuthorizeSecurityGroup(g,
add.ipPerms())
On 2012/02/22 20:12:08, niemeyer wrote:
> Feels like these methods should really be taking a pointer to the group.
Perhaps
> something to fix in the future.

i'm not sure i agree. i think ec2.SecurityGroup works well as a by-value object.
i've rarely come across a case where i want to change the fields of a group in
place.

it'd be nice if the name was shorter though.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode371
environs/ec2/ec2.go:371: type ipPerm struct {
On 2012/02/22 20:12:08, niemeyer wrote:
> s/ipPerm/permKey/, so we can refer to IPPerm and ipPerm unambiguously.

good suggestion. done.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode387
environs/ec2/ec2.go:387: ipp := ipPerm{
On 2012/02/22 20:12:08, niemeyer wrote:
> s/ipp/pk/

done. (well, i prefer k)

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/ec2.go#newcode405
environs/ec2/ec2.go:405: // ipPerms returns the given set of permissions
On 2012/02/22 20:12:08, niemeyer wrote:
> // ipPerms returns m as a slice of permissions usable
> // with the ec2 package.

Done.

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/5671086/diff/7008/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:77: // Add one permission the same as will be needed
On 2012/02/22 20:12:08, niemeyer wrote:
> This is reading poorly.

Done.

[fwereade, 2012-02-23 13:21:12]

LGTM

[niemeyer, 2012-02-24 15:03:21]

LGTM, thanks.

[rog, 2012-02-24 15:12:05]

*** Submitted:

ec2: avoid deleting security groups if we can.

It takes ages to delete a security group if an
instance has been using it, so this change
adjusts the old group to have the required
permissions instead.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/5671086