feat(rest-api): Enhance get-all VPC peering filtering and peer tenant visibility#2867
feat(rest-api): Enhance get-all VPC peering filtering and peer tenant visibility#2867hwadekar-nv wants to merge 8 commits into
Conversation
|
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:
WalkthroughThe PR adds VPC peering list filters for status, VPC ID, and peer tenant ID. It also returns tenant IDs for both peered VPCs and filters peerings by either side’s owning tenant. ChangesVPC peering API and persistence updates
Tenant listing pagination and filters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
hwadekar-nv
force-pushed
the
feat/enhance-vpc-peering
branch
from
4c4191a to
55e5ce7
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2867.docs.buildwithfern.com/infra-controller |
🔍 Container Scan SummaryNo Grype artifacts were found to aggregate. |
🔐 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-06-25 00:07:22 UTC | Commit: 4c4191a |
|
Exposing |
hwadekar-nv
force-pushed
the
feat/enhance-vpc-peering
branch
from
bfa3732 to
d9962df
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/handler/vpcpeering.go`:
- Around line 614-621: The relation preload logic in the VPC peering handler is
too narrowly gated by filterInput.VpcIDs and filterInput.PeerTenantIDs, which
can leave vpc1TenantId and vpc2TenantId unset in normal list responses. Update
the includeRelations handling in the vpc peering list path so Vpc1RelationName
and Vpc2RelationName are always loaded whenever the response includes tenant ID
fields, keeping the response consistent with the OpenAPI contract and preserving
compatibility.
- Around line 563-578: The peer tenant query handling in the VPC peering handler
only reads a single `peerTenantId`, so repeated query values are dropped and the
`PeerTenantIDs` filter is not populated consistently. Update the `Get`/query
parsing logic in `vpcpeering.go` to read all `peerTenantId` values from the
request, resolve each one through `common.GetTenantFromIDString`, and append
valid tenant IDs into `filterInput.PeerTenantIDs` while preserving the existing
invalid/not-found/error responses and `logger.Error` 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: 882a2acf-2c0d-4f35-a284-ff78d73528ab
⛔ Files ignored due to path filters (33)
rest-api/sdk/standard/api_vpc_peering.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_batch_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_machine.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_machine_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-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_peering.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 (7)
rest-api/api/pkg/api/handler/vpcpeering.gorest-api/api/pkg/api/handler/vpcpeering_test.gorest-api/api/pkg/api/model/vpcpeering.gorest-api/api/pkg/api/model/vpcpeering_test.gorest-api/db/pkg/db/model/vpcpeering.gorest-api/db/pkg/db/model/vpcpeering_test.gorest-api/openapi/spec.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- rest-api/db/pkg/db/model/vpcpeering_test.go
- rest-api/openapi/spec.yaml
- rest-api/api/pkg/api/handler/vpcpeering_test.go
- rest-api/api/pkg/api/model/vpcpeering.go
- rest-api/db/pkg/db/model/vpcpeering.go
- rest-api/api/pkg/api/model/vpcpeering_test.go
| in: query | ||
| name: vpcId | ||
| required: false | ||
| description: Optional filter by VPC ID involved in the peering as either vpc1 or vpc2. |
There was a problem hiding this comment.
For consistency, we should state that this can be repeated multiple times.
| OrderBy: pageRequest.OrderBy, | ||
| } | ||
| vpcPeerings, total, err := vpcPeeringDAO.GetAll(ctx, nil, filterInput, vpcPeeringPageInput, qIncludeRelations) | ||
| includeRelations := vpcPeeringIncludeRelationsForTenantIDs(qIncludeRelations) |
There was a problem hiding this comment.
Why are we sending the whole of vpc1 and vpc2 back in the response even when the client didn't ask for them? Agreed that we need to look at the VPCs to get the tenant values, but that doesn't mean we need to send all that extra information back unexpectedly.
There was a problem hiding this comment.
The implementation has been changed and now lives on lines 655-660, but the question still stands
| expectedCount: 1, | ||
| }, | ||
| { | ||
| name: "tenant admin 1 filters by pending status", |
There was a problem hiding this comment.
It might be more useful to validate that filtering by multiple statuses works
| name: "tenant admin 1 filters by vpcId", | ||
| reqOrgName: tnOrg1, | ||
| queryParams: map[string]string{"vpcId": vpc1.ID.String()}, | ||
| user: tnu1, | ||
| expectedStatus: http.StatusOK, | ||
| expectedCount: 3, | ||
| }, |
There was a problem hiding this comment.
Since all the created peerings have vpc1 as the first vpc, this doesn't really help test that the filter works regardless of whether the vpc specified in the filter is the first vpc or the second. It would be good to have a test case that covers that.
|
@nvlitagaki Thanks for reviewing it, for adding more readable filter by Tenant such as either |
My thought is that if they already have a close enough relationship with the other tenant that they are giving them access to at least one of their VPCs, then it's OK for them to know how that tenant is listed as an org. I also don't know how else they can realistically keep track of which tenant is which if they have peering relationships with multiple other tenants. |
| if vpcIDStrs := qParams["vpcId"]; len(vpcIDStrs) != 0 { | ||
| gavph.tracerSpan.SetAttribute(handlerSpan, attribute.StringSlice("vpcId", vpcIDStrs), logger) | ||
| for _, vpcIDStr := range vpcIDStrs { | ||
| vpc, verr := common.GetVpcFromIDString(ctx, nil, vpcIDStr, nil, gavph.dbSession) |
There was a problem hiding this comment.
We should use a single query to fetch all VPCs and load in map to validate. Other endpoints have established patterns.
| if peerTenantIDStrs := qParams["peerTenantId"]; len(peerTenantIDStrs) != 0 { | ||
| gavph.tracerSpan.SetAttribute(handlerSpan, attribute.StringSlice("peerTenantId", peerTenantIDStrs), logger) | ||
| for _, peerTenantIDStr := range peerTenantIDStrs { | ||
| peerTenant, verr := common.GetTenantFromIDString(ctx, nil, peerTenantIDStr, gavph.dbSession) |
There was a problem hiding this comment.
We should use a single query to fetch all Tenants and load in map to validate.
hwadekar-nv
force-pushed
the
feat/enhance-vpc-peering
branch
2 times, most recently
from
e58f637 to
8e91569
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rest-api/api/pkg/api/handler/vpcpeering.go (1)
532-542: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEcho the offending status value in the error response.
The
vpcIdandpeerTenantIdpaths surface the invalid value (Invalid VPC ID %v in query), but thestatuspath returns the generic"Invalid Status value in query". For both diagnosability and consistency across the three new filters, include the rejected value as is already logged on line 537.📝 Proposed fix
if !cdbm.VpcPeeringStatusMap[status] { logger.Warn().Msg(fmt.Sprintf("invalid value in status query: %v", status)) - return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Invalid Status value in query", nil) + return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, fmt.Sprintf("Invalid status value %v in query", status), nil) }Note: this mirrors
nvlitagaki's earlier request on line 538.🤖 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/vpcpeering.go` around lines 532 - 542, The status query validation in vpcpeering handler returns a generic bad-request message instead of echoing the rejected value like the vpcId and peerTenantId checks. Update the invalid-status branch in the status filter logic to include the offending status string in the API error response, matching the existing logger.Warn output and the pattern used by the other filter validations.
🧹 Nitpick comments (2)
rest-api/db/pkg/db/model/tenant.go (1)
202-205: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winApply the explicit empty-filter contract to every filter slice.
TenantIDs: []uuid.UUID{}returns zero rows, butOrgs: []string{}andOrgDisplayNames: []string{}still continue into query construction. Please short-circuit all non-nil empty filters consistently.Proposed fix
- if filter.TenantIDs != nil && len(filter.TenantIDs) == 0 { + if (filter.TenantIDs != nil && len(filter.TenantIDs) == 0) || + (filter.Orgs != nil && len(filter.Orgs) == 0) || + (filter.OrgDisplayNames != nil && len(filter.OrgDisplayNames) == 0) { return tns, 0, nil }As per path instructions, “Review database changes for Bun/pgx query correctness, transaction boundaries, migration ordering, data-model compatibility.”
🤖 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/tenant.go` around lines 202 - 205, The empty-filter shortcut in the tenant query logic only handles TenantIDs, so empty Orgs and OrgDisplayNames still flow into query construction. Update the filter handling in Tenant model query code to short-circuit any non-nil empty slice consistently across TenantIDs, Orgs, and OrgDisplayNames before building the Bun query, returning an empty result and zero count from the same method.Source: Path instructions
rest-api/db/pkg/db/model/tenant_test.go (1)
503-585: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd at least one real pagination case.
The new DAO contract includes pagination, but every case uses
TotalLimit, so regressions inLimit,Offset, or explicitOrderByhandling would not be caught. Add a small-page case that assertstotalstays at the full match count whilelen(got)reflects the page size.As per coding guidelines, “Verification should exercise the behavior that changed.”
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/handler/vpcpeering.go`:
- Around line 437-439: The Swagger annotation for vpcId in the VPC peering
handler is inconsistent with the query parsing behavior in the same handler,
since qParams["vpcId"] accepts multiple values like status and peerTenantId.
Update the comment near the VPC peering query parameters so vpcId also նշotes
that it is repeatable for multiple values, keeping the annotation aligned with
the handler contract and generated OpenAPI spec.
---
Duplicate comments:
In `@rest-api/api/pkg/api/handler/vpcpeering.go`:
- Around line 532-542: The status query validation in vpcpeering handler returns
a generic bad-request message instead of echoing the rejected value like the
vpcId and peerTenantId checks. Update the invalid-status branch in the status
filter logic to include the offending status string in the API error response,
matching the existing logger.Warn output and the pattern used by the other
filter validations.
---
Nitpick comments:
In `@rest-api/db/pkg/db/model/tenant.go`:
- Around line 202-205: The empty-filter shortcut in the tenant query logic only
handles TenantIDs, so empty Orgs and OrgDisplayNames still flow into query
construction. Update the filter handling in Tenant model query code to
short-circuit any non-nil empty slice consistently across TenantIDs, Orgs, and
OrgDisplayNames before building the Bun query, returning an empty result and
zero count from the same method.
🪄 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: efaf8af1-567e-42b2-8602-4e4cca156329
⛔ Files ignored due to path filters (40)
rest-api/sdk/standard/api_vpc_peering.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_batch_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_machine_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_power_shelf_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_rack_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_infini_band_partition_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_type_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_instance_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_interface.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_interface_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_machine.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_machine_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_network_security_group_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-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_peering.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_vpc_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/workflow-schema/flow/protobuf/v1/flow_grpc.pb.gois excluded by!**/*.pb.go,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.gorest-api/workflow-schema/schema/site-agent/workflows/v1/fmds_nico_grpc.pb.gois excluded by!**/*.pb.go,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.gorest-api/workflow-schema/schema/site-agent/workflows/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!rest-api/**/*.pb.gorest-api/workflow-schema/schema/site-agent/workflows/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.gorest-api/workflow-schema/schema/site-agent/workflows/v1/nmx_c_nico_grpc.pb.gois excluded by!**/*.pb.go,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (10)
rest-api/api/pkg/api/handler/vpcpeering.gorest-api/api/pkg/api/handler/vpcpeering_test.gorest-api/api/pkg/api/model/vpcpeering.gorest-api/api/pkg/api/model/vpcpeering_test.gorest-api/db/pkg/db/model/tenant.gorest-api/db/pkg/db/model/tenant_test.gorest-api/db/pkg/db/model/vpcpeering.gorest-api/db/pkg/db/model/vpcpeering_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- rest-api/openapi/spec.yaml
- rest-api/db/pkg/db/model/vpcpeering_test.go
- rest-api/api/pkg/api/model/vpcpeering.go
- rest-api/api/pkg/api/model/vpcpeering_test.go
- rest-api/db/pkg/db/model/vpcpeering.go
- rest-api/api/pkg/api/handler/vpcpeering_test.go
hwadekar-nv
force-pushed
the
feat/enhance-vpc-peering
branch
from
effc2d7 to
6c887bf
Compare
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Looking good @hwadekar-nv Let's merge after making a few other changes suggested.
| } | ||
|
|
||
| // Get status from query param | ||
| if statusStrings := qParams["status"]; len(statusStrings) != 0 { |
There was a problem hiding this comment.
Let's use this pattern:
qStatuses := qParams["status"]
if len(qStatuses) {
....
}| } | ||
|
|
||
| // Get vpcId from query param | ||
| if vpcIDStrs := qParams["vpcId"]; len(vpcIDStrs) != 0 { |
There was a problem hiding this comment.
Same as above, split the conditional:
qVpcIds := qParams["vpcId"]| // | ||
| GetAll(ctx context.Context, tx *db.Tx, filter TenantFilterInput, page paginator.PageInput, includeRelations []string) ([]Tenant, int, error) | ||
| // | ||
| GetAllByOrg(ctx context.Context, tx *db.Tx, org string, includeRelations []string) ([]Tenant, error) |
There was a problem hiding this comment.
Let's converge GetAllByOrg into GetAll
hwadekar-nv
force-pushed
the
feat/enhance-vpc-peering
branch
2 times, most recently
from
08bff14 to
17ca321
Compare
hwadekar-nv
force-pushed
the
feat/enhance-vpc-peering
branch
from
17ca321 to
b91d692
Compare
hwadekar-nv
force-pushed
the
feat/enhance-vpc-peering
branch
from
b91d692 to
f1269e5
Compare
3 participants
#2089
The get-all VPC peering endpoint only supported filtering by siteId and isMultiTenant, so users could not narrow results by status, a specific VPC, or a linked peer tenant. For multi-tenant peerings, the response also did not show which tenant owned each VPC.
Type of Change
Testing