From 118f4d825f70c37908a77b600d7b0ebf11f72c03 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 6 Jun 2019 13:28:25 +0530 Subject: [PATCH 01/13] template: pass empty checksum for template copy Since checksum value stored for source template while copying is for the downloaded file and not the installed template, an empty value is passed to download service to prevent checsum mismatch. Signed-off-by: Abhishek Kumar --- .../apache/cloudstack/storage/image/TemplateServiceImpl.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java index 097b85477daf..189b88f669a8 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java @@ -1045,6 +1045,8 @@ public AsyncCallFuture copyTemplate(TemplateInfo srcTemplate, } tmplForCopy.setUrl(url); + tmplForCopy.getImage().setChecksum(""); + if (s_logger.isDebugEnabled()) { s_logger.debug("Mark template_store_ref entry as Creating"); } From 54a3c9d4777824de5db159500fba00bccbba219f Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 15 Jan 2020 17:57:59 +0100 Subject: [PATCH 02/13] blank lines --- .../src/main/java/com/cloud/template/TemplateManagerImpl.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 646d2c3cd55a..93905fc0f3fb 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -927,8 +927,6 @@ public VirtualMachineTemplate copyTemplate(CopyTemplateCmd cmd) throws StorageUn } } } - - } if ((destZoneIds != null) && (destZoneIds.size() > failedZones.size())){ From a7ca76f713358f810b87577688d689b6f996b042 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 15 Jan 2020 18:04:06 +0100 Subject: [PATCH 03/13] SHA512 as default when no digest is present --- .../org/apache/cloudstack/utils/security/ChecksumValue.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/utils/src/main/java/org/apache/cloudstack/utils/security/ChecksumValue.java b/utils/src/main/java/org/apache/cloudstack/utils/security/ChecksumValue.java index e47bf393db28..43d92b651190 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/security/ChecksumValue.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/security/ChecksumValue.java @@ -80,7 +80,8 @@ private static String algorithmFromDigest(String digest) { if (s == 0 && e > s+1) { // we have an algorithm name of at least 1 char return digest.substring(s+1,e); } // else if no algoritm + return "MD5"; } // or if no digest at all - return "MD5"; + return "SHA512"; } } From 901b66c0e452748202102d94bab05c0dd1163940 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 15 Jan 2020 19:44:04 +0100 Subject: [PATCH 04/13] refactor postDownload to conditionally check checksum --- .../storage/template/DownloadManagerImpl.java | 111 ++++++++++++------ 1 file changed, 76 insertions(+), 35 deletions(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java index bd5c59131b6a..1ae84e63a528 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java @@ -371,6 +371,7 @@ private String postRemoteDownload(String jobId) { * Post local download activity (install and cleanup). Executed in context of * downloader thread * + * @return an error message describing why download failed or {code}null{code} on success * @throws IOException */ private String postLocalDownload(String jobId) { @@ -384,23 +385,56 @@ private String postLocalDownload(String jobId) { ResourceType resourceType = dnld.getResourceType(); File originalTemplate = new File(td.getDownloadLocalPath()); - ChecksumValue oldValue = new ChecksumValue(dnld.getChecksum()); - ChecksumValue newValue = null; - try { - newValue = computeCheckSum(oldValue.getAlgorithm(), originalTemplate); - } catch (NoSuchAlgorithmException e) { - return "checksum algorithm not recognised: " + oldValue.getAlgorithm(); + if(StringUtils.isNotBlank(dnld.getChecksum())) { + // we have a checksum so check: + String checksumErrorMessage = doTheChecksum(dnld, originalTemplate); + if (checksumErrorMessage != null) { + return checksumErrorMessage; + } } - if(StringUtils.isNotBlank(dnld.getChecksum()) && ! oldValue.equals(newValue)) { - return "checksum \"" + newValue +"\" didn't match the given value, \"" + oldValue + "\""; + + String result; + String extension = dnld.getFormat().getFileExtension(); + String templateName = makeTemplatename(jobId, extension); + String templateFilename = templateName + "." + extension; + + result = executeCreateScript(dnld, td, resourcePath, finalResourcePath, resourceType, templateFilename); + if (result != null) { + return result; } - String checksum = newValue.getChecksum(); - if (checksum == null) { - s_logger.warn("Something wrong happened when try to calculate the checksum of downloaded template!"); + + // Set permissions for the downloaded template + File downloadedTemplate = new File(resourcePath + "/" + templateFilename); + + _storage.setWorldReadableAndWriteable(downloadedTemplate); + setPermissionsForTheDownloadedTemplate(dnld, resourcePath, resourceType); + + TemplateLocation loc = new TemplateLocation(_storage, resourcePath); + try { + loc.create(dnld.getId(), true, dnld.getTmpltName()); + } catch (IOException e) { + s_logger.warn("Something is wrong with template location " + resourcePath, e); + loc.purge(); + return "Unable to download due to " + e.getMessage(); } - dnld.setCheckSum(checksum); + result = postProcessAfterDownloadComplete(dnld, resourcePath, templateName, loc); + if (result != null) { + return result; + } + + // if we don't have a checksum let's create one now + if(StringUtils.isBlank(dnld.getChecksum())) { + String checksumErrorMessage = doTheChecksum(dnld, originalTemplate); + if (checksumErrorMessage != null) { + return checksumErrorMessage; + } + } + return null; + } + private String executeCreateScript(DownloadJob dnld, TemplateDownloader td, String resourcePath, String finalResourcePath, ResourceType resourceType, String templateFilename) { + String result; int imgSizeGigs = (int)Math.ceil(_storage.getSize(td.getDownloadLocalPath()) * 1.0d / (1024 * 1024 * 1024)); imgSizeGigs++; // add one just in case long timeout = (long)imgSizeGigs * installTimeoutPerGig; @@ -416,35 +450,30 @@ private String postLocalDownload(String jobId) { scr.add("-h"); } - // add options common to ISO and template - String extension = dnld.getFormat().getFileExtension(); - String templateName = ""; - if (extension.equals("iso")) { - templateName = jobs.get(jobId).getTmpltName().trim().replace(" ", "_"); - } else { - templateName = java.util.UUID.nameUUIDFromBytes((jobs.get(jobId).getTmpltName() + System.currentTimeMillis()).getBytes(StringUtils.getPreferredCharset())).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()); // this is the temporary template file downloaded scr.add("-u"); // cleanup - String result; result = scr.execute(); + return result; + } - if (result != null) { - return result; + private String makeTemplatename(String jobId, String extension) { + // add options common to ISO and template + String templateName = ""; + if (extension.equals("iso")) { + templateName = jobs.get(jobId).getTmpltName().trim().replace(" ", "_"); + } else { + templateName = UUID.nameUUIDFromBytes((jobs.get(jobId).getTmpltName() + System.currentTimeMillis()).getBytes(StringUtils.getPreferredCharset())).toString(); } + return templateName; + } - // Set permissions for the downloaded template - File downloadedTemplate = new File(resourcePath + "/" + templateFilename); - _storage.setWorldReadableAndWriteable(downloadedTemplate); - + private void setPermissionsForTheDownloadedTemplate(DownloadJob dnld, String resourcePath, ResourceType resourceType) { // Set permissions for template/volume.properties String propertiesFile = resourcePath; if (resourceType == ResourceType.TEMPLATE) { @@ -454,16 +483,28 @@ private String postLocalDownload(String jobId) { } File templateProperties = new File(propertiesFile); _storage.setWorldReadableAndWriteable(templateProperties); + } - TemplateLocation loc = new TemplateLocation(_storage, resourcePath); + private String doTheChecksum(DownloadJob dnld, File originalTemplate) { + ChecksumValue oldValue = new ChecksumValue(dnld.getChecksum()); + ChecksumValue newValue = null; try { - loc.create(dnld.getId(), true, dnld.getTmpltName()); - } catch (IOException e) { - s_logger.warn("Something is wrong with template location " + resourcePath, e); - loc.purge(); - return "Unable to download due to " + e.getMessage(); + newValue = computeCheckSum(oldValue.getAlgorithm(), originalTemplate); + } catch (NoSuchAlgorithmException e) { + return "checksum algorithm not recognised: " + oldValue.getAlgorithm(); + } + if (StringUtils.isNotBlank(dnld.getChecksum()) && !oldValue.equals(newValue)) { + return "checksum \"" + newValue + "\" didn't match the given value, \"" + oldValue + "\""; + } + String checksum = newValue.getChecksum(); + if (checksum == null) { + s_logger.warn("Something wrong happened when try to calculate the checksum of downloaded template!"); } + dnld.setCheckSum(checksum); + return null; + } + private String postProcessAfterDownloadComplete(DownloadJob dnld, String resourcePath, String templateName, TemplateLocation loc) { Iterator en = _processors.values().iterator(); while (en.hasNext()) { Processor processor = en.next(); From 54b4cb88c43f5933a15c0905a75299362e732b03 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 16 Jan 2020 10:23:11 +0100 Subject: [PATCH 05/13] get the chksm from the right file --- .../storage/template/DownloadManagerImpl.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java index 1ae84e63a528..29fec8ed28e1 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java @@ -391,6 +391,10 @@ private String postLocalDownload(String jobId) { if (checksumErrorMessage != null) { return checksumErrorMessage; } + } else { + if(s_logger.isInfoEnabled()) { + s_logger.info(String.format("No checksum available for '%s'",originalTemplate.getName())); + } } String result; @@ -425,10 +429,13 @@ private String postLocalDownload(String jobId) { // if we don't have a checksum let's create one now if(StringUtils.isBlank(dnld.getChecksum())) { - String checksumErrorMessage = doTheChecksum(dnld, originalTemplate); + String checksumErrorMessage = doTheChecksum(dnld, downloadedTemplate); if (checksumErrorMessage != null) { return checksumErrorMessage; } + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("created new checksum (%s) for '%s'",dnld.getChecksum(), downloadedTemplate)); + } } return null; } @@ -490,6 +497,9 @@ private String doTheChecksum(DownloadJob dnld, File originalTemplate) { ChecksumValue newValue = null; try { newValue = computeCheckSum(oldValue.getAlgorithm(), originalTemplate); + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("computed checksum: %s", newValue)); + } } catch (NoSuchAlgorithmException e) { return "checksum algorithm not recognised: " + oldValue.getAlgorithm(); } From 59566b6eda913ebf6261ee8752e1a3410da940bc Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 16 Jan 2020 10:24:16 +0100 Subject: [PATCH 06/13] rename static to convention --- .../storage/template/DownloadManagerImpl.java | 98 +++++++++---------- 1 file changed, 49 insertions(+), 49 deletions(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java index 29fec8ed28e1..cc2d1867f398 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java @@ -231,7 +231,7 @@ public void setOvfProperties(List ovfProperties) { } } - public static final Logger s_logger = Logger.getLogger(DownloadManagerImpl.class); + public static final Logger LOGGER = Logger.getLogger(DownloadManagerImpl.class); private String _templateDir; private String _volumeDir; private String createTmpltScr; @@ -264,12 +264,12 @@ public void setStorageLayer(StorageLayer storage) { public void setDownloadStatus(String jobId, Status status) { DownloadJob dj = jobs.get(jobId); if (dj == null) { - s_logger.warn("setDownloadStatus for jobId: " + jobId + ", status=" + status + " no job found"); + LOGGER.warn("setDownloadStatus for jobId: " + jobId + ", status=" + status + " no job found"); return; } TemplateDownloader td = dj.getTemplateDownloader(); - s_logger.info("Download Completion for jobId: " + jobId + ", status=" + status); - s_logger.info("local: " + td.getDownloadLocalPath() + ", bytes=" + td.getDownloadedBytes() + ", error=" + td.getDownloadError() + ", pct=" + + LOGGER.info("Download Completion for jobId: " + jobId + ", status=" + status); + LOGGER.info("local: " + td.getDownloadLocalPath() + ", bytes=" + td.getDownloadedBytes() + ", error=" + td.getDownloadError() + ", pct=" + td.getDownloadPercent()); switch (status) { @@ -282,7 +282,7 @@ public void setDownloadStatus(String jobId, Status status) { case UNKNOWN: return; case IN_PROGRESS: - s_logger.info("Resuming jobId: " + jobId + ", status=" + status); + LOGGER.info("Resuming jobId: " + jobId + ", status=" + status); td.setResume(true); threadPool.execute(td); break; @@ -297,7 +297,7 @@ public void setDownloadStatus(String jobId, Status status) { td.setDownloadError("Download success, starting install "); String result = postRemoteDownload(jobId); if (result != null) { - s_logger.error("Failed post download install: " + result); + LOGGER.error("Failed post download install: " + result); td.setStatus(Status.UNRECOVERABLE_ERROR); td.setDownloadError("Failed post download install: " + result); ((S3TemplateDownloader) td).cleanupAfterError(); @@ -312,7 +312,7 @@ public void setDownloadStatus(String jobId, Status status) { td.setDownloadError("Download success, starting install "); String result = postLocalDownload(jobId); if (result != null) { - s_logger.error("Failed post download script: " + result); + LOGGER.error("Failed post download script: " + result); td.setStatus(Status.UNRECOVERABLE_ERROR); td.setDownloadError("Failed post download script: " + result); } else { @@ -392,8 +392,8 @@ private String postLocalDownload(String jobId) { return checksumErrorMessage; } } else { - if(s_logger.isInfoEnabled()) { - s_logger.info(String.format("No checksum available for '%s'",originalTemplate.getName())); + if(LOGGER.isInfoEnabled()) { + LOGGER.info(String.format("No checksum available for '%s'",originalTemplate.getName())); } } @@ -417,7 +417,7 @@ private String postLocalDownload(String jobId) { try { loc.create(dnld.getId(), true, dnld.getTmpltName()); } catch (IOException e) { - s_logger.warn("Something is wrong with template location " + resourcePath, e); + LOGGER.warn("Something is wrong with template location " + resourcePath, e); loc.purge(); return "Unable to download due to " + e.getMessage(); } @@ -433,8 +433,8 @@ private String postLocalDownload(String jobId) { if (checksumErrorMessage != null) { return checksumErrorMessage; } - if (s_logger.isDebugEnabled()) { - s_logger.debug(String.format("created new checksum (%s) for '%s'",dnld.getChecksum(), downloadedTemplate)); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug(String.format("created new checksum (%s) for '%s'",dnld.getChecksum(), downloadedTemplate)); } } return null; @@ -447,7 +447,7 @@ private String executeCreateScript(DownloadJob dnld, TemplateDownloader td, Stri long timeout = (long)imgSizeGigs * installTimeoutPerGig; Script scr = null; String script = resourceType == ResourceType.TEMPLATE ? createTmpltScr : createVolScr; - scr = new Script(script, timeout, s_logger); + scr = new Script(script, timeout, LOGGER); scr.add("-s", Integer.toString(imgSizeGigs)); scr.add("-S", Long.toString(td.getMaxTemplateSizeInBytes())); if (dnld.getDescription() != null && dnld.getDescription().length() > 1) { @@ -497,8 +497,8 @@ private String doTheChecksum(DownloadJob dnld, File originalTemplate) { ChecksumValue newValue = null; try { newValue = computeCheckSum(oldValue.getAlgorithm(), originalTemplate); - if (s_logger.isDebugEnabled()) { - s_logger.debug(String.format("computed checksum: %s", newValue)); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug(String.format("computed checksum: %s", newValue)); } } catch (NoSuchAlgorithmException e) { return "checksum algorithm not recognised: " + oldValue.getAlgorithm(); @@ -508,7 +508,7 @@ private String doTheChecksum(DownloadJob dnld, File originalTemplate) { } String checksum = newValue.getChecksum(); if (checksum == null) { - s_logger.warn("Something wrong happened when try to calculate the checksum of downloaded template!"); + LOGGER.warn("Something wrong happened when try to calculate the checksum of downloaded template!"); } dnld.setCheckSum(checksum); return null; @@ -523,7 +523,7 @@ private String postProcessAfterDownloadComplete(DownloadJob dnld, String resourc try { info = processor.process(resourcePath, null, templateName, this._processTimeout); } catch (InternalErrorException e) { - s_logger.error("Template process exception ", e); + LOGGER.error("Template process exception ", e); return e.toString(); } if (info != null) { @@ -541,7 +541,7 @@ private String postProcessAfterDownloadComplete(DownloadJob dnld, String resourc } if (!loc.save()) { - s_logger.warn("Cleaning up because we're unable to save the formats"); + LOGGER.warn("Cleaning up because we're unable to save the formats"); loc.purge(); } @@ -600,7 +600,7 @@ public String downloadPublicTemplate(long id, String url, String name, ImageForm try { if (!_storage.mkdirs(tmpDir)) { - s_logger.warn("Unable to create " + tmpDir); + LOGGER.warn("Unable to create " + tmpDir); return "Unable to create " + tmpDir; } // TO DO - define constant for volume properties. @@ -609,12 +609,12 @@ public String downloadPublicTemplate(long id, String url, String name, ImageForm "volume.properties"); if (file.exists()) { if(! file.delete()) { - s_logger.warn("Deletion of file '" + file.getAbsolutePath() + "' failed."); + LOGGER.warn("Deletion of file '" + file.getAbsolutePath() + "' failed."); } } if (!file.createNewFile()) { - s_logger.warn("Unable to create new file: " + file.getAbsolutePath()); + LOGGER.warn("Unable to create new file: " + file.getAbsolutePath()); return "Unable to create new file: " + file.getAbsolutePath(); } @@ -656,7 +656,7 @@ public String downloadPublicTemplate(long id, String url, String name, ImageForm return jobId; } catch (IOException e) { - s_logger.warn("Unable to download to " + tmpDir, e); + LOGGER.warn("Unable to download to " + tmpDir, e); return null; } } @@ -859,24 +859,24 @@ private String getInstallPath(String jobId) { private List listVolumes(String rootdir) { List result = new ArrayList(); - Script script = new Script(listVolScr, s_logger); + Script script = new Script(listVolScr, LOGGER); script.add("-r", rootdir); ZfsPathParser zpp = new ZfsPathParser(rootdir); script.execute(zpp); result.addAll(zpp.getPaths()); - s_logger.info("found " + zpp.getPaths().size() + " volumes" + zpp.getPaths()); + LOGGER.info("found " + zpp.getPaths().size() + " volumes" + zpp.getPaths()); return result; } private List listTemplates(String rootdir) { List result = new ArrayList(); - Script script = new Script(listTmpltScr, s_logger); + Script script = new Script(listTmpltScr, LOGGER); script.add("-r", rootdir); ZfsPathParser zpp = new ZfsPathParser(rootdir); script.execute(zpp); result.addAll(zpp.getPaths()); - s_logger.info("found " + zpp.getPaths().size() + " templates" + zpp.getPaths()); + LOGGER.info("found " + zpp.getPaths().size() + " templates" + zpp.getPaths()); return result; } @@ -895,13 +895,13 @@ public Map gatherTemplateInfo(String rootDir) { TemplateLocation loc = new TemplateLocation(_storage, path); try { if (!loc.load()) { - s_logger.warn("Post download installation was not completed for " + path); + LOGGER.warn("Post download installation was not completed for " + path); // loc.purge(); _storage.cleanup(path, templateDir); continue; } } catch (IOException e) { - s_logger.warn("Unable to load template location " + path, e); + LOGGER.warn("Unable to load template location " + path, e); continue; } @@ -916,12 +916,12 @@ public Map gatherTemplateInfo(String rootDir) { loc.updateVirtualSize(vSize); loc.save(); } catch (Exception e) { - s_logger.error("Unable to get the virtual size of the template: " + tInfo.getInstallPath() + " due to " + e.getMessage()); + LOGGER.error("Unable to get the virtual size of the template: " + tInfo.getInstallPath() + " due to " + e.getMessage()); } } result.put(tInfo.getTemplateName(), tInfo); - s_logger.debug("Added template name: " + tInfo.getTemplateName() + ", path: " + tmplt); + LOGGER.debug("Added template name: " + tInfo.getTemplateName() + ", path: " + tmplt); } return result; } @@ -941,13 +941,13 @@ public Map gatherVolumeInfo(String rootDir) { TemplateLocation loc = new TemplateLocation(_storage, path); try { if (!loc.load()) { - s_logger.warn("Post download installation was not completed for " + path); + LOGGER.warn("Post download installation was not completed for " + path); // loc.purge(); _storage.cleanup(path, volumeDir); continue; } } catch (IOException e) { - s_logger.warn("Unable to load volume location " + path, e); + LOGGER.warn("Unable to load volume location " + path, e); continue; } @@ -962,12 +962,12 @@ public Map gatherVolumeInfo(String rootDir) { loc.updateVirtualSize(vSize); loc.save(); } catch (Exception e) { - s_logger.error("Unable to get the virtual size of the volume: " + vInfo.getInstallPath() + " due to " + e.getMessage()); + LOGGER.error("Unable to get the virtual size of the volume: " + vInfo.getInstallPath() + " due to " + e.getMessage()); } } result.put(vInfo.getId(), vInfo); - s_logger.debug("Added volume name: " + vInfo.getTemplateName() + ", path: " + vol); + LOGGER.debug("Added volume name: " + vInfo.getTemplateName() + ", path: " + vol); } return result; } @@ -1031,7 +1031,7 @@ public boolean configure(String name, Map params) throws Configu String inSystemVM = (String)params.get("secondary.storage.vm"); if (inSystemVM != null && "true".equalsIgnoreCase(inSystemVM)) { - s_logger.info("DownloadManager: starting additional services since we are inside system vm"); + LOGGER.info("DownloadManager: starting additional services since we are inside system vm"); _nfsVersion = NfsSecondaryStorageResource.retrieveNfsVersionFromParams(params); startAdditionalServices(); blockOutgoingOnPrivate(); @@ -1052,25 +1052,25 @@ public boolean configure(String name, Map params) throws Configu if (listTmpltScr == null) { throw new ConfigurationException("Unable to find the listvmtmplt.sh"); } - s_logger.info("listvmtmplt.sh found in " + listTmpltScr); + LOGGER.info("listvmtmplt.sh found in " + listTmpltScr); createTmpltScr = Script.findScript(scriptsDir, "createtmplt.sh"); if (createTmpltScr == null) { throw new ConfigurationException("Unable to find createtmplt.sh"); } - s_logger.info("createtmplt.sh found in " + createTmpltScr); + LOGGER.info("createtmplt.sh found in " + createTmpltScr); listVolScr = Script.findScript(scriptsDir, "listvolume.sh"); if (listVolScr == null) { throw new ConfigurationException("Unable to find the listvolume.sh"); } - s_logger.info("listvolume.sh found in " + listVolScr); + LOGGER.info("listvolume.sh found in " + listVolScr); createVolScr = Script.findScript(scriptsDir, "createvolume.sh"); if (createVolScr == null) { throw new ConfigurationException("Unable to find createvolume.sh"); } - s_logger.info("createvolume.sh found in " + createVolScr); + LOGGER.info("createvolume.sh found in " + createVolScr); _processors = new HashMap(); @@ -1114,7 +1114,7 @@ public boolean configure(String name, Map params) throws Configu } private void blockOutgoingOnPrivate() { - Script command = new Script("/bin/bash", s_logger); + Script command = new Script("/bin/bash", LOGGER); String intf = "eth1"; command.add("-c"); command.add("iptables -A OUTPUT -o " + intf + " -p tcp -m state --state NEW -m tcp --dport " + "80" + " -j REJECT;" + "iptables -A OUTPUT -o " + intf + @@ -1122,7 +1122,7 @@ private void blockOutgoingOnPrivate() { String result = command.execute(); if (result != null) { - s_logger.warn("Error in blocking outgoing to port 80/443 err=" + result); + LOGGER.warn("Error in blocking outgoing to port 80/443 err=" + result); return; } } @@ -1143,37 +1143,37 @@ public boolean stop() { } private void startAdditionalServices() { - Script command = new Script("/bin/systemctl", s_logger); + Script command = new Script("/bin/systemctl", LOGGER); command.add("stop"); command.add("apache2"); String result = command.execute(); if (result != null) { - s_logger.warn("Error in stopping httpd service err=" + result); + LOGGER.warn("Error in stopping httpd service err=" + result); } String port = Integer.toString(TemplateConstants.DEFAULT_TMPLT_COPY_PORT); String intf = TemplateConstants.DEFAULT_TMPLT_COPY_INTF; - command = new Script("/bin/bash", s_logger); + command = new Script("/bin/bash", LOGGER); command.add("-c"); command.add("iptables -I INPUT -i " + intf + " -p tcp -m state --state NEW -m tcp --dport " + port + " -j ACCEPT;" + "iptables -I INPUT -i " + intf + " -p tcp -m state --state NEW -m tcp --dport " + "443" + " -j ACCEPT;"); result = command.execute(); if (result != null) { - s_logger.warn("Error in opening up apache2 port err=" + result); + LOGGER.warn("Error in opening up apache2 port err=" + result); return; } - command = new Script("/bin/systemctl", s_logger); + command = new Script("/bin/systemctl", LOGGER); command.add("start"); command.add("apache2"); result = command.execute(); if (result != null) { - s_logger.warn("Error in starting apache2 service err=" + result); + LOGGER.warn("Error in starting apache2 service err=" + result); return; } - command = new Script("/bin/su", s_logger); + command = new Script("/bin/su", LOGGER); command.add("-s"); command.add("/bin/bash"); command.add("-c"); @@ -1181,7 +1181,7 @@ private void startAdditionalServices() { command.add("www-data"); result = command.execute(); if (result != null) { - s_logger.warn("Error in creating directory =" + result); + LOGGER.warn("Error in creating directory =" + result); return; } } From b6bc31d6ef125090b54602241993dbce24c12f48 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 16 Jan 2020 12:36:30 +0100 Subject: [PATCH 07/13] message digest name --- .../org/apache/cloudstack/utils/security/ChecksumValue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/src/main/java/org/apache/cloudstack/utils/security/ChecksumValue.java b/utils/src/main/java/org/apache/cloudstack/utils/security/ChecksumValue.java index 43d92b651190..e7a7be46bad9 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/security/ChecksumValue.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/security/ChecksumValue.java @@ -82,6 +82,6 @@ private static String algorithmFromDigest(String digest) { } // else if no algoritm return "MD5"; } // or if no digest at all - return "SHA512"; + return "SHA-512"; } } From 386fdd8b9e319b6441798689e38cd07765e1899a Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 16 Jan 2020 13:56:19 +0100 Subject: [PATCH 08/13] store checksum with algorithm --- .../apache/cloudstack/storage/template/DownloadManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java index cc2d1867f398..df8860ab43cf 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java @@ -506,7 +506,7 @@ private String doTheChecksum(DownloadJob dnld, File originalTemplate) { if (StringUtils.isNotBlank(dnld.getChecksum()) && !oldValue.equals(newValue)) { return "checksum \"" + newValue + "\" didn't match the given value, \"" + oldValue + "\""; } - String checksum = newValue.getChecksum(); + String checksum = newValue.toString(); if (checksum == null) { LOGGER.warn("Something wrong happened when try to calculate the checksum of downloaded template!"); } From 77d5ea63379ef608f863ae7e6b82782656f2011f Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 16 Jan 2020 13:59:14 +0100 Subject: [PATCH 09/13] passing whatever checksum was in the command the checksum from the command will be ignored by the async job it will read it from the record anew --- .../apache/cloudstack/storage/image/TemplateServiceImpl.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java index 189b88f669a8..097b85477daf 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java @@ -1045,8 +1045,6 @@ public AsyncCallFuture copyTemplate(TemplateInfo srcTemplate, } tmplForCopy.setUrl(url); - tmplForCopy.getImage().setChecksum(""); - if (s_logger.isDebugEnabled()) { s_logger.debug("Mark template_store_ref entry as Creating"); } From c8077a6a98da616f8469bbdd193a1f43a517440c Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Fri, 17 Jan 2020 16:57:14 +0100 Subject: [PATCH 10/13] hack in the empty chksm --- .../apache/cloudstack/storage/image/TemplateServiceImpl.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java index 097b85477daf..4c2406d3a238 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java @@ -1052,6 +1052,9 @@ public AsyncCallFuture copyTemplate(TemplateInfo srcTemplate, DataObject templateOnStore = destStore.create(tmplForCopy); templateOnStore.processEvent(Event.CreateOnlyRequested); + // I am the big assumer^Wpretender + ((TemplateObject)templateOnStore).getImage().setChecksum(null); + if (s_logger.isDebugEnabled()) { s_logger.debug("Invoke datastore driver createAsync to create template on destination store"); } From e3c077a83c528e8d8869f621b038053c44e1bc9f Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Fri, 17 Jan 2020 17:37:20 +0100 Subject: [PATCH 11/13] unused removed etc and renamed static LOGGER --- .../storage/download/DownloadMonitorImpl.java | 81 ++++++++----------- 1 file changed, 34 insertions(+), 47 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/download/DownloadMonitorImpl.java b/server/src/main/java/com/cloud/storage/download/DownloadMonitorImpl.java index 68be52d842fb..1954cdea6879 100644 --- a/server/src/main/java/com/cloud/storage/download/DownloadMonitorImpl.java +++ b/server/src/main/java/com/cloud/storage/download/DownloadMonitorImpl.java @@ -53,29 +53,21 @@ import com.cloud.storage.RegisterVolumePayload; import com.cloud.storage.Storage.ImageFormat; import com.cloud.storage.VMTemplateStorageResourceAssoc.Status; -import com.cloud.storage.Volume; -import com.cloud.storage.dao.VMTemplateDao; -import com.cloud.storage.dao.VolumeDao; import com.cloud.storage.template.TemplateConstants; import com.cloud.storage.upload.UploadListener; -import com.cloud.template.VirtualMachineTemplate; import com.cloud.utils.component.ComponentContext; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.exception.CloudRuntimeException; @Component public class DownloadMonitorImpl extends ManagerBase implements DownloadMonitor { - static final Logger s_logger = Logger.getLogger(DownloadMonitorImpl.class); + static final Logger LOGGER = Logger.getLogger(DownloadMonitorImpl.class); @Inject private TemplateDataStoreDao _vmTemplateStoreDao; @Inject - private VolumeDao _volumeDao; - @Inject private VolumeDataStoreDao _volumeStoreDao; @Inject - private final VMTemplateDao _templateDao = null; - @Inject private AgentManager _agentMgr; @Inject private ConfigurationDao _configDao; @@ -94,7 +86,7 @@ public boolean configure(String name, Map params) { String cert = configs.get("secstorage.ssl.cert.domain"); if (!"realhostip.com".equalsIgnoreCase(cert)) { - s_logger.warn("Only realhostip.com ssl cert is supported, ignoring self-signed and other certs"); + LOGGER.warn("Only realhostip.com ssl cert is supported, ignoring self-signed and other certs"); } _copyAuthPasswd = configs.get("secstorage.copy.password"); @@ -125,7 +117,7 @@ public boolean isTemplateUpdateable(Long templateId, Long storeId) { private void initiateTemplateDownload(DataObject template, AsyncCompletionCallback callback) { boolean downloadJobExists = false; - TemplateDataStoreVO vmTemplateStore = null; + TemplateDataStoreVO vmTemplateStore; DataStore store = template.getDataStore(); vmTemplateStore = _vmTemplateStoreDao.findByStoreTemplate(store.getId(), template.getId()); @@ -141,7 +133,6 @@ private void initiateTemplateDownload(DataObject template, AsyncCompletionCallba Long maxTemplateSizeInBytes = getMaxTemplateSizeInBytes(); if (vmTemplateStore != null) { start(); - VirtualMachineTemplate tmpl = _templateDao.findById(template.getId()); DownloadCommand dcmd = new DownloadCommand((TemplateObjectTO)(template.getTO()), maxTemplateSizeInBytes); dcmd.setProxy(getHttpProxy()); if (downloadJobExists) { @@ -153,7 +144,7 @@ private void initiateTemplateDownload(DataObject template, AsyncCompletionCallba EndPoint ep = _epSelector.select(template); if (ep == null) { String errMsg = "There is no secondary storage VM for downloading template to image store " + store.getName(); - s_logger.warn(errMsg); + LOGGER.warn(errMsg); throw new CloudRuntimeException(errMsg); } DownloadListener dl = new DownloadListener(ep, store, template, _timer, this, dcmd, callback); @@ -164,14 +155,14 @@ private void initiateTemplateDownload(DataObject template, AsyncCompletionCallba // DownloadListener to use // new ObjectInDataStore.State transition. TODO: fix this later // to be able to remove downloadState from template_store_ref. - s_logger.info("found existing download job"); + LOGGER.info("found existing download job"); dl.setCurrState(vmTemplateStore.getDownloadState()); } try { ep.sendMessageAsync(dcmd, new UploadListener.Callback(ep.getId(), dl)); } catch (Exception e) { - s_logger.warn("Unable to start /resume download of template " + template.getId() + " to " + store.getName(), e); + LOGGER.warn("Unable to start /resume download of template " + template.getId() + " to " + store.getName(), e); dl.setDisconnected(); dl.scheduleStatusCheck(RequestType.GET_OR_RESTART); } @@ -187,12 +178,12 @@ public void downloadTemplateToStorage(DataObject template, AsyncCompletionCallba if (template.getUri() != null) { initiateTemplateDownload(template, callback); } else { - s_logger.info("Template url is null, cannot download"); + LOGGER.info("Template url is null, cannot download"); DownloadAnswer ans = new DownloadAnswer("Template url is null", Status.UNKNOWN); callback.complete(ans); } } else { - s_logger.info("Template download is already in progress or already downloaded"); + LOGGER.info("Template download is already in progress or already downloaded"); DownloadAnswer ans = new DownloadAnswer("Template download is already in progress or already downloaded", Status.UNKNOWN); callback.complete(ans); @@ -203,7 +194,7 @@ public void downloadTemplateToStorage(DataObject template, AsyncCompletionCallba @Override public void downloadVolumeToStorage(DataObject volume, AsyncCompletionCallback callback) { boolean downloadJobExists = false; - VolumeDataStoreVO volumeHost = null; + VolumeDataStoreVO volumeHost; DataStore store = volume.getDataStore(); VolumeInfo volInfo = (VolumeInfo)volume; RegisterVolumePayload payload = (RegisterVolumePayload)volInfo.getpayload(); @@ -214,7 +205,7 @@ public void downloadVolumeToStorage(DataObject volume, AsyncCompletionCallback 2)) { downloadJobExists = true; } else { @@ -225,35 +216,32 @@ public void downloadVolumeToStorage(DataObject volume, AsyncCompletionCallback Date: Mon, 20 Jan 2020 11:41:21 +0100 Subject: [PATCH 12/13] behaviour on no checksum backwards compatible --- .../storage/template/DownloadManagerImpl.java | 31 ++++++------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java index df8860ab43cf..f6de4c3d0e8c 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java @@ -385,17 +385,16 @@ private String postLocalDownload(String jobId) { ResourceType resourceType = dnld.getResourceType(); File originalTemplate = new File(td.getDownloadLocalPath()); - if(StringUtils.isNotBlank(dnld.getChecksum())) { - // we have a checksum so check: - String checksumErrorMessage = doTheChecksum(dnld, originalTemplate); - if (checksumErrorMessage != null) { - return checksumErrorMessage; - } - } else { - if(LOGGER.isInfoEnabled()) { - LOGGER.info(String.format("No checksum available for '%s'",originalTemplate.getName())); + if(StringUtils.isBlank(dnld.getChecksum())) { + if (LOGGER.isInfoEnabled()) { + LOGGER.info(String.format("No checksum available for '%s'", originalTemplate.getName())); } } + // check or create checksum + String checksumErrorMessage = checkOrCreateTheChecksum(dnld, originalTemplate); + if (checksumErrorMessage != null) { + return checksumErrorMessage; + } String result; String extension = dnld.getFormat().getFileExtension(); @@ -427,16 +426,6 @@ private String postLocalDownload(String jobId) { return result; } - // if we don't have a checksum let's create one now - if(StringUtils.isBlank(dnld.getChecksum())) { - String checksumErrorMessage = doTheChecksum(dnld, downloadedTemplate); - if (checksumErrorMessage != null) { - return checksumErrorMessage; - } - if (LOGGER.isDebugEnabled()) { - LOGGER.debug(String.format("created new checksum (%s) for '%s'",dnld.getChecksum(), downloadedTemplate)); - } - } return null; } @@ -492,11 +481,11 @@ private void setPermissionsForTheDownloadedTemplate(DownloadJob dnld, String res _storage.setWorldReadableAndWriteable(templateProperties); } - private String doTheChecksum(DownloadJob dnld, File originalTemplate) { + private String checkOrCreateTheChecksum(DownloadJob dnld, File targetFile) { ChecksumValue oldValue = new ChecksumValue(dnld.getChecksum()); ChecksumValue newValue = null; try { - newValue = computeCheckSum(oldValue.getAlgorithm(), originalTemplate); + newValue = computeCheckSum(oldValue.getAlgorithm(), targetFile); if (LOGGER.isDebugEnabled()) { LOGGER.debug(String.format("computed checksum: %s", newValue)); } From f483e27ee7e8fb3014e76625a9cc463ebb7f40d1 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 30 Jan 2020 11:12:23 +0100 Subject: [PATCH 13/13] check type to be sure --- .../apache/cloudstack/storage/image/TemplateServiceImpl.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java index 4c2406d3a238..97ac7c9037d4 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java @@ -1052,8 +1052,9 @@ public AsyncCallFuture copyTemplate(TemplateInfo srcTemplate, DataObject templateOnStore = destStore.create(tmplForCopy); templateOnStore.processEvent(Event.CreateOnlyRequested); - // I am the big assumer^Wpretender - ((TemplateObject)templateOnStore).getImage().setChecksum(null); + if (templateOnStore instanceof TemplateObject) { + ((TemplateObject)templateOnStore).getImage().setChecksum(null); + } // else we don't know what to do. if (s_logger.isDebugEnabled()) { s_logger.debug("Invoke datastore driver createAsync to create template on destination store");