From 6fc0c93c1119cbb0fe5481d38e3f1455afa426d6 Mon Sep 17 00:00:00 2001 From: waterWang Date: Tue, 25 Aug 2026 18:30:47 +0800 Subject: [PATCH 1/2] Fix SSVM entity download URL creation: ensure userdata base dir exists and is writable by www-data When handleCreateEntityURLCommand runs on the SSVM it builds the download URL by creating /var/www/html/userdata// as the www-data user (su www-data -c "mkdir -p ..."). On stock systemvm templates the /var/www/html/userdata base directory does not exist and /var/www/html is not writable by www-data, so the mkdir fails with "Permission denied" and getDiagnosticsData / extractVolume / extractTemplate async jobs error out with code 530 and no download URL is produced. Fix: create the base extract directory and set its ownership to www-data before dropping privileges to create the per-entity subdirectory. This makes the SSVM self-healing on first use and matches the directory layout expected by the Apache docroot. References #13959 --- .../storage/template/UploadManagerImpl.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java index aacca9926f7d..8bfd120b1f83 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java @@ -276,13 +276,24 @@ public CreateEntityDownloadURLAnswer handleCreateEntityURLCommand(CreateEntityDo // Create the directory structure so that its visible under apache server root String extractDir = BASE_EXTRACT_PATH; extractDir = extractDir + cmd.getFilepathInExtractURL() + File.separator; + // Ensure the base extract directory exists and is owned by www-data so that + // the www-data user can create the per-entity subdirectory inside it. + Script baseDirCommand = new Script("/bin/bash", logger); + baseDirCommand.add("-c"); + baseDirCommand.add("mkdir -p " + BASE_EXTRACT_PATH + " && chown www-data:www-data " + BASE_EXTRACT_PATH); + String result = baseDirCommand.execute(); + if (result != null) { + String errorString = "Error in creating base extract directory =" + result; + logger.error(errorString); + return new CreateEntityDownloadURLAnswer(errorString, CreateEntityDownloadURLAnswer.RESULT_FAILURE); + } Script command = new Script("/bin/su", logger); command.add("-s"); command.add("/bin/bash"); command.add("-c"); command.add("mkdir -p " + extractDir); command.add("www-data"); - String result = command.execute(); + result = command.execute(); if (result != null) { String errorString = "Error in creating directory =" + result; logger.error(errorString); From f5c2384972ea57073facfd6a6db8f70e4a0ff144 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 9 Nov 2026 14:28:14 +0100 Subject: [PATCH 2/2] ssvm: create userdata extract dir without a shell, only when needed --- .../storage/template/UploadManagerImpl.java | 33 ++++++++++--- .../template/UploadManagerImplTest.java | 46 +++++++++++++++++++ 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java index 8bfd120b1f83..a57dc18c6009 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java @@ -64,6 +64,7 @@ public class UploadManagerImpl extends ManagerBase implements UploadManager { protected static final String EXTRACT_USERDATA_DIR = "userdata"; protected static final String BASE_EXTRACT_PATH = String.format("/var/www/html/%s/", EXTRACT_USERDATA_DIR); + protected static final String APACHE_USER = "www-data"; public class Completion implements UploadCompleteCallback { private final String jobId; @@ -276,14 +277,9 @@ public CreateEntityDownloadURLAnswer handleCreateEntityURLCommand(CreateEntityDo // Create the directory structure so that its visible under apache server root String extractDir = BASE_EXTRACT_PATH; extractDir = extractDir + cmd.getFilepathInExtractURL() + File.separator; - // Ensure the base extract directory exists and is owned by www-data so that - // the www-data user can create the per-entity subdirectory inside it. - Script baseDirCommand = new Script("/bin/bash", logger); - baseDirCommand.add("-c"); - baseDirCommand.add("mkdir -p " + BASE_EXTRACT_PATH + " && chown www-data:www-data " + BASE_EXTRACT_PATH); - String result = baseDirCommand.execute(); + String result = ensureBaseExtractDirectory(); if (result != null) { - String errorString = "Error in creating base extract directory =" + result; + String errorString = String.format("Error in creating base extract directory %s owned by %s: %s", BASE_EXTRACT_PATH, APACHE_USER, result); logger.error(errorString); return new CreateEntityDownloadURLAnswer(errorString, CreateEntityDownloadURLAnswer.RESULT_FAILURE); } @@ -336,6 +332,29 @@ public CreateEntityDownloadURLAnswer handleCreateEntityURLCommand(CreateEntityDo } + /** + * Makes sure the base extract directory exists and is owned by the apache user, so that the apache user + * can create the per-entity subdirectory inside it. Stock systemvm templates do not ship this directory. + * @return null on success, the error output otherwise + */ + protected String ensureBaseExtractDirectory() { + if (isBaseExtractDirectoryReady(Path.of(BASE_EXTRACT_PATH))) { + return null; + } + Script command = new Script("install", logger); + command.add("-d", "-o", APACHE_USER, "-g", APACHE_USER, "-m", "755", BASE_EXTRACT_PATH); + return command.execute(); + } + + protected boolean isBaseExtractDirectoryReady(Path baseDir) { + try { + return Files.isDirectory(baseDir) && APACHE_USER.equals(Files.getOwner(baseDir).getName()); + } catch (IOException e) { + logger.debug("Unable to determine owner of {}: {}", baseDir, e.getMessage()); + return false; + } + } + @Override public Answer handleDeleteEntityDownloadURLCommand(DeleteEntityDownloadURLCommand cmd) { diff --git a/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java index 9038098e235f..4e86a7f1f52e 100644 --- a/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java +++ b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java @@ -16,29 +16,39 @@ // under the License. package org.apache.cloudstack.storage.template; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockConstruction; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; +import java.nio.file.Files; import java.nio.file.Path; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InjectMocks; +import org.mockito.MockedConstruction; import org.mockito.MockedStatic; import org.mockito.Mockito; +import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; import com.cloud.agent.api.storage.DeleteEntityDownloadURLCommand; import com.cloud.utils.FileUtil; +import com.cloud.utils.script.Script; @RunWith(MockitoJUnitRunner.class) public class UploadManagerImplTest { + @Spy @InjectMocks UploadManagerImpl uploadManager; @@ -82,4 +92,40 @@ public void deletesSymlinkRootDirectoryWhenNoFile() { uploadManager.deleteEntitySymlinkRootDirectoryIfNeeded(mock(DeleteEntityDownloadURLCommand.class), validLinkPath); fileUtilMock.verify(() -> FileUtil.deleteRecursively(any(Path.class)), times(1)); } + + @Test + public void baseExtractDirectoryIsNotReadyWhenMissing() { + assertFalse(uploadManager.isBaseExtractDirectoryReady(Path.of("/nonexistent/cloudstack/userdata/"))); + } + + @Test + public void baseExtractDirectoryIsNotReadyWhenNotOwnedByApacheUser() throws Exception { + Path dir = Files.createTempDirectory("userdata"); + try { + assertFalse(uploadManager.isBaseExtractDirectoryReady(dir)); + } finally { + Files.delete(dir); + } + } + + @Test + public void ensureBaseExtractDirectorySkipsInstallWhenReady() { + doReturn(true).when(uploadManager).isBaseExtractDirectoryReady(any(Path.class)); + try (MockedConstruction