Merge "Port set/unset SecGroup Bug Fix"

Author: Jenkins

openstackclient/network/v2/port.py

@@ -51,7 +51,6 @@ def _format_dns_assignment(dns_assignment):
     'extra_dhcp_opts': utils.format_list_of_dicts,
     'fixed_ips': utils.format_list_of_dicts,
     'security_group_ids': utils.format_list,
-    'security_groups': utils.format_list,
 }
 
 
@@ -64,7 +63,6 @@ def _get_columns(item):
         'binding:vnic_type': 'binding_vnic_type',
         'is_admin_state_up': 'admin_state_up',
         'is_port_security_enabled': 'port_security_enabled',
-        'security_group_ids': 'security_groups',
         'tenant_id': 'project_id',
     }
     return sdk_utils.get_osc_show_columns_for_sdk_resource(item, column_map)
@@ -349,7 +347,7 @@ def get_parser(self, prog_name):
             '--security-group',
             metavar='<security-group>',
             action='append',
-            dest='security_groups',
+            dest='security_group',
             help=_("Security group to associate with this port (name or ID) "
                    "(repeat option to set multiple security groups)")
         )
@@ -391,12 +389,13 @@ def take_action(self, parsed_args):
         _prepare_fixed_ips(self.app.client_manager, parsed_args)
         attrs = _get_attrs(self.app.client_manager, parsed_args)
 
-        if parsed_args.security_groups:
-            attrs['security_groups'] = [client.find_security_group(
-                                        sg, ignore_missing=False).id
-                                        for sg in parsed_args.security_groups]
-        if parsed_args.no_security_group:
-            attrs['security_groups'] = []
+        if parsed_args.security_group:
+            attrs['security_group_ids'] = [client.find_security_group(
+                                           sg, ignore_missing=False).id
+                                           for sg in
+                                           parsed_args.security_group]
+        elif parsed_args.no_security_group:
+            attrs['security_group_ids'] = []
         if parsed_args.allowed_address_pairs:
             attrs['allowed_address_pairs'] = (
                 _convert_address_pairs(parsed_args))
@@ -626,7 +625,7 @@ def get_parser(self, prog_name):
             '--security-group',
             metavar='<security-group>',
             action='append',
-            dest='security_groups',
+            dest='security_group',
             help=_("Security group to associate with this port (name or ID) "
                    "(repeat option to set multiple security groups)")
         )
@@ -694,17 +693,16 @@ def take_action(self, parsed_args):
                 attrs['fixed_ips'] += [ip for ip in obj.fixed_ips if ip]
         elif parsed_args.no_fixed_ip:
             attrs['fixed_ips'] = []
-        if parsed_args.security_groups and parsed_args.no_security_group:
-            attrs['security_groups'] = [client.find_security_group(sg,
-                                        ignore_missing=False).id
-                                        for sg in parsed_args.security_groups]
-        elif parsed_args.security_groups:
-            attrs['security_groups'] = obj.security_groups
-            for sg in parsed_args.security_groups:
-                sg_id = client.find_security_group(sg, ignore_missing=False).id
-                attrs['security_groups'].append(sg_id)
+
+        if parsed_args.security_group:
+            attrs['security_group_ids'] = [
+                client.find_security_group(sg, ignore_missing=False).id for
+                sg in parsed_args.security_group]
+            if not parsed_args.no_security_group:
+                attrs['security_group_ids'] += obj.security_group_ids
+
         elif parsed_args.no_security_group:
-            attrs['security_groups'] = []
+            attrs['security_group_ids'] = []
 
         if (parsed_args.allowed_address_pairs and
                 parsed_args.no_allowed_address_pair):
@@ -769,7 +767,7 @@ def get_parser(self, prog_name):
             '--security-group',
             metavar='<security-group>',
             action='append',
-            dest='security_groups',
+            dest='security_group_ids',
             help=_("Security group which should be removed this port (name "
                    "or ID) (repeat option to unset multiple security groups)")
         )
