chore: Move VPC update/delete proto building onto the DB model#501
chore: Move VPC update/delete proto building onto the DB model#501chet merged 1 commit intoNVIDIA:mainfrom
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis PR refactors VPC identifier and proto construction logic by consolidating scattered utility functions into dedicated methods on the ChangesVPC Model Method Refactoring and Handler Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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-07 17:02:53 UTC | Commit: 2546eb0 |
![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)
api/pkg/api/handler/vpc.go (1)
850-858:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve explicit NSG clears when building
UpdateVPCrequests.
vpc.ToUpdateRequestProto()copiesvpc.NetworkSecurityGroupIDas-is. After the clear path above, that value isnil, soNetworkSecurityGroupIdis omitted on the wire. The metadata activity comment makes the semantics explicit: omitted means “do not touch NSG.” As a result, a request withnetworkSecurityGroupId: ""will clear the DB field but leave the Site-side NSG association intact.Suggested fix
- updateVpcRequest := vpc.ToUpdateRequestProto() + updateVpcRequest := vpc.ToUpdateRequestProto() + if apiRequest.NetworkSecurityGroupID != nil && *apiRequest.NetworkSecurityGroupID == "" { + updateVpcRequest.NetworkSecurityGroupId = cdb.GetStrPtr("") + }🤖 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 850 - 858, vpc.ToUpdateRequestProto() currently omits NetworkSecurityGroupId when vpc.NetworkSecurityGroupID is nil, losing explicit "clear" semantics; ensure the handler inspects the incoming API update (similar to how NVLinkLogicalPartitionID is handled) and when the request explicitly sets NetworkSecurityGroupID to empty string assign updateVpcRequest.NetworkSecurityGroupId = &cwssaws.NetworkSecurityGroupId{Value: ""}, and when non-empty assign the actual ID; modify the code around vpc.ToUpdateRequestProto() to set updateVpcRequest.NetworkSecurityGroupId based on the API input (handling nil, empty string, and non-empty cases) so clears are propagated to the site controller.
🤖 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 `@db/pkg/db/model/vpc_test.go`:
- Around line 1520-1564: Add a new subtest in TestVpc_ToUpdateRequestProto that
verifies explicit NSG removal: create a Vpc with NetworkSecurityGroupID set to a
non-nil pointer to the empty string (e.g. &""), call v.ToUpdateRequestProto(),
and assert that req.NetworkSecurityGroupId is non-nil and its value equals ""
(distinct from the existing nil-omitted case); reference the
TestVpc_ToUpdateRequestProto test and the Vpc.NetworkSecurityGroupID /
ToUpdateRequestProto symbols when adding this case.
---
Outside diff comments:
In `@api/pkg/api/handler/vpc.go`:
- Around line 850-858: vpc.ToUpdateRequestProto() currently omits
NetworkSecurityGroupId when vpc.NetworkSecurityGroupID is nil, losing explicit
"clear" semantics; ensure the handler inspects the incoming API update (similar
to how NVLinkLogicalPartitionID is handled) and when the request explicitly sets
NetworkSecurityGroupID to empty string assign
updateVpcRequest.NetworkSecurityGroupId = &cwssaws.NetworkSecurityGroupId{Value:
""}, and when non-empty assign the actual ID; modify the code around
vpc.ToUpdateRequestProto() to set updateVpcRequest.NetworkSecurityGroupId based
on the API input (handling nil, empty string, and non-empty cases) so clears are
propagated to the site controller.
🪄 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: aa487abc-81a4-441b-a9ca-abb7d5a39287
📒 Files selected for processing (7)
api/pkg/api/handler/subnet.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/vpc.goapi/pkg/api/handler/vpcprefix.godb/pkg/db/model/vpc.godb/pkg/db/model/vpc_test.goworkflow/pkg/activity/vpc/vpc.go
💤 Files with no reviewable changes (1)
- api/pkg/api/handler/util/common/common.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Adds receiver methods on `cdbm.Vpc` so handlers don't have to build update/delete workflow protos inline: - `(*Vpc).ToUpdateRequestProto()` and `ToDeletionRequestProto()` -- used by the Update and Delete handlers respectively (we do something similar for `Tenant`). - `(*Vpc).GetSiteID()`, which replaces the `common.GetSiteVpcID` helper. Tests cover ID-vs-ControllerVpcID fallback, nil description, nil labels, and nil NSG. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
| Vni *int `bun:"vni,type:integer"` | ||
| } | ||
|
|
||
| // GetSiteID returns the VPC ID to use when communicating with the Site: |
There was a problem hiding this comment.
We should have primary receiver function that is simply called ToProto.
Converting API requests to proto should be done in ToProto method of APIVpcCreateRequest and etc.
There was a problem hiding this comment.
Oh sorry! I should have been more clear -- this PR was some general housekeeping I was doing that was going to be shared across vpc/vpcprefix/vpcpeering. Probably made it too small, lol. I've got three PRs related to this that are following up on it.
3 participants
Description
Adds receiver methods on
cdbm.Vpcso handlers don't have to build update/delete workflow protos inline:(*Vpc).ToUpdateRequestProto()andToDeletionRequestProto()-- used by the Update and Delete handlers respectively (we do something similar forTenant).(*Vpc).GetSiteID(), which replaces thecommon.GetSiteVpcIDhelper.Tests cover ID-vs-ControllerVpcID fallback, nil description, nil labels, and nil NSG.
This is part of a wider effort I'm going to slowly be churning on for standardizing
ToProto/FromProtoacross the board (which started with #500, where I'm also dropping anAGENTS.mdwhich mentions something similar).Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes