From dca1772a1f5853f0fb71dcaa9deb17b5f81b57e6 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Tue, 15 Oct 2024 13:59:20 +0100 Subject: [PATCH 01/10] Isolate CILogon delete client API call from other operations Moves most contents of delete_client to delete --- deployer/commands/cilogon.py | 109 ++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 53 deletions(-) diff --git a/deployer/commands/cilogon.py b/deployer/commands/cilogon.py index 0e20c5ff3..d0551f7d8 100644 --- a/deployer/commands/cilogon.py +++ b/deployer/commands/cilogon.py @@ -264,52 +264,19 @@ def get_client(admin_id, admin_secret, cluster_name, hub_name, client_id=None): return client_details -def delete_client(admin_id, admin_secret, cluster_name, hub_name, client_id=None): - """Deletes the client associated with the id. - - Args: - id (str): Id of the client to delete +def delete_client(admin_id, admin_secret, client_id=None): + """Deletes a CILogon client. Returns status code if response.ok or None if the `delete` request returned a status code not in the range 200-299. See: https://github.com/ncsa/OA4MP/blob/HEAD/oa4mp-server-admin-oauth2/src/main/scripts/oidc-cm-scripts/cm-delete.sh """ - config_filename = build_absolute_path_to_hub_encrypted_config_file( - cluster_name, hub_name - ) - - if not client_id: - if Path(config_filename).is_file(): - client_id = load_client_id_from_file(config_filename) - # Nothing to do if no client has been found - if not client_id: - print_colour( - "No `client_id` to delete was provided and couldn't find any in `config_filename`", - "red", - ) - return - else: - print_colour( - f"No `client_id` to delete was provided and couldn't find any {config_filename} file", - "red", - ) - return + if client_id is None: + print("Deleting a CILogon client fr unknown ID") else: - if not stored_client_id_same_with_cilogon_records( - admin_id, - admin_secret, - cluster_name, - hub_name, - client_id, - ): - print_colour( - "CILogon records are different than the client app stored in the configuration file. Consider updating the file.", - "red", - ) - return + print(f"Deleting the CILogon client details for {client_id}...") - print(f"Deleting the CILogon client details for {client_id}...") headers = build_request_headers(admin_id, admin_secret) response = requests.delete(build_request_url(client_id), headers=headers, timeout=5) if not response.ok: @@ -318,19 +285,6 @@ def delete_client(admin_id, admin_secret, cluster_name, hub_name, client_id=None print_colour("Done!") - # Delete client credentials from config file also if file exists - if Path(config_filename).is_file(): - print(f"Deleting the CILogon client details from the {config_filename} also...") - key = "CILogonOAuthenticator" - try: - remove_jupyterhub_hub_config_key_from_encrypted_file(config_filename, key) - except KeyError: - print_colour(f"No {key} found to delete from {config_filename}", "yellow") - return - print_colour(f"CILogonAuthenticator config removed from {config_filename}") - if not Path(config_filename).is_file(): - print_colour(f"Empty {config_filename} file also deleted.", "yellow") - def get_all_clients(admin_id, admin_secret): print("Getting all existing CILogon client applications...") @@ -432,7 +386,56 @@ def delete( """, ), ): - """Delete an existing CILogon client. This deletes both the CILogon client application, - and the client credentials from the configuration file.""" + """ + Delete an existing CILogon client. This deletes both the CILogon client application, + and the client credentials from the configuration file. + """ + config_filename = build_absolute_path_to_hub_encrypted_config_file( + cluster_name, hub_name + ) admin_id, admin_secret = get_2i2c_cilogon_admin_credentials() + + if not client_id: + if Path(config_filename).is_file(): + client_id = load_client_id_from_file(config_filename) + # Nothing to do if no client has been found + if not client_id: + print_colour( + "No `client_id` to delete was provided and couldn't find any in `config_filename`", + "red", + ) + return + else: + print_colour( + f"No `client_id` to delete was provided and couldn't find any {config_filename} file", + "red", + ) + return + else: + if not stored_client_id_same_with_cilogon_records( + admin_id, + admin_secret, + cluster_name, + hub_name, + client_id, + ): + print_colour( + "CILogon records are different than the client app stored in the configuration file. Consider updating the file.", + "red", + ) + return + delete_client(admin_id, admin_secret, cluster_name, hub_name, client_id) + + # Delete client credentials from config file also if file exists + if Path(config_filename).is_file(): + print(f"Deleting the CILogon client details from the {config_filename} also...") + key = "CILogonOAuthenticator" + try: + remove_jupyterhub_hub_config_key_from_encrypted_file(config_filename, key) + except KeyError: + print_colour(f"No {key} found to delete from {config_filename}", "yellow") + return + print_colour(f"CILogonAuthenticator config removed from {config_filename}") + if not Path(config_filename).is_file(): + print_colour(f"Empty {config_filename} file also deleted.", "yellow") From 6efdf78a76711bd85a52dbf362ad35174552a505 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Wed, 16 Oct 2024 14:01:15 +0100 Subject: [PATCH 02/10] Move the printing of existing clients from get_all_clients to get_all --- deployer/commands/cilogon.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/deployer/commands/cilogon.py b/deployer/commands/cilogon.py index d0551f7d8..ffa89ca35 100644 --- a/deployer/commands/cilogon.py +++ b/deployer/commands/cilogon.py @@ -298,8 +298,7 @@ def get_all_clients(admin_id, admin_secret): return clients = response.json() - for c in clients["clients"]: - print(c) + return [c for c in clients["clients"]] def get_2i2c_cilogon_admin_credentials(): @@ -369,7 +368,9 @@ def get( def get_all(): """Retrieve details about all existing 2i2c CILogon clients.""" admin_id, admin_secret = get_2i2c_cilogon_admin_credentials() - get_all_clients(admin_id, admin_secret) + clients = get_all_clients(admin_id, admin_secret) + for c in clients: + print(c) @cilogon_client_app.command() From d04e58f621a204b35eed0bf931d48750a4397492 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Wed, 16 Oct 2024 14:05:28 +0100 Subject: [PATCH 03/10] Add a function to identify duplicated clients by name --- deployer/commands/cilogon.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/deployer/commands/cilogon.py b/deployer/commands/cilogon.py index ffa89ca35..c4a084c5c 100644 --- a/deployer/commands/cilogon.py +++ b/deployer/commands/cilogon.py @@ -17,6 +17,7 @@ import base64 import json +from collections import Counter from pathlib import Path import requests @@ -301,6 +302,21 @@ def get_all_clients(admin_id, admin_secret): return [c for c in clients["clients"]] +def find_duplicated_clients(clients): + """Determine duplicated CILogon clients by comparing client names + + Args: + clients (list[dict]): A list of dictionaries containing information about + the existing CILogon clients. Generated by get_all_clients function. + + Returns: + list: A list of duplicated client names + """ + client_names = [c["name"] for c in clients] + client_names_count = Counter(client_names) + return [k for k, v in client_names_count.items() if v > 1] + + def get_2i2c_cilogon_admin_credentials(): """ Retrieve the 2i2c administrative client credentials From ab6b64939bfc44dc445876e58ff6293acff6f7e0 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Wed, 16 Oct 2024 14:59:37 +0100 Subject: [PATCH 04/10] Create a cleanup function that wraps duplicated app discovery and ensures safety for deletion --- deployer/commands/cilogon.py | 51 ++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/deployer/commands/cilogon.py b/deployer/commands/cilogon.py index c4a084c5c..b175fd39c 100644 --- a/deployer/commands/cilogon.py +++ b/deployer/commands/cilogon.py @@ -13,6 +13,7 @@ - `delete` a CILogon client application when a hub is removed or changes auth methods - `get` details about an existing hub CILogon client - `get-all` existing 2i2c CILogon client applications +- `cleanup` duplicated CILogon applications """ import base64 @@ -456,3 +457,53 @@ def delete( print_colour(f"CILogonAuthenticator config removed from {config_filename}") if not Path(config_filename).is_file(): print_colour(f"Empty {config_filename} file also deleted.", "yellow") + + +@cilogon_client_app.command() +def cleanup(): + client_ids_to_be_deleted = [] + + admin_id, admin_secret = get_2i2c_cilogon_admin_credentials() + clients = get_all_clients(admin_id, admin_secret) + duplicated_clients = find_duplicated_clients(clients) + + # Cycle over each duplicated client name + for duped_client in duplicated_clients: + # Finds all the client IDs associated with a duplicated name + ids = [c["client_id"] for c in clients if c["name"] == duped_client] + + # Establish the cluster and hub name from the client name and build the + # absolute path to the encrypted hub values file + cluster_name, hub_name = duped_client.split("-") + config_filename = build_absolute_path_to_hub_encrypted_config_file( + cluster_name, hub_name + ) + + with get_decrypted_file(config_filename) as decrypted_path: + with open(decrypted_path) as f: + secret_config = yaml.load(f) + + if ( + "CILogonOAuthenticator" + not in secret_config["jupyterhub"]["hub"]["config"].keys() + ): + print( + f"Hub {hub_name} on cluster {cluster_name} doesn't use CILogonOAuthenticator." + ) + else: + # Extract the client ID *currently in use* from the encrypted config and remove it from the list of IDs + config_client_id = secret_config["jupyterhub"]["hub"]["config"][ + "CILogonOAuthenticator" + ]["client_id"] + ids.remove(config_client_id) + + client_ids_to_be_deleted.extend( + [{"client_name": duped_client, "client_id": id} for id in ids] + ) + + # Remove the duplicated clients from the client list + clients = [c for c in clients if c["name"] != duped_client] + + print_colour("CILogon clients to be deleted...") + for c in client_ids_to_be_deleted: + print(c) From a7ff23b4dfed0970c442671e2d3ae4b9d92269c4 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Wed, 16 Oct 2024 15:09:12 +0100 Subject: [PATCH 05/10] Add functionality to optionally delete the CILogon apps --- deployer/commands/cilogon.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/deployer/commands/cilogon.py b/deployer/commands/cilogon.py index b175fd39c..b8cf34784 100644 --- a/deployer/commands/cilogon.py +++ b/deployer/commands/cilogon.py @@ -275,7 +275,7 @@ def delete_client(admin_id, admin_secret, client_id=None): See: https://github.com/ncsa/OA4MP/blob/HEAD/oa4mp-server-admin-oauth2/src/main/scripts/oidc-cm-scripts/cm-delete.sh """ if client_id is None: - print("Deleting a CILogon client fr unknown ID") + print("Deleting a CILogon client for unknown ID") else: print(f"Deleting the CILogon client details for {client_id}...") @@ -460,7 +460,17 @@ def delete( @cilogon_client_app.command() -def cleanup(): +def cleanup( + delete: bool = typer.Option( + False, help="Proceed with deleting duplicated CILogon apps" + ) +): + """Identify duplicated CILogon clients and which ID is being actively used in config, + and optionally delete unused duplicates. + + Args: + delete (bool, optional): Delete unused duplicate CILogon apps. Defaults to False. + """ client_ids_to_be_deleted = [] admin_id, admin_secret = get_2i2c_cilogon_admin_credentials() @@ -507,3 +517,7 @@ def cleanup(): print_colour("CILogon clients to be deleted...") for c in client_ids_to_be_deleted: print(c) + + if delete: + for c in client_ids_to_be_deleted: + delete_client(admin_id, admin_secret, client_id=c["client_id"]) From 241d6bf4dbbebd3641a9745a69feb6ce22a289c7 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Thu, 17 Oct 2024 11:29:30 +0100 Subject: [PATCH 06/10] Add a utils function that will list all cluster names from the config/clusters directory --- deployer/utils/file_acquisition.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/deployer/utils/file_acquisition.py b/deployer/utils/file_acquisition.py index f78ac8fb2..0f0a626f0 100644 --- a/deployer/utils/file_acquisition.py +++ b/deployer/utils/file_acquisition.py @@ -248,3 +248,12 @@ def get_all_cluster_yaml_files(): for path in CONFIG_CLUSTERS_PATH.glob("**/cluster.yaml") if "templates" not in path.as_posix() } + + +def get_cluster_names_list(): + """ + Returns a list of all the clusters currently listed under config/clusters + """ + return [ + d.name for d, _, _ in CONFIG_CLUSTERS_PATH.walk() if "templates" not in str(d) + ] From de04c2fecfcefc7e268ee062bc4790efa9155292 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Thu, 17 Oct 2024 11:30:26 +0100 Subject: [PATCH 07/10] Add a function to determine orphaned CILogon clients --- deployer/commands/cilogon.py | 50 ++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/deployer/commands/cilogon.py b/deployer/commands/cilogon.py index b8cf34784..68ff68d44 100644 --- a/deployer/commands/cilogon.py +++ b/deployer/commands/cilogon.py @@ -29,6 +29,8 @@ from deployer.cli_app import cilogon_client_app from deployer.utils.file_acquisition import ( build_absolute_path_to_hub_encrypted_config_file, + find_absolute_path_to_cluster_file, + get_cluster_names_list, get_decrypted_file, persist_config_in_encrypted_file, remove_jupyterhub_hub_config_key_from_encrypted_file, @@ -318,6 +320,51 @@ def find_duplicated_clients(clients): return [k for k, v in client_names_count.items() if v > 1] +def find_orphaned_clients(clients): + """Find CILogon clients for which an associated cluster or hub no longer + exists and can safely be deleted. + + Args: + clients (list[dict]): A list of existing CILogon client info + + Returns: + list[dict]: A list of 'orphaned' CILogon clients which don't have an + associated cluster or hub, which can be deleted + """ + clients_to_be_deleted = [] + clusters = get_cluster_names_list() + + for client in clients: + cluster = next((cl for cl in clusters if cl in client["name"]), "") + + if cluster: + cluster_config_file = find_absolute_path_to_cluster_file(cluster) + with open(cluster_config_file) as f: + cluster_config = yaml.load(f) + + hub = next( + ( + hub["name"] + for hub in cluster_config["hubs"] + if hub["name"] in client["name"] + ), + "", + ) + + if not hub: + print( + f"A hub pertaining to client {client['name']} does NOT exist. Marking client for deletion." + ) + clients_to_be_deleted.append(client) + else: + print( + f"A cluster pertaining to client {client['name']} does NOT exist. Marking client for deletion." + ) + clients_to_be_deleted.append(client) + + return clients_to_be_deleted + + def get_2i2c_cilogon_admin_credentials(): """ Retrieve the 2i2c administrative client credentials @@ -514,6 +561,9 @@ def cleanup( # Remove the duplicated clients from the client list clients = [c for c in clients if c["name"] != duped_client] + orphaned_clients = find_orphaned_clients(clients) + client_ids_to_be_deleted.extend(orphaned_clients) + print_colour("CILogon clients to be deleted...") for c in client_ids_to_be_deleted: print(c) From 49d7e1d084b7bb46e5d8bee40ed869fe5009dd12 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Thu, 17 Oct 2024 11:35:14 +0100 Subject: [PATCH 08/10] Clarify variable naming --- deployer/commands/cilogon.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/deployer/commands/cilogon.py b/deployer/commands/cilogon.py index 68ff68d44..59c8934bb 100644 --- a/deployer/commands/cilogon.py +++ b/deployer/commands/cilogon.py @@ -518,7 +518,7 @@ def cleanup( Args: delete (bool, optional): Delete unused duplicate CILogon apps. Defaults to False. """ - client_ids_to_be_deleted = [] + clients_to_be_deleted = [] admin_id, admin_secret = get_2i2c_cilogon_admin_credentials() clients = get_all_clients(admin_id, admin_secret) @@ -554,7 +554,7 @@ def cleanup( ]["client_id"] ids.remove(config_client_id) - client_ids_to_be_deleted.extend( + clients_to_be_deleted.extend( [{"client_name": duped_client, "client_id": id} for id in ids] ) @@ -562,12 +562,12 @@ def cleanup( clients = [c for c in clients if c["name"] != duped_client] orphaned_clients = find_orphaned_clients(clients) - client_ids_to_be_deleted.extend(orphaned_clients) + clients_to_be_deleted.extend(orphaned_clients) print_colour("CILogon clients to be deleted...") - for c in client_ids_to_be_deleted: + for c in clients_to_be_deleted: print(c) if delete: - for c in client_ids_to_be_deleted: + for c in clients_to_be_deleted: delete_client(admin_id, admin_secret, client_id=c["client_id"]) From 964cc6d25f9a4dcf467baba879b55902feacb589 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Thu, 17 Oct 2024 11:40:03 +0100 Subject: [PATCH 09/10] Provide user feedback about how manyh client spots are used in get-all command --- deployer/commands/cilogon.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/deployer/commands/cilogon.py b/deployer/commands/cilogon.py index 59c8934bb..43e1fd431 100644 --- a/deployer/commands/cilogon.py +++ b/deployer/commands/cilogon.py @@ -436,6 +436,10 @@ def get_all(): for c in clients: print(c) + # Our plan with CILogon only permits 100 clients, so provide feedback on that + # number here. Change this if our plan updates. + print_colour(f"{len(clients)} / 100 clients used", "yellow") + @cilogon_client_app.command() def delete( From ef3805213943c6a8ba9ba0e7ba90e5807df03871 Mon Sep 17 00:00:00 2001 From: Sarah Gibson Date: Thu, 17 Oct 2024 11:50:34 +0100 Subject: [PATCH 10/10] Update deployer readme --- deployer/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/deployer/README.md b/deployer/README.md index 5bc4562c0..141dabfa8 100644 --- a/deployer/README.md +++ b/deployer/README.md @@ -369,9 +369,9 @@ This allows us to validate that all required values are present and have the cor ### The `cilogon-client` sub-command for CILogon OAuth client management Deployer sub-command for managing CILogon clients for 2i2c hubs. -#### `cilogon-client create/delete/get/get-all/update` +#### `cilogon-client create/delete/get/get-all/update/cleanup` -create/delete/get/get-all/update/ CILogon clients using the 2i2c administrative client provided by CILogon. +create/delete/get/get-all/update/cleanup CILogon clients using the 2i2c administrative client provided by CILogon. ### The `exec` sub-command for executing shells and debugging commands