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

Added input validation, and fixed bug for cross cluster monitors. #1510

Merged
merged 15 commits into from
Apr 13, 2024

Conversation

AWSHurneyt
Copy link
Collaborator

@AWSHurneyt AWSHurneyt commented Apr 11, 2024

Issue #, if available:

Description of changes:

  1. Added input validation for GetRemoteIndexes API, and added related unit and integration tests.
  2. Fixed issue where InputService wouldn't wait for cluster metrics monitor to finish executing against all clusters.

Related PRs

opensearch-project/common-utils#633
opensearch-project/alerting-dashboards-plugin#933

CheckList:

  • 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.

…tor to finish executing against all clusters.

Signed-off-by: AWSHurneyt <[email protected]>
…imestamp of incoming request.

Signed-off-by: AWSHurneyt <[email protected]>
Signed-off-by: AWSHurneyt <[email protected]>
@AWSHurneyt
Copy link
Collaborator Author

Security tests are failing for unrelated reasons.
E.g., https://github.com/opensearch-project/alerting/actions/runs/8549717854/job/23425483546?pr=1503

@AWSHurneyt
Copy link
Collaborator Author

AWSHurneyt commented Apr 12, 2024

The regexes used in this PR have been moved to common utils in PR opensearch-project/common-utils#633.

The tests in this PR will continue to fail until that PR is merged; however, the tests passed before the regexes were moved.
https://github.com/opensearch-project/alerting/pull/1510/checks?check_run_id=23722927270

As mentioned above, the security tests are failing due to a pre-existing problem, and will be fixed in a separate PR.

Signed-off-by: AWSHurneyt <[email protected]>
Signed-off-by: AWSHurneyt <[email protected]>
@AWSHurneyt AWSHurneyt merged commit 71d0364 into opensearch-project:main Apr 13, 2024
15 of 18 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.x
# Create a new branch
git switch --create backport-1510-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 71d03648b8d39f15f64922cb3b3ff9e18d16ed35
# Push it to GitHub
git push --set-upstream origin backport-1510-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport-1510-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.12 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.12 2.12
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.12
# Create a new branch
git switch --create backport-1510-to-2.12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 71d03648b8d39f15f64922cb3b3ff9e18d16ed35
# Push it to GitHub
git push --set-upstream origin backport-1510-to-2.12
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.12

Then, create a pull request where the base branch is 2.12 and the compare/head branch is backport-1510-to-2.12.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.13 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.13 2.13
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.13
# Create a new branch
git switch --create backport-1510-to-2.13
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 71d03648b8d39f15f64922cb3b3ff9e18d16ed35
# Push it to GitHub
git push --set-upstream origin backport-1510-to-2.13
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.13

Then, create a pull request where the base branch is 2.13 and the compare/head branch is backport-1510-to-2.13.

AWSHurneyt added a commit to AWSHurneyt/OpenSearch-Alerting that referenced this pull request Apr 13, 2024
…ensearch-project#1510)

* Fixed issue where InputService wouldn't wait for cluster metrics monitor to finish executing against all clusters.

Signed-off-by: AWSHurneyt <[email protected]>

* Added input validation for GetRemoteIndexes API, and added related unit and integration tests.

Signed-off-by: AWSHurneyt <[email protected]>

* Removed unused variable, and imports.

Signed-off-by: AWSHurneyt <[email protected]>

* Made initial call to GetRemoteIndexes API log INFO level to capture timestamp of incoming request.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed comment.

Signed-off-by: AWSHurneyt <[email protected]>

* Moved some regex to common utils.

Signed-off-by: AWSHurneyt <[email protected]>

* Renamed cross cluster monitor setting.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed ktlint error.

Signed-off-by: AWSHurneyt <[email protected]>

* Added null checks for health statuses.

Signed-off-by: AWSHurneyt <[email protected]>

* Wrapped Monitor.parse calls in AlertingExceptions so IllegalArgumentExceptions are wrapped in 4xx-level exceptions.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed merge error.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>
AWSHurneyt added a commit that referenced this pull request Apr 13, 2024
…r monitors. (#1510) (#1515)

* Added input validation, and fixed bug for cross cluster monitors. (#1510)

* Fixed issue where InputService wouldn't wait for cluster metrics monitor to finish executing against all clusters.

Signed-off-by: AWSHurneyt <[email protected]>

* Added input validation for GetRemoteIndexes API, and added related unit and integration tests.

Signed-off-by: AWSHurneyt <[email protected]>

* Removed unused variable, and imports.

Signed-off-by: AWSHurneyt <[email protected]>

* Made initial call to GetRemoteIndexes API log INFO level to capture timestamp of incoming request.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed comment.

Signed-off-by: AWSHurneyt <[email protected]>

* Moved some regex to common utils.

Signed-off-by: AWSHurneyt <[email protected]>

* Renamed cross cluster monitor setting.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed ktlint error.

Signed-off-by: AWSHurneyt <[email protected]>

* Added null checks for health statuses.

Signed-off-by: AWSHurneyt <[email protected]>

* Wrapped Monitor.parse calls in AlertingExceptions so IllegalArgumentExceptions are wrapped in 4xx-level exceptions.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed merge error.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 13, 2024
…r monitors. (#1510) (#1515)

* Added input validation, and fixed bug for cross cluster monitors. (#1510)

* Fixed issue where InputService wouldn't wait for cluster metrics monitor to finish executing against all clusters.

Signed-off-by: AWSHurneyt <[email protected]>

* Added input validation for GetRemoteIndexes API, and added related unit and integration tests.

Signed-off-by: AWSHurneyt <[email protected]>

* Removed unused variable, and imports.

Signed-off-by: AWSHurneyt <[email protected]>

* Made initial call to GetRemoteIndexes API log INFO level to capture timestamp of incoming request.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed comment.

Signed-off-by: AWSHurneyt <[email protected]>

* Moved some regex to common utils.

Signed-off-by: AWSHurneyt <[email protected]>

* Renamed cross cluster monitor setting.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed import.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed ktlint error.

Signed-off-by: AWSHurneyt <[email protected]>

* Added null checks for health statuses.

Signed-off-by: AWSHurneyt <[email protected]>

* Wrapped Monitor.parse calls in AlertingExceptions so IllegalArgumentExceptions are wrapped in 4xx-level exceptions.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed merge error.

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>

* Fixed test.

Signed-off-by: AWSHurneyt <[email protected]>

---------

Signed-off-by: AWSHurneyt <[email protected]>
(cherry picked from commit 3f60cf1)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AWSHurneyt pushed a commit that referenced this pull request Apr 13, 2024
…r monitors. (#1510) (#1515) (#1516)

* Added input validation, and fixed bug for cross cluster monitors. (#1510)

* Fixed issue where InputService wouldn't wait for cluster metrics monitor to finish executing against all clusters.



* Added input validation for GetRemoteIndexes API, and added related unit and integration tests.



* Removed unused variable, and imports.



* Made initial call to GetRemoteIndexes API log INFO level to capture timestamp of incoming request.



* Fixed comment.



* Moved some regex to common utils.



* Renamed cross cluster monitor setting.



* Fixed import.



* Fixed import.



* Fixed ktlint error.



* Added null checks for health statuses.



* Wrapped Monitor.parse calls in AlertingExceptions so IllegalArgumentExceptions are wrapped in 4xx-level exceptions.



* Fixed merge error.



* Fixed test.



---------



* Fixed test.



---------


(cherry picked from commit 3f60cf1)

Signed-off-by: AWSHurneyt <[email protected]>
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>
@AWSHurneyt AWSHurneyt deleted the 3.0-cross-cluster-dev branch April 24, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants