Skip to content

Commit

Permalink
Handle S3 storage problems by introducing close methods on storage #3255
Browse files Browse the repository at this point in the history


- main problem was that transfer managers were not shutdown when no
  longer needed
- removed also old convienence getter method for job storage (was only
  used by one test and one implementation)
- renamed getJobStorage to createJobStorage and mentioned in javadoc
  the close method must be called after no longer need of object
- close job storage on every point (PDS, SecHub and also prepare
  wrapper when no longer needed
  • Loading branch information
de-jcup committed Jul 1, 2024
1 parent 99b3a50 commit c292267
Show file tree
Hide file tree
Showing 37 changed files with 233 additions and 112 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,25 @@ public enum PDSConfigDataKeyProvider implements PDSKeyProvider<ExecutionPDSKey>
"When 'true', the PDS instance will show up some debug information on scan time. The output level of debugging information differs on PDS solutions/launcher scripts.")
.markDefaultRecommended().withDefault(false).markSendToPDS().markAsAvailableInsideScript())

,
/**
* A flag for PDS scripts which are calling a wrapper application. The script
* can use this information to start the wrapper application with a remote debug
* connection. Here an example for a java based wrapper application:
*
* <pre>
* <code>
* if [[ "$PDS_WRAPPER_REMOTE_DEBUGGING_ENABLED" = "true" ]]; then
* options="$options -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=8000"
* fi
* java -jar $options "$prepare_wrapper"
* <code>
* </pre>
*/
PDS_WRAPPER_REMOTE_DEBUGGING_ENABLED(new ExecutionPDSKey(PDSDefaultParameterKeyConstants.PARAM_KEY_PDS_WRAPPER_REMOTE_DEBUGGING_ENABLED,
"Additional information will be always sent to launcher scripts. Interesting to debug wrapper applications remote.").markDefaultRecommended()
.withDefault(false).markSendToPDS().markAsAvailableInsideScript())

,
/**
* This is automatically given to PDS by SecHub - depending on scan type. E.g.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ public class PDSDefaultParameterKeyConstants {

public static final String PARAM_KEY_PDS_DEBUG_ENABLED = "pds.debug.enabled";

public static final String PARAM_KEY_PDS_WRAPPER_REMOTE_DEBUGGING_ENABLED = "pds.wrapper.remote.debugging.enabled";

/**
* Contains the SecHub configuration model as Json
*/
Expand Down
3 changes: 3 additions & 0 deletions sechub-pds-solutions/prepare/docker/scripts/prepare.sh
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ if [[ "$PDS_DEBUG_ENABLED" = "true" ]]; then
echo " - Java jar OPTIONS : $options"
fi

if [[ "$PDS_WRAPPER_REMOTE_DEBUGGING_ENABLED" = "true" ]]; then
options="$options -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=8000"
fi

echo ""
java -jar $options "$prepare_wrapper"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,18 @@ private void startUpload(UUID jobUUID, HttpServletRequest request, String fileNa
/* prepare */
LOG.debug("Start upload file: {} for PDS job: {}", fileName, jobUUID);

JobStorage jobStorage = storageService.createJobStorage(null, jobUUID);

try {
store(jobUUID, request, fileName, jobStorage);

} finally {
jobStorage.close();
}
}

