feat(dns): answer PTR queries in the DNS lookup handler#2719
Conversation
Reverse DNS resolves a PTR query name -- an address in `in-addr.arpa` / `ip6.arpa` form -- back to a hostname. The lookup direction, arpa name to address, is a clean inverse, so this does it in Rust rather than a large PL/pgSQL function: `arpa_qname_to_ip` reverses the four octets (IPv4) or the 32 nibbles (IPv6) and hands back an `IpAddr`. The PTR handler will use that to look the interface up by address directly -- an indexed equality, not a per-row arpa string computed across a view. `Ipv6Addr` does the address assembly, so the IPv6 case is a few lines instead of the manual hextet expansion the equivalent SQL needs. This is the conversion foundation for reverse DNS; the indexed address lookup (`find_ptr_record`) and the handler arm build on it. Tests cover the IPv4 and IPv6 forms plus rejection of non-arpa and malformed names. This supports NVIDIA#2637. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
With reverse-DNS resolution parsing a PTR query name into an address (NVIDIA#2637), this is the lookup that answers it: `find_ptr_record(address)` returns the FQDN(s) the forward shortname view publishes for whichever primary or BMC interface holds that address. The `WHERE` mirrors `dns_records_shortname_combined` exactly, so a forward A/AAAA record and its PTR round-trip. The query filters on `address`, and a new index on `machine_interface_addresses(address)` makes it an index probe rather than a scan. `DbPtrRecord` decodes the FQDN answer -- a hostname, unlike the IP-valued `DbResourceRecord`. A DB-backed test inserts a primary interface and its address, asserts it resolves to the right FQDN, and asserts an address no interface holds resolves to nothing. This supports NVIDIA#2639. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbitRelease Notes
WalkthroughAdds end-to-end PTR (reverse-DNS) record support. A new ChangesPTR Reverse-DNS Record Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes 🚥 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/api-core/src/handlers/dns.rs (1)
97-97: ⚡ Quick winSwitch PTR lookup debug log to structured fields.
Line 97 currently interpolates the qname; prefer key/value attributes for logfmt compatibility.
Suggested change
- tracing::debug!("Looking up PTR record for {}", query_name); + tracing::debug!(qname = %query_name, "looking up PTR record");As per coding guidelines, “All services should emit logs in 'logfmt' syntax” and “prefer placing common fields as attributes passed to tracing functions instead of using string interpolation.”
🤖 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/dns.rs` at line 97, The debug log statement uses string interpolation to include query_name in the message instead of structured fields. Refactor the tracing::debug! call to pass query_name as a key-value field attribute rather than interpolating it into the format string. This ensures logfmt compatibility by moving the dynamic value into structured field attributes while keeping the message as a static string.Source: Coding guidelines
crates/api-core/src/tests/dns.rs (1)
386-413: ⚡ Quick winRefactor repeated PTR assertions into table-driven cases.
Lines 386-413 execute one operation (
lookup_ptr) across multiple input/expected variants; a case table will make future PTR coverage cheaper to extend.As per coding guidelines, “Prefer table-driven tests for any function that maps inputs to outputs” 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-core/src/tests/dns.rs` around lines 386 - 413, Refactor the multiple `lookup_ptr` calls and their assertions (IPv4 reverse lookup, IPv6 reverse lookup, unheld address, and malformed qname cases) into a table-driven test structure. Create a slice of test case structs containing the input qname, expected content, and expected length for each variant, then iterate through the table executing the `lookup_ptr` call and performing the assertions within the loop. This eliminates the repeated assertion patterns and makes it easier to extend PTR test coverage in the future.Source: Coding guidelines
crates/api-db/src/dns/mod.rs (1)
24-27: ⚡ Quick winUse structured tracing fields in normalization logging.
Line 26 should log attributes instead of interpolated text so logfmt consumers can query keys consistently.
Suggested change
pub fn normalize_domain(name: &str) -> String { - let normalize_domain = name.trim_end_matches('.').to_lowercase(); - tracing::debug!("Normalized domain name: {} to: {}", name, normalize_domain); - normalize_domain + let normalized_domain = name.trim_end_matches('.').to_lowercase(); + tracing::debug!(input = %name, normalized = %normalized_domain, "normalized domain name"); + normalized_domain }As per coding guidelines, “All services should emit logs in 'logfmt' syntax” and “prefer placing common fields as attributes passed to tracing functions instead of using string interpolation.”
🤖 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 24 - 27, The tracing::debug! call in the normalize_domain function currently uses string interpolation to include the original domain name and normalized result in the log message. Instead, convert this to use structured tracing fields by passing the name and normalize_domain values as key-value attributes to the tracing macro, keeping only a descriptive message string. This allows logfmt consumers to query the fields consistently as structured attributes rather than parsing interpolated text.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-core/src/handlers/dns.rs`:
- Line 97: The debug log statement uses string interpolation to include
query_name in the message instead of structured fields. Refactor the
tracing::debug! call to pass query_name as a key-value field attribute rather
than interpolating it into the format string. This ensures logfmt compatibility
by moving the dynamic value into structured field attributes while keeping the
message as a static string.
In `@crates/api-core/src/tests/dns.rs`:
- Around line 386-413: Refactor the multiple `lookup_ptr` calls and their
assertions (IPv4 reverse lookup, IPv6 reverse lookup, unheld address, and
malformed qname cases) into a table-driven test structure. Create a slice of
test case structs containing the input qname, expected content, and expected
length for each variant, then iterate through the table executing the
`lookup_ptr` call and performing the assertions within the loop. This eliminates
the repeated assertion patterns and makes it easier to extend PTR test coverage
in the future.
In `@crates/api-db/src/dns/mod.rs`:
- Around line 24-27: The tracing::debug! call in the normalize_domain function
currently uses string interpolation to include the original domain name and
normalized result in the log message. Instead, convert this to use structured
tracing fields by passing the name and normalize_domain values as key-value
attributes to the tracing macro, keeping only a descriptive message string. This
allows logfmt consumers to query the fields consistently as structured
attributes rather than parsing interpolated text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a4e0dcaf-8d96-48f4-807b-2e685181d1ed
📒 Files selected for processing (5)
crates/api-core/src/handlers/dns.rscrates/api-core/src/tests/dns.rscrates/api-db/migrations/20260620153439_machine_interface_addresses_address_index.sqlcrates/api-db/src/dns/mod.rscrates/api-db/src/dns/resource_record.rs
With the arpa query name parsed to an address (NVIDIA#2637) and the address-to-FQDN lookup in place (NVIDIA#2639), this wires them into the `LookupRecord` RPC so it answers reverse DNS. A `PTR` query name is an address in `in-addr.arpa` / `ip6.arpa` form rather than a hostname, so it cannot go through the qname view lookup that serves A/AAAA/SOA: a new `PTR` arm parses the name back to an `IpAddr` with `arpa_qname_to_ip`, resolves it with `find_ptr_record`, and returns the holding interface's FQDN. Each reply echoes the queried name back as its `qname`, the form the PowerDNS remote backend expects. A name that does not parse, or that no interface holds, yields an empty answer rather than an error -- a reverse lookup with no record is the normal "no such record" response, not a failure. An end-to-end test resolves both an IPv4 and an IPv6 address on a managed host's interface to that interface's FQDN, and covers the empty-answer cases for an unheld address and a name that does not parse. This supports NVIDIA#2641. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai Thanks for the eyes. Both quick wins addressed -- the PTR lookup log now passes |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Triggering a full re-review now. ✅ Action performedFull review finished. |
|
Triggering a full re-review now. ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/api-core/src/tests/dns.rs (1)
420-434: ⚡ Quick winAssert
ttlanddomain_idon successful PTR replies.At Line 420, the success branch validates core fields but skips
ttl/domain_id, which are part of the returned contract. Adding those checks will better protect the PTR API mapping path from silent regressions.🤖 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 420 - 434, The success branch of the PTR test case (in the Some(expected_content) match arm) currently validates qtype, qname, and content but is missing assertions for ttl and domain_id fields. Add assert_eq! statements to verify records[0].ttl and records[0].domain_id match their expected values from the test case data, following the same pattern as the existing assertions for qtype, qname, and content to ensure the complete contract of the PTR API response is validated.crates/api-db/src/dns/mod.rs (1)
377-380: ⚡ Quick winStrengthen PTR DB test assertions for full row contract.
At Line 377, the success path validates
ptr_contentonly. Please also assertttlanddomain_idso regressions in theCOALESCE(meta.ttl, 300)/join mapping are caught by this test.Proposed test assertion update
match expected { Some(fqdn) => { assert_eq!(records.len(), 1, "address {address}"); assert_eq!(records[0].ptr_content, fqdn, "address {address}"); + assert_eq!(records[0].ttl, 300, "address {address}"); + assert_eq!( + records[0].domain_id.to_string(), + "10000000-0000-0000-0000-000000000001", + "address {address}" + ); } None => assert!( records.is_empty(), "address {address} should resolve to nothing" ), }🤖 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 377 - 380, In the Some(fqdn) success path block, strengthen the test assertions to validate the full row contract. After the existing assertion that checks records[0].ptr_content equals fqdn, add assertions to validate records[0].ttl (verifying it matches the expected default or configured value from the COALESCE logic) and records[0].domain_id (verifying the join mapping is correct). This ensures regressions in the database query logic and field mappings are caught by the test.
🤖 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 149-154: The PTR lookup query that joins machine_interfaces with
domains table and filters on mia.address does not apply a soft-deletion guard on
domains, unlike other DNS read paths in this file. Add a condition to the WHERE
clause to exclude soft-deleted domains by checking the appropriate deletion
column on the domains table d (likely d.deleted_at IS NULL or similar deletion
guard pattern used elsewhere in this file) to prevent returning stale DNS data
from deleted domains.
---
Nitpick comments:
In `@crates/api-core/src/tests/dns.rs`:
- Around line 420-434: The success branch of the PTR test case (in the
Some(expected_content) match arm) currently validates qtype, qname, and content
but is missing assertions for ttl and domain_id fields. Add assert_eq!
statements to verify records[0].ttl and records[0].domain_id match their
expected values from the test case data, following the same pattern as the
existing assertions for qtype, qname, and content to ensure the complete
contract of the PTR API response is validated.
In `@crates/api-db/src/dns/mod.rs`:
- Around line 377-380: In the Some(fqdn) success path block, strengthen the test
assertions to validate the full row contract. After the existing assertion that
checks records[0].ptr_content equals fqdn, add assertions to validate
records[0].ttl (verifying it matches the expected default or configured value
from the COALESCE logic) and records[0].domain_id (verifying the join mapping is
correct). This ensures regressions in the database query logic and field
mappings are caught by the test.
🪄 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: 1ae52872-95a0-4c85-aa3a-77047e046b40
📒 Files selected for processing (5)
crates/api-core/src/handlers/dns.rscrates/api-core/src/tests/dns.rscrates/api-db/migrations/20260620153439_machine_interface_addresses_address_index.sqlcrates/api-db/src/dns/mod.rscrates/api-db/src/dns/resource_record.rs
| FROM machine_interface_addresses mia | ||
| JOIN machine_interfaces mi ON mi.id = mia.interface_id | ||
| JOIN domains d ON d.id = mi.domain_id | ||
| LEFT JOIN dns_record_metadata meta ON meta.id = mi.id | ||
| WHERE mia.address = $1::inet | ||
| AND (mi.primary_interface = TRUE OR mi.interface_type = 'Bmc')"#; |
There was a problem hiding this comment.
Exclude soft-deleted domains in PTR lookup query.
At Line 153, PTR lookup can still return records from deleted domains because this query does not apply a domain deletion guard, unlike other DNS read paths in this file. That can leak stale DNS data.
Proposed SQL fix
WHERE mia.address = $1::inet
- AND (mi.primary_interface = TRUE OR mi.interface_type = 'Bmc')"#;
+ AND (mi.primary_interface = TRUE OR mi.interface_type = 'Bmc')
+ AND d.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 149 - 154, The PTR
lookup query that joins machine_interfaces with domains table and filters on
mia.address does not apply a soft-deletion guard on domains, unlike other DNS
read paths in this file. Add a condition to the WHERE clause to exclude
soft-deleted domains by checking the appropriate deletion column on the domains
table d (likely d.deleted_at IS NULL or similar deletion guard pattern used
elsewhere in this file) to prevent returning stale DNS data from deleted
domains.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
Summary
The handler that makes the
LookupRecordRPC answer reverse DNS. APTRquery name is an address inin-addr.arpa/ip6.arpaform, not a hostname, so it cannot go through the qname view lookup that serves A/AAAA/SOA — a newPTRarm parses the name back to anIpAddrwitharpa_qname_to_ip(#2637) and resolves it withfind_ptr_record(#2639).lookup_ptr_recordhelper: arpa qname →IpAddr→ the holding interface's FQDN.qname(the form the PowerDNS remote backend expects); a name that does not parse, or that no interface holds, yields an empty answer rather than an error.This completes the resolver side of reverse DNS; carbide-dns's PTR unlock is #2643.
Implements #2641. Part of the #2630 epic (reverse DNS / PTR records).
Draft pending review.