From e9d66df77a5a33ec770a6c3c453fa8efe304e5b4 Mon Sep 17 00:00:00 2001 From: beeankha Date: Wed, 19 Aug 2020 17:25:35 -0400 Subject: [PATCH 1/5] Initial commit for tower_role name vs id lookup bug fix --- .../plugins/module_utils/tower_api.py | 36 +++++++++---------- awx_collection/plugins/modules/tower_role.py | 6 ++-- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/awx_collection/plugins/module_utils/tower_api.py b/awx_collection/plugins/module_utils/tower_api.py index 6b5ed87531..a3d6faa4b8 100644 --- a/awx_collection/plugins/module_utils/tower_api.py +++ b/awx_collection/plugins/module_utils/tower_api.py @@ -105,32 +105,32 @@ class TowerAPIModule(TowerModule): return response['json']['results'][0] - def resolve_name_to_id(self, endpoint, name_or_id): - # Try to resolve the object by name + def get_one_by_name_or_id(self, endpoint, name_or_id): name_field = 'name' - if endpoint == 'users': + if endpoint is 'users': name_field = 'username' - response = self.get_endpoint(endpoint, **{'data': {name_field: name_or_id}}) - if response['status_code'] == 400: - self.fail_json(msg="Unable to try and resolve {0} for {1} : {2}".format(endpoint, name_or_id, response['json']['detail'])) + query_params = {'or__{0}'.format(name_field): name_or_id} + try: + query_params['or__id'] = int(name_or_id) + except ValueError: + # If we got a value error than we didn't have an integer so we can just pass and fall down to the fail + pass + response = self.get_endpoint(endpoint, **{'data': query_params}) if response['json']['count'] == 1: - return response['json']['results'][0]['id'] + return response['json']['results'][0] + elif response['json']['count'] == 2: + for tower_object in response['json']['results']: + return tower_object + # We shouldn't get here because we found 2 objects and ID has to be unique, so one of the objects must have a matching name elif response['json']['count'] == 0: - try: - int(name_or_id) - # If we got 0 items by name, maybe they gave us an ID, let's try looking it up by ID - response = self.head_endpoint("{0}/{1}".format(endpoint, name_or_id), **{'return_none_on_404': True}) - if response is not None: - return name_or_id - except ValueError: - # If we got a value error than we didn't have an integer so we can just pass and fall down to the fail - pass - self.fail_json(msg="The {0} {1} was not found on the Tower server".format(endpoint, name_or_id)) else: - self.fail_json(msg="Found too many names {0} at endpoint {1} try using an ID instead of a name".format(name_or_id, endpoint)) + self.fail_json(msg="Found too many names {0} at endpoint {1}, try using an ID instead of a name".format(name_or_id, endpoint)) + + def resolve_name_to_id(self, endpoint, name_or_id): + return self.get_one_by_name_or_id(endpoint, name_or_id)['id'] def make_request(self, method, endpoint, *args, **kwargs): # In case someone is calling us directly; make sure we were given a method, let's not just assume a GET diff --git a/awx_collection/plugins/modules/tower_role.py b/awx_collection/plugins/modules/tower_role.py index d0d010a0a7..634c3028d0 100644 --- a/awx_collection/plugins/modules/tower_role.py +++ b/awx_collection/plugins/modules/tower_role.py @@ -130,7 +130,7 @@ def main(): resource_name = params.get(param) if resource_name: - resource = module.get_one(endpoint, **{'data': {name_field: resource_name}}) + resource = module.get_one_by_name_or_id(module.param_to_endpoint(param), resource_name) if not resource: module.fail_json( msg='Failed to update role, {0} not found in {1}'.format(param, endpoint), @@ -170,14 +170,14 @@ def main(): if response['status_code'] == 204: module.json_output['changed'] = True else: - module.fail_json(msg="Failed to grant role {0}".format(response['json']['detail'])) + module.fail_json(msg="Failed to grant role. {0}".format(response['json'].get('detail', response['json'].get('msg', 'unknown')))) else: for an_id in list(set(existing_associated_ids) & set(new_association_list)): response = module.post_endpoint(association_endpoint, **{'data': {'id': int(an_id), 'disassociate': True}}) if response['status_code'] == 204: module.json_output['changed'] = True else: - module.fail_json(msg="Failed to revoke role {0}".format(response['json']['detail'])) + module.fail_json(msg="Failed to revoke role. {0}".format(response['json'].get('detail', response['json'].get('msg', 'unknown')))) module.exit_json(**module.json_output) From 0fd618d88b780977d3dc8d66aefa0736689452b5 Mon Sep 17 00:00:00 2001 From: John Westcott IV Date: Thu, 20 Aug 2020 11:14:58 -0400 Subject: [PATCH 2/5] Changing test suite and fixing user issue in tower_api --- .../plugins/module_utils/tower_api.py | 10 +- .../targets/tower_role/tasks/main.yml | 158 +++++++++++------- 2 files changed, 105 insertions(+), 63 deletions(-) diff --git a/awx_collection/plugins/module_utils/tower_api.py b/awx_collection/plugins/module_utils/tower_api.py index a3d6faa4b8..d95ca64f94 100644 --- a/awx_collection/plugins/module_utils/tower_api.py +++ b/awx_collection/plugins/module_utils/tower_api.py @@ -107,7 +107,7 @@ class TowerAPIModule(TowerModule): def get_one_by_name_or_id(self, endpoint, name_or_id): name_field = 'name' - if endpoint is 'users': + if endpoint == 'users': name_field = 'username' query_params = {'or__{0}'.format(name_field): name_or_id} @@ -118,12 +118,16 @@ class TowerAPIModule(TowerModule): pass response = self.get_endpoint(endpoint, **{'data': query_params}) + if response['status_code'] != 200: + self.fail_json(msg="Failed to query endpoint {0} for {1} {2} ({3}), see results".format(endpoint, name_field, name_or_id, response['status_code']), resuls=response) + if response['json']['count'] == 1: return response['json']['results'][0] elif response['json']['count'] == 2: for tower_object in response['json']['results']: - return tower_object - # We shouldn't get here because we found 2 objects and ID has to be unique, so one of the objects must have a matching name + if tower_object['name'] == name_or_id: + return tower_object + # We shouldn't get here because we found 2 objects and ID has to be unique, so one of the objects must have a matching name elif response['json']['count'] == 0: self.fail_json(msg="The {0} {1} was not found on the Tower server".format(endpoint, name_or_id)) else: diff --git a/awx_collection/tests/integration/targets/tower_role/tasks/main.yml b/awx_collection/tests/integration/targets/tower_role/tasks/main.yml index f0c26a7e04..4102a11402 100644 --- a/awx_collection/tests/integration/targets/tower_role/tasks/main.yml +++ b/awx_collection/tests/integration/targets/tower_role/tasks/main.yml @@ -1,74 +1,112 @@ --- +- name: Generate a test id + set_fact: + test_id: "{{ lookup('password', '/dev/null chars=ascii_letters length=16') }}" + - name: Generate names set_fact: - username: "AWX-Collection-tests-tower_role-user-{{ lookup('password', '/dev/null chars=ascii_letters length=16') }}" + username: "AWX-Collection-tests-tower_role-user-{{ test_id }}" + project_name: "AWX-Collection-tests-tower_role-project-{{ test_id }}" -- name: Create a User - tower_user: - first_name: Joe - last_name: User - username: "{{ username }}" - password: "{{ 65535 | random | to_uuid }}" - email: joe@example.org - state: present - register: result +- block: + - name: Create a User + tower_user: + first_name: Joe + last_name: User + username: "{{ username }}" + password: "{{ 65535 | random | to_uuid }}" + email: joe@example.org + state: present + register: result -- assert: - that: - - "result is changed" + - assert: + that: + - "result is changed" -- name: Add Joe to the update role of the default Project - tower_role: - user: "{{ username }}" - role: update - project: Demo Project - state: "{{ item }}" - register: result - with_items: - - "present" - - "absent" + - name: Create a project + tower_project: + name: "{{ project_name }}" + organization: Default + scm_type: git + scm_url: https://github.com/ansible/test-playbooks + wait: false + register: project_info -- assert: - that: - - "result is changed" + - assert: + that: + - project_info is changed -- name: Create a workflow - tower_workflow_job_template: - name: test-role-workflow - organization: Default - state: present + - name: Add Joe to the update role of the default Project + tower_role: + user: "{{ username }}" + role: update + project: "Demo Project" + state: "{{ item }}" + register: result + with_items: + - "present" + - "absent" -- name: Add Joe to workflow execute role - tower_role: - user: "{{ username }}" - role: execute - workflow: test-role-workflow - state: present - register: result + - assert: + that: + - "result is changed" -- assert: - that: - - "result is changed" + - name: Add Joe to the new project by ID + tower_role: + user: "{{ username }}" + role: update + project: "{{ project_info['id'] }}" + state: "{{ item }}" + register: result + with_items: + - "present" + - "absent" -- name: Add Joe to workflow execute role, no-op - tower_role: - user: "{{ username }}" - role: execute - workflow: test-role-workflow - state: present - register: result + - assert: + that: + - "result is changed" -- assert: - that: - - "result is not changed" + - name: Create a workflow + tower_workflow_job_template: + name: test-role-workflow + organization: Default + state: present -- name: Delete a User - tower_user: - username: "{{ username }}" - email: joe@example.org - state: absent - register: result + - name: Add Joe to workflow execute role + tower_role: + user: "{{ username }}" + role: execute + workflow: test-role-workflow + state: present + register: result -- assert: - that: - - "result is changed" + - assert: + that: + - "result is changed" + + - name: Add Joe to workflow execute role, no-op + tower_role: + user: "{{ username }}" + role: execute + workflow: test-role-workflow + state: present + register: result + + - assert: + that: + - "result is not changed" + + always: + - name: Delete a User + tower_user: + username: "{{ username }}" + email: joe@example.org + state: absent + register: result + + - name: Delete the project + tower_project: + name: "{{ project_name }}" + organization: Default + state: absent + register: result From 54317236f37c4ed7271c13e1c643da9aba9cdae9 Mon Sep 17 00:00:00 2001 From: beeankha Date: Thu, 20 Aug 2020 15:41:51 -0400 Subject: [PATCH 3/5] Fix pep8 error, remove redundant var assignment --- awx_collection/plugins/module_utils/tower_api.py | 7 +++++-- awx_collection/plugins/modules/tower_role.py | 1 - 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/awx_collection/plugins/module_utils/tower_api.py b/awx_collection/plugins/module_utils/tower_api.py index d95ca64f94..efd6791a17 100644 --- a/awx_collection/plugins/module_utils/tower_api.py +++ b/awx_collection/plugins/module_utils/tower_api.py @@ -114,12 +114,15 @@ class TowerAPIModule(TowerModule): try: query_params['or__id'] = int(name_or_id) except ValueError: - # If we got a value error than we didn't have an integer so we can just pass and fall down to the fail + # If we get a value error, then we didn't have an integer so we can just pass and fall down to the fail pass response = self.get_endpoint(endpoint, **{'data': query_params}) if response['status_code'] != 200: - self.fail_json(msg="Failed to query endpoint {0} for {1} {2} ({3}), see results".format(endpoint, name_field, name_or_id, response['status_code']), resuls=response) + self.fail_json( + msg="Failed to query endpoint {0} for {1} {2} ({3}), see results".format(endpoint, name_field, name_or_id, response['status_code']), + resuls=response + ) if response['json']['count'] == 1: return response['json']['results'][0] diff --git a/awx_collection/plugins/modules/tower_role.py b/awx_collection/plugins/modules/tower_role.py index 634c3028d0..f06cc16e4d 100644 --- a/awx_collection/plugins/modules/tower_role.py +++ b/awx_collection/plugins/modules/tower_role.py @@ -126,7 +126,6 @@ def main(): resource_data = {} for param in resource_param_keys: endpoint = module.param_to_endpoint(param) - name_field = 'username' if param == 'user' else 'name' resource_name = params.get(param) if resource_name: From 75a0c0ab1e9c69ef8e1ac9c978c7ee4ec07e35e5 Mon Sep 17 00:00:00 2001 From: beeankha Date: Fri, 21 Aug 2020 15:03:30 -0400 Subject: [PATCH 4/5] Prioritize integer names over IDs more effectively --- .../plugins/module_utils/tower_api.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/awx_collection/plugins/module_utils/tower_api.py b/awx_collection/plugins/module_utils/tower_api.py index efd6791a17..d2ef54b113 100644 --- a/awx_collection/plugins/module_utils/tower_api.py +++ b/awx_collection/plugins/module_utils/tower_api.py @@ -126,11 +126,21 @@ class TowerAPIModule(TowerModule): if response['json']['count'] == 1: return response['json']['results'][0] - elif response['json']['count'] == 2: + elif response['json']['count'] > 1: + object_to_return = None for tower_object in response['json']['results']: - if tower_object['name'] == name_or_id: - return tower_object - # We shouldn't get here because we found 2 objects and ID has to be unique, so one of the objects must have a matching name + # Name takes priority, so we match on name first + # If we already have an object to return and it has the right name, we need to fail + if object_to_return and object_to_return['name'] == name_or_id: + self.fail_json(msg="The requested name or id was ambiguous and resulted in too many items") + # If we found an object ID with this name (which has to be unique) and we don't already have + # something to return, select this object for now + elif tower_object['id'] == name_or_id and not object_to_return: + object_to_return = tower_object + # Otherwise this is the first named object we found, so record that as what to return + else: + object_to_return = tower_object + return object_to_return elif response['json']['count'] == 0: self.fail_json(msg="The {0} {1} was not found on the Tower server".format(endpoint, name_or_id)) else: From da732a39410a978200d9057389e71607b5c4904c Mon Sep 17 00:00:00 2001 From: beeankha Date: Tue, 25 Aug 2020 14:57:02 -0400 Subject: [PATCH 5/5] Add unit test, update lookup method to favor ID over name --- .../plugins/module_utils/tower_api.py | 20 ++++------------ awx_collection/test/awx/test_module_utils.py | 23 +++++++++++++++++++ 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/awx_collection/plugins/module_utils/tower_api.py b/awx_collection/plugins/module_utils/tower_api.py index d2ef54b113..36e3d045f0 100644 --- a/awx_collection/plugins/module_utils/tower_api.py +++ b/awx_collection/plugins/module_utils/tower_api.py @@ -127,24 +127,14 @@ class TowerAPIModule(TowerModule): if response['json']['count'] == 1: return response['json']['results'][0] elif response['json']['count'] > 1: - object_to_return = None for tower_object in response['json']['results']: - # Name takes priority, so we match on name first - # If we already have an object to return and it has the right name, we need to fail - if object_to_return and object_to_return['name'] == name_or_id: - self.fail_json(msg="The requested name or id was ambiguous and resulted in too many items") - # If we found an object ID with this name (which has to be unique) and we don't already have - # something to return, select this object for now - elif tower_object['id'] == name_or_id and not object_to_return: - object_to_return = tower_object - # Otherwise this is the first named object we found, so record that as what to return - else: - object_to_return = tower_object - return object_to_return + # ID takes priority, so we match on that first + if str(tower_object['id']) == name_or_id: + return tower_object + # We didn't match on an ID but we found more than 1 object, therefore the results are ambiguous + self.fail_json(msg="The requested name or id was ambiguous and resulted in too many items") elif response['json']['count'] == 0: self.fail_json(msg="The {0} {1} was not found on the Tower server".format(endpoint, name_or_id)) - else: - self.fail_json(msg="Found too many names {0} at endpoint {1}, try using an ID instead of a name".format(name_or_id, endpoint)) def resolve_name_to_id(self, endpoint, name_or_id): return self.get_one_by_name_or_id(endpoint, name_or_id)['id'] diff --git a/awx_collection/test/awx/test_module_utils.py b/awx_collection/test/awx/test_module_utils.py index e93b6ee939..2903621768 100644 --- a/awx_collection/test/awx/test_module_utils.py +++ b/awx_collection/test/awx/test_module_utils.py @@ -4,6 +4,7 @@ __metaclass__ = type import json import sys +from awx.main.models import Organization, Team from requests.models import Response from unittest import mock @@ -102,3 +103,25 @@ def test_no_templated_values(collection_import): 'The inventory plugin FQCN is templated when the collection is built ' 'and the code should retain the default of awx.awx.' ) + + +def test_conflicting_name_and_id(run_module, admin_user): + """In the event that 2 related items match our search criteria in this way: + one item has an id that matches input + one item has a name that matches input + We should preference the id over the name. + Otherwise, the universality of the tower_api lookup plugin is compromised. + """ + org_by_id = Organization.objects.create(name='foo') + slug = str(org_by_id.id) + org_by_name = Organization.objects.create(name=slug) + result = run_module('tower_team', { + 'name': 'foo_team', 'description': 'fooin around', + 'organization': slug + }, admin_user) + assert not result.get('failed', False), result.get('msg', result) + team = Team.objects.filter(name='foo_team').first() + assert str(team.organization_id) == slug, ( + 'Lookup by id should be preferenced over name in cases of conflict.' + ) + assert team.organization.name == 'foo'