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 a slash command to subscribe new AWS SNS topics to Mattermost channels #12

Open
jasonblais opened this issue Jun 1, 2019 · 9 comments
Assignees
Labels
Difficulty/2:Medium Medium ticket Hacktoberfest Help Wanted Community help wanted Tech/Go Type/Enhancement New feature or improvement of existing feature

Comments

@jasonblais
Copy link
Contributor

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

@jasonblais jasonblais added enhancement Help Wanted Community help wanted labels Jun 1, 2019
@hanzei hanzei added the Up For Grabs Ready for help from the community. Removed when someone volunteers label Jun 21, 2019
@elyscape
Copy link

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

@hanzei hanzei added Type/Enhancement New feature or improvement of existing feature and removed Enhancement labels Mar 7, 2020
@george-cionca
Copy link

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

cc @mickmister @jfrerich

@mickmister
Copy link
Contributor

@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) 👍


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
Copy link
Contributor

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

@george-cionca
Copy link

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
Copy link
Contributor

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

@mickmister mickmister removed the Up For Grabs Ready for help from the community. Removed when someone volunteers label May 23, 2020
@george-cionca
Copy link

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: localstack/localstack#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
Copy link
Contributor

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 👍

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty/2:Medium Medium ticket Hacktoberfest Help Wanted Community help wanted Tech/Go Type/Enhancement New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

9 participants