960009 - Implement latest tokenserver protocol changes

Status: VERIFIED FIXED

(Cloud Services Graveyard :: Server: Token, defect)

Description

[Ryan Kelly [:rfkelly]]

Unfortunately this is going to get a bit messy, there's a lot of shuffling to be done...

We've got four outstanding tokenserver protocol changes for fxa+sync:

  Bug 958462: add support for BrowserID generation numbers
  Bug 959441: add x-client-state header and supporting logic
  Bug 959476: remove x-conditions-accepted header
  Bug 957503: change "Browser-ID" to "BrowserID" for consistency

Together they add up to quite a change to the node-assignment backend and its interface, so I've made the changes in one fell swoop.  Patches for tokenserver and WIMMS coming in comments.

Comment 1

[Ryan Kelly [:rfkelly]]

Oh, and the addition of X-Backoff, X-Timestamp etc headers as well.

Comment 2

[Ryan Kelly [:rfkelly]]

Attachment: tokenserver-implementation.diff

Attaching part 1, tokenserver patch.  I've also put it up as a PR on github which might be nicer for review: https://github.com/mozilla-services/tokenserver/pull/29

The main change here is re-working the INodeAssignment interface to support generation numbers and client-state strings, including allowing us to update them as we see new ones from the client.  It also has to remember the set of old client-state strings so that we can give appropriate error messages.

Comment 3

[Ryan Kelly [:rfkelly]]

Attachment: wimms-implementation.diff

Attaching part 2, WIMMS code patch.  This is also available in github PR form: https://github.com/mozilla-services/wimms/pull/3

This changes the SQLMetadata and ShardedSQLMetadata classes to match the new INodeAssignment interface.  The thorny details are in the changed DB schema:

  * The "user_nodes" table now tracks additional data about the user, hence renamed to just "users".  It contains one row per client-state string seen for that user, each receiving a fresh uid via the magic of auto-increment.

  * User rows are tagged with a creation timestamp, and when we make a new row to update the client-state string, we tag all the existing rows with a deletion timestamp.  This will help us to page through them if/when we want to do any cleanup.

  * To get the current data for a user, we slurp in all their user table rows in descending creation-timestamp order.  The first such row is their current data, and subsequent rows tell us the previously-seen client-state strings.  We collect those into a set to return to the applicaiton code.

  * The generation number can be updated in-place in the user's current record.  We take some care not to move it backwards.

  * I've explicitly given each service an integer id, since they appear in several indexes.  We don't want the string "sync-1.5" repeated over and over again in the index.

Comment 4

[Ryan Kelly [:rfkelly]]

Also cc'ing Tarek and Alexis to (1) appologize for this sudden brutalization of your code in hurried pursuit of the FF29 deadline, and (2) ask if you have any thoughts on the schema described above.

Comment 5

[Toby Elliott [:telliott]]

Comment on attachment 8360334 [details] [diff] [review]
tokenserver-implementation.diff

Review of attachment 8360334 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Couple minor nits, but nothing to derail the +

::: tokenserver/assignment/__init__.py
@@ +6,5 @@
>  
> +    def get_user(self, service, email):
> +        """Returns the user record for the given service and email.
> +
> +        The returned object will be None if no record exists for that email,

"If no service record exists"

::: tokenserver/assignment/sqlnode.py
@@ +66,5 @@
>              self.logger = get_logger()
>          return self.logger
>  
> +    def _proxy_request(self, method, path, data=None, headers=None):
> +        url = self._dnslookup(self.proxy_uri) + "/1.0/" + path

version number should be variable-ized for clarity

::: tokenserver/tweens.py
@@ +6,5 @@
> +
> +from pyramid.httpexceptions import HTTPException, HTTPServiceUnavailable
> +
> +
> +def set_x_timestamp_header(handler, registry):

I'm surprised we don't just have this in server core, since we're moving towards using it everywhere.

Comment 6

[Ryan Kelly [:rfkelly]]

> > +    def _proxy_request(self, method, path, data=None, headers=None):
> > +        url = self._dnslookup(self.proxy_uri) + "/1.0/" + path
> version number should be variable-ized for clarity

OK.  FYI I don't think we have the matching server part of this yet.  Is this something we need to prioritize for FF29?

It would be cool to just have it talking to another, more secure instance of the tokenserver, but the APIs don't quite match up.

Comment 7

[Toby Elliott [:telliott]]

Comment on attachment 8360336 [details] [diff] [review]
wimms-implementation.diff

Review of attachment 8360336 [details] [diff] [review]:
-----------------------------------------------------------------

I'm r+ing, because this is fine, but please check the one potentially weird behavior below.

::: wimms/schemas.py
@@ +37,1 @@
>      """This table lists all the users associated to a service.

this isn't quite accurate. I think it should be "This table associates email addresses with services." UID is the PK

::: wimms/sql.py
@@ +6,3 @@
>  
> +For each available service, we maintain a list of user accounts and their
> +associated uid, node-assignemtn and metadata.  We also have a list of nodes

typo

@@ +200,5 @@
> +                    'service': service,
> +                    'email': user['email'],
> +                    'generation': generation
> +                }
> +                res = self._safe_execute(_UPDATE_GENERATION_NUMBER, **params)

If the client state is in an old client state, does this mean we're reverting? That seems like a really weird possibility. I think it's a non-damaging update, but I worry that if the situation occurs something more problematic has already happened and we should be very careful here.

One other consequence - we'll have a user for which the highest generation and most recent timestamp don't match. Hmm. It also means we can have a record with the highest generation that also has a replaced_at value set.

Comment 8

[Toby Elliott [:telliott]]

(In reply to Ryan Kelly [:rfkelly] from comment #6)

> OK.  FYI I don't think we have the matching server part of this yet.  Is
> this something we need to prioritize for FF29?

We used to have the s-servers because they had write access to LDAP. With that gone, I don't think this is a priority. That's FxA's problem ;)

Comment 9

[Ryan Kelly [:rfkelly]]

> If the client state is in an old client state, does this mean we're reverting?
> That seems like a really weird possibility. I think it's a non-damaging update,
> but I worry that if the situation occurs something more problematic has already
> happened and we should be very careful here.

Per IRC discussion, I'll  change this so that any attempt to update with a previously-seen client-state string throws a BackendError.  This should never happen unless there's a bug in the tokenserver.  If it does happen due to some weird client race, the database should still be in a consistent state due to unique index on client_state column, and due to refusing to move the generation number backwards at the db level.

Comment 10

[Ryan Kelly [:rfkelly]]

Committed in:

https://github.com/mozilla-services/tokenserver/commit/e079bd890091a74558f8319c6c9819131c291652
https://github.com/mozilla-services/wimms/commit/35f412014245826badb9db5a4d663560a9fc88aa

Comment 11

[James Bonacci [:jbonacci]]

:rfkelly - anything in particular you need QA to do for this?
Check the repos and code, for example? Run tokenserver unit tests?
If not, I will just move this to Verified state.

Comment 12

[Ryan Kelly [:rfkelly]]

please build tokenserver repo and confirm all its tests are passing for you

Comment 13

[James Bonacci [:jbonacci]]

:rfkelly easier said than done!
Is Mac OK for you (unless I have a linux/aws break-though soon...)

Comment 14

[Ryan Kelly [:rfkelly]]

Mac SGTM :-)

Comment 15

[James Bonacci [:jbonacci]]

Did this on Mac and Linux.
All tests Pass (or get Skipped) on Mac.
Some tests fail on ubuntu and rhel-flavored Linux - but probably not related to this ticket.
See https://github.com/mozilla-services/tokenserver/issues?state=open