Merge lp:~blake-rouse/maas/fix-1462507 into lp:maas/trunk

[Blake Rouse]

Fix endpoint for BlockDevice API. Add ability for a user to list all the block devices on a node and read a block device on a node. Include the type of blockdevice in the output.

[lp:~blake-rouse/maas/fix-1462507]

3983. By Blake Rouse on 2015-06-05

Add blockdevice read operation.

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

=== modified file 'src/maasserver/api/blockdevices.py'
--- src/maasserver/api/blockdevices.py	2015-06-01 13:00:47 +0000
+++ src/maasserver/api/blockdevices.py	2015-06-05 20:24:38 +0000
@@ -20,12 +20,18 @@
     OperationsHandler,
 )
 from maasserver.api.utils import get_mandatory_param
-from maasserver.models import BlockDevice
+from maasserver.enum import NODE_PERMISSION
+from maasserver.exceptions import MAASAPINotFound
+from maasserver.models import (
+    BlockDevice,
+    Node,
+)
 
 
 DISPLAYED_BLOCKDEVICE_FIELDS = (
     'id',
     'name',
+    'type',
     'path',
     'id_path',
     'size',
@@ -34,36 +40,70 @@
 )
 
 
+class BlockDevicesHandler(OperationsHandler):
+    """Manage block devices on a node."""
+    api_doc_section_name = "Block devices"
+    create = replace = update = delete = None
+    model = BlockDevice
+    fields = DISPLAYED_BLOCKDEVICE_FIELDS
+
+    def read(self, request, system_id):
+        """List all block devices belonging to node.
+
+        Returns 404 if the node is not found.
+        """
+        node = Node.nodes.get_node_or_404(
+            system_id, request.user, NODE_PERMISSION.VIEW)
+        return node.blockdevice_set.all()
+
+
 class BlockDeviceHandler(OperationsHandler):
-    """Manage a BlockDevice.
-
-    The device is identified by its database id.
-    """
-    api_doc_section_name = "BlockDevice"
-    create = replace = update = read = None
+    """Manage a block device on a node."""
+    api_doc_section_name = "Block device"
+    create = replace = update = None
     model = BlockDevice
     fields = DISPLAYED_BLOCKDEVICE_FIELDS
 
+    def read(self, request, system_id, device_id):
+        """Read block device on node.
+
+        Returns 404 if the node or block device is not found.
+        """
+        node = Node.nodes.get_node_or_404(
+            system_id, request.user, NODE_PERMISSION.VIEW)
+        device = get_object_or_404(BlockDevice, id=device_id)
+        if device.node != node:
+            raise MAASAPINotFound()
+        return device
+
     @admin_method
     @operation(idempotent=True)
-    def add_tag(self, request, device_id):
+    def add_tag(self, request, system_id, device_id):
         """Add a tag to a BlockDevice.
 
         :param tag: The tag being added.
         """
+        node = Node.nodes.get_node_or_404(
+            system_id, request.user, NODE_PERMISSION.ADMIN)
         device = get_object_or_404(BlockDevice, id=device_id)
+        if device.node != node:
+            raise MAASAPINotFound()
         device.add_tag(get_mandatory_param(request.GET, 'tag'))
         device.save()
         return device
 
     @admin_method
     @operation(idempotent=True)
-    def remove_tag(self, request, device_id):
+    def remove_tag(self, request, system_id, device_id):
         """Remove a tag from a BlockDevice.
 
         :param tag: The tag being removed.
         """
+        node = Node.nodes.get_node_or_404(
+            system_id, request.user, NODE_PERMISSION.ADMIN)
         device = get_object_or_404(BlockDevice, id=device_id)
+        if device.node != node:
+            raise MAASAPINotFound()
         device.remove_tag(get_mandatory_param(request.GET, 'tag'))
         device.save()
         return device

=== modified file 'src/maasserver/api/tests/test_blockdevice.py'
--- src/maasserver/api/tests/test_blockdevice.py	2015-06-01 13:00:47 +0000
+++ src/maasserver/api/tests/test_blockdevice.py	2015-06-05 20:24:38 +0000
@@ -23,16 +23,73 @@
 from maasserver.testing.orm import reload_object
 
 
