Skip to content

Commit de095ba

Browse files
shwstpprshwstppr
authored
server: fix url check for storages without a valid url (#8353)
Fixes #8352 Some managed storages may not need a valid URL to be passed. We can skip check and extraction of host or path from url of such storages.
1 parent 0bc7fb5 commit de095ba

2 files changed

Lines changed: 116 additions & 36 deletions

File tree

‎server/src/main/java/com/cloud/storage/StorageManagerImpl.java‎

Lines changed: 67 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@
1818

1919
import static com.cloud.utils.NumbersUtil.toHumanReadableSize;
2020

21+
import java.io.UnsupportedEncodingException;
2122
import java.math.BigDecimal;
2223
import java.math.BigInteger;
2324
import java.net.URI;
2425
import java.net.URISyntaxException;
26+
import java.net.URLDecoder;
2527
import java.net.UnknownHostException;
2628
import java.nio.file.Files;
2729
import java.sql.PreparedStatement;
@@ -132,8 +134,9 @@
132134
import org.apache.cloudstack.storage.object.ObjectStoreEntity;
133135
import org.apache.cloudstack.storage.to.VolumeObjectTO;
134136
import org.apache.commons.collections.CollectionUtils;
135-
import org.apache.commons.lang3.EnumUtils;
137+
import org.apache.commons.collections.MapUtils;
136138
import org.apache.commons.lang.time.DateUtils;
139+
import org.apache.commons.lang3.EnumUtils;
137140
import org.apache.commons.lang3.StringUtils;
138141
import org.apache.log4j.Logger;
139142
import org.springframework.stereotype.Component;
@@ -254,8 +257,6 @@
254257
import com.cloud.vm.VirtualMachine.State;
255258
import com.cloud.vm.dao.VMInstanceDao;
256259
import com.google.common.collect.Sets;
257-
import java.io.UnsupportedEncodingException;
258-
import java.net.URLDecoder;
259260

260261

261262
@Component
@@ -684,12 +685,21 @@ public boolean stop() {
684685
return true;
685686
}
686687

687-
private DataStore createLocalStorage(Map<String, Object> poolInfos) throws ConnectionException{
688+
protected String getValidatedPareForLocalStorage(Object obj, String paramName) {
689+
String result = obj == null ? null : obj.toString();
690+
if (StringUtils.isEmpty(result)) {
691+
throw new InvalidParameterValueException(String.format("Invalid %s provided", paramName));
692+
}
693+
return result;
694+
}
695+
696+
protected DataStore createLocalStorage(Map<String, Object> poolInfos) throws ConnectionException{
688697
Object existingUuid = poolInfos.get("uuid");
689698
if( existingUuid == null ){
690699
poolInfos.put("uuid", UUID.randomUUID().toString());
691700
}
692-
String hostAddress = poolInfos.get("host").toString();
701+
String hostAddress = getValidatedPareForLocalStorage(poolInfos.get("host"), "host");
702+
String hostPath = getValidatedPareForLocalStorage(poolInfos.get("hostPath"), "path");
693703
Host host = _hostDao.findByName(hostAddress);
694704

695705
if( host == null ) {
@@ -708,8 +718,8 @@ private DataStore createLocalStorage(Map<String, Object> poolInfos) throws Conne
708718

709719
StoragePoolInfo pInfo = new StoragePoolInfo(poolInfos.get("uuid").toString(),
710720
host.getPrivateIpAddress(),
711-
poolInfos.get("hostPath").toString(),
712-
poolInfos.get("hostPath").toString(),
721+
hostPath,
722+
hostPath,
713723
StoragePoolType.Filesystem,
714724
capacityBytes,
715725
0,
@@ -809,6 +819,7 @@ protected String createLocalStoragePoolName(Host host, StoragePoolInfo storagePo
809819
public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws ResourceInUseException, IllegalArgumentException, UnknownHostException, ResourceUnavailableException {
810820
String providerName = cmd.getStorageProviderName();
811821
Map<String,String> uriParams = extractUriParamsAsMap(cmd.getUrl());
822+
boolean isFileScheme = "file".equals(uriParams.get("scheme"));
812823
DataStoreProvider storeProvider = _dataStoreProviderMgr.getDataStoreProvider(providerName);
813824

814825
if (storeProvider == null) {
@@ -822,7 +833,10 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource
822833
Long podId = cmd.getPodId();
823834
Long zoneId = cmd.getZoneId();
824835

825-
ScopeType scopeType = uriParams.get("scheme").toString().equals("file") ? ScopeType.HOST : ScopeType.CLUSTER;
836+
ScopeType scopeType = ScopeType.CLUSTER;
837+
if (isFileScheme) {
838+
scopeType = ScopeType.HOST;
839+
}
826840
String scope = cmd.getScope();
827841
if (scope != null) {
828842
try {
@@ -889,12 +903,14 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource
889903
params.put("managed", cmd.isManaged());
890904
params.put("capacityBytes", cmd.getCapacityBytes());
891905
params.put("capacityIops", cmd.getCapacityIops());
892-
params.putAll(uriParams);
906+
if (MapUtils.isNotEmpty(uriParams)) {
907+
params.putAll(uriParams);
908+
}
893909

894910
DataStoreLifeCycle lifeCycle = storeProvider.getDataStoreLifeCycle();
895911
DataStore store = null;
896912
try {
897-
if (params.get("scheme").toString().equals("file")) {
913+
if (isFileScheme) {
898914
store = createLocalStorage(params);
899915
} else {
900916
store = lifeCycle.initialize(params);
@@ -923,42 +939,55 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource
923939
return (PrimaryDataStoreInfo)_dataStoreMgr.getDataStore(store.getId(), DataStoreRole.Primary);
924940
}
925941

926-
private Map<String,String> extractUriParamsAsMap(String url){
942+
protected Map<String,String> extractUriParamsAsMap(String url) {
927943
Map<String,String> uriParams = new HashMap<>();
928-
UriUtils.UriInfo uriInfo = UriUtils.getUriInfo(url);
944+
UriUtils.UriInfo uriInfo;
945+
try {
946+
uriInfo = UriUtils.getUriInfo(url);
947+
} catch (CloudRuntimeException cre) {
948+
if (s_logger.isDebugEnabled()) {
949+
s_logger.debug(String.format("URI validation for url: %s failed, returning empty uri params", url));
950+
}
951+
return uriParams;
952+
}
929953

930954
String scheme = uriInfo.getScheme();
931955
String storageHost = uriInfo.getStorageHost();
932956
String storagePath = uriInfo.getStoragePath();
933-
try {
934-
if (scheme == null) {
935-
throw new InvalidParameterValueException("scheme is null " + url + ", add nfs:// (or cifs://) as a prefix");
936-
} else if (scheme.equalsIgnoreCase("nfs")) {
937-
if (storageHost == null || storagePath == null || storageHost.trim().isEmpty() || storagePath.trim().isEmpty()) {
938-
throw new InvalidParameterValueException("host or path is null, should be nfs://hostname/path");
939-
}
940-
} else if (scheme.equalsIgnoreCase("cifs")) {
941-
// Don't validate against a URI encoded URI.
957+
if (scheme == null) {
958+
if (s_logger.isDebugEnabled()) {
959+
s_logger.debug(String.format("Scheme for url: %s is not found, returning empty uri params", url));
960+
}
961+
return uriParams;
962+
}
963+
boolean isHostOrPathBlank = StringUtils.isAnyBlank(storagePath, storageHost);
964+
if (scheme.equalsIgnoreCase("nfs")) {
965+
if (isHostOrPathBlank) {
966+
throw new InvalidParameterValueException("host or path is null, should be nfs://hostname/path");
967+
}
968+
} else if (scheme.equalsIgnoreCase("cifs")) {
969+
// Don't validate against a URI encoded URI.
970+
try {
942971
URI cifsUri = new URI(url);
943972
String warnMsg = UriUtils.getCifsUriParametersProblems(cifsUri);
944973
if (warnMsg != null) {
945974
throw new InvalidParameterValueException(warnMsg);
946975
}
947-
} else if (scheme.equalsIgnoreCase("sharedMountPoint")) {
948-
if (storagePath == null) {
949-
throw new InvalidParameterValueException("host or path is null, should be sharedmountpoint://localhost/path");
950-
}
951-
} else if (scheme.equalsIgnoreCase("rbd")) {
952-
if (storagePath == null) {
953-
throw new InvalidParameterValueException("host or path is null, should be rbd://hostname/pool");
954-
}
955-
} else if (scheme.equalsIgnoreCase("gluster")) {
956-
if (storageHost == null || storagePath == null || storageHost.trim().isEmpty() || storagePath.trim().isEmpty()) {
957-
throw new InvalidParameterValueException("host or path is null, should be gluster://hostname/volume");
958-
}
976+
} catch (URISyntaxException e) {
977+
throw new InvalidParameterValueException(url + " is not a valid uri");
978+
}
979+
} else if (scheme.equalsIgnoreCase("sharedMountPoint")) {
980+
if (storagePath == null) {
981+
throw new InvalidParameterValueException("host or path is null, should be sharedmountpoint://localhost/path");
982+
}
983+
} else if (scheme.equalsIgnoreCase("rbd")) {
984+
if (storagePath == null) {
985+
throw new InvalidParameterValueException("host or path is null, should be rbd://hostname/pool");
986+
}
987+
} else if (scheme.equalsIgnoreCase("gluster")) {
988+
if (isHostOrPathBlank) {
989+
throw new InvalidParameterValueException("host or path is null, should be gluster://hostname/volume");
959990
}
960-
} catch (URISyntaxException e) {
961-
throw new InvalidParameterValueException(url + " is not a valid uri");
962991
}
963992

964993
String hostPath = null;
@@ -975,7 +1004,9 @@ private Map<String,String> extractUriParamsAsMap(String url){
9751004
uriParams.put("host", storageHost);
9761005
uriParams.put("hostPath", hostPath);
9771006
uriParams.put("userInfo", uriInfo.getUserInfo());
978-
uriParams.put("port", uriInfo.getPort() + "");
1007+
if (uriInfo.getPort() > 0) {
1008+
uriParams.put("port", uriInfo.getPort() + "");
1009+
}
9791010
return uriParams;
9801011
}
9811012

‎server/src/test/java/com/cloud/storage/StorageManagerImplTest.java‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,15 @@
1717
package com.cloud.storage;
1818

1919
import com.cloud.agent.api.StoragePoolInfo;
20+
import com.cloud.exception.ConnectionException;
21+
import com.cloud.exception.InvalidParameterValueException;
2022
import com.cloud.host.Host;
2123
import com.cloud.storage.dao.VolumeDao;
2224
import com.cloud.vm.VMInstanceVO;
2325
import com.cloud.vm.dao.VMInstanceDao;
2426
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
2527
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
28+
import org.apache.commons.collections.MapUtils;
2629
import org.junit.Assert;
2730
import org.junit.Test;
2831
import org.junit.runner.RunWith;
@@ -33,7 +36,9 @@
3336
import org.mockito.junit.MockitoJUnitRunner;
3437

3538
import java.util.ArrayList;
39+
import java.util.HashMap;
3640
import java.util.List;
41+
import java.util.Map;
3742

3843
@RunWith(MockitoJUnitRunner.class)
3944
public class StorageManagerImplTest {
@@ -157,4 +162,48 @@ public void storagePoolCompatibleWithVolumePoolTestVolumeWithoutPoolIdInAllocate
157162

158163
}
159164

165+
@Test
166+
public void testExtractUriParamsAsMapWithSolidFireUrl() {
167+
String sfUrl = "MVIP=1.2.3.4;SVIP=6.7.8.9;clusterAdminUsername=admin;" +
168+
"clusterAdminPassword=password;clusterDefaultMinIops=1000;" +
169+
"clusterDefaultMaxIops=2000;clusterDefaultBurstIopsPercentOfMaxIops=2";
170+
Map<String,String> uriParams = storageManagerImpl.extractUriParamsAsMap(sfUrl);
171+
Assert.assertTrue(MapUtils.isEmpty(uriParams));
172+
}
173+
174+
@Test
175+
public void testExtractUriParamsAsMapWithNFSUrl() {
176+
String scheme = "nfs";
177+
String host = "HOST";
178+
String path = "/PATH";
179+
String sfUrl = String.format("%s://%s%s", scheme, host, path);
180+
Map<String,String> uriParams = storageManagerImpl.extractUriParamsAsMap(sfUrl);
181+
Assert.assertTrue(MapUtils.isNotEmpty(uriParams));
182+
Assert.assertEquals(scheme, uriParams.get("scheme"));
183+
Assert.assertEquals(host, uriParams.get("host"));
184+
Assert.assertEquals(path, uriParams.get("hostPath"));
185+
}
186+
187+
@Test(expected = InvalidParameterValueException.class)
188+
public void testCreateLocalStorageHostFailure() {
189+
Map<String, Object> test = new HashMap<>();
190+
test.put("host", null);
191+
try {
192+
storageManagerImpl.createLocalStorage(test);
193+
} catch (ConnectionException e) {
194+
throw new RuntimeException(e);
195+
}
196+
}
197+
198+
@Test(expected = InvalidParameterValueException.class)
199+
public void testCreateLocalStoragePathFailure() {
200+
Map<String, Object> test = new HashMap<>();
201+
test.put("host", "HOST");
202+
test.put("hostPath", "");
203+
try {
204+
storageManagerImpl.createLocalStorage(test);
205+
} catch (ConnectionException e) {
206+
throw new RuntimeException(e);
207+
}
208+
}
160209
}

0 commit comments

Comments
 (0)