Skip to content

Commit 0ad91cc

Browse files
engechasengechas
authored
Delete detector successfully if workflow is missing (opensearch-project#790)
* Delete detector successfully if workflow is missing Signed-off-by: Chase Engelbrecht <engechas@amazon.com> * Refactor to use existing NotFound exception checker Signed-off-by: Chase Engelbrecht <engechas@amazon.com> --------- Signed-off-by: Chase Engelbrecht <engechas@amazon.com>
1 parent c373343 commit 0ad91cc

File tree

3 files changed

+89
-28
lines changed

3 files changed

+89
-28
lines changed

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

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import org.apache.logging.log4j.LogManager;
88
import org.apache.logging.log4j.Logger;
9+
import org.opensearch.OpenSearchException;
910
import org.opensearch.OpenSearchStatusException;
1011
import org.opensearch.action.ActionRunnable;
1112
import org.opensearch.action.StepListener;
@@ -204,7 +205,7 @@ public void onResponse(Collection<DeleteMonitorResponse> responses) {
204205

205206
@Override
206207
public void onFailure(Exception e) {
207-
if (isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListener(e, detector.getId())) {
208+
if (isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(e, detector.getId())) {
208209
deleteDetectorFromConfig(detector.getId(), request.getRefreshPolicy());
209210
} else {
210211
log.error(String.format(Locale.ROOT, "Failed to delete detector %s", detector.getId()), e);
@@ -231,15 +232,25 @@ private void deleteWorkflow(Detector detector, ActionListener<AcknowledgedRespon
231232
log.debug(String.format("Deleting the workflow %s before deleting the detector", workflowId));
232233
StepListener<DeleteWorkflowResponse> onDeleteWorkflowStep = new StepListener<>();
233234
workflowService.deleteWorkflow(workflowId, onDeleteWorkflowStep);
234-
onDeleteWorkflowStep.whenComplete(deleteWorkflowResponse -> {
235-
actionListener.onResponse(new AcknowledgedResponse(true));
236-
}, actionListener::onFailure);
235+
onDeleteWorkflowStep.whenComplete(
236+
deleteWorkflowResponse -> actionListener.onResponse(new AcknowledgedResponse(true)),
237+
deleteWorkflowResponse -> handleDeleteWorkflowFailure(detector.getId(), deleteWorkflowResponse, actionListener)
238+
);
237239
} else {
238240
// If detector doesn't have the workflows it means that older version of the plugin is used and just skip the step
239241
actionListener.onResponse(new AcknowledgedResponse(true));
240242
}
241243
}
242244

245+
private void handleDeleteWorkflowFailure(final String detectorId, final Exception deleteWorkflowException,
246+
final ActionListener<AcknowledgedResponse> actionListener) {
247+
if (isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(deleteWorkflowException, detectorId)) {
248+
actionListener.onResponse(new AcknowledgedResponse(true));
249+
} else {
250+
actionListener.onFailure(deleteWorkflowException);
251+
}
252+
}
253+
243254
private void deleteDetectorFromConfig(String detectorId, WriteRequest.RefreshPolicy refreshPolicy) {
244255
deleteDetector(detectorId, refreshPolicy,
245256
new ActionListener<>() {
@@ -296,7 +307,7 @@ private void finishHim(String detectorId, Exception t) {
296307
}));
297308
}
298309

299-
private boolean isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListener(
310+
private boolean isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(
300311
Exception ex,
301312
String detectorId
302313
) {
@@ -305,12 +316,9 @@ private boolean isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListene
305316
int len = ex.getSuppressed().length;
306317
for (int i = 0; i <= len; i++) {
307318
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-
) {
319+
if (isMonitorNotFoundException(e) || isWorkflowNotFoundException(e) || isAlertingConfigIndexNotFoundException(e)) {
312320
log.error(
313-
String.format(Locale.ROOT, "Monitor or jobs index already deleted." +
321+
String.format(Locale.ROOT, "Workflow, monitor, or jobs index already deleted." +
314322
" Proceeding with detector %s deletion", detectorId),
315323
e);
316324
} else {
@@ -321,6 +329,18 @@ private boolean isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListene
321329
}
322330
}
323331

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

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

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

406+
protected Response deleteAlertingWorkflow(String workflowId) throws IOException {
407+
return deleteAlertingWorkflow(client(), workflowId);
408+
}
409+
410+
protected Response deleteAlertingWorkflow(RestClient client, String workflowId) throws IOException {
411+
return makeRequest(client, "DELETE", String.format(Locale.getDefault(), "/_plugins/_alerting/workflows/%s", workflowId), new HashMap<>(), null);
412+
}
413+
406414
protected List<SearchHit> executeSearch(String index, String request) throws IOException {
407415
return executeSearch(index, request, true);
408416
}

‎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)