Merge lp:~adeuring/juju-core/1227574 into lp:~go-bot/juju-core/trunk

[Abel Deuring]

[r=TheMue][bug=1227574] Remove OpenStack security groups a machine is removed or the environment is destroyed.

[r=TheMue] destroy-env should remove security groups

fix for bug 1227574.

The implementation is obvious, I think. As described in bug 1300755
it can happen that a security group cannot be deleted because it is
used in another environment. The Nova API call fails in this case,
so trying this call does not affect the other environment.

Unfortunately, goose hides the original error message, so Destroy()
ans StopInstance() cannot determine the exact reason of the failure.
Hence only a warning is logged.

https://codereview.appspot.com/84470053/

[Abel Deuring (adeuring)]

Reviewers: mp+214520_code.launchpad.net,

Message:
Please take a look.

Description:
destroy-env should remove security groups

fix for bug 1227574.

The implementation is obvious, I think. As described in bug 1300755
it can happen that a security group cannot be deleted because it is
used in another environment. The Nova API call fails in this case,
so trying this call does not affect the other environment.

Unfortunately, goose hides the original error message, so Destroy()
ans StopInstance() cannot determine the exact reason of the failure.
Hence only a warning is logged.

https://code.launchpad.net/~adeuring/juju-core/1227574/+merge/214520

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/84470053/

Affected files (+168, -2 lines):
   A [revision details]
   M provider/openstack/local_test.go
   M provider/openstack/provider.go

[Frank Mueller (themue)]

Only minor notes, otherwise LGTM.

https://codereview.appspot.com/84470053/diff/1/provider/openstack/local_test.go
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/84470053/diff/1/provider/openstack/local_test.go#newcode452
provider/openstack/local_test.go:452: func (s *localServerSuite)
TestDestroyEnvironmentDeletesSecurityGroupsFWModeGlobabel(c *gc.C) {
Hehe, only naming: ...FWModeGlobal. ;)

https://codereview.appspot.com/84470053/diff/1/provider/openstack/provider.go
File provider/openstack/provider.go (right):

https://codereview.appspot.com/84470053/diff/1/provider/openstack/provider.go#newcode879
provider/openstack/provider.go:879: lastDash :=
strings.LastIndex(openstackName, "-")
Would call it "lastDashPos".

https://codereview.appspot.com/84470053/diff/1/provider/openstack/provider.go#newcode881
provider/openstack/provider.go:881: return fmt.Errorf("Cannot identify
instance ID in openstack server name %q", openstackName)
IMHO errors start lower-case.

https://codereview.appspot.com/84470053/diff/1/provider/openstack/provider.go#newcode1001
provider/openstack/provider.go:1001: logger.Warningf("Cannot delete
security group %q. Used by another environment?", group.Name)
Same here, lower-case.

https://codereview.appspot.com/84470053/

[Abel Deuring (adeuring)]

review: Approve

[Go Bot (go-bot)]

The attempt to merge lp:~adeuring/juju-core/1227574 into lp:juju-core failed. Below is the output from the failed tests.

# launchpad.net/juju-core/provider/openstack
provider/openstack/provider.go:862: undefined: strings
mongod: no process found

[Abel Deuring (adeuring)]

review: Approve

=== modified file 'provider/openstack/local_test.go'
--- provider/openstack/local_test.go	2014-04-04 15:55:19 +0000
+++ provider/openstack/local_test.go	2014-04-08 15:52:29 +0000
@@ -321,6 +321,111 @@
 		"404; error info: .*itemNotFound.*")
 }
 