private void store(UUID jobUUID, HttpServletRequest request, String fileName, JobStorage jobStorage)
throws FileUploadException, IOException, UnsupportedEncodingException {
Long fileSizeFromUser = getFileSize(request);

String checksumFromUser = null;
Expand All @@ -138,8 +150,6 @@ private void startUpload(UUID jobUUID, HttpServletRequest request, String fileNa

long realContentLengthInBytes = -1;

JobStorage jobStorage = storageService.getJobStorage(jobUUID);

JakartaServletFileUpload<?, ?> upload = servletFileUploadFactory.create();

long maxUploadSize = configuration.getMaxUploadSizeInBytes();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,16 +194,20 @@ private void importWantedFilesFromJobStorage(UUID pdsJobUUID, PDSJobConfiguratio

LOG.debug("For pds jobUUID: {} following names are found in storage: {}", pdsJobUUID, names);

for (String name : names) {
try {
for (String name : names) {

if (isWantedStorageContent(name, configurationSupport, preparationContext)) {
resilientStorageReadExecutor.execute(() -> readAndCopyStorageToFileSystem(pdsJobUUID, jobFolder, storage, name),
"Read and copy storage: " + name + " for job: " + pdsJobUUID);
if (isWantedStorageContent(name, configurationSupport, preparationContext)) {
resilientStorageReadExecutor.execute(() -> readAndCopyStorageToFileSystem(pdsJobUUID, jobFolder, storage, name),
"Read and copy storage: " + name + " for job: " + pdsJobUUID);

} else {
LOG.debug("Did NOT import '{}' for job {} from storage - was not wanted", name, pdsJobUUID);
}
} else {
LOG.debug("Did NOT import '{}' for job {} from storage - was not wanted", name, pdsJobUUID);
}

}
} finally {
storage.close();
}
}

Expand Down Expand Up @@ -302,7 +306,7 @@ private JobStorage fetchStorage(UUID pdsJobUUID, PDSJobConfiguration config) {

LOG.debug("PDS job {}: feching storage for storagePath={}, {}-jobUUID={}, useSecHubStorage={}", pdsJobUUID, storagePath,
useSecHubStorage ? "sechub" : "pds", pdsOrSecHubJobUUID, useSecHubStorage);
JobStorage storage = storageService.getJobStorage(storagePath, pdsOrSecHubJobUUID);
JobStorage storage = storageService.createJobStorage(storagePath, pdsOrSecHubJobUUID);

storageInfoCollector.informFetchedStorage(storagePath, config.getSechubJobUUID(), pdsJobUUID, storage);

Expand Down Expand Up @@ -452,6 +456,7 @@ public void cleanup(UUID jobUUID, PDSJobConfiguration config) throws IOException
JobStorage storage = fetchStorage(jobUUID, config);
storage.deleteAll();
LOG.info("Removed storage for job {}", jobUUID);
storage.close();
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public PDSMultiStorageService(SharedVolumeSetup sharedVolumeSetup, S3Setup s3Set
}

@Override
public JobStorage getJobStorage(String storagePath, UUID jobUUID) {
public JobStorage createJobStorage(String storagePath, UUID jobUUID) {
if (storagePath == null) {
storagePath = serverConfigurationService.getStorageId();
LOG.debug("storage path parameter was null - fallback to default:{}", storagePath);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ void beforeEach() throws Exception {
when(archiveSupportProvider.getArchiveSupport()).thenReturn(archiveSupport);

storage = mock(JobStorage.class);
when(storageService.getJobStorage(jobUUID)).thenReturn(storage);
when(storageService.createJobStorage(null, jobUUID)).thenReturn(storage);

when(workspaceService.getUploadFolder(jobUUID)).thenReturn(new File(tmpUploadPath.toFile(), jobUUID.toString()));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ void beforeEach() {
PDSProductSetup setup = new PDSProductSetup();
when(serverConfigService.getProductSetupOrNull(any())).thenReturn(setup);

when(storageService.getJobStorage(any(), any())).thenReturn(storage);
when(storageService.createJobStorage(any(), any())).thenReturn(storage);

serviceToTest = new PDSWorkspaceService();
serviceToTest.storageService = storageService;
Expand Down Expand Up @@ -206,7 +206,7 @@ void when_configuration_tells_to_use_sechubstorage_sechub_storage_path_and_sechu
serviceToTest.prepare(jobUUID, config, null);

/* test */
verify(storageService).getJobStorage("xyz/abc/project1", config.getSechubJobUUID());
verify(storageService).createJobStorage("xyz/abc/project1", config.getSechubJobUUID());
}

private PDSExecutionParameterEntry createEntry(String key, String value) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ protected List<ProductResult> executeByAdapter(ProductExecutorData data) throws
UUID jobUUID = data.getSechubExecutionContext().getSechubJobUUID();
String projectId = data.getSechubExecutionContext().getConfiguration().getProjectId();

JobStorage storage = storageService.getJobStorage(projectId, jobUUID);
JobStorage storage = storageService.createJobStorage(projectId, jobUUID);

CheckmarxExecutorConfigSuppport configSupport = CheckmarxExecutorConfigSuppport
.createSupportAndAssertConfigValid(data.getProductExecutorContext().getExecutorConfig(), systemEnvironmentVariableSupport);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public class CheckmarxProductExecutorMockTest {
public void before() throws Exception {
JobStorage storage = Mockito.mock(JobStorage.class);
when(storage.fetch(any())).thenReturn(new StringInputStream("something as a code..."));
when(storageService.getJobStorage(any(), any())).thenReturn(storage);
when(storageService.createJobStorage(any(), any())).thenReturn(storage);

when(systemEnvironmentVariableSupport.getValueOrVariableContent("user")).thenReturn("checkmarx-user");
when(systemEnvironmentVariableSupport.getValueOrVariableContent("pwd")).thenReturn("checkmarx-password");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,11 @@ protected final List<ProductResult> executeByAdapter(ProductExecutorData data) t
configSupport.getDataTypesSupportedByPDSAsString());
return null;
}
return executeByAdapter(data, configSupport, contentProvider);

List<ProductResult> result = executeByAdapter(data, configSupport, contentProvider);
contentProvider.close();

return result;
}

protected abstract List<ProductResult> executeByAdapter(ProductExecutorData data, PDSExecutorConfigSupport configSupport,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ protected List<ProductResult> executeByAdapter(ProductExecutorData data, PDSExec
results.add(currentProductResult);

}
contentProvider.close();

return results;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,14 @@ public String getBinariesTarFileSizeOrNull() throws IOException {
}

}

/**
* Closes resources. After this the provider shall not be used anymore.
*/
public void close() {
if (storage == null) {
return;
}
storage.close();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public PDSStorageContentProvider createContentProvider(SecHubExecutionContext co
String projectId = model.getProjectId();
UUID jobUUID = context.getSechubJobUUID();

storage = storageService.getJobStorage(projectId, jobUUID);
storage = storageService.createJobStorage(projectId, jobUUID);
}

return new PDSStorageContentProvider(storage, reuseSecHubStorage, scanType, modelSupport, model);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ private void cleanupStorage(SecHubExecutionContext context) {
}
String projectId = configuration.getProjectId();
UUID jobUUID = context.getSechubJobUUID();
JobStorage storage = storageService.getJobStorage(projectId, jobUUID);
JobStorage storage = storageService.createJobStorage(projectId, jobUUID);

try {
storage.deleteAll();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public void before() throws Exception {
ProgressMonitor monitor = mock(ProgressMonitor.class);
when(monitor.getId()).thenReturn("monitor-test-id");

when(storageService.getJobStorage(any(), any())).thenReturn(jobStorage);
when(storageService.createJobStorage(any(), any())).thenReturn(jobStorage);
when(monitorFactory.createProgressMonitor(any())).thenReturn(monitor);

webScanProductExecutionService = mock(WebScanProductExecutionService.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,16 @@ private void handleUploadAndProblems(String projectId, UUID jobUUID, HttpServlet
}

private void startUpload(String projectId, UUID jobUUID, HttpServletRequest request) throws FileUploadException, IOException, UnsupportedEncodingException {
JobStorage jobStorage = storageService.createJobStorage(projectId, jobUUID);
try {
store(projectId, jobUUID, request, jobStorage);
} finally {
jobStorage.close();
}
}

private void store(String projectId, UUID jobUUID, HttpServletRequest request, JobStorage jobStorage)
throws FileUploadException, IOException, UnsupportedEncodingException {
/* prepare */
long binaryFileSizeFromUser = getBinaryFileSize(request);

Expand All @@ -146,8 +156,6 @@ private void startUpload(String projectId, UUID jobUUID, HttpServletRequest requ

long realContentLengthInBytes = -1;

JobStorage jobStorage = storageService.getJobStorage(projectId, jobUUID);

JakartaServletFileUpload<?, ?> upload = servletFileUploadFactory.create();

long maxUploadSize = configuration.getMaxUploadSizeInBytes();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,15 @@ public void uploadSourceCode(String projectId, UUID jobUUID, MultipartFile file,
}

private void storeUploadFileAndSha256Checksum(String projectId, UUID jobUUID, MultipartFile file, String checkSum, String traceLogID) {
JobStorage jobStorage = storageService.getJobStorage(projectId, jobUUID);
JobStorage jobStorage = storageService.createJobStorage(projectId, jobUUID);
try {
store(projectId, jobUUID, file, checkSum, traceLogID, jobStorage);
} finally {
jobStorage.close();
}
}

private void store(String projectId, UUID jobUUID, MultipartFile file, String checkSum, String traceLogID, JobStorage jobStorage) {
try (InputStream inputStream = file.getInputStream()) {
long fileSize = file.getSize();

Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,11 @@
// SPDX-License-Identifier: MIT
package com.mercedesbenz.sechub.domain.schedule;

import static com.mercedesbenz.sechub.commons.core.CommonConstants.FILE_SIZE_HEADER_FIELD_NAME;
import static com.mercedesbenz.sechub.test.JUnitAssertionAddon.assertThrowsExceptionContainingMessage;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;
import static com.mercedesbenz.sechub.commons.core.CommonConstants.*;
import static com.mercedesbenz.sechub.test.JUnitAssertionAddon.*;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.ArgumentMatchers.*;
import static org.mockito.Mockito.*;

import java.io.ByteArrayInputStream;
import java.io.IOException;
Expand Down Expand Up @@ -71,7 +66,7 @@ void beforeEach() {

ScheduleSecHubJob job = new ScheduleSecHubJob();
when(assertService.assertJob(PROJECT1, randomUuid)).thenReturn(job);
when(storageService.getJobStorage(PROJECT1, randomUuid)).thenReturn(storage);
when(storageService.createJobStorage(PROJECT1, randomUuid)).thenReturn(storage);

/* attach at service to test */
serviceToTest = new SchedulerBinariesUploadService();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ void beforeEach() throws IOException {
ScheduleSecHubJob job = new ScheduleSecHubJob();
when(mockedAssertService.assertJob(PROJECT1, randomUuid)).thenReturn(job);
storage = mock(JobStorage.class);
when(mockedStorageService.getJobStorage(PROJECT1, randomUuid)).thenReturn(storage);
when(mockedStorageService.createJobStorage(PROJECT1, randomUuid)).thenReturn(storage);

file = mock(MultipartFile.class);
when(file.getSize()).thenReturn(1024L); // just not empty
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ public ResponseEntity<Resource> getUploadedFile(@PathVariable("projectId") Strin
}
LOG.info("Integration test server: getJobStorage for {} {}", logSanitizer.sanitize(projectId, 30), jobUUID);

JobStorage storage = storageService.getJobStorage(projectId, jobUUID);
JobStorage storage = storageService.createJobStorage(projectId, jobUUID);
if (!storage.isExisting(fileName)) {
throw new NotFoundException("file not uploaded:" + fileName);
}
Expand All @@ -218,10 +218,14 @@ public ResponseEntity<Resource> getUploadedFile(@PathVariable("projectId") Strin

/* @formatter:off */
InputStreamResource resource = new InputStreamResource(inputStream);
return ResponseEntity.ok()
ResponseEntity<Resource> result = ResponseEntity.ok()
.headers(headers)
.contentType(MediaType.parseMediaType("application/octet-stream"))
.body(resource);

storage.close();

return result;
/* @formatter:on */

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public MultiStorageService(SharedVolumeSetup sharedVolumeSetup, S3Setup s3Setup)
}

@Override
public JobStorage getJobStorage(String projectId, UUID jobUUID) {
public JobStorage createJobStorage(String projectId, UUID jobUUID) {
/*
* we use here "jobstorage/${projectId} - so we have same job storage path as
* before in sechub itself - for PDS own prefix (storageId) is used insdide
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,11 @@ public interface JobStorage {
*/
public Set<String> listNames() throws IOException;

/**
* Closes this storage - will cleanup resources etc. After this method is called
* no interaction with storage is allowed any longer. The close method itself
* can be called multiple times.
*/
public void close();

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ public interface JobStorageFactory {
* @return job storage, never <code>null</code>
*/
public JobStorage createJobStorage(String storagePath, UUID jobUUID);

}
Loading

0 comments on commit c292267

Please sign in to comment.