Feature/amazon sns support #1149

rslota · 2017-01-12T12:21:49Z

This PR introduces Amazon SNS support for message and users' presence change notifications.

gadgetci · 2017-01-12T12:24:43Z

apps/ejabberd/src/mod_aws_sns_defaults.erl

+-module(mod_aws_sns_defaults).
+-author("Rafal Slota").
+
+-behavior(mod_aws_sns).


According to Compiler:

Warning: behaviour mod_aws_sns undefined

gadgetci · 2017-01-12T12:45:08Z

apps/ejabberd/src/mod_aws_sns.erl

+-module(mod_aws_sns).
+-author("Rafal Slota").
+
+-behavior(gen_mod).


According to Compiler:

Warning: behaviour gen_mod undefined

michalwski · 2017-01-13T07:59:04Z

apps/ejabberd/src/mod_aws_sns.erl

+%% @doc Returns AWS SNS handle base on configured AWS credentials
+-spec aws_handle(Host :: ejabberd:lserver()) -> erlcloud:sns_handle().
+aws_handle(Host) ->
+    AccessKeyId = [_ | _] = opt(Host, access_key_id),


Why is it checked here if the access_key_id is a list?

gen_mod's API doesn't crash with exception when we ask for the module option that is required. This is to ensure this option is set and to crash when it doesn't.

If you don't mind that check, I'll leave it here while adding a comment to make it clear :)

Do we have to validate it on every push to AWS? Can we move the validation to module's start and crash when the option doesn't validate? This will get feedback to the devops that sth is misconfigured earlier.

Sure, done.

michalwski · 2017-01-13T08:06:36Z

test/ejabberd_tests/tests/mod_aws_sns_SUITE.erl

+    escalus:end_per_suite(Config).
+
+init_per_group(_, Config) ->
+    dynamic_modules:start(<<"localhost">>, mod_aws_sns, ?SNS_OPTS),


The domain in big tests shouldn't be hardcoded. This is configurable and can be read like adhoc_SUITE:58

Thanks, done.

gadgetci · 2017-01-13T11:39:12Z

apps/ejabberd/test/mod_aws_sns_SUITE.erl

+%% Tests
+
+handles_unicode_messages(Config) ->
+    expect_message_entry(<<"message">>, <<"❤☀☆☂☻♞☯☭☢€"/utf8>>),


According to Elvis:

Missing space right "," on line 39

gadgetci · 2017-01-13T13:18:05Z

apps/ejabberd/test/mod_aws_sns_SUITE.erl

+%% Tests
+
+handles_unicode_messages(Config) ->
+    expect_message_entry(<<"message">>, <<"❤☀☆☂☻♞☯☭☢€"/utf8>>),


According to Elvis:

Missing space right "," on line 42

gadgetci · 2017-01-13T15:19:49Z

test/ejabberd_tests/tests/mod_aws_sns_SUITE.erl

+
+-define(assertReceivedMatch(Expect), ?assertReceivedMatch(Expect, 0)).
+
+-define(assertReceivedMatch(Expect, Timeout),


According to Elvis:

Invalid macro name assertReceivedMatch on line 36. Use UPPER_CASE.

And this as well.

gadgetci · 2017-01-13T15:19:50Z

test/ejabberd_tests/tests/mod_aws_sns_SUITE.erl

+    config
+}).
+
+-define(assertReceivedMatch(Expect), ?assertReceivedMatch(Expect, 0)).


According to Elvis:

Invalid macro name assertReceivedMatch on line 34. Use UPPER_CASE.

Can Elvis config be change to accept such macro names in tests only?

michalwski · 2017-01-17T11:06:49Z

apps/ejabberd/src/mod_aws_sns.erl

+    ejabberd_hooks:add(user_available_hook, Host, ?MODULE, user_present, 90),
+    ejabberd_hooks:add(unset_presence_hook, Host, ?MODULE, user_not_present, 90),
+
+    application:ensure_all_started(erlcloud),


