You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by sh...@apache.org on 2023/12/15 10:55:35 UTC
(cloudstack) branch main updated: server: fix url check for storages without a valid url (#8353)
This is an automated email from the ASF dual-hosted git repository.
shwstppr pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/main by this push:
new de095ba70d2 server: fix url check for storages without a valid url (#8353)
de095ba70d2 is described below
commit de095ba70d2a2261ffb2db7f3b8f29544939028e
Author: Abhishek Kumar <ab...@gmail.com>
AuthorDate: Fri Dec 15 16:25:28 2023 +0530
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.
---
.../java/com/cloud/storage/StorageManagerImpl.java | 103 ++++++++++++++-------
.../com/cloud/storage/StorageManagerImplTest.java | 49 ++++++++++
2 files changed, 116 insertions(+), 36 deletions(-)
diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java
index 4bc471c5084..04889d0b2a8 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 @@ package com.cloud.storage;
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.ObjectStore;
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.VMInstanceVO;
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 class StorageManagerImpl extends ManagerBase implements StorageManager, C
return true;
}
- private DataStore createLocalStorage(Map<String, Object> 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<String, Object> 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 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C
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 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C
public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws ResourceInUseException, IllegalArgumentException, UnknownHostException, ResourceUnavailableException {
String providerName = cmd.getStorageProviderName();
Map<String,String> uriParams = extractUriParamsAsMap(cmd.getUrl());
+ boolean isFileScheme = "file".equals(uriParams.get("scheme"));
DataStoreProvider storeProvider = _dataStoreProviderMgr.getDataStoreProvider(providerName);
if (storeProvider == null) {
@@ -822,7 +833,10 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C
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 class StorageManagerImpl extends ManagerBase implements StorageManager, C
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 class StorageManagerImpl extends ManagerBase implements StorageManager, C
return (PrimaryDataStoreInfo)_dataStoreMgr.getDataStore(store.getId(), DataStoreRole.Primary);
}
- private Map<String,String> extractUriParamsAsMap(String url){
+ protected Map<String,String> extractUriParamsAsMap(String url) {
Map<String,String> 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 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C
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 478547bff1e..ba5f2baf932 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.Spy;
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 class StorageManagerImplTest {
}
+ @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<String,String> 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<String,String> 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<String, Object> 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<String, Object> test = new HashMap<>();
+ test.put("host", "HOST");
+ test.put("hostPath", "");
+ try {
+ storageManagerImpl.createLocalStorage(test);
+ } catch (ConnectionException e) {
+ throw new RuntimeException(e);
+ }
+ }
}