fix(api): make reverse DNS safe for duplicate addresses - #4747
Conversation
Summary by CodeRabbit
WalkthroughThe change canonicalizes reverse-DNS names, adds transactional batch lifecycle operations, enforces domain concurrency checks, reconciles startup prefixes, and tightens SOA and PTR query behavior. ChangesReverse DNS ownership and lifecycle
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Startup
participant NetworkSegment
participant DnsLifecycle
participant PostgreSQL
Startup->>NetworkSegment: persist segments without reverse zones
Startup->>PostgreSQL: find persisted prefixes
Startup->>DnsLifecycle: ensure_reverse_zones(prefixes)
DnsLifecycle->>PostgreSQL: lock and reconcile canonical zones
PostgreSQL-->>DnsLifecycle: committed reverse-zone state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
crates/api-db/src/dns/domain_metadata.rs (1)
109-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
check_cases_asyncwithOutcomefor the three inputs.
metadata_for_domainis asynchronous, soscenarios!cannot call it directly. BuildCaserows and awaitcheck_cases_asyncinstead of using a handwritten loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain_metadata.rs` around lines 109 - 117, Replace the handwritten loop around metadata_for_domain with three Case rows using the existing check_cases_async and Outcome test helpers, then await the helper to run the asynchronous inputs. Preserve the same domain variants and failure reporting while adopting the shared table-driven test pattern.Source: Coding guidelines
crates/api-core/src/tests/network_segment.rs (1)
725-725: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: use
eyre::Reportfor consistency with the neighbouring tests.The new helper and the three new tests return
Box<dyn std::error::Error>, whiletest_create_initial_networksin the same module returnseyre::Report. The style guide reserveseyrefor tests, soeyre::Reportis the idiomatic choice and keeps one error type across the module.Also applies to: 747-747, 804-804, 852-852
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/network_segment.rs` at line 725, Update the new helper and the three added tests to return eyre::Report instead of Box<dyn std::error::Error>, matching test_create_initial_networks and the module’s test error convention.Source: Coding guidelines
crates/api-db/src/dns/domain.rs (2)
246-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMap
RowNotFoundto a concurrency error for consistency.
claim_network_prefix_managementperforms a compare-and-swap on the canonical name. When the guard fails,fetch_onereturnssqlx::Error::RowNotFound, and the current mapping converts it into a generic query error. The sibling functionsdeleteandupdatetranslate the same condition intoDatabaseError::ConcurrentModificationError. Aligning the mapping makes the failure mode self-describing for the lifecycle callers incrates/api-db/src/dns/mod.rs.♻️ Proposed error mapping
.fetch_one(txn) .await .map(Domain::from) - .map_err(|error| DatabaseError::query(query, error)) + .map_err(|error| match error { + sqlx::Error::RowNotFound => { + DatabaseError::ConcurrentModificationError("domain", expected_name.to_string()) + } + error => DatabaseError::query(query, error), + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain.rs` around lines 246 - 252, Update the error mapping in the claim_network_prefix_management query to detect sqlx::Error::RowNotFound and return DatabaseError::ConcurrentModificationError, matching the delete and update implementations; continue mapping all other errors through DatabaseError::query.
299-320: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNote the limits of the
updatedguard inside a single transaction.Both
deleteandupdateuseupdated = $Nas the optimistic-concurrency check. PostgreSQL evaluatesNOW()as the transaction start timestamp, so two successive writes to the same row inside one transaction produce an identicalupdatedvalue. Within one transaction, a stale snapshot is therefore rejected only by the accompanying name guard, not by the timestamp.The API handlers use one transaction per request, so the production path is safe. The
previous_nameguard already covers the reverse-zone rename case that the new test exercises. Record this assumption in the doc comment so a future caller does not perform two guarded writes in one transaction and expect the timestamp to discriminate.Also applies to: 346-376
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain.rs` around lines 299 - 320, Update the doc comments for both delete and update to document that PostgreSQL’s transaction-scoped NOW() can produce identical updated timestamps for successive writes in one transaction, so the timestamp guard cannot distinguish them; state that callers must use one transaction per request and rely on the name/previous_name guard for same-transaction protection.crates/api-core/src/handlers/domain.rs (1)
144-145: 📐 Maintainability & Code Quality | 🔵 TrivialOutstanding requirement: forward-domain reference validation.
The comment records that a forward domain can still be deleted while a
NetworkSegmentreferences it throughsubdomain_id. The managed reverse-zone guard above does not cover that case, becauselock_reverse_zone_namesintentionally ignores forward names.I can generate the reference check and its test, or open a tracking issue. State your preference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/domain.rs` around lines 144 - 145, Implement the missing forward-domain deletion validation in the handler around the TODO: query for any NetworkSegment referencing the domain through subdomain_id and reject deletion when one exists. Preserve the existing managed reverse-zone validation, remove the TODO once covered, and add a test exercising deletion with a referencing NetworkSegment.crates/api-db/src/dns/domain/test_create_domain.rs (1)
200-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument which guard this test actually exercises.
Both
firstandstalecarry theupdatedvalue frompersist. All three statements run in one transaction, soNOW()returns the same transaction timestamp and theupdated = $5guard matches for both writers. The rejection therefore comes from thename = $4guard, not from the timestamp guard.The test proves the intended protection. Add a short comment stating that the persisted-name guard is the discriminator here, so a later change to the timestamp guard does not appear to be covered when it is not. Consider a second test that commits between the two updates to exercise the timestamp guard directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain/test_create_domain.rs` around lines 200 - 221, Add a concise comment in stale_domain_rename_cannot_mutate_a_new_reverse_zone_identity explaining that this single-transaction test rejects the stale update via the persisted-name guard, not the updated-timestamp guard. Do not alter the test behavior; a separate committed-between-updates test is optional.crates/api-db/src/dns/mod.rs (2)
137-199: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: avoid duplicating the zone-name strings.
ensure_reverse_zonesandremove_reverse_zoneseach clone every zone name into a secondVec<String>only to hand it tolock_canonical_reverse_zone_names.reverse_zones_for_prefixesalready returns the batch sorted and deduplicated by zone name, so the lock helper could accept an iterator of&strand skip both the allocation and its own re-sort.The correctness of the batch is sound: the derivation from an aligned prefix to a zone name is injective, so deduplicating by zone name never discards a distinct prefix that
remove_reverse_zone_lockedwould need for its live-prefix check.♻️ Proposed signature change
-async fn lock_canonical_reverse_zone_names( - txn: &mut PgTransaction<'_>, - names: &[String], -) -> DatabaseResult<()> { - let mut names = names.to_vec(); - names.sort_unstable(); - names.dedup(); +async fn lock_canonical_reverse_zone_names<'a>( + txn: &mut PgTransaction<'_>, + names: impl IntoIterator<Item = &'a str>, +) -> DatabaseResult<()> { + let mut names = names.into_iter().collect::<Vec<_>>(); + names.sort_unstable(); + names.dedup();Callers then pass
reverse_zones.iter().map(|(zone, _)| zone.as_str()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 137 - 199, Update lock_canonical_reverse_zone_names to accept an iterator of borrowed zone-name strings, preserving its existing locking behavior while avoiding internal sorting. In ensure_reverse_zones and remove_reverse_zones, remove the intermediate Vec<String> allocation and pass reverse_zones.iter().map(|(zone, _)| zone.as_str()) directly; retain reverse_zones_for_prefixes ordering and deduplication.
275-291: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse an explicitly aliased, ordered batch query for large lock sets.
A single query can reduce round trips. If adopted, alias the
unnestoutput explicitly, such asAS zones(zone_name), and order byzone_name.AS namealiases the table, not its output column, so the proposednamereferences are invalid. Keep the advisory-lock call in the ordered query to preserve canonical lock acquisition order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 275 - 291, The lock_canonical_reverse_zone_names function currently executes one advisory-lock query per name; replace this loop with a single ordered batch query using an explicitly aliased unnest output (for example, AS zones(zone_name)), ordering by zone_name, and retain the advisory-lock call in that query to preserve canonical acquisition order. Bind the deduplicated names collection to the batch query and keep existing error mapping and return behavior.crates/api-core/src/handlers/network_segment.rs (1)
273-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the segment prefix-network accessor. Four call sites now repeat the same expression to turn
NetworkSegment.prefixesinto aVec<IpNetwork>before callingensure_reverse_zonesorremove_reverse_zones. The shared root cause is thatNetworkSegmentexposes no accessor for its prefix networks, so every reverse-zone caller reimplements the projection. The style guide states that logic operating primarily on one type belongs as a method on that type.Add
NetworkSegment::prefix_networks(&self) -> impl Iterator<Item = IpNetwork>incrates/api-model, mirroring the existingprefixes()accessor incrates/api-model/src/vpc/capability.rslines 388-390, then replace each site:
crates/api-core/src/handlers/network_segment.rs#L273-L278: replace the inline map withsegment.prefix_networks().collect::<Vec<_>>()beforedb::dns::remove_reverse_zones.crates/api-core/src/handlers/network_segment.rs#L424-L428: replace the inline map withnetwork_segment.prefix_networks().collect::<Vec<_>>()beforedb::dns::ensure_reverse_zones.crates/api-core/src/db_init.rs#L205-L210: replace the inline map withreverse_zone_prefixes.extend(static_assignments.prefix_networks());.crates/api-core/src/test_support/network_segment.rs#L112-L116: replace the inline map withseg.prefix_networks().collect::<Vec<_>>().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/network_segment.rs` around lines 273 - 278, NetworkSegment prefix projection is duplicated across reverse-zone callers; add NetworkSegment::prefix_networks(&self) -> impl Iterator<Item = IpNetwork> in crates/api-model, mirroring the existing prefixes() accessor. Update crates/api-core/src/handlers/network_segment.rs lines 273-278 and 424-428, crates/api-core/src/db_init.rs lines 205-210, and crates/api-core/src/test_support/network_segment.rs lines 112-116 to use the new accessor with the specified collect or extend behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-db/src/dns/domain_metadata.rs`:
- Around line 57-64: Exclude soft-deleted domains from both DNS lookup queries
by adding the live-domain predicate `d.deleted IS NULL` to the forward metadata
query in `domain_metadata.rs` lines 57-64 and the machine PTR candidate query in
`resource_record.rs` lines 194-200; add coverage verifying deleted forward
domains are not returned.
---
Nitpick comments:
In `@crates/api-core/src/handlers/domain.rs`:
- Around line 144-145: Implement the missing forward-domain deletion validation
in the handler around the TODO: query for any NetworkSegment referencing the
domain through subdomain_id and reject deletion when one exists. Preserve the
existing managed reverse-zone validation, remove the TODO once covered, and add
a test exercising deletion with a referencing NetworkSegment.
In `@crates/api-core/src/handlers/network_segment.rs`:
- Around line 273-278: NetworkSegment prefix projection is duplicated across
reverse-zone callers; add NetworkSegment::prefix_networks(&self) -> impl
Iterator<Item = IpNetwork> in crates/api-model, mirroring the existing
prefixes() accessor. Update crates/api-core/src/handlers/network_segment.rs
lines 273-278 and 424-428, crates/api-core/src/db_init.rs lines 205-210, and
crates/api-core/src/test_support/network_segment.rs lines 112-116 to use the new
accessor with the specified collect or extend behavior.
In `@crates/api-core/src/tests/network_segment.rs`:
- Line 725: Update the new helper and the three added tests to return
eyre::Report instead of Box<dyn std::error::Error>, matching
test_create_initial_networks and the module’s test error convention.
In `@crates/api-db/src/dns/domain_metadata.rs`:
- Around line 109-117: Replace the handwritten loop around metadata_for_domain
with three Case rows using the existing check_cases_async and Outcome test
helpers, then await the helper to run the asynchronous inputs. Preserve the same
domain variants and failure reporting while adopting the shared table-driven
test pattern.
In `@crates/api-db/src/dns/domain.rs`:
- Around line 246-252: Update the error mapping in the
claim_network_prefix_management query to detect sqlx::Error::RowNotFound and
return DatabaseError::ConcurrentModificationError, matching the delete and
update implementations; continue mapping all other errors through
DatabaseError::query.
- Around line 299-320: Update the doc comments for both delete and update to
document that PostgreSQL’s transaction-scoped NOW() can produce identical
updated timestamps for successive writes in one transaction, so the timestamp
guard cannot distinguish them; state that callers must use one transaction per
request and rely on the name/previous_name guard for same-transaction
protection.
In `@crates/api-db/src/dns/domain/test_create_domain.rs`:
- Around line 200-221: Add a concise comment in
stale_domain_rename_cannot_mutate_a_new_reverse_zone_identity explaining that
this single-transaction test rejects the stale update via the persisted-name
guard, not the updated-timestamp guard. Do not alter the test behavior; a
separate committed-between-updates test is optional.
In `@crates/api-db/src/dns/mod.rs`:
- Around line 137-199: Update lock_canonical_reverse_zone_names to accept an
iterator of borrowed zone-name strings, preserving its existing locking behavior
while avoiding internal sorting. In ensure_reverse_zones and
remove_reverse_zones, remove the intermediate Vec<String> allocation and pass
reverse_zones.iter().map(|(zone, _)| zone.as_str()) directly; retain
reverse_zones_for_prefixes ordering and deduplication.
- Around line 275-291: The lock_canonical_reverse_zone_names function currently
executes one advisory-lock query per name; replace this loop with a single
ordered batch query using an explicitly aliased unnest output (for example, AS
zones(zone_name)), ordering by zone_name, and retain the advisory-lock call in
that query to preserve canonical acquisition order. Bind the deduplicated names
collection to the batch query and keep existing error mapping and return
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a6ad38ba-9306-42b3-8267-2be09096da8a
📒 Files selected for processing (16)
crates/api-core/src/db_init.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/domain.rscrates/api-core/src/handlers/network_segment.rscrates/api-core/src/test_support/network_segment.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/network_segment.rscrates/api-db/migrations/20260807232133_reverse_zone_ownership.sqlcrates/api-db/migrations/20260807232134_instance_addresses_address_index.sqlcrates/api-db/src/dns/domain.rscrates/api-db/src/dns/domain/test_create_domain.rscrates/api-db/src/dns/domain_metadata.rscrates/api-db/src/dns/mod.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/migrations/mod.rscrates/api-db/src/network_prefix.rs
There was a problem hiding this comment.
🧹 Nitpick comments (5)
crates/api-core/src/tests/dns.rs (1)
460-465: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the slice pattern used elsewhere in this PR.
.into_iter().next().unwrap()panics without context if the lookup returns nothing, and it does not assert that exactly one live zone exists. The lifecycle testan_existing_manual_reverse_zone_is_adoptedincrates/api-db/src/dns/mod.rsalready uses the slice-pattern form. Aligning the two improves the failure message and strengthens the assertion.♻️ Proposed assertion
- let domain = db::dns::domain::find_by_name(txn.as_mut(), "100.51.198.in-addr.arpa") - .await - .unwrap() - .into_iter() - .next() - .unwrap(); + let domains = db::dns::domain::find_by_name(txn.as_mut(), "100.51.198.in-addr.arpa") + .await + .unwrap(); + let [domain] = domains.as_slice() else { + panic!("the managed prefix should have exactly one reverse zone"); + }; + let domain = domain.clone();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/dns.rs` around lines 460 - 465, Update the domain lookup in the relevant DNS test to use the slice-pattern assertion established by an_existing_manual_reverse_zone_is_adopted, requiring exactly one result and providing contextual failure output instead of chaining into_iter().next().unwrap().crates/api-core/src/handlers/domain.rs (1)
136-145: 📐 Maintainability & Code Quality | 🔵 TrivialThe managed-zone guard is correct. The TODO remains an open gap.
The lock and the managed rejection are correctly ordered. The remaining TODO states that a forward domain can still be deleted while a
NetworkSegmentreferences it throughsubdomain_id.Do you want me to open an issue to track the forward-domain reference validation?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/domain.rs` around lines 136 - 145, Implement the missing forward-domain reference validation in the deletion flow around the existing lock and is_network_prefix_managed guard. Before allowing deletion, query whether any NetworkSegment references the domain through subdomain_id and reject deletion with the appropriate failed-precondition error when one exists; remove the obsolete TODO once covered.crates/api-db/src/dns/domain.rs (1)
235-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe managed-domain path loses the concurrency signal that the manual path preserves.
deleteandupdatemapsqlx::Error::RowNotFoundtoConcurrentModificationError, because their conditionalWHEREclauses can legitimately match zero rows under contention. The two managed-path functions use the same conditional-fetch_oneshape but report the miss as an opaqueDatabaseError::querywrapping "no rows returned". Callers then cannot distinguish contention from a query fault.
crates/api-db/src/dns/domain.rs#L235-L253: mapRowNotFoundtoConcurrentModificationError("domain", ...)inclaim_network_prefix_management.crates/api-db/src/dns/domain.rs#L295-L341: apply the same mapping indelete_network_prefix_managed; leavedeleteunchanged, as it already maps the error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain.rs` around lines 235 - 253, Map sqlx::Error::RowNotFound to ConcurrentModificationError("domain", ...) in claim_network_prefix_management and delete_network_prefix_managed, while preserving DatabaseError::query for other errors; the sibling delete function requires no change because it already performs this mapping. Apply the same handling at crates/api-db/src/dns/domain.rs lines 235-253 and 295-341.crates/api-db/src/dns/mod.rs (1)
212-217: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider collapsing the read-then-update into a single idempotent claim.
claim_network_prefix_managementsetsnetwork_prefix_managed = trueunconditionally and returns the row. The precedingis_network_prefix_managedcheck therefore costs one extra round trip per zone, per transaction, and adds a branch that carries no semantic weight. The only behavioural difference is thetracing::info!adoption log, which can be derived from the returned row instead.This is a taste-level simplification, not a defect. Retain the current form if the adoption log must stay strictly conditional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 212 - 217, In the existing-zone branch, replace the separate is_network_prefix_managed check and conditional claim with a single claim_network_prefix_management call. Use the returned row to determine whether adoption logging is needed, preserving the current tracing::info! message only when the zone was previously unmanaged.crates/api-core/src/handlers/network_segment.rs (1)
369-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a small options struct for the three boolean parameters.
save_innernow accepts three consecutive booleans:set_to_ready,allocate_svi_ip, andensure_reverse_zones. At the call sites this renders as(api, &mut txn, ns, true, false), which conveys nothing without consulting the signature, and a transposed pair would compile silently.The two public wrappers already shield callers from the third flag, so this is contained. If the parameter list grows again, promote the flags to a
SaveOptionsstruct with named fields and aDefaultimpl.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/network_segment.rs` around lines 369 - 375, Introduce a SaveOptions struct with named fields for set_to_ready, allocate_svi_ip, and ensure_reverse_zones, including a Default implementation, and update save_inner to accept it instead of three boolean parameters. Adjust the existing internal call sites and public wrappers to construct or modify SaveOptions explicitly while preserving their current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-core/src/handlers/domain.rs`:
- Around line 136-145: Implement the missing forward-domain reference validation
in the deletion flow around the existing lock and is_network_prefix_managed
guard. Before allowing deletion, query whether any NetworkSegment references the
domain through subdomain_id and reject deletion with the appropriate
failed-precondition error when one exists; remove the obsolete TODO once
covered.
In `@crates/api-core/src/handlers/network_segment.rs`:
- Around line 369-375: Introduce a SaveOptions struct with named fields for
set_to_ready, allocate_svi_ip, and ensure_reverse_zones, including a Default
implementation, and update save_inner to accept it instead of three boolean
parameters. Adjust the existing internal call sites and public wrappers to
construct or modify SaveOptions explicitly while preserving their current
behavior.
In `@crates/api-core/src/tests/dns.rs`:
- Around line 460-465: Update the domain lookup in the relevant DNS test to use
the slice-pattern assertion established by
an_existing_manual_reverse_zone_is_adopted, requiring exactly one result and
providing contextual failure output instead of chaining
into_iter().next().unwrap().
In `@crates/api-db/src/dns/domain.rs`:
- Around line 235-253: Map sqlx::Error::RowNotFound to
ConcurrentModificationError("domain", ...) in claim_network_prefix_management
and delete_network_prefix_managed, while preserving DatabaseError::query for
other errors; the sibling delete function requires no change because it already
performs this mapping. Apply the same handling at
crates/api-db/src/dns/domain.rs lines 235-253 and 295-341.
In `@crates/api-db/src/dns/mod.rs`:
- Around line 212-217: In the existing-zone branch, replace the separate
is_network_prefix_managed check and conditional claim with a single
claim_network_prefix_management call. Use the returned row to determine whether
adoption logging is needed, preserving the current tracing::info! message only
when the zone was previously unmanaged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9fffc135-bc0c-4546-be85-54021234a0ae
📒 Files selected for processing (16)
crates/api-core/src/db_init.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/domain.rscrates/api-core/src/handlers/network_segment.rscrates/api-core/src/test_support/network_segment.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/network_segment.rscrates/api-db/migrations/20260807232133_reverse_zone_ownership.sqlcrates/api-db/migrations/20260807232134_instance_addresses_address_index.sqlcrates/api-db/src/dns/domain.rscrates/api-db/src/dns/domain/test_create_domain.rscrates/api-db/src/dns/domain_metadata.rscrates/api-db/src/dns/mod.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/migrations/mod.rscrates/api-db/src/network_prefix.rs
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
crates/api-db/src/dns/resource_record.rs (1)
85-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe arpa suffix predicate is redundant in both canonical reverse-zone queries. Each branch runs only when
canonical_reverse_zone_namereturnsSome, which guarantees the bound parameter already ends with.in-addr.arpaor.ip6.arpa. The equality onlower(rtrim(name, '.')) = $1therefore cannot match a forward domain, and the extraLIKEpair adds two more expression evaluations per candidate row without changing the result set. Removing it keeps the guarantee in one place.
crates/api-db/src/dns/resource_record.rs#L85-L91: drop theAND (... LIKE '%.in-addr.arpa' OR ... LIKE '%.ip6.arpa')clause from theget_soa_recordreverse-zone query and keepdeleted IS NULL.crates/api-db/src/dns/domain_metadata.rs#L46-L54: drop the same clause from themetadata_for_domainreverse-zone query and keepd.deleted IS NULL.♻️ Proposed simplification for `get_soa_record`
"SELECT soa FROM domains WHERE lower(rtrim(name, '.')) = $1 - AND deleted IS NULL - AND ( - lower(rtrim(name, '.')) LIKE '%.in-addr.arpa' - OR lower(rtrim(name, '.')) LIKE '%.ip6.arpa' - )", + AND deleted IS NULL",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/resource_record.rs` around lines 85 - 91, Remove the redundant reverse-zone suffix predicate from the get_soa_record query in crates/api-db/src/dns/resource_record.rs lines 85-91, retaining the deleted IS NULL condition. Apply the same removal to the metadata_for_domain query in crates/api-db/src/dns/domain_metadata.rs lines 46-54, retaining d.deleted IS NULL; rely on canonical_reverse_zone_name for suffix validation.crates/api-core/src/tests/network_segment.rs (1)
720-725: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
eyre::ReportoverBox<dyn std::error::Error>in these tests.
test_create_initial_networksin this same file returnsResult<(), eyre::Report>. The new helper and the three new tests returnResult<(), Box<dyn std::error::Error>>. Align the new code with the crate convention so the error type stays uniform across the module.As per coding guidelines: "Use custom
thiserrorerrors in libraries, reserveeyrefor tests/mocks or top-level binaries".♻️ Proposed change to the helper signature
async fn persist_initial_network_without_reverse_zone( pool: &sqlx::PgPool, name: &str, definition: &NetworkDefinition, stored_definition: Option<&NetworkDefinition>, -) -> Result<NetworkSegment, Box<dyn std::error::Error>> { +) -> Result<NetworkSegment, eyre::Report> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/network_segment.rs` around lines 720 - 725, Update persist_initial_network_without_reverse_zone and the three new tests in this module to return eyre::Report-based Results, matching test_create_initial_networks and the crate’s test convention; preserve their existing success and error propagation behavior.Source: Coding guidelines
crates/api-core/src/handlers/domain.rs (2)
144-145: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTrack the remaining forward-domain deletion gap.
The TODO now scopes the residual risk precisely, which is an improvement. The gap itself remains: deleting a forward domain that a
NetworkSegment.subdomain_idstill references. The outcome depends on the foreign key action — aRESTRICTproduces a raw database error rather than a cleanFailedPrecondition, and aSET NULLsilently orphans the segment.Would you like me to open an issue to track the forward-domain reference check, or draft the guard following the same lock-then-verify shape used for managed reverse zones above?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/domain.rs` around lines 144 - 145, Implement the missing forward-domain deletion guard in the surrounding domain deletion handler, using the same lock-then-verify pattern as the managed reverse-zone validation above. Check for any NetworkSegment whose subdomain_id references the domain, and return a clean FailedPrecondition instead of allowing deletion or relying on database foreign-key behavior; remove the TODO once enforced.
38-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider storing the canonical spelling at creation time.
The handler validates the reverse-zone identity canonically, then persists
req.nameverbatim. A request for0.10.IN-ADDR.ARPA.therefore stores that exact string. The unique index prevents a duplicate, andfind_by_nameis spelling-insensitive, so nothing breaks. The row simply carries a non-canonical name untilclaim_network_prefix_managementlater rewrites it during adoption.Normalizing at the point of creation would make the persisted state canonical from the outset and remove one source of spelling variation from the table.
♻️ Suggested change
- let new_domain = NewDomain::new(req.name); + // Reverse zones have one canonical identity; store it rather than the + // caller's spelling. Forward domain names keep exact-name semantics. + let name = db::dns::canonical_reverse_zone_name(&req.name).unwrap_or(req.name); + let new_domain = NewDomain::new(name);Note this changes the
namereturned in the response for dotted or upper-case reverse-zone creates. Confirm no client depends on the echoed spelling before adopting it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/domain.rs` around lines 38 - 52, Normalize reverse-zone names before persistence in the handler around canonical_reverse_zone_name and NewDomain::new, so dotted or differently cased reverse-zone requests store the canonical spelling rather than req.name verbatim. Preserve the existing canonical duplicate validation and use the normalized value when constructing NewDomain; verify the response contract accepts the canonical name.crates/api-db/src/dns/mod.rs (5)
145-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the derive-and-lock preamble.
ensure_reverse_zonesandremove_reverse_zonesopen with the same six lines: derive the batch, project the zone names, lock them all. The invariant they express — never touch a reverse zone before the whole batch is locked in canonical order — is the safety property this module rests on. Stating it once makes it harder to add a third entry point that forgets the lock.♻️ Suggested extraction
/// Derives the reverse zones for `prefixes` and locks every one of them in /// canonical order. Callers must not read or write a reverse zone before this /// returns. async fn locked_reverse_zones_for_prefixes( prefixes: &[IpNetwork], txn: &mut PgTransaction<'_>, ) -> DatabaseResult<Vec<(String, IpNetwork)>> { let reverse_zones = reverse_zones_for_prefixes(prefixes); let zone_names = reverse_zones .iter() .map(|(zone, _)| zone.clone()) .collect::<Vec<_>>(); lock_canonical_reverse_zone_names(txn, &zone_names).await?; Ok(reverse_zones) }Both entry points then reduce to the helper call plus their own loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 145 - 150, Extract the shared derive-and-lock preamble from ensure_reverse_zones and remove_reverse_zones into a helper named locked_reverse_zones_for_prefixes, preserving the canonical batch-locking order and returning the derived reverse zones. Replace both entry points’ duplicated setup with this helper, leaving their existing per-zone loops unchanged.
185-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord why deduplicating by zone name is safe.
dedup_bycollapses entries sharing a zone name but retains only oneIpNetwork. Inremove_reverse_zonesthat retained prefix is passed toreverse_zone_has_other_live_prefix, which matches on exact prefix equality. If two different prefixes could ever derive one zone name, the retention check would consult the wrong prefix and could delete a zone another live segment still needs.The code is correct today because
cidr_to_reverse_zoneis injective over aligned prefixes: the zone name encodes both the network bits and the mask length through its label count, so10.0.0.0/16and10.0.0.0/24yield distinct names. That property is load-bearing but implicit. A brief comment protects it against a future change to the derivation.📝 Suggested comment
reverse_zones.sort_unstable_by(|left, right| left.0.cmp(&right.0)); + // `cidr_to_reverse_zone` is injective over aligned prefixes -- the label + // count encodes the mask length -- so entries sharing a zone name always + // share the same prefix. `remove_reverse_zone_locked` relies on this when + // it uses the retained prefix to look for other live segments. reverse_zones.dedup_by(|left, right| left.0 == right.0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 185 - 199, Add a brief comment immediately before the deduplication in reverse_zones_for_prefixes explaining that deduplicating by reverse-zone name is safe because cidr_to_reverse_zone is injective for aligned prefixes: the zone encodes both network bits and prefix length, so distinct prefixes cannot share a zone name. Keep the existing deduplication behavior unchanged.
483-485: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the placeholder segment identifier.
NetworkSegmentId::new()produces a fresh identifier that matches no row. The call therefore means "no segment is being excluded", andreverse_zone_has_other_live_prefixfinds nothing because this test seeds nonetwork_prefixesrows at all. That reasoning is not evident from the call site, and the same pattern appears at line 552.A named binding states the intent directly.
💚 Suggested clarification
- super::remove_reverse_zones(&[prefix], NetworkSegmentId::new(), &mut txn) + // No segment owns this zone in this test, so the exclusion id matches nothing. + let unrelated_segment_id = NetworkSegmentId::new(); + super::remove_reverse_zones(&[prefix], unrelated_segment_id, &mut txn) .await .unwrap();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 483 - 485, Clarify the intentional “no segment excluded” placeholder in the tests around remove_reverse_zones by binding NetworkSegmentId::new() to a descriptively named local variable before passing it to remove_reverse_zones. Apply the same clarification to the matching call around the second occurrence, while preserving the existing behavior.
206-217: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider carrying the ownership flag on the loaded row.
find_reverse_zone_by_canonical_nameissuesSELECT *, sonetwork_prefix_managedis already on the wire, butDbDomaindoes not map it andDomaindoes not expose it. Each[existing]branch therefore issues a second query throughis_network_prefix_managedfor a value the first query already fetched.remove_reverse_zone_lockeddoes the same at line 241.The cost is one extra round trip per zone per lifecycle call, which is immaterial for a single segment and modest at startup across the full prefix set. Adding the column to
DbDomainand surfacing it onDomainwould remove the second query and let these branches read the flag directly. That is a wider change than this PR needs, so treat it as a follow-up rather than a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 206 - 217, Follow up by carrying network_prefix_managed through DbDomain into Domain, then update the existing-zone branch and remove_reverse_zone_locked to read the loaded ownership flag directly instead of calling is_network_prefix_managed. Preserve the current adoption and removal behavior while eliminating the redundant query.
282-289: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBatch advisory-lock acquisition in one statement.
Use
unnest($1::text[]) AS names(name) ORDER BY name. This preserves canonical lock ordering and reduces one round trip per zone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 282 - 289, Update the advisory-lock acquisition loop around the reverse-zone lock query to execute one SQL statement using unnest($1::text[]) AS names(name) with ORDER BY name, binding the full names collection once. Preserve the existing hashtextextended lock expression and DatabaseError::query error mapping while removing the per-name execution loop.crates/api-db/src/dns/domain/test_create_domain.rs (1)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific failure, not merely that the call fails.
duplicate.is_err()also holds if a future change makespersistreject a trailing dot during name validation, or fail for any unrelated reason. The test would then continue to pass while no longer exercising the canonical uniqueness index it is named for.Matching the expected error variant keeps the test tied to its stated intent.
💚 Suggested tightening
let duplicate = db::dns::domain::persist(NewDomain::new("0.10.in-addr.arpa."), txn.as_mut()).await; - assert!( - duplicate.is_err(), - "dotted and non-dotted spellings are one live reverse-zone identity" - ); + let error = duplicate.expect_err( + "dotted and non-dotted spellings are one live reverse-zone identity", + ); + assert!( + error.to_string().contains("domains_live_reverse_zone_name_key"), + "expected the canonical reverse-zone uniqueness index to reject the duplicate, got: {error}", + );Adjust the matching to whatever
DatabaseErrorexposes for constraint names; amatches!on the SQLx database-error variant withconstraint()is cleaner if reachable from this crate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain/test_create_domain.rs` around lines 60 - 66, Update the duplicate assertion in the domain persistence test to match the specific DatabaseError/SQLx constraint-violation variant and verify the expected canonical uniqueness constraint name, rather than only checking duplicate.is_err(). Preserve the existing test setup and ensure unrelated validation or database failures do not satisfy the assertion.crates/api-db/src/migrations/mod.rs (2)
740-742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the interface identifier to this assertion message.
The loop above at lines 731-739 passes
"interface: {interface_id}"as a context message. This loop omits it. If the assertion fails, the output does not identify which of the two interfaces regressed.💚 Suggested fix
for interface_id in [duplicate_conflict_id, stable_loser_id] { - assert_eq!(interface_domains.get(interface_id), Some(&None)); + assert_eq!( + interface_domains.get(interface_id), + Some(&None), + "interface: {interface_id}", + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/migrations/mod.rs` around lines 740 - 742, Update the assert_eq! call in the loop over duplicate_conflict_id and stable_loser_id to include an assertion context message identifying the current interface_id, matching the context format used by the preceding loop.
694-703: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the negative ownership case.
The fixture proves that a live reverse zone backed by a live prefix becomes
network_prefix_managed = true. It does not prove the converse: that a live reverse zone with no live prefix is left unmanaged. That branch is governed by thelive_reverse_zonesCTE at migration lines 119-130, and a mistake there — for example dropping thens.deleted IS NULLfilter — would wrongly mark orphan zones as managed and make them undeletable through the Domain API.Seeding one additional reverse-zone domain whose prefix has no live segment, then asserting it stays unmanaged, would close that gap cheaply.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/migrations/mod.rs` around lines 694 - 703, The migration test should also cover the negative ownership case by seeding a live reverse-zone domain whose corresponding prefix has no live segment, then asserting its managed flag remains false. Extend the fixture and assertions near the existing duplicate-domain checks, using the same domain lookup symbols, while preserving the current positive managed-case assertions.crates/api-core/src/tests/dns.rs (1)
484-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the zone survives both rejected attempts.
The test proves that both requests return
FailedPreconditionwith the expected messages. It does not prove that the domain is unchanged afterwards. A handler that rejected the request only after performing the mutation would still satisfy every current assertion.The persistence layer guards these writes independently, so the risk is small. A short closing check makes the test's claim complete.
💚 Suggested addition
assert_eq!( delete_error.message(), "network-prefix-managed reverse zones cannot be deleted directly" ); + + let mut txn = env.pool.begin().await.unwrap(); + let survivors = + db::dns::domain::find_reverse_zone_by_canonical_name(txn.as_mut(), "100.51.198.in-addr.arpa") + .await + .unwrap(); + let [unchanged] = survivors.as_slice() else { + panic!("the rejected mutations must leave exactly one live reverse zone"); + }; + assert_eq!(unchanged.id, domain.id); + assert_eq!(unchanged.name, domain.name); + txn.commit().await.unwrap(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/dns.rs` around lines 484 - 498, Extend the test around the rejected delete requests in the DNS test to fetch the domain afterward and assert that the network-prefix-managed reverse zone still exists unchanged. Keep the existing FailedPrecondition and message assertions, and use the established domain lookup API to verify persistence was not mutated.crates/api-db/src/dns/domain.rs (2)
219-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the reverse-zone suffix predicate into one shared constant.
This SQL predicate now appears verbatim in four places: here,
crates/api-db/src/dns/resource_record.rs(get_soa_record),crates/api-db/src/dns/domain_metadata.rs(metadata_for_domain), and the migrationcrates/api-db/migrations/20260807232133_reverse_zone_ownership.sql(both the CHECK constraint and the unique index). The Rust predicate incanonical_reverse_zone_nameis a fifth expression of the same rule.The definition of "is a reverse zone" is a single domain invariant. If a further suffix is ever supported, all five sites must change together, and a missed site produces a silent lookup gap rather than a compile error. Extract at least the Rust query sites into one shared
const.♻️ Suggested consolidation
// crates/api-db/src/dns/mod.rs /// SQL predicate identifying a reverse-DNS zone by name. Keep in sync with /// `canonical_reverse_zone_name` and the `domains_live_reverse_zone_name_key` /// index predicate. pub(crate) const IS_REVERSE_ZONE_SQL: &str = "( lower(rtrim(name, '.')) LIKE '%.in-addr.arpa' OR lower(rtrim(name, '.')) LIKE '%.ip6.arpa' )";Call sites then build the query with
format!, or, where the table is aliased, keep a second aliased constant. The migration necessarily keeps its own copy; a comment referencing the constant is sufficient there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain.rs` around lines 219 - 225, Extract the repeated reverse-zone SQL predicate into a shared `pub(crate) const` in `dns/mod.rs`, then update the query construction in `domain.rs`, `resource_record.rs::get_soa_record`, and `domain_metadata.rs::metadata_for_domain` to reuse it, accounting for any table aliases. Document that the constant must remain synchronized with `canonical_reverse_zone_name` and the migration’s CHECK/index predicates; leave the migration’s SQL copy in place with a reference comment.
354-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
previous_nameprecondition.
previous_namemust match the persistedDomain.nameexactly. The update guard does not canonicalize the value, so a canonicalized spelling of a dotted persisted name returnsConcurrentModificationError. Add this precondition to the function documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain.rs` around lines 354 - 362, Update the documentation for the Domain::update function to state that previous_name must exactly match the persisted Domain.name, without canonicalization; document that canonicalized alternatives for dotted names fail the concurrency guard with ConcurrentModificationError.crates/api-db/migrations/20260807232133_reverse_zone_ownership.sql (1)
88-98: 🗄️ Data Integrity & Integration | 🔵 TrivialConsider recording the discarded interface associations.
This statement sets
domain_id = NULLfor every losing interface in a hostname collision. The choice is correct and unavoidable underfqdn_must_be_unique, and the migration test covers it. However, the operation silently removes forward DNS for those interfaces, and after the migration commits there is no way to determine which associations were dropped.Consider capturing the affected interface IDs before the update, for example by inserting them into a small audit table or emitting them with
RAISE NOTICEfrom aDOblock. Operators can then reconcile hostnames after the upgrade.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/migrations/20260807232133_reverse_zone_ownership.sql` around lines 88 - 98, Record the interface associations that will be discarded by the reconciliation update before setting losing interfaces’ domain_id to NULL. Add an appropriate audit-table insert or RAISE NOTICE mechanism keyed by the affected interface IDs and hostname/domain details, while preserving the existing ownership_rank=1 update behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-db/src/dns/resource_record.rs`:
- Around line 170-186: Update the machine-owner query within the address_owners
CTE to join machine_interface_addresses through machine_interfaces and require
machine_interfaces.machine_id IS NOT NULL. Keep the existing address matching
and UNION structure unchanged, while leaving instance ownership counting
unaffected.
---
Nitpick comments:
In `@crates/api-core/src/handlers/domain.rs`:
- Around line 144-145: Implement the missing forward-domain deletion guard in
the surrounding domain deletion handler, using the same lock-then-verify pattern
as the managed reverse-zone validation above. Check for any NetworkSegment whose
subdomain_id references the domain, and return a clean FailedPrecondition
instead of allowing deletion or relying on database foreign-key behavior; remove
the TODO once enforced.
- Around line 38-52: Normalize reverse-zone names before persistence in the
handler around canonical_reverse_zone_name and NewDomain::new, so dotted or
differently cased reverse-zone requests store the canonical spelling rather than
req.name verbatim. Preserve the existing canonical duplicate validation and use
the normalized value when constructing NewDomain; verify the response contract
accepts the canonical name.
In `@crates/api-core/src/tests/dns.rs`:
- Around line 484-498: Extend the test around the rejected delete requests in
the DNS test to fetch the domain afterward and assert that the
network-prefix-managed reverse zone still exists unchanged. Keep the existing
FailedPrecondition and message assertions, and use the established domain lookup
API to verify persistence was not mutated.
In `@crates/api-core/src/tests/network_segment.rs`:
- Around line 720-725: Update persist_initial_network_without_reverse_zone and
the three new tests in this module to return eyre::Report-based Results,
matching test_create_initial_networks and the crate’s test convention; preserve
their existing success and error propagation behavior.
In `@crates/api-db/migrations/20260807232133_reverse_zone_ownership.sql`:
- Around line 88-98: Record the interface associations that will be discarded by
the reconciliation update before setting losing interfaces’ domain_id to NULL.
Add an appropriate audit-table insert or RAISE NOTICE mechanism keyed by the
affected interface IDs and hostname/domain details, while preserving the
existing ownership_rank=1 update behavior.
In `@crates/api-db/src/dns/domain.rs`:
- Around line 219-225: Extract the repeated reverse-zone SQL predicate into a
shared `pub(crate) const` in `dns/mod.rs`, then update the query construction in
`domain.rs`, `resource_record.rs::get_soa_record`, and
`domain_metadata.rs::metadata_for_domain` to reuse it, accounting for any table
aliases. Document that the constant must remain synchronized with
`canonical_reverse_zone_name` and the migration’s CHECK/index predicates; leave
the migration’s SQL copy in place with a reference comment.
- Around line 354-362: Update the documentation for the Domain::update function
to state that previous_name must exactly match the persisted Domain.name,
without canonicalization; document that canonicalized alternatives for dotted
names fail the concurrency guard with ConcurrentModificationError.
In `@crates/api-db/src/dns/domain/test_create_domain.rs`:
- Around line 60-66: Update the duplicate assertion in the domain persistence
test to match the specific DatabaseError/SQLx constraint-violation variant and
verify the expected canonical uniqueness constraint name, rather than only
checking duplicate.is_err(). Preserve the existing test setup and ensure
unrelated validation or database failures do not satisfy the assertion.
In `@crates/api-db/src/dns/mod.rs`:
- Around line 145-150: Extract the shared derive-and-lock preamble from
ensure_reverse_zones and remove_reverse_zones into a helper named
locked_reverse_zones_for_prefixes, preserving the canonical batch-locking order
and returning the derived reverse zones. Replace both entry points’ duplicated
setup with this helper, leaving their existing per-zone loops unchanged.
- Around line 185-199: Add a brief comment immediately before the deduplication
in reverse_zones_for_prefixes explaining that deduplicating by reverse-zone name
is safe because cidr_to_reverse_zone is injective for aligned prefixes: the zone
encodes both network bits and prefix length, so distinct prefixes cannot share a
zone name. Keep the existing deduplication behavior unchanged.
- Around line 483-485: Clarify the intentional “no segment excluded” placeholder
in the tests around remove_reverse_zones by binding NetworkSegmentId::new() to a
descriptively named local variable before passing it to remove_reverse_zones.
Apply the same clarification to the matching call around the second occurrence,
while preserving the existing behavior.
- Around line 206-217: Follow up by carrying network_prefix_managed through
DbDomain into Domain, then update the existing-zone branch and
remove_reverse_zone_locked to read the loaded ownership flag directly instead of
calling is_network_prefix_managed. Preserve the current adoption and removal
behavior while eliminating the redundant query.
- Around line 282-289: Update the advisory-lock acquisition loop around the
reverse-zone lock query to execute one SQL statement using unnest($1::text[]) AS
names(name) with ORDER BY name, binding the full names collection once. Preserve
the existing hashtextextended lock expression and DatabaseError::query error
mapping while removing the per-name execution loop.
In `@crates/api-db/src/dns/resource_record.rs`:
- Around line 85-91: Remove the redundant reverse-zone suffix predicate from the
get_soa_record query in crates/api-db/src/dns/resource_record.rs lines 85-91,
retaining the deleted IS NULL condition. Apply the same removal to the
metadata_for_domain query in crates/api-db/src/dns/domain_metadata.rs lines
46-54, retaining d.deleted IS NULL; rely on canonical_reverse_zone_name for
suffix validation.
In `@crates/api-db/src/migrations/mod.rs`:
- Around line 740-742: Update the assert_eq! call in the loop over
duplicate_conflict_id and stable_loser_id to include an assertion context
message identifying the current interface_id, matching the context format used
by the preceding loop.
- Around line 694-703: The migration test should also cover the negative
ownership case by seeding a live reverse-zone domain whose corresponding prefix
has no live segment, then asserting its managed flag remains false. Extend the
fixture and assertions near the existing duplicate-domain checks, using the same
domain lookup symbols, while preserving the current positive managed-case
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8c1bcf62-c5e3-47ef-a359-1a6990f2bd39
📒 Files selected for processing (16)
crates/api-core/src/db_init.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/domain.rscrates/api-core/src/handlers/network_segment.rscrates/api-core/src/test_support/network_segment.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/network_segment.rscrates/api-db/migrations/20260807232133_reverse_zone_ownership.sqlcrates/api-db/migrations/20260807232134_instance_addresses_address_index.sqlcrates/api-db/src/dns/domain.rscrates/api-db/src/dns/domain/test_create_domain.rscrates/api-db/src/dns/domain_metadata.rscrates/api-db/src/dns/mod.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/migrations/mod.rscrates/api-db/src/network_prefix.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/api-core/src/db_init.rs (1)
198-213: 🗄️ Data Integrity & Integration | 🔵 TrivialReverse-zone reconciliation is scoped to currently configured definition names.
network_definition_namescomes fromnetworks.keys(). A segment created by an earlier startup whose definition was later removed from configuration is therefore excluded fromfind_persisted_for_network_definitions. Its zone already exists, so this is a no-op today, and the deliberate scoping correctly prevents startup from acting on drifted CIDRs. Record this expectation so a future change does not turn the omission into a missing zone. If wider reconciliation is ever needed, drive it from persistednetwork_defrows rather than from the configuration map.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/db_init.rs` around lines 198 - 213, Document near the network_definition_names and find_persisted_for_network_definitions flow that reverse-zone reconciliation intentionally covers only currently configured network definition names; removed definitions are excluded to avoid acting on drifted CIDRs, and any future broader reconciliation must use persisted network_def rows.crates/api-db/src/dns/domain/test_create_domain.rs (1)
176-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe clone of
test_nameappears unused.
test_nameis not read after this line inupdate_domain. If the clone was added to assert that the original name changed, add that assertion. Otherwise remove the clone.♻️ Proposed change
- let domain = db::dns::domain::persist(NewDomain::new(test_name.clone()), &mut txn).await; + let domain = db::dns::domain::persist(NewDomain::new(test_name), &mut txn).await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/domain/test_create_domain.rs` at line 176, Update the domain persistence call in update_domain to pass test_name without cloning, since the original value is not used afterward. If the clone is intended to verify name mutation, add an assertion that reads the original value; otherwise remove the unnecessary clone.crates/api-db/src/dns/mod.rs (1)
137-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the intermediate cloned zone-name vector.
Both
ensure_reverse_zonesandremove_reverse_zonesclone every zone name only to pass the names to the lock helper.lock_normalized_reverse_zone_namescopies and sorts its input again. Accept&strin the lock helper and borrow fromreverse_zonesinstead.♻️ Proposed refactor
pub async fn ensure_reverse_zones( prefixes: &[IpNetwork], txn: &mut PgTransaction<'_>, ) -> DatabaseResult<()> { let reverse_zones = reverse_zones_for_prefixes(prefixes); - let zone_names = reverse_zones - .iter() - .map(|(zone, _)| zone.clone()) - .collect::<Vec<_>>(); - lock_normalized_reverse_zone_names(txn, &zone_names).await?; + lock_normalized_reverse_zone_names(txn, &reverse_zones).await?;Change the helper signature to take the already sorted and deduplicated zone list, for example
names: &[(String, IpNetwork)], or a dedicated&[&str]slice built without allocation of newStringvalues. Apply the same change inremove_reverse_zones.Note:
reverse_zones_for_prefixesalready sorts and deduplicates, so the second sort inside the helper is redundant for these two callers. Keep the sort only for the publiclock_reverse_zone_namesentry point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/dns/mod.rs` around lines 137 - 198, Remove the intermediate zone_names allocations from ensure_reverse_zones and remove_reverse_zones. Update the internal lock_normalized_reverse_zone_names helper to accept the already sorted and deduplicated reverse_zones entries or borrowed zone-name references, and avoid cloning or resorting them; retain sorting for the public lock_reverse_zone_names entry point.Source: Coding guidelines
crates/api-core/src/handlers/domain.rs (1)
84-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant clone of
previous_name.
previous_nameis only used to buildreverse_zone_names. Move the value into the array instead of cloning it a second time.♻️ Proposed refactor
- let previous_name = domain.name.clone(); + let previous_name = std::mem::replace(&mut domain.name, domain_proto.name); - domain.name = domain_proto.name; - - let reverse_zone_names = [previous_name.clone(), domain.name.clone()]; + let reverse_zone_names = [previous_name, domain.name.clone()];Note on ordering:
find_by_uuidruns before the lock, sodomain.updatedcan already be stale when the lock is granted.domain::updatethen returnsConcurrentModificationError, which is the correct fail-closed outcome. No change is required, but a comment stating this would help the next reader.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/domain.rs` around lines 84 - 100, Remove the redundant clone in the update flow around previous_name: move previous_name directly into reverse_zone_names and retain only the necessary clone of domain.name. Do not alter the locking, reverse-zone lookup, or concurrent-modification behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/handlers/domain.rs`:
- Around line 38-49: Enforce the NetworkPrefix ownership check for reverse zones
in both the update and delete handlers, reusing the existing shared validation
if available. Ensure writes to NetworkPrefix-managed zones are rejected while
preserving the current reverse-zone locking and lookup behavior.
---
Nitpick comments:
In `@crates/api-core/src/db_init.rs`:
- Around line 198-213: Document near the network_definition_names and
find_persisted_for_network_definitions flow that reverse-zone reconciliation
intentionally covers only currently configured network definition names; removed
definitions are excluded to avoid acting on drifted CIDRs, and any future
broader reconciliation must use persisted network_def rows.
In `@crates/api-core/src/handlers/domain.rs`:
- Around line 84-100: Remove the redundant clone in the update flow around
previous_name: move previous_name directly into reverse_zone_names and retain
only the necessary clone of domain.name. Do not alter the locking, reverse-zone
lookup, or concurrent-modification behavior.
In `@crates/api-db/src/dns/domain/test_create_domain.rs`:
- Line 176: Update the domain persistence call in update_domain to pass
test_name without cloning, since the original value is not used afterward. If
the clone is intended to verify name mutation, add an assertion that reads the
original value; otherwise remove the unnecessary clone.
In `@crates/api-db/src/dns/mod.rs`:
- Around line 137-198: Remove the intermediate zone_names allocations from
ensure_reverse_zones and remove_reverse_zones. Update the internal
lock_normalized_reverse_zone_names helper to accept the already sorted and
deduplicated reverse_zones entries or borrowed zone-name references, and avoid
cloning or resorting them; retain sorting for the public lock_reverse_zone_names
entry point.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 52c63b9a-1961-435c-8a21-161f1e39e25f
📒 Files selected for processing (10)
crates/api-core/src/db_init.rscrates/api-core/src/handlers/domain.rscrates/api-core/src/handlers/network_segment.rscrates/api-core/src/tests/network_segment.rscrates/api-db/migrations/20260807232133_duplicate_address_reverse_dns.sqlcrates/api-db/src/dns/domain.rscrates/api-db/src/dns/domain/test_create_domain.rscrates/api-db/src/dns/mod.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/network_prefix.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/api-core/src/handlers/network_segment.rs
- crates/api-core/src/tests/network_segment.rs
- crates/api-db/src/dns/resource_record.rs
Global prefix uniqueness meant a reverse zone corresponded to one `NetworkPrefix` and an address-only PTR lookup had one possible answer. Tenant-managed `SitePrefix` resources will let isolated VPCs reuse address space, so those assumptions need to go away before overlap is enabled. This serializes reverse-zone creation and removal by normalized zone name, shares one live zone across equal prefixes, and removes it only after the last live prefix is gone. PTR lookup now returns no record whenever an address has more than one raw owner instead of picking one tenant's name. The migration only adds normalized reverse-zone uniqueness and an overlay-address lookup index. Existing duplicate reverse-zone names fail the migration without rewriting DNS data, and startup loads persisted network prefixes so config drift cannot create a zone for an unapplied CIDR. Tests added! This supports NVIDIA#3889 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
2 participants
Global prefix uniqueness meant reverse DNS could assume one
NetworkPrefixowned each reverse zone and one address owner could answer each PTR lookup. Tenant-managed SitePrefixes will eventually allow isolated VPCs to reuse address space, so those assumptions must be removed before overlap is enabled.This change normalizes reverse-zone names and uses a transaction-scoped PostgreSQL advisory lock for each affected name. Equal prefixes reuse one live
Domainrow, removal keeps that row until the last live equal prefix is gone, and creation rechecks prefix liveness under the lock so a stale startup snapshot cannot recreate an orphan zone.PTR lookup now counts the raw owners of an address before returning the existing forward name. If more than one owner exists, it returns no record and preserves the current NXDOMAIN behavior instead of selecting one tenant's name. Startup derives zones from persisted
NetworkPrefixrows, so config drift cannot create a zone for a CIDR Core never applied.The database migration is one 16-line file with two indexes: normalized uniqueness for live reverse-zone names and a non-unique overlay-address lookup index. It adds no ownership column and rewrites no DNS data. Existing normalized duplicate live reverse-zone names stop the migration for operator reconciliation.
This is safety plumbing only. It does not enable duplicate prefix allocation, add VPC-scoped DNS views, or change direct
DomainCRUD authority.Related issues
This supports #3889
Type of Change
Breaking Changes
Testing
Tests cover unique and ambiguous PTRs, shared-zone create/delete races, lock ordering, startup config drift, stale snapshots, rollback, and unique-address compatibility.
Additional Notes
These are per-name advisory locks, not row or table locks. The existing global prefix-overlap constraints remain in place until the separate overlap work lands.