Skip to content

Commit

Permalink
security: change timeout for vulnerability requests (PROJQUAY-7751) (q…
Browse files Browse the repository at this point in the history
…uay#3194)

Changes the timeout value for non-indexing requests to clair from 600 to
30.

Because the timeout for a vulnerability report request is so high, heavy
traffic to the security enpdoint results in database connections being
exhausted. Lowering the timeout value should allow requests to complete
and connections to the database to close.
  • Loading branch information
Marcusk19 authored Sep 6, 2024
1 parent 06a816d commit c49ba17
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 12 deletions.
33 changes: 21 additions & 12 deletions util/secscan/v4/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
logger = logging.getLogger(__name__)

DOWNLOAD_VALIDITY_LIFETIME_S = 60 # Amount of time the security scanner has to call the layer URL
DEFAULT_REQUEST_TIMEOUT = 30
INDEX_REQUEST_TIMEOUT = 600


Expand Down Expand Up @@ -116,17 +117,23 @@ def retrieve_notification_page(self, notification_id, next_param=None):
Action = namedtuple("Action", ["name", "payload"])

actions: Dict[str, Callable[..., Action]] = {
"IndexState": lambda: Action("IndexState", ("GET", "/indexer/api/v1/index_state", None)),
"Index": lambda manifest: Action("Index", ("POST", "/indexer/api/v1/index_report", manifest)),
"IndexState": lambda: Action(
"IndexState", ("GET", "/indexer/api/v1/index_state", None, INDEX_REQUEST_TIMEOUT)
),
"Index": lambda manifest: Action(
"Index", ("POST", "/indexer/api/v1/index_report", manifest, INDEX_REQUEST_TIMEOUT)
),
"GetIndexReport": lambda manifest_hash: Action(
"GetIndexReport", ("GET", "/indexer/api/v1/index_report/" + manifest_hash, None)
"GetIndexReport",
("GET", "/indexer/api/v1/index_report/" + manifest_hash, None, INDEX_REQUEST_TIMEOUT),
),
"GetVulnerabilityReport": lambda manifest_hash: Action(
"GetVulnerabilityReport",
(
"GET",
"/matcher/api/v1/vulnerability_report/" + manifest_hash,
None,
DEFAULT_REQUEST_TIMEOUT,
),
),
"DeleteNotification": lambda notification_id: Action(
Expand All @@ -135,6 +142,7 @@ def retrieve_notification_page(self, notification_id, next_param=None):
"DELETE",
"/notifier/api/v1/notification/%s" % (notification_id),
None,
DEFAULT_REQUEST_TIMEOUT,
),
),
"GetNotification": lambda notification_id, next_param: Action(
Expand All @@ -144,15 +152,12 @@ def retrieve_notification_page(self, notification_id, next_param=None):
"/notifier/api/v1/notification/%s%s"
% (notification_id, "?next=" + next_param if next_param else ""),
None,
DEFAULT_REQUEST_TIMEOUT,
),
),
"DeleteIndexReport": lambda manifest_hash: Action(
"DeleteIndexReport",
(
"DELETE",
"/indexer/api/v1/index_report/" + manifest_hash,
None,
),
("DELETE", "/indexer/api/v1/index_report/" + manifest_hash, None, INDEX_REQUEST_TIMEOUT),
),
}

Expand Down Expand Up @@ -320,7 +325,7 @@ def vulnerability_report(self, manifest_hash):

def _perform(self, action):
request_start_time = time.time()
(method, path, body) = action.payload
(method, path, body, timeout) = action.payload
url = urljoin(self.secscan_api_endpoint, path)

headers = {}
Expand All @@ -331,15 +336,19 @@ def _perform(self, action):

logger.debug("%sing security URL %s", method.upper(), url)
try:
resp = self._client.request(
method, url, json=body, headers=headers, timeout=INDEX_REQUEST_TIMEOUT
)
resp = self._client.request(method, url, json=body, headers=headers, timeout=timeout)
except requests.exceptions.ConnectionError as ce:
logger.exception("Connection error when trying to connect to security scanner endpoint")
msg = "Connection error when trying to connect to security scanner endpoint: %s" % str(
ce
)
raise APIRequestFailure(msg)
except requests.exceptions.Timeout as te:
logger.exception("Security scanner endpoint timed out")
msg = "Connection error when trying to connect to security scanner endpoint: %s" % str(
te
)
raise APIRequestFailure(msg)

dur = time.time() - request_start_time
secscan_request_duration.labels(method, action.name, resp.status_code).observe(dur)
Expand Down
9 changes: 9 additions & 0 deletions util/secscan/v4/test/test_secscan.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import pytest
from mock import patch
from requests.exceptions import Timeout

from app import instance_keys, storage
from config import build_requests_session
Expand Down Expand Up @@ -101,3 +102,11 @@ def test_vulnerability_report_incompatible_api_response(api, initialized_db):
layers = registry_model.list_manifest_layers(manifest, storage, True)

api.vulnerability_report(manifest.digest)


def test_vulnerability_report_timeout(api, initialized_db):
with fake_security_scanner() as security_scanner:
with pytest.raises(APIRequestFailure):
with patch.object(api._client, "request", side_effect=Timeout):
manifest = manifest_for("devtable", "simple", "latest")
api.vulnerability_report(manifest.digest)

0 comments on commit c49ba17

Please sign in to comment.