Copilot commented on code in PR #12562:
URL: https://github.com/apache/cloudstack/pull/12562#discussion_r2767281324
##########
utils/src/main/java/com/cloud/utils/FileUtil.java:
##########
@@ -160,4 +160,19 @@ public static boolean writeToFile(String fileName, String
content) {
public static String readResourceFile(String resource) throws IOException {
return
IOUtils.toString(Objects.requireNonNull(Thread.currentThread().getContextClassLoader().getResourceAsStream(resource)),
com.cloud.utils.StringUtils.getPreferredCharset());
}
+
+ public static boolean deleteRecursively(Path path) throws IOException {
+ LOGGER.debug("Deleting path: {}", path);
+ if (Files.isDirectory(path)) {
Review Comment:
The deleteRecursively method may behave unexpectedly when encountering
symbolic links to directories. Files.isDirectory(path) follows symlinks by
default, which means if a symlink points to a directory, this method will
recursively delete the contents of the target directory instead of just
removing the symlink. Consider using Files.isDirectory(path,
LinkOption.NOFOLLOW_LINKS) to treat symlinks as files and delete them directly
without following them.
##########
services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java:
##########
@@ -356,6 +364,30 @@ public Answer
handleDeleteEntityDownloadURLCommand(DeleteEntityDownloadURLComman
return new Answer(cmd, true, "");
}
+ protected void
deleteEntitySymlinkRootPathIfNeeded(DeleteEntityDownloadURLCommand cmd, String
linkPath) {
+ if (StringUtils.isEmpty(linkPath)) {
+ return;
+ }
+ String[] parts = linkPath.split("/");
+ if (parts.length == 0) {
+ return;
+ }
+ String rootDir = parts[0];
+ if (StringUtils.isEmpty(rootDir) || !UuidUtils.isUuid(rootDir)) {
+ return;
+ }
+ logger.info("Deleting symlink root directory: {} for {}", rootDir,
cmd.getExtractUrl());
+ Path rootDirPath = Path.of(BASE_EXTRACT_DIR + rootDir);
Review Comment:
Consider using Path.of(BASE_EXTRACT_DIR, rootDir) instead of string
concatenation for path construction. This is more robust and handles path
separators correctly across different operating systems, even if
BASE_EXTRACT_DIR is modified in the future to not include a trailing separator.
```suggestion
Path rootDirPath = Path.of(BASE_EXTRACT_DIR, rootDir);
```
##########
services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java:
##########
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.storage.template;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+
+import java.nio.file.Path;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.agent.api.storage.DeleteEntityDownloadURLCommand;
+import com.cloud.utils.FileUtil;
+
+@RunWith(MockitoJUnitRunner.class)
+public class UploadManagerImplTest {
+
+ @InjectMocks
+ UploadManagerImpl uploadManager;
+
+ MockedStatic<FileUtil> fileUtilMock;
+
+ @Before
+ public void setup() {
+ fileUtilMock = mockStatic(FileUtil.class, Mockito.CALLS_REAL_METHODS);
+ fileUtilMock.when(() ->
FileUtil.deleteRecursively(any(Path.class))).thenReturn(true);
+ }
+
+ @After
+ public void tearDown() {
+ fileUtilMock.close();
+ }
+
+ @Test
+ public void doesNotDeleteWhenLinkPathIsEmpty() {
+ String emptyLinkPath = "";
+
uploadManager.deleteEntitySymlinkRootPathIfNeeded(mock(DeleteEntityDownloadURLCommand.class),
emptyLinkPath);
+ fileUtilMock.verify(() -> FileUtil.deleteRecursively(any(Path.class)),
never());
+ }
+
+ @Test
+ public void doesNotDeleteWhenRootDirIsNotUuid() {
+ String invalidLinkPath = "invalidRootDir/file";
+
uploadManager.deleteEntitySymlinkRootPathIfNeeded(mock(DeleteEntityDownloadURLCommand.class),
invalidLinkPath);
+ fileUtilMock.verify(() -> FileUtil.deleteRecursively(any(Path.class)),
never());
+ }
+
+ @Test
+ public void deletesSymlinkRootDirectoryWhenValidUuid() {
+ String validLinkPath = "123e4567-e89b-12d3-a456-426614174000/file";
+
uploadManager.deleteEntitySymlinkRootPathIfNeeded(mock(DeleteEntityDownloadURLCommand.class),
validLinkPath);
+ fileUtilMock.verify(() -> FileUtil.deleteRecursively(any(Path.class)),
times(1));
+ }
Review Comment:
Consider adding a test case for linkPath that contains only a UUID without a
filename (e.g., "123e4567-e89b-12d3-a456-426614174000"). While this may not
occur in normal operation given the URL format, it's a valid edge case that
should be handled correctly.
##########
services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java:
##########
@@ -330,12 +335,15 @@ public Answer
handleDeleteEntityDownloadURLCommand(DeleteEntityDownloadURLComman
String extractUrl = cmd.getExtractUrl();
String result;
if (extractUrl != null) {
- command.add("unlink /var/www/html/userdata/" +
extractUrl.substring(extractUrl.lastIndexOf(File.separator) + 1));
+ String linkPath =
extractUrl.substring(extractUrl.lastIndexOf(File.separator) + 1);
+ command.add("unlink " + BASE_EXTRACT_DIR + linkPath);
result = command.execute();
if (result != null) {
// FIXME - Ideally should bail out if you can't delete
symlink. Not doing it right now.
// This is because the ssvm might already be destroyed and the
symlinks do not exist.
logger.warn("Error in deleting symlink :" + result);
+ } else {
+ deleteEntitySymlinkRootPathIfNeeded(cmd, linkPath);
Review Comment:
The linkPath extraction logic is incorrect. Given extractUrl format
`http://host/userdata/UUID/filename`, using `lastIndexOf(File.separator)` will
find the last "/" before the filename, resulting in linkPath being just
"filename" instead of "UUID/filename". This breaks the
deleteEntitySymlinkRootPathIfNeeded method which expects linkPath to contain
the UUID directory name as the first path component. The code should extract
everything after "/userdata/" using
`extractUrl.substring(extractUrl.indexOf("/userdata/") +
"/userdata/".length())` instead, with proper validation that "/userdata/"
exists in the URL.
```suggestion
final String userDataSegment = "/userdata/";
int userDataIndex = extractUrl.indexOf(userDataSegment);
if (userDataIndex == -1) {
logger.warn("Extract URL does not contain expected segment
'/userdata/': " + extractUrl);
} else {
String linkPath = extractUrl.substring(userDataIndex +
userDataSegment.length());
if (StringUtils.isBlank(linkPath)) {
logger.warn("Extract URL does not contain a valid link
path after '/userdata/': " + extractUrl);
} else {
command.add("unlink " + BASE_EXTRACT_DIR + linkPath);
result = command.execute();
if (result != null) {
// FIXME - Ideally should bail out if you can't
delete symlink. Not doing it right now.
// This is because the ssvm might already be
destroyed and the symlinks do not exist.
logger.warn("Error in deleting symlink :" + result);
} else {
deleteEntitySymlinkRootPathIfNeeded(cmd, linkPath);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
