feat(dns): serve forward A/AAAA for overlay instance addresses#2786
feat(dns): serve forward A/AAAA for overlay instance addresses#2786chet wants to merge 1 commit into
Conversation
|
@coderabbitai PTAL, thanks! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Summary by CodeRabbit
WalkthroughAdds a persisted hostname for instance addresses, serves forward DNS records from a new ChangesOverlay Instance Forward DNS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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
# Get PR details
gh pr view 2786 --repo NVIDIA/infra-controller --json title,body,files,additions,deletions
# Get the diff
gh pr diff 2786 --repo NVIDIA/infra-controllerLength of output: 13488 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Look for existing dns_records view migrations to check resource_record types
fd '\.sql$' crates/api-db/migrations --exec grep -l 'dns_records' {} \; | sort
# Check dns_records_shortname and related view definitions
rg -n 'resource_record' crates/api-db/migrations/ -A2 -B2 | head -120Length of output: 1325 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Read the most recent dns_records view migrations before this PR
cat -n crates/api-db/migrations/20260622072105_remove_unused_instance_dns_view.sql
echo "---"
cat -n crates/api-db/migrations/20260607120000_ipv6_dns_hostname_normalization.sql
echo "---"
cat -n crates/api-db/migrations/20260218000000_ipv6_dns_aaaa_support.sql | head -120
echo "---"
# Check the dns_records view definition for resource_record types in the combined views
grep -n 'resource_record' crates/api-db/migrations/20260607120000_ipv6_dns_hostname_normalization.sql
grep -n 'resource_record' crates/api-db/migrations/20260512120000_link_bmc_machine_interfaces.sql
grep -n 'resource_record' crates/api-db/migrations/20250113220120_dns_record_views_update.sql
grep -n 'resource_record' crates/api-db/migrations/20260218000000_ipv6_dns_aaaa_support.sqlLength of output: 12926
1. Untested embedded-IPv4 branch in
|
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
When you say "Overlay (DPU-managed)," are you referring to instances that are allocated on a machine with a DPU? |
|
@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
🧹 Nitpick comments (1)
crates/api-db/migrations/20260623054901_serve_overlay_instance_forward_dns.sql (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the intentional NULL TTL in the instance DNS arm.
Line 27 sets
ttltoNULLintentionally; add a short inline comment so future edits do not add an unnecessary metadata join.Suggested patch
- NULL::integer AS ttl, + NULL::integer AS ttl, -- falls back to default TTL via find_record COALESCE(ttl, 300)🤖 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/migrations/20260623054901_serve_overlay_instance_forward_dns.sql` at line 27, The NULL::integer AS ttl assignment on line 27 of the serve_overlay_instance_forward_dns migration is intentionally set to NULL but lacks documentation. Add an inline SQL comment (using -- syntax) immediately before or after the NULL::integer AS ttl expression to explain that the NULL value is intentional and document why metadata joins should not be added for this field in future edits. This prevents future developers from misinterpreting the NULL as an oversight that needs correction.
🤖 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 268-325: The test overlay_instance_addresses_are_served_forward
currently validates native IPv4 and IPv6 addresses but omits IPv4-mapped IPv6
addresses. Add a third call to add_address with an IPv4-mapped IPv6 address
(such as ::ffff:192.168.1.1) paired with an appropriate subnet before the
backfill_missing_hostnames calls, update the assertion expecting 2 filled
addresses to expect 3 instead, and then add corresponding validation logic after
the IPv6 check that calls find_record with the appropriate hostname for the
mapped address and verifies it returns one AAAA record with the correct q_type
and parsed address matching the IPv4-mapped IPv6 address.
---
Nitpick comments:
In
`@crates/api-db/migrations/20260623054901_serve_overlay_instance_forward_dns.sql`:
- Line 27: The NULL::integer AS ttl assignment on line 27 of the
serve_overlay_instance_forward_dns migration is intentionally set to NULL but
lacks documentation. Add an inline SQL comment (using -- syntax) immediately
before or after the NULL::integer AS ttl expression to explain that the NULL
value is intentional and document why metadata joins should not be added for
this field in future edits. This prevents future developers from misinterpreting
the NULL as an oversight that needs correction.
🪄 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: 2c7a156e-150b-4dfe-bf96-53a7b6d0ba77
📒 Files selected for processing (6)
crates/api-core/src/db_init.rscrates/api-core/src/setup.rscrates/api-db/migrations/20260623054901_serve_overlay_instance_forward_dns.sqlcrates/api-db/src/dns/resource_record.rscrates/api-db/src/host_naming/mod.rscrates/api-db/src/instance_address.rs
✅ Files skipped from review due to trivial changes (1)
- crates/api-db/src/host_naming/mod.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai Not sure why you didn't re-review? I did push up changes? |
|
I will trigger a full re-review of the PR now to ensure all newly pushed changes are captured. (ᵔ◡ᵔ)🐇 ✅ Action performedFull review finished. |
|
@coderabbitai Got it, thank you! So if I squash + force-push, it won't trigger you to re-run, and I'd need to be more obvious thank "PTAL" or "review"? Like I should say something like "@coderabbitai Addressed/made some changes and pushed them back up. Can you PTAL again? Thank you!" -- would that work? |
|
Tip For best results, initiate chat on the files or code changes.
Squash + force-push and automatic reviews: A force-push replaces the branch's commit SHAs. If automatic reviews are enabled for the repo, CodeRabbit should fire automatically on the new push event (since the SHAs are new). If automatic reviews are paused, the push is silently ignored.
This forces a complete re-review of all current changes regardless of the internal tracking state, which is the correct hammer for rewritten history. Natural language like "PTAL" or your proposed phrasing: That will not reliably trigger a review. CodeRabbit parses explicit
That combination — explicit command + natural language context — is the most reliable pattern after history rewrites. (ᵔ◡ᵔ)🐇 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/api-db/src/host_naming/mod.rs (1)
413-424: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a mapped-IPv6 hostname test to lock the contract.
address_to_hostnamenow feeds persisted DNS labels across modules; please add a case for::ffff:192.168.1.1to prevent drift in this edge-path format.Suggested test addition
@@ fn address_to_hostname_v6_is_fully_expanded() { let address: IpAddr = "2001:db8:abcd::2".parse().unwrap(); assert_eq!( "2001-0db8-abcd-0000-0000-0000-0000-0002", address_to_hostname(&address).unwrap() ); } + + #[test] + fn address_to_hostname_v4_mapped_ipv6_is_stable() { + let address: IpAddr = "::ffff:192.168.1.1".parse().unwrap(); + assert_eq!( + "0000-0000-0000-0000-0000-ffff-c0a8-0101", + address_to_hostname(&address).unwrap() + ); + }As per coding guidelines, “Prefer table-driven tests … for test coverage in Rust,” and as per path instructions, “Prefer findings about behavior … and missing tests over style-only comments.”
🤖 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/host_naming/mod.rs` around lines 413 - 424, Add a table-driven test for the address_to_hostname function to lock down the contract for the IPv6-mapped IPv4 address edge case. Create test cases that verify the behavior for both standard IPv4 and IPv6 addresses, and specifically include a test case for the mapped-IPv6 format (::ffff:192.168.1.1) to prevent behavioral drift. Since address_to_hostname now feeds persisted DNS labels across modules, this test will ensure the hostname transformation remains consistent as the code evolves.Sources: Coding guidelines, Path instructions
crates/api-db/src/dns/resource_record.rs (1)
274-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer table-driven assertions for the address-family matrix.
This block repeats the same operation/assertion pattern for multiple address inputs. Refactoring to
value_scenarios!(orcheck_values) will make future coverage expansion cheaper and less error-prone.As per coding guidelines, "Prefer table-driven tests using the
carbide-test-supportcrate withscenarios!/value_scenarios!andcheck_cases/check_values."🤖 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 274 - 320, The test function overlay_instance_addresses_are_served_forward repeats the same assertion pattern twice for IPv4 and IPv6 addresses. Refactor this test to use table-driven assertions with value_scenarios! or check_values from the carbide-test-support crate. Define a test case table that includes the address, expected DNS name, expected record type (A or AAAA), and the parsed address value, then iterate through these scenarios using the appropriate macro instead of duplicating the add_address and find_record/assertion blocks for each address family. This will make the test more maintainable and easier to extend with additional address test cases.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 274-320: The test function
overlay_instance_addresses_are_served_forward repeats the same assertion pattern
twice for IPv4 and IPv6 addresses. Refactor this test to use table-driven
assertions with value_scenarios! or check_values from the carbide-test-support
crate. Define a test case table that includes the address, expected DNS name,
expected record type (A or AAAA), and the parsed address value, then iterate
through these scenarios using the appropriate macro instead of duplicating the
add_address and find_record/assertion blocks for each address family. This will
make the test more maintainable and easier to extend with additional address
test cases.
In `@crates/api-db/src/host_naming/mod.rs`:
- Around line 413-424: Add a table-driven test for the address_to_hostname
function to lock down the contract for the IPv6-mapped IPv4 address edge case.
Create test cases that verify the behavior for both standard IPv4 and IPv6
addresses, and specifically include a test case for the mapped-IPv6 format
(::ffff:192.168.1.1) to prevent behavioral drift. Since address_to_hostname now
feeds persisted DNS labels across modules, this test will ensure the hostname
transformation remains consistent as the code evolves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 90542313-7d21-4d40-bcd8-50b9b305f207
📒 Files selected for processing (4)
crates/api-db/migrations/20260623054901_serve_overlay_instance_forward_dns.sqlcrates/api-db/src/dns/resource_record.rscrates/api-db/src/host_naming/mod.rscrates/api-db/src/instance_address.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2786.docs.buildwithfern.com/infra-controller |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Overlay (DPU-managed) instances allocate their addresses from a segment's pool into instance_addresses -- addresses that never reach machine_interface_addresses, so the forward DNS views never published them and a tenant could not resolve an overlay instance by name. This serves them: each overlay instance address now answers a forward A/AAAA record, named by its address under the segment's forward zone -- the same IP-derived form the host-naming strategy uses elsewhere. The name is derived once in Rust by the same address_to_hostname helper machine interfaces use, and stored on instance_addresses; the dns_records view just reads the column, exactly as the shortname view reads machine_interfaces.hostname. One source of truth for the IP-to-name rule, and no hostname logic in SQL. - Store a Rust-derived hostname on instance_addresses (populated at allocation) and add a dns_records_instance arm to the served view that reads it, joined to the segment's forward zone (network_segments.subdomain_id). - Skip host_inband segments: those addresses are the host's own and are already published by the shortname view. New addresses pick up DNS as they are allocated; instance forward DNS had been unserved for over a year, so there is no existing state to retrofit. Tests added! This supports NVIDIA#2408 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Overlay (DPU-managed) instances allocate their addresses from a segment's pool into
instance_addresses-- addresses that never reachmachine_interface_addresses, so the forward DNS views never published them and a tenant could not resolve an overlay instance by name. This serves them: each overlay instance address now answers a forward A/AAAA record, named by its address under the segment's forward zone -- the same IP-derived form the host-naming strategy uses elsewhere.The name is derived once in Rust by the same
address_to_hostnamehelper machine interfaces use, and stored oninstance_addresses; thedns_recordsview just reads the column, exactly as the shortname view readsmachine_interfaces.hostname. One source of truth for the IP-to-name rule, and no hostname logic in SQL.instance_addresses(populated at allocation) and add adns_records_instancearm to the served view that reads it, joined to the segment's forward zone (network_segments.subdomain_id).host_inbandsegments: those addresses are the host's own and are already published by the shortname view.New addresses pick up DNS as they are allocated; instance forward DNS had been unserved for over a year, so there is no existing state to retrofit.
Tests added!
This supports #2408