Просмотр исходного кода

Merge pull request #71 from gvlproject/test_improvements

Added tests for resource name validation during resource creation
Nuwan Goonasekera 8 лет назад
Родитель
Сommit
433660e50f

+ 39 - 11
cloudbridge/cloud/base/resources.py

@@ -11,6 +11,7 @@ import time
 
 from cloudbridge.cloud.interfaces.exceptions \
     import InvalidConfigurationException
+from cloudbridge.cloud.interfaces.exceptions import InvalidNameException
 from cloudbridge.cloud.interfaces.exceptions import WaitStateException
 from cloudbridge.cloud.interfaces.resources import AttachmentInfo
 from cloudbridge.cloud.interfaces.resources import Bucket
@@ -188,8 +189,17 @@ class BaseCloudResource(CloudResource):
     def __init__(self, provider):
         self.__provider = provider
 
-    def is_valid_resource_name(self, name):
-        return True if self.CB_NAME_PATTERN.match(name) else False
+    @staticmethod
+    def is_valid_resource_name(name):
+        return True if BaseCloudResource.CB_NAME_PATTERN.match(name) else False
+
+    @staticmethod
+    def assert_valid_resource_name(name):
+        if not BaseCloudResource.is_valid_resource_name(name):
+            raise InvalidNameException(
+                u"Invalid name: %s. Name must be at most 63 characters "
+                "long and consist of lowercase letters, numbers, "
+                "underscores, dashes or international characters" % name)
 
     @property
     def _provider(self):
@@ -748,8 +758,17 @@ class BaseBucketObject(BaseCloudResource, BucketObject):
     def __init__(self, provider):
         super(BaseBucketObject, self).__init__(provider)
 
-    def is_valid_resource_name(self, name):
-        return True if self.CB_NAME_PATTERN.match(name) else False
+    @staticmethod
+    def is_valid_resource_name(name):
+        return True if BaseBucketObject.CB_NAME_PATTERN.match(name) else False
+
+    @staticmethod
+    def assert_valid_resource_name(name):
+        if not BaseBucketObject.is_valid_resource_name(name):
+            raise InvalidNameException(
+                u"Invalid object name: %s. Name must match criteria defined "
+                "in: http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMeta"
+                "data.html#object-key-guidelines" % name)
 
     def save_content(self, target_stream):
         """
@@ -779,13 +798,22 @@ class BaseBucket(BaseCloudResource, BasePageableObjectMixin, Bucket):
     #
     # NOTE: The following regex is based on: https://stackoverflow.com/questio
     # ns/2063213/regular-expression-for-validating-dns-label-host-name
-    CB_NAME_PATTERN = re.compile(r"^(?![0-9]+$)(?!-)[a-zA-Z0-9-]{,63}(?<!-)$")
+    CB_NAME_PATTERN = re.compile(r"^(?![0-9]+$)(?!-)[a-z0-9-]{3,63}(?<!-)$")
 
     def __init__(self, provider):
         super(BaseBucket, self).__init__(provider)
 
-    def is_valid_resource_name(self, name):
-        return True if self.CB_NAME_PATTERN.match(name) else False
+    @staticmethod
+    def is_valid_resource_name(name):
+        return True if BaseBucket.CB_NAME_PATTERN.match(name) else False
+
+    @staticmethod
+    def assert_valid_resource_name(name):
+        if not BaseBucket.is_valid_resource_name(name):
+            raise InvalidNameException(
+                u"Invalid bucket name: %s. Name must match criteria defined "
+                "in: http://docs.aws.amazon.com/awscloudtrail/latest/userguide"
+                "/cloudtrail-s3-bucket-naming-requirements.html" % name)
 
     def __eq__(self, other):
         return (isinstance(other, Bucket) and
@@ -803,7 +831,7 @@ class BaseBucket(BaseCloudResource, BasePageableObjectMixin, Bucket):
 class BaseNetwork(BaseCloudResource, BaseObjectLifeCycleMixin, Network):
 
     CB_DEFAULT_NETWORK_NAME = os.environ.get('CB_DEFAULT_NETWORK_NAME',
-                                             'CloudBridgeNet')
+                                             'cloudbridge_net')
 
     def __init__(self, provider):
         super(BaseNetwork, self).__init__(provider)
@@ -833,7 +861,7 @@ class BaseNetwork(BaseCloudResource, BaseObjectLifeCycleMixin, Network):
 class BaseSubnet(BaseCloudResource, BaseObjectLifeCycleMixin, Subnet):
 
     CB_DEFAULT_SUBNET_NAME = os.environ.get('CB_DEFAULT_SUBNET_NAME',
-                                            'CloudBridgeSubnet')
+                                            'cloudbridge_subnet')
 
     def __init__(self, provider):
         super(BaseSubnet, self).__init__(provider)
@@ -881,7 +909,7 @@ class BaseFloatingIP(BaseCloudResource, FloatingIP):
 class BaseRouter(BaseCloudResource, Router):
 
     CB_DEFAULT_ROUTER_NAME = os.environ.get('CB_DEFAULT_ROUTER_NAME',
-                                            'CloudBridgeRouter')
+                                            'cloudbridge_router')
 
     def __init__(self, provider):
         super(BaseRouter, self).__init__(provider)
@@ -901,7 +929,7 @@ class BaseInternetGateway(BaseCloudResource, BaseObjectLifeCycleMixin,
                           InternetGateway):
 
     CB_DEFAULT_INET_GATEWAY_NAME = os.environ.get(
-        'CB_DEFAULT_INET_GATEWAY_NAME', 'CloudBridgeInetGateway')
+        'CB_DEFAULT_INET_GATEWAY_NAME', 'cloudbridge_inetgateway')
 
     def __init__(self, provider):
         super(BaseInternetGateway, self).__init__(provider)

+ 2 - 5
cloudbridge/cloud/interfaces/exceptions.py

@@ -44,8 +44,5 @@ class InvalidNameException(CloudBridgeBaseException):
     a cloudbridge resource.An example would be setting uppercase
     letters, which are not allowed in a resource name.
     """
-    def __init__(self, name):
-        super(InvalidNameException, self).__init__(
-            u"Invalid name: %s. Name must be at most 63 characters long"
-            " and consist of lowercase letters, numbers, underscores, dashes"
-            " or international characters" % name)
+    def __init__(self, msg):
+        super(InvalidNameException, self).__init__(msg)

+ 16 - 29
cloudbridge/cloud/providers/aws/resources.py

