Introduce (basic) Instances#617
Open
bal-e wants to merge 6 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds
zone/instance.rsas the primary location for information about the (current, upcoming, and historical) instances of zones. It replaces several bits of state that were independently tracking some of this information, and offers space to store more information in the future (e.g. DNSSEC data for the current published instance for re-signing).This PR does not affect Cascade's external behavior, beyond some minor changes in logging. Its primary purpose is to simplify the codebase. It is a stripped-down version of #528.
The following bits of state are added:
zone/instance.rs:CurrentInstance: the state of the instance published by Cascade.UpcomingInstance: the state of an upcoming instance being built right now.zone/machine.rs):decidedboolean onLoadedReviewandSignedReviewwhich tracks whether a review decision has been received.They replace the following bits of state:
StorageState(zone/storage.rs):loaded_review_soa,signed_review_soa,published_loaded_soa,published_soazone/mod.rs:ZoneState::last_publishedandLastPublishedzone/mod.rs:ZoneState::{unsigned,signed},{Unsigned,Signed}ZoneVersionState, andZoneVersionReviewStateAn additional change in this PR is refactoring
ZoneServer::on_zone_review(). This method is called fromLoadedReviewServerandSignedReviewServer, and they contained TODOs for inliningon_zone_review()into them, as this would simplify the implementation. This PR heavily affected how zone review decisions are processed, so I had to rewriteon_zone_review(). I took the time to also inline it intoLoadedReviewServerandSignedReviewServer; the code looks much better for it.Open questions:
Future work:
Introduce instance IDs and use them for review. This will make review resilient against duplicate SOA serials (this is primarily a concern for loaded review).
Track obsolete instances of zones. These instances are relevant to IXFR out, so this might serve as the right place to track information about them (including persisted diffs).
Track abandoned instances of zones (zones that e.g. failed review). This could be nice for more zone history. We could choose to persist abandoned instances to disk for operators to review (e.g. if they use soft rejection but suspect a bug occurred), and store information about it here.
Move some zone signer state from
ZoneState(e.g.min_expiration) toinstance::SignedInstance. This will fix a subtle bug where abandoning a signed instance can desynchronize the incremental signer (it might assume the instance was published, and this could affect re-signing schedules).Cargo.*,crates/,etc/,integration-tests/,src/):actthrough theact-wrapper(as described inTESTING.md)?