provider/openstack: bug #1226996 SecurityGroup

provider/openstack: bug #1226996 SecurityGroup

We intended to allow access to any port for any instance in the
default security group. However, we didn't specify a CIDR and we
didn't reference the Source Group Id. Which meant we actually were
exposing *all* ports to *all* machines.

I do have some tests that the security group we get back from
EnsureGroup has the right bits set. I was hoping for a slightly better
cross-provider test that actually runs a service on a random port and
ensures that we are unable to actually connect to that port.

However, I think this is a nice small fix for 1.14 which is worthy of
landing.

https://code.launchpad.net/~jameinel/juju-core/security-group-group-id-1226996/+merge/186321

(do not edit description out of merge proposal)

[jameinel, 2013-09-18 13:44:05]

Please take a look.

[jameinel, 2013-09-19 10:53:03]

Please take a look.

[jameinel, 2013-09-19 10:54:57]

On 2013/09/19 10:53:03, jameinel wrote:
> Please take a look.

This should now properly bootstrap on Openstack. I'm still testing and
https://bugs.launchpad.net/bugs/1227533 got a bit in the way of manual testing,
but these security groups should be sane.

[rog, 2013-09-19 13:00:20]

LGTM with some minor suggestions below.

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/export_t...
File provider/openstack/export_test.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/export_t...
provider/openstack/export_test.go:241: func DiscardSecurityGroup(e
environs.Environ, name string) error {
This is more involved logic than I'd usually expect to see in export_test.go.
Perhaps define as discardSecurityGroup inside the implementation
and "var DiscardSecurityGroup = discardSecurityGroup" here?

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/live_tes...
File provider/openstack/live_test.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/live_tes...
provider/openstack/live_test.go:188: defer cleanup()
again?

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/provider.go
File provider/openstack/provider.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/provider...
provider/openstack/provider.go:964: return e.ensureGroup(groupName,
why not use e.jujuGroupName as before?

[fwereade, 2013-09-19 13:09:11]

LGTM

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/export_t...
File provider/openstack/export_test.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/export_t...
provider/openstack/export_test.go:241: func DiscardSecurityGroup(e
environs.Environ, name string) error {
On 2013/09/19 13:00:21, rog wrote:
> This is more involved logic than I'd usually expect to see in export_test.go.
> Perhaps define as discardSecurityGroup inside the implementation
> and "var DiscardSecurityGroup = discardSecurityGroup" here?

I think it's reasonable, though -- if it's useful for testing, but not "real"
functionality it's perfectly proper to put it here rather than clutter up the
package when built not as a test.

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/live_tes...
File provider/openstack/live_test.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/live_tes...
provider/openstack/live_test.go:194: // client-via-API we can disable the State
port as well.)
aside: this is exactly what the firewaller does, and is another reason to keep
self-hosted juju in mind.

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/provider.go
File provider/openstack/provider.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/provider...
provider/openstack/provider.go:1006: // other instances that might be running on
the same OpenStack account.
This does assume no two people will share both an account and an env name; this
has bitten people in practice, IIRC. Maybe worth pointing that out explicitly
here?

[rog, 2013-09-19 13:16:40]

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/export_t...
File provider/openstack/export_test.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/export_t...
provider/openstack/export_test.go:241: func DiscardSecurityGroup(e
environs.Environ, name string) error {
On 2013/09/19 13:09:11, fwereade wrote:
> On 2013/09/19 13:00:21, rog wrote:
> > This is more involved logic than I'd usually expect to see in
export_test.go.
> > Perhaps define as discardSecurityGroup inside the implementation
> > and "var DiscardSecurityGroup = discardSecurityGroup" here?
> 
> I think it's reasonable, though -- if it's useful for testing, but not "real"
> functionality it's perfectly proper to put it here rather than clutter up the
> package when built not as a test.

As an alternative, how about exporting only that functionality that needs to be
exported.

func NovaClient(e environs.Environ) *nova.Client {
    return e.(*environ).nova()
}

Then we can move DiscardSecurityGroup near the place it's used.

[jameinel, 2013-09-19 13:22:07]

Please take a look.

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/export_t...
File provider/openstack/export_test.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/export_t...
provider/openstack/export_test.go:241: func DiscardSecurityGroup(e
environs.Environ, name string) error {
On 2013/09/19 13:00:21, rog wrote:
> This is more involved logic than I'd usually expect to see in export_test.go.
> Perhaps define as discardSecurityGroup inside the implementation
> and "var DiscardSecurityGroup = discardSecurityGroup" here?

Well, it is deleting security groups which is something we *don't* ever do in
live code. So I'm hesitant to put something in there just to expose it for
tests.

If we fix bugs like: https://bugs.launchpad.net/bugs/1227574 then I think it is
reasonable to share the implementation.

So I could move it, but I'd rather wait until we are actually going to be
calling it from there.

both provider/ec2/export_test.go and provider/openstack/export_test.go have a
fair amount of test-specific logic in them, so it doesn't seem unprecedented.
state/export_test.go also has AddCustomCharm with lots of logic. It even has a
private helper function for two of the other exported functions. :)

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/live_tes...
File provider/openstack/live_test.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/live_tes...
provider/openstack/live_test.go:188: defer cleanup()
On 2013/09/19 13:00:21, rog wrote:
> again?

no, the third time should be removed :)

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/provider.go
File provider/openstack/provider.go (right):

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/provider...
provider/openstack/provider.go:964: return e.ensureGroup(groupName,
On 2013/09/19 13:00:21, rog wrote:
> why not use e.jujuGroupName as before?

for testability. jujuGroupName isn't exposed, but I can expose just this
function and then pass whatever name I want from the test suite.

It means I can test setting security groups in a "live" fashion without having
to actually start a machine.

https://codereview.appspot.com/13562045/diff/4001/provider/openstack/provider...
provider/openstack/provider.go:1006: // other instances that might be running on
the same OpenStack account.
On 2013/09/19 13:09:11, fwereade wrote:
> This does assume no two people will share both an account and an env name;
this
> has bitten people in practice, IIRC. Maybe worth pointing that out explicitly
> here?

This is just looking like a move because of how the diff was generated, but I
can try to update this per your request.