feat(dns): look up PTR records by interface address#2711
Open
chet wants to merge 2 commits into
Open
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>
|
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. |
Contributor
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughA database index is added on ChangesPTR Record Database Layer
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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/api-db/src/dns/mod.rs (1)
275-337: ⚡ Quick winExpand PTR integration coverage to include the BMC branch and table the inputs.
This test validates a primary-interface hit and an unknown miss, but it does not exercise the
interface_type = 'Bmc'branch used byfind_ptr_record. Please add a BMC case (and ideally a non-primary/non-BMC negative case) and run these as table cases for the same operation.As per coding guidelines, “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 275 - 337, The test find_ptr_record_resolves_address_to_hostname currently validates only primary interface and unknown address cases but does not cover the BMC interface branch. Refactor this test into a table-driven test structure with multiple cases that exercise the same super::resource_record::find_ptr_record function with different inputs: add setup steps for a BMC interface case, a non-primary/non-BMC negative case, while keeping the existing primary interface and unknown address cases, then iterate through each case calling find_ptr_record with different addresses and validating the expected outcomes.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/migrations/20260620153439_machine_interface_addresses_address_index.sql`:
- Around line 4-5: The CREATE INDEX statement for
machine_interface_addresses_address_idx is using a blocking index build strategy
that can lock writes on the machine_interface_addresses table during migration.
Modify the index creation to use the CONCURRENTLY keyword to perform a
non-blocking concurrent build, and ensure this migration follows the repo's
non-transaction migration pattern (typically by removing any transaction
wrapping) to allow online index creation without blocking production writes.
In `@crates/api-db/src/dns/resource_record.rs`:
- Around line 133-152: The comment for the find_ptr_record function overstates
parity with dns_records_shortname_combined by claiming the WHERE clause mirrors
"exactly". Update the comment to clarify that while the WHERE clause does match
dns_records_shortname_combined exactly, the JOIN structure intentionally differs
by omitting the LEFT JOIN dns_record_types (since the function does not select
q_type), and the TTL handling intentionally diverges by using COALESCE(meta.ttl,
300) with a hardcoded default of 300 instead of allowing NULL values like the
view does. Also document the rationale for choosing 300 as the TTL default to
aid future maintainers.
---
Nitpick comments:
In `@crates/api-db/src/dns/mod.rs`:
- Around line 275-337: The test find_ptr_record_resolves_address_to_hostname
currently validates only primary interface and unknown address cases but does
not cover the BMC interface branch. Refactor this test into a table-driven test
structure with multiple cases that exercise the same
super::resource_record::find_ptr_record function with different inputs: add
setup steps for a BMC interface case, a non-primary/non-BMC negative case, while
keeping the existing primary interface and unknown address cases, then iterate
through each case calling find_ptr_record with different addresses and
validating the expected outcomes.
🪄 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: 4b09d203-b307-4774-9f65-fb31bcf582cd
📒 Files selected for processing (3)
crates/api-db/migrations/20260620153439_machine_interface_addresses_address_index.sqlcrates/api-db/src/dns/mod.rscrates/api-db/src/dns/resource_record.rs
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>
Contributor
Author
|
@coderabbitai Thanks for the eyes. Updated -- the |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat crates/api-db/src/dns/mod.rs
echo "---FILE BOUNDARY---"
cat crates/api-db/src/dns/resource_record.rsLength of output: 20649
The SQL predicate, FQDN construction ( [resolve] |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The DB lookup behind reverse DNS:
find_ptr_record(address)returns the FQDN(s) the forward shortname view publishes for whichever primary or BMC interface holds that address — itsWHEREmirrorsdns_records_shortname_combined, so a forward A/AAAA record and its PTR round-trip.addressindex onmachine_interface_addresses→ the lookup is an index probe, not a scan.DbPtrRecorddecodes the FQDN answer (a hostname, unlike the IP-valuedDbResourceRecord).The handler (#2641) will parse the incoming arpa qname with
arpa_qname_to_ip(#2637) and call this; carbide-dns's PTR unlock is #2643.Implements #2639. Part of the #2630 epic (reverse DNS / PTR records).
Draft pending review.