Merge pull request #252 from conkiztador/secgroups

Fix creating servers with multiple security groups

Author: EarthmanT

nova_plugin/server.py

@@ -851,17 +851,21 @@ def connect_security_group(nova_client, **kwargs):
             'Expected external resources server {0} and security-group {1} to '
             'be connected'.format(server_id, security_group_id))
 
+    def group_matches(security_group):
+        return (
+            security_group_id == security_group.id or
+            security_group_name == security_group.name
+        )
+    # Since some security groups are already attached in
+    # create this will ensure that they are not attached twice.
     server = nova_client.servers.get(server_id)
-    for security_group in server.list_security_group():
-        # Since some security groups are already attached in
-        # create this will ensure that they are not attached twice.
-        if security_group_id != security_group.id and \
-                security_group_name != security_group.name:
-            # to support nova security groups as well,
-            # we connect the security group by name
-            # (as connecting by id
-            # doesn't seem to work well for nova SGs)
-            server.add_security_group(security_group_name)
+    present = any(map(group_matches, server.list_security_group()))
+    # to support nova security groups as well,
+    # we connect the security group by name
+    # (as connecting by id
+    # doesn't seem to work well for nova SGs)
+    if not present:
+        server.add_security_group(security_group_name)
 
     _validate_security_group_and_server_connection_status(nova_client,
                                                           server_id,

nova_plugin/tests/test_server.py

@@ -13,6 +13,7 @@
 #  * See the License for the specific language governing permissions and
 #  * limitations under the License.
 
+import collections
 from os import path
 import tempfile
 
@@ -1009,6 +1010,9 @@ def test_handle_boot_image_no_image(self, *_):
 
 @mock.patch('openstack_plugin_common.OpenStackClient._validate_auth_params')
 class TestServerSGAttachments(unittest.TestCase):
+    SecurityGroup = collections.namedtuple(
+        'SecurityGroup', ['id', 'name'], verbose=True)
+
     def setUp(self):
         ctx = MockCloudifyContext(
             target=MockContext({
@@ -1036,14 +1040,32 @@ def setUp(self):
         findctx.start()
         self.addCleanup(findctx.stop)
 
-    def test_detach_already_detached(self, *kwargs):
-        fake_client = mock.MagicMock()
-        fake_client().servers.get = mock.MagicMock()
-        fake_client().servers.get().remove_security_group = mock.MagicMock(
-            side_effect=nova_exceptions.NotFound('test'))
-        with mock.patch('openstack_plugin_common.NovaClientWithSugar',
-                        fake_client):
-            nova_plugin.server.disconnect_security_group()
+    @mock.patch('openstack_plugin_common.NovaClientWithSugar')
+    def test_detach_already_detached(self, client, *kwargs):
+        server = client.return_value.servers.get.return_value
+        server.remove_security_group.side_effect = \
+            nova_exceptions.NotFound('test')
+        nova_plugin.server.disconnect_security_group()
+
+    @mock.patch('openstack_plugin_common.NovaClientWithSugar')
+    def test_connect_not_connected(self, client, *kwargs):
+        security_groups = [self.SecurityGroup('test-sg-2', 'test-sg-2-name')]
+        server = client.return_value.servers.get.return_value
+        server.list_security_group.return_value = security_groups
+        server.add_security_group.side_effect = (
+            lambda _: security_groups.append(
+                self.SecurityGroup('test-sg', 'test-sg-name')))
+        nova_plugin.server.connect_security_group()
+        server.add_security_group.assert_called_once_with('test-sg-name')
+
+    @mock.patch('openstack_plugin_common.NovaClientWithSugar')
+    def test_connect_already_connected(self, client, *kwargs):
+        security_groups = [self.SecurityGroup('test-sg', 'test-sg-name'),
+                           self.SecurityGroup('test-sg-2', 'test-sg-2-name')]
+        server = client.return_value.servers.get.return_value
+        server.list_security_group.return_value = security_groups
+        nova_plugin.server.connect_security_group()
+        server.add_security_group.assert_not_called()
 
 
 class TestServerRelationships(unittest.TestCase):