WIP: server: refuse re-registration of deleted hosts by GUID

engine/schema/src/main/java/com/cloud/host/dao/HostDao.java

@@ -85,6 +85,18 @@ public interface HostDao extends GenericDao<HostVO, Long>, StateDao<Status, Stat
 
     HostVO findByGuid(String guid);
 
+    /**
+     * Finds a host by its exact GUID, including hosts that have been (soft-)removed.
+     * Used to detect an agent trying to re-register with the GUID of a previously deleted host.
+     */
+    HostVO findByGuidIncludingRemoved(String guid);
+
+    /**
+     * Finds a host whose GUID starts with the given prefix, including hosts that have been (soft-)removed.
+     * Used to detect an agent trying to re-register with the GUID of a previously deleted host.
+     */
+    HostVO findByGuidPrefixIncludingRemoved(String guidPrefix);
+
     HostVO findByTypeNameAndZoneId(long zoneId, String name, Host.Type type);
 
     List<HostVO> findHypervisorHostInCluster(long clusterId);

engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java

@@ -118,6 +118,7 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
     protected SearchBuilder<HostVO> UnremovedIpAddressSearch;
 
     protected SearchBuilder<HostVO> GuidSearch;
+    protected SearchBuilder<HostVO> GuidPrefixSearch;
     protected SearchBuilder<HostVO> DcSearch;
     protected SearchBuilder<HostVO> PodSearch;
     protected SearchBuilder<HostVO> ClusterSearch;
@@ -312,6 +313,10 @@ public void init() {
         GuidSearch.and("guid", GuidSearch.entity().getGuid(), SearchCriteria.Op.EQ);
         GuidSearch.done();
 
+        GuidPrefixSearch = createSearchBuilder();
+        GuidPrefixSearch.and("guid", GuidPrefixSearch.entity().getGuid(), SearchCriteria.Op.LIKE);
+        GuidPrefixSearch.done();
+
         DcSearch = createSearchBuilder();
         DcSearch.and("dc", DcSearch.entity().getDataCenterId(), SearchCriteria.Op.EQ);
         DcSearch.and("hypervisorType", DcSearch.entity().getHypervisorType(), Op.EQ);
@@ -637,6 +642,19 @@ public HostVO findByGuid(String guid) {
         return findOneBy(sc);
     }
 
+    @Override
+    public HostVO findByGuidIncludingRemoved(String guid) {
+        SearchCriteria<HostVO> sc = GuidSearch.create("guid", guid);
+        return findOneIncludingRemovedBy(sc);
+    }
+
+    @Override
+    public HostVO findByGuidPrefixIncludingRemoved(String guidPrefix) {
+        SearchCriteria<HostVO> sc = GuidPrefixSearch.create();
+        sc.setParameters("guid", guidPrefix + "%");
+        return findOneIncludingRemovedBy(sc);
+    }
+
     /*
      * Find hosts which is in Disconnected, Down, Alert and ping timeout and server is not null, set server to null
      */

server/src/main/java/com/cloud/resource/ResourceManagerImpl.java

@@ -16,6 +16,7 @@
 // under the License.
 package com.cloud.resource;
 
+import static com.cloud.configuration.ConfigurationManagerImpl.ADD_HOST_ON_SERVICE_RESTART_KVM;
 import static com.cloud.configuration.ConfigurationManagerImpl.MIGRATE_VM_ACROSS_CLUSTERS;
 import static com.cloud.configuration.ConfigurationManagerImpl.SET_HOST_DOWN_TO_MAINTENANCE;
 import static org.apache.cloudstack.gpu.GpuService.GpuDetachOnStop;
@@ -1066,7 +1067,6 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
                 logger.debug("Deleting tags from database for host with UUID [{}].", host.getUuid());
                 _hostTagsDao.deleteTags(hostId);
 
-                host.setGuid(null);
                 final Long clusterId = host.getClusterId();
                 host.setClusterId(null);
                 _hostDao.update(host.getId(), host);
@@ -3217,9 +3217,58 @@ private HostVO getNewHost(StartupCommand[] startupCommands) {
         }
 
         logger.debug(String.format("Could not find Host by guid %s", fullGuid));
+
+        rejectReAddOfDeletedHost(fullGuid, guidPrefix);
+
         return null;
     }
 
