Merge "Fix functional test for instance access operation"

trove/instance/service.py

@@ -495,6 +495,7 @@ class InstanceController(wsgi.Controller):
             if configuration_ref:
                 configuration_id = utils.get_id_from_href(configuration_ref)
                 return configuration_id
+            return ""
 
     def _modify_instance(self, context, req, instance, **kwargs):
         if 'detach_replica' in kwargs and kwargs['detach_replica']:
@@ -502,8 +503,6 @@ class InstanceController(wsgi.Controller):
                 context, request=req)
             with StartNotification(context, instance_id=instance.id):
                 instance.detach_replica()
-
-            instance.update_db(**kwargs)
         elif 'configuration_id' in kwargs:
             if kwargs['configuration_id']:
                 context.notification = (
@@ -519,8 +518,6 @@ class InstanceController(wsgi.Controller):
                                                                   request=req))
                 with StartNotification(context, instance_id=instance.id):
                     instance.detach_configuration()
-
-            instance.update_db(**kwargs)
         elif 'datastore_version' in kwargs:
             datastore_version = ds_models.DatastoreVersion.load(
                 instance.datastore, kwargs['datastore_version'])
@@ -529,8 +526,6 @@ class InstanceController(wsgi.Controller):
             with StartNotification(context, instance_id=instance.id,
                                    datastore_version_id=datastore_version.id):
                 instance.upgrade(datastore_version)
-
-            instance.update_db(**kwargs)
         elif 'access' in kwargs:
             instance.update_access(kwargs['access'])
 
@@ -538,6 +533,7 @@ class InstanceController(wsgi.Controller):
         """Updates the instance.
 
         - attach/detach configuration.
+        - detach from primary.
         - access information.
         """
         LOG.info("Updating database instance '%(instance_id)s' for tenant "
@@ -548,8 +544,8 @@ class InstanceController(wsgi.Controller):
         context = req.environ[wsgi.CONTEXT_KEY]
 
         name = body['instance'].get('name')
-        if ((name and len(body['instance']) != 2) or
+        if ((name and len(body['instance'].keys()) > 2) or
-                (not name and len(body['instance']) == 2)):
+            (not name and len(body['instance'].keys()) >= 2)):
             raise exception.BadRequest("Only one attribute (except 'name') is "
                                        "allowed to update.")
 
@@ -566,7 +562,7 @@ class InstanceController(wsgi.Controller):
             args['detach_replica'] = detach_replica
 
         configuration_id = self._configuration_parse(context, body)
-        if configuration_id:
+        if configuration_id is not None:
             args['configuration_id'] = configuration_id
 
         if 'access' in body['instance']:
@@ -576,31 +572,11 @@ class InstanceController(wsgi.Controller):
         return wsgi.Result(None, 202)
 
     def edit(self, req, id, body, tenant_id):
+        """Updates the instance to set or unset one or more attributes.
+
+        Deprecated. Use update method instead.
         """
-        Updates the instance to set or unset one or more attributes.
+        self.update(req, id, body, tenant_id)
-        """
-        LOG.info("Editing instance for tenant id %s.", tenant_id)
-        LOG.debug("req: %s", strutils.mask_password(req))
-        LOG.debug("body: %s", strutils.mask_password(body))
-        context = req.environ[wsgi.CONTEXT_KEY]
-
-        instance = models.Instance.load(context, id)
-        self.authorize_instance_action(context, 'edit', instance)
-
-        args = {}
-        args['detach_replica'] = ('replica_of' in body['instance'] or
-                                  'slave_of' in body['instance'])
-
-        if 'name' in body['instance']:
-            args['name'] = body['instance']['name']
-        if 'configuration' in body['instance']:
-            args['configuration_id'] = self._configuration_parse(context, body)
-        if 'datastore_version' in body['instance']:
-            args['datastore_version'] = body['instance'].get(
-                'datastore_version')
-
-        self._modify_instance(context, req, instance, **args)
-        return wsgi.Result(None, 202)
 
     def configuration(self, req, tenant_id, id):
         """

trove/tests/api/configurations.py

@@ -686,14 +686,13 @@ class DeleteConfigurations(ConfigurationsTestBase):
     @time_out(30)
     def test_unassign_configuration_from_instances(self):
         """test to unassign configuration from instance"""
-        instance_info.dbaas.instances.modify(configuration_instance.id,
+        instance_info.dbaas.instances.update(configuration_instance.id,
-                                             configuration="")
+                                             remove_configuration=True)
         resp, body = instance_info.dbaas.client.last_response
         assert_equal(resp.status, 202)
-        instance_info.dbaas.instances.get(configuration_instance.id)
+
-        # test that config group is not removed
+        instance_info.dbaas.instances.update(instance_info.id,
-        instance_info.dbaas.instances.modify(instance_info.id,
+                                             remove_configuration=True)
-                                             configuration=None)
         resp, body = instance_info.dbaas.client.last_response
         assert_equal(resp.status, 202)
         instance_info.dbaas.instances.get(instance_info.id)
@@ -704,10 +703,12 @@ class DeleteConfigurations(ConfigurationsTestBase):
                 return False
             else:
                 return True
+
         inst_info = instance_info
         poll_until(result_has_no_configuration)
         inst_info = configuration_instance
         poll_until(result_has_no_configuration)
+
         instance = instance_info.dbaas.instances.get(instance_info.id)
         assert_equal('RESTART_REQUIRED', instance.status)
 
@@ -780,17 +781,17 @@ class DeleteConfigurations(ConfigurationsTestBase):
         report.log("instance new name:%s" % new_name)
         saved_name = instance_info.name
         config_id = configuration_info.id
-        instance_info.dbaas.instances.edit(instance_info.id,
+        instance_info.dbaas.instances.update(instance_info.id,
-                                           configuration=config_id,
+                                             configuration=config_id,
-                                           name=new_name)
+                                             name=new_name)
         assert_equal(202, instance_info.dbaas.last_http_code)
         check = instance_info.dbaas.instances.get(instance_info.id)
         assert_equal(200, instance_info.dbaas.last_http_code)
         assert_equal(check.name, new_name)
 
         # restore instance name
-        instance_info.dbaas.instances.edit(instance_info.id,
+        instance_info.dbaas.instances.update(instance_info.id,
-                                           name=saved_name)
+                                             name=saved_name)
         assert_equal(202, instance_info.dbaas.last_http_code)
 
         instance = instance_info.dbaas.instances.get(instance_info.id)
@@ -813,14 +814,14 @@ class DeleteConfigurations(ConfigurationsTestBase):
         # already has an assigned configuration with patch
         config_id = configuration_info.id
         assert_raises(exceptions.BadRequest,
-                      instance_info.dbaas.instances.edit,
+                      instance_info.dbaas.instances.update,
                       instance_info.id, configuration=config_id)
 
     @test(runs_after=[test_assign_config_and_name_to_instance_using_patch])
     def test_unassign_configuration_after_patch(self):
         """Remove the configuration from the instance"""
-        instance_info.dbaas.instances.edit(instance_info.id,
+        instance_info.dbaas.instances.update(instance_info.id,
-                                           remove_configuration=True)
+                                             remove_configuration=True)
         assert_equal(202, instance_info.dbaas.last_http_code)
         instance = instance_info.dbaas.instances.get(instance_info.id)
         assert_equal('RESTART_REQUIRED', instance.status)
@@ -847,8 +848,8 @@ class DeleteConfigurations(ConfigurationsTestBase):
         # test unassign config group from an invalid instance
         invalid_id = "invalid-inst-id"
         try:
-            instance_info.dbaas.instances.edit(invalid_id,
+            instance_info.dbaas.instances.update(invalid_id,
-                                               remove_configuration=True)
+                                                 remove_configuration=True)
         except exceptions.NotFound:
             resp, body = instance_info.dbaas.client.last_response
             assert_equal(resp.status, 404)

trove/tests/api/instances.py

@@ -790,7 +790,7 @@ class CreateInstance(object):
         # Check these attrs only are returned in create response
         allowed_attrs = ['created', 'flavor', 'addresses', 'id', 'links',
                          'name', 'status', 'updated', 'datastore', 'fault',
-                         'region', 'service_status_updated']
+                         'region', 'service_status_updated', 'access']
         if ROOT_ON_CREATE:
             allowed_attrs.append('password')
         if VOLUME_SUPPORT:
