Merge pull request #3071 from quay/joseph.schorr/QUAY-932/namespace-robot-deletion

Fix issue where we didn't delete robots immediately under a namespace

data/migrations/versions/5b7503aada1b_cleanup_old_robots.py

@@ -0,0 +1,23 @@
+"""Cleanup old robots
+
+Revision ID: 5b7503aada1b
+Revises: 224ce4c72c2f
+Create Date: 2018-05-09 17:18:52.230504
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '5b7503aada1b'
+down_revision = '224ce4c72c2f'
+
+from alembic import op
+import sqlalchemy as sa
+
+from util.migrate.cleanup_old_robots import cleanup_old_robots
+
+def upgrade(tables):
+  cleanup_old_robots()
+
+def downgrade(tables):
+  # Nothing to do.
+  pass

data/model/test/test_user.py

@@ -7,6 +7,8 @@ from mock import patch
 from data.database import EmailConfirmation, User, DeletedNamespace
 from data.model.user import create_user_noverify, validate_reset_code, get_active_users
 from data.model.user import mark_namespace_for_deletion, delete_namespace_via_marker
+from data.model.user import create_robot, lookup_robot, list_namespace_robots
+from data.model.user import InvalidRobotException
 from util.timedeltastring import convert_to_timedelta
 from data.queue import WorkQueue
 from test.fixtures import *
@@ -48,12 +50,30 @@ def test_mark_namespace_for_deletion(initialized_db):
   # Create a user and then mark it for deletion.
   user = create_user_noverify('foobar', 'foo@example.com', email_required=False)
 
+  # Add some robots.
+  create_robot('foo', user)
+  create_robot('bar', user)
+
+  assert lookup_robot('foobar+foo') is not None
+  assert lookup_robot('foobar+bar') is not None
+  assert len(list(list_namespace_robots('foobar'))) == 2
+
   # Mark the user for deletion.
   queue = WorkQueue('testgcnamespace', create_transaction)
   mark_namespace_for_deletion(user, [], queue)
 
   # Ensure the older user is still in the DB.
-  assert User.get(id=user.id).username != 'foobar'
+  older_user = User.get(id=user.id)
+  assert older_user.username != 'foobar'
+
+  # Ensure the robots are deleted.
+  with pytest.raises(InvalidRobotException):
+    assert lookup_robot('foobar+foo')
+
+  with pytest.raises(InvalidRobotException):
+    assert lookup_robot('foobar+bar')
+
+  assert len(list(list_namespace_robots(older_user.username))) == 0
 
   # Ensure we can create a user with the same namespace again.
   new_user = create_user_noverify('foobar', 'foo@example.com', email_required=False)

data/model/user.py

@@ -68,7 +68,6 @@ def create_user_noverify(username, email, email_required=True, prompts=tuple(),
 
   try:
     existing = User.get((User.username == username) | (User.email == email))
-
     logger.info('Existing user with same username or email.')
 
     # A user already exists with either the same username or email
@@ -77,11 +76,11 @@ def create_user_noverify(username, email, email_required=True, prompts=tuple(),
                                      username)
     raise InvalidEmailAddressException('Email has already been used: %s' %
                                        email)
-
   except User.DoesNotExist:
     # This is actually the happy path
     logger.debug('Email and username are unique!')
 
+  # Create the user.
   try:
     default_expr_s = _convert_to_s(config.app_config['DEFAULT_TAG_EXPIRATION'])
     default_max_builds = config.app_config.get('DEFAULT_NAMESPACE_MAXIMUM_BUILD_COUNT')
@@ -970,20 +969,29 @@ def delete_user(user, queues):
 def _delete_user_linked_data(user):
   if user.organization:
     # Delete the organization's teams.
-    for team in Team.select().where(Team.organization == user):
-      team.delete_instance(recursive=True)
+    with db_transaction():
+      for team in Team.select().where(Team.organization == user):
+        team.delete_instance(recursive=True)
 
     # Delete any OAuth approvals and tokens associated with the user.
-    for app in OAuthApplication.select().where(OAuthApplication.organization == user):
-      app.delete_instance(recursive=True)
+    with db_transaction():
+      for app in OAuthApplication.select().where(OAuthApplication.organization == user):
+        app.delete_instance(recursive=True)
   else:
     # Remove the user from any teams in which they are a member.
     TeamMember.delete().where(TeamMember.user == user).execute()
 
   # Delete any repository buildtriggers where the user is the connected user.
-  triggers = RepositoryBuildTrigger.select().where(RepositoryBuildTrigger.connected_user == user)
-  for trigger in triggers:
-    trigger.delete_instance(recursive=True, delete_nullable=False)
+  with db_transaction():
+    triggers = RepositoryBuildTrigger.select().where(RepositoryBuildTrigger.connected_user == user)
+    for trigger in triggers:
+      trigger.delete_instance(recursive=True, delete_nullable=False)
+
+  # Delete any robots owned by this user.
+  with db_transaction():
+    robots = list(list_namespace_robots(user.username))
+    for robot in robots:
+      robot.delete_instance(recursive=True, delete_nullable=True)
 
   # Null out any service key approvals. We technically lose information here, but its better than
   # falling and only occurs if a superuser is being deleted.

util/migrate/cleanup_old_robots.py

@@ -0,0 +1,28 @@
+from data.database import User
+from util.names import parse_robot_username
+
+def cleanup_old_robots(page_size=50):
+  """ Deletes any robots that live under namespaces that no longer exist. """
+  # Collect the robot accounts to delete.
+  page_number = 1
+  to_delete = []
+
+  while True:
+    found_bots = False
+    for robot in list(User.select().where(User.robot == True).paginate(page_number, page_size)):
+      found_bots = True
+      namespace, _ = parse_robot_username(robot.username)
+      try:
+        User.get(username=namespace)
+      except User.DoesNotExist:
+        # Save the robot account for deletion.
+        to_delete.append(robot)
+
+    if not found_bots:
+      break
+
+    page_number = page_number + 1
+
+  # Cleanup any robot accounts whose corresponding namespace doesn't exist.
+  for robot in to_delete:
+    robot.delete_instance(recursive=True, delete_nullable=True)

util/migrate/test/test_cleanup_old_robots.py

@@ -0,0 +1,43 @@
+import pytest
+
+from data.database import User
+from util.migrate.cleanup_old_robots import cleanup_old_robots
+
+from test.fixtures import *
+
+def test_cleanup_old_robots(initialized_db):
+  before_robot_count = User.select().where(User.robot == True).count()
+  before_user_count = User.select().count()
+
+  # Run the cleanup once, and ensure it does nothing.
+  cleanup_old_robots()
+
+  after_robot_count = User.select().where(User.robot == True).count()
+  after_user_count = User.select().count()
+
+  assert before_robot_count == after_robot_count
+  assert before_user_count == after_user_count
+
+  # Create some orphan robots.
+  created = set()
+  for index in range(0, 50):
+    created.add('doesnotexist+a%s' % index)
+    created.add('anothernamespace+b%s' % index)
+
+    User.create(username='doesnotexist+a%s' % index, robot=True)
+    User.create(username='anothernamespace+b%s' % index, robot=True)
+
+  before_robot_count = User.select().where(User.robot == True).count()
+  before_user_count = User.select().count()
+
+  cleanup_old_robots(page_size=10)
+
+  after_robot_count = User.select().where(User.robot == True).count()
+  after_user_count = User.select().count()
+
+  assert before_robot_count == after_robot_count + len(created)
+  assert before_user_count == after_user_count + len(created)
+
+  for name in created:
+    with pytest.raises(User.DoesNotExist):
+      User.get(username=name)