feat(dns): serve PTR queries from carbide-dns#2720
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 ignored due to path filters (1)
📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughA new private helper ChangesPTR Support and RData Centralization
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.
🧹 Nitpick comments (1)
crates/dns/src/lib.rs (1)
534-558: ⚡ Quick winRefactor this test to table-driven scenarios for consistency with project standards.
This test maps multiple input variants to expected outcomes and should be expressed as table-driven cases to match repo testing conventions and improve extensibility.
Suggested refactor
#[test] fn content_to_rdata_builds_supported_types_and_drops_unparseable() { - // Supported types build the matching RData from valid content. - assert!(matches!( - content_to_rdata(DnsResourceRecordType::A, "192.0.2.1"), - Some(RData::A(_)) - )); - assert!(matches!( - content_to_rdata(DnsResourceRecordType::AAAA, "fd00::1"), - Some(RData::AAAA(_)) - )); - - // A PTR's content is the target FQDN, which round-trips into the RData. - match content_to_rdata(DnsResourceRecordType::PTR, "host.example.com.") { - Some(RData::PTR(PTR(name))) => assert_eq!(name.to_string(), "host.example.com."), - other => panic!("expected PTR rdata, got {other:?}"), - } - - // Unparseable content for a supported type is dropped rather than panicked on. - assert!(content_to_rdata(DnsResourceRecordType::A, "not-an-ip").is_none()); - assert!(content_to_rdata(DnsResourceRecordType::AAAA, "192.0.2.1").is_none()); - - // A type the gate never dispatches here produces no RData. - assert!(content_to_rdata(DnsResourceRecordType::SOA, "unused").is_none()); + let cases = [ + (DnsResourceRecordType::A, "192.0.2.1", true), + (DnsResourceRecordType::AAAA, "fd00::1", true), + (DnsResourceRecordType::A, "not-an-ip", false), + (DnsResourceRecordType::AAAA, "192.0.2.1", false), + (DnsResourceRecordType::SOA, "unused", false), + ]; + + for (qtype, content, expect_some) in cases { + assert_eq!(content_to_rdata(qtype, content).is_some(), expect_some); + } + + match content_to_rdata(DnsResourceRecordType::PTR, "host.example.com.") { + Some(RData::PTR(PTR(name))) => assert_eq!(name.to_string(), "host.example.com."), + other => panic!("expected PTR rdata, got {other:?}"), + } }As per coding guidelines, “Prefer table-driven tests for any function that maps inputs to outputs, errors, or other observable results,” 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/dns/src/lib.rs` around lines 534 - 558, The test function content_to_rdata_builds_supported_types_and_drops_unparseable uses individual assert statements to test multiple input-output scenarios for the content_to_rdata function. Refactor this to a table-driven test pattern by defining a slice or vector of test cases where each case contains the input parameters (DnsResourceRecordType and content string), the expected output (matching RData variant or None), and a test description. Then iterate through the table executing the same validation logic for each case, consolidating the repetitive assertions into a single parameterized test loop.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/dns/src/lib.rs`:
- Around line 534-558: The test function
content_to_rdata_builds_supported_types_and_drops_unparseable uses individual
assert statements to test multiple input-output scenarios for the
content_to_rdata function. Refactor this to a table-driven test pattern by
defining a slice or vector of test cases where each case contains the input
parameters (DnsResourceRecordType and content string), the expected output
(matching RData variant or None), and a test description. Then iterate through
the table executing the same validation logic for each case, consolidating the
repetitive assertions into a single parameterized test loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48e1f939-2f39-4f51-b483-128390d250f8
📒 Files selected for processing (1)
crates/dns/src/lib.rs
|
@coderabbitai Thanks! Done -- |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 2720 --repo NVIDIA/infra-controller --json files,title,body | jq '.title, .body'
gh pr diff 2720 --repo NVIDIA/infra-controllerLength of output: 8829
Gate extension — the multi-line
Test — the
Nothing else to flag. The implementation is solid. LGTM. 🐇 ✨ |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/dns/src/lib.rs (1)
534-604: ⚡ Quick winPrefer
value_scenarios!/check_valuesfor this total-operation table test.This test is well-structured, but for consistency with repo conventions, migrate the case table to
carbide_test_support::value_scenarios!(orcheck_valuesif clearer with multi-field expectations).As per coding guidelines, “Use
carbide_test_support::value_scenarios!macro for total operations (those returning a plain value,Option, orbool) in Rust tests.”🤖 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/dns/src/lib.rs` around lines 534 - 604, The test `content_to_rdata_builds_supported_types_and_drops_unparseable` should be migrated from a manual table-driven test pattern to use the `carbide_test_support::value_scenarios!` macro for consistency with repository conventions. Replace the current manual `Expect` enum, `Case` struct, and the for loop that iterates over the cases array with the `value_scenarios!` macro pattern. The macro will handle the test case iteration and validation, making the test more idiomatic for this codebase which uses value scenarios for total operations like the `content_to_rdata` function.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/dns/src/lib.rs`:
- Around line 534-604: The test
`content_to_rdata_builds_supported_types_and_drops_unparseable` should be
migrated from a manual table-driven test pattern to use the
`carbide_test_support::value_scenarios!` macro for consistency with repository
conventions. Replace the current manual `Expect` enum, `Case` struct, and the
for loop that iterates over the cases array with the `value_scenarios!` macro
pattern. The macro will handle the test case iteration and validation, making
the test more idiomatic for this codebase which uses value scenarios for total
operations like the `content_to_rdata` function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 72625392-5383-4191-b285-3eb77cd3f6f9
📒 Files selected for processing (1)
crates/dns/src/lib.rs
carbide-dns answers DNS by forwarding each query to carbide-api's `LookupRecord` RPC and building `RData` from the reply. Its qtype gate allowed only A and AAAA, returning NotImp for anything else, so reverse-DNS (PTR) queries never reached the resolver. This adds PTR to the gate and builds an `RData::PTR` from the reply, so a PTR query now flows through to the handler that resolves the address to a hostname (NVIDIA#2641) and returns that name. The reply's content is parsed by record type -- an address for A/AAAA, a `Name` for PTR, since a PTR answer is a hostname rather than an address. That mapping moves into a small `content_to_rdata` function so it can be tested without a mock upstream; a record whose content does not parse is logged and dropped, unchanged from before. A table test covers the A, AAAA, and PTR mappings, plus the drop-on-unparseable and unsupported-type paths. This supports NVIDIA#2643. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai Thanks! Migrated the |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 2720 --repo NVIDIA/infra-controllerLength of output: 6927
What changed and why it's better:
Cargo.toml — No further issues. The implementation is production-grade throughout. LGTM. 🐇 ✨ [approve] |
1 participant
Summary
carbide-dns forwards each DNS query to carbide-api's
LookupRecordRPC and buildsRDatafrom the reply. Its qtype gate allowed only A and AAAA (NotImp for the rest), so reverse-DNS (PTR) queries never reached the resolver. This adds PTR to the gate and builds anRData::PTRfrom the reply.handle_requestqtype gate;retrieve_recordsbuildsRData::PTRfrom the reply's content — aName, unlike the address-valued A/AAAA records.RDatamapping moves into a smallcontent_to_rdatafunction, so it is unit-testable without a mock upstream.With this, a PTR query reaches the resolver handler (#2641) end to end: carbide-dns forwards it, the handler resolves the address to a hostname, and carbide-dns returns it as a PTR record.
Implements #2643. Part of the #2630 epic (reverse DNS / PTR records).
Draft pending review.