search

mattermost-community / mattermost-plugin-aws-SNS

Plugin that receives SNS notification from Alerts created by AWS CloudWatch and sent via AWS SNS
Apache License 2.0
21 stars 22 forks source link

Add a slash command to subscribe new AWS SNS topics to Mattermost channels #12

Open jasonblais opened 5 years ago

jasonblais commented 5 years ago

Only users listed under AllowedUserIds should be allowed to execute this command

elyscape commented 5 years ago

This should probably also allow different topics to be sent to different channels.

george-cionca commented 4 years ago

I would be interested in starting to work on this ticket.

Before starting to work on this ticket, I have a few questions:

  1. Is it possible for this plugin to be enabled for multiple channels? This will represent my first contribution to Mattermost and I am not completely accustomed to the business logic, but the README file of this project states:

    Set the channel to send notifications to, specified in the format teamname,channelname. If the specified channel does not exist, the plugin will create the channel for you.

    Taking into consideration the previous explanation, is it required for the new slash command to handle multiple channel names and associated SNS topics? As an example, I'm referring to the next possible format for the command:

    /addSNSTopics (channelName1, [snsTopicName1, snsTopicName2, ...]), (channelName2, [snsTopicName1, snsTopicName2, ...]), etc.

    Or, is it required to have just a list of SNS topics to be added to the channel for which the plugin has been enabled? For example:

    /addSNSTopics snsTopicName1, snsTopicName2, etc.

  2. In the description of this ticket is mentioned that:

    Only users listed under AllowedUserIds should be allowed to execute this command

    Does this refer to point 3 from the README file? Point 3 from the README file states:

    Set authorized users who can accept AWS SNS subscriptions. Must be a comma-separated list of user ID's.

  3. I haven't noticed any tests for this plugin. I would like to know what testing approaches have been used when this plugin was developed?

    As for manual testing, I see that in the second section of the README file there is an explanation on how to configure AWS (CloudWatch and SNS). The free tier period from my AWS account has expired, but I plan on using LocalStack: https://github.com/localstack/localstack which provides the capability of starting AWS services locally as Docker containers. Of course, it will not replicate all the capabilities of AWS but I expect it to be sufficient for basic operations such as creating a CloudWatch alarm for an EC2 instance and creating an SNS subscription.

    Please let me know if previously there have been other approaches used to test the functionalities of this plugin.

For the time being, I think the previous questions should add enough context to allow me to start the work on this ticket. Thanks.

jasonblais commented 4 years ago

cc @mickmister @jfrerich

mickmister commented 4 years ago

@george-cionca

Is it possible for this plugin to be enabled for multiple channels?

Plugins are automatically enabled throughout the server. So they are enabled for all channels.

The way I see this feature is to run the following command in a given channel in which you want to receive notifications. This is most other plugins accomplish this: /sns subscribe (topic1) (topic2)

It might be better to only allow one topic at a time, to have a more fluid error-handling experience. Also we should support topic name and full ARN for the args.

In the description of this ticket is mentioned that:

Only users listed under AllowedUserIds should be allowed to execute this command

Does this refer to point 3 from the README file? Point 3 from the README file states:

Set authorized users who can accept AWS SNS subscriptions. Must be a comma-separated list of user ID's.

Yes, it is a config value set in the system console. It can be accessed through p.getConfiguration().AllowedUserIds.

I would like to know what testing approaches have been used when this plugin was developed?

The package structure would likely need to be refactored, so we can support mocking of interfaces in separate packages. There are examples of this in https://github.com/mattermost/mattermost-plugin-mscalendar .

I plan on using LocalStack: localstack/localstack which provides the capability of starting AWS services locally as Docker containers. Of course, it will not replicate all the capabilities of AWS but I expect it to be sufficient for basic operations such as creating a CloudWatch alarm for an EC2 instance and creating an SNS subscription.

Fantastic! We would certainly welcome a PR with a docker-compose.yml file configured to spin up the necessary container(s) :+1:

Thanks for your great questions and thanks for your interest in contributing @george-cionca!! You're welcome to join our community server and join the channel for this project to work more closely with the team.

jfrerich commented 4 years ago

Thanks for your interest and questions, @george-cionca! Do you have any additional questions or need any help getting started?

george-cionca commented 4 years ago

Thanks @mickmister and @jfrerich.

Sorry for the late reply. I was a bit busy in the past weeks and didn't have the chance to work on this ticket.

I plan to get started in the upcoming days. I will come back with further updates once I start to work on it.

mickmister commented 4 years ago

@george-cionca No problem, thanks for following up!

george-cionca commented 3 years ago

Sorry for the delay. I was a bit busy in the past months and I didn't have the chance to start the work on this ticket.

I started again in the past days and prepared the Docker Compose file for LocalStack.

The subscription to an SNS topic is working fine.

However, I noticed a missing feature within LocalStack.

Initially, I used the following command: https://awscli.amazonaws.com/v2/documentation/api/latest/reference/cloudwatch/set-alarm-state.html to change the state of the CloudWatch alarm to ALARM from the command-line with AWS CLI (i.e. configured to LocalStack's endpoint), but this command wasn't generating any events from CloudWatch, and therefore no events were being forwarded to the SNS topic.

After that, I used the following command: https://awscli.amazonaws.com/v2/documentation/api/latest/reference/events/put-events.html to send a JSON event directly to EventBridge from the command-line with AWS CLI, but the event was not being forwarded to the SNS topic. It's important to note here that before sending the event to EventBridge, I created a rule in EventBridge to use --event-pattern and to match any events which have their source = [aws.cloudwatch].

After sending the event to EventBridge, I noticed a message in LocalStack's logs indicating that the integration between EventBridge and SNS is not currently supported.

I created a new issue based on this observation: https://github.com/localstack/localstack/issues/3308

I think that I will continue by implementing this missing functionality in LocalStack, and after I confirm that the integration between this Mattermost plugin and LocalStack is working as expected, then I can continue with the implementation required for this ticket.

mickmister commented 3 years ago

I think that I will continue by implementing this missing functionality in LocalStack, and after I confirm that the integration between this Mattermost plugin and LocalStack is working as expected, then I can continue with the implementation required for this ticket.

@george-cionca Fine with me :+1:

Though for the purposes of this task, it may be simpler to work with AWS directly. You should just need to communicate with the SNS service, and not CloudWatch or EventBridge. For manually testing your solution, I recommend using the Publish message feature in the SNS topic's page:

image

I have an external integration with this feature implemented here if it is helpful https://github.com/mickmister/mattermost-sns-integration/blob/1c2a3bdfe80f6c14a69b870107fb9486e32c75ea/src/sns/client.js#L65.