fix(agent): leak IPv6 host routes to the underlay - #4543
Conversation
|
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)
Summary by CodeRabbit
WalkthroughThe change separates IPv4 and IPv6 NVUE address data, computes family-specific tenant host routes, renders dual-stack interface and underlay configuration, and adds routing-profile tests for FNN, L2, and ETV paths. ChangesDual-stack underlay leakage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VPCConfiguration
participant TmplNvue
participant FNNTemplate
participant UnderlayPrefixLists
VPCConfiguration->>TmplNvue: provide separated addresses and tenant host routes
TmplNvue->>FNNTemplate: render IPv4 and IPv6 interface data
TmplNvue->>FNNTemplate: pass family-specific leakage indicators and routes
FNNTemplate->>UnderlayPrefixLists: render conditional IPv4 and IPv6 permit rules
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ ✅ Action performedFull review 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/agent/src/nvue.rs (1)
2410-2446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an IPv4-only FNN leakage scenario.
The current scenarios only use a dual-stack port. Add a profile-enabled case with
host_ipv6_route: Noneor an empty route. Assert that IPv4 rule65002exists, IPv6 rule65002does not exist, and IPv6 rule65535remains the deny rule.As per PR objectives, IPv4-only VPCs must retain a deny-only IPv6 list.
🤖 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/agent/src/nvue.rs` around lines 2410 - 2446, Add an IPv4-only scenario to test_build_fnn_dual_stack_leaks_follow_profile by configuring the FNN profile with host_ipv6_route absent or empty while leakage is enabled. Extend the collected assertions to verify IPv4 rule 65002 exists, IPv6 rule 65002 is absent, and the IPv6 prefix list retains deny rule 65535, while preserving the existing dual-stack cases.
🤖 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/agent/src/nvue.rs`:
- Around line 2410-2446: Add an IPv4-only scenario to
test_build_fnn_dual_stack_leaks_follow_profile by configuring the FNN profile
with host_ipv6_route absent or empty while leakage is enabled. Extend the
collected assertions to verify IPv4 rule 65002 exists, IPv6 rule 65002 is
absent, and the IPv6 prefix list retains deny rule 65535, while preserving the
existing dual-stack cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f133a016-d9c1-453e-bfdc-ba6f8191058f
📒 Files selected for processing (3)
crates/agent/src/nvue.rscrates/agent/templates/nvue_startup_fnn.confcrates/agent/templates/tests/nvue_build_fnn_dual_stack.yaml.expected
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/agent/src/nvue.rs (1)
2410-2444: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an enabled IPv4-only scenario. Extend the table with an enabled profile and
host_ipv6_route: None; assert IPv4 rule65002exists, IPv6 rule65002is absent, and IPv6 rule65535still denies all traffic.🤖 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/agent/src/nvue.rs` around lines 2410 - 2444, Extend the scenarios in test_build_fnn_dual_stack_leaks_follow_profile with an enabled IPv4-only configuration using host_ipv6_route: None. Assert that IPv4 prefix-list rule 65002 exists, IPv6 rule 65002 is absent, and IPv6 rule 65535 remains present to deny all traffic.Source: Path instructions
🤖 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/agent/src/nvue.rs`:
- Around line 2410-2444: Extend the scenarios in
test_build_fnn_dual_stack_leaks_follow_profile with an enabled IPv4-only
configuration using host_ipv6_route: None. Assert that IPv4 prefix-list rule
65002 exists, IPv6 rule 65002 is absent, and IPv6 rule 65535 remains present to
deny all traffic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df675ad2-0a10-4f70-8541-098b7772b0a1
📒 Files selected for processing (3)
crates/agent/src/nvue.rscrates/agent/templates/nvue_startup_fnn.confcrates/agent/templates/tests/nvue_build_fnn_dual_stack.yaml.expected
Historically, `TmplConfigPort.IPs` has carried both IPv4 and IPv6 addresses so one port can render as dual stack. The underlay leak rule also iterates `IPs`, though, which means an IPv6 prefix can wind up under `ALLOW_TO_UNDERLAY_PREFIX_LIST` while the IPv6 list keeps only its default deny. So, split each port's interface and SVI addresses into `IPs`/`IPsIpv6` and `SviIPs`/`SviIPsIpv6` before Gtmpl sees them. Rust now also prepares the final family-specific leak inputs: IPv4 keeps the existing gateway CIDR behavior, and IPv6 uses `HostIPv6Route`. FNN only renders those prepared lists, while ETV renders both address buckets so its existing dual-stack behavior stays the same. The tests cover dual-stack FNN and ETV interfaces, dual-stack SVI and VRR addresses, profile gating, and an enabled IPv4-only VPC. They also check the exact prefixes in each allow-list and keep the IPv6 default deny in place. This supports NVIDIA#2391 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4543.docs.buildwithfern.com/infra-controller |
2 participants
Historically,
TmplConfigPort.IPshas carried both IPv4 and IPv6 addresses so one port can render as dual stack. The underlay leak rule also iteratesIPs, though, which means an IPv6 prefix can wind up underALLOW_TO_UNDERLAY_PREFIX_LISTwhile the IPv6 list keeps only its default deny.So, split each port's interface and SVI addresses into
IPs/IPsIpv6andSviIPs/SviIPsIpv6before Gtmpl sees them. Rust now also prepares the final family-specific leak inputs: IPv4 keeps the existing gateway CIDR behavior, and IPv6 usesHostIPv6Route. FNN only renders those prepared lists, while ETV renders both address buckets so its existing dual-stack behavior stays the same.The tests cover dual-stack FNN and ETV interfaces, dual-stack SVI and VRR addresses, profile gating, and an enabled IPv4-only VPC. They also check the exact prefixes in each allow-list and keep the IPv6 default deny in place.
Related issues
This supports #2391
Type of Change
Breaking Changes
Testing
Validation:
cargo test -p carbide-agent --lib-- 199 passedcargo make format-nightlycargo make clippycargo make carbide-lintsAdditional Notes
TmplConfigPortremains one logical port. Only its family-sensitive address collections are separated before templating, which avoids duplicating interface stanzas.IPsremains the IPv4 gateway CIDR source for compatibility; IPv6 deliberately usesHostIPv6Route.Closes #2391