Update pusher application #4033

danielfinke · 2017-08-04T21:55:15Z

Update apns4erl
Fix bugs
Send push even if registered to fix iOS BG bug
Support multiple APNs configs (allow both dev/prod APNs)

Also see: 2600hz/kazoo-configs-kamailio#17

fenollp · 2017-08-04T22:36:10Z

applications/pusher/src/modules/pm_apple.erl

-    maybe_load_apns(App, ETS, kapps_config:get_binary(?CONFIG_CAT, <<"apple">>, 'undefined', App)).
+    Config = kapps_config:get_json(?CONFIG_CAT, <<"apple">>, kz_json:new(), App),
+    CertBin = kz_json:get_ne_binary_value(<<"certificate">>, Config),
+    Host = kz_json:get_ne_binary_value(<<"host">>, Config, ?DEFAULT_APNS_HOST),


I would rather you call kapps_config:get_ne_binary(?CONFIG_CAT, [<<"apple">>,<<"certificate">>]) for these two parameters.They do the same thing extraction and it's more explicit.

Of course same goes for <<"host">>

fenollp · 2017-08-04T22:39:17Z

applications/pusher/src/modules/pm_apple.erl

+-spec apns_topic(kz_json:object()) -> binary().
+apns_topic(JObj) ->
+    TokenApp = kz_json:get_ne_binary_value(<<"Token-App">>, JObj),
+    re:replace(TokenApp, <<"\.(?:dev|prod)$">>, <<>>, [{'return', 'binary'}]).


You need to escape the backslash for it to have value in the eyes of re, as "\." = ".".

fenollp · 2017-08-04T22:49:20Z

applications/pusher/src/pusher_maintenance.erl

+            Config = kz_json:from_list([{<<"certificate">>, Binary}
+                                       ,{<<"host">>, Host}
+                                       ]),
+            kapps_config:set_node(?CONFIG_CAT, <<"apple">>, Config, AppId),


I would also prefer you setting these using multiple calls to set_node (using a path like [<<"apple">>,<<"host">>] in the second argument.

fenollp · 2017-08-04T22:53:59Z

core/kazoo_endpoint/src/kz_endpoint.erl

@@ -1177,6 +1177,12 @@ build_push_failover(Endpoint, Clid, PushJObj, Call) ->
        ,{<<"Metaflows">>, kz_json:get_value(<<"metaflows">>, Endpoint)}
        ])).

