feat(dns): parse reverse-DNS (PTR) query names into IP addresses#2691
feat(dns): parse reverse-DNS (PTR) query names into IP addresses#2691chet wants to merge 1 commit into
Conversation
|
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. |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbitRelease Notes
WalkthroughA new exported Rust function ChangesRust ARPA qname to IP parsing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
|
@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.
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 90-126: The test cases in the check_cases_async function call
currently cover standard IPv4 and IPv6 addresses but are missing a test case for
IPv6-mapped IPv4 addresses (in the format ::ffff:x.x.x.x), which exercise a
dedicated embedded-IPv4 conversion branch in the SQL function. Add a new Case
object to the test array with a scenario that uses an IPv6-mapped IPv4 address
as input (for example ::ffff:192.0.2.1), set an appropriate expect value with
the expected PTR record output for that mapped address, and place it alongside
the other test cases passed to check_cases_async to ensure this branch is
covered.
🪄 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: 3e40a21a-8107-4574-b58b-fd3918484804
📒 Files selected for processing (2)
crates/api-db/migrations/20260618070345_reverse_dns_ptr_records.sqlcrates/api-db/src/dns/mod.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/dns/mod.rs (1)
86-112: 💤 Low valueConsider adding a case-insensitivity test case.
The function normalizes input via
to_ascii_lowercase()(line 35), but no test case exercises this path. Adding a mixed-case variant would strengthen the contract.📝 Suggested addition
"rejects non-arpa and malformed" { "host.example.com." => None, "1.2.3.in-addr.arpa." => None, "300.0.0.0.in-addr.arpa." => None, "1.0.168.192.in-addr.arpa.extra." => None, } + "handles case normalization" { + "1.0.168.192.IN-ADDR.ARPA." => Some(IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1))), + }🤖 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 86 - 112, The parses_arpa_qname_to_ip test function does not verify that the arpa_qname_to_ip function correctly handles mixed-case input, even though the implementation normalizes input via to_ascii_lowercase(). Add test cases with mixed-case variants (such as uppercase letters in the arpa domain names like "IN-ADDR.ARPA" or "IP6.ARPA") to at least one of the existing test groups to ensure the case-insensitivity normalization path is exercised and working correctly.
🤖 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 86-112: The parses_arpa_qname_to_ip test function does not verify
that the arpa_qname_to_ip function correctly handles mixed-case input, even
though the implementation normalizes input via to_ascii_lowercase(). Add test
cases with mixed-case variants (such as uppercase letters in the arpa domain
names like "IN-ADDR.ARPA" or "IP6.ARPA") to at least one of the existing test
groups to ensure the case-insensitivity normalization path is exercised and
working correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c53b2f0f-008a-4a76-8de5-59c8ca2befd4
📒 Files selected for processing (1)
crates/api-db/src/dns/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-db/src/dns/mod.rs (1)
26-26: ⚡ Quick winPrefer structured tracing fields over interpolated log text.
Use tracing attributes so logs remain queryable in logfmt pipelines.
Suggested patch
- tracing::debug!("Normalized domain name: {} to: {}", name, normalize_domain); + tracing::debug!(original=%name, normalized=%normalize_domain, "normalized domain name");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` at line 26, The tracing::debug! call uses string interpolation which makes logs non-queryable in logfmt pipelines. Replace the interpolated string format in the debug call with structured tracing fields. Instead of passing the name and normalize_domain values as format string arguments, pass them as named attributes to the tracing macro (for example, using field_name = value syntax), keeping only a descriptive message string in the macro call itself. This ensures the log output uses logfmt syntax and remains queryable.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/src/dns/mod.rs`:
- Line 25: The normalize_domain variable uses to_lowercase() instead of
to_ascii_lowercase(), which creates an inconsistency with how PTR parsing
normalizes domains and can produce inconsistent DNS lookup keys for non-ASCII
input. Replace the to_lowercase() method call with to_ascii_lowercase() in the
normalize_domain assignment to ensure consistent ASCII case-folding for DNS
domain-key normalization.
---
Nitpick comments:
In `@crates/api-db/src/dns/mod.rs`:
- Line 26: The tracing::debug! call uses string interpolation which makes logs
non-queryable in logfmt pipelines. Replace the interpolated string format in the
debug call with structured tracing fields. Instead of passing the name and
normalize_domain values as format string arguments, pass them as named
attributes to the tracing macro (for example, using field_name = value syntax),
keeping only a descriptive message string in the macro call itself. This ensures
the log output uses logfmt syntax and remains queryable.
🪄 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: dd9fe382-037e-49c2-8cae-1dfe368b50a8
📒 Files selected for processing (1)
crates/api-db/src/dns/mod.rs
| use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; | ||
|
|
||
| pub fn normalize_domain(name: &str) -> String { | ||
| let normalize_domain = name.trim_end_matches('.').to_lowercase(); |
There was a problem hiding this comment.
Use ASCII case-folding in domain normalization for DNS keys.
normalize_domain uses to_lowercase(), while PTR parsing uses to_ascii_lowercase(). For DNS/domain-key normalization, this mismatch can produce inconsistent lookup keys for non-ASCII input paths.
Suggested patch
- let normalize_domain = name.trim_end_matches('.').to_lowercase();
+ let normalize_domain = name.trim_end_matches('.').to_ascii_lowercase();🤖 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` at line 25, The normalize_domain variable uses
to_lowercase() instead of to_ascii_lowercase(), which creates an inconsistency
with how PTR parsing normalizes domains and can produce inconsistent DNS lookup
keys for non-ASCII input. Replace the to_lowercase() method call with
to_ascii_lowercase() in the normalize_domain assignment to ensure consistent
ASCII case-folding for DNS domain-key normalization.
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>
|
@coderabbitai Thanks for the eyes. Updated -- added a case-normalization group (mixed-case |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the current state of the file under review
cat crates/api-db/src/dns/mod.rsLength of output: 9828 🏁 Script executed: #!/bin/bash
# List all CodeRabbit review comments on this PR to determine what to resolve
gh pr view 2691 --repo NVIDIA/infra-controller --comments | head -200Length of output: 8116 🏁 Script executed: #!/bin/bash
# Get open review comments to find IDs and determine what to resolve
gh api repos/NVIDIA/infra-controller/pulls/2691/comments --jq '.[] | {id: .id, body: .body[0:120], path: .path, line: .line}'Length of output: 1092 🏁 Script executed: #!/bin/bash
# Get full body of the open normalize_domain comment and the resolved mapped-IPv4 comment
gh api repos/NVIDIA/infra-controller/pulls/comments/3446815099 --jq '.body'
gh api repos/NVIDIA/infra-controller/pulls/comments/3433851517 --jq '.resolved // "not resolved field"'
# Check if comment 3433851517 thread was resolved
gh api repos/NVIDIA/infra-controller/pulls/comments/3433873291 --jq '.body[0:400]'Length of output: 2233
One item remains before I can resolve: comment on Line 25 ( - let normalize_domain = name.trim_end_matches('.').to_lowercase();
+ let normalize_domain = name.trim_end_matches('.').to_ascii_lowercase();DNS labels are restricted to ASCII by spec (RFC 1123), so |
2 participants
Summary
The conversion foundation for reverse DNS (PTR). A PTR query name is an address in
in-addr.arpa/ip6.arpaform; resolving it means going name → address, which is a clean inverse — so this does it in Rust instead of a large PL/pgSQL function:arpa_qname_to_ip(&str) -> Option<IpAddr>reverses the four octets (IPv4) or 32 nibbles (IPv6);Ipv6Addrassembles the address, so the IPv6 case is a few lines. ReturnsNonefor non-arpa / malformed names.value_scenarios!) cover IPv4, IPv6, and the reject cases.The PTR handler will parse the incoming qname with this and look the interface up by address (an indexed
WHERE address = $ip), rather than computing an arpa string for every row in a view. The address index +find_ptr_recordland with #2639; the handler with #2641.Implements #2637. Part of the #2630 epic (reverse DNS / PTR records).
Draft pending review.