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.

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

Remove PreSaveEventListener #1182

Open
timbru opened this issue Dec 28, 2023 · 0 comments
Open

Remove PreSaveEventListener #1182

timbru opened this issue Dec 28, 2023 · 0 comments

Comments

@timbru
Copy link
Contributor

timbru commented Dec 28, 2023

We currently use PreSaveEventListener to trigger certain actions before saving an update to an aggregate (a CertAuth in particular). The issue with this is that, in principle, it's possible for this triggered event to succeed and the saving of the updated CA to fail - e.g. because of a disk full or reboot happening right at the wrong moment.

While the chances of this happening are very low, it would still be better to use the PostSaveEventListener instead. I.e. these listeners are used to trigger that follow-up actions are scheduled on the task queue.

The code currently uses PreSaveEventListener to trigger a new manifest and CRL to be generated (in CaObjects) whenever a new RPKI object (such as a ROA) is created in a CA. The other path for updating the manifest and CRL is through a background job that triggers the re-issuance of the manifest and CRL before they expire. See: Task::RepublishIfNeeded.

This could lead to a corner case where a new CRL and Manifest are created, pre-save, and then saving the CertAuth fails. So, it would be better to have an idempotent task for this instead.

It would be better to make the CaObjects code for Task::RepublishIfNeeded idempotent and re-schedule it "PostSave" when a relevant change occurs. The CaObjects code would then no longer get an event with changed RPKI objects but would simply need to query the CA for the complete current object set so that it can decide whether a content change occured since the last manifest/CRL was issued, in which case it could re-issue.

This also allows one to re-implement CaObjects and use the WalSupport trait instead of the way things are done now. Reducing the different ways of handling state changes would make the code easier to understand and more consistent.

@timbru timbru created this issue from a note in Long Term Categorised Backlog (code) Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant