Fix logic that determines a share exists before manage

The share "manage" API checks whether
an existing/known share is being imported by
matching the export path provided to existing
shares.

This lookup does not consider the
fact that shares may have multiple export
locations, because it relies on an old/deprecated
"export_location" property on shares which
was added to provide backwards compatibility
to the API that presented only one export
location per share.

Further, it's possible to get a
"ERROR: Invalid share: Share already exists"
exception even when no such share exists in the
database.

Fix the lookup by using the "export_location_path"
based lookup which is faster, since it performs
a meaningful join on the export locations table;
and remove the parameters "protocol"
and "share_type_id" - these things make no
difference when there's a duplicated export
location. We'll consider "host" as a lookup
parameter since we can't be sure that export
locations are unique in a deployment - but they
ought to be unique for a given host.

Closes-Bug: #1848608
Closes-Bug: #1893718
Change-Id: I1d1aef0c2b48764789b43b91b258def6464b389f
Co-Authored-By: Goutham Pacha Ravi <gouthampravi@gmail.com>
Signed-off-by: Goutham Pacha Ravi <gouthampravi@gmail.com>

manila/api/v1/share_manage.py

@@ -43,7 +43,7 @@ class ShareManageMixin(object):
 
         share = {
             'host': share_data['service_host'],
-            'export_location': share_data['export_path'],
+            'export_location_path': share_data['export_path'],
             'share_proto': share_data['protocol'].upper(),
             'share_type_id': share_data['share_type_id'],
             'display_name': name,
@@ -86,6 +86,15 @@ class ShareManageMixin(object):
                 msg = _("Required parameter %s is empty") % parameter
                 raise exc.HTTPUnprocessableEntity(explanation=msg)
 
+        if isinstance(data['export_path'], dict):
+            # the path may be inside this dictionary
+            try:
+                data['export_path'] = data['export_path']['path']
+            except KeyError:
+                msg = ("Export path must be a string, or a dictionary "
+                       "with a 'path' item")
+                raise exc.HTTPUnprocessableEntity(explanation=msg)
+
         if not share_utils.extract_host(data['service_host'], 'pool'):
             msg = _("service_host parameter should contain pool.")
             raise exc.HTTPBadRequest(explanation=msg)

manila/db/sqlalchemy/api.py

@@ -1549,6 +1549,19 @@ def share_instances_get_all(context, filters=None, session=None):
     if instance_ids:
         query = query.filter(models.ShareInstance.id.in_(instance_ids))
 
+    # TODO(gouthamr): This DB API method needs to be generalized for all
+    # share instance fields.
+    host = filters.get('host')
+    if host:
+        query = query.filter(
+            or_(models.ShareInstance.host == host,
+                models.ShareInstance.host.like("{0}#%".format(host)))
+        )
+    share_server_id = filters.get('share_server_id')
+    if share_server_id:
+        query = query.filter(
+            models.ShareInstance.share_server_id == share_server_id)
+
     # Returns list of share instances that satisfy filters.
     query = query.all()
     return query

manila/share/api.py

@@ -769,18 +769,28 @@ class API(base.Base):
         self.share_rpcapi.update_share_replica(context, share_replica)
 
     def manage(self, context, share_data, driver_options):
-        shares = self.get_all(context, {
+
+        # Check whether there's a share already with the provided options:
+        filters = {
+            'export_location_path': share_data['export_location_path'],
             'host': share_data['host'],
-            'export_location': share_data['export_location'],
-            'share_proto': share_data['share_proto'],
-            'share_type_id': share_data['share_type_id']
-        })
+        }
+        share_server_id = share_data.get('share_server_id')
+        if share_server_id:
+            filters['share_server_id'] = share_data['share_server_id']
+
+        already_managed = self.db.share_instances_get_all(context,
+                                                          filters=filters)
+
+        if already_managed:
+            LOG.error("Found an existing share with export location %s!",
+                      share_data['export_location_path'])
+            msg = _("A share already exists with the export path specified.")
+            raise exception.InvalidShare(reason=msg)
 
         share_type_id = share_data['share_type_id']
         share_type = share_types.get_share_type(context, share_type_id)
 
-        share_server_id = share_data.get('share_server_id')
-
         dhss = share_types.parse_boolean_extra_spec(
             'driver_handles_share_servers',
             share_type['extra_specs']['driver_handles_share_servers'])
@@ -820,18 +830,11 @@ class API(base.Base):
         share_data.update(
             self.get_share_attributes_from_share_type(share_type))
 
-        LOG.debug("Manage: Found shares %s.", len(shares))
-
-        export_location = share_data.pop('export_location')
-
-        if len(shares) == 0:
-            share = self.db.share_create(context, share_data)
-        else:
-            msg = _("Share already exists.")
-            raise exception.InvalidShare(reason=msg)
+        share = self.db.share_create(context, share_data)
 
+        export_location_path = share_data.pop('export_location_path')
         self.db.share_export_locations_update(context, share.instance['id'],
-                                              export_location)
+                                              export_location_path)
 
         request_spec = self._get_request_spec_dict(
             share, share_type, size=0, share_proto=share_data['share_proto'],

manila/tests/api/v1/test_share_manage.py

@@ -60,7 +60,9 @@ class ShareManageTest(test.TestCase):
 
     @ddt.data({},
               {'shares': {}},
-              {'share': get_fake_manage_body('', None, None)})
+              {'share': get_fake_manage_body('', None, None)},
+              {'share': get_fake_manage_body(
+                  export_path={'not_path': '/fake'})})
     def test_share_manage_invalid_body(self, body):
         self.assertRaises(webob.exc.HTTPUnprocessableEntity,
                           self.controller.create,
@@ -192,6 +194,8 @@ class ShareManageTest(test.TestCase):
         get_fake_manage_body(display_name='foo', display_description='bar'),
         get_fake_manage_body(display_name='foo', display_description='bar',
                              driver_options=dict(volume_id='quuz')),
+        get_fake_manage_body(display_name='foo', display_description='bar',
+                             export_path={'path': '/fake'}),
     )
     def test_share_manage(self, data):
         self._setup_manage_mocks()
