Change AlertError message and remove deny-list destinations check dur…

…ing monitor creation (#270) * Change AlertError message and remove deny-list destinations during monitor creation * Removed experimental test code * Comment dead code * Fix async flow for monitor create
opendistro-for-elasticsearch · Oct 14, 2020 · 1d0e8ff · 1d0e8ff
1 parent 3629bfd
commit 1d0e8ff
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 49 deletions.
diff --git a/.../src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/MonitorRunResult.kt b/.../src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/MonitorRunResult.kt
@@ -61,11 +61,11 @@ data class MonitorRunResult(
     /** Returns error information to store in the Alert. Currently it's just the stack trace but it can be more */
     fun alertError(): AlertError? {
         if (error != null) {
-            return AlertError(Instant.now(), "Error running monitor:\n${error.userErrorMessage()}")
+            return AlertError(Instant.now(), "Failed running monitor:\n${error.userErrorMessage()}")
         }
 
         if (inputResults.error != null) {
-            return AlertError(Instant.now(), "Error fetching inputs:\n${inputResults.error.userErrorMessage()}")
+            return AlertError(Instant.now(), "Failed fetching inputs:\n${inputResults.error.userErrorMessage()}")
         }
         return null
     }
@@ -164,11 +164,11 @@ data class TriggerRunResult(
     /** Returns error information to store in the Alert. Currently it's just the stack trace but it can be more */
     fun alertError(): AlertError? {
         if (error != null) {
-            return AlertError(Instant.now(), "Error evaluating trigger:\n${error.userErrorMessage()}")
+            return AlertError(Instant.now(), "Failed evaluating trigger:\n${error.userErrorMessage()}")
         }
         for (actionResult in actionResults.values) {
             if (actionResult.error != null) {
-                return AlertError(Instant.now(), "Error running action:\n${actionResult.error.userErrorMessage()}")
+                return AlertError(Instant.now(), "Failed running action:\n${actionResult.error.userErrorMessage()}")
             }
         }
         return null

diff --git a/...n/com/amazon/opendistroforelasticsearch/alerting/transport/TransportIndexMonitorAction.kt b/...n/com/amazon/opendistroforelasticsearch/alerting/transport/TransportIndexMonitorAction.kt
@@ -24,7 +24,6 @@ import com.amazon.opendistroforelasticsearch.alerting.core.model.ScheduledJob.Co
 import com.amazon.opendistroforelasticsearch.alerting.core.model.ScheduledJob.Companion.SCHEDULED_JOB_TYPE
 import com.amazon.opendistroforelasticsearch.alerting.core.model.SearchInput
 import com.amazon.opendistroforelasticsearch.alerting.model.Monitor
-import com.amazon.opendistroforelasticsearch.alerting.model.destination.Destination
 import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings.Companion.ALERTING_MAX_MONITORS
 import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings.Companion.INDEX_TIMEOUT
 import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings.Companion.MAX_ACTION_THROTTLE_VALUE
@@ -35,6 +34,7 @@ import com.amazon.opendistroforelasticsearch.alerting.util.IndexUtils
 import com.amazon.opendistroforelasticsearch.commons.authuser.User
 import com.amazon.opendistroforelasticsearch.commons.authuser.AuthUserRequestBuilder
 import org.apache.logging.log4j.LogManager
+import org.elasticsearch.ElasticsearchSecurityException
 import org.elasticsearch.ElasticsearchStatusException
 import org.elasticsearch.action.ActionListener
 import org.elasticsearch.action.admin.indices.create.CreateIndexResponse
@@ -100,13 +100,46 @@ class TransportIndexMonitorAction @Inject constructor(
     }
 
     override fun doExecute(task: Task, request: IndexMonitorRequest, actionListener: ActionListener<IndexMonitorResponse>) {
+        checkIndicesAndExecute(client, actionListener, request)
+    }
 
-        if (!isValidIndex(request, actionListener))
-            return
-
-        client.threadPool().threadContext.stashContext().use {
-            IndexMonitorHandler(client, actionListener, request).resolveUserAndStart()
+    /**
+     *  Check if user has permissions to read the configured indices on the monitor and
+     *  then create monitor.
+     */
+    fun checkIndicesAndExecute(
+        client: Client,
+        actionListener: ActionListener<IndexMonitorResponse>,
+        request: IndexMonitorRequest
+    ) {
+        val indices = mutableListOf<String>()
+        val searchInputs = request.monitor.inputs.filter { it.name() == SearchInput.SEARCH_FIELD }
+        searchInputs.forEach {
+            val searchInput = it as SearchInput
+            indices.addAll(searchInput.indices)
         }
+        val searchRequest = SearchRequest().indices(*indices.toTypedArray())
+                .source(SearchSourceBuilder.searchSource().size(1).query(QueryBuilders.matchAllQuery()))
+        client.search(searchRequest, object : ActionListener<SearchResponse> {
+            override fun onResponse(searchResponse: SearchResponse) {
+                // User has read access to configured indices in the monitor, now create monitor with out user context.
+                client.threadPool().threadContext.stashContext().use {
+                    IndexMonitorHandler(client, actionListener, request).resolveUserAndStart()
+                }
+            }
+
+            //  Due to below issue with security plugin, we get security_exception when invalid index name is mentioned.
+            //  https://github.com/opendistro-for-elasticsearch/security/issues/718
+            override fun onFailure(t: Exception) {
+                actionListener.onFailure(AlertingException.wrap(
+                    when (t is ElasticsearchSecurityException) {
+                        true -> ElasticsearchStatusException("User doesn't have read permissions for one or more configured index " +
+                            "${indices.indices}", RestStatus.FORBIDDEN)
+                        false -> t
+                    }
+                ))
+            }
+        })
     }
 
     inner class IndexMonitorHandler(
@@ -175,7 +208,8 @@ class TransportIndexMonitorAction @Inject constructor(
          */
         private fun prepareMonitorIndexing() {
 
-            checkForDisallowedDestinations(allowList)
+            // Below check needs to be async operations and needs to be refactored issue#269
+            // checkForDisallowedDestinations(allowList)
 
             try {
                 validateActionThrottle(request.monitor, maxActionThrottle, TimeValue.timeValueMinutes(1))
@@ -348,7 +382,7 @@ class TransportIndexMonitorAction @Inject constructor(
             return null
         }
 
-        private fun checkForDisallowedDestinations(allowList: List<String>) {
+        /*private fun checkForDisallowedDestinations(allowList: List<String>) {
             this.request.monitor.triggers.forEach { trigger ->
                 trigger.actions.forEach { action ->
                     // Check for empty destinationId for test cases, otherwise we get test failures
@@ -380,34 +414,6 @@ class TransportIndexMonitorAction @Inject constructor(
                     }
                 })
             }
-        }
-    }
-
-    /**
-     *  Check if user has permissions to read the configured indices on the monitor.
-     *  Due to below issue with security plugin, we get security_exception when invalid index name is mentioned.
-     *  https://github.com/opendistro-for-elasticsearch/security/issues/718
-     */
-    private fun isValidIndex(request: IndexMonitorRequest, actionListener: ActionListener<IndexMonitorResponse>): Boolean {
-        var ret = true
-        val searchInputs = request.monitor.inputs.filter { it.name() == SearchInput.SEARCH_FIELD }
-        searchInputs.forEach {
-            val searchInput = it as SearchInput
-            val searchRequest = SearchRequest().indices(*searchInput.indices.toTypedArray())
-                    .source(SearchSourceBuilder.searchSource().size(1).query(QueryBuilders.matchAllQuery()))
-            client.search(searchRequest, object : ActionListener<SearchResponse> {
-                override fun onResponse(searchResponse: SearchResponse) {
-                    // ignore
-                }
-
-                override fun onFailure(t: Exception) {
-                    val ex = ElasticsearchStatusException("User doesn't have read permissions for the configured index " +
-                            "${searchInput.indices}", RestStatus.FORBIDDEN)
-                    actionListener.onFailure(AlertingException.wrap(ex))
-                    ret = false
-                }
-            })
-        }
-        return ret
+        }*/
     }
 }
diff --git a/alerting/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/MonitorRunnerIT.kt b/alerting/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/MonitorRunnerIT.kt
@@ -421,7 +421,7 @@ class MonitorRunnerIT : AlertingRestTestCase() {
         verifyAlert(errorAlert, monitor, ERROR)
         executeMonitor(monitor.id)
         assertEquals("Error does not match",
-                "Error evaluating trigger:\nparam[0]; return true\n^---- HERE", errorAlert.errorMessage)
+                "Failed evaluating trigger:\nparam[0]; return true\n^---- HERE", errorAlert.errorMessage)
     }
 
     fun `test execute monitor limits alert error history to 10 error messages`() {

diff --git a/...est/kotlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/MonitorRestApiIT.kt b/...est/kotlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/MonitorRestApiIT.kt
@@ -25,17 +25,12 @@ import com.amazon.opendistroforelasticsearch.alerting.makeRequest
 import com.amazon.opendistroforelasticsearch.alerting.model.Alert
 import com.amazon.opendistroforelasticsearch.alerting.model.Monitor
 import com.amazon.opendistroforelasticsearch.alerting.model.Trigger
-import com.amazon.opendistroforelasticsearch.alerting.model.destination.Chime
-import com.amazon.opendistroforelasticsearch.alerting.model.destination.Destination
 import com.amazon.opendistroforelasticsearch.alerting.randomAction
 import com.amazon.opendistroforelasticsearch.alerting.randomAlert
 import com.amazon.opendistroforelasticsearch.alerting.randomMonitor
 import com.amazon.opendistroforelasticsearch.alerting.randomThrottle
 import com.amazon.opendistroforelasticsearch.alerting.randomTrigger
-import com.amazon.opendistroforelasticsearch.alerting.randomUser
 import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings
-import com.amazon.opendistroforelasticsearch.alerting.settings.DestinationSettings
-import com.amazon.opendistroforelasticsearch.alerting.util.DestinationType
 import org.apache.http.HttpHeaders
 import org.apache.http.entity.ContentType
 import org.apache.http.message.BasicHeader
@@ -53,7 +48,6 @@ import org.elasticsearch.search.builder.SearchSourceBuilder
 import org.elasticsearch.test.ESTestCase
 import org.elasticsearch.test.junit.annotations.TestLogging
 import org.elasticsearch.test.rest.ESRestTestCase
-import java.time.Instant
 import java.time.ZoneId
 import java.time.temporal.ChronoUnit
 
@@ -163,6 +157,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
         }
     }
 
+    /* Enable this test case after issue issue#269 is fixed.
     fun `test creating a monitor with a disallowed destination type fails`() {
         try {
             // Create a Chime Destination
@@ -189,7 +184,7 @@ class MonitorRestApiIT : AlertingRestTestCase() {
         } catch (e: ResponseException) {
             assertEquals("Unexpected status", RestStatus.FORBIDDEN, e.response.restStatus())
         }
-    }
+    }*/
 
     @Throws(Exception::class)
     fun `test updating search for a monitor`() {

diff --git a/core/libs/common-utils-1.10.1.0.jar b/core/libs/common-utils-1.10.1.0.jar