fix: Allow setting routing profile type when creating VPCs#350
Draft
bcavnvidia wants to merge 1 commit intoNVIDIA:mainfrom
Draft
fix: Allow setting routing profile type when creating VPCs#350bcavnvidia wants to merge 1 commit intoNVIDIA:mainfrom
bcavnvidia wants to merge 1 commit intoNVIDIA:mainfrom
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. |
Contributor
|
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
WalkthroughAdds optional VPC routing profile type support end-to-end: API fields and validation, model conversions to protobuf/DB, persistence, Temporal workflow propagation, OpenAPI and SDK surface updates, tests, and a DB migration to add Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Handler
participant Model as Validation/Conversion
participant DB as Database
participant Temporal as Temporal Worker
Client->>API: POST /vpcs {networkVirtualizationType, routingProfileType?}
API->>Model: Validate APIVpcCreateRequest
alt routingProfileType provided
Model->>Model: ensure networkVirtualizationType == FNN
alt not FNN
Model-->>API: validation error (400)
API-->>Client: 400 Bad Request
else is FNN
Model->>Model: VpcRoutingProfileTypeProtobufFromAPI(routingProfileType)
alt conversion fails
Model-->>API: conversion error (500)
API-->>Client: 500 Internal Server Error
else conversion succeeds
Model-->>API: protobuf routingProfileType
end
end
end
API->>DB: Create VPC (routing_profile_type if present)
DB-->>API: created VPC
API->>Temporal: Start CreateVPCV2 (VpcCreationRequest with routingProfileType)
Temporal-->>API: workflow started
API-->>Client: 201 Created {routingProfileType?, ...}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/pkg/api/model/vpc_test.go (1)
95-133: Add one edge-case test forroutingProfileTypewith nilnetworkVirtualizationType.Current cases do not pin behavior when
RoutingProfileTypeis set andNetworkVirtualizationTypeis omitted. Adding this case will lock the intended contract and prevent future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/model/vpc_test.go` around lines 95 - 133, Add a table-driven test case in the vpc_test.go cases where RoutingProfileType is set (e.g. APIVpcRoutingProfileTypeInternal) but NetworkVirtualizationType is nil to assert the validator behavior; specifically add an entry in the test slice (same shape as the others using the fields struct and symbols RoutingProfileType and NetworkVirtualizationType) expecting an error (wantErr: true) so we pin the contract when network virtualization is omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/handler/vpc_test.go`:
- Around line 892-907: The test reuses the mock tsc across subtests so
tsc.AssertCalled("ExecuteWorkflow", ...) can falsely match earlier calls; fix by
isolating mock state per subtest—create a fresh mock (reinitialize tsc) inside
each subtest setup or call tsc.ExpectedCalls = nil / tsc.Calls = nil (or use
t.Cleanup to reset) before the assert, and ensure the specific ExecuteWorkflow
expectation is set on that per-subtest mock; refer to the tsc mock, the
ExecuteWorkflow assertion, and the subtest loop to locate where to
reinitialize/reset the mock.
In `@api/pkg/api/model/vpc.go`:
- Around line 123-135: The routingProfileType check currently allows a nil
NetworkVirtualizationType; update the validation in the block where
ascr.RoutingProfileType is checked so that you fail when
NetworkVirtualizationType is nil OR not equal to cdbm.VpcFNN. Specifically, keep
the existing existence/enum validation against
VpcRoutingProfileTypeProtobufFromAPI, then replace the second condition that
references ascr.NetworkVirtualizationType with a stricter check: if
ascr.NetworkVirtualizationType == nil || *ascr.NetworkVirtualizationType !=
cdbm.VpcFNN, return validation.Errors for "routingProfileType" stating it is
only supported when networkVirtualizationType is FNN.
---
Nitpick comments:
In `@api/pkg/api/model/vpc_test.go`:
- Around line 95-133: Add a table-driven test case in the vpc_test.go cases
where RoutingProfileType is set (e.g. APIVpcRoutingProfileTypeInternal) but
NetworkVirtualizationType is nil to assert the validator behavior; specifically
add an entry in the test slice (same shape as the others using the fields struct
and symbols RoutingProfileType and NetworkVirtualizationType) expecting an error
(wantErr: true) so we pin the contract when network virtualization is omitted.
🪄 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: Pro
Run ID: 4f788513-a809-4551-9136-9e749d9fc75a
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (13)
api/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/vpc.godb/pkg/db/model/vpc_test.godb/pkg/migrations/20260406130000_vpc_routing_profile_type.godocs/index.htmlopenapi/spec.yamlsdk/standard/api_allocation.gosdk/standard/api_instance_type.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
bcavnvidia
force-pushed
the
vpc-routing-profile
branch
from
155a833 to
64d79b9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
sdk/standard/api_allocation.go (1)
523-525: Correct Go deprecation marker syntax to improve IDE and tooling recognition.Lines 524, 933, and 940 use
// Deprecatedwithout the required colon. Go's doc-comment convention (used bygo doc, pkg.go.dev, and gopls) recognizes only paragraphs beginning withDeprecated:(case-sensitive). Update these comments to:// Deprecated: reason or replacement guidance here.This ensures the deprecation is properly surfaced by IDE inspectors and documentation tooling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/standard/api_allocation.go` around lines 523 - 525, Replace bare "// Deprecated" comments in this file with the proper Go doc comment prefix "Deprecated: " so tooling recognizes them; specifically update the comment above the method ApiGetAllAllocationRequest.InfrastructureProviderId to read "// Deprecated: <reason or replacement>" and do the same for the two other occurrences of bare "// Deprecated" elsewhere in the same file (replace with "// Deprecated: <reason or replacement>").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/handler/vpc.go`:
- Around line 303-313: The handler currently persists the protobuf enum string
by calling cdb.GetStrPtr(routingProfileTypeValue.String()), which couples
storage to generated protobuf labels; instead map the validated protobuf value
to the repo-owned DB constant and persist that. After resolving
routingProfileTypeValue via model.VpcRoutingProfileTypeProtobufFromAPI (based on
apiRequest.RoutingProfileType), replace the String() usage with the
corresponding db constant from the repo model (e.g.,
cdbm.VpcRoutingProfileType<Variant>) and pass that into cdb.GetStrPtr, keeping
protobuf-to-db conversion localized here and leaving routingProfileType
(cwssaws.RoutingProfileType) usage unchanged for API boundary handling.
In `@api/pkg/api/model/vpc.go`:
- Around line 123-135: The validation rejects routingProfileType when
NetworkVirtualizationType is nil, but site-aware defaults set by
CreateVPCHandler may later set it to FNN; update the logic in Validate() (around
ascr.RoutingProfileType handling) so you still verify routingProfileType exists
in VpcRoutingProfileTypeProtobufFromAPI, but only return the
"`routingProfileType` is only supported when `networkVirtualizationType` is FNN"
error when ascr.NetworkVirtualizationType is non-nil and its value is not
cdbm.VpcFNN (i.e., allow nil to pass and only reject explicit non-FNN values).
Ensure references: ascr.RoutingProfileType,
VpcRoutingProfileTypeProtobufFromAPI, ascr.NetworkVirtualizationType,
cdbm.VpcFNN, Validate(), and CreateVPCHandler.
In `@db/pkg/db/model/vpc.go`:
- Around line 91-97: Validate RoutingProfileType at the DAO boundary by checking
input.RoutingProfileType against VpcRoutingProfileTypeMap inside the VpcDAO
create and update methods (e.g., the functions/methods that persist VPCs in
vpc.go), returning a validation error if the value is not present; also add
similar checks in any bulk/patch methods referenced around the other noted
locations (the blocks around the symbols handling persistence at lines ~498-509
and ~578-582) and add a DB-level CHECK constraint for the routing_profile_type
column to enforce allowed values in the database as a final safeguard.
In `@sdk/standard/api_allocation.go`:
- Around line 530-531: Update the OpenAPI spec for the GetAllAllocation endpoint
so the tenantId parameter description clarifies it is only honored for
provider-role requests and ignored for tenant-role requests (rather than always
filtering); then regenerate the SDK so generated code (e.g., the
ApiGetAllAllocationRequest.TenantId method in sdk/standard/api_allocation.go)
contains the corrected parameter docstring and behavior note.
---
Nitpick comments:
In `@sdk/standard/api_allocation.go`:
- Around line 523-525: Replace bare "// Deprecated" comments in this file with
the proper Go doc comment prefix "Deprecated: " so tooling recognizes them;
specifically update the comment above the method
ApiGetAllAllocationRequest.InfrastructureProviderId to read "// Deprecated:
<reason or replacement>" and do the same for the two other occurrences of bare
"// Deprecated" elsewhere in the same file (replace with "// Deprecated: <reason
or replacement>").
🪄 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: Pro
Run ID: 87ae5a91-6d65-4c36-9cd2-d9429d5a9fa0
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (13)
api/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/vpc.godb/pkg/db/model/vpc_test.godb/pkg/migrations/20260406130000_vpc_routing_profile_type.godocs/index.htmlopenapi/spec.yamlsdk/standard/api_allocation.gosdk/standard/api_instance_type.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
🚧 Files skipped from review as they are similar to previous changes (6)
- db/pkg/migrations/20260406130000_vpc_routing_profile_type.go
- db/pkg/db/model/vpc_test.go
- sdk/standard/model_vpc.go
- sdk/standard/api_instance_type.go
- api/pkg/api/model/vpc_test.go
- openapi/spec.yaml
bcavnvidia
force-pushed
the
vpc-routing-profile
branch
from
64d79b9 to
d8fa778
Compare
hwadekar-nv
reviewed
Apr 6, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
db/pkg/db/model/vpc_test.go (1)
1021-1023: Exercise the supported FNN path in the new DAO coverage.Both new
routingProfileTypefixtures pair the field with Ethernet variants. If this test is meant to cover the feature, it is still exercising an unsupported state and can miss FNN-specific regressions. Switching these fixtures toVpcFNNwould keep the regression coverage aligned with the contract.Suggested adjustment
- NetworkVirtualizationType: db.GetStrPtr(VpcEthernetVirtualizer), + NetworkVirtualizationType: db.GetStrPtr(VpcFNN), RoutingProfileType: db.GetStrPtr(VpcRoutingProfileTypeInternal),- NetworkVirtualizationType: db.GetStrPtr(VpcEthernetVirtualizerWithNVUE), + NetworkVirtualizationType: db.GetStrPtr(VpcFNN), RoutingProfileType: db.GetStrPtr(VpcRoutingProfileTypeMaintenance),Also applies to: 1168-1170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/pkg/db/model/vpc_test.go` around lines 1021 - 1023, The test fixtures currently set NetworkVirtualizationType to VpcEthernetVirtualizer while using FNN-related routing profile fixtures, which exercises an unsupported Ethernet+FNN state; update the fixtures in vpc_test.go (the blocks that set NetworkVirtualizationType, RoutingProfileType, ControllerVpcID) to use VpcFNN instead of VpcEthernetVirtualizer so the RoutingProfileType FNN path is actually exercised (apply the same change to the second occurrence around lines shown in the comment).api/pkg/api/handler/vpc_test.go (1)
533-554: Add the site-default FNN happy path as well.This table now covers explicit FNN success and default-to-Ethernet rejection, but not the path that motivated relaxing validation: omitted
networkVirtualizationTypeon a native-networking-enabled site withroutingProfileTypeset. A regression in that resolution branch would still pass all new tests.Suggested table entry
+ { + name: "test VPC create API endpoint accepts routing profile when site default resolves to FNN", + fields: fields{ + dbSession: dbSession, + tc: tc, + cfg: cfg, + }, + args: args{ + reqData: &model.APIVpcCreateRequest{ + Name: "Test VPC default fnn routing profile", + Description: cdb.GetStrPtr("Test VPC Description"), + SiteID: st1.ID.String(), + RoutingProfileType: cdb.GetStrPtr(model.APIVpcRoutingProfileTypeInternal), + }, + reqOrg: tnOrg, + reqUser: tnu, + respCode: http.StatusCreated, + }, + wantErr: false, + verifyChildSpanner: true, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/vpc_test.go` around lines 533 - 554, Add a table entry in the same test case slice in vpc_test.go (the test that defines the cases for creating VPCs, likely in TestCreateVPC) that covers the happy path where the site default resolves to FNN: create an entry named like "site-default FNN allows routing profile when networkVirtualizationType omitted" using the same fields (dbSession, tc, cfg) and args where reqData is a model.APIVpcCreateRequest that omits NetworkVirtualizationType (leave zero value) but sets RoutingProfileType to model.APIVpcRoutingProfileTypeInternal and SiteID to the native-networking-enabled site (st3.ID.String()); set reqOrg/reqUser to tnOrg/tnu, expect a successful create response (http.StatusCreated) with no error (wantErr false) and verifyChildSpanner true so the test exercises the resolution branch for default-to-FNN behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/model/vpc.go`:
- Around line 344-345: The RoutingProfileType field on the VPC model is being
serialized as "routingProfileType": null for non-FNN VPCs; update the JSON
struct tag for RoutingProfileType in api/pkg/api/model/vpc.go to include
omitempty so nil values are omitted (preserving the FNN-only response contract),
and ensure callers that construct NewAPIVpc still leave RoutingProfileType as
nil for non-FNN rows so the field is dropped from JSON output.
---
Nitpick comments:
In `@api/pkg/api/handler/vpc_test.go`:
- Around line 533-554: Add a table entry in the same test case slice in
vpc_test.go (the test that defines the cases for creating VPCs, likely in
TestCreateVPC) that covers the happy path where the site default resolves to
FNN: create an entry named like "site-default FNN allows routing profile when
networkVirtualizationType omitted" using the same fields (dbSession, tc, cfg)
and args where reqData is a model.APIVpcCreateRequest that omits
NetworkVirtualizationType (leave zero value) but sets RoutingProfileType to
model.APIVpcRoutingProfileTypeInternal and SiteID to the
native-networking-enabled site (st3.ID.String()); set reqOrg/reqUser to
tnOrg/tnu, expect a successful create response (http.StatusCreated) with no
error (wantErr false) and verifyChildSpanner true so the test exercises the
resolution branch for default-to-FNN behavior.
In `@db/pkg/db/model/vpc_test.go`:
- Around line 1021-1023: The test fixtures currently set
NetworkVirtualizationType to VpcEthernetVirtualizer while using FNN-related
routing profile fixtures, which exercises an unsupported Ethernet+FNN state;
update the fixtures in vpc_test.go (the blocks that set
NetworkVirtualizationType, RoutingProfileType, ControllerVpcID) to use VpcFNN
instead of VpcEthernetVirtualizer so the RoutingProfileType FNN path is actually
exercised (apply the same change to the second occurrence around lines shown in
the comment).
🪄 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: Pro
Run ID: 68ff5b2d-fd01-4cb7-8f34-b1308de45354
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (13)
api/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/vpc.godb/pkg/db/model/vpc_test.godb/pkg/migrations/20260406130000_vpc_routing_profile_type.godocs/index.htmlopenapi/spec.yamlsdk/standard/api_allocation.gosdk/standard/api_instance_type.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
✅ Files skipped from review due to trivial changes (2)
- db/pkg/migrations/20260406130000_vpc_routing_profile_type.go
- db/pkg/db/model/vpc.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/pkg/api/handler/vpc.go
- sdk/standard/api_instance_type.go
- sdk/standard/api_allocation.go
- openapi/spec.yaml
bcavnvidia
force-pushed
the
vpc-routing-profile
branch
from
d8fa778 to
172521e
Compare
🔐 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-04-06 22:46:27 UTC | Commit: 172521e |
thossain-nv
changed the title
thossain-nv
reviewed
Apr 7, 2026
Contributor
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Thanks for the implementation @bcavnvidia Left some notes.
| if ascr.RoutingProfileType != nil { | ||
| if _, ok := VpcRoutingProfileTypeProtobufFromAPI[*ascr.RoutingProfileType]; !ok { | ||
| return validation.Errors{ | ||
| "routingProfileType": fmt.Errorf("unsupported `routingProfileType`: %s", *ascr.RoutingProfileType), |
Contributor
There was a problem hiding this comment.
Since we're providing the message with attribute keyed context, we could say:
"routingProfileType": fmt.Errorf("specified value: %s is not valid. Available options are: %s", *ascr.RoutingProfileType, <comma-separated-options>),
Contributor
Author
There was a problem hiding this comment.
We might be very close to switching this over to being a purely config/db-defined string. I.e., external might be a required minimum, but any number of profiles could be defined, and the proto would switch to being a string. I was trying to strike a middle-ground since we do need to expose this now.
Contributor
Author
There was a problem hiding this comment.
I'm going to switch this back to draft. Once this goes into -rest it'll make it a bit more concrete.
bcavnvidia
force-pushed
the
vpc-routing-profile
branch
from
172521e to
d71ff0c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
db/pkg/db/model/vpc_test.go (1)
1279-1279: Guard VNI pointer dereference in assertion.Use a nil guard before dereferencing
got.Vniso failures remain assertion-based instead of panicking.Proposed test-hardening diff
- assert.Equal(t, *tt.want.Vni, *got.Vni) + require.NotNil(t, got.Vni) + assert.Equal(t, *tt.want.Vni, *got.Vni)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/pkg/db/model/vpc_test.go` at line 1279, Guard the pointer dereference of got.Vni before asserting equality: check tt.want.Vni for nil and assert got.Vni is nil in that case, otherwise assert got.Vni is non-nil (e.g., assert.NotNil(t, got.Vni)) and then compare values by dereferencing both pointers; update the assertion around got.Vni and tt.want.Vni to follow this nil-guard pattern so the test fails with assertions instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/model/vpc.go`:
- Around line 56-61: Add the missing ADMIN routing profile type constant and
mapping so protobuf values round-trip into API responses: define
APIVpcRoutingProfileTypeAdmin (matching naming of
APIVpcRoutingProfileTypeExternal/Internal/PrivilegedInternal/Maintenance) and
add an entry in VpcRoutingProfileTypeAPIFromProtobuf mapping
cwssaws.RoutingProfileType_ROUTING_PROFILE_TYPE_ADMIN.String() to
APIVpcRoutingProfileTypeAdmin; update any related reverse map/lookups if present
so conversions (e.g., in the conversion at line ~377) no longer drop ADMIN
values.
---
Nitpick comments:
In `@db/pkg/db/model/vpc_test.go`:
- Line 1279: Guard the pointer dereference of got.Vni before asserting equality:
check tt.want.Vni for nil and assert got.Vni is nil in that case, otherwise
assert got.Vni is non-nil (e.g., assert.NotNil(t, got.Vni)) and then compare
values by dereferencing both pointers; update the assertion around got.Vni and
tt.want.Vni to follow this nil-guard pattern so the test fails with assertions
instead of panicking.
🪄 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: Pro
Run ID: 2398fea6-aeae-4bcb-9c6b-9ef657e4f207
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (13)
api/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/vpc.godb/pkg/db/model/vpc_test.godb/pkg/migrations/20260406130000_vpc_routing_profile_type.godocs/index.htmlopenapi/spec.yamlsdk/standard/api_allocation.gosdk/standard/api_instance_type.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
🚧 Files skipped from review as they are similar to previous changes (7)
- sdk/standard/model_vpc.go
- db/pkg/migrations/20260406130000_vpc_routing_profile_type.go
- sdk/standard/model_vpc_create_request.go
- db/pkg/db/model/vpc.go
- api/pkg/api/handler/vpc.go
- sdk/standard/api_instance_type.go
- openapi/spec.yaml
thossain-nv
reviewed
Apr 7, 2026
Contributor
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Thanks for the changes @bcavnvidia One more question I have, if routing profile type is not specified, should we set a default value for FNN VPCs?
Contributor
Author
In -core, it would default to the tenant's type, which defaults to external, but it can be changed via the admin-cli and there's a restriction that prevents a tenant from creating a VPC with a type "higher" than the type of the tenant. E.g., if a tenant is
I responded to code-rabbit to give it context. 😄 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change adds VPC routing profile selection to the REST API for VPC creation.
Callers can now include
routingProfileTypewhen creating a VPC.routingProfileTypeis only accepted whennetworkVirtualizationTypeis FNN, and the API response only returns routingProfileType for that network virtualization type.I considered using a string "enum" with conversion methods, but it didn't seem to add much benefit.
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes
Satisfies #327