From f7177006f52c3538f0db63329d65e679dd88bf5f Mon Sep 17 00:00:00 2001 From: The Magician Date: Fri, 22 Mar 2019 12:37:17 -0700 Subject: [PATCH] Bug fixes for GCP modules (#54256) --- lib/ansible/module_utils/gcp_utils.py | 119 ++++++++++++------ .../modules/cloud/google/gcp_sql_instance.py | 6 +- .../cloud/google/gcp_storage_bucket.py | 29 +---- 3 files changed, 85 insertions(+), 69 deletions(-) diff --git a/lib/ansible/module_utils/gcp_utils.py b/lib/ansible/module_utils/gcp_utils.py index d3dce29ee6..ffa5b5c565 100644 --- a/lib/ansible/module_utils/gcp_utils.py +++ b/lib/ansible/module_utils/gcp_utils.py @@ -237,12 +237,16 @@ class GcpModule(AnsibleModule): return new -# This class takes in two dictionaries `a` and `b`. -# These are dictionaries of arbitrary depth, but made up of standard Python -# types only. -# This differ will compare all values in `a` to those in `b`. -# Note: Only keys in `a` will be compared. Extra keys in `b` will be ignored. -# Note: On all lists, order does matter. +# This class does difference checking according to a set of GCP-specific rules. +# This will be primarily used for checking dictionaries. +# In an equivalence check, the left-hand dictionary will be the request and +# the right-hand side will be the response. + +# Rules: +# Extra keys in response will be ignored. +# Ordering of lists does not matter. +# - exception: lists of dictionaries are +# assumed to be in sorted order. class GcpRequest(object): def __init__(self, request): self.request = request @@ -253,31 +257,48 @@ class GcpRequest(object): def __ne__(self, other): return not self.__eq__(other) - # Returns the difference between `self.request` and `b` - def difference(self, b): - return self._compare_dicts(self.request, b.request) + # Returns the difference between a request + response. + # While this is used under the hood for __eq__ and __ne__, + # it is useful for debugging. + def difference(self, response): + return self._compare_value(self.request, response.request) - def _compare_dicts(self, dict1, dict2): + def _compare_dicts(self, req_dict, resp_dict): difference = {} - for key in dict1: - difference[key] = self._compare_value(dict1.get(key), dict2.get(key)) + for key in req_dict: + if resp_dict.get(key): + difference[key] = self._compare_value(req_dict.get(key), resp_dict.get(key)) # Remove all empty values from difference. - difference2 = {} + sanitized_difference = {} for key in difference: if difference[key]: - difference2[key] = difference[key] + sanitized_difference[key] = difference[key] - return difference2 + return sanitized_difference # Takes in two lists and compares them. - def _compare_lists(self, list1, list2): + # All things in the list should be identical (even if a dictionary) + def _compare_lists(self, req_list, resp_list): + # Have to convert each thing over to unicode. + # Python doesn't handle equality checks between unicode + non-unicode well. difference = [] - for index in range(len(list1)): - value1 = list1[index] - if index < len(list2): - value2 = list2[index] - difference.append(self._compare_value(value1, value2)) + new_req_list = self._convert_value(req_list) + new_resp_list = self._convert_value(resp_list) + + # We have to compare each thing in the request to every other thing + # in the response. + # This is because the request value will be a subset of the response value. + # The assumption is that these lists will be small enough that it won't + # be a performance burden. + for req_item in new_req_list: + found_item = False + for resp_item in new_resp_list: + # Looking for a None value here. + if not self._compare_value(req_item, resp_item): + found_item = True + if not found_item: + difference.append(req_item) difference2 = [] for value in difference: @@ -286,24 +307,25 @@ class GcpRequest(object): return difference2 - def _compare_value(self, value1, value2): + # Compare two values of arbitrary types. + def _compare_value(self, req_value, resp_value): diff = None # If a None is found, a difference does not exist. # Only differing values matter. - if not value2: + if not resp_value: return None # Can assume non-None types at this point. try: - if isinstance(value1, list): - diff = self._compare_lists(value1, value2) - elif isinstance(value2, dict): - diff = self._compare_dicts(value1, value2) - elif isinstance(value1, bool): - diff = self._compare_boolean(value1, value2) + if isinstance(req_value, list): + diff = self._compare_lists(req_value, resp_value) + elif isinstance(req_value, dict): + diff = self._compare_dicts(req_value, resp_value) + elif isinstance(req_value, bool): + diff = self._compare_boolean(req_value, resp_value) # Always use to_text values to avoid unicode issues. - elif to_text(value1) != to_text(value2): - diff = value1 + elif to_text(req_value) != to_text(resp_value): + diff = req_value # to_text may throw UnicodeErrors. # These errors shouldn't crash Ansible and should be hidden. except UnicodeError: @@ -311,24 +333,43 @@ class GcpRequest(object): return diff - def _compare_boolean(self, value1, value2): + # Compare two boolean values. + def _compare_boolean(self, req_value, resp_value): try: # Both True - if value1 and isinstance(value2, bool) and value2: + if req_value and isinstance(resp_value, bool) and resp_value: return None - # Value1 True, value2 'true' - elif value1 and to_text(value2) == 'true': + # Value1 True, resp_value 'true' + elif req_value and to_text(resp_value) == 'true': return None # Both False - elif not value1 and isinstance(value2, bool) and not value2: + elif not req_value and isinstance(resp_value, bool) and not resp_value: return None - # Value1 False, value2 'false' - elif not value1 and to_text(value2) == 'false': + # Value1 False, resp_value 'false' + elif not req_value and to_text(resp_value) == 'false': return None else: - return value2 + return resp_value # to_text may throw UnicodeErrors. # These errors shouldn't crash Ansible and should be hidden. except UnicodeError: return None + + # Python (2 esp.) doesn't do comparisons between unicode + non-unicode well. + # This leads to a lot of false positives when diffing values. + # The Ansible to_text() function is meant to get all strings + # into a standard format. + def _convert_value(self, value): + if isinstance(value, list): + new_list = [] + for item in value: + new_list.append(self._convert_value(item)) + return new_list + elif isinstance(value, dict): + new_dict = {} + for key in value: + new_dict[key] = self._convert_value(value[key]) + return new_dict + else: + return to_text(value) diff --git a/lib/ansible/modules/cloud/google/gcp_sql_instance.py b/lib/ansible/modules/cloud/google/gcp_sql_instance.py index 9301b4f515..9cfa57d2fb 100644 --- a/lib/ansible/modules/cloud/google/gcp_sql_instance.py +++ b/lib/ansible/modules/cloud/google/gcp_sql_instance.py @@ -852,10 +852,10 @@ class InstanceFailoverreplica(object): self.request = {} def to_request(self): - return remove_nones_from_dict({u'available': self.request.get('available'), u'name': self.request.get('name')}) + return remove_nones_from_dict({u'name': self.request.get('name')}) def from_response(self): - return remove_nones_from_dict({u'available': self.request.get(u'available'), u'name': self.request.get(u'name')}) + return remove_nones_from_dict({u'name': self.request.get(u'name')}) class InstanceIpaddressesArray(object): @@ -972,7 +972,6 @@ class InstanceSettings(object): u'tier': self.request.get('tier'), u'availabilityType': self.request.get('availability_type'), u'backupConfiguration': InstanceBackupconfiguration(self.request.get('backup_configuration', {}), self.module).to_request(), - u'settingsVersion': self.request.get('settings_version'), } ) @@ -983,7 +982,6 @@ class InstanceSettings(object): u'tier': self.request.get(u'tier'), u'availabilityType': self.request.get(u'availabilityType'), u'backupConfiguration': InstanceBackupconfiguration(self.request.get(u'backupConfiguration', {}), self.module).from_response(), - u'settingsVersion': self.request.get(u'settingsVersion'), } ) diff --git a/lib/ansible/modules/cloud/google/gcp_storage_bucket.py b/lib/ansible/modules/cloud/google/gcp_storage_bucket.py index 2e73866285..73b13f7363 100644 --- a/lib/ansible/modules/cloud/google/gcp_storage_bucket.py +++ b/lib/ansible/modules/cloud/google/gcp_storage_bucket.py @@ -979,11 +979,8 @@ class BucketAclArray(object): return remove_nones_from_dict( { u'bucket': replace_resource_dict(item.get(u'bucket', {}), 'name'), - u'domain': item.get('domain'), - u'email': item.get('email'), u'entity': item.get('entity'), u'entityId': item.get('entity_id'), - u'id': item.get('id'), u'projectTeam': BucketProjectteam(item.get('project_team', {}), self.module).to_request(), u'role': item.get('role'), } @@ -993,11 +990,8 @@ class BucketAclArray(object): return remove_nones_from_dict( { u'bucket': item.get(u'bucket'), - u'domain': item.get(u'domain'), - u'email': item.get(u'email'), u'entity': item.get(u'entity'), u'entityId': item.get(u'entityId'), - u'id': item.get(u'id'), u'projectTeam': BucketProjectteam(item.get(u'projectTeam', {}), self.module).from_response(), u'role': item.get(u'role'), } @@ -1084,32 +1078,15 @@ class BucketDefaultobjectaclArray(object): return remove_nones_from_dict( { u'bucket': replace_resource_dict(item.get(u'bucket', {}), 'name'), - u'domain': item.get('domain'), - u'email': item.get('email'), u'entity': item.get('entity'), - u'entityId': item.get('entity_id'), - u'generation': item.get('generation'), - u'id': item.get('id'), u'object': item.get('object'), - u'projectTeam': BucketProjectteam(item.get('project_team', {}), self.module).to_request(), u'role': item.get('role'), } ) def _response_from_item(self, item): return remove_nones_from_dict( - { - u'bucket': item.get(u'bucket'), - u'domain': item.get(u'domain'), - u'email': item.get(u'email'), - u'entity': item.get(u'entity'), - u'entityId': item.get(u'entityId'), - u'generation': item.get(u'generation'), - u'id': item.get(u'id'), - u'object': item.get(u'object'), - u'projectTeam': BucketProjectteam(item.get(u'projectTeam', {}), self.module).from_response(), - u'role': item.get(u'role'), - } + {u'bucket': item.get(u'bucket'), u'entity': item.get(u'entity'), u'object': item.get(u'object'), u'role': item.get(u'role')} ) @@ -1250,10 +1227,10 @@ class BucketOwner(object): self.request = {} def to_request(self): - return remove_nones_from_dict({u'entity': self.request.get('entity'), u'entityId': self.request.get('entity_id')}) + return remove_nones_from_dict({u'entity': self.request.get('entity')}) def from_response(self): - return remove_nones_from_dict({u'entity': self.request.get(u'entity'), u'entityId': self.request.get(u'entityId')}) + return remove_nones_from_dict({u'entity': self.request.get(u'entity')}) class BucketVersioning(object):