Added transport layer classes for getting and deleting the workflow #835

stevanbz · 2023-03-23T18:21:36Z

Issue #, if available:
#834
Description of changes:
Added classes for getting and deleting the workflows.

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.

eirsep · 2023-03-23T18:44:13Z

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

+
+            val searchRequest = SearchRequest()
+                .indices(ScheduledJob.SCHEDULED_JOBS_INDEX)
+                .source(SearchSourceBuilder().query(queryBuilder).fetchSource(true))


do we need fetch source as true?

eirsep · 2023-03-23T18:49:49Z

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

+                .indices(ScheduledJob.SCHEDULED_JOBS_INDEX)
+                .source(SearchSourceBuilder().query(queryBuilder).fetchSource(true))
+
+            val searchResponse: SearchResponse = client.suspendUntil { search(searchRequest, it) }


plz add a try catch for search response failure. Log error that validation failed and re-throw the error as required

eirsep · 2023-03-23T18:55:23Z

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

+            if (searchResponse.hits.totalHits?.value == 0L) {
+                return true
+            }
+            return false


add validation failure reason as info or error log

how are we returning this infromation to user in delete failed response?

it can't tailgate on 403 security error

You can see here - if the canDelete is false, user is getting the message that the monitor can't be deleted. The question is should we be more descriptive or it's enough to say that user is not allowed to do deleting.
Now I just saw Ashish comment - where he said that we should tell to end user that the monitor can't be deleted because it is a part of the workflow

eirsep

added minor comments

eirsep · 2023-03-23T18:55:53Z

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

+            if (searchResponse.hits.totalHits?.value == 0L) {
+                return true
+            }
+            return false


how are we returning this infromation to user in delete failed response?

eirsep · 2023-03-23T18:56:05Z

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

+            if (searchResponse.hits.totalHits?.value == 0L) {
+                return true
+            }
+            return false


it can't tailgate on 403 security error

eirsep · 2023-03-24T08:29:18Z

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

