feat(dns): derive reverse-DNS zones from network prefixes#2769
Conversation
|
@coderabbitai PTAL, thanks! |
Summary by CodeRabbit
WalkthroughThe PR introduces ChangesReverse-DNS Zone Derivation and Persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Files ==="
gh pr view 2769 --repo NVIDIA/infra-controller --json files --jq '.files[].path'
echo ""
echo "=== PR Diff ==="
gh pr diff 2769 --repo NVIDIA/infra-controllerLength of output: 6716 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== domain module in api-db ==="
fd -t f 'mod.rs' --search-path crates/api-db/src/dns/domain/ 2>/dev/null | head -5
fd -t f '*.rs' --search-path crates/api-db/src/dns/ 2>/dev/null
echo ""
echo "=== domain persist and find_by_name ==="
rg -n "pub async fn persist|pub async fn find_by_name|ON CONFLICT|on_conflict|INSERT INTO" crates/api-db/src/dns/ -A 8
echo ""
echo "=== NewDomain definition ==="
rg -n "struct NewDomain|impl NewDomain|fn new" crates/ --type rust -B 2 -A 10 | grep -A 10 "NewDomain"
echo ""
echo "=== network_segment save context around the hook ==="
sed -n '340,420p' crates/api-core/src/handlers/network_segment.rs
echo ""
echo "=== DatabaseResult type alias ==="
rg -n "pub type DatabaseResult|type DatabaseResult" crates/api-db/src/ -A 2
echo ""
echo "=== domain mod.rs full content ==="
cat crates/api-db/src/dns/domain/mod.rs 2>/dev/null || rg -rn "." crates/api-db/src/dns/domain/ 2>/dev/null | head -150Length of output: 1246 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Find dns-related rust files ==="
fd -t f '*.rs' | xargs grep -l "fn persist\|fn find_by_name\|NewDomain" 2>/dev/null | head -20
echo ""
echo "=== domain persist implementation ==="
rg -n "async fn persist" --type rust -A 15
echo ""
echo "=== find_by_name implementation ==="
rg -n "async fn find_by_name" --type rust -A 15
echo ""
echo "=== ON CONFLICT in dns-related SQL ==="
rg -n "ON CONFLICT|on_conflict" --type rust -B 2 -A 5 | head -60
echo ""
echo "=== NewDomain struct ==="
rg -n "struct NewDomain" --type rust -A 15
echo ""
echo "=== network_segment save lines 340-430 ==="
rg -n "." crates/api-core/src/handlers/network_segment.rs | sed -n '340,430p'
echo ""
echo "=== dns module tree ==="
fd -t f '*.rs' | grep dns | sortLength of output: 1342 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== dns module files ==="
fd -e rs | grep -i dns | sort
echo ""
echo "=== domain persist implementation ==="
rg -n "async fn persist" --type rust -A 15
echo ""
echo "=== find_by_name implementation ==="
rg -n "async fn find_by_name" --type rust -A 15
echo ""
echo "=== ON CONFLICT usage ==="
rg -n "ON CONFLICT|on_conflict" --type rust -B 2 -A 5 | head -80
echo ""
echo "=== NewDomain struct ==="
rg -n "struct NewDomain" --type rust -A 15
echo ""
echo "=== network_segment save handler (lines 340-430) ==="
awk 'NR>=340 && NR<=430' crates/api-core/src/handlers/network_segment.rs
echo ""
echo "=== DatabaseResult type ==="
rg -n "DatabaseResult" --type rust | head -10Length of output: 93380
1. TOCTOU race condition in
|
![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
🤖 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 125-134: The ensure_reverse_zone function has a race condition
where concurrent transactions can both see an empty result from
domain::find_by_name and then both attempt to call domain::persist, causing one
to fail with a unique constraint violation. Fix this by either catching the
unique constraint violation error from the domain::persist call and treating it
as success (since another transaction created the same domain concurrently), or
refactor domain::persist to support an upsert pattern using ON CONFLICT DO
NOTHING to atomically handle the insert-or-ignore scenario. This ensures the
function maintains its idempotency contract under concurrent access.
🪄 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: 51b07489-e102-4ee6-a961-b3694212ad3d
📒 Files selected for processing (2)
crates/api-core/src/handlers/network_segment.rscrates/api-db/src/dns/mod.rs
A network's reverse-DNS zone is a pure function of its prefix -- `10.0.0.0/16` is `0.10.in-addr.arpa` -- and every network already carries its prefix. So rather than have anyone author reverse zones by hand, this derives each zone from the network and creates it in the same transaction the network is created, at config init and at runtime alike. Reverse zones now track networks, static and dynamic, on their own. `cidr_to_reverse_zone` (the forward inverse of `arpa_qname_to_ip`) turns a prefix into its arpa zone name; `ensure_reverse_zone` creates the domain idempotently -- several segments can share a zone -- and skips prefixes that are not octet/nibble aligned (RFC 2317 classless is out of scope). The hook sits in `network_segment::save`, the chokepoint both the config seed (`create_initial_networks`) and the runtime create handler route through. The arpa domain registers the zone; PTR records within it already resolve through the address-keyed lookup. Tests cover the derivation (IPv4/IPv6 aligned prefixes and rejected non-aligned ones) and an idempotent, skip-on-non-aligned `ensure_reverse_zone` against the database. This supports NVIDIA#2766. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai Thanks for the eyes. Declining the concurrency hardening -- this race can't occur here. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
✏️ Learnings added
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 186-191: The test currently only verifies that
cidr_to_reverse_zone returns None for unaligned prefixes, but does not verify
that ensure_reverse_zone actually performs a no-op on the database. Add
assertions to verify database behavior by querying the domain count before and
after the ensure_reverse_zone call using sqlx::query_scalar to fetch a single
scalar count value, then assert that both counts are identical to confirm the
unaligned prefix insertion was skipped in the database.
🪄 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: dd944f71-bb79-4baf-9fa9-35d1bcadfb77
📒 Files selected for processing (2)
crates/api-core/src/handlers/network_segment.rscrates/api-db/src/dns/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/dns/mod.rs (1)
170-192: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd an IPv6 integration case to
ensure_reverse_zonepersistence test.Line 172-Line 184 validates idempotent persistence only for IPv4; an IPv6 case should be included so DB-path regressions in
ip6.arpacreation are caught by integration tests too.Suggested test tightening
#[crate::sqlx_test] async fn ensure_reverse_zone_creates_idempotently(pool: sqlx::PgPool) { let mut txn = pool.begin().await.unwrap(); - let prefix: ipnetwork::IpNetwork = "10.0.0.0/16".parse().unwrap(); - - // First call creates the zone; the second is a no-op. - super::ensure_reverse_zone(prefix, txn.as_mut()) - .await - .unwrap(); - super::ensure_reverse_zone(prefix, txn.as_mut()) - .await - .unwrap(); - let zones = super::domain::find_by_name(txn.as_mut(), "0.10.in-addr.arpa") - .await - .unwrap(); - assert_eq!(zones.len(), 1, "reverse zone created exactly once"); + for (prefix, expected_zone) in [ + ("10.0.0.0/16", "0.10.in-addr.arpa"), + ("fd00::/48", "0.0.d.f.ip6.arpa"), + ] { + let prefix: ipnetwork::IpNetwork = prefix.parse().unwrap(); + super::ensure_reverse_zone(prefix, txn.as_mut()) + .await + .unwrap(); + super::ensure_reverse_zone(prefix, txn.as_mut()) + .await + .unwrap(); + let zones = super::domain::find_by_name(txn.as_mut(), expected_zone) + .await + .unwrap(); + assert_eq!(zones.len(), 1, "reverse zone created exactly once for {prefix}"); + }As per coding guidelines, “Prefer table-driven tests…” and “Reach for a table whenever two or more tests call the same operation with different inputs.”
🤖 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 170 - 192, The test `ensure_reverse_zone_creates_idempotently` currently only validates IPv4 address handling; it needs to include IPv6 test cases to ensure `ip6.arpa` zone creation is properly tested for regressions. Refactor the test to use a table-driven test approach by creating a test data table containing multiple test cases (both IPv4 and IPv6 prefixes with their expected reverse zone names), then iterate through each case within the test body to validate that `ensure_reverse_zone` creates zones idempotently for both IPv4 and IPv6 address families.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.
Nitpick comments:
In `@crates/api-db/src/dns/mod.rs`:
- Around line 170-192: The test `ensure_reverse_zone_creates_idempotently`
currently only validates IPv4 address handling; it needs to include IPv6 test
cases to ensure `ip6.arpa` zone creation is properly tested for regressions.
Refactor the test to use a table-driven test approach by creating a test data
table containing multiple test cases (both IPv4 and IPv6 prefixes with their
expected reverse zone names), then iterate through each case within the test
body to validate that `ensure_reverse_zone` creates zones idempotently for both
IPv4 and IPv6 address families.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fd4fb5d5-5ab8-449b-930a-23976abb5074
📒 Files selected for processing (2)
crates/api-core/src/handlers/network_segment.rscrates/api-db/src/dns/mod.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
Summary
A network's reverse-DNS zone is a pure function of its prefix (
10.0.0.0/16→0.10.in-addr.arpa), and every network already carries its prefix. So this derives each reverse zone from the network and creates it in the same transaction the network is created — at config init and at runtime — rather than having anyone author it by hand. Reverse zones track networks (static and dynamic) automatically.cidr_to_reverse_zone(prefix)(api-dbdns) — the forward inverse ofarpa_qname_to_ip;Nonefor prefixes that are not octet/nibble aligned (RFC 2317 classless is out of scope).ensure_reverse_zone(prefix, txn)— creates the arpa domain idempotently (several segments can share one zone), skips non-aligned prefixes.network_segment::save— the chokepoint bothcreate_initial_networks(config seed) and the runtime create handler route through. The arpa domain registers the zone; PTR records within it resolve via the existing address-keyed lookup (the merged PTR train).Tests: a
cidr_to_reverse_zonetable (IPv4/IPv6 aligned + rejected non-aligned), and a DB test thatensure_reverse_zoneis idempotent and skips non-aligned prefixes.Follow-ups (out of scope here): the internal linknet-allocation path (
network_segment/allocate.rs) bypassessave, so its segments don't get reverse zones — linknets are point-to-point internals that likely don't want reverse DNS, but worth a deliberate call; and dropping a reverse zone on network deletion when nothing else maps to it.Implements #2766. Part of the #2630 reverse-DNS epic.