From 31ecfad2183f1feb6d788877211ae8021f9b9b23 Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Thu, 29 Jul 2021 14:19:36 +0200 Subject: [PATCH] Fix #524 - Fix incorrect journal state when combining scope filter and RPKI (#529) --- irrd/rpki/validators.py | 3 ++- irrd/scopefilter/validators.py | 2 +- irrd/storage/database_handler.py | 26 +++++++++++++++----------- irrd/storage/tests/test_database.py | 25 ++++++++++++++++++++++++- 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/irrd/rpki/validators.py b/irrd/rpki/validators.py index 0ed25be50..99d70eaa1 100644 --- a/irrd/rpki/validators.py +++ b/irrd/rpki/validators.py @@ -45,6 +45,7 @@ class BulkRouteROAValidator: 11000000000000000000001000, and therefore will (and should) not be included in the validation process. """ + def __init__(self, dh: DatabaseHandler, roas: Optional[List[ROA]] = None): """ Create a validator object. Can use either a list of ROA objects, @@ -85,7 +86,7 @@ def validate_all_routes(self, sources: List[str]=None) -> \ validation result, are not included in the return value. """ columns = ['pk', 'rpsl_pk', 'ip_first', 'prefix_length', 'asn_first', 'source', - 'rpki_status'] + 'rpki_status', 'scopefilter_status'] q = RPSLDatabaseQuery(column_names=columns, enable_ordering=False) q = q.object_classes(['route', 'route6']) if sources: diff --git a/irrd/scopefilter/validators.py b/irrd/scopefilter/validators.py index d3bac43c9..d6b496334 100644 --- a/irrd/scopefilter/validators.py +++ b/irrd/scopefilter/validators.py @@ -113,7 +113,7 @@ def validate_all_rpsl_objects(self, database_handler: DatabaseHandler) -> \ validation result, are not included in the return value. """ columns = ['rpsl_pk', 'ip_first', 'prefix_length', 'asn_first', 'source', 'object_class', - 'object_text', 'scopefilter_status'] + 'object_text', 'scopefilter_status', 'rpki_status'] objs_changed: Dict[ScopeFilterStatus, List[Dict[str, str]]] = defaultdict(list) diff --git a/irrd/storage/database_handler.py b/irrd/storage/database_handler.py index 0f4662d0a..cca876edd 100644 --- a/irrd/storage/database_handler.py +++ b/irrd/storage/database_handler.py @@ -275,7 +275,10 @@ def update_rpki_status(self, rpsl_objs_now_valid: List[Dict[str, str]]=[], self.execute_statement(stmt) for rpsl_obj in rpsl_objs_now_valid + rpsl_objs_now_not_found: - if rpsl_obj['old_status'] == RPKIStatus.invalid: + if all([ + rpsl_obj['old_status'] == RPKIStatus.invalid, + rpsl_obj['scopefilter_status'] == ScopeFilterStatus.in_scope + ]): self.status_tracker.record_operation( operation=DatabaseOperation.add_or_update, rpsl_pk=rpsl_obj['rpsl_pk'], @@ -324,16 +327,17 @@ def update_scopefilter_status(self, rpsl_objs_now_in_scope: List[Dict[str, str]] self.execute_statement(stmt) for rpsl_obj in rpsl_objs_now_in_scope: - self.status_tracker.record_operation( - operation=DatabaseOperation.add_or_update, - rpsl_pk=rpsl_obj['rpsl_pk'], - source=rpsl_obj['source'], - object_class=rpsl_obj['object_class'], - object_text=rpsl_obj['object_text'], - origin=JournalEntryOrigin.scope_filter, - source_serial=None, - ) - self._object_classes_modified.add(rpsl_obj['object_class']) + if rpsl_obj['rpki_status'] != RPKIStatus.invalid: + self.status_tracker.record_operation( + operation=DatabaseOperation.add_or_update, + rpsl_pk=rpsl_obj['rpsl_pk'], + source=rpsl_obj['source'], + object_class=rpsl_obj['object_class'], + object_text=rpsl_obj['object_text'], + origin=JournalEntryOrigin.scope_filter, + source_serial=None, + ) + self._object_classes_modified.add(rpsl_obj['object_class']) for rpsl_obj in rpsl_objs_now_out_scope_as + rpsl_objs_now_out_scope_prefix: if rpsl_obj['old_status'] == ScopeFilterStatus.in_scope: diff --git a/irrd/storage/tests/test_database.py b/irrd/storage/tests/test_database.py index b502c85b4..89d72249b 100644 --- a/irrd/storage/tests/test_database.py +++ b/irrd/storage/tests/test_database.py @@ -83,6 +83,7 @@ class TestDatabaseHandlerLive: """ This test covers mainly DatabaseHandler and DatabaseStatusTracker. """ + def test_object_writing_and_status_checking(self, monkeypatch, irrd_database, config_override): config_override({ 'sources': { @@ -405,6 +406,7 @@ def test_rpki_status_storage(self, monkeypatch, irrd_database, database_handler_ 'object_class': 'route', 'object_text': 'object-text', 'old_status': RPKIStatus.invalid, # This always triggers a journal entry + 'scopefilter_status': ScopeFilterStatus.in_scope, }] assert len(list(dh.execute_query(RPSLDatabaseQuery().rpki_status([RPKIStatus.invalid])))) == 1 @@ -444,6 +446,16 @@ def test_rpki_status_storage(self, monkeypatch, irrd_database, database_handler_ dh.update_rpki_status(rpsl_objs_now_valid=route_rpsl_objs) assert not list(dh.execute_query(RPSLDatabaseJournalQuery().serial_range(6, 6))) + # State change from invalid to valid should not create journal entry + # if scope filter is still out of scope + route_rpsl_objs[0].update({ + 'old_status': RPKIStatus.invalid, + 'scopefilter_status': ScopeFilterStatus.out_scope_as, + }) + dh.update_rpki_status(rpsl_objs_now_valid=route_rpsl_objs) + assert len(list(dh.execute_query(RPSLDatabaseQuery().rpki_status([RPKIStatus.valid])))) == 1 + assert not list(dh.execute_query(RPSLDatabaseJournalQuery().serial_range(6, 6))) + def test_scopefilter_status_storage(self, monkeypatch, irrd_database, database_handler_with_route): monkeypatch.setenv('IRRD_SOURCES_TEST_KEEP_JOURNAL', '1') dh = database_handler_with_route @@ -452,7 +464,8 @@ def test_scopefilter_status_storage(self, monkeypatch, irrd_database, database_h 'source': 'TEST', 'object_class': 'route', 'object_text': 'object-text', - 'old_status': ScopeFilterStatus.in_scope, # This always triggers a journal entry + 'old_status': ScopeFilterStatus.in_scope, + 'rpki_status': RPKIStatus.valid, }] assert len(list(dh.execute_query(RPSLDatabaseQuery().scopefilter_status([ScopeFilterStatus.in_scope])))) == 1 @@ -476,6 +489,16 @@ def test_scopefilter_status_storage(self, monkeypatch, irrd_database, database_h journal_entry = list(dh.execute_query(RPSLDatabaseJournalQuery().serial_range(2, 2))) assert journal_entry[0]['operation'] == DatabaseOperation.add_or_update + # Special case: updating the status from out to in scope while RPKI invalid, + # should change the status but not create a journal entry - see #524 + route_rpsl_objs[0].update({ + 'old_status': ScopeFilterStatus.out_scope_as, + 'rpki_status': RPKIStatus.invalid, + }) + dh.update_scopefilter_status(rpsl_objs_now_in_scope=route_rpsl_objs) + assert len(list(dh.execute_query(RPSLDatabaseQuery().scopefilter_status([ScopeFilterStatus.in_scope])))) == 1 + assert len(list(dh.execute_query(RPSLDatabaseJournalQuery()))) == 2 # no new entry since last test + def _clean_result(self, results): variable_fields = ['pk', 'timestamp', 'created', 'updated', 'last_error_timestamp'] return [{k: v for k, v in result.items() if k not in variable_fields} for result in list(results)]