fix(rest-api): Allow routing-profiles overrides for VPCs - #4502
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. |
Summary by CodeRabbit
WalkthroughThe PR adds REST API support for inline VPC routing-profile overrides and read-only effective routing profiles. It adds validation, tenant-scoped visibility, JSONB persistence, controller inventory synchronization, migrations, tests, and generated-file header scanning. ChangesVPC routing profiles
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant VPCHandler
participant APIModel
participant VPCSQLDAO
participant VPCWorkflow
Client->>VPCHandler: Submit routing-profile overrides
VPCHandler->>APIModel: Validate and serialize overrides
VPCHandler->>VPCSQLDAO: Persist VPC configuration
VPCHandler->>VPCWorkflow: Dispatch VPC workflow
VPCWorkflow-->>VPCHandler: Return effective routing profile
VPCHandler-->>Client: Return tenant-visible VPC response
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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. 🔧 ast-grep (0.45.0)rest-api/docs/index.htmlast-grep skipped this file: it is too large to scan (8441147 bytes) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/workflow/pkg/activity/vpc/vpc.go (1)
174-190: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist
NetworkSecurityGroupIDchangesAdd
!util.PtrsEqual(vpc.NetworkSecurityGroupID, reportedNSGID)toneedsUpdate. IfreportedNSGIDis nil, clear the database field explicitly becauseVpcDAO.Updateignores nil values. Add coverage for attach, change, and clear cases.🤖 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/vpc/vpc.go` around lines 174 - 190, Update needsUpdate in the VPC reconciliation logic to compare vpc.NetworkSecurityGroupID with reportedNSGID using util.PtrsEqual. When reportedNSGID is nil, explicitly clear the persisted NetworkSecurityGroupID before invoking VpcDAO.Update, since nil fields are otherwise ignored. Add coverage for attaching, changing, and clearing the network security group ID.Source: Path instructions
🧹 Nitpick comments (7)
rest-api/api/pkg/api/model/vpc_routing_profile_test.go (2)
158-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
assert.NotContainsfor a clearer failure message.
assert.False(t, strings.Contains(...))reports onlyfalse is not trueon failure.assert.NotContainsprints the offending payload, which shortens diagnosis when theomitemptybehavior regresses.♻️ Proposed assertion change
unprivilegedJSON, err := json.Marshal(unprivileged) require.NoError(t, err) - assert.False(t, strings.Contains(string(unprivilegedJSON), "effectiveRoutingProfile")) + assert.NotContains(t, string(unprivilegedJSON), "effectiveRoutingProfile")Remove the now-unused
stringsimport.🤖 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/api/pkg/api/model/vpc_routing_profile_test.go` around lines 158 - 160, Replace the `assert.False`/`strings.Contains` check in the unprivileged JSON assertion with `assert.NotContains`, passing the serialized payload and `"effectiveRoutingProfile"` directly. Remove the now-unused `strings` import while preserving the existing marshal and error assertion.
21-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert the scenario list into named
t.Runsubtests.This function verifies four independent scenarios in one flat body. The repository test convention requires one top-level
Test...function organized as named table-drivent.Runsubtests. Flat sequencing also means the firstrequirefailure hides the remaining scenarios, which slows diagnosis.The same pattern applies to
TestAPIVpcRoutingProfileOverridesVirtualizationValidation,TestAPIVpcRoutingProfileOverridesToProto, andTestNewAPIVpcEffectiveRoutingProfileVisibilityin this file.♻️ Proposed table-driven structure
func TestAPIVpcRoutingProfileOverridesValidate(t *testing.T) { - // Empty lists, duplicate prefixes, host bits, and both IP families are valid Core inputs. - validProfile := &APIVpcRoutingProfileOverrides{ - RouteTargetImports: &[]APIVpcRouteTarget{ - {ASN: 0, VNI: 0}, - {ASN: int(math.MaxUint32), VNI: int(math.MaxUint32)}, - }, - RouteTargetsOnExports: &[]APIVpcRouteTarget{}, - AcceptedLeaksFromUnderlay: &[]string{"10.0.0.1/24", "10.0.0.1/24", "2001:db8::1/64"}, - AllowedAnycastPrefixes: &[]string{}, - LeakDefaultRouteFromUnderlay: cutil.GetPtr(false), - } - require.NoError(t, validProfile.Validate()) - - // Negative values cannot be represented by the unsigned protobuf fields. - negativeTarget := &APIVpcRoutingProfileOverrides{ - RouteTargetImports: &[]APIVpcRouteTarget{{ASN: -1, VNI: 1}}, - } - require.Error(t, negativeTarget.Validate()) - - // Values above uint32 would otherwise be truncated during conversion. - overflowTarget := &APIVpcRoutingProfileOverrides{ - RouteTargetsOnExports: &[]APIVpcRouteTarget{{ASN: 1, VNI: int(math.MaxUint32) + 1}}, - } - require.Error(t, overflowTarget.Validate()) - - // A malformed prefix must not reach Core's IpNetwork parser. - invalidPrefix := &APIVpcRoutingProfileOverrides{ - AllowedAnycastPrefixes: &[]string{"not-a-prefix"}, - } - require.Error(t, invalidPrefix.Validate()) + tests := []struct { + name string + profile *APIVpcRoutingProfileOverrides + wantErr bool + }{ + { + // Empty lists, duplicate prefixes, host bits, and both IP families are valid Core inputs. + name: "accepts boundary values, empty lists, and both IP families", + profile: &APIVpcRoutingProfileOverrides{ + RouteTargetImports: &[]APIVpcRouteTarget{ + {ASN: 0, VNI: 0}, + {ASN: int(math.MaxUint32), VNI: int(math.MaxUint32)}, + }, + RouteTargetsOnExports: &[]APIVpcRouteTarget{}, + AcceptedLeaksFromUnderlay: &[]string{"10.0.0.1/24", "10.0.0.1/24", "2001:db8::1/64"}, + AllowedAnycastPrefixes: &[]string{}, + LeakDefaultRouteFromUnderlay: cutil.GetPtr(false), + }, + }, + { + // Negative values cannot be represented by the unsigned protobuf fields. + name: "rejects negative ASN", + profile: &APIVpcRoutingProfileOverrides{RouteTargetImports: &[]APIVpcRouteTarget{{ASN: -1, VNI: 1}}}, + wantErr: true, + }, + { + // Values above uint32 would otherwise be truncated during conversion. + name: "rejects VNI above uint32", + profile: &APIVpcRoutingProfileOverrides{RouteTargetsOnExports: &[]APIVpcRouteTarget{{ASN: 1, VNI: int(math.MaxUint32) + 1}}}, + wantErr: true, + }, + { + // A malformed prefix must not reach Core's IpNetwork parser. + name: "rejects malformed prefix", + profile: &APIVpcRoutingProfileOverrides{AllowedAnycastPrefixes: &[]string{"not-a-prefix"}}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.profile.Validate() + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } }🤖 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/api/pkg/api/model/vpc_routing_profile_test.go` around lines 21 - 52, Convert the independent scenarios in TestAPIVpcRoutingProfileOverridesValidate into descriptive named t.Run subtests so each validation case executes and reports separately. Apply the same named table-driven subtest structure to TestAPIVpcRoutingProfileOverridesVirtualizationValidation, TestAPIVpcRoutingProfileOverridesToProto, and TestNewAPIVpcEffectiveRoutingProfileVisibility, preserving each scenario’s existing assertions and behavior.Source: Path instructions
rest-api/api/pkg/api/handler/vpc.go (2)
1288-1293: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGuard the capability lookup on the VPC type, as the update handler does.
This resolves
TargetedInstanceCreationon every single-VPC GET. For a VPC whose type does not support routing profiles,EffectiveRoutingProfileis always nil andNewAPIVpcomits the field regardless of the flag, so the query result cannot change the response. GET by ID is a hot path, and ETHERNET_VIRTUALIZER and FLAT VPCs are likely the majority.
UpdateVPCHandler.Handle(lines 607-614) already uses the guarded form. Apply the same pattern here for consistency and to remove the redundant round trip.♻️ Proposed guarded lookup
- tenantCanViewEffectiveRoutingProfile, err := common.TenantHasTargetedInstanceCreation(ctx, nil, gvh.dbSession, tenant, &common.TenantPrivilegeScope{SiteID: &vpc.SiteID}) - if err != nil { - logger.Error().Err(err).Msg("error resolving TargetedInstanceCreation for Tenant/Site") - return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to resolve Tenant capability, DB error", nil) - } + tenantCanViewEffectiveRoutingProfile := false + if cdbm.VpcTypeSupportsRoutingProfile(vpc.NetworkVirtualizationType) { + tenantCanViewEffectiveRoutingProfile, err = common.TenantHasTargetedInstanceCreation(ctx, nil, gvh.dbSession, tenant, &common.TenantPrivilegeScope{SiteID: &vpc.SiteID}) + if err != nil { + logger.Error().Err(err).Msg("error resolving TargetedInstanceCreation for Tenant/Site") + return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to resolve Tenant capability, DB error", 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 `@rest-api/api/pkg/api/handler/vpc.go` around lines 1288 - 1293, Guard the TenantHasTargetedInstanceCreation call in the single-VPC GET handler using the VPC-type condition already applied by UpdateVPCHandler.Handle. Only perform the lookup for VPC types that support routing profiles, while preserving the existing error handling and capability value for eligible types.
237-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeparate the two concerns that share
tenantCanViewEffectiveRoutingProfile.This one variable gates two distinct decisions: whether the tenant may set
routingProfile(line 255, a write authorization), and whether the response may exposeeffectiveRoutingProfile(line 487, a read authorization). Both currently derive fromTargetedInstanceCreation, so the behavior is correct, but the name describes only the read concern. A reader auditing the write gate at line 255 sees a variable named after response visibility.Introduce a second, explicitly named binding so each gate reads as its own decision and the two can diverge later without touching the call sites.
♻️ Proposed clarification
var routingProfile *string if apiRequest.RoutingProfile != nil { // For now, we gate on TargetedInstanceCreation permission, // which must be effective for the VPC's Site. - if !tenantCanViewEffectiveRoutingProfile { + tenantCanSetRoutingProfile := tenantCanViewEffectiveRoutingProfile + if !tenantCanSetRoutingProfile { logger.Warn().Msg("tenant does not have sufficient privileges to set `routingProfile`") return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Tenant does not have sufficient privileges to set `routingProfile`", 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 `@rest-api/api/pkg/api/handler/vpc.go` around lines 237 - 258, Introduce a separate explicitly named authorization binding for the `routingProfile` write check in the handler, while retaining `tenantCanViewEffectiveRoutingProfile` for `effectiveRoutingProfile` response visibility. Update the `apiRequest.RoutingProfile` gate to use the write-oriented binding, and keep both values derived from the existing `TargetedInstanceCreation` result without changing behavior.rest-api/db/pkg/db/model/vpc_routing_profile_test.go (2)
106-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a case proving an omitted profile field preserves its stored value.
This update sets
RoutingProfileOverridesandEffectiveRoutingProfiletogether, so it cannot detect a regression in the conditional column selection added atrest-api/db/pkg/db/model/vpc.golines 941-950. If either field were unconditionally appended toupdatedFields, an update that omits it would null the column and this test would still pass.Add an update that supplies only one field and assert the other is unchanged.
💚 Proposed preservation case
persisted, err = dao.GetByID(ctx, nil, created.ID, nil) require.NoError(t, err) assert.Equal(t, replacementOverrides, persisted.RoutingProfileOverrides) assert.Equal(t, effectiveProfile, persisted.EffectiveRoutingProfile) + // An update that omits a profile field must preserve the stored value. + overridesOnly := &VpcRoutingProfileOverrides{LeakTenantHostRoutesToUnderlay: cutil.GetPtr(true)} + _, err = dao.Update(ctx, nil, VpcUpdateInput{ + VpcID: created.ID, + RoutingProfileOverrides: overridesOnly, + }) + require.NoError(t, err) + persisted, err = dao.GetByID(ctx, nil, created.ID, nil) + require.NoError(t, err) + assert.Equal(t, overridesOnly, persisted.RoutingProfileOverrides) + assert.Equal(t, effectiveProfile, persisted.EffectiveRoutingProfile) + // Clear removes both cached values, and a fresh lookup proves both columns are NULL.🤖 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/db/pkg/db/model/vpc_routing_profile_test.go` around lines 106 - 118, Extend the update test around dao.Update to add a second update supplying only one of RoutingProfileOverrides or EffectiveRoutingProfile, then reload the record and assert the omitted field retains its previously persisted value while the supplied field changes as expected. Ensure the case exercises conditional column selection in the VPC update path.Source: Coding guidelines
31-47: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a case with nil slices in the effective profile.
Every slice field in this fixture is non-nil, so the test cannot distinguish "preserved" from "normalized to empty".
vpcPrefixesFromProtoandvpcRouteTargetsFromProtoalways allocate, which means an effective profile arriving from Core with no prefixes becomes[]rather thannilafterFromProto. That normalization is load-bearing: it is what keeps API responses from emittingnullfor the required arrays declared inrest-api/openapi/spec.yaml.Assert it explicitly so a future change to the helpers cannot silently reintroduce nil slices.
💚 Proposed additional assertions
// An omitted config/status value clears stale cached routing-profile state. got.FromProto(&corev1.Vpc{Id: &corev1.VpcId{Value: original.ID.String()}}) assert.Nil(t, got.RoutingProfileOverrides) assert.Nil(t, got.EffectiveRoutingProfile) + + // A Core-reported effective profile without lists normalizes to empty, never nil, + // so API responses satisfy the required non-nullable array contract. + sparse := &Vpc{} + sparse.FromProto(&corev1.Vpc{ + Id: &corev1.VpcId{Value: original.ID.String()}, + Status: &corev1.VpcStatus{EffectiveRoutingProfile: &corev1.VpcEffectiveRoutingProfile{Internal: true}}, + }) + require.NotNil(t, sparse.EffectiveRoutingProfile) + assert.NotNil(t, sparse.EffectiveRoutingProfile.RouteTargetImports) + assert.Empty(t, sparse.EffectiveRoutingProfile.RouteTargetImports) + assert.NotNil(t, sparse.EffectiveRoutingProfile.AcceptedLeaksFromUnderlay) + assert.Empty(t, sparse.EffectiveRoutingProfile.AcceptedLeaksFromUnderlay) + assert.NotNil(t, sparse.EffectiveRoutingProfile.AllowedAnycastPrefixes) + assert.Empty(t, sparse.EffectiveRoutingProfile.AllowedAnycastPrefixes) }🤖 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/db/pkg/db/model/vpc_routing_profile_test.go` around lines 31 - 47, Add a separate round-trip test case for an effective profile whose slice fields are nil, covering all relevant prefix and route-target slices. Convert it through Vpc.ToProto and Vpc.FromProto, then assert those fields become non-nil empty slices while preserving the profile’s scalar values; keep the existing non-empty fixture assertions unchanged.rest-api/db/pkg/db/model/vpc.go (1)
156-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove per-element route-target conversion onto
VpcRouteTarget.
VpcRouteTargetis a domain type that round-trips withcorev1.RouteTarget, but its conversion logic lives in free helper functions. The repository convention places protobuf conversion on the owning type as receiver methods, and reserves free functions for plumbing. Receiver methods also make the mapping discoverable from the type definition.Keep the slice helpers as thin loops that delegate.
♻️ Proposed receiver-based conversion
+// ToProto converts a persisted route target to its Core wire representation. +func (target VpcRouteTarget) ToProto() *corev1.RouteTarget { + return &corev1.RouteTarget{Asn: target.ASN, Vni: target.VNI} +} + +// FromProto populates a persisted route target from its Core wire representation. +func (target *VpcRouteTarget) FromProto(protoTarget *corev1.RouteTarget) { + target.ASN = protoTarget.GetAsn() + target.VNI = protoTarget.GetVni() +} + // vpcRouteTargetsToProto converts persisted route targets to their Core wire representation. func vpcRouteTargetsToProto(targets []VpcRouteTarget) []*corev1.RouteTarget { protoTargets := make([]*corev1.RouteTarget, 0, len(targets)) for _, target := range targets { - protoTargets = append(protoTargets, &corev1.RouteTarget{Asn: target.ASN, Vni: target.VNI}) + protoTargets = append(protoTargets, target.ToProto()) } return protoTargets } // vpcRouteTargetsFromProto converts Core route targets to their persisted representation. func vpcRouteTargetsFromProto(targets []*corev1.RouteTarget) []VpcRouteTarget { dbTargets := make([]VpcRouteTarget, 0, len(targets)) - for _, target := range targets { - dbTargets = append(dbTargets, VpcRouteTarget{ASN: target.GetAsn(), VNI: target.GetVni()}) + for _, protoTarget := range targets { + dbTarget := VpcRouteTarget{} + dbTarget.FromProto(protoTarget) + dbTargets = append(dbTargets, dbTarget) } return dbTargets }Consider applying the same pattern in
rest-api/api/pkg/api/model/vpc.goforapiVpcRouteTargetsToDBandapiVpcRouteTargetsFromDB.🤖 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/db/pkg/db/model/vpc.go` around lines 156 - 172, Move the per-element protobuf mapping from vpcRouteTargetsToProto and vpcRouteTargetsFromProto onto VpcRouteTarget as receiver conversion methods, following the repository’s existing model convention. Keep both slice helpers as thin loops that call those methods, and apply the same receiver-based pattern to apiVpcRouteTargetsToDB and apiVpcRouteTargetsFromDB if the corresponding type is defined there.Source: Coding guidelines
🤖 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/vpc.go`:
- Around line 235-236: Normalize the nil results from slices.Clone in the
profile conversion by ensuring AcceptedLeaksFromUnderlay and
AllowedAnycastPrefixes are always initialized as empty slices when their
database values are nil. Update the assignments in the profile-mapping function
to match the always-allocated behavior of apiVpcRouteTargetsFromDB while
preserving cloned values for non-nil sources.
---
Outside diff comments:
In `@rest-api/workflow/pkg/activity/vpc/vpc.go`:
- Around line 174-190: Update needsUpdate in the VPC reconciliation logic to
compare vpc.NetworkSecurityGroupID with reportedNSGID using util.PtrsEqual. When
reportedNSGID is nil, explicitly clear the persisted NetworkSecurityGroupID
before invoking VpcDAO.Update, since nil fields are otherwise ignored. Add
coverage for attaching, changing, and clearing the network security group ID.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/vpc.go`:
- Around line 1288-1293: Guard the TenantHasTargetedInstanceCreation call in the
single-VPC GET handler using the VPC-type condition already applied by
UpdateVPCHandler.Handle. Only perform the lookup for VPC types that support
routing profiles, while preserving the existing error handling and capability
value for eligible types.
- Around line 237-258: Introduce a separate explicitly named authorization
binding for the `routingProfile` write check in the handler, while retaining
`tenantCanViewEffectiveRoutingProfile` for `effectiveRoutingProfile` response
visibility. Update the `apiRequest.RoutingProfile` gate to use the
write-oriented binding, and keep both values derived from the existing
`TargetedInstanceCreation` result without changing behavior.
In `@rest-api/api/pkg/api/model/vpc_routing_profile_test.go`:
- Around line 158-160: Replace the `assert.False`/`strings.Contains` check in
the unprivileged JSON assertion with `assert.NotContains`, passing the
serialized payload and `"effectiveRoutingProfile"` directly. Remove the
now-unused `strings` import while preserving the existing marshal and error
assertion.
- Around line 21-52: Convert the independent scenarios in
TestAPIVpcRoutingProfileOverridesValidate into descriptive named t.Run subtests
so each validation case executes and reports separately. Apply the same named
table-driven subtest structure to
TestAPIVpcRoutingProfileOverridesVirtualizationValidation,
TestAPIVpcRoutingProfileOverridesToProto, and
TestNewAPIVpcEffectiveRoutingProfileVisibility, preserving each scenario’s
existing assertions and behavior.
In `@rest-api/db/pkg/db/model/vpc_routing_profile_test.go`:
- Around line 106-118: Extend the update test around dao.Update to add a second
update supplying only one of RoutingProfileOverrides or EffectiveRoutingProfile,
then reload the record and assert the omitted field retains its previously
persisted value while the supplied field changes as expected. Ensure the case
exercises conditional column selection in the VPC update path.
- Around line 31-47: Add a separate round-trip test case for an effective
profile whose slice fields are nil, covering all relevant prefix and
route-target slices. Convert it through Vpc.ToProto and Vpc.FromProto, then
assert those fields become non-nil empty slices while preserving the profile’s
scalar values; keep the existing non-empty fixture assertions unchanged.
In `@rest-api/db/pkg/db/model/vpc.go`:
- Around line 156-172: Move the per-element protobuf mapping from
vpcRouteTargetsToProto and vpcRouteTargetsFromProto onto VpcRouteTarget as
receiver conversion methods, following the repository’s existing model
convention. Keep both slice helpers as thin loops that call those methods, and
apply the same receiver-based pattern to apiVpcRouteTargetsToDB and
apiVpcRouteTargetsFromDB if the corresponding type is defined there.
🪄 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: 6a8b95c2-835b-4614-93f0-9823df0c35c6
⛔ Files ignored due to path filters (6)
rest-api/sdk/standard/model_vpc.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_effective_routing_profile.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_route_target.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_routing_profile_overrides.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (12)
rest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/model/vpc.gorest-api/api/pkg/api/model/vpc_routing_profile_test.gorest-api/api/pkg/api/model/vpc_test.gorest-api/db/pkg/db/model/vpc.gorest-api/db/pkg/db/model/vpc_routing_profile_test.gorest-api/db/pkg/migrations/20260731215443_vpc_routing_profiles.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/workflow/pkg/activity/vpc/vpc.gorest-api/workflow/pkg/activity/vpc/vpc_test.go
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
a5a1f70 to
a3fa8cd
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4502.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rest-api/api/pkg/api/model/vpc.go (1)
143-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove route-target collection conversion to a receiver method.
apiVpcRouteTargetsToDBandapiVpcRouteTargetsFromDBare free conversion functions. Define a named route-target collection type and give it DB conversion receiver methods. Keep the current nil and empty-list semantics.As per coding guidelines, use “receiver methods such as
ToProto,FromProto,ToDBModel, andFromDBModelfor model conversions instead of free conversion functions.”🤖 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/api/pkg/api/model/vpc.go` around lines 143 - 160, Replace the free functions apiVpcRouteTargetsToDB and apiVpcRouteTargetsFromDB with a named route-target collection type and receiver methods for DB conversion, following the existing ToDBModel/FromDBModel conventions. Update callers to use the new methods while preserving the current nil and empty-list behavior.Source: Coding guidelines
🤖 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/workflow/pkg/activity/vpc/vpc_test.go`:
- Around line 734-743: Extend the assertions in the routing-profile update
verification block guarded by tt.routingProfileStateUpdatedVpc to validate every
reported profile value from the fixture: AllowedAnycastPrefixes,
EffectiveRoutingProfile.LeakDefaultRouteFromUnderlay, and
EffectiveRoutingProfile.Internal, in addition to the existing fields. Compare
each value with the fixture’s expected values so converter omissions fail the
test, then run the Go REST API unit tests through the repository’s rest-test
workflow.
---
Nitpick comments:
In `@rest-api/api/pkg/api/model/vpc.go`:
- Around line 143-160: Replace the free functions apiVpcRouteTargetsToDB and
apiVpcRouteTargetsFromDB with a named route-target collection type and receiver
methods for DB conversion, following the existing ToDBModel/FromDBModel
conventions. Update callers to use the new methods while preserving the current
nil and empty-list behavior.
🪄 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: a21bd445-9acf-48f5-a661-262f340d176e
⛔ Files ignored due to path filters (6)
rest-api/sdk/standard/model_vpc.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_effective_routing_profile.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_route_target.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_routing_profile_overrides.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (12)
rest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/model/vpc.gorest-api/api/pkg/api/model/vpc_routing_profile_test.gorest-api/api/pkg/api/model/vpc_test.gorest-api/db/pkg/db/model/vpc.gorest-api/db/pkg/db/model/vpc_routing_profile_test.gorest-api/db/pkg/migrations/20260731215443_vpc_routing_profiles.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/workflow/pkg/activity/vpc/vpc.gorest-api/workflow/pkg/activity/vpc/vpc_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- rest-api/api/pkg/api/model/vpc_test.go
- rest-api/db/pkg/db/model/vpc_routing_profile_test.go
- rest-api/workflow/pkg/activity/vpc/vpc.go
- rest-api/db/pkg/migrations/20260731215443_vpc_routing_profiles.go
- rest-api/openapi/spec.yaml
- rest-api/api/pkg/api/handler/vpc.go
- rest-api/db/pkg/db/model/vpc.go
- rest-api/api/pkg/api/model/vpc_routing_profile_test.go
- rest-api/api/pkg/api/handler/vpc_test.go
🔐 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-08-03 20:06:23 UTC | Commit: a3fa8cd |
a3fa8cd to
e594679
Compare
e594679 to
81a5501
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rest-api/db/pkg/db/model/vpc_test.go (1)
1330-1348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the implicit ordering dependency on the table loop.
This subtest asserts that
effectiveRoutingProfilesurvives a partial update. The expected value exists only because the preceding table case wrote it to the samevpc.IDrow. If a table case is reordered, renamed, or skipped, this subtest fails for a reason unrelated to the behaviour it verifies. Seed the required state inside the subtest so the assertion is self-contained.♻️ Suggested change
t.Run("preserves an omitted effective routing profile", func(t *testing.T) { // Updating desired overrides alone must not clear cached controller state. + _, err := NewVpcDAO(dbSession).Update(ctx, nil, VpcUpdateInput{ + VpcID: vpc.ID, + EffectiveRoutingProfile: effectiveRoutingProfile, + }) + require.NoError(t, err) + replacementOverrides := &VpcRoutingProfileOverrides{ AllowedAnycastPrefixes: &[]string{"192.0.2.0/24"}, } - updated, err := NewVpcDAO(dbSession).Update(ctx, nil, VpcUpdateInput{ + updated, err := NewVpcDAO(dbSession).Update(ctx, nil, VpcUpdateInput{ VpcID: vpc.ID, RoutingProfileOverrides: replacementOverrides, })🤖 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/db/pkg/db/model/vpc_test.go` around lines 1330 - 1348, Make the “preserves an omitted effective routing profile” subtest self-contained by explicitly seeding effectiveRoutingProfile for vpc.ID before calling Update. Do not rely on state written by an earlier table case; keep the existing partial-update and persisted-value assertions unchanged.rest-api/api/pkg/api/model/vpc_test.go (1)
430-479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the omitted-field half of the presence contract.
Both tests cover present values and explicitly empty lists. Neither test asserts that an omitted field stays
nilafter conversion. That assertion is the distinguishing property of the presence-aware design:nilinherits from the named profile, while a present empty list replaces it. A regression that allocated empty slices for omitted fields would pass the current assertions.The path instructions also ask for named table-driven
t.Runsubtests around the method under test. Consider grouping the cases accordingly.♻️ Suggested additional assertions
require.NotNil(t, dbProfile.AcceptedLeaksFromUnderlay) assert.Empty(t, *dbProfile.AcceptedLeaksFromUnderlay) + // Omitted fields must inherit from the named profile. + assert.Nil(t, dbProfile.LeakTenantHostRoutesToUnderlay) + assert.Nil(t, dbProfile.RouteTargetsOnExports == nil) }Apply the equivalent
assert.Nilchecks for the omitted fields inTestAPIVpcRoutingProfileOverrides_FromDB, and add a case that passes anildbProfile.🤖 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/api/pkg/api/model/vpc_test.go` around lines 430 - 479, Extend TestAPIVpcRoutingProfileOverrides_ToDB and TestAPIVpcRoutingProfileOverrides_FromDB with named table-driven t.Run cases covering omitted fields, asserting omitted optional values remain nil while present empty lists remain non-nil and empty. In the FromDB tests, also cover a nil dbProfile input and assert the API result remains appropriate without allocating omitted fields.Source: Path instructions
rest-api/api/pkg/api/model/vpc.go (1)
169-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
ToDBandFromDBtoToDBModelandFromDBModel.Use the same conversion method names as the route-target models in this file and update their call sites and tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/model/vpc.go` around lines 169 - 201, Rename APIVpcRoutingProfileOverrides.ToDB and FromDB to ToDBModel and FromDBModel, matching the route-target conversion methods in this file. Update every call site and affected test to use the new method names while preserving the existing conversion behavior.Source: Coding guidelines
🤖 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/api/pkg/api/model/vpc_test.go`:
- Around line 430-479: Extend TestAPIVpcRoutingProfileOverrides_ToDB and
TestAPIVpcRoutingProfileOverrides_FromDB with named table-driven t.Run cases
covering omitted fields, asserting omitted optional values remain nil while
present empty lists remain non-nil and empty. In the FromDB tests, also cover a
nil dbProfile input and assert the API result remains appropriate without
allocating omitted fields.
In `@rest-api/api/pkg/api/model/vpc.go`:
- Around line 169-201: Rename APIVpcRoutingProfileOverrides.ToDB and FromDB to
ToDBModel and FromDBModel, matching the route-target conversion methods in this
file. Update every call site and affected test to use the new method names while
preserving the existing conversion behavior.
In `@rest-api/db/pkg/db/model/vpc_test.go`:
- Around line 1330-1348: Make the “preserves an omitted effective routing
profile” subtest self-contained by explicitly seeding effectiveRoutingProfile
for vpc.ID before calling Update. Do not rely on state written by an earlier
table case; keep the existing partial-update and persisted-value assertions
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a21a9df3-ad89-4f58-ac88-cac6de1459d6
⛔ Files ignored due to path filters (6)
rest-api/sdk/standard/model_vpc.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_effective_routing_profile.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_route_target.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_routing_profile_overrides.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (13)
rest-api/Makefilerest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/model/vpc.gorest-api/api/pkg/api/model/vpc_test.gorest-api/db/pkg/db/model/vpc.gorest-api/db/pkg/db/model/vpc_test.gorest-api/db/pkg/migrations/20260731215443_vpc_routing_profiles.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/scripts/check_source_headers.pyrest-api/workflow/pkg/activity/vpc/vpc.gorest-api/workflow/pkg/activity/vpc/vpc_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- rest-api/db/pkg/migrations/20260731215443_vpc_routing_profiles.go
- rest-api/workflow/pkg/activity/vpc/vpc.go
- rest-api/workflow/pkg/activity/vpc/vpc_test.go
- rest-api/Makefile
- rest-api/openapi/spec.yaml
- rest-api/api/pkg/api/handler/vpc.go
- rest-api/db/pkg/db/model/vpc.go
- rest-api/api/pkg/api/handler/vpc_test.go
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Looks good @bcavnvidia!
4 participants
#4411 and #4463 added support for inline VPC routing-profiles overrides.
This PR exposes the support in the REST layer.
Related issues
#2624
Type of Change
Breaking Changes
Testing
Additional Notes
Closes #2624