@@ -933,7 +933,7 @@ class TestGetInstances(object):
     def test_index_list(self):
         allowed_attrs = ['id', 'links', 'name', 'status', 'flavor',
                          'datastore', 'ip', 'hostname', 'replica_of',
-                         'region', 'addresses']
+                         'region', 'addresses', 'access']
         if VOLUME_SUPPORT:
             allowed_attrs.append('volume')
         instances = dbaas.instances.list()
@@ -955,7 +955,7 @@ class TestGetInstances(object):
         allowed_attrs = ['created', 'databases', 'flavor', 'hostname', 'id',
                          'links', 'name', 'status', 'updated', 'ip',
                          'datastore', 'fault', 'region',
-                         'service_status_updated', 'addresses']
+                         'service_status_updated', 'addresses', 'access']
         if VOLUME_SUPPORT:
             allowed_attrs.append('volume')
         instances = dbaas.instances.list(detailed=True)
@@ -975,7 +975,7 @@ class TestGetInstances(object):
         allowed_attrs = ['created', 'databases', 'flavor', 'hostname', 'id',
                          'links', 'name', 'status', 'updated', 'ip',
                          'datastore', 'fault', 'region',
-                         'service_status_updated', 'addresses']
+                         'service_status_updated', 'addresses', 'access']
         if VOLUME_SUPPORT:
             allowed_attrs.append('volume')
         else:
@@ -1054,7 +1054,8 @@ class TestGetInstances(object):
                          'flavor', 'guest_status', 'host', 'hostname', 'id',
                          'name', 'root_enabled_at', 'root_enabled_by',
                          'server_state_description', 'status', 'datastore',
