-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add AWS SNS receiver #2615
Add AWS SNS receiver #2615
Conversation
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Yes, preferably SIGv4 would move to its own go module in common. |
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
…MS messages correctly Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
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.
Thanks @treid314 for working on this! I left some comments to hopefully help you progressing on it.
notify/sns/sns_test.go
Outdated
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestNotifier_Notify(t *testing.T) { |
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.
How is this test expected to work? We can't really push to SNS endpoint.
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 test has been used for my local testing and will be removed.
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.
Does the AWS SDK allows you to mock a service (SNS in this case)? Can we actually write a unit test on Notify()
?
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.
There's not a built in mocking in the AWS SDK. If I re-work the the way we create the publish input and move it to a function we could check that logic against the publish input we expect to generate. But I'm not seeing a way to test Notify() directly.
…code review comments Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
…ds, use GetTopicAttributes to check fifo Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
…gging, remove api_version Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
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.
Good job! I think we're very close. I left few more comments. Mostly nits.
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
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.
Good job! The logic LGTM. From my perspective the only missing thing is getting the sigv4 config merged into prometheus/common and vendor it here.
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
… extra api call get topic attributes and use '.fifo' strategy instead Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
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.
Looks sane to me overall, though I've never used AWS SNS.
@simonpasquier @w0rm can either of you take a look?
EDIT: meant to comment, not approve
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
did you see my comment about the notify method? can we split it to make it more readable/reviewable? |
This reverts commit 4c2a5f1. Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
…blish input to send. Check we populate a region for all requests. This reverts commit 4c2a5f1. Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
@roidelapluie Thank you for the review! I updated the notify method to make it more readable. Let me know if you have any other comments! |
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.
Thanks! There would be some clean-up to be done on the imports and one comment does not start with a capital letter but it has no consequences on the code, so let's merge this and create a followup PR later.
This is great! You guys rock! Can't wait to try it. |
Hi, @treid314 sorry to bother you, I'm wondering if there is a way to send alerts to sns without ak/sk? |
@just1900 SNS does not support unauthenticated calls against its APIs |
Thanks for your reply, I'm currently running |
I see, so https://docs.aws.amazon.com/emr/latest/EMR-on-EKS-DevelopmentGuide/setting-up-enable-IAM.html would not help? |
I've configured serviceaccount with IAM role. This does work finally :) sns_configs:
- topic_arn: arn:aws:sns:us-east-1:xxx:am-test
# api_url: https://sns.us-east-1.amazonaws.com # do not set this
sigv4:
region: us-east-1
# role_arn: <role> # do not set this The first thing that confuse me is that if setting the alertmanager/notify/sns/sns.go Lines 119 to 130 in dbc8618
Then I get alertmanager/notify/sns/sns.go Lines 102 to 106 in dbc8618
which may cause the AssumeRoleWithWebIdentity not work as documented in aws/aws-sdk-go#3972. IMO, we can optimize the code here like aws/aws-sdk-go#3972 (comment) to gain a better user experience. |
Work for issue: #2559
Work Remaining:
Signed-off-by: Tyler Reid tyler.reid@grafana.com