From ea2dd8e141ef1cf446a42ee93992d8fb44f4ba05 Mon Sep 17 00:00:00 2001 From: Richard Theis Date: Tue, 1 Mar 2016 15:34:35 -0600 Subject: [PATCH] Refactor security group create to use SDK Refactored the 'os security group create' command to use the SDK when neutron is enabled, but continue to use the nova client when nova network is enabled. Added a release note for the change in security group rules output due to Network v2. The tenant_id column name was fixed to align with the 'os security group show' command. Change-Id: Ib29df42edcddcc73a123fff6a64743a6bfcb7fbf Partial-Bug: #1519511 Implements: blueprint neutron-client --- openstackclient/compute/v2/security_group.py | 32 ---- openstackclient/network/v2/security_group.py | 50 ++++++ .../tests/compute/v2/test_security_group.py | 127 -------------- .../tests/network/v2/test_security_group.py | 159 +++++++++++++++++- .../notes/bug-1519511-65b8901ae6ea2e63.yaml | 13 ++ setup.cfg | 2 +- 6 files changed, 217 insertions(+), 166 deletions(-) delete mode 100644 openstackclient/tests/compute/v2/test_security_group.py create mode 100644 releasenotes/notes/bug-1519511-65b8901ae6ea2e63.yaml diff --git a/openstackclient/compute/v2/security_group.py b/openstackclient/compute/v2/security_group.py index 042e5caf3d..734bd78e3a 100644 --- a/openstackclient/compute/v2/security_group.py +++ b/openstackclient/compute/v2/security_group.py @@ -56,38 +56,6 @@ def _xform_security_group_rule(sgroup): return info -class CreateSecurityGroup(command.ShowOne): - """Create a new security group""" - - def get_parser(self, prog_name): - parser = super(CreateSecurityGroup, self).get_parser(prog_name) - parser.add_argument( - "name", - metavar="", - help="New security group name", - ) - parser.add_argument( - "--description", - metavar="", - help="Security group description", - ) - return parser - - def take_action(self, parsed_args): - compute_client = self.app.client_manager.compute - - description = parsed_args.description or parsed_args.name - - data = compute_client.security_groups.create( - parsed_args.name, - description, - ) - - info = {} - info.update(data._info) - return zip(*sorted(six.iteritems(info))) - - class CreateSecurityGroupRule(command.ShowOne): """Create a new security group rule""" diff --git a/openstackclient/network/v2/security_group.py b/openstackclient/network/v2/security_group.py index 64717945f3..f8162477b8 100644 --- a/openstackclient/network/v2/security_group.py +++ b/openstackclient/network/v2/security_group.py @@ -91,6 +91,56 @@ def _get_columns(item): return tuple(display_columns), property_columns +class CreateSecurityGroup(common.NetworkAndComputeShowOne): + """Create a new security group""" + + def update_parser_common(self, parser): + parser.add_argument( + "name", + metavar="", + help="New security group name", + ) + parser.add_argument( + "--description", + metavar="", + help="Security group description", + ) + return parser + + def _get_description(self, parsed_args): + if parsed_args.description is not None: + return parsed_args.description + else: + return parsed_args.name + + def take_action_network(self, client, parsed_args): + attrs = {} + attrs['name'] = parsed_args.name + attrs['description'] = self._get_description(parsed_args) + obj = client.create_security_group(**attrs) + display_columns, property_columns = _get_columns(obj) + data = utils.get_item_properties( + obj, + property_columns, + formatters=_formatters_network + ) + return (display_columns, data) + + def take_action_compute(self, client, parsed_args): + description = self._get_description(parsed_args) + obj = client.security_groups.create( + parsed_args.name, + description, + ) + display_columns, property_columns = _get_columns(obj._info) + data = utils.get_dict_properties( + obj._info, + property_columns, + formatters=_formatters_compute + ) + return (display_columns, data) + + class DeleteSecurityGroup(common.NetworkAndComputeCommand): """Delete a security group""" diff --git a/openstackclient/tests/compute/v2/test_security_group.py b/openstackclient/tests/compute/v2/test_security_group.py deleted file mode 100644 index 01e27b8201..0000000000 --- a/openstackclient/tests/compute/v2/test_security_group.py +++ /dev/null @@ -1,127 +0,0 @@ -# Licensed under the Apache License, Version 2.0 (the "License"); you may -# not use this file except in compliance with the License. You may obtain -# a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -# License for the specific language governing permissions and limitations -# under the License. -# - -import copy - -from openstackclient.compute.v2 import security_group -from openstackclient.tests.compute.v2 import fakes as compute_fakes -from openstackclient.tests import fakes -from openstackclient.tests.identity.v2_0 import fakes as identity_fakes - - -security_group_id = '11' -security_group_name = 'wide-open' -security_group_description = 'nothing but net' - -SECURITY_GROUP = { - 'id': security_group_id, - 'name': security_group_name, - 'description': security_group_description, - 'tenant_id': identity_fakes.project_id, -} - - -class FakeSecurityGroupResource(fakes.FakeResource): - - def get_keys(self): - return {'property': 'value'} - - -class TestSecurityGroup(compute_fakes.TestComputev2): - - def setUp(self): - super(TestSecurityGroup, self).setUp() - - # Get a shortcut compute client security_groups mock - self.secgroups_mock = self.app.client_manager.compute.security_groups - self.secgroups_mock.reset_mock() - - # Get a shortcut identity client projects mock - self.projects_mock = self.app.client_manager.identity.projects - self.projects_mock.reset_mock() - - -class TestSecurityGroupCreate(TestSecurityGroup): - - columns = ( - 'description', - 'id', - 'name', - 'tenant_id', - ) - data = ( - security_group_description, - security_group_id, - security_group_name, - identity_fakes.project_id, - ) - - def setUp(self): - super(TestSecurityGroupCreate, self).setUp() - - self.secgroups_mock.create.return_value = FakeSecurityGroupResource( - None, - copy.deepcopy(SECURITY_GROUP), - loaded=True, - ) - - # Get the command object to test - self.cmd = security_group.CreateSecurityGroup(self.app, None) - - def test_security_group_create_no_options(self): - arglist = [ - security_group_name, - ] - verifylist = [ - ('name', security_group_name), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - # In base command class ShowOne in cliff, abstract method take_action() - # returns a two-part tuple with a tuple of column names and a tuple of - # data to be shown. - columns, data = self.cmd.take_action(parsed_args) - - # SecurityGroupManager.create(name, description) - self.secgroups_mock.create.assert_called_with( - security_group_name, - security_group_name, - ) - - self.assertEqual(self.columns, columns) - self.assertEqual(self.data, data) - - def test_security_group_create_description(self): - arglist = [ - security_group_name, - '--description', security_group_description, - ] - verifylist = [ - ('name', security_group_name), - ('description', security_group_description), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - # In base command class ShowOne in cliff, abstract method take_action() - # returns a two-part tuple with a tuple of column names and a tuple of - # data to be shown. - columns, data = self.cmd.take_action(parsed_args) - - # SecurityGroupManager.create(name, description) - self.secgroups_mock.create.assert_called_with( - security_group_name, - security_group_description, - ) - - self.assertEqual(self.columns, columns) - self.assertEqual(self.data, data) diff --git a/openstackclient/tests/network/v2/test_security_group.py b/openstackclient/tests/network/v2/test_security_group.py index a66b7b0061..2d43d87218 100644 --- a/openstackclient/tests/network/v2/test_security_group.py +++ b/openstackclient/tests/network/v2/test_security_group.py @@ -37,6 +37,153 @@ class TestSecurityGroupCompute(compute_fakes.TestComputev2): self.compute = self.app.client_manager.compute +class TestCreateSecurityGroupNetwork(TestSecurityGroupNetwork): + + # The security group to be created. + _security_group = \ + network_fakes.FakeSecurityGroup.create_one_security_group() + + columns = ( + 'description', + 'id', + 'name', + 'project_id', + 'rules', + ) + + data = ( + _security_group.description, + _security_group.id, + _security_group.name, + _security_group.project_id, + '', + ) + + def setUp(self): + super(TestCreateSecurityGroupNetwork, self).setUp() + + self.network.create_security_group = mock.Mock( + return_value=self._security_group) + + # Get the command object to test + self.cmd = security_group.CreateSecurityGroup(self.app, self.namespace) + + def test_create_no_options(self): + self.assertRaises(tests_utils.ParserException, + self.check_parser, self.cmd, [], []) + + def test_create_min_options(self): + arglist = [ + self._security_group.name, + ] + verifylist = [ + ('name', self._security_group.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + columns, data = self.cmd.take_action(parsed_args) + + self.network.create_security_group.assert_called_once_with(**{ + 'description': self._security_group.name, + 'name': self._security_group.name, + }) + self.assertEqual(tuple(self.columns), columns) + self.assertEqual(self.data, data) + + def test_create_all_options(self): + arglist = [ + '--description', self._security_group.description, + self._security_group.name, + ] + verifylist = [ + ('description', self._security_group.description), + ('name', self._security_group.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + columns, data = self.cmd.take_action(parsed_args) + + self.network.create_security_group.assert_called_once_with(**{ + 'description': self._security_group.description, + 'name': self._security_group.name, + }) + self.assertEqual(tuple(self.columns), columns) + self.assertEqual(self.data, data) + + +class TestCreateSecurityGroupCompute(TestSecurityGroupCompute): + + # The security group to be shown. + _security_group = \ + compute_fakes.FakeSecurityGroup.create_one_security_group() + + columns = ( + 'description', + 'id', + 'name', + 'project_id', + 'rules', + ) + + data = ( + _security_group.description, + _security_group.id, + _security_group.name, + _security_group.tenant_id, + '', + ) + + def setUp(self): + super(TestCreateSecurityGroupCompute, self).setUp() + + self.app.client_manager.network_endpoint_enabled = False + + self.compute.security_groups.create.return_value = self._security_group + + # Get the command object to test + self.cmd = security_group.CreateSecurityGroup(self.app, None) + + def test_create_no_options(self): + self.assertRaises(tests_utils.ParserException, + self.check_parser, self.cmd, [], []) + + def test_create_min_options(self): + arglist = [ + self._security_group.name, + ] + verifylist = [ + ('name', self._security_group.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + columns, data = self.cmd.take_action(parsed_args) + + self.compute.security_groups.create.assert_called_once_with( + self._security_group.name, + self._security_group.name) + self.assertEqual(self.columns, columns) + self.assertEqual(self.data, data) + + def test_create_all_options(self): + arglist = [ + '--description', self._security_group.description, + self._security_group.name, + ] + verifylist = [ + ('description', self._security_group.description), + ('name', self._security_group.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + columns, data = self.cmd.take_action(parsed_args) + + self.compute.security_groups.create.assert_called_once_with( + self._security_group.name, + self._security_group.description) + self.assertEqual(self.columns, columns) + self.assertEqual(self.data, data) + + class TestDeleteSecurityGroupNetwork(TestSecurityGroupNetwork): # The security group to be deleted. @@ -65,7 +212,7 @@ class TestDeleteSecurityGroupNetwork(TestSecurityGroupNetwork): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - self.network.delete_security_group.assert_called_with( + self.network.delete_security_group.assert_called_once_with( self._security_group) self.assertIsNone(result) @@ -100,7 +247,7 @@ class TestDeleteSecurityGroupCompute(TestSecurityGroupCompute): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - self.compute.security_groups.delete.assert_called_with( + self.compute.security_groups.delete.assert_called_once_with( self._security_group.id) self.assertIsNone(result) @@ -143,7 +290,7 @@ class TestListSecurityGroupNetwork(TestSecurityGroupNetwork): columns, data = self.cmd.take_action(parsed_args) - self.network.security_groups.assert_called_with() + self.network.security_groups.assert_called_once_with() self.assertEqual(self.expected_columns, columns) self.assertEqual(self.expected_data, tuple(data)) @@ -158,7 +305,7 @@ class TestListSecurityGroupNetwork(TestSecurityGroupNetwork): columns, data = self.cmd.take_action(parsed_args) - self.network.security_groups.assert_called_with() + self.network.security_groups.assert_called_once_with() self.assertEqual(self.expected_columns, columns) self.assertEqual(self.expected_data, tuple(data)) @@ -212,7 +359,7 @@ class TestListSecurityGroupCompute(TestSecurityGroupCompute): columns, data = self.cmd.take_action(parsed_args) kwargs = {'search_opts': {'all_tenants': False}} - self.compute.security_groups.list.assert_called_with(**kwargs) + self.compute.security_groups.list.assert_called_once_with(**kwargs) self.assertEqual(self.expected_columns, columns) self.assertEqual(self.expected_data, tuple(data)) @@ -228,7 +375,7 @@ class TestListSecurityGroupCompute(TestSecurityGroupCompute): columns, data = self.cmd.take_action(parsed_args) kwargs = {'search_opts': {'all_tenants': True}} - self.compute.security_groups.list.assert_called_with(**kwargs) + self.compute.security_groups.list.assert_called_once_with(**kwargs) self.assertEqual(self.expected_columns_all_projects, columns) self.assertEqual(self.expected_data_all_projects, tuple(data)) diff --git a/releasenotes/notes/bug-1519511-65b8901ae6ea2e63.yaml b/releasenotes/notes/bug-1519511-65b8901ae6ea2e63.yaml new file mode 100644 index 0000000000..843c6cafa7 --- /dev/null +++ b/releasenotes/notes/bug-1519511-65b8901ae6ea2e63.yaml @@ -0,0 +1,13 @@ +--- +features: + - The ``security group create`` command now uses Network v2 when + enabled which results in a more detailed output for network + security group rules. + [Bug `1519511 `_] +fixes: + - The ``security group create`` command now uses Network v2 when + enabled which allows the security group description to be created + with an empty value. In addition, the ``tenant_id`` field changed + to ``project_id`` to match the ``security group show`` command + output. + [Bug `1519511 `_] diff --git a/setup.cfg b/setup.cfg index 4809cd2283..88d2203d63 100644 --- a/setup.cfg +++ b/setup.cfg @@ -99,7 +99,6 @@ openstack.compute.v2 = keypair_list = openstackclient.compute.v2.keypair:ListKeypair keypair_show = openstackclient.compute.v2.keypair:ShowKeypair - security_group_create = openstackclient.compute.v2.security_group:CreateSecurityGroup security_group_rule_create = openstackclient.compute.v2.security_group:CreateSecurityGroupRule security_group_rule_list = openstackclient.compute.v2.security_group:ListSecurityGroupRule @@ -344,6 +343,7 @@ openstack.network.v2 = router_set = openstackclient.network.v2.router:SetRouter router_show = openstackclient.network.v2.router:ShowRouter + security_group_create = openstackclient.network.v2.security_group:CreateSecurityGroup security_group_delete = openstackclient.network.v2.security_group:DeleteSecurityGroup security_group_list = openstackclient.network.v2.security_group:ListSecurityGroup security_group_set = openstackclient.network.v2.security_group:SetSecurityGroup