@@ -200,7 +204,7 @@ class ShareManageTest(test.TestCase):
             share_api.API, 'manage', mock.Mock(return_value=return_share))
         share = {
             'host': data['share']['service_host'],
-            'export_location': data['share']['export_path'],
+            'export_location_path': data['share']['export_path'],
             'share_proto': data['share']['protocol'].upper(),
             'share_type_id': 'fake',
             'display_name': 'foo',
@@ -208,6 +212,10 @@ class ShareManageTest(test.TestCase):
         }
         data['share']['is_public'] = 'foo'
         driver_options = data['share'].get('driver_options', {})
+        if isinstance(share['export_location_path'], dict):
+            share['export_location_path'] = (
+                share['export_location_path']['path']
+            )
 
         actual_result = self.controller.create(self.request, data)
 
@@ -225,7 +233,7 @@ class ShareManageTest(test.TestCase):
             share_api.API, 'manage', mock.Mock(return_value=return_share))
         share = {
             'host': data['share']['service_host'],
-            'export_location': data['share']['export_path'],
+            'export_location_path': data['share']['export_path'],
             'share_proto': data['share']['protocol'].upper(),
             'share_type_id': 'fake',
             'display_name': 'foo',

manila/tests/api/v2/test_shares.py

@@ -2892,7 +2892,7 @@ class ShareManageTest(test.TestCase):
             mock.Mock(side_effect=lambda *args, **kwargs: args[1]))
         share = {
             'host': data['share']['service_host'],
-            'export_location': data['share']['export_path'],
+            'export_location_path': data['share']['export_path'],
             'share_proto': data['share']['protocol'].upper(),
             'share_type_id': 'fake',
             'display_name': 'foo',

manila/tests/share/test_api.py

@@ -1019,7 +1019,7 @@ class ShareAPITestCase(test.TestCase):
     def test_manage_new(self, replication_type, dhss, share_server_id):
         share_data = {
             'host': 'fake',
-            'export_location': 'fake',
+            'export_location_path': 'fake',
             'share_proto': 'fake',
             'share_type_id': 'fake',
         }
@@ -1063,7 +1063,8 @@ class ShareAPITestCase(test.TestCase):
                          mock.Mock(return_value=share_server))
         self.mock_object(db_api, 'share_network_subnet_get',
                          mock.Mock(return_value=fake_subnet))