+-spec push_headers(kz_json:object()) -> kz_json:object().
+push_headers(PushJObj) ->
+    kz_json:foldl(fun(K, V, Acc) ->


Sorry I know this is not related but I hate seeing fold called when a map is implemented.
Can you replace this with:

push_headers(PushJObj) -> kz_json:from_list( lists:keymap(fun prefix_header/1, 1, kz_json:to_proplist(PushJObj)). prepend_header(Header) -> <<"X-KAZOO-PUSHER-", Header/binary>>.

fenollp · 2017-08-04T22:56:16Z

Please remove mention of apns' readme from the mkdocs files kazoo-dev.yml and mkdocs.yml (should fix circleci).

fenollp · 2017-08-04T23:00:52Z

make/deps.mk

@@ -46,6 +47,7 @@ ifeq ($(USER),travis)
 endif

 dep_amqp_client_commit = rabbitmq_v3_6_0
+dep_apns = git https://github.com/inaka/apns4erl.git 2.2.0


Have you tried specifying this as an hex.pm package? https://hex.pm/packages/apns4erl
I'm guessing it wouldn't work due to the app having a different name than the package but I haven't tried it.

fenollp · 2017-08-04T23:02:52Z

Looks like inaka's APNS needs OTP >= 19. Can you remove the line about OTP 18 in .travis.yml?
We want to be following OTP release closely enough, so 4.1 should be the last Kazoo release with support for 18.

EDIT: depend on inaka/apns4erl#198 for now maybe?

lazedo

this pr has some issues. i will address this later.
just putting this so it doesn't get merged

danielfinke · 2017-08-08T19:58:53Z

@fenollp @lazedo I've addressed the requested changes. I think this is going to fail CircleCI at the moment. I think the make apis script (specifically kapps_config_usage.erl) does not understand keys as list type for kapps_config:get/set

danielfinke · 2017-08-08T20:54:45Z

@fenollp Looks like you were right, can't specify it as a hex package right now. Everything actually works up to starting the apns4erl application -- because 'mod' isn't in the apns4erl.app, the module to start isn't specified and since it doesn't match the application name, it won't start

jamesaimonetti · 2017-08-08T20:56:53Z

@danielfinke I think you just need to add "pusher.host" and "pusher.certificate" to the descriptions file. At least, locally, make apis succeeds after adding them.

You'll also need to update pusher's .app.src to point to apns4erl, apparently to satisfy relx.

danielfinke · 2017-08-08T20:58:53Z

@jamesaimonetti Yeah, it just doesn't make sense since it should technically be "pusher.apple.host" and "pusher.apple.certificate".
As far as updating pusher's app.src, I did that, it wasn't enough. Everything seems to be fine transparently, but the first thing that should happen by starting apns4erl is an apns_sup:start_link and the apns_sup proc is not around (i.e. whereis(apns_sup) returns undefined)

Edit: to clarify, apns_app.erl is the module to start using the Erlang application framework. It should launch apns_sup. I don't think apns_app.erl even gets run, since its name does not match the application name.

jamesaimonetti · 2017-08-08T21:01:15Z

@danielfinke ah, gotcha. Will look at the keys to see why it's omitting "apple". As for the apns startup, that is strange. The .app file does name it apns but the directory is apns4erl...wonder if we can specify the directory name to apns when fetching...

fenollp · 2017-08-08T21:30:23Z

Yeah weird WRT the kapps_config path not being properly handled. But yes otherwise it's only a matter of adding the description to the json file.

WRT relx failing to build: it probably has to do with the fact that apns is fetched into deps/apns4 instead of deps/apns or the other way arround. If so, look into rel/relx.config.src to do some renaming.
If this is not the case try changing apns into apns4erl in https://github.com/voxter/kazoo/blob/master/applications/pusher/src/pusher.app.src#L17

@lazedo Mind voicing your reserves here?

jamesaimonetti · 2017-08-08T21:56:07Z

@danielfinke I think #4045 addresses the issue with the configs (at least merging it with this PR doesn't break make apis.

danielfinke · 2017-08-08T23:24:45Z

@jamesaimonetti I'll rebase this once #4045 is merged, thanks!
@fenollp Maybe you can give me an idea where to look for the issue with hex vs git for apns4erl. Excluding relx (not doing a make release at this point), a regular make of the project results in this apns4erl.app when using git:

{application,apns,
             [{description,"APNs provider for Apple Push Notification services (APNs) in Erlang. It supports the HTTP2 API."},
              {vsn,"2.2.0"},
              {id,"apns4erl"},
              {pkg_name,apns4erl},
              {registered,[]},
              {applications,[kernel,stdlib,jsx,chatterbox,base64url]},
              {modules, ['apns','apns_app','apns_connection','apns_feedback','apns_os','apns_sup','apns_utils']},
              {mod,{apns_app,[]}},
              {maintainers,["Inaka"]},
              {licenses,["Apache 2.0"]},
              {links,[{"Github","https://github.com/inaka/apns4erl"}]},
              {build_tools,["rebar3"]}]}.

and this apns4erl.app when using hex:

{application, 'apns4erl', [
	{description, ""},
	{vsn, "rolling"},
	{id, ""},
	{modules, ['apns','apns_app','apns_connection','apns_feedback','apns_os','apns_sup','apns_utils']},
	{registered, []},
	{applications, [kernel,stdlib,chatterbox,jsx,base64url]},
	{env, []}
]}.

Aside from the obvious issue of the omitted data, mod being missing means the OTP application module doesn't know what MFA to execute when the apns4erl is told to start.

I'm not familiar with what part of the make process generates these. Thoughts?

fenollp · 2017-08-08T23:46:01Z

Merged the other PR.
WRT mod: let's just go with the git version. Specifying the app with dep_apns should solve the issue.

danielfinke · 2017-08-09T00:04:23Z

@fenollp OK, rebased on master after #4045.
After that revert commit, sticking with git for apns4erl.

@lazedo hey, I want to get your thoughts yet

lazedo

@danielfinke thanks for this pr, awesome work.
the concern here is a device app that gets an INVITE and a PUSH. i think that you may be trying to fix a client app that should behave differently. existing apps may broke because of that double signaling.

one minor issue
the apns_topic should come from the registered App doc. for that you'll probably need to change handle_cast({'push' .. / get_apns to pass the App or apns_topic from App to maybe_send_push_notification

danielfinke · 2017-08-09T19:31:26Z

@lazedo So my understanding is pusher was designed specifically for the Linphone project, given the code lines up with how the UA string in Linphone contains exactly the right fields as pusher is looking for.

I'm trying to handle a situation that is broken in Linphone (iOS) at this time: the app transitioning to the Suspended state (after user presses Home for example). Because you will have a registration that is still valid, the existing pusher implementation will not send you a push and iOS will have already closed the socket that would be receiving the INVITE. Apple finished deprecating support for persistent background TCP sockets in 10.2.1 (I think that's the version). UDP already did not work. We had been previously using persistent background TCP sockets without any push notifications which was fine until Apple removed support.

This will have become broken for old apps compiled with Linphone - we know, it happened to us ;)