@@ -29,7 +29,6 @@ from cloudbridge.cloud.base.resources import BaseSnapshot
 from cloudbridge.cloud.base.resources import BaseSubnet
 from cloudbridge.cloud.base.resources import BaseVolume
 from cloudbridge.cloud.base.resources import ClientPagedResultList
-from cloudbridge.cloud.interfaces.exceptions import InvalidNameException
 from cloudbridge.cloud.interfaces.resources import GatewayState
 from cloudbridge.cloud.interfaces.resources import InstanceState
 from cloudbridge.cloud.interfaces.resources import MachineImageState
@@ -261,10 +260,8 @@ class AWSInstance(BaseInstance):
         """
         Set the instance name.
         """
-        if self.is_valid_resource_name(value):
-            self._ec2_instance.add_tag('Name', value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._ec2_instance.add_tag('Name', value)
 
     @property
     def public_ips(self):
@@ -350,6 +347,8 @@ class AWSInstance(BaseInstance):
         """
         Create a new image based on this instance.
         """
+        self.assert_valid_resource_name(name)
+
         image_id = self._ec2_instance.create_image(name)
         # Sometimes, the image takes a while to register, so retry a few times
         # if the image cannot be found
@@ -451,10 +450,8 @@ class AWSVolume(BaseVolume):
         """
         Set the volume name.
         """
-        if self.is_valid_resource_name(value):
-            self._volume.add_tag('Name', value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._volume.add_tag('Name', value)
 
     @property
     def description(self):
@@ -575,10 +572,8 @@ class AWSSnapshot(BaseSnapshot):
         """
         Set the snapshot name.
         """
-        if self.is_valid_resource_name(value):
-            self._snapshot.add_tag('Name', value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._snapshot.add_tag('Name', value)
 
     @property
     def description(self):
@@ -1002,10 +997,8 @@ class AWSNetwork(BaseNetwork):
         """
         Set the network name.
         """
-        if self.is_valid_resource_name(value):
-            self._vpc.add_tag('Name', value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._vpc.add_tag('Name', value)
 
     @property
     def external(self):
@@ -1077,10 +1070,8 @@ class AWSSubnet(BaseSubnet):
         """
         Set the subnet name.
         """
-        if self.is_valid_resource_name(value):
-            self._subnet.add_tag('Name', value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._subnet.add_tag('Name', value)
 
     @property
     def cidr_block(self):
@@ -1163,10 +1154,8 @@ class AWSRouter(BaseRouter):
         """
         Set the router name.
         """
-        if self.is_valid_resource_name(value):
-            self._route_table.add_tag('Name', value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._route_table.add_tag('Name', value)
 
     def refresh(self):
         self._route_table = self._provider.vpc_conn.get_all_route_tables(
@@ -1232,10 +1221,8 @@ class AWSInternetGateway(BaseInternetGateway):
         """
         Set the router name.
         """
-        if self.is_valid_resource_name(value):
-            self._gateway.add_tag('Name', value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._gateway.add_tag('Name', value)
 
     def refresh(self):
         gateways = self._provider.vpc_conn.get_all_internet_gateways([self.id])

+ 20 - 0
cloudbridge/cloud/providers/aws/services.py

@@ -152,6 +152,8 @@ class AWSKeyPairService(BaseKeyPairService):
         :rtype: ``object`` of :class:`.KeyPair`
         :return:  A key pair instance or ``None`` if one was not be created.
         """
+        AWSKeyPair.assert_valid_resource_name(name)
+
         kp = self.provider.ec2_conn.create_key_pair(name)
         if kp:
             return AWSKeyPair(self.provider, kp)
@@ -209,6 +211,8 @@ class AWSSecurityGroupService(BaseSecurityGroupService):
         :rtype: ``object`` of :class:`.SecurityGroup`
         :return:  A SecurityGroup instance or ``None`` if one was not created.
         """
+        AWSSecurityGroup.assert_valid_resource_name(name)
+
         sg = self.provider.ec2_conn.create_security_group(name, description,
                                                           network_id)
         if sg:
@@ -308,6 +312,8 @@ class AWSVolumeService(BaseVolumeService):
         """
         Creates a new volume.
         """
+        AWSVolume.assert_valid_resource_name(name)
+
         zone_id = zone.id if isinstance(zone, PlacementZone) else zone
         snapshot_id = snapshot.id if isinstance(
             snapshot, AWSSnapshot) and snapshot else snapshot
@@ -368,6 +374,8 @@ class AWSSnapshotService(BaseSnapshotService):
         """
         Creates a new snapshot of a given volume.
         """
+        AWSSnapshot.assert_valid_resource_name(name)
+
         volume_id = volume.id if isinstance(volume, AWSVolume) else volume
 
         ec2_snap = self.provider.ec2_conn.create_snapshot(
@@ -434,6 +442,8 @@ class AWSObjectStoreService(BaseObjectStoreService):
         """
         Create a new bucket.
         """
+        AWSBucket.assert_valid_resource_name(name)
+
         bucket = self.provider.s3_conn.create_bucket(
             name,
             location=location if location else '')
@@ -514,6 +524,8 @@ class AWSInstanceService(BaseInstanceService):
     def create(self, name, image, instance_type, subnet, zone=None,
                key_pair=None, security_groups=None, user_data=None,
                launch_config=None, **kwargs):
+        AWSInstance.assert_valid_resource_name(name)
+
         image_id = image.id if isinstance(image, MachineImage) else image
         instance_size = instance_type.id if \
             isinstance(instance_type, InstanceType) else instance_type
@@ -797,6 +809,8 @@ class AWSNetworkService(BaseNetworkService):
                                      limit=limit, marker=marker)
 
     def create(self, name, cidr_block):
+        AWSNetwork.assert_valid_resource_name(name)
+
         network = self.provider.vpc_conn.create_vpc(cidr_block=cidr_block)
         cb_network = AWSNetwork(self.provider, network)
         if name:
@@ -864,6 +878,8 @@ class AWSSubnetService(BaseSubnetService):
                                      limit=limit, marker=marker)
 
     def create(self, name, network, cidr_block, zone=None):
+        AWSSubnet.assert_valid_resource_name(name)
+
         network_id = network.id if isinstance(network, AWSNetwork) else network
         subnet = self.provider.vpc_conn.create_subnet(network_id, cidr_block,
                                                       availability_zone=zone)
@@ -939,6 +955,8 @@ class AWSRouterService(BaseRouterService):
                                      marker=marker)
 
     def create(self, name, network):
+        AWSRouter.assert_valid_resource_name(name)
+
         network_id = network.id if isinstance(network, AWSNetwork) else network
         router = self.provider.vpc_conn.create_route_table(vpc_id=network_id)
         cb_router = AWSRouter(self.provider, router)
@@ -954,6 +972,8 @@ class AWSGatewayService(BaseGatewayService):
         super(AWSGatewayService, self).__init__(provider)
 
     def get_or_create_inet_gateway(self, name):
+        AWSInternetGateway.assert_valid_resource_name(name)
+
         gateway = self.provider.vpc_conn.create_internet_gateway()
         cb_gateway = AWSInternetGateway(self.provider, gateway)
         cb_gateway.wait_till_ready()

+ 28 - 40
cloudbridge/cloud/providers/openstack/resources.py

@@ -25,7 +25,6 @@ from cloudbridge.cloud.base.resources import BaseSnapshot
 from cloudbridge.cloud.base.resources import BaseSubnet
 from cloudbridge.cloud.base.resources import BaseVolume
 from cloudbridge.cloud.base.resources import ClientPagedResultList
-from cloudbridge.cloud.interfaces.exceptions import InvalidNameException
 from cloudbridge.cloud.interfaces.resources import GatewayState
 from cloudbridge.cloud.interfaces.resources import InstanceState
 from cloudbridge.cloud.interfaces.resources import MachineImageState
@@ -270,11 +269,10 @@ class OpenStackInstance(BaseInstance):
         """
         Set the instance name.
         """
-        if self.is_valid_resource_name(value):
-            self._os_instance.name = value
-            self._os_instance.update(name=value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+
+        self._os_instance.name = value
+        self._os_instance.update(name=value)
 
     @property
     def public_ips(self):
@@ -377,6 +375,8 @@ class OpenStackInstance(BaseInstance):
         """
         Create a new image based on this instance.
         """
+        self.assert_valid_resource_name(name)
+
         image_id = self._os_instance.create_image(name)
         return OpenStackMachineImage(
             self._provider, self._provider.compute.images.get(image_id))
@@ -498,11 +498,9 @@ class OpenStackVolume(BaseVolume):
         """
         Set the volume name.
         """
-        if self.is_valid_resource_name(value):
-            self._volume.name = value
-            self._volume.update(name=value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._volume.name = value
+        self._volume.update(name=value)
 
     @property
     def description(self):
@@ -621,11 +619,9 @@ class OpenStackSnapshot(BaseSnapshot):
         """
         Set the snapshot name.
         """
-        if self.is_valid_resource_name(value):
-            self._snapshot.name = value
-            self._snapshot.update(name=value)
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._snapshot.name = value
+        self._snapshot.update(name=value)
 
     @property
     def description(self):
@@ -719,12 +715,10 @@ class OpenStackNetwork(BaseNetwork):
         """
         Set the network name.
         """
-        if self.is_valid_resource_name(value):
-            self._provider.neutron.update_network(self.id,
-                                                  {'network': {'name': value}})
-            self.refresh()
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._provider.neutron.update_network(self.id,
+                                              {'network': {'name': value}})
+        self.refresh()
 
     @property
     def external(self):
@@ -791,12 +785,10 @@ class OpenStackSubnet(BaseSubnet):
         """
         Set the subnet name.
         """
-        if self.is_valid_resource_name(value):
-            self._provider.neutron.update_subnet(
-                self.id, {'subnet': {'name': value}})
-            self._subnet['name'] = value
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._provider.neutron.update_subnet(
+            self.id, {'subnet': {'name': value}})
+        self._subnet['name'] = value
 
     @property
     def cidr_block(self):
@@ -885,12 +877,10 @@ class OpenStackRouter(BaseRouter):
         """
         Set the router name.
         """
-        if self.is_valid_resource_name(value):
-            self._provider.neutron.update_router(
-                self.id, {'router': {'name': value}})
-            self.refresh()
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._provider.neutron.update_router(
+            self.id, {'router': {'name': value}})
+        self.refresh()
 
     def refresh(self):
         self._router = self._provider.neutron.show_router(self.id)['router']
@@ -963,12 +953,10 @@ class OpenStackInternetGateway(BaseInternetGateway):
     @name.setter
     # pylint:disable=arguments-differ
     def name(self, value):
-        if self.is_valid_resource_name(value):
-            self._provider.neutron.update_network(self.id,
-                                                  {'network': {'name': value}})
-            self.refresh()
-        else:
-            raise InvalidNameException(value)
+        self.assert_valid_resource_name(value)
+        self._provider.neutron.update_network(self.id,
+                                              {'network': {'name': value}})
+        self.refresh()
 
     @property
     def network_id(self):

+ 22 - 0
cloudbridge/cloud/providers/openstack/services.py

@@ -167,6 +167,8 @@ class OpenStackKeyPairService(BaseKeyPairService):
         :rtype: ``object`` of :class:`.KeyPair`
         :return:  A key pair instance or ``None`` if one was not be created.
         """
+        OpenStackKeyPair.assert_valid_resource_name(name)
+
         kp = self.provider.nova.keypairs.create(name)
         if kp:
             return OpenStackKeyPair(self.provider, kp)
@@ -219,6 +221,8 @@ class OpenStackSecurityGroupService(BaseSecurityGroupService):
         :rtype: ``object`` of :class:`.SecurityGroup`
         :return: a SecurityGroup object
         """
+        OpenStackSecurityGroup.assert_valid_resource_name(name)
+
         sg = self.provider.nova.security_groups.create(name, description)
         if sg:
             return OpenStackSecurityGroup(self.provider, sg)
@@ -372,6 +376,8 @@ class OpenStackVolumeService(BaseVolumeService):
         """
         Creates a new volume.
         """
+        OpenStackVolume.assert_valid_resource_name(name)
+
         zone_id = zone.id if isinstance(zone, PlacementZone) else zone
         snapshot_id = snapshot.id if isinstance(
             snapshot, OpenStackSnapshot) and snapshot else snapshot
@@ -429,6 +435,8 @@ class OpenStackSnapshotService(BaseSnapshotService):
         """
         Creates a new snapshot of a given volume.
         """
+        OpenStackSnapshot.assert_valid_resource_name(name)
+
         volume_id = (volume.id if isinstance(volume, OpenStackVolume)
                      else volume)
 
@@ -484,6 +492,8 @@ class OpenStackObjectStoreService(BaseObjectStoreService):
         """
         Create a new bucket.
         """
+        OpenStackBucket.assert_valid_resource_name(name)
+
         self.provider.swift.put_container(name)
         return self.get(name)
 
@@ -561,6 +571,8 @@ class OpenStackInstanceService(BaseInstanceService):
                launch_config=None,
                **kwargs):
         """Create a new virtual machine instance."""
+        OpenStackInstance.assert_valid_resource_name(name)
+
         image_id = image.id if isinstance(image, MachineImage) else image
         instance_size = instance_type.id if \
             isinstance(instance_type, InstanceType) else \
@@ -774,6 +786,8 @@ class OpenStackNetworkService(BaseNetworkService):
                                      limit=limit, marker=marker)
 
     def create(self, name, cidr_block):
+        OpenStackNetwork.assert_valid_resource_name(name)
+
         net_info = {'name': name}
         network = self.provider.neutron.create_network({'network': net_info})
         return OpenStackNetwork(self.provider, network.get('network'))
@@ -804,6 +818,8 @@ class OpenStackNetworkService(BaseNetworkService):
         return [OpenStackRouter(self.provider, r) for r in routers]
 
     def create_router(self, name=None):
+        OpenStackRouter.assert_valid_resource_name(name)
+
         router = self.provider.neutron.create_router(
             {'router': {'name': name}})
         return OpenStackRouter(self.provider, router.get('router'))
@@ -832,6 +848,8 @@ class OpenStackSubnetService(BaseSubnetService):
 
     def create(self, name, network, cidr_block, zone=None):
         """zone param is ignored."""
+        OpenStackSubnet.assert_valid_resource_name(name)
+
         network_id = (network.id if isinstance(network, OpenStackNetwork)
                       else network)
         subnet_info = {'name': name, 'network_id': network_id,
@@ -904,6 +922,8 @@ class OpenStackRouterService(BaseRouterService):
         https://developer.openstack.org/api-ref/networking/v2/
             ?expanded=delete-router-detail,create-router-detail#create-router
         """
+        OpenStackRouter.assert_valid_resource_name(name)
+
         body = {'router': {'name': name}} if name else None
         router = self.provider.neutron.create_router(body)
         return OpenStackRouter(self.provider, router.get('router'))
@@ -915,6 +935,8 @@ class OpenStackGatewayService(BaseGatewayService):
         super(OpenStackGatewayService, self).__init__(provider)
 
     def get_or_create_inet_gateway(self, name):
+        OpenStackInternetGateway.assert_valid_resource_name(name)
+
         for n in self.provider.networking.networks:
             if n.external:
                 return OpenStackInternetGateway(self.provider, n)

+ 94 - 2
test/helpers/standard_interface_tests.py

@@ -5,10 +5,12 @@ This includes:
    2. Checking for object equality and repr
    3. Checking standard behaviour for list, iter, find, get, delete
 """
+import test.helpers as helpers
 import uuid
 
 from cloudbridge.cloud.interfaces.exceptions \
     import InvalidNameException
+from cloudbridge.cloud.interfaces.resources import ObjectLifeCycleMixin
 from cloudbridge.cloud.interfaces.resources import ResultList
 
 
@@ -129,7 +131,7 @@ def check_obj_name(test, obj):
             obj.name = "hello world"
         # setting upper case characters should raise an exception
         with test.assertRaises(InvalidNameException):
-            obj.name = "hello World"
+            obj.name = "helloWorld"
         # setting special characters should raise an exception
         with test.assertRaises(InvalidNameException):
             obj.name = "hello.world:how_goes_it"
@@ -141,7 +143,6 @@ def check_obj_name(test, obj):
         obj.refresh()
         test.assertEqual(obj.name, VALID_NAME)
         obj.name = original_name
-    pass
 
 
 def check_standard_behaviour(test, service, obj):
@@ -181,3 +182,94 @@ def check_standard_behaviour(test, service, obj):
         " are not as expected: {4}" .format(objs_list[0].id, objs_iter[0].id,
                                             objs_find[0].id, obj_get.id,
                                             obj.id))
+
+
+def check_create(test, service, iface, name_prefix,
+                 create_func, cleanup_func):
+
+    # check create with invalid name
+    with test.assertRaises(InvalidNameException):
+        # spaces should raise an exception
+        create_func("hello world")
+    # check create with invalid name
+    with test.assertRaises(InvalidNameException):
+        # uppercase characters should raise an exception
+        create_func("helloWorld")
+    # setting special characters should raise an exception
+    with test.assertRaises(InvalidNameException):
+        create_func("hello.world:how_goes_it")
+    # setting a length > 63 should result in an exception
+    with test.assertRaises(InvalidNameException,
+                           msg="Name of length > 64 should be disallowed"):
+        create_func("a" * 64)
+
+
+def check_crud(test, service, iface, name_prefix,
+               create_func, cleanup_func, extra_test_func=None,
+               custom_check_delete=None, skip_name_check=False):
+    """
+    Checks crud behaviour of a given cloudbridge service. The create_func will
+    be used as a factory function to create a service object and the
+    cleanup_func will be used to destroy the object. Once an object is created
+    using the create_func, all other standard behavioural tests can be run
+    against that object.
+
+    :type  test: ``TestCase``
+    :param test: The TestCase object to use
+
+    :type  service: ``CloudService``
+    :param service: The CloudService object under test. For example,
+                    a VolumeService object.
+
+    :type  iface: ``type``
+    :param iface: The type to test behaviour against. This type must be a
+                  subclass of ``CloudResource``.
+
+    :type  name_prefix: ``str``
+    :param name_prefix: The name to prefix all created objects with. This
+                        function will generated a new name with the
+                        specified name_prefix for each test object created
+                        and pass that name into the create_func
+
+    :type  create_func: ``func``
+    :param create_func: The create_func must accept the name of the object to
+                        create as a parameter and return the constructed
+                        object.
+
+    :type  cleanup_func: ``func``
+    :param cleanup_func: The cleanup_func must accept the created object
+                         and perform all cleanup tasks required to delete the
+                         object.
+
+    :type  extra_test_func: ``func``
+    :param extra_test_func: This function will be called to perform additional
+                            tests after object construction and initialization,
+                            but before object cleanup. It will receive the
+                            created object as a parameter.
+
+    :type  custom_check_delete: ``func``
+    :param custom_check_delete: If provided, this function will be called
+                                instead of the standard check_delete function
+                                to make sure that the object has been deleted.
+
+    :type  skip_name_check: ``boolean``
+    :param skip_name_check:  If True, the invalid name checking will be
+                             skipped.
+    """
+
+    obj = None
+    with helpers.cleanup_action(lambda: cleanup_func(obj)):
+        if not skip_name_check:
+            check_create(test, service, iface, name_prefix,
+                         create_func, cleanup_func)
+        name = "{0}-{1}".format(name_prefix, helpers.get_uuid())
+        obj = create_func(name)
+        if issubclass(iface, ObjectLifeCycleMixin):
+            obj.wait_till_ready()
+        check_standard_behaviour(test, service, obj)
+        if extra_test_func:
+            extra_test_func(obj)
+    if custom_check_delete:
+        custom_check_delete(obj)
+    else:
+        check_delete(test, service, obj)

+ 42 - 45
test/test_block_store_service.py

@@ -5,9 +5,13 @@ from test import helpers
 from test.helpers import ProviderTestBase
 from test.helpers import standard_interface_tests as sit
 
+from cloudbridge.cloud.factory import ProviderList
 from cloudbridge.cloud.interfaces import SnapshotState
 from cloudbridge.cloud.interfaces import VolumeState
+from cloudbridge.cloud.interfaces.provider import TestMockHelperMixin
 from cloudbridge.cloud.interfaces.resources import AttachmentInfo
+from cloudbridge.cloud.interfaces.resources import Snapshot
+from cloudbridge.cloud.interfaces.resources import Volume
 
 import six
 
@@ -20,23 +24,19 @@ class CloudBlockStoreServiceTestCase(ProviderTestBase):
         Create a new volume, check whether the expected values are set,
         and delete it
         """
-        name = "cb_createvol-{0}".format(helpers.get_uuid())
-        test_vol = self.provider.block_store.volumes.create(
-            name,
-            1,
-            helpers.get_provider_test_data(self.provider, "placement"))
+        def create_vol(name):
+            return self.provider.block_store.volumes.create(
+                name,
+                1,
+                helpers.get_provider_test_data(self.provider, "placement"))
 
         def cleanup_vol(vol):
             vol.delete()
             vol.wait_for([VolumeState.DELETED, VolumeState.UNKNOWN],
                          terminal_states=[VolumeState.ERROR])
 
-        with helpers.cleanup_action(lambda: cleanup_vol(test_vol)):
-            test_vol.wait_till_ready()
-            sit.check_standard_behaviour(
-                self, self.provider.block_store.volumes, test_vol)
-
-        sit.check_delete(self, self.provider.block_store.volumes, test_vol)
+        sit.check_crud(self, self.provider.block_store.volumes, Volume,
+                       "cb_createvol", create_vol, cleanup_vol)
 
     @helpers.skipIfNoService(['block_store.volumes'])
     def test_attach_detach_volume(self):
@@ -142,9 +142,10 @@ class CloudBlockStoreServiceTestCase(ProviderTestBase):
             helpers.get_provider_test_data(self.provider, "placement"))
         with helpers.cleanup_action(lambda: test_vol.delete()):
             test_vol.wait_till_ready()
-            snap_name = "cb_snap-{0}".format(name)
-            test_snap = test_vol.create_snapshot(name=snap_name,
-                                                 description=snap_name)
+
+            def create_snap(name):
+                return test_vol.create_snapshot(name=name,
+                                                description=name)
 
             def cleanup_snap(snap):
                 snap.delete()
@@ -152,39 +153,19 @@ class CloudBlockStoreServiceTestCase(ProviderTestBase):
                     [SnapshotState.UNKNOWN],
                     terminal_states=[SnapshotState.ERROR])
 
-            with helpers.cleanup_action(lambda: cleanup_snap(test_snap)):
-                test_snap.wait_till_ready()
-                sit.check_standard_behaviour(
-                    self, self.provider.block_store.snapshots, test_snap)
-
-                # Test volume creation from a snapshot (via VolumeService)
-                sv_name = "cb_snapvol_{0}".format(name)
-                snap_vol = self.provider.block_store.volumes.create(
-                    sv_name,
-                    1,
-                    helpers.get_provider_test_data(self.provider, "placement"),
-                    snapshot=test_snap)
-                with helpers.cleanup_action(lambda: snap_vol.delete()):
-                    snap_vol.wait_till_ready()
-
-                # Test volume creation from a snapshot (via Snapshot)
-                snap_vol2 = test_snap.create_volume(
-                    helpers.get_provider_test_data(self.provider, "placement"))
-                with helpers.cleanup_action(lambda: snap_vol2.delete()):
-                    snap_vol2.wait_till_ready()
-
-            sit.check_delete(
-                self, self.provider.block_store.snapshots, test_snap)
+            sit.check_crud(self, self.provider.block_store.snapshots, Snapshot,
+                           "cb_snap", create_snap, cleanup_snap)
 
             # Test creation of a snap via SnapshotService
-            snap_two_name = "cb_snaptwo-{0}".format(name)
-            time.sleep(15)  # Or get SnapshotCreationPerVolumeRateExceeded
-            test_snap_two = self.provider.block_store.snapshots.create(
-                name=snap_two_name, volume=test_vol, description=snap_two_name)
-            with helpers.cleanup_action(lambda: cleanup_snap(test_snap_two)):
-                test_snap_two.wait_till_ready()
-                sit.check_standard_behaviour(
-                    self, self.provider.block_store.snapshots, test_snap_two)
+            def create_snap2(name):
+                return self.provider.block_store.snapshots.create(
+                    name=name, volume=test_vol, description=name)
+
+            if not isinstance(self.provider, TestMockHelperMixin) and \
+               self.provider.PROVIDER_ID == ProviderList.AWS:
+                time.sleep(15)  # Or get SnapshotCreationPerVolumeRateExceeded
+            sit.check_crud(self, self.provider.block_store.snapshots, Snapshot,
+                           "cb_snaptwo", create_snap2, cleanup_snap)
 
     @helpers.skipIfNoService(['block_store.snapshots'])
     def test_snapshot_properties(self):
@@ -227,3 +208,19 @@ class CloudBlockStoreServiceTestCase(ProviderTestBase):
                 test_snap.refresh()
                 self.assertEqual(test_snap.name, 'snapnewname1')
                 self.assertEqual(test_snap.description, 'snapnewdescription1')
+
+                # Test volume creation from a snapshot (via VolumeService)
+                sv_name = "cb_snapvol_{0}".format(test_snap.name)
+                snap_vol = self.provider.block_store.volumes.create(
+                    sv_name,
+                    1,
+                    helpers.get_provider_test_data(self.provider, "placement"),
+                    snapshot=test_snap)
+                with helpers.cleanup_action(lambda: snap_vol.delete()):
+                    snap_vol.wait_till_ready()
+
+                # Test volume creation from a snapshot (via Snapshot)
+                snap_vol2 = test_snap.create_volume(
+                    helpers.get_provider_test_data(self.provider, "placement"))
+                with helpers.cleanup_action(lambda: snap_vol2.delete()):
+                    snap_vol2.wait_till_ready()

+ 25 - 14
test/test_compute_service.py

@@ -9,6 +9,7 @@ from cloudbridge.cloud.interfaces import InstanceState
 from cloudbridge.cloud.interfaces import InvalidConfigurationException
 from cloudbridge.cloud.interfaces import TestMockHelperMixin
 from cloudbridge.cloud.interfaces.exceptions import WaitStateException
+from cloudbridge.cloud.interfaces.resources import Instance
 from cloudbridge.cloud.interfaces.resources import InstanceType
 from cloudbridge.cloud.interfaces.resources import SnapshotState
 
@@ -22,24 +23,34 @@ class CloudComputeServiceTestCase(ProviderTestBase):
         name = "cb_instcrud-{0}".format(helpers.get_uuid())
         # Declare these variables and late binding will allow
         # the cleanup method access to the most current values
-        inst = None
         net = None
+        subnet = None
+
+        def create_inst(name):
+            return helpers.get_test_instance(self.provider, name,
+                                             subnet=subnet)
+
+        def cleanup_inst(inst):
+            inst.terminate()
+            inst.wait_for([InstanceState.TERMINATED, InstanceState.UNKNOWN])
+
+        def check_deleted(inst):
+            deleted_inst = self.provider.compute.instances.get(
+                inst.id)
+            self.assertTrue(
+                deleted_inst is None or deleted_inst.state in (
+                    InstanceState.TERMINATED,
+                    InstanceState.UNKNOWN),
+                "Instance %s should have been deleted but still exists." %
+                name)
+
         with helpers.cleanup_action(lambda: helpers.cleanup_test_resources(
-                inst, net)):
+                                               network=net)):
             net, subnet = helpers.create_test_network(self.provider, name)
-            inst = helpers.get_test_instance(self.provider, name,
-                                             subnet=subnet)
 
-            sit.check_standard_behaviour(
-                self, self.provider.compute.instances, inst)
-        deleted_inst = self.provider.compute.instances.get(
-            inst.id)
-        self.assertTrue(
-            deleted_inst is None or deleted_inst.state in (
-                InstanceState.TERMINATED,
-                InstanceState.UNKNOWN),
-            "Instance %s should have been deleted but still exists." %
-            name)
+            sit.check_crud(self, self.provider.compute.instances, Instance,
+                           "cb_instcrud", create_inst, cleanup_inst,
+                           custom_check_delete=check_deleted)
 
     def _is_valid_ip(self, address):
         try:

+ 21 - 22
test/test_image_service.py

@@ -4,6 +4,7 @@ from test.helpers import standard_interface_tests as sit
 
 from cloudbridge.cloud.interfaces import MachineImageState
 from cloudbridge.cloud.interfaces import TestMockHelperMixin
+from cloudbridge.cloud.interfaces.resources import MachineImage
 
 
 class CloudImageServiceTestCase(ProviderTestBase):
@@ -22,6 +23,23 @@ class CloudImageServiceTestCase(ProviderTestBase):
         # the cleanup method access to the most current values
         test_instance = None
         net = None
+
+        def create_img(name):
+            return test_instance.create_image(name)
+
+        def cleanup_img(img):
+            img.delete()
+            img.wait_for(
+                [MachineImageState.UNKNOWN, MachineImageState.ERROR])
+
+        def extra_tests(img):
+            # TODO: Fix moto so that the BDM is populated correctly
+            if not isinstance(self.provider, TestMockHelperMixin):
+                # check image size
+                img.refresh()
+                self.assertGreater(img.min_disk, 0, "Minimum disk"
+                                   " size required by image is invalid")
+
         with helpers.cleanup_action(lambda: helpers.cleanup_test_resources(
                 test_instance, net)):
             net, subnet = helpers.create_test_network(
@@ -29,25 +47,6 @@ class CloudImageServiceTestCase(ProviderTestBase):
             test_instance = helpers.get_test_instance(
                 self.provider, instance_name, subnet=subnet)
 
-            name = "cb_listimg-{0}".format(helpers.get_uuid())
-            test_image = test_instance.create_image(name)
-
-            def cleanup_img(img):
-                img.delete()
-                img.wait_for(
-                    [MachineImageState.UNKNOWN, MachineImageState.ERROR])
-
-            with helpers.cleanup_action(lambda: cleanup_img(test_image)):
-                test_image.wait_till_ready()
-                sit.check_standard_behaviour(
-                    self, self.provider.compute.images, test_image)
-
-                # TODO: Fix moto so that the BDM is populated correctly
-                if not isinstance(self.provider, TestMockHelperMixin):
-                    # check image size
-                    test_image.refresh()
-                    self.assertGreater(test_image.min_disk, 0, "Minimum disk"
-                                       " size required by image is invalid")
-            # TODO: Images take a long time to deregister on EC2. Needs
-            # investigation
-            sit.check_delete(self, self.provider.compute.images, test_image)
+            sit.check_crud(self, self.provider.compute.images, MachineImage,
+                           "cb_listimg", create_img, cleanup_img,
+                           extra_test_func=extra_tests)

+ 66 - 57
test/test_network_service.py

@@ -3,6 +3,7 @@ import test.helpers as helpers
 from test.helpers import ProviderTestBase
 from test.helpers import standard_interface_tests as sit
 
+from cloudbridge.cloud.interfaces.resources import Network
 from cloudbridge.cloud.interfaces.resources import RouterState
 from cloudbridge.cloud.interfaces.resources import Subnet
 
@@ -10,64 +11,17 @@ from cloudbridge.cloud.interfaces.resources import Subnet
 class CloudNetworkServiceTestCase(ProviderTestBase):
 
     @helpers.skipIfNoService(['networking.networks'])
-    def test_crud_network_service(self):
-        name = 'cb_crudnetwork-{0}'.format(helpers.get_uuid())
-        subnet_name = 'cb_crudsubnet-{0}'.format(helpers.get_uuid())
-        net = self.provider.networking.networks.create(
-            name=name, cidr_block='10.0.0.0/16')
-        with helpers.cleanup_action(
-            lambda:
-                self.provider.networking.networks.delete(network_id=net.id)
-        ):
-            sit.check_standard_behaviour(
-                self, self.provider.networking.networks, net)
-
-            # check subnet
-            subnet = self.provider.networking.subnets.create(
-                network=net, cidr_block="10.0.0.1/24", name=subnet_name)
-            with helpers.cleanup_action(
-                lambda:
-                    self.provider.networking.subnets.delete(subnet=subnet)
-            ):
-                sit.check_standard_behaviour(
-                    self, self.provider.networking.subnets, subnet)
-
-            sit.check_delete(self, self.provider.networking.subnets, subnet)
-
-            # Check floating IP address
-            ip = self.provider.networking.networks.create_floating_ip()
-            ip_id = ip.id
-            with helpers.cleanup_action(lambda: ip.delete()):
-                ipl = self.provider.networking.networks.floating_ips
-                self.assertTrue(
-                    ip in ipl,
-                    "Floating IP address {0} should exist in the list {1}"
-                    .format(ip.id, ipl))
-                # 2016-08: address filtering not implemented in moto
-                # empty_ipl = self.provider.networking.networks.floating_ips(
-                #   'dummy-net')
-                # self.assertFalse(
-                #     empty_ipl,
-                #     "Bogus network should not have any floating IPs: {0}"
-                #     .format(empty_ipl))
-                self.assertIn(
-                    ip.public_ip, repr(ip),
-                    "repr(obj) should contain the address public IP value.")
-                self.assertFalse(
-                    ip.private_ip,
-                    "Floating IP should not have a private IP value ({0})."
-                    .format(ip.private_ip))
-                self.assertFalse(
-                    ip.in_use(),
-                    "Newly created floating IP address should not be in use.")
-            ipl = self.provider.networking.networks.floating_ips
-            found_ip = [a for a in ipl if a.id == ip_id]
-            self.assertTrue(
-                len(found_ip) == 0,
-                "Floating IP {0} should have been deleted but still exists."
-                .format(ip_id))
+    def test_crud_network(self):
+
+        def create_net(name):
+            return self.provider.networking.networks.create(
+                name=name, cidr_block='10.0.0.0/16')
+
+        def cleanup_net(net):
+            self.provider.networking.networks.delete(network_id=net.id)
 
-        sit.check_delete(self, self.provider.networking.networks, net)
+        sit.check_crud(self, self.provider.networking.networks, Network,
+                       "cb_crudnetwork", create_net, cleanup_net)
 
     @helpers.skipIfNoService(['networking.networks'])
     def test_network_properties(self):
@@ -110,6 +64,61 @@ class CloudNetworkServiceTestCase(ProviderTestBase):
                     "Subnet's CIDR %s should match the specified one %s." % (
                         sn.cidr_block, cidr))
 
+    def test_crud_subnet(self):
+        # Late binding will make sure that create_subnet gets the
+        # correct value
+        net = None
+
+        def create_subnet(name):
+            return self.provider.networking.subnets.create(
+                network=net, cidr_block="10.0.0.1/24", name=name)
+
+        def cleanup_subnet(subnet):
+            self.provider.networking.subnets.delete(subnet=subnet)
+
+        net_name = 'cb_crudsubnet-{0}'.format(helpers.get_uuid())
+        net = self.provider.networking.networks.create(
+            name=net_name, cidr_block='10.0.0.0/16')
+        with helpers.cleanup_action(
+            lambda:
+                self.provider.networking.networks.delete(network_id=net.id)
+        ):
+            sit.check_crud(self, self.provider.networking.subnets, Subnet,
+                           "cb_crudsubnet", create_subnet, cleanup_subnet)
+
+    def test_floating_ip_properties(self):
+        # Check floating IP address
+        ip = self.provider.networking.networks.create_floating_ip()
+        ip_id = ip.id
+        with helpers.cleanup_action(lambda: ip.delete()):
+            ipl = self.provider.networking.networks.floating_ips
+            self.assertTrue(
+                ip in ipl,
+                "Floating IP address {0} should exist in the list {1}"
+                .format(ip.id, ipl))
+            # 2016-08: address filtering not implemented in moto
+            # empty_ipl = self.provider.network.floating_ips('dummy-net')
+            # self.assertFalse(
+            #     empty_ipl,
+            #     "Bogus network should not have any floating IPs: {0}"
+            #     .format(empty_ipl))
+            self.assertIn(
+                ip.public_ip, repr(ip),
+                "repr(obj) should contain the address public IP value.")
+            self.assertFalse(
+                ip.private_ip,
+                "Floating IP should not have a private IP value ({0})."
+                .format(ip.private_ip))
+            self.assertFalse(
+                ip.in_use(),
+                "Newly created floating IP address should not be in use.")
+        ipl = self.provider.networking.networks.floating_ips
+        found_ip = [a for a in ipl if a.id == ip_id]
+        self.assertTrue(
+            len(found_ip) == 0,
+            "Floating IP {0} should have been deleted but still exists."
+            .format(ip_id))
+
     @helpers.skipIfNoService(['networking.networks.routers'])
     def test_crud_router(self):
 

+ 50 - 8
test/test_object_store_service.py

@@ -10,6 +10,8 @@ from test.helpers import ProviderTestBase
 from test.helpers import standard_interface_tests as sit
 from unittest import skip
 
+from cloudbridge.cloud.interfaces.exceptions import InvalidNameException
+from cloudbridge.cloud.interfaces.resources import Bucket
 from cloudbridge.cloud.interfaces.resources import BucketObject
 
 import requests
@@ -23,16 +25,58 @@ class CloudObjectStoreServiceTestCase(ProviderTestBase):
         Create a new bucket, check whether the expected values are set,
         and delete it.
         """
-        name = "cbtestcreatebucket-{0}".format(uuid.uuid4())
-        test_bucket = self.provider.object_store.create(name)
+        def create_bucket(name):
+            return self.provider.object_store.create(name)
+
+        def cleanup_bucket(bucket):
+            bucket.delete()
+
+        with self.assertRaises(InvalidNameException):
+            # underscores are not allowed in bucket names
+            create_bucket("cb_bucket")
+
+        with self.assertRaises(InvalidNameException):
+            # names of length less than 3 should raise an exception
+            create_bucket("cb")
+
+        with self.assertRaises(InvalidNameException):
+            # names of length less than 63 should raise an exception
+            create_bucket("a" * 64)
+
+        with self.assertRaises(InvalidNameException):
+            # bucket name cannot be an IP address
+            create_bucket("197.10.100.42")
+
+        sit.check_crud(self, self.provider.object_store, Bucket,
+                       "cb-crudbucket", create_bucket, cleanup_bucket,
+                       skip_name_check=True)
+
+    @helpers.skipIfNoService(['object_store'])
+    def test_crud_bucket_object(self):
+        test_bucket = None
+
+        def create_bucket_obj(name):
+            obj = test_bucket.create_object(name)
+            # TODO: This is wrong. We shouldn't have to have a separate
+            # call to upload some content before being able to delete
+            # the content. Maybe the create_object method should accept
+            # the file content as a parameter.
+            obj.upload("dummy content")
+            return obj
+
+        def cleanup_bucket_obj(bucket_obj):
+            bucket_obj.delete()
+
         with helpers.cleanup_action(lambda: test_bucket.delete()):
-            sit.check_standard_behaviour(
-                self, self.provider.object_store, test_bucket)
+            name = "cb-crudbucketobj-{0}".format(uuid.uuid4())
+            test_bucket = self.provider.object_store.create(name)
 
-        sit.check_delete(self, self.provider.object_store, test_bucket)
+            sit.check_crud(self, test_bucket, BucketObject,
+                           "cb_bucketobj", create_bucket_obj,
+                           cleanup_bucket_obj, skip_name_check=True)
 
     @helpers.skipIfNoService(['object_store'])
-    def test_crud_bucket_objects(self):
+    def test_crud_bucket_object_properties(self):
         """
         Create a new bucket, upload some contents into the bucket, and
         check whether list properly detects the new content.
@@ -72,8 +116,6 @@ class CloudObjectStoreServiceTestCase(ProviderTestBase):
                 iter_objs = list(test_bucket)
                 self.assertListEqual(iter_objs, objs)
 
-                sit.check_standard_behaviour(self, test_bucket, obj)
-
                 obj_too = test_bucket.get(obj_name)
                 self.assertTrue(
                     isinstance(obj_too, BucketObject),

+ 28 - 39
test/test_security_service.py

@@ -6,26 +6,29 @@ from test.helpers import ProviderTestBase
 from test.helpers import standard_interface_tests as sit
 
 from cloudbridge.cloud.interfaces import TestMockHelperMixin
+from cloudbridge.cloud.interfaces.resources import KeyPair
+from cloudbridge.cloud.interfaces.resources import SecurityGroup
 
 
 class CloudSecurityServiceTestCase(ProviderTestBase):
 
     @helpers.skipIfNoService(['security.key_pairs'])
     def test_crud_key_pair_service(self):
-        name = 'cb_crudkp-{0}'.format(helpers.get_uuid())
-        kp = self.provider.security.key_pairs.create(name=name)
-        with helpers.cleanup_action(
-            lambda:
-                self.provider.security.key_pairs.delete(key_pair_id=kp.id)
-        ):
-            sit.check_standard_behaviour(
-                self, self.provider.security.key_pairs, kp)
 
+        def create_kp(name):
+            return self.provider.security.key_pairs.create(name=name)
+
+        def cleanup_kp(kp):
+            self.provider.security.key_pairs.delete(key_pair_id=kp.id)
+
+        def extra_tests(kp):
             # Recreating existing keypair should raise an exception
             with self.assertRaises(Exception):
-                self.provider.security.key_pairs.create(name=name)
+                self.provider.security.key_pairs.create(name=kp.name)
 
-        sit.check_delete(self, self.provider.security.key_pairs, kp)
+        sit.check_crud(self, self.provider.security.key_pairs, KeyPair,
+                       "cb_crudkp", create_kp, cleanup_kp,
+                       extra_test_func=extra_tests)
 
     @helpers.skipIfNoService(['security.key_pairs'])
     def test_key_pair_properties(self):
@@ -36,36 +39,31 @@ class CloudSecurityServiceTestCase(ProviderTestBase):
                 kp.material,
                 "KeyPair material is empty but it should not be.")
 
-    def cleanup_sg(self, sg, net):
-        with helpers.cleanup_action(
-                lambda: self.provider.networking.network.delete(
-                    network_id=net.id)):
-            self.provider.security.security_groups.delete(group_id=sg.id)
-
     @helpers.skipIfNoService(['security.security_groups'])
-    def test_crud_security_group_service(self):
+    def test_crud_security_group(self):
         name = 'cb_crudsg-{0}'.format(helpers.get_uuid())
 
         # Declare these variables and late binding will allow
         # the cleanup method access to the most current values
         net = None
-        sg = None
-        with helpers.cleanup_action(lambda: helpers.cleanup_test_resources(
-                network=net, security_group=sg)):
-            net, _ = helpers.create_test_network(self.provider, name)
-            sg = self.provider.security.security_groups.create(
+
+        def create_sg(name):
+            return self.provider.security.security_groups.create(
                 name=name, description=name, network_id=net.id)
 
-            self.assertEqual(name, sg.description)
+        def cleanup_sg(sg):
+            sg.delete()
 
-            sit.check_standard_behaviour(
-                self, self.provider.security.security_groups, sg)
+        with helpers.cleanup_action(lambda: helpers.cleanup_test_resources(
+                network=net)):
+            net, _ = helpers.create_test_network(self.provider, name)
 
-        sit.check_delete(self, self.provider.security.security_groups, sg)
+            sit.check_crud(self, self.provider.security.security_groups,
+                           SecurityGroup, "cb_crudsg", create_sg, cleanup_sg)
 
     @helpers.skipIfNoService(['security.security_groups'])
-    def test_security_group(self):
-        """Test for proper creation of a security group."""
+    def test_security_group_properties(self):
+        """Test properties of a security group."""
         name = 'cb_propsg-{0}'.format(helpers.get_uuid())
 
         # Declare these variables and late binding will allow
@@ -78,6 +76,8 @@ class CloudSecurityServiceTestCase(ProviderTestBase):
             sg = self.provider.security.security_groups.create(
                 name=name, description=name, network_id=net.id)
 
+            self.assertEqual(name, sg.description)
+
             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,
@@ -102,17 +102,6 @@ class CloudSecurityServiceTestCase(ProviderTestBase):
             self.assertFalse(
                 sg != sg,
                 "The same security groups should still be equal?")
-#             json_repr = json.dumps(
-#                 {"description": name, "name": name, "id": sg.id,
-#                  "rules":
-#                     [{"from_port": 1111, "group": "", "cidr_ip": "0.0.0.0/0",
-#                       "parent": sg.id, "to_port": 1111, "ip_protocol": "tcp",
-#                       "id": sg.rules[0].id}]},
-#                 sort_keys=True)
-#             self.assertTrue(
-#                 sg.to_json() == json_repr,
-#                 "JSON SG representation {0} does not match expected {1}"
-#                 .format(sg.to_json(), json_repr))
 
         sit.check_delete(self, self.provider.security.security_groups, sg)
         sgl = self.provider.security.security_groups.list()