Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds first-class multi-tenancy: tenant domain models, DB migrations, Tenant repository and service, tenant-aware auth and middleware, tenant HTTP handlers and tests, and converts many resource repositories and tests from user-scoped to tenant-scoped ownership and access. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Handler as TenantHandler
participant Service as TenantService
participant Repo as TenantRepository
participant UserRepo as UserRepository
participant DB as PostgreSQL
User->>Handler: POST /tenants (name, slug)
Handler->>Service: CreateTenant(ctx, name, slug, userID)
Service->>Repo: GetBySlug(ctx, slug)
Repo->>DB: SELECT tenants WHERE slug=...
DB-->>Repo: no rows
Repo-->>Service: not found
Service->>Repo: Create(ctx, tenant)
Repo->>DB: INSERT tenants
DB-->>Repo: inserted
Service->>Repo: AddMember(ctx, tenantID, userID, "owner")
Repo->>DB: INSERT tenant_members
DB-->>Repo: inserted
Service->>UserRepo: GetByID(ctx, userID)
UserRepo->>DB: SELECT users WHERE id=...
DB-->>UserRepo: user row
Service->>UserRepo: Update(ctx, default_tenant_id)
UserRepo->>DB: UPDATE users
DB-->>UserRepo: updated
Service-->>Handler: tenant created
Handler-->>User: 201 Created (tenant)
sequenceDiagram
actor Client
participant Auth as Auth Middleware
participant TenantMW as TenantMember Middleware
participant Handler as Resource Handler
participant Service as TenantService
participant Repo as TenantRepository
participant DB as PostgreSQL
Client->>Auth: Request (API key + optional X-Tenant-ID)
Auth->>Auth: resolve tenant ID (header or api key default)
Auth-->>Client: context includes tenantID
Client->>TenantMW: request proceeds
TenantMW->>TenantMW: GetUserID & GetTenantID from context
TenantMW->>Service: GetMembership(ctx, tenantID, userID)
Service->>Repo: GetMembership(...)
Repo->>DB: SELECT tenant_members ...
DB-->>Repo: membership / no rows
alt membership exists
TenantMW->>Handler: allow (stores role)
Handler->>Handler: process tenant-scoped operation
Handler-->>Client: 200 OK
else no membership
TenantMW-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/errors/errors.go (1)
12-22: Map the new error types to HTTP status codes to avoid 500s.
PermissionDeniedandQuotaExceededaren't inpkg/httputil/response.go'sstatusCodeMap, so they'll fall back to 500. Add mappings (403 and 429 respectively) to keep client behavior consistent.🛠️ Suggested fix in
pkg/httputil/response.gostatusCodeMap := map[errors.Type]int{ errors.NotFound: http.StatusNotFound, errors.InvalidInput: http.StatusBadRequest, errors.Unauthorized: http.StatusUnauthorized, errors.Forbidden: http.StatusForbidden, + errors.PermissionDenied: http.StatusForbidden, + errors.QuotaExceeded: http.StatusTooManyRequests, errors.Conflict: http.StatusConflict, errors.BucketNotFound: http.StatusNotFound, errors.ObjectNotFound: http.StatusNotFound, errors.ObjectTooLarge: http.StatusRequestEntityTooLarge, errors.InstanceNotRunning: http.StatusConflict, errors.PortConflict: http.StatusConflict, errors.TooManyPorts: http.StatusConflict, errors.ResourceLimitExceeded: http.StatusTooManyRequests, }internal/api/setup/router.go (2)
217-252: Add RequireTenant and TenantMember middleware to snapshot, image, and cluster routes; update their repositories to filter by tenant.The migration added
tenant_idcolumns and created tenant indexes for snapshots, images, and clusters, indicating these resources should be tenant-scoped. However:
- The routes only apply
httputil.Auth(svcs.Identity)withouthttputil.RequireTenant()andhttputil.TenantMember(svcs.Tenant)(inconsistent with instances and VPCs)- The repositories still filter by
UserIDonly and do not useTenantIDFromContext, meaning thetenant_idcolumn is never queriedUpdate the middleware on these route groups and refactor their repositories to enforce tenant isolation like instances and VPCs do.
268-298: AddRequireTenant()andTenantMember()middleware to subnet, security-groups, and lb route groups.These routes lack tenant isolation middleware despite migration 071 adding
tenant_idtosubnets,security_groups, andload_balancerstables. The subnet and load balancer repositories only filter byuser_id, while the security group repository attempts to filter by tenant but has no middleware to populate the tenant context. Follow the established pattern used on instanceGroup, vpcGroup, storageGroup, and volumeGroup.internal/repositories/postgres/security_group_repo.go (1)
232-244: Missing tenant filtering inListInstanceGroups- potential cross-tenant data leak.This method does not filter by
tenant_id, unlike other methods in this file. If an attacker provides aninstanceIDbelonging to another tenant, they could enumerate security groups from that tenant.🔒 Suggested fix
func (r *SecurityGroupRepository) ListInstanceGroups(ctx context.Context, instanceID uuid.UUID) ([]*domain.SecurityGroup, error) { + tenantID := appcontext.TenantIDFromContext(ctx) query := ` SELECT sg.id, sg.user_id, sg.tenant_id, sg.vpc_id, sg.name, sg.description, sg.arn, sg.created_at FROM security_groups sg JOIN instance_security_groups isg ON sg.id = isg.group_id - WHERE isg.instance_id = $1 + JOIN instances i ON isg.instance_id = i.id + WHERE isg.instance_id = $1 AND i.tenant_id = $2 AND sg.tenant_id = $2 ` - rows, err := r.db.Query(ctx, query, instanceID) + rows, err := r.db.Query(ctx, query, instanceID, tenantID) if err != nil { return nil, err } return r.scanSecurityGroups(rows) }
🤖 Fix all issues with AI agents
In `@internal/core/services/tenant.go`:
- Around line 70-75: The update call to s.userRepo.Update is currently ignored
with `_ = ...`, which causes silent failures; replace this with proper error
handling in the tenant creation flow: after retrieving the user with
s.userRepo.GetByID and setting user.DefaultTenantID = &tenant.ID, call
s.userRepo.Update(ctx, user) and check its returned error (e.g., err :=
s.userRepo.Update(...)); if non-nil, handle it consistently with surrounding
code (return the error or log it via the service logger) so failures are
surfaced instead of discarded; ensure you reference s.userRepo.Update,
s.userRepo.GetByID, user.DefaultTenantID, ownerID and tenant.ID when making the
change.
- Around line 152-154: The switch default currently returns nil which silently
allows unknown resource types to pass quota checks; update the default case in
the switch inside the quota-checking function (the switch handling the resource
string) to return a non-nil error (e.g., fmt.Errorf("unknown resource type: %q",
resource)) and/or emit a warning via the logger (processLogger or tenant service
logger) so typos or unsupported resources no longer bypass quota enforcement;
ensure callers propagate this error instead of assuming success.
In `@internal/repositories/postgres/migrations/070_create_tenants.down.sql`:
- Around line 1-4: The down migration is missing removal of the
api_keys.default_tenant_id column; update the rollback SQL in
070_create_tenants.down.sql to mirror the users change by dropping
default_tenant_id from the api_keys table (use the same DROP COLUMN IF EXISTS
pattern), ensuring the schema is reverted for both users.default_tenant_id and
api_keys.default_tenant_id.
In `@internal/repositories/postgres/migrations/072_migrate_to_tenants.up.sql`:
- Around line 3-14: The slug generation in the INSERT SELECT can produce invalid
characters and collisions; update the slug expression to normalize accents and
remove non-alphanumeric characters (e.g., use the unaccent extension and a
regexp_replace to convert runs of non a-z0-9 into single hyphens, trim
leading/trailing hyphens, and lower-case the result) and ensure uniqueness by
appending a deterministic short suffix (for example SUBSTR(id::text,1,8) or a
short random token) if needed; modify the slug expression used in the INSERT
INTO tenants SELECT (the current LOWER(REPLACE(name, ' ', '-')) || '-' ||
SUBSTR(id::text, 1, 8) portion) to use these normalization and uniqueness steps
so produced slugs are URL-safe and collision-resistant.
- Around line 17-20: The INSERT into tenant_members uses ON CONFLICT DO NOTHING
without a conflict target which Postgres requires; update the statement that
inserts from tenants to specify the PRIMARY KEY conflict target by changing ON
CONFLICT DO NOTHING to ON CONFLICT (tenant_id, user_id) DO NOTHING so conflicts
on the tenant_members primary key (tenant_id, user_id) are handled correctly;
locate the INSERT INTO tenant_members ... SELECT id, owner_id, 'owner', NOW()
FROM tenants and add the conflict target (tenant_id, user_id) to the ON CONFLICT
clause.
- Around line 96-99: The INSERT into tenant_quotas uses a bare ON CONFLICT DO
NOTHING which should explicitly target the primary key; update the INSERT ...
SELECT statement that inserts into tenant_quotas to use ON CONFLICT (tenant_id)
DO NOTHING so conflicts are resolved against the tenant_id primary key
(referencing tenant_quotas and tenant_id in the INSERT statement).
In `@internal/repositories/postgres/tenant_repo.go`:
- Around line 162-173: GetQuota currently only reads max_* fields into
domain.TenantQuota leaving Used* zeroed, so quota checks in CheckQuota use stale
zeros; update TenantRepo.GetQuota to populate UsedInstances, UsedVPCs,
UsedStorageGB, UsedMemoryGB, and UsedVCPUs either by (recommended) running
additional COUNT/SUM subqueries (or JOINs) against the relevant resource tables
(e.g., instances, vpcs, volumes) in the same DB call and scanning those results
into q.Used* fields, or (alternatively) remove Used* usage from CheckQuota and
compute usage on-the-fly there; modify the GetQuota method (TenantRepo.GetQuota)
to return a fully populated domain.TenantQuota used by CheckQuota.
In `@internal/repositories/postgres/volume_repo.go`:
- Around line 24-27: Create and Update currently use v.TenantID for writes,
opening a tenant bypass; change both VolumeRepository.Create and
VolumeRepository.Update to derive the tenant ID from the context (the same way
GetByID/GetByName/List/Delete do), use that derived tenantID in the
INSERT/UPDATE parameter list instead of v.TenantID, and return an error if no
tenant is found in context (or otherwise handle a missing tenant consistently
with the read methods).
In `@pkg/httputil/auth.go`:
- Around line 33-51: The Auth() middleware currently accepts X-Tenant-ID and
sets context via appcontext.WithTenantID / c.Set("tenantID") without verifying
the authenticated user's membership; either validate membership inside Auth()
(lookup user/ API key tenants and ensure the parsed tenantID from X-Tenant-ID is
among them before calling appcontext.WithTenantID and c.Set) or enforce that
every tenant-aware route registers TenantMember() (and Keep RequireTenant()
where used) so membership checks run before handlers execute; update Auth() to
perform the membership check against the authenticated principal (API key/user)
or add gating to route registration to ensure TenantMember() is applied for all
endpoints that read tenantID from context.
🧹 Nitpick comments (18)
internal/core/domain/security_group.go (1)
27-38: Consider addingTenantIDvalidation for consistency.The
Validate()method checks thatUserIDis notuuid.Nil(line 34-36), but the newly addedTenantIDfield lacks the same validation. IfTenantIDis required for all security groups, consider adding a similar check.Proposed validation addition
func (sg *SecurityGroup) Validate() error { if sg.Name == "" { return errors.New("security group name cannot be empty") } if sg.VPCID == uuid.Nil { return errors.New("security group must be associated with a VPC") } if sg.UserID == uuid.Nil { return errors.New("security group must have a user owner") } + if sg.TenantID == uuid.Nil { + return errors.New("security group must be associated with a tenant") + } return nil }pkg/httputil/auth.go (1)
60-71: Rename the local variable to avoid confusion.The value is a tenant ID, but the variable is named
userID. Renaming improves readability.♻️ Suggested rename
- userID, ok := val.(uuid.UUID) + tenantID, ok := val.(uuid.UUID) if !ok { return uuid.Nil } - return userID + return tenantIDinternal/repositories/postgres/migrations/071_add_tenant_id_to_resources.up.sql (1)
4-66: Consider adding NOT NULL constraint after data migration.The
tenant_idcolumn is added as nullable. While this is necessary for the initial migration to avoid breaking existing rows, consider adding a follow-up migration (after072_migrate_to_tenants.up.sqlpopulates the data) to add aNOT NULLconstraint. This would prevent accidental insertion of resources without tenant association.Example follow-up migration:
ALTER TABLE instances ALTER COLUMN tenant_id SET NOT NULL; -- repeat for other tables...pkg/httputil/tenant_middleware.go (2)
35-40: Consider distinguishing error types in membership check.Currently, both service errors and missing membership return the same "Forbidden" response. If
GetMembershipreturns an internal error (e.g., database failure), returning "Forbidden" could mislead clients and complicate debugging.♻️ Suggested refinement
membership, err := tenantSvc.GetMembership(c.Request.Context(), tenantID, userID) - if err != nil || membership == nil { + if err != nil { + Error(c, err) // propagate actual error (could be internal) + c.Abort() + return + } + if membership == nil { Error(c, errors.New(errors.Forbidden, "not a member of this tenant")) c.Abort() return }
42-43: Consider using a named constant for the context key.Using
"tenantRole"as a string literal works but could lead to typos when accessed elsewhere. A named constant would improve maintainability.const TenantRoleKey = "tenantRole" // ... c.Set(TenantRoleKey, membership.Role)internal/core/domain/tenant.go (2)
9-19: Consider defining constants forPlanandStatusfields.The
PlanandStatusfields use magic strings documented in comments. Defining typed constants would improve type safety and prevent typos.♻️ Suggested constants
// Tenant plans const ( PlanFree = "free" PlanPro = "pro" PlanEnterprise = "enterprise" ) // Tenant statuses const ( StatusActive = "active" StatusSuspended = "suspended" )
21-27: Consider defining constants forRolefield.Similar to the
Tenantstruct, theRolefield uses magic strings. Constants would provide better maintainability.♻️ Suggested constants
// Tenant member roles const ( RoleOwner = "owner" RoleAdmin = "admin" RoleMember = "member" )internal/handlers/tenant_handler.go (3)
31-51: Swagger annotations are incomplete.The Swagger documentation is missing
@Accept,@Produce,@Param,@Success, and@Failuredirectives. Additionally, the router path format should use{id}instead of:idfor OpenAPI compliance.📝 Complete Swagger annotations example
// Create godoc // `@Summary` Create a new tenant // `@Tags` Tenant // `@Accept` json // `@Produce` json // `@Security` APIKeyAuth // `@Param` request body CreateTenantRequest true "Tenant creation request" // `@Success` 201 {object} domain.Tenant // `@Failure` 400 {object} errors.Error // `@Failure` 500 {object} errors.Error // `@Router` /tenants [post]
53-77: Swagger router path uses incorrect format.The
@Routerdirective uses:idwhich is Gin's format, but OpenAPI/Swagger expects{id}.📝 Proposed fix
-// `@Router` /tenants/:id/members [post] +// `@Router` /tenants/{id}/members [post]
79-98: Swagger router path uses incorrect format.Same issue as
InviteMember- use{id}instead of:id.📝 Proposed fix
-// `@Router` /tenants/:id/switch [post] +// `@Router` /tenants/{id}/switch [post]internal/handlers/tenant_handler_test.go (2)
68-95: Consider using table-driven tests for better coverage.Per coding guidelines, table-driven tests are recommended. This would also make it easier to add edge cases.
♻️ Table-driven test example
func TestTenantHandler_Create(t *testing.T) { tests := []struct { name string body any setupMock func(*mockTenantService, uuid.UUID) expectedStatus int expectedBody string }{ { name: "success", body: CreateTenantRequest{Name: "My Tenant", Slug: "my-tenant"}, setupMock: func(svc *mockTenantService, userID uuid.UUID) { svc.On("CreateTenant", mock.Anything, "My Tenant", "my-tenant", userID). Return(&domain.Tenant{Name: "My Tenant"}, nil) }, expectedStatus: http.StatusCreated, expectedBody: "My Tenant", }, // Add more cases... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // ... test implementation }) } }
89-91: SetContent-Typeheader on POST requests.JSON binding in Gin may behave differently without the proper Content-Type header. Setting it explicitly ensures consistent behavior.
♻️ Proposed fix
w := httptest.NewRecorder() req, _ := http.NewRequest("POST", "/tenants", bytes.NewBuffer(body)) +req.Header.Set("Content-Type", "application/json") r.ServeHTTP(w, req)This applies to all POST requests in the test file (lines 113, 131, 147, 165, 198, 227).
internal/core/services/tenant_test.go (1)
14-21: Remove unusedauditSvcfrom setup function.The
MockAuditServiceis created but never used in any test, andTenantServicedoesn't appear to require it based on the constructor intenant.go.♻️ Proposed fix
-func setupTenantServiceTest(_ *testing.T) (*MockTenantRepo, *MockUserRepo, *MockAuditService, *services.TenantService) { +func setupTenantServiceTest() (*MockTenantRepo, *MockUserRepo, *services.TenantService) { tenantRepo := new(MockTenantRepo) userRepo := new(MockUserRepo) - auditSvc := new(MockAuditService) // We pass nil for logger for now, or you can mock it if needed svc := services.NewTenantService(tenantRepo, userRepo, nil) - return tenantRepo, userRepo, auditSvc, svc + return tenantRepo, userRepo, svc }Then update all test functions to use the simplified return:
-tenantRepo, userRepo, _, svc := setupTenantServiceTest(t) +tenantRepo, userRepo, svc := setupTenantServiceTest()internal/core/ports/tenant.go (1)
28-37: Consider addingUpdateTenantandDeleteTenantmethods for completeness.The service interface lacks update and delete operations that are present in the repository. This may be intentional if those operations aren't needed yet, but adding them would provide a complete CRUD interface.
♻️ Suggested additions
type TenantService interface { CreateTenant(ctx context.Context, name, slug string, ownerID uuid.UUID) (*domain.Tenant, error) GetTenant(ctx context.Context, id uuid.UUID) (*domain.Tenant, error) + UpdateTenant(ctx context.Context, id uuid.UUID, name string) (*domain.Tenant, error) + DeleteTenant(ctx context.Context, id uuid.UUID) error InviteMember(ctx context.Context, tenantID uuid.UUID, email, role string) error RemoveMember(ctx context.Context, tenantID, userID uuid.UUID) error SwitchTenant(ctx context.Context, userID, tenantID uuid.UUID) error CheckQuota(ctx context.Context, tenantID uuid.UUID, resource string, requested int) error GetMembership(ctx context.Context, tenantID, userID uuid.UUID) (*domain.TenantMember, error) }internal/repositories/postgres/migrations/070_create_tenants.up.sql (1)
5-5: Redundant index onslugcolumn.The
slugcolumn already has aUNIQUEconstraint (line 5), which automatically creates a unique index in PostgreSQL. The explicitidx_tenants_slugindex on line 13 is redundant and wastes storage.♻️ Suggested fix
CREATE INDEX idx_tenants_slug ON tenants(slug); +-- Remove this line: UNIQUE constraint already creates an indexOr simply remove line 13 entirely.
Also applies to: 13-13
internal/core/services/tenant.go (1)
29-33: Discarded error could mask database issues.The error from
GetBySlugis discarded with_, _. If the database is unavailable or times out, this would be silently ignored and the code would proceed to create a potentially duplicate tenant (which would then fail on DB constraint).Consider distinguishing between "not found" (expected) and actual errors.
♻️ Suggested fix
// 1. Check if slug exists - existing, _ := s.repo.GetBySlug(ctx, slug) - if existing != nil { + existing, err := s.repo.GetBySlug(ctx, slug) + if err == nil && existing != nil { return nil, errors.New(errors.Conflict, "tenant slug already exists") } + // If err is not a NotFound error, it could indicate a DB issue + // but we'll let the Create call handle the conflict via DB constraintinternal/repositories/postgres/security_group_repo.go (1)
128-135: Redundant error check afterscanSecurityRule.The
scanSecurityRulefunction already handlespgx.ErrNoRowsinternally and returns the error. The check forpgx.ErrNoRowson lines 130-131 will never be true because the scan function would have returned a different error type.♻️ Suggested fix
rule, err := r.scanSecurityRule(r.db.QueryRow(ctx, query, ruleID, tenantID)) if err != nil { - if err == pgx.ErrNoRows { - return nil, errors.New(errors.NotFound, "security rule not found") - } - return nil, errors.Wrap(errors.Internal, "failed to get security rule", err) + return nil, err } return &rule, nilNote: This assumes
scanSecurityRuleshould handle the NotFound case. If not, the error handling should be inscanSecurityRuleinstead.internal/repositories/postgres/multitenancy_integration_test.go (1)
21-23: Silent failure on TRUNCATE could mask test setup issues.Using
_, _ = db.Execdiscards any errors from the TRUNCATE command. If the database connection fails or tables don't exist, tests would proceed with corrupt state.♻️ Suggested fix
// Clear tables in correct order using TRUNCATE CASCADE to handle circular dependencies ctx := context.Background() - _, _ = db.Exec(ctx, "TRUNCATE users, tenants, tenant_members, vpcs, instances, volumes CASCADE") + _, err := db.Exec(ctx, "TRUNCATE users, tenants, tenant_members, vpcs, instances, volumes CASCADE") + require.NoError(t, err, "failed to truncate tables for test setup")
There was a problem hiding this comment.
Pull request overview
This PR introduces tenant-based multi-tenancy support (tenant context propagation, tenant-scoped data access, and tenant management endpoints), along with updated integration/unit tests and database migrations to support tenant isolation.
Changes:
- Added tenant context handling in HTTP auth flow plus new tenant membership/require-tenant middleware.
- Introduced tenant domain model, ports, service, handler, and Postgres repository implementation.
- Updated core repositories (instances, VPCs, volumes, security groups) and added migrations to tenant-scope persisted resources; expanded integration tests for tenant isolation.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/httputil/tenant_middleware.go | Adds middleware to require an active tenant and validate tenant membership. |
| pkg/httputil/auth.go | Resolves active tenant from header/default and exposes GetTenantID. |
| internal/repositories/postgres/vpc_repo.go | Scopes VPC persistence/queries by tenant_id. |
| internal/repositories/postgres/volume_repo.go | Scopes volume queries by tenant_id and persists tenant_id. |
| internal/repositories/postgres/tenant_repo.go | New Postgres repository for tenants, members, and quotas. |
| internal/repositories/postgres/security_group_repo.go | Scopes security group and rule access by tenant_id; updates tenant ownership checks. |
| internal/repositories/postgres/multitenancy_integration_test.go | Updates integration test to validate tenant isolation and tenant membership behavior. |
| internal/repositories/postgres/migrations/070_create_tenants.up.sql | Creates tenants/members/quotas tables and adds default tenant columns. |
| internal/repositories/postgres/migrations/070_create_tenants.down.sql | Rollback for tenant schema changes. |
| internal/repositories/postgres/migrations/071_add_tenant_id_to_resources.up.sql | Adds tenant_id columns + indexes across resource tables. |
| internal/repositories/postgres/migrations/071_add_tenant_id_to_resources.down.sql | Rollback for tenant_id columns + indexes. |
| internal/repositories/postgres/migrations/072_migrate_to_tenants.up.sql | Backfills tenants, memberships, default tenant IDs, and resource tenant IDs. |
| internal/repositories/postgres/instance_repo.go | Scopes instance persistence/queries by tenant_id. |
| internal/handlers/tenant_handler.go | New API handler for tenant create/invite/switch endpoints. |
| internal/handlers/tenant_handler_test.go | Unit tests for the new tenant handler endpoints. |
| internal/errors/errors.go | Adds new error types for permissions/quota. |
| internal/core/services/tenant.go | New tenant service for tenant management and quota checking. |
| internal/core/services/tenant_test.go | Unit tests for tenant service behaviors. |
| internal/core/services/shared_test.go | Adds mocks for tenant repo/service used by tests. |
| internal/core/ports/tenant.go | Adds tenant repository/service interfaces. |
| internal/core/domain/tenant.go | Adds tenant, member, and quota domain models. |
| internal/core/domain/user.go | Adds DefaultTenantID to User. |
| internal/core/domain/identity.go | Adds DefaultTenantID to APIKey. |
| internal/core/domain/instance.go | Adds TenantID to Instance. |
| internal/core/domain/vpc.go | Adds TenantID to VPC. |
| internal/core/domain/volume.go | Adds TenantID to Volume. |
| internal/core/domain/security_group.go | Adds TenantID to SecurityGroup. |
| internal/core/context/context.go | Adds tenant ID helpers on request context. |
| internal/api/setup/dependencies.go | Wires tenant repo/service into app dependencies. |
| internal/api/setup/router.go | Adds tenant routes and applies tenant middleware to selected route groups. |
Comments suppressed due to low confidence (1)
internal/repositories/postgres/security_group_repo.go:150
- The not-found message still says “you do not own it”, but the query is now tenant-scoped (
sg.tenant_id). This is misleading for clients and harder to debug. Update the message to reflect tenant membership/authorization instead of ownership.
}
if res.RowsAffected() == 0 {
return errors.New(errors.NotFound, "security rule not found or you do not own it")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| query := `UPDATE volumes SET status = $1, instance_id = $2, backend_path = $3, mount_path = $4, updated_at = $5 WHERE id = $6 AND tenant_id = $7` | ||
| _, err := r.db.Exec(ctx, query, string(v.Status), v.InstanceID, v.BackendPath, v.MountPath, v.UpdatedAt, v.ID, v.TenantID) |
There was a problem hiding this comment.
Update scopes by tenant_id using v.TenantID from the mutable volume object rather than the tenant ID from ctx (while reads/deletes use appcontext.TenantIDFromContext(ctx)). To prevent cross-tenant updates, derive the tenant for authorization from context and not from the request payload/object.
| query := `UPDATE volumes SET status = $1, instance_id = $2, backend_path = $3, mount_path = $4, updated_at = $5 WHERE id = $6 AND tenant_id = $7` | |
| _, err := r.db.Exec(ctx, query, string(v.Status), v.InstanceID, v.BackendPath, v.MountPath, v.UpdatedAt, v.ID, v.TenantID) | |
| tenantID := appcontext.TenantIDFromContext(ctx) | |
| query := `UPDATE volumes SET status = $1, instance_id = $2, backend_path = $3, mount_path = $4, updated_at = $5 WHERE id = $6 AND tenant_id = $7` | |
| _, err := r.db.Exec(ctx, query, string(v.Status), v.InstanceID, v.BackendPath, v.MountPath, v.UpdatedAt, v.ID, tenantID) |
| // GetTenantID returns the active tenant ID from the request context. | ||
| func GetTenantID(c *gin.Context) uuid.UUID { | ||
| val, exists := c.Get("tenantID") | ||
| if !exists { | ||
| return uuid.Nil |
There was a problem hiding this comment.
GetTenantID’s docstring says it reads from the “request context”, but it actually reads from the Gin context key (c.Get("tenantID")). This is misleading for callers and makes it easy to assume appcontext.WithTenantID alone is sufficient. Consider updating the comment and renaming the local var used for the type assertion to tenantID for clarity.
| ` | ||
| now := time.Now() | ||
| cmd, err := r.db.Exec(ctx, query, inst.Name, string(inst.Status), now, inst.ContainerID, inst.Ports, inst.VpcID, inst.SubnetID, inst.PrivateIP, inst.OvsPort, inst.ID, inst.Version, inst.UserID) | ||
| cmd, err := r.db.Exec(ctx, query, inst.Name, string(inst.Status), now, inst.ContainerID, inst.Ports, inst.VpcID, inst.SubnetID, inst.PrivateIP, inst.OvsPort, inst.ID, inst.Version, inst.TenantID) |
There was a problem hiding this comment.
Update scopes the WHERE tenant_id = ... clause using inst.TenantID from the mutable domain object rather than the tenant ID from the request context (unlike reads/deletes in this repo). This makes cross-tenant updates easier if a caller supplies a mismatched TenantID. Use appcontext.TenantIDFromContext(ctx) for the WHERE clause and treat inst.TenantID as data, not authorization.
| cmd, err := r.db.Exec(ctx, query, inst.Name, string(inst.Status), now, inst.ContainerID, inst.Ports, inst.VpcID, inst.SubnetID, inst.PrivateIP, inst.OvsPort, inst.ID, inst.Version, inst.TenantID) | |
| tenantID := appcontext.TenantIDFromContext(ctx) | |
| cmd, err := r.db.Exec(ctx, query, inst.Name, string(inst.Status), now, inst.ContainerID, inst.Ports, inst.VpcID, inst.SubnetID, inst.PrivateIP, inst.OvsPort, inst.ID, inst.Version, tenantID) |
| registerNetworkRoutes(r, handlers, services) | ||
| registerDataRoutes(r, handlers, services) | ||
| registerDevOpsRoutes(r, handlers, services) | ||
| registerTenantRoutes(r, handlers, services) |
There was a problem hiding this comment.
The PR title/description focus on test coverage improvements, but this change set introduces significant new production functionality (tenants schema, middleware, new service/repo, route wiring). Please update the PR description to reflect the scope (or split into separate PRs) so reviewers can assess risk and rollout/migration implications appropriately.
| Forbidden Type = "FORBIDDEN" | ||
| PermissionDenied Type = "PERMISSION_DENIED" | ||
| ResourceLimitExceeded Type = "RESOURCE_LIMIT_EXCEEDED" | ||
| QuotaExceeded Type = "QUOTA_EXCEEDED" |
There was a problem hiding this comment.
New error types PermissionDenied and QuotaExceeded are added, but pkg/httputil/response.go’s status mapping doesn’t include them. TenantService.CheckQuota returns errors.QuotaExceeded, which will currently be rendered as HTTP 500. Add explicit HTTP status mappings to keep API responses consistent.
| if err := s.repo.UpdateQuota(ctx, quota); err != nil { | ||
| // Log but don't fail tenant creation | ||
| s.logger.Error("failed to set default quota", "tenant_id", tenant.ID, "err", err) | ||
| } |
There was a problem hiding this comment.
CreateTenant can panic when UpdateQuota fails because s.logger can be nil (e.g., unit tests currently pass nil, and nil loggers are easy to introduce in other wiring). Guard the log call (if s.logger != nil) or default logger in NewTenantService to slog.Default().
| @@ -0,0 +1,4 @@ | |||
| ALTER TABLE users DROP COLUMN IF EXISTS default_tenant_id; | |||
There was a problem hiding this comment.
Rollback is incomplete: the UP migration adds default_tenant_id to api_keys, but the DOWN migration only drops it from users. Dropping tenants will fail while api_keys.default_tenant_id still has a FK to tenants. Add a corresponding ALTER TABLE api_keys DROP COLUMN IF EXISTS default_tenant_id (or drop the FK) in the down migration.
| ALTER TABLE users DROP COLUMN IF EXISTS default_tenant_id; | |
| ALTER TABLE users DROP COLUMN IF EXISTS default_tenant_id; | |
| ALTER TABLE api_keys DROP COLUMN IF EXISTS default_tenant_id; |
| // 5. Update user's default tenant if not set | ||
| user, err := s.userRepo.GetByID(ctx, ownerID) | ||
| if err == nil && user.DefaultTenantID == nil { | ||
| user.DefaultTenantID = &tenant.ID | ||
| _ = s.userRepo.Update(ctx, user) | ||
| } | ||
|
|
There was a problem hiding this comment.
This code attempts to persist user.DefaultTenantID, but the current Postgres UserRepo does not select or update the default_tenant_id column (see internal/repositories/postgres/user_repo.go:55-76). That means default-tenant assignment won’t actually persist. Update UserRepo queries/scans/updates to include default_tenant_id.
| // 5. Update user's default tenant if not set | |
| user, err := s.userRepo.GetByID(ctx, ownerID) | |
| if err == nil && user.DefaultTenantID == nil { | |
| user.DefaultTenantID = &tenant.ID | |
| _ = s.userRepo.Update(ctx, user) | |
| } |
| DROP INDEX IF EXISTS idx_security_groups_tenant; | ||
| DROP INDEX IF EXISTS idx_floating_ips_tenant; | ||
| DROP INDEX IF EXISTS idx_clusters_tenant; |
There was a problem hiding this comment.
This DOWN migration drops idx_floating_ips_tenant (and later floating_ips.tenant_id), but the corresponding UP migration never creates those (and there’s no floating_ips table in the migrations set). This will break rollback/migration tests on a clean schema. Remove the floating_ips entries or add matching UP changes.
| } else if apiKeyObj.DefaultTenantID != nil { | ||
| tenantID = *apiKeyObj.DefaultTenantID | ||
| } |
There was a problem hiding this comment.
Auth now falls back to apiKeyObj.DefaultTenantID, but the PostgreSQL identity repository doesn’t select/scan default_tenant_id from api_keys (see internal/repositories/postgres/identity_repo.go:38-71). As a result, default-tenant resolution will never work and RequireTenant() will reject requests unless X-Tenant-ID is provided. Update the identity repo queries/scans to include default_tenant_id (nullable UUID) so DefaultTenantID is populated.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/swagger/docs.go`:
- Around line 3705-3745: The Swagger annotations for tenant endpoints are
missing path param and response tags; in internal/handlers/tenant_handler.go add
`@Param` id path string true "Tenant ID" to the InviteMember and SwitchTenant
handlers (refer to functions InviteMember and SwitchTenant) and add appropriate
`@Success` and `@Failure` response annotations to all three tenant handlers
(CreateTenant, InviteMember, SwitchTenant) matching your existing response
schemas (e.g., success 200/201 models and failure 4xx/5xx error models); after
updating the comments, regenerate the docs so docs/swagger/docs.go uses {id} in
paths and contains populated responses.
In `@docs/swagger/swagger.json`:
- Around line 3694-3734: The Swagger paths "/tenants/:id/members" and
"/tenants/:id/switch" use Express-style ":id" which is invalid for Swagger;
change the path keys to "/tenants/{id}/members" and "/tenants/{id}/switch" and
add a path parameter declaration for "id" (in the operations under the "post"
for the Tenant tag) with "name": "id", "in": "path", "required": true and an
appropriate "type"/"schema" (string or integer as used elsewhere) so clients and
parameter binding work correctly; leave the "/tenants" POST as-is.
In `@docs/swagger/swagger.yaml`:
- Around line 3697-3720: Update the tenant paths to use Swagger 2.0 compliant
path parameter syntax and add explicit responses and a path parameter
declaration: replace any occurrences of "/tenants/:id/members" and
"/tenants/:id/switch" with "/tenants/{id}/members" and "/tenants/{id}/switch",
add a "parameters" array for each path including a path parameter named "id"
(in: path, required: true, type: string, description), and replace the empty
"responses: {}" blocks with explicit response objects matching the handlers
(e.g., 201 or 200 for successful creation/invite, 204 or 200 for switch, and at
least a default or 4xx/5xx error response with a schema reference) for the POST
operations under "/tenants", "/tenants/{id}/members", and
"/tenants/{id}/switch".
🧹 Nitpick comments (3)
docs/swagger/swagger.json (3)
4095-4097: Clarifydefault_tenant_idas server-managed.To avoid clients treating this as writable, consider adding a description and
readOnly.✍️ Suggested doc tweak
"default_tenant_id": { - "type": "string" + "type": "string", + "description": "Server-assigned default tenant for the principal", + "readOnly": true },Also applies to: 5344-5346
4428-4457: Documenttenant_idas non-client-writable in Instance schema.If
tenant_idis derived from auth context, mark it read-only to prevent clients from trying to set it.📝 Suggested doc tweak
"tenant_id": { - "type": "string" + "type": "string", + "description": "Tenant owning the instance (server-assigned)", + "readOnly": true },
5068-5073: Apply the sametenant_iddocumentation to other resource schemas.For SecurityGroup/VPC/Volume, consider adding descriptions and
readOnlyto reduce ambiguity for client SDKs.🧩 Suggested doc tweak (apply similarly to each schema)
"tenant_id": { - "type": "string" + "type": "string", + "description": "Tenant owning the resource (server-assigned)", + "readOnly": true },Also applies to: 5393-5398, 5435-5443
| "/tenants": { | ||
| "post": { | ||
| "security": [ | ||
| { | ||
| "APIKeyAuth": [] | ||
| } | ||
| ], | ||
| "tags": [ | ||
| "Tenant" | ||
| ], | ||
| "summary": "Create a new tenant", | ||
| "responses": {} | ||
| } | ||
| }, | ||
| "/tenants/:id/members": { | ||
| "post": { | ||
| "security": [ | ||
| { | ||
| "APIKeyAuth": [] | ||
| } | ||
| ], | ||
| "tags": [ | ||
| "Tenant" | ||
| ], | ||
| "summary": "Invite member to tenant", | ||
| "responses": {} | ||
| } | ||
| }, | ||
| "/tenants/:id/switch": { | ||
| "post": { | ||
| "security": [ | ||
| { | ||
| "APIKeyAuth": [] | ||
| } | ||
| ], | ||
| "tags": [ | ||
| "Tenant" | ||
| ], | ||
| "summary": "Switch active tenant", | ||
| "responses": {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get basic info
ls -la docs/swagger/docs.go
wc -l docs/swagger/docs.goRepository: PoyrazK/thecloud
Length of output: 157
🏁 Script executed:
# Read the specific lines mentioned in the review (3705-3745)
sed -n '3705,3745p' docs/swagger/docs.goRepository: PoyrazK/thecloud
Length of output: 1153
🏁 Script executed:
# Also check around those lines for context
sed -n '3700,3750p' docs/swagger/docs.goRepository: PoyrazK/thecloud
Length of output: 1350
🏁 Script executed:
# Search for tenant-related Swagger annotations in the codebase
fd -e go -type f | xargs grep -l "Tenant" | head -20Repository: PoyrazK/thecloud
Length of output: 230
🏁 Script executed:
# Look for swagger:operation or swagger:route annotations related to tenants
rg 'swagger:(operation|route).*tenant|/tenants' --type go -A 5 -B 2 | head -100Repository: PoyrazK/thecloud
Length of output: 6308
🏁 Script executed:
# Check how other endpoints with path params are defined in docs.go
sed -n '1,100p' docs/swagger/docs.go | head -50Repository: PoyrazK/thecloud
Length of output: 1645
🏁 Script executed:
# Search for another endpoint with path params to compare format
rg '"/[a-z]+/{id}' docs/swagger/docs.go -A 10 | head -50Repository: PoyrazK/thecloud
Length of output: 165
🏁 Script executed:
# Check the full definition of tenant endpoints including parameters
sed -n '3700,3800p' docs/swagger/docs.goRepository: PoyrazK/thecloud
Length of output: 3069
🏁 Script executed:
# Look for parameters definition in tenant endpoints
rg '/tenants' docs/swagger/docs.go -A 30 | head -100Repository: PoyrazK/thecloud
Length of output: 1695
🏁 Script executed:
# Look for other path parameter examples in docs.go
rg '\{id\}' docs/swagger/docs.go -B 2 -A 10 | head -100Repository: PoyrazK/thecloud
Length of output: 2886
🏁 Script executed:
# Search for instances of path params to see the pattern used
rg '/:id|/{id}' docs/swagger/docs.go | head -20Repository: PoyrazK/thecloud
Length of output: 159
🏁 Script executed:
# Check the tenant_handler.go file for full swagger annotations
cat -n internal/handlers/tenant_handler.go | grep -A 15 "Create\|InviteMember\|SwitchTenant" | head -80Repository: PoyrazK/thecloud
Length of output: 2698
🏁 Script executed:
# Search for swagger annotations that have `@Param` for path parameters
rg '@Param.*path' . -t go -B 2 -A 2 | head -50Repository: PoyrazK/thecloud
Length of output: 2937
🏁 Script executed:
# Look at the complete swagger annotation block for one of the tenant handlers
sed -n '1,200p' internal/handlers/tenant_handler.goRepository: PoyrazK/thecloud
Length of output: 2551
Add path parameter declarations and responses to tenant endpoint annotations.
The tenant endpoints are missing @Param declarations for the id path parameter and lack @Success/@Failure response annotations. Compare with other handlers like lb_handler.go: they use @Param id path string true "..." for each path parameter and define response schemas. Without these, the generated Swagger spec has empty responses: {} and uses invalid :id path syntax instead of OpenAPI 2.0's {id} format, which breaks client generation and API documentation.
Update internal/handlers/tenant_handler.go lines 53–83 to add:
@Param id path string true "Tenant ID"for InviteMember and SwitchTenant@Successand@Failureresponse annotations for all three endpoints- Then regenerate
docs/swagger/docs.go
🤖 Prompt for AI Agents
In `@docs/swagger/docs.go` around lines 3705 - 3745, The Swagger annotations for
tenant endpoints are missing path param and response tags; in
internal/handlers/tenant_handler.go add `@Param` id path string true "Tenant ID"
to the InviteMember and SwitchTenant handlers (refer to functions InviteMember
and SwitchTenant) and add appropriate `@Success` and `@Failure` response annotations
to all three tenant handlers (CreateTenant, InviteMember, SwitchTenant) matching
your existing response schemas (e.g., success 200/201 models and failure 4xx/5xx
error models); after updating the comments, regenerate the docs so
docs/swagger/docs.go uses {id} in paths and contains populated responses.
| "/tenants": { | ||
| "post": { | ||
| "security": [ | ||
| { | ||
| "APIKeyAuth": [] | ||
| } | ||
| ], | ||
| "tags": [ | ||
| "Tenant" | ||
| ], | ||
| "summary": "Create a new tenant", | ||
| "responses": {} | ||
| } | ||
| }, | ||
| "/tenants/:id/members": { | ||
| "post": { | ||
| "security": [ | ||
| { | ||
| "APIKeyAuth": [] | ||
| } | ||
| ], | ||
| "tags": [ | ||
| "Tenant" | ||
| ], | ||
| "summary": "Invite member to tenant", | ||
| "responses": {} | ||
| } | ||
| }, | ||
| "/tenants/:id/switch": { | ||
| "post": { | ||
| "security": [ | ||
| { | ||
| "APIKeyAuth": [] | ||
| } | ||
| ], | ||
| "tags": [ | ||
| "Tenant" | ||
| ], | ||
| "summary": "Switch active tenant", | ||
| "responses": {} | ||
| } |
There was a problem hiding this comment.
Fix invalid Swagger path templates for tenant routes.
Swagger 2.0 does not recognize :id in path keys, so these endpoints won’t generate correct clients or parameter binding. Use {id} and declare the path parameter.
🔧 Proposed fix
- "/tenants/:id/members": {
+ "/tenants/{id}/members": {
"post": {
"security": [
{
"APIKeyAuth": []
}
],
"tags": [
"Tenant"
],
"summary": "Invite member to tenant",
+ "parameters": [
+ {
+ "type": "string",
+ "description": "Tenant ID",
+ "name": "id",
+ "in": "path",
+ "required": true
+ }
+ ],
"responses": {}
}
},
- "/tenants/:id/switch": {
+ "/tenants/{id}/switch": {
"post": {
"security": [
{
"APIKeyAuth": []
}
],
"tags": [
"Tenant"
],
"summary": "Switch active tenant",
+ "parameters": [
+ {
+ "type": "string",
+ "description": "Tenant ID",
+ "name": "id",
+ "in": "path",
+ "required": true
+ }
+ ],
"responses": {}
}
}📝 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.
| "/tenants": { | |
| "post": { | |
| "security": [ | |
| { | |
| "APIKeyAuth": [] | |
| } | |
| ], | |
| "tags": [ | |
| "Tenant" | |
| ], | |
| "summary": "Create a new tenant", | |
| "responses": {} | |
| } | |
| }, | |
| "/tenants/:id/members": { | |
| "post": { | |
| "security": [ | |
| { | |
| "APIKeyAuth": [] | |
| } | |
| ], | |
| "tags": [ | |
| "Tenant" | |
| ], | |
| "summary": "Invite member to tenant", | |
| "responses": {} | |
| } | |
| }, | |
| "/tenants/:id/switch": { | |
| "post": { | |
| "security": [ | |
| { | |
| "APIKeyAuth": [] | |
| } | |
| ], | |
| "tags": [ | |
| "Tenant" | |
| ], | |
| "summary": "Switch active tenant", | |
| "responses": {} | |
| } | |
| "/tenants": { | |
| "post": { | |
| "security": [ | |
| { | |
| "APIKeyAuth": [] | |
| } | |
| ], | |
| "tags": [ | |
| "Tenant" | |
| ], | |
| "summary": "Create a new tenant", | |
| "responses": {} | |
| } | |
| }, | |
| "/tenants/{id}/members": { | |
| "post": { | |
| "security": [ | |
| { | |
| "APIKeyAuth": [] | |
| } | |
| ], | |
| "tags": [ | |
| "Tenant" | |
| ], | |
| "summary": "Invite member to tenant", | |
| "parameters": [ | |
| { | |
| "type": "string", | |
| "description": "Tenant ID", | |
| "name": "id", | |
| "in": "path", | |
| "required": true | |
| } | |
| ], | |
| "responses": {} | |
| } | |
| }, | |
| "/tenants/{id}/switch": { | |
| "post": { | |
| "security": [ | |
| { | |
| "APIKeyAuth": [] | |
| } | |
| ], | |
| "tags": [ | |
| "Tenant" | |
| ], | |
| "summary": "Switch active tenant", | |
| "parameters": [ | |
| { | |
| "type": "string", | |
| "description": "Tenant ID", | |
| "name": "id", | |
| "in": "path", | |
| "required": true | |
| } | |
| ], | |
| "responses": {} | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@docs/swagger/swagger.json` around lines 3694 - 3734, The Swagger paths
"/tenants/:id/members" and "/tenants/:id/switch" use Express-style ":id" which
is invalid for Swagger; change the path keys to "/tenants/{id}/members" and
"/tenants/{id}/switch" and add a path parameter declaration for "id" (in the
operations under the "post" for the Tenant tag) with "name": "id", "in": "path",
"required": true and an appropriate "type"/"schema" (string or integer as used
elsewhere) so clients and parameter binding work correctly; leave the "/tenants"
POST as-is.
| /tenants: | ||
| post: | ||
| responses: {} | ||
| security: | ||
| - APIKeyAuth: [] | ||
| summary: Create a new tenant | ||
| tags: | ||
| - Tenant | ||
| /tenants/:id/members: | ||
| post: | ||
| responses: {} | ||
| security: | ||
| - APIKeyAuth: [] | ||
| summary: Invite member to tenant | ||
| tags: | ||
| - Tenant | ||
| /tenants/:id/switch: | ||
| post: | ||
| responses: {} | ||
| security: | ||
| - APIKeyAuth: [] | ||
| summary: Switch active tenant | ||
| tags: | ||
| - Tenant |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
ls -lh docs/swagger/swagger.yaml
# Get the OpenAPI/Swagger version declared in the file
head -20 docs/swagger/swagger.yaml
# Check the specific lines mentioned (3697-3720)
sed -n '3697,3720p' docs/swagger/swagger.yaml
# Also check a broader context to see if there are other path param examples
sed -n '3700,3750p' docs/swagger/swagger.yamlRepository: PoyrazK/thecloud
Length of output: 2144
🏁 Script executed:
# Search for existing path parameter patterns in the file
rg "/{[a-zA-Z]" docs/swagger/swagger.yaml -A 2 -B 2 | head -60
# Also search for :id style params to see if this is a consistent pattern
rg ":[a-zA-Z]" docs/swagger/swagger.yaml | head -20Repository: PoyrazK/thecloud
Length of output: 634
🏁 Script executed:
# Get the full header to see swagger version
head -30 docs/swagger/swagger.yaml
# Search for proper path params with curly braces (need to escape properly)
rg '\{[a-zA-Z]+\}' docs/swagger/swagger.yaml -B 2 | head -100
# Search for :id style params more broadly
rg ':[a-zA-Z]+' docs/swagger/swagger.yaml | head -30Repository: PoyrazK/thecloud
Length of output: 2827
🏁 Script executed:
# Check how other endpoints with path parameters are defined
rg "parameters:" docs/swagger/swagger.yaml -A 10 | head -100Repository: PoyrazK/thecloud
Length of output: 2124
🏁 Script executed:
# Verify swagger version declaration
head -5 docs/swagger/swagger.yaml
# Confirm the problematic tenant endpoints exist exactly as shown
sed -n '3697,3720p' docs/swagger/swagger.yamlRepository: PoyrazK/thecloud
Length of output: 601
Fix Swagger 2.0 compliance for tenant endpoints: use {id} paths, add responses, and declare path parameters.
The tenant endpoints use invalid :id syntax and have incomplete definitions. Swagger 2.0 requires {id} format for path parameters, explicit response codes (not empty responses: {}), and parameter declarations. All other endpoints in this file follow the correct pattern.
🛠️ Proposed fix (adjust status codes/schemas to match handlers)
- /tenants:
+ /tenants:
post:
- responses: {}
+ responses:
+ "201":
+ description: Created
security:
- APIKeyAuth: []
summary: Create a new tenant
tags:
- Tenant
- /tenants/:id/members:
+ /tenants/{id}/members:
post:
+ parameters:
+ - in: path
+ name: id
+ required: true
+ type: string
- responses: {}
+ responses:
+ "200":
+ description: OK
security:
- APIKeyAuth: []
summary: Invite member to tenant
tags:
- Tenant
- /tenants/:id/switch:
+ /tenants/{id}/switch:
post:
+ parameters:
+ - in: path
+ name: id
+ required: true
+ type: string
- responses: {}
+ responses:
+ "200":
+ description: OK
security:
- APIKeyAuth: []
summary: Switch active tenant
tags:
- Tenant📝 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.
| /tenants: | |
| post: | |
| responses: {} | |
| security: | |
| - APIKeyAuth: [] | |
| summary: Create a new tenant | |
| tags: | |
| - Tenant | |
| /tenants/:id/members: | |
| post: | |
| responses: {} | |
| security: | |
| - APIKeyAuth: [] | |
| summary: Invite member to tenant | |
| tags: | |
| - Tenant | |
| /tenants/:id/switch: | |
| post: | |
| responses: {} | |
| security: | |
| - APIKeyAuth: [] | |
| summary: Switch active tenant | |
| tags: | |
| - Tenant | |
| /tenants: | |
| post: | |
| responses: | |
| "201": | |
| description: Created | |
| security: | |
| - APIKeyAuth: [] | |
| summary: Create a new tenant | |
| tags: | |
| - Tenant | |
| /tenants/{id}/members: | |
| post: | |
| parameters: | |
| - in: path | |
| name: id | |
| required: true | |
| type: string | |
| responses: | |
| "200": | |
| description: OK | |
| security: | |
| - APIKeyAuth: [] | |
| summary: Invite member to tenant | |
| tags: | |
| - Tenant | |
| /tenants/{id}/switch: | |
| post: | |
| parameters: | |
| - in: path | |
| name: id | |
| required: true | |
| type: string | |
| responses: | |
| "200": | |
| description: OK | |
| security: | |
| - APIKeyAuth: [] | |
| summary: Switch active tenant | |
| tags: | |
| - Tenant |
🤖 Prompt for AI Agents
In `@docs/swagger/swagger.yaml` around lines 3697 - 3720, Update the tenant paths
to use Swagger 2.0 compliant path parameter syntax and add explicit responses
and a path parameter declaration: replace any occurrences of
"/tenants/:id/members" and "/tenants/:id/switch" with "/tenants/{id}/members"
and "/tenants/{id}/switch", add a "parameters" array for each path including a
path parameter named "id" (in: path, required: true, type: string, description),
and replace the empty "responses: {}" blocks with explicit response objects
matching the handlers (e.g., 201 or 200 for successful creation/invite, 204 or
200 for switch, and at least a default or 4xx/5xx error response with a schema
reference) for the POST operations under "/tenants", "/tenants/{id}/members",
and "/tenants/{id}/switch".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/core/services/auth.go`:
- Around line 81-114: The user is created before the tenant so a failed
s.tenantSvc.CreateTenant leaves an orphan user; fix by making the two operations
atomic: either (A) use a DB transaction around s.userRepo.Create and
s.tenantSvc.CreateTenant (use your repo/DB transaction functions and commit only
if both succeed, rolling back on error), or (B) implement a compensating delete
by adding and calling a removal method (e.g., userRepo.Delete(ctx, user.ID) or
s.userRepo.DeleteUser/DeleteByID) when s.tenantSvc.CreateTenant returns an error
(ensure Delete exists and handle its error/logging); update the code paths
around s.userRepo.Create and s.tenantSvc.CreateTenant accordingly.
- Around line 87-88: Remove the duplicated comment line "Create Personal Tenant"
so only a single instance of that comment remains; locate the consecutive
duplicate occurrences of the "Create Personal Tenant" comment in internal core
auth service code (the repeated comment above the personal-tenant creation
logic) and delete the extra line to leave one clear comment.
- Around line 116-120: The reload of the user via s.userRepo.GetByID silently
swallows errors which can mask real failures; update the block around
s.userRepo.GetByID(ctx, user.ID) so that when err != nil you either log the
error with context (including user.ID and operation like "reload user after
tenant creation") using the existing logger or propagate the error to the caller
instead of ignoring it; if you choose to propagate, return the err (or wrap it)
from the surrounding function, otherwise call the service logger (e.g., s.logger
or processLogger) with a clear message and keep the original user only after
successful updatedUser assignment.
In `@internal/repositories/postgres/instance_repo_test.go`:
- Around line 85-88: The test's mock for GetByID uses the wrong context and
args: GetByID extracts tenantID from context and queries WHERE id = $1 AND
tenant_id = $2, but the test only sets userID and calls WithArgs(id, userID).
Update the test to set both userID and tenantID in the context (use
appcontext.WithUserID and appcontext.WithTenantID around context.Background()),
change the mock's WithArgs to WithArgs(id, tenantID), and ensure the returned
row includes tenantID (the AddRow call) so mock.ExpectQuery(selectQuery) matches
the tenant-scoped query.
🧹 Nitpick comments (2)
internal/core/services/benchmarks_test.go (1)
160-182: Consider movingNoopTenantServiceto thenooppackage for consistency.Other noop service implementations (
NoopIdentityService,NoopAuditService) are defined ininternal/repositories/noop/adapters.go. PlacingNoopTenantServicethere would maintain consistency and enable reuse across other test files.♻️ Suggested location
Move
NoopTenantServicetointernal/repositories/noop/adapters.goalongside the other noop service implementations, then import it here:-type NoopTenantService struct{} - -func (s *NoopTenantService) CreateTenant(ctx context.Context, name, slug string, ownerID uuid.UUID) (*domain.Tenant, error) { - return &domain.Tenant{ID: uuid.New()}, nil -} -func (s *NoopTenantService) GetTenant(ctx context.Context, id uuid.UUID) (*domain.Tenant, error) { - return &domain.Tenant{ID: id}, nil -} -func (s *NoopTenantService) InviteMember(ctx context.Context, tenantID uuid.UUID, email, role string) error { - return nil -} -func (s *NoopTenantService) RemoveMember(ctx context.Context, tenantID, userID uuid.UUID) error { - return nil -} -func (s *NoopTenantService) SwitchTenant(ctx context.Context, userID, tenantID uuid.UUID) error { - return nil -} -func (s *NoopTenantService) CheckQuota(ctx context.Context, tenantID uuid.UUID, resource string, requested int) error { - return nil -} -func (s *NoopTenantService) GetMembership(ctx context.Context, tenantID, userID uuid.UUID) (*domain.TenantMember, error) { - return &domain.TenantMember{}, nil -}Then use
&noop.NoopTenantService{}in the benchmarks.internal/repositories/postgres/instance_repo_test.go (1)
294-299: Consider table-driven cases for delete success/not-found.This would align the test structure with the project’s testing style and make it easier to add new cases.
As per coding guidelines, please prefer table-driven tests in
*_test.gofiles.
| // Transactionality would be better here, but avoiding for simplicity unless needed | ||
| err = s.userRepo.Create(ctx, user) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Create Personal Tenant | ||
| // Create Personal Tenant | ||
| tenantName := fmt.Sprintf("%s's Personal Tenant", name) | ||
|
|
||
| // Simple slugify: lowercase, replace spaces with hyphens, keep only alphanumeric | ||
| slugName := strings.ToLower(name) | ||
| slugName = strings.ReplaceAll(slugName, " ", "-") | ||
|
|
||
| // Remove non-alphanumeric chars | ||
| var cleanSlug strings.Builder | ||
| for _, r := range slugName { | ||
| if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' { | ||
| cleanSlug.WriteRune(r) | ||
| } | ||
| } | ||
| slugName = cleanSlug.String() | ||
|
|
||
| if slugName == "" { | ||
| slugName = "personal" | ||
| } | ||
| tenantSlug := fmt.Sprintf("personal-%s-%s", slugName, user.ID.String()[:8]) | ||
|
|
||
| _, err = s.tenantSvc.CreateTenant(ctx, tenantName, tenantSlug, user.ID) | ||
| if err != nil { | ||
| // TODO: Implement user rollback here if tenant creation fails. | ||
| // Currently UserRepository does not expose a Delete method. | ||
| return nil, fmt.Errorf("failed to create personal tenant: %w", err) | ||
| } |
There was a problem hiding this comment.
Orphan user risk if tenant creation fails.
The user is persisted before tenant creation, and if CreateTenant fails, the user remains in the database without a tenant. The TODO comment acknowledges this, but the current behavior could lead to data inconsistencies.
Consider either:
- Wrapping both operations in a database transaction.
- Implementing a compensating delete operation for the user.
- Creating the tenant first (if feasible with your domain constraints).
Do you want me to help design a transactional approach or a compensating action pattern for this flow?
🤖 Prompt for AI Agents
In `@internal/core/services/auth.go` around lines 81 - 114, The user is created
before the tenant so a failed s.tenantSvc.CreateTenant leaves an orphan user;
fix by making the two operations atomic: either (A) use a DB transaction around
s.userRepo.Create and s.tenantSvc.CreateTenant (use your repo/DB transaction
functions and commit only if both succeed, rolling back on error), or (B)
implement a compensating delete by adding and calling a removal method (e.g.,
userRepo.Delete(ctx, user.ID) or s.userRepo.DeleteUser/DeleteByID) when
s.tenantSvc.CreateTenant returns an error (ensure Delete exists and handle its
error/logging); update the code paths around s.userRepo.Create and
s.tenantSvc.CreateTenant accordingly.
| // Create Personal Tenant | ||
| // Create Personal Tenant |
There was a problem hiding this comment.
Remove duplicate comment.
The comment "Create Personal Tenant" is duplicated on consecutive lines.
🔧 Proposed fix
- // Create Personal Tenant
// Create Personal Tenant
tenantName := fmt.Sprintf("%s's Personal Tenant", name)📝 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.
| // Create Personal Tenant | |
| // Create Personal Tenant | |
| // Create Personal Tenant | |
| tenantName := fmt.Sprintf("%s's Personal Tenant", name) |
🤖 Prompt for AI Agents
In `@internal/core/services/auth.go` around lines 87 - 88, Remove the duplicated
comment line "Create Personal Tenant" so only a single instance of that comment
remains; locate the consecutive duplicate occurrences of the "Create Personal
Tenant" comment in internal core auth service code (the repeated comment above
the personal-tenant creation logic) and delete the extra line to leave one clear
comment.
| // Reload user to reflect changes made during tenant creation (e.g. DefaultTenantID) | ||
| updatedUser, err := s.userRepo.GetByID(ctx, user.ID) | ||
| if err == nil { | ||
| user = updatedUser | ||
| } |
There was a problem hiding this comment.
Silent error swallowing on user reload.
If GetByID fails, the error is silently ignored and the original (potentially stale) user object is returned. This could mask database connectivity issues or data inconsistencies.
Consider logging the error or returning it to the caller.
🔧 Proposed fix
// Reload user to reflect changes made during tenant creation (e.g. DefaultTenantID)
updatedUser, err := s.userRepo.GetByID(ctx, user.ID)
- if err == nil {
+ if err != nil {
+ // Log but don't fail - user was created successfully
+ // The caller will get the user without DefaultTenantID populated
+ _ = fmt.Errorf("failed to reload user after tenant creation: %w", err)
+ } else {
user = updatedUser
}Or alternatively, return the error if this reload is critical:
updatedUser, err := s.userRepo.GetByID(ctx, user.ID)
if err != nil {
- if err == nil {
- user = updatedUser
+ return nil, fmt.Errorf("failed to reload user after tenant creation: %w", err)
}
+ user = updatedUser📝 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.
| // Reload user to reflect changes made during tenant creation (e.g. DefaultTenantID) | |
| updatedUser, err := s.userRepo.GetByID(ctx, user.ID) | |
| if err == nil { | |
| user = updatedUser | |
| } | |
| // Reload user to reflect changes made during tenant creation (e.g. DefaultTenantID) | |
| updatedUser, err := s.userRepo.GetByID(ctx, user.ID) | |
| if err != nil { | |
| // Log but don't fail - user was created successfully | |
| // The caller will get the user without DefaultTenantID populated | |
| _ = fmt.Errorf("failed to reload user after tenant creation: %w", err) | |
| } else { | |
| user = updatedUser | |
| } |
🤖 Prompt for AI Agents
In `@internal/core/services/auth.go` around lines 116 - 120, The reload of the
user via s.userRepo.GetByID silently swallows errors which can mask real
failures; update the block around s.userRepo.GetByID(ctx, user.ID) so that when
err != nil you either log the error with context (including user.ID and
operation like "reload user after tenant creation") using the existing logger or
propagate the error to the caller instead of ignoring it; if you choose to
propagate, return the err (or wrap it) from the surrounding function, otherwise
call the service logger (e.g., s.logger or processLogger) with a clear message
and keep the original user only after successful updatedUser assignment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| userID, ok := val.(uuid.UUID) | ||
| if !ok { | ||
| return uuid.Nil | ||
| } | ||
| return userID | ||
| } |
There was a problem hiding this comment.
GetTenantID assigns the context value to a variable named userID and returns it, which is misleading and makes it easy to accidentally introduce bugs later. Rename the local variable/return variable to tenantID (and keep types consistent) to reflect what is actually being read from the Gin context.
| } | ||
|
|
||
| // Check if already a member | ||
| membership, _ := s.repo.GetMembership(ctx, tenantID, user.ID) |
There was a problem hiding this comment.
InviteMember ignores the error returned by repo.GetMembership. If the repo errors (e.g., DB outage), this code will treat it as “not a member” and proceed to add a member, which can hide operational issues and lead to inconsistent state. Propagate/handle the repo error instead of discarding it.
| membership, _ := s.repo.GetMembership(ctx, tenantID, user.ID) | |
| membership, err := s.repo.GetMembership(ctx, tenantID, user.ID) | |
| if err != nil { | |
| return err | |
| } |
| (SELECT COUNT(*) FROM instances WHERE tenant_id = tq.tenant_id AND status != 'DELETED') as used_instances, | ||
| (SELECT COUNT(*) FROM vpcs WHERE tenant_id = tq.tenant_id) as used_vpcs, | ||
| (SELECT COALESCE(SUM(size_gb), 0) FROM volumes WHERE tenant_id = tq.tenant_id AND status != 'deleted') as used_storage_gb, | ||
| 0 as used_memory_gb, -- Placeholder as instance sizing is not yet normalized | ||
| 0 as used_vcpus -- Placeholder |
There was a problem hiding this comment.
GetQuota filters volumes with status != 'deleted', but domain.VolumeStatus values are uppercase (e.g., AVAILABLE, IN-USE, DELETING). This condition will always be true and will over-count used_storage_gb. Use the correct status value(s) that represent deletion for volumes (or drop the filter if rows are hard-deleted).
| // Unknown resource (should pass or fail depending on logic, implementation returns nil currently) | ||
| assert.NoError(t, svc.CheckQuota(ctx, tenantID, "unknown", 1)) |
There was a problem hiding this comment.
This test expects CheckQuota(..., "unknown", ...) to succeed, but TenantService.CheckQuota currently returns an InvalidInput error for unknown resource types. Either update the test to expect an error, or change the service behavior to treat unknown resources as no-op (whichever matches the intended API contract).
| // Unknown resource (should pass or fail depending on logic, implementation returns nil currently) | |
| assert.NoError(t, svc.CheckQuota(ctx, tenantID, "unknown", 1)) | |
| // Unknown resource should be treated as invalid input and return an error | |
| err = svc.CheckQuota(ctx, tenantID, "unknown", 1) | |
| assert.Error(t, err) |
| // Create godoc | ||
| // @Summary Create a new tenant | ||
| // @Tags Tenant | ||
| // @Security APIKeyAuth | ||
| // @Router /tenants [post] | ||
| func (h *TenantHandler) Create(c *gin.Context) { | ||
| var req CreateTenantRequest |
There was a problem hiding this comment.
The Swagger annotations for these tenant endpoints omit @Param/@Success/@Failure details (and path params), which results in generated Swagger paths with empty responses blocks. Add the missing annotations so the generated OpenAPI spec includes request/response schemas and status codes.
| ALTER TABLE snapshots DROP COLUMN IF EXISTS tenant_id; | ||
| ALTER TABLE subnets DROP COLUMN IF EXISTS tenant_id; | ||
| ALTER TABLE security_groups DROP COLUMN IF EXISTS tenant_id; | ||
| ALTER TABLE floating_ips DROP COLUMN IF EXISTS tenant_id; |
There was a problem hiding this comment.
This down migration runs ALTER TABLE floating_ips ... without checking that the floating_ips table exists. Since the corresponding up migration does not add tenant_id to floating_ips (and the repo doesn’t have a migration creating that table), rolling back can fail on environments without this table. Wrap it in an existence check (similar to the up migration) or remove these statements if floating_ips isn’t part of the schema.
| ALTER TABLE floating_ips DROP COLUMN IF EXISTS tenant_id; | |
| DO $$ | |
| BEGIN | |
| IF EXISTS ( | |
| SELECT 1 | |
| FROM information_schema.tables | |
| WHERE table_schema = 'public' | |
| AND table_name = 'floating_ips' | |
| ) THEN | |
| EXECUTE 'ALTER TABLE floating_ips DROP COLUMN IF EXISTS tenant_id'; | |
| END IF; | |
| END $$; |
| func (r *VpcRepository) Create(ctx context.Context, vpc *domain.VPC) error { | ||
| query := ` | ||
| INSERT INTO vpcs (id, user_id, name, cidr_block, network_id, vxlan_id, status, arn, created_at) | ||
| VALUES ($1, $2, $3, NULLIF($4, '')::cidr, $5, $6, $7, $8, $9) | ||
| INSERT INTO vpcs (id, user_id, tenant_id, name, cidr_block, network_id, vxlan_id, status, arn, created_at) | ||
| VALUES ($1, $2, $3, $4, NULLIF($5, '')::cidr, $6, $7, $8, $9, $10) | ||
| ` | ||
| _, err := r.db.Exec(ctx, query, vpc.ID, vpc.UserID, vpc.Name, vpc.CIDRBlock, vpc.NetworkID, vpc.VXLANID, vpc.Status, vpc.ARN, vpc.CreatedAt) | ||
| _, err := r.db.Exec(ctx, query, vpc.ID, vpc.UserID, vpc.TenantID, vpc.Name, vpc.CIDRBlock, vpc.NetworkID, vpc.VXLANID, vpc.Status, vpc.ARN, vpc.CreatedAt) |
There was a problem hiding this comment.
Create inserts tenant_id using vpc.TenantID. However, the current VpcService.CreateVPC does not populate TenantID (it only sets UserID), so this will typically insert uuid.Nil and fail the FK to tenants(id) or create an unscoped record. Prefer deriving tenant_id from appcontext.TenantIDFromContext(ctx) (and error if missing) or ensure the service always sets vpc.TenantID before calling the repo.
| func (r *InstanceRepository) Create(ctx context.Context, inst *domain.Instance) error { | ||
| query := ` | ||
| INSERT INTO instances (id, user_id, name, image, container_id, status, ports, vpc_id, subnet_id, private_ip, ovs_port, version, created_at, updated_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, NULLIF($10, '')::inet, $11, $12, $13, $14) | ||
| INSERT INTO instances (id, user_id, tenant_id, name, image, container_id, status, ports, vpc_id, subnet_id, private_ip, ovs_port, version, created_at, updated_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, NULLIF($11, '')::inet, $12, $13, $14, $15) | ||
| ` | ||
| _, err := r.db.Exec(ctx, query, | ||
| inst.ID, inst.UserID, inst.Name, inst.Image, inst.ContainerID, string(inst.Status), inst.Ports, inst.VpcID, inst.SubnetID, | ||
| inst.ID, inst.UserID, inst.TenantID, inst.Name, inst.Image, inst.ContainerID, string(inst.Status), inst.Ports, inst.VpcID, inst.SubnetID, | ||
| inst.PrivateIP, inst.OvsPort, inst.Version, inst.CreatedAt, inst.UpdatedAt, | ||
| ) |
There was a problem hiding this comment.
Create inserts tenant_id using inst.TenantID, but InstanceService.LaunchInstance currently never sets TenantID on the domain instance. This will usually pass uuid.Nil and break FK constraints / tenant scoping. Use appcontext.TenantIDFromContext(ctx) for the insert (and reject missing tenant), or set inst.TenantID in the service before calling the repo.
| func (r *SecurityGroupRepository) Create(ctx context.Context, sg *domain.SecurityGroup) error { | ||
| query := ` | ||
| INSERT INTO security_groups (id, user_id, vpc_id, name, description, arn, created_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7) | ||
| INSERT INTO security_groups (id, user_id, tenant_id, vpc_id, name, description, arn, created_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8) | ||
| ` | ||
| _, err := r.db.Exec(ctx, query, sg.ID, sg.UserID, sg.VPCID, sg.Name, sg.Description, sg.ARN, sg.CreatedAt) | ||
| _, err := r.db.Exec(ctx, query, sg.ID, sg.UserID, sg.TenantID, sg.VPCID, sg.Name, sg.Description, sg.ARN, sg.CreatedAt) | ||
| if err != nil { |
There was a problem hiding this comment.
Create inserts tenant_id using sg.TenantID, but SecurityGroupService.CreateGroup currently does not set TenantID on the domain object. This will typically insert uuid.Nil, violating the tenant FK and/or creating unusable records. Consider reading the tenant ID from appcontext.TenantIDFromContext(ctx) (and failing fast if missing) or ensure the service always populates sg.TenantID.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/repositories/postgres/instance_repo_test.go (3)
181-188: List test mock is missingtenant_idcolumn in the row schema.The mock on line 183 defines only 14 columns without
tenant_id, but the actualListquery (line 84 of instance_repo.go) selects all 15 columns includingtenant_id, andscanInstance(line 57 of instance_repo.go) scans them in order. All other tests (GetByID, GetByName, ListBySubnet) correctly includetenant_idas the third column. This mismatch will cause a scan error when the test runs.Expected fix
WillReturnRows(pgxmock.NewRows([]string{"id", "user_id", "tenant_id", "name", "image", "container_id", "status", "ports", "vpc_id", "subnet_id", "private_ip", "ovs_port", "version", "created_at", "updated_at"}). AddRow(uuid.New(), userID, uuid.New(), testInstanceName, testInstanceImg, "cid-1", string(domain.StatusRunning), "80:80", nil, nil, testutil.TestIPHost, "ovs-1", 1, now, now))Add
tenant_idto the column list and auuid.New()to the row values.
32-50: AddTenantIDto the instance struct initialization and expected INSERT args in the Create test.The domain.Instance struct includes a
TenantIDfield (internal/core/domain/instance.go:63), and the repository's Create method (internal/repositories/postgres/instance_repo.go:29-33) includestenant_idin the INSERT statement and passesinst.TenantIDin the query arguments. However, the test fixture doesn't setTenantIDon the instance struct or include it in the expected INSERT args, which means the test won't validate tenant isolation on create or catch breaking changes to tenant handling.
130-143: GetByName test needs tenantID in context and mock expectations.The test sets only
userIDin context (line 130) butGetByNameextractstenantIDviaappcontext.TenantIDFromContext(ctx)and queries withWHERE name = $1 AND tenant_id = $2. The mock expectsWithArgs(name, userID)but the actual call will beWithArgs(name, tenantID). Update the test to:
- Set tenantID in context using
appcontext.WithTenantID()- Update the mock to expect the correct tenantID parameter
🤖 Fix all issues with AI agents
In `@internal/core/services/auth.go`:
- Around line 109-113: The error returned from s.tenantSvc.CreateTenant should
be converted to the service-layer typed internal error instead of returning a
plain fmt.Errorf; update the failure branch in the function containing
s.tenantSvc.CreateTenant to wrap the underlying err using the internal/errors
package (e.g., internal/errors.Wrap or internal/errors.NewInternalError with the
message "failed to create personal tenant") and return that typed error so error
mapping remains consistent.
In `@pkg/httputil/auth.go`:
- Around line 115-118: The current handler masks any error from
tenantSvc.GetMembership(ctx, tenantID, userID) as a Forbidden error; change the
logic in that call site so that if GetMembership returns an error you
wrap/return it as an Internal error (preserving the original error as the cause)
rather than errors.Forbidden, while still returning Forbidden only when the
membership lookup succeeds but indicates no membership; update the return to
include the original err (e.g., wrap with errors.New(errors.Internal, "failed to
verify tenant membership: %w") or equivalent) and keep the member/non-member
check using the member variable from GetMembership.
♻️ Duplicate comments (9)
internal/repositories/postgres/instance_repo_test.go (1)
82-94: Context and mock args still incorrect for tenant-scoped GetByID query.The context only sets
userID(line 82), but per the previous review,GetByIDextractstenantIDfrom context and queries withWHERE id = $1 AND tenant_id = $2. The mock expectation at line 86 still usesWithArgs(id, userID)instead ofWithArgs(id, tenantID).Suggested fix
id := uuid.New() userID := uuid.New() - ctx := appcontext.WithUserID(context.Background(), userID) + tenantID := uuid.New() + ctx := appcontext.WithTenantID(appcontext.WithUserID(context.Background(), userID), tenantID) now := time.Now() mock.ExpectQuery(selectQuery). - WithArgs(id, userID). + WithArgs(id, tenantID). WillReturnRows(pgxmock.NewRows([]string{"id", "user_id", "tenant_id", "name", "image", "container_id", "status", "ports", "vpc_id", "subnet_id", "private_ip", "ovs_port", "version", "created_at", "updated_at"}). - AddRow(id, userID, uuid.New(), testInstanceName, testInstanceImg, "cid-1", string(domain.StatusRunning), "80:80", nil, nil, testutil.TestIPHost, "ovs-1", 1, now, now)) + AddRow(id, userID, tenantID, testInstanceName, testInstanceImg, "cid-1", string(domain.StatusRunning), "80:80", nil, nil, testutil.TestIPHost, "ovs-1", 1, now, now))internal/core/services/tenant.go (3)
28-33: Error fromGetBySlugis silently ignored, risking duplicate creation on DB errors.If the repository returns a real database error (not "not found"), this code incorrectly proceeds to create the tenant. This could mask operational failures or allow duplicate slugs if the check fails due to a transient DB issue.
As per coding guidelines, do not use silent failures.
🐛 Proposed fix
func (s *TenantService) CreateTenant(ctx context.Context, name, slug string, ownerID uuid.UUID) (*domain.Tenant, error) { // 1. Check if slug exists - existing, _ := s.repo.GetBySlug(ctx, slug) + existing, err := s.repo.GetBySlug(ctx, slug) + if err != nil { + // Check if it's a "not found" error (slug available) vs actual DB error + var domainErr errors.Error + if !stderrors.As(err, &domainErr) || domainErr.Type != errors.NotFound { + return nil, errors.Wrap(errors.Internal, "failed to check slug availability", err) + } + } if existing != nil { return nil, errors.New(errors.Conflict, "tenant slug already exists") }Note: You'll need to import
stderrors "errors"and use your error type checking pattern.
65-68: Potential nil pointer dereference if logger is nil.If
NewTenantServiceis called with a nil logger (e.g., in tests), callings.logger.Error()will panic. Consider guarding the log call or defaulting toslog.Default()in the constructor.🐛 Proposed fix in constructor
func NewTenantService(repo ports.TenantRepository, userRepo ports.UserRepository, logger *slog.Logger) *TenantService { + if logger == nil { + logger = slog.Default() + } return &TenantService{ repo: repo, userRepo: userRepo, logger: logger, } }
96-100: Error fromGetMembershipis silently ignored, masking DB failures.If the repository returns a database error, this code treats it as "not a member" and proceeds with
AddMember. This could mask operational issues and lead to inconsistent state.As per coding guidelines, do not use silent failures.
🐛 Proposed fix
// Check if already a member - membership, _ := s.repo.GetMembership(ctx, tenantID, user.ID) + membership, err := s.repo.GetMembership(ctx, tenantID, user.ID) + if err != nil { + return errors.Wrap(errors.Internal, "failed to check membership", err) + } if membership != nil { return errors.New(errors.Conflict, "user is already a member of this tenant") }internal/repositories/postgres/tenant_repo.go (1)
162-200: Volume status filter uses incorrect case, causing over-counting.Line 173 filters volumes with
status != 'deleted'(lowercase), butdomain.VolumeStatusconstants are uppercase (e.g.,DELETED,AVAILABLE). This condition will always be true, including deleted volumes in the storage count.Note the inconsistency: line 171 correctly uses
status != 'DELETED'for instances.🐛 Proposed fix
(SELECT COUNT(*) FROM instances WHERE tenant_id = tq.tenant_id AND status != 'DELETED') as used_instances, (SELECT COUNT(*) FROM vpcs WHERE tenant_id = tq.tenant_id) as used_vpcs, - (SELECT COALESCE(SUM(size_gb), 0) FROM volumes WHERE tenant_id = tq.tenant_id AND status != 'deleted') as used_storage_gb, + (SELECT COALESCE(SUM(size_gb), 0) FROM volumes WHERE tenant_id = tq.tenant_id AND status != 'DELETED') as used_storage_gb,pkg/httputil/auth.go (1)
54-65: Rename local variable and fix docstring to match Gin context source.This still reads from Gin context and should use
tenantIDnaming for clarity.✏️ Suggested fix
-// GetTenantID returns the active tenant ID from the request context. +// GetTenantID returns the active tenant ID from the Gin context. func GetTenantID(c *gin.Context) uuid.UUID { val, exists := c.Get("tenantID") if !exists { return uuid.Nil } - userID, ok := val.(uuid.UUID) + tenantID, ok := val.(uuid.UUID) if !ok { return uuid.Nil } - return userID + return tenantID }internal/core/services/auth.go (3)
81-114: Orphan user risk if tenant creation fails.User is persisted before tenant creation; a failure leaves an orphaned user. Please make the two operations atomic or add compensating deletion.
87-88: Remove the duplicate comment.🧹 Suggested fix
- // Create Personal Tenant // Create Personal Tenant
116-120: Don’t silently swallow user reload errors.Either log the error or return it to the caller to avoid hiding DB failures.
🛠️ Example handling
updatedUser, err := s.userRepo.GetByID(ctx, user.ID) - if err == nil { - user = updatedUser - } + if err != nil { + return nil, fmt.Errorf("failed to reload user after tenant creation: %w", err) + } + user = updatedUser
🧹 Nitpick comments (6)
tests/edge_cases_test.go (1)
42-60: Consider using named constants andstrings.Repeatfor boundary tests.The magic numbers
64and65should be named constants per coding guidelines. Additionally,strings.Repeatis clearer and more efficient than loop concatenation.♻️ Suggested refactor
Define a constant (ideally imported from the validation package or a shared test constants file):
const MaxInstanceNameLength = 64Then refactor the test:
t.Run("Boundary Values", func(t *testing.T) { - // 1. Instance name exactly 64 chars (max allowed) - longName := "" - for i := 0; i < 64; i++ { - longName += "a" - } + // 1. Instance name exactly at max allowed length + longName := strings.Repeat("a", MaxInstanceNameLength) payload := map[string]string{ "name": longName, "image": "alpine", } resp := postRequest(t, client, testutil.TestBaseURL+testutil.TestRouteInstances, token, payload) defer resp.Body.Close() assert.Contains(t, []int{http.StatusCreated, http.StatusAccepted}, resp.StatusCode) - // 2. Instance name 65 chars (overflow) - tooLongName := longName + "a" + // 2. Instance name exceeds max (overflow) + tooLongName := strings.Repeat("a", MaxInstanceNameLength+1) payload["name"] = tooLongName resp = postRequest(t, client, testutil.TestBaseURL+testutil.TestRouteInstances, token, payload) defer resp.Body.Close() helpers.AssertErrorCode(t, resp, http.StatusBadRequest) })Based on learnings, use named constants instead of magic numbers.
pkg/httputil/httputil_test.go (1)
52-80: Consider using consistent mock patterns across all methods.Most methods in
mockTenantServiceare hard-coded stubs returningnil, whileGetMembershipproperly usesmock.Called. This inconsistency may cause issues when tests need to verify tenant service interactions or return different values.Based on learnings, use
testify/mockfor creating mock objects in tests consistently.♻️ Suggested refactor for consistency
func (m *mockTenantService) CreateTenant(ctx context.Context, name, slug string, ownerID uuid.UUID) (*domain.Tenant, error) { - return nil, nil + args := m.Called(ctx, name, slug, ownerID) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*domain.Tenant), args.Error(1) } func (m *mockTenantService) GetTenant(ctx context.Context, id uuid.UUID) (*domain.Tenant, error) { - return nil, nil + args := m.Called(ctx, id) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*domain.Tenant), args.Error(1) }Apply similar changes to
InviteMember,RemoveMember,SwitchTenant, andCheckQuota.internal/repositories/postgres/volume_repo.go (1)
35-45: Consider validating tenant context in read operations for consistency.Read operations derive
tenantIDfrom context but don't validate it. If the context lacks a tenant ID (uuid.Nil), queries will silently return no results rather than failing explicitly. While this is a safe fallback, explicit validation would provide clearer error messages and consistency withCreate/Update.♻️ Optional: Add validation to reads
func (r *VolumeRepository) GetByID(ctx context.Context, id uuid.UUID) (*domain.Volume, error) { tenantID := appcontext.TenantIDFromContext(ctx) + if tenantID == uuid.Nil { + return nil, errors.New(errors.Unauthorized, "tenant ID required in context") + } query := `SELECT id, user_id, tenant_id, name, size_gb, status, instance_id, backend_path, mount_path, created_at, updated_at FROM volumes WHERE id = $1 AND tenant_id = $2` return r.scanVolume(r.db.QueryRow(ctx, query, id, tenantID)) }internal/repositories/postgres/tenant_repo.go (1)
81-87: Delete doesn't verify the tenant existed.Unlike
VolumeRepository.Delete, this method doesn't checkRowsAffected()to return aNotFounderror when the tenant doesn't exist. This may be intentional (idempotent deletes), but it's inconsistent with other delete implementations.♻️ Optional: Add existence check
func (r *TenantRepo) Delete(ctx context.Context, id uuid.UUID) error { - _, err := r.db.Exec(ctx, "DELETE FROM tenants WHERE id = $1", id) + cmd, err := r.db.Exec(ctx, "DELETE FROM tenants WHERE id = $1", id) if err != nil { return errors.Wrap(errors.Internal, "failed to delete tenant", err) } + if cmd.RowsAffected() == 0 { + return errors.New(errors.NotFound, "tenant not found") + } return nil }internal/core/services/auth.go (2)
38-45: Use a params struct for the constructor.The constructor now has 4 dependencies; a params struct keeps call sites stable and readable. As per coding guidelines, use a params struct for 3+ deps.
♻️ Example shape
+type AuthServiceParams struct { + UserRepo ports.UserRepository + APIKeySvc ports.IdentityService + AuditSvc ports.AuditService + TenantSvc ports.TenantService +} + -func NewAuthService(userRepo ports.UserRepository, apiKeySvc ports.IdentityService, auditSvc ports.AuditService, tenantSvc ports.TenantService) *AuthService { +func NewAuthService(p AuthServiceParams) *AuthService { return &AuthService{ - userRepo: userRepo, - apiKeySvc: apiKeySvc, - auditSvc: auditSvc, - tenantSvc: tenantSvc, + userRepo: p.UserRepo, + apiKeySvc: p.APIKeySvc, + auditSvc: p.AuditSvc, + tenantSvc: p.TenantSvc, failedAttempts: make(map[string]int), lockouts: make(map[string]time.Time), } }
107-107: Replace magic slug length with a named constant.As per coding guidelines, avoid magic numbers—define a const for the suffix length.
🔧 Example fix
const ( maxFailedAttempts = 5 lockoutDuration = 15 * time.Minute + tenantSlugSuffixLen = 8 ) ... - tenantSlug := fmt.Sprintf("personal-%s-%s", slugName, user.ID.String()[:8]) + tenantSlug := fmt.Sprintf("personal-%s-%s", slugName, user.ID.String()[:tenantSlugSuffixLen])
| _, err = s.tenantSvc.CreateTenant(ctx, tenantName, tenantSlug, user.ID) | ||
| if err != nil { | ||
| // TODO: Implement user rollback here if tenant creation fails. | ||
| // Currently UserRepository does not expose a Delete method. | ||
| return nil, fmt.Errorf("failed to create personal tenant: %w", err) |
There was a problem hiding this comment.
Use typed internal errors for tenant creation failures.
This is a service-layer error and should be wrapped with the internal error taxonomy to keep error mapping consistent. As per coding guidelines, use internal/errors.
🩹 Suggested fix
- return nil, fmt.Errorf("failed to create personal tenant: %w", err)
+ return nil, errors.Wrap(errors.Internal, "failed to create personal tenant", err)🤖 Prompt for AI Agents
In `@internal/core/services/auth.go` around lines 109 - 113, The error returned
from s.tenantSvc.CreateTenant should be converted to the service-layer typed
internal error instead of returning a plain fmt.Errorf; update the failure
branch in the function containing s.tenantSvc.CreateTenant to wrap the
underlying err using the internal/errors package (e.g., internal/errors.Wrap or
internal/errors.NewInternalError with the message "failed to create personal
tenant") and return that typed error so error mapping remains consistent.
| member, err := tenantSvc.GetMembership(ctx, tenantID, userID) | ||
| if err != nil { | ||
| return uuid.Nil, errors.New(errors.Forbidden, "failed to verify tenant membership") | ||
| } |
There was a problem hiding this comment.
Don’t mask membership lookup failures as Forbidden.
If GetMembership fails due to a backend issue, returning 403 misrepresents the error and complicates debugging. Prefer an Internal error and preserve the cause.
🩹 Suggested fix
- if err != nil {
- return uuid.Nil, errors.New(errors.Forbidden, "failed to verify tenant membership")
- }
+ if err != nil {
+ return uuid.Nil, errors.Wrap(errors.Internal, "failed to verify tenant membership", err)
+ }📝 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.
| member, err := tenantSvc.GetMembership(ctx, tenantID, userID) | |
| if err != nil { | |
| return uuid.Nil, errors.New(errors.Forbidden, "failed to verify tenant membership") | |
| } | |
| member, err := tenantSvc.GetMembership(ctx, tenantID, userID) | |
| if err != nil { | |
| return uuid.Nil, errors.Wrap(errors.Internal, "failed to verify tenant membership", err) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/httputil/auth.go` around lines 115 - 118, The current handler masks any
error from tenantSvc.GetMembership(ctx, tenantID, userID) as a Forbidden error;
change the logic in that call site so that if GetMembership returns an error you
wrap/return it as an Internal error (preserving the original error as the cause)
rather than errors.Forbidden, while still returning Forbidden only when the
membership lookup succeeds but indicates no membership; update the return to
include the original err (e.g., wrap with errors.New(errors.Internal, "failed to
verify tenant membership: %w") or equivalent) and keep the member/non-member
check using the member variable from GetMembership.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/util/password_test.go`:
- Around line 17-19: Add back a basic randomness assertion to the test: call
GenerateRandomPassword a second time (in the same test in
pkg/util/password_test.go), verify the two returned passwords are not identical
(use the existing test assertion helper like assert.NotEqual or assert.False
with equality), and keep the existing per-character check against passwordChars;
reference GenerateRandomPassword and the passwordChars validation in the same
test so deterministic implementations will fail the test.
🧹 Nitpick comments (1)
pkg/util/password_test.go (1)
10-26: Consider consolidating into a table-driven test.As per coding guidelines, table-driven tests are preferred. The two test functions could be merged into a single table-driven test for better maintainability and easier extension with additional cases.
Example refactor
func TestGenerateRandomPassword(t *testing.T) { tests := []struct { name string length int wantLen int wantErr bool }{ {"standard length", 16, 16, false}, {"zero length", 0, 0, false}, {"negative length", -1, 0, false}, // or expect error depending on impl } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { password, err := GenerateRandomPassword(tt.length) if tt.wantErr { assert.Error(t, err) return } assert.NoError(t, err) assert.Len(t, password, tt.wantLen) for _, ch := range password { assert.True(t, strings.ContainsRune(passwordChars, ch)) } }) } }Based on learnings and coding guidelines, table-driven tests are preferred for test files.
| for _, ch := range password { | ||
| assert.True(t, strings.ContainsRune(passwordChars, ch), "password contains invalid character") | ||
| } |
There was a problem hiding this comment.
Consider restoring the randomness assertion.
The character validation is a good addition, but based on the summary, this change removed the previous randomness check (generating a second password and comparing). Without a randomness assertion, the test no longer verifies that GenerateRandomPassword produces different outputs across calls—a deterministic implementation would pass.
Consider adding back a basic randomness check:
Suggested addition
for _, ch := range password {
assert.True(t, strings.ContainsRune(passwordChars, ch), "password contains invalid character")
}
+
+ // Verify randomness: a second call should produce a different password
+ password2, err := GenerateRandomPassword(length)
+ assert.NoError(t, err)
+ assert.NotEqual(t, password, password2, "expected different passwords on subsequent calls")
}📝 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.
| for _, ch := range password { | |
| assert.True(t, strings.ContainsRune(passwordChars, ch), "password contains invalid character") | |
| } | |
| for _, ch := range password { | |
| assert.True(t, strings.ContainsRune(passwordChars, ch), "password contains invalid character") | |
| } | |
| // Verify randomness: a second call should produce a different password | |
| password2, err := GenerateRandomPassword(length) | |
| assert.NoError(t, err) | |
| assert.NotEqual(t, password, password2, "expected different passwords on subsequent calls") | |
| } |
🤖 Prompt for AI Agents
In `@pkg/util/password_test.go` around lines 17 - 19, Add back a basic randomness
assertion to the test: call GenerateRandomPassword a second time (in the same
test in pkg/util/password_test.go), verify the two returned passwords are not
identical (use the existing test assertion helper like assert.NotEqual or
assert.False with equality), and keep the existing per-character check against
passwordChars; reference GenerateRandomPassword and the passwordChars validation
in the same test so deterministic implementations will fail the test.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| userID, ok := val.(uuid.UUID) | ||
| if !ok { | ||
| return uuid.Nil | ||
| } | ||
| return userID |
There was a problem hiding this comment.
GetTenantID casts the stored value into a variable named userID and returns it. The logic works, but the variable name is misleading and makes future changes error-prone; rename it to tenantID for clarity.
| if err := h.svc.InviteMember(c.Request.Context(), tenantID, req.Email, req.Role); err != nil { | ||
| httputil.Error(c, err) |
There was a problem hiding this comment.
InviteMember calls TenantService.InviteMember for the tenant ID from the URL without verifying that the authenticated user is authorized for that tenant (or that it matches the active tenant in context). Add an explicit authorization check (at minimum: ensure active tenant == :id and user is a member; ideally also enforce role like owner/admin) before invoking the service.
| } | ||
|
|
||
| if current+requested > limit { | ||
| return errors.New(errors.QuotaExceeded, "quota exceeded for "+resource) |
There was a problem hiding this comment.
This returns errors.QuotaExceeded, but pkg/httputil.Error does not currently map QUOTA_EXCEEDED to an HTTP status, so clients will receive a 500. Add a mapping for errors.QuotaExceeded (typically 429 or 403) in the HTTP error mapping to avoid incorrect 500 responses.
| return errors.New(errors.QuotaExceeded, "quota exceeded for "+resource) | |
| return errors.New(errors.Forbidden, "quota exceeded for "+resource) |
| quota := &domain.TenantQuota{ | ||
| TenantID: tenant.ID, | ||
| MaxInstances: 10, | ||
| MaxVPCs: 2, | ||
| MaxStorageGB: 50, |
There was a problem hiding this comment.
Default quota values here differ from the defaults established in migrations (e.g., 070_create_tenants.up.sql / 072_migrate_to_tenants.up.sql). This can lead to surprising behavior depending on whether quotas are created by migration or by service. Consider aligning these defaults or documenting why they intentionally differ.
| if err != nil { | ||
| // TODO: Implement user rollback here if tenant creation fails. | ||
| // Currently UserRepository does not expose a Delete method. | ||
| return nil, fmt.Errorf("failed to create personal tenant: %w", err) |
There was a problem hiding this comment.
If personal-tenant creation fails, this returns a plain fmt.Errorf(...) which will be treated as an unknown error by httputil.Error (generic 500) and loses the domain error type. Prefer returning a typed internal error (e.g., wrap with your internal/errors type) so it maps consistently and can be surfaced/handled predictably.
| return nil, fmt.Errorf("failed to create personal tenant: %w", err) | |
| return nil, errors.New(errors.Internal, "failed to create personal tenant: "+err.Error()) |
| DROP INDEX IF EXISTS idx_security_groups_tenant; | ||
| DROP INDEX IF EXISTS idx_floating_ips_tenant; | ||
| DROP INDEX IF EXISTS idx_clusters_tenant; | ||
| DROP INDEX IF EXISTS idx_buckets_tenant; | ||
| DROP INDEX IF EXISTS idx_scaling_groups_tenant; |
There was a problem hiding this comment.
The down migration drops idx_floating_ips_tenant and floating_ips.tenant_id, but the corresponding up migration doesn’t add tenant_id to floating_ips or create that index. Consider removing these lines or adding the missing up-migration steps so the migration pair is symmetric and easier to reason about.
| // Unknown resource (should pass or fail depending on logic, implementation returns nil currently) | ||
| assert.NoError(t, svc.CheckQuota(ctx, tenantID, "unknown", 1)) |
There was a problem hiding this comment.
The service implementation returns an InvalidInput error for unknown resources, but this test asserts NoError. Update the expectation to match the current behavior (and optionally assert the error type/message).
| // Unknown resource (should pass or fail depending on logic, implementation returns nil currently) | |
| assert.NoError(t, svc.CheckQuota(ctx, tenantID, "unknown", 1)) | |
| // Unknown resource should result in an error (invalid input) | |
| err = svc.CheckQuota(ctx, tenantID, "unknown", 1) | |
| assert.Error(t, err) |
| mock.ExpectQuery(selectQuery). | ||
| WithArgs(name, userID). | ||
| WillReturnRows(pgxmock.NewRows([]string{"id", "user_id", "name", "image", "container_id", "status", "ports", "vpc_id", "subnet_id", "private_ip", "ovs_port", "version", "created_at", "updated_at"}). | ||
| AddRow(id, userID, name, testInstanceImg, "cid-1", string(domain.StatusRunning), "80:80", nil, nil, testutil.TestIPHost, "ovs-1", 1, now, now)) | ||
| WillReturnRows(pgxmock.NewRows([]string{"id", "user_id", "tenant_id", "name", "image", "container_id", "status", "ports", "vpc_id", "subnet_id", "private_ip", "ovs_port", "version", "created_at", "updated_at"}). | ||
| AddRow(id, userID, uuid.New(), name, testInstanceImg, "cid-1", string(domain.StatusRunning), "80:80", nil, nil, testutil.TestIPHost, "ovs-1", 1, now, now)) |
There was a problem hiding this comment.
InstanceRepository.GetByName is now tenant-scoped, but this test still expects the second query arg to be userID. Update it to provide a tenant ID in context and assert WithArgs(name, tenantID) to match the repository query.
| Forbidden Type = "FORBIDDEN" | ||
| PermissionDenied Type = "PERMISSION_DENIED" | ||
| ResourceLimitExceeded Type = "RESOURCE_LIMIT_EXCEEDED" | ||
| QuotaExceeded Type = "QUOTA_EXCEEDED" | ||
| NotImplemented Type = "NOT_IMPLEMENTED" |
There was a problem hiding this comment.
New error types (PERMISSION_DENIED, QUOTA_EXCEEDED) are introduced here, but the HTTP error mapping in pkg/httputil.Error needs to be updated accordingly; otherwise these errors will fall back to 500 responses. Add explicit status mappings for the new types (and tests) to keep API behavior consistent.
| // GetByID retrieves a single VPC by its UUID and ensures it belongs to the authenticated user. | ||
| func (r *VpcRepository) GetByID(ctx context.Context, id uuid.UUID) (*domain.VPC, error) { | ||
| userID := appcontext.UserIDFromContext(ctx) | ||
| query := `SELECT id, user_id, name, COALESCE(cidr_block::text, ''), network_id, vxlan_id, status, arn, created_at FROM vpcs WHERE id = $1 AND user_id = $2` | ||
| return r.scanVPC(r.db.QueryRow(ctx, query, id, userID)) | ||
| tenantID := appcontext.TenantIDFromContext(ctx) | ||
| query := `SELECT id, user_id, tenant_id, name, COALESCE(cidr_block::text, ''), network_id, vxlan_id, status, arn, created_at FROM vpcs WHERE id = $1 AND tenant_id = $2` | ||
| return r.scanVPC(r.db.QueryRow(ctx, query, id, tenantID)) |
There was a problem hiding this comment.
The method comment says this ensures the VPC belongs to the authenticated user, but the implementation now scopes by tenant_id. Update the comment to reflect tenant-scoped access to avoid misleading future changes.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
internal/repositories/postgres/security_group_repo.go:239
ListInstanceGroupsdoes not apply any tenant scoping; anyone with aninstanceIDcould potentially enumerate security groups across tenants. This should be filtered by the tenant inctx(e.g., joininstancesand requireinstances.tenant_id = $2, or joinsecurity_groupsand filter bysg.tenant_id = $2) and pass the tenant ID as a parameter.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| sgGroup := r.Group("/security-groups") | ||
| sgGroup.Use(httputil.Auth(svcs.Identity)) | ||
| sgGroup.Use(httputil.Auth(svcs.Identity, svcs.Tenant)) |
There was a problem hiding this comment.
/security-groups routes are tenant-scoped at the repository layer now, but this route group only applies Auth and doesn’t enforce a tenant context (RequireTenant) or membership check (TenantMember) like /vpcs, /instances, /volumes, etc. Without these, handlers may run with tenant_id = uuid.Nil and fail unpredictably or return empty data. Consider adding RequireTenant() and TenantMember(...) here for consistency and clearer errors.
| sgGroup.Use(httputil.Auth(svcs.Identity, svcs.Tenant)) | |
| sgGroup.Use(httputil.Auth(svcs.Identity, svcs.Tenant), httputil.RequireTenant(), httputil.TenantMember(svcs.Tenant)) |
| // Transactionality would be better here, but avoiding for simplicity unless needed | ||
| err = s.userRepo.Create(ctx, user) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Registration now creates the user first and then creates a personal tenant. If tenant creation fails, the method returns an error but leaves the user persisted without a default tenant, which can create an inconsistent account state. Consider wrapping user+tenant creation in a DB transaction or adding a compensating delete/rollback path (and returning a typed internal/errors error instead of a plain fmt.Errorf so HTTP status codes remain accurate).
| _, err = s.tenantSvc.CreateTenant(ctx, tenantName, tenantSlug, user.ID) | ||
| if err != nil { | ||
| // TODO: Implement user rollback here if tenant creation fails. | ||
| // Currently UserRepository does not expose a Delete method. | ||
| return nil, fmt.Errorf("failed to create personal tenant: %w", err) | ||
| } |
There was a problem hiding this comment.
On tenant creation failure during registration, this returns a plain fmt.Errorf and doesn’t rollback the already-created user. That will surface as an INTERNAL error to API clients and can leave orphaned users. Prefer returning an internal/errors-typed error (e.g. Conflict/InvalidInput/Internal as appropriate) and adding a compensating rollback (or transaction) so registration is atomic.
| PermissionDenied Type = "PERMISSION_DENIED" | ||
| ResourceLimitExceeded Type = "RESOURCE_LIMIT_EXCEEDED" | ||
| QuotaExceeded Type = "QUOTA_EXCEEDED" |
There was a problem hiding this comment.
New error types (PermissionDenied, QuotaExceeded) are introduced here, but pkg/httputil/response.go’s statusCodeMap doesn’t include them. As a result, these errors will likely be returned as HTTP 500. Add explicit mappings (e.g. PermissionDenied -> 403, QuotaExceeded -> 429/403 depending on semantics) to keep API responses consistent.
| PermissionDenied Type = "PERMISSION_DENIED" | |
| ResourceLimitExceeded Type = "RESOURCE_LIMIT_EXCEEDED" | |
| QuotaExceeded Type = "QUOTA_EXCEEDED" | |
| PermissionDenied Type = Forbidden | |
| ResourceLimitExceeded Type = "RESOURCE_LIMIT_EXCEEDED" | |
| QuotaExceeded Type = ResourceLimitExceeded |
| ALTER TABLE images DROP COLUMN IF EXISTS tenant_id; | ||
| ALTER TABLE stacks DROP COLUMN IF EXISTS tenant_id; | ||
| ALTER TABLE deployments DROP COLUMN IF EXISTS tenant_id; | ||
| ALTER TABLE gateway_routes DROP COLUMN IF EXISTS tenant_id; | ||
| ALTER TABLE cron_jobs DROP COLUMN IF EXISTS tenant_id; | ||
| ALTER TABLE subscriptions DROP COLUMN IF EXISTS tenant_id; |
There was a problem hiding this comment.
This down migration drops tenant_id columns from many tables without ALTER TABLE IF EXISTS .... The corresponding up migration guards with table-existence checks, so rollback can fail in environments where some optional tables don’t exist. Consider mirroring the up migration’s existence checks (or using ALTER TABLE IF EXISTS ... DROP COLUMN IF EXISTS tenant_id consistently) for all tables here.
| func (r *VpcRepository) Create(ctx context.Context, vpc *domain.VPC) error { | ||
| query := ` | ||
| INSERT INTO vpcs (id, user_id, name, cidr_block, network_id, vxlan_id, status, arn, created_at) | ||
| VALUES ($1, $2, $3, NULLIF($4, '')::cidr, $5, $6, $7, $8, $9) | ||
| INSERT INTO vpcs (id, user_id, tenant_id, name, cidr_block, network_id, vxlan_id, status, arn, created_at) | ||
| VALUES ($1, $2, $3, $4, NULLIF($5, '')::cidr, $6, $7, $8, $9, $10) | ||
| ` | ||
| _, err := r.db.Exec(ctx, query, vpc.ID, vpc.UserID, vpc.Name, vpc.CIDRBlock, vpc.NetworkID, vpc.VXLANID, vpc.Status, vpc.ARN, vpc.CreatedAt) | ||
| _, err := r.db.Exec(ctx, query, vpc.ID, vpc.UserID, vpc.TenantID, vpc.Name, vpc.CIDRBlock, vpc.NetworkID, vpc.VXLANID, vpc.Status, vpc.ARN, vpc.CreatedAt) | ||
| if err != nil { |
There was a problem hiding this comment.
VpcRepository.Create takes tenant_id from vpc.TenantID, but all read/list/delete methods scope by TenantIDFromContext(ctx). Current services (e.g. VpcService.CreateVPC) don’t populate vpc.TenantID, so this will insert uuid.Nil and likely fail FK checks or associate the VPC with the wrong tenant. Prefer deriving tenant_id from context (and returning an Unauthorized/InvalidInput error if missing) to keep scoping consistent and prevent cross-tenant writes.
| func (r *SecurityGroupRepository) Create(ctx context.Context, sg *domain.SecurityGroup) error { | ||
| query := ` | ||
| INSERT INTO security_groups (id, user_id, vpc_id, name, description, arn, created_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7) | ||
| INSERT INTO security_groups (id, user_id, tenant_id, vpc_id, name, description, arn, created_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8) | ||
| ` | ||
| _, err := r.db.Exec(ctx, query, sg.ID, sg.UserID, sg.VPCID, sg.Name, sg.Description, sg.ARN, sg.CreatedAt) | ||
| _, err := r.db.Exec(ctx, query, sg.ID, sg.UserID, sg.TenantID, sg.VPCID, sg.Name, sg.Description, sg.ARN, sg.CreatedAt) | ||
| if err != nil { | ||
| return errors.Wrap(errors.Internal, "failed to create security group", err) |
There was a problem hiding this comment.
SecurityGroupRepository.Create uses sg.TenantID for tenant_id, but other methods scope by TenantIDFromContext(ctx). SecurityGroupService.CreateGroup doesn’t set TenantID, so creates will likely fail or write incorrect data. Consider deriving tenant_id from context (and erroring if missing) to keep tenant scoping consistent across CRUD operations.
| // @Summary Invite member to tenant | ||
| // @Tags Tenant | ||
| // @Security APIKeyAuth | ||
| // @Router /tenants/:id/members [post] |
There was a problem hiding this comment.
The Swagger @Router annotation uses Gin-style :id instead of OpenAPI path params ({id}), which results in invalid/incorrect paths in generated Swagger docs. Update this to /tenants/{id}/members [post] so clients and tooling resolve parameters correctly.
| // @Router /tenants/:id/members [post] | |
| // @Router /tenants/{id}/members [post] |
Changes
Coverage Improvements
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.