xfr sources: republish upstream changes, propagate removals, advance the serial (#195, #197, #198) - #202
Closed
johanix wants to merge 4 commits into
Closed
xfr sources: republish upstream changes, propagate removals, advance the serial (#195, #197, #198)#202johanix wants to merge 4 commits into
johanix wants to merge 4 commits into
Conversation
…197) An RPZ zone-transfer source stopped affecting the served zone once pop had started. The transfer itself was fine -- the upstream was polled, the serial increase was noticed, the zone was pulled and parsed -- but nothing downstream of the parse ever ran, so the new data sat in pop's lists and never reached the zone it serves. Only the periodic refresh path was affected, which is why startup looked correct and everything after it did not. Removals had a second problem on top: the parse function wrote names straight into the source's Names map, which only ever grew. A name withdrawn upstream stayed in pop's list for the lifetime of the process, so even a republish would have kept serving it. Both are fixed by making a transfer produce a DELTA: - rpzFeedIngest stages the names one transfer carried. Commit() diffs that against what the source already held, applies the difference, and returns the added and removed names. A failed transfer is discarded, so a partial one cannot leak into the next delta. - applyUpstreamDelta turns that delta into a publish, via GenerateRpzIxfr plus NotifyDownstreams, and is called from every path that transfers a source -- including the two at startup. Missing one of those is not a partial fix but a broken daemon: with the parse function staging rather than writing, an uncommitted transfer serves nothing at all. A delta rather than replacing the name set wholesale, because inbound IXFR will deliver adds and deletes directly off the wire and can be applied through the same Commit path without materialising the whole zone. The initial transfer of a source commits without publishing: ParseSourcesNG builds the full zone once every source has loaded, and publishing per-source before that would emit a partial zone. Not covered, and documented at the type: the shared doubt_catchall and allow_catchall buckets still only grow. They carry no record of which source contributed a name, so a per-source delta cannot be applied to them. Filed separately as #199.
…#198) A rebuild that changed the served zone left the SOA serial where it was, so a downstream holding that serial had no reason to transfer and kept serving the old zone. An IXFR-following downstream was told nothing had changed at all. The serial arithmetic was correct; its input was not. ParseOutputs re-reads the serial cache into pd.Rpz.CurrentSerial, and it runs twice at startup -- once from NewPopData, then again from main after ParseSourcesNG has already built and published the zone. The second run reset the counter behind the running zone's back. With no cache file present that meant serving at serial 2 while the counter said 1, and the next update computed 1+1 and republished CHANGED content under a serial downstreams already held. With a cache file present it still rewinds, discarding whatever the startup build had advanced. - GenerateRpzAxfr and GenerateRpzIxfr now derive the next serial from the PUBLISHED snapshot rather than from the counter. The snapshot is what downstreams actually hold, and it is the one value that cannot be rewound by config code re-running. - The serial-cache read moves out of ParseOutputs into loadCachedSerial, called once from NewPopData before anything is published. Reading a cached serial was never part of parsing outputs; keeping it there made an otherwise idempotent function destructive to call twice, which both of its call sites already assumed it was not. Also: a rebuild that produces identical content no longer republishes at all. Bumping the serial unconditionally would trade a stale downstream for pointless transfers and serial churn on every rebuild. Content is compared by name and action, not by RR pointer, which differs on every rebuild regardless.
The RPZ parse function carefully stores an Action off each upstream rule's CNAME target, and decide() then answers a denylisted name with the configured policy.denylist.action instead. Read cold, that looks like stored data nobody uses, and the obvious "fix" is to honour the upstream's action. That would be wrong. Every input -- RPZ feeds, MQTT, files -- is only a trigger saying a name is interesting; local policy decides what is actually done about it. The plain RPZ model, where the first zone whose rule matches settles the action, is deliberately not pop's model. Comment only, no behaviour change.
The two tests that reproduced #195 assert the behaviour the preceding commits deliver, so they no longer describe an open bug. They land with the fix rather than with the rig tooling, because ungating them without the fix present is a red CI: they fail correctly. The #175 reproducer stays gated; that one is still open.
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 24, 2026
Collaborator
Author
|
Split so the pop changes can go for external review on their own:
Same commits, cherry-picked rather than rewritten. #206 must merge first: the ungated reproducers fail correctly without the fix, so landing #207 first would be a red CI. |
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
1 participant
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.
Fixes #195, #197 and #198. Changes to pop itself — splits out of #200, which mixed rig and pop changes. The rig tooling is now separate in #201.
The defects
#195 — an RPZ zone-transfer source stopped affecting the served zone once pop had started. The transfer worked: the upstream was polled, the serial increase noticed, the zone pulled and parsed. Nothing downstream of the parse ran, so the data sat in pop's lists and never reached the served zone. Only the periodic refresh path was affected, which is why startup looked correct and everything after it did not.
#197 — the parse function wrote names straight into the source's
Namesmap, which only ever grew. A name withdrawn upstream stayed in pop's list for the lifetime of the process.#198 — a rebuild that changed the served zone left the SOA serial where it was, so a downstream holding that serial had no reason to transfer.
The fixes
#195 + #197 are one mechanism: a transfer now produces a delta.
rpzFeedIngeststages what one transfer carried;Commit()diffs it against what the source held, applies the difference, and returns the added and removed names.applyUpstreamDeltaturns that into a publish and is called from every path that transfers a source, including both startup paths.A delta rather than replacing the name set wholesale, because inbound IXFR will deliver adds and deletes off the wire and can go through the same
Commitpath without materialising the whole zone.#198's serial arithmetic turned out to be correct; its input was not.
ParseOutputsre-reads the serial cache intopd.Rpz.CurrentSerial, and it runs twice at startup — fromNewPopData, then again frommainafterParseSourcesNGhas already built and published the zone. The second run reset the counter behind the running zone's back:Changed content, republished under a serial downstreams already held. With a cache file present it still rewinds, discarding whatever the startup build advanced.
Both generate paths now derive the next serial from the published snapshot — what downstreams actually hold, and the one value config code re-running cannot rewind. The cache read moves into
loadCachedSerial, called once before anything is published; reading a cached serial was never part of parsing outputs, and keeping it there made an otherwise idempotent function destructive to call twice.A rebuild producing identical content no longer republishes at all, so the serial does not churn on every rebuild.
Verification
The two #195 reproducers are ungated in the last commit and pass; they land with the fix because ungating them without it is a red CI. Full suite green with
-race. Confirmed by hand against a live pop, checked withdigrather than through the rig's own reading of the zone:The rig earned its keep immediately: the first cut of this fix broke startup, because the parse function stages instead of writing and only the periodic path committed. Every name silently vanished. The rig caught it on the initial assertion of the first run.
Not covered
The shared
doubt_catchall/allow_catchallbuckets still only grow — they carry no record of which source contributed a name, so a per-source delta cannot be applied to them. Filed as #199, which is reachable by hand and worse than it sounds: a name that lands inallow_catchallcan never be blocked again.One commit is comment-only: it records why
decide()ignores the upstream's per-name action, which reads like a defect otherwise — I nearly filed it as one.