feat(dns): serve reverse PTR for overlay instance addresses#2855
Conversation
|
@coderabbitai PTAL, thanks! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughPTR reverse lookup now resolves records from machine interface and overlay instance sources. The query adds an overlay arm, excludes ChangesPTR reverse lookup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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
# Fetch PR details
gh pr view 2855 --repo NVIDIA/infra-controller --json title,body,files,additions,deletions,state
echo "=== PR DIFF ==="
gh pr diff 2855 --repo NVIDIA/infra-controllerLength of output: 7183 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Read the full file for complete context
cat -n crates/api-db/src/dns/resource_record.rsLength of output: 19106
Primary concern —
|
| Area | Assessment |
|---|---|
SQL join chain (instance_addresses → network_segments → domains) |
Correctly mirrors the dns_records_instance forward view |
host_inband exclusion (<> on an enum discriminant) |
Correct and consistent with the forward view |
TTL (hardcoded 300) |
Appropriate — no dns_record_metadata join exists for instance addresses in the forward path either |
Index use (ia.address = $1::inet) |
Rides the address index as intended |
PTR FQDN format (concat(ia.hostname, '.', d.name, '.')) |
Produces a correct trailing-dot FQDN |
| Test coverage | Both address families, plus the host_inband exclusion case — comprehensive |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/dns/resource_record.rs (1)
391-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a small table for the IPv4/IPv6 PTR assertions.
The two lookup/assert blocks exercise the same operation with different inputs; a table keeps the reverse test aligned with the nearby forward DNS test and makes future address-family cases cheaper to add.
As per coding guidelines, “Use table-driven test style when writing tests in Rust.”
♻️ Proposed refactor
- // The PTR answers with the instance's forward FQDN -- the reverse of `#2408`'s - // A/AAAA record, so the two round-trip. - let v4 = super::find_ptr_record( - txn.as_mut(), - "10.1.2.3".parse::<std::net::IpAddr>().unwrap(), - ) - .await - .unwrap(); - assert_eq!(v4.len(), 1, "one PTR for the overlay address"); - assert_eq!(v4[0].ptr_content, "10-1-2-3.tenant.example.com."); - - let v6 = super::find_ptr_record( - txn.as_mut(), - "2001:db8:abcd::2".parse::<std::net::IpAddr>().unwrap(), - ) - .await - .unwrap(); - assert_eq!(v6.len(), 1, "one PTR for the overlay address"); - assert_eq!( - v6[0].ptr_content, - "2001-0db8-abcd-0000-0000-0000-0000-0002.tenant.example.com." - ); + let cases = [ + ("10.1.2.3", "10-1-2-3.tenant.example.com."), + ( + "2001:db8:abcd::2", + "2001-0db8-abcd-0000-0000-0000-0000-0002.tenant.example.com.", + ), + ]; + + for (address, expected_ptr) in cases { + let ptrs = + super::find_ptr_record(txn.as_mut(), address.parse::<std::net::IpAddr>().unwrap()) + .await + .unwrap(); + + assert_eq!(ptrs.len(), 1, "one PTR for {address}"); + assert_eq!(ptrs[0].ptr_content, expected_ptr, "PTR for {address}"); + }🤖 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 391 - 431, The PTR test in the `find_ptr_record` block repeats the same lookup/assert pattern for IPv4 and IPv6, so refactor it into a small table-driven test. Keep the existing `add_address` setup, then iterate over a table of address, expected PTR content, and expected label count, using `super::find_ptr_record` and the same assertions for each case. This should live in the same `resource_record.rs` test area and preserve the current behavior while making it easier to add more address-family cases later.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/resource_record.rs`:
- Around line 391-431: The PTR test in the `find_ptr_record` block repeats the
same lookup/assert pattern for IPv4 and IPv6, so refactor it into a small
table-driven test. Keep the existing `add_address` setup, then iterate over a
table of address, expected PTR content, and expected label count, using
`super::find_ptr_record` and the same assertions for each case. This should live
in the same `resource_record.rs` test area and preserve the current behavior
while making it easier to add more address-family cases later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7ced9313-fa45-4495-8b32-f2e9dfb90e7b
📒 Files selected for processing (1)
crates/api-db/src/dns/resource_record.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full_review please, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/dns/resource_record.rs (1)
423-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the overlay PTR TTL in the reverse tests.
The overlay SQL branch explicitly returns
300 AS ttl, and the DNS handler forwardsttlto clients. Add a direct assertion so the test covers that changed contract, not justptr_content.As per coding guidelines, “Verification should exercise the behavior that changed.”
Proposed test assertion
assert_eq!(ptrs.len(), 1, "one PTR for {}", case.address); assert_eq!(ptrs[0].ptr_content, case.ptr); + assert_eq!(ptrs[0].ttl, 300, "PTR TTL for {}", case.address);🤖 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 423 - 431, The reverse DNS test currently verifies only ptr_content, but it should also validate the new overlay TTL contract. Update the loop in the PTR reverse test around find_ptr_record and the existing assert_eq! checks to add a direct assertion that the returned PTR record ttl is 300, since the overlay SQL branch in the DNS path now explicitly sets ttl and the handler forwards it to clients.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/resource_record.rs`:
- Around line 423-431: The reverse DNS test currently verifies only ptr_content,
but it should also validate the new overlay TTL contract. Update the loop in the
PTR reverse test around find_ptr_record and the existing assert_eq! checks to
add a direct assertion that the returned PTR record ttl is 300, since the
overlay SQL branch in the DNS path now explicitly sets ttl and the handler
forwards it to clients.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aec03b6d-6503-444d-90fd-711fa99dc162
📒 Files selected for processing (1)
crates/api-db/src/dns/resource_record.rs
Overlay (DPU-managed) instances got forward A/AAAA records in NVIDIA#2408, but their addresses still resolved to nothing in reverse -- find_ptr_record only answered from machine interfaces, and overlay addresses live in instance_addresses. This adds the reverse half: an overlay instance's address now answers a PTR with its own FQDN, so a forward record and its PTR round-trip. find_ptr_record gains a second UNION arm that reads the dns_records_instance forward view by IP, so forward and reverse resolve overlay names through one definition. The view already carries the stored hostname, the segment's forward zone, and the host_inband exclusion (those addresses are the host's own, answered by the machine arm) -- so there is no duplicated join or IP-to-name derivation. The lookup is by address, like the machine arm. - Add a UNION arm to find_ptr_record that reads dns_records_instance by IP, answering an overlay address with <hostname>.<zone>. - Reuse NVIDIA#2408's forward view outright -- forward matches it by name, reverse by IP -- so the two cannot drift. Tests added! This supports NVIDIA#2776 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full_review |
|
✅ Action performedFull review finished. |
|
@coderabbitai full_review |
|
✅ Action performedFull review finished. |
2 participants
Overlay (DPU-managed) instances got forward A/AAAA records in #2408, but their addresses still resolved to nothing in reverse --
find_ptr_recordonly answered from machine interfaces, and overlay addresses live ininstance_addresses. This adds the reverse half: an overlay instance's address now answers a PTR with its own FQDN, so a forward record and its PTR round-trip.find_ptr_recordgains a secondUNIONarm overinstance_addressesthat mirrors thedns_records_instanceforward view -- same join to the segment's forward zone, samehost_inbandexclusion (those addresses are the host's own, already answered by the machine arm). It reads the hostname stored at allocation, so there is no second IP-to-name derivation; the lookup is by address, like the machine arm.instance_addressesarm tofind_ptr_record, resolving an overlay address to<hostname>.<zone>.host_inband(the machine arm already answers those).Tests added!
This supports #2776