-def get_blockdevice_uri(device):
+def get_blockdevices_uri(node):
+    """Return a Node's BlockDevice URI on the API."""
+    return reverse(
+        'blockdevices_handler', args=[node.system_id])
+
+
+def get_blockdevice_uri(device, node=None):
     """Return a BlockDevice's URI on the API."""
-    return reverse('blockdevice_handler', args=[device.id])
+    if node is None:
+        node = device.node
+    return reverse(
+        'blockdevice_handler', args=[node.system_id, device.id])
+
+
+class TestBlockDevices(APITestCase):
+
+    def test_read(self):
+        node = factory.make_Node()
+        devices = [
+            factory.make_PhysicalBlockDevice(node=node)
+            for _ in range(3)
+            ]
+        uri = get_blockdevices_uri(node)
+        response = self.client.get(uri)
+
+        # Ensure the response status is OK
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+
+        # Ensure all the device ids match.
+        expected_device_ids = [
+            device.id
+            for device in devices
+            ]
+        result_device_ids = [
+            device["id"]
+            for device in json.loads(response.content)
+            ]
+        self.assertItemsEqual(expected_device_ids, result_device_ids)
 
 
 class TestBlockDeviceAPI(APITestCase):
 
+    def test_read(self):
+        device = factory.make_PhysicalBlockDevice()
+        uri = get_blockdevice_uri(device)
+        response = self.client.get(uri)
+
+        # Ensure the response status is OK
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+
+        parsed_device = json.loads(response.content)
+        self.assertEquals(device.id, parsed_device["id"])
+        self.assertEquals(device.type, parsed_device["type"])
+
+    def test_add_tag_returns_403_for_non_admin(self):
+        device = factory.make_PhysicalBlockDevice()
+        uri = get_blockdevice_uri(device)
+        response = self.client.get(
+            uri, {'op': 'add_tag', 'tag': factory.make_name('tag')})
+
+        # Ensure the response status is FORBIDDEN
+        self.assertEqual(
+            httplib.FORBIDDEN, response.status_code, response.content)
+
     def test_add_tag_to_block_device(self):
         self.become_admin()