So we moved to using Linphone with push, but I made the quick discovery that it wouldn't work after the app was Suspended until my registration expired. My changes fix this problem. The Linphone source code is also smart enough to discard the push if the app is in the foreground and received the INVITE (checks for the call in an active calls array).

For the apns_topic, are you thinking it should be stored in the system_config/pusher doc per application?

lazedo · 2017-08-09T21:04:40Z

@danielfinke thanks for the reply. AFAIR linphone used to unregister before going into background, it needed a patch to do so. it was developed with linphone as an example but it works for others with the principle of having apps unregister when going into background mode. trying to figure out a way to make this work.

for the apnstopic my thought is to put it in the app registration doc.

danielfinke · 2017-08-09T21:08:51Z

@lazedo That was actually my first approach. I couldn't find a public function in liblinphone that would send a deregister (i.e. disabling a proxy config) without storing that in the proxy config and preventing a register the next time the app was opened (push, launch from switcher, launch from home screen). The answer here might be exposing (or adding if necessary) functions to liblinphone to simply send a single deregister/REGISTER with 0 expiry.

danielfinke · 2017-08-09T21:13:19Z

I guess beyond getting this use case solved, could there be a worthwhile benefit to having the redundancy of both push and INVITE?
I am not coming up with something at the moment, but it may be worth thinking about

lazedo · 2017-08-14T14:59:24Z

@danielfinke or have a way to choose between push/sip when registered.

danielfinke · 2017-08-16T02:42:11Z

@lazedo Are you thinking statically configured or dynamically?
I don't think having a static setting helps. If we're trying to avoid significant changes to Linphone (or other apps) just using push isn't going to work. All the signalling is implemented in the SIP stack. Would be a major reinventing of the wheel.
I am not sure how one would dynamically detect if a push or INVITE should be used.

lazedo · 2017-08-16T03:27:20Z

@danielfinke i'm thinking in a config header that is added to endpoint push headers (maybe when it saves the push stuff) that will allow different behavior depending on registered agent (application). we should
definitely have as default push for offline , invite for registered (as it works today). depending on the registered app values, we can send an action as part of x-push headers to kamailio that will act accordingly, but we cannot change the existing behaviour. you can have kamailio sending push or sending the invite, or both for registered user agents.

danielfinke · 2017-08-23T17:18:16Z

@lazedo I've added the "X-KAZOO-PUSHER-Simultaneous-Push-and-Invite" header for this purpose. If it is not set and true, the default behaviour is retained. The code is still shuffled around a little bit to avoid duplication and multiple checks of the header value, but the behaviour is the same as it was originally.

The change was made in the kazoo-configs-kamailio PR.

fenollp · 2017-08-23T19:22:01Z

Let's see what @lazedo thinks about this but if he likes it let's not forget to document this new header in Kazoo.

lazedo

no reflection of X-KAZOO-PUSHER-Simultaneous-Push-and-Invite in this pr

lazedo · 2017-08-23T20:45:19Z

applications/pusher/src/modules/pm_apple.erl

