Change doc level query name validation #630

jowg-amazon · 2024-04-05T18:56:28Z

Description

Changes the doc level query name validation to restrict the name from 0-256 characters, similar to the validation done on the front end

Name validation regex modified from https://github.com/opensearch-project/alerting/blob/d2a590e744c920820536efc5e1a88a4185eeed74/alerting/src/main/kotlin/org/opensearch/alerting/util/IndexUtils.kt#L29

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

Signed-off-by: jowg-amazon <jowg@amazon.com>

codecov · 2024-04-05T19:01:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.91%. Comparing base (3f5421f) to head (150413f).
Report is 1 commits behind head on main.

❗ Current head 150413f differs from pull request most recent head 6904220. Consider uploading reports for the commit 6904220 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #630      +/-   ##
============================================
- Coverage     73.91%   73.91%   -0.01%     
  Complexity      891      891              
============================================
  Files           133      133              
  Lines          5870     5873       +3     
  Branches        717      718       +1     
============================================
+ Hits           4339     4341       +2     
  Misses         1219     1219              
- Partials        312      313       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

amsiglan · 2024-04-05T20:53:03Z

src/main/kotlin/org/opensearch/commons/alerting/model/DocLevelQuery.kt

@@ -160,15 +161,20 @@ data class DocLevelQuery(
        }

        // TODO: add test for this


Do you want to address this as part of this PR?

This PR modifies the existing test in DocLevelMonitorInputTests, should we add more testing for this? If not I will remove this comment.

common-utils/src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt

Line 45 in 150413f

