From 3315b23595629b963d40650dabe81ff0e3543057 Mon Sep 17 00:00:00 2001 From: Min Chen Date: Tue, 7 May 2013 13:34:27 -0700 Subject: [PATCH] Relax url port validation and some MockLocalNfsSecondaryStorageResource fix. --- .../storage/template/DownloadManager.java | 2 +- .../storage/template/DownloadManagerImpl.java | 30 +++++++------------ .../template/HttpTemplateDownloader.java | 4 +-- .../MockLocalNfsSecondaryStorageResource.java | 15 ++++++++++ .../storage/test/CloudStackTestNGBase.java | 15 ++++++++-- .../cloudstack/storage/test/TemplateTest.java | 1 + .../integration-test/test/resource/testng.xml | 4 ++- .../com/cloud/storage/VolumeManagerImpl.java | 18 +++++------ .../template/HypervisorTemplateAdapter.java | 4 +-- utils/src/com/cloud/utils/UriUtils.java | 4 +-- 10 files changed, 59 insertions(+), 38 deletions(-) diff --git a/core/src/com/cloud/storage/template/DownloadManager.java b/core/src/com/cloud/storage/template/DownloadManager.java index 1897afd9c00..42ffe2ac3f9 100644 --- a/core/src/com/cloud/storage/template/DownloadManager.java +++ b/core/src/com/cloud/storage/template/DownloadManager.java @@ -44,7 +44,7 @@ public interface DownloadManager extends Manager { * @param resourceType signifying the type of resource like template, volume etc. * @return job-id that can be used to interrogate the status of the download. */ - public String downloadPublicTemplate(long id, String url, String name, ImageFormat format, boolean hvm, Long accountId, String descr, String cksum, String installPathPrefix, String userName, String passwd, long maxDownloadSizeInBytes, Proxy proxy, ResourceType resourceType); + public String downloadPublicTemplate(long id, String url, String name, ImageFormat format, boolean hvm, Long accountId, String descr, String cksum, String installPathPrefix, String templatePath, String userName, String passwd, long maxDownloadSizeInBytes, Proxy proxy, ResourceType resourceType); public String downloadS3Template(S3TO s3, long id, String url, String name, ImageFormat format, boolean hvm, Long accountId, String descr, String cksum, String installPathPrefix, String user, String password, long maxTemplateSizeInBytes, Proxy proxy, ResourceType resourceType); diff --git a/core/src/com/cloud/storage/template/DownloadManagerImpl.java b/core/src/com/cloud/storage/template/DownloadManagerImpl.java index 53810e9083c..7a69dab3704 100755 --- a/core/src/com/cloud/storage/template/DownloadManagerImpl.java +++ b/core/src/com/cloud/storage/template/DownloadManagerImpl.java @@ -16,15 +16,9 @@ // under the License. package com.cloud.storage.template; -import static com.cloud.utils.S3Utils.putDirectory; -import static com.cloud.utils.StringUtils.join; -import static java.lang.String.format; -import static java.util.Arrays.asList; - import java.io.BufferedReader; import java.io.File; import java.io.FileInputStream; -import java.io.FilenameFilter; import java.io.IOException; import java.io.InputStream; import java.math.BigInteger; @@ -51,14 +45,8 @@ import org.apache.cloudstack.storage.command.DownloadCommand; import org.apache.cloudstack.storage.command.DownloadProgressCommand; import org.apache.cloudstack.storage.command.DownloadCommand.ResourceType; import org.apache.cloudstack.storage.command.DownloadProgressCommand.RequestType; -import org.apache.commons.httpclient.Credentials; -import org.apache.commons.httpclient.HttpClient; -import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager; -import org.apache.commons.httpclient.UsernamePasswordCredentials; -import org.apache.commons.httpclient.auth.AuthScope; import org.apache.log4j.Logger; -import com.cloud.agent.api.Answer; import com.cloud.agent.api.storage.DownloadAnswer; import com.cloud.agent.api.storage.Proxy; import com.cloud.agent.api.to.DataStoreTO; @@ -74,9 +62,6 @@ import com.cloud.storage.template.Processor.FormatInfo; import com.cloud.storage.template.TemplateDownloader.DownloadCompleteCallback; import com.cloud.storage.template.TemplateDownloader.Status; import com.cloud.utils.NumbersUtil; -import com.cloud.utils.S3Utils; -import com.cloud.utils.UriUtils; -import com.cloud.utils.S3Utils.ObjectNamingStrategy; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.script.OutputInterpreter; @@ -348,9 +333,11 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager private String postDownload(String jobId) { DownloadJob dnld = jobs.get(jobId); TemplateDownloader td = dnld.getTemplateDownloader(); - String resourcePath = null; + String resourcePath = dnld.getInstallPathPrefix(); // path with mount directory + String finalResourcePath = dnld.getTmpltPath(); // template download path on secondary storage ResourceType resourceType = dnld.getResourceType(); + /* // once template path is set, remove the parent dir so that the template // is installed with a relative path String finalResourcePath = ""; @@ -364,6 +351,7 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager _storage.mkdirs(resourcePath); dnld.setTmpltPath(finalResourcePath); + */ File originalTemplate = new File(td.getDownloadLocalPath()); String checkSum = computeCheckSum(originalTemplate); @@ -396,12 +384,13 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager templateName = java.util.UUID.nameUUIDFromBytes((jobs.get(jobId).getTmpltName() + System.currentTimeMillis()).getBytes()).toString(); } + // run script to mv the temporary template file to the final template file String templateFilename = templateName + "." + extension; dnld.setTmpltPath(finalResourcePath + "/" + templateFilename); scr.add("-n", templateFilename); scr.add("-t", resourcePath); - scr.add("-f", td.getDownloadLocalPath()); + scr.add("-f", td.getDownloadLocalPath()); // this is the temporary template file downloaded if (dnld.getChecksum() != null && dnld.getChecksum().length() > 1) { scr.add("-c", dnld.getChecksum()); } @@ -507,7 +496,7 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager @Override public String downloadPublicTemplate(long id, String url, String name, ImageFormat format, boolean hvm, Long accountId, String descr, - String cksum, String installPathPrefix, String user, String password, long maxTemplateSizeInBytes, Proxy proxy, ResourceType resourceType) { + String cksum, String installPathPrefix, String templatePath, String user, String password, long maxTemplateSizeInBytes, Proxy proxy, ResourceType resourceType) { UUID uuid = UUID.randomUUID(); String jobId = uuid.toString(); String tmpDir = installPathPrefix; @@ -555,7 +544,10 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager } else { throw new CloudRuntimeException("Unable to download from URL: " + url); } + // NOTE the difference between installPathPrefix and templatePath here. instalPathPrefix is the absolute path for template including mount directory + // on ssvm, while templatePath is the final relative path on secondary storage. DownloadJob dj = new DownloadJob(td, jobId, id, name, format, hvm, accountId, descr, cksum, installPathPrefix, resourceType); + dj.setTmpltPath(templatePath); jobs.put(jobId, dj); threadPool.execute(td); @@ -686,7 +678,7 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager cmd.getDescription(), cmd.getChecksum(), installPathPrefix, user, password, maxDownloadSizeInBytes, cmd.getProxy(), resourceType); } else { jobId = downloadPublicTemplate(cmd.getId(), cmd.getUrl(), cmd.getName(), cmd.getFormat(), cmd.isHvm(), cmd.getAccountId(), - cmd.getDescription(), cmd.getChecksum(), installPathPrefix, user, password, maxDownloadSizeInBytes, cmd.getProxy(), resourceType); + cmd.getDescription(), cmd.getChecksum(), installPathPrefix, cmd.getInstallPath(), user, password, maxDownloadSizeInBytes, cmd.getProxy(), resourceType); } sleep(); if (jobId == null) { diff --git a/core/src/com/cloud/storage/template/HttpTemplateDownloader.java b/core/src/com/cloud/storage/template/HttpTemplateDownloader.java index 56da8c0e7d1..e832c0291e5 100644 --- a/core/src/com/cloud/storage/template/HttpTemplateDownloader.java +++ b/core/src/com/cloud/storage/template/HttpTemplateDownloader.java @@ -166,8 +166,8 @@ public class HttpTemplateDownloader implements TemplateDownloader { throw new IllegalArgumentException("Unsupported scheme for url"); } int port = uri.getPort(); - if (!(port == 80 || port == 443 || port == -1)) { - throw new IllegalArgumentException("Only ports 80 and 443 are allowed"); + if (!(port == 80 || port == 8080 || port == 443 || port == -1)) { + throw new IllegalArgumentException("Only ports 80, 8080 and 443 are allowed"); } if (port == -1 && uri.getScheme().equalsIgnoreCase("https")) { diff --git a/engine/storage/integration-test/test/org/apache/cloudstack/storage/MockLocalNfsSecondaryStorageResource.java b/engine/storage/integration-test/test/org/apache/cloudstack/storage/MockLocalNfsSecondaryStorageResource.java index 60dc16127fa..0a460466036 100644 --- a/engine/storage/integration-test/test/org/apache/cloudstack/storage/MockLocalNfsSecondaryStorageResource.java +++ b/engine/storage/integration-test/test/org/apache/cloudstack/storage/MockLocalNfsSecondaryStorageResource.java @@ -8,9 +8,12 @@ import java.net.MalformedURLException; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; +import java.util.HashMap; import java.util.List; import java.util.concurrent.Executors; +import javax.naming.ConfigurationException; + import org.apache.cloudstack.storage.command.DownloadSystemTemplateCommand; import org.springframework.stereotype.Component; @@ -24,6 +27,7 @@ import com.cloud.agent.api.to.NfsTO; import com.cloud.agent.api.to.S3TO; import com.cloud.agent.api.to.SwiftTO; import com.cloud.storage.JavaStorageLayer; +import com.cloud.storage.StorageLayer; import com.cloud.storage.VMTemplateStorageResourceAssoc.Status; import com.cloud.storage.resource.NfsSecondaryStorageResource; import com.cloud.storage.template.DownloadManagerImpl; @@ -37,10 +41,21 @@ public class MockLocalNfsSecondaryStorageResource extends public MockLocalNfsSecondaryStorageResource(){ _dlMgr = new DownloadManagerImpl(); + _storage = new JavaStorageLayer(); + HashMap params = new HashMap(); + params.put(StorageLayer.InstanceConfigKey, _storage); + try { + _dlMgr.configure("downloadMgr", params); + } catch (ConfigurationException e) { + // TODO Auto-generated catch block + e.printStackTrace(); + } + /* _storage = new JavaStorageLayer(); ((DownloadManagerImpl)_dlMgr).setThreadPool(Executors.newFixedThreadPool(10)); ((DownloadManagerImpl)_dlMgr).setStorageLayer(_storage); + */ } diff --git a/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/CloudStackTestNGBase.java b/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/CloudStackTestNGBase.java index 4d600785526..8bf14f60eac 100644 --- a/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/CloudStackTestNGBase.java +++ b/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/CloudStackTestNGBase.java @@ -37,6 +37,7 @@ public class CloudStackTestNGBase extends AbstractTestNGSpringContextTests { private String primaryStorageUrl; private String secondaryStorage; private String imageInstallPath; + private String scriptPath; private Transaction txn; private String s3AccessKey; @@ -71,11 +72,11 @@ public class CloudStackTestNGBase extends AbstractTestNGSpringContextTests { @Parameters({"devcloud-host-uuid", "devcloud-host-gateway", "devcloud-host-cidr", "devcloud-host-ip", "template-url", "devcloud-local-storage-uuid", - "primary-storage-want-to-add", "devcloud-secondary-storage", "s3-accesskey", "s3-secretkey", "s3-endpoint", "s3-template-bucket", "s3-usehttps", "image-install-path", "primary-storage-uuid-want-to-add"}) + "primary-storage-want-to-add", "devcloud-secondary-storage", "s3-accesskey", "s3-secretkey", "s3-endpoint", "s3-template-bucket", "s3-usehttps", "image-install-path", "primary-storage-uuid-want-to-add", "script-path"}) protected void setup(String hostuuid, String gateway, String cidr, String hostIp, String templateUrl, String localStorageUuid, String primaryStorage, String secondaryStorage, String s3_accessKey, String s3_secretKey, String s3_endpoint, String s3_template_bucket, - String s3_usehttps, String imageInstallPath, String primaryStorageUuid) { + String s3_usehttps, String imageInstallPath, String primaryStorageUuid, String scriptPath) { this.hostGuid = hostuuid; this.hostGateway = gateway; this.hostCidr = cidr; @@ -92,6 +93,7 @@ public class CloudStackTestNGBase extends AbstractTestNGSpringContextTests { this.s3EndPoint = s3_endpoint; this.s3TemplateBucket = s3_template_bucket; this.s3UseHttps = Boolean.parseBoolean(s3_usehttps); + this.scriptPath = scriptPath; } protected String getHostGuid() { @@ -167,4 +169,13 @@ public class CloudStackTestNGBase extends AbstractTestNGSpringContextTests { this.primaryStorageUuid = primaryStorageUuid; } + public String getScriptPath() { + return scriptPath; + } + + public void setScriptPath(String scriptPath) { + this.scriptPath = scriptPath; + } + + } diff --git a/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/TemplateTest.java b/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/TemplateTest.java index a38b91c3252..3ef1b542743 100644 --- a/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/TemplateTest.java +++ b/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/TemplateTest.java @@ -68,6 +68,7 @@ public class TemplateTest extends CloudStackTestNGBase { @Test(priority = -1) public void setUp() { ComponentContext.initComponentsLifeCycle(); + System.setProperty("paths.script", this.getScriptPath()); //create data center DataCenterVO dc = new DataCenterVO(UUID.randomUUID().toString(), "test", "8.8.8.8", null, "10.0.0.1", null, "10.0.0.1/24", null, null, NetworkType.Basic, null, null, true, true, null, null); diff --git a/engine/storage/integration-test/test/resource/testng.xml b/engine/storage/integration-test/test/resource/testng.xml index 4b074487b4c..a6dc392caa7 100644 --- a/engine/storage/integration-test/test/resource/testng.xml +++ b/engine/storage/integration-test/test/resource/testng.xml @@ -23,12 +23,14 @@ - + + + diff --git a/server/src/com/cloud/storage/VolumeManagerImpl.java b/server/src/com/cloud/storage/VolumeManagerImpl.java index 39d3ce20c16..828f34a71aa 100644 --- a/server/src/com/cloud/storage/VolumeManagerImpl.java +++ b/server/src/com/cloud/storage/VolumeManagerImpl.java @@ -2380,9 +2380,9 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { } int port = uri.getPort(); - if (!(port == 80 || port == 443 || port == -1)) { + if (!(port == 80 || port == 8080 || port == 443 || port == -1)) { throw new IllegalArgumentException( - "Only ports 80 and 443 are allowed"); + "Only ports 80, 8080 and 443 are allowed"); } String host = uri.getHost(); try { @@ -2468,21 +2468,21 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { throw new CloudRuntimeException("Failed to destroy volume" + volume.getId(), e); } } - - - - + + + + @Override public Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account account) throws ResourceAllocationException { VolumeInfo volume = this.volFactory.getVolume(volumeId); if (volume == null) { throw new InvalidParameterValueException("Creating snapshot failed due to volume:" + volumeId + " doesn't exist"); } - + if (volume.getState() != Volume.State.Ready) { throw new InvalidParameterValueException("VolumeId: " + volumeId + " is not in " + Volume.State.Ready + " state but " + volume.getState() + ". Cannot take snapshot."); } - + CreateSnapshotPayload payload = new CreateSnapshotPayload(); payload.setSnapshotId(snapshotId); payload.setSnapshotPolicyId(policyId); @@ -2523,7 +2523,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { if (storagePool == null) { throw new InvalidParameterValueException("VolumeId: " + volumeId + " please attach this volume to a VM before create snapshot for it"); } - + return this.snapshotMgr.allocSnapshot(volumeId, policyId); } diff --git a/server/src/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/com/cloud/template/HypervisorTemplateAdapter.java index 35bc5e5d678..2e50406debb 100755 --- a/server/src/com/cloud/template/HypervisorTemplateAdapter.java +++ b/server/src/com/cloud/template/HypervisorTemplateAdapter.java @@ -86,8 +86,8 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase { } int port = uri.getPort(); - if (!(port == 80 || port == 443 || port == -1)) { - throw new IllegalArgumentException("Only ports 80 and 443 are allowed"); + if (!(port == 80 || port == 8080 || port == 443 || port == -1)) { + throw new IllegalArgumentException("Only ports 80, 8080 and 443 are allowed"); } String host = uri.getHost(); try { diff --git a/utils/src/com/cloud/utils/UriUtils.java b/utils/src/com/cloud/utils/UriUtils.java index ef1aba24a00..2673177ad96 100644 --- a/utils/src/com/cloud/utils/UriUtils.java +++ b/utils/src/com/cloud/utils/UriUtils.java @@ -137,8 +137,8 @@ public class UriUtils { throw new IllegalArgumentException("Unsupported scheme for url"); } int port = uri.getPort(); - if (!(port == 80 || port == 443 || port == -1)) { - throw new IllegalArgumentException("Only ports 80 and 443 are allowed"); + if (!(port == 80 || port == 8080 || port == 443 || port == -1)) { + throw new IllegalArgumentException("Only ports 80, 8080 and 443 are allowed"); } if (port == -1 && uri.getScheme().equalsIgnoreCase("https")) {