mozilla-services / syncstorage-rs

Sync Storage server in Rust
Mozilla Public License 2.0
863 stars 46 forks source link

feat: special case purging of users previously migrated to Spanner #1543

Closed pjenvey closed 4 months ago

pjenvey commented 4 months ago

don't issue deletes to their current data on syncstorage if their old "migration records" point to it

Closes SYNC-4225

jrconlin commented 4 months ago

Looks like a cargo update might help with the audit.

jrconlin commented 4 months ago

if it helps:

index 31680938b0..e7abe84426 100644
--- a/tools/tokenserver/purge_old_records.py
+++ b/tools/tokenserver/purge_old_records.py
@@ -108,7 +108,8 @@ def purge_old_records(
                         database.delete_user_record(row.uid)
                     counter += 1
                 elif force:
-                    delete_sd = not points_to_active(database, row, override_node)
+                    delete_sd = not points_to_active(
+                        database, row, override_node)
                     logger.info(
                         "Forcing tokenserver record delete: "
                         f"{row.uid} on {row.node} "
@@ -129,7 +130,8 @@ def purge_old_records(
                                     secret,
                                     timeout=request_timeout,
                                     dryrun=dryrun,
-                                    # if an override was specifed, use that node ID
+                                    # if an override was specifed,
+                                    # use that node ID
                                     override_node=override_node
                                 )
                             except requests.HTTPError:
@@ -138,7 +140,8 @@ def purge_old_records(
                                     f"{row.uid} [{row.node}]"
                                 )
                                 if override_node:
-                                    # Assume the override_node should be reachable
+                                    # Assume the override_node should be
+                                    # reachable
                                     raise
                         database.delete_user_record(row.uid)
                     counter += 1
@@ -155,7 +158,8 @@ def purge_old_records(
         return True

-def delete_service_data(user, secret, timeout=60, dryrun=False, override_node=None):
+def delete_service_data(
+        user, secret, timeout=60, dryrun=False, override_node=None):
     """Send a data-deletion request to the user's service node.

     This is a little bit of hackery to cause the user's service node to
@@ -204,6 +208,7 @@ def points_to_active(database, replaced_at_row, override_node):
                 user["client_state"] == replaced_at_row.client_state)
     return False

+
 class HawkAuth(requests.auth.AuthBase):
     """Hawk-signing auth helper class."""

diff --git a/tools/tokenserver/test_purge_old_records.py b/tools/tokenserver/test_purge_old_records.py
index 1c911605b2..c3aa00435e 100644
--- a/tools/tokenserver/test_purge_old_records.py
+++ b/tools/tokenserver/test_purge_old_records.py
@@ -12,6 +12,7 @@ from wsgiref.simple_server import make_server
 from database import Database
 from purge_old_records import purge_old_records

+
 class PurgeOldRecordsTestCase(unittest.TestCase):

     @classmethod
@@ -196,10 +197,12 @@ class TestMigrationRecords(PurgeOldRecordsTestCase):
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
-        cls.spanner_service = make_server("localhost", 0, cls._service_app)
+        cls.spanner_service = make_server(
+            "localhost", 0, cls._service_app)
         host, port = cls.spanner_service.server_address
         cls.spanner_node = f"http://{host}:{port}"
-        cls.spanner_thread = threading.Thread(target=cls.spanner_service.serve_forever)
+        cls.spanner_thread = threading.Thread(
+            target=cls.spanner_service.serve_forever)
         cls.spanner_thread.start()
         cls.downed_node = f"http://{host}:9999"

@@ -230,7 +233,8 @@ class TestMigrationRecords(PurgeOldRecordsTestCase):
         email = "test@mozilla.com"
         user = self.database.allocate_user(email, client_state="aa")
         self.database.replace_user_record(user["uid"])
-        user = self.database.allocate_user(email, node=self.spanner_node, client_state="aa")
+        user = self.database.allocate_user(
+            email, node=self.spanner_node, client_state="aa")

         self.assertTrue(purge_old_records(node_secret, grace_period=0))
         user_records = list(self.database.get_user_records(email))
@@ -259,7 +263,10 @@ class TestMigrationRecords(PurgeOldRecordsTestCase):

         self.assertTrue(
             purge_old_records(
-                node_secret, grace_period=0, force=True, override_node=self.spanner_node
+                node_secret,
+                grace_period=0,
+                force=True,
+                override_node=self.spanner_node
             )
         )
         user_records = list(self.database.get_user_records(email))
@@ -290,7 +297,10 @@ class TestMigrationRecords(PurgeOldRecordsTestCase):

         self.assertTrue(
             purge_old_records(
-                node_secret, grace_period=0, force=True, override_node=self.spanner_node
+                node_secret,
+                grace_period=0,
+                force=True,
+                override_node=self.spanner_node
             )
         )
         user_records = list(self.database.get_user_records(email))
pjenvey commented 4 months ago

thanks, forgot we had lint checks somewhere

jrconlin commented 4 months ago

Yeah, I kinda wonder if we should standardize to black for python stuff, since autopush wants to run both. It'd save us a bit of hassle to just have the various IDEs run black on save and be done with that sort of thing.

taddes commented 4 months ago

How do you feel if I were to draft a quick PR to give us those same Makefile commands to run linting and style enforcement? At the least, flake8. That way it's consistent as @jrconlin mentioned.

jrconlin commented 4 months ago

We're going to land this PR as is, mostly because of time constraints, but @taddes has a few good suggestions for clean up. Let's move those to a different PR (feel free to create one @taddes) and we can land that later.