Delete detector successfully if workflow is missing (#790)

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

@@ -204,7 +204,7 @@
 
                     @Override
                     public void onFailure(Exception e) {
-                        if (isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListener(e, detector.getId())) {
+                        if (isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(e, detector.getId())) {
                             deleteDetectorFromConfig(detector.getId(), request.getRefreshPolicy());
                         } else {
                             log.error(String.format(Locale.ROOT, "Failed to delete detector %s", detector.getId()), e);
@@ -231,15 +231,25 @@
                 log.debug(String.format("Deleting the workflow %s before deleting the detector", workflowId));
                 StepListener<DeleteWorkflowResponse> onDeleteWorkflowStep = new StepListener<>();
                 workflowService.deleteWorkflow(workflowId, onDeleteWorkflowStep);
-                onDeleteWorkflowStep.whenComplete(deleteWorkflowResponse -> {
-                    actionListener.onResponse(new AcknowledgedResponse(true));
-                }, actionListener::onFailure);
+                onDeleteWorkflowStep.whenComplete(
+                        deleteWorkflowResponse -> actionListener.onResponse(new AcknowledgedResponse(true)),
+                        deleteWorkflowResponse -> handleDeleteWorkflowFailure(detector.getId(), deleteWorkflowResponse, actionListener)
+                );
             } else {
                 // If detector doesn't have the workflows it means that older version of the plugin is used and just skip the step
                 actionListener.onResponse(new AcknowledgedResponse(true));
             }
         }
 
+        private void handleDeleteWorkflowFailure(final String detectorId, final Exception deleteWorkflowException,
+                                                 final ActionListener<AcknowledgedResponse> actionListener) {
+            if (isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(deleteWorkflowException, detectorId)) {
+                actionListener.onResponse(new AcknowledgedResponse(true));
+            } else {
+                actionListener.onFailure(deleteWorkflowException);
+            }
+        }
+
         private void deleteDetectorFromConfig(String detectorId, WriteRequest.RefreshPolicy refreshPolicy) {
             deleteDetector(detectorId, refreshPolicy,
                     new ActionListener<>() {
@@ -296,7 +306,7 @@
             }));
         }
 
-        private boolean isOnlyMonitorOrIndexMissingExceptionThrownByGroupedActionListener(
+        private boolean isOnlyWorkflowOrMonitorOrIndexMissingExceptionThrownByGroupedActionListener(
                 Exception ex,
                 String detectorId
         ) {
@@ -305,12 +315,9 @@
             int len = ex.getSuppressed().length;
             for (int i = 0; i <= len; i++) {
                 Throwable e = i == len ? ex : ex.getSuppressed()[i];
-                if (e.getMessage().matches("(.*)Monitor(.*) is not found(.*)")
-                        || e.getMessage().contains(
-                        "Configured indices are not found: [.opendistro-alerting-config]")
-                ) {
+                if (isMonitorNotFoundException(e) || isWorkflowNotFoundException(e) || isAlertingConfigIndexNotFoundException(e)) {
                     log.error(
-                            String.format(Locale.ROOT, "Monitor or jobs index already deleted." +
+                            String.format(Locale.ROOT, "Workflow, monitor, or jobs index already deleted." +
                                     " Proceeding with detector %s deletion", detectorId),
                             e);
                 } else {
@@ -321,6 +328,18 @@
         }
     }
 
+    private boolean isMonitorNotFoundException(final Throwable e) {
+        return e.getMessage().matches("(.*)Monitor(.*) is not found(.*)");
+    }
+
+    private boolean isWorkflowNotFoundException(final Throwable e) {
+        return e.getMessage().matches("(.*)Workflow(.*) not found(.*)");
+    }
+
+    private boolean isAlertingConfigIndexNotFoundException(final Throwable e) {
+        return e.getMessage().contains("Configured indices are not found: [.opendistro-alerting-config]");
+    }
+
     private void setEnabledWorkflowUsage(boolean enabledWorkflowUsage) {
         this.enabledWorkflowUsage = enabledWorkflowUsage;
     }

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

@@ -400,6 +400,14 @@ protected Response executeAlertingWorkflow(RestClient client, String workflowId,
         return makeRequest(client, "POST", String.format(Locale.getDefault(), "/_plugins/_alerting/workflows/%s/_execute", workflowId), params, null);
     }
 
+    protected Response deleteAlertingWorkflow(String workflowId) throws IOException {
+        return deleteAlertingWorkflow(client(), workflowId);
+    }
+
+    protected Response deleteAlertingWorkflow(RestClient client, String workflowId) throws IOException {
+        return makeRequest(client, "DELETE", String.format(Locale.getDefault(), "/_plugins/_alerting/workflows/%s", workflowId), new HashMap<>(), null);
+    }
+
     protected List<SearchHit> executeSearch(String index, String request) throws IOException {
         return executeSearch(index, request, true);
     }

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

@@ -23,6 +23,7 @@
 import org.opensearch.client.ResponseException;
 import org.opensearch.commons.alerting.model.IntervalSchedule;
 import org.opensearch.commons.alerting.model.Monitor.MonitorType;
+import org.opensearch.commons.alerting.model.ScheduledJob;
 import org.opensearch.core.rest.RestStatus;
 import org.opensearch.core.xcontent.MediaTypeRegistry;
 import org.opensearch.search.SearchHit;
@@ -72,10 +73,34 @@ public void testNewLogTypes() throws IOException {
     @SuppressWarnings("unchecked")
     public void testDeletingADetector_MonitorNotExists() throws IOException {
         updateClusterSetting(ENABLE_WORKFLOW_USAGE.getKey(), "false");
-        String index = createTestIndex(randomIndex(), windowsIndexMapping());
+        final String detectorId = setupDetector();
+        final Map<String, Object> detectorSourceAsMap = getDetectorSourceAsMap(detectorId);
+
+        final String monitorId = ((List<String>) detectorSourceAsMap.get("monitor_id")).get(0);
+        final Response deleteMonitorResponse = deleteAlertingMonitor(monitorId);
+        assertEquals(200, deleteMonitorResponse.getStatusLine().getStatusCode());
+        entityAsMap(deleteMonitorResponse);
+
+        validateDetectorDeletion(detectorId);
+    }
+
+    public void testDeletingADetector_WorkflowUsageEnabled_WorkflowDoesntExist() throws IOException {
+        final String detectorId = setupDetector();
+        final Map<String, Object> detectorSourceAsMap = getDetectorSourceAsMap(detectorId);
+
+        final String workflowId = ((List<String>) detectorSourceAsMap.get("workflow_ids")).get(0);
+        final Response deleteWorkflowResponse = deleteAlertingWorkflow(workflowId);
+        assertEquals(200, deleteWorkflowResponse.getStatusLine().getStatusCode());
+        entityAsMap(deleteWorkflowResponse);
+
+        validateDetectorDeletion(detectorId);
+    }
+
+    private String setupDetector() throws IOException {
+        final String index = createTestIndex(randomIndex(), windowsIndexMapping());
 
         // Execute CreateMappingsAction to add alias mapping for index
-        Request createMappingRequest = new Request("POST", SecurityAnalyticsPlugin.MAPPER_BASE_URI);
+        final Request createMappingRequest = new Request("POST", SecurityAnalyticsPlugin.MAPPER_BASE_URI);
         // both req params and req body are supported
         createMappingRequest.setJsonEntity(
                 "{ \"index_name\":\"" + index + "\"," +
@@ -84,31 +109,39 @@ public void testDeletingADetector_MonitorNotExists() throws IOException {
                         "}"
         );
 
-        Response response = client().performRequest(createMappingRequest);
+        final Response response = client().performRequest(createMappingRequest);
         assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode());
-        // Create detector #1 of type test_windows
-        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())));
-        String detectorId1 = createDetector(detector1);
+        // Create detector of type test_windows
+        final DetectorTrigger detectorTrigger = new DetectorTrigger(null, "test-trigger", "1", List.of(randomDetectorType()),
+                List.of(), List.of(), List.of(), List.of(), List.of());
+        final Detector detector = randomDetectorWithTriggers(getRandomPrePackagedRules(), List.of(detectorTrigger));
+        return createDetector(detector);
+    }
 
-        String request = "{\n" +
+    private Map<String, Object> getDetectorSourceAsMap(final String detectorId) throws IOException {
+        final String request = getDetectorQuery(detectorId);
+        final List<SearchHit> hits = executeSearch(Detector.DETECTORS_INDEX, request);
+        final SearchHit hit = hits.get(0);
+        return (Map<String, Object>) hit.getSourceAsMap().get("detector");
+    }
+
+    private String getDetectorQuery(final String detectorId) {
+        return "{\n" +
                 "   \"query\" : {\n" +
                 "     \"match\":{\n" +
-                "        \"_id\": \"" + detectorId1 + "\"\n" +
+                "        \"_id\": \"" + detectorId + "\"\n" +
                 "     }\n" +
                 "   }\n" +
                 "}";
-        List<SearchHit> hits = executeSearch(Detector.DETECTORS_INDEX, request);
-        SearchHit hit = hits.get(0);
-
-        String monitorId = ((List<String>) ((Map<String, Object>) hit.getSourceAsMap().get("detector")).get("monitor_id")).get(0);
-
-        Response deleteMonitorResponse = deleteAlertingMonitor(monitorId);
-        assertEquals(200, deleteMonitorResponse.getStatusLine().getStatusCode());
-        entityAsMap(deleteMonitorResponse);
+    }
 
-        Response deleteResponse = makeRequest(client(), "DELETE", SecurityAnalyticsPlugin.DETECTOR_BASE_URI + "/" + detectorId1, Collections.emptyMap(), null);
+    private void validateDetectorDeletion(final String detectorId) throws IOException {
+        final Response deleteResponse = makeRequest(client(), "DELETE", SecurityAnalyticsPlugin.DETECTOR_BASE_URI + "/" + detectorId,
+                Collections.emptyMap(), null);
         Assert.assertEquals("Delete detector failed", RestStatus.OK, restStatus(deleteResponse));
-        hits = executeSearch(Detector.DETECTORS_INDEX, request);
+
+        final String request = getDetectorQuery(detectorId);
+        final List<SearchHit> hits = executeSearch(Detector.DETECTORS_INDEX, request);
         Assert.assertEquals(0, hits.size());
     }