-        self.mock_object(self.api, 'get_all', mock.Mock(return_value=[]))
+        self.mock_object(db_api, 'share_instances_get_all',
+                         mock.Mock(return_value=[]))
 
         self.api.manage(self.context, copy.deepcopy(share_data),
                         driver_options)
@@ -1090,13 +1091,18 @@ class ShareAPITestCase(test.TestCase):
         if dhss:
             share_data.update({
                 'share_network_id': fake_subnet['share_network_id']})
-        export_location = share_data.pop('export_location')
-        self.api.get_all.assert_called_once_with(self.context, mock.ANY)
+        export_location = share_data.pop('export_location_path')
+        filters = {'export_location_path': export_location,
+                   'host': share_data['host']
+                   }
+        if share_server_id:
+            filters['share_server_id'] = share_server_id
+        db_api.share_instances_get_all.assert_called_once_with(
+            self.context, filters=filters)
         db_api.share_create.assert_called_once_with(self.context, share_data)
         db_api.share_get.assert_called_once_with(self.context, share['id'])
         db_api.share_export_locations_update.assert_called_once_with(
-            self.context, share.instance['id'], export_location
-        )
+            self.context, share.instance['id'], export_location)
         self.scheduler_rpcapi.manage_share.assert_called_once_with(
             self.context, share['id'], driver_options, expected_request_spec)
         if dhss:
@@ -1114,7 +1120,7 @@ class ShareAPITestCase(test.TestCase):
                                             has_share_server_id):
         share_data = {
             'host': 'fake',
-            'export_location': 'fake',
+            'export_location_path': 'fake',
             'share_proto': 'fake',
             'share_type_id': 'fake',
         }
@@ -1137,7 +1143,8 @@ class ShareAPITestCase(test.TestCase):
 
         self.mock_object(share_types, 'get_share_type',
                          mock.Mock(return_value=fake_type))