+func assertSecurityGroups(c *gc.C, env environs.Environ, expected []string) {
+	novaClient := openstack.GetNovaClient(env)
+	groups, err := novaClient.ListSecurityGroups()
+	c.Assert(err, gc.IsNil)
+	for _, name := range expected {
+		found := false
+		for _, group := range groups {
+			if group.Name == name {
+				found = true
+				break
+			}
+		}
+		if !found {
+			c.Errorf("expected security group %g not found", name)
+		}
+	}
+	for _, group := range groups {
+		found := false
+		for _, name := range expected {
+			if group.Name == name {
+				found = true
+				break
+			}
+		}
+		if !found {
+			c.Errorf("existing security group %g is not expected", group.Name)
+		}
+	}
+}
+
+func (s *localServerSuite) TestStopInstance(c *gc.C) {
+	cfg, err := config.New(config.NoDefaults, s.TestConfig.Merge(coretesting.Attrs{
+		"firewall-mode": "instance"}))
+	c.Assert(err, gc.IsNil)
+	env, err := environs.New(cfg)
+	c.Assert(err, gc.IsNil)
+	instanceName := "100"
+	inst, _ := testing.AssertStartInstance(c, env, instanceName)
+	// Openstack now has three security groups for the server, the default
+	// group, one group for the entire environment, and another for the
+	// new instance.
+	assertSecurityGroups(c, env, []string{"default", fmt.Sprintf("juju-%v", env.Name()), fmt.Sprintf("juju-%v-%v", env.Name(), instanceName)})
+	err = env.StopInstances([]instance.Instance{inst})
+	c.Assert(err, gc.IsNil)
+	// The security group for this instance is now removed.
+	assertSecurityGroups(c, env, []string{"default", fmt.Sprintf("juju-%v", env.Name())})
+}
+
+// Due to bug #1300755 it can happen that the security group intended for
+// an instance is also used as the common security group of another
+// environment. If this is the case, the attempt to delete the instance's
+// security group fails but StopInstance succeeds.
+func (s *localServerSuite) TestStopInstanceSecurityGroupNotDeleted(c *gc.C) {
+	// Force an error when a security group is deleted.
+	cleanup := s.srv.Service.Nova.RegisterControlPoint(
+		"removeSecurityGroup",
+		func(sc hook.ServiceControl, args ...interface{}) error {
+			return fmt.Errorf("failed on purpose")
+		},
+	)
+	defer cleanup()
+	cfg, err := config.New(config.NoDefaults, s.TestConfig.Merge(coretesting.Attrs{
+		"firewall-mode": "instance"}))
+	c.Assert(err, gc.IsNil)
+	env, err := environs.New(cfg)
+	c.Assert(err, gc.IsNil)
+	instanceName := "100"
+	inst, _ := testing.AssertStartInstance(c, env, instanceName)
+	allSecurityGroups := []string{"default", fmt.Sprintf("juju-%v", env.Name()), fmt.Sprintf("juju-%v-%v", env.Name(), instanceName)}
+	assertSecurityGroups(c, env, allSecurityGroups)
+	err = env.StopInstances([]instance.Instance{inst})
+	c.Assert(err, gc.IsNil)
+	assertSecurityGroups(c, env, allSecurityGroups)
+}
+
+func (s *localServerSuite) TestDestroyEnvironmentDeletesSecurityGroupsFWModeInstance(c *gc.C) {
+	cfg, err := config.New(config.NoDefaults, s.TestConfig.Merge(coretesting.Attrs{
+		"firewall-mode": "instance"}))
+	c.Assert(err, gc.IsNil)
+	env, err := environs.New(cfg)
+	c.Assert(err, gc.IsNil)
+	instanceName := "100"
+	testing.AssertStartInstance(c, env, instanceName)
+	allSecurityGroups := []string{"default", fmt.Sprintf("juju-%v", env.Name()), fmt.Sprintf("juju-%v-%v", env.Name(), instanceName)}
+	assertSecurityGroups(c, env, allSecurityGroups)
+	err = env.Destroy()
+	c.Check(err, gc.IsNil)
+	assertSecurityGroups(c, env, []string{"default"})
+}
+
+func (s *localServerSuite) TestDestroyEnvironmentDeletesSecurityGroupsFWModeGlobal(c *gc.C) {
+	cfg, err := config.New(config.NoDefaults, s.TestConfig.Merge(coretesting.Attrs{
+		"firewall-mode": "global"}))
+	c.Assert(err, gc.IsNil)
+	env, err := environs.New(cfg)
+	c.Assert(err, gc.IsNil)
+	instanceName := "100"
+	testing.AssertStartInstance(c, env, instanceName)
+	allSecurityGroups := []string{"default", fmt.Sprintf("juju-%v", env.Name()), fmt.Sprintf("juju-%v-global", env.Name())}
+	assertSecurityGroups(c, env, allSecurityGroups)
+	err = env.Destroy()
+	c.Check(err, gc.IsNil)
+	assertSecurityGroups(c, env, []string{"default"})
+}
+
 var instanceGathering = []struct {
 	ids []instance.Id
 	err error

=== modified file 'provider/openstack/provider.go'
--- provider/openstack/provider.go	2014-04-07 07:06:59 +0000
+++ provider/openstack/provider.go	2014-04-08 15:52:29 +0000
@@ -11,6 +11,7 @@
 	"io/ioutil"
 	"net/http"
 	"regexp"
+	"strings"
 	"sync"
 	"time"
 
@@ -851,15 +852,29 @@
 
 func (e *environ) StopInstances(insts []instance.Instance) error {
 	ids := make([]instance.Id, len(insts))
+	securityGroupNames := make([]string, len(insts))
 	for i, inst := range insts {
 		instanceValue, ok := inst.(*openstackInstance)
 		if !ok {
 			return errors.New("Incompatible instance.Instance supplied")
 		}
 		ids[i] = instanceValue.Id()
+		openstackName := instanceValue.getServerDetail().Name
+		lastDashPos := strings.LastIndex(openstackName, "-")
+		if lastDashPos == -1 {
+			return fmt.Errorf("cannot identify instance ID in openstack server name %q", openstackName)
+		}
+		securityGroupNames[i] = e.machineGroupName(openstackName[lastDashPos+1:])
 	}
 	logger.Debugf("terminating instances %v", ids)
-	return e.terminateInstances(ids)
+	err := e.terminateInstances(ids)
+	if err != nil {
+		return err
+	}
+	if e.Config().FirewallMode() == config.FwInstance {
+		return e.deleteSecurityGroups(securityGroupNames)
+	}
+	return nil
 }
 
 // collectInstances tries to get information on each instance id in ids.
@@ -949,7 +964,29 @@
 }
 
 func (e *environ) Destroy() error {
-	return common.Destroy(e)
+	err := common.Destroy(e)
+	if err != nil {
+		return err
+	}
+	novaClient := e.nova()
+	securityGroups, err := novaClient.ListSecurityGroups()
+	if err != nil {
+		return err
+	}
+	re, err := regexp.Compile(fmt.Sprintf("^%s(-\\d+)?$", e.jujuGroupName()))
+	if err != nil {
+		return err
+	}
+	globalGroupName := e.globalGroupName()
+	for _, group := range securityGroups {
+		if re.MatchString(group.Name) || group.Name == globalGroupName {
+			err = novaClient.DeleteSecurityGroup(group.Id)
+			if err != nil {
+				logger.Warningf("cannot delete security group %q. Used by another environment?", group.Name)
+			}
+		}
+	}
+	return nil
 }
 
 func (e *environ) globalGroupName() string {
@@ -1207,6 +1244,29 @@
 	return *group, nil
 }
 
+// deleteSecurityGroups deletes the given security groups. If a security
+// group is also used by another environment (see bug #1300755), an attempt
+// to delete this group fails. A warning is logged in this case.
+func (e *environ) deleteSecurityGroups(securityGroupNames []string) error {
+	novaclient := e.nova()
+	allSecurityGroups, err := novaclient.ListSecurityGroups()
+	if err != nil {
+		return err
+	}
+	for _, securityGroup := range allSecurityGroups {
+		for _, name := range securityGroupNames {
+			if securityGroup.Name == name {
+				err := novaclient.DeleteSecurityGroup(securityGroup.Id)
+				if err != nil {
+					logger.Warningf("cannot delete security group %q. Used by another environment?", name)
+				}
+				break
+			}
+		}
+	}
+	return nil
+}
+
 func (e *environ) terminateInstances(ids []instance.Id) error {
 	if len(ids) == 0 {
 		return nil