Submit more files to Log Detective for analysis (#3106)

Submit more files to Log Detective for analysis

References to multiple artifacts are now used when asking Log Detective for analysis.
Since we can not be completely sure about their availability, all URLs are checked using get request.
If no artifacts pass the check, the trigger fails and the information is logged.
In order to avoid references to files that can't be possibly analyzed by Log Detective, only URLs pointing to files of the text/plain type are allowed, all others are dropped from the final request.
This check should see more use as we interact with other build backends, and submit artifacts other than just logs.
New tests for the verify_artifact were introduced and existing tests adjusted.
Merge after

fedora-copr/logdetective-packit#36
fedora-copr/logdetective#492

RELEASE NOTES BEGIN
Packit now sends build.log, root.log and mock_output.log from failed Koji builds to Log Detective.
RELEASE NOTES END

Reviewed-by: gemini-code-assist[bot]
Reviewed-by: Nikola Forró
Reviewed-by: Jiří Podivín
Reviewed-by: Laura Barcziová

Author: centosinfra-prod-github-app[bot]

packit_service/utils.py

@@ -12,7 +12,8 @@
 from logging import StreamHandler
 from pathlib import Path
 from re import search
-from typing import Optional
+from typing import Optional, Union
+from urllib.parse import urlparse
 
 import requests
 from cachetools.func import ttl_cache
@@ -428,6 +429,84 @@ def get_user_agent() -> str:
     )
 
 
+def check_url(url: str) -> bool:
+    """
+    Verify that URL is valid.
+
+    Args:
+        url: URL to be validated
+
+    Returns:
+        True if the URL can be parsed, contains both scheme and netloc
+    """
+
+    try:
+        result = urlparse(url)
+        return all([result.scheme, result.netloc])
+    except ValueError:
+        return False
+
+
+def check_content_type(content_type: Union[str, None], url: str) -> bool:
+    """
+    Verify that content_type is text/plain and log failures.
+
+    Args:
+        content_type: Value of Content-Type header field
+        url: URL of the checked artifact
+
+    Returns:
+        True if content type is text/plain
+    """
+    if not content_type:
+        logger.warning("No content-type reported for artifact at %s", url)
+        return False
+
+    if "text/plain" not in content_type.lower():
+        logger.warning("Artifact: %s is not of allowed text/plain type, but %s", url, content_type)
+        return False
+
+    return True
+
+
+def verify_artifact(url: str) -> bool:
+    """
+    Verify that URL points to an text/plain artifact that can be downloaded.
+
+    Checks Content-Type in headers using GET request.
+
+    Args:
+        url: URL of the artifact to be verified
+
+    Returns:
+        True if the artifact satisfies all requirements
+    """
+
+    if not check_url(url=url):
+        logger.warning("Invalid URL for build artifact: %s", url)
+        return False
+    try:
+        with requests.get(
+            url=url,
+            headers={"User-Agent": get_user_agent()},
+            timeout=(10, 30),
+            stream=True,
+            allow_redirects=True,
+        ) as response:
+            if response.status_code != 200:
+                logger.warning(
+                    "Request for artifact: %s failed with: %s", url, response.status_code
+                )
+                return False
+            return check_content_type(response.headers.get("Content-Type"), url)
+
+    except requests.exceptions.RequestException as exc:
+        logger.warning(
+            "Verification of artifact %s failed with error: %s", url, exc, stack_info=True
+        )
+        return False
+
+
 def download_file(url: str, path: Path):
     """
     Download a file from given url to the given path.

packit_service/worker/helpers/logdetective.py

@@ -18,6 +18,7 @@
     LogDetectiveRunGroupModel,
     LogDetectiveRunModel,
 )
+from packit_service.utils import verify_artifact
 from packit_service.worker.monitoring import Pushgateway
 
 logger = logging.getLogger(__name__)
@@ -84,14 +85,30 @@ def trigger_log_detective_analysis_for_arch(self, arch: str) -> bool:
         Instead of returning TaskResults() object, as job handlers do,
         we just return a boolean signaling whether or not the trigger succeeded.
         """
-
+        artifacts = {}
         build_arch_task_id = self.koji_event.rpm_build_task_ids[arch]