+    /**
+     * Refuses the (re-)registration of an agent whose GUID matches a host that was previously deleted
+     * (soft-removed) from CloudStack.
+     * <p>
+     * This is the management-server-side, hypervisor-agnostic enforcement of the intent already expressed by
+     * the {@code add.host.on.service.restart.kvm} setting: when that setting is {@code false} the operator has
+     * indicated a deleted host must not come back. The existing enforcement (in LibvirtServerDiscoverer) only
+     * works for KVM/LXC and only if the agent was still connected at delete time; this guard also covers the
+     * case where the agent was offline when the host was deleted and later reconnects with the same GUID.
+     */
+    protected void rejectReAddOfDeletedHost(String fullGuid, String guidPrefix) {
+        if (ADD_HOST_ON_SERVICE_RESTART_KVM.value()) {
+            return;
+        }
+
+        HostVO deletedHost = findRemovedHostByGuid(fullGuid);
+        if (deletedHost == null && StringUtils.isNotBlank(guidPrefix)) {
+            deletedHost = findRemovedHostByGuidPrefix(guidPrefix);
+        }
+
+        if (deletedHost != null) {
+            String msg = String.format(
+                    "Refusing to (re-)register agent with GUID [%s]: a host with this GUID (id: %d, uuid: %s, name: %s) was previously deleted from CloudStack on %s. " +
+                    "Set the global setting '%s' to true to allow a deleted host to re-register when its agent reconnects.",
+                    fullGuid, deletedHost.getId(), deletedHost.getUuid(), deletedHost.getName(), deletedHost.getRemoved(), ADD_HOST_ON_SERVICE_RESTART_KVM.key());
+            logger.warn(msg);
+            throw new CloudRuntimeException(msg);
+        }
+    }
+
+    private HostVO findRemovedHostByGuid(String guid) {
+        if (StringUtils.isBlank(guid)) {
+            return null;
+        }
+        HostVO host = _hostDao.findByGuidIncludingRemoved(guid);
+        return host != null && host.getRemoved() != null ? host : null;
+    }
+
+    private HostVO findRemovedHostByGuidPrefix(String guidPrefix) {
+        if (StringUtils.isBlank(guidPrefix)) {
+            return null;
+        }
+        HostVO host = _hostDao.findByGuidPrefixIncludingRemoved(guidPrefix);
+        return host != null && host.getRemoved() != null ? host : null;
+    }
+
     protected void validateExistingHostLocationImmutable(final HostVO host, final boolean newHost,
             final long dcId, final Long podId, final Long clusterId, final StartupCommand startup) {
         if (newHost || host == null || host.getType() != Host.Type.Routing) {

server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java

@@ -81,9 +81,11 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Date;
 import java.util.List;
 import java.util.UUID;
 
+import static com.cloud.configuration.ConfigurationManagerImpl.ADD_HOST_ON_SERVICE_RESTART_KVM;
 import static com.cloud.resource.ResourceState.Event.ErrorsCorrected;
 import static com.cloud.resource.ResourceState.Event.InternalEnterMaintenance;
 import static com.cloud.resource.ResourceState.Event.UnableToMaintain;
@@ -240,6 +242,9 @@ public void setup() throws Exception {
 
     @After
     public void tearDown() throws Exception {
+        // rejectReAddOfDeletedHost tests mutate this static ConfigKey; restore its declared
+        // default so the change cannot leak into other tests sharing this JVM fork.
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "true");
         sshHelperMocked.close();
         actionEventUtilsMocked.close();
         getVncPortCommandMockedConstruction.close();
@@ -1393,4 +1398,122 @@ public void testCheckIfAllHostsInUseWithEmptyHostsInMultipleLevels() {
         Mockito.verify(hostDao).findByClusterId(clusterId, Host.Type.Routing);
         Mockito.verify(hostDao).findByPodId(podId, Host.Type.Routing);
     }
+
+    private static final String DELETED_HOST_GUID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee-LibvirtComputingResource";
+    private static final String DELETED_HOST_GUID_PREFIX = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee";
+
+    private HostVO mockDeletedHost() {
+        HostVO deletedHost = Mockito.mock(HostVO.class);
+        when(deletedHost.getRemoved()).thenReturn(new Date());
+        when(deletedHost.getId()).thenReturn(42L);
+        when(deletedHost.getUuid()).thenReturn("some-host-uuid");
+        when(deletedHost.getName()).thenReturn("kvm-host-1");
+        return deletedHost;
+    }
+
+    /**
+     * When 'add.host.on.service.restart.kvm' is true the operator has opted in to letting a deleted
+     * host come back, so the guard must not even query the database.
+     */
+    @Test
+    public void testRejectReAddOfDeletedHostDoesNothingWhenSettingEnabled() throws Exception {
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "true");
+
+        resourceManager.rejectReAddOfDeletedHost(DELETED_HOST_GUID, DELETED_HOST_GUID_PREFIX);
+
+        verify(hostDao, never()).findByGuidIncludingRemoved(anyString());
+        verify(hostDao, never()).findByGuidPrefixIncludingRemoved(anyString());
+    }
+
+    @Test
+    public void testRejectReAddOfDeletedHostDoesNotThrowWhenGuidIsUnknown() throws Exception {
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "false");
+        when(hostDao.findByGuidIncludingRemoved(DELETED_HOST_GUID)).thenReturn(null);
+        when(hostDao.findByGuidPrefixIncludingRemoved(DELETED_HOST_GUID_PREFIX)).thenReturn(null);
+
+        resourceManager.rejectReAddOfDeletedHost(DELETED_HOST_GUID, DELETED_HOST_GUID_PREFIX);
+    }
+
+    @Test
+    public void testRejectReAddOfDeletedHostThrowsWhenFullGuidMatchesDeletedHost() throws Exception {
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "false");
+        HostVO deletedHost = mockDeletedHost();
+        when(hostDao.findByGuidIncludingRemoved(DELETED_HOST_GUID)).thenReturn(deletedHost);
+
+        try {
+            resourceManager.rejectReAddOfDeletedHost(DELETED_HOST_GUID, DELETED_HOST_GUID_PREFIX);
+            Assert.fail("Expected CloudRuntimeException for an agent whose GUID belongs to a deleted host");
+        } catch (CloudRuntimeException e) {
+            Assert.assertTrue(e.getMessage().contains(DELETED_HOST_GUID));
+            Assert.assertTrue(e.getMessage().contains(ADD_HOST_ON_SERVICE_RESTART_KVM.key()));
+        }
+
+        // A full-GUID hit short-circuits; the prefix lookup must not be issued.
+        verify(hostDao, never()).findByGuidPrefixIncludingRemoved(anyString());
+    }
+
+    @Test
+    public void testRejectReAddOfDeletedHostThrowsWhenGuidPrefixMatchesDeletedHost() throws Exception {
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "false");
+        when(hostDao.findByGuidIncludingRemoved(DELETED_HOST_GUID)).thenReturn(null);
+        HostVO deletedHost = mockDeletedHost();
+        when(hostDao.findByGuidPrefixIncludingRemoved(DELETED_HOST_GUID_PREFIX)).thenReturn(deletedHost);
+
+        try {
+            resourceManager.rejectReAddOfDeletedHost(DELETED_HOST_GUID, DELETED_HOST_GUID_PREFIX);
+            Assert.fail("Expected CloudRuntimeException when only the GUID prefix matches a deleted host");
+        } catch (CloudRuntimeException e) {
+            Assert.assertTrue(e.getMessage().contains(ADD_HOST_ON_SERVICE_RESTART_KVM.key()));
+        }
+    }
+
+    /**
+     * A row returned by the *IncludingRemoved lookups may still be a live host. Only soft-deleted
+     * rows (removed != null) may be refused, otherwise a normal agent reconnect would break.
+     */
+    @Test
+    public void testRejectReAddOfDeletedHostAllowsLiveHostWithSameGuid() throws Exception {
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "false");
+        HostVO liveHost = Mockito.mock(HostVO.class);
+        when(liveHost.getRemoved()).thenReturn(null);
+        when(hostDao.findByGuidIncludingRemoved(DELETED_HOST_GUID)).thenReturn(liveHost);
+        when(hostDao.findByGuidPrefixIncludingRemoved(DELETED_HOST_GUID_PREFIX)).thenReturn(null);
+
+        resourceManager.rejectReAddOfDeletedHost(DELETED_HOST_GUID, DELETED_HOST_GUID_PREFIX);
+    }
+
+    @Test
+    public void testRejectReAddOfDeletedHostAllowsLiveHostMatchedByPrefix() throws Exception {
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "false");
+        HostVO liveHost = Mockito.mock(HostVO.class);
+        when(liveHost.getRemoved()).thenReturn(null);
+        when(hostDao.findByGuidIncludingRemoved(DELETED_HOST_GUID)).thenReturn(null);
+        when(hostDao.findByGuidPrefixIncludingRemoved(DELETED_HOST_GUID_PREFIX)).thenReturn(liveHost);
+
+        resourceManager.rejectReAddOfDeletedHost(DELETED_HOST_GUID, DELETED_HOST_GUID_PREFIX);
+    }
+
+    @Test
+    public void testRejectReAddOfDeletedHostSkipsLookupsForBlankGuidAndPrefix() throws Exception {
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "false");
+
+        resourceManager.rejectReAddOfDeletedHost(null, null);
+        resourceManager.rejectReAddOfDeletedHost("", "  ");
+
+        verify(hostDao, never()).findByGuidIncludingRemoved(anyString());
+        verify(hostDao, never()).findByGuidPrefixIncludingRemoved(anyString());
+    }
+
+    /**
+     * A blank prefix must not be turned into a wildcard lookup that could match an unrelated host.
+     */
+    @Test
+    public void testRejectReAddOfDeletedHostSkipsPrefixLookupWhenPrefixBlank() throws Exception {
+        overrideDefaultConfigValue(ADD_HOST_ON_SERVICE_RESTART_KVM, "_defaultValue", "false");
+        when(hostDao.findByGuidIncludingRemoved(DELETED_HOST_GUID)).thenReturn(null);
+
+        resourceManager.rejectReAddOfDeletedHost(DELETED_HOST_GUID, "");
+
+        verify(hostDao, never()).findByGuidPrefixIncludingRemoved(anyString());
+    }
 }