Skip to content

Commit

Permalink
Remove StorageError exception handling
Browse files Browse the repository at this point in the history 
We decided that it's best to not handle a StorageError exception
and instead allow the test to fail. Fast API will fail if there is an
exception from the worker and given that the StorageError exception
has enough details there is no need for us to handle it.

Additionally, this allowed us to remove the boolean return value
from _run_online_roles_bump and bump_snapshot.

Signed-off-by: Martin Vrachev <[email protected]>
  • Loading branch information
@MVrachev
MVrachev committed Jan 4, 2024
1 parent 558ac3b commit 8aede98
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 47 deletions.
27 changes: 5 additions & 22 deletions repository_service_tuf_worker/repository.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@
from celery.exceptions import ChordError
from celery.result import AsyncResult, states
from dynaconf.loaders import redis_loader
from securesystemslib.exceptions import ( # type: ignore
StorageError,
UnverifiedSignatureError,
)
from securesystemslib.exceptions import UnverifiedSignatureError
from securesystemslib.signer import Signature, SSlibKey
from tuf.api.exceptions import (
BadVersionNumberError,
Expand Down Expand Up @@ -960,7 +957,7 @@ def remove_targets(
},
)

def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
def _run_online_roles_bump(self, force: Optional[bool] = False):
"""
Bumps version and expiration date of all online roles (`Targets`,
`Succinct Delegated` targets roles, `Timestamp` and `Snapshot`).
Expand All @@ -975,12 +972,7 @@ def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
force: force all target roles bump even if they have more than
`self._hours_before_expire` hours to expire.
"""
try:
targets: Metadata = self._storage_backend.get(Targets.type)
except StorageError:
logging.error(f"{Targets.type} not found, not bumping.")
return False

targets: Metadata = self._storage_backend.get(Targets.type)
timestamp: Metadata
snapshot_bump = False
if self._settings.get_fresh("TARGETS_ONLINE_KEY") is None:
Expand Down Expand Up @@ -1040,9 +1032,7 @@ def _run_online_roles_bump(self, force: Optional[bool] = False) -> bool:
f"[scheduled bump] Timestamp version bumped: {timestamp_v}"
)

return True

def bump_snapshot(self, force: Optional[bool] = False) -> bool:
def bump_snapshot(self, force: Optional[bool] = False):
"""
Bumps version and expiration date of TUF 'snapshot' role metadata.
Expand All @@ -1057,12 +1047,7 @@ def bump_snapshot(self, force: Optional[bool] = False) -> bool:
expire (`self._hours_before_expire`)
"""

try:
snapshot = self._storage_backend.get(Snapshot.type)
except StorageError:
logging.error(f"{Snapshot.type} not found, not bumping.")
return False

snapshot = self._storage_backend.get(Snapshot.type)
if (snapshot.signed.expires - datetime.now()) < timedelta(
hours=self._hours_before_expire
) or force:
Expand All @@ -1084,8 +1069,6 @@ def bump_snapshot(self, force: Optional[bool] = False) -> bool:
f"{self._hours_before_expire} hour, skipping"
)

return True

def bump_online_roles(self, force: Optional[bool] = False) -> bool:
"""
Bump online roles (Snapshot, Timestamp, Targets and BINS).
Expand Down
40 changes: 15 additions & 25 deletions tests/unit/tuf_repository_service_worker/test_repository.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import pytest
from celery.exceptions import ChordError
from celery.result import states
from securesystemslib.exceptions import StorageError
from tuf.api.metadata import (
Metadata,
MetaFile,
Expand Down Expand Up @@ -2143,8 +2144,7 @@ def test__run_online_roles_bump_only_expired(
)
)
)
result = test_repo._run_online_roles_bump()
assert result is True
test_repo._run_online_roles_bump()
assert test_repo._storage_backend.get.calls == [
pretend.call(Targets.type),
pretend.call("bin-a"),
Expand Down Expand Up @@ -2198,8 +2198,7 @@ def test__run_online_roles_bump_force(
)
)
)
result = test_repo._run_online_roles_bump(force=True)
assert result is True
test_repo._run_online_roles_bump(force=True)
assert test_repo._storage_backend.get.calls == [
pretend.call(Targets.type),
]
Expand Down Expand Up @@ -2262,10 +2261,9 @@ def test__run_online_roles_bump_target_targets_online_key_config_false(
)
)
)
result = test_repo._run_online_roles_bump()
test_repo._run_online_roles_bump()
msg = "targets don't use online key, skipping 'Targets' role"
assert msg == caplog.messages[0]
assert result is True
assert test_repo._storage_backend.get.calls == [
pretend.call(Targets.type),
pretend.call("bin-a"),
Expand Down Expand Up @@ -2315,10 +2313,9 @@ def test__run_online_roles_bump_warning_missing_config(
)
)
)
result = test_repo._run_online_roles_bump()
test_repo._run_online_roles_bump()
msg = "No configuration found for TARGETS_ONLINE_KEY"
assert msg == caplog.messages[0]
assert result is True
assert test_repo._storage_backend.get.calls == [
pretend.call(Targets.type),
pretend.call("bin-a"),
Expand Down Expand Up @@ -2355,8 +2352,7 @@ def test__run_online_roles_bump_no_changes(self, test_repo, caplog):
else fake_bins
)

result = test_repo._run_online_roles_bump()
assert result is True
test_repo._run_online_roles_bump()
assert test_repo._storage_backend.get.calls == [
pretend.call(Targets.type),
pretend.call("bin-a"),
Expand All @@ -2370,11 +2366,11 @@ def test__run_online_roles_bump_no_changes(self, test_repo, caplog):

def test__run_online_roles_bump_StorageError(self, test_repo):
test_repo._storage_backend.get = pretend.raiser(
repository.StorageError("Overwrite it")
StorageError("Overwrite it")
)

result = test_repo._run_online_roles_bump()
assert result is False
with pytest.raises(StorageError):
test_repo._run_online_roles_bump()

def test_bump_snapshot(self, test_repo, mocked_datetime):
fake_snapshot = pretend.stub(
Expand All @@ -2400,8 +2396,7 @@ def test_bump_snapshot(self, test_repo, mocked_datetime):
)
)

result = test_repo.bump_snapshot()
assert result is True
test_repo.bump_snapshot()
assert test_repo._storage_backend.get.calls == [
pretend.call("snapshot")
]
Expand All @@ -2422,8 +2417,7 @@ def test_bump_snapshot_unexpired(self, test_repo):
lambda *a: fake_snapshot
)

result = test_repo.bump_snapshot()
assert result is True
test_repo.bump_snapshot()
assert test_repo._storage_backend.get.calls == [
pretend.call("snapshot")
]
Expand Down Expand Up @@ -2466,8 +2460,7 @@ def test_bump_snapshot_check_force_is_acknowledged(
)
)

result = test_repo.bump_snapshot(force=True)
assert result is True
test_repo.bump_snapshot(force=True)
assert test_repo._storage_backend.get.calls == [
pretend.call(Snapshot.type)
]
Expand All @@ -2477,12 +2470,9 @@ def test_bump_snapshot_check_force_is_acknowledged(
]

def test_bump_snapshot_not_found(self, test_repo):
test_repo._storage_backend.get = pretend.raiser(
repository.StorageError
)

result = test_repo.bump_snapshot()
assert result is False
test_repo._storage_backend.get = pretend.raiser(StorageError)
with pytest.raises(StorageError):
test_repo.bump_snapshot()

def test_bump_online_roles(self, monkeypatch, test_repo):
@contextmanager
Expand Down

0 comments on commit 8aede98

Please sign in to comment.