Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@

package org.opensearch.alerting.transport

import kotlinx.coroutines.CoroutineName
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.launch
import org.apache.logging.log4j.LogManager
import org.apache.lucene.search.join.ScoreMode
Expand Down Expand Up @@ -55,6 +54,7 @@ import org.opensearch.search.builder.SearchSourceBuilder
import org.opensearch.tasks.Task
import org.opensearch.transport.TransportService

private val scope: CoroutineScope = CoroutineScope(Dispatchers.IO)
/**
* Transport class that deletes the workflow.
* If the deleteDelegateMonitor flag is set to true, deletes the workflow delegates that are not part of another workflow
Expand Down Expand Up @@ -91,7 +91,7 @@ class TransportDeleteWorkflowAction @Inject constructor(
return
}

GlobalScope.launch(Dispatchers.IO + CoroutineName("DeleteWorkflowAction")) {
scope.launch {
DeleteWorkflowHandler(
client,
actionListener,
Expand Down Expand Up @@ -126,18 +126,31 @@ class TransportDeleteWorkflowAction @Inject constructor(
)

if (canDelete) {
val deleteResponse = deleteWorkflow(workflow)
// TODO - uncomment once the metadata are introduced
// deleteMetadata(workflow)
val delegateMonitorIds = (workflow.inputs[0] as CompositeInput).getMonitorIds()

// User can only delete the delegate monitors only in the case if all monitors can be deleted
// Partial monitor deletion is not available
if (deleteDelegateMonitors == true) {
val delegateMonitorIds = (workflow.inputs[0] as CompositeInput).getMonitorIds()
val monitorIdsToBeDeleted = getDeletableDelegates(workflowId, delegateMonitorIds, user)

// Delete the monitor ids
if (monitorIdsToBeDeleted.isNotEmpty()) {
deleteMonitors(monitorIdsToBeDeleted, RefreshPolicy.IMMEDIATE)
if (
delegateMonitorIds.size != monitorIdsToBeDeleted.size || delegateMonitorIds.toSet() != monitorIdsToBeDeleted.toSet()
) {
actionListener.onFailure(
AlertingException(
"Not allowed to delete ${delegateMonitorIds.joinToString()} monitors",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RestStatus.FORBIDDEN,
IllegalStateException()
)
)
return
}
}

val deleteResponse = deleteWorkflow(workflow)
if (deleteDelegateMonitors == true) {
deleteMonitors(delegateMonitorIds, RefreshPolicy.IMMEDIATE)
}
actionListener.onResponse(DeleteWorkflowResponse(deleteResponse.id, deleteResponse.version))
} else {
actionListener.onFailure(
Expand Down Expand Up @@ -198,7 +211,7 @@ class TransportDeleteWorkflowAction @Inject constructor(
.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)
// 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")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -559,15 +559,13 @@ class WorkflowMonitorIT : WorkflowSingleNodeTestCase() {
assertNotNull(getWorkflowResponse2)
assertEquals(workflowId2, getWorkflowResponse2.id)

deleteWorkflow(workflowId, true)
// Verify that the workflow is deleted
try {
getWorkflowById(workflowId)
deleteWorkflow(workflowId, true)
} catch (e: Exception) {
e.message?.let {
assertTrue(
"Exception not returning GetWorkflow Action error ",
it.contains("Workflow not found.")
it.contains("[Not allowed to delete ${monitorResponse.id} monitors")
)
}
}
Expand Down
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this

Binary file not shown.
15 changes: 0 additions & 15 deletions ci - alerting/Dockerfile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this

This file was deleted.

20 changes: 0 additions & 20 deletions ci - alerting/docker_build.sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this

This file was deleted.