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

Adds snapshot management notification implementation #387

Merged
merged 2 commits into from
Jun 24, 2022

Conversation

downsrob
Copy link
Contributor

@downsrob downsrob commented Jun 21, 2022

Signed-off-by: Clay Downs [email protected]

Issue #, if available:

Description of changes:

  • Introduces the model changes for snapshot management notification integration
"sm_policy": {
  ...
  "notification": {
    "channel": {
      "id": "some_channel_id"
    },
    "conditions": {
      "creation": true,
      "deletion": false,
      "failure": false,
      "time_limit_exceeded": false
    }
  }
}

The hooks configure the following:
Creation: when enabled, a notification will be published to the configured channel noting the creation of a snapshot. The message sent will be ""Snapshot <snapshot_name> was created successfully"
Deletion: when enabled, a notification will be published to the configured channel noting the deletion of a snapshot.
"Snapshot(s) <deleted_snapshots> deletion has finished."
Failure: when enabled, a notification will be published to the configured channel noting that a failure occurred while creating or deleting a snapshot.
Either a specific message related to the failure will be sent, or if an uncaught exception is thrown, the message sent will be "There was an exception while executing Snapshot Management policy ${job.policyName}, skipping execution."
Time limit exceeded: when enabled, a notification will be published to the configured channel noting that the configured time limit was exceeded. When this occurs, we simply skip the current creation or deletion, so it isn't explicitly a failure.
"Time limit $timeLimit exceeded during snapshot $workflow step"

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.

@downsrob downsrob requested a review from bowenlan-amzn June 21, 2022 19:56
@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2022

Codecov Report

Merging #387 (d3dd136) into sm-dev (fca8469) will decrease coverage by 0.03%.
The diff coverage is 71.96%.

❗ Current head d3dd136 differs from pull request most recent head 2b65ea9. Consider uploading reports for the commit 2b65ea9 to get more accurate results

@@             Coverage Diff              @@
##             sm-dev     #387      +/-   ##
============================================
- Coverage     75.31%   75.28%   -0.04%     
- Complexity     2345     2444      +99     
============================================
  Files           298      312      +14     
  Lines         13517    14478     +961     
  Branches       2072     2232     +160     
============================================
+ Hits          10181    10900     +719     
- Misses         2190     2353     +163     
- Partials       1146     1225      +79     
Impacted Files Coverage Δ
...pensearch/indexmanagement/IndexManagementRunner.kt 55.55% <ø> (ø)
...dexmanagement/common/model/notification/Channel.kt 48.38% <0.00%> (ø)
...ent/common/model/notification/NotificationUtils.kt 0.00% <0.00%> (ø)
.../indexstatemanagement/action/NotificationAction.kt 68.18% <ø> (ø)
...statemanagement/action/NotificationActionParser.kt 83.33% <ø> (ø)
...nt/indexstatemanagement/model/ErrorNotification.kt 13.15% <ø> (ø)
...ent/indexstatemanagement/util/NotificationUtils.kt 0.00% <0.00%> (ø)
...ment/snapshotmanagement/api/transport/SMActions.kt 100.00% <ø> (ø)
...g/opensearch/indexmanagement/util/SecurityUtils.kt 13.23% <0.00%> (-5.52%) ⬇️
...hotmanagement/api/transport/BaseTransportAction.kt 78.26% <33.33%> (-7.46%) ⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d64e100...2b65ea9. Read the comment docs.

@downsrob downsrob marked this pull request as ready for review June 21, 2022 20:37
@downsrob downsrob requested a review from a team June 21, 2022 20:37
@downsrob downsrob changed the title Adds notification model changes Adds snapshot management notification model changes Jun 21, 2022
@downsrob downsrob changed the title Adds snapshot management notification model changes Adds snapshot management notification implementation Jun 23, 2022
val roles = user.roles.joinToString(",")
val requestedTenant = user.requestedTenant
val userName = user.name
return "$userName|$backendRoles|$roles|$requestedTenant"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming User.toString() wasn't following this format which is why a utility method was added for it here. Should we instead add a formattedString() method option to common-utils's User method so that it can roundtrip between String and parse in a self-contained way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, yes I believe this would be useful to expose for everyone in common-utils

@@ -22,6 +22,7 @@ import java.time.temporal.ChronoUnit

@Suppress("UNCHECKED_CAST")
class RestExplainSnapshotManagementIT : SnapshotManagementRestTestCase() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a general comment but if you haven't already, you can try adding an integration test like this which uses a custom webhook Notification channel which indexes locally to test the Notification integration functionality.

It looks like that test linked above needs to be looked over before it's usable again though so I don't see it as a blocker for approval, more of a nice-to-have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was planning on adding unit and integration tests but wanted to get this in as soon as possible to make it available for pentesters. That test broke when we switched to the notification plugin, I'll try to fix it and will sync up with you if I get blocked.

@@ -51,6 +52,8 @@ abstract class BaseTransportAction<Request : ActionRequest, Response : ActionRes
client.threadPool().threadContext.stashContext().use { threadContext ->
listener.onResponse(executeRequest(request, user, threadContext))
}
} catch (ex: IndexManagementException) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the exception thrown by the security utils, catching and rethrowing it so security exceptions aren't logged as "uncaught exception"

@downsrob downsrob merged commit a46f118 into opensearch-project:sm-dev Jun 24, 2022
@downsrob downsrob deleted the sm-notification branch June 24, 2022 16:51
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 24, 2022
* Adds notification integration for snapshot management

Signed-off-by: Clay Downs <[email protected]>

* Changes based on PR comments

Signed-off-by: Clay Downs <[email protected]>
(cherry picked from commit a46f118)
downsrob added a commit that referenced this pull request Jun 24, 2022
* Adds notification integration for snapshot management

Signed-off-by: Clay Downs <[email protected]>

* Changes based on PR comments

Signed-off-by: Clay Downs <[email protected]>
(cherry picked from commit a46f118)

Co-authored-by: Clay Downs <[email protected]>
ronnaksaxena pushed a commit to ronnaksaxena/index-management that referenced this pull request Jul 25, 2022
…ect#387) (opensearch-project#391)

* Adds notification integration for snapshot management

Signed-off-by: Clay Downs <[email protected]>

* Changes based on PR comments

Signed-off-by: Clay Downs <[email protected]>
(cherry picked from commit a46f118)

Co-authored-by: Clay Downs <[email protected]>
Signed-off-by: Ronnak Saxena <[email protected]>
wuychn pushed a commit to ochprince/index-management that referenced this pull request Mar 16, 2023
…ect#387) (opensearch-project#391)

* Adds notification integration for snapshot management

Signed-off-by: Clay Downs <[email protected]>

* Changes based on PR comments

Signed-off-by: Clay Downs <[email protected]>
(cherry picked from commit a46f118)

Co-authored-by: Clay Downs <[email protected]>
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