-        artifacts = {
-            "mock_output.log": koji.result.KojiEvent.get_koji_build_logs_url(
-                build_arch_task_id,
-                self.koji_logs_url,
+
+        # Following artifacts are the most likely to contain valuable information
+        # Including too many could impact latency and response quality
+        possible_koji_artifacts = [
+            "root.log",
+            "mock_output.log",
+            "build.log",
+        ]
+
+        # Use only artifacts with valid URLs
+        for artifact in possible_koji_artifacts:
+            url = koji.result.KojiEvent.get_koji_build_logs_url(
+                rpm_build_task_id=build_arch_task_id,
+                koji_logs_url=self.koji_logs_url,
+                log_file=artifact,
             )
-        }
+            if verify_artifact(url):
+                artifacts[artifact] = url
+
+        if len(artifacts) == 0:
+            logger.warning("No artifact URLs passed verification")
+            return False
 
         endpoint_url = f"{self.url}/analyze"
         request_json = {

tests/integration/test_logdetective_koji.py

@@ -10,6 +10,7 @@
 from flexmock import Mock, flexmock
 from packit.config.common_package_config import Deployment
 
+import packit_service.worker.helpers.logdetective as logdetective_module
 from packit_service.config import FedoraCISettings, ServiceConfig
 from packit_service.constants import LOGDETECTIVE_PACKIT_SERVER_URL
 from packit_service.events import koji
@@ -101,6 +102,7 @@ def test_logdetective_koji_build_scratch_downstream(
             },
         ).one_by_one()
         flexmock(requests).should_receive("post").and_return(mock_ld_response)
+        flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
 
     mock_group_run = flexmock(id=1)
     flexmock(LogDetectiveRunGroupModel).should_receive("create").times(

tests/unit/test_logdetective_koji_helper.py

@@ -9,6 +9,7 @@
 import requests
 from flexmock import flexmock
 
+import packit_service.worker.helpers.logdetective as logdetective_module
 from packit_service.constants import LOGDETECTIVE_PACKIT_SERVER_URL, KojiTaskState
 from packit_service.models import (
     LogDetectiveBuildSystem,
@@ -81,15 +82,17 @@ def test_logdetective_koji_set_payload(mock_koji_task_failed_event, mock_event_d
 
     request_json = {
         "artifacts": {
+            "root.log": "https://kojipkgs.fedoraproject.org//work/tasks/2345/12345/root.log",
             "mock_output.log": "https://kojipkgs.fedoraproject.org//work/tasks/2345/12345/mock_output.log",
+            "build.log": "https://kojipkgs.fedoraproject.org//work/tasks/2345/12345/build.log",
         },
         "target_build": "12345",
         "build_system": "koji",
         "commit_sha": "abc123",
         "project_url": "https://github.com/test/repo",
         "pr_id": 42,
     }
-
+    flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
     flexmock(requests).should_receive("post").with_args(
         "https://logdetective01.fedorainfracloud.org/analyze",
         json=request_json,
@@ -118,12 +121,14 @@ def test_logdetective_koji_success(
             "creation_time": "2026-01-01T12:00:00",
         }
     )
-
+    flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
     flexmock(requests).should_receive("post").with_args(
         f"{LOGDETECTIVE_PACKIT_SERVER_URL}/analyze",
         json={
             "artifacts": {
-                "mock_output.log": "https://kojipkgs.fedoraproject.org//work/tasks/2345/12345/mock_output.log"
+                "root.log": "https://kojipkgs.fedoraproject.org//work/tasks/2345/12345/root.log",
+                "mock_output.log": "https://kojipkgs.fedoraproject.org//work/tasks/2345/12345/mock_output.log",
+                "build.log": "https://kojipkgs.fedoraproject.org//work/tasks/2345/12345/build.log",
             },
             "target_build": "12345",
             "build_system": LogDetectiveBuildSystem.koji.value,
@@ -175,7 +180,7 @@ def test_logdetective_koji_http_error(
 ):
     mock_response = flexmock(status_code=500)
     mock_response.should_receive("raise_for_status")
-
+    flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
     flexmock(requests).should_receive("post").and_raise(
         requests.exceptions.HTTPError("500 Server Error")
     )
@@ -203,6 +208,7 @@ def test_logdetective_koji_connection_error(
 ):
     mock_response = flexmock()
     mock_response.should_receive("raise_for_status")
+    flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
     flexmock(requests).should_receive("post").and_raise(requests.exceptions.ConnectionError)
 
     flexmock(LogDetectiveRunGroupModel).should_receive("create").never()
@@ -230,7 +236,7 @@ def test_logdetective_koji_json_decode_error(
     mock_response.should_receive("json").and_raise(
         requests.exceptions.JSONDecodeError("Invalid JSON", "", 0)
     )
-
+    flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
     flexmock(requests).should_receive("post").and_return(mock_response)
     flexmock(LogDetectiveRunGroupModel).should_receive("create").never()
     flexmock(LogDetectiveRunModel).should_receive("create").never()
@@ -252,6 +258,7 @@ def test_logdetective_koji_json_decode_error(
 def test_logdetective_koji_timeout(
     mock_koji_task_failed_event, mock_event_data, mock_pushgateway_log_detective_no_inc
 ):
+    flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
     flexmock(requests).should_receive("post").and_raise(
         requests.exceptions.Timeout("Request timed out")
     )
@@ -283,6 +290,7 @@ def test_logdetective_koji_missing_id(
             "creation_time": "2026-01-01T12:00:00",
         }
     )
+    flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
     flexmock(requests).should_receive("post").and_return(mock_response)
     flexmock(logger).should_receive("warning").with_args(
         "Log Detective response is missing log_detective_analysis_id",
@@ -311,6 +319,7 @@ def test_logdetective_koji_missing_time(
             "log_detective_analysis_id": "test-uuid-123",
         }
     )
+    flexmock(logdetective_module).should_receive("verify_artifact").and_return(True)
     flexmock(requests).should_receive("post").and_return(mock_response)
     flexmock(logger).should_receive("warning").with_args(
         "Log Detective response is missing creation_time",

tests/unit/test_utils.py

@@ -1,6 +1,7 @@
 # Copyright Contributors to the Packit project.
 # SPDX-License-Identifier: MIT
 import pytest
+import requests
 from flexmock import flexmock
 from packit.config.aliases import Distro
 
@@ -9,6 +10,7 @@
     get_default_tf_mapping,
     only_once,
     pr_labels_match_configuration,
+    verify_artifact,
 )
 
 
@@ -190,3 +192,47 @@ def test_get_default_tf_mapping(internal, target, compose):
     )
     mapping = get_default_tf_mapping(internal)
     assert mapping[target] == compose
+
+
+def test_verify_artifact_valid():
+    url = "https://example.com/valid.txt"
+    mock_response = flexmock(status_code=200, headers={"Content-Type": "text/plain; charset=utf-8"})
+
+    mock_response.should_receive("__enter__").and_return(mock_response)
+    mock_response.should_receive("__exit__").and_return(None)
+
+    flexmock(requests).should_receive("get").once().and_return(mock_response)
+
+    assert verify_artifact(url) is True
+
+
+def test_verify_artifact_404_not_found():
+    url = "https://example.com/missing.txt"
+
+    mock_response = flexmock(status_code=404, headers={})
+    mock_response.should_receive("__enter__").and_return(mock_response)
+    mock_response.should_receive("__exit__").and_return(None)
+
+    flexmock(requests).should_receive("get").once().and_return(mock_response)
+
+    assert verify_artifact(url) is False
+
+
+def test_verify_artifact_wrong_type():
+    url = "https://example.com/api/data.json"
+
+    mock_response = flexmock(status_code=200, headers={"Content-Type": "application/json"})
+    mock_response.should_receive("__enter__").and_return(mock_response)
+    mock_response.should_receive("__exit__").and_return(None)
+
+    flexmock(requests).should_receive("get").once().and_return(mock_response)
+
+    assert verify_artifact(url) is False
+
+
+def test_verify_artifact_network_failure():
+    url = "https://broken-link.com"
+
+    flexmock(requests).should_receive("get").and_raise(requests.exceptions.ConnectionError)
+
+    assert verify_artifact(url) is False