feat: Add Flat VPC type and instance auto interface support#574
Open
chet wants to merge 1 commit into
Open
Conversation
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 an ChangesAuto Network Instance Support
Sequence DiagramsequenceDiagram
participant Client as Client
participant Handler as CreateInstanceHandler
participant Cap as VpcTypeSupportsAutoInterface
participant Validator as APIInstanceCreateRequest/UpdateRequest
participant DAO as InstanceCreateInput/InstanceUpdateInput
participant DB as Database
participant Workflow as TemporalInstanceWorkflow
Client->>Handler: POST/PUT with auto flag
Handler->>Cap: check VPC supports auto (FLAT)
Cap-->>Handler: supportsAutoInterface? (bool)
alt supports auto
Handler->>Validator: validate (interfaces empty when auto)
Validator-->>Handler: valid
Handler->>DAO: persist NetworkAuto
DAO->>DB: INSERT/UPDATE instance (network_auto)
Handler->>Workflow: trigger workflow with Config.Network built by buildInstanceNetworkConfig(auto, interfaces)
Workflow-->>Handler: workflow started/completed
Handler-->>Client: 201/200 with instance {auto: true}
else does not support auto
Handler-->>Client: 400 BadRequest (validation: networkVirtualizationType must be FLAT)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 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-05-23 16:43:12 UTC | Commit: 9e55832 |
![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: 3
♻️ Duplicate comments (2)
openapi/spec.yaml (2)
16211-16213: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument the Flat VPC requirement for auto mode.
Same as InstanceCreateRequest: the specification does not document that
auto: truerequires the target VPC to havenetworkVirtualizationType: FLAT. This constraint is enforced at the REST API layer and should be explicitly documented so API consumers are aware of the validation requirements.🤖 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 `@openapi/spec.yaml` around lines 16211 - 16213, The OpenAPI schema for the "auto" boolean (the field named auto on Instance/InstanceCreateRequest) is missing the required-note that auto: true mandates the target VPC have networkVirtualizationType: FLAT; update the description for the auto property to explicitly state this constraint (mirror the wording used in InstanceCreateRequest), e.g. add a sentence like "When true, the target VPC MUST have networkVirtualizationType: FLAT; otherwise the request will be rejected by the REST API" so API consumers see the validation requirement.
16392-16396: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument the Flat VPC requirement for auto mode.
Same as InstanceCreateRequest and BatchInstanceCreateRequest: when setting
auto: true(whether initially or via update), the REST API validates that the Instance's VPC hasnetworkVirtualizationType: FLAT. This constraint should be explicitly documented in the specification.🤖 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 `@openapi/spec.yaml` around lines 16392 - 16396, Update the description for the schema property named "auto" to explicitly state that when auto is set to true (either on create or update) the Instance's VPC must have networkVirtualizationType: FLAT; mirror the wording used in InstanceCreateRequest and BatchInstanceCreateRequest so the REST API validation constraint is clearly documented, and mention that null leaves unchanged and true requires FLAT VPC and empty interfaces.
🧹 Nitpick comments (8)
db/pkg/db/model/instance_test.go (1)
1587-1587: ⚡ Quick winAdd DAO round-trip coverage for
NetworkAuto.The test schema now knows about
network_auto, but the suite still does not assert create/update persistence for it. In particular,TestInstanceSQLDAO_UpdateMultiple_AllFieldsno longer covers all fields inInstanceUpdateInput, so regressions on this new column can slip through unnoticed.🤖 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 `@db/pkg/db/model/instance_test.go` at line 1587, Test coverage missing for the new NetworkAuto field: update TestInstanceSQLDAO_UpdateMultiple_AllFields to set NetworkAuto on the initial create and include NetworkAuto in the InstanceUpdateInput used for the update, then after calling the DAO update (e.g., UpdateMultiple or Update) fetch the instance back and assert the persisted NetworkAuto matches the expected value; ensure the create path also sets/reads NetworkAuto so the DAO round-trip (create → update → get) verifies persistence for NetworkAuto.api/pkg/api/model/instance_test.go (1)
2426-2438: 💤 Low valueConsider relying on zero-value for
Autofield clarity.The explicit
Auto: falseon line 2433 is redundant, as Go's zero-value forboolisfalse. This test case validates the default behavior (whenautois not set,interfacesmust be provided). Consider either:
- Omitting the
Autofield entirely to make it clear you're testing the default, or- Adding a comment explaining that this tests the explicit false case vs. the implicit default.
The current form may confuse readers about whether this tests an explicit
falseAPI request or default behavior.♻️ Suggested refactor for clarity
{ name: "auto=false with empty interfaces is rejected", req: APIInstanceCreateRequest{ Name: "manual-instance", TenantID: uuid.NewString(), InstanceTypeID: cdb.GetStrPtr(uuid.NewString()), VpcID: uuid.NewString(), OperatingSystemID: cdb.GetStrPtr(uuid.NewString()), - Auto: false, + // Auto omitted (defaults to false) - interfaces are required Interfaces: nil, }, wantErr: true, wantErrorMessage: "at least one Interface must be specified", },🤖 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 `@api/pkg/api/model/instance_test.go` around lines 2426 - 2438, The test case for APIInstanceCreateRequest currently sets Auto: false explicitly which is redundant because bool zero-value is false; update the test to remove the explicit Auto field to clarify it's testing the default behavior (leave Interfaces: nil to assert validation fails), or alternatively keep Auto: false but add a one-line comment above the case clarifying this is intentionally testing the explicit-false case vs the implicit default; reference the APIInstanceCreateRequest struct and its Auto and Interfaces fields when making the change.api/pkg/api/handler/instance_test.go (2)
3386-3417: ⚡ Quick winAssert that validation failure does not dispatch Temporal workflow.
Both new negative cases assert
400+ message, but they do not verify thatExecuteWorkflowwas not called. Since these tests specifically document “reject before workflow invocation,” add an explicit no-dispatch assertion (or stable call-count delta assertion) per case.Suggested test hardening
+ // Capture call count before handler invocation for this test case. + beforeCalls := len(tsc.Calls) if err := csh.Handle(ec); (err != nil) != tt.wantErr { t.Errorf("CreateInstanceHandler.Handle() error = %v, wantErr %v", err, tt.wantErr) } + if tt.name == "test Instance create API endpoint rejects auto=true on a non-Flat VPC" { + assert.Equal(t, beforeCalls, len(tsc.Calls), "workflow should not be invoked on validation failure") + }+ beforeCalls := len(ttscm.Calls) if err := uih.Handle(ec); (err != nil) != tt.wantErr { t.Errorf("UpdateInstanceHandler.Handle() error = %v, wantErr %v", err, tt.wantErr) } + if tt.name == "test Instance update API endpoint rejects auto=true on a non-Flat VPC" { + assert.Equal(t, beforeCalls, len(ttscm.Calls), "workflow should not be invoked on validation failure") + }Also applies to: 6076-6100
🤖 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 `@api/pkg/api/handler/instance_test.go` around lines 3386 - 3417, The test cases that expect a 400 response for invalid `auto` + non-Flat VPC must also assert the Temporal workflow was not dispatched; update the two negative cases in instance_test.go (the "auto=true on a non-Flat VPC" case and the other similar case at lines ~6076-6100) to assert that the mocked Temporal client's ExecuteWorkflow was not called (e.g., use mockTemporalClient.AssertNotCalled(t, "ExecuteWorkflow", mock.Anything, mock.Anything) or capture call-count before/after and assert no delta). Locate references to ExecuteWorkflow on your mock Temporal client in the test setup and add the no-dispatch assertion immediately after the HTTP response assertions to ensure the handler rejected the request before invoking the workflow.
3415-3417: ⚡ Quick win
verifyChildSpanneris currently a no-op for this negative create test.In
TestCreateInstanceHandler_Handle, span verification runs only on201 Createdpaths, so this flag has no effect here. Either set it tofalsefor clarity or move span validation before the non-created early return if you intend to cover error paths too.🤖 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 `@api/pkg/api/handler/instance_test.go` around lines 3415 - 3417, The verifyChildSpanner flag in TestCreateInstanceHandler_Handle is ineffective for this negative-create case because span verification only runs on the 201-created path; either set verifyChildSpanner to false in the failing-case test entry to reflect that it's a no-op, or move the span verification logic inside TestCreateInstanceHandler_Handle (the code that asserts child Spanner spans) so it runs before the handler returns early on non-201 responses; reference the test table entry where verifyChildSpanner is set and the span verification routine inside TestCreateInstanceHandler_Handle when making the change.api/pkg/api/handler/vpc.go (1)
267-269: 💤 Low valueError message coupling to specific VPC type.
Similar to the batch handler, Line 268's error message explicitly references
"FNN"and site configuration while the validation uses the genericVpcTypeSupportsRoutingProfilehelper. This creates coupling between the user-facing message and the capability implementation. If the routing profile eligibility rules evolve, this message may become inaccurate.The detailed error message is valuable for user guidance. If the coupling is intentional for clarity, consider documenting this design decision. Otherwise, refactor to derive the message from the capability matrix itself.
🤖 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 `@api/pkg/api/handler/vpc.go` around lines 267 - 269, The error message is hard-coded to "FNN" and site config while the validation uses cdbm.VpcTypeSupportsRoutingProfile, causing coupling; change the handler to produce a message derived from the capability helper instead of naming a specific VPC type: call the capability API (e.g., use VpcTypeSupportsRoutingProfile and a new or existing capability query on cdbm to obtain allowed VPC types or an explanatory string) and build the logger.Warn and cutil.NewAPIErrorResponse text from that capability result, referencing apiRequest.RoutingProfile, networkVirtualizationType, logger.Warn, cdbm.VpcTypeSupportsRoutingProfile and cutil.NewAPIErrorResponse so the message stays accurate if rules change.api/pkg/api/handler/instancebatch.go (1)
429-429: 💤 Low valueConsider error message maintainability.
The error message hardcodes
"FLAT", creating tight coupling between the user-facing message and the current capability matrix implementation. IfVpcTypeSupportsAutoInterfaceevolves to support additional types beyond Flat, this message becomes stale.Consider either:
- Dynamically constructing the error message based on supported types from the capability layer, or
- Accepting this coupling as a deliberate trade-off for user clarity (document the decision if so)
🤖 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 `@api/pkg/api/handler/instancebatch.go` at line 429, The error message hardcodes "FLAT" which can become incorrect if VpcTypeSupportsAutoInterface supports more types; update the handler in instancebatch.go so the API error message is built dynamically using the capability layer (call VpcTypeSupportsAutoInterface or the function that returns supported VPC types) and join the returned types into the message passed to cutil.NewAPIErrorResponse, e.g., "auto is only supported when the VPC has networkVirtualizationType set to: <supported-types>", or alternatively replace the message with a stable generic statement and document the trade-off if you intentionally keep the hardcoded value.openapi/spec.yaml (2)
15794-15796: 💤 Low valueConsider subject-verb clarity in the description.
In line 15796, "
interfaceshere reflects the resolved set" treats the field name as a singular entity. While grammatically defensible when referring to a field name, consider rephrasing to "theinterfacesfield here reflects" or "thisinterfaceslist reflects" for improved clarity.🤖 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 `@openapi/spec.yaml` around lines 15794 - 15796, The description for the boolean field "auto" uses "interfaces here reflects" which is slightly ambiguous; update the description string to improve subject-verb clarity by referring explicitly to the field or list (for example: "when true, the caller's request `interfaces` list was empty and this `interfaces` list reflects the resolved set" or "the `interfaces` field here reflects the resolved set"), i.e., edit the description text for the "auto" property in the OpenAPI spec to replace "interfaces here reflects" with one of the clearer phrasings.
13684-13685: 💤 Low valueConsider restructuring the description for improved clarity.
The current description defines the general field purpose in one sentence, then describes only the FLAT value in the next sentence. This leaves the relationship between the field and its various enum values implicit. Consider restructuring to clarify that this field controls the VPC's network virtualization model, with FLAT being one specific mode that uses underlay segments without NICo data-plane control.
🤖 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 `@openapi/spec.yaml` around lines 13684 - 13685, The description for the VPC network virtualization enum is unclear because it mixes the field purpose with a value-specific explanation; update the description for the enum (the description string that presently mentions "Network virtualization type of the VPC" and the "FLAT" sentence) to first state the field's purpose (e.g., "Controls the VPC's network virtualization model") and then append a short clause describing FLAT as a specific mode (e.g., "FLAT: instances on zero‑DPU hosts or hosts with DPU in NIC mode; interfaces bound to underlay (HostInband) segments and NICo does not control the data plane"), so the relationship between the field and its values is explicit.
🤖 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 `@api/pkg/api/handler/instance.go`:
- Line 1568: Replace the use of the incoming request flag with the persisted
instance's value when building the workflow request: instead of using
apiRequest.Auto, use the persisted instance's NetworkAuto (the instance returned
by the DAO after create, e.g., the variable named instance or ui) so the Auto
field reflects the saved state and any DAO defaults/transformations.
In `@db/pkg/db/model/instance.go`:
- Around line 1033-1039: The bulk UpdateMultiple path is including
"network_auto" for every row when any input has NetworkAuto non-nil, causing
rows with NetworkAuto == nil to be overwritten to false; to fix, change
UpdateMultiple (or the caller that prepares inputs for Bulk) to partition the
input slice into batches that share the same update mask for NetworkAuto (e.g.,
group inputs by whether NetworkAuto is nil) before calling Bulk so that
"network_auto" is only included in the columns list for batches where every item
has NetworkAuto set; update the batching logic in instance.go (the
UpdateMultiple/Bulk preparation path) to build columns per-partition or enforce
a uniform update mask per batch rather than relying on the union of touched
columns.
In `@openapi/spec.yaml`:
- Around line 16062-16064: Update the "auto" property description to explicitly
state the REST validation: clarify that auto: true is only allowed when the
target VPC's networkVirtualizationType is FLAT (the REST API enforces this), and
add that when auto is true the request MUST have an empty or omitted interfaces
array and must not include secondaryVpcIds (secondaryVpcIds must be empty or
omitted); reference the property names "auto", "interfaces", "secondaryVpcIds"
and the VPC field "networkVirtualizationType" with the required value "FLAT" in
the description so API consumers see these constraints.
---
Duplicate comments:
In `@openapi/spec.yaml`:
- Around line 16211-16213: The OpenAPI schema for the "auto" boolean (the field
named auto on Instance/InstanceCreateRequest) is missing the required-note that
auto: true mandates the target VPC have networkVirtualizationType: FLAT; update
the description for the auto property to explicitly state this constraint
(mirror the wording used in InstanceCreateRequest), e.g. add a sentence like
"When true, the target VPC MUST have networkVirtualizationType: FLAT; otherwise
the request will be rejected by the REST API" so API consumers see the
validation requirement.
- Around line 16392-16396: Update the description for the schema property named
"auto" to explicitly state that when auto is set to true (either on create or
update) the Instance's VPC must have networkVirtualizationType: FLAT; mirror the
wording used in InstanceCreateRequest and BatchInstanceCreateRequest so the REST
API validation constraint is clearly documented, and mention that null leaves
unchanged and true requires FLAT VPC and empty interfaces.
---
Nitpick comments:
In `@api/pkg/api/handler/instance_test.go`:
- Around line 3386-3417: The test cases that expect a 400 response for invalid
`auto` + non-Flat VPC must also assert the Temporal workflow was not dispatched;
update the two negative cases in instance_test.go (the "auto=true on a non-Flat
VPC" case and the other similar case at lines ~6076-6100) to assert that the
mocked Temporal client's ExecuteWorkflow was not called (e.g., use
mockTemporalClient.AssertNotCalled(t, "ExecuteWorkflow", mock.Anything,
mock.Anything) or capture call-count before/after and assert no delta). Locate
references to ExecuteWorkflow on your mock Temporal client in the test setup and
add the no-dispatch assertion immediately after the HTTP response assertions to
ensure the handler rejected the request before invoking the workflow.
- Around line 3415-3417: The verifyChildSpanner flag in
TestCreateInstanceHandler_Handle is ineffective for this negative-create case
because span verification only runs on the 201-created path; either set
verifyChildSpanner to false in the failing-case test entry to reflect that it's
a no-op, or move the span verification logic inside
TestCreateInstanceHandler_Handle (the code that asserts child Spanner spans) so
it runs before the handler returns early on non-201 responses; reference the
test table entry where verifyChildSpanner is set and the span verification
routine inside TestCreateInstanceHandler_Handle when making the change.
In `@api/pkg/api/handler/instancebatch.go`:
- Line 429: The error message hardcodes "FLAT" which can become incorrect if
VpcTypeSupportsAutoInterface supports more types; update the handler in
instancebatch.go so the API error message is built dynamically using the
capability layer (call VpcTypeSupportsAutoInterface or the function that returns
supported VPC types) and join the returned types into the message passed to
cutil.NewAPIErrorResponse, e.g., "auto is only supported when the VPC has
networkVirtualizationType set to: <supported-types>", or alternatively replace
the message with a stable generic statement and document the trade-off if you
intentionally keep the hardcoded value.
In `@api/pkg/api/handler/vpc.go`:
- Around line 267-269: The error message is hard-coded to "FNN" and site config
while the validation uses cdbm.VpcTypeSupportsRoutingProfile, causing coupling;
change the handler to produce a message derived from the capability helper
instead of naming a specific VPC type: call the capability API (e.g., use
VpcTypeSupportsRoutingProfile and a new or existing capability query on cdbm to
obtain allowed VPC types or an explanatory string) and build the logger.Warn and
cutil.NewAPIErrorResponse text from that capability result, referencing
apiRequest.RoutingProfile, networkVirtualizationType, logger.Warn,
cdbm.VpcTypeSupportsRoutingProfile and cutil.NewAPIErrorResponse so the message
stays accurate if rules change.
In `@api/pkg/api/model/instance_test.go`:
- Around line 2426-2438: The test case for APIInstanceCreateRequest currently
sets Auto: false explicitly which is redundant because bool zero-value is false;
update the test to remove the explicit Auto field to clarify it's testing the
default behavior (leave Interfaces: nil to assert validation fails), or
alternatively keep Auto: false but add a one-line comment above the case
clarifying this is intentionally testing the explicit-false case vs the implicit
default; reference the APIInstanceCreateRequest struct and its Auto and
Interfaces fields when making the change.
In `@db/pkg/db/model/instance_test.go`:
- Line 1587: Test coverage missing for the new NetworkAuto field: update
TestInstanceSQLDAO_UpdateMultiple_AllFields to set NetworkAuto on the initial
create and include NetworkAuto in the InstanceUpdateInput used for the update,
then after calling the DAO update (e.g., UpdateMultiple or Update) fetch the
instance back and assert the persisted NetworkAuto matches the expected value;
ensure the create path also sets/reads NetworkAuto so the DAO round-trip (create
→ update → get) verifies persistence for NetworkAuto.
In `@openapi/spec.yaml`:
- Around line 15794-15796: The description for the boolean field "auto" uses
"interfaces here reflects" which is slightly ambiguous; update the description
string to improve subject-verb clarity by referring explicitly to the field or
list (for example: "when true, the caller's request `interfaces` list was empty
and this `interfaces` list reflects the resolved set" or "the `interfaces` field
here reflects the resolved set"), i.e., edit the description text for the "auto"
property in the OpenAPI spec to replace "interfaces here reflects" with one of
the clearer phrasings.
- Around line 13684-13685: The description for the VPC network virtualization
enum is unclear because it mixes the field purpose with a value-specific
explanation; update the description for the enum (the description string that
presently mentions "Network virtualization type of the VPC" and the "FLAT"
sentence) to first state the field's purpose (e.g., "Controls the VPC's network
virtualization model") and then append a short clause describing FLAT as a
specific mode (e.g., "FLAT: instances on zero‑DPU hosts or hosts with DPU in NIC
mode; interfaces bound to underlay (HostInband) segments and NICo does not
control the data plane"), so the relationship between the field and its values
is explicit.
🪄 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: 563aee29-3568-461e-8884-8eb6c7e78f81
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/nico_nico.protois excluded by!workflow-schema/site-agent/workflows/v1/*_nico.proto
📒 Files selected for processing (22)
api/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/vpc.godb/pkg/migrations/20260523061229_instance_network_auto.godb/pkg/util/testing.goopenapi/spec.yamlsdk/standard/model_batch_instance_create_request.gosdk/standard/model_instance.gosdk/standard/model_instance_create_request.gosdk/standard/model_instance_update_request.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
force-pushed
the
feat/flat-vpc-and-auto-interface
branch
from
9e55832 to
2af73a3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/pkg/api/handler/instancebatch.go (2)
1624-1653:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPass the
Autoflag to the workflow payload.The workflow payload construction does not include the
Autofield. According to the PR stack context, handlers must "pass persisted Auto into workflow payloads." The workflow (CreateInstances) requires this flag to signal the site-agent that interface configuration should be auto-resolved from HostInband segments rather than using explicit interface definitions. Without this field, the workflow cannot distinguish auto instances from explicit-interface instances, breaking the core feature.🛠️ Proposed fix
Add the
Autofield to theInstanceNetworkConfigwithin the workflow payload:Os: osConfig, Network: &cwssaws.InstanceNetworkConfig{ Interfaces: data.interfaceConfigs, + Auto: instance.NetworkAuto, }, Infiniband: &cwssaws.InstanceInfinibandConfig{Note: Ensure that
cwssaws.InstanceNetworkConfighas anAutofield defined in the protobuf schema. If not, the proto definition must be updated first (this should be covered in another file in the PR stack).🤖 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 `@api/pkg/api/handler/instancebatch.go` around lines 1624 - 1653, The InstanceNetworkConfig in the CreateInstances workflow payload is missing the persisted Auto flag; update the instance allocation payload building code (the cwssaws.InstanceAllocationRequest -> Config -> InstanceNetworkConfig) to set Auto to the persisted value from the instance (i.e., pass instance.Auto or the equivalent persisted field into cwssaws.InstanceNetworkConfig.Auto) so the CreateInstances workflow can distinguish auto-resolved interfaces; ensure you reference cwssaws.InstanceNetworkConfig and the CreateInstances workflow payload when making this change and confirm the protobuf has an Auto field before setting it.
1236-1260:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPersist the
NetworkAutofield to the database.The handler validates
apiRequest.Autoat line 427 but does not persist it to the database. According to the PR stack context, the DB layer has added anInstance.NetworkAutofield (with migration), and handlers must persist theAutoflag. Without persisting this field, the auto-interface configuration is lost, breaking the feature and preventing partial updates from knowing the instance was created withauto: true.🛠️ Proposed fix
instanceCreateInputs = append(instanceCreateInputs, cdbm.InstanceCreateInput{ Name: generateInstanceName(i), Description: apiRequest.Description, TenantID: tenant.ID, InfrastructureProviderID: machine.InfrastructureProviderID, SiteID: machine.SiteID, VpcID: vpc.ID, MachineID: cdb.GetStrPtr(machine.ID), OperatingSystemID: osID, IpxeScript: apiRequest.IpxeScript, AlwaysBootWithCustomIpxe: *apiRequest.AlwaysBootWithCustomIpxe, PhoneHomeEnabled: *apiRequest.PhoneHomeEnabled, UserData: apiRequest.UserData, NetworkSecurityGroupID: apiRequest.NetworkSecurityGroupID, Labels: apiRequest.Labels, InstanceTypeID: &apiInstanceTypeID, + NetworkAuto: apiRequest.Auto, IsUpdatePending: false, Status: cdbm.InstanceStatusPending, PowerStatus: cdb.GetStrPtr(cdbm.InstancePowerStatusRebooting), CreatedBy: dbUser.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 `@api/pkg/api/handler/instancebatch.go` around lines 1236 - 1260, The InstanceCreateInput being built in instancebatch.go does not set the new NetworkAuto/Auto flag, so persist apiRequest.Auto into the DB by adding the NetworkAuto (or Auto) field on cdbm.InstanceCreateInput when constructing instanceCreateInputs; update the loop that builds instanceCreateInputs (the cdbm.InstanceCreateInput in the for i, machine := range machines block) to include NetworkAuto: apiRequest.Auto (or the correct boolean pointer/type matching cdbm.InstanceCreateInput) so the handler stores the auto-interface configuration.
🧹 Nitpick comments (2)
db/pkg/db/model/instance.go (1)
1130-1141: 💤 Low valuePotential slice header mutation when appending
"updated"column.The expression
cols := append(p.columns, "updated")may mutate the backing array ofp.columnsif it has spare capacity, which could affect the partition's stored slice on subsequent iterations or if the partition is reused. Sincep.columnswas created withappend([]string{}, columns...)at line 1120, this is safe in practice. However, for defensive clarity and to avoid future regressions if the initialization changes, consider an explicit copy.♻️ Defensive copy suggestion
for _, p := range partitions { - cols := append(p.columns, "updated") + cols := make([]string, len(p.columns)+1) + copy(cols, p.columns) + cols[len(p.columns)] = "updated" if _, err := db.GetIDB(tx, isd.dbSession).NewUpdate().🤖 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 `@db/pkg/db/model/instance.go` around lines 1130 - 1141, The current append(cols := append(p.columns, "updated")) can mutate p.columns' backing array; change to build a defensive copy before adding "updated" (e.g., allocate a new slice and copy p.columns into it, then append "updated") so p.columns remains unmodified when iterating partitions; update the code around p.columns/cols used with db.GetIDB(...).NewUpdate().Model(&p.instances).Column(cols...).Bulk().Exec(...) accordingly.db/pkg/db/model/instance_test.go (1)
3128-3266: ⚖️ Poor tradeoffConsider adding test coverage for partition-based bulk update behavior.
The
UpdateMultiplemethod was refactored to partition inputs by their column signature to prevent zero-value overwrites. The existing tests cover same-field updates effectively, but do not exercise the partition split path where different inputs update different column sets (e.g., one input setsNetworkAuto, another does not). A dedicated test case would validate this critical correctness fix.🤖 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 `@db/pkg/db/model/instance_test.go` around lines 3128 - 3266, Add a test case in TestInstanceSQLDAO_UpdateMultiple that forces UpdateMultiple to partition inputs by differing column signatures: create inputs where one InstanceUpdateInput sets a field A (e.g., NetworkAuto or MachineID) and another sets a different field B (e.g., Labels or InstanceTypeID) so their update column sets differ, then call isd.UpdateMultiple(ctx, nil, inputs) and assert no zero-value overwrites occurred (each instance retains only its intended updated fields), the returned slice preserves input order (instance.ID matches inputs[i].InstanceID), and results reflect the per-input updates; reference InstanceUpdateInput and UpdateMultiple to locate where to add the new subtest.
🤖 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 `@api/pkg/api/handler/instance.go`:
- Around line 1568-1573: When building the payload/response struct that sets
Auto from instance.NetworkAuto, avoid including Network.Interfaces when Auto
(instance.NetworkAuto) is true; specifically, only populate Network.Interfaces
if Auto == false (leave it nil/empty otherwise) so the explicit→auto transition
isn't blocked. Update the code path that constructs the Network field (the
section using Auto: instance.NetworkAuto and Network.Interfaces) and apply the
same conditional logic to the other similar construction block elsewhere in this
file (the second occurrence that mirrors create/update behavior).
In `@api/pkg/api/handler/instancebatch.go`:
- Line 427: The batch create path fails to persist and forward the `auto` flag:
set InstanceCreateInput.NetworkAuto from apiRequest.Auto when building the
per-instance inputs (refer to InstanceCreateInput and apiRequest.Auto in
instancebatch.go) so the DB `network_auto` column is populated, and also
populate the workflow payload's InstanceNetworkConfig.Auto (alongside
Interfaces) when constructing the workflow request (refer to
InstanceNetworkConfig.Auto in the workflow payload code); keep the existing
VpcTypeSupportsAutoInterface check as-is since it safely handles nil virtType.
---
Outside diff comments:
In `@api/pkg/api/handler/instancebatch.go`:
- Around line 1624-1653: The InstanceNetworkConfig in the CreateInstances
workflow payload is missing the persisted Auto flag; update the instance
allocation payload building code (the cwssaws.InstanceAllocationRequest ->
Config -> InstanceNetworkConfig) to set Auto to the persisted value from the
instance (i.e., pass instance.Auto or the equivalent persisted field into
cwssaws.InstanceNetworkConfig.Auto) so the CreateInstances workflow can
distinguish auto-resolved interfaces; ensure you reference
cwssaws.InstanceNetworkConfig and the CreateInstances workflow payload when
making this change and confirm the protobuf has an Auto field before setting it.
- Around line 1236-1260: The InstanceCreateInput being built in instancebatch.go
does not set the new NetworkAuto/Auto flag, so persist apiRequest.Auto into the
DB by adding the NetworkAuto (or Auto) field on cdbm.InstanceCreateInput when
constructing instanceCreateInputs; update the loop that builds
instanceCreateInputs (the cdbm.InstanceCreateInput in the for i, machine :=
range machines block) to include NetworkAuto: apiRequest.Auto (or the correct
boolean pointer/type matching cdbm.InstanceCreateInput) so the handler stores
the auto-interface configuration.
---
Nitpick comments:
In `@db/pkg/db/model/instance_test.go`:
- Around line 3128-3266: Add a test case in TestInstanceSQLDAO_UpdateMultiple
that forces UpdateMultiple to partition inputs by differing column signatures:
create inputs where one InstanceUpdateInput sets a field A (e.g., NetworkAuto or
MachineID) and another sets a different field B (e.g., Labels or InstanceTypeID)
so their update column sets differ, then call isd.UpdateMultiple(ctx, nil,
inputs) and assert no zero-value overwrites occurred (each instance retains only
its intended updated fields), the returned slice preserves input order
(instance.ID matches inputs[i].InstanceID), and results reflect the per-input
updates; reference InstanceUpdateInput and UpdateMultiple to locate where to add
the new subtest.
In `@db/pkg/db/model/instance.go`:
- Around line 1130-1141: The current append(cols := append(p.columns,
"updated")) can mutate p.columns' backing array; change to build a defensive
copy before adding "updated" (e.g., allocate a new slice and copy p.columns into
it, then append "updated") so p.columns remains unmodified when iterating
partitions; update the code around p.columns/cols used with
db.GetIDB(...).NewUpdate().Model(&p.instances).Column(cols...).Bulk().Exec(...)
accordingly.
🪄 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: 423c2615-b2f7-4c4c-9702-b807726fc787
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/nico_nico.protois excluded by!workflow-schema/site-agent/workflows/v1/*_nico.proto
📒 Files selected for processing (22)
api/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/vpc.godb/pkg/migrations/20260523061229_instance_network_auto.godb/pkg/util/testing.goopenapi/spec.yamlsdk/standard/model_batch_instance_create_request.gosdk/standard/model_instance.gosdk/standard/model_instance_create_request.gosdk/standard/model_instance_update_request.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
✅ Files skipped from review due to trivial changes (2)
- sdk/standard/model_vpc.go
- sdk/standard/model_vpc_create_request.go
chet
force-pushed
the
feat/flat-vpc-and-auto-interface
branch
from
2af73a3 to
305d38d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
api/pkg/api/handler/instancebatch_test.go (1)
1250-1278: ⚡ Quick winAssert that no workflow is dispatched on this validation failure path.
This case checks the 400/message, but it does not verify the “reject-before-workflow” behavior described in the test comment. Add an explicit assertion that the site Temporal client did not receive
ExecuteWorkflowfor this scenario (with per-subtest mock call reset to avoid cross-test bleed).🤖 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 `@api/pkg/api/handler/instancebatch_test.go` around lines 1250 - 1278, Add an assertion in this subtest that the site temporal client's ExecuteWorkflow was not invoked (to ensure the handler rejected the request before dispatching a workflow) and ensure the mock’s call history is reset per-subtest to avoid bleed; specifically, after invoking the handler for this case assert that the mock for ExecuteWorkflow on the site temporal client (the mocked method ExecuteWorkflow) was never called, and add a mock reset/clear call at the start of each subtest iteration so prior expectations do not carry over.db/pkg/migrations/20260523061229_instance_network_auto.go (1)
49-52: ⚡ Quick winSplit the inline
ExecContextdeclaration out of theif.This new conditional uses the short-declaration form the repo explicitly avoids in Go code. Hoist
errinto a separate statement here as well.Suggested fix
- if _, err := db.ExecContext(ctx, `ALTER TABLE "instance" DROP COLUMN IF EXISTS network_auto`); err != nil { + _, err := db.ExecContext(ctx, `ALTER TABLE "instance" DROP COLUMN IF EXISTS network_auto`) + if err != nil { return err }As per coding guidelines "Split assign-and-condition into two statements; prefer separate
derr := action()andif derr != niloverif derr := action(); derr != nil".🤖 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 `@db/pkg/migrations/20260523061229_instance_network_auto.go` around lines 49 - 52, The code uses a short-declaration inside the if (if _, err := db.ExecContext(ctx, ...); err != nil) which your style guide forbids; change this by calling db.ExecContext(ctx, `ALTER TABLE "instance" DROP COLUMN IF EXISTS network_auto`) into a separate statement (e.g., result, derr := db.ExecContext(...)) and then check derr with if derr != nil return derr, updating variable names to avoid shadowing; ensure you reference db.ExecContext and the err/derr variable used by the migration function to perform the split.db/pkg/db/model/instance.go (1)
1117-1123: ⚡ Quick winSplit the inline short declarations out of these conditionals.
These new
if ... := ...; ...forms violate the repo's Go style rule. Hoisting the assignments keeps this block consistent with the rest of the codebase and makes the control flow easier to scan.Suggested fix
- if _, ok := partitions[signature]; !ok { - partitions[signature] = &partition{ + p := partitions[signature] + if p == nil { + p = &partition{ instances: make([]*Instance, 0), columns: append([]string{}, columns...), } + partitions[signature] = p } - partitions[signature].instances = append(partitions[signature].instances, i) + p.instances = append(p.instances, i) ... for _, p := range partitions { cols := append(p.columns, "updated") - if _, err := db.GetIDB(tx, isd.dbSession).NewUpdate(). + _, err := db.GetIDB(tx, isd.dbSession).NewUpdate(). Model(&p.instances). Column(cols...). Bulk(). - Exec(ctx); err != nil { + Exec(ctx) + if err != nil { return nil, err } } ... - if err := db.GetIDB(tx, isd.dbSession).NewSelect().Model(&result).Where("i.id IN (?)", bun.In(ids)).Scan(ctx); err != nil { + err = db.GetIDB(tx, isd.dbSession).NewSelect().Model(&result).Where("i.id IN (?)", bun.In(ids)).Scan(ctx) + if err != nil { return nil, err }As per coding guidelines "Split assign-and-condition into two statements; prefer separate
derr := action()andif derr != niloverif derr := action(); derr != nil".Also applies to: 1132-1140, 1145-1146
🤖 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 `@db/pkg/db/model/instance.go` around lines 1117 - 1123, Hoist the inline short-declaration out of the conditional: replace constructs like "if _, ok := partitions[signature]; !ok {" with two statements — first assign "_, ok := partitions[signature]" (or "p, ok := partitions[signature]" if you need to reuse the value) and then "if !ok { ... }" so you no longer use the assign-in-if form; apply the same refactor for the other occurrences mentioned (the blocks manipulating partitions, partition, instances, columns and variable i around the subsequent ranges) to conform to the codebase style.db/pkg/db/model/instance_test.go (1)
1587-1587: ⚡ Quick winAdd a DAO regression for
NetworkAutopersistence and mixed-mask updates.This updates the test schema for the new column, but the file still does not prove the two behaviors this PR relies on:
CreateMultiplepersistsNetworkAuto, andUpdateMultiplepreserves an existingnetwork_autovalue when another row in the same batch omits that field. A focused regression here would lock down the new partitioning logic.🤖 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 `@db/pkg/db/model/instance_test.go` at line 1587, Add a regression test that verifies DAO persistence and mixed-mask update semantics for the new NetworkAuto field: write tests that (1) call CreateMultiple with at least one Instance row setting NetworkAuto=true and assert the persisted row has NetworkAuto=true (use the same DAO CreateMultiple helper used elsewhere), and (2) call UpdateMultiple with a batch where one row omits NetworkAuto while another row in the same batch updates other fields, then assert the omitted-row’s existing network_auto value remains unchanged after UpdateMultiple. Target the Instance model (NetworkAuto bool) and the DAO methods CreateMultiple and UpdateMultiple so the test exercises partitioning/masked-update logic.
🤖 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 `@api/pkg/api/handler/instance.go`:
- Line 2802: The code sets NetworkAuto: apiRequest.Auto but leaves old explicit
interfaces in place, causing later logic (where newdbIfcs = existingIfcs when
interfaces are omitted) to return stale interfaces; update the handler so that
when apiRequest.Auto is true you also clear stored interfaces—e.g., delete the
instance's explicit interface rows from the DB or set newdbIfcs = nil/empty when
NetworkAuto is being turned on—so that the branch which falls back to
existingIfcs no longer returns stale data; ensure you modify the same handler
code path that assigns NetworkAuto (and the code that uses
existingIfcs/newdbIfcs) to remove or reset stored interfaces when transitioning
to auto.
In `@db/pkg/db/model/instance.go`:
- Around line 1125-1126: The code currently appends input.InstanceID into ids
and instances without checking for duplicates (see the append calls for
instances and ids), which allows duplicate InstanceIDs in a single batch; before
any partitioning or DB writes, iterate the batch and use a set/map keyed by
input.InstanceID to detect duplicates and return a caller error if any duplicate
is found; apply the same deduplication check to the other similar block around
the append at 1143-1153 so no batch is processed when a duplicate InstanceID
exists.
In `@sdk/standard/model_batch_instance_create_request.go`:
- Around line 90-99: The exported constructor NewBatchInstanceCreateRequest
removed the interfaces parameter causing a breaking change; restore backwards
compatibility by adding a deprecated wrapper that preserves the old signature
(including the interfaces parameter) and delegates to the new constructor and
SetInterfaces; implement this wrapper alongside generator inputs or in
sdk/standard/helpers (not by editing generated file content) and ensure it calls
NewBatchInstanceCreateRequest(...) then invokes SetInterfaces(...) to apply the
passed interfaces before returning the instance.
In `@sdk/standard/model_instance_create_request.go`:
- Around line 89-95: The exported constructor NewInstanceCreateRequest was
changed to remove the interfaces parameter and will break existing callers;
restore source compatibility by adding a deprecated wrapper with the old
signature (e.g., NewInstanceCreateRequest(name string, tenantId string, vpcId
string, interfaces []InterfaceType) *InstanceCreateRequest) that forwards to the
new constructor (or sets the Interfaces field) and marks it deprecated, and
place this wrapper in the generated-output-safe location (generator inputs or
sdk/standard/helpers) rather than editing generated files directly; reference
the existing NewInstanceCreateRequest function and the InstanceCreateRequest
type when implementing the wrapper so callers continue to compile.
---
Nitpick comments:
In `@api/pkg/api/handler/instancebatch_test.go`:
- Around line 1250-1278: Add an assertion in this subtest that the site temporal
client's ExecuteWorkflow was not invoked (to ensure the handler rejected the
request before dispatching a workflow) and ensure the mock’s call history is
reset per-subtest to avoid bleed; specifically, after invoking the handler for
this case assert that the mock for ExecuteWorkflow on the site temporal client
(the mocked method ExecuteWorkflow) was never called, and add a mock reset/clear
call at the start of each subtest iteration so prior expectations do not carry
over.
In `@db/pkg/db/model/instance_test.go`:
- Line 1587: Add a regression test that verifies DAO persistence and mixed-mask
update semantics for the new NetworkAuto field: write tests that (1) call
CreateMultiple with at least one Instance row setting NetworkAuto=true and
assert the persisted row has NetworkAuto=true (use the same DAO CreateMultiple
helper used elsewhere), and (2) call UpdateMultiple with a batch where one row
omits NetworkAuto while another row in the same batch updates other fields, then
assert the omitted-row’s existing network_auto value remains unchanged after
UpdateMultiple. Target the Instance model (NetworkAuto bool) and the DAO methods
CreateMultiple and UpdateMultiple so the test exercises
partitioning/masked-update logic.
In `@db/pkg/db/model/instance.go`:
- Around line 1117-1123: Hoist the inline short-declaration out of the
conditional: replace constructs like "if _, ok := partitions[signature]; !ok {"
with two statements — first assign "_, ok := partitions[signature]" (or "p, ok
:= partitions[signature]" if you need to reuse the value) and then "if !ok { ...
}" so you no longer use the assign-in-if form; apply the same refactor for the
other occurrences mentioned (the blocks manipulating partitions, partition,
instances, columns and variable i around the subsequent ranges) to conform to
the codebase style.
In `@db/pkg/migrations/20260523061229_instance_network_auto.go`:
- Around line 49-52: The code uses a short-declaration inside the if (if _, err
:= db.ExecContext(ctx, ...); err != nil) which your style guide forbids; change
this by calling db.ExecContext(ctx, `ALTER TABLE "instance" DROP COLUMN IF
EXISTS network_auto`) into a separate statement (e.g., result, derr :=
db.ExecContext(...)) and then check derr with if derr != nil return derr,
updating variable names to avoid shadowing; ensure you reference db.ExecContext
and the err/derr variable used by the migration function to perform the split.
🪄 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: 2978aa25-2c2e-4ea4-85d4-aec731a2992a
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/nico_nico.protois excluded by!workflow-schema/site-agent/workflows/v1/*_nico.proto
📒 Files selected for processing (22)
api/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/vpc.godb/pkg/migrations/20260523061229_instance_network_auto.godb/pkg/util/testing.goopenapi/spec.yamlsdk/standard/model_batch_instance_create_request.gosdk/standard/model_instance.gosdk/standard/model_instance_create_request.gosdk/standard/model_instance_update_request.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
✅ Files skipped from review due to trivial changes (5)
- sdk/standard/model_vpc.go
- sdk/standard/model_vpc_create_request.go
- db/pkg/util/testing.go
- sdk/standard/model_instance.go
- sdk/standard/model_instance_update_request.go
chet
force-pushed
the
feat/flat-vpc-and-auto-interface
branch
from
305d38d to
1a46191
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/pkg/api/model/instance_test.go (1)
2470-2520: ⚡ Quick winAdd batch coverage for
secondaryVpcIdsrejection whenauto=true.
APIBatchInstanceCreateRequest.Validate()now rejectssecondaryVpcIdswithauto=true, but this branch is not exercised in this new batch-auto test table.✅ Proposed test case addition
func TestAPIBatchInstanceCreateRequest_Validate_Auto(t *testing.T) { tests := []struct { name string req APIBatchInstanceCreateRequest wantErr bool wantErrorMessage string }{ { name: "auto=true with empty interfaces succeeds", req: APIBatchInstanceCreateRequest{ NamePrefix: "auto-batch", Count: 2, TenantID: uuid.NewString(), InstanceTypeID: uuid.NewString(), VpcID: uuid.NewString(), IpxeScript: cdb.GetStrPtr("test ipxe"), Auto: true, }, wantErr: false, }, + { + name: "auto=true with secondaryVpcIds is rejected", + req: APIBatchInstanceCreateRequest{ + NamePrefix: "auto-batch", + Count: 2, + TenantID: uuid.NewString(), + InstanceTypeID: uuid.NewString(), + VpcID: uuid.NewString(), + IpxeScript: cdb.GetStrPtr("test ipxe"), + Auto: true, + SecondaryVpcIDs: []string{uuid.NewString()}, + }, + wantErr: true, + wantErrorMessage: "`secondaryVpcIds` is not supported when `auto` is true", + }, { name: "auto=true with interfaces is rejected", req: APIBatchInstanceCreateRequest{ NamePrefix: "auto-batch", Count: 2, TenantID: uuid.NewString(), InstanceTypeID: uuid.NewString(), VpcID: uuid.NewString(), IpxeScript: cdb.GetStrPtr("test ipxe"), Auto: true, Interfaces: []APIInterfaceCreateOrUpdateRequest{ {SubnetID: cdb.GetStrPtr(uuid.NewString())}, }, }, wantErr: true, wantErrorMessage: "`interfaces` must be empty when `auto` is true", }, }🤖 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 `@api/pkg/api/model/instance_test.go` around lines 2470 - 2520, The table-driven test TestAPIBatchInstanceCreateRequest_Validate_Auto is missing a case for rejecting SecondaryVpcIDs when Auto is true; add a new test entry in that tests slice where req.Auto = true and req.SecondaryVpcIds (or SecondaryVpcIDs) is non-empty, set wantErr = true and wantErrorMessage to the exact validation message produced by APIBatchInstanceCreateRequest.Validate (e.g. "`secondaryVpcIds` must be empty when `auto` is true"), so the APIBatchInstanceCreateRequest.Validate() branch that rejects secondary VPC IDs is exercised.
🤖 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 `@api/pkg/api/handler/instance.go`:
- Around line 2169-2178: Current code only validates VPC type for
apiRequest.Auto but does not reject a PATCH that supplies interfaces while the
existing instance is still in auto mode (ui.NetworkAuto), which leads to DB/API
divergence because buildInstanceNetworkConfig(ui.NetworkAuto, interfaceConfigs)
will drop those interfaces; update the handler to detect when ui.NetworkAuto is
true and the request includes interfaces while the request does not turn auto
off (i.e. apiRequest.Auto == nil or *apiRequest.Auto == true) and return a 400
error via cutil.NewAPIErrorResponse (similar to the existing VPC check) with a
message like "`interfaces` cannot be set while `auto` is true; disable auto
first or omit interfaces", referencing apiRequest.Interfaces, apiRequest.Auto,
and ui.NetworkAuto so the change is applied before persisting Interface rows.
In `@openapi/spec.yaml`:
- Around line 15794-15796: Update the description for the boolean field named
"auto" to reflect the read-model contract: state that when auto is true the
caller's request-level "interfaces" remained empty and the resolved interfaces
are exposed under "status.network.interfaces" (not the top-level "interfaces");
modify the text in the "auto" field's description to point clients to
"status.network.interfaces" as the source of the resolved set and remove the
current incorrect reference to the top-level "interfaces".
---
Nitpick comments:
In `@api/pkg/api/model/instance_test.go`:
- Around line 2470-2520: The table-driven test
TestAPIBatchInstanceCreateRequest_Validate_Auto is missing a case for rejecting
SecondaryVpcIDs when Auto is true; add a new test entry in that tests slice
where req.Auto = true and req.SecondaryVpcIds (or SecondaryVpcIDs) is non-empty,
set wantErr = true and wantErrorMessage to the exact validation message produced
by APIBatchInstanceCreateRequest.Validate (e.g. "`secondaryVpcIds` must be empty
when `auto` is true"), so the APIBatchInstanceCreateRequest.Validate() branch
that rejects secondary VPC IDs is exercised.
🪄 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: 5eeb2134-9102-413a-8c77-5876d4a432cd
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/nico_nico.protois excluded by!workflow-schema/site-agent/workflows/v1/*_nico.proto
📒 Files selected for processing (23)
api/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/vpc.godb/pkg/migrations/20260523061229_instance_network_auto.godb/pkg/util/testing.goopenapi/spec.yamlsdk/standard/compat/instance_create.gosdk/standard/model_batch_instance_create_request.gosdk/standard/model_instance.gosdk/standard/model_instance_create_request.gosdk/standard/model_instance_update_request.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
✅ Files skipped from review due to trivial changes (4)
- sdk/standard/model_vpc.go
- sdk/standard/model_vpc_create_request.go
- sdk/standard/model_instance_update_request.go
- db/pkg/db/model/instance_test.go
This closes NVIDIA#351, and is the REST-side counterpart to the Flat VPC and `auto` interface work that landed in Core (NVIDIA/infra-controller#1775, NVIDIA/infra-controller#1576, and NVIDIA/infra-controller#1674), which together let tenants put instances on zero-DPU hosts (or hosts with their DPU in NIC mode). The discussion on NVIDIA#351 was originally about whether to model this as `vpcId: null` or a special "unmanaged VPC" type. What landed in Core is closer to the latter -- Flat VPCs are real VPCs with VNIs, NSGs, and peering, but NICo doesn't drive their data plane. How it's done: - Brought the two new fields into `nico_nico.proto`. - `Flat` is a new value in the `network_virtualization_type` enum across the API, DB, OpenAPI, and proto layers. - `auto` is a new boolean on instance create / update that flips the interface configuration model from "explicit list" to "NICo resolves from HostInband segments." Mutually exclusive with `interfaces`, persisted on the instance row so partial updates can re-issue the signal without the caller re-supplying it. - Defense-in-depth check at the REST layer that `auto: true` requires a Flat VPC. Core enforces the same rule, but we fail-fast here too to avoid round-tripping the site for an obviously bad request. This also adds some "VPC capability-like" mappings (similar to what we have in Core) to drive logic decisions at the REST API layer. Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
feat/flat-vpc-and-auto-interface
branch
from
1a46191 to
57bce3e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
db/pkg/migrations/20260523061229_instance_network_auto.go (1)
50-52: ⚡ Quick winSplit inline assignment and condition in down migration.
Line 50 uses
if _, err := ...; err != nil, but this repo’s Go guideline requires separate assignment and error check.Proposed change
- if _, err := db.ExecContext(ctx, `ALTER TABLE "instance" DROP COLUMN IF EXISTS network_auto`); err != nil { - return err - } + _, err := db.ExecContext(ctx, `ALTER TABLE "instance" DROP COLUMN IF EXISTS network_auto`) + if err != nil { + return err + }As per coding guidelines, "Split assign-and-condition into two statements; prefer separate
derr := action()andif derr != niloverif derr := action(); derr != nil."🤖 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 `@db/pkg/migrations/20260523061229_instance_network_auto.go` around lines 50 - 52, Split the inline assignment in the down migration by calling db.ExecContext(ctx, `ALTER TABLE "instance" DROP COLUMN IF EXISTS network_auto`) into a standalone assignment (e.g. derr := db.ExecContext(...)) and then check the error in a separate if statement (if derr != nil { return derr }) so you avoid the `if _, err := ...; err != nil` pattern; locate the call to db.ExecContext in the down migration and replace the inline short declaration with separate assignment and error check using a clear error variable.openapi/spec.yaml (1)
16392-16396: 💤 Low valueClarify the
auto: falsetransition workflow.The description states that
false"returns to explicit interface configuration" but does not specify whetherinterfacesmust be provided in the same update request or may be supplied in a subsequent call. API consumers transitioning from auto to explicit mode would benefit from explicit guidance on the expected workflow.Suggested clarification
auto: type: - boolean - 'null' - description: 'When set, asks NICo to auto-resolve the Instance''s network interfaces from the host''s underlay (HostInband) segments. `null` leaves the value unchanged; `true` (re-)resolves; `false` returns to explicit interface configuration. When `true`, the Instance''s VPC MUST already have `networkVirtualizationType: FLAT`, `interfaces` MUST be empty or omitted, and `secondaryVpcIds` MUST be empty or omitted.' + description: 'When set, asks NICo to auto-resolve the Instance''s network interfaces from the host''s underlay (HostInband) segments. `null` leaves the value unchanged; `true` (re-)resolves; `false` returns to explicit interface configuration and requires `interfaces` to be provided in this or a subsequent update. When `true`, the Instance''s VPC MUST already have `networkVirtualizationType: FLAT`, `interfaces` MUST be empty or omitted, and `secondaryVpcIds` MUST be empty or omitted.'🤖 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 `@openapi/spec.yaml` around lines 16392 - 16396, Update the OpenAPI description for the "auto" property to explicitly state the workflow when setting auto: false: clarify whether the client must include the explicit "interfaces" array in the same update request (and what happens if it is omitted), or whether "interfaces" may be provided in a subsequent request, and describe server behavior/validation (e.g., reject the update unless interfaces are provided, or accept and leave interfaces empty until a follow-up call). Modify the description text for the "auto" field to include this concrete guidance so API consumers know whether to send "interfaces" alongside auto: false or can supply them later.
🤖 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 `@db/pkg/migrations/20260523061229_instance_network_auto.go`:
- Around line 50-52: Split the inline assignment in the down migration by
calling db.ExecContext(ctx, `ALTER TABLE "instance" DROP COLUMN IF EXISTS
network_auto`) into a standalone assignment (e.g. derr := db.ExecContext(...))
and then check the error in a separate if statement (if derr != nil { return
derr }) so you avoid the `if _, err := ...; err != nil` pattern; locate the call
to db.ExecContext in the down migration and replace the inline short declaration
with separate assignment and error check using a clear error variable.
In `@openapi/spec.yaml`:
- Around line 16392-16396: Update the OpenAPI description for the "auto"
property to explicitly state the workflow when setting auto: false: clarify
whether the client must include the explicit "interfaces" array in the same
update request (and what happens if it is omitted), or whether "interfaces" may
be provided in a subsequent request, and describe server behavior/validation
(e.g., reject the update unless interfaces are provided, or accept and leave
interfaces empty until a follow-up call). Modify the description text for the
"auto" field to include this concrete guidance so API consumers know whether to
send "interfaces" alongside auto: false or can supply them later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 31bf27e9-7a83-4aba-94d2-87f64263c7c1
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/nico_nico.protois excluded by!workflow-schema/site-agent/workflows/v1/*_nico.proto
📒 Files selected for processing (23)
api/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/model/instance.goapi/pkg/api/model/instance_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/instance.godb/pkg/db/model/instance_test.godb/pkg/db/model/vpc.godb/pkg/migrations/20260523061229_instance_network_auto.godb/pkg/util/testing.goopenapi/spec.yamlsdk/standard/compat/instance_create.gosdk/standard/model_batch_instance_create_request.gosdk/standard/model_instance.gosdk/standard/model_instance_create_request.gosdk/standard/model_instance_update_request.gosdk/standard/model_vpc.gosdk/standard/model_vpc_create_request.go
✅ Files skipped from review due to trivial changes (3)
- sdk/standard/model_vpc_create_request.go
- sdk/standard/model_vpc.go
- sdk/standard/model_instance.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.
Description
This closes #351, and is the REST-side counterpart to the Flat VPC and
autointerface work that landed in Core (NVIDIA/infra-controller#1775, NVIDIA/infra-controller#1576, and NVIDIA/infra-controller#1674), which together let tenants put instances on zero-DPU hosts (or hosts with their DPU in NIC mode).The discussion on #351 was originally about whether to model this as
vpcId: nullor a special "unmanaged VPC" type. What landed in Core is closer to the latter -- Flat VPCs are real VPCs with VNIs, NSGs, and peering, but NICo doesn't drive their data plane.Changes included:
nico_nico.proto.Flatis a new value in thenetwork_virtualization_typeenum across the API, DB, OpenAPI, and proto layers.autois a new boolean on instance create / update that flips the interface configuration model from "explicit list" to "NICo resolves from HostInband segments." Mutually exclusive withinterfaces, persisted on the instance row so partial updates can re-issue the signal without the caller re-supplying it.auto: truerequires a Flat VPC. Core enforces the same rule, but we fail-fast here too to avoid round-tripping the site for an obviously bad request.This also adds some "VPC capability-like" mappings (similar to what we have in Core) to drive logic decisions at the REST API layer.
Tests added!
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes