Added layer for creating and updating the workflow #831

stevanbz · 2023-03-14T17:38:46Z

Issue #, if available:
#834

Description of changes:
Added transport layer for creating/updating of the workflows and appropriate integration tests

CheckList:
[ ] 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]>

lezzago · 2023-03-16T18:05:57Z

core/src/main/resources/mappings/scheduled-jobs.json

@@ -299,6 +299,312 @@
        }
      }
    },
+    "workflow": {


The schema version at the top needs to be incremented.

lezzago · 2023-03-16T18:14:30Z

core/src/main/resources/mappings/scheduled-jobs.json

+            }
+          }
+        },
+        "triggers": {


This is not in the workflow object. ref: https://github.com/opensearch-project/common-utils/pull/380/files#diff-bba54255f406156efae1c6e081dd845fa7cd3444b381314a2fb58064cdb0e76d

This also seems like we are storing all the monitor information here and if we create monitors based on the workflows, this doesnt make sense to duplicate the data.

Yes I agree. It should be removed.

lezzago · 2023-03-16T18:15:16Z

core/build.gradle

@@ -15,7 +15,7 @@ dependencies {
    implementation "com.cronutils:cron-utils:9.1.6"
    api "org.opensearch.client:opensearch-rest-client:${opensearch_version}"
    implementation 'com.google.googlejavaformat:google-java-format:1.10.0'
-    api "org.opensearch:common-utils:${common_utils_version}"
+    api files("/home/stevan/git/opensearch/repo/common-utils/build/libs/common-utils-2.7.0.0-SNAPSHOT.jar")


Please remove this from the PR. You can keep this locally for testing.

A-ha ok cool - will do. Added in order to be "visible" that we need to merge the common-util first. Tnx

lezzago · 2023-03-16T19:08:22Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+        }
+    }
+
+    suspend fun validateRequest(request: IndexWorkflowRequest) {


Should there be another check to make sure there is only 1 input since that is what is being supported here?

Makes sense!

lezzago · 2023-03-16T19:23:19Z

Can you share the integ test runs in here to add confidence on things working?

stevanbz · 2023-03-16T20:10:15Z

Can you share the integ test runs in here to add confidence on things working?

I think I added - it's just reduced view because of it's size. Here is the integ-test file

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

eirsep · 2023-03-20T04:46:18Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+
+        val user = readUserFromThreadContext(client)
+
+        if (!validateUserBackendRoles(user, actionListener)) {


can we also validate if the user has EXECUTE permission on the delegate monitors?

eirsep · 2023-03-20T05:34:21Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+                }
+
+                if (user == null) {
+                    // Security is disabled, add empty user to Monitor. user is null for older versions.


plz write a workflow relevant comment instead

eirsep · 2023-03-20T05:35:08Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+            transformedRequest.rbacRoles != null
+        ) {
+            if (transformedRequest.rbacRoles?.stream()?.anyMatch { !user.backendRoles.contains(it) } == true) {
+                log.debug(


this is an error log

not debug

eirsep · 2023-03-20T05:37:04Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+                                // Retry mapping of monitor
+                                onCreateMappingsResponse(true)
+                            }
+                        } else {


Add error log:
log.error("Failed to create workflow", e)

plz add error log with such messages at every failure/exception

eirsep

plz add error logs at every exception with descriptive message

eirsep · 2023-03-20T05:37:21Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+                                // Retry mapping of monitor
+                                onCreateMappingsResponse(true)
+                            }
+                        } else {


plz add error log with such messages at every failure/exception

eirsep · 2023-03-20T05:39:03Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+                prepareWorkflowIndexing()
+                IndexUtils.scheduledJobIndexUpdated()
+            } else {
+                log.info("Create $SCHEDULED_JOBS_INDEX mappings call not acknowledged.")


this is an error log

eirsep · 2023-03-20T15:41:31Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+                            onUpdateMappingsResponse(response)
+                        }
+
+                        override fun onFailure(t: Exception) {


Add error log

eirsep · 2023-03-20T15:41:56Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+                        indexResponse.primaryTerm, request.workflow
+                    )
+                )
+            } catch (t: Exception) {


add error log

stevanbz · 2023-03-20T18:48:14Z

Can you share the integ test runs in here to add confidence on things working?

Sorry didn't understood you correctly :) will attach the test results

stevanbz · 2023-03-20T18:50:33Z

TEST-org.opensearch.alerting.WorkflowMonitorIT.txt

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

eirsep · 2023-03-20T19:53:59Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+                actionListener.onResponse(
+                    IndexWorkflowResponse(
+                        indexResponse.id, indexResponse.version, indexResponse.seqNo,
+                        indexResponse.primaryTerm, request.workflow


can we set workflow id in workflow object

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

eirsep · 2023-03-21T21:37:59Z

#834

lezzago · 2023-03-22T18:10:27Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+    private suspend fun getDelegateMonitors(
+        monitorIds: MutableList<String>
+    ): List<Monitor> {


We dont check if the user has access to these monitors. We need to also check for the backend roles. Lastly, we need to ensure the users have access to the indices in the monitors for when it tries to execute those monitors.

lezzago · 2023-03-22T18:13:38Z

core/src/main/resources/mappings/scheduled-jobs.json

+                        "monitor_id": {
+                          "type": "keyword"
+                        },
+                        "chained_findings": {


this should be chained_monitor_findings, right?

Ah... Good catch! Hm... But wrong param naming went on common-util feature branch -> I renamed the property but forgot to rename the property used in a builder:
https://github.com/opensearch-project/common-utils/blob/feature/composite-monitors/src/main/kotlin/org/opensearch/commons/alerting/model/Delegate.kt#L77

So we have inconsistency now in property naming and mapping. What do you suggest? Should I do a change on common-util first and then change the mappings here or? Tnx

Yea lets get both updated. Since these are going into the feature branch, we can do both at the same time.

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

lezzago · 2023-03-23T15:52:32Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexWorkflowAction.kt

+                    request.workflow = request.workflow
+                        .copy(user = User(user.name, currentWorkflow.user!!.backendRoles, user.roles, user.customAttNames))
+                }
+                log.debug("Update monitor backend roles to: ${request.workflow.user?.backendRoles}")


nitpick: change this to Update workflow instead of Update monitor

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

eirsep · 2023-03-23T17:15:19Z

core/src/main/resources/mappings/scheduled-jobs.json

@@ -418,7 +418,7 @@
                        "monitor_id": {
                          "type": "keyword"
                        },
-                        "chained_findings": {
+                        "chained_monitor_findings": {


do we need to fix this in common utils for the model also ?

I see the PR https://github.com/opensearch-project/common-utils/pull/390/files
thank you

…t#831) * Renamed chainedFindings to chainedMonitorFindings * Removed unecessary mappings from workflow definition * Improved logging when saving the workflows * Added a workflow id in response * Added role check and index access once the workflow is being created * Updated mappings for the workflow --------- Signed-off-by: Stevan Buzejic <[email protected]>

* Added layer for creating and updating the workflow (#831) * Renamed chainedFindings to chainedMonitorFindings * Removed unecessary mappings from workflow definition * Improved logging when saving the workflows * Added a workflow id in response * Added role check and index access once the workflow is being created * Updated mappings for the workflow --------- Signed-off-by: Stevan Buzejic <[email protected]> * Fixed xContent dependencies due to OSCore changes (#839) Signed-off-by: Angie Zhang <[email protected]> * Dependency fix (#846) Signed-off-by: Stevan Buzejic <[email protected]> * Refactored workflowIndexing validation - removed coroutine and contex… (#857) * Refactored workflowIndexing validation - removed coroutine and context client context lost Signed-off-by: Stevan Buzejic <[email protected]> * refactored getting the workflows Signed-off-by: Stevan Buzejic <[email protected]> * Changed the logic according to secure test findings Signed-off-by: Stevan Buzejic <[email protected]> * [Backport 2.x] Notification security fix (#861) (#863) * Notification security fix (#852) * added injecting whole user object in threadContext before calling notification APIs so that backend roles are available to notification plugin * compile fix * refactored user_info injection to use InjectSecurity * ktlint fix --------- (cherry picked from commit e0b7a5a) * remove unneeded import --------- Signed-off-by: Ashish Agrawal <[email protected]> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: Petar Dzepina <[email protected]> Co-authored-by: Ashish Agrawal <[email protected]> * Stashed user together with it's roles Signed-off-by: Stevan Buzejic <[email protected]> --------- Signed-off-by: Stevan Buzejic <[email protected]> Signed-off-by: Ashish Agrawal <[email protected]> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: Petar Dzepina <[email protected]> Co-authored-by: Ashish Agrawal <[email protected]> * Added workflow execution logic (#850) * Added workflow execution logic Signed-off-by: Stevan Buzejic <[email protected]> * Adjusted code according to comments Signed-off-by: Stevan Buzejic <[email protected]> * Updated version of the findings json Signed-off-by: Stevan Buzejic <[email protected]> * Updating the workflow metadata in the case of updating flag set to false while the metadata alerady exist Signed-off-by: Stevan Buzejic <[email protected]> * Added logging for workflow metadata update Signed-off-by: Stevan Buzejic <[email protected]> * Added Rest Execute Workflow action Signed-off-by: Stevan Buzejic <[email protected]> * Extended workflow context with workflowMetadataId. Adjusted the doc level monitor findings Signed-off-by: Stevan Buzejic <[email protected]> * Updated conditions for unstashing the context when indexing and deleting the workflow Signed-off-by: Stevan Buzejic <[email protected]> --------- Signed-off-by: Stevan Buzejic <[email protected]> * Added fix when executing the workflow and when chained findings index… (#890) Signed-off-by: Stevan Buzejic <[email protected]> * Fixed deleting monitor workflow metadata (#882) * Fixed deleting monitor metadata and workflow metadata. Signed-off-by: Stevan Buzejic <[email protected]> Signed-off-by: Surya Sashank Nistala <[email protected]> * fix monitor metadata error from conflict resolution Signed-off-by: Surya Sashank Nistala <[email protected]> * remove unused import Signed-off-by: Surya Sashank Nistala <[email protected]> * remove rest execute workflow action Signed-off-by: Surya Sashank Nistala <[email protected]> * increment schema version for findings mapping json Signed-off-by: Surya Sashank Nistala <[email protected]> --------- Signed-off-by: Stevan Buzejic <[email protected]> Signed-off-by: Angie Zhang <[email protected]> Signed-off-by: Ashish Agrawal <[email protected]> Signed-off-by: Surya Sashank Nistala <[email protected]> Co-authored-by: Stevan Buzejic <[email protected]> Co-authored-by: Angie Zhang <[email protected]> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: Petar Dzepina <[email protected]> Co-authored-by: Ashish Agrawal <[email protected]>

Added layer for creating and updating the workflow

7d24882

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

stevanbz requested a review from a team March 14, 2023 17:38

eirsep added the composite-monitors label Mar 14, 2023

eirsep requested a review from lezzago March 14, 2023 17:40

Renamed chainedFindings to chainedMonitorFindings

e6c63ff

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

lezzago requested changes Mar 16, 2023

View reviewed changes

Removed unecessary mappings from workflow definition

2704c99

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

eirsep reviewed Mar 20, 2023

View reviewed changes

eirsep requested changes Mar 20, 2023

View reviewed changes

Improved logging when saving the workflows

c8e7a18

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

eirsep requested changes Mar 20, 2023

View reviewed changes

Added a workflow id in response

8054138

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

opensearch-project deleted a comment from stevanbz Mar 21, 2023

eirsep approved these changes Mar 21, 2023

View reviewed changes

lezzago requested changes Mar 22, 2023

View reviewed changes

Added role check and index access once the workflow is being created

18c1f5e

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

lezzago requested changes Mar 23, 2023

View reviewed changes

Updated mappings for the workflow

2c6a56e

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

eirsep reviewed Mar 23, 2023

View reviewed changes

eirsep approved these changes Mar 23, 2023

View reviewed changes

lezzago approved these changes Mar 23, 2023

View reviewed changes

eirsep merged commit 8b95195 into opensearch-project:feature/composite-monitors Mar 23, 2023

eirsep mentioned this pull request May 24, 2023

Adds transport layer actions for CRUD workflows #932

Closed

eirsep mentioned this pull request May 24, 2023

Adds transport layer actions for CRUD workflows #934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added layer for creating and updating the workflow #831

Added layer for creating and updating the workflow #831

stevanbz commented Mar 14, 2023 •

edited by eirsep

Loading

lezzago Mar 16, 2023

lezzago Mar 16, 2023

stevanbz Mar 16, 2023

lezzago Mar 16, 2023

stevanbz Mar 16, 2023

lezzago Mar 16, 2023

stevanbz Mar 16, 2023

lezzago commented Mar 16, 2023

stevanbz commented Mar 16, 2023 •

edited

Loading

eirsep Mar 20, 2023

eirsep Mar 20, 2023

eirsep Mar 20, 2023

eirsep Mar 20, 2023

eirsep Mar 20, 2023

eirsep left a comment

eirsep Mar 20, 2023

eirsep Mar 20, 2023

eirsep Mar 20, 2023

eirsep Mar 20, 2023

stevanbz commented Mar 20, 2023

stevanbz commented Mar 20, 2023

eirsep Mar 20, 2023

eirsep commented Mar 21, 2023

lezzago Mar 22, 2023

lezzago Mar 22, 2023

stevanbz Mar 22, 2023 •

edited

Loading

lezzago Mar 23, 2023

lezzago Mar 23, 2023

eirsep Mar 23, 2023

eirsep Mar 23, 2023


		val user = readUserFromThreadContext(client)

		if (!validateUserBackendRoles(user, actionListener)) {

Added layer for creating and updating the workflow #831

Added layer for creating and updating the workflow #831

Conversation

stevanbz commented Mar 14, 2023 • edited by eirsep Loading

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

lezzago commented Mar 16, 2023

stevanbz commented Mar 16, 2023 • edited Loading

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

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

stevanbz commented Mar 20, 2023

stevanbz commented Mar 20, 2023

Choose a reason for hiding this comment

eirsep commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevanbz Mar 22, 2023 • edited Loading

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 Mar 14, 2023 •

edited by eirsep

Loading

stevanbz commented Mar 16, 2023 •

edited

Loading

stevanbz Mar 22, 2023 •

edited

Loading