Make network_id parameter mandatory when creating a security group

cloudbridge/cloud/interfaces/services.py

@@ -935,7 +935,7 @@ class SecurityGroupService(PageableObjectMixin, CloudService):
         pass
 
     @abstractmethod
-    def create(self, name, description, network_id=None):
+    def create(self, name, description, network_id):
         """
         Create a new SecurityGroup.
 
@@ -946,9 +946,7 @@ class SecurityGroupService(PageableObjectMixin, CloudService):
         :param description: The description of the new security group.
 
         :type  network_id: ``str``
-        :param network_id: An optional network ID under which to create the
-                           security group that may be supported by some
-                           providers.
+        :param network_id: Network ID under which to create the security group.
 
         :rtype: ``object`` of :class:`.SecurityGroup`
         :return:  A SecurityGroup instance or ``None`` if one was not created.

cloudbridge/cloud/providers/aws/resources.py

@@ -675,8 +675,8 @@ class AWSSecurityGroup(BaseSecurityGroup):
                rule.from_port == from_port and
                rule.to_port == to_port and
                rule.grants[0].cidr_ip == cidr_ip) or \
-               (rule.grants[0].name == src_group.name if src_group and
-               hasattr(rule.grants[0], 'name') else False):
+               (rule.grants[0].group_id == src_group.id if src_group and
+               hasattr(rule.grants[0], 'group_id') else False):
                 return AWSSecurityGroupRule(self._provider, rule, self)
         return None
 
@@ -685,6 +685,8 @@ class AWSSecurityGroup(BaseSecurityGroup):
         js = {k: v for(k, v) in attr if not k.startswith('_')}
         json_rules = [r.to_json() for r in self.rules]
         js['rules'] = [json.loads(r) for r in json_rules]
+        if js.get('network_id'):
+            js.pop('network_id')  # Omit for consistency across cloud providers
         return json.dumps(js, sort_keys=True)
 
 
@@ -729,9 +731,9 @@ class AWSSecurityGroupRule(BaseSecurityGroupRule):
     @property
     def group(self):
         if len(self._rule.grants) > 0:
-            if self._rule.grants[0].name:
+            if self._rule.grants[0].group_id:
                 cg = self._provider.ec2_conn.get_all_security_groups(
-                    groupnames=[self._rule.grants[0].name])[0]
+                    group_ids=[self._rule.grants[0].group_id])[0]
                 return AWSSecurityGroup(self._provider, cg)
         return None
 
@@ -746,6 +748,9 @@ class AWSSecurityGroupRule(BaseSecurityGroupRule):
         if self.group:
             # pylint:disable=protected-access
             self.parent._security_group.revoke(
+                ip_protocol=self.ip_protocol,
+                from_port=self.from_port,
+                to_port=self.to_port,
                 src_group=self.group._security_group)
         else:
             # pylint:disable=protected-access

cloudbridge/cloud/providers/aws/services.py

@@ -171,7 +171,7 @@ class AWSSecurityGroupService(BaseSecurityGroupService):
         return ClientPagedResultList(self.provider, sgs,
                                      limit=limit, marker=marker)
 
-    def create(self, name, description, network_id=None):
+    def create(self, name, description, network_id):
         """
         Create a new SecurityGroup.
 
@@ -182,8 +182,8 @@ class AWSSecurityGroupService(BaseSecurityGroupService):
         :param description: The description of the new security group.
 
         :type  network_id: ``str``
-        :param network_id: The ID of the VPC to create the security group in,
-                           if any.
+        :param network_id: The ID of the VPC under which to create the security
+                           group.
 
         :rtype: ``object`` of :class:`.SecurityGroup`
         :return:  A SecurityGroup instance or ``None`` if one was not created.

cloudbridge/cloud/providers/openstack/services.py

@@ -196,7 +196,7 @@ class OpenStackSecurityGroupService(BaseSecurityGroupService):
         return ClientPagedResultList(self.provider, sgs,
                                      limit=limit, marker=marker)
 
