From 04ddb3d9507a98425098bf8322f46592e0bd6ebc Mon Sep 17 00:00:00 2001 From: AWSHurneyt Date: Fri, 12 Apr 2024 10:53:07 -0700 Subject: [PATCH] Added validation for the clusters field. Refactored ClusterMetricsInput validiation to throw 4xx-level CommonUtilsExceptions instead of 5xx-level IllegalArgumentException. Signed-off-by: AWSHurneyt --- .../alerting/model/ClusterMetricsInput.kt | 82 ++++++---- .../alerting/util/CommonUtilsException.kt | 71 ++++++++ .../model/ClusterMetricsInputTests.kt | 151 ++++++++++++++++-- 3 files changed, 258 insertions(+), 46 deletions(-) create mode 100644 src/main/kotlin/org/opensearch/commons/alerting/util/CommonUtilsException.kt diff --git a/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt b/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt index 8c8b2429..274bc543 100644 --- a/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt +++ b/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt @@ -3,6 +3,7 @@ package org.opensearch.commons.alerting.model import org.apache.commons.validator.routines.UrlValidator import org.apache.hc.core5.net.URIBuilder import org.opensearch.common.CheckedFunction +import org.opensearch.commons.alerting.util.CommonUtilsException import org.opensearch.core.ParseField import org.opensearch.core.common.io.stream.StreamInput import org.opensearch.core.common.io.stream.StreamOutput @@ -15,6 +16,14 @@ import java.io.IOException import java.net.URI import java.net.URISyntaxException +/** + * This regex asserts that the string: + * Starts with a lowercase letter, or digit + * Contains a sequence of characters followed by an optional colon and another sequence of characters + * The sequences of characters can include lowercase letters, uppercase letters, digits, underscores, or hyphens + * The total length of the string can range from 1 to 255 characters + */ +val CLUSTER_NAME_REGEX = Regex("^(?=.{1,255}$)[a-z0-9]([a-zA-Z0-9_-]*:?[a-zA-Z0-9_-]*)$") val ILLEGAL_PATH_PARAMETER_CHARACTERS = arrayOf(':', '"', '+', '\\', '|', '?', '#', '>', '<', ' ') /** @@ -31,35 +40,46 @@ data class ClusterMetricsInput( // Verify parameters are valid during creation init { - require(validateFields()) { - "The uri.api_type field, uri.path field, or uri.uri field must be defined." - } + // Wrap any validation exceptions in CommonUtilsException. + try { + require(validateFields()) { + "The uri.api_type field, uri.path field, or uri.uri field must be defined." + } - // Create an UrlValidator that only accepts "http" and "https" as valid scheme and allows local URLs. - val urlValidator = UrlValidator(arrayOf("http", "https"), UrlValidator.ALLOW_LOCAL_URLS) + // Create an UrlValidator that only accepts "http" and "https" as valid scheme and allows local URLs. + val urlValidator = UrlValidator(arrayOf("http", "https"), UrlValidator.ALLOW_LOCAL_URLS) - // Build url field by field if not provided as whole. - constructedUri = toConstructedUri() + // Build url field by field if not provided as whole. + constructedUri = toConstructedUri() - require(urlValidator.isValid(constructedUri.toString())) { - "Invalid URI constructed from the path and path_params inputs, or the url input." - } + require(urlValidator.isValid(constructedUri.toString())) { + "Invalid URI constructed from the path and path_params inputs, or the url input." + } - if (url.isNotEmpty() && validateFieldsNotEmpty()) { - require(constructedUri == constructUrlFromInputs()) { - "The provided URL and URI fields form different URLs." + if (url.isNotEmpty() && validateFieldsNotEmpty()) { + require(constructedUri == constructUrlFromInputs()) { + "The provided URL and URI fields form different URLs." + } } - } - require(constructedUri.host.lowercase() == SUPPORTED_HOST) { - "Only host '$SUPPORTED_HOST' is supported." - } - require(constructedUri.port == SUPPORTED_PORT) { - "Only port '$SUPPORTED_PORT' is supported." - } + require(constructedUri.host.lowercase() == SUPPORTED_HOST) { + "Only host '$SUPPORTED_HOST' is supported." + } + require(constructedUri.port == SUPPORTED_PORT) { + "Only port '$SUPPORTED_PORT' is supported." + } + + if (clusters.isNotEmpty()) { + require(clusters.all { CLUSTER_NAME_REGEX.matches(it) }) { + "Cluster names are not valid." + } + } - clusterMetricType = findApiType(constructedUri.path) - this.parseEmptyFields() + clusterMetricType = findApiType(constructedUri.path) + this.parseEmptyFields() + } catch (exception: Exception) { + throw CommonUtilsException.wrap(exception) + } } @Throws(IOException::class) @@ -158,7 +178,7 @@ data class ClusterMetricsInput( /** * Isolates just the path parameters from the [ClusterMetricsInput] URI. * @return The path parameters portion of the [ClusterMetricsInput] URI. - * @throws IllegalArgumentException if the [ClusterMetricType] requires path parameters, but none are supplied; + * @throws CommonUtilsException if the [ClusterMetricType] requires path parameters, but none are supplied; * or when path parameters are provided for an [ClusterMetricType] that does not use path parameters. */ fun parsePathParams(): String { @@ -178,18 +198,22 @@ data class ClusterMetricsInput( pathParams = pathParams.trim('/') ILLEGAL_PATH_PARAMETER_CHARACTERS.forEach { character -> if (pathParams.contains(character)) { - throw IllegalArgumentException( - "The provided path parameters contain invalid characters or spaces. Please omit: " + ILLEGAL_PATH_PARAMETER_CHARACTERS.joinToString(" ") + throw CommonUtilsException.wrap( + IllegalArgumentException( + "The provided path parameters contain invalid characters or spaces. Please omit: " + ILLEGAL_PATH_PARAMETER_CHARACTERS.joinToString( + " " + ) + ) ) } } } if (apiType.requiresPathParams && pathParams.isEmpty()) { - throw IllegalArgumentException("The API requires path parameters.") + throw CommonUtilsException.wrap(IllegalArgumentException("The API requires path parameters.")) } if (!apiType.supportsPathParams && pathParams.isNotEmpty()) { - throw IllegalArgumentException("The API does not use path parameters.") + throw CommonUtilsException.wrap(IllegalArgumentException("The API does not use path parameters.")) } return pathParams @@ -199,7 +223,7 @@ data class ClusterMetricsInput( * Examines the path of a [ClusterMetricsInput] to determine which API is being called. * @param uriPath The path to examine. * @return The [ClusterMetricType] associated with the [ClusterMetricsInput] monitor. - * @throws IllegalArgumentException when the API to call cannot be determined from the URI. + * @throws CommonUtilsException when the API to call cannot be determined from the URI. */ private fun findApiType(uriPath: String): ClusterMetricType { var apiType = ClusterMetricType.BLANK @@ -211,7 +235,7 @@ data class ClusterMetricsInput( } } if (apiType.isBlank()) { - throw IllegalArgumentException("The API could not be determined from the provided URI.") + throw CommonUtilsException.wrap(IllegalArgumentException("The API could not be determined from the provided URI.")) } return apiType } diff --git a/src/main/kotlin/org/opensearch/commons/alerting/util/CommonUtilsException.kt b/src/main/kotlin/org/opensearch/commons/alerting/util/CommonUtilsException.kt new file mode 100644 index 00000000..0fb19610 --- /dev/null +++ b/src/main/kotlin/org/opensearch/commons/alerting/util/CommonUtilsException.kt @@ -0,0 +1,71 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.commons.alerting.util + +import org.apache.logging.log4j.LogManager +import org.opensearch.OpenSearchException +import org.opensearch.OpenSearchSecurityException +import org.opensearch.OpenSearchStatusException +import org.opensearch.core.common.Strings +import org.opensearch.core.rest.RestStatus +import org.opensearch.index.IndexNotFoundException +import org.opensearch.index.engine.VersionConflictEngineException +import org.opensearch.indices.InvalidIndexNameException + +private val log = LogManager.getLogger(CommonUtilsException::class.java) + +class CommonUtilsException(message: String, val status: RestStatus, ex: Exception) : OpenSearchException(message, ex) { + + override fun status(): RestStatus { + return status + } + + companion object { + + @JvmStatic + fun wrap(ex: Exception): OpenSearchException { + log.error("Common utils error: $ex") + + var friendlyMsg = "Unknown error" + var status = RestStatus.INTERNAL_SERVER_ERROR + when (ex) { + is IndexNotFoundException -> { + status = ex.status() + friendlyMsg = "Configured indices are not found: ${ex.index}" + } + is OpenSearchSecurityException -> { + status = ex.status() + friendlyMsg = "User doesn't have permissions to execute this action. Contact administrator." + } + is OpenSearchStatusException -> { + status = ex.status() + friendlyMsg = ex.message as String + } + is IllegalArgumentException -> { + status = RestStatus.BAD_REQUEST + friendlyMsg = ex.message as String + } + is VersionConflictEngineException -> { + status = ex.status() + friendlyMsg = ex.message as String + } + is InvalidIndexNameException -> { + status = RestStatus.BAD_REQUEST + friendlyMsg = ex.message as String + } + else -> { + if (!Strings.isNullOrEmpty(ex.message)) { + friendlyMsg = ex.message as String + } + } + } + // Wrapping the origin exception as runtime to avoid it being formatted. + // Currently, alerting-kibana is using `error.root_cause.reason` as text in the toast message. + // Below logic is to set friendly message to error.root_cause.reason. + return CommonUtilsException(friendlyMsg, status, Exception("${ex.javaClass.name}: ${ex.message}")) + } + } +} diff --git a/src/test/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInputTests.kt b/src/test/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInputTests.kt index b96038d3..8e92403c 100644 --- a/src/test/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInputTests.kt +++ b/src/test/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInputTests.kt @@ -1,6 +1,7 @@ package org.opensearch.commons.alerting.model import org.junit.jupiter.api.Test +import org.opensearch.commons.alerting.util.CommonUtilsException import kotlin.test.assertEquals import kotlin.test.assertFailsWith @@ -9,6 +10,26 @@ class ClusterMetricsInputTests { private var pathParams = "" private var url = "" + private val validClusters = listOf( + "cluster-name", + "cluster:name" + ) + + private val invalidClusters = listOf( + // Character length less than 1 should return FALSE + "", + + // Character length greater than 255 should return FALSE + (0..255).joinToString(separator = "") { "a" }, + + // Invalid characters should return FALSE + "cluster-#name", + "cluster:#name", + + // More than 1 `:` character should return FALSE + "bad:cluster:name" + ) + @Test fun `test valid ClusterMetricsInput creation using HTTP URI component fields`() { // GIVEN @@ -21,6 +42,7 @@ class ClusterMetricsInputTests { assertEquals(path, clusterMetricsInput.path) assertEquals(pathParams, clusterMetricsInput.pathParams) assertEquals(testUrl, clusterMetricsInput.url) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -34,6 +56,7 @@ class ClusterMetricsInputTests { // THEN assertEquals(url, clusterMetricsInput.url) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -47,6 +70,7 @@ class ClusterMetricsInputTests { // THEN assertEquals(url, clusterMetricsInput.url) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -55,7 +79,7 @@ class ClusterMetricsInputTests { path = "///" // WHEN + THEN - assertFailsWith("Invalid URL.") { + assertFailsWith("Invalid URL.") { ClusterMetricsInput(path, pathParams, url) } } @@ -66,7 +90,7 @@ class ClusterMetricsInputTests { url = "///" // WHEN + THEN - assertFailsWith("Invalid URL.") { + assertFailsWith("Invalid URL.") { ClusterMetricsInput(path, pathParams, url) } } @@ -84,6 +108,7 @@ class ClusterMetricsInputTests { assertEquals(pathParams, clusterMetricsInput.pathParams) assertEquals(url, clusterMetricsInput.url) assertEquals(url, clusterMetricsInput.constructedUri.toString()) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -101,6 +126,7 @@ class ClusterMetricsInputTests { assertEquals(pathParams, clusterMetricsInput.pathParams) assertEquals(url, clusterMetricsInput.url) assertEquals(url, clusterMetricsInput.constructedUri.toString()) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -109,7 +135,7 @@ class ClusterMetricsInputTests { url = "http://localhost:9200/_cluster/stats" // WHEN + THEN - assertFailsWith("The provided URL and URI fields form different URLs.") { + assertFailsWith("The provided URL and URI fields form different URLs.") { ClusterMetricsInput(path, pathParams, url) } } @@ -121,7 +147,7 @@ class ClusterMetricsInputTests { url = "http://localhost:9200/_cluster/stats/index1,index2,index3,index4,index5" // WHEN + THEN - assertFailsWith("The provided URL and URI fields form different URLs.") { + assertFailsWith("The provided URL and URI fields form different URLs.") { ClusterMetricsInput(path, pathParams, url) } } @@ -134,7 +160,7 @@ class ClusterMetricsInputTests { url = "" // WHEN + THEN - assertFailsWith("The uri.api_type field, uri.path field, or uri.uri field must be defined.") { + assertFailsWith("The uri.api_type field, uri.path field, or uri.uri field must be defined.") { ClusterMetricsInput(path, pathParams, url) } } @@ -147,7 +173,7 @@ class ClusterMetricsInputTests { url = "" // WHEN + THEN - assertFailsWith("The uri.api_type field, uri.path field, or uri.uri field must be defined.") { + assertFailsWith("The uri.api_type field, uri.path field, or uri.uri field must be defined.") { ClusterMetricsInput(path, pathParams, url) } } @@ -159,7 +185,7 @@ class ClusterMetricsInputTests { url = "invalidScheme://localhost:9200/_cluster/health" // WHEN + THEN - assertFailsWith("Invalid URL.") { + assertFailsWith("Invalid URL.") { ClusterMetricsInput(path, pathParams, url) } } @@ -171,7 +197,7 @@ class ClusterMetricsInputTests { url = "http://127.0.0.1:9200/_cluster/health" // WHEN + THEN - assertFailsWith("Only host '${ClusterMetricsInput.SUPPORTED_HOST}' is supported.") { + assertFailsWith("Only host '${ClusterMetricsInput.SUPPORTED_HOST}' is supported.") { ClusterMetricsInput(path, pathParams, url) } } @@ -183,7 +209,7 @@ class ClusterMetricsInputTests { url = "http://localhost:${ClusterMetricsInput.SUPPORTED_PORT + 1}/_cluster/health" // WHEN + THEN - assertFailsWith("Only port '${ClusterMetricsInput.SUPPORTED_PORT}' is supported.") { + assertFailsWith("Only port '${ClusterMetricsInput.SUPPORTED_PORT}' is supported.") { ClusterMetricsInput(path, pathParams, url) } } @@ -200,6 +226,7 @@ class ClusterMetricsInputTests { // THEN assertEquals(pathParams, params) assertEquals(testUrl, clusterMetricsInput.constructedUri.toString()) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -216,6 +243,7 @@ class ClusterMetricsInputTests { // THEN assertEquals(pathParams, params) assertEquals(testUrl, clusterMetricsInput.constructedUri.toString()) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -232,6 +260,7 @@ class ClusterMetricsInputTests { // THEN assertEquals(testParams, params) assertEquals(url, clusterMetricsInput.constructedUri.toString()) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -240,7 +269,7 @@ class ClusterMetricsInputTests { path = "/_cat/snapshots" // WHEN + THEN - assertFailsWith("The API requires path parameters.") { + assertFailsWith("The API requires path parameters.") { ClusterMetricsInput(path, pathParams, url) } } @@ -253,7 +282,7 @@ class ClusterMetricsInputTests { val clusterMetricsInput = ClusterMetricsInput(path, pathParams, url) // WHEN + THEN - assertFailsWith("The API does not use path parameters.") { + assertFailsWith("The API does not use path parameters.") { clusterMetricsInput.parsePathParams() } } @@ -267,8 +296,8 @@ class ClusterMetricsInputTests { val clusterMetricsInput = ClusterMetricsInput(path, pathParams, url) // WHEN + THEN - assertFailsWith( - "The provided path parameters contain invalid characters or spaces. Please omit: " + "${ILLEGAL_PATH_PARAMETER_CHARACTERS.joinToString(" ")}" + assertFailsWith( + "The provided path parameters contain invalid characters or spaces. Please omit: " + ILLEGAL_PATH_PARAMETER_CHARACTERS.joinToString(" ") ) { clusterMetricsInput.parsePathParams() } @@ -365,7 +394,7 @@ class ClusterMetricsInputTests { path = "/_cat/paws" // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput(path, pathParams, url) } } @@ -377,7 +406,7 @@ class ClusterMetricsInputTests { pathParams = "index1,index2,index3,index4,index5" // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput(path, pathParams, url) } } @@ -389,7 +418,7 @@ class ClusterMetricsInputTests { url = "http://localhost:9200/_cat/paws" // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput(path, pathParams, url) } } @@ -401,7 +430,7 @@ class ClusterMetricsInputTests { url = "http://localhost:9200/_cat/paws/index1,index2,index3,index4,index5" // WHEN + THEN - assertFailsWith("The API could not be determined from the provided URI.") { + assertFailsWith("The API could not be determined from the provided URI.") { ClusterMetricsInput(path, pathParams, url) } } @@ -422,6 +451,7 @@ class ClusterMetricsInputTests { assertEquals(testPath, clusterMetricsInput.path) assertEquals(testPathParams, clusterMetricsInput.pathParams) assertEquals(url, clusterMetricsInput.url) + assertEquals(emptyList(), clusterMetricsInput.clusters) } @Test @@ -438,5 +468,92 @@ class ClusterMetricsInputTests { assertEquals(path, clusterMetricsInput.path) assertEquals(pathParams, clusterMetricsInput.pathParams) assertEquals(testUrl, clusterMetricsInput.url) + assertEquals(emptyList(), clusterMetricsInput.clusters) + } + + @Test + fun `test a single valid cluster`() { + validClusters.forEach { + // GIVEN + path = "/_cluster/health" + pathParams = "index1,index2,index3,index4,index5" + url = "" + val clusters = listOf(it) + + // WHEN + val clusterMetricsInput = ClusterMetricsInput( + path = path, + pathParams = pathParams, + url = url, + clusters = clusters + ) + + // THEN + assertEquals(path, clusterMetricsInput.path) + assertEquals(pathParams, clusterMetricsInput.pathParams) + assertEquals(clusters, clusterMetricsInput.clusters) + } + } + + @Test + fun `test multiple valid clusters`() { + // GIVEN + path = "/_cluster/health" + pathParams = "index1,index2,index3,index4,index5" + url = "" + val clusters = validClusters + + // WHEN + val clusterMetricsInput = ClusterMetricsInput( + path = path, + pathParams = pathParams, + url = url, + clusters = clusters + ) + + // THEN + assertEquals(path, clusterMetricsInput.path) + assertEquals(pathParams, clusterMetricsInput.pathParams) + assertEquals(clusters, clusterMetricsInput.clusters) + } + + @Test + fun `test a single invalid cluster`() { + invalidClusters.forEach { + // GIVEN + path = "/_cluster/health" + pathParams = "index1,index2,index3,index4,index5" + url = "" + val clusters = listOf(it) + + // WHEN + THEN + assertFailsWith("The API could not be determined from the provided URI.") { + ClusterMetricsInput( + path = path, + pathParams = pathParams, + url = url, + clusters = clusters + ) + } + } + } + + @Test + fun `test multiple invalid clusters`() { + // GIVEN + path = "/_cluster/health" + pathParams = "index1,index2,index3,index4,index5" + url = "" + val clusters = invalidClusters + + // WHEN + THEN + assertFailsWith("The API could not be determined from the provided URI.") { + ClusterMetricsInput( + path = path, + pathParams = pathParams, + url = url, + clusters = clusters + ) + } } }