Add per-VPC routing-profile overrides - #4411
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. |
|
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:
Summary by CodeRabbit
WalkthroughVPC routing-profile overrides are added to RPC, REST, model, and database contracts. VPC creation resolves and persists overrides, startup validation rejects seeded overrides, networking consumes effective profiles, and CLI/web views display configured and effective values. ChangesVPC routing-profile contracts and configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant VpcClient
participant VpcHandler
participant FnnConfig
participant VpcDatabase
VpcClient->>VpcHandler: Submit VPC creation request with overrides
VpcHandler->>FnnConfig: Resolve base profile and apply overrides
FnnConfig-->>VpcHandler: Return effective routing profile
VpcHandler->>VpcDatabase: Persist VPC and overrides
VpcDatabase-->>VpcHandler: Return stored VPC
VpcHandler-->>VpcClient: Return VPC status with effective profile
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (3)
crates/api-core/src/setup.rs (1)
2141-2146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer variant-based classification over message substring matching.
Two concerns with this arm: the
"routing_profile_overrides"substring couples the table to error prose that the repository's error-message lint may reword, and theelsebranch labels every otherConfigValidationErrorasInvalidNetwork— a future non-network validation failure would be silently misattributed even thoughResolveFailure::Unexpectedexists for exactly that case.♻️ Suggested refactor to keep unknown failures distinguishable
if let Some(error) = error.downcast_ref::<model::ConfigValidationError>() { - return if error.to_string().contains("routing_profile_overrides") { - Err(ResolveFailure::InvalidVpcOverrides) - } else { - Err(ResolveFailure::InvalidNetwork) - }; + let message = error.to_string(); + return match error { + model::ConfigValidationError::InvalidValue(_) + if message.contains("test-vpc") => + { + Err(ResolveFailure::InvalidVpcOverrides) + } + model::ConfigValidationError::InvalidValue(_) => { + Err(ResolveFailure::InvalidNetwork) + } + _ => Err(ResolveFailure::Unexpected(message)), + }; }🤖 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-core/src/setup.rs` around lines 2141 - 2146, Update the ConfigValidationError classification in the surrounding resolver to match its concrete error variant or structured field for routing_profile_overrides instead of inspecting error.to_string(). Map only the known network-related variants to InvalidVpcOverrides or InvalidNetwork, and route any other ConfigValidationError to ResolveFailure::Unexpected rather than treating it as a network failure.crates/api-core/src/tests/vpc.rs (1)
605-637: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a table-driven form for these two same-operation inputs.
Both requests invoke
create_vpcwith a single differing field and share one expectation — the case the repository reserves for thecarbide-test-supporthelpers. Beyond convention,check_cases/scenarios!labels each input, so a regression on only the second request is reported directly instead of surfacing as an anonymous loop iteration. Naming the scenarios"routing_profile_type"and"routing_profile_overrides"would suffice.As per coding guidelines, "Use tables whenever multiple tests call the same operation with different inputs, but keep genuinely distinct tests as standalone 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/api-core/src/tests/vpc.rs` around lines 605 - 637, Replace the anonymous requests loop in the VPC creation test with the repository’s table-driven test helper, such as check_cases/scenarios!, using cases named "routing_profile_type" and "routing_profile_overrides". Keep each existing request input and shared create_vpc error assertion unchanged so failures identify the specific scenario.Source: Coding guidelines
crates/api-core/src/handlers/vpc.rs (1)
705-882: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for
vpc_profile_overridesinresolve_vpc_routing.Every test call in this module (updated or new) passes
Nonefor the newvpc_profile_overridesparameter. Two branches introduced by this parameter are consequently unverified at the unit level:
(None, None)with overrides present must fall through to the "no tenant/profile-type found" error.- FNN-disabled + overrides-present (with no explicit
request_profile_type) must be rejected via the "FNN configuration required" error.Adding two focused cases would close this gap and guard the new validation logic against regressions.
🧪 Suggested additional test cases
#[test] fn overrides_without_tenant_or_request_profile_is_rejected() { let err = resolve_vpc_routing( VpcVirtualizationType::Fnn, None, Some(&VpcRoutingProfileOverrides::default()), None, None, "test-org", ) .expect_err("overrides without any named profile must be rejected"); assert!(matches!(err, CarbideError::FailedPrecondition(_))); } #[test] fn fnn_disabled_with_overrides_only_is_rejected() { let tenant = tenant_with_profile(Some("INTERNAL")); let err = resolve_vpc_routing( VpcVirtualizationType::Fnn, None, Some(&VpcRoutingProfileOverrides::default()), Some(&tenant), None, "test-org", ) .expect_err("overrides require FNN configuration even without an explicit request"); assert!(matches!(err, CarbideError::FailedPrecondition(_))); }🤖 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-core/src/handlers/vpc.rs` around lines 705 - 882, Add two focused unit tests for the vpc_profile_overrides branches in resolve_vpc_routing: verify overrides without a tenant or requested profile returns FailedPrecondition, and verify FNN-disabled routing with tenant profile plus overrides but no request also returns FailedPrecondition. Use VpcRoutingProfileOverrides::default() and preserve the existing error assertions.
🤖 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-core/src/ethernet_virtualization.rs`:
- Around line 179-190: Extract the duplicated base-profile lookup and VPC
override application into an FnnConfig helper such as resolve_effective_profile,
returning the existing Cow-based Result and preserving the current Internal and
NotFoundError cases. Replace the inline resolution in
validate_instance_interface_routing_profiles
(crates/api-core/src/ethernet_virtualization.rs#L179-L190) and tenant_network
(crates/api-core/src/ethernet_virtualization.rs#L673-L683) with the helper and
propagate via ?. Replace the inline lookup in vpc_to_rpc
(crates/api-core/src/handlers/vpc.rs#L395-L431) with the helper and convert its
Result using .ok() to retain graceful missing-profile handling.
---
Nitpick comments:
In `@crates/api-core/src/handlers/vpc.rs`:
- Around line 705-882: Add two focused unit tests for the vpc_profile_overrides
branches in resolve_vpc_routing: verify overrides without a tenant or requested
profile returns FailedPrecondition, and verify FNN-disabled routing with tenant
profile plus overrides but no request also returns FailedPrecondition. Use
VpcRoutingProfileOverrides::default() and preserve the existing error
assertions.
In `@crates/api-core/src/setup.rs`:
- Around line 2141-2146: Update the ConfigValidationError classification in the
surrounding resolver to match its concrete error variant or structured field for
routing_profile_overrides instead of inspecting error.to_string(). Map only the
known network-related variants to InvalidVpcOverrides or InvalidNetwork, and
route any other ConfigValidationError to ResolveFailure::Unexpected rather than
treating it as a network failure.
In `@crates/api-core/src/tests/vpc.rs`:
- Around line 605-637: Replace the anonymous requests loop in the VPC creation
test with the repository’s table-driven test helper, such as
check_cases/scenarios!, using cases named "routing_profile_type" and
"routing_profile_overrides". Keep each existing request input and shared
create_vpc error assertion unchanged so failures identify the specific scenario.
🪄 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: 6e28a912-fc52-4473-9b45-594ffe36e988
📒 Files selected for processing (26)
crates/admin-cli/src/rpc.rscrates/admin-cli/src/vpc/create/args.rscrates/admin-cli/src/vpc/show/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/db_init.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/network_segment.rscrates/api-core/src/tests/vpc.rscrates/api-db/migrations/20260728120000_vpc_routing_profile_overrides.sqlcrates/api-db/src/vpc.rscrates/api-model/src/vpc/capability.rscrates/api-model/src/vpc/mod.rscrates/api-model/src/vpc/routing_profile.rscrates/api-web/src/vpc.rscrates/api-web/templates/vpc_detail.htmlcrates/machine-a-tron/src/api_client.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc.rs
c080aa4 to
862a21d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/rpc/src/model/vpc.rs (1)
112-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared helpers to eliminate the repeated route-target/prefix conversion logic.
The
TryFrom/Fromimpls forVpcRoutingProfileOverridesrepeat the same two shapes of logic four times each: theRouteTargetConfiglist mapping (route_target_imports/route_targets_on_exports) and thePrefixFilterPolicyEntrylist mapping (accepted_leaks_from_underlay/allowed_anycast_prefixes). Any future field tweak (e.g., adding validation) risks being applied to only one of the four copies.Extracting small conversion helpers keeps this DRY and easier to maintain.
♻️ Proposed refactor sketch
+fn route_targets_from_rpc(targets: rpc::forge::RouteTargets) -> Vec<RouteTargetConfig> { + targets + .values + .into_iter() + .map(|target| RouteTargetConfig { + asn: target.asn, + vni: target.vni, + }) + .collect() +} + +fn prefix_entries_from_rpc( + entries: rpc::forge::PrefixFilterPolicyEntries, +) -> Result<Vec<PrefixFilterPolicyEntry>, RpcDataConversionError> { + entries + .values + .into_iter() + .map(|entry| Ok(PrefixFilterPolicyEntry { prefix: entry.prefix.parse()? })) + .collect() +} + impl TryFrom<rpc::forge::VpcRoutingProfileOverrides> for VpcRoutingProfileOverrides { type Error = RpcDataConversionError; fn try_from(profile: rpc::forge::VpcRoutingProfileOverrides) -> Result<Self, Self::Error> { Ok(Self { - route_target_imports: profile.route_target_imports.map(|targets| { - targets.values.into_iter().map(|target| RouteTargetConfig { - asn: target.asn, - vni: target.vni, - }).collect() - }), - route_targets_on_exports: profile.route_targets_on_exports.map(|targets| { /* same */ }), + route_target_imports: profile.route_target_imports.map(route_targets_from_rpc), + route_targets_on_exports: profile.route_targets_on_exports.map(route_targets_from_rpc), ... - accepted_leaks_from_underlay: profile.accepted_leaks_from_underlay.map(|entries| { /* parse loop */ }).transpose()?, - allowed_anycast_prefixes: profile.allowed_anycast_prefixes.map(|entries| { /* parse loop */ }).transpose()?, + accepted_leaks_from_underlay: profile.accepted_leaks_from_underlay.map(prefix_entries_from_rpc).transpose()?, + allowed_anycast_prefixes: profile.allowed_anycast_prefixes.map(prefix_entries_from_rpc).transpose()?, }) } }A symmetric pair of helpers (
route_targets_to_rpc,prefix_entries_to_rpc) would similarly simplify the reverseFromimpl.🤖 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/rpc/src/model/vpc.rs` around lines 112 - 221, Extract shared helpers for converting route-target collections and prefix-filter entries, then reuse them in both VpcRoutingProfileOverrides TryFrom and From implementations. Replace the duplicated mappings for route_target_imports/route_targets_on_exports with the route-target helper, and accepted_leaks_from_underlay/allowed_anycast_prefixes with the prefix-entry helper, including parsing and RPC serialization behavior.
🤖 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/rpc/src/model/vpc.rs`:
- Around line 112-221: Extract shared helpers for converting route-target
collections and prefix-filter entries, then reuse them in both
VpcRoutingProfileOverrides TryFrom and From implementations. Replace the
duplicated mappings for route_target_imports/route_targets_on_exports with the
route-target helper, and accepted_leaks_from_underlay/allowed_anycast_prefixes
with the prefix-entry helper, including parsing and RPC serialization behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d9e604f1-5cbc-40b7-a3e3-fde91a25fdda
📒 Files selected for processing (27)
crates/admin-cli/src/rpc.rscrates/admin-cli/src/vpc/create/args.rscrates/admin-cli/src/vpc/show/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/db_init.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/network_segment.rscrates/api-core/src/tests/vpc.rscrates/api-db/migrations/20260728120000_vpc_routing_profile_overrides.sqlcrates/api-db/src/vpc.rscrates/api-model/src/lib.rscrates/api-model/src/vpc/capability.rscrates/api-model/src/vpc/mod.rscrates/api-model/src/vpc/routing_profile.rscrates/api-web/src/vpc.rscrates/api-web/templates/vpc_detail.htmlcrates/machine-a-tron/src/api_client.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/api-core/src/cfg/README.md
- crates/api-core/src/tests/instance_config_update.rs
- crates/api-core/src/tests/network_segment.rs
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-30 21:53:32 UTC | Commit: 862a21d |
c1f801c to
2392f2f
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4411.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/db_init.rs (1)
68-114: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent trailing-dot normalization when matching the configured domain.
forward_domainsclassification trims a trailing.before checking reverse-zone suffixes (line 86), but the configured-domain equality check on line 96 compares the raw, untrimmeddomain.nameagainstdomain_name. If a forward domain row is stored as an FQDN with a trailing dot (as seen for reverse zones in this file's own test fixtures, e.g."0.0.ip6.arpa."), whileinitial_domain_namein config is typically entered without one, the exact match silently fails and the function falls through to the ambiguous/fallback path instead of correctly selecting the configured domain. This edge case isn't covered by the new table-driven tests.🐛 Proposed fix
if let Some(domain_name) = configured_domain_name { let configured_domains = forward_domains .iter() - .filter(|domain| domain.name == domain_name) + .filter(|domain| domain.name.trim_end_matches('.') == domain_name) .collect_vec();🤖 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-core/src/db_init.rs` around lines 68 - 114, Normalize trailing dots consistently in select_seed_network_domain when matching configured_domain_name: compare the domain name after removing its trailing dot against the configured value using the same normalization applied during forward-domain classification. Preserve the existing single-match selection and fallback behavior, and add coverage for a stored forward domain with a trailing dot and an undotted configured name.
🧹 Nitpick comments (5)
crates/api-core/src/cfg/file.rs (1)
2078-2084: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the not-found resource kind with the existing routing-profile lookup.
resolve_vpc_routing_profilereportskind: "routing_profile_type", whereas the equivalent lookup incrates/api-core/src/handlers/vpc.rs(resolve_vpc_routing) reportskind: "routing_profile"for the very samefnn.routing_profilesmiss. Operators therefore see two different resource names for one failure mode.♻️ Suggested alignment
.ok_or_else(|| CarbideError::NotFoundError { - kind: "routing_profile_type", + kind: "routing_profile", id: profile_type.to_string(), })?;Note the assertion in
vpc_routing_profile_resolution_reports_consistent_errors(Line 3754) needs the matching update.🤖 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-core/src/cfg/file.rs` around lines 2078 - 2084, Update the NotFoundError construction in resolve_vpc_routing_profile to use kind "routing_profile", matching resolve_vpc_routing for the same routing_profiles lookup, and update the expected kind in vpc_routing_profile_resolution_reports_consistent_errors accordingly.crates/rpc/src/model/vpc.rs (2)
175-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the four repeated collection mappings into reusable conversions.
The route-target and prefix-entry mappings are duplicated in both directions (eight near-identical closures across
TryFromandFrom). DedicatedFrom/TryFromimpls forRouteTargets⇄Vec<RouteTargetConfig>andPrefixFilterPolicyEntries⇄Vec<PrefixFilterPolicyEntry>would keep the override conversion declarative and prevent the two directions from drifting.♻️ Sketch
impl From<Vec<RouteTargetConfig>> for rpc::forge::RouteTargets { fn from(targets: Vec<RouteTargetConfig>) -> Self { Self { values: targets .into_iter() .map(|target| rpc::common::RouteTarget { asn: target.asn, vni: target.vni }) .collect(), } } }The override conversion then reduces to
profile.route_target_imports.map(Into::into), with the mirroredFrom<rpc::forge::RouteTargets>used on the inbound side.As per coding guidelines: "Prefer
From/TryFromandFromStrconversions over bespoke conversion methods".🤖 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/rpc/src/model/vpc.rs` around lines 175 - 219, Extract the duplicated route-target and prefix-entry collection mappings into reusable From/TryFrom implementations for rpc::forge::RouteTargets and rpc::forge::PrefixFilterPolicyEntries with their corresponding Vec configuration types. Update both outbound and inbound conversions in the relevant TryFrom/From implementations to use map(Into::into) or the established conversion path, preserving prefix parsing errors and existing field values.Source: Coding guidelines
140-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve prefix context in parse errors
The existing
?resolves throughRpcDataConversionError::NetworkParseError, but the resulting message omits the profile field and raw prefix. Map each parse failure to a lowercaseInvalidArgumentcontaining the field name, offending prefix, and parser error; apply this to both lists.🤖 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/rpc/src/model/vpc.rs` around lines 140 - 167, Update both prefix parsing closures in the profile conversion to map parse failures into lowercase InvalidArgument errors instead of relying on the existing RpcDataConversionError conversion. Include the corresponding profile field name, raw prefix value, and parser error in each message, preserving the current successful conversion and collection behavior for accepted_leaks_from_underlay and allowed_anycast_prefixes.crates/api-core/src/handlers/vpc.rs (1)
698-920: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the
resolve_vpc_routingcases into a table, and cover the FNN-enabled + overrides success path.Thirteen tests now invoke the same function with only argument variations, which is precisely the shape the repository reserves for table-driven scenarios. A
scenarios!-style table keyed on (virt type, requested profile, overrides, tenant, FNN) would also make the one currently untested branch obvious: overrides supplied with FNN enabled and a tenant profile, asserting the resolvedprofile_typeand inheritedinternalflag. Today only the rejection paths for overrides are asserted at this level.As per coding guidelines, "Use tables whenever multiple tests call the same operation with different inputs, but keep genuinely distinct tests as standalone tests," and "prefer table-driven tests using
carbide-test-supportscenarios such asscenarios!/value_scenarios!".🤖 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-core/src/handlers/vpc.rs` around lines 698 - 920, Consolidate the repetitive `resolve_vpc_routing` tests into a `carbide-test-support` `scenarios!` or equivalent table-driven scenario covering their varied inputs and expected results, while keeping genuinely distinct assertions standalone. Add a successful FNN-enabled overrides scenario with a tenant profile, asserting the resolved `profile_type` and inherited `internal` flag alongside the existing rejection and default cases.Source: Coding guidelines
crates/admin-cli/src/vpc/show/cmd.rs (1)
184-252: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCLI "nice" output omits the effective routing profile shown by the web view.
The PR-stack description for this cohort states the CLI and web views both display "serialized routing overrides and effective routing profiles when available," and
crates/api-web/src/vpc.rs'sVpcDetaildoes add an effective-profile section in this same PR. This function only addsROUTING PROFILE TYPE/ROUTING PROFILE OVERRIDESrows —vpc.status.effective_routing_profile(already available on the sameVpcvalue) is never surfaced here. If there isn't a separate raw/JSON output path that already covers this forvpc show, operators lose visibility into the resolved effective profile from the primary human-readable output.♻️ Suggested addition for parity with the web detail view
( "ROUTING PROFILE OVERRIDES", routing_profile_overrides.into(), ), + ( + "EFFECTIVE ROUTING PROFILE", + vpc.status + .as_ref() + .and_then(|status| status.effective_routing_profile.as_ref()) + .map(serde_json::to_string_pretty) + .transpose()? + .unwrap_or_else(|| "Not applicable or unavailable".to_string()) + .into(), + ), ];🤖 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/admin-cli/src/vpc/show/cmd.rs` around lines 184 - 252, Update convert_vpc_to_nice_format to include vpc.status.effective_routing_profile in the human-readable rows, alongside ROUTING PROFILE TYPE and ROUTING PROFILE OVERRIDES. Serialize or format the effective profile consistently with the web detail view, handling its absence as an unavailable/empty value, while preserving the existing output fields.
🤖 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.
Outside diff comments:
In `@crates/api-core/src/db_init.rs`:
- Around line 68-114: Normalize trailing dots consistently in
select_seed_network_domain when matching configured_domain_name: compare the
domain name after removing its trailing dot against the configured value using
the same normalization applied during forward-domain classification. Preserve
the existing single-match selection and fallback behavior, and add coverage for
a stored forward domain with a trailing dot and an undotted configured name.
---
Nitpick comments:
In `@crates/admin-cli/src/vpc/show/cmd.rs`:
- Around line 184-252: Update convert_vpc_to_nice_format to include
vpc.status.effective_routing_profile in the human-readable rows, alongside
ROUTING PROFILE TYPE and ROUTING PROFILE OVERRIDES. Serialize or format the
effective profile consistently with the web detail view, handling its absence as
an unavailable/empty value, while preserving the existing output fields.
In `@crates/api-core/src/cfg/file.rs`:
- Around line 2078-2084: Update the NotFoundError construction in
resolve_vpc_routing_profile to use kind "routing_profile", matching
resolve_vpc_routing for the same routing_profiles lookup, and update the
expected kind in vpc_routing_profile_resolution_reports_consistent_errors
accordingly.
In `@crates/api-core/src/handlers/vpc.rs`:
- Around line 698-920: Consolidate the repetitive `resolve_vpc_routing` tests
into a `carbide-test-support` `scenarios!` or equivalent table-driven scenario
covering their varied inputs and expected results, while keeping genuinely
distinct assertions standalone. Add a successful FNN-enabled overrides scenario
with a tenant profile, asserting the resolved `profile_type` and inherited
`internal` flag alongside the existing rejection and default cases.
In `@crates/rpc/src/model/vpc.rs`:
- Around line 175-219: Extract the duplicated route-target and prefix-entry
collection mappings into reusable From/TryFrom implementations for
rpc::forge::RouteTargets and rpc::forge::PrefixFilterPolicyEntries with their
corresponding Vec configuration types. Update both outbound and inbound
conversions in the relevant TryFrom/From implementations to use map(Into::into)
or the established conversion path, preserving prefix parsing errors and
existing field values.
- Around line 140-167: Update both prefix parsing closures in the profile
conversion to map parse failures into lowercase InvalidArgument errors instead
of relying on the existing RpcDataConversionError conversion. Include the
corresponding profile field name, raw prefix value, and parser error in each
message, preserving the current successful conversion and collection behavior
for accepted_leaks_from_underlay and allowed_anycast_prefixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4428bff3-4a0b-4726-aaca-a6c7b5cc3579
📒 Files selected for processing (27)
crates/admin-cli/src/rpc.rscrates/admin-cli/src/vpc/create/args.rscrates/admin-cli/src/vpc/show/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/db_init.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/network_segment.rscrates/api-core/src/tests/vpc.rscrates/api-db/migrations/20260728120000_vpc_routing_profile_overrides.sqlcrates/api-db/src/vpc.rscrates/api-model/src/lib.rscrates/api-model/src/vpc/capability.rscrates/api-model/src/vpc/mod.rscrates/api-model/src/vpc/routing_profile.rscrates/api-web/src/vpc.rscrates/api-web/templates/vpc_detail.htmlcrates/machine-a-tron/src/api_client.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-core/src/cfg/README.md
2392f2f to
8244e44
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
crates/api-core/src/handlers/vpc.rs (3)
826-847: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a scenario for an unset
access_tier.Line 640 and Line 641 both rely on
unwrap_or_default()to map an absentaccess_tierto tier0, the broadest tier.profile()always setsaccess_tier: Some(..), so no scenario pins that fallback. A profile that omitsaccess_tierin configuration becomes maximally broad as a base and maximally restrictive as a tenant profile, which is a consequential default to leave untested.💚 Suggested scenario addition
} => FailsWith(FailedPrecondition), + RoutingResolutionInput { + network_virtualization_type: Fnn, + requested_profile_type: Some("UNSET_TIER"), + routing_profile_overrides: None, + tenant: Some(tenant_with_profile(Some("EXTERNAL"))), + fnn_config: Some(fnn_with_profiles(&[ + ("EXTERNAL", profile(false, 2)), + ("UNSET_TIER", FnnRoutingProfileConfig { + internal: Some(false), + ..Default::default() + }), + ])), + // An unset access tier resolves to the broadest tier `0`. + } => FailsWith(FailedPrecondition), }🤖 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-core/src/handlers/vpc.rs` around lines 826 - 847, Add a routing-resolution test scenario in the existing profile-access-tier cases covering a profile with access_tier unset. Construct the relevant profile/configuration without profile()’s explicit tier value, and assert the resulting behavior verifies unwrap_or_default() maps the missing tier to 0 for both base and tenant-profile resolution as applicable.
405-428: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging the discarded resolution error.
vpc_to_rpcdiscards the error fromresolve_vpc_routing_profilewith.ok(). Two distinct conditions collapse into the same empty result: a named profile removed from the configuration, and an FNN VPC persisted withoutrouting_profile_type. Neither leaves a trace. A structured warning keeps the response contract unchanged and makes the condition observable.♻️ Suggested refactor to record the suppressed error
fnn_config.and_then(|fnn| { // A persisted VPC can outlive its named runtime profile. In that // case the status remains available without an effective profile. fnn.resolve_vpc_routing_profile(&vpc.config) - .ok() + .inspect_err(|error| { + tracing::warn!( + vpc_id = %vpc.id, + %error, + "Effective routing profile unresolved for VPC" + ); + }) + .ok() .map(|profile| rpc::VpcEffectiveRoutingProfile::from(profile.as_ref())) })🤖 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-core/src/handlers/vpc.rs` around lines 405 - 428, Update vpc_to_rpc’s resolve_vpc_routing_profile handling to preserve the existing None response behavior while recording a structured warning when resolution returns an error. Distinguish the error path from a successful resolution and include sufficient VPC/profile context in the warning; leave successful profile conversion and unsupported-network handling unchanged.
625-647: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAlign the
NotFoundErrorkind with the configuration resolver.These lookups report
kind: "routing_profile".FnnConfig::resolve_vpc_routing_profileincrates/api-core/src/cfg/file.rsreportskind: "routing_profile_type"for the same missing-profile condition, as does the VNI release path at Line 322 of this file. A client that keys onkindobserves two different values for one failure mode. Select one identifier and use it in all three sites.🤖 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-core/src/handlers/vpc.rs` around lines 625 - 647, Align the missing-profile error identifiers in the VPC handler with the established value used by FnnConfig::resolve_vpc_routing_profile and the VNI release path. Update both NotFoundError constructions in the Some(fnn) branch to use the same kind value, preserving their existing profile IDs and lookup behavior.crates/api-core/src/tests/vpc.rs (2)
639-653: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCarry the error message into the expectation.
The closure collapses the error to a
bool. When this test regresses, the report states only thattruewas expected andfalsewas produced. The actual gRPC message is discarded, so the failure gives no diagnostic value. Map the error to its message and assert on the fragment so the report contains the observed text.♻️ Suggested refactor for a self-describing failure
|request| { let api = env.api.clone(); async move { api.create_vpc(request) .await .map(drop) - .map_err(|error| { - error.message().contains( - "`routing_profile_type` and `routing_profile_overrides` fields are FNN-only", - ) - }) + .map_err(|error| error.message().to_string()) } },Then assert the fragment in each
Caseexpectation, for example with a helper that checks containment against the returned message.🤖 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-core/src/tests/vpc.rs` around lines 639 - 653, Update the create_vpc error handling in the test closure to return the actual gRPC error message, rather than collapsing it to a boolean via error.message().contains(...). Adjust the corresponding Case expectations to assert that the returned message contains the required “routing_profile_type” and “routing_profile_overrides” fragment, preserving self-describing failure output.
759-839: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for named-profile changes.
Create a VPC with a named profile, change that profile in the API configuration, then assert that
find_vpcs_by_idsandupdate_vpcreturn the neweffective_routing_profile. Existing assertions use one fixed profile configuration.🤖 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-core/src/tests/vpc.rs` around lines 759 - 839, Extend the test around the existing VPC create/find/update flow to use a named routing profile, then modify that profile through the API configuration before calling find_vpcs_by_ids and update_vpc. Assert both responses expose the updated effective_routing_profile rather than the original fixed expected profile, while preserving the existing override and virtualization-transition assertions.
🤖 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-core/src/handlers/vpc.rs`:
- Around line 598-602: Update the FailedPrecondition message in the (_, None)
match arm to accurately cover requests containing routing_profile_overrides as
well as a requested profile type, and replace “routing profile-type” with
“routing profile type.”
---
Nitpick comments:
In `@crates/api-core/src/handlers/vpc.rs`:
- Around line 826-847: Add a routing-resolution test scenario in the existing
profile-access-tier cases covering a profile with access_tier unset. Construct
the relevant profile/configuration without profile()’s explicit tier value, and
assert the resulting behavior verifies unwrap_or_default() maps the missing tier
to 0 for both base and tenant-profile resolution as applicable.
- Around line 405-428: Update vpc_to_rpc’s resolve_vpc_routing_profile handling
to preserve the existing None response behavior while recording a structured
warning when resolution returns an error. Distinguish the error path from a
successful resolution and include sufficient VPC/profile context in the warning;
leave successful profile conversion and unsupported-network handling unchanged.
- Around line 625-647: Align the missing-profile error identifiers in the VPC
handler with the established value used by
FnnConfig::resolve_vpc_routing_profile and the VNI release path. Update both
NotFoundError constructions in the Some(fnn) branch to use the same kind value,
preserving their existing profile IDs and lookup behavior.
In `@crates/api-core/src/tests/vpc.rs`:
- Around line 639-653: Update the create_vpc error handling in the test closure
to return the actual gRPC error message, rather than collapsing it to a boolean
via error.message().contains(...). Adjust the corresponding Case expectations to
assert that the returned message contains the required “routing_profile_type”
and “routing_profile_overrides” fragment, preserving self-describing failure
output.
- Around line 759-839: Extend the test around the existing VPC
create/find/update flow to use a named routing profile, then modify that profile
through the API configuration before calling find_vpcs_by_ids and update_vpc.
Assert both responses expose the updated effective_routing_profile rather than
the original fixed expected profile, while preserving the existing override and
virtualization-transition assertions.
🪄 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: 9251bbb3-b75a-4cf6-822e-d972df99613f
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (28)
crates/admin-cli/src/rpc.rscrates/admin-cli/src/vpc/create/args.rscrates/admin-cli/src/vpc/show/cmd.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/db_init.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/instance_allocate.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/network_segment.rscrates/api-core/src/tests/vpc.rscrates/api-db/migrations/20260728120000_vpc_routing_profile_overrides.sqlcrates/api-db/src/vpc.rscrates/api-model/src/lib.rscrates/api-model/src/vpc/capability.rscrates/api-model/src/vpc/mod.rscrates/api-model/src/vpc/routing_profile.rscrates/api-web/src/vpc.rscrates/api-web/templates/vpc_detail.htmlcrates/machine-a-tron/src/api_client.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (22)
- crates/admin-cli/src/vpc/create/args.rs
- crates/api-core/src/tests/instance_config_update.rs
- crates/api-model/src/vpc/capability.rs
- crates/api-db/src/vpc.rs
- crates/api-web/src/vpc.rs
- crates/api-model/src/lib.rs
- crates/api-core/src/tests/network_segment.rs
- crates/rpc/src/model/vpc.rs
- crates/admin-cli/src/vpc/show/cmd.rs
- crates/admin-cli/src/rpc.rs
- crates/api-core/src/setup.rs
- crates/api-core/src/tests/machine_network.rs
- crates/api-core/src/db_init.rs
- crates/machine-a-tron/src/api_client.rs
- crates/api-core/src/tests/instance_allocate.rs
- crates/api-db/migrations/20260728120000_vpc_routing_profile_overrides.sql
- crates/api-web/templates/vpc_detail.html
- crates/rpc/build.rs
- crates/api-core/src/cfg/file.rs
- crates/api-model/src/vpc/routing_profile.rs
- crates/api-model/src/vpc/mod.rs
- crates/api-core/src/ethernet_virtualization.rs
efc8039 to
9297f4f
Compare
9297f4f to
3b24ed9
Compare
…s defined in API configuration
3b24ed9 to
28d8f6b
Compare
Adds create-time, per-VPC routing-profile overrides on top of named routing-profiles. Each explicitly supplied override replaces the corresponding base-profile value, while omitted properties continue to inherit from the named profile. The operator-controlled `internal` and `access_tier` properties cannot be overridden at the VPC level. The effective profile is resolved from the current API configuration and persisted VPC overrides rather than stored separately, so API responses reflect changes to the named base profile. ## Related issues NVIDIA#2624 ## Type of Change - [x] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes NVIDIA#2624 Signed-off-by: Alex Ball <aball@nvidia.com>
#4411 allows routing-profiles overrides for VPC creation. This PR continues the support through VPC updates. Similarly, each explicitly supplied override replaces the corresponding base-profile value, while omitted properties continue to inherit from the named profile. The operator-controlled internal and access_tier properties cannot be overridden at the VPC level. Agent also has a minor update to include interface-level profiles in the additional hashing it does to detect config changes. ## Related issues #2624 ## Type of Change - [x] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes #2624
2 participants
Adds create-time, per-VPC routing-profile overrides on top of named routing-profiles.
Each explicitly supplied override replaces the corresponding base-profile value, while omitted properties continue to inherit from the named profile. The operator-controlled
internalandaccess_tierproperties cannot be overridden at the VPC level.The effective profile is resolved from the current API configuration and persisted VPC overrides rather than stored separately, so API responses reflect changes to the named base profile.
Related issues
#2624
Type of Change
Breaking Changes
Testing
Additional Notes
#2624