From de2d94172bebc8d8771e2e67860d94559a86cece Mon Sep 17 00:00:00 2001 From: silvacarloss Date: Wed, 9 Oct 2019 14:25:57 +0000 Subject: [PATCH] Fix share network update erroneously returns success This patch fixes an issue while performing share network update. Manila was allowing the user to update a share network even if it did not contain a default subnet. Now, we throw an error if there is no default subnet to be updated. Also, adds an extra validation. Now, Manila do not allow the default share network subnet update if it is going to have only `neutron_net_id` or `neutron_subnet_id` after the update. Change-Id: Iba2761db53da68a1c497e2e5dd370c36a22ebbed Closes-Bug: #1846836 --- manila/api/v2/share_networks.py | 33 +++++++++++-- manila/tests/api/v2/test_share_networks.py | 46 +++++++++++++++++++ ...e-unexpected-success-eba8f40db392c467.yaml | 6 +++ 3 files changed, 81 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/bug-1846836-fix-share-network-update-unexpected-success-eba8f40db392c467.yaml diff --git a/manila/api/v2/share_networks.py b/manila/api/v2/share_networks.py index 11f954b150..6d351ff3c0 100644 --- a/manila/api/v2/share_networks.py +++ b/manila/api/v2/share_networks.py @@ -272,10 +272,35 @@ class ShareNetworkController(wsgi.Controller): 'neutron_subnet_id' in update_values): subnet = db_api.share_network_subnet_get_default_subnet( context, id) - if subnet: - db_api.share_network_subnet_update(context, - subnet['id'], - update_values) + if not subnet: + msg = _("The share network %(id)s does not have a " + "'default' subnet that serves all availability " + "zones, so subnet details " + "('neutron_net_id', 'neutron_subnet_id') cannot " + "be updated.") % {'id': id} + raise exc.HTTPBadRequest(explanation=msg) + + # NOTE(silvacarlose): If the default share network subnet have + # the fields neutron_net_id and neutron_subnet_id set as None, + # we need to make sure that in the update request the user is + # passing both parameter since a share network subnet must + # have both fields filled or empty. + subnet_neutron_net_and_subnet_id_are_empty = ( + subnet['neutron_net_id'] is None + and subnet['neutron_subnet_id'] is None) + update_values_without_neutron_net_or_subnet = ( + update_values.get('neutron_net_id') is None or + update_values.get('neutron_subnet_id') is None) + if (subnet_neutron_net_and_subnet_id_are_empty + and update_values_without_neutron_net_or_subnet): + msg = _( + "To update the share network %(id)s you need to " + "specify both 'neutron_net_id' and " + "'neutron_subnet_id'.") % {'id': id} + raise webob.exc.HTTPBadRequest(explanation=msg) + db_api.share_network_subnet_update(context, + subnet['id'], + update_values) share_network = db_api.share_network_update(context, id, update_values) diff --git a/manila/tests/api/v2/test_share_networks.py b/manila/tests/api/v2/test_share_networks.py index d7a068bd12..c1dce8f771 100644 --- a/manila/tests/api/v2/test_share_networks.py +++ b/manila/tests/api/v2/test_share_networks.py @@ -813,6 +813,9 @@ class ShareNetworkAPITest(test.TestCase): self.mock_object( self.controller, '_share_network_subnets_contain_share_servers', mock.Mock(return_value=False)) + self.mock_object(db_api, 'share_network_subnet_get_default_subnet', + mock.Mock(return_value=fake_share_network_subnet)) + self.mock_object(db_api, 'share_network_subnet_update') body = {share_networks.RESOURCE_NAME: {'neutron_subnet_id': 'new subnet'}} @@ -825,6 +828,49 @@ class ShareNetworkAPITest(test.TestCase): self.req, share_nw, body) + db_api.share_network_subnet_get_default_subnet.assert_called_once_with( + self.context, share_nw) + db_api.share_network_subnet_update.assert_called_once_with( + self.context, fake_share_network_subnet['id'], + body['share_network']) + + @ddt.data((webob_exc.HTTPBadRequest, fake_share_network_subnet, None, + 'new subnet'), + (webob_exc.HTTPBadRequest, None, 'neutron net', None)) + @ddt.unpack + def test_update_default_subnet_errors(self, exception_to_raise, + get_default_subnet_return, + neutron_net_id, neutron_subnet_id): + share_nw = 'fake network id' + self.mock_object(db_api, 'share_network_get', + mock.Mock(return_value=fake_share_network)) + self.mock_object( + self.controller, '_share_network_subnets_contain_share_servers', + mock.Mock(return_value=False)) + self.mock_object(db_api, 'share_network_subnet_get_default_subnet', + mock.Mock(return_value=get_default_subnet_return)) + + if get_default_subnet_return: + fake_subnet = copy.deepcopy(fake_share_network_subnet) + fake_subnet['neutron_net_id'] = None + fake_subnet['neutron_subnet_id'] = None + db_api.share_network_subnet_get_default_subnet.return_value = ( + fake_subnet) + body = { + share_networks.RESOURCE_NAME: { + 'neutron_net_id': neutron_net_id, + 'neutron_subnet_id': neutron_subnet_id + } + } + + self.assertRaises(exception_to_raise, + self.controller.update, + self.req, + share_nw, + body) + + db_api.share_network_subnet_get_default_subnet.assert_called_once_with( + self.context, share_nw) @ddt.data(*set(("1.0", "2.25", "2.26", api_version._MAX_API_VERSION))) def test_action_add_security_service(self, microversion): diff --git a/releasenotes/notes/bug-1846836-fix-share-network-update-unexpected-success-eba8f40db392c467.yaml b/releasenotes/notes/bug-1846836-fix-share-network-update-unexpected-success-eba8f40db392c467.yaml new file mode 100644 index 0000000000..cb8a769c89 --- /dev/null +++ b/releasenotes/notes/bug-1846836-fix-share-network-update-unexpected-success-eba8f40db392c467.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixed unexpected behavior when updating a share network's `neutron_net_id` + or `neutron_subnet_id`. Now, Manila does not allow updating a share + network that does not contain a default subnet.