-        self.mock_object(self.api, 'get_all', mock.Mock(return_value=[]))
+        self.mock_object(db_api, 'share_instances_get_all',
+                         mock.Mock(return_value=[]))
 
         self.assertRaises(exception_type,
                           self.api.manage,
@@ -1148,19 +1155,18 @@ class ShareAPITestCase(test.TestCase):
         share_types.get_share_type.assert_called_once_with(
             self.context, share_data['share_type_id']
         )
-        self.api.get_all.assert_called_once_with(
-            self.context, {
-                'host': share_data['host'],
-                'export_location': share_data['export_location'],
-                'share_proto': share_data['share_proto'],
-                'share_type_id': share_data['share_type_id']
-            }
-        )
+        filters = {'export_location_path': share_data['export_location_path'],
+                   'host': share_data['host']
+                   }
+        if has_share_server_id:
+            filters['share_server_id'] = 'fake'
+        db_api.share_instances_get_all.assert_called_once_with(
+            self.context, filters=filters)
 
     def test_manage_new_share_server_not_found(self):
         share_data = {
             'host': 'fake',
-            'export_location': 'fake',
+            'export_location_path': 'fake',
             'share_proto': 'fake',
             'share_type_id': 'fake',
             'share_server_id': 'fake'
@@ -1184,7 +1190,8 @@ class ShareAPITestCase(test.TestCase):
 
         self.mock_object(share_types, 'get_share_type',
                          mock.Mock(return_value=fake_type))
-        self.mock_object(self.api, 'get_all', mock.Mock(return_value=[]))
+        self.mock_object(db_api, 'share_instances_get_all',
+                         mock.Mock(return_value=[]))
 
         self.assertRaises(exception.InvalidInput,
                           self.api.manage,
@@ -1195,19 +1202,18 @@ class ShareAPITestCase(test.TestCase):
         share_types.get_share_type.assert_called_once_with(
             self.context, share_data['share_type_id']
         )
-        self.api.get_all.assert_called_once_with(
-            self.context, {
+        db_api.share_instances_get_all.assert_called_once_with(
+            self.context, filters={
+                'export_location_path': share_data['export_location_path'],
                 'host': share_data['host'],
-                'export_location': share_data['export_location'],
-                'share_proto': share_data['share_proto'],
-                'share_type_id': share_data['share_type_id']
+                'share_server_id': share_data['share_server_id']
             }
         )
 
     def test_manage_new_share_server_not_active(self):
         share_data = {
             'host': 'fake',
-            'export_location': 'fake',
+            'export_location_path': 'fake',
             'share_proto': 'fake',
             'share_type_id': 'fake',
             'share_server_id': 'fake'
@@ -1237,7 +1243,8 @@ class ShareAPITestCase(test.TestCase):
 
         self.mock_object(share_types, 'get_share_type',
                          mock.Mock(return_value=fake_type))
-        self.mock_object(self.api, 'get_all', mock.Mock(return_value=[]))
+        self.mock_object(db_api, 'share_instances_get_all',
+                         mock.Mock(return_value=[]))
         self.mock_object(db_api, 'share_server_get',
                          mock.Mock(return_value=share))
 
@@ -1250,12 +1257,11 @@ class ShareAPITestCase(test.TestCase):
         share_types.get_share_type.assert_called_once_with(
             self.context, share_data['share_type_id']
         )
-        self.api.get_all.assert_called_once_with(
-            self.context, {
+        db_api.share_instances_get_all.assert_called_once_with(
+            self.context, filters={
+                'export_location_path': share_data['export_location_path'],
                 'host': share_data['host'],
-                'export_location': share_data['export_location'],
-                'share_proto': share_data['share_proto'],
-                'share_type_id': share_data['share_type_id']
+                'share_server_id': share_data['share_server_id']
             }
         )
         db_api.share_server_get.assert_called_once_with(
@@ -1266,7 +1272,7 @@ class ShareAPITestCase(test.TestCase):
     def test_manage_duplicate(self, status):
         share_data = {
             'host': 'fake',
-            'export_location': 'fake',
+            'export_location_path': 'fake',
             'share_proto': 'fake',
             'share_type_id': 'fake',
         }
@@ -1279,9 +1285,9 @@ class ShareAPITestCase(test.TestCase):
                 'driver_handles_share_servers': False,
             },
         }
-        shares = [{'id': 'fake', 'status': status}]
-        self.mock_object(self.api, 'get_all',
-                         mock.Mock(return_value=shares))
+        already_managed = [{'id': 'fake', 'status': status}]
+        self.mock_object(db_api, 'share_instances_get_all',
+                         mock.Mock(return_value=already_managed))
         self.mock_object(share_types, 'get_share_type',
                          mock.Mock(return_value=fake_type))
         self.assertRaises(exception.InvalidShare, self.api.manage,

releasenotes/notes/bug-1848608-1893718-fix-manage-api-for-shares-with-multiple-export-locations-32ade25e9d82535b.yaml

@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    The `API to import shares into manila
+    <https://docs.openstack.org/api-ref/shared-file-system/#manage-share-since-api-v2-7>`_
+    could sometimes allow a share to be "managed" into manila multiple times
+    via different export paths. This API could also incorrectly
+    disallow a manage operation citing a new share in question was already
+    managed. Both issues have now been fixed. See `bug
+    #1848608 <https://launchpad.net/bugs/1848608>`_ and `bug #1893718
+    <https://launchpad.net/bugs/1893718>`_ for more details.