Why do we need this here? Can we run erlcloud app while starting the node?

When on start? Our reltool.config does not start applications listed by configure script. Anyway, theres just no need for loading all those apps until this module is started.

Right, I forgot about the configure script.

michalwski · 2017-01-17T11:08:08Z

apps/ejabberd/src/mod_aws_sns.erl

+user_send_packet(From = #jid{lserver = Host}, To, Packet) ->
+    ?DEBUG("SNS Packet handle~n    from ~p ~n    to ~p~n    packet ~p.", [From, To, Packet]),
+
+    case {get_topic(Host, Packet), xml:get_subtag(Packet, <<"body">>)} of


I strongly encourage use of exml_query module instead of xml. Especially in new code.

michalwski · 2017-01-17T11:09:10Z

apps/ejabberd/src/mod_aws_sns.erl

+            FromGUID = user_guid(Host, From),
+            ToGUID = user_guid(Host, To),
+
+            MessageBody = xml:get_tag_cdata(BodyTag),


This one can also be replaced with an equivalent from exml_query.

michalwski · 2017-01-17T11:21:31Z

apps/ejabberd/src/mod_aws_sns.erl

+%% @doc Returns message type
+-spec message_type(Packet :: jlib:xmlel()) -> pm | muc | undefined.
+message_type(Packet) ->
+    case xml:get_tag_attr_s(<<"type">>, Packet) of


Another candidate to replace with exml_query.

michalwski · 2017-01-17T11:25:45Z

test/ejabberd_tests/tests/mod_aws_sns_SUITE.erl

+    config
+}).
+
+-define(assertReceivedMatch(Expect), ?assertReceivedMatch(Expect, 0)).


Can Elvis config be change to accept such macro names in tests only?

michalwski · 2017-01-17T11:26:02Z

test/ejabberd_tests/tests/mod_aws_sns_SUITE.erl