@@ -802,7 +800,7 @@ def take_action(self, parsed_args):
         # Unset* classes
         tmp_fixed_ips = copy.deepcopy(obj.fixed_ips)
         tmp_binding_profile = copy.deepcopy(obj.binding_profile)
-        tmp_secgroups = copy.deepcopy(obj.security_groups)
+        tmp_secgroups = copy.deepcopy(obj.security_group_ids)
         tmp_addr_pairs = copy.deepcopy(obj.allowed_address_pairs)
         _prepare_fixed_ips(self.app.client_manager, parsed_args)
         attrs = {}
@@ -822,16 +820,16 @@ def take_action(self, parsed_args):
                 msg = _("Port does not contain binding-profile %s") % key
                 raise exceptions.CommandError(msg)
             attrs['binding:profile'] = tmp_binding_profile
-        if parsed_args.security_groups:
+        if parsed_args.security_group_ids:
             try:
-                for sg in parsed_args.security_groups:
+                for sg in parsed_args.security_group_ids:
                     sg_id = client.find_security_group(
                         sg, ignore_missing=False).id
                     tmp_secgroups.remove(sg_id)
             except ValueError:
                 msg = _("Port does not contain security group %s") % sg
                 raise exceptions.CommandError(msg)
-            attrs['security_groups'] = tmp_secgroups
+            attrs['security_group_ids'] = tmp_secgroups
         if parsed_args.allowed_address_pairs:
             try:
                 for addr in _convert_address_pairs(parsed_args):

openstackclient/tests/functional/network/v2/test_port.py

@@ -20,6 +20,7 @@ class PortTests(base.TestCase):
     """Functional tests for port. """
     NAME = uuid.uuid4().hex
     NETWORK_NAME = uuid.uuid4().hex
+    SG_NAME = uuid.uuid4().hex
 
     @classmethod
     def setUpClass(cls):
@@ -124,13 +125,25 @@ def test_port_set(self):
         self.assertEqual('xyzpdq', json_output.get('description'))
         self.assertEqual('DOWN', json_output.get('admin_state_up'))
 
-        raw_output = self.openstack('port set ' + '--enable ' + self.NAME)
+        raw_output = self.openstack(
+            'port set ' + '--enable ' + self.NAME)
         self.assertOutput('', raw_output)
 
         json_output = json.loads(self.openstack(
             'port show -f json ' + self.NAME
         ))
+        sg_id = json_output.get('security_group_ids')
+
         self.assertEqual(self.NAME, json_output.get('name'))
         self.assertEqual('xyzpdq', json_output.get('description'))
         self.assertEqual('UP', json_output.get('admin_state_up'))
         self.assertIsNotNone(json_output.get('mac_address'))
+
+        raw_output = self.openstack(
+            'port unset --security-group ' + sg_id + ' ' + id1)
+        self.assertOutput('', raw_output)
+
+        json_output = json.loads(self.openstack(
+            'port show -f json ' + self.NAME
+        ))
+        self.assertEqual('', json_output.get('security_group_ids'))

openstackclient/tests/unit/network/v2/fakes.py

@@ -544,7 +544,7 @@ def create_one_port(attrs=None):
             'name': 'port-name-' + uuid.uuid4().hex,
             'network_id': 'network-id-' + uuid.uuid4().hex,
             'port_security_enabled': True,
-            'security_groups': [],
+            'security_group_ids': [],
             'status': 'ACTIVE',
             'tenant_id': 'project-id-' + uuid.uuid4().hex,
         }
@@ -564,7 +564,7 @@ def create_one_port(attrs=None):
         port.is_admin_state_up = port_attrs['admin_state_up']
         port.is_port_security_enabled = port_attrs['port_security_enabled']
         port.project_id = port_attrs['tenant_id']
