diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 4bc471c5084c..04889d0b2a83 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -18,10 +18,12 @@ import static com.cloud.utils.NumbersUtil.toHumanReadableSize; +import java.io.UnsupportedEncodingException; import java.math.BigDecimal; import java.math.BigInteger; import java.net.URI; import java.net.URISyntaxException; +import java.net.URLDecoder; import java.net.UnknownHostException; import java.nio.file.Files; import java.sql.PreparedStatement; @@ -132,8 +134,9 @@ import org.apache.cloudstack.storage.object.ObjectStoreEntity; import org.apache.cloudstack.storage.to.VolumeObjectTO; import org.apache.commons.collections.CollectionUtils; -import org.apache.commons.lang3.EnumUtils; +import org.apache.commons.collections.MapUtils; import org.apache.commons.lang.time.DateUtils; +import org.apache.commons.lang3.EnumUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -254,8 +257,6 @@ import com.cloud.vm.VirtualMachine.State; import com.cloud.vm.dao.VMInstanceDao; import com.google.common.collect.Sets; -import java.io.UnsupportedEncodingException; -import java.net.URLDecoder; @Component @@ -684,12 +685,21 @@ public boolean stop() { return true; } - private DataStore createLocalStorage(Map poolInfos) throws ConnectionException{ + protected String getValidatedPareForLocalStorage(Object obj, String paramName) { + String result = obj == null ? null : obj.toString(); + if (StringUtils.isEmpty(result)) { + throw new InvalidParameterValueException(String.format("Invalid %s provided", paramName)); + } + return result; + } + + protected DataStore createLocalStorage(Map poolInfos) throws ConnectionException{ Object existingUuid = poolInfos.get("uuid"); if( existingUuid == null ){ poolInfos.put("uuid", UUID.randomUUID().toString()); } - String hostAddress = poolInfos.get("host").toString(); + String hostAddress = getValidatedPareForLocalStorage(poolInfos.get("host"), "host"); + String hostPath = getValidatedPareForLocalStorage(poolInfos.get("hostPath"), "path"); Host host = _hostDao.findByName(hostAddress); if( host == null ) { @@ -708,8 +718,8 @@ private DataStore createLocalStorage(Map poolInfos) throws Conne StoragePoolInfo pInfo = new StoragePoolInfo(poolInfos.get("uuid").toString(), host.getPrivateIpAddress(), - poolInfos.get("hostPath").toString(), - poolInfos.get("hostPath").toString(), + hostPath, + hostPath, StoragePoolType.Filesystem, capacityBytes, 0, @@ -809,6 +819,7 @@ protected String createLocalStoragePoolName(Host host, StoragePoolInfo storagePo public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws ResourceInUseException, IllegalArgumentException, UnknownHostException, ResourceUnavailableException { String providerName = cmd.getStorageProviderName(); Map uriParams = extractUriParamsAsMap(cmd.getUrl()); + boolean isFileScheme = "file".equals(uriParams.get("scheme")); DataStoreProvider storeProvider = _dataStoreProviderMgr.getDataStoreProvider(providerName); if (storeProvider == null) { @@ -822,7 +833,10 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource Long podId = cmd.getPodId(); Long zoneId = cmd.getZoneId(); - ScopeType scopeType = uriParams.get("scheme").toString().equals("file") ? ScopeType.HOST : ScopeType.CLUSTER; + ScopeType scopeType = ScopeType.CLUSTER; + if (isFileScheme) { + scopeType = ScopeType.HOST; + } String scope = cmd.getScope(); if (scope != null) { try { @@ -889,12 +903,14 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource params.put("managed", cmd.isManaged()); params.put("capacityBytes", cmd.getCapacityBytes()); params.put("capacityIops", cmd.getCapacityIops()); - params.putAll(uriParams); + if (MapUtils.isNotEmpty(uriParams)) { + params.putAll(uriParams); + } DataStoreLifeCycle lifeCycle = storeProvider.getDataStoreLifeCycle(); DataStore store = null; try { - if (params.get("scheme").toString().equals("file")) { + if (isFileScheme) { store = createLocalStorage(params); } else { store = lifeCycle.initialize(params); @@ -923,42 +939,55 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource return (PrimaryDataStoreInfo)_dataStoreMgr.getDataStore(store.getId(), DataStoreRole.Primary); } - private Map extractUriParamsAsMap(String url){ + protected Map extractUriParamsAsMap(String url) { Map uriParams = new HashMap<>(); - UriUtils.UriInfo uriInfo = UriUtils.getUriInfo(url); + UriUtils.UriInfo uriInfo; + try { + uriInfo = UriUtils.getUriInfo(url); + } catch (CloudRuntimeException cre) { + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("URI validation for url: %s failed, returning empty uri params", url)); + } + return uriParams; + } String scheme = uriInfo.getScheme(); String storageHost = uriInfo.getStorageHost(); String storagePath = uriInfo.getStoragePath(); - try { - if (scheme == null) { - throw new InvalidParameterValueException("scheme is null " + url + ", add nfs:// (or cifs://) as a prefix"); - } else if (scheme.equalsIgnoreCase("nfs")) { - if (storageHost == null || storagePath == null || storageHost.trim().isEmpty() || storagePath.trim().isEmpty()) { - throw new InvalidParameterValueException("host or path is null, should be nfs://hostname/path"); - } - } else if (scheme.equalsIgnoreCase("cifs")) { - // Don't validate against a URI encoded URI. + if (scheme == null) { + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Scheme for url: %s is not found, returning empty uri params", url)); + } + return uriParams; + } + boolean isHostOrPathBlank = StringUtils.isAnyBlank(storagePath, storageHost); + if (scheme.equalsIgnoreCase("nfs")) { + if (isHostOrPathBlank) { + throw new InvalidParameterValueException("host or path is null, should be nfs://hostname/path"); + } + } else if (scheme.equalsIgnoreCase("cifs")) { + // Don't validate against a URI encoded URI. + try { URI cifsUri = new URI(url); String warnMsg = UriUtils.getCifsUriParametersProblems(cifsUri); if (warnMsg != null) { throw new InvalidParameterValueException(warnMsg); } - } else if (scheme.equalsIgnoreCase("sharedMountPoint")) { - if (storagePath == null) { - throw new InvalidParameterValueException("host or path is null, should be sharedmountpoint://localhost/path"); - } - } else if (scheme.equalsIgnoreCase("rbd")) { - if (storagePath == null) { - throw new InvalidParameterValueException("host or path is null, should be rbd://hostname/pool"); - } - } else if (scheme.equalsIgnoreCase("gluster")) { - if (storageHost == null || storagePath == null || storageHost.trim().isEmpty() || storagePath.trim().isEmpty()) { - throw new InvalidParameterValueException("host or path is null, should be gluster://hostname/volume"); - } + } catch (URISyntaxException e) { + throw new InvalidParameterValueException(url + " is not a valid uri"); + } + } else if (scheme.equalsIgnoreCase("sharedMountPoint")) { + if (storagePath == null) { + throw new InvalidParameterValueException("host or path is null, should be sharedmountpoint://localhost/path"); + } + } else if (scheme.equalsIgnoreCase("rbd")) { + if (storagePath == null) { + throw new InvalidParameterValueException("host or path is null, should be rbd://hostname/pool"); + } + } else if (scheme.equalsIgnoreCase("gluster")) { + if (isHostOrPathBlank) { + throw new InvalidParameterValueException("host or path is null, should be gluster://hostname/volume"); } - } catch (URISyntaxException e) { - throw new InvalidParameterValueException(url + " is not a valid uri"); } String hostPath = null; @@ -975,7 +1004,9 @@ private Map extractUriParamsAsMap(String url){ uriParams.put("host", storageHost); uriParams.put("hostPath", hostPath); uriParams.put("userInfo", uriInfo.getUserInfo()); - uriParams.put("port", uriInfo.getPort() + ""); + if (uriInfo.getPort() > 0) { + uriParams.put("port", uriInfo.getPort() + ""); + } return uriParams; } diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 478547bff1ef..ba5f2baf9327 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -17,12 +17,15 @@ package com.cloud.storage; import com.cloud.agent.api.StoragePoolInfo; +import com.cloud.exception.ConnectionException; +import com.cloud.exception.InvalidParameterValueException; import com.cloud.host.Host; import com.cloud.storage.dao.VolumeDao; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.dao.VMInstanceDao; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.commons.collections.MapUtils; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -33,7 +36,9 @@ import org.mockito.junit.MockitoJUnitRunner; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; @RunWith(MockitoJUnitRunner.class) public class StorageManagerImplTest { @@ -157,4 +162,48 @@ public void storagePoolCompatibleWithVolumePoolTestVolumeWithoutPoolIdInAllocate } + @Test + public void testExtractUriParamsAsMapWithSolidFireUrl() { + String sfUrl = "MVIP=1.2.3.4;SVIP=6.7.8.9;clusterAdminUsername=admin;" + + "clusterAdminPassword=password;clusterDefaultMinIops=1000;" + + "clusterDefaultMaxIops=2000;clusterDefaultBurstIopsPercentOfMaxIops=2"; + Map uriParams = storageManagerImpl.extractUriParamsAsMap(sfUrl); + Assert.assertTrue(MapUtils.isEmpty(uriParams)); + } + + @Test + public void testExtractUriParamsAsMapWithNFSUrl() { + String scheme = "nfs"; + String host = "HOST"; + String path = "/PATH"; + String sfUrl = String.format("%s://%s%s", scheme, host, path); + Map uriParams = storageManagerImpl.extractUriParamsAsMap(sfUrl); + Assert.assertTrue(MapUtils.isNotEmpty(uriParams)); + Assert.assertEquals(scheme, uriParams.get("scheme")); + Assert.assertEquals(host, uriParams.get("host")); + Assert.assertEquals(path, uriParams.get("hostPath")); + } + + @Test(expected = InvalidParameterValueException.class) + public void testCreateLocalStorageHostFailure() { + Map test = new HashMap<>(); + test.put("host", null); + try { + storageManagerImpl.createLocalStorage(test); + } catch (ConnectionException e) { + throw new RuntimeException(e); + } + } + + @Test(expected = InvalidParameterValueException.class) + public void testCreateLocalStoragePathFailure() { + Map test = new HashMap<>(); + test.put("host", "HOST"); + test.put("hostPath", ""); + try { + storageManagerImpl.createLocalStorage(test); + } catch (ConnectionException e) { + throw new RuntimeException(e); + } + } }