diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index c905ab1ce..439f705f3 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -366,7 +366,7 @@ class Domain(TimeStampedModel, DomainHelper): # get deleted values-which are values in previous nameserver list # but are not in the list of new host values if prevHost not in newHostDict: - deleted_values.append((prevHost, addrs)) + deleted_values.append(prevHost) # if the host exists in both, check if the addresses changed else: # TODO - host is being updated when previous was None+new is empty list @@ -388,16 +388,6 @@ class Domain(TimeStampedModel, DomainHelper): return (deleted_values, updated_values, new_values, previousHostDict) - def _deleted_host_values(self, deleted_values) -> int: - successDeletedCount = 0 - - for hostTuple in deleted_values: - deleted_response_code = self._delete_host(hostTuple[0]) - if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: - successDeletedCount += 1 - - return successDeletedCount - def _update_host_values(self, updated_values, oldNameservers): for hostTuple in updated_values: updated_response_code = self._update_host( @@ -412,8 +402,15 @@ class Domain(TimeStampedModel, DomainHelper): % (hostTuple[0], updated_response_code) ) - def _new_host_values(self, new_values) -> int: - successCreatedCount = 0 + def createNewHostList(self, new_values: dict) -> list: + """convert the dictionary of new values to a list of HostObjSet + for use in the UpdateDomain epp message + Args: + new_values: dict(str,list)- dict of {nameserver:ips} to add to domain + Returns: + list[epp.HostObjSet]-epp object list for use in the UpdateDomain epp message + """ + addToDomainList = [] for key, value in new_values.items(): createdCode = self._create_host( host=key, addrs=value @@ -422,18 +419,21 @@ class Domain(TimeStampedModel, DomainHelper): createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY or createdCode == ErrorCode.OBJECT_EXISTS ): - request = commands.UpdateDomain( - name=self.name, add=[epp.HostObjSet([key])] - ) - try: - registry.send(request, cleaned=True) - successCreatedCount += 1 - except RegistryError as e: - logger.error( - "Error adding nameserver, code was %s error was %s" - % (e.code, e) - ) - return successCreatedCount + addToDomainList.append(epp.HostObjSet([key])) + + return addToDomainList + + def createDeleteHostList(self, hostsToDelete: list[str]): + """ + Args: + hostsToDelete (list[str])- list of nameserver/host names to remove + Returns: + list[epp.HostObjSet]-epp object list for use in the UpdateDomain epp message + """ + deleteList = [] + for nameserver in hostsToDelete: + deleteList.append(epp.HostObjSet([nameserver])) + return deleteList @Cache def dnssecdata(self) -> extensions.DNSSECExtension: @@ -479,16 +479,23 @@ class Domain(TimeStampedModel, DomainHelper): oldNameservers, ) = self.getNameserverChanges(hosts=hosts) - successCreatedCount = self._new_host_values(new_values) - successDeletedCount = self._deleted_host_values(deleted_values) # returns value _ = self._update_host_values( updated_values, oldNameservers ) # returns nothing, just need to be run and errors - - successTotalNameservers = ( - len(oldNameservers) - successDeletedCount + successCreatedCount + addToDomainList = self.createNewHostList(new_values) + deleteHostList = self.createDeleteHostList(deleted_values) + responseCode = self.addAndRemoveHostsFromDomain( + hostsToAdd=addToDomainList, hostsToDelete=deleteHostList ) + # if unable to update domain raise error and stop + if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: + raise NameserverError(code=nsErrorCodes.UNABLE_TO_UPDATE_DOMAIN) + + successTotalNameservers = ( + len(oldNameservers) - len(deleteHostList) + len(addToDomainList) + ) + self._delete_hosts_if_not_used(hostsToDelete=deleted_values) if successTotalNameservers < 2: try: self.dns_needed() @@ -1459,36 +1466,64 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("_update_host()-> sending req as %s" % request) return response.code except RegistryError as e: - logger.error("Error _delete_host, code was %s error was %s" % (e.code, e)) + logger.error("Error _update_host, code was %s error was %s" % (e.code, e)) return e.code - def _delete_host(self, nameserver: str): - """Remove this host from the domain and delete the host object in registry, - will only delete the host object, if it's not being used by another domain - Performs two epp calls and can result in a RegistryError + def addAndRemoveHostsFromDomain( + self, hostsToAdd: list[str], hostsToDelete: list[str] + ): + """sends an UpdateDomain message to the registry with the hosts provided Args: - nameserver (str): nameserver or subdomain - ip_list (list[str]): the new list of ips, may be empty - old_ip_list (list[str]): the old ip list, may also be empty - + hostsToDelete (list[epp.HostObjSet])- list of host objects to delete + hostsToAdd (list[epp.HostObjSet])- list of host objects to add Returns: - errorCode (int): one of ErrorCode enum type values - + response code (int)- RegistryErrorCode integer value + defaults to return COMMAND_COMPLETED_SUCCESSFULLY + if there is nothing to add or delete """ + + if hostsToAdd == [] and hostsToDelete == []: + return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY + try: updateReq = commands.UpdateDomain( - name=self.name, rem=[epp.HostObjSet([nameserver])] + name=self.name, rem=hostsToDelete, add=hostsToAdd ) response = registry.send(updateReq, cleaned=True) - logger.info("_delete_host()-> sending update domain req as %s" % updateReq) - - deleteHostReq = commands.DeleteHost(name=nameserver) - response = registry.send(deleteHostReq, cleaned=True) logger.info( - "_delete_host()-> sending delete host req as %s" % deleteHostReq + "addAndRemoveHostsFromDomain()-> sending update domain req as %s" + % updateReq ) - return response.code + + except RegistryError as e: + logger.error( + "Error addAndRemoveHostsFromDomain, code was %s error was %s" + % (e.code, e) + ) + + return response.code + + def _delete_hosts_if_not_used(self, hostsToDelete: list[str]): + """delete the host object in registry, + will only delete the host object, if it's not being used by another domain + Performs just the DeleteHost epp call + Supresses regstry error, as registry can disallow delete for various reasons + Args: + hostsToDelete (list[str])- list of nameserver/host names to remove + Returns: + None + + """ + try: + for nameserver in hostsToDelete: + deleteHostReq = commands.DeleteHost(name=nameserver) + registry.send(deleteHostReq, cleaned=True) + logger.info( + "_delete_hosts_if_not_used()-> sending delete host req as %s" + % deleteHostReq + ) + except RegistryError as e: if e.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: logger.info( @@ -1497,9 +1532,9 @@ class Domain(TimeStampedModel, DomainHelper): ) else: logger.error( - "Error _delete_host, code was %s error was %s" % (e.code, e) + "Error _delete_hosts_if_not_used, code was %s error was %s" + % (e.code, e) ) - return e.code def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): """Contact registry for info about a domain.""" diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index b0bd701aa..4c532947f 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -784,9 +784,12 @@ class MockEppLib(TestCase): res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, ) - # elif isinstance(_request, commands.UpdateHost): - # if getattr(_request, "name", None) == "ns1.failednameserver.gov": - # raise RegistryError(code=ErrorCode.OBJECT_EXISTS) + elif isinstance(_request, commands.UpdateHost): + return MagicMock( + res_data=[self.mockDataHostChange], + code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, + ) + elif isinstance(_request, commands.UpdateDomain): return MagicMock( res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 34ba15cd5..f018d647a 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -882,7 +882,7 @@ class TestRegistrantNameservers(MockEppLib): oldNameservers, ) = self.domain.getNameserverChanges(newChanges) - self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3.4"])]) + self.assertEqual(deleted_values, ["ns2.example.com"]) self.assertEqual(updated_values, []) self.assertEqual(new_values, {}) self.assertEqual( @@ -956,7 +956,9 @@ class TestRegistrantNameservers(MockEppLib): # when we create a host, we should've updated at the same time created_host = commands.CreateHost(nameserver) update_domain_with_created = commands.UpdateDomain( - name=self.domain.name, add=[common.HostObjSet([created_host.name])] + name=self.domain.name, + add=[common.HostObjSet([created_host.name])], + rem=[], ) # checking if commands were sent (commands have to be sent in order) @@ -986,13 +988,15 @@ class TestRegistrantNameservers(MockEppLib): # when you create a host, you also have to update at same time created_host1 = commands.CreateHost(self.nameserver1) - update_domain_with_created1 = commands.UpdateDomain( - name=self.domain.name, add=[common.HostObjSet([created_host1.name])] - ) - created_host2 = commands.CreateHost(self.nameserver2) - update_domain_with_created2 = commands.UpdateDomain( - name=self.domain.name, add=[common.HostObjSet([created_host2.name])] + + update_domain_with_created = commands.UpdateDomain( + name=self.domain.name, + add=[ + common.HostObjSet([created_host1.name]), + common.HostObjSet([created_host2.name]), + ], + rem=[], ) infoDomain = commands.InfoDomain(name="my-nameserver.gov", auth_info=None) @@ -1000,13 +1004,12 @@ class TestRegistrantNameservers(MockEppLib): expectedCalls = [ call(infoDomain, cleaned=True), call(created_host1, cleaned=True), - call(update_domain_with_created1, cleaned=True), call(created_host2, cleaned=True), - call(update_domain_with_created2, cleaned=True), + call(update_domain_with_created, cleaned=True), ] self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) - self.assertEqual(5, self.mockedSendFunction.call_count) + self.assertEqual(4, self.mockedSendFunction.call_count) # check that status is READY self.assertTrue(self.domain.is_active()) @@ -1116,24 +1119,15 @@ class TestRegistrantNameservers(MockEppLib): call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True), call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), - call( - commands.UpdateDomain( - name=self.domainWithThreeNS.name, - add=[], - rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])], - nsset=None, - keyset=None, - registrant=None, - auth_info=None, - ), - cleaned=True, - ), call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True), call( commands.UpdateDomain( name=self.domainWithThreeNS.name, add=[], - rem=[common.HostObjSet(hosts=["ns1.cats-are-superior3.com"])], + rem=[ + common.HostObjSet(hosts=["ns1.my-nameserver-2.com"]), + common.HostObjSet(hosts=["ns1.cats-are-superior3.com"]), + ], nsset=None, keyset=None, registrant=None, @@ -1157,7 +1151,6 @@ class TestRegistrantNameservers(MockEppLib): And `commands.UpdateDomain` is sent to add #4 and #5 plus remove #2 and #3 And `commands.DeleteHost` is sent to delete #2 and #3 """ - print("in second failing test") self.domainWithThreeNS.nameservers = [ (self.nameserver1,), ("ns1.cats-are-superior1.com",), @@ -1172,35 +1165,11 @@ class TestRegistrantNameservers(MockEppLib): call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True), call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), - call( - commands.UpdateDomain( - name=self.domainWithThreeNS.name, - add=[], - rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])], - nsset=None, - keyset=None, - registrant=None, - auth_info=None, - ), - cleaned=True, - ), call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True), call( commands.CreateHost(name="ns1.cats-are-superior1.com", addrs=[]), cleaned=True, ), - call( - commands.UpdateDomain( - name=self.domainWithThreeNS.name, - add=[common.HostObjSet(hosts=["ns1.cats-are-superior1.com"])], - rem=[], - nsset=None, - keyset=None, - registrant=None, - auth_info=None, - ), - cleaned=True, - ), call( commands.CreateHost(name="ns1.cats-are-superior2.com", addrs=[]), cleaned=True, @@ -1208,8 +1177,14 @@ class TestRegistrantNameservers(MockEppLib): call( commands.UpdateDomain( name=self.domainWithThreeNS.name, - add=[common.HostObjSet(hosts=["ns1.cats-are-superior2.com"])], - rem=[], + add=[ + common.HostObjSet(hosts=["ns1.cats-are-superior1.com"]), + common.HostObjSet(hosts=["ns1.cats-are-superior2.com"]), + ], + rem=[ + common.HostObjSet(hosts=["ns1.my-nameserver-2.com"]), + common.HostObjSet(hosts=["ns1.cats-are-superior3.com"]), + ], nsset=None, keyset=None, registrant=None, @@ -1395,7 +1370,6 @@ class TestRegistrantNameservers(MockEppLib): domain.nameservers = [("ns1.failednameserver.gov", ["4.5.6"])] def tearDown(self): - print("in tear down") Domain.objects.all().delete() return super().tearDown() diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 5eb92750e..39d24caee 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -35,6 +35,7 @@ class NameserverErrorCodes(IntEnum): GLUE_RECORD_NOT_ALLOWED = 2 INVALID_IP = 3 TOO_MANY_HOSTS = 4 + UNABLE_TO_UPDATE_DOMAIN = 5 class NameserverError(Exception): @@ -52,6 +53,10 @@ class NameserverError(Exception): NameserverErrorCodes.TOO_MANY_HOSTS: ( "Too many hosts provided, you may not have more than " "13 nameservers." ), + NameserverErrorCodes.UNABLE_TO_UPDATE_DOMAIN: ( + "Unable to update domain, changes were not applied." + "Check logs as a Registry Error is the likely cause" + ), } def __init__(self, *args, code=None, nameserver=None, ip=None, **kwargs):