From 5a94ed7b3a92497fc440aacf49813673cc89279b Mon Sep 17 00:00:00 2001 From: Jacob Craiglow Date: Wed, 22 Jul 2026 14:12:11 -0400 Subject: [PATCH 1/3] sonar issues: Logging exception instead of logging errors for better traceback Signed-off-by: Jacob Craiglow --- src/aap_eda/analytics/analytics_collectors.py | 4 +- src/aap_eda/analytics/collector.py | 2 +- src/aap_eda/analytics/utils.py | 12 ++-- src/aap_eda/api/serializers/activation.py | 4 +- src/aap_eda/api/views/activation.py | 3 +- src/aap_eda/api/views/credential_type.py | 2 +- src/aap_eda/api/views/eda_credential.py | 2 +- src/aap_eda/api/views/event_stream.py | 6 +- .../api/views/external_event_stream.py | 6 +- src/aap_eda/conf/registry.py | 2 +- src/aap_eda/core/health.py | 9 +-- .../core/management/commands/dispatcherctl.py | 2 +- .../core/management/commands/dispatcherd.py | 2 +- src/aap_eda/core/models/project.py | 2 +- src/aap_eda/core/tasking/__init__.py | 3 +- src/aap_eda/core/utils/credential_plugins.py | 2 +- src/aap_eda/core/utils/credentials.py | 6 +- src/aap_eda/core/utils/external_sms.py | 2 +- src/aap_eda/core/utils/rulebook.py | 2 +- .../services/activation/activation_manager.py | 28 +++++----- .../services/activation/drools_cleanup.py | 3 +- .../services/activation/engine/kubernetes.py | 4 +- .../services/activation/engine/podman.py | 16 +++--- src/aap_eda/services/pg_notify.py | 2 +- src/aap_eda/services/sync_certs.py | 20 +++---- src/aap_eda/tasks/orchestrator.py | 10 ++-- src/aap_eda/tasks/project.py | 55 +++++++------------ src/aap_eda/utils/__init__.py | 2 +- src/aap_eda/wsapi/consumers.py | 12 ++-- tests/integration/analytics/test_collector.py | 2 +- tests/integration/analytics/test_utils.py | 12 ++-- .../integration/core/test_dispatcherd_real.py | 18 ++---- .../management/test_dispatcherd_commands.py | 8 +-- .../activation/test_drools_cleanup.py | 4 +- .../tasks/test_project_dispatcherd.py | 5 +- tests/integration/tasks/test_projects.py | 31 +++++------ tests/unit/test_orchestrator.py | 5 +- 37 files changed, 138 insertions(+), 172 deletions(-) diff --git a/src/aap_eda/analytics/analytics_collectors.py b/src/aap_eda/analytics/analytics_collectors.py index ee3f293c3..0dc67aff3 100644 --- a/src/aap_eda/analytics/analytics_collectors.py +++ b/src/aap_eda/analytics/analytics_collectors.py @@ -811,10 +811,10 @@ def _copy_table( cursor.execute(f"DROP VIEW IF EXISTS {view_name}") return file.file_list() except DatabaseError as e: - logger.error(f"Database error occurred: {e}") + logger.exception(f"Database error occurred: {e}") return None except IOError as e: - logger.error(f"File I/O error occurred: {e}") + logger.exception(f"File I/O error occurred: {e}") return None except Exception as e: logger.exception(f"An unexpected error occurred: {e}") diff --git a/src/aap_eda/analytics/collector.py b/src/aap_eda/analytics/collector.py index 88c4ab620..eb56df794 100644 --- a/src/aap_eda/analytics/collector.py +++ b/src/aap_eda/analytics/collector.py @@ -66,7 +66,7 @@ def _load_last_gathered_entries(self) -> dict: return json.loads(last_entries, object_hook=utils.datetime_hook) except (json.JSONDecodeError, TypeError) as e: - self.logger.error(f"Failed to load last entries: {str(e)}") + self.logger.exception(f"Failed to load last entries: {str(e)}") return {} def _save_last_gathered_entries(self, last_gathered_entries: dict) -> None: diff --git a/src/aap_eda/analytics/utils.py b/src/aap_eda/analytics/utils.py index b490f42c2..efd1f93ce 100644 --- a/src/aap_eda/analytics/utils.py +++ b/src/aap_eda/analytics/utils.py @@ -92,10 +92,10 @@ def collect_controllers_info() -> dict: info[host] = controller_info except KeyError as e: - logger.error(f"Missing key in credential inputs: {e}") + logger.exception(f"Missing key in credential inputs: {e}") continue except yaml.YAMLError as e: - logger.error( + logger.exception( f"YAML parsing error for credential {credential.id}: {e}" ) continue @@ -205,13 +205,13 @@ def generate_token() -> ServiceToken: ) resp.raise_for_status() except requests.exceptions.SSLError: - logger.error("SSL certificate verification failed") + logger.exception("SSL certificate verification failed") raise except requests.exceptions.Timeout: - logger.error("Token request timed out") + logger.exception("Token request timed out") raise except requests.exceptions.RequestException as e: - logger.error(f"Token request failed: {str(e)}") + logger.exception(f"Token request failed: {str(e)}") raise data = resp.json() @@ -304,7 +304,7 @@ def get_analytics_interval() -> int: try: return int(interval) except ValueError as e: - logger.error( + logger.exception( f"Invalid analytics interval value '{interval}'. " f"Using default interval. Error: {str(e)}" ) diff --git a/src/aap_eda/api/serializers/activation.py b/src/aap_eda/api/serializers/activation.py index 09fa0b71e..c6af4570c 100644 --- a/src/aap_eda/api/serializers/activation.py +++ b/src/aap_eda/api/serializers/activation.py @@ -107,7 +107,7 @@ def _update_event_stream_source(validated_data: dict) -> str: ) # TODO: Can we catch a better exception except Exception as e: - logger.error( + logger.exception( "Failed to update event stream source in rulesets: %s", str(e) ) raise InvalidEventStreamSource(e) from e @@ -1619,7 +1619,7 @@ def _validate_sources_with_event_streams(data: dict) -> None: try: source_mappings = yaml.safe_load(source_mappings) except yaml.MarkedYAMLError as ex: - logger.error("Invalid source mappings: %s", str(ex)) + logger.exception("Invalid source mappings: %s", str(ex)) raise serializers.ValidationError( { SOURCE_MAPPING_ERROR_KEY: [ diff --git a/src/aap_eda/api/views/activation.py b/src/aap_eda/api/views/activation.py index 357f7580e..a49d5a4cf 100644 --- a/src/aap_eda/api/views/activation.py +++ b/src/aap_eda/api/views/activation.py @@ -712,10 +712,9 @@ def _sync_project_if_needed( try: sync_project(activation.project.id) except Exception as e: - logger.error( + logger.exception( f"Failed to start project sync for " f"'{activation.name}': {e}", - exc_info=True, ) activation.awaiting_project_sync = False activation.status = ActivationStatus.ERROR diff --git a/src/aap_eda/api/views/credential_type.py b/src/aap_eda/api/views/credential_type.py index af547a073..c8d684325 100644 --- a/src/aap_eda/api/views/credential_type.py +++ b/src/aap_eda/api/views/credential_type.py @@ -246,7 +246,7 @@ def test(self, request, pk): serializer.validated_data["metadata"], ) except Exception as err: - logger.error("Plugin call failed %s", err) + logger.exception("Plugin call failed %s", err) return Response(status=status.HTTP_400_BAD_REQUEST, data={}) return Response(status=status.HTTP_202_ACCEPTED, data={}) diff --git a/src/aap_eda/api/views/eda_credential.py b/src/aap_eda/api/views/eda_credential.py index 705c4311d..c81192a6d 100644 --- a/src/aap_eda/api/views/eda_credential.py +++ b/src/aap_eda/api/views/eda_credential.py @@ -325,7 +325,7 @@ def test(self, request, pk): serializer.validated_data["metadata"], ) except Exception as err: - logger.error( + logger.exception( "Plugin : %s call failed %s", eda_credential.credential_type.namespace, err, diff --git a/src/aap_eda/api/views/event_stream.py b/src/aap_eda/api/views/event_stream.py index cccb6620b..b56762055 100644 --- a/src/aap_eda/api/views/event_stream.py +++ b/src/aap_eda/api/views/event_stream.py @@ -366,13 +366,15 @@ def _sync_certificates( else: obj.update() except CoreGatewayAPIError as ex: - logger.error("Could not %s certificates: %s", action, str(ex)) + logger.exception( + "Could not %s certificates: %s", action, str(ex) + ) raise api_exc.GatewayAPIError( detail=f"Gateway API error during certificate {action}: " f"{str(ex)}" ) except CoreMissingCredentials as ex: - logger.error( + logger.exception( "Missing credentials for certificate %s: %s", action, str(ex), diff --git a/src/aap_eda/api/views/external_event_stream.py b/src/aap_eda/api/views/external_event_stream.py index 2cf030298..488f4ef46 100644 --- a/src/aap_eda/api/views/external_event_stream.py +++ b/src/aap_eda/api/views/external_event_stream.py @@ -143,14 +143,14 @@ def _parse_body(self, content_type: str, body: bytes) -> dict: ) except ValueError as exc: message = f"Invalid content. Type: {content_type}" - logger.error(message) + logger.exception(message) raise ParseError(message) from exc else: try: data = yaml.safe_load(body.decode()) except yaml.YAMLError as exc: message = f"Invalid content. Type: {content_type}" - logger.error(message) + logger.exception(message) raise ParseError(message) from exc return data @@ -346,7 +346,7 @@ def post(self, request, *_args, **kwargs): payload, )() except PGNotifyError as e: - logger.error(e) + logger.exception(e) return Response(status=status.HTTP_500_INTERNAL_SERVER_ERROR) return Response(status=status.HTTP_200_OK) diff --git a/src/aap_eda/conf/registry.py b/src/aap_eda/conf/registry.py index 1b830f473..cd4944075 100644 --- a/src/aap_eda/conf/registry.py +++ b/src/aap_eda/conf/registry.py @@ -254,7 +254,7 @@ def _resync_remote_settings(self, sync_group: str) -> None: "Failed to fetch settings from gateway. Exception: " f"{str(e)}. Default or stored values will be used" ) - logger.error(msg) + logger.exception(msg) return if not res.ok: msg = ( diff --git a/src/aap_eda/core/health.py b/src/aap_eda/core/health.py index 494267d25..887fcfea3 100644 --- a/src/aap_eda/core/health.py +++ b/src/aap_eda/core/health.py @@ -49,9 +49,8 @@ def check_activation_worker_health( # healthy return True except Exception as e: - logger.error( + logger.exception( f"Health check failed for activation workers: {e}", - exc_info=True, ) return False @@ -95,9 +94,8 @@ def check_dispatcherd_workers_health( except Exception as e: if not raise_exceptions: - logger.error( + logger.exception( f"Health check failed for dispatcherd workers: {e}", - exc_info=True, ) return False else: @@ -108,8 +106,7 @@ def check_dispatcherd_workers_health( ): raise # For other exceptions, log and raise a generic WorkerUnavailable - logger.error( + logger.exception( f"Health check failed for dispatcherd workers: {e}", - exc_info=True, ) raise api_exc.WorkerUnavailable() diff --git a/src/aap_eda/core/management/commands/dispatcherctl.py b/src/aap_eda/core/management/commands/dispatcherctl.py index 29316e531..31c3b1f3b 100644 --- a/src/aap_eda/core/management/commands/dispatcherctl.py +++ b/src/aap_eda/core/management/commands/dispatcherctl.py @@ -139,5 +139,5 @@ def handle(self, *args, **options) -> None: raise CommandError("Command interrupted by user") except Exception as e: error_msg = f"Failed to execute {command}: {e}" - logger.error(error_msg, exc_info=True) + logger.exception(error_msg) raise CommandError(error_msg) diff --git a/src/aap_eda/core/management/commands/dispatcherd.py b/src/aap_eda/core/management/commands/dispatcherd.py index f7e345131..03eec2100 100644 --- a/src/aap_eda/core/management/commands/dispatcherd.py +++ b/src/aap_eda/core/management/commands/dispatcherd.py @@ -107,5 +107,5 @@ def handle(self, *args, **options) -> None: except Exception as e: error_msg = f"Failed to start {worker_class}: {e}" self.stderr.write(self.style.ERROR(error_msg)) - logger.error(error_msg, exc_info=True) + logger.exception(error_msg) raise SystemExit(1) diff --git a/src/aap_eda/core/models/project.py b/src/aap_eda/core/models/project.py index c706e96c1..603a189b8 100644 --- a/src/aap_eda/core/models/project.py +++ b/src/aap_eda/core/models/project.py @@ -174,7 +174,7 @@ def needs_update_on_launch(self) -> bool: except (AttributeError, TypeError, ValueError) as e: # Log error but return safe default to prevent activation failures - logger.error( + logger.exception( f"Error determining sync status for project {self.pk}: {e}" ) # Safe default: assume sync needed to prevent stale content diff --git a/src/aap_eda/core/tasking/__init__.py b/src/aap_eda/core/tasking/__init__.py index b3823d467..9da43e7f0 100644 --- a/src/aap_eda/core/tasking/__init__.py +++ b/src/aap_eda/core/tasking/__init__.py @@ -47,9 +47,8 @@ def queue_cancel_job(queue_name: str, job_id: str) -> None: else: logger.debug(f"No jobs running with id {job_id} to cancel") except Exception as e: - logger.error( + logger.exception( f"Failed to cancel job {job_id} in queue {queue_name}: {e}", - exc_info=True, ) diff --git a/src/aap_eda/core/utils/credential_plugins.py b/src/aap_eda/core/utils/credential_plugins.py index f80160c58..dcc84befe 100644 --- a/src/aap_eda/core/utils/credential_plugins.py +++ b/src/aap_eda/core/utils/credential_plugins.py @@ -58,7 +58,7 @@ def run_plugin(plugin_type: str, inputs: dict, metadata: dict) -> dict: msg = ( f"Error executing credential plugin {plugin_type}: {str(err)}" ) - LOGGER.error(msg) + LOGGER.exception(msg) raise CredentialPluginError(msg) from err raise UnknownPluginTypeError( diff --git a/src/aap_eda/core/utils/credentials.py b/src/aap_eda/core/utils/credentials.py index 0345b16b8..134e05047 100644 --- a/src/aap_eda/core/utils/credentials.py +++ b/src/aap_eda/core/utils/credentials.py @@ -948,7 +948,7 @@ def validate_x509_subject_match(expected: str, actual: str) -> bool: try: actual_name = x509.Name.from_rfc4514_string(actual) except ValueError as e: - LOGGER.error(f"Invalid actual DN format: '{actual}': {e}") + LOGGER.exception(f"Invalid actual DN format: '{actual}': {e}") return False # Parse expected DN manually to handle regex patterns @@ -1088,7 +1088,7 @@ def _match_regex_pattern_against_attrs( for attr in actual_attrs ) except re.error as e: - LOGGER.error(f"Invalid regex pattern '{pattern}': {e}") + LOGGER.exception(f"Invalid regex pattern '{pattern}': {e}") return False @@ -1123,7 +1123,7 @@ def _get_aes_key( ) return base64.b64encode(raw_key).decode("utf-8") except Exception as e: - LOGGER.error(f"Failed to derive AES key: {e}") + LOGGER.exception(f"Failed to derive AES key: {e}") raise ValidationError(f"Failed to generate encryption key: {str(e)}") diff --git a/src/aap_eda/core/utils/external_sms.py b/src/aap_eda/core/utils/external_sms.py index 94a08a58d..e6e0a2137 100644 --- a/src/aap_eda/core/utils/external_sms.py +++ b/src/aap_eda/core/utils/external_sms.py @@ -47,6 +47,6 @@ def get_external_secrets(credential_id: int) -> dict: f"credentials defined in: {obj.source_credential.name} " f"Error: {str(err)}" ) - LOGGER.error(msg) + LOGGER.exception(msg) raise CredentialPluginError(msg) from err return result diff --git a/src/aap_eda/core/utils/rulebook.py b/src/aap_eda/core/utils/rulebook.py index 5ba8af87a..f2abebdcd 100644 --- a/src/aap_eda/core/utils/rulebook.py +++ b/src/aap_eda/core/utils/rulebook.py @@ -41,7 +41,7 @@ def build_source_list(rulesets_data: str) -> list[dict]: try: rulesets = yaml.safe_load(rulesets_data) except yaml.MarkedYAMLError as ex: - LOGGER.error("Invalid rulesets: %s", str(ex)) + LOGGER.exception("Invalid rulesets: %s", str(ex)) raise ParseError("Failed to parse rulebook data") from ex rulebook_hash = get_rulebook_hash(rulesets_data) diff --git a/src/aap_eda/services/activation/activation_manager.py b/src/aap_eda/services/activation/activation_manager.py index fb2c59c53..031bcb8fc 100644 --- a/src/aap_eda/services/activation/activation_manager.py +++ b/src/aap_eda/services/activation/activation_manager.py @@ -298,7 +298,7 @@ def _cleanup(self): f"latest instance {latest_instance.id} with pod id {pod_id}. " f"Reason: {exc}" ) - LOGGER.error(msg) + LOGGER.exception(msg) log_handler.write(msg, flush=True) return @@ -435,7 +435,7 @@ def _missing_container_policy(self): f"Activation {self.db_instance.id} failed to cleanup. " f"Reason: {exc}" ) - LOGGER.error(msg) + LOGGER.exception(msg) # Alex: Not sure if we want to change the status here. self.set_status(ActivationStatus.ERROR, msg) raise exceptions.ActivationMonitorError(msg) from exc @@ -542,7 +542,7 @@ def _failed_policy(self, container_msg: str): f"Activation {self.db_instance.id} failed to cleanup. " f"Reason: {exc}" ) - LOGGER.error(msg) + LOGGER.exception(msg) # Alex: Not sure if we want to change the status here. self._error_instance(msg) self.set_status(ActivationStatus.ERROR, msg) @@ -577,7 +577,7 @@ def _failed_policy(self, container_msg: str): f"Activation {self.db_instance.id} failed to cleanup. " f"Reason: {exc}" ) - LOGGER.error(msg) + LOGGER.exception(msg) # Alex: Not sure if we want to change the status here. self._error_instance(msg) self.set_status(ActivationStatus.ERROR, msg) @@ -609,7 +609,7 @@ def _failed_policy(self, container_msg: str): f"Activation {self.db_instance.id} failed to cleanup. " f"Reason: {exc}" ) - LOGGER.error(msg) + LOGGER.exception(msg) # Alex: Not sure if we want to change the status here. self.set_status(ActivationStatus.ERROR, msg) self.set_latest_instance_status( @@ -711,7 +711,7 @@ def stop(self): try: self.db_instance.refresh_from_db() except ObjectDoesNotExist: - LOGGER.error( + LOGGER.exception( f"Stop operation failed: Activation {self.db_instance.id} " "does not exist.", ) @@ -725,7 +725,7 @@ def stop(self): LOGGER.info(msg) return except exceptions.ActivationInstanceNotFound: - LOGGER.error( + LOGGER.exception( f"Stop operation activation id: {self.db_instance.id} " "No instance found.", ) @@ -791,7 +791,7 @@ def delete(self): f"Activation {self.db_instance.id} failed to clean up. " f"Reason: {exc}" ) - LOGGER.error(msg) + LOGGER.exception(msg) finally: drools_cleanup(self.db_instance) @@ -804,7 +804,7 @@ def delete(self): f"Delete operation failed: Activation {self.db_instance.id} " "does not exist." ) - LOGGER.error(msg) + LOGGER.exception(msg) raise exceptions.ActivationManagerError(msg) from None # Cancel any outstanding restart. @@ -843,7 +843,7 @@ def monitor(self): try: self.db_instance.refresh_from_db() except ObjectDoesNotExist: - LOGGER.error( + LOGGER.exception( f"Monitor operation Failed: Activation {self.db_instance.id} " "does not exist.", ) @@ -856,11 +856,11 @@ def monitor(self): try: self._check_latest_instance_and_pod_id() except exceptions.ActivationInstanceNotFound as e: - LOGGER.error(f"Monitor operation Failed: {e}") + LOGGER.exception(f"Monitor operation Failed: {e}") self._error_activation(f"{e}") raise exceptions.ActivationMonitorError(f"{e}") except exceptions.ActivationInstancePodIdNotFound as e: - LOGGER.error(f"Monitor operation Failed: {e}") + LOGGER.exception(f"Monitor operation Failed: {e}") self._error_activation(f"{e}") self._error_instance(f"{e}") raise exceptions.ActivationMonitorError(f"{e}") @@ -1032,7 +1032,7 @@ def update_logs(self): f"Update logs operation failed for activation id: " f"{self.db_instance.id} No instance or pod id found." ) - LOGGER.error(msg) + LOGGER.exception(msg) # Alex: Not sure if we want to change the status here. # For now, we are not changing the status of the activation return @@ -1051,7 +1051,7 @@ def update_logs(self): f"Logs for activation {self.db_instance.id} could not be " f"retrieved. Reason: {exc}" ) - LOGGER.error(msg) + LOGGER.exception(msg) log_handler.write(msg, flush=True) # Alex: Not sure if we want to change the status here. # For now, we are not changing the status of the activation diff --git a/src/aap_eda/services/activation/drools_cleanup.py b/src/aap_eda/services/activation/drools_cleanup.py index 660e4c5e0..b8c3da81c 100644 --- a/src/aap_eda/services/activation/drools_cleanup.py +++ b/src/aap_eda/services/activation/drools_cleanup.py @@ -294,11 +294,10 @@ def _delete_rows_by_ha_uuid( conn.commit() except psycopg.Error as e: - LOGGER.error( + LOGGER.exception( "Error during Drools cleanup for ha_uuid %s: %s", ha_uuid, str(e), - exc_info=True, ) # Return empty results dict to indicate cleanup failed # but don't re-raise to allow other cleanup operations to continue diff --git a/src/aap_eda/services/activation/engine/kubernetes.py b/src/aap_eda/services/activation/engine/kubernetes.py index a668cb4e4..82f968a67 100644 --- a/src/aap_eda/services/activation/engine/kubernetes.py +++ b/src/aap_eda/services/activation/engine/kubernetes.py @@ -139,7 +139,7 @@ def start(self, request: ContainerRequest, log_handler: LogHandler) -> str: f"Failed to start job {self.job_name}, doing cleanup." f"Reason: {e}" ) - LOGGER.error(msg) + LOGGER.exception(msg) log_handler.write(msg, flush=True) self.cleanup(self.job_name, log_handler) raise @@ -709,7 +709,7 @@ def _set_namespace(self) -> None: LOGGER.info(f"Namespace is {self.namespace}") except FileNotFoundError as e: message = f"Namespace file {namespace_file} does not exist." - LOGGER.error(message) + LOGGER.exception(message) raise ContainerEngineInitError(message) from e def _create_secret( diff --git a/src/aap_eda/services/activation/engine/podman.py b/src/aap_eda/services/activation/engine/podman.py index 26e580d23..0418529a0 100644 --- a/src/aap_eda/services/activation/engine/podman.py +++ b/src/aap_eda/services/activation/engine/podman.py @@ -56,7 +56,7 @@ def get_podman_client() -> PodmanClient: try: return PodmanClient(**params) except ValueError as e: - LOGGER.error(f"Failed to initialize podman client: f{e}") + LOGGER.exception(f"Failed to initialize podman client: f{e}") raise exceptions.ContainerEngineInitError(str(e)) from e @@ -75,7 +75,7 @@ def __init__( LOGGER.debug(self.client.version()) except APIError as e: - LOGGER.error(f"Failed to initialize podman engine: f{e}") + LOGGER.exception(f"Failed to initialize podman engine: f{e}") raise exceptions.ContainerEngineInitError(str(e)) self.JobTimeoutException = self._get_job_timeout_exception() @@ -211,7 +211,7 @@ def start(self, request: ContainerRequest, log_handler: LogHandler) -> str: APIError, ) as e: error_message = f"Container Start Error: {e}" - LOGGER.error(error_message) + LOGGER.exception(error_message) log_handler.write(error_message, flush=True) raise exceptions.ContainerStartError(error_message) from e @@ -354,7 +354,7 @@ def _cleanup(self, container_id: str, _log_handler: LogHandler) -> None: except NotFound: LOGGER.warning(f"Container {container_id} not found.") except APIError as e: - LOGGER.error(f"Failed to cleanup {container_id}: {e}") + LOGGER.exception(f"Failed to cleanup {container_id}: {e}") raise exceptions.ContainerCleanupError(str(e)) def _get_ports(self, found_ports: list[tuple]) -> dict: @@ -416,7 +416,7 @@ def _pull_image( return image except ImageNotFound as e: msg = f"Image {request.image_url} not found" - LOGGER.error(msg) + LOGGER.exception(msg) log_handler.write(msg, True) raise exceptions.ContainerImagePullError(msg) from e except APIError as e: @@ -424,14 +424,14 @@ def _pull_image( msg = messages.IMAGE_PULL_ERROR.format( image_url=request.image_url, ) - LOGGER.error(msg) + LOGGER.exception(msg) log_handler.write(msg, True) raise exceptions.ContainerImagePullError(msg) - LOGGER.error(f"Failed to pull image {request.image_url}: {e}") + LOGGER.exception(f"Failed to pull image {request.image_url}: {e}") raise exceptions.ContainerStartError(str(e)) except self.JobTimeoutException as e: msg = f"Timeout: {e}" - LOGGER.error(msg) + LOGGER.exception(msg) log_handler.write(msg, True) raise exceptions.ContainerImagePullError(msg) from e diff --git a/src/aap_eda/services/pg_notify.py b/src/aap_eda/services/pg_notify.py index 971ad2b58..7b301e98d 100644 --- a/src/aap_eda/services/pg_notify.py +++ b/src/aap_eda/services/pg_notify.py @@ -98,5 +98,5 @@ def __call__(self): "SELECT pg_notify(%s, %s)", [self.channel, payload] ) except psycopg.OperationalError as e: - logger.error("PG Notify operational error %s", str(e)) + logger.exception("PG Notify operational error %s", str(e)) raise PGNotifyError() from e diff --git a/src/aap_eda/services/sync_certs.py b/src/aap_eda/services/sync_certs.py index 3f0ed1bc8..cc12b4e5c 100644 --- a/src/aap_eda/services/sync_certs.py +++ b/src/aap_eda/services/sync_certs.py @@ -142,15 +142,15 @@ def _make_request( ) return response except requests.exceptions.ConnectionError as ex: - LOGGER.error( + LOGGER.exception( "Connection error while updating certificate: %s", str(ex) ) raise GatewayAPIError(f"Connection error: {str(ex)}") except requests.exceptions.Timeout as ex: - LOGGER.error("Timeout while updating certificate: %s", str(ex)) + LOGGER.exception("Timeout while updating certificate: %s", str(ex)) raise GatewayAPIError(f"Request timeout: {str(ex)}") except requests.exceptions.RequestException as ex: - LOGGER.error( + LOGGER.exception( "Request error while updating certificate: %s", str(ex) ) raise GatewayAPIError(f"Request error: {str(ex)}") @@ -199,15 +199,15 @@ def _delete_from_gateway(self, existing_object: Dict[str, Any]) -> None: timeout=DEFAULT_TIMEOUT, ) except requests.exceptions.ConnectionError as ex: - LOGGER.error( + LOGGER.exception( "Connection error while deleting certificate: %s", str(ex) ) raise GatewayAPIError(f"Connection error: {str(ex)}") except requests.exceptions.Timeout as ex: - LOGGER.error("Timeout while deleting certificate: %s", str(ex)) + LOGGER.exception("Timeout while deleting certificate: %s", str(ex)) raise GatewayAPIError(f"Request timeout: {str(ex)}") except requests.exceptions.RequestException as ex: - LOGGER.error( + LOGGER.exception( "Request error while deleting certificate: %s", str(ex) ) raise GatewayAPIError(f"Request error: {str(ex)}") @@ -240,15 +240,15 @@ def _fetch_from_gateway(self) -> Dict[str, Any]: timeout=DEFAULT_TIMEOUT, ) except requests.exceptions.ConnectionError as ex: - LOGGER.error( + LOGGER.exception( "Connection error while fetching certificate: %s", str(ex) ) raise GatewayAPIError(f"Connection error: {str(ex)}") except requests.exceptions.Timeout as ex: - LOGGER.error("Timeout while fetching certificate: %s", str(ex)) + LOGGER.exception("Timeout while fetching certificate: %s", str(ex)) raise GatewayAPIError(f"Request timeout: {str(ex)}") except requests.exceptions.RequestException as ex: - LOGGER.error( + LOGGER.exception( "Request error while fetching certificate: %s", str(ex) ) raise GatewayAPIError(f"Request error: {str(ex)}") @@ -301,6 +301,6 @@ def gw_handler( if len(objects) > 0: SyncCertificates(instance.id).update() except (GatewayAPIError, MissingCredentials) as ex: - LOGGER.error( + LOGGER.exception( "Couldn't trigger gateway certificate updates %s", str(ex) ) diff --git a/src/aap_eda/tasks/orchestrator.py b/src/aap_eda/tasks/orchestrator.py index a39f308c2..ceca75819 100644 --- a/src/aap_eda/tasks/orchestrator.py +++ b/src/aap_eda/tasks/orchestrator.py @@ -258,7 +258,7 @@ def queue_dispatch( "There may be an issue with the system; please contact " "the administrator." ) - LOGGER.error(msg) + LOGGER.exception(msg) status_manager.set_status( ActivationStatus.PENDING, msg, @@ -343,7 +343,7 @@ def _resolve_existing_queue( "There may be an issue with the system; " "please contact the administrator." ) - LOGGER.error(msg) + LOGGER.exception(msg) status_manager.set_status( ActivationStatus.PENDING, msg, @@ -432,7 +432,7 @@ def _handle_unhealthy_queue( "with the system; please contact the " "administrator." ) - LOGGER.error(msg) + LOGGER.exception(msg) status_manager.set_status( ActivationStatus.PENDING, msg, @@ -513,9 +513,7 @@ def check_rulebook_queue_health(queue_name: str) -> bool: ) return bool(alive) except Exception as e: - LOGGER.error( - f"Health check failed for queue {queue_name}: {e}", exc_info=True - ) + LOGGER.exception(f"Health check failed for queue {queue_name}: {e}") return False diff --git a/src/aap_eda/tasks/project.py b/src/aap_eda/tasks/project.py index 76505735f..a5c2c0bfb 100644 --- a/src/aap_eda/tasks/project.py +++ b/src/aap_eda/tasks/project.py @@ -46,7 +46,7 @@ def check_default_worker_health() -> bool: queue_name = utils.sanitize_postgres_identifier(PROJECT_TASKS_QUEUE) return check_rulebook_queue_health(queue_name) except Exception as e: - logger.error(f"Project queue health check failed: {e}", exc_info=True) + logger.exception(f"Project queue health check failed: {e}") return False @@ -95,21 +95,18 @@ def _import_project_no_lock(project_id: int): project.last_synced_at = timezone.now() project.save(update_fields=["last_synced_at"]) except ProjectImportError as e: - logger.error( + logger.exception( f"Project import error for project {project_id}: {e}", - exc_info=True, ) error_message = f"Import failed: {str(e)}" except DatabaseError as e: - logger.error( + logger.exception( f"Database error during project import {project_id}: {e}", - exc_info=True, ) error_message = "Database error during import" except Exception as e: - logger.error( + logger.exception( f"Unexpected error during project import {project_id}: {e}", - exc_info=True, ) error_message = f"Unexpected error during import: {str(e)}" finally: @@ -169,21 +166,18 @@ def _sync_project_no_lock(project_id: int): project.last_synced_at = timezone.now() project.save(update_fields=["last_synced_at"]) except ProjectImportError as e: - logger.error( + logger.exception( f"Project sync error for project {project_id}: {e}", - exc_info=True, ) error_message = f"Sync failed: {str(e)}" except DatabaseError as e: - logger.error( + logger.exception( f"Database error during project sync {project_id}: {e}", - exc_info=True, ) error_message = "Database error during sync" except Exception as e: - logger.error( + logger.exception( f"Unexpected error during project sync {project_id}: {e}", - exc_info=True, ) error_message = f"Unexpected error during sync: {str(e)}" @@ -205,18 +199,16 @@ def _handle_post_sync_activations(project: models.Project): try: _auto_restart_activations(project) except Exception as e: - logger.error( + logger.exception( f"Auto-restart failed for project {project.id}: {e}", - exc_info=True, ) try: _resume_waiting_activations(project) except Exception as e: - logger.error( + logger.exception( f"Resume waiting activations failed for " f"project {project.id}: {e}", - exc_info=True, ) @@ -247,11 +239,10 @@ def _auto_restart_activations(project: models.Project): f"'{activation.name}' no longer exists" ) except Exception as e: - logger.error( + logger.exception( f"Failed to check activation " f"'{activation.name}' for " f"auto-restart: {e}", - exc_info=True, ) logger.info( @@ -333,10 +324,9 @@ def _restart_activation(activation: models.Activation) -> bool: request_id="", ) except Exception as e: - logger.error( + logger.exception( f"Failed to restart activation " f"'{activation.name}' after sync: {e}", - exc_info=True, ) try: activation.status = ActivationStatus.ERROR @@ -345,10 +335,9 @@ def _restart_activation(activation: models.Activation) -> bool: ) activation.save(update_fields=["status", "status_message"]) except Exception as save_err: - logger.error( + logger.exception( f"Failed to set error state for " f"'{activation.name}': {save_err}", - exc_info=True, ) return False @@ -434,10 +423,9 @@ def _resume_waiting_activations(project: models.Project): request_id="", ) except Exception as e: - logger.error( + logger.exception( f"Failed to resume activation " f"'{activation.name}' after sync: {e}", - exc_info=True, ) try: activation.status = ActivationStatus.ERROR @@ -453,10 +441,9 @@ def _resume_waiting_activations(project: models.Project): ] ) except Exception as save_err: - logger.error( + logger.exception( f"Failed to update activation status " f"after resume failure: {save_err}", - exc_info=True, ) @@ -486,18 +473,16 @@ def _handle_sync_failure_activations(project_id: int, error_message: str): f"to ERROR after sync failure" ) except Exception as e: - logger.error( + logger.exception( f"Failed to update activation " f"'{activation.name}' after " f"sync failure: {e}", - exc_info=True, ) except Exception as e: - logger.error( + logger.exception( f"Failed to handle sync failure " f"activations for project " f"{project_id}: {e}", - exc_info=True, ) @@ -595,9 +580,8 @@ def _recover_stuck_projects( except ObjectDoesNotExist: logger.warning(f"Project {project.id} was deleted during recovery") except DatabaseError as e: - logger.error( + logger.exception( f"Failed to recover project {project.id}: {e}", - exc_info=True, ) return recovered @@ -660,10 +644,9 @@ def _recover_orphaned_awaiting_activations() -> int: ) recovered += 1 except Exception as e: - logger.error( + logger.exception( f"Failed to recover orphaned " f"activation '{activation.name}': {e}", - exc_info=True, ) return recovered @@ -679,7 +662,7 @@ def _get_project_safely(project_id: int) -> Optional[models.Project]: try: return models.Project.objects.get(pk=project_id) except ObjectDoesNotExist: - logger.error(f"Project {project_id} does not exist or was deleted") + logger.exception(f"Project {project_id} does not exist or was deleted") return None diff --git a/src/aap_eda/utils/__init__.py b/src/aap_eda/utils/__init__.py index db654e530..078e50915 100644 --- a/src/aap_eda/utils/__init__.py +++ b/src/aap_eda/utils/__init__.py @@ -28,7 +28,7 @@ def get_package_version(package_name: str) -> str: try: return importlib.metadata.version(package_name) except importlib.metadata.PackageNotFoundError: - logger.error( + logger.exception( "The package '%s' is not installed; returning 'unknown' " "version for it", package_name, diff --git a/src/aap_eda/wsapi/consumers.py b/src/aap_eda/wsapi/consumers.py index 1d785267f..857d67908 100644 --- a/src/aap_eda/wsapi/consumers.py +++ b/src/aap_eda/wsapi/consumers.py @@ -126,9 +126,9 @@ async def receive(self, text_data=None, bytes_data=None): else: logger.warning(f"Unsupported message received: {data}") except (DatabaseError, ObjectDoesNotExist) as err: - logger.error(f"Failed to parse {data} due to DB error: {err}") + logger.exception(f"Failed to parse {data} due to DB error: {err}") except InvalidEnvKeyError as err: - logger.error(f"Failed to parse {data} due to Env error: {err}") + logger.exception(f"Failed to parse {data} due to Env error: {err}") async def handle_workers(self, message: WorkerMessage): additional_credentials = [] @@ -280,7 +280,9 @@ def insert_audit_rule_data(self, message: ActionMessage) -> None: id=message.activation_id ) except ObjectDoesNotExist: - logger.error(f"RulebookProcess {message.activation_id} not found") + logger.exception( + f"RulebookProcess {message.activation_id} not found" + ) raise audit_rule = self._get_or_create_audit_rule( @@ -424,7 +426,9 @@ def get_activation(self, rulebook_process_id: str) -> models.Activation: ) return rulebook_process_instance.get_parent() except ObjectDoesNotExist: - logger.error(f"RulebookProcess {rulebook_process_id} not found") + logger.exception( + f"RulebookProcess {rulebook_process_id} not found" + ) raise @database_sync_to_async diff --git a/tests/integration/analytics/test_collector.py b/tests/integration/analytics/test_collector.py index aa4559a7b..eeeaa3c4f 100644 --- a/tests/integration/analytics/test_collector.py +++ b/tests/integration/analytics/test_collector.py @@ -174,7 +174,7 @@ def test_load_entries_with_invalid_json_logs_error(collector, mock_settings): ): result = collector._load_last_gathered_entries() assert result == {} - collector.logger.error.assert_called_once_with( + collector.logger.exception.assert_called_once_with( "Failed to load last entries: Expecting value: " "line 1 column 1 (char 0)" ) diff --git a/tests/integration/analytics/test_utils.py b/tests/integration/analytics/test_utils.py index 8bab7ada2..3dce9680b 100644 --- a/tests/integration/analytics/test_utils.py +++ b/tests/integration/analytics/test_utils.py @@ -250,7 +250,7 @@ def test_yaml_error_handling(): "aap_eda.analytics.utils.models.EdaCredential.objects.filter", return_value=[mock_credential], ), mock.patch( - "aap_eda.analytics.utils.logger.error" + "aap_eda.analytics.utils.logger.exception" ) as mock_logger: result = collect_controllers_info() assert result == {} @@ -284,12 +284,12 @@ def test_key_error_handling(error_input, error_msg): "aap_eda.analytics.utils.models.EdaCredential.objects.filter", return_value=[mock_credential], ), mock.patch( - "aap_eda.analytics.utils.logger.error" + "aap_eda.analytics.utils.logger.exception" ) as mock_logger: result = collect_controllers_info() assert result == {} if error_msg.startswith("Unexpected error"): - mock_logger.assert_called_with(f"{error_msg}", exc_info=True) + mock_logger.assert_called_with(f"{error_msg}") else: mock_logger.assert_called_with(f"{error_msg}") @@ -298,7 +298,7 @@ def test_key_error_handling(error_input, error_msg): "exception_cls,log_level", [ (RequestException, "warning"), - (TimeoutError, "error"), + (TimeoutError, "exception"), ], ) def test_request_exceptions(exception_cls, log_level): @@ -333,7 +333,6 @@ def test_request_exceptions(exception_cls, log_level): else: mock_logger.assert_called_with( f"Unexpected error processing credential 1: {exception_cls()}", - exc_info=True, ) @@ -366,7 +365,7 @@ def test_mixed_success_and_failure(): ), mock.patch( "aap_eda.analytics.utils.requests.get" ) as mock_get, mock.patch( - "aap_eda.analytics.utils.logger.error" + "aap_eda.analytics.utils.logger.exception" ) as mock_logger: mock_response = mock.MagicMock() mock_response.status_code = 200 @@ -379,7 +378,6 @@ def test_mixed_success_and_failure(): "Unexpected error processing credential 2: " "Invalid authentication configuration, must provide " "Token or username/password", - exc_info=True, ) diff --git a/tests/integration/core/test_dispatcherd_real.py b/tests/integration/core/test_dispatcherd_real.py index c7f649b90..b58886baf 100644 --- a/tests/integration/core/test_dispatcherd_real.py +++ b/tests/integration/core/test_dispatcherd_real.py @@ -241,8 +241,8 @@ def test_queue_cancel_job_connection_error(): queue_cancel_job(queue_name, job_id) # Verify error logging was called (lines 50-53) - mock_logger.error.assert_called_once() - call_args = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + call_args = mock_logger.exception.call_args # Verify error message format error_msg = call_args[0][0] @@ -251,8 +251,6 @@ def test_queue_cancel_job_connection_error(): in error_msg ) assert "Failed to connect to dispatcherd" in error_msg - # Verify exc_info=True was passed - assert call_args[1]["exc_info"] is True @pytest.mark.django_db @@ -275,8 +273,8 @@ def test_queue_cancel_job_control_interface_error(): queue_cancel_job(queue_name, job_id) # Verify error logging was called (lines 50-53) - mock_logger.error.assert_called_once() - call_args = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + call_args = mock_logger.exception.call_args # Verify error message format error_msg = call_args[0][0] @@ -285,8 +283,6 @@ def test_queue_cancel_job_control_interface_error(): in error_msg ) assert "Control timeout" in error_msg - # Verify exc_info=True was passed - assert call_args[1]["exc_info"] is True @pytest.mark.django_db @@ -309,8 +305,8 @@ def test_queue_cancel_job_general_exception(): queue_cancel_job(queue_name, job_id) # Verify error logging was called (lines 50-53) - mock_logger.error.assert_called_once() - call_args = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + call_args = mock_logger.exception.call_args # Verify error message format error_msg = call_args[0][0] @@ -319,5 +315,3 @@ def test_queue_cancel_job_general_exception(): in error_msg ) assert "Invalid format" in error_msg - # Verify exc_info=True was passed - assert call_args[1]["exc_info"] is True diff --git a/tests/integration/management/test_dispatcherd_commands.py b/tests/integration/management/test_dispatcherd_commands.py index 932a1c363..b577da449 100644 --- a/tests/integration/management/test_dispatcherd_commands.py +++ b/tests/integration/management/test_dispatcherd_commands.py @@ -215,8 +215,8 @@ def test_dispatcherd_command_general_exception(mock_logger): assert ( "Failed to start DefaultWorker: Service failed" in error_output ) - mock_logger.error.assert_called_with( - "Failed to start DefaultWorker: Service failed", exc_info=True + mock_logger.exception.assert_called_with( + "Failed to start DefaultWorker: Service failed" ) @@ -413,8 +413,8 @@ def test_dispatcherctl_command_general_exception(mock_logger): assert "Failed to execute status: Control failed" in str( exc_info.value ) - mock_logger.error.assert_called_with( - "Failed to execute status: Control failed", exc_info=True + mock_logger.exception.assert_called_with( + "Failed to execute status: Control failed" ) diff --git a/tests/integration/services/activation/test_drools_cleanup.py b/tests/integration/services/activation/test_drools_cleanup.py index c5fc7f5e5..17813f8da 100644 --- a/tests/integration/services/activation/test_drools_cleanup.py +++ b/tests/integration/services/activation/test_drools_cleanup.py @@ -317,9 +317,9 @@ def test_connection_error(self, mock_connect): assert result == {} # Verify error was logged - mock_logger.error.assert_called_once() + mock_logger.exception.assert_called_once() assert "Error during Drools cleanup" in str( - mock_logger.error.call_args + mock_logger.exception.call_args ) @patch("aap_eda.services.activation.drools_cleanup.psycopg.connect") diff --git a/tests/integration/tasks/test_project_dispatcherd.py b/tests/integration/tasks/test_project_dispatcherd.py index 21c8359c5..8dbbe3a33 100644 --- a/tests/integration/tasks/test_project_dispatcherd.py +++ b/tests/integration/tasks/test_project_dispatcherd.py @@ -65,11 +65,10 @@ def test_check_project_queue_health_exception_handling( assert result is False mock_health_check.assert_called_once_with("default") - mock_logger.error.assert_called_once() + mock_logger.exception.assert_called_once() # Verify the error message contains expected content - call_args = mock_logger.error.call_args + call_args = mock_logger.exception.call_args assert "Project queue health check failed" in call_args[0][0] - assert call_args[1]["exc_info"] is True @pytest.mark.django_db diff --git a/tests/integration/tasks/test_projects.py b/tests/integration/tasks/test_projects.py index e67bfa78e..831f95ea4 100644 --- a/tests/integration/tasks/test_projects.py +++ b/tests/integration/tasks/test_projects.py @@ -138,10 +138,9 @@ def test_check_default_worker_health_logs_exceptions( mock_check_rulebook_health.assert_called_once_with("default") # Verify error was logged with exception info - mock_logger.error.assert_called_once() - log_call = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + log_call = mock_logger.exception.call_args assert "Project queue health check failed" in log_call[0][0] - assert log_call[1]["exc_info"] is True @patch("aap_eda.tasks.project.check_rulebook_queue_health") @@ -372,8 +371,8 @@ def test_import_project_no_lock_project_import_error( _import_project_no_lock(project.id) mock_import_service.import_project.assert_called_once_with(project) - mock_logger.error.assert_called_once() - log_call = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + log_call = mock_logger.exception.call_args assert "Project import error for project" in log_call[0][0] # Should call error recovery to set FAILED state # (transaction.atomic rollback undoes the wrapper's state save) @@ -410,10 +409,9 @@ def test_import_project_no_lock_database_error( mock_recovery.assert_called_once_with( project.id, "Database error during import" ) - mock_logger.error.assert_called_once() - log_call = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + log_call = mock_logger.exception.call_args assert "Database error during project import" in log_call[0][0] - assert log_call[1]["exc_info"] is True @pytest.mark.django_db @@ -591,10 +589,9 @@ def test_import_project_no_lock_unexpected_exception( project.id, "Unexpected error during import: Something broke", ) - mock_logger.error.assert_called_once() - log_call = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + log_call = mock_logger.exception.call_args assert "Unexpected error during project import" in log_call[0][0] - assert log_call[1]["exc_info"] is True # ------------------------------------------------------------------ @@ -651,8 +648,8 @@ def test_sync_project_no_lock_project_import_error( _sync_project_no_lock(project.id) - mock_logger.error.assert_called_once() - log_call = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + log_call = mock_logger.exception.call_args assert "Project sync error for project" in log_call[0][0] mock_recovery.assert_called_once_with( project.id, "Sync failed: Sync failed" @@ -695,8 +692,7 @@ def test_sync_project_no_lock_database_error( mock_failure_handler.assert_called_once_with( project.id, "Database error during sync" ) - mock_logger.error.assert_called_once() - assert mock_logger.error.call_args[1]["exc_info"] is True + mock_logger.exception.assert_called_once() @pytest.mark.django_db @@ -736,8 +732,7 @@ def test_sync_project_no_lock_unexpected_exception( project.id, "Unexpected error during sync: Something broke", ) - mock_logger.error.assert_called_once() - assert mock_logger.error.call_args[1]["exc_info"] is True + mock_logger.exception.assert_called_once() # ------------------------------------------------------------------ @@ -804,7 +799,7 @@ def test_recover_stuck_projects_database_error( _monitor_project_tasks() - error_calls = [c[0][0] for c in mock_logger.error.call_args_list] + error_calls = [c[0][0] for c in mock_logger.exception.call_args_list] assert any("Failed to recover project" in msg for msg in error_calls) diff --git a/tests/unit/test_orchestrator.py b/tests/unit/test_orchestrator.py index d255d0ab5..c3a5c5a45 100644 --- a/tests/unit/test_orchestrator.py +++ b/tests/unit/test_orchestrator.py @@ -513,10 +513,9 @@ def test_check_rulebook_queue_health_logs_exceptions( assert result is False # Verify error was logged with exception info - mock_logger.error.assert_called_once() - log_call = mock_logger.error.call_args + mock_logger.exception.assert_called_once() + log_call = mock_logger.exception.call_args assert "Health check failed for queue test_queue" in log_call[0][0] - assert log_call[1]["exc_info"] is True @pytest.mark.django_db From 028f3c2bf47369b4e409de839917dd6e553726ae Mon Sep 17 00:00:00 2001 From: Jacob Craiglow Date: Thu, 23 Jul 2026 11:18:49 -0400 Subject: [PATCH 2/3] sonar issues: Logging exception instead of logging errors for better traceback Signed-off-by: Jacob Craiglow --- src/aap_eda/analytics/analytics_collectors.py | 12 +- src/aap_eda/analytics/collector.py | 4 +- src/aap_eda/analytics/utils.py | 16 +- src/aap_eda/api/serializers/activation.py | 2 +- src/aap_eda/api/views/event_stream.py | 7 +- src/aap_eda/core/utils/credentials.py | 10 +- src/aap_eda/core/utils/rulebook.py | 2 +- .../services/activation/activation_manager.py | 4 +- .../services/activation/engine/podman.py | 8 +- .../services/activation/engine/ports.py | 2 +- src/aap_eda/services/pg_notify.py | 2 +- src/aap_eda/services/sync_certs.py | 36 +--- src/aap_eda/tasks/orchestrator.py | 4 +- src/aap_eda/tasks/project.py | 4 +- src/aap_eda/wsapi/consumers.py | 8 +- .../analytics/test_analytics_collectors.py | 2 +- tests/integration/analytics/test_collector.py | 3 +- tests/integration/analytics/test_utils.py | 14 +- tests/integration/api/test_activation.py | 61 ++++++ tests/integration/api/test_credential_type.py | 36 ++++ tests/integration/api/test_eda_credential.py | 49 +++++ tests/integration/api/test_event_stream.py | 105 ++++++++++ .../api/test_event_stream_basic.py | 184 ++++++++++++++++- tests/integration/api/test_project.py | 26 +++ tests/integration/core/test_health.py | 57 +++++- tests/integration/core/test_rulebook.py | 17 ++ .../activation/engine/test_kubernetes.py | 55 +++++ .../services/activation/engine/test_podman.py | 81 ++++++++ .../services/activation/test_manager.py | 93 +++++++++ tests/integration/services/test_sync_certs.py | 191 +++++------------- tests/integration/wsapi/test_consumer.py | 52 +++++ tests/unit/services/test_pg_notify.py | 25 +++ tests/unit/test_credential_plugins.py | 18 ++ tests/unit/test_credential_validation.py | 52 +++++ tests/unit/test_external_sms.py | 36 ++++ 35 files changed, 1055 insertions(+), 223 deletions(-) diff --git a/src/aap_eda/analytics/analytics_collectors.py b/src/aap_eda/analytics/analytics_collectors.py index 0dc67aff3..af228aa34 100644 --- a/src/aap_eda/analytics/analytics_collectors.py +++ b/src/aap_eda/analytics/analytics_collectors.py @@ -810,12 +810,12 @@ def _copy_table( cursor.execute(f"DROP VIEW IF EXISTS {view_name}") return file.file_list() - except DatabaseError as e: - logger.exception(f"Database error occurred: {e}") + except DatabaseError: + logger.exception("Database error occurred") return None - except IOError as e: - logger.exception(f"File I/O error occurred: {e}") + except IOError: + logger.exception("File I/O error occurred") return None - except Exception as e: - logger.exception(f"An unexpected error occurred: {e}") + except Exception: + logger.exception("An unexpected error occurred") return None diff --git a/src/aap_eda/analytics/collector.py b/src/aap_eda/analytics/collector.py index eb56df794..e5b15bfab 100644 --- a/src/aap_eda/analytics/collector.py +++ b/src/aap_eda/analytics/collector.py @@ -65,8 +65,8 @@ def _load_last_gathered_entries(self) -> dict: self.logger.info(f"Last collect entries: {last_entries}") return json.loads(last_entries, object_hook=utils.datetime_hook) - except (json.JSONDecodeError, TypeError) as e: - self.logger.exception(f"Failed to load last entries: {str(e)}") + except (json.JSONDecodeError, TypeError): + self.logger.exception("Failed to load last entries") return {} def _save_last_gathered_entries(self, last_gathered_entries: dict) -> None: diff --git a/src/aap_eda/analytics/utils.py b/src/aap_eda/analytics/utils.py index efd1f93ce..de6e6075e 100644 --- a/src/aap_eda/analytics/utils.py +++ b/src/aap_eda/analytics/utils.py @@ -91,12 +91,12 @@ def collect_controllers_info() -> dict: controller_info["install_uuid"] = resp.json()["install_uuid"] info[host] = controller_info - except KeyError as e: - logger.exception(f"Missing key in credential inputs: {e}") + except KeyError: + logger.exception("Missing key in credential inputs") continue - except yaml.YAMLError as e: + except yaml.YAMLError: logger.exception( - f"YAML parsing error for credential {credential.id}: {e}" + "YAML parsing error for credential" f" {credential.id}" ) continue except requests.exceptions.RequestException as e: @@ -104,9 +104,9 @@ def collect_controllers_info() -> dict: f"Controller connection failed for {credential.name}: {e}" ) continue - except Exception as e: + except Exception: logger.exception( - f"Unexpected error processing credential {credential.id}: {e}" + "Unexpected error processing credential" f" {credential.id}" ) continue @@ -210,8 +210,8 @@ def generate_token() -> ServiceToken: except requests.exceptions.Timeout: logger.exception("Token request timed out") raise - except requests.exceptions.RequestException as e: - logger.exception(f"Token request failed: {str(e)}") + except requests.exceptions.RequestException: + logger.exception("Token request failed") raise data = resp.json() diff --git a/src/aap_eda/api/serializers/activation.py b/src/aap_eda/api/serializers/activation.py index c6af4570c..9b55d5d3c 100644 --- a/src/aap_eda/api/serializers/activation.py +++ b/src/aap_eda/api/serializers/activation.py @@ -1619,7 +1619,7 @@ def _validate_sources_with_event_streams(data: dict) -> None: try: source_mappings = yaml.safe_load(source_mappings) except yaml.MarkedYAMLError as ex: - logger.exception("Invalid source mappings: %s", str(ex)) + logger.exception("Invalid source mappings") raise serializers.ValidationError( { SOURCE_MAPPING_ERROR_KEY: [ diff --git a/src/aap_eda/api/views/event_stream.py b/src/aap_eda/api/views/event_stream.py index b56762055..b062c2ab3 100644 --- a/src/aap_eda/api/views/event_stream.py +++ b/src/aap_eda/api/views/event_stream.py @@ -366,18 +366,15 @@ def _sync_certificates( else: obj.update() except CoreGatewayAPIError as ex: - logger.exception( - "Could not %s certificates: %s", action, str(ex) - ) + logger.exception("Could not %s certificates", action) raise api_exc.GatewayAPIError( detail=f"Gateway API error during certificate {action}: " f"{str(ex)}" ) except CoreMissingCredentials as ex: logger.exception( - "Missing credentials for certificate %s: %s", + "Missing credentials for certificate %s", action, - str(ex), ) raise api_exc.MissingCredentialsError( detail=f"Missing credentials for certificate {action}: " diff --git a/src/aap_eda/core/utils/credentials.py b/src/aap_eda/core/utils/credentials.py index 134e05047..8e8480630 100644 --- a/src/aap_eda/core/utils/credentials.py +++ b/src/aap_eda/core/utils/credentials.py @@ -947,8 +947,8 @@ def validate_x509_subject_match(expected: str, actual: str) -> bool: # Parse actual DN into X.509 Name object try: actual_name = x509.Name.from_rfc4514_string(actual) - except ValueError as e: - LOGGER.exception(f"Invalid actual DN format: '{actual}': {e}") + except ValueError: + LOGGER.exception(f"Invalid actual DN format: '{actual}'") return False # Parse expected DN manually to handle regex patterns @@ -1087,8 +1087,8 @@ def _match_regex_pattern_against_attrs( re.match(regex_pattern, attr.value, re.IGNORECASE) for attr in actual_attrs ) - except re.error as e: - LOGGER.exception(f"Invalid regex pattern '{pattern}': {e}") + except re.error: + LOGGER.exception(f"Invalid regex pattern '{pattern}'") return False @@ -1123,7 +1123,7 @@ def _get_aes_key( ) return base64.b64encode(raw_key).decode("utf-8") except Exception as e: - LOGGER.exception(f"Failed to derive AES key: {e}") + LOGGER.exception("Failed to derive AES key") raise ValidationError(f"Failed to generate encryption key: {str(e)}") diff --git a/src/aap_eda/core/utils/rulebook.py b/src/aap_eda/core/utils/rulebook.py index f2abebdcd..724e40d86 100644 --- a/src/aap_eda/core/utils/rulebook.py +++ b/src/aap_eda/core/utils/rulebook.py @@ -41,7 +41,7 @@ def build_source_list(rulesets_data: str) -> list[dict]: try: rulesets = yaml.safe_load(rulesets_data) except yaml.MarkedYAMLError as ex: - LOGGER.exception("Invalid rulesets: %s", str(ex)) + LOGGER.exception("Invalid rulesets") raise ParseError("Failed to parse rulebook data") from ex rulebook_hash = get_rulebook_hash(rulesets_data) diff --git a/src/aap_eda/services/activation/activation_manager.py b/src/aap_eda/services/activation/activation_manager.py index 031bcb8fc..e3220c76a 100644 --- a/src/aap_eda/services/activation/activation_manager.py +++ b/src/aap_eda/services/activation/activation_manager.py @@ -856,11 +856,11 @@ def monitor(self): try: self._check_latest_instance_and_pod_id() except exceptions.ActivationInstanceNotFound as e: - LOGGER.exception(f"Monitor operation Failed: {e}") + LOGGER.exception("Monitor operation Failed") self._error_activation(f"{e}") raise exceptions.ActivationMonitorError(f"{e}") except exceptions.ActivationInstancePodIdNotFound as e: - LOGGER.exception(f"Monitor operation Failed: {e}") + LOGGER.exception("Monitor operation Failed") self._error_activation(f"{e}") self._error_instance(f"{e}") raise exceptions.ActivationMonitorError(f"{e}") diff --git a/src/aap_eda/services/activation/engine/podman.py b/src/aap_eda/services/activation/engine/podman.py index 0418529a0..ce8567241 100644 --- a/src/aap_eda/services/activation/engine/podman.py +++ b/src/aap_eda/services/activation/engine/podman.py @@ -56,7 +56,7 @@ def get_podman_client() -> PodmanClient: try: return PodmanClient(**params) except ValueError as e: - LOGGER.exception(f"Failed to initialize podman client: f{e}") + LOGGER.exception("Failed to initialize podman client") raise exceptions.ContainerEngineInitError(str(e)) from e @@ -75,7 +75,7 @@ def __init__( LOGGER.debug(self.client.version()) except APIError as e: - LOGGER.exception(f"Failed to initialize podman engine: f{e}") + LOGGER.exception("Failed to initialize podman engine") raise exceptions.ContainerEngineInitError(str(e)) self.JobTimeoutException = self._get_job_timeout_exception() @@ -354,7 +354,7 @@ def _cleanup(self, container_id: str, _log_handler: LogHandler) -> None: except NotFound: LOGGER.warning(f"Container {container_id} not found.") except APIError as e: - LOGGER.exception(f"Failed to cleanup {container_id}: {e}") + LOGGER.exception(f"Failed to cleanup {container_id}") raise exceptions.ContainerCleanupError(str(e)) def _get_ports(self, found_ports: list[tuple]) -> dict: @@ -427,7 +427,7 @@ def _pull_image( LOGGER.exception(msg) log_handler.write(msg, True) raise exceptions.ContainerImagePullError(msg) - LOGGER.exception(f"Failed to pull image {request.image_url}: {e}") + LOGGER.exception(f"Failed to pull image {request.image_url}") raise exceptions.ContainerStartError(str(e)) except self.JobTimeoutException as e: msg = f"Timeout: {e}" diff --git a/src/aap_eda/services/activation/engine/ports.py b/src/aap_eda/services/activation/engine/ports.py index 9e4ed2353..9a8f14b0f 100644 --- a/src/aap_eda/services/activation/engine/ports.py +++ b/src/aap_eda/services/activation/engine/ports.py @@ -56,7 +56,7 @@ def _extract_port(source, context): return (host, int(maybe_port)) return None except ValueError as e: - LOGGER.exception(f"find_ports error: {e}") + LOGGER.exception("find_ports error") raise exceptions.ActivationStartError(str(e)) except (UndefinedError, SecurityError) as e: raise exceptions.ActivationStartError(str(e)) diff --git a/src/aap_eda/services/pg_notify.py b/src/aap_eda/services/pg_notify.py index 7b301e98d..c2bbdfef2 100644 --- a/src/aap_eda/services/pg_notify.py +++ b/src/aap_eda/services/pg_notify.py @@ -98,5 +98,5 @@ def __call__(self): "SELECT pg_notify(%s, %s)", [self.channel, payload] ) except psycopg.OperationalError as e: - logger.exception("PG Notify operational error %s", str(e)) + logger.exception("PG Notify operational error") raise PGNotifyError() from e diff --git a/src/aap_eda/services/sync_certs.py b/src/aap_eda/services/sync_certs.py index cc12b4e5c..bf23edc73 100644 --- a/src/aap_eda/services/sync_certs.py +++ b/src/aap_eda/services/sync_certs.py @@ -142,17 +142,13 @@ def _make_request( ) return response except requests.exceptions.ConnectionError as ex: - LOGGER.exception( - "Connection error while updating certificate: %s", str(ex) - ) + LOGGER.exception("Connection error while updating certificate") raise GatewayAPIError(f"Connection error: {str(ex)}") except requests.exceptions.Timeout as ex: - LOGGER.exception("Timeout while updating certificate: %s", str(ex)) + LOGGER.exception("Timeout while updating certificate") raise GatewayAPIError(f"Request timeout: {str(ex)}") except requests.exceptions.RequestException as ex: - LOGGER.exception( - "Request error while updating certificate: %s", str(ex) - ) + LOGGER.exception("Request error while updating certificate") raise GatewayAPIError(f"Request error: {str(ex)}") def _handle_certificate_response( @@ -199,17 +195,13 @@ def _delete_from_gateway(self, existing_object: Dict[str, Any]) -> None: timeout=DEFAULT_TIMEOUT, ) except requests.exceptions.ConnectionError as ex: - LOGGER.exception( - "Connection error while deleting certificate: %s", str(ex) - ) + LOGGER.exception("Connection error while deleting certificate") raise GatewayAPIError(f"Connection error: {str(ex)}") except requests.exceptions.Timeout as ex: - LOGGER.exception("Timeout while deleting certificate: %s", str(ex)) + LOGGER.exception("Timeout while deleting certificate") raise GatewayAPIError(f"Request timeout: {str(ex)}") except requests.exceptions.RequestException as ex: - LOGGER.exception( - "Request error while deleting certificate: %s", str(ex) - ) + LOGGER.exception("Request error while deleting certificate") raise GatewayAPIError(f"Request error: {str(ex)}") if response.status_code in [ @@ -240,17 +232,13 @@ def _fetch_from_gateway(self) -> Dict[str, Any]: timeout=DEFAULT_TIMEOUT, ) except requests.exceptions.ConnectionError as ex: - LOGGER.exception( - "Connection error while fetching certificate: %s", str(ex) - ) + LOGGER.exception("Connection error while fetching certificate") raise GatewayAPIError(f"Connection error: {str(ex)}") except requests.exceptions.Timeout as ex: - LOGGER.exception("Timeout while fetching certificate: %s", str(ex)) + LOGGER.exception("Timeout while fetching certificate") raise GatewayAPIError(f"Request timeout: {str(ex)}") except requests.exceptions.RequestException as ex: - LOGGER.exception( - "Request error while fetching certificate: %s", str(ex) - ) + LOGGER.exception("Request error while fetching certificate") raise GatewayAPIError(f"Request error: {str(ex)}") if response.status_code == status.HTTP_200_OK: @@ -300,7 +288,5 @@ def gw_handler( ) if len(objects) > 0: SyncCertificates(instance.id).update() - except (GatewayAPIError, MissingCredentials) as ex: - LOGGER.exception( - "Couldn't trigger gateway certificate updates %s", str(ex) - ) + except (GatewayAPIError, MissingCredentials): + LOGGER.exception("Couldn't trigger gateway certificate updates") diff --git a/src/aap_eda/tasks/orchestrator.py b/src/aap_eda/tasks/orchestrator.py index ceca75819..7bbf6026c 100644 --- a/src/aap_eda/tasks/orchestrator.py +++ b/src/aap_eda/tasks/orchestrator.py @@ -512,8 +512,8 @@ def check_rulebook_queue_health(queue_name: str) -> bool: f"Worker queue [{queue_name}] was found to not be healthy" ) return bool(alive) - except Exception as e: - LOGGER.exception(f"Health check failed for queue {queue_name}: {e}") + except Exception: + LOGGER.exception(f"Health check failed for queue {queue_name}") return False diff --git a/src/aap_eda/tasks/project.py b/src/aap_eda/tasks/project.py index a5c2c0bfb..78bd41546 100644 --- a/src/aap_eda/tasks/project.py +++ b/src/aap_eda/tasks/project.py @@ -45,8 +45,8 @@ def check_default_worker_health() -> bool: try: queue_name = utils.sanitize_postgres_identifier(PROJECT_TASKS_QUEUE) return check_rulebook_queue_health(queue_name) - except Exception as e: - logger.exception(f"Project queue health check failed: {e}") + except Exception: + logger.exception("Project queue health check failed") return False diff --git a/src/aap_eda/wsapi/consumers.py b/src/aap_eda/wsapi/consumers.py index 857d67908..21686c6c3 100644 --- a/src/aap_eda/wsapi/consumers.py +++ b/src/aap_eda/wsapi/consumers.py @@ -125,10 +125,10 @@ async def receive(self, text_data=None, bytes_data=None): await self.handle_heartbeat(HeartbeatMessage.parse_obj(data)) else: logger.warning(f"Unsupported message received: {data}") - except (DatabaseError, ObjectDoesNotExist) as err: - logger.exception(f"Failed to parse {data} due to DB error: {err}") - except InvalidEnvKeyError as err: - logger.exception(f"Failed to parse {data} due to Env error: {err}") + except (DatabaseError, ObjectDoesNotExist): + logger.exception(f"Failed to parse {data} due to DB error") + except InvalidEnvKeyError: + logger.exception(f"Failed to parse {data} due to Env error") async def handle_workers(self, message: WorkerMessage): additional_credentials = [] diff --git a/tests/integration/analytics/test_analytics_collectors.py b/tests/integration/analytics/test_analytics_collectors.py index 24f540715..1778f1348 100644 --- a/tests/integration/analytics/test_analytics_collectors.py +++ b/tests/integration/analytics/test_analytics_collectors.py @@ -947,7 +947,7 @@ def test_database_error_propagation(tmp_path, mock_queryset, caplog_factory): result = _copy_table("test", mock_queryset, str(tmp_path)) assert result is None - assert "Database error occurred: View creation failed" in eda_log.text + assert "Database error occurred" in eda_log.text @pytest.mark.django_db diff --git a/tests/integration/analytics/test_collector.py b/tests/integration/analytics/test_collector.py index eeeaa3c4f..d2d23d172 100644 --- a/tests/integration/analytics/test_collector.py +++ b/tests/integration/analytics/test_collector.py @@ -175,8 +175,7 @@ def test_load_entries_with_invalid_json_logs_error(collector, mock_settings): result = collector._load_last_gathered_entries() assert result == {} collector.logger.exception.assert_called_once_with( - "Failed to load last entries: Expecting value: " - "line 1 column 1 (char 0)" + "Failed to load last entries" ) diff --git a/tests/integration/analytics/test_utils.py b/tests/integration/analytics/test_utils.py index 3dce9680b..059a570a3 100644 --- a/tests/integration/analytics/test_utils.py +++ b/tests/integration/analytics/test_utils.py @@ -255,7 +255,7 @@ def test_yaml_error_handling(): result = collect_controllers_info() assert result == {} args, _ = mock_logger.call_args - assert args[0].startswith("YAML parsing error for credential 1: ") + assert args[0] == "YAML parsing error for credential 1" @pytest.mark.parametrize( @@ -263,13 +263,11 @@ def test_yaml_error_handling(): [ ( yaml.dump({"verify_ssl": "True", "oauth_token": "token"}), - "Missing key in credential inputs: 'host'", + "Missing key in credential inputs", ), ( yaml.dump({"host": "https://test", "auth": {"type": "basic"}}), - "Unexpected error processing credential 1: " - "Invalid authentication configuration, must provide " - "Token or username/password", + "Unexpected error processing credential 1", ), ], ) @@ -332,7 +330,7 @@ def test_request_exceptions(exception_cls, log_level): ) else: mock_logger.assert_called_with( - f"Unexpected error processing credential 1: {exception_cls()}", + "Unexpected error processing credential 1", ) @@ -375,9 +373,7 @@ def test_mixed_success_and_failure(): result = collect_controllers_info() assert list(result.keys()) == ["https://good"] mock_logger.assert_called_with( - "Unexpected error processing credential 2: " - "Invalid authentication configuration, must provide " - "Token or username/password", + "Unexpected error processing credential 2", ) diff --git a/tests/integration/api/test_activation.py b/tests/integration/api/test_activation.py index 900a7e6e4..e9201b366 100644 --- a/tests/integration/api/test_activation.py +++ b/tests/integration/api/test_activation.py @@ -22,7 +22,9 @@ from rest_framework import status from rest_framework.test import APIClient +from aap_eda.api.exceptions import InvalidEventStreamSource from aap_eda.api.serializers.activation import ( + _update_event_stream_source, get_rules_count, is_activation_valid, ) @@ -1587,6 +1589,46 @@ def test_enable_sync_failure_resets_flag( assert default_activation.status == (enums.ActivationStatus.ERROR) +@pytest.mark.django_db +@mock.patch.object(settings, "RULEBOOK_WORKER_QUEUES", []) +@mock.patch( + "aap_eda.api.views.activation.check_dispatcherd_workers_health", +) +@mock.patch("aap_eda.api.views.activation.sync_project") +@mock.patch("aap_eda.api.views.activation.logger") +def test_enable_sync_failure_logs_exception( + mock_logger, + mock_sync, + mock_health, + default_activation: models.Activation, + default_project: models.Project, + admin_client: APIClient, + preseed_credential_types, +): + """Enable logs via logger.exception when sync_project throws.""" + mock_sync.side_effect = RuntimeError("dispatcherd down") + default_project.update_revision_on_launch = True + default_project.scm_update_cache_timeout = 0 + default_project.save( + update_fields=[ + "update_revision_on_launch", + "scm_update_cache_timeout", + ] + ) + default_activation.is_enabled = False + default_activation.status = enums.ActivationStatus.STOPPED + default_activation.save(update_fields=["is_enabled", "status"]) + + response = admin_client.post( + f"{api_url_v1}/activations/" f"{default_activation.id}/enable/" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Failed to start project sync" in log_msg + + @pytest.mark.django_db @mock.patch.object(settings, "RULEBOOK_WORKER_QUEUES", []) @mock.patch( @@ -2897,3 +2939,22 @@ def test_create_activation_with_only_rule_engine_credential( assert activation.rule_engine_credential.id == ( rule_engine_credential.id ), "Rule engine credential should be in FK field" + + +@pytest.mark.django_db +@mock.patch("aap_eda.api.serializers.activation.logger") +def test_update_event_stream_source_logs_exception_on_failure( + mock_logger, +): + """_update_event_stream_source logs via logger.exception on error.""" + validated_data = { + "source_mappings": "[{event_stream_id: 99999}]", + "rulebook_rulesets": "---", + } + + with pytest.raises(InvalidEventStreamSource): + _update_event_stream_source(validated_data) + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Failed to update event stream source" in log_msg diff --git a/tests/integration/api/test_credential_type.py b/tests/integration/api/test_credential_type.py index 797517f42..6cf16d96d 100644 --- a/tests/integration/api/test_credential_type.py +++ b/tests/integration/api/test_credential_type.py @@ -1267,3 +1267,39 @@ def test_eda_rule_engine_credential_validates_required_fields( assert response.status_code == status.HTTP_400_BAD_REQUEST assert "postgres_db_name" in str(response.data) + + +@pytest.mark.django_db +def test_credential_type_test_logs_exception_on_plugin_failure( + superuser_client: APIClient, + preseed_credential_types, +): + """Verify logger.exception is called when run_plugin raises.""" + hashi_type = models.CredentialType.objects.get( + name=enums.DefaultCredentialType.HASHICORP_LOOKUP + ) + + data_in = { + "inputs": { + "url": "https://www.example.com", + "api_version": "v2", + "token": "token123", + }, + "metadata": { + "secret_path": "secret/foo", + "secret_key": "bar", + }, + } + + url = f"{api_url_v1}/credential-types/{hashi_type.id}/test/" + with patch( + "aap_eda.api.views.credential_type.run_plugin", + side_effect=Exception("kaboom"), + ), patch( + "aap_eda.api.views.credential_type.logger", + ) as mock_logger: + response = superuser_client.post(url, data=data_in) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + mock_logger.exception.assert_called_once() + assert "Plugin call failed" in mock_logger.exception.call_args[0][0] diff --git a/tests/integration/api/test_eda_credential.py b/tests/integration/api/test_eda_credential.py index 35fa9cb38..70de9d835 100644 --- a/tests/integration/api/test_eda_credential.py +++ b/tests/integration/api/test_eda_credential.py @@ -2812,3 +2812,52 @@ def test_list_eda_credentials_filter_kind_and_namespace_not_in( assert ( response.data["results"][0]["credential_type"]["namespace"] != "drools" ) + + +@pytest.mark.django_db +def test_eda_credential_test_logs_exception_on_plugin_failure( + admin_client: APIClient, + default_organization: models.Organization, + preseed_credential_types, +): + """Verify logger.exception is called when run_plugin raises.""" + credential_type = models.CredentialType.objects.get( + name=enums.DefaultCredentialType.HASHICORP_LOOKUP + ) + + data_in = { + "name": "eda-credential-log-test", + "inputs": { + "url": "https://www.example.com", + "api_version": "v2", + "token": secrets.token_hex(32), + }, + "credential_type_id": credential_type.id, + "organization_id": default_organization.id, + } + + response = admin_client.post( + f"{api_url_v1}/eda-credentials/", data=data_in + ) + assert response.status_code == status.HTTP_201_CREATED + obj = response.json() + + with patch( + "aap_eda.api.views.eda_credential.run_plugin", + side_effect=Exception("kaboom"), + ), patch( + "aap_eda.api.views.eda_credential.logger", + ) as mock_logger: + response = admin_client.post( + f"{api_url_v1}/eda-credentials/{obj['id']}/test/", + data={ + "metadata": { + "secret_path": "secret/foo", + "secret_key": "bar", + }, + }, + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + mock_logger.exception.assert_called_once() + assert "call failed" in mock_logger.exception.call_args[0][0] diff --git a/tests/integration/api/test_event_stream.py b/tests/integration/api/test_event_stream.py index 7da871f25..550a096cc 100644 --- a/tests/integration/api/test_event_stream.py +++ b/tests/integration/api/test_event_stream.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import hmac +import logging import secrets import uuid from typing import List @@ -24,6 +25,7 @@ from rest_framework.renderers import JSONRenderer from rest_framework.test import APIClient +from aap_eda.api.views.event_stream import logger as event_stream_logger from aap_eda.core import enums, models from aap_eda.core.exceptions import ( GatewayAPIError as CoreGatewayAPIError, @@ -1007,3 +1009,106 @@ def test_create_event_stream_sync_success( # Verify event stream was created event_stream = models.EventStream.objects.get(name="test-stream") assert event_stream.eda_credential == credential + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.SyncCertificates.update") +def test_sync_certificates_gateway_error_logs_exception( + mock_sync_update, + admin_client: APIClient, + default_organization: models.Organization, + preseed_credential_types, + caplog_factory, +): + """Verify logger.exception is called for CoreGatewayAPIError.""" + eda_caplog = caplog_factory(event_stream_logger, logging.ERROR) + + mtls_type = models.CredentialType.objects.get( + name=enums.EventStreamCredentialType.MTLS + ) + credential = models.EdaCredential.objects.create( + name="mtls-credential", + inputs={ + "auth_type": "mtls", + "certificate": "", + "http_header_key": "Subject", + }, + credential_type=mtls_type, + organization=default_organization, + ) + + mock_sync_update.side_effect = CoreGatewayAPIError( + "Gateway connection timeout" + ) + + data_in = { + "name": "test-stream-log", + "eda_credential_id": credential.id, + "organization_id": default_organization.id, + } + + with override_settings( + EVENT_STREAM_BASE_URL="https://www.example.com/", + EVENT_STREAM_MTLS_BASE_URL="https://www.example.com/", + ): + admin_client.post(f"{api_url_v1}/event-streams/", data=data_in) + + error_records = [ + r for r in eda_caplog.records if r.levelno >= logging.ERROR + ] + assert any( + "Could not create certificates" in r.message for r in error_records + ) + assert any(r.exc_info is not None for r in error_records) + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.SyncCertificates.update") +def test_sync_certificates_missing_credentials_logs_exception( + mock_sync_update, + admin_client: APIClient, + default_organization: models.Organization, + preseed_credential_types, + caplog_factory, +): + """Verify logger.exception is called for CoreMissingCredentials.""" + eda_caplog = caplog_factory(event_stream_logger, logging.ERROR) + + mtls_type = models.CredentialType.objects.get( + name=enums.EventStreamCredentialType.MTLS + ) + credential = models.EdaCredential.objects.create( + name="mtls-credential", + inputs={ + "auth_type": "mtls", + "certificate": "", + "http_header_key": "Subject", + }, + credential_type=mtls_type, + organization=default_organization, + ) + + mock_sync_update.side_effect = CoreMissingCredentials( + "Required credentials not found" + ) + + data_in = { + "name": "test-stream-log", + "eda_credential_id": credential.id, + "organization_id": default_organization.id, + } + + with override_settings( + EVENT_STREAM_BASE_URL="https://www.example.com/", + EVENT_STREAM_MTLS_BASE_URL="https://www.example.com/", + ): + admin_client.post(f"{api_url_v1}/event-streams/", data=data_in) + + error_records = [ + r for r in eda_caplog.records if r.levelno >= logging.ERROR + ] + assert any( + "Missing credentials for certificate create" in r.message + for r in error_records + ) + assert any(r.exc_info is not None for r in error_records) diff --git a/tests/integration/api/test_event_stream_basic.py b/tests/integration/api/test_event_stream_basic.py index f9af157f3..a662dea51 100644 --- a/tests/integration/api/test_event_stream_basic.py +++ b/tests/integration/api/test_event_stream_basic.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import base64 +import logging import secrets from unittest import mock from urllib.parse import urlencode @@ -20,8 +21,11 @@ from rest_framework import status from rest_framework.test import APIClient +from aap_eda.api.views.external_event_stream import ( + logger as external_event_stream_logger, +) from aap_eda.core import enums, models -from aap_eda.core.exceptions import CredentialPluginError +from aap_eda.core.exceptions import CredentialPluginError, PGNotifyError from tests.integration.api.test_event_stream import ( create_event_stream, create_event_stream_credential, @@ -179,3 +183,181 @@ def test_post_event_stream_with_basic_auth_bad_encoding( ) assert response.status_code == status + + +@pytest.mark.django_db +def test_parse_body_yaml_error_logs_exception( + admin_client: APIClient, + preseed_credential_types, + caplog_factory, +): + """Verify logger.exception is called for YAMLError in _parse_body.""" + eda_caplog = caplog_factory(external_event_stream_logger, logging.ERROR) + + secret = secrets.token_hex(32) + username = "fred" + inputs = { + "auth_type": "basic", + "username": username, + "password": secret, + "http_header_key": "Authorization", + } + + obj = create_event_stream_credential( + admin_client, + enums.EventStreamCredentialType.BASIC.value, + inputs, + ) + + data_in = { + "name": "test-es-yaml-log", + "eda_credential_id": obj["id"], + "organization_id": get_default_test_org().id, + "test_mode": True, + } + event_stream = create_event_stream(admin_client, data_in) + user_pass = f"{username}:{secret}" + auth_value = f"Basic {base64.b64encode(user_pass.encode()).decode()}" + + # Invalid JSON triggers YAMLError in yaml.safe_load + content_type = "application/json" + data_bytes = '{"a": 1,'.encode() + headers = { + "Authorization": auth_value, + "Content-Type": content_type, + } + + response = admin_client.post( + event_stream_post_url(event_stream.uuid), + headers=headers, + data=data_bytes, + content_type=content_type, + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + error_records = [ + r for r in eda_caplog.records if r.levelno >= logging.ERROR + ] + assert any("Invalid content" in r.message for r in error_records) + assert any(r.exc_info is not None for r in error_records) + + +@pytest.mark.django_db +def test_parse_body_value_error_logs_exception( + admin_client: APIClient, + preseed_credential_types, + caplog_factory, +): + """Verify logger.exception is called for ValueError in _parse_body.""" + eda_caplog = caplog_factory(external_event_stream_logger, logging.ERROR) + + secret = secrets.token_hex(32) + username = "fred" + inputs = { + "auth_type": "basic", + "username": username, + "password": secret, + "http_header_key": "Authorization", + } + + obj = create_event_stream_credential( + admin_client, + enums.EventStreamCredentialType.BASIC.value, + inputs, + ) + + data_in = { + "name": "test-es-value-log", + "eda_credential_id": obj["id"], + "organization_id": get_default_test_org().id, + "test_mode": True, + } + event_stream = create_event_stream(admin_client, data_in) + user_pass = f"{username}:{secret}" + auth_value = f"Basic {base64.b64encode(user_pass.encode()).decode()}" + + # Invalid form-urlencoded data triggers ValueError + content_type = "application/x-www-form-urlencoded" + data_bytes = '{"a": 1,'.encode() + headers = { + "Authorization": auth_value, + "Content-Type": content_type, + } + + response = admin_client.post( + event_stream_post_url(event_stream.uuid), + headers=headers, + data=data_bytes, + content_type=content_type, + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + error_records = [ + r for r in eda_caplog.records if r.levelno >= logging.ERROR + ] + assert any("Invalid content" in r.message for r in error_records) + assert any(r.exc_info is not None for r in error_records) + + +@pytest.mark.django_db +def test_post_pgnotify_error_logs_exception( + admin_client: APIClient, + preseed_credential_types, + caplog_factory, +): + """Verify logger.exception is called for PGNotifyError.""" + eda_caplog = caplog_factory(external_event_stream_logger, logging.ERROR) + + secret = secrets.token_hex(32) + username = "fred" + inputs = { + "auth_type": "basic", + "username": username, + "password": secret, + "http_header_key": "Authorization", + } + + obj = create_event_stream_credential( + admin_client, + enums.EventStreamCredentialType.BASIC.value, + inputs, + ) + + # test_mode=False so the PGNotify path is exercised + data_in = { + "name": "test-es-pgnotify-log", + "eda_credential_id": obj["id"], + "organization_id": get_default_test_org().id, + "test_mode": False, + } + event_stream = create_event_stream(admin_client, data_in) + user_pass = f"{username}:{secret}" + auth_value = f"Basic {base64.b64encode(user_pass.encode()).decode()}" + + data = {"a": 1, "b": 2} + content_type = "application/json" + headers = { + "Authorization": auth_value, + "Content-Type": content_type, + } + + with mock.patch( + "aap_eda.api.views.external_event_stream.PGNotify" + ) as mock_pgnotify: + mock_instance = mock.MagicMock() + mock_instance.side_effect = PGNotifyError("Connection refused") + mock_pgnotify.return_value = mock_instance + response = admin_client.post( + event_stream_post_url(event_stream.uuid), + headers=headers, + data=data, + content_type=content_type, + ) + + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + + error_records = [ + r for r in eda_caplog.records if r.levelno >= logging.ERROR + ] + assert len(error_records) >= 1 + assert any(r.exc_info is not None for r in error_records) diff --git a/tests/integration/api/test_project.py b/tests/integration/api/test_project.py index 6523337fa..7e083973c 100644 --- a/tests/integration/api/test_project.py +++ b/tests/integration/api/test_project.py @@ -1444,6 +1444,32 @@ def test_project_needs_update_on_launch_property( assert project.needs_update_on_launch is True +@pytest.mark.django_db +@mock.patch("aap_eda.core.models.project.logger") +def test_project_needs_update_on_launch_logs_exception( + mock_logger, + default_organization: models.Organization, +): + """Test that needs_update_on_launch logs via logger.exception on error.""" + project = models.Project.objects.create( + name="test-project-exception-log", + url="https://git.example.com/repo.git", + organization=default_organization, + git_hash="abc123", + update_revision_on_launch=True, + scm_update_cache_timeout=300, + ) + + project.last_synced_at = "not-a-datetime" + result = project.needs_update_on_launch + + assert result is True + mock_logger.exception.assert_called_once() + assert "Error determining sync status" in ( + mock_logger.exception.call_args[0][0] + ) + + @pytest.mark.django_db @mock.patch("aap_eda.api.views.project.check_default_worker_health") def test_project_retrieve_includes_sync_fields( diff --git a/tests/integration/core/test_health.py b/tests/integration/core/test_health.py index 4828e3434..58e026700 100644 --- a/tests/integration/core/test_health.py +++ b/tests/integration/core/test_health.py @@ -17,7 +17,10 @@ import pytest from aap_eda.api import exceptions as api_exc -from aap_eda.core.health import check_dispatcherd_workers_health +from aap_eda.core.health import ( + check_activation_worker_health, + check_dispatcherd_workers_health, +) @pytest.mark.django_db @@ -184,3 +187,55 @@ def test_check_dispatcherd_workers_health_specific_queue_raises(): raise_exceptions=True, queue_name="secondary" ) assert "Activation" in str(exc_info.value.detail) + + +@pytest.mark.django_db +def test_check_activation_worker_health_logs_exception(): + """Test that check_activation_worker_health uses logger.exception + when an unexpected error occurs (S8572).""" + with patch( + "aap_eda.core.health.check_rulebook_queue_health", + side_effect=RuntimeError("connection lost"), + ), patch( + "aap_eda.core.health.settings.RULEBOOK_WORKER_QUEUES", + ["activation"], + ), patch( + "aap_eda.core.health.logger", + ) as mock_logger: + result = check_activation_worker_health() + assert result is False + mock_logger.exception.assert_called_once() + assert "activation workers" in mock_logger.exception.call_args[0][0] + + +@pytest.mark.django_db +def test_check_dispatcherd_workers_health_logs_exception_no_raise(): + """Test that check_dispatcherd_workers_health uses logger.exception + when raise_exceptions=False and an error occurs (S8572).""" + with patch( + "aap_eda.core.health.check_default_worker_health", + side_effect=Exception("unexpected failure"), + ), patch( + "aap_eda.core.health.logger", + ) as mock_logger: + result = check_dispatcherd_workers_health(raise_exceptions=False) + assert result is False + mock_logger.exception.assert_called_once() + assert "dispatcherd workers" in (mock_logger.exception.call_args[0][0]) + + +@pytest.mark.django_db +def test_check_dispatcherd_workers_health_logs_exception_with_raise(): + """Test that check_dispatcherd_workers_health uses logger.exception + and raises WorkerUnavailable for non-WorkerUnavailable exceptions + when raise_exceptions=True (S8572).""" + with patch( + "aap_eda.core.health.check_default_worker_health", + side_effect=RuntimeError("database gone"), + ), patch( + "aap_eda.core.health.logger", + ) as mock_logger: + with pytest.raises(api_exc.WorkerUnavailable): + check_dispatcherd_workers_health(raise_exceptions=True) + mock_logger.exception.assert_called_once() + assert "dispatcherd workers" in (mock_logger.exception.call_args[0][0]) diff --git a/tests/integration/core/test_rulebook.py b/tests/integration/core/test_rulebook.py index e33b01f8d..94c9a149e 100644 --- a/tests/integration/core/test_rulebook.py +++ b/tests/integration/core/test_rulebook.py @@ -11,9 +11,12 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from unittest import mock + import pytest import yaml +from aap_eda.core.exceptions import ParseError from aap_eda.core.utils.rulebook import ( DEFAULT_SOURCE_NAME_PREFIX, build_source_list, @@ -337,3 +340,17 @@ def test_swap_event_stream_sources( input_rulesets, event_stream_source, source_mappings ) assert yaml.safe_load(result) == yaml.safe_load(output_rulesets) + + +@mock.patch("aap_eda.core.utils.rulebook.LOGGER") +def test_build_source_list_invalid_yaml_logs_exception( + mock_logger, +): + """Test that invalid YAML rulesets logs via LOGGER.exception (S8572).""" + invalid_yaml = "---\n- name: test\n sources:\n - {bad yaml: [" + + with pytest.raises(ParseError): + build_source_list(invalid_yaml) + + mock_logger.exception.assert_called_once() + assert "Invalid rulesets" in (mock_logger.exception.call_args[0][0]) diff --git a/tests/integration/services/activation/engine/test_kubernetes.py b/tests/integration/services/activation/engine/test_kubernetes.py index cb60dece7..a72e61341 100644 --- a/tests/integration/services/activation/engine/test_kubernetes.py +++ b/tests/integration/services/activation/engine/test_kubernetes.py @@ -1242,3 +1242,58 @@ def test_process_pod_start_event_unrecognized(kubernetes_engine): ) event = {"object": pod} assert engine._process_pod_start_event(event) is False + + +@mock.patch( + "aap_eda.services.activation.engine.kubernetes.LOGGER", +) +@pytest.mark.django_db +def test_set_namespace_logs_exception_on_missing_file( + mock_logger, + init_kubernetes_data, +): + """_set_namespace calls LOGGER.exception when file is missing.""" + activation_id = init_kubernetes_data.activation.id + with pytest.raises(ContainerEngineInitError): + Engine( + activation_id=str(activation_id), + resource_prefix=ProcessParentType.ACTIVATION, + client=mock.Mock(), + ) + + mock_logger.exception.assert_called_once() + call_msg = mock_logger.exception.call_args[0][0] + assert "does not exist" in call_msg + + +@mock.patch( + "aap_eda.services.activation.engine.kubernetes.LOGGER", +) +@pytest.mark.django_db +def test_start_logs_exception_on_container_engine_error( + mock_logger, + init_kubernetes_data, + kubernetes_engine, + default_organization, +): + """start() calls LOGGER.exception on ContainerEngineError.""" + engine = kubernetes_engine + request = get_request( + init_kubernetes_data, + "admin", + default_organization, + k8s_service_name=(init_kubernetes_data.activation.k8s_service_name), + ) + log_handler = mock.MagicMock(spec=LogHandler) + + with mock.patch.object(engine.client, "batch_api") as batch_api_mock: + batch_api_mock.create_namespaced_job.side_effect = ApiException( + "Job create error" + ) + with mock.patch.object(engine, "cleanup"): + with pytest.raises(ContainerEngineError): + engine.start(request, log_handler) + + mock_logger.exception.assert_called_once() + call_msg = mock_logger.exception.call_args[0][0] + assert "Failed to start job" in call_msg diff --git a/tests/integration/services/activation/engine/test_podman.py b/tests/integration/services/activation/engine/test_podman.py index 868ebeb76..58023b777 100644 --- a/tests/integration/services/activation/engine/test_podman.py +++ b/tests/integration/services/activation/engine/test_podman.py @@ -53,6 +53,7 @@ from .utils import InitData, get_ansible_rulebook_cmdline, get_request DATA_DIR = Path(__file__).parent / "data" +PODMAN_LOGGER = "aap_eda.services.activation.engine.podman" def get_request_with_never_pull_policy( @@ -972,3 +973,83 @@ def raise_error(*args, **kwargs): with pytest.raises(ContainerUpdateLogsError, match="Not found"): engine.update_logs("100", log_handler) + + +def test_get_podman_client_logs_exception_on_value_error(): + """Verify LOGGER.exception on ValueError (S8572).""" + with mock.patch.object( + PodmanClient, + "__init__", + side_effect=ValueError("bad socket"), + ), mock.patch( + f"{PODMAN_LOGGER}.LOGGER", + ) as mock_logger, pytest.raises( + ContainerEngineInitError + ): + get_podman_client() + + mock_logger.exception.assert_called_once() + assert "Failed to initialize podman client" in ( + mock_logger.exception.call_args[0][0] + ) + + +@pytest.mark.django_db +def test_cleanup_logs_exception_on_api_error( + init_podman_data, + podman_engine, +): + """Verify LOGGER.exception in _cleanup on APIError (S8572).""" + engine = podman_engine + log_handler = DBLogger( + init_podman_data.activation_instance.id, + ) + + container_mock = mock.Mock() + engine.client.containers.get.return_value = container_mock + container_mock.logs.return_value = [] + container_mock.remove.side_effect = APIError( + "remove fail", + ) + + with mock.patch( + f"{PODMAN_LOGGER}.LOGGER", + ) as mock_logger, pytest.raises(ContainerCleanupError): + engine.cleanup("100", log_handler) + + mock_logger.exception.assert_called_once() + assert "Failed to cleanup 100" in (mock_logger.exception.call_args[0][0]) + + +@pytest.mark.django_db +def test_start_logs_exception_on_container_error( + init_podman_data, + podman_engine, + default_organization, +): + """Verify LOGGER.exception in start (S8572).""" + engine = podman_engine + log_handler = DBLogger( + init_podman_data.activation_instance.id, + ) + request = get_request( + init_podman_data, + "me", + default_organization, + mounts=[{"/dev": "/opt"}], + ) + + engine.client.containers.run.side_effect = ContainerError( + container="c", + exit_status=1, + command="cmd", + image="img", + ) + + with mock.patch( + f"{PODMAN_LOGGER}.LOGGER", + ) as mock_logger, pytest.raises(ContainerStartError): + engine.start(request, log_handler) + + mock_logger.exception.assert_called_once() + assert "Container Start Error" in (mock_logger.exception.call_args[0][0]) diff --git a/tests/integration/services/activation/test_manager.py b/tests/integration/services/activation/test_manager.py index 5582f872d..e0c7e0a08 100644 --- a/tests/integration/services/activation/test_manager.py +++ b/tests/integration/services/activation/test_manager.py @@ -999,3 +999,96 @@ def test_stop_cancels_auto_restart_when_no_instance( enums.ProcessParentType.ACTIVATION, basic_activation.id, ) + + +# ----------------------------------------------------------- +# Tests for LOGGER.exception (S8572: .error -> .exception) +# Verify that except blocks use LOGGER.exception (which +# includes exc_info / traceback) rather than LOGGER.error. +# ----------------------------------------------------------- + + +@pytest.mark.django_db +def test_cleanup_logs_exception_on_cleanup_error( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, +): + """Test _cleanup uses LOGGER.exception on cleanup error.""" + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + container_engine_mock.cleanup.side_effect = ( + engine_exceptions.ContainerCleanupError("boom") + ) + + # _cleanup swallows the error and returns + manager._cleanup() + + assert "failed to cleanup" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) == 1 + assert issubclass( + exc_records[0].exc_info[0], + engine_exceptions.ContainerCleanupError, + ) + + +@pytest.mark.django_db +def test_update_logs_logs_exception_on_engine_error( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, +): + """Test update_logs uses LOGGER.exception on engine error.""" + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + container_engine_mock.update_logs.side_effect = ( + engine_exceptions.ContainerEngineError("log err") + ) + + manager.update_logs() + + assert "could not be retrieved" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) == 1 + assert issubclass( + exc_records[0].exc_info[0], + engine_exceptions.ContainerEngineError, + ) + + +@pytest.mark.django_db +def test_monitor_logs_exception_when_activation_deleted( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, +): + """Test monitor uses LOGGER.exception when activation deleted.""" + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + running_activation.delete() + + with pytest.raises(exceptions.ActivationMonitorError): + manager.monitor() + + assert "does not exist" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) >= 1 diff --git a/tests/integration/services/test_sync_certs.py b/tests/integration/services/test_sync_certs.py index fe68c8a7b..7e10c8a5c 100644 --- a/tests/integration/services/test_sync_certs.py +++ b/tests/integration/services/test_sync_certs.py @@ -775,167 +775,78 @@ def test_fetch_response_codes( assert result == {} -# Exception handling tests for new network error scenarios +# LOGGER.exception coverage tests (SonarCloud S8572) -@pytest.mark.parametrize( - "request_method,exception_class,error_message,expected_prefix," - "existing_object", - [ - # POST requests (new certificates) - ( - "post", - requests.exceptions.ConnectionError, - "Connection refused", - "Connection error", - {}, - ), - ( - "post", - requests.exceptions.Timeout, - "Request timed out", - "Request timeout", - {}, - ), - ( - "post", - requests.exceptions.RequestException, - "Generic request error", - "Request error", - {}, - ), - # PATCH requests (updating existing certificates) - ( - "patch", - requests.exceptions.ConnectionError, - "Network unreachable", - "Connection error", - {"id": 123, "sha256": "different-hash"}, - ), - ( - "patch", - requests.exceptions.Timeout, - "Read timeout occurred", - "Request timeout", - {"id": 123, "sha256": "different-hash"}, - ), - ( - "patch", - requests.exceptions.RequestException, - "SSL certificate error", - "Request error", - {"id": 123, "sha256": "different-hash"}, - ), - ], -) @pytest.mark.django_db -def test_update_handles_network_exceptions( - request_method, - exception_class, - error_message, - expected_prefix, - existing_object, +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.post") +def test_make_request_connection_error_logs_exception( + mock_post, + mock_logger, mock_settings, default_mtls_credential, mock_service_token, ): - """Test handling of network exceptions during update() method.""" + """Verify LOGGER.exception on ConnectionError in _make_request.""" + mock_post.side_effect = requests.exceptions.ConnectionError("refused") sync = SyncCertificates(default_mtls_credential.id) - with patch.object( - sync, "_fetch_from_gateway", return_value=existing_object - ): - with patch( - f"aap_eda.services.sync_certs.requests.{request_method}" - ) as mock_request: - mock_request.side_effect = exception_class(error_message) - - with pytest.raises( - GatewayAPIError, match=f"{expected_prefix}: {error_message}" - ): - sync.update() + with patch.object(sync, "_fetch_from_gateway", return_value={}): + with pytest.raises(GatewayAPIError): + sync.update() + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Connection error" in log_msg -@pytest.mark.parametrize( - "exception_class,error_message,expected_prefix", - [ - ( - requests.exceptions.ConnectionError, - "Connection refused", - "Connection error", - ), - ( - requests.exceptions.Timeout, - "Delete timed out", - "Request timeout", - ), - ( - requests.exceptions.RequestException, - "HTTP adapter error", - "Request error", - ), - ], -) @pytest.mark.django_db -@patch("aap_eda.services.sync_certs.requests.delete") -def test_delete_from_gateway_handles_network_exceptions( - mock_delete, - exception_class, - error_message, - expected_prefix, +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.get") +def test_fetch_from_gateway_timeout_logs_exception( + mock_get, + mock_logger, mock_settings, default_mtls_credential, mock_service_token, ): - """Test handling of network exceptions in _delete_from_gateway().""" - mock_delete.side_effect = exception_class(error_message) - + """Verify LOGGER.exception on Timeout in _fetch_from_gateway.""" + mock_get.side_effect = requests.exceptions.Timeout("Request timed out") sync = SyncCertificates(default_mtls_credential.id) - existing_object = {"id": 123} - with pytest.raises( - GatewayAPIError, match=f"{expected_prefix}: {error_message}" - ): - sync._delete_from_gateway(existing_object) + with pytest.raises(GatewayAPIError): + sync._fetch_from_gateway() + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Timeout" in log_msg -@pytest.mark.parametrize( - "exception_class,error_message,expected_prefix", - [ - ( - requests.exceptions.ConnectionError, - "Connection refused", - "Connection error", - ), - ( - requests.exceptions.Timeout, - "Fetch timed out", - "Request timeout", - ), - ( - requests.exceptions.RequestException, - "DNS resolution failed", - "Request error", - ), - ], -) @pytest.mark.django_db -@patch("aap_eda.services.sync_certs.requests.get") -def test_fetch_from_gateway_handles_network_exceptions( - mock_get_request, - exception_class, - error_message, - expected_prefix, - mock_settings, - default_mtls_credential, - mock_service_token, +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs" ".models.EventStream.objects.filter") +@patch("aap_eda.services.sync_certs.SyncCertificates") +def test_gw_handler_gateway_error_logs_exception( + mock_sync_class, + mock_filter, + mock_logger, + mtls_credential_type, ): - """Test handling of network exceptions in _fetch_from_gateway().""" - mock_get_request.side_effect = exception_class(error_message) + """Verify LOGGER.exception on GatewayAPIError in gw_handler.""" + instance = Mock() + instance.id = 1 + instance.credential_type = mtls_credential_type + instance._request = Mock() - sync = SyncCertificates(default_mtls_credential.id) + mock_filter.return_value = [Mock()] - with pytest.raises( - GatewayAPIError, match=f"{expected_prefix}: {error_message}" - ): - sync._fetch_from_gateway() + mock_sync_instance = Mock() + mock_sync_instance.update.side_effect = GatewayAPIError("API Error") + mock_sync_class.return_value = mock_sync_instance + + gw_handler(models.EdaCredential, instance) + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "gateway certificate" in log_msg diff --git a/tests/integration/wsapi/test_consumer.py b/tests/integration/wsapi/test_consumer.py index d9257b36d..8effd7284 100644 --- a/tests/integration/wsapi/test_consumer.py +++ b/tests/integration/wsapi/test_consumer.py @@ -12,10 +12,12 @@ from channels.db import database_sync_to_async from channels.testing import WebsocketCommunicator from django.core.exceptions import ObjectDoesNotExist +from django.db import DatabaseError from django.utils import timezone from pydantic.error_wrappers import ValidationError from aap_eda.core import enums, models +from aap_eda.core.exceptions import InvalidEnvKeyError from aap_eda.core.models.activation import ActivationStatus from aap_eda.services.activation.activation_manager import ActivationManager from aap_eda.wsapi.consumers import AnsibleRulebookConsumer, logger @@ -1349,6 +1351,56 @@ async def test_receive_object_not_exist( await ws_communicator.wait() +@pytest.mark.django_db(transaction=True) +async def test_receive_logs_db_error( + ws_communicator: WebsocketCommunicator, + eda_caplog, + default_organization: models.Organization, +): + """Test receive logs DatabaseError via logger.exception.""" + rulebook_process_id = await _prepare_db_data( + default_organization, + ) + payload = { + "type": "Worker", + "activation_id": rulebook_process_id, + } + with patch( + "aap_eda.wsapi.consumers." "AnsibleRulebookConsumer.handle_workers", + side_effect=DatabaseError("test db error"), + ): + await ws_communicator.send_json_to(payload) + await ws_communicator.wait() + + assert "due to DB error" in eda_caplog.text + assert "test db error" in eda_caplog.text + + +@pytest.mark.django_db(transaction=True) +async def test_receive_logs_env_error( + ws_communicator: WebsocketCommunicator, + eda_caplog, + default_organization: models.Organization, +): + """Test receive logs InvalidEnvKeyError via logger.exception.""" + rulebook_process_id = await _prepare_db_data( + default_organization, + ) + payload = { + "type": "Worker", + "activation_id": rulebook_process_id, + } + with patch( + "aap_eda.wsapi.consumers." "AnsibleRulebookConsumer.handle_workers", + side_effect=InvalidEnvKeyError("bad key"), + ): + await ws_communicator.send_json_to(payload) + await ws_communicator.wait() + + assert "due to Env error" in eda_caplog.text + assert "bad key" in eda_caplog.text + + @pytest.mark.django_db(transaction=True) async def test_insert_audit_rule_invalid_activation( ws_communicator: WebsocketCommunicator, diff --git a/tests/unit/services/test_pg_notify.py b/tests/unit/services/test_pg_notify.py index 391bfc821..a20de9ff0 100644 --- a/tests/unit/services/test_pg_notify.py +++ b/tests/unit/services/test_pg_notify.py @@ -1,7 +1,9 @@ from unittest.mock import MagicMock, patch +import psycopg import pytest +from aap_eda.core.exceptions import PGNotifyError from aap_eda.services.pg_notify import PGNotify @@ -87,3 +89,26 @@ def test_sql_metacharacters_are_parameterized(mock_psycopg, malicious_value): sql, params = mock_cursor.execute.call_args[0] assert sql == "SELECT pg_notify(%s, %s)" assert malicious_value in params[1] + + +@patch("aap_eda.services.pg_notify.logger") +@patch("aap_eda.services.pg_notify.psycopg") +def test_operational_error_logs_exception(mock_psycopg, mock_logger): + mock_psycopg.OperationalError = psycopg.OperationalError + mock_psycopg.connect.side_effect = psycopg.OperationalError( + "connection refused" + ) + + notifier = PGNotify( + dsn="postgresql://localhost/eda", + channel="test_chan", + data={"event": "test"}, + ) + + with pytest.raises(PGNotifyError): + notifier() + + mock_logger.exception.assert_called_once() + assert ( + "PG Notify operational error" in mock_logger.exception.call_args[0][0] + ) diff --git a/tests/unit/test_credential_plugins.py b/tests/unit/test_credential_plugins.py index da3809a62..5b1562dae 100644 --- a/tests/unit/test_credential_plugins.py +++ b/tests/unit/test_credential_plugins.py @@ -64,3 +64,21 @@ def test_run_plugin_exception(): str(exc_info.value) == "Error executing credential plugin aim: Kaboom" ) + + +@mock.patch( + "aap_eda.core.utils.credential_plugins.LOGGER", +) +def test_run_plugin_exception_logs_via_logger_exception( + mock_logger, +): + """Test that plugin failure logs via LOGGER.exception (S8572).""" + with mock.patch.object(CredentialPlugin, "backend") as mock_backend: + mock_backend.side_effect = Exception("connection refused") + with pytest.raises(CredentialPluginError): + run_plugin("aim", {}, {}) + + mock_logger.exception.assert_called_once() + assert "Error executing credential plugin aim" in ( + mock_logger.exception.call_args[0][0] + ) diff --git a/tests/unit/test_credential_validation.py b/tests/unit/test_credential_validation.py index 0b4d8f0ec..1939614af 100644 --- a/tests/unit/test_credential_validation.py +++ b/tests/unit/test_credential_validation.py @@ -15,15 +15,19 @@ from unittest import mock import pytest +from django.core.exceptions import ValidationError from aap_eda.core import enums, models from aap_eda.core.utils.credentials import ( PROTECTED_PASSPHRASE_ERROR, SUPPORTED_KEYS_IN_INJECTORS, + _get_aes_key, + _match_regex_pattern_against_attrs, add_default_values_to_user_inputs, validate_injectors, validate_inputs, validate_schema, + validate_x509_subject_match, ) DATA_DIR = Path(__file__).parent / "data" @@ -713,3 +717,51 @@ def test_add_default_values_to_user_inputs(): "", False, ] + + +@mock.patch("aap_eda.core.utils.credentials.LOGGER") +def test_validate_x509_subject_match_invalid_dn_logs_exception( + mock_logger, +): + """Test that invalid actual DN logs via LOGGER.exception (S8572).""" + result = validate_x509_subject_match("CN=test", "===invalid dn===") + + assert result is False + mock_logger.exception.assert_called_once() + assert "Invalid actual DN format" in ( + mock_logger.exception.call_args[0][0] + ) + + +@mock.patch("aap_eda.core.utils.credentials.LOGGER") +def test_match_regex_pattern_invalid_regex_logs_exception( + mock_logger, +): + """Test that invalid regex logs via LOGGER.exception (S8572).""" + from cryptography import x509 + from cryptography.x509.oid import NameOID + + name = x509.Name.from_rfc4514_string("CN=test.example.com") + actual_attrs = name.get_attributes_for_oid(NameOID.COMMON_NAME) + + result = _match_regex_pattern_against_attrs("[invalid", actual_attrs) + + assert result is False + mock_logger.exception.assert_called_once() + assert "Invalid regex pattern" in (mock_logger.exception.call_args[0][0]) + + +@mock.patch("aap_eda.core.utils.credentials.LOGGER") +def test_get_aes_key_failure_logs_exception(mock_logger): + """Test that AES key derivation failure logs (S8572).""" + with mock.patch( + "aap_eda.core.utils.credentials.hashlib.pbkdf2_hmac" + ) as mock_pbkdf2: + mock_pbkdf2.side_effect = ValueError("hash error") + with pytest.raises(ValidationError): + _get_aes_key("password", "salt") + + mock_logger.exception.assert_called_once() + assert "Failed to derive AES key" in ( + mock_logger.exception.call_args[0][0] + ) diff --git a/tests/unit/test_external_sms.py b/tests/unit/test_external_sms.py index cd56ad83f..4a4a80491 100644 --- a/tests/unit/test_external_sms.py +++ b/tests/unit/test_external_sms.py @@ -127,3 +127,39 @@ def test_get_external_secrets( ): result = get_external_secrets(target_credential["id"]) assert result[input_field_name] == "abc" + + +@mock.patch("aap_eda.core.utils.external_sms.LOGGER") +@mock.patch("aap_eda.core.utils.external_sms.run_plugin") +@mock.patch("aap_eda.core.utils.external_sms.models") +def test_get_external_secrets_logs_exception( + mock_models, mock_run_plugin, mock_logger +): + """Test that CredentialPluginError is logged.""" + mock_source = mock.MagicMock() + mock_source.credential_type.namespace = "test" + mock_source.inputs.get_secret_value.return_value = ( + "url: https://example.com" + ) + mock_source.name = "source-cred" + + mock_target = mock.MagicMock() + mock_target.name = "target-cred" + + mock_obj = mock.MagicMock() + mock_obj.input_field_name = "password" + mock_obj.source_credential = mock_source + mock_obj.target_credential = mock_target + mock_obj.metadata.get_secret_value.return_value = "secret_key: bar" + + qs = mock_models.CredentialInputSource.objects + qs.filter.return_value = [mock_obj] + mock_run_plugin.side_effect = CredentialPluginError("Kaboom") + + with pytest.raises(CredentialPluginError): + get_external_secrets(1) + + mock_logger.exception.assert_called_once() + logged_msg = mock_logger.exception.call_args[0][0] + assert "password" in logged_msg + assert "Kaboom" in logged_msg From 68ebe0d8fddea75616a330a0ef38c12dfece6ed1 Mon Sep 17 00:00:00 2001 From: Jacob Craiglow Date: Thu, 23 Jul 2026 15:55:34 -0400 Subject: [PATCH 3/3] sonar issues: Logging exception instead of logging errors for better traceback --- .../activation/engine/test_kubernetes.py | 7 +- .../services/activation/test_manager.py | 224 ++++++++++++++++++ tests/integration/services/test_sync_certs.py | 161 +++++++++++++ tests/integration/tasks/test_projects.py | 45 ++++ tests/unit/test_orchestrator.py | 53 +++++ 5 files changed, 487 insertions(+), 3 deletions(-) diff --git a/tests/integration/services/activation/engine/test_kubernetes.py b/tests/integration/services/activation/engine/test_kubernetes.py index a72e61341..c903cf3ab 100644 --- a/tests/integration/services/activation/engine/test_kubernetes.py +++ b/tests/integration/services/activation/engine/test_kubernetes.py @@ -1253,12 +1253,13 @@ def test_set_namespace_logs_exception_on_missing_file( init_kubernetes_data, ): """_set_namespace calls LOGGER.exception when file is missing.""" - activation_id = init_kubernetes_data.activation.id + aid = str(init_kubernetes_data.activation.id) + client = mock.Mock() with pytest.raises(ContainerEngineInitError): Engine( - activation_id=str(activation_id), + activation_id=aid, resource_prefix=ProcessParentType.ACTIVATION, - client=mock.Mock(), + client=client, ) mock_logger.exception.assert_called_once() diff --git a/tests/integration/services/activation/test_manager.py b/tests/integration/services/activation/test_manager.py index e0c7e0a08..457a05cb5 100644 --- a/tests/integration/services/activation/test_manager.py +++ b/tests/integration/services/activation/test_manager.py @@ -1092,3 +1092,227 @@ def test_monitor_logs_exception_when_activation_deleted( if r.exc_info and r.exc_info[0] is not None ] assert len(exc_records) >= 1 + + +@pytest.mark.django_db +def test_missing_container_policy_cleanup_failure_logs_exception( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, +): + """Test _missing_container_policy LOGGER.exception on cleanup error.""" + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + + with patch.object( + manager, + "_fail_instance", + side_effect=engine_exceptions.ContainerCleanupError("boom"), + ): + with pytest.raises(exceptions.ActivationMonitorError): + manager._missing_container_policy() + + assert "failed to cleanup" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) >= 1 + assert any( + issubclass(r.exc_info[0], engine_exceptions.ContainerCleanupError) + for r in exc_records + ) + + +@pytest.mark.django_db +def test_failed_policy_never_restart_cleanup_failure_logs_exception( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, +): + """Test _failed_policy NEVER restart LOGGER.exception on cleanup.""" + running_activation.restart_policy = "never" + running_activation.save(update_fields=["restart_policy"]) + + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + + with patch.object( + manager, + "_fail_instance", + side_effect=engine_exceptions.ContainerCleanupError("boom"), + ): + with pytest.raises(exceptions.ActivationMonitorError): + manager._failed_policy("") + + assert "failed to cleanup" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) >= 1 + + +@pytest.mark.django_db +def test_failed_policy_max_restarts_cleanup_failure_logs_exception( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, + settings: SettingsWrapper, +): + """Test _failed_policy max restarts LOGGER.exception on cleanup.""" + settings.ACTIVATION_MAX_RESTARTS_ON_FAILURE = 3 + running_activation.restart_policy = "always" + running_activation.failure_count = 3 + running_activation.save( + update_fields=["restart_policy", "failure_count"], + ) + + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + + with patch.object( + manager, + "_fail_instance", + side_effect=engine_exceptions.ContainerCleanupError("boom"), + ): + with pytest.raises(exceptions.ActivationMonitorError): + manager._failed_policy("") + + assert "failed to cleanup" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) >= 1 + + +@pytest.mark.django_db +def test_failed_policy_restart_path_cleanup_failure_logs_exception( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, + settings: SettingsWrapper, +): + """Test _failed_policy restart path LOGGER.exception on cleanup.""" + settings.ACTIVATION_MAX_RESTARTS_ON_FAILURE = 5 + running_activation.restart_policy = "always" + running_activation.failure_count = 0 + running_activation.save( + update_fields=["restart_policy", "failure_count"], + ) + + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + + with patch.object( + manager, + "_fail_instance", + side_effect=engine_exceptions.ContainerCleanupError("boom"), + ): + with pytest.raises(exceptions.ActivationMonitorError): + manager._failed_policy("") + + assert "failed to cleanup" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) >= 1 + + +@pytest.mark.django_db +def test_monitor_logs_exception_on_instance_not_found( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, +): + """Test monitor LOGGER.exception on ActivationInstanceNotFound.""" + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + # Remove the latest instance so _check_latest_instance raises + running_activation.latest_instance.delete() + models.RulebookProcess.objects.filter( + activation=running_activation, + ).delete() + + with pytest.raises(exceptions.ActivationMonitorError): + manager.monitor() + + assert "Monitor operation Failed" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) >= 1 + + +@pytest.mark.django_db +def test_monitor_logs_exception_on_pod_id_not_found( + running_activation: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, +): + """Test monitor LOGGER.exception on ActivationInstancePodIdNotFound.""" + running_activation.latest_instance.activation_pod_id = None + running_activation.latest_instance.save( + update_fields=["activation_pod_id"], + ) + + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=running_activation, + ) + + with pytest.raises(exceptions.ActivationMonitorError): + manager.monitor() + + assert "Monitor operation Failed" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) >= 1 + + +@pytest.mark.django_db +def test_update_logs_no_instance_or_pod_id_logs_exception( + activation_with_instance: models.Activation, + container_engine_mock: MagicMock, + eda_caplog: LogCaptureFixture, +): + """Test update_logs LOGGER.exception when no instance or pod_id.""" + activation_with_instance.latest_instance.activation_pod_id = None + activation_with_instance.latest_instance.save( + update_fields=["activation_pod_id"], + ) + + manager = ActivationManager( + container_engine=container_engine_mock, + db_instance=activation_with_instance, + ) + manager.update_logs() + + assert "No instance or pod id found" in eda_caplog.text + exc_records = [ + r + for r in eda_caplog.records + if r.exc_info and r.exc_info[0] is not None + ] + assert len(exc_records) >= 1 diff --git a/tests/integration/services/test_sync_certs.py b/tests/integration/services/test_sync_certs.py index 7e10c8a5c..9292adf9c 100644 --- a/tests/integration/services/test_sync_certs.py +++ b/tests/integration/services/test_sync_certs.py @@ -850,3 +850,164 @@ def test_gw_handler_gateway_error_logs_exception( mock_logger.exception.assert_called_once() log_msg = mock_logger.exception.call_args[0][0] assert "gateway certificate" in log_msg + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.post") +def test_make_request_timeout_logs_exception( + mock_post, + mock_logger, + mock_settings, + default_mtls_credential, + mock_service_token, +): + """Verify LOGGER.exception on Timeout in _make_request.""" + mock_post.side_effect = requests.exceptions.Timeout("timed out") + sync = SyncCertificates(default_mtls_credential.id) + + with patch.object(sync, "_fetch_from_gateway", return_value={}): + with pytest.raises(GatewayAPIError): + sync.update() + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Timeout" in log_msg + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.post") +def test_make_request_request_exception_logs_exception( + mock_post, + mock_logger, + mock_settings, + default_mtls_credential, + mock_service_token, +): + """Verify LOGGER.exception on RequestException in _make_request.""" + mock_post.side_effect = requests.exceptions.RequestException("fail") + sync = SyncCertificates(default_mtls_credential.id) + + with patch.object(sync, "_fetch_from_gateway", return_value={}): + with pytest.raises(GatewayAPIError): + sync.update() + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Request error" in log_msg + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.delete") +def test_delete_from_gateway_connection_error_logs_exception( + mock_delete, + mock_logger, + mock_settings, + default_mtls_credential, + mock_service_token, +): + """Verify LOGGER.exception on ConnectionError in _delete_from_gateway.""" + mock_delete.side_effect = requests.exceptions.ConnectionError("refused") + sync = SyncCertificates(default_mtls_credential.id) + + with pytest.raises(GatewayAPIError): + sync._delete_from_gateway({"id": 123}) + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Connection error" in log_msg + assert "deleting" in log_msg + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.delete") +def test_delete_from_gateway_timeout_logs_exception( + mock_delete, + mock_logger, + mock_settings, + default_mtls_credential, + mock_service_token, +): + """Verify LOGGER.exception on Timeout in _delete_from_gateway.""" + mock_delete.side_effect = requests.exceptions.Timeout("timed out") + sync = SyncCertificates(default_mtls_credential.id) + + with pytest.raises(GatewayAPIError): + sync._delete_from_gateway({"id": 123}) + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Timeout" in log_msg + assert "deleting" in log_msg + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.delete") +def test_delete_from_gateway_request_exception_logs_exception( + mock_delete, + mock_logger, + mock_settings, + default_mtls_credential, + mock_service_token, +): + """Verify LOGGER.exception on RequestException in _delete_from_gateway.""" + mock_delete.side_effect = requests.exceptions.RequestException("fail") + sync = SyncCertificates(default_mtls_credential.id) + + with pytest.raises(GatewayAPIError): + sync._delete_from_gateway({"id": 123}) + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Request error" in log_msg + assert "deleting" in log_msg + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.get") +def test_fetch_from_gateway_connection_error_logs_exception( + mock_get, + mock_logger, + mock_settings, + default_mtls_credential, + mock_service_token, +): + """Verify LOGGER.exception on ConnectionError in _fetch_from_gateway.""" + mock_get.side_effect = requests.exceptions.ConnectionError("refused") + sync = SyncCertificates(default_mtls_credential.id) + + with pytest.raises(GatewayAPIError): + sync._fetch_from_gateway() + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Connection error" in log_msg + assert "fetching" in log_msg + + +@pytest.mark.django_db +@patch("aap_eda.services.sync_certs.LOGGER") +@patch("aap_eda.services.sync_certs.requests.get") +def test_fetch_from_gateway_request_exception_logs_exception( + mock_get, + mock_logger, + mock_settings, + default_mtls_credential, + mock_service_token, +): + """Verify LOGGER.exception on RequestException in _fetch_from_gateway.""" + mock_get.side_effect = requests.exceptions.RequestException("fail") + sync = SyncCertificates(default_mtls_credential.id) + + with pytest.raises(GatewayAPIError): + sync._fetch_from_gateway() + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "Request error" in log_msg + assert "fetching" in log_msg diff --git a/tests/integration/tasks/test_projects.py b/tests/integration/tasks/test_projects.py index 831f95ea4..d0fca5e0b 100644 --- a/tests/integration/tasks/test_projects.py +++ b/tests/integration/tasks/test_projects.py @@ -2220,6 +2220,51 @@ def test_auto_restart_content_changed_still_works( assert activation.rulebook_rulesets == "new-content" +@pytest.mark.django_db +@patch("aap_eda.tasks.project.logger") +@patch("aap_eda.tasks.project.start_rulebook_process") +def test_resume_waiting_double_failure_logs_exception( + mock_start, + mock_logger, + default_organization, +): + """Test _resume_waiting_activations double-failure logs exception.""" + mock_start.side_effect = RuntimeError("queue full") + project = models.Project.objects.create( + name="Test Project", + url="https://github.com/example/repo", + organization=default_organization, + ) + rulebook = _create_test_rulebook( + project, + default_organization, + rulesets="content", + ) + activation = _create_test_activation( + project, + default_organization, + rulebook, + name="double-fail-resume", + is_enabled=False, + awaiting_project_sync=True, + status=ActivationStatus.PENDING, + ) + + with patch.object( + type(activation), + "save", + side_effect=[None, RuntimeError("save failed")], + ): + _resume_waiting_activations(project) + + exception_calls = [ + c + for c in mock_logger.exception.call_args_list + if "Failed to update activation status" in c[0][0] + ] + assert len(exception_calls) >= 1 + + @pytest.mark.django_db @patch("aap_eda.tasks.project.restart_rulebook_process") def test_auto_restart_failed_with_content_change( diff --git a/tests/unit/test_orchestrator.py b/tests/unit/test_orchestrator.py index c3a5c5a45..3fd72df9f 100644 --- a/tests/unit/test_orchestrator.py +++ b/tests/unit/test_orchestrator.py @@ -35,6 +35,7 @@ _resolve_existing_queue, get_least_busy_queue_name, get_process_parent, + queue_dispatch, ) @@ -987,3 +988,55 @@ def test_handle_unhealthy_queue_restart_no_healthy(): status_manager.set_status.assert_called_once_with( ActivationStatus.PENDING, mock.ANY ) + + +@pytest.mark.django_db +@mock.patch("aap_eda.tasks.orchestrator.LOGGER") +def test_queue_dispatch_start_no_healthy_queues_logs_exception( + mock_logger, +): + """Test queue_dispatch LOGGER.exception on HealthyQueueNotFoundError.""" + process_parent = mock.Mock(spec=Activation) + process_parent.id = 1 + process_parent.status = ActivationStatus.RUNNING + process_parent.log_tracking_id = str(uuid.uuid4()) + + with ( + mock.patch( + "aap_eda.tasks.orchestrator.get_process_parent", + return_value=process_parent, + ), + mock.patch( + "aap_eda.tasks.orchestrator.assign_request_id", + ), + mock.patch( + "aap_eda.tasks.orchestrator.assign_log_tracking_id", + ), + mock.patch( + "aap_eda.tasks.orchestrator.advisory_lock", + ) as mock_lock, + mock.patch( + "aap_eda.tasks.orchestrator.get_least_busy_queue_name", + side_effect=HealthyQueueNotFoundError, + ), + mock.patch( + "aap_eda.tasks.orchestrator.StatusManager", + ) as mock_status_cls, + ): + mock_lock.return_value.__enter__ = mock.Mock(return_value=True) + mock_lock.return_value.__exit__ = mock.Mock(return_value=None) + mock_status_manager = mock.Mock() + mock_status_cls.return_value = mock_status_manager + + queue_dispatch( + ProcessParentType.ACTIVATION, + 1, + ActivationRequest.START, + ) + + mock_logger.exception.assert_called_once() + log_msg = mock_logger.exception.call_args[0][0] + assert "no healthy queues" in log_msg + mock_status_manager.set_status.assert_called_once_with( + ActivationStatus.PENDING, mock.ANY + )