Skip to content

Commit 30edec0

Browse files
engechasengechas
authored
Delete detector successfully if workflow is missing (opensearch-project#790) (opensearch-project#809)
* Delete detector successfully if workflow is missing Signed-off-by: Chase Engelbrecht <[email protected]> * Refactor to use existing NotFound exception checker Signed-off-by: Chase Engelbrecht <[email protected]> --------- Signed-off-by: Chase Engelbrecht <[email protected]> (cherry picked from commit 0ad91cc)
1 parent bd2ad2b commit 30edec0

File tree

3 files changed

+88
-28
lines changed

3 files changed

+88
-28
lines changed

‎src/main/java/org/opensearch/securityanalytics/transport/TransportDeleteDetectorAction.java

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ public void onResponse(Collection<DeleteMonitorResponse> responses) {
204204

205205
@Override
206206
public void onFailure(Exception e) {
207-
if (isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListener(e, detector.getId())) {
207+
if (isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(e, detector.getId())) {
208208
deleteDetectorFromConfig(detector.getId(), request.getRefreshPolicy());
209209
} else {
210210
log.error(String.format(Locale.ROOT, "Failed to delete detector %s", detector.getId()), e);
@@ -231,15 +231,25 @@ private void deleteWorkflow(Detector detector, ActionListener<AcknowledgedRespon
231231
log.debug(String.format("Deleting the workflow %s before deleting the detector", workflowId));
232232
StepListener<DeleteWorkflowResponse> onDeleteWorkflowStep = new StepListener<>();
233233
workflowService.deleteWorkflow(workflowId, onDeleteWorkflowStep);
234-
onDeleteWorkflowStep.whenComplete(deleteWorkflowResponse -> {
235-
actionListener.onResponse(new AcknowledgedResponse(true));
236-
}, actionListener::onFailure);
234+
onDeleteWorkflowStep.whenComplete(
235+
deleteWorkflowResponse -> actionListener.onResponse(new AcknowledgedResponse(true)),
236+
deleteWorkflowResponse -> handleDeleteWorkflowFailure(detector.getId(), deleteWorkflowResponse, actionListener)
237+
);
237238
} else {
238239
// If detector doesn't have the workflows it means that older version of the plugin is used and just skip the step
239240
actionListener.onResponse(new AcknowledgedResponse(true));
240241
}
241242
}
242243

244+
private void handleDeleteWorkflowFailure(final String detectorId, final Exception deleteWorkflowException,
245+
final ActionListener<AcknowledgedResponse> actionListener) {
246+
if (isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(deleteWorkflowException, detectorId)) {
247+
actionListener.onResponse(new AcknowledgedResponse(true));
248+
} else {
249+
actionListener.onFailure(deleteWorkflowException);
250+
}
251+
}
252+
243253
private void deleteDetectorFromConfig(String detectorId, WriteRequest.RefreshPolicy refreshPolicy) {
244254
deleteDetector(detectorId, refreshPolicy,
245255
new ActionListener<>() {
@@ -296,7 +306,7 @@ private void finishHim(String detectorId, Exception t) {
296306
}));
297307
}
298308

299-
private boolean isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListener(
309+
private boolean isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(
300310
Exception ex,
301311
String detectorId
302312
) {
@@ -305,12 +315,9 @@ private boolean isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListene
305315
int len = ex.getSuppressed().length;
306316
for (int i = 0; i <= len; i++) {
307317
Throwable e = i == len ? ex : ex.getSuppressed()[i];
308-
if (e.getMessage().matches("(.*)Monitor(.*) is not found(.*)")
309-
|| e.getMessage().contains(
310-
"Configured indices are not found: [.opendistro-alerting-config]")
311-
) {
318+
if (isMonitorNotFoundException(e) || isWorkflowNotFoundException(e) || isAlertingConfigIndexNotFoundException(e)) {
312319
log.error(
313-
String.format(Locale.ROOT, "Monitor or jobs index already deleted." +
320+
String.format(Locale.ROOT, "Workflow, monitor, or jobs index already deleted." +
314321
" Proceeding with detector %s deletion", detectorId),
315322
e);
316323
} else {
@@ -321,6 +328,18 @@ private boolean isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListene
321328
}
322329
}
323330

331+
private boolean isMonitorNotFoundException(final Throwable e) {
332+
return e.getMessage().matches("(.*)Monitor(.*) is not found(.*)");
333+
}
334+
335+
private boolean isWorkflowNotFoundException(final Throwable e) {
336+
return e.getMessage().matches("(.*)Workflow(.*) not found(.*)");
337+
}
338+
339+
private boolean isAlertingConfigIndexNotFoundException(final Throwable e) {
340+
return e.getMessage().contains("Configured indices are not found: [.opendistro-alerting-config]");
341+
}
342+
324343
private void setEnabledWorkflowUsage(boolean enabledWorkflowUsage) {
325344
this.enabledWorkflowUsage = enabledWorkflowUsage;
326345
}

‎src/test/java/org/opensearch/securityanalytics/SecurityAnalyticsRestTestCase.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,14 @@ protected Response executeAlertingWorkflow(RestClient client, String workflowId,
400400
return makeRequest(client, "POST", String.format(Locale.getDefault(), "/_plugins/_alerting/workflows/%s/_execute", workflowId), params, null);
401401
}
402402

403+
protected Response deleteAlertingWorkflow(String workflowId) throws IOException {
404+
return deleteAlertingWorkflow(client(), workflowId);
405+
}
406+
407+
protected Response deleteAlertingWorkflow(RestClient client, String workflowId) throws IOException {
408+
return makeRequest(client, "DELETE", String.format(Locale.getDefault(), "/_plugins/_alerting/workflows/%s", workflowId), new HashMap<>(), null);
409+
}
410+
403411
protected List<SearchHit> executeSearch(String index, String request) throws IOException {
404412
return executeSearch(index, request, true);
405413
}

‎src/test/java/org/opensearch/securityanalytics/resthandler/DetectorRestApiIT.java

Lines changed: 51 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.opensearch.client.ResponseException;
2424
import org.opensearch.commons.alerting.model.IntervalSchedule;
2525
import org.opensearch.commons.alerting.model.Monitor.MonitorType;
26+
import org.opensearch.commons.alerting.model.ScheduledJob;
2627
import org.opensearch.core.rest.RestStatus;
2728
import org.opensearch.core.xcontent.MediaTypeRegistry;
2829
import org.opensearch.search.SearchHit;
@@ -72,10 +73,34 @@ public void testNewLogTypes() throws IOException {
7273
@SuppressWarnings("unchecked")
7374
public void testDeletingADetector_MonitorNotExists() throws IOException {
7475
updateClusterSetting(ENABLE_WORKFLOW_USAGE.getKey(), "false");
75-
String index = createTestIndex(randomIndex(), windowsIndexMapping());
76+
final String detectorId = setupDetector();
77+
final Map<String, Object> detectorSourceAsMap = getDetectorSourceAsMap(detectorId);
78+
79+
final String monitorId = ((List<String>) detectorSourceAsMap.get("monitor_id")).get(0);
80+
final Response deleteMonitorResponse = deleteAlertingMonitor(monitorId);
81+
assertEquals(200, deleteMonitorResponse.getStatusLine().getStatusCode());
82+
entityAsMap(deleteMonitorResponse);
83+
84+
validateDetectorDeletion(detectorId);
85+
}
86+
87+
public void testDeletingADetector_WorkflowUsageEnabled_WorkflowDoesntExist() throws IOException {
88+
final String detectorId = setupDetector();
89+
final Map<String, Object> detectorSourceAsMap = getDetectorSourceAsMap(detectorId);
90+
91+
final String workflowId = ((List<String>) detectorSourceAsMap.get("workflow_ids")).get(0);
92+
final Response deleteWorkflowResponse = deleteAlertingWorkflow(workflowId);
93+
assertEquals(200, deleteWorkflowResponse.getStatusLine().getStatusCode());
94+
entityAsMap(deleteWorkflowResponse);
95+
96+
validateDetectorDeletion(detectorId);
97+
}
98+
99+
private String setupDetector() throws IOException {
100+
final String index = createTestIndex(randomIndex(), windowsIndexMapping());
76101

77102
// Execute CreateMappingsAction to add alias mapping for index
78-
Request createMappingRequest = new Request("POST", SecurityAnalyticsPlugin.MAPPER_BASE_URI);
103+
final Request createMappingRequest = new Request("POST", SecurityAnalyticsPlugin.MAPPER_BASE_URI);
79104
// both req params and req body are supported
80105
createMappingRequest.setJsonEntity(
81106
"{ \"index_name\":\"" + index + "\"," +
@@ -84,31 +109,39 @@ public void testDeletingADetector_MonitorNotExists() throws IOException {
84109
"}"
85110
);
86111

87-
Response response = client().performRequest(createMappingRequest);
112+
final Response response = client().performRequest(createMappingRequest);
88113
assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode());
89-
// Create detector #1 of type test_windows
90-
Detector detector1 = randomDetectorWithTriggers(getRandomPrePackagedRules(), List.of(new DetectorTrigger(null, "test-trigger", "1", List.of(randomDetectorType()), List.of(), List.of(), List.of(), List.of(), List.of())));
91-
String detectorId1 = createDetector(detector1);
114+
// Create detector of type test_windows
115+
final DetectorTrigger detectorTrigger = new DetectorTrigger(null, "test-trigger", "1", List.of(randomDetectorType()),
116+
List.of(), List.of(), List.of(), List.of(), List.of());
117+
final Detector detector = randomDetectorWithTriggers(getRandomPrePackagedRules(), List.of(detectorTrigger));
118+
return createDetector(detector);
119+
}
92120

93-
String request = "{\n" +
121+
private Map<String, Object> getDetectorSourceAsMap(final String detectorId) throws IOException {
122+
final String request = getDetectorQuery(detectorId);
123+
final List<SearchHit> hits = executeSearch(Detector.DETECTORS_INDEX, request);
124+
final SearchHit hit = hits.get(0);
125+
return (Map<String, Object>) hit.getSourceAsMap().get("detector");
126+
}
127+
128+
private String getDetectorQuery(final String detectorId) {
129+
return "{\n" +
94130
" \"query\" : {\n" +
95131
" \"match\":{\n" +
96-
" \"_id\": \"" + detectorId1 + "\"\n" +
132+
" \"_id\": \"" + detectorId + "\"\n" +
97133
" }\n" +
98134
" }\n" +
99135
"}";
100-
List<SearchHit> hits = executeSearch(Detector.DETECTORS_INDEX, request);
101-
SearchHit hit = hits.get(0);
102-
103-
String monitorId = ((List<String>) ((Map<String, Object>) hit.getSourceAsMap().get("detector")).get("monitor_id")).get(0);
104-
105-
Response deleteMonitorResponse = deleteAlertingMonitor(monitorId);
106-
assertEquals(200, deleteMonitorResponse.getStatusLine().getStatusCode());
107-
entityAsMap(deleteMonitorResponse);
136+
}
108137

109-
Response deleteResponse = makeRequest(client(), "DELETE", SecurityAnalyticsPlugin.DETECTOR_BASE_URI + "/" + detectorId1, Collections.emptyMap(), null);
138+
private void validateDetectorDeletion(final String detectorId) throws IOException {
139+
final Response deleteResponse = makeRequest(client(), "DELETE", SecurityAnalyticsPlugin.DETECTOR_BASE_URI + "/" + detectorId,
140+
Collections.emptyMap(), null);
110141
Assert.assertEquals("Delete detector failed", RestStatus.OK, restStatus(deleteResponse));
111-
hits = executeSearch(Detector.DETECTORS_INDEX, request);
142+
143+
final String request = getDetectorQuery(detectorId);
144+
final List<SearchHit> hits = executeSearch(Detector.DETECTORS_INDEX, request);
112145
Assert.assertEquals(0, hits.size());
113146
}
114147

0 commit comments

Comments
 (0)