compute: Allow adding, removing multiple SGs

stephenfin · stephenfin · commit ece30e8f703f · 2024-07-18T12:41:51.000+01:00
We also ensure we call neutron rather than the deprecated nova proxy API
in the event that neutron is available.

Change-Id: I8315ea164fd3fa6c1d759f16677bfd6c24c4ef63
Signed-off-by: Stephen Finucane &lt;stephenfin@redhat.com&gt;
diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
@@ -670,7 +670,7 @@ def take_action(self, parsed_args):
 
 
 class AddServerSecurityGroup(command.Command):
-    _description = _("Add security group to server")
+    _description = _("Add security group(s) to server")
 
     def get_parser(self, prog_name):
         parser = super().get_parser(prog_name)
@@ -680,9 +680,13 @@ def get_parser(self, prog_name):
             help=_('Server (name or ID)'),
         )
         parser.add_argument(
-            'group',
-            metavar='<group>',
-            help=_('Security group to add (name or ID)'),
+            'security_groups',
+            metavar='<security-group>',
+            nargs='+',
+            help=_(
+                'Security group(s) to add to the server (name or ID) '
+                '(repeat option to add multiple groups)'
+            ),
         )
         return parser
 
@@ -694,14 +698,43 @@ def take_action(self, parsed_args):
         )
         if self.app.client_manager.is_network_endpoint_enabled():
             # the server handles both names and IDs for neutron SGs, so just
-            # pass things through
-            security_group = parsed_args.group
+            # pass things through if using neutron
+            security_groups = parsed_args.security_groups
         else:
-            # however, if using nova-network then it needs a name, not an ID
-            security_group = compute_v2.find_security_group(
-                compute_client, parsed_args.group
-            )['name']
-        compute_client.add_security_group_to_server(server, security_group)
+            # however, if using nova-network then it needs names, not IDs
+            security_groups = []
+            for security_group in parsed_args.security_groups:
+                security_groups.append(
+                    compute_v2.find_security_group(
+                        compute_client, security_group
+                    )['name']
+                )
+
+        errors = 0
+        for security_group in security_groups:
+            try:
+                compute_client.add_security_group_to_server(
+                    server, security_group
+                )
+            except sdk_exceptions.HttpException as e:
+                errors += 1
+                LOG.error(
+                    _(
+                        "Failed to add security group with name or ID "
+                        "'%(security_group)s' to server '%(server)s': %(e)s"
+                    ),
+                    {
+                        'security_group': security_group,
+                        'server': server.id,
+                        'e': e,
+                    },
+                )
+
+        if errors > 0:
+            msg = _(
+                "%(errors)d of %(total)d security groups were not added."
+            ) % {'errors': errors, 'total': len(security_groups)}
+            raise exceptions.CommandError(msg)
 
 
 class AddServerVolume(command.ShowOne):
@@ -1328,6 +1361,7 @@ def get_parser(self, prog_name):
             metavar='<security-group>',
             action='append',
             default=[],
+            dest='security_groups',
             help=_(
                 'Security group to assign to this server (name or ID) '
                 '(repeat option to set multiple groups)'
@@ -1948,21 +1982,22 @@ def _match_image(image_api, wanted_properties):
             # 'auto' to maintain legacy behavior if a nic wasn't specified.
             networks = 'auto'
 
-        # Check security group exist and convert ID to name
+        # Check security group(s) exist and convert ID to name
         security_groups = []
         if self.app.client_manager.is_network_endpoint_enabled():
             network_client = self.app.client_manager.network
-            for each_sg in parsed_args.security_group:
+            for security_group in parsed_args.security_groups:
                 sg = network_client.find_security_group(
-                    each_sg, ignore_missing=False
+                    security_group, ignore_missing=False
                 )
                 # Use security group ID to avoid multiple security group have
                 # same name in neutron networking backend
                 security_groups.append({'name': sg.id})
-        else:
-            # Handle nova-network case
-            for each_sg in parsed_args.security_group:
-                sg = compute_v2.find_security_group(compute_client, each_sg)
+        else:  # nova-network
+            for security_group in parsed_args.security_groups:
+                sg = compute_v2.find_security_group(
+                    compute_client, security_group
+                )
                 security_groups.append({'name': sg['name']})
 
         hints = {}
@@ -4016,9 +4051,13 @@ def get_parser(self, prog_name):
             help=_('Server (name or ID)'),
         )
         parser.add_argument(
-            'group',
-            metavar='<group>',
-            help=_('Security group to remove (name or ID)'),
+            'security_groups',
+            metavar='<security-group>',
+            nargs='+',
+            help=_(
+                'Security group(s) to remove from server (name or ID) '
+                '(repeat option to remove multiple groups)'
+            ),
         )
         return parser
 
