environs/ec2: implement Ports, OpenPorts and ClosePorts.

environs/ec2: implement Ports, OpenPorts and ClosePorts.



https://code.launchpad.net/~rogpeppe/juju-core/ec2-ports/+merge/114897

(do not edit description out of merge proposal)

[rog, 2012-07-13 16:05:01]

Please take a look.

[rog, 2012-07-13 16:07:28]

Please take a look.

[dave_cheney.net, 2012-07-14 08:32:11]

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go#newcode516
environs/ec2/ec2.go:516: }
can this be moved to a function, it is repeated with ClosePorts below. ie,
portToSecurityGroup ?

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go#newcode574
environs/ec2/ec2.go:574: if len(p.SourceIPs) != 1 || p.SourceIPs[0] !=
"0.0.0.0/0" {
ipv6 ?

[fwereade, 2012-07-16 14:32:12]

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go#newcode559
environs/ec2/ec2.go:559: return fmt.Errorf("cannot open ports: %v", err)
"cannot close ports"

Also, do we get errors when we can't revoke, eg because those ports are not open
in he first place? If not, a comment explaining this would be helpful; if we do,
I think we need to be a little more sophisticated.

[rog, 2012-07-16 15:03:01]

Please take a look.

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go#newcode516
environs/ec2/ec2.go:516: }
On 2012/07/14 08:32:11, dfc wrote:
> can this be moved to a function, it is repeated with ClosePorts below. ie,
> portToSecurityGroup ?

Done.

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go#newcode559
environs/ec2/ec2.go:559: return fmt.Errorf("cannot open ports: %v", err)
On 2012/07/16 14:32:13, fwereade wrote:
> "cannot close ports"

oops, done.

> Also, do we get errors when we can't revoke, eg because those ports are not
open
> in he first place? If not, a comment explaining this would be helpful; if we
do,
> I think we need to be a little more sophisticated.

no we don't. ec2 works fine in that case and the tests verify that. i'll add a
comment though.

https://codereview.appspot.com/6344113/diff/7/environs/ec2/ec2.go#newcode574
environs/ec2/ec2.go:574: if len(p.SourceIPs) != 1 || p.SourceIPs[0] !=
"0.0.0.0/0" {
On 2012/07/14 08:32:11, dfc wrote:
> ipv6 ?

i *presume* that because i've set the security group with an ipv4 address, i'll
get one back, but maybe not.
in fact, i'll just take out the check - no-one else should be changing these
security groups and they deserve what they get if they do...

[fwereade, 2012-07-16 22:18:31]

LGTM

[niemeyer, 2012-07-17 14:07:35]

LGTM

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

https://codereview.appspot.com/6344113/diff/7002/environs/jujutest/livetests....
environs/jujutest/livetests.go:131: c.Assert(sortedPorts(ports), DeepEquals,
[]state.Port{{"tcp", 45}, {"tcp", 89}, {"tcp", 99}})
Should we return ports sorted already?

[rog, 2012-07-17 15:40:05]

Please take a look.

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

https://codereview.appspot.com/6344113/diff/7002/environs/jujutest/livetests....
environs/jujutest/livetests.go:131: c.Assert(sortedPorts(ports), DeepEquals,
[]state.Port{{"tcp", 45}, {"tcp", 89}, {"tcp", 99}})
On 2012/07/17 14:07:35, niemeyer wrote:
> Should we return ports sorted already?

Done.

[rog, 2012-07-17 15:43:03]

*** Submitted:

environs/ec2: implement Ports, OpenPorts and ClosePorts.

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