Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow overriding MetadataSyncJob timeout via Qubes service flag #1552

Merged
merged 4 commits into from
Sep 13, 2022
Merged

feat: allow overriding MetadataSyncJob timeout via Qubes service flag #1552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e83215
refactor(MetadataSyncJob): define constant DEFAULT_REQUEST_TIMEOUT
cfm Aug 30, 2022
2e4ba6b
chore: export Qubes service flag "SDEXTENDEDTIMEOUT_N" as SDEXTENDEDT…
cfm Aug 30, 2022
715d7ab
test: MetadataSyncJob obeys $SDEXTENDEDTIMEOUT
cfm Aug 30, 2022
0679ae4
feat(MetadataSyncJob): override API.default_request_timeout if $SDEXT…
cfm Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions files/securedrop-client
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,13 @@ cd /opt/venvs/securedrop-client
# Check if qubes-db exists (and we are running in qubes)
if [ ! -f "/usr/bin/qubesdb-read" ]; then echo "Not running in Qubes, client not starting." && exit; fi

# EXPERIMENTAL(#1547): Check for the SDEXTENDEDTIMEOUT_N service flag and export it as
# SDEXTENDEDTIMEOUT=N.
timeout_flag_value=$(qubesdb-list /qubes-service/SDEXTENDEDTIMEOUT_)
if [ -n "$timeout_flag_value" ]; then
echo "SDEXTENDEDTIMEOUT=$timeout_flag_value"
export SDEXTENDEDTIMEOUT="$timeout_flag_value"
fi

# Now execute the actual client, only if running in an sd-app
if [ "$(qubesdb-read /name)" = "sd-app" ]; then ./bin/sd-client; else echo "Not running in sd-app, client not starting."; fi
12 changes: 11 additions & 1 deletion securedrop_client/api_jobs/sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
import os
from typing import Any, List, Optional

from sdclientapi import API
Expand All @@ -18,6 +19,7 @@ class MetadataSyncJob(ApiJob):
Update source metadata such that new download jobs can be added to the queue.
"""

DEFAULT_REQUEST_TIMEOUT = 60 # sec
NUMBER_OF_TIMES_TO_RETRY_AN_API_CALL = 2

def __init__(self, data_dir: str, app_state: Optional[state.State] = None) -> None:
Expand All @@ -40,7 +42,15 @@ def call_api(self, api_client: API, session: Session) -> Any:
#
# This timeout is used for 3 different requests: `get_sources`, `get_all_submissions`, and
# `get_all_replies`
api_client.default_request_timeout = 60
api_client.default_request_timeout = int(
os.environ.get("SDEXTENDEDTIMEOUT", self.DEFAULT_REQUEST_TIMEOUT)
)
if api_client.default_request_timeout != self.DEFAULT_REQUEST_TIMEOUT:
logger.warn(
f"{self.__class__.__name__} will use "
f"default_request_timeout={api_client.default_request_timeout}"
)
Comment on lines +49 to +52
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor pet peeve of mine: since the message doesn't require action (e.g. like a deprecation warning does), I would prefer it to be at INFO level.

I say pet peeve because I find personally that most warnings shouldn't be emitted (potentially controversial opinion) because they fall into the following categories:

  • Errors that are not fatal because they're handled. (In my experience, the source of the error is unlikely to be fixed anyway if given the opportunity the choice was made, instead, to print a warning. 🤷 So the warning is mostly noise IMHO.)
  • Fatal errors, which shouldn't have happened and would better be logged at the ERROR level since they signal a bug to be fixed. (Foreseeable errors must be handled.)
  • Or messages that don't require user action and only provide context. (At which point I think competing for attention at the warning level contributes more to error-fatigue than it really helps keeping us informed. Using an INFO level seems more honest to me.)

What would I make a warning? Things that will become fatal errors unless action is taken, e.g. deprecation warnings. YMMV! Admittedly it's nitpickey and mostly a matter of personal preference. 🙂

Copy link
Member Author

@cfm cfm Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree with your criteria, @gonzalo-bulnes. In this specific case, I'm rounding up slightly from our decision to call out this flag "explicitly as experimental, i.e. we cannot guarantee that we will support it in feature" to something akin to a deprecation warning. But I'm happy to demote this message to INFO if that logic isn't persuasive.

(See also: #1166.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that makes sense @cfm I missed that intent! I'm not sure what context goes into the current wording: would there be an opportunity to call out explicitly (and additionally to the warning level): "WARNING: ... use experimental default_..."?


users = api_client.get_users()
MetadataSyncJob._update_users(session, users)
sources, submissions, replies = get_remote_data(api_client)
Expand Down
27 changes: 27 additions & 0 deletions tests/api_jobs/test_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
Submission = namedtuple("Submission", ["uuid", "source_uuid", "is_file", "is_downloaded"])
File = namedtuple("File", ["is_downloaded"])

TIMEOUT_OVERRIDE = 600 # sec


class TestUpdateState(unittest.TestCase):
def setUp(self):
Expand All @@ -37,6 +39,31 @@ def test_handles_missing_files_gracefully(self):
assert self._state.file("9")


def test_MetadataSyncJob_has_default_timeout(mocker, homedir, session, session_maker):
api_client = mocker.patch("securedrop_client.logic.sdclientapi.API")
remote_user = factory.RemoteUser()
api_client.get_users = mocker.MagicMock(return_value=[remote_user])

job = MetadataSyncJob(homedir)
job.call_api(api_client, session)
assert api_client.default_request_timeout == job.DEFAULT_REQUEST_TIMEOUT


def test_MetadataSyncJob_takes_overriden_timeout(mocker, homedir, session, session_maker):
api_client = mocker.patch("securedrop_client.logic.sdclientapi.API")
remote_user = factory.RemoteUser()
api_client.get_users = mocker.MagicMock(return_value=[remote_user])

os.environ["SDEXTENDEDTIMEOUT"] = str(TIMEOUT_OVERRIDE) # environment value must be string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One alternative strategy to consider for tests that modify env vars might be to mock os.environ, see https://github.com/freedomofpress/securedrop-workstation/blob/b31d0acdf35cf5248acbe2dcd9372483dfde424f/launcher/tests/test_util.py#L259-L266 for an example (not a blocking comment, just an observation).


job = MetadataSyncJob(homedir)
job.call_api(api_client, session)
assert api_client.default_request_timeout == TIMEOUT_OVERRIDE

# Don't pollute the environment for subsequent/out-of-order tests.
del os.environ["SDEXTENDEDTIMEOUT"]


def test_MetadataSyncJob_creates_new_user(mocker, homedir, session, session_maker):
api_client = mocker.patch("securedrop_client.logic.sdclientapi.API")
remote_user = factory.RemoteUser()
Expand Down