From 7fbb8b92301a4443600de80bbaf91f5e940c9391 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 13:20:52 -0400 Subject: [PATCH 01/11] Replace TTL-cached Koji sessions with a private singleton and in-process Kerberos. get_buildsys() and its cachetools TTL are gone. call_koji() now owns a private _bsys, renews the TGT via python-gssapi only when remaining lifetime is under 55 minutes (and only when --krb5-keytab-file is set), recreates the Koji session as part of that renew unit, and runs acquire/login/RPC work in worker threads. Without a keytab, an existing TGT from $KRB5CCNAME or the system default ccache is used. GSSAPI "Already logged in" is treated as success. Call sites use method names or helpers that receive the session as the first argument. Daemon/run.sh/local_test gain --krb5-keytab-file and --krb5-keytab-principal; the background kinit and koji hello loops are removed. Add connection unit tests and document the new auth path. Co-authored-by: Cursor --- README.md | 27 ++- elnbuildsync/batching.py | 6 +- elnbuildsync/cleanup.py | 5 +- elnbuildsync/daemon.py | 43 ++++ elnbuildsync/kojihelpers/builds.py | 23 +- elnbuildsync/kojihelpers/connection.py | 320 +++++++++++++++++++++---- elnbuildsync/kojihelpers/errors.py | 12 + elnbuildsync/kojihelpers/tags.py | 28 +-- elnbuildsync/status.py | 10 +- requirements.txt | 1 + run.sh | 70 +++--- tests/local_test_daemon.sh | 41 +++- tests/test_koji_connection.py | 298 +++++++++++++++++++++++ 13 files changed, 742 insertions(+), 142 deletions(-) create mode 100644 tests/test_koji_connection.py diff --git a/README.md b/README.md index ae52a45..dd81646 100644 --- a/README.md +++ b/README.md @@ -214,14 +214,17 @@ You also need: - A **Fedora account** with permission to run builds against the Koji targets referenced in your test configuration (the sample config uses staging-oriented settings and `scratch_build: true`). -- **Kerberos credentials** for Koji. EBS inside the container uses the - host's KCM socket: +- **Kerberos credentials** for Koji. Local testing uses the host KCM + socket after you obtain a ticket: ```bash kinit your_fedora_username@FEDORAPROJECT.ORG - koji hello ``` + Production/`run.sh` authenticates in-process via python-gssapi when + `--krb5-keytab-file` is set; otherwise an existing TGT in `$KRB5CCNAME` + (or the system default ccache) is used. There is no background `kinit` + or `koji hello` readiness loop. - **Test configuration** under `tests/etc/`, mirroring the container layout at `/etc/elnbuildsync/`: @@ -316,6 +319,13 @@ Useful options: # Use production Fedora Messaging broker config (instead of staging) ./tests/local_test_daemon.sh --environment prod + +# Optional Kerberos overrides for keytab-based TGT acquisition. Without a +# keytab, the host KCM / existing ccache is used after kinit. +./tests/local_test_daemon.sh --krb5-keytab-file /path/to/krb5.keytab +./tests/local_test_daemon.sh \ + --krb5-keytab-file /path/to/krb5.keytab \ + --krb5-keytab-principal 'eln-buildsync@FEDORAPROJECT.ORG' ``` When you stop the script (Ctrl+C), the ephemeral PostgreSQL container is @@ -323,7 +333,8 @@ removed. ### Verify it is working -1. Confirm `koji hello` works on the host before starting the daemon. +1. Confirm you have a valid Kerberos TGT on the host (`klist`) before + starting the local test daemon. 2. After startup, open [http://localhost:8080/status.html](http://localhost:8080/status.html). 3. Watch `/tmp/elnbuildsync.log` for batch activity when Rawhide tag @@ -353,8 +364,12 @@ git repository (see `run.sh --dynamic-config-url`) or SMTP, and OIDC client secrets mount at `/etc/elnbuildsync/secrets/` (`ebs_db_pw`, `ebs_smtp_pw`, `ebs_oidc_client_secret`). Pass `--openid-client-secret-file` when the secret is mounted elsewhere, and -`--openid-ca-file` when the OIDC provider uses a non-public CA. A -service keytab is used for `eln-buildsync@FEDORAPROJECT.ORG`. OpenShift +`--openid-ca-file` when the OIDC provider uses a non-public CA. Kerberos +is handled in-process with python-gssapi: optionally pass +`--krb5-keytab-file` for TGT acquisition and `--krb5-keytab-principal` +(or rely on guessing `koji.username` plus realm from `koji.profile`) for +that keytab kinit only. Without a keytab, the daemon uses an existing TGT +from `$KRB5CCNAME` or the system default ccache. OpenShift deployment is managed via [infra-ansible](https://forge.fedoraproject.org/infra/ansible) (`playbooks/openshift-apps/elnbuildsync.yml`). diff --git a/elnbuildsync/batching.py b/elnbuildsync/batching.py index bade8a7..d641409 100644 --- a/elnbuildsync/batching.py +++ b/elnbuildsync/batching.py @@ -94,11 +94,9 @@ async def rebuild_from_components(downstream_components): """Takes an iterable of downstream component names and rebuilds them.""" # Fake up a TagMessage for each of these to enqueue into the next batch - bsys = kojihelpers.connection.get_buildsys() - src_tag = config.control["trigger_tag"] latest_tagged_rawhide_pkgs = await call_koji( - bsys.listTagged, src_tag, latest=True, inherit=True + "listTagged", src_tag, latest=True, inherit=True ) latest_tagged_rawhide_table = { pkg["name"]: pkg for pkg in latest_tagged_rawhide_pkgs @@ -109,7 +107,7 @@ async def rebuild_from_components(downstream_components): dest_tag, ) = await kojihelpers.tags.get_tags_for_target(config.main["koji"]["build_target"]) latest_tagged_eln_pkgs = await call_koji( - bsys.listTagged, dest_tag, latest=True, inherit=True + "listTagged", dest_tag, latest=True, inherit=True ) latest_tagged_eln_table = {pkg["name"]: pkg for pkg in latest_tagged_eln_pkgs} diff --git a/elnbuildsync/cleanup.py b/elnbuildsync/cleanup.py index 6560891..059e9d4 100644 --- a/elnbuildsync/cleanup.py +++ b/elnbuildsync/cleanup.py @@ -32,14 +32,13 @@ async def periodic_cleanup(): return logger.debug("Starting periodic cleanup.") - bsys = kojihelpers.connection.get_buildsys() # We have the set of desired packages from Content Resolver desired_pkg_names = set(config.comps["downstream_components"].keys()) # Get the list of packages currently tagged into the stable tag latest_tagged_dest_pkgs = await call_koji( - bsys.listTagged, config.main["koji"]["stable_tag"], latest=True + "listTagged", config.main["koji"]["stable_tag"], latest=True ) # Get the list of up-to-date packages in the destination tag @@ -52,7 +51,7 @@ async def periodic_cleanup(): # Get the complete list of builds tagged into the stable tag all_tagged_dest_pkgs = await call_koji( - bsys.listTagged, config.main["koji"]["stable_tag"], latest=False + "listTagged", config.main["koji"]["stable_tag"], latest=False ) all_tagged_dest_nvrs = {pkg["nvr"] for pkg in all_tagged_dest_pkgs} diff --git a/elnbuildsync/daemon.py b/elnbuildsync/daemon.py index 53bb990..a85d790 100644 --- a/elnbuildsync/daemon.py +++ b/elnbuildsync/daemon.py @@ -39,6 +39,8 @@ status, web, ) +from .config import ConfigError +from .kojihelpers import connection as koji_connection logger = logging.getLogger(__name__) @@ -126,6 +128,24 @@ def _resolve_dynamic_source(dynamic_config_url, dynamic_config_file): default=False, help="Untag all but the most recent builds in the destination target", ) +@click.option( + "--krb5-keytab-file", + default=None, + type=click.Path(dir_okay=False), + help=( + "Kerberos keytab for in-process TGT acquisition (kinit). " + "If omitted, use an existing TGT from $KRB5CCNAME or the system default ccache" + ), +) +@click.option( + "--krb5-keytab-principal", + default=None, + show_default="automatic, guessed from static configuration when using a keytab", + help=( + "Kerberos principal to use when acquiring a TGT from --krb5-keytab-file. " + "Ignored unless a keytab is specified" + ), +) def main( log_level, dry_run, @@ -138,6 +158,8 @@ def main( openid_client_secret_file, openid_ca_file, untagging, + krb5_keytab_file, + krb5_keytab_principal, ): logging.basicConfig( format="%(asctime)s : %(name)s : %(levelname)s : %(message)s", @@ -156,6 +178,9 @@ def main( config.do_untagging = untagging config.message_batch_timer = lull_time + if krb5_keytab_principal and not krb5_keytab_file: + raise click.UsageError("--krb5-keytab-principal requires --krb5-keytab-file") + dynamic_url, dynamic_file = _resolve_dynamic_source( dynamic_config_url, dynamic_config_file ) @@ -172,6 +197,8 @@ def main( dynamic_file, openid_client_secret_file, openid_ca_file, + krb5_keytab_file, + krb5_keytab_principal, ) ) ) @@ -186,6 +213,8 @@ async def _main( dynamic_config_file=None, openid_client_secret_file=None, openid_ca_file=None, + krb5_keytab_file=None, + krb5_keytab_principal=None, ) -> None: auth.openid_ca_file = openid_ca_file config.terminator = Deferred() @@ -206,6 +235,20 @@ async def _main( db_pw, oidc_client_secret_file=openid_client_secret_file, ) + keytab_principal = None + if krb5_keytab_file: + try: + keytab_principal = koji_connection.resolve_krb5_keytab_principal( + krb5_keytab_principal, + config.main["koji"]["profile"], + config.main["koji"].get("username"), + ) + except ValueError as e: + raise ConfigError(str(e)) from e + koji_connection.configure_kerberos( + keytab_file=krb5_keytab_file, + keytab_principal=keytab_principal, + ) await config.load_dynamic_config( dynamic_config_git_url=dynamic_config_url, dynamic_config_file=dynamic_config_file, diff --git a/elnbuildsync/kojihelpers/builds.py b/elnbuildsync/kojihelpers/builds.py index 47a4572..cf8baa1 100644 --- a/elnbuildsync/kojihelpers/builds.py +++ b/elnbuildsync/kojihelpers/builds.py @@ -23,7 +23,7 @@ from twisted.internet.defer import DeferredList from .. import config -from .connection import call_koji, get_buildsys +from .connection import call_koji from .errors import InfoUnavailableError logger = logging.getLogger(__name__) @@ -59,10 +59,8 @@ async def get_buildinfo(build_id, **kwargs): :param build_id: The ID of the build (likely retrieved from a tagging message) :returns: A dictionary of information about the build """ - bsys = get_buildsys() - try: - buildinfo = await call_koji(bsys.getBuild, build_id, **kwargs) + buildinfo = await call_koji("getBuild", build_id, **kwargs) except koji.GenericError as e: logger.exception(f"Could not retrieve information for build {build_id}") raise InfoUnavailableError( @@ -72,8 +70,7 @@ async def get_buildinfo(build_id, **kwargs): return buildinfo -def _get_multi_buildinfo_thread(build_ids, **kwargs): - bsys = get_buildsys() +def _get_multi_buildinfo_thread(bsys, build_ids, **kwargs): build_vcalls = {} with bsys.multicall(batch=config.koji_batch) as mc: @@ -108,10 +105,8 @@ async def get_multi_buildinfo(build_ids, **kwargs): async def get_taskinfo(task_id, **kwargs): - bsys = get_buildsys() - try: - taskinfo = await call_koji(bsys.getTaskInfo, task_id, **kwargs) + taskinfo = await call_koji("getTaskInfo", task_id, **kwargs) except koji.GenericError as e: logger.exception(f"Could not retrieve information for task {task_id}") raise InfoUnavailableError( @@ -128,7 +123,7 @@ async def get_taskinfo(task_id, **kwargs): try: children = await call_koji( - bsys.getTaskChildren, task_id, request=True, strict=True + "getTaskChildren", task_id, request=True, strict=True ) except koji.GenericError as e: logger.exception(f"Could not retrieve child information for task {task_id}") @@ -148,7 +143,7 @@ async def get_taskinfo(task_id, **kwargs): for child in children: if child["method"] == "buildSRPMFromSCM": try: - child["result"] = await call_koji(bsys.getTaskResult, child["id"]) + child["result"] = await call_koji("getTaskResult", child["id"]) except koji.GenericError as e: raise InfoUnavailableError( f"SRPM build failed for {task_id}" @@ -176,8 +171,7 @@ async def start_builds(target, scm_urls, scratch=False, fail_fast=False): return task_index -def _start_builds_thread(target, scm_urls, scratch=False, fail_fast=False): - bsys = get_buildsys() +def _start_builds_thread(bsys, target, scm_urls, scratch=False, fail_fast=False): build_vcalls = {} try: with bsys.multicall(batch=config.koji_batch) as mc: @@ -234,8 +228,7 @@ async def wait_for_tasks(task_ids, timeout=config.task_timeout): async def cancel_task(task_id): logger.debug(f"Canceling task {task_id}") try: - bsys = get_buildsys() - await call_koji(bsys.cancelTask, task_id, recurse=True) + await call_koji("cancelTask", task_id, recurse=True) except Exception: # Cancellation is best-effort logger.exception("Could not cancel task %s. Ignoring.", task_id) diff --git a/elnbuildsync/kojihelpers/connection.py b/elnbuildsync/kojihelpers/connection.py index 5e499b5..45470a0 100644 --- a/elnbuildsync/kojihelpers/connection.py +++ b/elnbuildsync/kojihelpers/connection.py @@ -16,90 +16,296 @@ # SPDX-License-Identifier: GPL-3.0-or-later - import logging -import time +import os +import threading +import gssapi import koji -from cachetools import TTLCache, cached +from gssapi.raw import store_cred_into from requests.exceptions import RequestException -from tenacity import retry, retry_if_exception, stop_after_delay, wait_exponential +from tenacity import ( + retry, + retry_if_exception, + stop_after_attempt, + stop_after_delay, + wait_exponential, +) from twisted.internet.threads import deferToThread from .. import config -from .errors import KojiHelperBaseError +from .errors import BuildSysUnavailable, KerberosAuthError, KojiLoginError logger = logging.getLogger(__name__) +# Re-export for historical imports from this module. +__all__ = [ + "TGT_RENEW_THRESHOLD_SECONDS", + "BuildSysUnavailable", + "KerberosAuthError", + "KojiLoginError", + "call_koji", + "configure_kerberos", + "get_koji_url", + "resolve_krb5_keytab_principal", +] + +TGT_RENEW_THRESHOLD_SECONDS = 55 * 60 -class BuildSysUnavailable(KojiHelperBaseError): - pass +# koji.profile → Kerberos realm for automatic principal guessing +PROFILE_REALMS = { + "koji": "FEDORAPROJECT.ORG", + "stg": "STG.FEDORAPROJECT.ORG", +} +_bsys = None +_krb_creds = None +_krb5_principal = None +_krb5_keytab_file = None +_auth_lock = threading.Lock() -# Single cached session; TTL slightly less than an hour to be safe. -@cached(cache=TTLCache(maxsize=1, ttl=3550)) -def get_buildsys(): - """Get an authenticated koji build system session. Caches the session - so future calls are cheap. - :returns: Koji session object, or None on error +def resolve_krb5_keytab_principal(principal, koji_profile, koji_username): + """Return keytab principal for TGT acquisition, or guess from koji config. + + Only used when ``--krb5-keytab-file`` is set. Raises ValueError when + guessing is not possible. """ - if not config.main: - logger.critical("DistroBuildSync is not configured, aborting.") - raise BuildSysUnavailable + if principal: + return principal + realm = PROFILE_REALMS.get(koji_profile) + if realm is None: + raise ValueError( + f"Cannot guess Kerberos keytab principal for " + f"koji.profile={koji_profile!r}; " + "pass --krb5-keytab-principal explicitly" + ) + if not koji_username: + raise ValueError( + "koji.username is required to guess Kerberos keytab principal, " + "or pass --krb5-keytab-principal explicitly" + ) + return f"{koji_username}@{realm}" - profile = config.main["koji"]["profile"] + +def configure_kerberos(keytab_file=None, keytab_principal=None): + """Configure optional keytab-based TGT acquisition. + + When ``keytab_file`` is None, TGT acquisition is skipped and the existing + credential cache (``$KRB5CCNAME`` or the system default) is used as-is. + ``keytab_principal`` is only meaningful together with ``keytab_file``. + """ + global _krb5_principal, _krb5_keytab_file + _krb5_keytab_file = keytab_file + _krb5_principal = keytab_principal if keytab_file else None logger.debug( - 'Initializing the koji instance with the "%s" profile.', - profile, + "Kerberos configured: keytab=%s keytab_principal=%s", + keytab_file, + _krb5_principal, ) - bsys = None - while not bsys: + +def _principal_name(): + if not _krb5_principal: + raise KerberosAuthError("Kerberos keytab principal is not configured") + return gssapi.Name(_krb5_principal, gssapi.NameType.kerberos_principal) + + +def _ccache_store(): + ccache = os.environ.get("KRB5CCNAME") + if ccache: + return {"ccache": ccache} + return {} + + +def _tgt_lifetime_seconds(): + """Return remaining TGT lifetime in seconds (0 if missing/expired). + + Safe to call on the main thread (local credential cache read). + Uses ``$KRB5CCNAME`` when set, otherwise the system default ccache. + + Does not filter by ``_krb5_principal``: the active cache often holds a + different initiate principal (e.g. a personal ``kinit`` during local + testing) than the service principal used with a keytab. + """ + try: + store = _ccache_store() or None + if store: + creds = gssapi.Credentials(usage="initiate", store=store) + else: + creds = gssapi.Credentials(usage="initiate") + lifetime = creds.lifetime + if lifetime is None: + return TGT_RENEW_THRESHOLD_SECONDS + return max(0, int(lifetime)) + except Exception: + logger.debug("Could not read TGT lifetime", exc_info=True) + return 0 + + +def _store_creds(creds): + global _krb_creds + store = _ccache_store() + if store: try: - cfg = koji.read_config(profile_name=profile) - bsys = koji.ClientSession(cfg["server"], opts=cfg) + store_cred_into(store, creds, usage="initiate", overwrite=True) except Exception: - logger.exception( - 'Failed initializing the koji instance with the "%s" profile, skipping.', - profile, + logger.debug( + "store_cred_into failed; continuing with acquired creds", + exc_info=True, + ) + _krb_creds = creds + + +def _acquire_tgt_sync(): + """Acquire/renew a TGT from the configured keytab into the active ccache.""" + if not _krb5_keytab_file: + raise KerberosAuthError("No Kerberos keytab configured") + name = _principal_name() + store = {"client_keytab": _krb5_keytab_file} + store.update(_ccache_store()) + try: + creds = gssapi.Credentials(name=name, usage="initiate", store=store) + # Force acquisition / refresh by inspecting lifetime + _ = creds.lifetime + _store_creds(creds) + logger.info("Acquired Kerberos TGT for %s via keytab", _krb5_principal) + except Exception as e: + raise KerberosAuthError( + f"Failed to acquire TGT for {_krb5_principal} from keytab" + ) from e + + +def _recreate_bsys_sync(): + """Create a fresh Koji ClientSession (does not log in).""" + global _bsys + if not config.main: + raise BuildSysUnavailable("Configuration unavailable") + profile = config.main["koji"]["profile"] + try: + cfg = koji.read_config(profile_name=profile) + _bsys = koji.ClientSession(cfg["server"], opts=cfg) + logger.debug("Created new Koji ClientSession for profile %s", profile) + except Exception as e: + _bsys = None + raise BuildSysUnavailable( + f'Failed initializing koji with profile "{profile}"' + ) from e + + +def _renew_tgt_and_bsys_once(): + """One renew attempt: acquire TGT then recreate _bsys. + + Raises KerberosAuthError on failure. Does not leave a half-updated _bsys. + """ + global _bsys + old_bsys = _bsys + try: + _acquire_tgt_sync() + _recreate_bsys_sync() + except KerberosAuthError: + _bsys = old_bsys + raise + except Exception as e: + _bsys = old_bsys + raise KerberosAuthError("TGT acquire or Koji session recreate failed") from e + + +@retry( + wait=wait_exponential(), + stop=stop_after_attempt(5), + reraise=True, +) +def _renew_tgt_and_bsys_with_retries(): + _renew_tgt_and_bsys_once() + + +async def _ensure_tgt(): + """Ensure TGT is usable; renew from keytab (+ recreate _bsys) when needed. + + Without a configured keytab, never attempts acquisition: the existing + credential cache (``$KRB5CCNAME`` or system default) must already hold a TGT. + """ + lifetime = _tgt_lifetime_seconds() + if lifetime >= TGT_RENEW_THRESHOLD_SECONDS: + return + + if not _krb5_keytab_file: + if lifetime == 0: + raise KerberosAuthError( + "No Kerberos TGT available in the credential cache " + "($KRB5CCNAME or system default) and no --krb5-keytab-file " + "configured for acquisition" ) - bsys = None - time.sleep(1) - logger.debug("The koji instance initialized.") + logger.debug( + "TGT lifetime %ss is below renew threshold but no keytab " + "configured; using existing credential cache", + lifetime, + ) + return - logger.debug("Authenticating with the koji instance.") - while not bsys.logged_in: + if lifetime > 0: try: - bsys.logout() - bsys.gssapi_login() - except koji.GSSAPIAuthError: + await deferToThread(_renew_tgt_and_bsys_once) + except KerberosAuthError: logger.exception( - "Failed authenticating against the koji instance, retrying." + "TGT renew failed; continuing with existing ticket (lifetime=%ss)", + lifetime, ) - time.sleep(1) - continue + return - username = bsys.getLoggedInUser()["name"] - logger.debug( - "Successfully authenticated with the koji instance as user %s", - username, - ) + await deferToThread(_renew_tgt_and_bsys_with_retries) - return bsys +def _ensure_bsys_sync(): + if _bsys is None: + _recreate_bsys_sync() -def get_koji_url(): - cfg = koji.read_config(profile_name=config.main["koji"]["profile"]) - return cfg["weburl"] + +def _ensure_logged_in_sync(): + if _bsys is None: + raise BuildSysUnavailable("Koji session is not initialized") + if getattr(_bsys, "logged_in", False): + return + try: + _bsys.gssapi_login() + except koji.GSSAPIAuthError as e: + # Session is already authenticated; gssapi_login is idempotent for us. + if "Already logged in" in str(e): + return + raise KojiLoginError("Koji GSSAPI login failed") from e + except Exception as e: + raise KojiLoginError("Koji GSSAPI login failed") from e + + +def _invoke_koji_sync(method, args, kwargs): + with _auth_lock: + _ensure_bsys_sync() + _ensure_logged_in_sync() + if isinstance(method, str): + return getattr(_bsys, method)(*args, **kwargs) + return method(_bsys, *args, **kwargs) # HTTP 4xx codes that are still worth retrying (transient client/server behavior). _RETRYABLE_4XX = frozenset((408, 429)) +# Auth / session setup failures are not transient; do not burn retry budget on them. +_NON_RETRYABLE_KOJI = (KerberosAuthError, KojiLoginError, BuildSysUnavailable) + + +async def _reactor_sleep(seconds: float) -> None: + """Sleep via the Twisted reactor (safe under asyncioreactor + tenacity).""" + from twisted.internet import reactor + from twisted.internet.task import deferLater + + await deferLater(reactor, seconds) + def _retry_koji_request_exception(exc: BaseException) -> bool: """Do not retry most HTTP 4xx responses; still retry 408 and 429.""" + if isinstance(exc, _NON_RETRYABLE_KOJI): + return False if not isinstance(exc, RequestException): return False resp = getattr(exc, "response", None) @@ -111,17 +317,35 @@ def _retry_koji_request_exception(exc: BaseException) -> bool: return not (400 <= code < 500) -# Wrap the call to koji in retries for transient errors; give up on 4xx except 408/429. +def _retry_transient_koji_exception(exc: BaseException) -> bool: + """Retry unexpected transient failures; never retry auth/session errors.""" + return not isinstance(exc, _NON_RETRYABLE_KOJI) + + +def get_koji_url(): + cfg = koji.read_config(profile_name=config.main["koji"]["profile"]) + return cfg["weburl"] + + @retry( wait=wait_exponential(), stop=stop_after_delay(60), retry=retry_if_exception(_retry_koji_request_exception), + sleep=_reactor_sleep, reraise=True, ) @retry( wait=wait_exponential(), stop=stop_after_delay(60), + retry=retry_if_exception(_retry_transient_koji_exception), + sleep=_reactor_sleep, reraise=True, ) async def call_koji(method, *args, **kwargs): - return await deferToThread(method, *args, **kwargs) + """Authenticate as needed and invoke a Koji method or helper in a worker thread. + + ``method`` may be a string attribute name on the private session, or a + callable that receives the session as its first argument. + """ + await _ensure_tgt() + return await deferToThread(_invoke_koji_sync, method, args, kwargs) diff --git a/elnbuildsync/kojihelpers/errors.py b/elnbuildsync/kojihelpers/errors.py index a671ab4..71207e3 100644 --- a/elnbuildsync/kojihelpers/errors.py +++ b/elnbuildsync/kojihelpers/errors.py @@ -21,6 +21,18 @@ class KojiHelperBaseError(Exception): pass +class KerberosAuthError(KojiHelperBaseError): + """Kerberos TGT acquire/renew failed.""" + + +class KojiLoginError(KojiHelperBaseError): + """Koji GSSAPI login failed.""" + + +class BuildSysUnavailable(KojiHelperBaseError): + """Koji ClientSession could not be created or is unavailable.""" + + class InfoUnavailableError(KojiHelperBaseError): pass diff --git a/elnbuildsync/kojihelpers/tags.py b/elnbuildsync/kojihelpers/tags.py index f21c92e..4a12f8c 100644 --- a/elnbuildsync/kojihelpers/tags.py +++ b/elnbuildsync/kojihelpers/tags.py @@ -19,10 +19,11 @@ import logging from cachetools import LRUCache, cached +from cachetools.keys import hashkey from twisted.internet.defer import DeferredList from .. import config, kojihelpers -from .connection import call_koji, get_buildsys +from .connection import call_koji logger = logging.getLogger(__name__) @@ -42,10 +43,9 @@ async def prepare_side_tag(base_tag, initial_build_ids=None): if initial_build_ids is None: initial_build_ids = [] - downstream_koji = get_buildsys() # Trigger the creation of the side-tag logger.info(f"Creating side tag from {base_tag}") - side_tag_info = await call_koji(downstream_koji.createSideTag, base_tag) + side_tag_info = await call_koji("createSideTag", base_tag) side_tag_name = side_tag_info["name"] logger.debug(f"Side {side_tag_name} created.") @@ -90,7 +90,7 @@ async def tag_builds(tag, build_ids): return task_index -def _tag_builds_thread(tag, build_ids): +def _tag_builds_thread(bsys, tag, build_ids): """ Tag a list of nvrs into a tag. @@ -98,11 +98,10 @@ def _tag_builds_thread(tag, build_ids): :params list build_ids: The list of nvrs or build IDs to tag :return dict: A dictionary of task_id -> Koji vcall """ - downstream_koji = get_buildsys() build_vcalls = {} try: - with downstream_koji.multicall(batch=config.koji_batch) as mc: + with bsys.multicall(batch=config.koji_batch) as mc: logger.info(f"Tagging {len(build_ids)} builds into {tag}") for build_id in build_ids: build_vcalls[build_id] = mc.tagBuild(tag, build_id) @@ -124,10 +123,8 @@ async def untag_builds(tag, builds): logger.debug(f"Untagged {len(builds)} builds from {tag}") -def _untag_builds_thread(tag, build_ids): - downstream_koji = get_buildsys() - - with downstream_koji.multicall(batch=config.koji_batch) as mc: +def _untag_builds_thread(bsys, tag, build_ids): + with bsys.multicall(batch=config.koji_batch) as mc: logger.info(f"Untagging {len(build_ids)} builds from {tag}") for build_id in build_ids: mc.untagBuild(tag, build_id, strict=False) @@ -145,9 +142,8 @@ async def get_tags_for_target(target): return buildroot_tag, destination_tag -@cached(cache=LRUCache(maxsize=4)) -def _get_tags_for_target_thread(target): - bsys = kojihelpers.connection.get_buildsys() +@cached(cache=LRUCache(maxsize=4), key=lambda bsys, target: hashkey(target)) +def _get_tags_for_target_thread(bsys, target): targetinfo = bsys.getBuildTarget(target) logger.debug(f"Target info: {targetinfo}") return targetinfo["build_tag_name"], targetinfo["dest_tag_name"] @@ -157,8 +153,7 @@ async def remove_side_tag(side_tag): await call_koji(_remove_side_tag_thread, side_tag) -def _remove_side_tag_thread(side_tag): - bsys = kojihelpers.connection.get_buildsys() +def _remove_side_tag_thread(bsys, side_tag): bsys.removeSideTag(side_tag) @@ -191,6 +186,5 @@ async def get_nvrs_from_tag(tag): :params str tag: The tag name to get builds from :return dict: A dictionary of nvr -> buildinfo """ - bsys = kojihelpers.connection.get_buildsys() - builds = await call_koji(bsys.listTagged, tag, latest=False, inherit=True) + builds = await call_koji("listTagged", tag, latest=False, inherit=True) return {build["nvr"]: build for build in builds} diff --git a/elnbuildsync/status.py b/elnbuildsync/status.py index da6067e..039bea6 100644 --- a/elnbuildsync/status.py +++ b/elnbuildsync/status.py @@ -57,14 +57,12 @@ async def create_status_page(): ) ] - bsys = kojihelpers.connection.get_buildsys() - # Use the configured username, or self-identify if not configured username = config.main["koji"].get("username", None) if username is None: try: # Self-identify - username = bsys.getLoggedInUser()["name"] + username = (await call_koji("getLoggedInUser"))["name"] except koji.GenericError: logger.exception( "Could not self-identify with Koji. Will retry in a few minutes." @@ -77,7 +75,7 @@ async def create_status_page(): try: # Look up packages tagged into the stable tag tagged_pkgs = await call_koji( - bsys.listTagged, config.main["koji"]["stable_tag"], latest=True + "listTagged", config.main["koji"]["stable_tag"], latest=True ) except koji.GenericError: logger.exception( @@ -93,7 +91,7 @@ async def create_status_page(): # Get the list of packages that DBS has built. built_packages = await call_koji( - bsys.listBuilds, userID=username, queryOpts={"order": "start_ts"} + "listBuilds", userID=username, queryOpts={"order": "start_ts"} ) for build in built_packages: if build["start_ts"] is not None: @@ -107,7 +105,7 @@ async def create_status_page(): # Check whether the package was built by another user builds = await call_koji( - bsys.listBuilds, packageID=pname, queryOpts={"order": "start_ts"} + "listBuilds", packageID=pname, queryOpts={"order": "start_ts"} ) for build in builds: # The ordering oddly puts "None" at the end, so we need to diff --git a/requirements.txt b/requirements.txt index 6540774..83d77a9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,6 +4,7 @@ cachetools click >= 8 fedora_messaging >= 3.7.1, < 4 GitPython +gssapi koji pyyaml requests diff --git a/run.sh b/run.sh index d28547f..aea04a4 100755 --- a/run.sh +++ b/run.sh @@ -24,9 +24,8 @@ # ARG_OPTIONAL_SINGLE([dynamic-config-file],[],[Dynamic configuration file]) # ARG_OPTIONAL_SINGLE([dynamic-config-url],[],[Dynamic configuration Git URL],[https://github.com/fedora-eln/elnbuildsync-config.git]) # ARG_OPTIONAL_SINGLE([dynamic-config-branch],[],[Dynamic configuration Git branch],[main]) -# ARG_OPTIONAL_SINGLE([keytab-principal],[],[Keytab principal],[eln-buildsync@FEDORAPROJECT.ORG]) -# ARG_OPTIONAL_SINGLE([keytab-file],[],[Keytab file],[]) -# ARG_OPTIONAL_SINGLE([koji-profile],[],[Koji profile],[koji]) +# ARG_OPTIONAL_SINGLE([krb5-keytab-file],[],[Kerberos keytab for in-process TGT acquisition (optional)]) +# ARG_OPTIONAL_SINGLE([krb5-keytab-principal],[],[Kerberos principal for keytab kinit only (default: guessed from static configuration)]) # ARG_OPTIONAL_SINGLE([openid-ca-file],[],[OIDC CA certificate file],[]) # ARG_POSITIONAL_DOUBLEDASH([]) # ARG_POSITIONAL_INF([custom],[Additional arguments to pass to the ELNBuildSync daemon]) @@ -63,24 +62,22 @@ _arg_static_config_file="/etc/elnbuildsync/static-config/elnbuildsync.yaml" _arg_dynamic_config_file= _arg_dynamic_config_url="https://github.com/fedora-eln/elnbuildsync-config.git" _arg_dynamic_config_branch="main" -_arg_keytab_principal="eln-buildsync@FEDORAPROJECT.ORG" -_arg_keytab_file= -_arg_koji_profile="koji" +_arg_krb5_keytab_file= +_arg_krb5_keytab_principal= _arg_openid_ca_file= print_help() { - printf 'Usage: %s [--log-level ] [--static-config-file ] [--dynamic-config-file ] [--dynamic-config-url ] [--dynamic-config-branch ] [--keytab-principal ] [--keytab-file ] [--koji-profile ] [--openid-ca-file ] [-h|--help] [--] [] ... [] ...\n' "$0" + printf 'Usage: %s [--log-level ] [--static-config-file ] [--dynamic-config-file ] [--dynamic-config-url ] [--dynamic-config-branch ] [--krb5-keytab-file ] [--krb5-keytab-principal ] [--openid-ca-file ] [-h|--help] [--] [] ... [] ...\n' "$0" printf '\t%s\n' ": Additional arguments to pass to the ELNBuildSync daemon" printf '\t%s\n' "--log-level: Log verbosity (default: 'INFO')" printf '\t%s\n' "--static-config-file: Static configuration file (default: '/etc/elnbuildsync/static-config/elnbuildsync.yaml')" printf '\t%s\n' "--dynamic-config-file: Dynamic configuration file (no default)" printf '\t%s\n' "--dynamic-config-url: Dynamic configuration Git URL (default: 'https://github.com/fedora-eln/elnbuildsync-config.git')" printf '\t%s\n' "--dynamic-config-branch: Dynamic configuration Git branch (default: 'main')" - printf '\t%s\n' "--keytab-principal: Keytab principal (default: 'eln-buildsync@FEDORAPROJECT.ORG')" - printf '\t%s\n' "--keytab-file: Keytab file (no default)" - printf '\t%s\n' "--koji-profile: Koji profile (default: 'koji')" + printf '\t%s\n' "--krb5-keytab-file: Kerberos keytab for in-process TGT acquisition (optional; otherwise use existing ccache)" + printf '\t%s\n' "--krb5-keytab-principal: Principal for keytab kinit only (default: guessed from static configuration when a keytab is set)" printf '\t%s\n' "--openid-ca-file: OIDC CA certificate file (no default)" printf '\t%s\n' "-h, --help: Prints help" printf '\n%s\n' "Run the ELNBuildSync daemon" @@ -145,29 +142,21 @@ parse_commandline() --dynamic-config-branch=*) _arg_dynamic_config_branch="${_key##--dynamic-config-branch=}" ;; - --keytab-principal) + --krb5-keytab-file) test $# -lt 2 && die "Missing value for the optional argument '$_key'." 1 - _arg_keytab_principal="$2" + _arg_krb5_keytab_file="$2" shift ;; - --keytab-principal=*) - _arg_keytab_principal="${_key##--keytab-principal=}" + --krb5-keytab-file=*) + _arg_krb5_keytab_file="${_key##--krb5-keytab-file=}" ;; - --keytab-file) + --krb5-keytab-principal) test $# -lt 2 && die "Missing value for the optional argument '$_key'." 1 - _arg_keytab_file="$2" + _arg_krb5_keytab_principal="$2" shift ;; - --keytab-file=*) - _arg_keytab_file="${_key##--keytab-file=}" - ;; - --koji-profile) - test $# -lt 2 && die "Missing value for the optional argument '$_key'." 1 - _arg_koji_profile="$2" - shift - ;; - --koji-profile=*) - _arg_koji_profile="${_key##--koji-profile=}" + --krb5-keytab-principal=*) + _arg_krb5_keytab_principal="${_key##--krb5-keytab-principal=}" ;; --openid-ca-file) test $# -lt 2 && die "Missing value for the optional argument '$_key'." 1 @@ -228,21 +217,15 @@ set -eo pipefail SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" export TMPDIR=/var/tmp -if [ -n "${_arg_keytab_file}" ]; then - # If we have a keytab, use it to get a Kerberos TGT - # Otherwise, this is probably being run locally for testing and the - # host KCM configuration will be used (see local_test_daemon.sh). +# When acquiring via keytab, use a shared file ccache unless one is already set. +# Without a keytab, leave $KRB5CCNAME alone (existing TGT / KCM / system default). +if [ -n "${_arg_krb5_keytab_principal}" ] && [ -z "${_arg_krb5_keytab_file}" ]; then + die "--krb5-keytab-principal requires --krb5-keytab-file" 1 +fi +if [ -n "${_arg_krb5_keytab_file}" ] && [ -z "${KRB5CCNAME:-}" ]; then export KRB5CCNAME=FILE:${TMPDIR}/tgt - - echo "Getting Kerberos TGT every hour from ${_arg_keytab_file} for ${_arg_keytab_principal}" - (while true; do kinit -k -t "${_arg_keytab_file}" ${_arg_keytab_principal}; sleep 55m; done) & fi -# Make sure Kerberos is working by trying to connect to koji -while true; do - koji -p ${_arg_koji_profile} hello && break || sleep 3 -done - STATIC_ARG="--static-config-file /etc/elnbuildsync/static-config/elnbuildsync.yaml" if [ -n "${_arg_static_config_file}" ]; then STATIC_ARG="--static-config-file ${_arg_static_config_file}" @@ -286,8 +269,16 @@ if [ -n "${_arg_openid_ca_file}" ]; then OPENID_CA_ARG=(--openid-ca-file "${_arg_openid_ca_file}") fi +KRB5_ARGS=() +if [ -n "${_arg_krb5_keytab_file}" ]; then + KRB5_ARGS+=(--krb5-keytab-file "${_arg_krb5_keytab_file}") +fi +if [ -n "${_arg_krb5_keytab_principal}" ]; then + KRB5_ARGS+=(--krb5-keytab-principal "${_arg_krb5_keytab_principal}") +fi + echo "EXECUTING klist" -KRB5_TRACE=/dev/stderr klist -A +KRB5_TRACE=/dev/stderr klist -A || true python3 --version @@ -308,6 +299,7 @@ elnbuildsync \ $SMTP_ARG \ $OIDC_ARG \ "${OPENID_CA_ARG[@]}" \ + "${KRB5_ARGS[@]}" \ ${_arg_custom[@]} # ] <-- needed because of Argbash diff --git a/tests/local_test_daemon.sh b/tests/local_test_daemon.sh index 6540a86..650ee3e 100755 --- a/tests/local_test_daemon.sh +++ b/tests/local_test_daemon.sh @@ -28,6 +28,8 @@ # ARG_OPTIONAL_SINGLE([static-config-file],[],[Static configuration file],[tests/etc/static-config/elnbuildsync.yaml]) # ARG_OPTIONAL_SINGLE([dynamic-config-file],[],[Dynamic configuration file],[tests/etc/dynamic-config/elnbuildsync_dynamic.yaml]) # ARG_OPTIONAL_SINGLE([environment],[],[Environment],[stg]) +# ARG_OPTIONAL_SINGLE([krb5-keytab-file],[],[Kerberos keytab for in-process TGT acquisition (optional)]) +# ARG_OPTIONAL_SINGLE([krb5-keytab-principal],[],[Kerberos principal for keytab kinit only (default: guessed from static configuration)]) # ARG_OPTIONAL_BOOLEAN([persistent-db],[],[Use persistent database],[off]) # ARG_OPTIONAL_SINGLE([persistent-db-volume],[],[Volume name for persistent database],[ebs_test_pgdata]) # ARG_OPTIONAL_BOOLEAN([build-container],[],[Build the ELNBuildSync container],[off]) @@ -70,6 +72,8 @@ _arg_lull_time="5" _arg_static_config_file="tests/etc/static-config/elnbuildsync.yaml" _arg_dynamic_config_file="tests/etc/dynamic-config/elnbuildsync_dynamic.yaml" _arg_environment="stg" +_arg_krb5_keytab_file= +_arg_krb5_keytab_principal= _arg_persistent_db="off" _arg_persistent_db_volume="ebs_test_pgdata" _arg_build_container="off" @@ -77,7 +81,7 @@ _arg_build_container="off" print_help() { - printf 'Usage: %s [--log-level ] [--db-pw-file ] [--smtp-pw-file ] [--openid-client-secret-file ] [--openid-ca-file ] [--lull-time ] [--static-config-file ] [--dynamic-config-file ] [--environment ] [--(no-)persistent-db] [--persistent-db-volume ] [--(no-)build-container] [-h|--help] [--] [] ... [] ...\n' "$0" + printf 'Usage: %s [--log-level ] [--db-pw-file ] [--smtp-pw-file ] [--openid-client-secret-file ] [--openid-ca-file ] [--lull-time ] [--static-config-file ] [--dynamic-config-file ] [--environment ] [--krb5-keytab-file ] [--krb5-keytab-principal ] [--(no-)persistent-db] [--persistent-db-volume ] [--(no-)build-container] [-h|--help] [--] [] ... [] ...\n' "$0" printf '\t%s\n' ": Additional arguments to pass to the ELNBuildSync daemon" printf '\t%s\n' "--log-level: Log verbosity (default: 'INFO')" printf '\t%s\n' "--db-pw-file: Database password file (default: 'tests/etc/secrets/ebs_db_pw')" @@ -88,6 +92,8 @@ print_help() printf '\t%s\n' "--static-config-file: Static configuration file (default: 'tests/etc/static-config/elnbuildsync.yaml')" printf '\t%s\n' "--dynamic-config-file: Dynamic configuration file (default: 'tests/etc/dynamic-config/elnbuildsync_dynamic.yaml')" printf '\t%s\n' "--environment: Environment (default: 'stg')" + printf '\t%s\n' "--krb5-keytab-file: Kerberos keytab for in-process TGT acquisition (optional; otherwise use host KCM / existing ccache)" + printf '\t%s\n' "--krb5-keytab-principal: Principal for keytab kinit only (default: guessed from static configuration when a keytab is set)" printf '\t%s\n' "--persistent-db, --no-persistent-db: Use persistent database (off by default)" printf '\t%s\n' "--persistent-db-volume: Volume name for persistent database (default: 'ebs_test_pgdata')" printf '\t%s\n' "--build-container, --no-build-container: Build the ELNBuildSync container (off by default)" @@ -186,6 +192,22 @@ parse_commandline() --environment=*) _arg_environment="${_key##--environment=}" ;; + --krb5-keytab-file) + test $# -lt 2 && die "Missing value for the optional argument '$_key'." 1 + _arg_krb5_keytab_file="$2" + shift + ;; + --krb5-keytab-file=*) + _arg_krb5_keytab_file="${_key##--krb5-keytab-file=}" + ;; + --krb5-keytab-principal) + test $# -lt 2 && die "Missing value for the optional argument '$_key'." 1 + _arg_krb5_keytab_principal="$2" + shift + ;; + --krb5-keytab-principal=*) + _arg_krb5_keytab_principal="${_key##--krb5-keytab-principal=}" + ;; --no-persistent-db|--persistent-db) _arg_persistent_db="on" test "${1:0:5}" = "--no-" && _arg_persistent_db="off" @@ -325,10 +347,8 @@ if [ $db_ready -ne 0 ]; then fi if [ "$_arg_environment" == "stg" ]; then - KOJI_PROFILE_ARG=(--koji-profile=stg) export FEDORA_MESSAGING_CONF="$SCRIPT_DIR/fedora-messaging/fedora.stg.toml" else - KOJI_PROFILE_ARG=(--koji-profile=koji) export FEDORA_MESSAGING_CONF="$SCRIPT_DIR/fedora-messaging/fedora.toml" fi @@ -362,6 +382,19 @@ if [ "${_arg_dynamic_config_file}" != "${DEFAULT_DYNAMIC_CONFIG_FILE}" ]; then CUSTOM_MOUNT_ARGS+=(--volume "$(realpath "${_arg_dynamic_config_file}"):${CONTAINER_DYNAMIC_CONFIG}:ro,Z") fi +KRB5_CONTAINER_ARG=() +if [ -n "${_arg_krb5_keytab_principal}" ] && [ -z "${_arg_krb5_keytab_file}" ]; then + die "--krb5-keytab-principal requires --krb5-keytab-file" 1 +fi +if [ -n "${_arg_krb5_keytab_file}" ]; then + CONTAINER_KRB5_KEYTAB_FILE="/etc/elnbuildsync/custom/krb5.keytab" + CUSTOM_MOUNT_ARGS+=(--volume "$(realpath "${_arg_krb5_keytab_file}"):${CONTAINER_KRB5_KEYTAB_FILE}:ro,Z") + KRB5_CONTAINER_ARG+=(--krb5-keytab-file "${CONTAINER_KRB5_KEYTAB_FILE}") +fi +if [ -n "${_arg_krb5_keytab_principal}" ]; then + KRB5_CONTAINER_ARG+=(--krb5-keytab-principal "${_arg_krb5_keytab_principal}") +fi + ${CONTAINER_ENGINE} run --rm --interactive --tty \ --name ebs_test \ --publish 8080:8080 \ @@ -377,12 +410,12 @@ ${CONTAINER_ENGINE} run --rm --interactive --tty \ --volume "${PROJ_DIR}:/tmp:Z" \ localhost/elnbuildsync:local_test_daemon \ --log-level "$_arg_log_level" \ - "${KOJI_PROFILE_ARG[@]}" \ --static-config-file "${CONTAINER_STATIC_CONFIG}" \ --dynamic-config-file "${CONTAINER_DYNAMIC_CONFIG}" \ --lull-time "$_arg_lull_time" \ --openid-client-secret-file "${CONTAINER_OIDC_CLIENT_SECRET}" \ "${OPENID_CA_CONTAINER_ARG[@]}" \ + "${KRB5_CONTAINER_ARG[@]}" \ "${_arg_custom[@]}" \ 2>&1 | tee /tmp/elnbuildsync.log diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py new file mode 100644 index 0000000..36b9ed4 --- /dev/null +++ b/tests/test_koji_connection.py @@ -0,0 +1,298 @@ +# This file is part of ELNBuildSync +# Copyright (C) 2026 Stephen Gallagher + +# SPDX-License-Identifier: GPL-3.0-or-later + +from unittest.mock import MagicMock, patch + +import pytest +from twisted.internet.defer import succeed + +from elnbuildsync.kojihelpers import connection as conn +from elnbuildsync.kojihelpers.errors import KerberosAuthError + + +def _defer_immediately(f, *args, **kwargs): + return succeed(f(*args, **kwargs)) + + +@pytest.fixture(autouse=True) +def _reset_connection_state(): + conn._bsys = None + conn._krb_creds = None + conn._krb5_principal = None + conn._krb5_keytab_file = None + yield + conn._bsys = None + conn._krb_creds = None + conn._krb5_principal = None + conn._krb5_keytab_file = None + + +class TestResolveKrb5KeytabPrincipal: + def test_explicit_principal(self): + assert ( + conn.resolve_krb5_keytab_principal("user@REALM", "koji", "ignored") + == "user@REALM" + ) + + def test_guess_koji_profile(self): + assert ( + conn.resolve_krb5_keytab_principal(None, "koji", "eln-buildsync") + == "eln-buildsync@FEDORAPROJECT.ORG" + ) + + def test_guess_stg_profile(self): + assert ( + conn.resolve_krb5_keytab_principal(None, "stg", "eln-buildsync") + == "eln-buildsync@STG.FEDORAPROJECT.ORG" + ) + + def test_unknown_profile_requires_explicit(self): + with pytest.raises(ValueError, match="Cannot guess"): + conn.resolve_krb5_keytab_principal(None, "custom", "user") + + def test_missing_username_when_guessing(self): + with pytest.raises(ValueError, match="koji.username"): + conn.resolve_krb5_keytab_principal(None, "koji", None) + + +class TestTgtLifetime: + def test_reads_ccache_without_principal_name(self): + """Lifetime must not require configured principal to match ccache.""" + mock_creds = MagicMock() + mock_creds.lifetime = 3600 + conn.configure_kerberos( + keytab_file="/kt", + keytab_principal="eln-buildsync@FEDORAPROJECT.ORG", + ) + with ( + patch.dict("os.environ", {"KRB5CCNAME": "KCM:"}, clear=False), + patch( + "elnbuildsync.kojihelpers.connection.gssapi.Credentials", + return_value=mock_creds, + ) as creds_cls, + ): + assert conn._tgt_lifetime_seconds() == 3600 + kwargs = creds_cls.call_args.kwargs + assert "name" not in kwargs + assert kwargs["usage"] == "initiate" + assert kwargs["store"] == {"ccache": "KCM:"} + + +class TestRenewUnit: + def test_renew_acquires_tgt_then_recreates_bsys(self): + with ( + patch.object(conn, "_acquire_tgt_sync") as acquire, + patch.object(conn, "_recreate_bsys_sync") as recreate, + ): + conn._renew_tgt_and_bsys_once() + acquire.assert_called_once() + recreate.assert_called_once() + + def test_renew_failure_restores_old_bsys(self): + old = MagicMock(name="old_bsys") + conn._bsys = old + with ( + patch.object( + conn, "_acquire_tgt_sync", side_effect=KerberosAuthError("boom") + ), + pytest.raises(KerberosAuthError), + ): + conn._renew_tgt_and_bsys_once() + assert conn._bsys is old + + def test_recreate_failure_wrapped_as_kerberos_auth_error(self): + old = MagicMock(name="old_bsys") + conn._bsys = old + with ( + patch.object(conn, "_acquire_tgt_sync"), + patch.object( + conn, "_recreate_bsys_sync", side_effect=RuntimeError("session") + ), + pytest.raises(KerberosAuthError, match="recreate failed"), + ): + conn._renew_tgt_and_bsys_once() + assert conn._bsys is old + + def test_retry_wrapper_attempts_five_times(self): + stop = conn._renew_tgt_and_bsys_with_retries.retry.stop + assert stop.max_attempt_number == 5 + + +@pytest.mark.asyncio +class TestEnsureTgt: + async def test_skips_renew_when_lifetime_at_threshold(self): + conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + with ( + patch.object( + conn, + "_tgt_lifetime_seconds", + return_value=conn.TGT_RENEW_THRESHOLD_SECONDS, + ), + patch.object(conn, "_renew_tgt_and_bsys_once") as renew, + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + ): + await conn._ensure_tgt() + renew.assert_not_called() + + async def test_renews_when_under_threshold_with_keytab(self): + conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + with ( + patch.object(conn, "_tgt_lifetime_seconds", return_value=60), + patch.object(conn, "_renew_tgt_and_bsys_once") as renew, + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + ): + await conn._ensure_tgt() + renew.assert_called_once() + + async def test_no_keytab_uses_existing_ccache_when_lifetime_positive(self): + conn.configure_kerberos(keytab_file=None) + with ( + patch.object(conn, "_tgt_lifetime_seconds", return_value=60), + patch.object(conn, "_renew_tgt_and_bsys_once") as renew, + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + ): + await conn._ensure_tgt() + renew.assert_not_called() + + async def test_no_keytab_raises_when_no_tgt(self): + conn.configure_kerberos(keytab_file=None) + with ( + patch.object(conn, "_tgt_lifetime_seconds", return_value=0), + pytest.raises(KerberosAuthError, match="No Kerberos TGT available"), + ): + await conn._ensure_tgt() + + async def test_soft_fail_logs_and_keeps_bsys(self): + conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + old = MagicMock(name="old_bsys") + conn._bsys = old + with ( + patch.object(conn, "_tgt_lifetime_seconds", return_value=120), + patch.object( + conn, + "_renew_tgt_and_bsys_once", + side_effect=KerberosAuthError("renew failed"), + ), + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + patch.object(conn.logger, "exception") as log_exc, + ): + await conn._ensure_tgt() + assert conn._bsys is old + log_exc.assert_called_once() + + async def test_zero_lifetime_uses_retry_wrapper(self): + conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + with ( + patch.object(conn, "_tgt_lifetime_seconds", return_value=0), + patch.object(conn, "_renew_tgt_and_bsys_with_retries") as retries, + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + ): + await conn._ensure_tgt() + retries.assert_called_once() + + async def test_zero_lifetime_reraises_after_retries(self): + conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + with ( + patch.object(conn, "_tgt_lifetime_seconds", return_value=0), + patch.object( + conn, + "_renew_tgt_and_bsys_with_retries", + side_effect=KerberosAuthError("no ticket"), + ), + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + pytest.raises(KerberosAuthError, match="no ticket"), + ): + await conn._ensure_tgt() + + +def test_auth_errors_are_not_retried(): + assert not conn._retry_transient_koji_exception(KerberosAuthError("x")) + assert not conn._retry_koji_request_exception(KerberosAuthError("x")) + + +def test_ensure_logged_in_skips_when_already_logged_in_flag(): + mock_bsys = MagicMock() + mock_bsys.logged_in = True + conn._bsys = mock_bsys + conn._ensure_logged_in_sync() + mock_bsys.gssapi_login.assert_not_called() + + +def test_ensure_logged_in_ignores_already_logged_in_error(): + import koji + + mock_bsys = MagicMock() + mock_bsys.logged_in = False + mock_bsys.gssapi_login.side_effect = koji.GSSAPIAuthError( + "unable to obtain a session (gssapi auth failed: koji.AuthError: Already logged in)" + ) + conn._bsys = mock_bsys + conn._ensure_logged_in_sync() + mock_bsys.gssapi_login.assert_called_once() + + +@pytest.mark.asyncio +async def test_call_koji_string_method(): + mock_bsys = MagicMock() + mock_bsys.listTagged.return_value = [{"nvr": "pkg-1"}] + conn._bsys = mock_bsys + conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + + with ( + patch.object(conn, "_ensure_tgt"), + patch.object(conn, "_ensure_logged_in_sync"), + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + ): + # Bypass outer tenacity by calling the undecorated body via wrap + result = await conn.call_koji.__wrapped__.__wrapped__( + "listTagged", "tag", latest=True + ) + + assert result == [{"nvr": "pkg-1"}] + mock_bsys.listTagged.assert_called_once_with("tag", latest=True) + + +@pytest.mark.asyncio +async def test_call_koji_callable_receives_bsys(): + mock_bsys = MagicMock() + conn._bsys = mock_bsys + conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + + def helper(bsys, tag, ids): + assert bsys is mock_bsys + return (tag, ids) + + with ( + patch.object(conn, "_ensure_tgt"), + patch.object(conn, "_ensure_logged_in_sync"), + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + ): + result = await conn.call_koji.__wrapped__.__wrapped__(helper, "mytag", [1, 2]) + + assert result == ("mytag", [1, 2]) From e59a4e39bc87cdeb9c52ad0e79f072a8b2da3c29 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:38:13 -0400 Subject: [PATCH 02/11] tests: normalize SPDX header spacing in test_koji_connection.py --- tests/test_koji_connection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py index 36b9ed4..72f0de7 100644 --- a/tests/test_koji_connection.py +++ b/tests/test_koji_connection.py @@ -1,7 +1,7 @@ # This file is part of ELNBuildSync # Copyright (C) 2026 Stephen Gallagher -# SPDX-License-Identifier: GPL-3.0-or-later +# SPDX-License-Identifier: GPL-3.0-or-later from unittest.mock import MagicMock, patch From 4c425d91e9ace8eca78937550f0658bba9b6f6ea Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:38:14 -0400 Subject: [PATCH 03/11] docs: replace keytab kinit wording with keytab-based TGT acquisition --- README.md | 4 ++-- run.sh | 4 ++-- tests/local_test_daemon.sh | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index dd81646..4ae40da 100644 --- a/README.md +++ b/README.md @@ -368,8 +368,8 @@ SMTP, and OIDC client secrets mount at `/etc/elnbuildsync/secrets/` is handled in-process with python-gssapi: optionally pass `--krb5-keytab-file` for TGT acquisition and `--krb5-keytab-principal` (or rely on guessing `koji.username` plus realm from `koji.profile`) for -that keytab kinit only. Without a keytab, the daemon uses an existing TGT -from `$KRB5CCNAME` or the system default ccache. OpenShift +that keytab-based TGT acquisition only. Without a keytab, the daemon uses +an existing TGT from `$KRB5CCNAME` or the system default ccache. OpenShift deployment is managed via [infra-ansible](https://forge.fedoraproject.org/infra/ansible) (`playbooks/openshift-apps/elnbuildsync.yml`). diff --git a/run.sh b/run.sh index aea04a4..c47402a 100755 --- a/run.sh +++ b/run.sh @@ -25,7 +25,7 @@ # ARG_OPTIONAL_SINGLE([dynamic-config-url],[],[Dynamic configuration Git URL],[https://github.com/fedora-eln/elnbuildsync-config.git]) # ARG_OPTIONAL_SINGLE([dynamic-config-branch],[],[Dynamic configuration Git branch],[main]) # ARG_OPTIONAL_SINGLE([krb5-keytab-file],[],[Kerberos keytab for in-process TGT acquisition (optional)]) -# ARG_OPTIONAL_SINGLE([krb5-keytab-principal],[],[Kerberos principal for keytab kinit only (default: guessed from static configuration)]) +# ARG_OPTIONAL_SINGLE([krb5-keytab-principal],[],[Kerberos principal for keytab-based TGT acquisition only (default: guessed from static configuration)]) # ARG_OPTIONAL_SINGLE([openid-ca-file],[],[OIDC CA certificate file],[]) # ARG_POSITIONAL_DOUBLEDASH([]) # ARG_POSITIONAL_INF([custom],[Additional arguments to pass to the ELNBuildSync daemon]) @@ -77,7 +77,7 @@ print_help() printf '\t%s\n' "--dynamic-config-url: Dynamic configuration Git URL (default: 'https://github.com/fedora-eln/elnbuildsync-config.git')" printf '\t%s\n' "--dynamic-config-branch: Dynamic configuration Git branch (default: 'main')" printf '\t%s\n' "--krb5-keytab-file: Kerberos keytab for in-process TGT acquisition (optional; otherwise use existing ccache)" - printf '\t%s\n' "--krb5-keytab-principal: Principal for keytab kinit only (default: guessed from static configuration when a keytab is set)" + printf '\t%s\n' "--krb5-keytab-principal: Principal for keytab-based TGT acquisition only (default: guessed from static configuration when a keytab is set)" printf '\t%s\n' "--openid-ca-file: OIDC CA certificate file (no default)" printf '\t%s\n' "-h, --help: Prints help" printf '\n%s\n' "Run the ELNBuildSync daemon" diff --git a/tests/local_test_daemon.sh b/tests/local_test_daemon.sh index 650ee3e..25d99af 100755 --- a/tests/local_test_daemon.sh +++ b/tests/local_test_daemon.sh @@ -29,7 +29,7 @@ # ARG_OPTIONAL_SINGLE([dynamic-config-file],[],[Dynamic configuration file],[tests/etc/dynamic-config/elnbuildsync_dynamic.yaml]) # ARG_OPTIONAL_SINGLE([environment],[],[Environment],[stg]) # ARG_OPTIONAL_SINGLE([krb5-keytab-file],[],[Kerberos keytab for in-process TGT acquisition (optional)]) -# ARG_OPTIONAL_SINGLE([krb5-keytab-principal],[],[Kerberos principal for keytab kinit only (default: guessed from static configuration)]) +# ARG_OPTIONAL_SINGLE([krb5-keytab-principal],[],[Kerberos principal for keytab-based TGT acquisition only (default: guessed from static configuration)]) # ARG_OPTIONAL_BOOLEAN([persistent-db],[],[Use persistent database],[off]) # ARG_OPTIONAL_SINGLE([persistent-db-volume],[],[Volume name for persistent database],[ebs_test_pgdata]) # ARG_OPTIONAL_BOOLEAN([build-container],[],[Build the ELNBuildSync container],[off]) @@ -93,7 +93,7 @@ print_help() printf '\t%s\n' "--dynamic-config-file: Dynamic configuration file (default: 'tests/etc/dynamic-config/elnbuildsync_dynamic.yaml')" printf '\t%s\n' "--environment: Environment (default: 'stg')" printf '\t%s\n' "--krb5-keytab-file: Kerberos keytab for in-process TGT acquisition (optional; otherwise use host KCM / existing ccache)" - printf '\t%s\n' "--krb5-keytab-principal: Principal for keytab kinit only (default: guessed from static configuration when a keytab is set)" + printf '\t%s\n' "--krb5-keytab-principal: Principal for keytab-based TGT acquisition only (default: guessed from static configuration when a keytab is set)" printf '\t%s\n' "--persistent-db, --no-persistent-db: Use persistent database (off by default)" printf '\t%s\n' "--persistent-db-volume: Volume name for persistent database (default: 'ebs_test_pgdata')" printf '\t%s\n' "--build-container, --no-build-container: Build the ELNBuildSync container (off by default)" From 3a8b61b42045ccd64e1f41b3b7130d6266ec7536 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:38:14 -0400 Subject: [PATCH 04/11] kojihelpers: always persist Kerberos creds and fail on store errors --- elnbuildsync/kojihelpers/connection.py | 20 ++++++++++------- tests/test_koji_connection.py | 31 ++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/elnbuildsync/kojihelpers/connection.py b/elnbuildsync/kojihelpers/connection.py index 45470a0..9f9b975 100644 --- a/elnbuildsync/kojihelpers/connection.py +++ b/elnbuildsync/kojihelpers/connection.py @@ -144,16 +144,20 @@ def _tgt_lifetime_seconds(): def _store_creds(creds): + """Persist ``creds`` to the active ccache, then retain them in-process. + + Always calls ``store_cred_into``: an empty store selects the default + ccache when ``KRB5CCNAME`` is unset. Failures raise ``KerberosAuthError``; + ``_krb_creds`` is updated only after successful persistence. + """ global _krb_creds store = _ccache_store() - if store: - try: - store_cred_into(store, creds, usage="initiate", overwrite=True) - except Exception: - logger.debug( - "store_cred_into failed; continuing with acquired creds", - exc_info=True, - ) + try: + store_cred_into(store, creds, usage="initiate", overwrite=True) + except Exception as e: + raise KerberosAuthError( + "Failed to persist Kerberos credentials to ccache" + ) from e _krb_creds = creds diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py index 72f0de7..bc8376e 100644 --- a/tests/test_koji_connection.py +++ b/tests/test_koji_connection.py @@ -120,6 +120,37 @@ def test_retry_wrapper_attempts_five_times(self): assert stop.max_attempt_number == 5 + +class TestStoreCreds: + def test_persists_to_default_ccache_when_krb5ccname_unset(self): + creds = MagicMock(name="creds") + env = {k: v for k, v in __import__("os").environ.items() if k != "KRB5CCNAME"} + with ( + patch.dict("os.environ", env, clear=True), + patch( + "elnbuildsync.kojihelpers.connection.store_cred_into" + ) as store_into, + ): + conn._store_creds(creds) + store_into.assert_called_once_with( + {}, creds, usage="initiate", overwrite=True + ) + assert conn._krb_creds is creds + + def test_store_failure_raises_and_skips_assignment(self): + creds = MagicMock(name="creds") + with ( + patch.dict("os.environ", {"KRB5CCNAME": "FILE:/tmp/cc"}, clear=False), + patch( + "elnbuildsync.kojihelpers.connection.store_cred_into", + side_effect=OSError("disk full"), + ), + pytest.raises(KerberosAuthError, match="persist"), + ): + conn._store_creds(creds) + assert conn._krb_creds is None + + @pytest.mark.asyncio class TestEnsureTgt: async def test_skips_renew_when_lifetime_at_threshold(self): From a4d058a664a481c87bfb6d6f655f14a7bb08f432 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:38:15 -0400 Subject: [PATCH 05/11] kojihelpers: hold auth lock across TGT renew and session recreate --- elnbuildsync/kojihelpers/connection.py | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/elnbuildsync/kojihelpers/connection.py b/elnbuildsync/kojihelpers/connection.py index 9f9b975..f83fb94 100644 --- a/elnbuildsync/kojihelpers/connection.py +++ b/elnbuildsync/kojihelpers/connection.py @@ -200,19 +200,25 @@ def _recreate_bsys_sync(): def _renew_tgt_and_bsys_once(): """One renew attempt: acquire TGT then recreate _bsys. + Holds ``_auth_lock`` for the full acquire/recreate/rollback sequence so + renewal cannot interleave with ``_invoke_koji_sync`` or concurrent renewals. + Raises KerberosAuthError on failure. Does not leave a half-updated _bsys. """ global _bsys - old_bsys = _bsys - try: - _acquire_tgt_sync() - _recreate_bsys_sync() - except KerberosAuthError: - _bsys = old_bsys - raise - except Exception as e: - _bsys = old_bsys - raise KerberosAuthError("TGT acquire or Koji session recreate failed") from e + with _auth_lock: + old_bsys = _bsys + try: + _acquire_tgt_sync() + _recreate_bsys_sync() + except KerberosAuthError: + _bsys = old_bsys + raise + except Exception as e: + _bsys = old_bsys + raise KerberosAuthError( + "TGT acquire or Koji session recreate failed" + ) from e @retry( From 2db50d87b310dd85f64e00a445c681049bbce998 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:38:49 -0400 Subject: [PATCH 06/11] kojihelpers: close superseded Koji requests session after recreate --- elnbuildsync/kojihelpers/connection.py | 28 ++++++++++++++++++---- tests/test_koji_connection.py | 33 ++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/elnbuildsync/kojihelpers/connection.py b/elnbuildsync/kojihelpers/connection.py index f83fb94..a236fd0 100644 --- a/elnbuildsync/kojihelpers/connection.py +++ b/elnbuildsync/kojihelpers/connection.py @@ -180,21 +180,41 @@ def _acquire_tgt_sync(): ) from e +def _close_bsys_rsession(bsys) -> None: + """Close a superseded ClientSession's underlying requests session.""" + if bsys is None: + return + rsession = getattr(bsys, "rsession", None) + if rsession is None: + return + try: + rsession.close() + except Exception: + logger.debug("Failed closing superseded Koji requests session", exc_info=True) + + def _recreate_bsys_sync(): - """Create a fresh Koji ClientSession (does not log in).""" + """Create a fresh Koji ClientSession (does not log in). + + On success, replaces ``_bsys`` and closes the previous session's requests + session. On failure, leaves the previous ``_bsys`` intact. + """ global _bsys if not config.main: raise BuildSysUnavailable("Configuration unavailable") profile = config.main["koji"]["profile"] + old_bsys = _bsys try: cfg = koji.read_config(profile_name=profile) - _bsys = koji.ClientSession(cfg["server"], opts=cfg) - logger.debug("Created new Koji ClientSession for profile %s", profile) + new_bsys = koji.ClientSession(cfg["server"], opts=cfg) except Exception as e: - _bsys = None raise BuildSysUnavailable( f'Failed initializing koji with profile "{profile}"' ) from e + _bsys = new_bsys + logger.debug("Created new Koji ClientSession for profile %s", profile) + if old_bsys is not None and old_bsys is not new_bsys: + _close_bsys_rsession(old_bsys) def _renew_tgt_and_bsys_once(): diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py index bc8376e..f59130b 100644 --- a/tests/test_koji_connection.py +++ b/tests/test_koji_connection.py @@ -119,6 +119,39 @@ def test_retry_wrapper_attempts_five_times(self): stop = conn._renew_tgt_and_bsys_with_retries.retry.stop assert stop.max_attempt_number == 5 + def test_recreate_closes_old_rsession_after_success(self): + old = MagicMock(name="old_bsys") + old.rsession = MagicMock(name="old_rsession") + conn._bsys = old + new_session = MagicMock(name="new_bsys") + with ( + patch.object(conn.config, "main", {"koji": {"profile": "koji"}}), + patch( + "elnbuildsync.kojihelpers.connection.koji.read_config", + return_value={"server": "https://koji.example/"}, + ), + patch( + "elnbuildsync.kojihelpers.connection.koji.ClientSession", + return_value=new_session, + ), + ): + conn._recreate_bsys_sync() + assert conn._bsys is new_session + old.rsession.close.assert_called_once() + + def test_recreate_failure_leaves_previous_bsys(self): + old = MagicMock(name="old_bsys") + conn._bsys = old + with ( + patch.object(conn.config, "main", {"koji": {"profile": "koji"}}), + patch( + "elnbuildsync.kojihelpers.connection.koji.read_config", + side_effect=RuntimeError("boom"), + ), + pytest.raises(conn.BuildSysUnavailable), + ): + conn._recreate_bsys_sync() + assert conn._bsys is old class TestStoreCreds: From ec4137be982399db9d46b5fd3b7aaa74a9f9c137 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:38:49 -0400 Subject: [PATCH 07/11] tests: invoke call_koji dispatch via _call_koji_once instead of __wrapped__ --- elnbuildsync/kojihelpers/connection.py | 9 +++++++-- tests/test_koji_connection.py | 7 ++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/elnbuildsync/kojihelpers/connection.py b/elnbuildsync/kojihelpers/connection.py index a236fd0..1fffcfd 100644 --- a/elnbuildsync/kojihelpers/connection.py +++ b/elnbuildsync/kojihelpers/connection.py @@ -357,6 +357,12 @@ def get_koji_url(): return cfg["weburl"] +async def _call_koji_once(method, *args, **kwargs): + """Single attempt: ensure TGT, then invoke in a worker thread.""" + await _ensure_tgt() + return await deferToThread(_invoke_koji_sync, method, args, kwargs) + + @retry( wait=wait_exponential(), stop=stop_after_delay(60), @@ -377,5 +383,4 @@ async def call_koji(method, *args, **kwargs): ``method`` may be a string attribute name on the private session, or a callable that receives the session as its first argument. """ - await _ensure_tgt() - return await deferToThread(_invoke_koji_sync, method, args, kwargs) + return await _call_koji_once(method, *args, **kwargs) diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py index f59130b..2da2f27 100644 --- a/tests/test_koji_connection.py +++ b/tests/test_koji_connection.py @@ -330,10 +330,7 @@ async def test_call_koji_string_method(): side_effect=_defer_immediately, ), ): - # Bypass outer tenacity by calling the undecorated body via wrap - result = await conn.call_koji.__wrapped__.__wrapped__( - "listTagged", "tag", latest=True - ) + result = await conn._call_koji_once("listTagged", "tag", latest=True) assert result == [{"nvr": "pkg-1"}] mock_bsys.listTagged.assert_called_once_with("tag", latest=True) @@ -357,6 +354,6 @@ def helper(bsys, tag, ids): side_effect=_defer_immediately, ), ): - result = await conn.call_koji.__wrapped__.__wrapped__(helper, "mytag", [1, 2]) + result = await conn._call_koji_once(helper, "mytag", [1, 2]) assert result == ("mytag", [1, 2]) From fffedd1d5d2dd688bf0a52e948c5247a9869dfe0 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:48:48 -0400 Subject: [PATCH 08/11] kojihelpers: collapse call_koji retries into one allow-list predicate --- elnbuildsync/kojihelpers/connection.py | 57 +++++++++++++------------- tests/test_koji_connection.py | 14 +++++-- 2 files changed, 40 insertions(+), 31 deletions(-) diff --git a/elnbuildsync/kojihelpers/connection.py b/elnbuildsync/kojihelpers/connection.py index 1fffcfd..8fb9af9 100644 --- a/elnbuildsync/kojihelpers/connection.py +++ b/elnbuildsync/kojihelpers/connection.py @@ -320,9 +320,6 @@ def _invoke_koji_sync(method, args, kwargs): # HTTP 4xx codes that are still worth retrying (transient client/server behavior). _RETRYABLE_4XX = frozenset((408, 429)) -# Auth / session setup failures are not transient; do not burn retry budget on them. -_NON_RETRYABLE_KOJI = (KerberosAuthError, KojiLoginError, BuildSysUnavailable) - async def _reactor_sleep(seconds: float) -> None: """Sleep via the Twisted reactor (safe under asyncioreactor + tenacity).""" @@ -332,24 +329,35 @@ async def _reactor_sleep(seconds: float) -> None: await deferLater(reactor, seconds) -def _retry_koji_request_exception(exc: BaseException) -> bool: - """Do not retry most HTTP 4xx responses; still retry 408 and 429.""" - if isinstance(exc, _NON_RETRYABLE_KOJI): - return False - if not isinstance(exc, RequestException): - return False - resp = getattr(exc, "response", None) - if resp is None: - return True - code = resp.status_code - if code in _RETRYABLE_4XX: - return True - return not (400 <= code < 500) - +def _should_retry_koji_exception(exc: BaseException) -> bool: + """Retry only genuinely transient failures within a single 60s budget. -def _retry_transient_koji_exception(exc: BaseException) -> bool: - """Retry unexpected transient failures; never retry auth/session errors.""" - return not isinstance(exc, _NON_RETRYABLE_KOJI) + ``RequestException`` uses HTTP status-code rules (retry 5xx, 408, 429; + fail-fast on other 4xx). Auth/session errors, ``koji.GenericError``, + ``AttributeError``, and ``TypeError`` are never retried. Other exception + types are not on the allow-list and fail fast. + """ + if isinstance( + exc, + ( + KerberosAuthError, + KojiLoginError, + BuildSysUnavailable, + koji.GenericError, + AttributeError, + TypeError, + ), + ): + return False + if isinstance(exc, RequestException): + resp = getattr(exc, "response", None) + if resp is None: + return True + code = resp.status_code + if code in _RETRYABLE_4XX: + return True + return not (400 <= code < 500) + return False def get_koji_url(): @@ -366,14 +374,7 @@ async def _call_koji_once(method, *args, **kwargs): @retry( wait=wait_exponential(), stop=stop_after_delay(60), - retry=retry_if_exception(_retry_koji_request_exception), - sleep=_reactor_sleep, - reraise=True, -) -@retry( - wait=wait_exponential(), - stop=stop_after_delay(60), - retry=retry_if_exception(_retry_transient_koji_exception), + retry=retry_if_exception(_should_retry_koji_exception), sleep=_reactor_sleep, reraise=True, ) diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py index 2da2f27..7256668 100644 --- a/tests/test_koji_connection.py +++ b/tests/test_koji_connection.py @@ -6,6 +6,7 @@ from unittest.mock import MagicMock, patch import pytest +from requests.exceptions import HTTPError from twisted.internet.defer import succeed from elnbuildsync.kojihelpers import connection as conn @@ -289,9 +290,16 @@ async def test_zero_lifetime_reraises_after_retries(self): await conn._ensure_tgt() -def test_auth_errors_are_not_retried(): - assert not conn._retry_transient_koji_exception(KerberosAuthError("x")) - assert not conn._retry_koji_request_exception(KerberosAuthError("x")) +def test_auth_and_client_errors_are_not_retried(): + assert not conn._should_retry_koji_exception(KerberosAuthError("x")) + assert not conn._should_retry_koji_exception(AttributeError("x")) + assert not conn._should_retry_koji_exception(TypeError("x")) + err_403 = HTTPError("forbidden") + err_403.response = MagicMock(status_code=403) + assert not conn._should_retry_koji_exception(err_403) + err_429 = HTTPError("slow down") + err_429.response = MagicMock(status_code=429) + assert conn._should_retry_koji_exception(err_429) def test_ensure_logged_in_skips_when_already_logged_in_flag(): From 4162d38f19cec7bc15a950f00199b2165d4be9e9 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:48:49 -0400 Subject: [PATCH 09/11] tests: assert call_koji does not retry auth errors or HTTP 403 --- tests/test_koji_connection.py | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py index 7256668..bb9c329 100644 --- a/tests/test_koji_connection.py +++ b/tests/test_koji_connection.py @@ -365,3 +365,40 @@ def helper(bsys, tag, ids): result = await conn._call_koji_once(helper, "mytag", [1, 2]) assert result == ("mytag", [1, 2]) + +@pytest.mark.asyncio +async def test_call_koji_does_not_retry_auth_or_403(): + conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + + with ( + patch.object(conn, "_ensure_tgt"), + patch.object( + conn, + "_invoke_koji_sync", + side_effect=KerberosAuthError("auth boom"), + ) as invoke, + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + patch.object(conn, "_reactor_sleep", return_value=succeed(None)), + pytest.raises(KerberosAuthError, match="auth boom"), + ): + await conn.call_koji("listTagged", "tag") + assert invoke.call_count == 1 + + err_403 = HTTPError("forbidden") + err_403.response = MagicMock(status_code=403) + with ( + patch.object(conn, "_ensure_tgt"), + patch.object(conn, "_invoke_koji_sync", side_effect=err_403) as invoke403, + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + patch.object(conn, "_reactor_sleep", return_value=succeed(None)), + pytest.raises(HTTPError), + ): + await conn.call_koji("listTagged", "tag") + assert invoke403.call_count == 1 + From 8e06d27b589953e6c71ea2fc82aa3e51b510833f Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:48:49 -0400 Subject: [PATCH 10/11] kojihelpers: cache TGT expiry to avoid duplicate credential reads --- elnbuildsync/kojihelpers/connection.py | 85 ++++++++++++++++++-------- tests/test_koji_connection.py | 22 +++++++ 2 files changed, 82 insertions(+), 25 deletions(-) diff --git a/elnbuildsync/kojihelpers/connection.py b/elnbuildsync/kojihelpers/connection.py index 8fb9af9..a1c4846 100644 --- a/elnbuildsync/kojihelpers/connection.py +++ b/elnbuildsync/kojihelpers/connection.py @@ -19,6 +19,7 @@ import logging import os import threading +import time import gssapi import koji @@ -63,6 +64,8 @@ _krb5_principal = None _krb5_keytab_file = None _auth_lock = threading.Lock() +# Monotonic clock time at which the cached TGT lifetime reaches zero. +_tgt_expiry_mono = None def resolve_krb5_keytab_principal(principal, koji_profile, koji_username): @@ -143,6 +146,19 @@ def _tgt_lifetime_seconds(): return 0 +def _update_tgt_expiry_cache(lifetime_seconds: int) -> None: + """Record monotonic expiry from a just-observed remaining lifetime.""" + global _tgt_expiry_mono + _tgt_expiry_mono = time.monotonic() + max(0, int(lifetime_seconds)) + + +def _cached_tgt_remaining(): + """Seconds remaining from the TGT expiry cache, or None if unset.""" + if _tgt_expiry_mono is None: + return None + return _tgt_expiry_mono - time.monotonic() + + def _store_creds(creds): """Persist ``creds`` to the active ccache, then retain them in-process. @@ -231,6 +247,7 @@ def _renew_tgt_and_bsys_once(): try: _acquire_tgt_sync() _recreate_bsys_sync() + _update_tgt_expiry_cache(_tgt_lifetime_seconds()) except KerberosAuthError: _bsys = old_bsys raise @@ -250,33 +267,36 @@ def _renew_tgt_and_bsys_with_retries(): _renew_tgt_and_bsys_once() -async def _ensure_tgt(): - """Ensure TGT is usable; renew from keytab (+ recreate _bsys) when needed. - - Without a configured keytab, never attempts acquisition: the existing - credential cache (``$KRB5CCNAME`` or system default) must already hold a TGT. - """ - lifetime = _tgt_lifetime_seconds() - if lifetime >= TGT_RENEW_THRESHOLD_SECONDS: - return - - if not _krb5_keytab_file: - if lifetime == 0: - raise KerberosAuthError( - "No Kerberos TGT available in the credential cache " - "($KRB5CCNAME or system default) and no --krb5-keytab-file " - "configured for acquisition" +def _ensure_tgt_sync(): + """Re-read and renew TGT under ``_auth_lock`` (worker-thread entrypoint).""" + with _auth_lock: + remaining = _cached_tgt_remaining() + if remaining is not None and remaining >= TGT_RENEW_THRESHOLD_SECONDS: + return + lifetime = _tgt_lifetime_seconds() + _update_tgt_expiry_cache(lifetime) + if lifetime >= TGT_RENEW_THRESHOLD_SECONDS: + return + keytab_configured = bool(_krb5_keytab_file) + if not keytab_configured: + if lifetime == 0: + raise KerberosAuthError( + "No Kerberos TGT available in the credential cache " + "($KRB5CCNAME or system default) and no --krb5-keytab-file " + "configured for acquisition" + ) + logger.debug( + "TGT lifetime %ss is below renew threshold but no keytab " + "configured; using existing credential cache", + lifetime, ) - logger.debug( - "TGT lifetime %ss is below renew threshold but no keytab " - "configured; using existing credential cache", - lifetime, - ) - return + return + soft_renew = lifetime > 0 - if lifetime > 0: + # Renew outside the read section; each renew attempt takes _auth_lock. + if soft_renew: try: - await deferToThread(_renew_tgt_and_bsys_once) + _renew_tgt_and_bsys_once() except KerberosAuthError: logger.exception( "TGT renew failed; continuing with existing ticket (lifetime=%ss)", @@ -284,7 +304,22 @@ async def _ensure_tgt(): ) return - await deferToThread(_renew_tgt_and_bsys_with_retries) + _renew_tgt_and_bsys_with_retries() + + +async def _ensure_tgt(): + """Ensure TGT is usable; renew from keytab (+ recreate _bsys) when needed. + + Without a configured keytab, never attempts acquisition: the existing + credential cache (``$KRB5CCNAME`` or system default) must already hold a TGT. + + Uses a monotonic expiry cache so concurrent ``call_koji`` callers skip + duplicate credential reads while the ticket is safely above threshold. + """ + remaining = _cached_tgt_remaining() + if remaining is not None and remaining >= TGT_RENEW_THRESHOLD_SECONDS: + return + await deferToThread(_ensure_tgt_sync) def _ensure_bsys_sync(): diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py index bb9c329..88c2b38 100644 --- a/tests/test_koji_connection.py +++ b/tests/test_koji_connection.py @@ -23,11 +23,13 @@ def _reset_connection_state(): conn._krb_creds = None conn._krb5_principal = None conn._krb5_keytab_file = None + conn._tgt_expiry_mono = None yield conn._bsys = None conn._krb_creds = None conn._krb5_principal = None conn._krb5_keytab_file = None + conn._tgt_expiry_mono = None class TestResolveKrb5KeytabPrincipal: @@ -86,6 +88,7 @@ def test_renew_acquires_tgt_then_recreates_bsys(self): with ( patch.object(conn, "_acquire_tgt_sync") as acquire, patch.object(conn, "_recreate_bsys_sync") as recreate, + patch.object(conn, "_tgt_lifetime_seconds", return_value=3600), ): conn._renew_tgt_and_bsys_once() acquire.assert_called_once() @@ -204,6 +207,20 @@ async def test_skips_renew_when_lifetime_at_threshold(self): await conn._ensure_tgt() renew.assert_not_called() + async def test_cache_hit_skips_credential_read(self): + conn._update_tgt_expiry_cache(conn.TGT_RENEW_THRESHOLD_SECONDS + 60) + with ( + patch.object(conn, "_tgt_lifetime_seconds") as lifetime, + patch.object(conn, "_renew_tgt_and_bsys_once") as renew, + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), + ): + await conn._ensure_tgt() + lifetime.assert_not_called() + renew.assert_not_called() + async def test_renews_when_under_threshold_with_keytab(self): conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") with ( @@ -234,6 +251,10 @@ async def test_no_keytab_raises_when_no_tgt(self): conn.configure_kerberos(keytab_file=None) with ( patch.object(conn, "_tgt_lifetime_seconds", return_value=0), + patch( + "elnbuildsync.kojihelpers.connection.deferToThread", + side_effect=_defer_immediately, + ), pytest.raises(KerberosAuthError, match="No Kerberos TGT available"), ): await conn._ensure_tgt() @@ -369,6 +390,7 @@ def helper(bsys, tag, ids): @pytest.mark.asyncio async def test_call_koji_does_not_retry_auth_or_403(): conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") + conn._update_tgt_expiry_cache(conn.TGT_RENEW_THRESHOLD_SECONDS + 10) with ( patch.object(conn, "_ensure_tgt"), From 29c534d1b3e5aafcf6d4624104d319bee9083c14 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Mon, 27 Jul 2026 15:48:50 -0400 Subject: [PATCH 11/11] tests: cover remaining _tgt_lifetime_seconds branches --- tests/test_koji_connection.py | 50 ++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 7 deletions(-) diff --git a/tests/test_koji_connection.py b/tests/test_koji_connection.py index 88c2b38..e9287bb 100644 --- a/tests/test_koji_connection.py +++ b/tests/test_koji_connection.py @@ -82,6 +82,46 @@ def test_reads_ccache_without_principal_name(self): assert kwargs["usage"] == "initiate" assert kwargs["store"] == {"ccache": "KCM:"} + def test_none_lifetime_returns_renew_threshold(self): + mock_creds = MagicMock() + mock_creds.lifetime = None + with ( + patch.dict("os.environ", {"KRB5CCNAME": "FILE:/tmp/cc"}, clear=False), + patch( + "elnbuildsync.kojihelpers.connection.gssapi.Credentials", + return_value=mock_creds, + ) as creds_cls, + ): + assert conn._tgt_lifetime_seconds() == conn.TGT_RENEW_THRESHOLD_SECONDS + assert creds_cls.call_args.kwargs["store"] == {"ccache": "FILE:/tmp/cc"} + + def test_credentials_error_returns_zero(self): + with ( + patch.dict("os.environ", {"KRB5CCNAME": "KCM:"}, clear=False), + patch( + "elnbuildsync.kojihelpers.connection.gssapi.Credentials", + side_effect=RuntimeError("no ccache"), + ) as creds_cls, + ): + assert conn._tgt_lifetime_seconds() == 0 + assert creds_cls.call_args.kwargs["store"] == {"ccache": "KCM:"} + + def test_omits_store_when_krb5ccname_unset(self): + mock_creds = MagicMock() + mock_creds.lifetime = 120 + env = {k: v for k, v in __import__("os").environ.items() if k != "KRB5CCNAME"} + with ( + patch.dict("os.environ", env, clear=True), + patch( + "elnbuildsync.kojihelpers.connection.gssapi.Credentials", + return_value=mock_creds, + ) as creds_cls, + ): + assert conn._tgt_lifetime_seconds() == 120 + kwargs = creds_cls.call_args.kwargs + assert "store" not in kwargs + assert kwargs["usage"] == "initiate" + class TestRenewUnit: def test_renew_acquires_tgt_then_recreates_bsys(self): @@ -164,14 +204,10 @@ def test_persists_to_default_ccache_when_krb5ccname_unset(self): env = {k: v for k, v in __import__("os").environ.items() if k != "KRB5CCNAME"} with ( patch.dict("os.environ", env, clear=True), - patch( - "elnbuildsync.kojihelpers.connection.store_cred_into" - ) as store_into, + patch("elnbuildsync.kojihelpers.connection.store_cred_into") as store_into, ): conn._store_creds(creds) - store_into.assert_called_once_with( - {}, creds, usage="initiate", overwrite=True - ) + store_into.assert_called_once_with({}, creds, usage="initiate", overwrite=True) assert conn._krb_creds is creds def test_store_failure_raises_and_skips_assignment(self): @@ -387,6 +423,7 @@ def helper(bsys, tag, ids): assert result == ("mytag", [1, 2]) + @pytest.mark.asyncio async def test_call_koji_does_not_retry_auth_or_403(): conn.configure_kerberos(keytab_file="/kt", keytab_principal="user@REALM") @@ -423,4 +460,3 @@ async def test_call_koji_does_not_retry_auth_or_403(): ): await conn.call_koji("listTagged", "tag") assert invoke403.call_count == 1 -