Skip to content

Commit b56bbc2

Browse files
DaanHooglandwaterWang
andauthored
SSVM: entity download URL creation: ensure userdata base dir exists (#14447)
Co-authored-by: waterWang <waterWang@users.noreply.github.com>
1 parent d02adcd commit b56bbc2

2 files changed

Lines changed: 77 additions & 1 deletion

File tree

‎services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ public class UploadManagerImpl extends ManagerBase implements UploadManager {
6464

6565
protected static final String EXTRACT_USERDATA_DIR = "userdata";
6666
protected static final String BASE_EXTRACT_PATH = String.format("/var/www/html/%s/", EXTRACT_USERDATA_DIR);
67+
protected static final String APACHE_USER = "www-data";
6768

6869
public class Completion implements UploadCompleteCallback {
6970
private final String jobId;
@@ -276,13 +277,19 @@ public CreateEntityDownloadURLAnswer handleCreateEntityURLCommand(CreateEntityDo
276277
// Create the directory structure so that its visible under apache server root
277278
String extractDir = BASE_EXTRACT_PATH;
278279
extractDir = extractDir + cmd.getFilepathInExtractURL() + File.separator;
280+
String result = ensureBaseExtractDirectory();
281+
if (result != null) {
282+
String errorString = String.format("Error in creating base extract directory %s owned by %s: %s", BASE_EXTRACT_PATH, APACHE_USER, result);
283+
logger.error(errorString);
284+
return new CreateEntityDownloadURLAnswer(errorString, CreateEntityDownloadURLAnswer.RESULT_FAILURE);
285+
}
279286
Script command = new Script("/bin/su", logger);
280287
command.add("-s");
281288
command.add("/bin/bash");
282289
command.add("-c");
283290
command.add("mkdir -p " + extractDir);
284291
command.add("www-data");
285-
String result = command.execute();
292+
result = command.execute();
286293
if (result != null) {
287294
String errorString = "Error in creating directory =" + result;
288295
logger.error(errorString);
@@ -325,6 +332,29 @@ public CreateEntityDownloadURLAnswer handleCreateEntityURLCommand(CreateEntityDo
325332

326333
}
327334

335+
/**
336+
* Makes sure the base extract directory exists and is owned by the apache user, so that the apache user
337+
* can create the per-entity subdirectory inside it. Stock systemvm templates do not ship this directory.
338+
* @return null on success, the error output otherwise
339+
*/
340+
protected String ensureBaseExtractDirectory() {
341+
if (isBaseExtractDirectoryReady(Path.of(BASE_EXTRACT_PATH))) {
342+
return null;
343+
}
344+
Script command = new Script("install", logger);
345+
command.add("-d", "-o", APACHE_USER, "-g", APACHE_USER, "-m", "755", BASE_EXTRACT_PATH);
346+
return command.execute();
347+
}
348+
349+
protected boolean isBaseExtractDirectoryReady(Path baseDir) {
350+
try {
351+
return Files.isDirectory(baseDir) && APACHE_USER.equals(Files.getOwner(baseDir).getName());
352+
} catch (IOException e) {
353+
logger.debug("Unable to determine owner of {}: {}", baseDir, e.getMessage());
354+
return false;
355+
}
356+
}
357+
328358
@Override
329359
public Answer handleDeleteEntityDownloadURLCommand(DeleteEntityDownloadURLCommand cmd) {
330360

‎services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,29 +16,39 @@
1616
// under the License.
1717
package org.apache.cloudstack.storage.template;
1818

19+
import static org.junit.Assert.assertFalse;
20+
import static org.junit.Assert.assertNull;
1921
import static org.mockito.ArgumentMatchers.any;
22+
import static org.mockito.Mockito.doReturn;
2023
import static org.mockito.Mockito.mock;
24+
import static org.mockito.Mockito.mockConstruction;
2125
import static org.mockito.Mockito.mockStatic;
2226
import static org.mockito.Mockito.never;
2327
import static org.mockito.Mockito.times;
2428

29+
import java.nio.file.Files;
2530
import java.nio.file.Path;
2631

2732
import org.junit.After;
33+
import org.junit.Assert;
2834
import org.junit.Before;
2935
import org.junit.Test;
3036
import org.junit.runner.RunWith;
3137
import org.mockito.InjectMocks;
38+
import org.mockito.MockedConstruction;
3239
import org.mockito.MockedStatic;
3340
import org.mockito.Mockito;
41+
import org.mockito.Spy;
3442
import org.mockito.junit.MockitoJUnitRunner;
3543

3644
import com.cloud.agent.api.storage.DeleteEntityDownloadURLCommand;
3745
import com.cloud.utils.FileUtil;
46+
import com.cloud.utils.script.Script;
3847

3948
@RunWith(MockitoJUnitRunner.class)
4049
public class UploadManagerImplTest {
4150

51+
@Spy
4252
@InjectMocks
4353
UploadManagerImpl uploadManager;
4454

@@ -82,4 +92,40 @@ public void deletesSymlinkRootDirectoryWhenNoFile() {
8292
uploadManager.deleteEntitySymlinkRootDirectoryIfNeeded(mock(DeleteEntityDownloadURLCommand.class), validLinkPath);
8393
fileUtilMock.verify(() -> FileUtil.deleteRecursively(any(Path.class)), times(1));
8494
}
95+
96+
@Test
97+
public void baseExtractDirectoryIsNotReadyWhenMissing() {
98+
assertFalse(uploadManager.isBaseExtractDirectoryReady(Path.of("/nonexistent/cloudstack/userdata/")));
99+
}
100+
101+
@Test
102+
public void baseExtractDirectoryIsNotReadyWhenNotOwnedByApacheUser() throws Exception {
103+
Path dir = Files.createTempDirectory("userdata");
104+
try {
105+
assertFalse(uploadManager.isBaseExtractDirectoryReady(dir));
106+
} finally {
107+
Files.delete(dir);
108+
}
109+
}
110+
111+
@Test
112+
public void ensureBaseExtractDirectorySkipsInstallWhenReady() {
113+
doReturn(true).when(uploadManager).isBaseExtractDirectoryReady(any(Path.class));
114+
try (MockedConstruction<Script> scripts = mockConstruction(Script.class)) {
115+
assertNull(uploadManager.ensureBaseExtractDirectory());
116+
Assert.assertTrue(scripts.constructed().isEmpty());
117+
}
118+
}
119+
120+
@Test
121+
public void ensureBaseExtractDirectoryInstallsWhenNotReady() {
122+
doReturn(false).when(uploadManager).isBaseExtractDirectoryReady(any(Path.class));
123+
try (MockedConstruction<Script> scripts = mockConstruction(Script.class,
124+
(script, context) -> Mockito.when(script.execute()).thenReturn("Permission denied"))) {
125+
Assert.assertEquals("Permission denied", uploadManager.ensureBaseExtractDirectory());
126+
Assert.assertEquals(1, scripts.constructed().size());
127+
Mockito.verify(scripts.constructed().get(0)).add("-d", "-o", UploadManagerImpl.APACHE_USER, "-g", UploadManagerImpl.APACHE_USER,
128+
"-m", "755", UploadManagerImpl.BASE_EXTRACT_PATH);
129+
}
130+
}
85131
}

0 commit comments

Comments
 (0)