diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 89f27db5d5f..fd4fe7af668 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -1314,17 +1314,17 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C throw new InvalidParameterValueException("Storage pool must be in Maintenance state before its URL can be changed. " + "Please put the pool into maintenance first."); } - URI newUri; - try { - newUri = new URI(cmd.getUrl()); - } catch (URISyntaxException e) { - throw new InvalidParameterValueException("Invalid URL format: " + cmd.getUrl()); - } - storagePool.setHostAddress(newUri.getHost()); - storagePool.setPath(newUri.getPath()); - if (newUri.getPort() != -1) { - storagePool.setPort(newUri.getPort()); + Map newUriParams = extractUriParamsAsMap(cmd.getUrl()); + String newHost = newUriParams.get("host"); + String newPath = newUriParams.get("hostPath"); + if (StringUtils.isAnyBlank(newHost, newPath)) { + throw new InvalidParameterValueException("Invalid URL: host and path must not be empty. " + + "Expected format: nfs://hostname/path or gluster://hostname/volume"); } + storagePool.setHostAddress(newHost); + storagePool.setPath(newPath); + String newPortStr = newUriParams.get("port"); + storagePool.setPort(newPortStr != null ? Integer.parseInt(newPortStr) : 0); } } _storagePoolDao.update(id, storagePool); @@ -1366,10 +1366,12 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C logger.info("Propagating new URL for storage pool [{}] to {} connected host(s).", updatedPool.getName(), connectedHostIds.size()); + List failedHosts = new ArrayList<>(); for (Long hostId : connectedHostIds) { HostVO host = _hostDao.findById(hostId); if (host == null) { logger.warn("Host [{}] not found; skipping URL propagation for pool [{}].", hostId, updatedPool.getName()); + failedHosts.add("unknown-host-" + hostId); continue; } @@ -1380,12 +1382,21 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C String detail = (answer == null) ? "no answer returned" : answer.getDetails(); logger.warn("Failed to propagate new URL for storage pool [{}] to host [{}]: {}", updatedPool.getName(), host.getName(), detail); + failedHosts.add(host.getName()); } else { logger.info("Successfully propagated new URL for storage pool [{}] to host [{}].", updatedPool.getName(), host.getName()); updateStoragePoolHostVOAndBytes(freshPool, hostId, (ModifyStoragePoolAnswer) answer); } } + + if (!failedHosts.isEmpty()) { + throw new CloudRuntimeException(String.format( + "Storage pool [%s] URL was updated in the database, but the new mount could not be applied " + + "on the following host(s): %s. The pool may be in an inconsistent state; please verify " + + "connectivity and remount manually if necessary.", + updatedPool.getName(), String.join(", ", failedHosts))); + } } private void changeStoragePoolScopeToZone(StoragePoolVO primaryStorage) { @@ -4220,10 +4231,14 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C throw new IllegalArgumentException("Unable to find image store with ID: " + id); } if (url != null) { - if (!imageStoreVO.isReadonly()) { + boolean willBeReadonly = (readonly != null && readonly) || imageStoreVO.isReadonly(); + if (!willBeReadonly) { throw new InvalidParameterValueException("Image store must be set to read-only (maintenance) state before its URL can be changed. " + "Please set readOnly=true on the image store first."); } + + String existingProtocol = imageStoreVO.getProtocol(); + validateUrl(url, existingProtocol); imageStoreVO.setUrl(url); } if (com.cloud.utils.StringUtils.isNotBlank(name)) { @@ -4239,6 +4254,26 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C return imageStoreVO; } + private void validateUrl(String url, String existingProtocol) { + try { + UriUtils.UriInfo newUriInfo = UriUtils.getUriInfo(url); + String newScheme = newUriInfo.getScheme(); + String newHost = newUriInfo.getStorageHost(); + String newPath = newUriInfo.getStoragePath(); + if (StringUtils.isAnyBlank(newScheme, newHost, newPath)) { + throw new InvalidParameterValueException("Invalid image store URL: scheme, host and path must not be empty."); + } + + if (existingProtocol != null && !existingProtocol.equalsIgnoreCase(newScheme)) { + throw new InvalidParameterValueException(String.format( + "URL scheme '%s' does not match the image store's existing protocol '%s'.", + newScheme, existingProtocol)); + } + } catch (CloudRuntimeException e) { + throw new InvalidParameterValueException("Invalid image store URL: " + e.getMessage()); + } + } + @Override public ImageStore updateImageStoreStatus(Long id, Boolean readonly) { return updateImageStoreStatus(id, null, readonly, null, null); diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 8f88800d549..be65675be47 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -30,6 +30,8 @@ import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.resource.ResourceManager; import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao; +import org.apache.cloudstack.storage.datastore.db.ImageStoreDao; +import org.apache.cloudstack.storage.datastore.db.ImageStoreVO; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.command.admin.storage.ChangeStoragePoolScopeCmd; import org.apache.cloudstack.api.command.admin.storage.ConfigureStorageAccessCmd; @@ -170,6 +172,9 @@ public class StorageManagerImplTest { @Mock DataStoreManager dataStoreMgr; + @Mock + ImageStoreDao imageStoreDao; + @Test public void createLocalStoragePoolName() { String hostMockName = "host1"; @@ -1716,4 +1721,69 @@ public class StorageManagerImplTest { storageManagerImpl.discoverObjectStore(name, url, size, providerName, details); } + + private ImageStoreVO mockImageStoreVO(String protocol, boolean readonly) { + ImageStoreVO vo = Mockito.mock(ImageStoreVO.class); + Mockito.when(vo.getProtocol()).thenReturn(protocol); + Mockito.when(vo.isReadonly()).thenReturn(readonly); + return vo; + } + + @Test(expected = InvalidParameterValueException.class) + public void testUpdateImageStoreStatus_rejectsUrlChangeWhenNotReadonly() { + long storeId = 1L; + ImageStoreVO vo = mockImageStoreVO("nfs", false); + Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo); + + storageManagerImpl.updateImageStoreStatus(storeId, null, null, null, "nfs://newhost/newpath"); + } + + @Test + public void testUpdateImageStoreStatus_allowsUrlChangeWhenAlreadyReadonly() { + long storeId = 2L; + ImageStoreVO vo = mockImageStoreVO("nfs", true); + Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo); + Mockito.when(imageStoreDao.update(Mockito.eq(storeId), Mockito.any())).thenReturn(true); + + ImageStore result = storageManagerImpl.updateImageStoreStatus(storeId, null, null, null, "nfs://newhost/newpath"); + + assertNotNull(result); + Mockito.verify(imageStoreDao).update(Mockito.eq(storeId), Mockito.any(ImageStoreVO.class)); + } + + @Test + public void testUpdateImageStoreStatus_allowsUrlChangeWhenReadonlySetInSameRequest() { + long storeId = 3L; + ImageStoreVO vo = mockImageStoreVO("nfs", false); + Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo); + Mockito.when(imageStoreDao.update(Mockito.eq(storeId), Mockito.any())).thenReturn(true); + + ImageStore result = storageManagerImpl.updateImageStoreStatus(storeId, null, Boolean.TRUE, null, "nfs://newhost/newpath"); + + assertNotNull(result); + Mockito.verify(imageStoreDao).update(Mockito.eq(storeId), Mockito.any(ImageStoreVO.class)); + } + + @Test(expected = InvalidParameterValueException.class) + public void testUpdateImageStoreStatus_rejectsMalformedUrl() { + long storeId = 4L; + ImageStoreVO vo = mockImageStoreVO("nfs", true); + Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo); + + storageManagerImpl.updateImageStoreStatus(storeId, null, null, null, "nfs://"); + } + + @Test(expected = InvalidParameterValueException.class) + public void testUpdateImageStoreStatus_rejectsMismatchedProtocol() { + long storeId = 5L; + ImageStoreVO vo = mockImageStoreVO("nfs", true); + Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo); + + storageManagerImpl.updateImageStoreStatus(storeId, null, null, null, "http://somehost/path"); + } + + @Test(expected = InvalidParameterValueException.class) + public void testExtractUriParamsAsMap_rejectsMissingHostForNfs() { + storageManagerImpl.extractUriParamsAsMap("nfs:///just-a-path"); + } }