+            return client.suspendUntil { delete(deleteRequest, it) }
+        }
+
+        private suspend fun deleteMetadata(workflow: Workflow) {


plz add debug logs like "deleting metadata",

Ok this part is copy pasted - even we don't have workflow metada. will comment out the call and add TODO - since we are going to introduce the metadata with workflow execution. Tnx!

eirsep · 2023-03-24T08:29:51Z

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

+
+        private suspend fun deleteMetadata(workflow: Workflow) {
+            val deleteRequest = DeleteRequest(ScheduledJob.SCHEDULED_JOBS_INDEX, "${workflow.id}-metadata")
+            val deleteResponse: DeleteResponse = client.suspendUntil { delete(deleteRequest, it) }


add try catch for failure in deletion and log error and re-throw wrapped exception

eirsep · 2023-03-24T08:30:11Z

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

+                getRequest,
+                object : ActionListener<GetResponse> {
+                    override fun onResponse(response: GetResponse) {
+                        if (!response.isExists) {


eirsep · 2023-03-24T08:31:27Z

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

+                    }
+
+                    override fun onFailure(t: Exception) {
+                        if (t is IndexNotFoundException) {


lezzago · 2023-03-24T14:34:57Z

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

-                val canDelete = user == null ||
-                    !doFilterForUser(user) ||
-                    checkUserPermissionsWithResource(user, monitor.user, actionListener, "monitor", monitorId)
+                val canDelete = monitorIsNotInWorkflows(monitor.id) && (


If the monitor cannot be deleted if it is part of a workflow, we need to mention it is part of a workflow and that is why it cannot be deleted.

lezzago · 2023-03-24T14:37:56Z

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

+                .indices(ScheduledJob.SCHEDULED_JOBS_INDEX)
+                .source(SearchSourceBuilder().query(queryBuilder).fetchSource(true))
+
+            val searchResponse: SearchResponse = client.suspendUntil { search(searchRequest, it) }


We need to stash the context since the user would not have permissions to search the alerting config index directly.
reference: https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetMonitorAction.kt#L66-L72

got it good point!

lezzago · 2023-03-24T14:43:07Z

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

+         */
+        private suspend fun getDeletableDelegates(workflowIdToBeDeleted: String, monitorIds: List<String>): List<String> {
+            // Retrieve monitors belonging to another workflows
+            val queryBuilder = QueryBuilders.boolQuery().mustNot(QueryBuilders.termQuery("_id", workflowIdToBeDeleted)).filter(


We need to check if the user has permissions to these monitor with the backend roles too as the monitors could get modified later and the user might not have the backend roles to access those monitors.

Aaahh got it! Good point

sbcd90 · 2023-03-28T07:59:00Z

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

+            // Check  if user can access the monitors (since the monitors could get modified later and the user might not have the backend roles to access the monitors)
+            if (user != null && filterByEnabled) {
+                addFilter(user, searchRequest.source(), "monitor.user.backend_roles.keyword")
+            }


can we add a SecureWorkflowMonitorIT test case to test this part of code (both with & without) backend roles.

sbcd90 · 2023-03-28T08:03:24Z

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

+
+        val user = readUserFromThreadContext(client)
+        val deleteRequest = DeleteRequest(ScheduledJob.SCHEDULED_JOBS_INDEX, transformedRequest.workflowId)
+            .setRefreshPolicy(RefreshPolicy.IMMEDIATE)


can we receiverefresh policy from transport request like this https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteMonitorAction.kt#L85?

sbcd90 · 2023-03-28T08:05:15Z

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

+                transformedRequest.deleteDelegateMonitors,
+                user,
+                transformedRequest.workflowId
+            ).resolveUserAndStart()


do we need make resolveUserAndStart a suspend function? is there any advantage? i see TransportIndexMonitorAction not doing it. https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt#L247

deleteMonitors and deleteWorkflow are suspend functions - both are called by resolveUserAndStart

sbcd90 · 2023-03-28T08:10:57Z

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

+                                        actionListener,
+                                        "workflow",
+                                        getWorkflowRequest.workflowId
+                                    )


Lets add a SecureWorkflowMonitorIT test case for TransportGetWorkflowAction also.

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

lezzago · 2023-04-03T19:15:49Z

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

@@ -126,10 +127,11 @@ class TransportDeleteWorkflowAction @Inject constructor(

                if (canDelete) {
                    val deleteResponse = deleteWorkflow(workflow)


Should we attempt to delete monitors first and then delete the workflow? And only delete everything if the user has permission to delete everything?

lezzago · 2023-04-03T19:18:24Z

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

+                .indices(ScheduledJob.SCHEDULED_JOBS_INDEX)
+                .source(SearchSourceBuilder().query(queryBuilder))
+
+            // Check  if user can access the monitors (since the monitors could get modified later and the user might not have the backend roles to access the monitors)


Nitpick: remove extra space ( ) between Check and if

Good catch!

lezzago · 2023-04-03T19:19:59Z

alerting/src/test/resources/bwc/alerting/1.1.0.0/opensearch-alerting-1.1.0.0.zip

remove this

lezzago · 2023-04-03T19:20:05Z

ci - alerting/Dockerfile

remove this

lezzago · 2023-04-03T19:20:13Z

ci - alerting/docker_build.sh

remove this

…specified when deleting the workflow Signed-off-by: Stevan Buzejic <[email protected]>

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

lezzago · 2023-04-03T23:24:08Z

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

+                        ) {
+                            actionListener.onFailure(
+                                AlertingException(
+                                    "Not allowed to delete ${delegateMonitorIds.joinToString()} monitors",


Lets remove the monitors from this list that can be deleted from monitorIdsToBeDeleted

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

…835) * Added transport layer classes for getting and deleting the workflow Signed-off-by: Stevan Buzejic <[email protected]>

stevanbz requested a review from a team March 23, 2023 18:21

eirsep reviewed Mar 23, 2023

View reviewed changes

eirsep requested changes Mar 24, 2023

View reviewed changes

lezzago requested changes Mar 24, 2023

View reviewed changes

sbcd90 requested changes Mar 28, 2023

View reviewed changes

stevanbz added 3 commits March 31, 2023 21:53

Added transport layer classes for getting and deleting the workflow

21669c3

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

Added stashing the context when fetching the monitor workflows

4566de0

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

Dependency fix

0ab964a

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

stevanbz force-pushed the feature/composite-monitors-get-delete-workflow branch from e571313 to 0ab964a Compare March 31, 2023 20:08

eirsep approved these changes Apr 3, 2023

View reviewed changes

lezzago requested changes Apr 3, 2023

View reviewed changes

eirsep added the composite-monitors label Apr 3, 2023

stevanbz added 3 commits April 3, 2023 23:31

Refactor code to check if all monitors can be deleted if the flag is …

691824c

…specified when deleting the workflow Signed-off-by: Stevan Buzejic <[email protected]>

Removed files

ca264ee

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

Removed using global coroutine scope

e70fccd

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

lezzago requested changes Apr 3, 2023

View reviewed changes

removed monitors that can be deleted from the list of all delegate mo…

a98a8c9

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

lezzago approved these changes Apr 4, 2023

View reviewed changes

lezzago merged commit 9f4a61f into opensearch-project:feature/composite-monitors Apr 4, 2023

eirsep pushed a commit that referenced this pull request May 24, 2023

Added transport layer classes for getting and deleting the workflow (#…

8b6dae6

…835) * Added transport layer classes for getting and deleting the workflow Signed-off-by: Stevan Buzejic <[email protected]>

This was referenced May 24, 2023

Adds transport layer actions for CRUD workflows #932

Closed

Adds transport layer actions for CRUD workflows #934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added transport layer classes for getting and deleting the workflow #835

Added transport layer classes for getting and deleting the workflow #835

stevanbz commented Mar 23, 2023 •

edited by eirsep

Loading

eirsep Mar 23, 2023

eirsep Mar 23, 2023

eirsep Mar 23, 2023

eirsep Mar 23, 2023

eirsep Mar 23, 2023

stevanbz Mar 24, 2023

eirsep left a comment

eirsep Mar 23, 2023

eirsep Mar 23, 2023

eirsep Mar 24, 2023

stevanbz Mar 24, 2023

eirsep Mar 24, 2023

eirsep Mar 24, 2023

eirsep Mar 24, 2023

lezzago Mar 24, 2023

lezzago Mar 24, 2023

stevanbz Mar 24, 2023

lezzago Mar 24, 2023

stevanbz Mar 24, 2023

sbcd90 Mar 28, 2023

sbcd90 Mar 28, 2023

sbcd90 Mar 28, 2023

stevanbz Mar 31, 2023

sbcd90 Mar 28, 2023

lezzago Apr 3, 2023

lezzago Apr 3, 2023

stevanbz Apr 3, 2023

lezzago Apr 3, 2023

lezzago Apr 3, 2023

lezzago Apr 3, 2023

lezzago Apr 3, 2023

		@@ -126,10 +127,11 @@ class TransportDeleteWorkflowAction @Inject constructor(

		if (canDelete) {
		val deleteResponse = deleteWorkflow(workflow)

Added transport layer classes for getting and deleting the workflow #835

Added transport layer classes for getting and deleting the workflow #835

Conversation

stevanbz commented Mar 23, 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

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

edited by eirsep

Loading