Merge "[Share Groups] Fix creation of share group types with wrong specs values"

manila/api/v2/share_group_types.py

@@ -131,6 +131,14 @@ class ShareGroupTypesController(wsgi.Controller):
         if name is None or name == "" or len(name) > 255:
             msg = _("Share group type name is not valid.")
             raise webob.exc.HTTPBadRequest(explanation=msg)
+        if not (specs is None or isinstance(specs,  dict)):
+            msg = _("Group specs can be either of 'None' or 'dict' types.")
+            raise webob.exc.HTTPBadRequest(explanation=msg)
+        if specs:
+            for element in list(specs.keys()) + list(specs.values()):
+                if not isinstance(element, six.string_types):
+                    msg = _("Group specs keys and values should be strings.")
+                    raise webob.exc.HTTPBadRequest(explanation=msg)
         try:
             share_group_types.create(
                 context, name, share_types, specs, is_public)

manila/tests/api/v2/test_share_group_types.py

@@ -298,11 +298,13 @@ class ShareGroupTypesAPITest(test.TestCase):
         mock_get.assert_called_once_with(mock.ANY, GROUP_TYPE_1['name'])
         self.assertEqual(expected_type, res_dict['share_group_type'])
 
-    def test_create_with_group_specs(self):
-        fake_group_specs = {'my_fake_group_spec': 'false'}
+    @ddt.data(
+        None, {'my_fake_group_spec': 'false'},
+    )
+    def test_create_with_group_specs(self, specs):
         fake_type = copy.deepcopy(GROUP_TYPE_1)
         fake_type['share_types'] = [{'share_type_id': SHARE_TYPE_ID}]
-        fake_type['group_specs'] = fake_group_specs
+        fake_type['group_specs'] = specs
         mock_create = self.mock_object(share_group_types, 'create')
         mock_get = self.mock_object(
             share_group_types, 'get_by_name',
@@ -311,24 +313,49 @@ class ShareGroupTypesAPITest(test.TestCase):
         fake_body = {'share_group_type': {
             'name': GROUP_TYPE_1['name'],
             'share_types': [SHARE_TYPE_ID],
-            'group_specs': fake_group_specs,
+            'group_specs': specs,
         }}
         expected_type = {
             'id': GROUP_TYPE_1['id'],
             'name': GROUP_TYPE_1['name'],
             'is_public': True,
-            'group_specs': fake_group_specs,
+            'group_specs': specs,
             'share_types': [SHARE_TYPE_ID],
         }
 
         res_dict = self.controller._create(req, fake_body)
 
         mock_create.assert_called_once_with(
-            mock.ANY, GROUP_TYPE_1['name'], [SHARE_TYPE_ID], fake_group_specs,
+            mock.ANY, GROUP_TYPE_1['name'], [SHARE_TYPE_ID], specs,
             True)
         mock_get.assert_called_once_with(mock.ANY, GROUP_TYPE_1['name'])
         self.assertEqual(expected_type, res_dict['share_group_type'])
 
+    @ddt.data(
+        'str', ['l', 'i', 's', 't'], set([1]), ('t', 'u', 'p', 'l', 'e'), 1,
+        {"foo": 1}, {1: "foo"}, {"foo": "bar", "quuz": []}
+    )
+    def test_create_with_wrong_group_specs(self, specs):
+        fake_type = copy.deepcopy(GROUP_TYPE_1)
+        fake_type['share_types'] = [{'share_type_id': SHARE_TYPE_ID}]
+        fake_type['group_specs'] = specs
+        mock_create = self.mock_object(share_group_types, 'create')
+        mock_get = self.mock_object(
+            share_group_types, 'get_by_name',
+            mock.Mock(return_value=fake_type))
+        req = fake_request('/v2/fake/share-group-types')
+        fake_body = {'share_group_type': {
+            'name': GROUP_TYPE_1['name'],
+            'share_types': [SHARE_TYPE_ID],
+            'group_specs': specs,
+        }}
+
+        self.assertRaises(
+            webob.exc.HTTPBadRequest, self.controller._create, req, fake_body)
+
+        self.assertEqual(0, mock_create.call_count)
+        self.assertEqual(0, mock_get.call_count)
+
     def test_create_private_share_group_type(self):
         fake_type = copy.deepcopy(GROUP_TYPE_1)
         fake_type['share_types'] = [{'share_type_id': SHARE_TYPE_ID}]

manila_tempest_tests/tests/api/admin/test_share_group_types_negative.py

@@ -69,6 +69,15 @@ class ShareGroupTypesAdminNegativeTest(base.BaseSharesMixedTest):
             self.create_share_group_type,
             "x" * 256, client=self.admin_shares_v2_client)
 
+    @tc.attr(base.TAG_NEGATIVE, base.TAG_API)
+    def test_create_share_group_type_with_wrong_value_for_group_specs(self):
+        self.assertRaises(
+            lib_exc.BadRequest,
+            self.admin_shares_v2_client.create_share_group_type,
+            name=data_utils.rand_name("tempest_manila"),
+            share_types=[self.share_type['share_type']['id']],
+            group_specs="expecting_error_code_400")
+
     @tc.attr(base.TAG_NEGATIVE, base.TAG_API)
     def test_get_share_group_type_using_nonexistent_id(self):
         self.assertRaises(

manila_tempest_tests/tests/api/base.py

@@ -595,7 +595,8 @@ class BaseSharesTest(test.BaseTestCase):
                                 group_specs=None, client=None,
                                 cleanup_in_class=True, **kwargs):
         client = client or cls.shares_v2_client
-        if group_specs is None:
+        if (group_specs is None and
+                CONF.share.capability_sg_consistent_snapshot_support):
             group_specs = {
                 'consistent_snapshot_support': (
                     CONF.share.capability_sg_consistent_snapshot_support),