feat: Add multiple cert handlers for APNs #660

jrconlin · 2016-09-14T23:23:11Z

This patch updates APNs handlers to accept platform based cert
configurations. See configs/autopush_shared.ini.sample. In addition,
this patch clarifies some argument references for routers (e.g. less
than useful result is now slightly more descriptive uaid_data)

Custom item names have been normalized to match gcm/fcm.
Document errors cleaned up a bit as well.

BREAKING CHANGE: the APNS configuration options have been altered, see
configs/autopush_shared.ini.sample for new APNS configuration
settings.

Closes #655

@bbangert @pjenvey r?
@kitcambridge (optional r?)

codecov-io · 2016-09-14T23:34:27Z

Current coverage is 100% (diff: 100%)

Merging #660 into master will not change coverage

@@           master   #660   diff @@
====================================
  Files          45     45          
  Lines        9171   9222    +51   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         9171   9222    +51   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update b1a7e2d...7eed1ff

ghost

Thanks, @jrconlin! This looks really good, but I think _route has some concurrency issues.

ghost · 2016-09-15T05:01:04Z

autopush/router/apnsrouter.py

        self.log.debug("Starting APNS router...")
        self.ap_settings = ap_settings

-    def register(self, uaid, router_data, *args, **kwargs):
-        """Validate that an APNs instance token is in the ``router_data``"""
+    def register(self, uaid, router_data, router_token="firefox",


What do you think about calling this app_id to match the docs, and throwing instead of having a default?

changed router_token to app_id for registration, added app_id as a default arg for register

ghost · 2016-09-15T05:08:12Z

autopush/router/apnsrouter.py

+
+        """
+        router_token = router_data["token"]
+        self._channel = router_data["channel"]


ISTM there's only one instance of APNSRouter per app, so this will clobber self._channel for concurrent requests (if one request comes in, then another one comes in while we're still processing the first one).

moved rel_channel to messages hash.

ghost · 2016-09-15T05:14:12Z

autopush/router/apnsrouter.py

-        # TODO: Add listener for error handling.
-        self.apns.gateway_server.register_response_listener(self._error)
+        self.messages[now] = {"token": router_token, "payload": payload}
+        apns_client.gateway_server.register_response_listener(self._error)


Same here, self.messages and register_response_listener don't seem safe for concurrent use if APNSRouter is a singleton. time.time() is in seconds, so we might also clobber the previous entry if we handle two requests close together.

I believe this one is intentional to track sent messages.

Does the gateway_server need to have the self._error continually registered? The response listeners for each channel can't be regg'd at setup?

bbangert

Kit pointed out some issues that need changes, and I noticed on this read-through existing issues in how outbound messages are tracked and updated.

bbangert · 2016-09-15T15:52:10Z

autopush/router/apnsrouter.py

-        self._connect()
+        self.apns = dict()
+        self._config = router_conf
+        for channel in self._config.keys():


What are APNS channels?

the iOS team have certs assigned to the firefox channel release, so these would map to "firefox", "beta", "gecko" and whatever other channel definition they want to use (e.g. "nightly?")

Could that be better expressed by a better variable name or more documentation around this code?

changing to rel_channel as an abbreviation for "release channel".

bbangert · 2016-09-15T15:53:33Z

autopush/router/apnsrouter.py

+        :type uaid: str
+        :param router_data: Dict containing router specific configuration info
+        :type router_data: dict
+        :param router_token: The channel identifier for cert info lookup


More tokens? This one is weird because you immediately say that 'router_token' is actually a 'channel_identifier'.

Yep, resolving that. There will still be one "router_token" because that's what APNs calls it.

bbangert · 2016-09-15T15:58:10Z

autopush/router/apnsrouter.py

+            alert=router_data.get("title", config.get('default_title',
+                                                      'Mozilla Push')),
+            content_available=1,
+            custom=custom)
        now = int(time.time())


This results only in second granularity, if multiple requests occur at the same second (pretty easy), then their now value will all be the same.

since I'm collapsing key indexes with the expry, made this nanosecond based.

bbangert · 2016-09-15T15:58:36Z

autopush/router/apnsrouter.py

-        # TODO: Add listener for error handling.
-        self.apns.gateway_server.register_response_listener(self._error)
+        self.messages[now] = {"token": router_token, "payload": payload}
+        apns_client.gateway_server.register_response_listener(self._error)


I believe this one is intentional to track sent messages.

bbangert · 2016-09-15T15:59:52Z

autopush/router/apnsrouter.py

-        # TODO: Add listener for error handling.
-        self.apns.gateway_server.register_response_listener(self._error)
+        self.messages[now] = {"token": router_token, "payload": payload}
+        apns_client.gateway_server.register_response_listener(self._error)


Does the gateway_server need to have the self._error continually registered? The response listeners for each channel can't be regg'd at setup?

bbangert · 2016-09-15T16:02:56Z

autopush/router/apnsrouter.py

            self._base_tags)

-        self.apns.gateway_server.send_notification(token, payload, now)
+        apns_client.gateway_server.send_notification(router_token, payload,
+                                                     now)

        # cleanup sent messages
        if self.messages:
            for time_sent in self.messages.keys():


It's possible that multiple threads could be iterating on this at once, which means that one of the del calls will throw a KeyError. Would probably be better to toss the message cleanup function back into the event loop so it could clean-up without worry about locking the object.

bbangert · 2016-09-15T16:04:50Z

autopush/router/apnsrouter.py

-        self.messages[now] = {"token": token, "payload": payload}
-        # TODO: Add listener for error handling.
-        self.apns.gateway_server.register_response_listener(self._error)
+        self.messages[now] = {"token": router_token, "payload": payload}


Is this a router_token, a channel_identifier, or a token? It's not great that its name keeps changing as it moves around to be more and more obscure in meaning.

router_token is now unique and locally defined as the router_data["token"] from above. I'm fine making it more explicit but router_data_token = router_data["token"] feels verbose.

bbangert · 2016-09-15T16:11:51Z

configs/autopush_shared.ini.sample

+;     ... }
+; e.g {"firefox": {"cert": "certs/main.cert", "key": "certs/main.key"}, "beta": {"cert": "certs/beta.cert", "key": "certs/beta.key"}}
+#apns_creds =
+


The connection node needs this? Or is there some other reason its now in shared.ini?

Consistency, mostly. The GCM/FCM definitions are located in shared. In addition, the router args are currently pulled in by main via add_shared_args().

bbangert · 2016-09-15T18:42:35Z

autopush/router/apnsrouter.py

        router_data = uaid_data["router_data"]
        # Kick the entire notification routing off to a thread
-        return deferToThread(self._route, notification, router_data)
+        d = deferToThread(self._route, notification, router_data)
+        d.addCallback(self._cleanup)


Rather than cleaning up after every single message, maybe have the router creation spawn a recurring task that runs every 5 seconds that does this?

ghost · 2016-09-16T18:41:22Z

autopush/router/apnsrouter.py

+            content_available=1,
+            custom=custom)
+        now = time.time()
+        self.messages[now] = {"rel_channel": router_data["rel_channel"],


Is rel_channel the same as app_id, or is it the instance_id from {“token”:{instance_id}}? And is router_token also the instance_id?

ghost · 2016-09-16T18:45:38Z

autopush/router/apnsrouter.py

            self._base_tags)
+
+        apns_client.gateway_server.send_notification(router_token, payload,
+                                                     now)


I think you meant to pass the ID here, too, instead of now?

ghost · 2016-09-16T18:54:13Z

autopush/router/apnsrouter.py

+            if message["time_sent"] < now - expry:
+                del self.messages[msg_id]
+        return response
+
    def _error(self, err):
        """Error handler"""
        if err['status'] == 0:


Nit: self.messages.pop(err['identifier'], None) might be better; del will throw a KeyError if the entry was already removed from the dict.

That shouldn't happen now that cleanup is run off the event loop.

ghost · 2016-09-16T22:18:11Z

autopush/router/apnsrouter.py

+            self.apns[rel_channel] = self._connect(self._config[rel_channel])
+            self.apns[rel_channel].gateway_server.register_response_listener(
+                self._error)
+        self._connect(router_conf)


Is this still needed, now that the config is a dict?

I'd like to keep the connection in a function, even if it adds some slight overhead. There's a bit of messy history about that with the apns lib.

I meant you're calling self._connect() for each entry in router_conf, and then you're calling self._connect(router_conf). ISTM that won't work? If it does, I'd like to understand why. 😄

ah, quite right. context is a good thing for me to have when I reply. I should try it more.

ah-HA! turns out this was a merge artifact, since it just happened again after a conflict from master. ._.

ghost · 2016-09-16T22:18:42Z

autopush/router/apnsrouter.py

+        release channel name.
+
+        """
+        # Do I still need to call this in _error?


I guess not. 😄

so, yeah, remember my cryptic comment about calling self._connect() before? ...

ghost · 2016-09-16T22:20:06Z

autopush/router/apnsrouter.py

+        # "unclean" is used by testing to prevent unclean reactor errors.
+        # It also prevents the local cleanup loop from running without
+        # requiring heavy lifting.
+        if "unclean" not in router_conf:


Ugh. Unclean reactor errors make me sad. 😭

same here, thus why I added this to just not call the reactor when we're running tests.

ghost · 2016-09-16T22:22:49Z

Great! r+ with nits fixed.

bbangert · 2016-09-16T22:17:28Z

autopush/router/apnsrouter.py

+        self._config = router_conf
+        if self._config.get('max_messages'):
+            self._max_messages = self._config['max_messages']
+            del(self._config['max_messages'])


del the function vs. del the keyword?

bbangert · 2016-09-16T22:18:58Z

autopush/router/apnsrouter.py

+        self.apns = dict()
+        self.messages = dict()
+        self._config = router_conf
+        if self._config.get('max_messages'):


self._max_messages = self._config.pop('max_messages', 100)

bbangert · 2016-09-16T22:21:14Z

autopush/router/apnsrouter.py

+            del(self._config['max_messages'])
+        else:
+            self._max_messages = 100
+        for rel_channel in self._config.keys():


Default behavior on iteration of a dict is to iterate through its keys. Usually keys() is only needed if you might mutate the dict during iteration such that you need a list first.

bbangert · 2016-09-16T22:28:15Z

autopush/router/apnsrouter.py

+            self.apns[rel_channel] = self._connect(self._config[rel_channel])
+            self.apns[rel_channel].gateway_server.register_response_listener(
+                self._error)
+        self._connect(router_conf)


So, first router_conf is assigned to self._config, then we iterate through keys in self._config (which is now router_conf minux the max_messages key) calling self._connect. But then we also call self._connect with the whole thing again? What's going on here?

git ~~blend~~ merge failure.

bbangert · 2016-09-16T22:31:04Z

autopush/router/apnsrouter.py

+        # It also prevents the local cleanup loop from running without
+        # requiring heavy lifting.
+        if "unclean" not in router_conf:
+            reactor.callLater(10, self._cleanup)


There's a task looping call used in main.py for functions that should be called regularly.

bbangert · 2016-09-16T22:33:01Z

autopush/router/apnsrouter.py

+        :param app_id: The release channel identifier for cert info lookup
+        :type app_id: str
+
+        returns a modified router_data dict to be stored


:returns: ?

pjenvey · 2016-09-16T22:34:49Z

autopush/router/apnsrouter.py

+        self.apns = dict()
+        self.messages = dict()
+        self._config = router_conf
+        if self._config.get('max_messages'):


self._max_messages = self._config.pop('max_messages', 100)

pjenvey · 2016-09-17T00:13:04Z

autopush/router/apnsrouter.py

        self.ap_settings.metrics.increment(
-            "updates.client.bridge.apns.succeed",
+            "updates.client.bridge.apns.%s.attempted" %


FYI send_notification appears to never fail w/ enhanced=True (assuming the input is valid for a payload), which if I'm right makes the new separate metrics unnecessary

pjenvey · 2016-09-17T00:15:16Z

autopush/router/apnsrouter.py

        router_data = uaid_data["router_data"]
        # Kick the entire notification routing off to a thread
-        return deferToThread(self._route, notification, router_data)
+        d = deferToThread(self._route, notification, router_data)


(since I know you're touching this again) could you please restore: return deferToThread..

pjenvey

r+ pending coverage

This patch updates APNs handlers to accept platform based cert configurations. See `configs/autopush_shared.ini.sample`. In addition, this patch clarifies some argument references for routers (e.g. less than useful `result` is now slightly more descriptive `uaid_data`) Custom item names have been normalized to match gcm/fcm. Document errors cleaned up a bit as well. BREAKING CHANGE: the APNS configuration options have been altered, see `configs/autopush_shared.ini.sample` for new APNS configuration settings. Closes #655

bbangert · 2016-09-16T23:55:36Z

autopush/main.py

@@ -566,6 +569,9 @@ def endpoint_main(sysargs=None, use_files=True):
    # Start the table rotation checker/updater
    l = task.LoopingCall(settings.update_rotating_tables)
    l.start(60)
+    if settings.routers.get('apns'):


We don't need to get it, a key check should be fine. "if 'apns' in ...."

bbangert · 2016-09-17T00:01:46Z

autopush/router/apnsrouter.py

+        """
+        router_token = router_data["token"]
+        rel_channel = router_data["rel_channel"]
+        config = self._config[rel_channel]


Sure hope we never, ever, change valid release channel names.

If we do, thumbscrews. Fortunately ops has the ability to generate more certs. They'll just have to change the app_id they're sending in to use the new cert pair.

jrconlin added the in progress label Sep 14, 2016

ghost suggested changes Sep 15, 2016

View reviewed changes

bbangert suggested changes Sep 15, 2016

View reviewed changes

jrconlin force-pushed the feat/655 branch 4 times, most recently from 055e311 to 2aa7a59 Compare September 15, 2016 22:08

jrconlin assigned pjenvey and bbangert Sep 15, 2016

bbangert suggested changes Sep 15, 2016

View reviewed changes

ghost reviewed Sep 16, 2016

View reviewed changes

jrconlin force-pushed the feat/655 branch from 2aa7a59 to 7cd5d6e Compare September 16, 2016 20:36

ghost reviewed Sep 16, 2016

View reviewed changes

ghost approved these changes Sep 16, 2016

View reviewed changes

bbangert suggested changes Sep 16, 2016

View reviewed changes

jrconlin force-pushed the feat/655 branch 2 times, most recently from f5d79b6 to 167bde7 Compare September 16, 2016 23:36

pjenvey reviewed Sep 16, 2016

View reviewed changes

pjenvey reviewed Sep 17, 2016

View reviewed changes

pjenvey approved these changes Sep 17, 2016

View reviewed changes

bbangert approved these changes Sep 17, 2016

View reviewed changes

jrconlin force-pushed the feat/655 branch from 167bde7 to 7eed1ff Compare September 17, 2016 00:21

jrconlin merged commit 70d0319 into master Sep 17, 2016

jrconlin removed the in progress label Sep 17, 2016

bbangert deleted the feat/655 branch September 17, 2016 04:08

bbangert restored the feat/655 branch September 17, 2016 04:08

bbangert deleted the feat/655 branch September 17, 2016 04:08

feat: Add multiple cert handlers for APNs #660

feat: Add multiple cert handlers for APNs #660

Conversation

jrconlin commented Sep 14, 2016

codecov-io commented Sep 14, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 15, 2016 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 15, 2016 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbangert 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

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

bbangert Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

ghost Sep 16, 2016 • edited by ghost 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

ghost commented Sep 16, 2016

bbangert Sep 16, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey 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

codecov-io commented Sep 14, 2016 •

edited

Loading

ghost Sep 15, 2016 •

edited by ghost

Loading

ghost Sep 15, 2016 •

edited by ghost

Loading

bbangert Sep 16, 2016 •

edited

Loading

ghost Sep 16, 2016 •

edited by ghost

Loading

bbangert Sep 16, 2016 •

edited

Loading