Extended detector use-cases to consider the workflow #394

stevanbz · 2023-04-10T19:22:33Z

Description

This PR contains the changes related with workflow.

Enables workflow creation - all the monitors related with the given detector will be part of the detector (220)
Enables workflow update during detector update (222)
Deletes the workflow during the detector deletion (225)
Adds additional field to a detector (226)

This PR considers if the current detectors are supporting the workflow or not

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Stevan Buzejic <[email protected]>

eirsep · 2023-04-11T20:03:53Z

src/main/java/org/opensearch/securityanalytics/model/Detector.java

@@ -51,6 +51,8 @@ public class Detector implements Writeable, ToXContentObject {
    public static final String ENABLED_TIME_FIELD = "enabled_time";
    public static final String ALERTING_MONITOR_ID = "monitor_id";

+    public static final String ALERTING_WORKFLOW_ID = "workflow_ids";


NIT: shouldnt we have single workflow id?
Any motivations for list?

Hope we are not exposing workflow id in response serialized to user

I added list because I though it's more generic solution - which will allow us (if we are going consider that scenario in one moment) to add multiple workflows without taking care about the detectors created in the period once we had only one workflow (and creating special case branches in the code with some if-else logic).
Does this makes sense? If not, I will remove it

eirsep

what happens

User creates detector in 2.6
upgrades cluster to version where workflows are introduced
updates detector rules? Will new workflow be created and the monitors be disabled? or will we not create workflows for existing detectors.
let's closely check the udpate scenario

eirsep · 2023-04-11T20:05:08Z

src/main/java/org/opensearch/securityanalytics/model/Detector.java

+                case ALERTING_WORKFLOW_ID:
+                    XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_ARRAY, xcp.currentToken(), xcp);
+                    while (xcp.nextToken() != XContentParser.Token.END_ARRAY) {
+                        String workflowId = xcp.text();


null check?

eirsep · 2023-04-11T20:11:50Z

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

-                            errorStatusSupplier.trySet(response.getStatus());
-                            return true;
+            // If detector doesn't have the workflows it means that older version of the plugin is used
+            if (detector.isWorkflowSupported()) {


debug log "deleting workflow before deleting detector

eirsep · 2023-04-11T20:12:22Z

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

+                workflowService.deleteWorkflow(detector.getWorkflowIds().get(0),
+                    new ActionListener<>() {
+                        @Override
+                        public void onResponse(DeleteWorkflowResponse deleteWorkflowResponse) {


debug log "workflow deleted. deleting monitors before detector deletion"

eirsep · 2023-04-11T20:13:13Z

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

                        }
-                        return false;
-                    }).count() > 0) {
-                        onFailures(new OpenSearchStatusException("Monitor associated with detected could not be deleted", errorStatusSupplier.get()));


why is this removed?

It is not removed - it's extracted to monitor service. Check it out here

eirsep · 2023-04-11T20:14:22Z

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

-                            log.error("Detector not being deleted because monitor [{}] could not be deleted. Status [{}]", response.getId(), response.getStatus());
-                            errorStatusSupplier.trySet(response.getStatus());
-                            return true;
+            // If detector doesn't have the workflows it means that older version of the plugin is used


we can use step listener instead of repeating logic
first step is deleting workflow. if workflow doesnt exist listener can return success response or soemthing like that.
much more cleaner and understandable that way

eirsep · 2023-04-11T20:15:21Z

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

+                if (numberOfUnprocessedResponses == 0) {
+                    workflowService.upsertWorkflow(
+                        monitorResponses.stream().map(IndexMonitorResponse::getId).collect(Collectors.toList()),
+                        null,


which field is being set as null?

updatedMonitors

eirsep · 2023-04-11T20:22:24Z

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

+
+                                        @Override
+                                        public void onFailure(Exception e) {
+                                            listener.onFailure(e);


delete monitors if we fail to create workflow?

we are deleting the monitors in workflowService.upsertWorkflow if upserts fails

Can the same monitor be referred to other workflow?

@getsaurabh02 Yes - one monitor can be part of delegate list of two different workflows

eirsep · 2023-04-11T20:54:09Z

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

+
+                                        @Override
+                                        public void onFailure(Exception e) {
+                                            listener.onFailure(e);


add error logs everywhere log.error("message", e)

eirsep · 2023-04-11T20:54:39Z

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

+
+                @Override
+                public void onFailure(Exception e) {
+                    listener.onFailure(e);


eirsep · 2023-04-11T20:56:17Z

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

@@ -1141,7 +1230,30 @@ public void onResponse(IndexResponse response) {

                @Override
                public void onFailure(Exception e) {
-                    onFailures(e);
+                    // Revert the workflow and monitors created in previous steps
+                    workflowService.deleteWorkflow(request.getDetector().getWorkflowIds().get(0),


what if workflow never gets created

how will we delete monitors then

Then it's an error in program flow - if the workflow is not created (because of the error) the detector won't be created.

eirsep · 2023-04-11T20:57:24Z

src/main/java/org/opensearch/securityanalytics/util/MonitorService.java

+            }
+            @Override
+            public void onFailure(Exception e) {
+                listener.onFailure(e);


error logs for e and e.suppressedExceptions

I thought to add something like this:
log.error("Error deleting monitors", e);
When you say e.suppressedExceptions you mean we should also do
log.error("Error deleting monitors", e.getSuppressed()); or?

eirsep · 2023-04-11T20:58:01Z

src/main/java/org/opensearch/securityanalytics/util/MonitorService.java

+import org.opensearch.commons.alerting.action.DeleteMonitorResponse;
+import org.opensearch.rest.RestStatus;
+
+public class MonitorService {


java doc to explain this is an Alerting commons interface

eirsep · 2023-04-11T20:58:09Z

src/main/java/org/opensearch/securityanalytics/util/WorkflowService.java

+import org.opensearch.rest.RestRequest.Method;
+import org.opensearch.securityanalytics.model.Detector;
+
+public class WorkflowService {


java doc to explain this is an Alerting commons interface

eirsep · 2023-04-11T20:59:13Z

src/main/resources/mappings/detectors.json

@@ -88,6 +88,9 @@
            }
          }
        },
+        "workflow_ids": {


keyword or text?

Makes sense to be keyword

stevanbz · 2023-04-11T21:13:22Z

what happens

1. User creates detector in 2.6

2. upgrades cluster to version where workflows are introduced

3. updates detector rules? Will new workflow be created and the monitors be disabled? or will we not create workflows for existing detectors.

4. let's closely check the udpate scenario

Check it out here
You can see that we are checkign if the detector is supporting the workflow. If it does, we're updating the workflow delegate list if not, we are just deleting the monitors.

Signed-off-by: Stevan Buzejic <[email protected]>

…ed loggings Signed-off-by: Stevan Buzejic <[email protected]>

eirsep · 2023-04-19T22:41:40Z

@stevanbz
Can we have an INTERNAL cluster setting security_analytics.enable_workflow_usage to turn off or turn on the feature.
If flag is enabled, we will create workflow in detector
If flag is disabled, we will create only individual enabled monitors
then create following test:

disable the above setting
create detector (which would not have workflow)
enabled the above setting
update the workflow and verify that no workflow is created for that detector

stevanbz · 2023-04-25T17:44:00Z

@stevanbz Can we have an INTERNAL cluster setting security_analytics.enable_workflow_usage to turn off or turn on the feature. If flag is enabled, we will create workflow in detector If flag is disabled, we will create only individual enabled monitors then create following test:
1. disable the above setting

2. create detector (which would not have workflow)

3. enabled the above setting

4. update the workflow and verify that no workflow is created for that detector

Yeah sure. Makes sense. Will add the flag. Just one correction re the item 4.
instead of "update the workflow and verify that no workflow is created for that detector" I guess you wanted to say "update the detector..."

… enable_workflow_usage. Added integration test that verify that the workflow is not created during update Signed-off-by: Stevan Buzejic <[email protected]>

sbcd90 · 2023-04-28T08:17:22Z

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

@@ -546,7 +707,7 @@ private IndexMonitorRequest createBucketLevelMonitorRequest(
         triggers.add(bucketLevelTrigger1);


i still do not see the bucket level triggers passed to the workflow. will it be part of a future pr?

Will address this in a quick follow up PR

sbcd90 · 2023-04-28T08:27:10Z

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

+        verifyWorkflow(detectorMap, monitorIds, 2);
+    }
+
+    public void testCreateDetector_verifyWorkflowExecutionBucketLevelDocLevelMonitors_success() throws IOException {


can this test case be create a detector with an aggregation sigma rule with a detector trigger that generates a finding but not an alert?

can we add another test case to create a detector with an aggregation sigma rule with a detector trigger that generates a finding & also an alert?

Will address this in a quick follow up PR

Enables workflow creation - all the monitors related with the given detector will be part of the detector Enables workflow update during detector update Deletes the workflow during the detector deletion Adds additional field to a detector Signed-off-by: Stevan Buzejic <[email protected]>

* Using alerting workflows in detectors (#394) Enables workflow creation - all the monitors related with the given detector will be part of the detector Enables workflow update during detector update Deletes the workflow during the detector deletion Adds additional field to a detector Signed-off-by: Stevan Buzejic <[email protected]> * disable delegate monitors as workflow is synced to detector enabled/disabled flag Signed-off-by: Surya Sashank Nistala <[email protected]> * fix test Signed-off-by: Surya Sashank Nistala <[email protected]> --------- Signed-off-by: Stevan Buzejic <[email protected]> Signed-off-by: Surya Sashank Nistala <[email protected]> Signed-off-by: Subhobrata Dey <[email protected]> Co-authored-by: Stevan Buzejic <[email protected]> Co-authored-by: Subhobrata Dey <[email protected]>

Enables workflow creation - all the monitors related with the given detector will be part of the detector Enables workflow update during detector update Deletes the workflow during the detector deletion Adds additional field to a detector Signed-off-by: Stevan Buzejic <[email protected]>

…oject#394 (opensearch-project#396) Signed-off-by: Jovan Cvetkovic <[email protected]>

Extended detector use-cases to consider the workflow

13d83a2

Signed-off-by: Stevan Buzejic <[email protected]>

eirsep reviewed Apr 11, 2023

View reviewed changes

eirsep requested changes Apr 11, 2023

View reviewed changes

eirsep reviewed Apr 11, 2023

View reviewed changes

stevanbz added 2 commits April 11, 2023 23:24

Added integTest for workflow execution verification

8d2b5c9

Signed-off-by: Stevan Buzejic <[email protected]>

Updated workflow delete. Changed the mapping for the workflowIds. Add…

1aee052

…ed loggings Signed-off-by: Stevan Buzejic <[email protected]>

eirsep marked this pull request as ready for review April 18, 2023 17:50

eirsep requested a review from a team April 18, 2023 17:50

Updated workflow indexing and deletion to consider the flag setting -…

8ad56b2

… enable_workflow_usage. Added integration test that verify that the workflow is not created during update Signed-off-by: Stevan Buzejic <[email protected]>

sbcd90 requested changes Apr 28, 2023

View reviewed changes

sbcd90 approved these changes Apr 28, 2023

View reviewed changes

eirsep self-requested a review April 28, 2023 18:09

eirsep approved these changes Apr 28, 2023

View reviewed changes

eirsep merged commit 4f40fc7 into opensearch-project:feature/composite-monitors Apr 28, 2023

riysaxen-amzn pushed a commit to riysaxen-amzn/security-analytics that referenced this pull request Feb 20, 2024

[FEATURE] Add "Create detector" item to the breadcrumbs opensearch-pr…

a137a80

…oject#394 (opensearch-project#396) Signed-off-by: Jovan Cvetkovic <[email protected]>

engechas mentioned this pull request Apr 10, 2024

Add field to store composite monitor id in detector document #226

Closed

		@@ -546,7 +707,7 @@ private IndexMonitorRequest createBucketLevelMonitorRequest(
		triggers.add(bucketLevelTrigger1);

Extended detector use-cases to consider the workflow #394

Extended detector use-cases to consider the workflow #394

Conversation

stevanbz commented Apr 10, 2023

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevanbz Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

eirsep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevanbz commented Apr 11, 2023

eirsep commented Apr 19, 2023

stevanbz commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevanbz Apr 12, 2023 •

edited

Loading