feat(rest-api): Tenant TargetedInstanceCreation capability implementation#2134
Open
hwadekar-nv wants to merge 20 commits into
Open
feat(rest-api): Tenant TargetedInstanceCreation capability implementation#2134hwadekar-nv wants to merge 20 commits into
hwadekar-nv wants to merge 20 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:
Summary by CodeRabbit
WalkthroughThis PR moves targeted instance creation from tenant-level configuration to tenant-account and tenant-site scoped JSONB configuration. It adds site-capability APIs and resolution helpers, updates authorization paths and persistence models, migrates existing data, and documents the deprecation and replacement contract. 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)rest-api/docs/index.htmlast-grep timed out on this file 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
|
ᕕ( ᐛ )ᕗ ✅ Action performedFull review finished. |
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 (4)
rest-api/api/pkg/api/handler/machine.go (2)
348-386: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing page limit can cause valid
tenantIdvalues to be wrongly rejected.
tenantAccountDAO.GetAll(...)at Line 362-365 is called withcdbp.PageInput{}(noLimit), so if the number of matchingTenantAccountrows exceeds the DAO's default page size, some legitimately-associatedtenantIds won't appear intenantAccounts, and the loop at Line 374-378 will incorrectly return 403 for them. Elsewhere in this codebase,cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)}is used explicitly whenever all matching rows are required.🐛 Proposed fix
tenantAccounts, _, err := tenantAccountDAO.GetAll(ctx, nil, cdbm.TenantAccountFilterInput{ TenantIDs: tenantIDs, InfrastructureProviderID: &infrastructureProvider.ID, - }, cdbp.PageInput{}, nil) + }, cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)}, nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/machine.go` around lines 348 - 386, Update the tenantAccountDAO.GetAll call in the tenantId validation flow to request all matching records by passing a PageInput with Limit set to cdbp.TotalLimit via cutil.GetPtr. Keep the tenantIDsMap construction and subsequent authorization checks unchanged.
239-288: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDual-role machine filters are over-constrained.
When both
infrastructureProviderandtenantare set, this branch appliesInfrastructureProviderIDsandSiteIDstogether.MachineSQLDAO.setQueryWithFilteradds both predicates, so the query becomes an intersection and can drop valid machines for callers that hold both roles. Build a singleSiteIDslist that unions provider-owned and tenant-privileged sites, as the adjacent handlers do.🤖 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 239 - 288, Update the machine filter construction around filterInput, infrastructureProvider, and privilegedSiteIDs so dual-role requests use one SiteIDs list that unions infrastructure-provider-owned sites with tenant-privileged sites instead of applying InfrastructureProviderIDs and SiteIDs as an intersection. Preserve the existing siteId validation and authorization behavior, while ensuring callers with both roles can retrieve machines from either authorized source.rest-api/api/pkg/api/model/tenantaccount.go (1)
72-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject empty
siteCapabilitiespayloads.HasSiteCapabilities()treats[]the same as an omitted field, so an explicit empty array falls through to tenant invite acceptance instead of returning 400. Track field presence explicitly so a provided but emptysiteCapabilitiesbody is rejected.🤖 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/tenantaccount.go` around lines 72 - 86, Update APITenantAccountUpdateRequest and HasSiteCapabilities so field presence is tracked separately from slice length, allowing an explicitly provided empty siteCapabilities array to be distinguished from an omitted field. Reject the explicit empty payload with the existing validation error path before tenant invite acceptance, while preserving current handling for omitted and non-empty payloads.Source: Path instructions
rest-api/api/pkg/api/handler/tenantaccount.go (1)
919-948: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
AllocationCounthardcoded to0in the siteCapabilities update response.Unlike
GetTenantAccountHandler(Lines 551-563), which computes the real count viaaDAO.GetCount(...), this path always returnsAllocationCount: 0at Line 944 — even though this endpoint updates capabilities on ReadyTenantAccounts that may already have active Allocations. Clients relying on this response field after a PATCH will see a misleading0.🐛 Proposed fix mirroring GetTenantAccountHandler
+ aDAO := cdbm.NewAllocationDAO(utah.dbSession) + allocationCount := 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) + } + allocationCount = cnt + } + - apiInstance := model.NewAPITenantAccount(ta, ssds, 0, tenantSites) + apiInstance := model.NewAPITenantAccount(ta, ssds, allocationCount, 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 919 - 948, Replace the hardcoded allocation count passed to model.NewAPITenantAccount in the site-capabilities update handler with the real count, mirroring GetTenantAccountHandler: initialize the allocation DAO, call GetCount for the updated TenantAccount, handle any error consistently with the surrounding database queries, and pass the resulting count into the response.
🧹 Nitpick comments (2)
rest-api/api/pkg/api/handler/tenantaccount.go (1)
397-449: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winList endpoint drops per-site capability overrides.
GetAllTenantAccountHandleralways passesnilfortenantSites(Line 447), sotenantAccountSiteCapabilitiesToAPIonly ever emits the global entry for every account in a list response — any "limited" per-site override is silently omitted, unlikeGetTenantAccountHandler's single-account response which fetches and includestenantSites. This creates a data-completeness discrepancy between list and detail views of the same resource.Consider batching one
TenantSiteDAO.GetAllcall keyed by the union ofTenantIDspresent intas, grouping the results byTenantID, and passing the matching slice per account instead ofnil.🤖 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 - 449, Update GetAllTenantAccountHandler to batch-fetch tenant sites once using the union of non-nil TenantIDs in tas, group the results by TenantID, and pass each account’s matching tenant-site slice to NewAPITenantAccount instead of nil. Preserve empty slices for accounts without sites and keep the existing allocation-count logic unchanged.rest-api/api/pkg/api/model/tenantaccountsitecapability.go (1)
186-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead helper or route the handler through it
parseSiteCapabilitySiteIDsinrest-api/api/pkg/api/model/tenantaccountsitecapability.go:186is unused, whilerest-api/api/pkg/api/handler/tenantaccount.go:851parses the sameLimited-scopeSiteIDsinline. Delete the helper or call it from the handler so this parsing logic has a single owner.🤖 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 186 - 201, The helper parseSiteCapabilitySiteIDs is unused while tenantaccount.go duplicates its Limited-scope SiteIDs parsing inline. Route the handler through parseSiteCapabilitySiteIDs and remove the duplicated parsing logic, or delete the helper if the handler should remain inline; ensure only one implementation owns this parsing behavior.
🤖 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/expectedmachine_test.go`:
- Around line 633-643: Adjust the Ready TenantAccount setup in the external-site
handler tests so it does not globally enable the capability for unmanagedIP.
Disable TargetedInstanceCreation in its Config or remove this account, ensuring
TenantSite authorization remains the sole access grant while preserving the
intended test setup.
In `@rest-api/api/pkg/api/handler/expectedpowershelf.go`:
- Line 74: Resolve each requested Site before authorization and pass a populated
TenantPrivilegeScope containing its SiteID and InfrastructureProviderID to all
authorization calls in rest-api/api/pkg/api/handler/expectedpowershelf.go at
lines 74, 251, 408, 516, and 686; expectedrack.go at lines 74, 250, 407, 513,
696, 824, and 989; and expectedswitch.go at lines 74, 278, 435, 543, and 742.
Preserve tenant/resource ownership checks; use nil only if these operations are
intentionally not capability-gated.
In `@rest-api/api/pkg/api/handler/instance.go`:
- Around line 894-903: Move the targeted instance creation capability checks in
CreateInstanceHandler and DeleteInstanceHandler before their respective
cdb.WithTx calls, invoking common.TenantHasTargetedInstanceCreation with nil for
the transaction. Preserve the existing error and forbidden responses, using the
already available site and instance tenant/site/provider values, and remove the
in-transaction checks so validation occurs before any transaction writes or
locks.
In `@rest-api/api/pkg/api/model/tenantaccountsitecapability.go`:
- Around line 168-184: Update filterTenantSitesForAccount to fail closed when
TenantSite.Site is nil: remove the branch that appends entries with missing Site
relations, so only sites whose InfrastructureProviderID matches the tenant
account are included.
- Around line 61-104: Normalize each validated siteID to its canonical UUID
representation before checking or recording it in seenSiteIDs within the
capability validation loop. Use the parsed UUID’s canonical String value as the
deduplication key, while preserving the existing duplicate-siteID validation
error and validation flow.
In `@rest-api/db/pkg/db/model/tenantaccount.go`:
- Around line 550-559: The tenant-account update flow must keep ordinary-field
and JSONB configuration changes atomic when tx is nil. Update the surrounding
method containing the input.Config block to combine both changes into one
UPDATE, or require and use a transaction whenever both updates are needed,
ensuring a configuration failure cannot leave the earlier field changes
committed.
In `@rest-api/db/pkg/migrations/migrations_test.go`:
- Around line 902-909: Update the tenantConfigUpMigration test to query
tenant1.config and assert the backfilled value equals an empty JSON object,
rather than only counting rows where config IS NULL. Preserve the existing error
assertion and use the test’s established JSON/value representation.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/machine.go`:
- Around line 348-386: Update the tenantAccountDAO.GetAll call in the tenantId
validation flow to request all matching records by passing a PageInput with
Limit set to cdbp.TotalLimit via cutil.GetPtr. Keep the tenantIDsMap
construction and subsequent authorization checks unchanged.
- Around line 239-288: Update the machine filter construction around
filterInput, infrastructureProvider, and privilegedSiteIDs so dual-role requests
use one SiteIDs list that unions infrastructure-provider-owned sites with
tenant-privileged sites instead of applying InfrastructureProviderIDs and
SiteIDs as an intersection. Preserve the existing siteId validation and
authorization behavior, while ensuring callers with both roles can retrieve
machines from either authorized source.
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Around line 919-948: Replace the hardcoded allocation count passed to
model.NewAPITenantAccount in the site-capabilities update handler with the real
count, mirroring GetTenantAccountHandler: initialize the allocation DAO, call
GetCount for the updated TenantAccount, handle any error consistently with the
surrounding database queries, and pass the resulting count into the response.
In `@rest-api/api/pkg/api/model/tenantaccount.go`:
- Around line 72-86: Update APITenantAccountUpdateRequest and
HasSiteCapabilities so field presence is tracked separately from slice length,
allowing an explicitly provided empty siteCapabilities array to be distinguished
from an omitted field. Reject the explicit empty payload with the existing
validation error path before tenant invite acceptance, while preserving current
handling for omitted and non-empty payloads.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Around line 397-449: Update GetAllTenantAccountHandler to batch-fetch tenant
sites once using the union of non-nil TenantIDs in tas, group the results by
TenantID, and pass each account’s matching tenant-site slice to
NewAPITenantAccount instead of nil. Preserve empty slices for accounts without
sites and keep the existing allocation-count logic unchanged.
In `@rest-api/api/pkg/api/model/tenantaccountsitecapability.go`:
- Around line 186-201: The helper parseSiteCapabilitySiteIDs is unused while
tenantaccount.go duplicates its Limited-scope SiteIDs parsing inline. Route the
handler through parseSiteCapabilitySiteIDs and remove the duplicated parsing
logic, or delete the helper if the handler should remain inline; ensure only one
implementation owns this parsing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e34d3bd1-3c9c-4069-bdc1-dba212dba6ad
⛔ Files ignored due to path filters (9)
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/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_*.go
📒 Files selected for processing (63)
rest-api/api/pkg/api/handler/allocation.gorest-api/api/pkg/api/handler/bmcreset.gorest-api/api/pkg/api/handler/dpureprovision.gorest-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/expectedpowershelf.gorest-api/api/pkg/api/handler/expectedrack.gorest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/fabric.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/instancetype.gorest-api/api/pkg/api/handler/ipblock.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machinehealth.gorest-api/api/pkg/api/handler/machinehealth_test.gorest-api/api/pkg/api/handler/machinepower.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/handler/vpcpeering.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
|
|
||
| // ensure our user is a provider or tenant for the org | ||
| infrastructureProvider, tenant, apiError := common.IsProviderOrTenant(ctx, logger, cepsh.dbSession, org, dbUser, false, true) | ||
| infrastructureProvider, tenant, apiError := common.IsProviderOrTenant(ctx, logger, cepsh.dbSession, org, dbUser, false, &common.TenantPrivilegeScope{}) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Resolve expected-resource authorization with concrete Site scope.
All three handlers pass a non-nil empty TenantPrivilegeScope, so site-specific capability configuration cannot be evaluated against the requested Site. Resolve the Site before the privilege check and pass its SiteID and InfrastructureProviderID, or pass nil if these operations are not intended to be capability-gated.
rest-api/api/pkg/api/handler/expectedpowershelf.go#L74-L74: apply the concrete scope to all five ExpectedPowerShelf authorization calls at Lines 74, 251, 408, 516, and 686.rest-api/api/pkg/api/handler/expectedrack.go#L74-L74: apply the concrete scope to all seven ExpectedRack authorization calls at Lines 74, 250, 407, 513, 696, 824, and 989.rest-api/api/pkg/api/handler/expectedswitch.go#L74-L74: apply the concrete scope to all five ExpectedSwitch authorization calls at Lines 74, 278, 435, 543, and 742.
As per path instructions, REST API authorization must preserve tenant/resource ownership checks. Based on the supplied authorization contract, non-nil scopes invoke site-aware capability enforcement.
📍 Affects 3 files
rest-api/api/pkg/api/handler/expectedpowershelf.go#L74-L74(this comment)rest-api/api/pkg/api/handler/expectedrack.go#L74-L74rest-api/api/pkg/api/handler/expectedswitch.go#L74-L74
🤖 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/expectedpowershelf.go` at line 74, Resolve each
requested Site before authorization and pass a populated TenantPrivilegeScope
containing its SiteID and InfrastructureProviderID to all authorization calls in
rest-api/api/pkg/api/handler/expectedpowershelf.go at lines 74, 251, 408, 516,
and 686; expectedrack.go at lines 74, 250, 407, 513, 696, 824, and 989; and
expectedswitch.go at lines 74, 278, 435, 543, and 742. Preserve tenant/resource
ownership checks; use nil only if these operations are intentionally not
capability-gated.
Source: Path instructions
Contributor
|
ᕕ( ᐛ )ᕗ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
rest-api/api/pkg/api/handler/machine.go (2)
2037-2063: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDPU handler doesn't actually check the Site-effective capability for Tenants.
isAssociatedfor the Tenant path only confirms someTenantAccountrow exists between the Tenant and the Site's Infrastructure Provider — noStatuses: []string{cdbm.TenantAccountStatusReady}filter and noConfig.TargetedInstanceCreation/TenantSiteoverride check at all. Combined with the coarse pre-gate above (which only requires the capability to be effective somewhere, not on this Site), a Tenant withTargetedInstanceCreationenabled on an unrelated Site, plus a merely-existing (e.g. Pending, capability-disabled)TenantAccountfor this Site's Provider, can view DPU details for Machines they have no capability-based access to. Every sibling handler in this file (GetMachineHandler,UpdateMachineHandler) already resolves this correctly viacommon.TenantHasTargetedInstanceCreation(ctx, nil, dbSession, tenant, common.SiteScope(site)), andsiteis already available here.🔒 Proposed fix
- // Validate role: Provider Admins, or privileged Tenant Admins - provider, tenant, apiError := common.IsProviderOrTenant(ctx, logger, gadmh.dbSession, org, dbUser, false, &common.TenantPrivilegeScope{}) + // Validate role: Provider Admins, or Tenant Admins (site-effective capability enforced below) + provider, tenant, apiError := common.IsProviderOrTenant(ctx, logger, gadmh.dbSession, org, dbUser, false, nil) if apiError != nil { return cutil.NewAPIErrorResponse(c, apiError.Code, apiError.Message, apiError.Data) } // Validate org is associated with the Machine's Site: the org's Provider - // owns the Site, or the org's (privileged) Tenant has a Tenant Account on the - // Site's Infrastructure Provider. + // owns the Site, or the org's Tenant has effective TargetedInstanceCreation + // for the Site. isAssociated := false if provider != nil { isAssociated = site.InfrastructureProviderID == provider.ID } else if tenant != nil { - taDAO := cdbm.NewTenantAccountDAO(gadmh.dbSession) - _, taCount, serr := taDAO.GetAll(ctx, nil, cdbm.TenantAccountFilterInput{ - InfrastructureProviderID: &site.InfrastructureProviderID, - TenantIDs: []uuid.UUID{tenant.ID}, - }, cdbp.PageInput{}, []string{}) + enabled, serr := common.TenantHasTargetedInstanceCreation(ctx, nil, gadmh.dbSession, tenant, common.SiteScope(site)) if serr != nil { - logger.Error().Err(serr).Msg("error retrieving Tenant Account with Site's Infrastructure Provider") + logger.Error().Err(serr).Msg("error resolving TargetedInstanceCreation for Tenant/Site") return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to determine org's association with Site, DB error", nil) } - isAssociated = taCount > 0 + isAssociated = enabled }🤖 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 2037 - 2063, Update the Tenant branch of the machine association check in the relevant DPU handler to use common.TenantHasTargetedInstanceCreation(ctx, nil, gadmh.dbSession, tenant, common.SiteScope(site)) instead of counting TenantAccount rows. Preserve the existing provider ownership check, error response behavior, and final forbidden response while ensuring the tenant capability is effective for this specific site.Source: Path instructions
348-369: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet an explicit limit for the tenant lookup
cdbp.PageInput{}falls back to the paginator default limit of 20. If a request includes more than 20 distincttenantIdvalues, the lookup can miss valid matches and return a false 403. Use an explicit limit or an unpaged path here.🤖 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 348 - 369, Update the tenant lookup using tenantAccountDAO.GetAll in the infrastructureProvider branch to avoid the default paginator limit of 20. Supply an explicit PageInput limit large enough for all requested tenantIDs, or use the available unpaged lookup path, so every tenantId is evaluated before authorization.rest-api/api/pkg/api/handler/sku_test.go (1)
179-220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
tenantWithoutCapabilityno longer differs fromtenantWithoutAccount— the "capability disabled" branch is untested.Previously
tenantWithoutCapabilitycarriedcdbm.TenantConfig{TargetedInstanceCreation: false}on theTenantitself, distinguishing it fromtenantWithoutAccount(no account at all). Now neithertenantWithoutCapabilitynortenantWithoutAccounthas anyTenantAccountinserted for their org — onlytenantWithCapability/tenantOrggets one (lines 190-200 / 465-475). As a result, the "tenant without TargetedInstanceCreation capability is denied" test case and "tenant without TenantAccount with Provider is denied" test case now exercise the exact same "no account at all" code path, and the intended "Ready account exists but capability disabled" branch is no longer covered by either test file.Add a
TenantAccountfixture fortenantOrgNoCapabilitywithStatus: cdbm.TenantAccountStatusReadyandConfigomitted (or explicitlyfalse) so the disabled-capability branch is actually exercised, distinct from the missing-account branch.🧪 Proposed fix for TestGetAllSkuHandler_Handle (apply analogously in TestGetSkuHandler_Handle)
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) + + // Account exists and is Ready, but capability is explicitly disabled — + // distinct from the "no account at all" scenario below. + 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)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. Avoid reusing a fixture that would trigger a different validation earlier."
Also applies to: 454-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 179 - 220, Add a ready TenantAccount fixture for tenantOrgNoCapability, linked to tenantWithoutCapability and the existing infrastructure provider, with TargetedInstanceCreation omitted or false; keep tenantWithoutAccount without an account so the two tests exercise distinct validation branches. Apply the same fixture correction in TestGetSkuHandler_Handle.Source: Learnings
rest-api/db/pkg/db/model/tenant.go (1)
55-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winScope the
Deprecated:annotation toTargetedInstanceCreationonly, not the wholeTenantConfigtype.Per prior review feedback, only the
TargetedInstanceCreation-related capability was meant to be marked deprecated here — notEnableSSHAccess, which has no documented replacement path in this PR (TenantAccount.config/TenantSite.configonly coverTargetedInstanceCreation). Marking the entire typeDeprecated:risks misleading future maintainers into thinkingEnableSSHAccessis also being retired.✍️ Suggested doc scoping
-// TenantConfig captures legacy per-Tenant capability flags. -// -// Deprecated: Tenant capabilities now live on TenantAccount.config -// (provider-scoped) and TenantSite.config (per-site overrides). This type and -// the Tenant.Config column it maps are retained only so that, during a rolling -// deployment, API pods still running the previous release can keep reading the -// column after the capability migration has run. Do not add new readers or -// writers; remove once no supported release reads tenant.config. +// TenantConfig captures legacy per-Tenant capability flags. type TenantConfig struct { - EnableSSHAccess bool `json:"enableSshAccess"` - TargetedInstanceCreation bool `json:"targetedInstanceCreation"` + EnableSSHAccess bool `json:"enableSshAccess"` + + // Deprecated: TargetedInstanceCreation now lives on TenantAccount.config + // (provider-scoped) and TenantSite.config (per-site overrides). Retained + // only so that, during a rolling deployment, API pods still running the + // previous release can keep reading the column after the capability + // migration has run. Do not add new readers or writers; remove once no + // supported release reads tenant.config.targetedInstanceCreation. + TargetedInstanceCreation bool `json:"targetedInstanceCreation"` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/db/model/tenant.go` around lines 55 - 66, Move the deprecation documentation from the TenantConfig type to the TargetedInstanceCreation field only. Keep EnableSSHAccess and the TenantConfig type undocumented as deprecated, and scope the legacy migration explanation to TargetedInstanceCreation without changing its JSON mapping.rest-api/db/pkg/db/model/tenantsite.go (1)
291-330: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winComment overstates a guarantee the code doesn't implement.
"Validate mutually exclusive inputs before applying any write so a rejected request cannot leave the row partially updated" implies active runtime validation/rejection, but no such check exists — mutual exclusivity between the merge and remove branches is only guaranteed structurally (single pointer field, can't be both nil and non-nil). Meanwhile
EnableSerialConsoleandConfigare still applied via two separateExec()calls; if this DAO method is ever invoked withtx == nil, a failure in the second call after the first succeeds would leave the row partially updated — the exact scenario the comment claims is prevented.Recommend rewriting the comment to describe what's actually guaranteed (type-safe exclusivity between merge/remove), or, for full correctness, collapsing both writes into a single
Exec()/CTE so partial application is impossible regardless of caller.📝 Minimal fix: correct the comment
- // Validate mutually exclusive inputs before applying any write so a - // rejected request cannot leave the row partially updated. + // EnableSerialConsole and Config are applied via separate statements below. + // Config.TargetedInstanceCreation nil vs non-nil selects between removing + // and merging the key; the two are structurally exclusive since they share + // a single pointer field. Callers must supply a transaction (tx) if both + // EnableSerialConsole and Config are set, to avoid a partially applied + // update on failure. updatedFields := []string{}🤖 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 291 - 330, Rewrite the comment above updatedFields in the tenant-site update flow to state only that the Config merge and removal paths are mutually exclusive through the single TargetedInstanceCreation pointer; remove the unsupported claim about pre-write validation and preventing partial updates. Do not change the separate EnableSerialConsole and Config Exec operations.Source: Path instructions
🧹 Nitpick comments (4)
rest-api/api/pkg/api/model/tenantaccountsitecapability.go (1)
98-104: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDuplicate-siteId check is case-sensitive on raw strings.
seenSiteIDscomparescap.SiteIDsverbatim; two entries carrying the same UUID with different casing (e.g. mixed-case vs. lowercase) would slip past this check even though they resolve to one Site, since duplicates are only caught when the raw strings match. Since limited-scope entries are already validated as UUIDs by this point, canonicalize before comparing.🛡️ Proposed fix
- for _, siteID := range cap.SiteIDs { - if _, ok := seenSiteIDs[siteID]; ok { - return validation.Errors{"siteCapabilities": fmt.Errorf(validationErrorDuplicateSiteID)} - } - seenSiteIDs[siteID] = struct{}{} - } + for _, siteIDStr := range cap.SiteIDs { + id, err := uuid.Parse(siteIDStr) + if err != nil { + continue // format already validated above + } + if _, ok := seenSiteIDs[id]; ok { + return validation.Errors{"siteCapabilities": fmt.Errorf(validationErrorDuplicateSiteID)} + } + seenSiteIDs[id] = struct{}{} + }(and change
seenSiteIDs := map[string]struct{}{}tomap[uuid.UUID]struct{}{})🤖 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 98 - 104, Update the duplicate check in the capability validation loop to parse each validated cap.SiteIDs value into a uuid.UUID and track seenSiteIDs as map[uuid.UUID]struct{} instead of raw strings. Compare the canonical UUID values so differently cased representations are treated as duplicates, while preserving the existing validation error and registration flow.rest-api/api/pkg/api/handler/instance.go (1)
890-903: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the capability check above the transaction boundary.
This is a pure authorization read with no dependency on the transaction (the Machine advisory lock that follows is unrelated to it), yet it executes inside
cdb.WithTx. As per path instructions, "Perform pure validation reads outside transactions; keep reads inside only when required for locking, race-safe write decisions, or read-your-writes consistency."tenantandsiteare already resolved before the transaction starts, so this check (and itsDeleteInstanceHandlercounterpart) can be moved abovecdb.WithTx, avoiding an unrelated DB round-trip while holding a transaction open — matching howvpc.goalready places the identical check outside its transaction.♻️ Proposed refactor
+ // Verify capability before opening the transaction — this is a pure + // authorization read unrelated to the Machine advisory lock below. + var privilegedAccess bool + if apiRequest.MachineID != nil { + privilegedAccess, err = common.TenantHasTargetedInstanceCreation(ctx, nil, cih.dbSession, tenant, common.SiteScope(site)) + if err != nil { + logger.Error().Err(err).Msg("error checking effective targeted instance creation for Site") + return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to verify capability for Site", nil) + } + if !privilegedAccess { + logger.Warn().Msg("tenant does not have capability to create instances from specific machine") + return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Tenant does not have capability to create Instances using specific Machine ID", nil) + } + } + err = cdb.WithTx(ctx, cih.dbSession, func(tx *cdb.Tx) error { // ==================== Step 4: Machine Selection ==================== // Begin validating Machine ID if apiRequest.MachineID != nil { - privilegedAccess, derr := common.TenantHasTargetedInstanceCreation(ctx, tx, cih.dbSession, tenant, common.SiteScope(site)) - if derr != nil { - logger.Error().Err(derr).Msg("error checking effective targeted instance creation for Site") - return cutil.NewAPIError(http.StatusInternalServerError, "Failed to verify capability for Site", nil) - } - if !privilegedAccess { - logger.Warn().Msg("tenant does not have capability to create instances from specific machine") - return cutil.NewAPIError(http.StatusForbidden, "Tenant does not have capability to create Instances using specific Machine ID", nil) - } - mDAO := cdbm.NewMachineDAO(cih.dbSession)🤖 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 890 - 903, Move the TenantHasTargetedInstanceCreation capability check for apiRequest.MachineID out of the cdb.WithTx callback and execute it before the transaction begins, using the already resolved tenant and site values. Preserve its existing error responses and authorization behavior, and apply the same hoisting to the corresponding DeleteInstanceHandler check.Source: Path instructions
rest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.go (1)
101-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why the down migration is a no-op.
Unlike the up migration, which explicitly documents why
tenant.configisn't dropped (rolling-deploy compatibility), the down migration silently does nothing — it neither drops the newtenant_account.configcolumn nor reverts thetenant_site.confignormalization. A short comment explaining this (e.g., forward-only migration policy, or explicit call-out that reverting would be unsafe post rolling-deploy) would match the up-migration's documentation standard.As per path instructions, "Document intentionally omitted code that readers might expect and add TODO comments for deferred features or nuances."
🤖 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 101 - 104, Document the intentional no-op in the down migration callback for tenant_account.config with a concise comment explaining why rollback is omitted, such as the forward-only policy or rolling-deploy safety concerns. Keep the existing callback behavior unchanged and align the explanation with the up migration’s compatibility documentation.Source: Path instructions
rest-api/api/pkg/api/handler/tenantaccount.go (1)
781-913: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo audit trail recorded for provider siteCapabilities changes;
dbUseris unused.
handleProviderSiteCapabilitiesUpdatereceivesdbUserbut never references it, and — unlikeCreateTenantAccountHandlerandhandleTenantInviteAcceptancein this same file — never creates aStatusDetailrecord for the change. A Provider Admin can silently flip a tenant's global/per-siteTargetedInstanceCreationcapability with no attributable history of who made the change or when.As per path instructions, REST API handler changes should be reviewed for "audit logging." Consider recording a
StatusDetail(or equivalent) entry referencingdbUser.IDbefore the closure returns.🔍 Suggested addition
for _, ts := range allTenantSites { ... } + sdDAO := cdbm.NewStatusDetailDAO(utah.dbSession) + _, derr = sdDAO.Create(ctx, tx, cdbm.StatusDetailCreateInput{ + EntityID: ta.ID.String(), + Status: ta.Status, + Message: cutil.GetPtr(fmt.Sprintf("siteCapabilities updated by user %s", dbUser.ID.String())), + }) + if derr != nil { + logger.Error().Err(derr).Msg("error creating Status Detail for TenantAccount siteCapabilities update") + return cutil.NewAPIError(http.StatusInternalServerError, "Failed to update Tenant Account capabilities", nil) + } + 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/handler/tenantaccount.go` around lines 781 - 913, Update handleProviderSiteCapabilitiesUpdate to use dbUser and record an attributable StatusDetail (or the established equivalent) for the provider siteCapabilities change, including dbUser.ID, before the transaction closure returns. Follow the existing audit-recording patterns in CreateTenantAccountHandler and handleTenantInviteAcceptance, and ensure the audit write participates in the same transaction as the capability updates.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/bmcreset.go`:
- Line 87: Bind TargetedInstanceCreation authorization to each target machine’s
provider and site instead of using an empty TenantPrivilegeScope. In
bmcreset.go:87, machinehealth.go:79, machinehealth.go:224, machinehealth.go:359,
and dpureprovision.go:91, load the target machine/site before calling
common.IsProviderOrTenant, populate the scope with that machine’s provider and
site capabilities, and perform the scoped authorization check before the
corresponding operation.
In `@rest-api/api/pkg/api/handler/machine.go`:
- Around line 263-288: Update the no-siteId branch in the machine listing
handler so filterInput.SiteIDs is assigned privilegedSiteIDs only for
tenant-only callers, where infrastructureProvider is nil. Prevent dual-role
callers with both tenant and infrastructureProvider from having their
provider-wide filter narrowed by tenant-privileged sites; preserve the existing
provider and explicit-site filtering behavior.
In `@rest-api/api/pkg/api/handler/util/common/testing.go`:
- Around line 351-362: Update the tenant-account fixture creation in the helper
around taDAO.Create to use require.NoError(t, err) instead of assert.Nil(t,
err), ensuring setup failures stop execution before returning ta.
In `@rest-api/api/pkg/api/model/tenantaccountsitecapability.go`:
- Around line 63-89: In the validation rules for cap.SiteIDs within the tenant
capability validation, replace both validation.By closures with ozzo-validation
built-ins: use validation.Empty when cap.Scope is
TenantAccountSiteCapabilityScopeGlobal and validation.Required when it is
TenantAccountSiteCapabilityScopeLimited. Preserve the existing validationis.UUID
rule and the corresponding error messages by applying the appropriate Error
mappings to the built-in rules.
In `@rest-api/db/pkg/migrations/migrations_test.go`:
- Around line 894-909: Expand the test around tenantConfigUpMigration to seed
multiple tenants with NULL configs and at least one tenant retaining a non-NULL
config. After the migration, verify every NULL config is backfilled and the
pre-existing non-NULL value remains unchanged, preserving multi-row coverage.
In `@rest-api/openapi/spec.yaml`:
- Around line 109-117: Update the Tenant Account description to remove the
sentence about site-scoped overrides via Tenant Site configuration, unless this
specification also defines the corresponding TenantSite schema and REST
configuration path; keep the existing account capability and deprecation details
unchanged.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/machine.go`:
- Around line 2037-2063: Update the Tenant branch of the machine association
check in the relevant DPU handler to use
common.TenantHasTargetedInstanceCreation(ctx, nil, gadmh.dbSession, tenant,
common.SiteScope(site)) instead of counting TenantAccount rows. Preserve the
existing provider ownership check, error response behavior, and final forbidden
response while ensuring the tenant capability is effective for this specific
site.
- Around line 348-369: Update the tenant lookup using tenantAccountDAO.GetAll in
the infrastructureProvider branch to avoid the default paginator limit of 20.
Supply an explicit PageInput limit large enough for all requested tenantIDs, or
use the available unpaged lookup path, so every tenantId is evaluated before
authorization.
In `@rest-api/api/pkg/api/handler/sku_test.go`:
- Around line 179-220: Add a ready TenantAccount fixture for
tenantOrgNoCapability, linked to tenantWithoutCapability and the existing
infrastructure provider, with TargetedInstanceCreation omitted or false; keep
tenantWithoutAccount without an account so the two tests exercise distinct
validation branches. Apply the same fixture correction in
TestGetSkuHandler_Handle.
In `@rest-api/db/pkg/db/model/tenant.go`:
- Around line 55-66: Move the deprecation documentation from the TenantConfig
type to the TargetedInstanceCreation field only. Keep EnableSSHAccess and the
TenantConfig type undocumented as deprecated, and scope the legacy migration
explanation to TargetedInstanceCreation without changing its JSON mapping.
In `@rest-api/db/pkg/db/model/tenantsite.go`:
- Around line 291-330: Rewrite the comment above updatedFields in the
tenant-site update flow to state only that the Config merge and removal paths
are mutually exclusive through the single TargetedInstanceCreation pointer;
remove the unsupported claim about pre-write validation and preventing partial
updates. Do not change the separate EnableSerialConsole and Config Exec
operations.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/instance.go`:
- Around line 890-903: Move the TenantHasTargetedInstanceCreation capability
check for apiRequest.MachineID out of the cdb.WithTx callback and execute it
before the transaction begins, using the already resolved tenant and site
values. Preserve its existing error responses and authorization behavior, and
apply the same hoisting to the corresponding DeleteInstanceHandler check.
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Around line 781-913: Update handleProviderSiteCapabilitiesUpdate to use dbUser
and record an attributable StatusDetail (or the established equivalent) for the
provider siteCapabilities change, including dbUser.ID, before the transaction
closure returns. Follow the existing audit-recording patterns in
CreateTenantAccountHandler and handleTenantInviteAcceptance, and ensure the
audit write participates in the same transaction as the capability updates.
In `@rest-api/api/pkg/api/model/tenantaccountsitecapability.go`:
- Around line 98-104: Update the duplicate check in the capability validation
loop to parse each validated cap.SiteIDs value into a uuid.UUID and track
seenSiteIDs as map[uuid.UUID]struct{} instead of raw strings. Compare the
canonical UUID values so differently cased representations are treated as
duplicates, while preserving the existing validation error and registration
flow.
In
`@rest-api/db/pkg/migrations/20260701120000_tenant_account_site_capabilities.go`:
- Around line 101-104: Document the intentional no-op in the down migration
callback for tenant_account.config with a concise comment explaining why
rollback is omitted, such as the forward-only policy or rolling-deploy safety
concerns. Keep the existing callback behavior unchanged and align the
explanation with the up migration’s compatibility documentation.
🪄 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: e34d3bd1-3c9c-4069-bdc1-dba212dba6ad
⛔ Files ignored due to path filters (9)
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/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_*.go
📒 Files selected for processing (63)
rest-api/api/pkg/api/handler/allocation.gorest-api/api/pkg/api/handler/bmcreset.gorest-api/api/pkg/api/handler/dpureprovision.gorest-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/expectedpowershelf.gorest-api/api/pkg/api/handler/expectedrack.gorest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/fabric.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/instancetype.gorest-api/api/pkg/api/handler/ipblock.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machinehealth.gorest-api/api/pkg/api/handler/machinehealth_test.gorest-api/api/pkg/api/handler/machinepower.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/handler/vpcpeering.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
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 (2)
rest-api/api/pkg/api/handler/sku_test.go (1)
202-210: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCreate a Ready account with the capability disabled for these fixtures.
Both “without capability” tenants currently have no TenantAccount, so they duplicate the separate missing-account cases instead of testing
TargetedInstanceCreation: false.
rest-api/api/pkg/api/handler/sku_test.go#L202-L210: add a Ready TenantAccount fortenantWithoutCapabilitywith the same provider and disabled configuration.rest-api/api/pkg/api/handler/sku_test.go#L477-L485: add the equivalent disabled Ready TenantAccount for the single-SKU test.Based on learnings, each failure-path test should satisfy all prerequisites except the exact condition under test.
🤖 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, The “without capability” fixtures currently lack TenantAccounts, so they test missing accounts instead of disabled TargetedInstanceCreation. In rest-api/api/pkg/api/handler/sku_test.go lines 202-210 and 477-485, add a Ready TenantAccount for each corresponding tenant using the same provider and configuration with TargetedInstanceCreation disabled; leave other prerequisites unchanged.Source: Learnings
rest-api/api/pkg/api/model/tenantaccount.go (1)
69-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrack field presence independently from slice length.
An explicit
"siteCapabilities":[]makesHasSiteCapabilitiesreturn false, bypassing the required non-empty validation and routing the request into tenant invite handling. Use a pointer or explicit presence flag so an empty supplied array reachesValidate()and returns HTTP 400.Proposed fix
type APITenantAccountUpdateRequest struct { TenantContactID *string `json:"tenantContactId"` - SiteCapabilities APITenantAccountSiteCapabilitiesUpdateRequest `json:"siteCapabilities"` + SiteCapabilities *APITenantAccountSiteCapabilitiesUpdateRequest `json:"siteCapabilities"` } func (taur APITenantAccountUpdateRequest) HasSiteCapabilities() bool { - return len(taur.SiteCapabilities) > 0 + return taur.SiteCapabilities != nil }Update downstream calls to dereference the validated payload.
As per path instructions, PATCH
siteCapabilitiespayloads must be non-empty and mutually exclusive withtenantContactId.🤖 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/tenantaccount.go` around lines 69 - 86, Track whether siteCapabilities was supplied independently of its slice length in APITenantAccountUpdateRequest, so an explicit empty array is recognized as present. Update HasSiteCapabilities and downstream validated-payload usage to use the presence-aware representation, ensuring Validate rejects supplied empty payloads and enforces mutual exclusivity with TenantContactID while preserving normal non-empty payload handling.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/expectedmachine.go`:
- Around line 60-68: The ExpectedMachine authorization currently lets TenantSite
association access override an explicit false Site capability. In
rest-api/api/pkg/api/handler/expectedmachine.go lines 60-68, call
TenantHasTargetedInstanceCreation with SiteScope(site) for every Tenant request
and overwrite association-based access with its result, preserving existing
error handling. In rest-api/api/pkg/api/handler/expectedmachine_test.go lines
1596-1605, add an isolated single-item test covering global enablement with an
explicit false Site override and assert HTTP 403.
In `@rest-api/api/pkg/api/handler/machine.go`:
- Line 2037: Update the IsProviderOrTenant call in the machine handler to pass
common.SiteScope(site) instead of an empty TenantPrivilegeScope, ensuring DPU
access is evaluated against the Machine’s Site and its capability/ownership
rules.
In `@rest-api/api/pkg/api/handler/machinepower.go`:
- Line 96: Update the IsProviderOrTenant call in the machine power authorization
flow to disable viewer-role access and avoid tenant capability resolution,
ensuring only Provider Admins receive a non-nil provider for the existing
authorization check and machine power control remains restricted accordingly.
In `@rest-api/api/pkg/api/handler/serviceaccount.go`:
- Around line 161-171: The service-account handler must not change an existing
false TargetedInstanceCreation setting to true during reads. Remove the update
logic around taDAO.Update and leave the tenant account configuration unchanged
when the capability is disabled; add a regression test confirming disabled
accounts remain disabled, and ensure any siteCapabilities replacement requires
PROVIDER_ADMIN.
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Line 447: Update the collection response flow around NewAPITenantAccount to
batch-load TenantSite records for all returned tenant accounts, map each account
ID to its corresponding rows, and pass that data instead of nil. Preserve the
existing single-item endpoint behavior and ensure accounts without overrides
still receive an empty or otherwise compatible site-capabilities value.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/sku_test.go`:
- Around line 202-210: The “without capability” fixtures currently lack
TenantAccounts, so they test missing accounts instead of disabled
TargetedInstanceCreation. In rest-api/api/pkg/api/handler/sku_test.go lines
202-210 and 477-485, add a Ready TenantAccount for each corresponding tenant
using the same provider and configuration with TargetedInstanceCreation
disabled; leave other prerequisites unchanged.
In `@rest-api/api/pkg/api/model/tenantaccount.go`:
- Around line 69-86: Track whether siteCapabilities was supplied independently
of its slice length in APITenantAccountUpdateRequest, so an explicit empty array
is recognized as present. Update HasSiteCapabilities and downstream
validated-payload usage to use the presence-aware representation, ensuring
Validate rejects supplied empty payloads and enforces mutual exclusivity with
TenantContactID while preserving normal non-empty payload handling.
🪄 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: e34d3bd1-3c9c-4069-bdc1-dba212dba6ad
⛔ Files ignored due to path filters (9)
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/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_*.go
📒 Files selected for processing (63)
rest-api/api/pkg/api/handler/allocation.gorest-api/api/pkg/api/handler/bmcreset.gorest-api/api/pkg/api/handler/dpureprovision.gorest-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/expectedpowershelf.gorest-api/api/pkg/api/handler/expectedrack.gorest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/fabric.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/instancetype.gorest-api/api/pkg/api/handler/ipblock.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machinehealth.gorest-api/api/pkg/api/handler/machinehealth_test.gorest-api/api/pkg/api/handler/machinepower.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/handler/vpcpeering.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
+161
to
+171
| } else if !tas[0].Config.TargetedInstanceCreation { | ||
| _, serr = taDAO.Update(ctx, nil, cdbm.TenantAccountUpdateInput{ | ||
| TenantAccountID: tas[0].ID, | ||
| Config: &cdbm.TenantAccountConfig{ | ||
| TargetedInstanceCreation: true, | ||
| }, | ||
| }) | ||
| if serr != nil { | ||
| logger.Error().Err(serr).Msg("error updating Tenant Account capabilities for org") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to update Tenant Account capabilities for org, DB error", nil) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not re-enable a provider-disabled capability on read.
An existing false value is an explicit disabled state, but this handler silently changes it to true. Calling this endpoint can therefore bypass the provider-admin capability decision and restore machine-ID privilege. Leave existing account configuration untouched and add a regression test proving a disabled account remains disabled.
As per path instructions, siteCapabilities replacement requires PROVIDER_ADMIN.
🤖 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/serviceaccount.go` around lines 161 - 171, The
service-account handler must not change an existing false
TargetedInstanceCreation setting to true during reads. Remove the update logic
around taDAO.Update and leave the tenant account configuration unchanged when
the capability is disabled; add a regression test confirming disabled accounts
remain disabled, and ensure any siteCapabilities replacement requires
PROVIDER_ADMIN.
Source: Path instructions
Contributor
Author
There was a problem hiding this comment.
It make sense that only PROVIDER_ADMIN can update TargetedInstanceCreation for Tenant, let us confirm this with @thossain-nv , for time being will keep the implementation as it is
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
47ce928 to
945ba84
Compare
Contributor
Author
|
@coderabbitai full_review, thanks ! |
Contributor
|
ᕕ( ᐛ )ᕗ ✅ Action performedFull review finished. |
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 (2)
rest-api/api/pkg/api/model/tenantaccount.go (1)
69-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDistinguish an empty replacement payload from an omitted field.
len(...) > 0treats an explicit"siteCapabilities":[]as absent, bypassing its non-empty validation and the mutual-exclusion check withtenantContactId. Check the slice againstnilso the handler returns HTTP 400 for a supplied empty replacement.Proposed fix
func (taur APITenantAccountUpdateRequest) HasSiteCapabilities() bool { - return len(taur.SiteCapabilities) > 0 + return taur.SiteCapabilities != nil }As per path instructions, REST API models must preserve client-facing request semantics and remain consistent with
spec.yaml.🤖 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/tenantaccount.go` around lines 69 - 86, Update APITenantAccountUpdateRequest.HasSiteCapabilities to check whether SiteCapabilities is non-nil rather than whether its length is greater than zero, so an explicitly supplied empty array is recognized and reaches validation and mutual-exclusion checks while an omitted field remains absent.Sources: Coding guidelines, Path instructions
rest-api/api/pkg/api/handler/sku_test.go (1)
202-210: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winGive the disabled-capability tenants a Ready account with
TargetedInstanceCreation: false.These fixtures currently have no account, so both “without capability” tests exercise the same missing-account branch as the dedicated “without TenantAccount” cases. Add a Ready account with disabled configuration in both test setups to preserve distinct authorization coverage.
Based on learnings, failure-path fixtures should satisfy every prerequisite except the condition under test. As per coding guidelines, endpoint tests must cover authorization behavior.
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, Update both disabled-capability tenant fixtures, including the setup around tenantOrgNoCapability and the corresponding second fixture, to create and associate a Ready account configured with TargetedInstanceCreation set to false. Preserve the existing no-account fixtures for dedicated missing TenantAccount cases so each authorization test isolates only its intended condition.Sources: Coding guidelines, Learnings
🤖 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/expectedmachine.go`:
- Around line 330-356: The GetAll Tenant authorization flow must not include
association-only TenantSite IDs: in
rest-api/api/pkg/api/handler/expectedmachine.go lines 330-356, remove that Site
ID union and scope access through GetPrivilegedAccessSiteIDsForTenant so
effective capabilities are enforced. In
rest-api/api/pkg/api/handler/expectedmachine_test.go lines 633-645, update the
test to grant access via an explicit true Site override, or expect 403 for the
association-only case; ensure each failure-path test exercises the intended
authorization branch.
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Around line 770-776: Update the transaction closure containing taDAO.Update to
acquire the TenantAccount-specific advisory lock first, then re-read the
TenantAccount through tx and repeat the Invited status and tenant-ownership
checks before assigning TenantContactID. Preserve the existing update and
success behavior only when the locked, freshly read record still passes both
validations.
- Around line 965-972: Update the TenantAccount update response flow around
NewAPITenantAccount to retrieve the provider-and-tenant allocation count using
the same query or helper as the GET handlers, then pass that count instead of 0.
Preserve the existing status-detail and tenant-site handling so PATCH responses
match subsequent GET responses.
In `@rest-api/api/pkg/api/handler/util/common/common.go`:
- Around line 1631-1641: Update the privilege check around the tas loop and
getPrivilegedAccessSiteIDsForTenant so TargetedInstanceCreation does not return
immediately. Resolve Ready TenantAccount.siteCapabilities together with each
Site’s explicit override, honoring disabled overrides in both coarse and
provider-scoped checks; report the Tenant as privileged only when at least one
effective Site capability remains enabled.
In `@rest-api/api/pkg/api/model/tenantaccountsitecapability.go`:
- Around line 42-45: The APITenantAccountSiteCapability model currently cannot
distinguish an omitted targetedInstanceCreation from an explicit false value.
Introduce a separate update DTO using *bool for targetedInstanceCreation,
require and validate its presence, then convert it to the response/domain
model’s bool; preserve the existing required/nullable semantics for the related
fields and update all affected request handling.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/sku_test.go`:
- Around line 202-210: Update both disabled-capability tenant fixtures,
including the setup around tenantOrgNoCapability and the corresponding second
fixture, to create and associate a Ready account configured with
TargetedInstanceCreation set to false. Preserve the existing no-account fixtures
for dedicated missing TenantAccount cases so each authorization test isolates
only its intended condition.
In `@rest-api/api/pkg/api/model/tenantaccount.go`:
- Around line 69-86: Update APITenantAccountUpdateRequest.HasSiteCapabilities to
check whether SiteCapabilities is non-nil rather than whether its length is
greater than zero, so an explicitly supplied empty array is recognized and
reaches validation and mutual-exclusion checks while an omitted field remains
absent.
🪄 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: 81b5f771-4eb4-4940-be68-86745eee9b4e
⛔ Files ignored due to path filters (9)
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/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_*.go
📒 Files selected for processing (63)
rest-api/api/pkg/api/handler/allocation.gorest-api/api/pkg/api/handler/bmcreset.gorest-api/api/pkg/api/handler/dpureprovision.gorest-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/expectedpowershelf.gorest-api/api/pkg/api/handler/expectedrack.gorest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/fabric.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/instancetype.gorest-api/api/pkg/api/handler/ipblock.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machinehealth.gorest-api/api/pkg/api/handler/machinehealth_test.gorest-api/api/pkg/api/handler/machinepower.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/handler/vpcpeering.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
+330
to
+356
| // Sites the Tenant is associated with (TenantSite) are visible for | ||
| // listing, unioned with the Sites where the Tenant has effective | ||
| // TargetedInstanceCreation (per-site TenantSite.config overrides honored). | ||
| tsDAO := cdbm.NewTenantSiteDAO(gaemh.dbSession) | ||
| tenantSites, _, err := tsDAO.GetAll(ctx, nil, | ||
| cdbm.TenantSiteFilterInput{TenantIDs: []uuid.UUID{tenant.ID}}, | ||
| paginator.PageInput{Limit: cutil.GetPtr(math.MaxInt)}, | ||
| nil, | ||
| ) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error retrieving Tenant Site associations from DB") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Tenant Site associations due to DB error", nil) | ||
| } | ||
| for _, ts := range tenantSites { | ||
| filterInput.SiteIDs = append(filterInput.SiteIDs, ts.SiteID) | ||
| } | ||
|
|
||
| for _, tenantSite := range tenantSites { | ||
| filterInput.SiteIDs = append(filterInput.SiteIDs, tenantSite.SiteID) | ||
| } | ||
| privilegedSiteIDs, err := common.GetPrivilegedAccessSiteIDsForTenant(ctx, nil, gaemh.dbSession, tenant) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error resolving privileged Site access for Tenant") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to resolve Tenant capability due to DB error", nil) | ||
| } | ||
| filterInput.SiteIDs = append(filterInput.SiteIDs, privilegedSiteIDs...) | ||
| } | ||
|
|
||
| if infrastructureProvider == nil && tenant != nil && len(filterInput.SiteIDs) == 0 { | ||
| return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "Tenant is not associated with any Site", nil) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not let TenantSite association bypass ExpectedMachine capability enforcement.
Single-resource operations make the exact Site capability authoritative, but GetAll unions every TenantSite into its authorization scope. A false override can therefore hide an ExpectedMachine from GET-by-ID while still exposing it in the list.
rest-api/api/pkg/api/handler/expectedmachine.go#L330-L356: remove association-only Site IDs; scope Tenant access usingGetPrivilegedAccessSiteIDsForTenant.rest-api/api/pkg/api/handler/expectedmachine_test.go#L633-L645: grant the external Site through an explicit true Site override, or change the association-only case to expect 403.
As per path instructions, REST authorization must enforce effective Site capabilities; based on learnings, each failure-path test should target the exact authorization branch exercised.
📍 Affects 2 files
rest-api/api/pkg/api/handler/expectedmachine.go#L330-L356(this comment)rest-api/api/pkg/api/handler/expectedmachine_test.go#L633-L645
🤖 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 330 - 356, The
GetAll Tenant authorization flow must not include association-only TenantSite
IDs: in rest-api/api/pkg/api/handler/expectedmachine.go lines 330-356, remove
that Site ID union and scope access through GetPrivilegedAccessSiteIDsForTenant
so effective capabilities are enforced. In
rest-api/api/pkg/api/handler/expectedmachine_test.go lines 633-645, update the
test to grant access via an explicit true Site override, or expect 403 for the
association-only case; ensure each failure-path test exercises the intended
authorization branch.
Sources: Path instructions, Learnings
Comment on lines
770
to
+776
| err = cdb.WithTx(ctx, utah.dbSession, func(tx *cdb.Tx) error { | ||
| var derr error | ||
| uta, derr = taDAO.Update(ctx, tx, cdbm.TenantAccountUpdateInput{TenantAccountID: taID, TenantContactID: cutil.GetPtr(dbUser.ID), Status: cutil.GetPtr(cdbm.TenantAccountStatusReady)}) | ||
| uta, derr = taDAO.Update(ctx, tx, cdbm.TenantAccountUpdateInput{ | ||
| TenantAccountID: taID, | ||
| TenantContactID: cutil.GetPtr(dbUser.ID), | ||
| Status: cutil.GetPtr(cdbm.TenantAccountStatusReady), | ||
| }) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize invite acceptance before assigning the Tenant contact.
The status check on Line 761 occurs before the transaction. Two Tenant Admins can both observe Invited, then overwrite TenantContactID and both receive success. Acquire a TenantAccount-specific advisory lock at the start of the closure, re-read through tx, and repeat the status and tenant-ownership checks before updating.
As per coding guidelines, “For TOCTOU-prone read-then-mutate flows, acquire the advisory lock at the start of the transaction closure and re-read state after acquiring 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/api/pkg/api/handler/tenantaccount.go` around lines 770 - 776, Update
the transaction closure containing taDAO.Update to acquire the
TenantAccount-specific advisory lock first, then re-read the TenantAccount
through tx and repeat the Invited status and tenant-ownership checks before
assigning TenantContactID. Preserve the existing update and success behavior
only when the locked, freshly read record still passes both validations.
Source: Coding guidelines
Comment on lines
+965
to
+972
| 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) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return the actual allocation count in the update response.
A Ready TenantAccount may already have allocations, but this new update path always passes 0 to NewAPITenantAccount. Query the provider-and-tenant allocation count as the GET handlers do; otherwise a successful PATCH returns an object inconsistent with subsequent GET responses.
Proposed fix
+ allocationDAO := cdbm.NewAllocationDAO(utah.dbSession)
+ allocationCount, err := allocationDAO.GetCount(ctx, nil, cdbm.AllocationFilterInput{
+ InfrastructureProviderIDs: []uuid.UUID{ta.InfrastructureProviderID},
+ TenantIDs: []uuid.UUID{*ta.TenantID},
+ })
+ if err != nil {
+ logger.Error().Err(err).Msg("error retrieving allocation count for updated Tenant Account")
+ return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve allocation count for Tenant Account", nil)
+ }
+
- apiInstance := model.NewAPITenantAccount(ta, ssds, 0, tenantSites)
+ apiInstance := model.NewAPITenantAccount(ta, ssds, allocationCount, tenantSites)As per path instructions, REST API changes must be reviewed for “response compatibility.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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) | |
| allocationDAO := cdbm.NewAllocationDAO(utah.dbSession) | |
| allocationCount, err := allocationDAO.GetCount(ctx, nil, cdbm.AllocationFilterInput{ | |
| InfrastructureProviderIDs: []uuid.UUID{ta.InfrastructureProviderID}, | |
| TenantIDs: []uuid.UUID{*ta.TenantID}, | |
| }) | |
| if err != nil { | |
| logger.Error().Err(err).Msg("error retrieving allocation count for updated Tenant Account") | |
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve allocation count for Tenant Account", nil) | |
| } | |
| 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, allocationCount, 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 965 - 972, Update
the TenantAccount update response flow around NewAPITenantAccount to retrieve
the provider-and-tenant allocation count using the same query or helper as the
GET handlers, then pass that count instead of 0. Preserve the existing
status-detail and tenant-site handling so PATCH responses match subsequent GET
responses.
Source: Path instructions
Comment on lines
+1631
to
+1641
| for _, ta := range tas { | ||
| if ta.Config.TargetedInstanceCreation { | ||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| privilegedSiteIDs, err := getPrivilegedAccessSiteIDsForTenant(ctx, tx, dbSession, tenant, providerFilter) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return len(privilegedSiteIDs) > 0, nil |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Honor Site overrides in coarse and provider-scoped checks.
Returning immediately for a globally enabled account ignores explicit false overrides. If every Site is disabled, this still reports the Tenant as privileged and makes /tenant/current advertise an ineffective capability.
Proposed fix
- for _, ta := range tas {
- if ta.Config.TargetedInstanceCreation {
- return true, nil
- }
- }
-
privilegedSiteIDs, err := getPrivilegedAccessSiteIDsForTenant(ctx, tx, dbSession, tenant, providerFilter)As per path instructions, privileged Tenant behavior is determined by Ready TenantAccount.siteCapabilities with per-Site overrides.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, ta := range tas { | |
| if ta.Config.TargetedInstanceCreation { | |
| return true, nil | |
| } | |
| } | |
| privilegedSiteIDs, err := getPrivilegedAccessSiteIDsForTenant(ctx, tx, dbSession, tenant, providerFilter) | |
| if err != nil { | |
| return false, err | |
| } | |
| return len(privilegedSiteIDs) > 0, nil | |
| privilegedSiteIDs, err := getPrivilegedAccessSiteIDsForTenant(ctx, tx, dbSession, tenant, providerFilter) | |
| if err != nil { | |
| return false, err | |
| } | |
| return len(privilegedSiteIDs) > 0, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/handler/util/common/common.go` around lines 1631 - 1641,
Update the privilege check around the tas loop and
getPrivilegedAccessSiteIDsForTenant so TargetedInstanceCreation does not return
immediately. Resolve Ready TenantAccount.siteCapabilities together with each
Site’s explicit override, honoring disabled overrides in both coarse and
provider-scoped checks; report the Tenant as privileged only when at least one
effective Site capability remains enabled.
Source: Path instructions
Comment on lines
+42
to
+45
| type APITenantAccountSiteCapability struct { | ||
| SiteIDs []string `json:"siteIds,omitempty"` | ||
| Scope TenantAccountSiteCapabilityScope `json:"scope"` | ||
| TargetedInstanceCreation bool `json:"targetedInstanceCreation"` |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve presence for the required capability boolean.
An omitted targetedInstanceCreation binds as false and passes validation, silently turning a malformed replacement request into capability disablement. Use a separate update DTO with *bool, validate it as required, then convert to the response/domain boolean.
As per path instructions, targetedInstanceCreation is required and model changes must preserve matching required/nullable semantics.
Also applies to: 63-74
🤖 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 42 -
45, The APITenantAccountSiteCapability model currently cannot distinguish an
omitted targetedInstanceCreation from an explicit false value. Introduce a
separate update DTO using *bool for targetedInstanceCreation, require and
validate its presence, then convert it to the response/domain model’s bool;
preserve the existing required/nullable semantics for the related fields and
update all affected request handling.
Source: Path instructions
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
Problem
Privileged tenant access (TargetedInstanceCreation) was stored on the global Tenant.config and surfaced as a single Tenant.capabilities.targetedInstanceCreation flag. That model is too coarse: the capability can differ by Provider and Site, and tenants may need per-site overrides. A tenant-wide flag also made it easy to over-grant access on list endpoints (e.g. machines/sites) when only some sites should be privileged.
What this PR does
Moves capability configuration and enforcement to the account/site model and makes authorization site-aware end to end.
Configuration model
TenantAccount API (siteCapabilities)
Provider Admin + siteCapabilities — replace capability configuration globally (scope: global) or for explicit sites (scope: limited + siteIds). Server validates: non-empty array, exactly one global entry, no duplicate siteIds, siteIds required for limited / omitted for global.
Tenant Admin + tenantContactId — accept an Invited account (existing flow). Cannot be combined with siteCapabilities in the same request (400).
Authorization / enforcement
TenantHasTargetedInstanceCreation(tenant, site?) — tenant-wide or effective per-site check (account default + TenantSite override)
GetPrivilegedAccessSiteIDsForTenant — returns the exact set of privileged site IDs
Sites, Machines, Expected Machines, Instances, VPCs, SKUs, etc.
API / docs
Typical capability setup flow
Reviewer focus areas
Related
Closes #2104
Type of Change
Related Issues (Optional)
#2104
Breaking Changes
Testing
Additional Notes