+
+-define(assertReceivedMatch(Expect), ?assertReceivedMatch(Expect, 0)).
+
+-define(assertReceivedMatch(Expect, Timeout),


And this as well.

mentels · 2017-01-17T13:47:56Z

@rslota @kzemek when I'm running "big tests" I'm getting the following error:

[error] <0.12290.0>@ejabberd_hooks:run1:241 {{sns_error,"InvalidClientTokenId","The security token included in the request is invalid."},[{erlcloud_sns,sns_xml_request,3,[{file,"src/erlcloud_sns.erl"},{line,649}]},{erlcloud_sns,publish,6,[{file,"src/erlcloud_sns.erl"},{line,452}]},{mod_aws_sns,user_presence_changed,2,[{file,"src/mod_aws_sns.erl"},{line,155}]},{safely,apply,3,[{file,"src/safely.erl"},{line,19}]},{ejabberd_hooks,run1,3,[{file,"src/ejabberd_hooks.erl"},{line,237}]},{ejabberd_c2s,presence_update,3,[{file,"src/ejabberd_c2s.erl"},{line,1862}]},{ejabberd_c2s,process_outgoing_stanza,2,[{file,"src/ejabberd_c2s.erl"},{line,960}]},{p1_fsm_old,handle_msg,10,[{file,"src/p1_fsm_old.erl"},{line,576}]}]}
    Running hook: {user_available_hook,[{jid,<<"bOb">>,<<"localhost">>,<<"res1">>,<<"bob">>,<<"localhost">>,<<"res1">>}]}
    Callback: mod_aws_sns:user_present

It looks like the credentials are incorrect.

mentels · 2017-01-18T11:33:43Z

As the publishing to Amazon SNS is synchronous, if the ercloud_sns:publish/X fails, then the whole C2S process fails. I suggest using the catch to prevent that and log the error without crashing:

publish(Host, TopicARN, Content, Attributes) ->
    EncodedContent = jiffy:encode(Content),
    case (catch erlcloud_sns:publish(topic,
                                     TopicARN,
                                     unicode:characters_to_list(EncodedContent),
                                     undefined,
                                     maps:to_list(Attributes),
                                     aws_handle(Host))) of
        {'EXIT', Error} ->
            ?ERROR_MSG("Failed publishing to Amazon SNS: ~p", [Error]);
        MessageId ->
            MessageId
    end.

I believe that the end goal will be to make a pool of connections for that. How that sounds for you?

kzemek · 2017-01-18T11:40:14Z

@mentels

As the publishing to Amazon SNS is synchronous, if the ercloud_sns:publish/X fails, then the whole C2S process fails.

Hooks are already ran with catch (in ejabberd_hooks via safely module) and in case of failure they are skipped. I agree that a pool is needed as an end goal because talking with SNS can slow down message processing, though.

mentels · 2017-01-18T11:42:41Z

@kzemek cool. I didn't know that, thanks.

mentels · 2017-01-18T12:18:25Z

@kzemek when I'm running the big tests I'm getting this error:

=== Ended at 2017-01-18 13:15:35
=== Location: [{escalus_new_assert,assert_true,84},
              {escalus_story,'-drop_presences/2-lc$^0/1-0-',144},
              {escalus_story,drop_presences,144},
              {escalus_story,'-start_ready_clients/2-fun-0-',97},
              {lists,foldl,1262},
              {escalus_story,start_ready_clients,90},
              {lists,flatmap,1249},
              {lists,flatmap,1249}]
=== Reason: {assertion_failed,assert,is_presence,
                     {xmlel,<<"message">>,
                         [{<<"from">>,<<"bOb@localhost/res1">>},
                          {<<"to">>,<<"alicE@localhost/res1">>},
                          {<<"xml:lang">>,<<"en">>},
                          {<<"type">>,<<"chat">>}],
                         [{xmlel,<<"body">>,[],
                              [{xmlcdata,<<"How are you?">>}]},
                          {xmlel,<<"delay">>,
                              [{<<"xmlns">>,<<"urn:xmpp:delay">>},
                               {<<"from">>,<<"localhost">>},
                               {<<"stamp">>,
                                <<"2017-01-18T12:15:32.976994Z">>}],
                              [{xmlcdata,<<"Offline Storage">>}]}]},
                     "<message from='bOb@localhost/res1' to='alicE@localhost/res1' xml:lang='en' type='chat'><body>How are you?</body><delay xmlns='urn:xmpp:delay' from='localhost' stamp='2017-01-18T12:15:32.976994Z'>Offline Storage</delay></message>"}
  in function  escalus_new_assert:assert_true/2 (src/escalus_new_assert.erl, line 84)
  in call from escalus_story:'-drop_presences/2-lc$^0/1-0-'/1 (src/escalus_story.erl, line 144)
  in call from escalus_story:drop_presences/2 (src/escalus_story.erl, line 144)
  in call from escalus_story:'-start_ready_clients/2-fun-0-'/3 (src/escalus_story.erl, line 97)
  in call from lists:foldl/3 (lists.erl, line 1262)
  in call from escalus_story:start_ready_clients/2 (src/escalus_story.erl, line 90)
  in call from lists:flatmap/2 (lists.erl, line 1249)
  in call from lists:flatmap/2 (lists.erl, line 1249)

I'll look into it after lunch but maybe you have a clue straight away.

mentels · 2017-01-18T12:18:42Z

And the config was:

=== Test case: mod_aws_sns_SUITE:muc_messages/1 (click for source code)

=== Config value:

    [{sns_config,[{access_key_id,"AKIAIH54ALYGMZTESTID"},
                  {secret_access_key,"buRqHOxXCFUQkiuYgdUAy+XoGKt0Ec6DTESTKEY+"},
                  {region,"eu-west-1"},
                  {account_id,"123456789012"},
                  {sns_host,"sns.eu-west-1.amazonaws.com"},
                  {plugin_module,mod_aws_sns_defaults},
                  {presence_updates_topic,"user_presence_updated-dev-1"},
                  {pm_messages_topic,"user_message_sent-dev-1"},
                  {muc_messages_topic,"user_messagegroup_sent-dev-1"},
                  {muc_host,"muc.@HOST@"}]},
     {watchdog,<0.1945.0>},
     {preset,undefined},
     {tc_logfile,"/Users/szymonmentel/dev/faceit/sns/faceit_mim/test/ejabberd_tests/ct_report/ct_run.test@szm-mac.2017-01-18_13.15.24/ejabberd_tests.tests.mod_aws_sns_SUITE.logs/run.2017-01-18_13.15.26/mod_aws_sns_suite.muc_messages.html"},
     {tc_group_properties,[{name,message_publish}]},
     {tc_group_path,[]},
     {data_dir,"/Users/szymonmentel/dev/faceit/sns/faceit_mim/test/ejabberd_tests/tests/mod_aws_sns_SUITE_data/"},
     {priv_dir,"/Users/szymonmentel/dev/faceit/sns/faceit_mim/test/ejabberd_tests/ct_report/ct_run.test@szm-mac.2017-01-18_13.15.24/ejabberd_tests.tests.mod_aws_sns_SUITE.logs/run.2017-01-18_13.15.26/log_private/"},
     {escalus_users,[{bob,[{username,<<"bOb">>},
                           {server,<<"localhost">>},
                           {password,<<"makrolika">>}]},
                     {alice,[{username,<<"alicE">>},
                             {server,<<"localhost">>},
                             {password,<<"matygrysa">>}]}]}]

gadgetci · 2017-02-14T13:02:43Z

apps/ejabberd/test/mod_aws_sns_SUITE.erl

+%% Tests
+
+handles_unicode_messages(Config) ->
+    expect_message_entry(<<"message">>, <<"❤☀☆☂☻♞☯☭☢€"/utf8>>),


According to Elvis:

Missing space right "," on line 42

gadgetci · 2017-02-14T15:14:17Z

apps/ejabberd/test/mod_aws_sns_SUITE.erl

+%% Tests
+
+handles_unicode_messages(Config) ->
+    expect_message_entry(<<"message">>, <<"❤☀☆☂☻♞☯☭☢€"/utf8>>),


According to Elvis:

Missing space right "," on line 42

michalwski · 2017-02-16T14:54:06Z

All right, I'm merging this.
@bartekgorny your general remark is valid, thanks for that. The implementation most likely end up in such a shape when there is a need for that. Currently this is the only place where MongooseIM talks to SNS.

rslota added to-discuss WIP 🚧 labels Jan 12, 2017

gadgetci reviewed Jan 12, 2017

View reviewed changes

rslota force-pushed the feature/amazon_sns_support branch from f5fc1f7 to ca6a208 Compare January 12, 2017 12:42

gadgetci reviewed Jan 12, 2017

View reviewed changes

rslota force-pushed the feature/amazon_sns_support branch 2 times, most recently from de7db75 to 4beb9da Compare January 12, 2017 13:36

michalwski suggested changes Jan 13, 2017

View reviewed changes

rslota force-pushed the feature/amazon_sns_support branch from cf11ee1 to e5b28a4 Compare January 13, 2017 08:34

michalwski approved these changes Jan 13, 2017

View reviewed changes

gadgetci reviewed Jan 13, 2017

View reviewed changes

rslota force-pushed the feature/amazon_sns_support branch 3 times, most recently from 5dfb4e8 to f2def99 Compare January 17, 2017 11:21

michalwski reviewed Jan 17, 2017

View reviewed changes

rslota force-pushed the feature/amazon_sns_support branch from ba74a4c to c71ddb2 Compare January 17, 2017 16:12

rslota force-pushed the feature/amazon_sns_support branch 3 times, most recently from 86407ad to ce7fc5c Compare January 20, 2017 13:54

rslota added ready and removed to-discuss labels Jan 24, 2017

kzemek closed this Feb 6, 2017

kzemek reopened this Feb 6, 2017

rslota and others added 16 commits February 14, 2017 12:50

Add AWS SNS draft implementation

3d87b5b

Code and doc improvements for AWS SNS module

80cae36

Add mod_aws_sns.md documentation

2f4f288

Add empty test module for mod_aws_sns

4c49962

Check required SNS options when module starts

22617ba

Add small tests for mod_aws_sns.

1b4e7fb

Extend topic ARN test for mod_aws_sns.

4e2118e

Add tests for custom callback module of mod_aws_sns.

468b8b2

Add integration test for AWS SNS module

43d8a05

Change mod_aws_sns MUC host option name to muc_host, fix docs.

93060d6

Skip Amazon SNS test case if erlcloud is not available

9ffde34

Use exml_query:* instead of xml:*

e54ec3b

Add worker pool to amazon SNS module

01acda1

Fix small tests after introducint worker_pool to SNS.

42defdb

Fix dialyzer.

6a2ed25

Move mod_aws_sns tests to a proper directory.

6d87c57

rslota force-pushed the feature/amazon_sns_support branch from 18046e5 to 31706ca Compare February 14, 2017 13:02

gadgetci reviewed Feb 14, 2017

View reviewed changes

Fix big tests

31706ca

gadgetci reviewed Feb 14, 2017

View reviewed changes

Fix small tests

ca308c6

michalwski merged commit 04c4e4d into master Feb 16, 2017

michalwski deleted the feature/amazon_sns_support branch February 16, 2017 14:54

michalwski restored the feature/amazon_sns_support branch February 17, 2017 07:24

michalwski mentioned this pull request Feb 17, 2017

Feature/amazon sns support #1187

Merged

michalwski deleted the feature/amazon_sns_support branch February 17, 2017 14:07

michalwski mentioned this pull request Mar 28, 2017

MongooseIM 2.1.0beta1 #1244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/amazon sns support #1149

Feature/amazon sns support #1149

rslota commented Jan 12, 2017 •

edited by kzemek

Loading

gadgetci Jan 12, 2017

gadgetci Jan 12, 2017

michalwski Jan 13, 2017

rslota Jan 13, 2017

rslota Jan 13, 2017

michalwski Jan 13, 2017

rslota Jan 13, 2017

michalwski Jan 13, 2017

rslota Jan 13, 2017

gadgetci Jan 13, 2017

gadgetci Jan 13, 2017

gadgetci Jan 13, 2017

michalwski Jan 17, 2017

gadgetci Jan 13, 2017

michalwski Jan 17, 2017

michalwski Jan 17, 2017

rslota Jan 17, 2017 •

edited

Loading

michalwski Jan 17, 2017

michalwski Jan 17, 2017

michalwski Jan 17, 2017

michalwski Jan 17, 2017

michalwski Jan 17, 2017

michalwski Jan 17, 2017

mentels commented Jan 17, 2017

mentels commented Jan 18, 2017

kzemek commented Jan 18, 2017

mentels commented Jan 18, 2017

mentels commented Jan 18, 2017

mentels commented Jan 18, 2017

gadgetci Feb 14, 2017

gadgetci Feb 14, 2017

michalwski commented Feb 16, 2017


		-define(assertReceivedMatch(Expect), ?assertReceivedMatch(Expect, 0)).

		-define(assertReceivedMatch(Expect, Timeout),

Feature/amazon sns support #1149

Feature/amazon sns support #1149

Conversation

rslota commented Jan 12, 2017 • edited by kzemek Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rslota Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mentels commented Jan 17, 2017

mentels commented Jan 18, 2017

kzemek commented Jan 18, 2017

mentels commented Jan 18, 2017

mentels commented Jan 18, 2017

mentels commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski commented Feb 16, 2017

rslota commented Jan 12, 2017 •

edited by kzemek

Loading

rslota Jan 17, 2017 •

edited

Loading