Add validation check for doc level query name during monitor creation #1506
Conversation
Signed-off-by: Joanne Wang <jowg@amazon.com>
Signed-off-by: Joanne Wang <jowg@amazon.com>
jowg-amazon
requested review from
lezzago,
AWSHurneyt,
sbcd90,
eirsep,
getsaurabh02,
praveensameneni,
qreshi,
bowenlan-amzn,
rishabhmaurya,
engechas and
riysaxen-amzn
as code owners
| @@ -116,9 +122,11 @@ class RestIndexMonitorAction : BaseRestHandler() { | |||
| if (it !is DocumentLevelTrigger) { | |||
| throw IllegalArgumentException("Illegal trigger type, ${it.javaClass.name}, for document level monitor") | |||
| } | |||
| validateDocLevelQueryName(monitor) | |||
There was a problem hiding this comment.
Looks like this is iterating through each trigger configured for the monitor, and then validating all of the query names during each iteration. Since queries are at the monitor level, not the trigger level, would it make sense to move this validation check out of the forEach loop?
There was a problem hiding this comment.
Yes agreed, moved the validation outside the forEach loop
| docLevelMonitorInput.queries.forEach { dlq -> | ||
| if (!dlq.name.matches(Regex(regex))) { | ||
| throw IllegalArgumentException( | ||
| "Doc level query name, ${dlq.name}, may only contain alphanumeric values and " + |
There was a problem hiding this comment.
We shouldn't return user input in error messages; so could we reword this error message to leave out ${dlq.name}?
There was a problem hiding this comment.
Reworded the error message to remove dlq.name
| monitor.inputs.filterIsInstance<DocLevelMonitorInput>().forEach { docLevelMonitorInput -> | ||
| docLevelMonitorInput.queries.forEach { dlq -> | ||
| if (!dlq.name.matches(Regex(regex))) { | ||
| throw IllegalArgumentException( |
There was a problem hiding this comment.
Just to confirm, does this end up throwing an IllegalArgumentException, or does it get wrapped in an AlertingException somewhere else? I believe IllegalArgumentException results in a 500 error, which we want to avoid.
There was a problem hiding this comment.
Never mind, I see in your integ test that the expected error status is RestStatus.BAD_REQUEST, which is a 400 error; so the IllegalArgumentException must be getting caught, and wrapped somewhere else.
Signed-off-by: Joanne Wang <jowg@amazon.com>
|
Security tests have been failing in previous PRs |
…#1506) * add validation check for doc level query name during monitor creation Signed-off-by: Joanne Wang <jowg@amazon.com> * change to 0-256 chars Signed-off-by: Joanne Wang <jowg@amazon.com> * change validation message and move validation loc Signed-off-by: Joanne Wang <jowg@amazon.com> --------- Signed-off-by: Joanne Wang <jowg@amazon.com> (cherry picked from commit 636b43f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…#1506) * add validation check for doc level query name during monitor creation Signed-off-by: Joanne Wang <jowg@amazon.com> * change to 0-256 chars Signed-off-by: Joanne Wang <jowg@amazon.com> * change validation message and move validation loc Signed-off-by: Joanne Wang <jowg@amazon.com> --------- Signed-off-by: Joanne Wang <jowg@amazon.com> (cherry picked from commit 636b43f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…#1506) (#1511) * add validation check for doc level query name during monitor creation * change to 0-256 chars * change validation message and move validation loc --------- (cherry picked from commit 636b43f) Signed-off-by: Joanne Wang <jowg@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…#1506) (#1512) * add validation check for doc level query name during monitor creation * change to 0-256 chars * change validation message and move validation loc --------- (cherry picked from commit 636b43f) Signed-off-by: Joanne Wang <jowg@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…#1506) * add validation check for doc level query name during monitor creation Signed-off-by: Joanne Wang <jowg@amazon.com> * change to 0-256 chars Signed-off-by: Joanne Wang <jowg@amazon.com> * change validation message and move validation loc Signed-off-by: Joanne Wang <jowg@amazon.com> --------- Signed-off-by: Joanne Wang <jowg@amazon.com> (cherry picked from commit 636b43f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Issue #, if available:
PR related to: opensearch-project/common-utils#630
CI's will fail until common utils PR is merged in
Description of changes:
Adds validation check for doc level query name during monitor creation. Doc level query is restricted so that it may not start with:
[_, +, -], contain.., or contain:[* ? < > | #]CheckList:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.