feat(dns): remove a reverse-DNS zone with its network segment

[chet]

A network's reverse-DNS zone is derived from its prefix and created with the segment, so PTR lookups resolve without anyone hand-authoring the zone. This completes that lifecycle at the other end: deleting a segment removes the reverse zone it created, so a torn-down network leaves no orphaned in-addr.arpa / ip6.arpa domain behind.

• Add db::dns::remove_reverse_zone, the inverse of ensure_reverse_zone -- it derives the same arpa zone from the prefix and soft-deletes the domain.
• Hook it into the network_segment::delete handler, mirroring the create-time hook in save.
• No refcount needed: prefixes are globally non-overlapping, so a zone has exactly one owner; re-deleting a segment finds no live zone and is a no-op.

Tests added!

This supports #2774

[chet]

@coderabbitai PTAL, thanks!

[coderabbitai]

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 685e4680-7c5c-4b97-82f3-838257368878

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

The PR introduces a remove_reverse_zone async function in the DNS database layer that derives a reverse zone name from an aligned IpNetwork prefix and deletes all matching live domain records. The network segment delete handler is updated to call this function for each prefix in the segment's prefix list within the existing transaction before committing.

Changes

Reverse-DNS zone cleanup on network segment deletion

Layer / File(s)	Summary
remove_reverse_zone implementation and test crates/api-db/src/dns/mod.rs	Adds the exported remove_reverse_zone function that derives the reverse zone via cidr_to_reverse_zone, queries live domain records by zone name, deletes each, and returns Ok(()). Non-aligned prefixes are a no-op. An SQLx integration test validates creation-then-deletion, idempotency, and the non-aligned no-op case.
Delete handler wired to remove_reverse_zone crates/api-core/src/handlers/network_segment.rs	Replaces the inline mark_as_deleted mapping expression with an explicit sequence: await mark_as_deleted, iterate segment.prefixes calling remove_reverse_zone for each, commit, and return NetworkSegmentDeletionResult.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• DNS: drop a reverse zone when its network segment is deleted #2774: This PR directly implements the reverse-DNS zone cleanup described in that issue by adding remove_reverse_zone and invoking it from the segment delete handler.
• DNS: derive reverse zones from network prefixes (config + runtime) #2766: The present work is the deletion counterpart to the reverse zone creation work tracked in issue #2766.

Possibly related PRs

• NVIDIA/infra-controller#2769: Adds ensure_reverse_zone in the same crates/api-db/src/dns/mod.rs file during segment save; this PR is the symmetric deletion counterpart operating on the same module and zone derivation logic.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the primary change: adding reverse-DNS zone removal functionality during network segment deletion.
Description check	✅ Passed	The description comprehensively explains the lifecycle completion for reverse-DNS zones, implementation approach, and design rationale including idempotency guarantees.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== PR Details ==="
gh pr view 2783 --repo NVIDIA/infra-controller --json title,body,files,additions,deletions

echo ""
echo "=== PR Diff ==="
gh pr diff 2783 --repo NVIDIA/infra-controller

Length of output: 5517

---

---

@chet — clean implementation overall. The transaction ordering is correct, the soft-delete mirrors the create-time hook faithfully, and the idempotency is a sound design decision. A few observations worth considering:

---

1. for loop vs. the stated single-owner invariant (minor)

remove_reverse_zone uses a for loop over find_by_name results while the PR description explicitly states "a zone has exactly one owner." The loop is defensive (and harmless), but it slightly undermines the invariant — a reader may wonder whether duplicates are actually possible. Consider either:

• Replacing the loop with a direct if let Some(domain) = find_by_name(...).await?.into_iter().next(), which makes the single-owner assumption explicit, or
• Adding a debug_assert!(domains.len() <= 1) / a warn! if the vec has more than one entry, to flag a broken invariant in development.

// Option A – explicit single-owner
if let Some(domain) = domain::find_by_name(&mut *txn, &zone).await?.into_iter().next() {
    tracing::info!(%zone, %prefix, "removing reverse-DNS zone for deleted network prefix");
    domain::delete(domain, &mut *txn).await?;
}

---

2. No IPv6 test case

The test exercises 10.0.0.0/16 (IPv4) and 10.1.0.0/25 (unaligned). There is no IPv6 path exercised. Given that ensure_reverse_zone supports ip6.arpa, a corresponding remove_reverse_zone test for, say, 2001:db8::/32 would close that gap without much effort.

---

3. Minor log field inconsistency

tracing::info!(zone = %zone, %prefix, "removing reverse-DNS zone …");

zone is given an explicit key (zone = %zone) while prefix is anonymous (%prefix). Both emit correctly, but for consistency either use the shorthand for both (%zone, %prefix) or the explicit form for both.

---

4. Handler simplification — positive note

Removing the intermediate response binding and writing:

Ok(Response::new(rpc::NetworkSegmentDeletionResult {}))

is a clear improvement in readability. No notes here.

---

Summary

The PR is functionally correct, well-documented, and the tests cover the critical paths (create/delete, idempotent delete, non-aligned no-op). The items above are non-blocking; item 1 is the most worth addressing for long-term clarity. Happy to approve once you've had a look at these.

[coderabbitai]

Actionable comments posted: 2

🤖 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/mod.rs`:
- Around line 148-160: The remove_reverse_zone function deletes domains by name
only without validating ownership, which can accidentally remove pre-existing
manual domains that share the same reverse-zone name. Modify the logic to
persist ownership metadata (associating the domain with the specific
prefix/segment) when creating reverse-DNS zones, then update the deletion loop
to filter by that ownership marker before calling domain::delete. This ensures
only domains actually owned by the deleted segment are removed, not any
pre-existing domains that happen to match the zone name.
- Around line 234-244: The test in the remove_reverse_zone idempotency block
(around lines 235-243) only verifies that the operations do not error by using
unwrap(), but does not assert the actual database state after each operation.
Add explicit post-condition assertions after the first remove_reverse_zone call
(the second removal) to verify that no reverse zone exists, and after the second
remove_reverse_zone call with the unaligned prefix to verify that the database
state remains unchanged and no zone was affected. This ensures that regressions
in no-op behavior are caught.

🪄 Autofix (Beta)

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: 21ff5378-2a4b-40eb-827d-e61520f3ffc2

📥 Commits

Reviewing files that changed from the base of the PR and between 86dd05c and bf2045b.

📒 Files selected for processing (2)

• crates/api-core/src/handlers/network_segment.rs
• crates/api-db/src/dns/mod.rs

[github-actions]

🔍 Container Scan Summary

Service	Total	Critical	High	Medium	Low	Other
boot-artifacts-aarch64	3	0	0	3	0	0
boot-artifacts-x86_64	3	0	0	3	0	0
forge-admin-cli-x86_64	264	6	23	99	6	130
machine-validation-runner	714	34	183	266	35	196
machine_validation	714	34	183	266	35	196
nvmetal-carbide	714	34	183	266	35	196
TOTAL	2412	108	572	903	111	718

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

[chet]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.