-        device = factory.make_BlockDevice()
+        device = factory.make_PhysicalBlockDevice()
         tag_to_be_added = factory.make_name('tag')
         uri = get_blockdevice_uri(device)
         response = self.client.get(
@@ -49,9 +106,31 @@
         parsed_device = json.loads(response.content)
         self.assertIn(tag_to_be_added, parsed_device['tags'])
 
+    def test_add_tag_returns_404_when_system_id_doesnt_match(self):
+        self.become_admin()
+        device = factory.make_PhysicalBlockDevice()
+        other_node = factory.make_Node()
+        uri = get_blockdevice_uri(device, node=other_node)
+        response = self.client.get(
+            uri, {'op': 'add_tag', 'tag': factory.make_name('tag')})
+
+        # Ensure the response status is NOT_FOUND.
+        self.assertEqual(
+            httplib.NOT_FOUND, response.status_code, response.content)
+
+    def test_remove_tag_returns_403_for_non_admin(self):
+        device = factory.make_PhysicalBlockDevice()
+        uri = get_blockdevice_uri(device)
+        response = self.client.get(
+            uri, {'op': 'remove_tag', 'tag': factory.make_name('tag')})
+
+        # Ensure the response status is FORBIDDEN
+        self.assertEqual(
+            httplib.FORBIDDEN, response.status_code, response.content)
+
     def test_remove_tag_from_block_device(self):
         self.become_admin()
-        device = factory.make_BlockDevice()
+        device = factory.make_PhysicalBlockDevice()
         tag_to_be_removed = device.tags[0]
         uri = get_blockdevice_uri(device)
         response = self.client.get(

=== modified file 'src/maasserver/models/blockdevice.py'
--- src/maasserver/models/blockdevice.py	2015-05-07 18:14:38 +0000
+++ src/maasserver/models/blockdevice.py	2015-06-05 20:24:38 +0000
@@ -84,6 +84,25 @@
     tags = ArrayField(
         dbtype="text", blank=True, null=False, default=[])
 
+    @property
+    def type(self):
+        # Circular imports, since PhysicalBlockDevice and VirtualBlockDevice
+        # extend from this calss.
+        from maasserver.models.physicalblockdevice import PhysicalBlockDevice
+        from maasserver.models.virtualblockdevice import VirtualBlockDevice
+        try:
+            self.physicalblockdevice
+            return "physical"
+        except PhysicalBlockDevice.DoesNotExist:
+            try:
+                self.virtualblockdevice
+                return "virtual"
+            except VirtualBlockDevice.DoesNotExist:
+                pass
+        raise ValueError(
+            "BlockDevice is not a subclass of "
+            "PhysicalBlockDevice or VirtualBlockDevice")
+
     def display_size(self, include_suffix=True):
         return human_readable_bytes(self.size, include_suffix=include_suffix)
 

=== modified file 'src/maasserver/models/tests/test_blockdevice.py'
--- src/maasserver/models/tests/test_blockdevice.py	2015-05-07 18:14:38 +0000
+++ src/maasserver/models/tests/test_blockdevice.py	2015-06-05 20:24:38 +0000
@@ -17,6 +17,7 @@
 from maasserver.models import BlockDevice
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from testtools import ExpectedException
 from testtools.matchers import Equals
 
 
@@ -87,6 +88,19 @@
 class TestBlockDevice(MAASServerTestCase):
     """Tests for the `BlockDevice` model."""
 
+    def test_type_physical(self):
+        block_device = factory.make_PhysicalBlockDevice()
+        self.assertEquals("physical", block_device.type)
+
+    def test_type_virtual(self):
+        block_device = factory.make_VirtualBlockDevice()
+        self.assertEquals("virtual", block_device.type)
+
+    def test_type_raise_ValueError(self):
+        block_device = factory.make_BlockDevice()
+        with ExpectedException(ValueError):
+            block_device.type
+
     def test_display_size(self):
         sizes = (
             (45, '45.0 bytes'),

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2015-06-01 13:00:47 +0000
+++ src/maasserver/urls_api.py	2015-06-05 20:24:38 +0000
@@ -20,7 +20,10 @@
 )
 from maasserver.api.account import AccountHandler
 from maasserver.api.auth import api_auth
-from maasserver.api.blockdevices import BlockDeviceHandler
+from maasserver.api.blockdevices import (
+    BlockDeviceHandler,
+    BlockDevicesHandler,
+)
 from maasserver.api.boot_images import BootImagesHandler
 from maasserver.api.boot_resources import (
     BootResourceFileUploadHandler,
@@ -133,6 +136,10 @@
 node_mac_handler = RestrictedResource(NodeMacHandler, authentication=api_auth)
 node_macs_handler = RestrictedResource(
     NodeMacsHandler, authentication=api_auth)
+blockdevices_handler = RestrictedResource(
+    BlockDevicesHandler, authentication=api_auth)
+blockdevice_handler = RestrictedResource(
+    BlockDeviceHandler, authentication=api_auth)
 nodegroup_handler = RestrictedResource(
     NodeGroupHandler, authentication=api_auth)
 nodegroups_handler = RestrictedResource(
@@ -184,8 +191,6 @@
     LicenseKeyHandler, authentication=api_auth)
 license_keys_handler = AdminRestrictedResource(
     LicenseKeysHandler, authentication=api_auth)
-blockdevice_handler = AdminRestrictedResource(
-    BlockDeviceHandler, authentication=api_auth)
 
 
 # API URLs accessible to anonymous users.
@@ -207,7 +212,10 @@
     url(
         r'^nodes/(?P<system_id>[^/]+)/macs/$', node_macs_handler,
         name='node_macs_handler'),
-
+    url(r'^nodes/(?P<system_id>[^/]+)/blockdevices/$',
+        blockdevices_handler, name='blockdevices_handler'),
+    url(r'^nodes/(?P<system_id>[^/]+)/blockdevices/(?P<device_id>[^/]+)/$',
+        blockdevice_handler, name='blockdevice_handler'),
     url(
         r'^nodes/(?P<system_id>[^/]+)/$', node_handler,
         name='node_handler'),
@@ -308,8 +316,6 @@
         'selections/(?P<id>[^/]+)/$',
         boot_source_selection_backward_handler,
         name='boot_source_selection_backward_handler'),
-    url(r'^blockdevice/(?P<device_id>[^/]+)/$',
-        blockdevice_handler, name='blockdevice_handler'),
 )