repository-service-tuf / repository-service-tuf-worker

Repository Service for TUF: Worker
MIT License
8 stars 15 forks source link

Fix two bugs & do a small refactoring #501

Closed MVrachev closed 4 months ago

MVrachev commented 5 months ago

In this pr I address two bugs and one refactoring:

  1. Don't persist root if the online roles bump fails. This makes sure that the root is stored last when all online roles are bumped.
  2. Give a trusted_root_update() with a more telling name. As we don't update trusted root with _trusted_root_update() and this makes the function name misleading.
  3. Fix: update the delegation key when bumping targets. Strangely, we forgot to change the key referenced in top level targets role when bumping targets with an online key change. This caused a bug that was reported: https://github.com/repository-service-tuf/repository-service-tuf-worker/issues/491

Fixes #491

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (714a29d) to head (f2e3f85). Report is 37 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #501 +/- ## ========================================== Coverage 100.00% 100.00% ========================================== Files 15 15 Lines 1071 1207 +136 ========================================== + Hits 1071 1207 +136 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MVrachev commented 5 months ago

With this pr, I think I have solved issue #494 as well. I have made sure we persist root as the last step.

The other part - is to make sure we are persisting timestamp as the last online role is solved as well as:

  1. We have implemented _run_online_roles_bump() in such a way that makes sure that targets role is first updated, then we call update_snapshot which updates the delegated targets roles, then updates snapshot and finally the new snapshot version is passed to update_timestamp() where we update timestamp.
  2. Each time when we call update_timestamp we call update_snapshot() first which updates all delegated target roles and snapshot. There is only one exception to that rule - when the user forces a new version of timestamp only: https://github.com/repository-service-tuf/repository-service-tuf-worker/blob/63c77974a402b80f618aecb9f124a6466d61149b/repository_service_tuf_worker/repository.py#L1268, but there we don't need to update anything beforehand.

Because of these reasons, I think we do make sure the timestamp will be the last saved from the online metadata roles. What do you think @kairoaraujo? Do we need any tests for that and what they should be?

kairoaraujo commented 4 months ago

Because of these reasons, I think we do make sure the timestamp will be the last saved from the online metadata roles. What do you think @kairoaraujo? Do we need any tests for that and what they should be?

The overall issue #494 is solved with your implementation. I agree that the issue solves the bug #494. The only part I think we can still improve is the recovery from a failure while saving Root. But we should do it in another issue.

MVrachev commented 4 months ago

I addressed your comment @kairoaraujo in my latest commit.

kairoaraujo commented 4 months ago

Amazing work @MVrachev!

matglas commented 4 months ago

Awesome!