-
Notifications
You must be signed in to change notification settings - Fork 80
Support SNS as a destination #348
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #348 +/- ##
============================================
+ Coverage 79.63% 79.98% +0.34%
- Complexity 209 245 +36
============================================
Files 151 156 +5
Lines 5342 5490 +148
Branches 700 721 +21
============================================
+ Hits 4254 4391 +137
+ Misses 717 708 -9
- Partials 371 391 +20
Continue to review full report at Codecov.
|
.../main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/destination/Destination.kt
Outdated
Show resolved
Hide resolved
alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/MonitorRunner.kt
Show resolved
Hide resolved
CUSTOM_WEBHOOK("custom_webhook"), | ||
EMAIL("email"), | ||
TEST_ACTION("test_action"); | ||
|
||
override fun toString(): String { | ||
return value | ||
} | ||
|
||
companion object { | ||
var snsUseIamRole = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little strange to me. I'm not sure if we should maintain state on using IAM role in the DestinationType
enum class.
It looks like this is mostly used for null assertion in the SNS
data class. Perhaps you can remove this variable and instead validate if either roleARN
or access/secret
is set somewhere. It looks like you do something like that in SNSMessage
already, that might be sufficient.
And ideally, we should allow the access/secret
settings to be changed using the reload API and if we support that then you can have scenarios where:
access/secret
settings are originally set- SNS Destinations are created (allowing
roleARN
to be null sincesnsUseIamRole = false
) - The
access/secret
settings are removed, checkingroleARN
on any subsequent SNS Destination but now you have one that's already created with a nullroleARN
and noaccess/secret
settings so it bypassed this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need this in case the customer uses the indexDestination API to create the SNS destination and we need some validation for the role arn.
Also I have updated the code such that snsUseIamRole
can change whenever the settings are reloaded.
There are some problems discovered, will reopen PR once they have been resolved |
|
||
// needed because of problems in ClientConfiguration | ||
// TODO: get these fixed in aws sdk | ||
permission java.lang.RuntimePermission "accessDeclaredMembers"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need accessDeclaredMembers
? Could you please double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is required. The AWS SDK uses jackson databind, which needs accessDeclaredMembers
.
Would love this merge!! |
Issue #, if available:
#91
Description of changes:
Support SNS as a destination
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.