From d7bca47d357f59b62b3298ff94d7b5363d30d33c Mon Sep 17 00:00:00 2001 From: Clark Boylan Date: Mon, 18 Oct 2021 16:33:58 -0700 Subject: [PATCH] Cleanup empty secrets dirs when deleting secrets The zuul delete-keys command can leave us with empty org and project dirs in zookeeper. When this happens the zuul export-keys command complaisn about secrets not being present. Address this by checking if the project dir and org dir should be cleaned up when calling delete-keys. Note this happend to OpenDev after renaming all projects from foo/* to bar/* orphaning the org level portion of the name. Change-Id: I6bba5ea29a752593b76b8e58a0d84615cc639346 --- tests/unit/test_client.py | 38 +++++++++++++++++++++++++++++++++++--- tests/unit/test_strings.py | 6 +++--- zuul/cmd/client.py | 1 + zuul/lib/keystorage.py | 37 +++++++++++++++++++++++++++++++------ zuul/lib/strings.py | 7 +++---- 5 files changed, 73 insertions(+), 16 deletions(-) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 7dc27ceee1..9b888ee3b0 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -226,7 +226,7 @@ class TestKeyOperations(ZuulTestCase): '-c', config_file, 'copy-keys', 'gerrit', 'org/project', - 'gerrit', 'org/newproject', + 'gerrit', 'neworg/newproject', ], stdout=subprocess.PIPE) out, _ = p.communicate() @@ -236,10 +236,10 @@ class TestKeyOperations(ZuulTestCase): data = self.getZKTree('/keystorage') self.assertEqual( data['/keystorage/gerrit/org/org%2Fproject/secrets'], - data['/keystorage/gerrit/org/org%2Fnewproject/secrets']) + data['/keystorage/gerrit/neworg/neworg%2Fnewproject/secrets']) self.assertEqual( data['/keystorage/gerrit/org/org%2Fproject/ssh'], - data['/keystorage/gerrit/org/org%2Fnewproject/ssh']) + data['/keystorage/gerrit/neworg/neworg%2Fnewproject/ssh']) p = subprocess.Popen( [os.path.join(sys.prefix, 'bin/zuul'), @@ -257,6 +257,38 @@ class TestKeyOperations(ZuulTestCase): data.get('/keystorage/gerrit/org/org%2Fproject/secrets')) self.assertIsNone( data.get('/keystorage/gerrit/org/org%2Fproject/ssh')) + self.assertIsNone( + data.get('/keystorage/gerrit/org/org%2Fproject')) + + p = subprocess.Popen( + [os.path.join(sys.prefix, 'bin/zuul'), + '-c', config_file, + 'delete-keys', + 'gerrit', 'org/project1', + ], + stdout=subprocess.PIPE) + out, _ = p.communicate() + self.log.debug(out.decode('utf8')) + self.assertEqual(p.returncode, 0) + + p = subprocess.Popen( + [os.path.join(sys.prefix, 'bin/zuul'), + '-c', config_file, + 'delete-keys', + 'gerrit', 'org/project2', + ], + stdout=subprocess.PIPE) + out, _ = p.communicate() + self.log.debug(out.decode('utf8')) + self.assertEqual(p.returncode, 0) + + data = self.getZKTree('/keystorage') + self.assertIsNone( + data.get('/keystorage/gerrit/org/org%2Fproject1')) + self.assertIsNone( + data.get('/keystorage/gerrit/org/org%2Fproject2')) + self.assertIsNone( + data.get('/keystorage/gerrit/org')) class TestZKOperations(ZuulTestCase): diff --git a/tests/unit/test_strings.py b/tests/unit/test_strings.py index 6e8f867700..8abdbdebf0 100644 --- a/tests/unit/test_strings.py +++ b/tests/unit/test_strings.py @@ -20,9 +20,9 @@ from tests.base import BaseTestCase class TestStrings(BaseTestCase): def test_unique_project_name(self): - self.assertEqual('project/project', + self.assertEqual(('project', 'project'), strings.unique_project_name('project')) - self.assertEqual('project/project%2Fsubproject', + self.assertEqual(('project', 'project%2Fsubproject'), strings.unique_project_name('project/subproject')) - self.assertEqual('project/project%2Fsub%2Fproject', + self.assertEqual(('project', 'project%2Fsub%2Fproject'), strings.unique_project_name('project/sub/project')) diff --git a/zuul/cmd/client.py b/zuul/cmd/client.py index b6579d97ee..247327b8f7 100755 --- a/zuul/cmd/client.py +++ b/zuul/cmd/client.py @@ -917,6 +917,7 @@ class Client(zuul.cmd.ZuulApp): args = self.args keystore.deleteProjectSSHKeys(args.connection, args.project) keystore.deleteProjectsSecretsKeys(args.connection, args.project) + keystore.deleteProjectDir(args.connection, args.project) self.log.info("Delete keys from %s %s", args.connection, args.project) sys.exit(0) diff --git a/zuul/lib/keystorage.py b/zuul/lib/keystorage.py index bb7b1e21d5..a6284ee184 100644 --- a/zuul/lib/keystorage.py +++ b/zuul/lib/keystorage.py @@ -30,8 +30,12 @@ RSA_KEY_SIZE = 2048 class KeyStorage(ZooKeeperBase): log = logging.getLogger("zuul.KeyStorage") - SECRETS_PATH = "/keystorage/{}/{}/secrets" - SSH_PATH = "/keystorage/{}/{}/ssh" + # /keystorage/connection/orgname + PREFIX_PATH = "/keystorage/{}/{}" + # /keystorage/connection/orgname/projectuniqname + PROJECT_PATH = PREFIX_PATH + "/{}" + SECRETS_PATH = PROJECT_PATH + "/secrets" + SSH_PATH = PROJECT_PATH + "/ssh" def __init__(self, zookeeper_client, password, backup=None): super().__init__(zookeeper_client) @@ -78,8 +82,8 @@ class KeyStorage(ZooKeeperBase): self.log.warning(f"Not overwriting existing key at {path}") def getSSHKeysPath(self, connection_name, project_name): - key_project_name = strings.unique_project_name(project_name) - key_path = self.SSH_PATH.format(connection_name, key_project_name) + prefix, name = strings.unique_project_name(project_name) + key_path = self.SSH_PATH.format(connection_name, prefix, name) return key_path @cachetools.cached(cache={}) @@ -157,8 +161,8 @@ class KeyStorage(ZooKeeperBase): self.saveProjectSSHKeys(connection_name, project_name, keydata) def getProjectSecretsKeysPath(self, connection_name, project_name): - key_project_name = strings.unique_project_name(project_name) - key_path = self.SECRETS_PATH.format(connection_name, key_project_name) + prefix, name = strings.unique_project_name(project_name) + key_path = self.SECRETS_PATH.format(connection_name, prefix, name) return key_path @cachetools.cached(cache={}) @@ -235,3 +239,24 @@ class KeyStorage(ZooKeeperBase): 'keys': keys } self.saveProjectsSecretsKeys(connection_name, project_name, keydata) + + def deleteProjectDir(self, connection_name, project_name): + prefix, name = strings.unique_project_name(project_name) + project_path = self.PROJECT_PATH.format(connection_name, prefix, name) + prefix_path = self.PREFIX_PATH.format(connection_name, prefix) + try: + self.kazoo_client.delete(project_path) + except kazoo.exceptions.NotEmptyError: + # Rely on delete only deleting empty paths by default + self.log.warning(f"Not deleting non empty path {project_path}") + except kazoo.exceptions.NoNodeError: + # Already deleted + pass + try: + self.kazoo_client.delete(prefix_path) + except kazoo.exceptions.NotEmptyError: + # Normal for the org to remain due to other projects existing. + pass + except kazoo.exceptions.NoNodeError: + # Already deleted + pass diff --git a/zuul/lib/strings.py b/zuul/lib/strings.py index 15b327745a..84a119456f 100644 --- a/zuul/lib/strings.py +++ b/zuul/lib/strings.py @@ -22,16 +22,15 @@ import zuul.model def unique_project_name(project_name): parts = project_name.split('/') prefix = parts[0] - name = quote_plus(project_name) - return f'{prefix}/{name}' + return (prefix, name) def workspace_project_path(hostname, project_name, scheme): """Return the project path based on the specified scheme""" if scheme == zuul.model.SCHEME_UNIQUE: - project_name = unique_project_name(project_name) - return os.path.join(hostname, project_name) + prefix, project_name = unique_project_name(project_name) + return os.path.join(hostname, prefix, project_name) elif scheme == zuul.model.SCHEME_GOLANG: return os.path.join(hostname, project_name) elif scheme == zuul.model.SCHEME_FLAT: