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

Add AWS SNS receiver #2615

Merged
merged 35 commits into from Jul 26, 2021
Merged

Add AWS SNS receiver #2615

merged 35 commits into from Jul 26, 2021

Conversation

treid314
Copy link
Contributor

@treid314 treid314 commented Jun 10, 2021

Work for issue: #2559

Work Remaining:

  • Support AWS ARN auth
  • Get AWS keys from environment variables
  • Properly truncate SNS messages
  • Add email templates
  • Document new config options for SNS

Signed-off-by: Tyler Reid tyler.reid@grafana.com

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
@roidelapluie
Copy link
Member

Yes, preferably SIGv4 would move to its own go module in common.

Tyler Reid added 3 commits June 11, 2021 10:30
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>
Copy link
Contributor

@pracucci pracucci left a 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.

config/notifiers.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
config/testdata/conf.sns-test.yml Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Show resolved Hide resolved
"github.com/stretchr/testify/require"
)

func TestNotifier_Notify(t *testing.T) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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()?

Copy link
Contributor Author

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.

Tyler Reid added 2 commits June 14, 2021 18:28
…code review comments

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
config/notifiers.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Show resolved Hide resolved
Tyler Reid added 6 commits June 15, 2021 09:09
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>
notify/sns/sns.go Outdated Show resolved Hide resolved
pracucci and others added 4 commits June 16, 2021 15:55
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>
config/notifiers.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
Tyler Reid added 3 commits June 16, 2021 14:27
…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>
Copy link
Contributor

@pracucci pracucci left a 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.

docs/configuration.md Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
notify/sns/sns.go Outdated Show resolved Hide resolved
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Copy link
Contributor

@pracucci pracucci left a 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.

docs/configuration.md Outdated Show resolved Hide resolved
Tyler Reid added 2 commits June 17, 2021 10:45
Tyler Reid added 3 commits June 22, 2021 14:43
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>
Tyler Reid added 2 commits June 27, 2021 20:26
… 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>
Copy link
Member

@cstyan cstyan left a 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

notify/sns/sns.go Show resolved Hide resolved
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Show resolved Hide resolved
config/config_test.go Show resolved Hide resolved
notify/sns/sns.go Show resolved Hide resolved
Tyler Reid added 2 commits July 6, 2021 18:17
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
Signed-off-by: Tyler Reid <tyler.reid@grafana.com>
@roidelapluie
Copy link
Member

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>
@treid314
Copy link
Contributor Author

@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!

Copy link
Member

@roidelapluie roidelapluie left a 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.

@kevinayres
Copy link

This is great! You guys rock! Can't wait to try it.

@just1900
Copy link

just1900 commented Sep 6, 2022

Hi, @treid314 sorry to bother you, I'm wondering if there is a way to send alerts to sns without ak/sk?

@jeromeinsf
Copy link

@just1900 SNS does not support unauthenticated calls against its APIs

@just1900
Copy link

just1900 commented Sep 6, 2022

@just1900 SNS does not support unauthenticated calls against its APIs

Thanks for your reply, I'm currently running alertmanager under eks, I mean using the identity token instead of ak/sk directly.

@jeromeinsf
Copy link

@just1900
Copy link

just1900 commented Sep 6, 2022

I've configured serviceaccount with IAM role. This does work finally :)
I dive a little bit into the implementation of sns notifier and figure out how to send sns request with identity token. The final config would be like

  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 role_arn, aws sdk will try to assume the role of role_arn with a AssumeRoleWithWebIdentity role, and return an unauthorized error. I'm not sure how to bypass this.

stsSess, err = session.NewSessionWithOptions(session.Options{
Config: aws.Config{
Region: aws.String(n.conf.Sigv4.Region),
Credentials: creds,
},
Profile: n.conf.Sigv4.Profile,
})
if err != nil {
return nil, err
}
}
creds = stscreds.NewCredentials(stsSess, n.conf.Sigv4.RoleARN)

Then I get role_arn commented, however, alertmanager will use the api_url as an endpoint when retrieving sts credentials.

sess, err := session.NewSessionWithOptions(session.Options{
Config: aws.Config{
Region: aws.String(n.conf.Sigv4.Region),
Endpoint: aws.String(tmpl(n.conf.APIUrl)),
},

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants