fix(rest-api): Auto vpc-prefix support#3850
Draft
bcavnvidia wants to merge 1 commit into
Draft
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
Summary by CodeRabbit
WalkthroughInstance create, update, and batch flows now accept controller-managed VPC interface selection through ChangesVPC selection support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant InstanceHandler
participant InterfaceDAO
participant SiteWorkflow
participant InventoryActivity
Client->>InstanceHandler: submit vpcId and ipFamilies
InstanceHandler->>InterfaceDAO: persist VPC interface intent
InstanceHandler->>SiteWorkflow: send VPC network configuration
SiteWorkflow-->>InventoryActivity: report interface inventory
InventoryActivity->>InterfaceDAO: reconcile VPC prefix and status
InterfaceDAO-->>Client: return updated interface state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)rest-api/openapi/spec.yamlTraceback (most recent call last): 🔧 ast-grep (0.44.1)rest-api/docs/index.htmlast-grep timed out on this file 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/openapi/spec.yaml (1)
18514-18531: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
vpcIdshould be nullable to match its sibling selectors.
subnetIdandvpcPrefixIdare both typed[string, 'null']since they are optional, mutually-exclusive selectors on the sameInterface. The newvpcIdfield (Line 18521) is typed as a plainstring, breaking that convention. If the server ever emits an explicitvpcId: nullfor a Subnet- or explicit-prefix-backed interface (as it evidently does forsubnetId/vpcPrefixIdtoday), strict JSON Schema validation of this OpenAPI 3.1 document will reject the response.🛡️ Proposed fix for schema consistency
vpcId: - type: string + type: + - string + - 'null' description: ID of the VPC from which the Controller selects a prefix format: uuid🤖 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 `@rest-api/openapi/spec.yaml` around lines 18514 - 18531, Update the vpcId property in the relevant interface schema to allow both string and null, matching the nullable typing of subnetId and vpcPrefixId while preserving its UUID format and description.
🤖 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 `@rest-api/api/pkg/api/model/interface.go`:
- Around line 328-350: Update NewAPIInterface in
rest-api/api/pkg/api/model/interface.go (lines 328-350) to assign VpcPrefix only
when VpcPrefixID is present and VpcID is nil, matching the existing VpcPrefixID
condition. Extend the vpcInterface fixture and expectations in
rest-api/api/pkg/api/model/interface_test.go (lines 34-52) to include a loaded
VpcPrefix and verify it is omitted from the response.
In `@rest-api/db/pkg/db/model/interface.go`:
- Around line 435-450: Update the interface update path around VpcID and
VpcIPFamilyMode so transitioning from VPC-ID selection to subnet- or
prefix-based selection clears the stale VpcID and related VPC selector state
instead of retaining it. Ensure the clear/update handling can persist null
values and that API projection no longer prioritizes the old VPC selection after
the transition; preserve existing behavior when VPC selector values are
provided.
In `@rest-api/openapi/spec.yaml`:
- Around line 18654-18658: Update the dependentSchemas.subnetId constraint for
the inlineRoutingProfile property so Subnet-backed interfaces only allow
inlineRoutingProfile to be null, matching the existing ipAddress restriction and
the field description. Preserve the current behavior for interfaces without
subnetId.
- Around line 18590-18596: Update InterfaceCreateRequest.ipFamilies to reference
the shared IPFamily schema used by Interface.ipFamilies instead of defining a
hard-coded IPv4 enum, ensuring both properties stay aligned when IPv6 support
changes.
In `@rest-api/site-workflow/pkg/error/error.go`:
- Around line 94-95: Add a table-driven test case for WrapErr covering
codes.ResourceExhausted, asserting it returns a non-retryable application error
whose type is ErrTypeNICoResourceExhausted. Exercise the WrapErr branch directly
rather than constructing ErrTypeNICoResourceExhausted independently, and
preserve the existing expectations for other codes.
---
Outside diff comments:
In `@rest-api/openapi/spec.yaml`:
- Around line 18514-18531: Update the vpcId property in the relevant interface
schema to allow both string and null, matching the nullable typing of subnetId
and vpcPrefixId while preserving its UUID format and description.
🪄 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: 34edbe99-4078-4571-80d4-6d5eaef40ce8
⛔ Files ignored due to path filters (8)
rest-api/sdk/standard/api_instance.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_batch_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_interface.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_interface_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_ip_family.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (20)
rest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/instancebatch_test.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/model/instance.gorest-api/api/pkg/api/model/instance_test.gorest-api/api/pkg/api/model/interface.gorest-api/api/pkg/api/model/interface_test.gorest-api/db/pkg/db/model/instance.gorest-api/db/pkg/db/model/instance_test.gorest-api/db/pkg/db/model/interface.gorest-api/db/pkg/db/model/interface_test.gorest-api/db/pkg/migrations/20260720120000_interface_vpc_selection.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/site-workflow/pkg/error/error.gorest-api/workflow/pkg/activity/instance/instance.gorest-api/workflow/pkg/activity/instance/instance_test.go
bcavnvidia
force-pushed
the
auto-vpc-prefix-rest
branch
from
July 22, 2026 20:55
4179c59 to
f209e98
Compare
bcavnvidia
marked this pull request as ready for review
July 22, 2026 21:34
bcavnvidia
marked this pull request as draft
July 22, 2026 21:34
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3850.docs.buildwithfern.com/infra-controller |
🔐 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-22 21:38:55 UTC | Commit: f209e98 |
bcavnvidia
force-pushed
the
auto-vpc-prefix-rest
branch
from
July 22, 2026 22:04
f209e98 to
2c93971
Compare
bcavnvidia
marked this pull request as ready for review
July 22, 2026 22:04
bcavnvidia
marked this pull request as draft
July 22, 2026 22:04
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/workflow/pkg/activity/instance/instance_test.go (1)
231-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: consolidate the repeated "mark instance stale" mutation.
The
UPDATE instance SET updated = ...staleness setup is duplicated here and at Lines 369-373 and 390-394 (with an inline variant insidebuildInstance). Extracting a small helper would remove the copy/paste and centralize the magic*2interval, improving readability without altering behavior.♻️ Proposed helper extraction
markInstanceStale := func(instanceID uuid.UUID) { _, staleErr := dbSession.DB.Exec( "UPDATE instance SET updated = ? WHERE id = ?", time.Now().Add(-time.Duration(cutil.InventoryReceiptInterval)*2), instanceID, ) require.NoError(t, staleErr) }Then replace the three standalone blocks (and the inline exec in
buildInstance) withmarkInstanceStale(<id>).🤖 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 `@rest-api/workflow/pkg/activity/instance/instance_test.go` around lines 231 - 235, Consolidate the repeated stale-instance UPDATE setup in the test by adding a local markInstanceStale helper that accepts an instance ID, performs the existing timestamp mutation, and asserts no error. Replace the duplicate mutations near the current setup and the corresponding blocks in buildInstance, preserving the existing behavior and interval calculation.
🤖 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 `@rest-api/workflow/pkg/activity/instance/instance_test.go`:
- Around line 231-235: Consolidate the repeated stale-instance UPDATE setup in
the test by adding a local markInstanceStale helper that accepts an instance ID,
performs the existing timestamp mutation, and asserts no error. Replace the
duplicate mutations near the current setup and the corresponding blocks in
buildInstance, preserving the existing behavior and interval calculation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 696321a1-5200-46e2-81a0-8f8882894f7d
⛔ Files ignored due to path filters (8)
rest-api/sdk/standard/api_instance.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_batch_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_interface.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_interface_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_ip_family.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (21)
rest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancebatch.gorest-api/api/pkg/api/handler/instancebatch_test.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/model/instance.gorest-api/api/pkg/api/model/instance_test.gorest-api/api/pkg/api/model/interface.gorest-api/api/pkg/api/model/interface_test.gorest-api/db/pkg/db/model/instance.gorest-api/db/pkg/db/model/instance_test.gorest-api/db/pkg/db/model/interface.gorest-api/db/pkg/db/model/interface_test.gorest-api/db/pkg/migrations/20260720120000_interface_vpc_selection.gorest-api/docs/index.htmlrest-api/openapi/oasdiff-breaking-changes-ignore.txtrest-api/openapi/spec.yamlrest-api/site-workflow/pkg/error/error.gorest-api/workflow/pkg/activity/instance/instance.gorest-api/workflow/pkg/activity/instance/instance_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- rest-api/db/pkg/migrations/20260720120000_interface_vpc_selection.go
- rest-api/site-workflow/pkg/error/error.go
- rest-api/api/pkg/api/handler/util/common/common_test.go
- rest-api/api/pkg/api/handler/util/common/common.go
- rest-api/db/pkg/db/model/instance_test.go
- rest-api/api/pkg/api/model/instance_test.go
- rest-api/workflow/pkg/activity/instance/instance.go
- rest-api/db/pkg/db/model/interface.go
- rest-api/api/pkg/api/handler/instancebatch_test.go
- rest-api/api/pkg/api/model/interface.go
- rest-api/api/pkg/api/model/instance.go
- rest-api/db/pkg/db/model/interface_test.go
- rest-api/openapi/spec.yaml
- rest-api/api/pkg/api/model/interface_test.go
- rest-api/api/pkg/api/handler/instance_test.go
- rest-api/api/pkg/api/handler/instance.go
- rest-api/api/pkg/api/handler/instancebatch.go
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
1 participant
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.
#3456 added support for auto-selecting VPC prefixes via VPC ID on interfaces. This PR does the work of exposing that in the rest-api layer.
Related issues
#3110
Type of Change
Breaking Changes
Testing
Additional Notes
Closes #3110