From 4be8ce25181b42c54ecdd67efaa3e4e07535c5f3 Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Tue, 20 Aug 2024 16:24:28 -0700 Subject: [PATCH] adds toggling refresh disable/enable for deactivate/activate operation while updating URL_DOWNLOAD type configs (#1240) (#1246) * adds toggling refresh disable/enable for deactivate/activate operation while updating URL_DOWNLOAD type configs * add sleep for lock issue * wait on release lock operation completion before returning update tif source config response * reset flag after integ tests for SourceConfigWithoutS3RestApiIT --------- (cherry picked from commit 3e1f59d00125f522f565014bb7bd4d8ea8df2d73) Signed-off-by: Surya Sashank Nistala Signed-off-by: github-actions[bot] Co-authored-by: github-actions[bot] --- .../action/SAGetTIFSourceConfigResponse.java | 20 +--- .../SAIndexTIFSourceConfigResponse.java | 26 ++--- .../threatIntel/common/TIFLockService.java | 15 ++- .../model/SATIFSourceConfigDto.java | 19 ++-- .../SATIFSourceConfigManagementService.java | 6 +- .../TransportIndexTIFSourceConfigAction.java | 56 ++++++++--- .../SourceConfigWithoutS3RestApiIT.java | 95 +++++++++++++++++-- 7 files changed, 169 insertions(+), 68 deletions(-) diff --git a/src/main/java/org/opensearch/securityanalytics/threatIntel/action/SAGetTIFSourceConfigResponse.java b/src/main/java/org/opensearch/securityanalytics/threatIntel/action/SAGetTIFSourceConfigResponse.java index 247bcd134..7bebd8fb1 100644 --- a/src/main/java/org/opensearch/securityanalytics/threatIntel/action/SAGetTIFSourceConfigResponse.java +++ b/src/main/java/org/opensearch/securityanalytics/threatIntel/action/SAGetTIFSourceConfigResponse.java @@ -63,25 +63,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject() .field(_ID, id) .field(_VERSION, version); - builder.startObject("source_config") - .field(SATIFSourceConfigDto.NAME_FIELD, saTifSourceConfigDto.getName()) - .field(SATIFSourceConfigDto.FORMAT_FIELD, saTifSourceConfigDto.getFormat()) - .field(SATIFSourceConfigDto.TYPE_FIELD, saTifSourceConfigDto.getType()) - .field(SATIFSourceConfigDto.IOC_TYPES_FIELD, saTifSourceConfigDto.getIocTypes()) - .field(SATIFSourceConfigDto.DESCRIPTION_FIELD, saTifSourceConfigDto.getDescription()) - .field(SATIFSourceConfigDto.CREATED_BY_USER_FIELD, saTifSourceConfigDto.getCreatedByUser()) - .field(SATIFSourceConfigDto.CREATED_AT_FIELD, saTifSourceConfigDto.getCreatedAt()) - .field(SATIFSourceConfigDto.SOURCE_FIELD, saTifSourceConfigDto.getSource()) - .field(SATIFSourceConfigDto.ENABLED_FIELD, saTifSourceConfigDto.isEnabled()) - .field(SATIFSourceConfigDto.ENABLED_TIME_FIELD, saTifSourceConfigDto.getEnabledTime()) - .field(SATIFSourceConfigDto.LAST_UPDATE_TIME_FIELD, saTifSourceConfigDto.getLastUpdateTime()) - .field(SATIFSourceConfigDto.SCHEDULE_FIELD, saTifSourceConfigDto.getSchedule()) - .field(SATIFSourceConfigDto.STATE_FIELD, saTifSourceConfigDto.getState()) - .field(SATIFSourceConfigDto.REFRESH_TYPE_FIELD, saTifSourceConfigDto.getRefreshType()) - .field(SATIFSourceConfigDto.LAST_REFRESHED_USER_FIELD, saTifSourceConfigDto.getLastRefreshedUser()) - .field(SATIFSourceConfigDto.LAST_REFRESHED_TIME_FIELD, saTifSourceConfigDto.getLastRefreshedTime()); - - builder.endObject(); + saTifSourceConfigDto.innerXcontent(builder); return builder.endObject(); } diff --git a/src/main/java/org/opensearch/securityanalytics/threatIntel/action/SAIndexTIFSourceConfigResponse.java b/src/main/java/org/opensearch/securityanalytics/threatIntel/action/SAIndexTIFSourceConfigResponse.java index 7a1881162..209563f7c 100644 --- a/src/main/java/org/opensearch/securityanalytics/threatIntel/action/SAIndexTIFSourceConfigResponse.java +++ b/src/main/java/org/opensearch/securityanalytics/threatIntel/action/SAIndexTIFSourceConfigResponse.java @@ -17,6 +17,7 @@ import java.io.IOException; +import static org.opensearch.securityanalytics.threatIntel.model.SATIFSourceConfigDto.SOURCE_CONFIG_FIELD; import static org.opensearch.securityanalytics.util.RestHandlerUtils._ID; import static org.opensearch.securityanalytics.util.RestHandlerUtils._VERSION; @@ -56,40 +57,25 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject() .field(_ID, id) .field(_VERSION, version); - - builder.startObject("source_config") - .field(SATIFSourceConfigDto.NAME_FIELD, saTifSourceConfigDto.getName()) - .field(SATIFSourceConfigDto.FORMAT_FIELD, saTifSourceConfigDto.getFormat()) - .field(SATIFSourceConfigDto.TYPE_FIELD, saTifSourceConfigDto.getType()) - .field(SATIFSourceConfigDto.IOC_TYPES_FIELD, saTifSourceConfigDto.getIocTypes()) - .field(SATIFSourceConfigDto.DESCRIPTION_FIELD, saTifSourceConfigDto.getDescription()) - .field(SATIFSourceConfigDto.CREATED_BY_USER_FIELD, saTifSourceConfigDto.getCreatedByUser()) - .field(SATIFSourceConfigDto.CREATED_AT_FIELD, saTifSourceConfigDto.getCreatedAt()) - .field(SATIFSourceConfigDto.SOURCE_FIELD, saTifSourceConfigDto.getSource()) - .field(SATIFSourceConfigDto.ENABLED_FIELD, saTifSourceConfigDto.isEnabled()) - .field(SATIFSourceConfigDto.ENABLED_TIME_FIELD, saTifSourceConfigDto.getEnabledTime()) - .field(SATIFSourceConfigDto.LAST_UPDATE_TIME_FIELD, saTifSourceConfigDto.getLastUpdateTime()) - .field(SATIFSourceConfigDto.SCHEDULE_FIELD, saTifSourceConfigDto.getSchedule()) - .field(SATIFSourceConfigDto.STATE_FIELD, saTifSourceConfigDto.getState()) - .field(SATIFSourceConfigDto.REFRESH_TYPE_FIELD, saTifSourceConfigDto.getRefreshType()) - .field(SATIFSourceConfigDto.LAST_REFRESHED_USER_FIELD, saTifSourceConfigDto.getLastRefreshedUser()) - .field(SATIFSourceConfigDto.LAST_REFRESHED_TIME_FIELD, saTifSourceConfigDto.getLastRefreshedTime()); - - builder.endObject(); + saTifSourceConfigDto.innerXcontent(builder); return builder.endObject(); } + @Override public String getTIFConfigId() { return id; } + @Override public Long getVersion() { return version; } + @Override public TIFSourceConfigDto getTIFConfigDto() { return saTifSourceConfigDto; } + public RestStatus getStatus() { return status; } diff --git a/src/main/java/org/opensearch/securityanalytics/threatIntel/common/TIFLockService.java b/src/main/java/org/opensearch/securityanalytics/threatIntel/common/TIFLockService.java index 8b3917791..f88aebd9b 100644 --- a/src/main/java/org/opensearch/securityanalytics/threatIntel/common/TIFLockService.java +++ b/src/main/java/org/opensearch/securityanalytics/threatIntel/common/TIFLockService.java @@ -46,7 +46,7 @@ public TIFLockService(final ClusterService clusterService, final Client client) } /** - * Synchronous method of #acquireLock + * Event-driven method of #acquireLock * * @param tifJobName tifJobName to acquire lock on * @param lockDurationSeconds the lock duration in seconds @@ -81,6 +81,19 @@ public void releaseLock(final LockModel lockModel) { ); } + /** + * Wrapper method of LockService#release + * + * @param lockModel the lock model + */ + public void releaseLockEventDriven(final LockModel lockModel, final ActionListener listener) { + log.debug("Releasing lock with id [{}]", lockModel.getLockId()); + lockService.release( + lockModel, + ActionListener.wrap(listener::onResponse, exception -> log.error("Failed to release the lock", exception)) + ); + } + /** * Synchronous method of LockService#renewLock * diff --git a/src/main/java/org/opensearch/securityanalytics/threatIntel/model/SATIFSourceConfigDto.java b/src/main/java/org/opensearch/securityanalytics/threatIntel/model/SATIFSourceConfigDto.java index a293c881e..3ba64d47a 100644 --- a/src/main/java/org/opensearch/securityanalytics/threatIntel/model/SATIFSourceConfigDto.java +++ b/src/main/java/org/opensearch/securityanalytics/threatIntel/model/SATIFSourceConfigDto.java @@ -215,12 +215,18 @@ public void writeTo(final StreamOutput out) throws IOException { @Override public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { - builder.startObject() - .startObject(SOURCE_CONFIG_FIELD) - .field(NAME_FIELD, name) - .field(FORMAT_FIELD, format) - .field(TYPE_FIELD, type.name()) - .field(DESCRIPTION_FIELD, description); + builder.startObject(); + innerXcontent(builder); + builder.endObject(); + return builder; + } + + public XContentBuilder innerXcontent(XContentBuilder builder) throws IOException { + builder.startObject(SOURCE_CONFIG_FIELD); + builder.field(NAME_FIELD, name) + .field(FORMAT_FIELD, format) + .field(TYPE_FIELD, type.name()) + .field(DESCRIPTION_FIELD, description); if (createdByUser == null) { builder.nullField(CREATED_BY_USER_FIELD); } else { @@ -274,7 +280,6 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa builder.field(ENABLED_FOR_SCAN_FIELD, enabledForScan); builder.field(IOC_TYPES_FIELD, iocTypes); builder.endObject(); - builder.endObject(); return builder; } diff --git a/src/main/java/org/opensearch/securityanalytics/threatIntel/service/SATIFSourceConfigManagementService.java b/src/main/java/org/opensearch/securityanalytics/threatIntel/service/SATIFSourceConfigManagementService.java index 2e2c0a5c6..f7869c1df 100644 --- a/src/main/java/org/opensearch/securityanalytics/threatIntel/service/SATIFSourceConfigManagementService.java +++ b/src/main/java/org/opensearch/securityanalytics/threatIntel/service/SATIFSourceConfigManagementService.java @@ -280,6 +280,10 @@ public void updateIocAndTIFSourceConfig( // Due to the lack of a different API to do activate/deactivate we will check if enabled_for_scan variable is changed between model and request. // If yes, we will ONLY update enabled_for_scan field and ignore any updates to the rest of the fields to simulate a dedicated activate/deactivate API. if (retrievedSaTifSourceConfig.isEnabledForScan() != saTifSourceConfigDto.isEnabledForScan()) { + // FIXME add a disable_refresh api independent of update api so that it can be supported for default configs also + boolean isEnabled = URL_DOWNLOAD.equals(retrievedSaTifSourceConfig.getType()) ? + saTifSourceConfigDto.isEnabledForScan() : + retrievedSaTifSourceConfig.isEnabled(); SATIFSourceConfig config = new SATIFSourceConfig( retrievedSaTifSourceConfig.getId(), retrievedSaTifSourceConfig.getVersion(), @@ -297,7 +301,7 @@ public void updateIocAndTIFSourceConfig( retrievedSaTifSourceConfig.getRefreshType(), Instant.now(), updatedByUser, - retrievedSaTifSourceConfig.isEnabled(), + isEnabled, retrievedSaTifSourceConfig.getIocStoreConfig(), retrievedSaTifSourceConfig.getIocTypes(), saTifSourceConfigDto.isEnabledForScan() // update only enabled_for_scan diff --git a/src/main/java/org/opensearch/securityanalytics/threatIntel/transport/TransportIndexTIFSourceConfigAction.java b/src/main/java/org/opensearch/securityanalytics/threatIntel/transport/TransportIndexTIFSourceConfigAction.java index 440191146..d12e4542e 100644 --- a/src/main/java/org/opensearch/securityanalytics/threatIntel/transport/TransportIndexTIFSourceConfigAction.java +++ b/src/main/java/org/opensearch/securityanalytics/threatIntel/transport/TransportIndexTIFSourceConfigAction.java @@ -91,10 +91,10 @@ private void retrieveLockAndCreateTIFConfig(SAIndexTIFSourceConfigRequest reques try { lockService.acquireLock(request.getTIFConfigDto().getId(), LOCK_DURATION_IN_SECONDS, ActionListener.wrap(lock -> { if (lock == null) { + log.error("another processor is a lock, BAD_REQUEST error", RestStatus.BAD_REQUEST); listener.onFailure( new ConcurrentModificationException("another processor is holding a lock on the resource. Try again later") ); - log.error("another processor is a lock, BAD_REQUEST error", RestStatus.BAD_REQUEST); return; } try { @@ -106,29 +106,59 @@ private void retrieveLockAndCreateTIFConfig(SAIndexTIFSourceConfigRequest reques user, ActionListener.wrap( saTifSourceConfigDtoResponse -> { - lockService.releaseLock(lock); - listener.onResponse(new SAIndexTIFSourceConfigResponse( - saTifSourceConfigDtoResponse.getId(), - saTifSourceConfigDtoResponse.getVersion(), - RestStatus.OK, - saTifSourceConfigDtoResponse + lockService.releaseLockEventDriven(lock, ActionListener.wrap( + r -> listener.onResponse(new SAIndexTIFSourceConfigResponse( + saTifSourceConfigDtoResponse.getId(), + saTifSourceConfigDtoResponse.getVersion(), + RestStatus.OK, + saTifSourceConfigDtoResponse + )), + e -> { + log.error(String.format("Unexpected failure while trying to release lock [%s] for tif source config [%s].", lock.getLockId(), saTifSourceConfigDto.getId()), e); + listener.onResponse(new SAIndexTIFSourceConfigResponse( + saTifSourceConfigDtoResponse.getId(), + saTifSourceConfigDtoResponse.getVersion(), + RestStatus.OK, + saTifSourceConfigDtoResponse + )); + } )); }, e -> { - lockService.releaseLock(lock); - log.error("Failed to create IOCs and threat intel source config"); - listener.onFailure(e); + lockService.releaseLockEventDriven(lock, ActionListener.wrap( + r -> { + log.error("Failed to create IOCs and threat intel source config", e); + listener.onFailure(e); + }, + ex -> { + String action = RestRequest.Method.PUT.equals(request.getMethod()) ? "update" : "create"; + log.error(String.format("Failed to %s IOCs and threat intel source config", action), e); + log.error(String.format("Unexpected failure while trying to release lock [%s] for tif source config.", lock.getLockId()), e); + listener.onFailure(e); + } + )); } ) ); } catch (Exception e) { - lockService.releaseLock(lock); - listener.onFailure(e); log.error("listener failed when executing", e); + lockService.releaseLockEventDriven(lock, ActionListener.wrap( + r -> { + log.error("Failed to create IOCs and threat intel source config", e); + listener.onFailure(e); + }, + ex -> { + String action = RestRequest.Method.PUT.equals(request.getMethod()) ? "update" : "create"; + log.error(String.format("Failed to %s IOCs and threat intel source config", action), e); + log.error(String.format("Unexpected failure while trying to release lock [%s] for tif source config.", lock.getLockId()), e); + listener.onFailure(e); + } + )); } }, exception -> { + String action = RestRequest.Method.PUT.equals(request.getMethod()) ? "update" : "create"; + log.error(String.format("Failed to acquire lock while trying to %s tif source config", action), exception); listener.onFailure(exception); - log.error("execution failed", exception); })); } catch (Exception e) { log.error("Failed to acquire lock for job", e); diff --git a/src/test/java/org/opensearch/securityanalytics/resthandler/SourceConfigWithoutS3RestApiIT.java b/src/test/java/org/opensearch/securityanalytics/resthandler/SourceConfigWithoutS3RestApiIT.java index 862983efc..bef7999ed 100644 --- a/src/test/java/org/opensearch/securityanalytics/resthandler/SourceConfigWithoutS3RestApiIT.java +++ b/src/test/java/org/opensearch/securityanalytics/resthandler/SourceConfigWithoutS3RestApiIT.java @@ -35,6 +35,7 @@ import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -361,7 +362,6 @@ public void testUpdateIocUploadSourceConfig() throws IOException, InterruptedExc iocHits = (List>) respMap.get(ListIOCsActionResponse.HITS_FIELD); assertEquals(1, iocHits.size()); - Thread.sleep(10000); } public void testActivateDeactivateIocUploadSourceConfig() throws IOException, InterruptedException { @@ -473,10 +473,17 @@ public void testActivateDeactivateIocUploadSourceConfig() throws IOException, In iocTypes, false ); - Thread.sleep(10000); // update source config with hashes ioc type response = makeRequest(client(), "PUT", SecurityAnalyticsPlugin.THREAT_INTEL_SOURCE_URI +"/" + createdId, Collections.emptyMap(), toHttpEntity(saTifSourceConfigDto)); Assert.assertEquals(RestStatus.OK, restStatus(response)); + Map updateResponseAsMap = asMap(response); + assertNotNull(updateResponseAsMap); + assertTrue(updateResponseAsMap.containsKey("source_config")); + HashMap scr = (HashMap) updateResponseAsMap.get("source_config"); + assertTrue(scr.containsKey("enabled")); + assertFalse((Boolean) scr.get("enabled")); + assertTrue(scr.containsKey("enabled_for_scan")); + assertFalse((Boolean) scr.get("enabled_for_scan")); // Ensure that old ioc indices are retained (2 created from ioc upload source config + 1 from default source config) List findingIndices = getIocIndices(); @@ -493,7 +500,39 @@ public void testActivateDeactivateIocUploadSourceConfig() throws IOException, In iocHits = (List>) respMap.get(ListIOCsActionResponse.HITS_FIELD); assertEquals(1, iocHits.size()); - Thread.sleep(10000); + + saTifSourceConfigDto = new SATIFSourceConfigDto( + saTifSourceConfigDto.getId(), + null, + feedName, + feedFormat, + sourceConfigType, + null, + null, + null, + iocUploadSource, + null, + null, + null, + null, + null, + null, + null, + enabled, + iocTypes, true + ); + + // update source config with hashes ioc type + response = makeRequest(client(), "PUT", SecurityAnalyticsPlugin.THREAT_INTEL_SOURCE_URI +"/" + createdId, Collections.emptyMap(), toHttpEntity(saTifSourceConfigDto)); + Assert.assertEquals(RestStatus.OK, restStatus(response)); + updateResponseAsMap = asMap(response); + assertNotNull(updateResponseAsMap); + assertTrue(updateResponseAsMap.containsKey("source_config")); + scr = (HashMap) updateResponseAsMap.get("source_config"); + assertTrue(scr.containsKey("enabled")); + assertFalse((Boolean) scr.get("enabled")); // since its not url_download type, this flag should remain unaffected by the activate action in update source api + assertTrue(scr.containsKey("enabled_for_scan")); + assertTrue((Boolean) scr.get("enabled_for_scan")); } public void testActivateDeactivateUrlDownloadSourceConfig() throws IOException, InterruptedException { @@ -546,19 +585,58 @@ public void testActivateDeactivateUrlDownloadSourceConfig() throws IOException, // update default source config with enabled_for_scan updated Response response = makeRequest(client(), "PUT", SecurityAnalyticsPlugin.THREAT_INTEL_SOURCE_URI +"/" + id, Collections.emptyMap(), toHttpEntity(saTifSourceConfigDto)); Assert.assertEquals(RestStatus.OK, restStatus(response)); + Map updateResponseAsMap = asMap(response); + assertNotNull(updateResponseAsMap); + assertTrue(updateResponseAsMap.containsKey("source_config")); + HashMap scr = (HashMap) updateResponseAsMap.get("source_config"); + assertTrue(scr.containsKey("enabled")); + assertFalse((Boolean) scr.get("enabled")); + assertTrue(scr.containsKey("enabled_for_scan")); + assertFalse((Boolean) scr.get("enabled_for_scan")); // Ensure that only 1 ioc index is present from default source List findingIndices = getIocIndices(); Assert.assertEquals(1, findingIndices.size()); - Thread.sleep(100); // TODO: pass in action listener when releasing lock - // try to update default source config again to ensure operation is not accepted when enabled_for_scan is unchanged try { makeRequest(client(), "PUT", SecurityAnalyticsPlugin.THREAT_INTEL_SOURCE_URI +"/" + id, Collections.emptyMap(), toHttpEntity(saTifSourceConfigDto)); } catch (Exception e) { Assert.assertTrue(e.getMessage().contains("unsupported_operation_exception")); } + // activate source + saTifSourceConfigDto = new SATIFSourceConfigDto( + id, + null, + feedName, + feedFormat, + sourceConfigType, + null, + null, + null, + urlDownloadSource, + null, + null, + schedule, + null, + null, + null, + null, + enabled, + iocTypes, true + ); + + // update default source config with enabled_for_scan updated + response = makeRequest(client(), "PUT", SecurityAnalyticsPlugin.THREAT_INTEL_SOURCE_URI +"/" + id, Collections.emptyMap(), toHttpEntity(saTifSourceConfigDto)); + Assert.assertEquals(RestStatus.OK, restStatus(response)); + updateResponseAsMap = asMap(response); + assertNotNull(updateResponseAsMap); + assertTrue(updateResponseAsMap.containsKey("source_config")); + scr = (HashMap) updateResponseAsMap.get("source_config"); + assertTrue(scr.containsKey("enabled")); + assertTrue((Boolean) scr.get("enabled")); + assertTrue(scr.containsKey("enabled_for_scan")); + assertTrue((Boolean) scr.get("enabled_for_scan")); } public void testDeleteIocUploadSourceConfigAndAllIocs() throws IOException { @@ -860,8 +938,6 @@ public void testUpdateDefaultSourceConfigThrowsError() throws IOException, Inter Assert.assertTrue(e.getMessage().contains("unsupported_operation_exception")); } - Thread.sleep(100); // TODO: pass in action listener when releasing lock - // update default source config again to ensure lock was released try { makeRequest(client(), "PUT", SecurityAnalyticsPlugin.THREAT_INTEL_SOURCE_URI +"/" + id, Collections.emptyMap(), toHttpEntity(saTifSourceConfigDto)); @@ -869,4 +945,9 @@ public void testUpdateDefaultSourceConfigThrowsError() throws IOException, Inter Assert.assertTrue(e.getMessage().contains("unsupported_operation_exception")); } } + + @Override + protected boolean preserveIndicesUponCompletion() { + return false; + } }