-        port.security_group_ids = port_attrs['security_groups']
+        port.security_group_ids = port_attrs['security_group_ids']
 
         return port
 

openstackclient/tests/unit/network/v2/test_port.py

@@ -57,7 +57,7 @@ def _get_common_cols_data(self, fake_port):
             'network_id',
             'port_security_enabled',
             'project_id',
-            'security_groups',
+            'security_group_ids',
             'status',
         )
 
@@ -82,7 +82,7 @@ def _get_common_cols_data(self, fake_port):
             fake_port.network_id,
             fake_port.port_security_enabled,
             fake_port.project_id,
-            utils.format_list(fake_port.security_groups),
+            utils.format_list(fake_port.security_group_ids),
             fake_port.status,
         )
 
@@ -251,7 +251,7 @@ def test_create_with_security_group(self):
         verifylist = [
             ('network', self._port.network_id,),
             ('enable', True),
-            ('security_groups', [secgroup.id]),
+            ('security_group', [secgroup.id]),
             ('name', 'test-port'),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -261,7 +261,7 @@ def test_create_with_security_group(self):
         self.network.create_port.assert_called_once_with(**{
             'admin_state_up': True,
             'network_id': self._port.network_id,
-            'security_groups': [secgroup.id],
+            'security_group_ids': [secgroup.id],
             'name': 'test-port',
         })
 
@@ -309,7 +309,7 @@ def test_create_with_security_groups(self):
         verifylist = [
             ('network', self._port.network_id,),
             ('enable', True),
-            ('security_groups', [sg_1.id, sg_2.id]),
+            ('security_group', [sg_1.id, sg_2.id]),
             ('name', 'test-port'),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -319,7 +319,7 @@ def test_create_with_security_groups(self):
         self.network.create_port.assert_called_once_with(**{
             'admin_state_up': True,
             'network_id': self._port.network_id,
-            'security_groups': [sg_1.id, sg_2.id],
+            'security_group_ids': [sg_1.id, sg_2.id],
             'name': 'test-port',
         })
 
@@ -346,7 +346,7 @@ def test_create_with_no_security_groups(self):
         self.network.create_port.assert_called_once_with(**{
             'admin_state_up': True,
             'network_id': self._port.network_id,
-            'security_groups': [],
+            'security_group_ids': [],
             'name': 'test-port',
         })
 
@@ -590,7 +590,7 @@ class TestListPort(TestPort):
             prt.mac_address,
             utils.format_list_of_dicts(prt.fixed_ips),
             prt.status,
-            utils.format_list(prt.security_groups),
+            utils.format_list(prt.security_group_ids),
             prt.device_owner,
         ))
 