-    def create(self, name, description, network_id=None):
+    def create(self, name, description, network_id):
         """
         Create a new security group under the current account.
 

test/test_security_service.py

@@ -1,3 +1,4 @@
+"""Test cloudbridge.security modules."""
 import json
 import uuid
 
@@ -96,14 +97,16 @@ class CloudSecurityServiceTestCase(ProviderTestBase):
             "Key pair {0} should have been deleted but still exists."
             .format(name))
 
+    def cleanup_sg(self, sg, net):
+        self.provider.security.security_groups.delete(group_id=sg.id)
+        self.provider.network.delete(network_id=net.id)
+
     def test_crud_security_group_service(self):
         name = 'cbtestsecuritygroupA-{0}'.format(uuid.uuid4())
+        net = self.provider.network.create(name=name)
         sg = self.provider.security.security_groups.create(
-            name=name, description=name)
-        with helpers.cleanup_action(
-            lambda:
-                self.provider.security.security_groups.delete(group_id=sg.id)
-        ):
+            name=name, description=name, network_id=net.id)
+        with helpers.cleanup_action(lambda: self.cleanup_sg(sg, net)):
             self.assertEqual(name, sg.description)
 
             # test list method
@@ -154,9 +157,10 @@ class CloudSecurityServiceTestCase(ProviderTestBase):
     def test_security_group(self):
         """Test for proper creation of a security group."""
         name = 'cbtestsecuritygroupB-{0}'.format(uuid.uuid4())
+        net = self.provider.network.create(name=name)
         sg = self.provider.security.security_groups.create(
-            name=name, description=name)
-        with helpers.cleanup_action(lambda: sg.delete()):
+            name=name, description=name, network_id=net.id)
+        with helpers.cleanup_action(lambda: self.cleanup_sg(sg, net)):
             rule = sg.add_rule(ip_protocol='tcp', from_port=1111, to_port=1111,
                                cidr_ip='0.0.0.0/0')
             found_rule = sg.get_rule(ip_protocol='tcp', from_port=1111,
@@ -202,9 +206,10 @@ class CloudSecurityServiceTestCase(ProviderTestBase):
     def test_security_group_rule_add_twice(self):
         """Test whether adding the same rule twice succeeds."""
         name = 'cbtestsecuritygroupB-{0}'.format(uuid.uuid4())
+        net = self.provider.network.create(name=name)
         sg = self.provider.security.security_groups.create(
-            name=name, description=name)
-        with helpers.cleanup_action(lambda: sg.delete()):
+            name=name, description=name, network_id=net.id)
+        with helpers.cleanup_action(lambda: self.cleanup_sg(sg, net)):
             rule = sg.add_rule(ip_protocol='tcp', from_port=1111, to_port=1111,
                                cidr_ip='0.0.0.0/0')
             # attempting to add the same rule twice should succeed
@@ -218,14 +223,16 @@ class CloudSecurityServiceTestCase(ProviderTestBase):
     def test_security_group_group_rule(self):
         """Test for proper creation of a security group rule."""
         name = 'cbtestsecuritygroupC-{0}'.format(uuid.uuid4())
+        net = self.provider.network.create(name=name)
         sg = self.provider.security.security_groups.create(
-            name=name, description=name)
-        with helpers.cleanup_action(lambda: sg.delete()):
+            name=name, description=name, network_id=net.id)
+        with helpers.cleanup_action(lambda: self.cleanup_sg(sg, net)):
             self.assertTrue(
                 len(sg.rules) == 0,
                 "Expected no security group group rule. Got {0}."
                 .format(sg.rules))
-            rule = sg.add_rule(src_group=sg)
+            rule = sg.add_rule(src_group=sg, ip_protocol='tcp', from_port=0,
+                               to_port=65535)
             self.assertTrue(
                 rule.group.name == name,
                 "Expected security group rule name {0}. Got {1}."