-                         'updated', 'users', 'volume', 'fault', 'region']
+                         'updated', 'users', 'volume', 'fault', 'region',
+                         'access']
         with CheckInstance(result._info) as check:
             check.contains_allowed_attrs(
                 result._info, allowed_attrs,
@@ -1128,19 +1129,20 @@ class TestUpdateInstance(object):
     @test
     def test_update_name(self):
         new_name = 'new-name'
-        result = dbaas.instances.edit(instance_info.id, name=new_name)
+        result = dbaas.instances.update(instance_info.id, name=new_name)
         assert_equal(202, dbaas.last_http_code)
         result = dbaas.instances.get(instance_info.id)
         assert_equal(200, dbaas.last_http_code)
         assert_equal(new_name, result.name)
         # Restore instance name because other tests depend on it
-        dbaas.instances.edit(instance_info.id, name=instance_info.name)
+        dbaas.instances.update(instance_info.id, name=instance_info.name)
         assert_equal(202, dbaas.last_http_code)
 
     @test
     def test_update_name_to_invalid_instance(self):
         # test assigning to an instance that does not exist
         invalid_id = "invalid-inst-id"
-        assert_raises(exceptions.NotFound, instance_info.dbaas.instances.edit,
+        assert_raises(exceptions.NotFound,
+                      instance_info.dbaas.instances.update,
                       invalid_id, name='name')
         assert_equal(404, instance_info.dbaas.last_http_code)

trove/tests/api/replication.py

@@ -386,8 +386,8 @@ class DetachReplica(object):
         if CONFIG.fake_mode:
             raise SkipTest("Detach replica not supported in fake mode")
 
-        instance_info.dbaas.instances.edit(slave_instance.id,
+        instance_info.dbaas.instances.update(slave_instance.id,
-                                           detach_replica_source=True)
+                                             detach_replica_source=True)
         assert_equal(202, instance_info.dbaas.last_http_code)
 
         poll_until(slave_is_running(False))

trove/tests/scenario/runners/replication_runners.py

@@ -354,7 +354,7 @@ class ReplicationRunner(TestRunner):
     def assert_detach_replica(
             self, replica_id, expected_states, expected_http_code):
         client = self.auth_client
-        client.instances.edit(replica_id, detach_replica_source=True)
+        client.instances.update(replica_id, detach_replica_source=True)
         self.assert_client_code(client, expected_http_code)
         self.assert_instance_action(replica_id, expected_states)
 

trove/tests/unittests/instance/test_instance_controller.py

@@ -14,6 +14,8 @@
 #    under the License.
 #
 import copy
+from unittest import mock
+from unittest.mock import MagicMock
 from unittest.mock import Mock
 import uuid
 
@@ -23,6 +25,7 @@ from testtools.matchers import Is
 from testtools.testcase import skip
 
 from trove.common import apischema
+from trove.common import exception
 from trove.instance.service import InstanceController
 from trove.tests.unittests import trove_testtools
 
@@ -359,12 +362,61 @@ class TestInstanceController(trove_testtools.TestCase):
 
         self.assertEqual(1, instance.detach_configuration.call_count)
 
-    def test_modify_instance_with_access(self):
+    def test_update_api_invalid_field(self):
-        instance = self._setup_modify_instance_mocks()
+        body = {
-        args = {}
+            'instance': {
-        args['access'] = {'is_public': True}
+                'invalid': 'invalid'
+            }
+        }
+        schema = self.controller.get_schema('update', body)
+        validator = jsonschema.Draft4Validator(schema)
+        self.assertFalse(validator.is_valid(body))
 
-        self.controller._modify_instance(self.context, self.req,
+    @mock.patch('trove.instance.models.Instance.load')
-                                         instance, **args)
+    def test_update_name(self, load_mock):
+        body = {
+            'instance': {
+                'name': 'new_name'
+            }
+        }
+        ins_mock = MagicMock()
+        load_mock.return_value = ins_mock
 
-        instance.update_access.assert_called_once_with({'is_public': True})
+        self.controller.update(MagicMock(), 'fake_id', body, 'fake_tenant_id')
+
+        ins_mock.update_db.assert_called_once_with(name='new_name')
+
+    def test_update_multiple_operations(self):
+        body = {
+            'instance': {
+                'name': 'new_name',
+                'replica_of': None,
+                'configuration': 'fake_config_id'
+            }
+        }
+
+        self.assertRaises(
+            exception.BadRequest,
+            self.controller.update,
+            MagicMock(), 'fake_id', body, 'fake_tenant_id'
+        )
+
+    @mock.patch('trove.instance.models.Instance.load')
+    def test_update_name_and_access(self, load_mock):
+        body = {
+            'instance': {
+                'name': 'new_name',
+                'access': {
+                    'is_public': True,
+                    'allowed_cidrs': []
+                }
+            }
+        }
+        ins_mock = MagicMock()
+        load_mock.return_value = ins_mock
+
+        self.controller.update(MagicMock(), 'fake_id', body, 'fake_tenant_id')
+
+        ins_mock.update_db.assert_called_once_with(name='new_name')
+        ins_mock.update_access.assert_called_once_with(
+            body['instance']['access'])