@@ -4031,15 +4070,42 @@ def take_action(self, parsed_args):
         if self.app.client_manager.is_network_endpoint_enabled():
             # the server handles both names and IDs for neutron SGs, so just
             # pass things through
-            security_group = parsed_args.group
+            security_groups = parsed_args.security_groups
         else:
-            # however, if using nova-network then it needs a name, not an ID
-            security_group = compute_v2.find_security_group(
-                compute_client, parsed_args.group
-            )['name']
-        compute_client.remove_security_group_from_server(
-            server, security_group
-        )
+            # however, if using nova-network then it needs names, not IDs
+            security_groups = []
+            for security_group in parsed_args.security_groups:
+                security_groups.append(
+                    compute_v2.find_security_group(
+                        compute_client, security_group
+                    )['name']
+                )
+
+        errors = 0
+        for security_group in security_groups:
+            try:
+                compute_client.remove_security_group_from_server(
+                    server, security_group
+                )
+            except sdk_exceptions.HttpException as e:
+                errors += 1
+                LOG.error(
+                    _(
+                        "Failed to remove security group with name or ID "
+                        "'%(security_group)s' from server '%(server)s': %(e)s"
+                    ),
+                    {
+                        'security_group': security_group,
+                        'server': server.id,
+                        'e': e,
+                    },
+                )
+
+        if errors > 0:
+            msg = _(
+                "%(errors)d of %(total)d security groups were not removed."
+            ) % {'errors': errors, 'total': len(security_groups)}
+            raise exceptions.CommandError(msg)
 
 
 class RemoveServerVolume(command.Command):
diff --git a/openstackclient/tests/functional/common/test_help.py b/openstackclient/tests/functional/common/test_help.py
@@ -21,7 +21,7 @@ class HelpTests(base.TestCase):
     """Functional tests for openstackclient help output."""
 
     SERVER_COMMANDS = [
-        ('server add security group', 'Add security group to server'),
+        ('server add security group', 'Add security group(s) to server'),
         ('server add volume', 'Add volume to server'),
         ('server backup create', 'Create a server backup image'),
         ('server create', 'Create a new server'),
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -1166,7 +1166,7 @@ def test_server_add_security_group__nova_network(self):
         arglist = [self.server.id, 'fake_sg']
         verifylist = [
             ('server', self.server.id),
-            ('group', 'fake_sg'),
+            ('security_groups', ['fake_sg']),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -1197,7 +1197,7 @@ def test_server_add_security_group(self):
         arglist = [self.server.id, 'fake_sg']
         verifylist = [
             ('server', self.server.id),
-            ('group', 'fake_sg'),
+            ('security_groups', ['fake_sg']),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -1429,7 +1429,7 @@ def test_server_create_with_options(self):
             ('flavor', self.flavor.id),
             ('key_name', 'keyname'),
             ('properties', {'Beta': 'b'}),
-            ('security_group', [security_group.id]),
+            ('security_groups', [security_group.id]),
             ('hints', {'a': ['b', 'c']}),
             ('server_group', server_group.id),
             ('config_drive', True),
@@ -1499,7 +1499,7 @@ def test_server_create_with_not_exist_security_group(self):
             ('image', self.image.id),
             ('flavor', self.flavor.id),
             ('key_name', 'keyname'),
-            ('security_group', ['not_exist_sg']),
+            ('security_groups', ['not_exist_sg']),
             ('server_name', self.server.name),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -1525,7 +1525,7 @@ def test_server_create_with_security_group_in_nova_network(self):
         verifylist = [
             ('image', self.image.id),
             ('flavor', self.flavor.id),
-            ('security_group', [sg_name]),
+            ('security_groups', [sg_name]),
             ('server_name', self.server.name),
         ]
 
@@ -7416,7 +7416,7 @@ def test_server_remove_security_group__nova_network(self):
         arglist = [self.server.id, 'fake_sg']
         verifylist = [
             ('server', self.server.id),
-            ('group', 'fake_sg'),
+            ('security_groups', ['fake_sg']),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -7447,7 +7447,7 @@ def test_server_remove_security_group(self):
         arglist = [self.server.id, 'fake_sg']
         verifylist = [
             ('server', self.server.id),
-            ('group', 'fake_sg'),
+            ('security_groups', ['fake_sg']),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
diff --git a/releasenotes/notes/add-remove-multiple-security-groups-2c0b2d599124c9c9.yaml b/releasenotes/notes/add-remove-multiple-security-groups-2c0b2d599124c9c9.yaml
@@ -0,0 +1,5 @@
+---
+features:
+  - |
+    The ``server add security group`` and ``server remove security group``
+    commands now accept multiple security groups.
