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

[Backport 2.x] Added validation for the new clusters field. (#633) #636

Merged
merged 2 commits into from
Apr 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

plz add error log

Copy link
Member

Choose a reason for hiding this comment

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

why are we creating a common utils exception?
The plugin using common utils can handle the exception and wrap it as necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed CommonUtilsException in PR #639. Will cherry-pick that commit once that PR is merged.

}
}

@Throws(IOException::class)
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

what benefit are we getting from removing the delegation of execption handling to the plugin? there is no business logic in common utils
why should we have to throw a common utils exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed CommonUtilsException in PR #639. Will cherry-pick that commit once that PR is merged.


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}"))
}
}
}
18 changes: 18 additions & 0 deletions src/main/kotlin/org/opensearch/commons/alerting/util/IndexUtils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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}$""")
Copy link
Member

Choose a reason for hiding this comment

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

why aren't we using opensearch core's index name validation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was just moved to common utils from alerting; it wasn't implemented as part of these changes. Core didn't have a regex for index names. This comment explains where the validation came from https://github.com/opensearch-project/alerting/pull/992/files#r1259175796


/**
* 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
Expand Down
18 changes: 18 additions & 0 deletions src/main/kotlin/org/opensearch/commons/utils/ValidationHelpers.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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_-]*)$")
Copy link
Member

Choose a reason for hiding this comment

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

why are we not re-using cross cluster validation from core?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed this in this comment
#633 (comment)


/**
* 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<Char> = (('a'..'z') + ('A'..'Z') + ('0'..'9') + '+' + '/' + '_' + '-').toSet()

Expand Down
Loading
Loading