feat(rest-api): Tenant TargetedInstanceCreation capability implementation#2134
Open
hwadekar-nv wants to merge 12 commits into
Open
feat(rest-api): Tenant TargetedInstanceCreation capability implementation#2134hwadekar-nv wants to merge 12 commits into
hwadekar-nv wants to merge 12 commits into
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. |
🔐 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-02 21:53:17 UTC | Commit: 1feb71c |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
hwadekar-nv
changed the title
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
from
2fff422 to
7d6bcc6
Compare
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:
WalkthroughThis PR moves targeted instance creation from tenant-level config to tenant-account and tenant-site scoped JSONB config, adds capability-resolution helpers, updates authorization checks across handlers, and documents the new ChangesTargeted Instance Creation Capability Rework
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
2 times, most recently
from
ec337d8 to
eeb42e0
Compare
Contributor
Author
|
Will start working on this PR after 2.0. We have new strategies to allow tenants to update capabilities via TenantAccount and TenantSite across all Sites or specific Site. |
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
2 times, most recently
from
685f8f1 to
c3402d9
Compare
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
2 times, most recently
from
482fc88 to
d20920e
Compare
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2134.docs.buildwithfern.com/infra-controller |
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
from
d20920e to
3c8f601
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/db/pkg/db/model/tenantaccount.go (1)
546-564: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake mixed
TenantAccountupdates atomic.
Updatecan run the scalar-column update and the config merge as separate statements. If a caller passestx == nilwith both normal fields andConfig, the first update can commit before the config merge fails. Prefer building oneUPDATE, or require/wrap mixed updates incdb.WithTx.🤖 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/tenantaccount.go` around lines 546 - 564, The TenantAccount update path in the update method currently splits scalar field changes and Config merging into separate UPDATE statements, which can leave mixed updates partially applied when tx is nil. Refactor the logic around updatedFields/input.Config in TenantAccount.Update to perform a single atomic update when both kinds of changes are present, or ensure mixed updates are always executed inside cdb.WithTx by wrapping/requiring a transaction before running db.GetIDB(...).NewUpdate().Exec.Source: Path instructions
🧹 Nitpick comments (3)
rest-api/api/pkg/api/model/tenantaccountsitecapability.go (1)
199-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a receiver method over a free function on the named type.
GlobalTargetedInstanceCreationFromRequestoperates entirely onAPITenantAccountSiteCapabilitiesUpdateRequest; expressing it as a method (e.g.caps.GlobalTargetedInstanceCreation()) keeps behavior colocated with the type and reads more naturally at the call site.As per path instructions: "discourage scattered independent functions when a receiver method would make ownership and responsibilities clearer."
♻️ Proposed conversion to a receiver method
-func GlobalTargetedInstanceCreationFromRequest(caps APITenantAccountSiteCapabilitiesUpdateRequest) *bool { - for _, cap := range caps { +func (caps APITenantAccountSiteCapabilitiesUpdateRequest) GlobalTargetedInstanceCreation() *bool { + for _, cap := range caps { if cap.Scope == TenantAccountSiteCapabilityScopeGlobal { return cutil.GetPtr(cap.TargetedInstanceCreation) } } return 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/model/tenantaccountsitecapability.go` around lines 199 - 206, Convert GlobalTargetedInstanceCreationFromRequest into a receiver method on APITenantAccountSiteCapabilitiesUpdateRequest so the behavior lives with the type and can be called as caps.GlobalTargetedInstanceCreation(). Keep the same logic that scans for TenantAccountSiteCapabilityScopeGlobal and returns the targeted instance creation value, but move it onto the named type and update any call sites accordingly.Source: Path instructions
rest-api/db/pkg/db/model/tenantsite_test.go (2)
287-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse range-over-integer for this counting loop.
- for i := 0; i < siteCount; i++ { + for i := range siteCount {As per coding guidelines, “Counting loop → range-over-integer.” As per path instructions, prefer range-based iteration over C-style
forloops.🤖 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/tenantsite_test.go` around lines 287 - 293, The counting loop in the tenant site test should use range-over-integer instead of a C-style for loop. Update the iteration in the site-building block within the test that uses TestBuildSite and TestBuildTenantSite so it iterates with a range-based integer loop over siteCount, while preserving the existing indexing and even/odd behavior.Sources: Coding guidelines, Path instructions
602-701: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for
RemoveTargetedInstanceCreation.The DAO now has a clear-override branch, but this table only verifies config merge. Add a case that starts with
targetedInstanceCreation=true, updates withRemoveTargetedInstanceCreation: true, and asserts the pointer is cleared.🤖 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/tenantsite_test.go` around lines 602 - 701, Add a missing Update test for the clear-override path in TenantSiteSQLDAO.Update: the current TestTenantSiteSQLDAO_Update only covers config merge, so add a case that seeds TenantSiteConfig.TargetedInstanceCreation as true, calls Update with TenantSiteConfigUpdateInput.RemoveTargetedInstanceCreation set to true, and asserts the resulting TenantSite.Config.TargetedInstanceCreation is cleared/nil. Use the existing TenantSiteSQLDAO.Update, TenantSiteUpdateInput, and TenantSiteConfigUpdateInput symbols to locate the right spot in the table-driven test.Source: Path instructions
🤖 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/machine.go`:
- Line 245: Update the privileged machine read authorization to use the
site-effective targeted-instance capability instead of the legacy tenant
flag/provider-account check. In the machine read paths around the
TenantHasTargetedInstanceCreation logic, switch single-machine reads to
EffectiveTargetedInstanceCreation(ctx, nil, ..., tenant, machine.SiteID), and
for list reads apply the same site-aware rule when siteId is present so
disabled-site machines are not returned in tenant-privileged results. Make sure
the behavior in the machine handler keeps honoring TenantAccount.config, Ready
status, and TenantSite.config overrides.
- Around line 762-768: The site-scoped capability check in IsProviderOrTenant is
being bypassed by the early requirePrivilegedTenant gate, which can reject
legacy TenantAccount tenants before machine.SiteID is evaluated. Update the
IsProviderOrTenant flow to defer the privileged-tenant rejection until after the
EffectiveTargetedInstanceCreation check runs, keeping the machine.SiteID-based
logic as the authoritative permission decision and removing the premature
pre-gate.
In `@rest-api/api/pkg/api/handler/util/common/common.go`:
- Around line 1467-1483: The privileged pre-check is still using the deprecated
tenant-level flag in TenantHasTargetedInstanceCreation, which can incorrectly
reject tenants enabled via the new TenantAccount.config path. Update the
requirePrivileged / requirePrivilegedTenant flow in common.go so non-site-scoped
privilege is resolved from the ready TenantAccount config, or remove this early
gate for site-aware callers and let EffectiveTargetedInstanceCreation be the
authoritative check. Keep the fix localized around
TenantHasTargetedInstanceCreation and the caller(s) that enforce the privileged
gate.
In `@rest-api/db/pkg/db/model/tenantsite.go`:
- Around line 314-334: The TenantSite update path currently applies Config
merging and RemoveTargetedInstanceCreation as separate updates, which can leave
partial state and makes conflicting input ambiguous. Update the logic in the
TenantSite model update flow to validate that both inputs are not used together,
then build a single NewUpdate/Exec path that applies either the JSONB merge or
the targetedInstanceCreation removal, not both. Use the existing update block
around input.Config and input.RemoveTargetedInstanceCreation to centralize the
decision and preserve one atomic update per request.
In
`@rest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.go`:
- Around line 26-33: The backfill in the migration only applies to Ready
tenant_account rows, so invited or pending accounts that later become Ready will
miss the legacy targetedInstanceCreation capability. Update the migration logic
in the tenant_account/tenant backfill to copy the capability for all matching
tenant_account rows tied to tenants with
tenant.config.targetedInstanceCreation=true, and if the status filter must
remain elsewhere, ensure the status transition path in tenant account handling
also propagates this config field when moving to Ready.
---
Outside diff comments:
In `@rest-api/db/pkg/db/model/tenantaccount.go`:
- Around line 546-564: The TenantAccount update path in the update method
currently splits scalar field changes and Config merging into separate UPDATE
statements, which can leave mixed updates partially applied when tx is nil.
Refactor the logic around updatedFields/input.Config in TenantAccount.Update to
perform a single atomic update when both kinds of changes are present, or ensure
mixed updates are always executed inside cdb.WithTx by wrapping/requiring a
transaction before running db.GetIDB(...).NewUpdate().Exec.
---
Nitpick comments:
In `@rest-api/api/pkg/api/model/tenantaccountsitecapability.go`:
- Around line 199-206: Convert GlobalTargetedInstanceCreationFromRequest into a
receiver method on APITenantAccountSiteCapabilitiesUpdateRequest so the behavior
lives with the type and can be called as caps.GlobalTargetedInstanceCreation().
Keep the same logic that scans for TenantAccountSiteCapabilityScopeGlobal and
returns the targeted instance creation value, but move it onto the named type
and update any call sites accordingly.
In `@rest-api/db/pkg/db/model/tenantsite_test.go`:
- Around line 287-293: The counting loop in the tenant site test should use
range-over-integer instead of a C-style for loop. Update the iteration in the
site-building block within the test that uses TestBuildSite and
TestBuildTenantSite so it iterates with a range-based integer loop over
siteCount, while preserving the existing indexing and even/odd behavior.
- Around line 602-701: Add a missing Update test for the clear-override path in
TenantSiteSQLDAO.Update: the current TestTenantSiteSQLDAO_Update only covers
config merge, so add a case that seeds TenantSiteConfig.TargetedInstanceCreation
as true, calls Update with
TenantSiteConfigUpdateInput.RemoveTargetedInstanceCreation set to true, and
asserts the resulting TenantSite.Config.TargetedInstanceCreation is cleared/nil.
Use the existing TenantSiteSQLDAO.Update, TenantSiteUpdateInput, and
TenantSiteConfigUpdateInput symbols to locate the right spot in the table-driven
test.
🪄 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: aef13451-a675-454a-9c90-57bbe59434be
⛔ Files ignored due to path filters (40)
rest-api/sdk/standard/api_tenant_account.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_tenant.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability_scope.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_capabilities.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_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (21)
rest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/model/operatingsystem_test.gorest-api/api/pkg/api/model/sshkeygroup_test.gorest-api/api/pkg/api/model/tenant.gorest-api/api/pkg/api/model/tenant_test.gorest-api/api/pkg/api/model/tenantaccount.gorest-api/api/pkg/api/model/tenantaccount_test.gorest-api/api/pkg/api/model/tenantaccountsitecapability.gorest-api/db/pkg/db/model/tenantaccount.gorest-api/db/pkg/db/model/tenantsite.gorest-api/db/pkg/db/model/tenantsite_test.gorest-api/db/pkg/db/model/testing.gorest-api/db/pkg/migrations/20230412224951_tenant_site.gorest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.gorest-api/openapi/spec.yaml
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
from
4a12952 to
b0ee8a6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rest-api/db/pkg/db/model/tenantsite_test.go (1)
287-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer range-over-integer for this counting loop.
The index is used solely as a counter (
i%2,Sprintf), so this is a textbook candidate for Go's range-over-integer form.♻️ Suggested refactor
- sites := []*Site{} - siteCount := 30 - for i := 0; i < siteCount; i++ { + sites := []*Site{} + siteCount := 30 + for i := range siteCount {As per path instructions: "Counting loop → range-over-integer. When the bound is a count ... use
for i := range n".🤖 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/tenantsite_test.go` around lines 287 - 297, The counting loop in the tenant site test uses an index only as a counter for naming and parity checks, so refactor the loop to use Go’s range-over-integer form instead of a classic three-part loop. Update the loop in the test setup that builds sites and calls TestBuildSite/TestBuildTenantSite so the iteration variable still supports the Sprintf name generation and i%2 branch, but the loop itself follows the preferred range-over-integer pattern.Source: Path instructions
rest-api/api/pkg/api/handler/machine.go (1)
305-331: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftRedundant per-site resolution creates an N+1 on the machine-list hot path.
EffectiveTargetedInstanceCreationinternally re-fetches theSiteby ID and re-queries ReadyTenantAccounts per provider, yet this loop already holds eachsitefromsiteDAO.GetAlland the provider is fixed for the inner iteration. The cost isO(providers × sites)with three DB round-trips per site. Consider resolving the provider's ReadyTenantAccountonce outside the inner loop and evaluating the per-siteTenantSiteoverride directly against the already-loadedsite.🤖 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/machine.go` around lines 305 - 331, The machine list path has an N+1 problem because the loop in the handler around siteDAO.GetAll and common.EffectiveTargetedInstanceCreation re-fetches each Site and re-queries TenantAccount data for every site/provider pair. Refactor this block so the provider’s Ready TenantAccount is resolved once per provider outside the inner site loop, then use the already-loaded site data to check the per-site TenantSite override directly instead of calling EffectiveTargetedInstanceCreation for every site. Keep the existing error handling/logging structure in the same handler flow.
🤖 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/machine.go`:
- Around line 294-337: The machine list filtering in the tenant branch is using
provider IDs even though EffectiveTargetedInstanceCreation is evaluated per
site, which can widen results to disabled sites. Update the logic in machine.go
around the tenant/filterSite handling to collect and append enabled site IDs
instead of provider IDs, and apply them through MachineFilterInput.SiteIDs
rather than InfrastructureProviderIDs. Keep the existing
EffectiveTargetedInstanceCreation and SiteDAO.GetAll flow, but change the
aggregation variable and final filter assignment so the list stays scoped to
sites where targeted instance creation is enabled.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/machine.go`:
- Around line 305-331: The machine list path has an N+1 problem because the loop
in the handler around siteDAO.GetAll and
common.EffectiveTargetedInstanceCreation re-fetches each Site and re-queries
TenantAccount data for every site/provider pair. Refactor this block so the
provider’s Ready TenantAccount is resolved once per provider outside the inner
site loop, then use the already-loaded site data to check the per-site
TenantSite override directly instead of calling
EffectiveTargetedInstanceCreation for every site. Keep the existing error
handling/logging structure in the same handler flow.
In `@rest-api/db/pkg/db/model/tenantsite_test.go`:
- Around line 287-297: The counting loop in the tenant site test uses an index
only as a counter for naming and parity checks, so refactor the loop to use Go’s
range-over-integer form instead of a classic three-part loop. Update the loop in
the test setup that builds sites and calls TestBuildSite/TestBuildTenantSite so
the iteration variable still supports the Sprintf name generation and i%2
branch, but the loop itself follows the preferred range-over-integer pattern.
🪄 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: 8bd15369-c9f6-45e7-b4dd-b1d7c80569ca
⛔ Files ignored due to path filters (40)
rest-api/sdk/standard/api_tenant_account.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_tenant.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability_scope.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_capabilities.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_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (23)
rest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/model/operatingsystem_test.gorest-api/api/pkg/api/model/sshkeygroup_test.gorest-api/api/pkg/api/model/tenant.gorest-api/api/pkg/api/model/tenant_test.gorest-api/api/pkg/api/model/tenantaccount.gorest-api/api/pkg/api/model/tenantaccount_test.gorest-api/api/pkg/api/model/tenantaccountsitecapability.gorest-api/db/pkg/db/model/tenantaccount.gorest-api/db/pkg/db/model/tenantaccount_test.gorest-api/db/pkg/db/model/tenantsite.gorest-api/db/pkg/db/model/tenantsite_test.gorest-api/db/pkg/db/model/testing.gorest-api/db/pkg/migrations/20230412224951_tenant_site.gorest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.gorest-api/openapi/spec.yaml
✅ Files skipped from review due to trivial changes (1)
- rest-api/api/pkg/api/model/sshkeygroup_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- rest-api/db/pkg/migrations/20230412224951_tenant_site.go
- rest-api/db/pkg/db/model/testing.go
- rest-api/api/pkg/api/model/tenant_test.go
- rest-api/api/pkg/api/model/operatingsystem_test.go
- rest-api/openapi/spec.yaml
- rest-api/api/pkg/api/model/tenantaccount_test.go
- rest-api/api/pkg/api/handler/instance.go
- rest-api/api/pkg/api/handler/vpc.go
- rest-api/db/pkg/db/model/tenantaccount.go
- rest-api/api/pkg/api/model/tenantaccount.go
- rest-api/api/pkg/api/model/tenantaccountsitecapability.go
- rest-api/api/pkg/api/model/tenant.go
- rest-api/db/pkg/db/model/tenantsite.go
- rest-api/api/pkg/api/handler/tenantaccount.go
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
from
fdc9a2d to
ffd8c7b
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rest-api/api/pkg/api/handler/tenantaccount.go (2)
397-464: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
GetAllTenantAccountHandlersilently drops per-site capability overrides.Every list-response
APITenantAccountis built withtenantSites=nil(Line 447), whileGetTenantAccountHandler(Line 585) and the new PUT response (Line 932) now fetch and pass the actualTenantSiteslice. SinceNewAPITenantAccountderivesSiteCapabilitiesfromtenantSites, the list endpoint will only ever report the global scope and silently omit anylimitedper-site overrides — producing an inconsistent contract betweenGET /tenant-account(list) andGET /tenant-account/{id}for the same brand-new field.Consider batch-fetching
TenantSiterows grouped by tenant ID (mirroring the existing allocation-count batching pattern already in this handler) so list responses stay consistent with the detail view.🤖 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/tenantaccount.go` around lines 397 - 464, GetAllTenantAccountHandler is building APITenantAccount list items with tenantSites left nil, so NewAPITenantAccount cannot compute SiteCapabilities and per-site limited overrides are lost. Update the list path in tenantaccount.go to batch-load TenantSite rows for the tenants in tas, grouped by tenant ID similar to the existing allocation-count fetch, and pass the matching tenantSites into model.NewAPITenantAccount just like GetTenantAccountHandler and the PUT response do.
907-937: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHardcoded
allocationCount: 0in the siteCapabilities update response.
model.NewAPITenantAccount(ta, ssds, 0, tenantSites)always reports zero allocations, even thoughGetTenantAccountHandlercomputes the real value viaaDAO.GetCount(...)for the exact same entity. A Provider Admin updatingsiteCapabilitieson an active TenantAccount with existing allocations will get back a response claiming zero allocations.🐛 Proposed fix
+ aDAO := cdbm.NewAllocationDAO(utah.dbSession) + total := 0 + if ta.TenantID != nil { + cnt, cerr := aDAO.GetCount(ctx, nil, cdbm.AllocationFilterInput{ + InfrastructureProviderIDs: []uuid.UUID{ta.InfrastructureProviderID}, + TenantIDs: []uuid.UUID{*ta.TenantID}, + }) + if cerr != nil { + logger.Error().Err(cerr).Msg("error retrieving allocation count for Tenant Account from DB") + return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Allocations to determine total allocation for tenant account", nil) + } + total = cnt + } + sdDAO := cdbm.NewStatusDetailDAO(utah.dbSession) ssds, _, err := sdDAO.GetAll(ctx, nil, cdbm.StatusDetailFilterInput{EntityIDs: []string{ta.ID.String()}}, cdbp.PageInput{Limit: cutil.GetPtr(pagination.MaxPageSize)}) if err != nil { logger.Error().Err(err).Msg("error retrieving Status Details for TenantAccount from DB") return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Status Details for TenantAccount", nil) } - apiInstance := model.NewAPITenantAccount(ta, ssds, 0, tenantSites) + apiInstance := model.NewAPITenantAccount(ta, ssds, total, tenantSites)🤖 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/tenantaccount.go` around lines 907 - 937, The update response is hardcoding allocationCount to 0 in the call to model.NewAPITenantAccount, so the returned TenantAccount can incorrectly show no allocations. Reuse the same allocation-count logic already used in GetTenantAccountHandler by fetching the real count for ta.ID before building the response, then pass that value instead of 0 when constructing apiInstance.rest-api/api/pkg/api/handler/expectedmachine.go (1)
351-365: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMirror the TenantAccount fallback in the ExpectedMachine list filter.
GetAllExpectedMachineHandleronly addsTenantSite.SiteIDvalues tofilterInput.SiteIDs. A privileged tenant that is authorized through a readyTenantAccountbut has no explicitTenantSiterow for a provider site will get an empty result set, and asiteIdquery for that site will be rejected, even thoughValidateProviderOrTenantSiteAccessallows the same access path. Reuse the same access resolution here or extract a shared helper.🤖 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/expectedmachine.go` around lines 351 - 365, The ExpectedMachine list filter only uses TenantSite rows, so privileged tenants can miss access granted through a ready TenantAccount. Update GetAllExpectedMachineHandler to resolve site access the same way ValidateProviderOrTenantSiteAccess does, or extract and reuse a shared helper, so filterInput.SiteIDs includes both TenantSite.SiteID values and any provider sites reachable via the TenantAccount fallback before applying the list filter.Source: Path instructions
🧹 Nitpick comments (1)
rest-api/api/pkg/api/handler/tenantaccount.go (1)
785-932: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd handler-level test coverage for the new provider siteCapabilities flow.
This introduces a non-trivial, security-sensitive authorization path (advisory-locked transaction, global vs. limited scope resolution, stale-override cleanup, cross-provider site scoping) with no accompanying
tenantaccount_test.gochanges in this layer. Given the privilege-granting nature of this endpoint, targeted tests (ownership rejection, stale-override cleanup, limited-site-not-owned-by-provider rejection) would materially reduce regression risk.Based on path instructions: "Review Go code for correctness, clean control flow, error handling, context propagation, test coverage, performance, and cohesive organization..."
🤖 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/tenantaccount.go` around lines 785 - 932, Add handler-level tests for handleProviderSiteCapabilitiesUpdate in tenantaccount.go to cover the new provider siteCapabilities flow. Exercise the security-sensitive branches by verifying ownership/provider rejection, limited-site validation when a Site is not owned by the Tenant Account Infrastructure Provider, and stale TenantSite override cleanup when a site is removed from the limited set. Use the existing UpdateTenantAccountHandler, TenantAccountDAO/TenantSiteDAO behavior, and the advisory-locked transaction path to locate the right handler logic.Source: Path instructions
🤖 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.
Outside diff comments:
In `@rest-api/api/pkg/api/handler/expectedmachine.go`:
- Around line 351-365: The ExpectedMachine list filter only uses TenantSite
rows, so privileged tenants can miss access granted through a ready
TenantAccount. Update GetAllExpectedMachineHandler to resolve site access the
same way ValidateProviderOrTenantSiteAccess does, or extract and reuse a shared
helper, so filterInput.SiteIDs includes both TenantSite.SiteID values and any
provider sites reachable via the TenantAccount fallback before applying the list
filter.
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Around line 397-464: GetAllTenantAccountHandler is building APITenantAccount
list items with tenantSites left nil, so NewAPITenantAccount cannot compute
SiteCapabilities and per-site limited overrides are lost. Update the list path
in tenantaccount.go to batch-load TenantSite rows for the tenants in tas,
grouped by tenant ID similar to the existing allocation-count fetch, and pass
the matching tenantSites into model.NewAPITenantAccount just like
GetTenantAccountHandler and the PUT response do.
- Around line 907-937: The update response is hardcoding allocationCount to 0 in
the call to model.NewAPITenantAccount, so the returned TenantAccount can
incorrectly show no allocations. Reuse the same allocation-count logic already
used in GetTenantAccountHandler by fetching the real count for ta.ID before
building the response, then pass that value instead of 0 when constructing
apiInstance.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Around line 785-932: Add handler-level tests for
handleProviderSiteCapabilitiesUpdate in tenantaccount.go to cover the new
provider siteCapabilities flow. Exercise the security-sensitive branches by
verifying ownership/provider rejection, limited-site validation when a Site is
not owned by the Tenant Account Infrastructure Provider, and stale TenantSite
override cleanup when a site is removed from the limited set. Use the existing
UpdateTenantAccountHandler, TenantAccountDAO/TenantSiteDAO behavior, and the
advisory-locked transaction path to locate the right handler logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 337a24c6-0bfa-4a62-ab56-b6e48d7222c7
⛔ Files ignored due to path filters (40)
rest-api/sdk/standard/api_tenant_account.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_tenant.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability_scope.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_capabilities.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_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (31)
rest-api/api/pkg/api/handler/dpureprovision_test.gorest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machinehealth_test.gorest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/sku_test.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/model/operatingsystem_test.gorest-api/api/pkg/api/model/sshkeygroup_test.gorest-api/api/pkg/api/model/tenant.gorest-api/api/pkg/api/model/tenant_test.gorest-api/api/pkg/api/model/tenantaccount.gorest-api/api/pkg/api/model/tenantaccount_test.gorest-api/api/pkg/api/model/tenantaccountsitecapability.gorest-api/db/pkg/db/model/tenantaccount.gorest-api/db/pkg/db/model/tenantaccount_test.gorest-api/db/pkg/db/model/tenantsite.gorest-api/db/pkg/db/model/tenantsite_test.gorest-api/db/pkg/db/model/testing.gorest-api/db/pkg/migrations/20230412224951_tenant_site.gorest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.gorest-api/openapi/spec.yaml
✅ Files skipped from review due to trivial changes (2)
- rest-api/api/pkg/api/model/sshkeygroup_test.go
- rest-api/db/pkg/migrations/20230412224951_tenant_site.go
🚧 Files skipped from review as they are similar to previous changes (20)
- rest-api/api/pkg/api/model/tenantaccount_test.go
- rest-api/api/pkg/api/model/operatingsystem_test.go
- rest-api/api/pkg/api/model/tenant_test.go
- rest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.go
- rest-api/db/pkg/db/model/testing.go
- rest-api/openapi/spec.yaml
- rest-api/api/pkg/api/handler/vpc.go
- rest-api/api/pkg/api/handler/instance.go
- rest-api/api/pkg/api/handler/machine_test.go
- rest-api/api/pkg/api/model/tenantaccountsitecapability.go
- rest-api/api/pkg/api/handler/util/common/common.go
- rest-api/api/pkg/api/handler/util/common/common_test.go
- rest-api/api/pkg/api/model/tenant.go
- rest-api/api/pkg/api/handler/serviceaccount.go
- rest-api/db/pkg/db/model/tenantaccount_test.go
- rest-api/db/pkg/db/model/tenantsite.go
- rest-api/api/pkg/api/handler/machine.go
- rest-api/api/pkg/api/model/tenantaccount.go
- rest-api/db/pkg/db/model/tenantaccount.go
- rest-api/db/pkg/db/model/tenantsite_test.go
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
from
1375da6 to
04ce4d9
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/api/pkg/api/handler/sku_test.go (1)
202-220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Without capability" fixture doesn't actually test capability-disabled — it tests missing-account.
In both
TestGetAllSkuHandler_Handle(lines 202-220) andTestGetSkuHandler_Handle(lines 477-495),tenantWithoutCapabilityis inserted with no correspondingTenantAccountat all — identical in setup totenantWithoutAccount. SinceEffectiveTargetedInstanceCreationreturnsfalse, nilwhenever no ReadyTenantAccountrow exists, the "tenant without TargetedInstanceCreation capability is denied" test case exercises the exact same code path (len(tas) == 0) as "tenant without TenantAccount with Provider is denied", rather than the intended path where a ReadyTenantAccountexists withConfig.TargetedInstanceCreation: false.To genuinely cover the capability-disabled branch, give
tenantWithoutCapabilitya ReadyTenantAccountwith an explicitConfig: cdbm.TenantAccountConfig{TargetedInstanceCreation: false}(mirroring howtenantAccountis built fortenantWithCapability).Based on learnings, "ensure each failure-path test intentionally targets a single validation branch... construct fixtures so they satisfy all prerequisite conditions except the one being tested."
🧪 Proposed fix to isolate the capability-disabled branch
// Create tenant WITHOUT TargetedInstanceCreation capability tenantOrgNoCapability := "test-tenant-org-no-capability" tenantWithoutCapability := &cdbm.Tenant{ ID: uuid.New(), Name: "test-tenant-no-capability", Org: tenantOrgNoCapability, } _, err = dbSession.DB.NewInsert().Model(tenantWithoutCapability).Exec(ctx) assert.Nil(t, err) + + // Ready TenantAccount that explicitly disables the capability, so this + // case is distinct from "no TenantAccount at all". + tenantAccountNoCapability := &cdbm.TenantAccount{ + ID: uuid.New(), + AccountNumber: "test-account-no-capability", + TenantID: &tenantWithoutCapability.ID, + TenantOrg: tenantOrgNoCapability, + InfrastructureProviderID: infraProv.ID, + Status: cdbm.TenantAccountStatusReady, + Config: cdbm.TenantAccountConfig{TargetedInstanceCreation: false}, + } + _, err = dbSession.DB.NewInsert().Model(tenantAccountNoCapability).Exec(ctx) + assert.Nil(t, err)Also applies to: 477-495
🤖 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/sku_test.go` around lines 202 - 220, The “without capability” setup in the SKU handler tests is currently indistinguishable from the “without tenant account” case because `tenantWithoutCapability` never gets a Ready `TenantAccount`, so `EffectiveTargetedInstanceCreation` exits on the missing-account branch instead of the capability-disabled branch. Update the fixtures in `TestGetAllSkuHandler_Handle` and `TestGetSkuHandler_Handle` so the `tenantWithoutCapability` tenant has a Ready `TenantAccount` with `Config.TargetedInstanceCreation` explicitly set to false, while keeping `tenantWithoutAccount` free of any account rows. Use the existing `tenantAccount`/`EffectiveTargetedInstanceCreation` setup as the reference point so each test isolates a single validation path.Source: Learnings
🧹 Nitpick comments (1)
rest-api/api/pkg/api/handler/site.go (1)
889-926: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollapse the duplicate
TenantAccountlookup into a single query.
common.TenantHasTargetedInstanceCreationalready fetches the tenant's ReadyTenantAccountrows and scans theirConfig.TargetedInstanceCreation. Immediately afterwards, the privileged branch issues a secondtaDAO.GetAllwith the identicalTenantIDs+Readyfilter and re-scans the same flag. On a list endpoint this is an avoidable extra round-trip per Tenant request. Fetch the accounts once here and deriveproviderIDsdirectly, dropping the separate capability probe.♻️ Proposed single-query shape
taDAO := cdbm.NewTenantAccountDAO(gash.dbSession) tas, _, serr := taDAO.GetAll(ctx, nil, cdbm.TenantAccountFilterInput{ TenantIDs: []uuid.UUID{tenant.ID}, Statuses: []string{cdbm.TenantAccountStatusReady}, }, cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)}, nil) if serr != nil { logger.Error().Err(serr).Msg("error retrieving Tenant Accounts for Tenant") return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Tenant Accounts", nil) } providerIDs := make([]uuid.UUID, 0, len(tas)) for _, ta := range tas { if ta.Config.TargetedInstanceCreation { providerIDs = append(providerIDs, ta.InfrastructureProviderID) } } if len(providerIDs) > 0 { // ...fetch providerSites as before }🤖 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/site.go` around lines 889 - 926, The privileged Tenant path is doing a duplicate Ready TenantAccount fetch after common.TenantHasTargetedInstanceCreation already checked the same capability, causing an extra query. In site.go’s Tenant site lookup flow, remove the separate capability probe and use the existing TenantAccountDAO/GetAll result once to derive providerIDs from ta.Config.TargetedInstanceCreation. Keep the providerSites fetch in the same branch, and update the error handling/messages around the single TenantAccount lookup in the relevant site handler 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.
Outside diff comments:
In `@rest-api/api/pkg/api/handler/sku_test.go`:
- Around line 202-220: The “without capability” setup in the SKU handler tests
is currently indistinguishable from the “without tenant account” case because
`tenantWithoutCapability` never gets a Ready `TenantAccount`, so
`EffectiveTargetedInstanceCreation` exits on the missing-account branch instead
of the capability-disabled branch. Update the fixtures in
`TestGetAllSkuHandler_Handle` and `TestGetSkuHandler_Handle` so the
`tenantWithoutCapability` tenant has a Ready `TenantAccount` with
`Config.TargetedInstanceCreation` explicitly set to false, while keeping
`tenantWithoutAccount` free of any account rows. Use the existing
`tenantAccount`/`EffectiveTargetedInstanceCreation` setup as the reference point
so each test isolates a single validation path.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/site.go`:
- Around line 889-926: The privileged Tenant path is doing a duplicate Ready
TenantAccount fetch after common.TenantHasTargetedInstanceCreation already
checked the same capability, causing an extra query. In site.go’s Tenant site
lookup flow, remove the separate capability probe and use the existing
TenantAccountDAO/GetAll result once to derive providerIDs from
ta.Config.TargetedInstanceCreation. Keep the providerSites fetch in the same
branch, and update the error handling/messages around the single TenantAccount
lookup in the relevant site handler logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7da1fca7-0ffc-484c-b108-5ebb970e2e6b
⛔ Files ignored due to path filters (42)
rest-api/sdk/standard/api_tenant.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_tenant_account.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_tenant.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability_scope.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_capabilities.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_summary.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_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (50)
rest-api/api/pkg/api/handler/dpureprovision_test.gorest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancebatch_test.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machinehealth_test.gorest-api/api/pkg/api/handler/rack_test.gorest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/site.gorest-api/api/pkg/api/handler/site_test.gorest-api/api/pkg/api/handler/sku.gorest-api/api/pkg/api/handler/sku_test.gorest-api/api/pkg/api/handler/tenant.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/tray_test.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/model/operatingsystem_test.gorest-api/api/pkg/api/model/sshkeygroup_test.gorest-api/api/pkg/api/model/tenant.gorest-api/api/pkg/api/model/tenant_test.gorest-api/api/pkg/api/model/tenantaccount.gorest-api/api/pkg/api/model/tenantaccount_test.gorest-api/api/pkg/api/model/tenantaccountsitecapability.gorest-api/db/pkg/db/model/tenant.gorest-api/db/pkg/db/model/tenant_test.gorest-api/db/pkg/db/model/tenantaccount.gorest-api/db/pkg/db/model/tenantsite.gorest-api/db/pkg/db/model/tenantsite_test.gorest-api/db/pkg/db/model/testing.gorest-api/db/pkg/migrations/20230412224951_tenant_site.gorest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.gorest-api/docs/index.htmlrest-api/openapi/oasdiff-breaking-changes-ignore.txtrest-api/openapi/spec.yamlrest-api/workflow/pkg/activity/dpuextensionservice/dpuextensionservice_test.gorest-api/workflow/pkg/activity/infinibandpartition/infinibandpartition_test.gorest-api/workflow/pkg/activity/instance/instance_test.gorest-api/workflow/pkg/activity/nvlinklogicalpartition/nvlinklogicalpartition_test.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.gorest-api/workflow/pkg/activity/site/site_test.gorest-api/workflow/pkg/activity/sshkeygroup/sshkeygroup_test.gorest-api/workflow/pkg/activity/tenant/tenant_test.gorest-api/workflow/pkg/util/testing.go
🚧 Files skipped from review as they are similar to previous changes (19)
- rest-api/api/pkg/api/model/sshkeygroup_test.go
- rest-api/db/pkg/migrations/20230412224951_tenant_site.go
- rest-api/api/pkg/api/handler/vpc_test.go
- rest-api/api/pkg/api/model/tenantaccount_test.go
- rest-api/api/pkg/api/handler/dpureprovision_test.go
- rest-api/api/pkg/api/model/operatingsystem_test.go
- rest-api/api/pkg/api/model/tenantaccount.go
- rest-api/openapi/oasdiff-breaking-changes-ignore.txt
- rest-api/api/pkg/api/handler/util/common/common_test.go
- rest-api/api/pkg/api/handler/serviceaccount.go
- rest-api/api/pkg/api/handler/vpc.go
- rest-api/api/pkg/api/handler/util/common/testing.go
- rest-api/db/pkg/db/model/tenantsite.go
- rest-api/api/pkg/api/model/tenantaccountsitecapability.go
- rest-api/api/pkg/api/handler/tenantaccount.go
- rest-api/api/pkg/api/handler/instance.go
- rest-api/api/pkg/api/handler/machine_test.go
- rest-api/api/pkg/api/handler/machine.go
- rest-api/db/pkg/db/model/tenantsite_test.go
Contributor
Author
|
@coderabbitai full_review, thanks ! |
Contributor
|
ᕕ( ᐛ )ᕗ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rest-api/db/pkg/db/model/tenantaccount.go (1)
504-574: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake this update atomic when
txis nil.TenantAccountSQLDAO.Updateperforms two independent writes whenConfigis set, andrest-api/api/pkg/api/handler/serviceaccount.go:162calls it withtx == nil. A failure in the secondUPDATEcan leave the row partially applied; wrap this path in a transaction or collapse it into one statement.🤖 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/tenantaccount.go` around lines 504 - 574, TenantAccountSQLDAO.Update currently performs separate write operations when input.Config is set, so a nil tx call from the service account handler can leave the tenant account partially updated if the second write fails. Update the Update method to make the no-transaction path atomic by either wrapping the full update flow in a transaction when tx is nil or combining the field and config changes into a single database statement; keep the logic centered in TenantAccountSQLDAO.Update and preserve the final GetByID fetch after the atomic write completes.rest-api/api/pkg/api/handler/sku_test.go (1)
202-210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
tenantWithoutCapabilityno longer exercises the "capability disabled" branch — it's now a duplicate oftenantWithoutAccount.Both
tenantWithoutCapability(Lines 202-210 / 477-485) andtenantWithoutAccount(Lines 212-220 / 487-495) create a bareTenantwith noTenantAccountrow.EffectiveTargetedInstanceCreationtreats "no ReadyTenantAccount" and "TenantAccountexists butConfig.TargetedInstanceCreation == false" as distinct branches — this test previously covered the latter viaTenant.Config.TargetedInstanceCreation: false, but that signal was dropped without an equivalentTenantAccountfixture, so the two "denied" subtests now assert the exact same code path under different names.Based on learnings, failure-path tests should target a single, correctly-named validation branch with fixtures that isolate it.
🧪 Proposed fix to restore branch coverage
// Create tenant WITHOUT TargetedInstanceCreation capability tenantOrgNoCapability := "test-tenant-org-no-capability" tenantWithoutCapability := &cdbm.Tenant{ ID: uuid.New(), Name: "test-tenant-no-capability", Org: tenantOrgNoCapability, } _, err = dbSession.DB.NewInsert().Model(tenantWithoutCapability).Exec(ctx) assert.Nil(t, err) + + // Ready TenantAccount that explicitly disables the capability, so this + // test isolates the "capability disabled" branch, not "no account". + tenantAccountNoCapability := &cdbm.TenantAccount{ + ID: uuid.New(), + AccountNumber: "test-account-no-capability", + TenantID: &tenantWithoutCapability.ID, + TenantOrg: tenantOrgNoCapability, + InfrastructureProviderID: infraProv.ID, + Status: cdbm.TenantAccountStatusReady, + Config: cdbm.TenantAccountConfig{TargetedInstanceCreation: false}, + } + _, err = dbSession.DB.NewInsert().Model(tenantAccountNoCapability).Exec(ctx) + assert.Nil(t, err)Apply the analogous change in
TestGetSkuHandler_Handle(Lines 477-485).Also applies to: 477-485
🤖 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/sku_test.go` around lines 202 - 210, `tenantWithoutCapability` is no longer testing the disabled-capability branch and now duplicates `tenantWithoutAccount`; update the fixtures in `TestEffectiveTargetedInstanceCreation` and the matching `TestGetSkuHandler_Handle` case so each subtest isolates a distinct `EffectiveTargetedInstanceCreation` path. Keep `tenantWithoutCapability` backed by a ready `TenantAccount` with `Config.TargetedInstanceCreation` set to false, and leave `tenantWithoutAccount` as the no-`TenantAccount` case, so the two denied scenarios exercise different branches again.Source: Learnings
♻️ Duplicate comments (1)
rest-api/db/pkg/db/model/tenantsite.go (1)
285-339: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate before writing — the mutual-exclusion check still runs after a DB mutation.
The
Config/RemoveTargetedInstanceCreationmutual-exclusion check (lines 315-317) executes after theEnableSerialConsoleupdate (lines 300-313) has already been sent to the database. If a caller passesEnableSerialConsoleplus a conflictingConfig+RemoveTargetedInstanceCreationcombination, theEnableSerialConsolechange is committed even thoughUpdateultimately returns an error — a partial, silently-persisted write on an "invalid input" error path.Move the validation to the top of the function, before any write occurs.
🛡️ Proposed fix — validate before any write
func (tssd TenantSiteSQLDAO) Update(ctx context.Context, tx *db.Tx, input TenantSiteUpdateInput) (*TenantSite, error) { // Create a child span and set the attributes for current request ctx, tnsDAOSpan := tssd.tracerSpan.CreateChildInCurrentContext(ctx, "TenantSiteDAO.Update") if tnsDAOSpan != nil { defer tnsDAOSpan.End() tssd.tracerSpan.SetAttribute(tnsDAOSpan, "id", input.TenantSiteID.String()) } + if input.Config != nil && input.RemoveTargetedInstanceCreation { + return nil, errors.Wrap(db.ErrInvalidParams, "Config and RemoveTargetedInstanceCreation are mutually exclusive") + } + ts := &TenantSite{ ID: input.TenantSiteID, } updatedFields := []string{} if input.EnableSerialConsole != nil { ts.EnableSerialConsole = *input.EnableSerialConsole updatedFields = append(updatedFields, "enable_serial_console") tssd.tracerSpan.SetAttribute(tnsDAOSpan, "enable_serial_console", *input.EnableSerialConsole) } if len(updatedFields) > 0 { updatedFields = append(updatedFields, "updated") _, err := db.GetIDB(tx, tssd.dbSession).NewUpdate().Model(ts).Column(updatedFields...).Where("id = ?", input.TenantSiteID).Exec(ctx) if err != nil { return nil, err } } - if input.Config != nil && input.RemoveTargetedInstanceCreation { - return nil, errors.Wrap(db.ErrInvalidParams, "Config and RemoveTargetedInstanceCreation are mutually exclusive") - } - switch {🤖 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/tenantsite.go` around lines 285 - 339, Move the Config and RemoveTargetedInstanceCreation mutual-exclusion validation in TenantSiteSQLDAO.Update to the top of the method, before any database write or EnableSerialConsole update occurs. Keep the existing error handling with errors.Wrap(db.ErrInvalidParams, ...) but run it before building ts or calling db.GetIDB(...).NewUpdate().Exec so invalid input cannot partially persist changes.
🧹 Nitpick comments (7)
rest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.go (1)
15-59: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a dedicated test for this migration.
This migration moves capability data and drops a column irreversibly, yet — unlike the historical
tenantConfigUpMigration(covered byTest_tenantConfigUpMigrationinmigrations_test.go) — it ships without any unit test verifying the backfill predicates or the column drop.As per coding guidelines, "Add or update focused tests for bug fixes, shared behavior, API contracts, migrations, and cross-module changes."
🤖 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/migrations/20260701120000_tenant_account_site_capabilities.go` around lines 15 - 59, Add a focused migration test for the new tenant account site capabilities migration in the same test suite that covers tenantConfigUpMigration. The test should exercise the migration function registered in init, verify the backfill predicates for targetedInstanceCreation and enableSshAccess on tenant_account.config, and confirm that tenant.config is dropped after the up migration. Use the migration registration and the existing migration test helpers as the entry points so the test stays aligned with the actual apply path.Source: Coding guidelines
rest-api/workflow/pkg/util/testing.go (1)
227-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfig removal is correct, but the identically-named sibling helper is a landmine.
Dropping
config *cdbm.TenantConfighere tracks cleanly withcdbm.Tenantno longer carrying aConfigfield. No objection to the removal itself.However,
rest-api/db/pkg/db/model/testing.goexposes anotherTestBuildTenantwith the reversed first two string parameters —(name, org, user)vs. this one's(org, orgDisplayName, user). That collision is exactly what produced the argument-swap bugs flagged inoperatingsystem_test.goandsite_test.goin this same PR (callers reached for the wrong ordering because two same-named helpers disagree on it). Consider aligning both helpers' parameter order (or renaming one, e.g.TestBuildTenantWithDisplayName) to eliminate this trap going forward.♻️ Illustrative alignment (pick one helper's order and mirror it)
-func TestBuildTenant(t *testing.T, dbSession *cdb.Session, org string, orgDisplayName string, user *cdbm.User) *cdbm.Tenant { +// Parameter order matches db/pkg/db/model.TestBuildTenant: (name, org, user). +func TestBuildTenant(t *testing.T, dbSession *cdb.Session, orgDisplayName string, org string, user *cdbm.User) *cdbm.Tenant {🤖 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/util/testing.go` around lines 227 - 238, The two TestBuildTenant helpers are inconsistent: this util.TestBuildTenant takes (org, orgDisplayName, user) while the sibling db/pkg/db/model.TestBuildTenant uses a different string order, which caused callers to swap arguments accidentally. Align the parameter order across both TestBuildTenant functions, or rename one of them (for example, TestBuildTenantWithDisplayName) so the intended ordering is unambiguous. Update any call sites to match the chosen signature, especially the helper references used in tenant-related tests.rest-api/api/pkg/api/handler/instance.go (1)
4881-4917: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the
IsRepairTenantauthorization read out of the transaction.The
EffectiveTargetedInstanceCreationcapability check at Line 4908 is a pure authorization read that depends only oninstance.Tenantandinstance.SiteID, both resolved before the transaction begins. As written, the closure first mutates state (UpdatetoTerminatingat Line 4883 and aStatusDetailinsert) and only then verifies the caller is permitted to setIsRepairTenant. While the rollback preserves correctness, performing writes ahead of an authorization gate is an inversion of control flow that pins a DB connection for the duration of the check.Relocate the check above
cdb.WithTx(passingnilfor the transaction), returningcutil.NewAPIErrorResponseon the forbidden path.As per path instructions: "Pure input-validation reads ... hold the tx open and pin a DB connection for no benefit. Move them above the
cdb.WithTx(...)call and start the tx only when writes begin."🤖 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/instance.go` around lines 4881 - 4917, The `IsRepairTenant` authorization check in `instance.go` is happening inside `cdb.WithTx` after state-changing writes have already started; move the `common.EffectiveTargetedInstanceCreation` call above the transaction, using the already-loaded `instance.Tenant` and `instance.SiteID` and passing a nil transaction if needed. If the capability is disabled, return `cutil.NewAPIErrorResponse` (forbidden) before opening the transaction, and keep `cdb.WithTx`, `instanceDAO.Update`, and `StatusDetailDAO.Create` limited to the actual write path only.Source: Path instructions
rest-api/api/pkg/api/handler/tenantaccount.go (1)
781-781: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
dbUserparameter.
handleProviderSiteCapabilitiesUpdatetakesdbUser *cdbm.Userbut never references it in the body, unlike its siblinghandleTenantInviteAcceptancewhich uses it for the contact-identity check. If it isn't needed for this provider-admin flow (e.g. for audit trail on the capability change), drop it; otherwise this is arevive/golangci-lintunused-parameter target worth verifying against CI config.🤖 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/tenantaccount.go` at line 781, The handleProviderSiteCapabilitiesUpdate method currently takes dbUser *cdbm.User but never uses it, so update the method signature and all call sites to remove the unused parameter if it is not needed for this provider-admin flow; if it is intended for audit or authorization, reference it inside the method similarly to handleTenantInviteAcceptance so the parameter is no longer unused and lint checks pass.rest-api/api/pkg/api/model/tenantaccountsitecapability.go (3)
105-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMisleading error message when zero global entries are supplied.
validationErrorDuplicateGlobalScope("only one global siteCapabilities entry is allowed") is returned both whenglobalCount > 1and whenglobalCount == 0. In the zero case the message tells the caller the opposite of the actual problem. The handler already uses a clearer phrase for this exact concept ("siteCapabilities must include exactly one global entry", tenantaccount.go:812) — reuse it here.🐛 Proposed fix
- if globalCount != 1 { - return validation.Errors{"siteCapabilities": fmt.Errorf(validationErrorDuplicateGlobalScope)} - } + if globalCount != 1 { + return validation.Errors{"siteCapabilities": fmt.Errorf("siteCapabilities must include exactly one global entry")} + }🤖 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/tenantaccountsitecapability.go` around lines 105 - 107, The validation in tenant account site capability handling uses validationErrorDuplicateGlobalScope for every non-1 globalCount, which makes the zero-global case return a misleading message. Update the check around the siteCapabilities validation so it distinguishes between globalCount == 0 and globalCount > 1, and reuse the clearer “siteCapabilities must include exactly one global entry” wording already used in tenantaccount.go rather than the duplicate-global message for both cases.
182-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
parseSiteCapabilitySiteIDsappears unused and duplicates handler logic.This helper isn't called anywhere in the reviewed handler;
handleProviderSiteCapabilitiesUpdate(tenantaccount.go:834-841) re-implements the identicaluuid.Parseloop inline instead of reusing it. Either wire the handler up to call this helper (removing the duplication) or delete the dead code.🤖 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/tenantaccountsitecapability.go` around lines 182 - 197, The parseSiteCapabilitySiteIDs helper is currently dead code and duplicates the UUID parsing loop already in handleProviderSiteCapabilitiesUpdate. Update the handler to call parseSiteCapabilitySiteIDs so the parsing logic is centralized, or remove parseSiteCapabilitySiteIDs entirely if it is not meant to be reused; use the existing function name and the handler name to locate the duplicated logic.
52-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReinvented validation instead of the canonical typed-slice pattern.
Validate()hand-rolls per-entry iteration, ad-hocfmt.Errorferrors, and inline anonymousvalidation.By(func(value interface{}) error {...})closures for the global/limited scope checks. The established convention here is a typed-sliceValidate()that delegates per-element checks viavalidation.Validate(s, validation.Each())and pulls cross-field checks into named methods referenced withvalidation.By(t.methodName)(seeAPIMachineCapabilities.Validateas the canonical shape). As written, this is harder to unit-test per-rule and diverges from the pattern used elsewhere for the same problem shape (list + per-element + list-level constraints).♻️ Suggested direction
-func (caps APITenantAccountSiteCapabilitiesUpdateRequest) Validate() error { - ... - validation.Field(&cap.SiteIDs, - validation.When(cap.Scope == TenantAccountSiteCapabilityScopeGlobal, - validation.By(func(value interface{}) error { ... })), - validation.When(cap.Scope == TenantAccountSiteCapabilityScopeLimited, - validation.By(func(value interface{}) error { ... }), - validation.Each(validationis.UUID.Error(validationErrorInvalidUUID)), - ), - ), - ... -} +func (cap APITenantAccountSiteCapability) Validate() error { + return validation.ValidateStruct(&cap, + validation.Field(&cap.Scope, validation.Required, validation.In(tenantAccountSiteCapabilityScopes...)), + validation.Field(&cap.SiteIDs, validation.By(cap.validateSiteIDsForScope)), + ) +} + +func (cap APITenantAccountSiteCapability) validateSiteIDsForScope(value interface{}) error { ... } + +func (caps APITenantAccountSiteCapabilitiesUpdateRequest) Validate() error { + if err := validation.Validate([]APITenantAccountSiteCapability(caps), validation.Each()); err != nil { + return err + } + return caps.validateListLevelRules() // global-count-exactly-one, duplicate siteIds +}🤖 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/tenantaccountsitecapability.go` around lines 52 - 110, `APITenantAccountSiteCapabilitiesUpdateRequest.Validate` is hand-rolled instead of following the typed-slice validation pattern used elsewhere. Refactor it to use `validation.Validate(..., validation.Each(...))` for per-item checks and move the scope/siteID cross-field rules into named methods on `APITenantAccountSiteCapabilitiesUpdateRequest` so they can be referenced with `validation.By(...)` like `APIMachineCapabilities.Validate`. Keep the existing rules (global/limited scope, UUID validation, duplicate site IDs, single global entry), but express them through named validators rather than inline anonymous closures and ad-hoc iteration.Source: Path instructions
🤖 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/tenantaccount.go`:
- Around line 781-820: The `handleProviderSiteCapabilitiesUpdate` flow reads
`ta` before entering `cdb.WithTx`, then uses that stale snapshot to build the
advisory lock key and perform ownership checks inside the transaction. Re-fetch
the `TenantAccount` with `taDAO.GetByID` after acquiring the lock inside the
`cdb.WithTx` closure, and use that refreshed entity for the
`TenantID`/`InfrastructureProviderID` validation and subsequent update so the
check-and-write happen against the same transactional snapshot.
In `@rest-api/api/pkg/api/handler/util/common/common.go`:
- Around line 1525-1568: The EffectiveTargetedInstanceCreation helper currently
assumes only one Ready TenantAccount exists and uses tas[0], which makes the
result order-dependent if duplicates slip in. Update the logic in
EffectiveTargetedInstanceCreation to fail closed or otherwise handle the case
where taDAO.GetAll returns more than one Ready account instead of trusting the
first row, and align the tenantaccount creation path in
api/pkg/api/handler/tenantaccount.go with a database-enforced uniqueness
guarantee for tenant/infrastructure provider.
In `@rest-api/api/pkg/api/model/tenantaccount_test.go`:
- Line 232: Add test coverage in NewAPITenantAccount tests for the new
SiteCapabilities derivation, since all current cases pass nil tenantSites and
never exercise tenantAccountSiteCapabilitiesToAPI or
filterTenantSitesForAccount. Update the test setup to include at least one
non-empty tenantSites case with mixed provider IDs and overrides, then assert
the resulting got.SiteCapabilities for global values, per-site overrides,
provider filtering, and enabled/disabled ordering.
In `@rest-api/openapi/oasdiff-breaking-changes-ignore.txt`:
- Around line 13-17: The ignore list still suppresses the removed POST
/v2/org/{org}/nico/machine/{machineId}/bmc-reset path, which hides a real
client-facing breaking change. Update oasdiff-breaking-changes-ignore.txt to
remove that specific ignore entry, and only keep it if the old route is
explicitly documented as deprecated or renamed; use the nearby
/v2/org/{org}/nico/machine/{machineId}/bmc/reset path change as the reference
when adjusting the ignore rules.
In `@rest-api/openapi/spec.yaml`:
- Around line 14481-14498: Document the server-enforced siteCapabilities rules
so the OpenAPI contract matches Validate() in
APITenantAccountSiteCapabilitiesUpdateRequest: the
TenantAccountUpdateRequest.siteCapabilities array must be non-empty, must
contain exactly one global entry, and must not repeat any siteId across entries.
Update the TenantAccountSiteCapability / siteCapabilities descriptions to spell
out these constraints and add minItems: 1 where the array is required so clients
cannot build payloads that pass schema validation but are rejected by the
server.
In `@rest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.go`:
- Line 48: `util.TestBuildTenant` is being called with the tenant org and
display name in the wrong order, so `Tenant.Org` and `OrgDisplayName` are
swapped. Update the `TestBuildTenant` call in `operatingsystem_test.go` to pass
`tnOrg` as the org argument and use a human-readable tenant name for the
display-name argument, matching the `(org, orgDisplayName)` signature.
In `@rest-api/workflow/pkg/activity/site/site_test.go`:
- Line 91: The `util.TestBuildTenant` call in `site_test.go` is passing the
organization arguments in the wrong order, so swap the `"test-tenant"` and
`tnOrg` values to match `TestBuildTenant(t, dbSession, org, orgDisplayName,
user)`. Check the other call site in this test file as well and make sure any
fixture setup that depends on `tenant.Org` uses the corrected organization
value, including the SSH key group fixture.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/sku_test.go`:
- Around line 202-210: `tenantWithoutCapability` is no longer testing the
disabled-capability branch and now duplicates `tenantWithoutAccount`; update the
fixtures in `TestEffectiveTargetedInstanceCreation` and the matching
`TestGetSkuHandler_Handle` case so each subtest isolates a distinct
`EffectiveTargetedInstanceCreation` path. Keep `tenantWithoutCapability` backed
by a ready `TenantAccount` with `Config.TargetedInstanceCreation` set to false,
and leave `tenantWithoutAccount` as the no-`TenantAccount` case, so the two
denied scenarios exercise different branches again.
In `@rest-api/db/pkg/db/model/tenantaccount.go`:
- Around line 504-574: TenantAccountSQLDAO.Update currently performs separate
write operations when input.Config is set, so a nil tx call from the service
account handler can leave the tenant account partially updated if the second
write fails. Update the Update method to make the no-transaction path atomic by
either wrapping the full update flow in a transaction when tx is nil or
combining the field and config changes into a single database statement; keep
the logic centered in TenantAccountSQLDAO.Update and preserve the final GetByID
fetch after the atomic write completes.
---
Duplicate comments:
In `@rest-api/db/pkg/db/model/tenantsite.go`:
- Around line 285-339: Move the Config and RemoveTargetedInstanceCreation
mutual-exclusion validation in TenantSiteSQLDAO.Update to the top of the method,
before any database write or EnableSerialConsole update occurs. Keep the
existing error handling with errors.Wrap(db.ErrInvalidParams, ...) but run it
before building ts or calling db.GetIDB(...).NewUpdate().Exec so invalid input
cannot partially persist changes.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/instance.go`:
- Around line 4881-4917: The `IsRepairTenant` authorization check in
`instance.go` is happening inside `cdb.WithTx` after state-changing writes have
already started; move the `common.EffectiveTargetedInstanceCreation` call above
the transaction, using the already-loaded `instance.Tenant` and
`instance.SiteID` and passing a nil transaction if needed. If the capability is
disabled, return `cutil.NewAPIErrorResponse` (forbidden) before opening the
transaction, and keep `cdb.WithTx`, `instanceDAO.Update`, and
`StatusDetailDAO.Create` limited to the actual write path only.
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Line 781: The handleProviderSiteCapabilitiesUpdate method currently takes
dbUser *cdbm.User but never uses it, so update the method signature and all call
sites to remove the unused parameter if it is not needed for this provider-admin
flow; if it is intended for audit or authorization, reference it inside the
method similarly to handleTenantInviteAcceptance so the parameter is no longer
unused and lint checks pass.
In `@rest-api/api/pkg/api/model/tenantaccountsitecapability.go`:
- Around line 105-107: The validation in tenant account site capability handling
uses validationErrorDuplicateGlobalScope for every non-1 globalCount, which
makes the zero-global case return a misleading message. Update the check around
the siteCapabilities validation so it distinguishes between globalCount == 0 and
globalCount > 1, and reuse the clearer “siteCapabilities must include exactly
one global entry” wording already used in tenantaccount.go rather than the
duplicate-global message for both cases.
- Around line 182-197: The parseSiteCapabilitySiteIDs helper is currently dead
code and duplicates the UUID parsing loop already in
handleProviderSiteCapabilitiesUpdate. Update the handler to call
parseSiteCapabilitySiteIDs so the parsing logic is centralized, or remove
parseSiteCapabilitySiteIDs entirely if it is not meant to be reused; use the
existing function name and the handler name to locate the duplicated logic.
- Around line 52-110: `APITenantAccountSiteCapabilitiesUpdateRequest.Validate`
is hand-rolled instead of following the typed-slice validation pattern used
elsewhere. Refactor it to use `validation.Validate(..., validation.Each(...))`
for per-item checks and move the scope/siteID cross-field rules into named
methods on `APITenantAccountSiteCapabilitiesUpdateRequest` so they can be
referenced with `validation.By(...)` like `APIMachineCapabilities.Validate`.
Keep the existing rules (global/limited scope, UUID validation, duplicate site
IDs, single global entry), but express them through named validators rather than
inline anonymous closures and ad-hoc iteration.
In
`@rest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.go`:
- Around line 15-59: Add a focused migration test for the new tenant account
site capabilities migration in the same test suite that covers
tenantConfigUpMigration. The test should exercise the migration function
registered in init, verify the backfill predicates for targetedInstanceCreation
and enableSshAccess on tenant_account.config, and confirm that tenant.config is
dropped after the up migration. Use the migration registration and the existing
migration test helpers as the entry points so the test stays aligned with the
actual apply path.
In `@rest-api/workflow/pkg/util/testing.go`:
- Around line 227-238: The two TestBuildTenant helpers are inconsistent: this
util.TestBuildTenant takes (org, orgDisplayName, user) while the sibling
db/pkg/db/model.TestBuildTenant uses a different string order, which caused
callers to swap arguments accidentally. Align the parameter order across both
TestBuildTenant functions, or rename one of them (for example,
TestBuildTenantWithDisplayName) so the intended ordering is unambiguous. Update
any call sites to match the chosen signature, especially the helper references
used in tenant-related tests.
🪄 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: 793aae45-fe4f-4e93-af35-1e408b260506
⛔ Files ignored due to path filters (42)
rest-api/sdk/standard/api_tenant.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_tenant_account.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_tenant.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability_scope.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_capabilities.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_summary.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_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (51)
rest-api/api/pkg/api/handler/dpureprovision_test.gorest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancebatch_test.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machinehealth_test.gorest-api/api/pkg/api/handler/rack_test.gorest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/site.gorest-api/api/pkg/api/handler/site_test.gorest-api/api/pkg/api/handler/sku.gorest-api/api/pkg/api/handler/sku_test.gorest-api/api/pkg/api/handler/tenant.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/tray_test.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/model/operatingsystem_test.gorest-api/api/pkg/api/model/sshkeygroup_test.gorest-api/api/pkg/api/model/tenant.gorest-api/api/pkg/api/model/tenant_test.gorest-api/api/pkg/api/model/tenantaccount.gorest-api/api/pkg/api/model/tenantaccount_test.gorest-api/api/pkg/api/model/tenantaccountsitecapability.gorest-api/db/pkg/db/model/tenant.gorest-api/db/pkg/db/model/tenant_test.gorest-api/db/pkg/db/model/tenantaccount.gorest-api/db/pkg/db/model/tenantsite.gorest-api/db/pkg/db/model/tenantsite_test.gorest-api/db/pkg/db/model/testing.gorest-api/db/pkg/migrations/20230412224951_tenant_site.gorest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.gorest-api/db/pkg/migrations/migrations_test.gorest-api/docs/index.htmlrest-api/openapi/oasdiff-breaking-changes-ignore.txtrest-api/openapi/spec.yamlrest-api/workflow/pkg/activity/dpuextensionservice/dpuextensionservice_test.gorest-api/workflow/pkg/activity/infinibandpartition/infinibandpartition_test.gorest-api/workflow/pkg/activity/instance/instance_test.gorest-api/workflow/pkg/activity/nvlinklogicalpartition/nvlinklogicalpartition_test.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.gorest-api/workflow/pkg/activity/site/site_test.gorest-api/workflow/pkg/activity/sshkeygroup/sshkeygroup_test.gorest-api/workflow/pkg/activity/tenant/tenant_test.gorest-api/workflow/pkg/util/testing.go
Comment on lines
+13
to
+17
| POST /v2/org/{org}/nico/machine/{machineId}/bmc-reset api path removed without deprecation | ||
| GET /v2/org/{org}/nico/tenant/current the `capabilities/targetedInstanceCreation` response's property type/format changed from `boolean`/`uuid` to `boolean`/`` for status `200` | ||
| GET /v2/org/{org}/nico/vpc-peering the `items/tenant/capabilities/targetedInstanceCreation` response's property type/format changed from `boolean`/`uuid` to `boolean`/`` for status `200` | ||
| POST /v2/org/{org}/nico/vpc-peering the `tenant/capabilities/targetedInstanceCreation` response's property type/format changed from `boolean`/`uuid` to `boolean`/`` for status `201` | ||
| GET /v2/org/{org}/nico/vpc-peering/{id} the `tenant/capabilities/targetedInstanceCreation` response's property type/format changed from `boolean`/`uuid` to `boolean`/`` for status `200` No newline at end of file |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate bmc-reset references =="
rg -n --hidden --no-ignore-vcs 'bmc-reset|bmc reset|BMC reset|/machine/\{machineId\}/bmc-reset' rest-api openapi .github docs book || true
echo
echo "== inspect openapi ignore file context =="
cat -n rest-api/openapi/oasdiff-breaking-changes-ignore.txt | sed -n '1,80p'
echo
echo "== inspect openapi spec for the endpoint =="
rg -n --hidden --no-ignore-vcs 'machine/.+\breset\b|bmc-reset|BMCReset|bmcReset' rest-api/openapi/spec.yaml rest-api/openapi/**/*.yaml rest-api/openapi/**/*.yml || trueRepository: NVIDIA/infra-controller
Length of output: 50380
Remove the /bmc-reset ignore entry unless the rename is documented.
rest-api/openapi/spec.yaml now exposes POST /v2/org/{org}/nico/machine/{machineId}/bmc/reset; the old hyphenated path is gone. Suppressing that removal here hides a client-facing break. If the change is intentional, add a deprecation/removal note for the old URL instead of ignoring it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/oasdiff-breaking-changes-ignore.txt` around lines 13 - 17,
The ignore list still suppresses the removed POST
/v2/org/{org}/nico/machine/{machineId}/bmc-reset path, which hides a real
client-facing breaking change. Update oasdiff-breaking-changes-ignore.txt to
remove that specific ignore entry, and only keep it if the old route is
explicitly documented as deprecated or renamed; use the nearby
/v2/org/{org}/nico/machine/{machineId}/bmc/reset path change as the reference
when adjusting the ignore rules.
Source: Path instructions
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== current ignore file =="
cat -n rest-api/openapi/oasdiff-breaking-changes-ignore.txt
echo
echo "== search spec for bmc-reset and bmc/reset =="
rg -n 'bmc-reset|bmc/reset|bmcReset' rest-api/openapi/spec.yaml || true
echo
echo "== last commits touching ignore file =="
git log --oneline -n 5 -- rest-api/openapi/oasdiff-breaking-changes-ignore.txtLength of output: 3748
@hwadekar-nv, thanks for the update. However, checking the current state of rest-api/openapi/oasdiff-breaking-changes-ignore.txt on this branch, Line 13 still contains:
POST /v2/org/{org}/nico/machine/{machineId}/bmc-reset api path removed without deprecation
and rest-api/openapi/spec.yaml confirms only the renamed /v2/org/{org}/nico/machine/{machineId}/bmc/reset path exists now. Could you confirm whether the fix has been pushed yet? If it's still pending, I'll hold off on marking this resolved until the ignore entry is removed (or the old path is properly documented as deprecated/renamed).
thossain-nv
reviewed
Jul 8, 2026
| // TenantAccount.config. Site-scoped capability actions must use | ||
| // EffectiveTargetedInstanceCreation, which additionally honors per-site | ||
| // TenantSite.config overrides. | ||
| func TenantHasTargetedInstanceCreation(ctx context.Context, tx *cdb.Tx, dbSession *cdb.Session, tenant *cdbm.Tenant) (bool, error) { |
Contributor
There was a problem hiding this comment.
Let's add optional site *cdbm.Site here so we don't need a separate EffectiveTargetedInstanceCreation
thossain-nv
reviewed
Jul 8, 2026
thossain-nv
left a comment
thossain-nv
left a comment
Contributor
There was a problem hiding this comment.
Need to look through more but added some initial comments.
| // Begin validating Machine ID | ||
| if apiRequest.MachineID != nil { | ||
| if tenant.Config == nil || !tenant.Config.TargetedInstanceCreation { | ||
| enabledForSite, derr := common.EffectiveTargetedInstanceCreation(ctx, tx, cih.dbSession, tenant, site.ID) |
Contributor
There was a problem hiding this comment.
Let's use var privilegedAccess instead of enabledForSite
| logger.Error().Err(serr).Msg("error retrieving Tenant Accounts for privileged Tenant") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Error retrieving Tenant Accounts for privileged Tenant", nil) | ||
| } | ||
| taDAO := cdbm.NewTenantAccountDAO(gamh.dbSession) |
Contributor
There was a problem hiding this comment.
Should we add a utility function GetPrivilegedAccessSitesForTenant that returns all Site IDs where Tenant has privileged access? That'll make the logic in this handler simpler. Instead of filtering by Provider IDs, we should filter by Site IDs here for privileged Tenant. If the returned Site ID count is 0 then we reject this request.
| }, cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)}, []string{"Site"}) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error retrieving Tenant Sites for Tenant Account") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Tenant Sites for Tenant Account", nil) |
Contributor
There was a problem hiding this comment.
Failed to retrieve Tenant Sites for Tenant should be ok.
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
2 times, most recently
from
0080f33 to
f9f5549
Compare
Contributor
Author
|
@coderabbitai full_review, thanks ! |
Contributor
|
ᕕ( ᐛ )ᕗ ✅ Action performedFull review finished. |
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 (1)
rest-api/db/pkg/db/model/tenantsite.go (1)
298-338: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove the Config/RemoveTargetedInstanceCreation guard before any update
The current order can persist
enable_serial_consolebefore returningdb.ErrInvalidParamswhenUpdateis called without a transaction, leaving the row partially updated. Check the mutually exclusive inputs first, then perform any writes.🤖 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/tenantsite.go` around lines 298 - 338, The Update path in tenantsite.go applies the EnableSerialConsole change before validating the mutually exclusive Config and RemoveTargetedInstanceCreation inputs, which can leave the row partially updated when db.ErrInvalidParams is returned. Move the guard check for input.Config and input.RemoveTargetedInstanceCreation in the tenant site update flow ahead of any write logic in the Update method, then keep the existing update branches for enable_serial_console, Config, and RemoveTargetedInstanceCreation after validation.
🧹 Nitpick comments (1)
rest-api/db/pkg/db/model/tenantsite_test.go (1)
285-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReach for range-over-integer for this counting loop.
siteCountis a plain count andiis only read (forSprintfand the modulo check), so this is exactly the "counting loop" case the repo's Go conventions call out forrange.♻️ Proposed refactor
- sites := []*Site{} siteCount := 30 - for i := 0; i < siteCount; i++ { + sites := make([]*Site, 0, siteCount) + for i := range siteCount { site := TestBuildSite(t, dbSession, ip, fmt.Sprintf("test-site-%d", i), ipu) sites = append(sites, site) if i%2 == 0 { TestBuildTenantSite(t, dbSession, tn1, site, nil, tnu1) } else { TestBuildTenantSite(t, dbSession, tn2, site, config, tnu2) } }As per path instructions, "Prefer range-based iteration over C-style
forloops; userange/slices.Backward/range-over-integer where they express the loop more clearly."🤖 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/tenantsite_test.go` around lines 285 - 297, Refactor the counting loop in the tenant site test to use range-over-integer instead of a C-style `for` loop. In the `TestBuildTenantSite` setup block, replace the manual `siteCount`/`i` iteration with a range-based integer loop, keeping the existing `TestBuildSite` and `TestBuildTenantSite` calls, the `fmt.Sprintf("test-site-%d", i)` naming, and the even/odd branch logic intact.Source: Path instructions
🤖 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/util/common/common.go`:
- Around line 1551-1626: The site and expected-machine list authorization is
still using tenant-level privileged access expansion instead of the effective
per-site set. Update the list-path checks in site.go and expectedmachine.go to
call GetPrivilegedAccessSiteIDsForTenant, and use its returned Site IDs for
filtering/authorization so the list endpoints match the single-site behavior,
including TenantSite.config overrides.
In
`@rest-api/workflow/pkg/activity/dpuextensionservice/dpuextensionservice_test.go`:
- Around line 98-100: The test setup in dpuextensionservice_test.go still has
the org/orgDisplayName arguments swapped in the TestBuildTenant call, causing
the tenant org to mismatch the TestBuildUser org. Update the TestBuildTenant
invocation in this test to match the corrected pattern used in site_test.go,
operatingsystem_test.go, and nvlinklogicalpartition_test.go, with the org value
passed first and the display name second so tenant.Org aligns with the user’s
org.
---
Outside diff comments:
In `@rest-api/db/pkg/db/model/tenantsite.go`:
- Around line 298-338: The Update path in tenantsite.go applies the
EnableSerialConsole change before validating the mutually exclusive Config and
RemoveTargetedInstanceCreation inputs, which can leave the row partially updated
when db.ErrInvalidParams is returned. Move the guard check for input.Config and
input.RemoveTargetedInstanceCreation in the tenant site update flow ahead of any
write logic in the Update method, then keep the existing update branches for
enable_serial_console, Config, and RemoveTargetedInstanceCreation after
validation.
---
Nitpick comments:
In `@rest-api/db/pkg/db/model/tenantsite_test.go`:
- Around line 285-297: Refactor the counting loop in the tenant site test to use
range-over-integer instead of a C-style `for` loop. In the `TestBuildTenantSite`
setup block, replace the manual `siteCount`/`i` iteration with a range-based
integer loop, keeping the existing `TestBuildSite` and `TestBuildTenantSite`
calls, the `fmt.Sprintf("test-site-%d", i)` naming, and the even/odd branch
logic intact.
🪄 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: 282a661b-b6f5-4e29-8780-fa9f971905a9
⛔ Files ignored due to path filters (42)
rest-api/sdk/standard/api_tenant.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_tenant_account.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_tenant.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_site_capability_scope.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_account_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_capabilities.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_summary.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_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (51)
rest-api/api/pkg/api/handler/dpureprovision_test.gorest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancebatch_test.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machinehealth_test.gorest-api/api/pkg/api/handler/rack_test.gorest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/site.gorest-api/api/pkg/api/handler/site_test.gorest-api/api/pkg/api/handler/sku.gorest-api/api/pkg/api/handler/sku_test.gorest-api/api/pkg/api/handler/tenant.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/tray_test.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/model/operatingsystem_test.gorest-api/api/pkg/api/model/sshkeygroup_test.gorest-api/api/pkg/api/model/tenant.gorest-api/api/pkg/api/model/tenant_test.gorest-api/api/pkg/api/model/tenantaccount.gorest-api/api/pkg/api/model/tenantaccount_test.gorest-api/api/pkg/api/model/tenantaccountsitecapability.gorest-api/db/pkg/db/model/tenant.gorest-api/db/pkg/db/model/tenant_test.gorest-api/db/pkg/db/model/tenantaccount.gorest-api/db/pkg/db/model/tenantsite.gorest-api/db/pkg/db/model/tenantsite_test.gorest-api/db/pkg/db/model/testing.gorest-api/db/pkg/migrations/20230412224951_tenant_site.gorest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.gorest-api/db/pkg/migrations/migrations_test.gorest-api/docs/index.htmlrest-api/openapi/oasdiff-breaking-changes-ignore.txtrest-api/openapi/spec.yamlrest-api/workflow/pkg/activity/dpuextensionservice/dpuextensionservice_test.gorest-api/workflow/pkg/activity/infinibandpartition/infinibandpartition_test.gorest-api/workflow/pkg/activity/instance/instance_test.gorest-api/workflow/pkg/activity/nvlinklogicalpartition/nvlinklogicalpartition_test.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.gorest-api/workflow/pkg/activity/site/site_test.gorest-api/workflow/pkg/activity/sshkeygroup/sshkeygroup_test.gorest-api/workflow/pkg/activity/tenant/tenant_test.gorest-api/workflow/pkg/util/testing.go
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
from
2bbf0ac to
a8e6e5c
Compare
Move privileged tenant capability configuration to TenantAccount.config and TenantSite.config, expose it on TenantAccount GET/PATCH, and enforce effective site-scoped checks on instance, machine, and VPC actions while deprecating the legacy Tenant.capabilities field.
hwadekar-nv
force-pushed
the
feat/tenant-capabilities-update
branch
from
a8e6e5c to
a26937d
Compare
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
2 participants
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
Tenant TargetedInstanceCreation capability
Currently We allow Tenant to config capability via Tenant config. As it is a global
confighere which problematic. We can use this for flags that apply to all Providers/Sites. Likely not going to be used unless there is such use case.Today the API Tenant object returns a
capabilitiesobject with a single fieldtargetedInstanceCreation. Because this value can vary per SiteThis PR contain Tenant to configure capabilities for custom sites as well as providers, also we will deprecate field
configfrom Tenant and use newly approach to validate and configure capabilities (TenantAccount)Type of Change
Related Issues (Optional)
#2104
Breaking Changes
Testing
Additional Notes