+-spec apns_topic(kz_json:object()) -> binary().
+apns_topic(JObj) ->
+    TokenApp = kz_json:get_ne_binary_value(<<"Token-App">>, JObj),
+    re:replace(TokenApp, <<"\\.(?:dev|prod)$">>, <<>>, [{'return', 'binary'}]).


@danielfinke this still needs to be fixed

@lazedo that is the fix - it was previously <<"\.(?:dev|prod)$">>

ah wait, you mean to pass it via the registered app - will take care of it

@lazedo can you elaborate a bit on what you are thinking I should do here? I think if I do not retain this behaviour some old apps may break. If it needs to be specified in the pusher config that will also break existing deployments. And adding it to the UA of apps will naturally require changes to those apps

@danielfinke the .dev|.prod replacement at the end of the TokenApp is restrictive. the topic can also be part of the app configuration the same way you did in kz_endpoint to get the extra headers

@lazedo sure, I'll add it the same way as the extra headers. I'm going to leave a fallback to the .dev|.prod replacement to preserve old apps' behaviour

lazedo · 2017-08-23T21:38:23Z

@danielfinke global per app, ie, merged at runtime before push. you may also change the location to
kapps_config:get_json(?CONFIG_CAT, 'push-headers', kz_json:new(), AppId),because you know theappid` but not the user-agent string.

danielfinke · 2017-08-23T23:41:27Z

@lazedo I've updated this PR to load some "extra_headers" just before each push. I am also using the TokenType as it will help distinguish between iOS/Android - our AppId is identical for both, so I needed to handle that case. You'll see the relevant code in kz_endpoint.

Example of system_config/pusher for app:

"com.bundle.id": {
    "apple": {
      "certificate": "<cert>",
      "host": "api.push.apple.com",
      "extra_headers": {
        "Invite-Format": "push_and_invite"
      }
    }
  }

I'll probably need to update some API docs - will see what Circle says

I've also updated the kazoo-configs-kamailio PR

lazedo · 2017-08-24T01:14:53Z

applications/pusher/src/modules/pm_apple.erl

+-spec apns_topic(kz_json:object()) -> binary().
+apns_topic(JObj) ->
+    TokenApp = kz_json:get_ne_binary_value(<<"Token-App">>, JObj),
+    re:replace(TokenApp, <<"\\.(?:dev|prod)$">>, <<>>, [{'return', 'binary'}]).


@danielfinke the .dev|.prod replacement at the end of the TokenApp is restrictive. the topic can also be part of the app configuration the same way you did in kz_endpoint to get the extra headers

lazedo · 2017-08-26T10:50:18Z

@danielfinke hi, can you please check the conflict in mkdocs.yml and address the topic comment so it can be merged ? thanks

lazedo · 2017-09-02T01:44:08Z

@danielfinke ping

PISTON-449: WIP update apns4erl PISTON-449: OriginalContact doesn't exist anymore PISTON-449: update to apns4erl 2.2.0 with certdata/keydata binary support PISTON-449: dialyzed PISTON-449: always send push headers (even if device is registered)

…kazoo-dev.yml

This reverts commit a68897b.

lazedo · 2017-09-07T20:27:45Z

applications/pusher/src/modules/pm_apple.erl

+                                   )
+    of
+        'undefined' ->
+            %% Retain the old behaviour


@danielfinke i think this comment is making the formatting break in circle ci. can you move to it to description of apns_topic (above the spec) ? sorry about that.

lazedo · 2017-09-08T09:41:53Z

@danielfinke i added the fix and merged. thanks.

danielfinke · 2017-09-08T16:00:07Z

@lazedo thanks for your work with me on this!
Please make sure 2600hz/kazoo-configs-kamailio#17 is merged too

danielfinke mentioned this pull request Aug 4, 2017

PUSHER_ROLE: Continue processing SIP messaging even if push notification sent 2600hz/kazoo-configs-kamailio#17

Merged

fenollp suggested changes Aug 4, 2017

View reviewed changes

fenollp reviewed Aug 4, 2017

View reviewed changes

lazedo requested changes Aug 5, 2017

View reviewed changes

danielfinke force-pushed the master-pr-pusher-updates branch from f6bbafe to 0741205 Compare August 8, 2017 19:55

fenollp approved these changes Aug 8, 2017

View reviewed changes

danielfinke force-pushed the master-pr-pusher-updates branch from c106c12 to 9b0a432 Compare August 9, 2017 00:02

lazedo requested changes Aug 9, 2017

View reviewed changes

lazedo requested changes Aug 23, 2017

View reviewed changes

danielfinke force-pushed the master-pr-pusher-updates branch from 14d8e13 to d6d32ce Compare August 23, 2017 23:36

lazedo requested changes Aug 24, 2017

View reviewed changes

danielfinke added 15 commits September 5, 2017 17:09

PISTON-449: updated pusher documentation

8cd425e

PISTON-463: support multiple APNs configs (for multiple apps)

a789188

PISTON-463: update pusher_maintenance for new APNs config

2b14803

PISTON-463: final documentation updates

55672d6

PISTON-463: use list for kapps_config keys

fed91b4

PISTON-463: fix apns_topic regular expression

b348362

PISTON-463: make push_headers/1 use kz_json:map/2

3447ff3

PISTON-463: swagger apple host default value

c076349

PISTON-463: specify apns4erl as hex package dependency

8dd1be5

PISTON-463: remove reference to core/apns readme from mkdocs.yml and …

7a2baf2

…kazoo-dev.yml

Revert "PISTON-463: specify apns4erl as hex package dependency"

1ea30a2

This reverts commit a68897b.

PISTON-463: add pusher.apple descriptions

ba2ea1b

PISTON-463: add "extra_headers" to App in pusher system_config

3455510

PISTON-463: allow specifying apns_topic in the pusher app config

730c7f7

danielfinke force-pushed the master-pr-pusher-updates branch from d6d32ce to 730c7f7 Compare September 5, 2017 17:13

lazedo requested changes Sep 7, 2017

View reviewed changes

handle default/old behaviour in a separate function

2fe12de

lazedo approved these changes Sep 8, 2017

View reviewed changes

lazedo merged commit 071f774 into 2600hz:master Sep 8, 2017

danielfinke deleted the master-pr-pusher-updates branch September 8, 2017 16:00

danielfinke mentioned this pull request Sep 8, 2017

PISTON-463: add pusher.apple spec #4155

Closed

Update pusher application #4033

Update pusher application #4033

Conversation

danielfinke commented Aug 4, 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

fenollp commented Aug 4, 2017

Choose a reason for hiding this comment

fenollp commented Aug 4, 2017 • edited Loading

lazedo left a comment

Choose a reason for hiding this comment

danielfinke commented Aug 8, 2017

danielfinke commented Aug 8, 2017

jamesaimonetti commented Aug 8, 2017

danielfinke commented Aug 8, 2017 • edited Loading

jamesaimonetti commented Aug 8, 2017

fenollp commented Aug 8, 2017

jamesaimonetti commented Aug 8, 2017

danielfinke commented Aug 8, 2017

fenollp commented Aug 8, 2017

danielfinke commented Aug 9, 2017

lazedo left a comment

Choose a reason for hiding this comment

danielfinke commented Aug 9, 2017 • edited Loading

lazedo commented Aug 9, 2017

danielfinke commented Aug 9, 2017 • edited Loading

danielfinke commented Aug 9, 2017

lazedo commented Aug 14, 2017

danielfinke commented Aug 16, 2017

lazedo commented Aug 16, 2017

danielfinke commented Aug 23, 2017 • edited Loading

fenollp commented Aug 23, 2017

lazedo left a 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

lazedo commented Aug 23, 2017

danielfinke commented Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

lazedo commented Aug 26, 2017

lazedo commented Sep 2, 2017

Choose a reason for hiding this comment

lazedo commented Sep 8, 2017

danielfinke commented Sep 8, 2017

danielfinke commented Aug 4, 2017 •

edited

Loading

fenollp commented Aug 4, 2017 •

edited

Loading

danielfinke commented Aug 8, 2017 •

edited

Loading

danielfinke commented Aug 9, 2017 •

edited

Loading

danielfinke commented Aug 9, 2017 •

edited

Loading

danielfinke commented Aug 23, 2017 •

edited

Loading

danielfinke commented Aug 23, 2017 •

edited

Loading