From e70256ca8a415466936cc2ec024bd39858a64b5d Mon Sep 17 00:00:00 2001 From: AWSHurneyt Date: Fri, 12 Apr 2024 14:38:31 -0700 Subject: [PATCH] Added validation for the new clusters field. (#633) * Added validation for the clusters field. Refactored ClusterMetricsInput validiation to throw 4xx-level CommonUtilsExceptions instead of 5xx-level IllegalArgumentException. Signed-off-by: AWSHurneyt * Moved some regex from alerting plugin to common utils. Signed-off-by: AWSHurneyt * Moved cluster-based regex to separate file. Signed-off-by: AWSHurneyt * Fixed ktlint error. Signed-off-by: AWSHurneyt * Fixed regex. Moved cluster-related regexes. Signed-off-by: AWSHurneyt --------- Signed-off-by: AWSHurneyt --- .../alerting/model/ClusterMetricsInput.kt | 75 +++++---- .../alerting/util/CommonUtilsException.kt | 71 +++++++++ .../commons/alerting/util/IndexUtils.kt | 18 +++ .../commons/utils/ValidationHelpers.kt | 18 +++ .../model/ClusterMetricsInputTests.kt | 149 ++++++++++++++++-- 5 files changed, 286 insertions(+), 45 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 f834b435..b2c454e2 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,8 @@ package org.opensearch.commons.alerting.model import org.apache.commons.validator.routines.UrlValidator import org.apache.http.client.utils.URIBuilder import org.opensearch.common.CheckedFunction +import org.opensearch.commons.alerting.util.CommonUtilsException +import org.opensearch.commons.utils.CLUSTER_NAME_REGEX import org.opensearch.core.ParseField import org.opensearch.core.common.io.stream.StreamInput import org.opensearch.core.common.io.stream.StreamOutput @@ -31,35 +33,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." + } - clusterMetricType = findApiType(constructedUri.path) - this.parseEmptyFields() + if (clusters.isNotEmpty()) { + require(clusters.all { CLUSTER_NAME_REGEX.matches(it) }) { + "Cluster names are not valid." + } + } + + clusterMetricType = findApiType(constructedUri.path) + this.parseEmptyFields() + } catch (exception: Exception) { + throw CommonUtilsException.wrap(exception) + } } @Throws(IOException::class) @@ -158,7 +171,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 +191,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 +216,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 +228,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/main/kotlin/org/opensearch/commons/alerting/util/IndexUtils.kt b/src/main/kotlin/org/opensearch/commons/alerting/util/IndexUtils.kt index 2dbda47b..18454465 100644 --- a/src/main/kotlin/org/opensearch/commons/alerting/util/IndexUtils.kt +++ b/src/main/kotlin/org/opensearch/commons/alerting/util/IndexUtils.kt @@ -11,6 +11,24 @@ import java.time.Instant class IndexUtils { companion object { + /** + * This regex asserts that the string: + * The index does not start with an underscore _, hyphen -, or plus sign + + * The index does not contain two consecutive periods (e.g., `..`) + * The index does not contain any whitespace characters, commas, backslashes, forward slashes, asterisks, + * question marks, double quotes, less than or greater than signs, pipes, colons, or periods. + * The length of the index must be between 1 and 255 characters + */ + val VALID_INDEX_NAME_REGEX = Regex("""^(?![_\-\+])(?!.*\.\.)[^\s,\\\/\*\?"<>|#:\.]{1,255}$""") + + /** + * This regex asserts that the string: + * The index pattern can start with an optional period + * The index pattern can contain lowercase letters, digits, underscores, hyphens, asterisks, and periods + * The length of the index pattern must be between 1 and 255 characters + */ + val INDEX_PATTERN_REGEX = Regex("""^(?=.{1,255}$)\.?[a-z0-9_\-\*\.]+$""") + const val NO_SCHEMA_VERSION = 0 const val MONITOR_MAX_INPUTS = 1 diff --git a/src/main/kotlin/org/opensearch/commons/utils/ValidationHelpers.kt b/src/main/kotlin/org/opensearch/commons/utils/ValidationHelpers.kt index ab9f7409..c34a7850 100644 --- a/src/main/kotlin/org/opensearch/commons/utils/ValidationHelpers.kt +++ b/src/main/kotlin/org/opensearch/commons/utils/ValidationHelpers.kt @@ -8,6 +8,24 @@ package org.opensearch.commons.utils import java.net.URL import java.util.regex.Pattern +/** + * 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_-]*)$") + +/** + * This regex asserts that the string: + * Starts with a lowercase letter, digit, or asterisk + * 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, asterisks, or hyphens + * The total length of the string can range from 1 to 255 characters + */ +val CLUSTER_PATTERN_REGEX = Regex("^(?=.{1,255}$)[a-z0-9*]([a-zA-Z0-9_*-]*:?[a-zA-Z0-9_*-]*)$") + // Valid ID characters = (All Base64 chars + "_-") to support UUID format and Base64 encoded IDs private val VALID_ID_CHARS: Set = (('a'..'z') + ('A'..'Z') + ('0'..'9') + '+' + '/' + '_' + '-').toSet() 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 9980d5db..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,7 +296,7 @@ class ClusterMetricsInputTests { val clusterMetricsInput = ClusterMetricsInput(path, pathParams, url) // WHEN + THEN - assertFailsWith( + 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 + ) + } } }