From f13a007dc9ef8fa8c058c5ef8a082fbad4b66edc Mon Sep 17 00:00:00 2001 From: OpenStack Release Bot Date: Wed, 11 Sep 2024 16:07:16 +0000 Subject: [PATCH 01/15] Update .gitreview for stable/2024.2 Change-Id: Ib0377b5c9ca8ec264a81173835580d6bb6c4b3e4 --- .gitreview | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitreview b/.gitreview index 4eee726db6..f099ca66b3 100644 --- a/.gitreview +++ b/.gitreview @@ -2,3 +2,4 @@ host=review.opendev.org port=29418 project=openstack/python-openstackclient.git +defaultbranch=stable/2024.2 From f74c88560b944ec4c00864dde6cbf608b235f8d4 Mon Sep 17 00:00:00 2001 From: OpenStack Release Bot Date: Wed, 11 Sep 2024 16:07:18 +0000 Subject: [PATCH 02/15] Update TOX_CONSTRAINTS_FILE for stable/2024.2 Update the URL to the upper-constraints file to point to the redirect rule on releases.openstack.org so that anyone working on this branch will switch to the correct upper-constraints list automatically when the requirements repository branches. Until the requirements repository has as stable/2024.2 branch, tests will continue to use the upper-constraints list on master. Change-Id: I23254d63d6f87a11e1b633ac7f7756f46625533d --- tox.ini | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tox.ini b/tox.ini index e7c84c5110..51495096aa 100644 --- a/tox.ini +++ b/tox.ini @@ -11,7 +11,7 @@ setenv = OS_STDERR_CAPTURE=1 OS_TEST_TIMEOUT=60 deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2024.2} -r{toxinidir}/test-requirements.txt -r{toxinidir}/requirements.txt commands = @@ -63,7 +63,7 @@ commands = description = Run specified command in a virtual environment with all dependencies installed. deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2024.2} -r{toxinidir}/requirements.txt -r{toxinidir}/doc/requirements.txt commands = @@ -93,7 +93,7 @@ commands = description = Build documentation in HTML format. deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2024.2} -r{toxinidir}/doc/requirements.txt commands = sphinx-build -a -E -W -d doc/build/doctrees -b html doc/source doc/build/html @@ -105,7 +105,7 @@ commands = description = Build release note documentation in HTML format. deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2024.2} -r{toxinidir}/doc/requirements.txt commands = sphinx-build -a -E -W -d releasenotes/build/doctrees -b html releasenotes/source releasenotes/build/html From fb958fa5d00c38f399268956473c1e6e3090f2cf Mon Sep 17 00:00:00 2001 From: "Dr. Jens Harbott" Date: Fri, 13 Sep 2024 12:39:52 +0200 Subject: [PATCH 03/15] evacuate: Fix password parameter name for SDK The parameter is called admin_password on the SDK side. Change-Id: I0cd86675a884e6c2cbd3a861b8e111f961f0f336 (cherry picked from commit 8932282952f85320f364d2bdc5efd0d213019299) --- openstackclient/compute/v2/server.py | 2 +- openstackclient/tests/unit/compute/v2/test_server.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 661453b65a..199b387558 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -3840,7 +3840,7 @@ def _show_progress(progress): kwargs = { 'host': parsed_args.host, - 'password': parsed_args.password, + 'admin_password': parsed_args.password, } if not sdk_utils.supports_microversion(compute_client, '2.14'): diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 434d2e61f7..f60262f448 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -6974,7 +6974,7 @@ def test_evacuate(self): evac_args = { 'host': None, 'on_shared_storage': False, - 'password': None, + 'admin_password': None, } self._test_evacuate(args, verify_args, evac_args) @@ -6991,7 +6991,7 @@ def test_evacuate_with_password(self): evac_args = { 'host': None, 'on_shared_storage': False, - 'password': 'password', + 'admin_password': 'password', } self._test_evacuate(args, verify_args, evac_args) @@ -7008,7 +7008,7 @@ def test_evacuate_with_host(self): ('server', self.server.id), ('host', 'target-host'), ] - evac_args = {'host': host, 'password': None} + evac_args = {'host': host, 'admin_password': None} self._test_evacuate(args, verify_args, evac_args) @@ -7041,7 +7041,7 @@ def test_evacuate_without_share_storage(self): evac_args = { 'host': None, 'on_shared_storage': True, - 'password': None, + 'admin_password': None, } self._test_evacuate(args, verify_args, evac_args) @@ -7072,7 +7072,7 @@ def test_evacuate_with_wait_ok(self, mock_wait_for_status): evac_args = { 'host': None, 'on_shared_storage': False, - 'password': None, + 'admin_password': None, } self._test_evacuate(args, verify_args, evac_args) mock_wait_for_status.assert_called_once_with( From 08c84454c05c78f67bd2a2a6e53c9d9d91357405 Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Wed, 18 Sep 2024 08:46:44 +0200 Subject: [PATCH 04/15] evacuate SDK actually uses admin_pass param Change I0cd86675a884e6c2cbd3a861b8e111f961f0f336 was incorrect, the SDK param name is admin_pass. Change-Id: Ibe22c3d7d7ba0f1a5178475143e35fee5cac2ca2 (cherry picked from commit 58d1b06fdcda8e147ee0de77b9f1c835b97030fd) --- openstackclient/compute/v2/server.py | 2 +- openstackclient/tests/unit/compute/v2/test_server.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 199b387558..ec4ec612d7 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -3840,7 +3840,7 @@ def _show_progress(progress): kwargs = { 'host': parsed_args.host, - 'admin_password': parsed_args.password, + 'admin_pass': parsed_args.password, } if not sdk_utils.supports_microversion(compute_client, '2.14'): diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index f60262f448..d33506d0e1 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -6974,7 +6974,7 @@ def test_evacuate(self): evac_args = { 'host': None, 'on_shared_storage': False, - 'admin_password': None, + 'admin_pass': None, } self._test_evacuate(args, verify_args, evac_args) @@ -6991,7 +6991,7 @@ def test_evacuate_with_password(self): evac_args = { 'host': None, 'on_shared_storage': False, - 'admin_password': 'password', + 'admin_pass': 'password', } self._test_evacuate(args, verify_args, evac_args) @@ -7008,7 +7008,7 @@ def test_evacuate_with_host(self): ('server', self.server.id), ('host', 'target-host'), ] - evac_args = {'host': host, 'admin_password': None} + evac_args = {'host': host, 'admin_pass': None} self._test_evacuate(args, verify_args, evac_args) @@ -7041,7 +7041,7 @@ def test_evacuate_without_share_storage(self): evac_args = { 'host': None, 'on_shared_storage': True, - 'admin_password': None, + 'admin_pass': None, } self._test_evacuate(args, verify_args, evac_args) @@ -7072,7 +7072,7 @@ def test_evacuate_with_wait_ok(self, mock_wait_for_status): evac_args = { 'host': None, 'on_shared_storage': False, - 'admin_password': None, + 'admin_pass': None, } self._test_evacuate(args, verify_args, evac_args) mock_wait_for_status.assert_called_once_with( From 54b4f45829ff49419e468eb5153dc5251562cb74 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 16 Sep 2024 12:43:04 +0100 Subject: [PATCH 05/15] identity: Don't pass unset options when creating user In change I06f3848812bce60c65909f1311f36b70eba427d4, we migrated the 'user *' commands from keystoneclient to SDK. One side effect of this is that we are no longer able to rely on keystoneclient's 'filter_none' helper method that filters out parameters that are set to None. As such, we now need to do this ourselves. Eventually, it would be nice if SDK provided such functionality itself. The same change also introduced a bug where the '--domain' argument was being used to lookup a project rather than the '--project-domain' argument. This is also corrected. Change-Id: I1204ca611a74d134c879467d6c2b73f16e043213 Signed-off-by: Stephen Finucane Closes-bug: #2080600 (cherry picked from commit 033793aa0e96e3b8c8e729ff8fa67a9a37029e55) --- openstackclient/identity/v3/user.py | 50 +++++++--- .../tests/unit/identity/v3/test_user.py | 94 +------------------ 2 files changed, 39 insertions(+), 105 deletions(-) diff --git a/openstackclient/identity/v3/user.py b/openstackclient/identity/v3/user.py index 38b9a0ab0c..545b1cda77 100644 --- a/openstackclient/identity/v3/user.py +++ b/openstackclient/identity/v3/user.py @@ -249,26 +249,44 @@ def get_parser(self, prog_name): def take_action(self, parsed_args): identity_client = self.app.client_manager.sdk_connection.identity + kwargs = {} + domain_id = None if parsed_args.domain: domain_id = identity_client.find_domain( - name_or_id=parsed_args.domain, + parsed_args.domain, ignore_missing=False, ).id + kwargs['domain_id'] = domain_id - project_id = None if parsed_args.project: - project_id = identity_client.find_project( - name_or_id=parsed_args.project, + project_domain_id = None + if parsed_args.project_domain: + project_domain_id = identity_client.find_domain( + parsed_args.project_domain, + ignore_missing=False, + ).id + kwargs['default_project_id'] = identity_client.find_project( + parsed_args.project, ignore_missing=False, - domain_id=domain_id, + domain_id=project_domain_id, ).id + if parsed_args.description: + kwargs['description'] = parsed_args.description + + if parsed_args.email: + kwargs['email'] = parsed_args.email + is_enabled = True if parsed_args.disable: is_enabled = False - if parsed_args.password_prompt: - parsed_args.password = utils.get_password(self.app.stdin) + + password = None + if parsed_args.password: + password = parsed_args.password + elif parsed_args.password_prompt: + password = utils.get_password(self.app.stdin) if not parsed_args.password: LOG.warning( @@ -278,24 +296,26 @@ def take_action(self, parsed_args): ) ) options = _get_options_for_user(identity_client, parsed_args) + if options: + kwargs['options'] = options try: user = identity_client.create_user( - default_project_id=project_id, - description=parsed_args.description, - domain_id=domain_id, - email=parsed_args.email, is_enabled=is_enabled, name=parsed_args.name, - password=parsed_args.password, - options=options, + password=password, + **kwargs, ) except sdk_exc.ConflictException: if parsed_args.or_show: + kwargs = {} + if domain_id: + kwargs['domain_id'] = domain_id + user = identity_client.find_user( - name_or_id=parsed_args.name, - domain_id=domain_id, + parsed_args.name, ignore_missing=False, + **kwargs, ) LOG.info(_('Returning existing user %s'), user.name) else: diff --git a/openstackclient/tests/unit/identity/v3/test_user.py b/openstackclient/tests/unit/identity/v3/test_user.py index 19c1a6dcc7..7f4c2497ec 100644 --- a/openstackclient/tests/unit/identity/v3/test_user.py +++ b/openstackclient/tests/unit/identity/v3/test_user.py @@ -91,11 +91,6 @@ def test_user_create_no_options(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, - 'options': {}, 'is_enabled': True, 'password': None, } @@ -127,11 +122,6 @@ def test_user_create_password(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, - 'options': {}, 'is_enabled': True, 'password': 'secret', } @@ -165,11 +155,6 @@ def test_user_create_password_prompt(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, - 'options': {}, 'is_enabled': True, 'password': 'abc123', } @@ -200,12 +185,8 @@ def test_user_create_email(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, 'email': 'barney@example.com', 'is_enabled': True, - 'options': {}, 'password': None, } self.identity_sdk_client.create_user.assert_called_with(**kwargs) @@ -236,11 +217,7 @@ def test_user_create_project(self): kwargs = { 'name': self.user.name, 'default_project_id': self.project.id, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, - 'options': {}, 'password': None, } self.identity_sdk_client.create_user.assert_called_with(**kwargs) @@ -284,14 +261,13 @@ def test_user_create_project_domain(self): kwargs = { 'name': self.user.name, 'default_project_id': self.project.id, - 'description': None, - 'domain_id': None, - 'email': None, - 'options': {}, 'is_enabled': True, 'password': None, } - self.identity_sdk_client.create_user.assert_called_with(**kwargs) + self.identity_sdk_client.create_user.assert_called_once_with(**kwargs) + self.identity_sdk_client.find_domain.assert_called_once_with( + self.project.domain_id, ignore_missing=False + ) self.assertEqual(self.columns, columns) datalist = ( @@ -328,11 +304,7 @@ def test_user_create_domain(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, 'domain_id': self.domain.id, - 'email': None, - 'options': {}, 'is_enabled': True, 'password': None, } @@ -361,11 +333,6 @@ def test_user_create_enable(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, - 'options': {}, 'is_enabled': True, 'password': None, } @@ -394,11 +361,6 @@ def test_user_create_disable(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, - 'options': {}, 'is_enabled': False, 'password': None, } @@ -428,10 +390,6 @@ def test_user_create_ignore_lockout_failure_attempts(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'ignore_lockout_failure_attempts': True}, 'password': None, @@ -462,10 +420,6 @@ def test_user_create_no_ignore_lockout_failure_attempts(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'ignore_lockout_failure_attempts': False}, 'password': None, @@ -496,10 +450,6 @@ def test_user_create_ignore_password_expiry(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'ignore_password_expiry': True}, 'password': None, @@ -530,10 +480,6 @@ def test_user_create_no_ignore_password_expiry(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'ignore_password_expiry': False}, 'password': None, @@ -564,10 +510,6 @@ def test_user_create_ignore_change_password_upon_first_use(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'ignore_change_password_upon_first_use': True}, 'password': None, @@ -598,10 +540,6 @@ def test_user_create_no_ignore_change_password_upon_first_use(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'ignore_change_password_upon_first_use': False}, 'password': None, @@ -632,10 +570,6 @@ def test_user_create_enables_lock_password(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'lock_password': True}, 'password': None, @@ -666,10 +600,6 @@ def test_user_create_disables_lock_password(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'lock_password': False}, 'password': None, @@ -700,10 +630,6 @@ def test_user_create_enable_multi_factor_auth(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'multi_factor_auth_enabled': True}, 'password': None, @@ -734,10 +660,6 @@ def test_user_create_disable_multi_factor_auth(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': {'multi_factor_auth_enabled': False}, 'password': None, @@ -774,10 +696,6 @@ def test_user_create_option_with_multi_factor_auth_rule(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': { 'multi_factor_auth_rules': [["password", "totp"], ["password"]] @@ -815,10 +733,6 @@ def test_user_create_with_multiple_options(self): # Set expected values kwargs = { 'name': self.user.name, - 'default_project_id': None, - 'description': None, - 'domain_id': None, - 'email': None, 'is_enabled': True, 'options': { 'ignore_password_expiry': True, From 25bc3bbc07e1dfb0961e30c3a4b0229b70b800d7 Mon Sep 17 00:00:00 2001 From: Alfredo Moralejo Date: Mon, 30 Sep 2024 12:08:47 +0200 Subject: [PATCH 06/15] identity: in `service set` command, don't pass the enable option when it is None Currently, it is passing None value which is not accepted by keystone parameters validation: BadRequestException: 400: Client Error for url: ... Invalid input for field 'enabled': None is not of type 'boolean' Failed validating 'type' in schema['properties']['enabled']: {'enum': [True, False, None], 'type': 'boolean'} On instance['enabled']: None Closes-Bug: #2083021 Change-Id: Ia8772560deb54e71672102157659d4eb22e6ad59 --- openstackclient/identity/v3/service.py | 3 ++- openstackclient/tests/unit/identity/v3/test_service.py | 3 --- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/openstackclient/identity/v3/service.py b/openstackclient/identity/v3/service.py index f04fa979d3..41faa25517 100644 --- a/openstackclient/identity/v3/service.py +++ b/openstackclient/identity/v3/service.py @@ -225,7 +225,8 @@ def take_action(self, parsed_args): kwargs['name'] = parsed_args.name if parsed_args.description: kwargs['description'] = parsed_args.description - kwargs['is_enabled'] = parsed_args.is_enabled + if parsed_args.is_enabled is not None: + kwargs['is_enabled'] = parsed_args.is_enabled identity_client.update_service(service.id, **kwargs) diff --git a/openstackclient/tests/unit/identity/v3/test_service.py b/openstackclient/tests/unit/identity/v3/test_service.py index 4c02d9891b..d6dd0287b1 100644 --- a/openstackclient/tests/unit/identity/v3/test_service.py +++ b/openstackclient/tests/unit/identity/v3/test_service.py @@ -311,7 +311,6 @@ def test_service_set_type(self): # Set expected values kwargs = { 'type': self.service.type, - 'is_enabled': None, } self.identity_sdk_client.update_service.assert_called_with( self.service.id, **kwargs @@ -338,7 +337,6 @@ def test_service_set_name(self): # Set expected values kwargs = { 'name': self.service.name, - 'is_enabled': None, } self.identity_sdk_client.update_service.assert_called_with( self.service.id, **kwargs @@ -365,7 +363,6 @@ def test_service_set_description(self): # Set expected values kwargs = { 'description': self.service.description, - 'is_enabled': None, } self.identity_sdk_client.update_service.assert_called_with( self.service.id, **kwargs From 834bd93bf5ff0065b4b921ebe2415c3288479c51 Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Wed, 2 Oct 2024 00:16:40 +0900 Subject: [PATCH 07/15] Always resolve domain id The --user-domain option and the --project-domain option may take id or name. In case name is given it should be translated to id. Closes-Bug: 2083390 Change-Id: Idf3f113a74452daabc80660574030cb9b24b1a15 --- openstackclient/identity/common.py | 3 +++ .../identity/v3/role_assignment.py | 27 ++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py index 4e5f083de2..5d012efea8 100644 --- a/openstackclient/identity/common.py +++ b/openstackclient/identity/common.py @@ -204,6 +204,7 @@ def find_group(identity_client, name_or_id, domain_name_or_id=None): identity_client.groups, name_or_id, groups.Group ) else: + domain_id = find_domain(identity_client, domain_id).id return _find_identity_resource( identity_client.groups, name_or_id, @@ -219,6 +220,7 @@ def find_project(identity_client, name_or_id, domain_name_or_id=None): identity_client.projects, name_or_id, projects.Project ) else: + domain_id = find_domain(identity_client, domain_id).id return _find_identity_resource( identity_client.projects, name_or_id, @@ -234,6 +236,7 @@ def find_user(identity_client, name_or_id, domain_name_or_id=None): identity_client.users, name_or_id, users.User ) else: + domain_id = find_domain(identity_client, domain_id).id return _find_identity_resource( identity_client.users, name_or_id, users.User, domain_id=domain_id ) diff --git a/openstackclient/identity/v3/role_assignment.py b/openstackclient/identity/v3/role_assignment.py index 9753297933..7d4dfcdb06 100644 --- a/openstackclient/identity/v3/role_assignment.py +++ b/openstackclient/identity/v3/role_assignment.py @@ -146,12 +146,19 @@ def take_action(self, parsed_args): domain_id=role_domain_id, ) + user_domain_id = None + if parsed_args.user_domain: + project_domain_id = _find_sdk_id( + identity_client.find_domain, + name_or_id=parsed_args.user_domain, + ) + user_id = None if parsed_args.user: user_id = _find_sdk_id( identity_client.find_user, name_or_id=parsed_args.user, - domain_id=parsed_args.user_domain, + domain_id=user_domain_id, ) elif parsed_args.authuser: if auth_ref: @@ -171,6 +178,13 @@ def take_action(self, parsed_args): name_or_id=parsed_args.domain, ) + project_domain_id = None + if parsed_args.project_domain: + project_domain_id = _find_sdk_id( + identity_client.find_domain, + name_or_id=parsed_args.project_domain, + ) + project_id = None if parsed_args.project: project_id = _find_sdk_id( @@ -178,7 +192,7 @@ def take_action(self, parsed_args): name_or_id=common._get_token_resource( identity_client, 'project', parsed_args.project ), - domain_id=parsed_args.project_domain, + domain_id=project_domain_id, ) elif parsed_args.authproject: if auth_ref: @@ -187,12 +201,19 @@ def take_action(self, parsed_args): name_or_id=auth_ref.project_id, ) + group_domain_id = None + if parsed_args.group_domain: + group_domain_id = _find_sdk_id( + identity_client.find_domain, + name_or_id=parsed_args.group_domain, + ) + group_id = None if parsed_args.group: group_id = _find_sdk_id( identity_client.find_group, name_or_id=parsed_args.group, - domain_id=parsed_args.group_domain, + domain_id=group_domain_id, ) include_names = True if parsed_args.names else None From 773b869323ffc294170ffbd51bc5b1284cd59fe4 Mon Sep 17 00:00:00 2001 From: Johannes Kulik Date: Tue, 13 Aug 2024 12:14:31 +0200 Subject: [PATCH 08/15] compute: Fix --host in server list for new openstacksdk With `openstacksdk` 3.2.0 the `host` attribute of an Instance got added to the `Server` class [0]. With that change, listing servers with the `host` attribute leads to a query-filter for `compute_host` as expected, but `openstacksdk` will also filter for the `host` attribute locally after the results are returned. Since `compute_host` being `OS-EXT-SRV-ATTR:host` is not the same as `host, this means no results are returned. Since we want to keep the old behaviour of filtering by `compute_host` i.e. the service host name, we need to switch to filter for `compute_host`. This is already supported in older versions of `openstacksdk`. [0] https://github.com/openstack/openstacksdk/commit/0f311ff3e2e57bf3659cef77e98551b6c0c7e3c9 Change-Id: I0cd32c5b7d6d4d21194f3efdcfb9b205dea6a91e Closes-bug: #2074200 (cherry picked from commit ffa683ab4edf44219b687af54e2ace5177dbc2f3) --- openstackclient/compute/v2/server.py | 2 +- openstackclient/tests/unit/compute/v2/test_server.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index ec4ec612d7..55aa6469ec 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -2648,7 +2648,7 @@ def take_action(self, parsed_args): 'status': parsed_args.status, 'flavor': flavor_id, 'image': image_id, - 'host': parsed_args.host, + 'compute_host': parsed_args.host, 'project_id': project_id, 'all_projects': parsed_args.all_projects, 'user_id': user_id, diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index d33506d0e1..0420bfc17f 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -4581,7 +4581,7 @@ def setUp(self): 'status': None, 'flavor': None, 'image': None, - 'host': None, + 'compute_host': None, 'project_id': None, 'all_projects': False, 'user_id': None, From c6946f4743533449b5b8a3db2b1a8fe099b842da Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 15 Oct 2024 17:41:50 +0100 Subject: [PATCH 09/15] clientmanager: Check for 'block-storage' service type This is a fun one driven by two separate changes. We recently started checking whether the volume service was available before setting quotas in order to allow us to use quota set for other services [1]. This merged a number of weeks ago and was included in 7.1.0. More recently, we modified DevStack to stop publishing a service catalog entry with a service type of 'volumev3', preferring instead to use the correct 'block-storage' service type. Taken separately, neither of these changes would have caused issues. Together, they mean our lookups for the volume service now fail and we can't set volume quotas. Fix things by checking for the block-storage service type also. A future change will raise a warning (later an error) if the volume service is not found and you're attempting to set a quota since this is clearly a mistake. Change-Id: Ibbeef52225e18757cd28d0fbfb14c1ca06975b60 Signed-off-by: Stephen Finucane Closes-bug: #2084580 (cherry picked from commit 7c6b47b451f30d1d3965358c515baae87955d7dc) --- openstackclient/common/clientmanager.py | 7 ++++++- releasenotes/notes/bug-2084580-cb1e8c47501e730c.yaml | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/bug-2084580-cb1e8c47501e730c.yaml diff --git a/openstackclient/common/clientmanager.py b/openstackclient/common/clientmanager.py index 2e990985b9..de7400f9a1 100644 --- a/openstackclient/common/clientmanager.py +++ b/openstackclient/common/clientmanager.py @@ -129,10 +129,15 @@ def is_compute_endpoint_enabled(self): # TODO(stephenfin): Drop volume_client argument in OSC 8.0 or later. def is_volume_endpoint_enabled(self, volume_client=None): """Check if volume endpoint is enabled""" + # We check against the service type and all aliases defined by the + # Service Types Authority + # https://service-types.openstack.org/service-types.json return ( - self.is_service_available('volume') is not False + self.is_service_available('block-storage') is not False + or self.is_service_available('volume') is not False or self.is_service_available('volumev3') is not False or self.is_service_available('volumev2') is not False + or self.is_service_available('block-store') is not False ) diff --git a/releasenotes/notes/bug-2084580-cb1e8c47501e730c.yaml b/releasenotes/notes/bug-2084580-cb1e8c47501e730c.yaml new file mode 100644 index 0000000000..5ac30b3733 --- /dev/null +++ b/releasenotes/notes/bug-2084580-cb1e8c47501e730c.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + The ``quota set`` and ``limits show`` commands will now check for the + ``block-storage`` and ``block-store`` service types along with ``volume``, + ``volumev2`` and ``volumev3``. + + [Bug `2084580 `_] From 6c9d0a55f30660262a761533b26af9519e4dbf23 Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Fri, 25 Oct 2024 18:27:48 +0900 Subject: [PATCH 10/15] Fix ignored --user-domain in role assignment list Fix the wrong value assignment which made the --user-domain option ignored. Unit tests are updated to verify usage of domain options to avoid further regressions. Also drop the redundant look up of domain id to avoid unnecessary API call. Closes-Bug: #2085604 Change-Id: I5112b8e831fb26eb6544615277f0d3fe4f15dc5a (cherry picked from commit 2e491191e56dba338e7604cbc0c45b7658d4ee7a) --- openstackclient/identity/common.py | 56 ++-- .../identity/v3/role_assignment.py | 2 +- .../unit/identity/v3/test_role_assignment.py | 261 ++++++++++++++++++ 3 files changed, 284 insertions(+), 35 deletions(-) diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py index 5d012efea8..12a7f1479d 100644 --- a/openstackclient/identity/common.py +++ b/openstackclient/identity/common.py @@ -184,13 +184,6 @@ def _get_token_resource(client, resource, parsed_name, parsed_domain=None): return parsed_name -def _get_domain_id_if_requested(identity_client, domain_name_or_id): - if not domain_name_or_id: - return None - domain = find_domain(identity_client, domain_name_or_id) - return domain.id - - def find_domain(identity_client, name_or_id): return _find_identity_resource( identity_client.domains, name_or_id, domains.Domain @@ -198,48 +191,43 @@ def find_domain(identity_client, name_or_id): def find_group(identity_client, name_or_id, domain_name_or_id=None): - domain_id = _get_domain_id_if_requested(identity_client, domain_name_or_id) - if not domain_id: + if domain_name_or_id is None: return _find_identity_resource( identity_client.groups, name_or_id, groups.Group ) - else: - domain_id = find_domain(identity_client, domain_id).id - return _find_identity_resource( - identity_client.groups, - name_or_id, - groups.Group, - domain_id=domain_id, - ) + + domain_id = find_domain(identity_client, domain_name_or_id).id + return _find_identity_resource( + identity_client.groups, + name_or_id, + groups.Group, + domain_id=domain_id, + ) def find_project(identity_client, name_or_id, domain_name_or_id=None): - domain_id = _get_domain_id_if_requested(identity_client, domain_name_or_id) - if not domain_id: + if domain_name_or_id is None: return _find_identity_resource( identity_client.projects, name_or_id, projects.Project ) - else: - domain_id = find_domain(identity_client, domain_id).id - return _find_identity_resource( - identity_client.projects, - name_or_id, - projects.Project, - domain_id=domain_id, - ) + domain_id = find_domain(identity_client, domain_name_or_id).id + return _find_identity_resource( + identity_client.projects, + name_or_id, + projects.Project, + domain_id=domain_id, + ) def find_user(identity_client, name_or_id, domain_name_or_id=None): - domain_id = _get_domain_id_if_requested(identity_client, domain_name_or_id) - if not domain_id: + if domain_name_or_id is None: return _find_identity_resource( identity_client.users, name_or_id, users.User ) - else: - domain_id = find_domain(identity_client, domain_id).id - return _find_identity_resource( - identity_client.users, name_or_id, users.User, domain_id=domain_id - ) + domain_id = find_domain(identity_client, domain_name_or_id).id + return _find_identity_resource( + identity_client.users, name_or_id, users.User, domain_id=domain_id + ) def _find_identity_resource( diff --git a/openstackclient/identity/v3/role_assignment.py b/openstackclient/identity/v3/role_assignment.py index 7d4dfcdb06..12e1527b67 100644 --- a/openstackclient/identity/v3/role_assignment.py +++ b/openstackclient/identity/v3/role_assignment.py @@ -148,7 +148,7 @@ def take_action(self, parsed_args): user_domain_id = None if parsed_args.user_domain: - project_domain_id = _find_sdk_id( + user_domain_id = _find_sdk_id( identity_client.find_domain, name_or_id=parsed_args.user_domain, ) diff --git a/openstackclient/tests/unit/identity/v3/test_role_assignment.py b/openstackclient/tests/unit/identity/v3/test_role_assignment.py index ec7c7da560..65dad0d775 100644 --- a/openstackclient/tests/unit/identity/v3/test_role_assignment.py +++ b/openstackclient/tests/unit/identity/v3/test_role_assignment.py @@ -165,10 +165,13 @@ def test_role_assignment_list_user(self): arglist = ['--user', self.user.name] verifylist = [ ('user', self.user.name), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -181,6 +184,79 @@ def test_role_assignment_list_user(self): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) + self.identity_sdk_client.find_user.assert_called_with( + name_or_id=self.user.name, ignore_missing=False, domain_id=None + ) + self.identity_sdk_client.role_assignments.assert_called_with( + role_id=None, + user_id=self.user.id, + group_id=None, + scope_project_id=None, + scope_domain_id=None, + scope_system=None, + effective=None, + include_names=None, + inherited_to=None, + ) + + self.assertEqual(self.columns, columns) + datalist = ( + ( + self.role.id, + self.user.id, + '', + '', + self.domain.id, + '', + False, + ), + ( + self.role.id, + self.user.id, + '', + self.project.id, + '', + '', + False, + ), + ) + self.assertEqual(datalist, tuple(data)) + + def test_role_assignment_list_user_with_domain(self): + self.identity_sdk_client.role_assignments.return_value = [ + self.assignment_with_domain_id_and_user_id, + self.assignment_with_project_id_and_user_id, + ] + + arglist = ['--user', self.user.name, '--user-domain', self.domain.name] + verifylist = [ + ('user', self.user.name), + ('user_domain', self.domain.name), + ('group', None), + ('group_domain', None), + ('system', None), + ('domain', None), + ('project', None), + ('role', None), + ('effective', None), + ('inherited', False), + ('names', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # In base command class Lister in cliff, abstract method take_action() + # returns a tuple containing the column names and an iterable + # containing the data to be listed. + columns, data = self.cmd.take_action(parsed_args) + + self.identity_sdk_client.find_domain.assert_called_with( + name_or_id=self.domain.name, ignore_missing=False + ) + self.identity_sdk_client.find_user.assert_called_with( + name_or_id=self.user.name, + ignore_missing=False, + domain_id=self.domain.id, + ) self.identity_sdk_client.role_assignments.assert_called_with( role_id=None, user_id=self.user.id, @@ -230,10 +306,13 @@ def test_role_assignment_list_user_not_found(self, find_mock): arglist = ['--user', self.user.id] verifylist = [ ('user', self.user.id), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -290,10 +369,13 @@ def test_role_assignment_list_group(self): arglist = ['--group', self.group.name] verifylist = [ ('user', None), + ('user_domain', None), ('group', self.group.name), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -306,6 +388,85 @@ def test_role_assignment_list_group(self): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) + self.identity_sdk_client.find_group.assert_called_with( + name_or_id=self.group.name, ignore_missing=False, domain_id=None + ) + self.identity_sdk_client.role_assignments.assert_called_with( + role_id=None, + user_id=None, + group_id=self.group.id, + scope_project_id=None, + scope_domain_id=None, + scope_system=None, + effective=None, + include_names=None, + inherited_to=None, + ) + + self.assertEqual(self.columns, columns) + datalist = ( + ( + self.role.id, + '', + self.group.id, + '', + self.domain.id, + '', + False, + ), + ( + self.role.id, + '', + self.group.id, + self.project.id, + '', + '', + False, + ), + ) + self.assertEqual(datalist, tuple(data)) + + def test_role_assignment_list_group_with_domain(self): + self.identity_sdk_client.role_assignments.return_value = [ + self.assignment_with_domain_id_and_group_id, + self.assignment_with_project_id_and_group_id, + ] + + arglist = [ + '--group', + self.group.name, + '--group-domain', + self.domain.name, + ] + verifylist = [ + ('user', None), + ('user_domain', None), + ('group', self.group.name), + ('group_domain', self.domain.name), + ('system', None), + ('domain', None), + ('project', None), + ('project_domain', None), + ('role', None), + ('effective', None), + ('inherited', False), + ('names', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # In base command class Lister in cliff, abstract method take_action() + # returns a tuple containing the column names and an iterable + # containing the data to be listed. + columns, data = self.cmd.take_action(parsed_args) + + self.identity_sdk_client.find_domain.assert_called_with( + name_or_id=self.domain.name, ignore_missing=False + ) + self.identity_sdk_client.find_group.assert_called_with( + name_or_id=self.group.name, + ignore_missing=False, + domain_id=self.domain.id, + ) self.identity_sdk_client.role_assignments.assert_called_with( role_id=None, user_id=None, @@ -350,10 +511,13 @@ def test_role_assignment_list_domain(self): arglist = ['--domain', self.domain.name] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', self.domain.name), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -410,10 +574,13 @@ def test_role_assignment_list_project(self): arglist = ['--project', self.project.name] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', self.project.name), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -426,6 +593,85 @@ def test_role_assignment_list_project(self): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) + self.identity_sdk_client.find_project.assert_called_with( + name_or_id=self.project.name, ignore_missing=False, domain_id=None + ) + self.identity_sdk_client.role_assignments.assert_called_with( + role_id=None, + user_id=None, + group_id=None, + scope_project_id=self.project.id, + scope_domain_id=None, + scope_system=None, + effective=None, + include_names=None, + inherited_to=None, + ) + + self.assertEqual(self.columns, columns) + datalist = ( + ( + self.role.id, + self.user.id, + '', + self.project.id, + '', + '', + False, + ), + ( + self.role.id, + '', + self.group.id, + self.project.id, + '', + '', + False, + ), + ) + self.assertEqual(datalist, tuple(data)) + + def test_role_assignment_list_project_with_domain(self): + self.identity_sdk_client.role_assignments.return_value = [ + self.assignment_with_project_id_and_user_id, + self.assignment_with_project_id_and_group_id, + ] + + arglist = [ + '--project', + self.project.name, + '--project-domain', + self.domain.name, + ] + verifylist = [ + ('user', None), + ('user_domain', None), + ('group', None), + ('group_domain', None), + ('system', None), + ('domain', None), + ('project', self.project.name), + ('project_domain', self.domain.name), + ('role', None), + ('effective', None), + ('inherited', False), + ('names', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # In base command class Lister in cliff, abstract method take_action() + # returns a tuple containing the column names and an iterable + # containing the data to be listed. + columns, data = self.cmd.take_action(parsed_args) + + self.identity_sdk_client.find_domain.assert_called_with( + name_or_id=self.domain.name, ignore_missing=False + ) + self.identity_sdk_client.find_project.assert_called_with( + name_or_id=self.project.name, + ignore_missing=False, + domain_id=self.domain.id, + ) self.identity_sdk_client.role_assignments.assert_called_with( role_id=None, user_id=None, @@ -476,10 +722,13 @@ def test_role_assignment_list_def_creds(self): ] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -529,10 +778,13 @@ def test_role_assignment_list_effective(self): arglist = ['--effective'] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', True), ('inherited', False), @@ -611,10 +863,13 @@ def test_role_assignment_list_inherited(self): arglist = ['--inherited'] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', True), @@ -707,10 +962,13 @@ def test_role_assignment_list_include_names(self): arglist = ['--names'] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -799,10 +1057,13 @@ def test_role_assignment_list_domain_role(self): ] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', role_2.name), ('effective', None), ('inherited', False), From d258cd03cf731fdc87eacc00f2826adb08e28592 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 5 Nov 2024 17:07:49 +0000 Subject: [PATCH 11/15] common: Use correct argument for volume limits The sooner we have type hints in SDK, the better /o\ Change-Id: Iaf9596aea02f683c280ae68504a14d43dbd6134a Closes-bug: #2077634 Signed-off-by: Stephen Finucane (cherry picked from commit e5ccf1eb1ca4c2efe946f6dc8967041ee82d4ec5) --- openstackclient/common/limits.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openstackclient/common/limits.py b/openstackclient/common/limits.py index cb2f331f65..ac613fd83b 100644 --- a/openstackclient/common/limits.py +++ b/openstackclient/common/limits.py @@ -130,7 +130,7 @@ def take_action(self, parsed_args): if self.app.client_manager.is_volume_endpoint_enabled(): volume_client = self.app.client_manager.sdk_connection.volume volume_limits = volume_client.get_limits( - project_id=project_id, + project=project_id, ) if parsed_args.is_absolute: From f42ade4305d974580ae6dc40db0e0d58ba5a6b93 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 5 Dec 2024 10:56:52 +0000 Subject: [PATCH 12/15] quota: Catch correct exception type for Compute quotas There is a flaw (IMO) in the design of Nova's os-quota-sets API: despite project IDs forming the identifier for an individual resource, we get a HTTP 400 (Bad Request) error if you pass an ID that does not exist, rather than the HTTP 404 (Not Found) we would expect. Correct this, noting why we're doing what we're doing for readers from the future (hi!). Note that HTTP 400 is unfortunately quite broad and means we'll also catch things like invalid requests but the exception may have been translated so we can't rely on a string match. Change-Id: I720502930d50be8ead5f2033d9dbcab5d99a37a9 Signed-off-by: Stephen Finucane Closes-bug: #2091086 (cherry picked from commit 99cef9354b70fe8c5a227dd1b3fa41908c290d0d) --- openstackclient/common/quota.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/openstackclient/common/quota.py b/openstackclient/common/quota.py index ec1f91a6cd..d75003a674 100644 --- a/openstackclient/common/quota.py +++ b/openstackclient/common/quota.py @@ -249,9 +249,14 @@ def _list_quota_compute(self, parsed_args, project_ids): for project_id in project_ids: try: project_data = compute_client.get_quota_set(project_id) + # NOTE(stephenfin): Unfortunately, Nova raises a HTTP 400 (Bad + # Request) if the project ID is invalid, even though the project + # ID is actually the resource's identifier which would normally + # lead us to expect a HTTP 404 (Not Found). except ( - sdk_exceptions.NotFoundException, + sdk_exceptions.BadRequestException, sdk_exceptions.ForbiddenException, + sdk_exceptions.NotFoundException, ) as exc: # Project not found, move on to next one LOG.warning(f"Project {project_id} not found: {exc}") @@ -312,8 +317,8 @@ def _list_quota_volume(self, parsed_args, project_ids): try: project_data = volume_client.get_quota_set(project_id) except ( - sdk_exceptions.NotFoundException, sdk_exceptions.ForbiddenException, + sdk_exceptions.NotFoundException, ) as exc: # Project not found, move on to next one LOG.warning(f"Project {project_id} not found: {exc}") From 8390467304cb553d23bc9d42ae1fdf59f441b28a Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 21 Mar 2025 15:03:41 +0000 Subject: [PATCH 13/15] zuul: Make image job non-voting We may need to remove this soon enough, given the new Docker rate limits that we keep bumping into. Change-Id: Id4a9d8df770d107986b20e4a98835ee4e0b6117d Signed-off-by: Stephen Finucane (cherry picked from commit 7ef588d6952cf4c90f39759e88fdba38007e5975) (cherry picked from commit 94fe341fa5b6eefdb60cdab56a14cb9cbd751d46) --- .zuul.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.zuul.yaml b/.zuul.yaml index 9c0f0b22ce..5538dff574 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -219,7 +219,8 @@ - release-notes-jobs-python3 check: jobs: - - osc-build-image + - osc-build-image: + voting: false - osc-functional-devstack - osc-functional-devstack-tips: # The functional-tips job only tests the latest and shouldn't be run From f9cc9013ae72795a291dcfda26f6488c449cd3ad Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 9 Dec 2024 13:32:15 +0000 Subject: [PATCH 14/15] compute: Workaround bug #2089821 By passing a dict instead of a single value, we force SDK to populate the correct attribute on the object. Also fixed conflicts in openstackclient/compute/v2/server.py added by ece30e8f703f918e391e934ecd2b201f211bfbfe Change-Id: I9f4c5964dc0546215474c92db567966ffad68a1a Signed-off-by: Stephen Finucane Related-bug: #2089821 (cherry picked from commit 22b30b99ce0e7b64aed1a13df73ab52a0b33204d) --- openstackclient/compute/v2/server.py | 8 ++++++-- openstackclient/tests/unit/compute/v2/test_server.py | 8 ++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 55aa6469ec..7abc5b92e3 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -701,7 +701,10 @@ def take_action(self, parsed_args): security_group = compute_v2.find_security_group( compute_client, parsed_args.group )['name'] - compute_client.add_security_group_to_server(server, security_group) + compute_client.add_security_group_to_server( + server, + {'name': security_group}, + ) class AddServerVolume(command.ShowOne): @@ -4039,7 +4042,8 @@ def take_action(self, parsed_args): compute_client, parsed_args.group )['name'] compute_client.remove_security_group_from_server( - server, security_group + server, + {'name': security_group}, ) diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 0420bfc17f..67171e2bd4 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -1186,7 +1186,7 @@ def test_server_add_security_group__nova_network(self): self.server.id, ignore_missing=False ) self.compute_sdk_client.add_security_group_to_server.assert_called_once_with( - self.server, 'fake_sg' + self.server, {'name': 'fake_sg'} ) mock_find_nova_net_sg.assert_called_once_with( self.compute_sdk_client, 'fake_sg' @@ -1207,7 +1207,7 @@ def test_server_add_security_group(self): self.server.id, ignore_missing=False ) self.compute_sdk_client.add_security_group_to_server.assert_called_once_with( - self.server, 'fake_sg' + self.server, {'name': 'fake_sg'} ) self.assertIsNone(result) @@ -7394,7 +7394,7 @@ def test_server_remove_security_group__nova_network(self): self.server.id, ignore_missing=False ) self.compute_sdk_client.remove_security_group_from_server.assert_called_once_with( - self.server, 'fake_sg' + self.server, {'name': 'fake_sg'} ) mock_find_nova_net_sg.assert_called_once_with( self.compute_sdk_client, 'fake_sg' @@ -7415,7 +7415,7 @@ def test_server_remove_security_group(self): self.server.id, ignore_missing=False ) self.compute_sdk_client.remove_security_group_from_server.assert_called_once_with( - self.server, 'fake_sg' + self.server, {'name': 'fake_sg'} ) self.assertIsNone(result) From 67f5509fcb696abcb70c13cb8f9546c61950764f Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 3 Dec 2024 15:32:26 +0000 Subject: [PATCH 15/15] tests: Add functional test for adding, removing SGs The fix is in openstacksdk. Let's test it here though. Change-Id: I661e6d66c8196e8c9ca8b9cda3d08e756e3d5877 Signed-off-by: Stephen Finucane Depends-on: https://review.opendev.org/c/openstack/openstacksdk/+/963945 Related-bug: #2089821 (cherry picked from commit e736394d1bd3d13fc6939e9b0e954d646c152777) --- .../functional/compute/v2/test_server.py | 88 ++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-) diff --git a/openstackclient/tests/functional/compute/v2/test_server.py b/openstackclient/tests/functional/compute/v2/test_server.py index d6638d2f71..b7aaf510ed 100644 --- a/openstackclient/tests/functional/compute/v2/test_server.py +++ b/openstackclient/tests/functional/compute/v2/test_server.py @@ -1295,7 +1295,7 @@ def test_server_add_remove_port(self): ) if ip_address in cmd_output['addresses']['private']: # Hang out for a bit and try again - print('retrying add port check') + print('retrying remove port check') wait_time += 10 time.sleep(10) else: @@ -1367,6 +1367,92 @@ def test_server_add_fixed_ip(self): addresses = cmd_output['addresses']['private'] self.assertIn(ip_address, addresses) + def test_server_add_remove_security_group(self): + name = uuid.uuid4().hex + cmd_output = self.openstack( + 'server create ' + + '--network private ' + + '--flavor ' + + self.flavor_name + + ' ' + + '--image ' + + self.image_name + + ' ' + + '--wait ' + + name, + parse_output=True, + ) + + self.assertIsNotNone(cmd_output['id']) + self.assertEqual(name, cmd_output['name']) + self.addCleanup(self.openstack, 'server delete --wait ' + name) + + # create security group + security_group_name = uuid.uuid4().hex + + cmd_output = self.openstack( + 'security group list', + parse_output=True, + ) + self.assertNotIn(security_group_name, cmd_output) + + cmd_output = self.openstack( + 'security group create ' + security_group_name, + parse_output=True, + ) + self.assertIsNotNone(cmd_output['id']) + self.addCleanup( + self.openstack, 'security group delete ' + security_group_name + ) + + # add security group to server, assert the name of the security group + # appears + self.openstack( + 'server add security group ' + name + ' ' + security_group_name + ) + + wait_time = 0 + while wait_time < 60: + cmd_output = self.openstack( + 'server show ' + name, + parse_output=True, + ) + if security_group_name not in [ + x['name'] for x in cmd_output['security_groups'] + ]: + # Hang out for a bit and try again + print('retrying add security group check') + wait_time += 10 + time.sleep(10) + else: + break + security_groups = [x['name'] for x in cmd_output['security_groups']] + self.assertIn(security_group_name, security_groups) + + # remove security group, assert the name of the security group doesn't + # appear + self.openstack( + 'server remove security group ' + name + ' ' + security_group_name + ) + + wait_time = 0 + while wait_time < 60: + cmd_output = self.openstack( + 'server show ' + name, + parse_output=True, + ) + if security_group_name not in [ + x['name'] for x in cmd_output['security_groups'] + ]: + # Hang out for a bit and try again + print('retrying remove security group check') + wait_time += 10 + time.sleep(10) + else: + break + security_groups = [x['name'] for x in cmd_output['security_groups']] + self.assertNotIn(security_group_name, security_groups) + def test_server_add_remove_volume(self): volume_wait_for = volume_common.BaseVolumeTests.wait_for_status