fix(dns plugin): write to the record set that holds the name - #115
Open
scotwells wants to merge 3 commits into
Open
fix(dns plugin): write to the record set that holds the name#115scotwells wants to merge 3 commits into
scotwells wants to merge 3 commits into
Conversation
A (zone, type) pair is not one object. The CLI writes to its own DNSRecordSet, the operator creates `<zone>-soa` and `<zone>-ns`, and a zone served by AI Edge gets one per Gateway — so several sets of the same type routinely coexist, each carrying entries for different names. The owner name is what the backend collides on, which is why a zone with the same name in two sets reports Conflict or Not owner. findSet resolved the name across every set of the type and returned the one holding it, which was right, but fell back to the first set by object name when no set held it. That set can belong to a controller, and then the write guard refuses: in a zone whose first A set is Gateway-owned, no A record could be created at any name, under the message "the A records for blog.example.com are managed by AI Edge and are read-only" — naming a record the controller has nothing to do with. A controller owns the names inside its set, not the type. The fallback now prefers a set the user may write to, and returns nil when every set of the type is machine-owned, so a new one is created instead. createSet gained the matching case: the conventional `<zone>-<type>` name is already taken in exactly that situation, so an AlreadyExists falls back to GenerateName rather than surfacing as "changed while this command was running". Letting the server pick the suffix keeps two concurrent creates from choosing the same name. Nothing about the read side changes. `-o wide` already named the set each record came from, which is what makes a split name diagnosable at all — every other column renders the two rows identically — but nothing tested it deliberately and the docs did not mention it. Both now do. Tests cover the four ways a write can pick wrong: a managed set blocking an unrelated name, a set that merely sorts first winning over the one holding the name, a new name fragmenting into its own object instead of joining the bucket, and a write landing in another type. Reverting either half of the fix fails the first of them, with a different error each time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
The red E2E is not from this PR. The three They arrived with #65 and are meant to run under Everything else on this PR is green, including the six chainsaw suites that were there before. This change only touches |
`record apply` treated a record type as a single API object: it took the first DNSRecordSet of each type by object name and wrote every record of that type into it. A type is not one object. Nothing forbids several sets of one type in a zone, and a live zone routinely has them — the CLI writes to its own, and a Gateway gets one per listener. The result was the duplication the resolution rules exist to prevent. Applying a file containing `www A` to a zone where `www` lived in the second set by name wrote it into the first instead, leaving the real holder stale and giving the zone two entries for one key. That is the shape the backend reports back as Conflict and Not owner, and it is visible today in staging zones. The plan is now keyed by (type, set) rather than by type. A type's desired records are partitioned across the sets that already hold their owner names, and the names no set holds go to one unit together — the first set the command may write to, or a new object when a controller owns them all — so a zone gains one set per type rather than one per record. Each unit diffs, resolves and prunes against its own set, which is what makes --prune correct here: the file is the whole truth for each object separately. holderOf prefers a set the user may write to when several hold a name, which is the opposite of findSet's preference and deliberately so. findSet picks for a single-record write, where resolving to the controller's copy makes the guard refuse — the safe answer when the alternative is writing where the controller reverts. apply reconciles a whole file, so refusing to update the copy the user owns would block legitimate work; the controller's copy is protected by resolve and reported as skipped instead. applyEdit's retry had the matching hole. It re-resolved the target by name, so a second attempt in a zone with several sets of a type could write a plan computed against one set into another — the same duplication through the back door. A retry now re-reads the object the first attempt held, and starts resolution over only when there was none or it has since been deleted. The forty existing apply tests pass unchanged, which is the evidence that the partition preserves the single-set behaviour they cover. Two new ones fix the multi-set case in both directions, merge and --prune, and both fail if the partition is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`zone import` carried the same one-object-per-type assumption `record apply` did, in the same words: bulkSetsByType indexed a zone's record sets by type and kept only the first by object name. Two defects followed, both reproduced against the fake API before this change. It duplicated. Importing a file containing `www A` into a zone where `www` lived in the second set by name wrote it into the first, leaving the real holder stale and giving the zone two entries for one key. Import is worse than apply here because bulkWriteSet does a full Update of the type rather than a merge, so the first set was overwritten with the whole type's records while the other sets kept their copies. It refused too much. When the first set of a type belonged to a Gateway, the guard failed every record of that type — `blog A` was rejected with "the A records for this zone are managed by AI Edge (Gateway edge-gw)" over a name the Gateway does not hold. A zone served by AI Edge could not be imported into at all for that type. A controller owns the names inside its set, not the type. bulkSetsByType now keeps every set of a type, and convergeImport plans one write per (type, set): partitionByHolder splits a type's input across the sets that already hold their owner names, with the names no set holds going to one group together — the first set the import may write to, or a new object when a controller owns them all. The Gateway guard is unchanged and now sees a group containing only the names that controller genuinely holds, which is what narrows the refusal to those records. bulkWriteSet gained the create-collision fallback the record package has, since the conventional object name is taken in exactly that situation. A group with no input records is skipped rather than emptied, so --replace still means "replace the records I am giving you" and not "dismantle the zone" — the intent planType already documents for the platform's own entries. Two existing guard tests imported a name the Gateway did not hold and asserted the whole type was refused, which is the behaviour being corrected; they now name the record the Gateway actually holds, so they still pin the guard and the three-label rule they were written for. Reverting the partition fails those two and the new placement test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
A zone's records of one type do not live in one object. The CLI writes to its own record set, and AI Edge gets one per Gateway — so several sets of a type coexist, each holding different names. The name is what the backend collides on, which is why a name landing in two of them shows up as
ConflictorNot owner.All three write paths got that wrong, in different ways.
Creating a record resolved to whichever set sorted first rather than one you can write to. If that was a Gateway's, no record of that type could be created at any name:
blog.example.comisn't managed by anything.Applying a zone file wrote rather than refused.
record applytreated a type as one object, so a file containingwww Aapplied to a zone wherewwwlived in another set wrote a second copy and left the real one stale. That's the duplication theConflictstatus reports, and it's visible in staging zones today.Importing a zone file did both, and worse: it does a full replace of the type rather than a merge, and it refused every record of a type whose first set belonged to a Gateway — so a zone served by AI Edge couldn't be imported into at all.
All three now resolve the same way: a record goes to the set already holding its name, failing that to a set you own, and if a controller owns them all, to a new one. Records another system manages stay read-only, and a Gateway now blocks only the names it actually holds.
-o widealready named the record set each record came from — the only column that distinguishes two rows for the same name, and what aConflictstatus is pointing you at. That's now tested and documented rather than incidental.Every existing test passes unchanged except two import guard tests that asserted the over-broad refusal; they now name the record the Gateway really holds, so they still pin the guard they were written for.
🤖 Generated with Claude Code