@@ -1111,15 +1111,15 @@ def test_set_security_group(self):
             self._port.name,
         ]
         verifylist = [
-            ('security_groups', [sg.id]),
+            ('security_group', [sg.id]),
             ('port', self._port.name),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         result = self.cmd.take_action(parsed_args)
 
         attrs = {
-            'security_groups': [sg.id],
+            'security_group_ids': [sg.id],
         }
         self.network.update_port.assert_called_once_with(self._port, **attrs)
         self.assertIsNone(result)
@@ -1130,21 +1130,21 @@ def test_append_security_group(self):
         sg_3 = network_fakes.FakeSecurityGroup.create_one_security_group()
         self.network.find_security_group = mock.Mock(side_effect=[sg_2, sg_3])
         _testport = network_fakes.FakePort.create_one_port(
-            {'security_groups': [sg_1.id]})
+            {'security_group_ids': [sg_1.id]})
         self.network.find_port = mock.Mock(return_value=_testport)
         arglist = [
             '--security-group', sg_2.id,
             '--security-group', sg_3.id,
             _testport.name,
         ]
         verifylist = [
-            ('security_groups', [sg_2.id, sg_3.id]),
+            ('security_group', [sg_2.id, sg_3.id]),
             ('port', _testport.name),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         result = self.cmd.take_action(parsed_args)
         attrs = {
-            'security_groups': [sg_1.id, sg_2.id, sg_3.id],
+            'security_group_ids': [sg_2.id, sg_3.id, sg_1.id],
         }
         self.network.update_port.assert_called_once_with(_testport, **attrs)
         self.assertIsNone(result)
@@ -1163,7 +1163,7 @@ def test_set_no_security_groups(self):
         result = self.cmd.take_action(parsed_args)
 
         attrs = {
-            'security_groups': [],
+            'security_group_ids': [],
         }
         self.network.update_port.assert_called_once_with(self._port, **attrs)
         self.assertIsNone(result)
@@ -1172,7 +1172,7 @@ def test_overwrite_security_group(self):
         sg1 = network_fakes.FakeSecurityGroup.create_one_security_group()
         sg2 = network_fakes.FakeSecurityGroup.create_one_security_group()
         _testport = network_fakes.FakePort.create_one_port(
-            {'security_groups': [sg1.id]})
+            {'security_group_ids': [sg1.id]})
         self.network.find_port = mock.Mock(return_value=_testport)
         self.network.find_security_group = mock.Mock(return_value=sg2)
         arglist = [
@@ -1181,13 +1181,13 @@ def test_overwrite_security_group(self):
             _testport.name,
         ]
         verifylist = [
-            ('security_groups', [sg2.id]),
+            ('security_group', [sg2.id]),
             ('no_security_group', True)
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         result = self.cmd.take_action(parsed_args)
         attrs = {
-            'security_groups': [sg2.id],
+            'security_group_ids': [sg2.id],
         }
         self.network.update_port.assert_called_once_with(_testport, **attrs)
         self.assertIsNone(result)
@@ -1434,22 +1434,22 @@ def test_unset_security_group(self):
         _fake_sg1 = network_fakes.FakeSecurityGroup.create_one_security_group()
         _fake_sg2 = network_fakes.FakeSecurityGroup.create_one_security_group()
         _fake_port = network_fakes.FakePort.create_one_port(
-            {'security_groups': [_fake_sg1.id, _fake_sg2.id]})
+            {'security_group_ids': [_fake_sg1.id, _fake_sg2.id]})
         self.network.find_port = mock.Mock(return_value=_fake_port)
         self.network.find_security_group = mock.Mock(return_value=_fake_sg2)
         arglist = [
             '--security-group', _fake_sg2.id,
             _fake_port.name,
         ]
         verifylist = [
-            ('security_groups', [_fake_sg2.id]),
+            ('security_group_ids', [_fake_sg2.id]),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         result = self.cmd.take_action(parsed_args)
 
         attrs = {
-            'security_groups': [_fake_sg1.id]
+            'security_group_ids': [_fake_sg1.id]
         }
         self.network.update_port.assert_called_once_with(
             _fake_port, **attrs)
@@ -1459,14 +1459,14 @@ def test_unset_port_security_group_not_existent(self):
         _fake_sg1 = network_fakes.FakeSecurityGroup.create_one_security_group()
         _fake_sg2 = network_fakes.FakeSecurityGroup.create_one_security_group()
         _fake_port = network_fakes.FakePort.create_one_port(
-            {'security_groups': [_fake_sg1.id]})
+            {'security_group_ids': [_fake_sg1.id]})
         self.network.find_security_group = mock.Mock(return_value=_fake_sg2)
         arglist = [
             '--security-group', _fake_sg2.id,
             _fake_port.name,
         ]
         verifylist = [
-            ('security_groups', [_fake_sg2.id]),
+            ('security_group_ids', [_fake_sg2.id]),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)

releasenotes/notes/bug-1656788-2f5bda2205bc0329.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixed the ``port set`` and ``port unset`` command failures
+    (AttributeError) when ``--security-group`` option is included.
+    [Bug `1656788 <https://bugs.launchpad.net/python-openstackclient/+bug/1656788>`_]