From af902f24a2b58ba9ad408217ef3f17bbdef9433b Mon Sep 17 00:00:00 2001 From: corubba Date: Thu, 19 May 2022 00:56:13 +0200 Subject: [PATCH] Update using only one api call Starting with the very first commit, the update was always done with two api calls: one for DELETE and one for REPLACE. It is however perfectly valid and save to do both at once, which makes it atomic, so no need for the rollback. Plus it only updates the serial once. There is no point in sending the full RRset data when deleting it, the key attributes to identify it are enough. This also make the behaviour consistent with the api docs [0] where it says "MUST NOT be included when changetype is set to DELETE." [0] https://doc.powerdns.com/authoritative/http-api/zone.html#rrset --- powerdnsadmin/models/record.py | 97 ++++++++++++++++------------------ 1 file changed, 45 insertions(+), 52 deletions(-) diff --git a/powerdnsadmin/models/record.py b/powerdnsadmin/models/record.py index 112b9e3..b5e3050 100644 --- a/powerdnsadmin/models/record.py +++ b/powerdnsadmin/models/record.py @@ -303,10 +303,47 @@ class Record(object): data=rrsets) return jdata + @staticmethod + def to_api_payload(new_rrsets, del_rrsets): + """Turn the given changes into a single api payload.""" + + def replace_for_api(rrset): + """Return a modified copy of the given RRset with changetype REPLACE.""" + if not rrset or rrset.get('changetype', None) != 'REPLACE': + return rrset + replace_copy = dict(rrset) + # For compatibility with some backends: Remove comments from rrset if all are blank + if not any((bool(c.get('content', None)) for c in replace_copy.get('comments', []))): + replace_copy.pop('comments', None) + return replace_copy + + def rrset_in(needle, haystack): + """Return whether the given RRset (identified by name and type) is in the list.""" + for hay in haystack: + if needle['name'] == hay['name'] and needle['type'] == hay['type']: + return True + return False + + def delete_for_api(rrset): + """Return a minified copy of the given RRset with changetype DELETE.""" + if not rrset or rrset.get('changetype', None) != 'DELETE': + return rrset + delete_copy = dict(rrset) + delete_copy.pop('ttl', None) + delete_copy.pop('records', None) + delete_copy.pop('comments', None) + return delete_copy + + replaces = [replace_for_api(r) for r in new_rrsets] + deletes = [delete_for_api(r) for r in del_rrsets if not rrset_in(r, replaces)] + return { + 'rrsets': replaces + deletes + } + def apply(self, domain_name, submitted_records): """ Apply record changes to a domain. This function - will make 2 calls to the PDNS API to DELETE and + will make 1 call to the PDNS API to DELETE and REPLACE records (rrsets) """ current_app.logger.debug( @@ -315,68 +352,24 @@ class Record(object): # Get the list of rrsets to be added and deleted new_rrsets, del_rrsets = self.compare(domain_name, submitted_records) - # Remove blank comments from rrsets for compatibility with some backends - def remove_blank_comments(rrset): - if not rrset['comments']: - del rrset['comments'] - elif isinstance(rrset['comments'], list): - # Merge all non-blank comment values into a list - merged_comments = [ - v - for c in rrset['comments'] - for v in c.values() - if v - ] - # Delete comment if all values are blank (len(merged_comments) == 0) - if not merged_comments: - del rrset['comments'] - - for r in new_rrsets['rrsets']: - remove_blank_comments(r) - - for r in del_rrsets['rrsets']: - remove_blank_comments(r) + # The history logic still needs *all* the deletes with full data to display a useful diff. + # So create a "minified" copy for the api call, and return the original data back up + api_payload = self.to_api_payload(new_rrsets['rrsets'], del_rrsets['rrsets']) + current_app.logger.debug(f"api payload: \n{utils.pretty_json(api_payload)}") # Submit the changes to PDNS API try: - if del_rrsets["rrsets"]: - result = self.apply_rrsets(domain_name, del_rrsets) + if api_payload["rrsets"]: + result = self.apply_rrsets(domain_name, api_payload) if 'error' in result.keys(): current_app.logger.error( - 'Cannot apply record changes with deleting rrsets step. PDNS error: {}' + 'Cannot apply record changes. PDNS error: {}' .format(result['error'])) return { 'status': 'error', 'msg': result['error'].replace("'", "") } - if new_rrsets["rrsets"]: - result = self.apply_rrsets(domain_name, new_rrsets) - if 'error' in result.keys(): - current_app.logger.error( - 'Cannot apply record changes with adding rrsets step. PDNS error: {}' - .format(result['error'])) - - # rollback - re-add the removed record if the adding operation is failed. - if del_rrsets["rrsets"]: - rollback_rrsets = del_rrsets - for r in del_rrsets["rrsets"]: - r['changetype'] = 'REPLACE' - rollback = self.apply_rrsets(domain_name, rollback_rrsets) - if 'error' in rollback.keys(): - return dict(status='error', - msg='Failed to apply changes. Cannot rollback previous failed operation: {}' - .format(rollback['error'].replace("'", ""))) - else: - return dict(status='error', - msg='Failed to apply changes. Rolled back previous failed operation: {}' - .format(result['error'].replace("'", ""))) - else: - return { - 'status': 'error', - 'msg': result['error'].replace("'", "") - } - self.auto_ptr(domain_name, new_rrsets, del_rrsets) self.update_db_serial(domain_name) current_app.logger.info('Record was applied successfully.')