fun `test create Doc Level Query with invalid name length`() {

If there is no test for the part of the code that the TODO was added for, then let's add one?

These two unit tests should be sufficient to test the two validation functions. I think the TODO may have been left in after the tests were created. These two tests create an invalid doc level query so that validation for a doc level query name and doc level query tag exceptions are caught.

common-utils/src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt

Line 45 in aa2b6c8

fun `test create Doc Level Query with invalid name length`() {

common-utils/src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt

Line 65 in aa2b6c8

fun `test create Doc Level Query with invalid characters for tags`() {

Will take out the TODO comment

amsiglan · 2024-04-05T20:53:31Z

src/main/kotlin/org/opensearch/commons/alerting/model/DocLevelQuery.kt

@@ -80,6 +80,7 @@ data class DocLevelQuery(
        const val QUERY_FIELD_NAMES_FIELD = "query_field_names"
        const val NO_ID = ""
        val INVALID_CHARACTERS: List<String> = listOf(" ", "[", "]", "{", "}", "(", ")")
+        val QUERY_NAME_REGEX = "^.{0,256}$".toRegex() // regex to restrict string length 256 chars


Doubt: How did we decide to cap the length at 256?

Another comment - this regex differs from the one in alerting: https://github.com/opensearch-project/alerting/pull/1506/files#diff-c8b7d067c22deb1581efc7a7286769d1c24d1a96146f8a4ecb92ef0dec16c44aR53

What's the reason for a difference?

I chose 256 as the cap based on security analytics front end validation for a rule name in hopes of remaining somewhat consistent:
https://github.com/opensearch-project/security-analytics-dashboards-plugin/blob/3e01e1631fa8aaff4e0968c68424a81abff03981/public/utils/validation.ts#L14

I had to change the length to 0-256 because when security analytics creates findings from bucket level monitors, it will create a doc level query with an empty name.
https://github.com/opensearch-project/security-analytics/blob/0507239054d238dac1e9cf53cfde488aeb4be1c2/src/main/java/org/opensearch/securityanalytics/findings/FindingsService.java#L302

The regex is different from alerting and security analytics because when security analytics converts the prepackaged sigma rules into doc level queries, there are some existing prepackaged rules that have some of the special characters like ( so we can't include all the validation at the doc level query level.

engechas

Curious on the 256 length limit as well but seems fine to me regardless so approving

engechas · 2024-04-05T21:32:40Z

src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt

+        repeat(256) {
+            stringBuilder.append("a")
+        }
+        val badString = stringBuilder.toString() + "b".repeat(256)


Minor: Looks like this is testing a string of length 512. Would be better to test at the limit (257) imo

changed testing to 257 chars

engechas · 2024-04-05T21:33:32Z

src/main/kotlin/org/opensearch/commons/alerting/model/DocLevelQuery.kt

@@ -80,6 +80,7 @@ data class DocLevelQuery(
        const val QUERY_FIELD_NAMES_FIELD = "query_field_names"
        const val NO_ID = ""
        val INVALID_CHARACTERS: List<String> = listOf(" ", "[", "]", "{", "}", "(", ")")
+        val QUERY_NAME_REGEX = "^.{0,256}$".toRegex() // regex to restrict string length 256 chars


Signed-off-by: jowg-amazon <jowg@amazon.com>

engechas · 2024-04-12T21:08:25Z

src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt

+            Assertions.fail("Expecting an illegal argument exception")
+        } catch (e: IllegalArgumentException) {
+            Assertions.assertEquals(
+                "The query name, $emptyString, can be max 256 characters.",


Minor: the error message should say between 1-256 characters

Signed-off-by: jowg-amazon <jowg@amazon.com>

* change validation and tests Signed-off-by: jowg-amazon <jowg@amazon.com> * change regex to 0-256 chars Signed-off-by: jowg-amazon <jowg@amazon.com> * moved validation to common utils and fix test Signed-off-by: jowg-amazon <jowg@amazon.com> * remove TODO Signed-off-by: jowg-amazon <jowg@amazon.com> * ensure name is at lest 1 char Signed-off-by: jowg-amazon <jowg@amazon.com> * change error message Signed-off-by: jowg-amazon <jowg@amazon.com> --------- Signed-off-by: jowg-amazon <jowg@amazon.com> (cherry picked from commit 9beae87) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* change validation and tests * change regex to 0-256 chars * moved validation to common utils and fix test * remove TODO * ensure name is at lest 1 char * change error message --------- (cherry picked from commit 9beae87) Signed-off-by: jowg-amazon <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>

* change validation and tests Signed-off-by: jowg-amazon <jowg@amazon.com> * change regex to 0-256 chars Signed-off-by: jowg-amazon <jowg@amazon.com> * moved validation to common utils and fix test Signed-off-by: jowg-amazon <jowg@amazon.com> * remove TODO Signed-off-by: jowg-amazon <jowg@amazon.com> * ensure name is at lest 1 char Signed-off-by: jowg-amazon <jowg@amazon.com> * change error message Signed-off-by: jowg-amazon <jowg@amazon.com> --------- Signed-off-by: jowg-amazon <jowg@amazon.com> (cherry picked from commit 9beae87) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

jowg-amazon added 2 commits April 5, 2024 11:20

change validation and tests

6ed40de

Signed-off-by: jowg-amazon <jowg@amazon.com>

change regex to 0-256 chars

150413f

Signed-off-by: jowg-amazon <jowg@amazon.com>

jowg-amazon requested review from lezzago, qreshi, bowenlan-amzn, rishabhmaurya, getsaurabh02, eirsep, sbcd90, AWSHurneyt, engechas and riysaxen-amzn as code owners April 5, 2024 18:56

jowg-amazon mentioned this pull request Apr 5, 2024

Change doc level query name validation #629

Closed

5 tasks

This was referenced Apr 5, 2024

Add validation check for doc level query name during monitor creation opensearch-project/alerting#1506

Merged

[BUG] Changes doc level query name field from id to rule name and adds validation opensearch-project/security-analytics#972

Merged

amsiglan reviewed Apr 5, 2024

View reviewed changes

engechas previously approved these changes Apr 5, 2024

View reviewed changes

moved validation to common utils and fix test

aa2b6c8

Signed-off-by: jowg-amazon <jowg@amazon.com>

jowg-amazon dismissed engechas’s stale review via aa2b6c8 April 8, 2024 22:23

engechas previously approved these changes Apr 10, 2024

View reviewed changes

remove TODO

272513e

Signed-off-by: jowg-amazon <jowg@amazon.com>

jowg-amazon dismissed engechas’s stale review via 272513e April 10, 2024 23:32

engechas previously approved these changes Apr 10, 2024

View reviewed changes

ensure name is at lest 1 char

5c8888f

Signed-off-by: jowg-amazon <jowg@amazon.com>

jowg-amazon dismissed engechas’s stale review via 5c8888f April 12, 2024 21:03

engechas reviewed Apr 12, 2024

View reviewed changes

change error message

6904220

Signed-off-by: jowg-amazon <jowg@amazon.com>

engechas approved these changes Apr 12, 2024

View reviewed changes

AWSHurneyt approved these changes Apr 12, 2024

View reviewed changes

amsiglan approved these changes Apr 12, 2024

View reviewed changes

jowg-amazon merged commit 9beae87 into opensearch-project:main Apr 12, 2024
9 checks passed

jowg-amazon added backport 2.x backport 2.13 labels Apr 12, 2024

opensearch-trigger-bot bot mentioned this pull request Apr 12, 2024

[Backport 2.x] Change doc level query name validation #637

Merged

opensearch-trigger-bot bot mentioned this pull request Apr 12, 2024

[Backport 2.13] Change doc level query name validation #638

Merged

jowg-amazon added the backport 2.11 label Apr 19, 2024

opensearch-trigger-bot bot mentioned this pull request Apr 19, 2024

[Backport 2.11] Change doc level query name validation #642

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change doc level query name validation #630

Change doc level query name validation #630

jowg-amazon commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

amsiglan Apr 5, 2024

jowg-amazon Apr 5, 2024

amsiglan Apr 10, 2024

jowg-amazon Apr 10, 2024 •

edited

Loading

amsiglan Apr 5, 2024

engechas Apr 5, 2024

engechas Apr 5, 2024

jowg-amazon Apr 5, 2024

jowg-amazon Apr 5, 2024

engechas left a comment

engechas Apr 5, 2024

jowg-amazon Apr 8, 2024

engechas Apr 5, 2024

engechas Apr 12, 2024

jowg-amazon Apr 12, 2024

		@@ -160,15 +161,20 @@ data class DocLevelQuery(
		}

		// TODO: add test for this

Change doc level query name validation #630

Change doc level query name validation #630

Conversation

jowg-amazon commented Apr 5, 2024 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jowg-amazon Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engechas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jowg-amazon commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

jowg-amazon Apr 10, 2024 •

edited

Loading