feat: implement security groups with full CLI, tracing, and auditing#20
feat: implement security groups with full CLI, tracing, and auditing#20
Conversation
- Created pkg/testutil/constants.go with centralized test constants - Replaced hardcoded passwords in auth_test.go with testutil.TestPasswordStrong - Updated benchmarks_test.go to use testutil constants - Removed local password constants in favor of shared ones
- Renamed test functions to CamelCase in auth/storage/subnet handlers - Added constants for repeated literals (paths, content-types) - Introduced storageBasePath and storageObjectPath in storage tests - Replaced hardcoded subnet paths with constants
- Renamed TestVpcHandler_* to TestVpcHandler* (CamelCase) - Renamed TestCacheHandler_Errors to TestCacheHandlerErrors - Modified GetInstanceLogs mock to have distinct implementation - Resolved linter warnings about identical code in mocks
- Replaced hardcoded IPs with testutil.TestIPLocalhost/TestIPPrivate - Replaced hardcoded user agent with testutil.TestUserAgent - Renamed test functions to CamelCase (TestSimpleAuditLogger*, BenchmarkRateLimiter*) - Fixed TestGenerateRandomPassword_ZeroLength naming
- Fixed test naming in audit_test.go, health_test.go, identity_test.go - Removed underscores from test function names - Conforms to Go naming conventions and linter requirements
- Modified GetInstanceStats in shared_test.go with distinct implementation - Renamed TestMultiTenancy_E2E to TestMultiTenancyE2E - Replaced hardcoded URLs/headers in e2e tests with testutil constants - Added instancesPathFmt constant to reduce duplication - Resolved linter warnings about identical code and duplicate literals
- Renamed test functions to CamelCase in instance_test.go - Updated code formatting and removed emojis per linter rules - Minor formatting improvements in cron.go and sg.go
- Removed emojis from storage.go per linter rules - Code formatting improvements in security_group.go - Ensures compliance with Go linting standards
|
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 a full Security Groups feature (API, handlers, service, repo, SDK, CLI, docs, Swagger) with OVS flow sync, tracing and audit instrumentation; centralizes many test constants in pkg/testutil and refactors numerous tests; adds a brainstorming workflow doc and exposes API port in docker-compose. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant HTTP as HTTP Handler
participant Service
participant Repo
participant OVS as OVS/Net Sync
participant Audit
Client->>HTTP: DELETE /security-groups/{id}/rules/{rule_id}
HTTP->>Service: RemoveRule(ctx, groupID, ruleID)
Service->>Repo: GetRuleByID(ctx, ruleID)
alt rule found
Service->>OVS: RemoveFlow(rule -> flow match)
OVS-->>Service: flow removal result
Service->>Repo: DeleteRule(ctx, ruleID) // ownership-checked delete
Repo-->>Service: deletion result
Service->>Audit: Log(action="remove-rule", details)
Service-->>HTTP: 200 OK
else not found / not owned
Service-->>HTTP: 404 / 403
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (GEMINI.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (GEMINI.md)
Files:
🧠 Learnings (5)📚 Learning: 2026-01-14T11:33:57.670ZApplied to files:
📚 Learning: 2026-01-14T11:33:57.670ZApplied to files:
📚 Learning: 2026-01-14T11:33:57.670ZApplied to files:
📚 Learning: 2026-01-14T11:33:57.670ZApplied to files:
📚 Learning: 2026-01-14T11:33:57.670ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Go Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: be7f1df | Previous: 20aeede | Ratio |
|---|---|---|---|
BenchmarkFunctionServiceInvoke |
54666 ns/op 1120 B/op 20 allocs/op |
28668 ns/op 1120 B/op 20 allocs/op |
1.91 |
BenchmarkFunctionServiceInvoke - ns/op |
54666 ns/op |
28668 ns/op |
1.91 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Pull request overview
This PR completes the Security Groups feature implementation by adding the missing RemoveRule and Detach methods to the service layer, implementing full OpenTelemetry tracing, adding audit logging for all operations, creating a unified CLI with all CRUD operations, and updating API documentation with Swagger annotations.
Changes:
- Implemented
RemoveRuleandDetachmethods in SecurityGroupService with full OVS flow cleanup - Added OpenTelemetry tracing spans to all service methods
- Added audit logging for group creation, deletion, rule changes, and instance associations
- Extended CLI with
get,delete,remove-rule, anddetachsubcommands - Added Swagger annotations for all security group endpoints
- Refactored handlers to use named request/response models
- Created centralized test constants in
pkg/testutil/constants.goto eliminate magic strings - Fixed test naming to follow Go conventions (removed underscores)
- Updated E2E tests to accept both 201 and 202 status codes for async operations
- Added new repository method
GetRuleByIDto support rule deletion
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/core/services/security_group.go | Added RemoveRule and Detach implementations with tracing and audit logging |
| internal/handlers/security_group_handler.go | Added RemoveRule and Detach handlers with Swagger docs |
| cmd/cloud/sg.go | Added remove-rule, detach, get, and delete CLI commands |
| pkg/testutil/constants.go | New file with centralized test constants |
| internal/repositories/postgres/security_group_repo.go | Added GetRuleByID method |
| tests/multitenancy_e2e_test.go | Updated to use testutil constants and accept 202 status |
| docs/swagger/*.{json,yaml,go} | Generated Swagger documentation for security group APIs |
| docs/guides/networking.md | Added Security Groups documentation |
| docs/api-reference.md | Added Security Groups API reference |
| docs/cli-reference.md | Added Security Groups CLI reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
pkg/ratelimit/limiter_test.go (1)
38-42: Key generation produces non-printable control characters.
string(rune(i%100))converts integers 0-99 to their Unicode code points. Values 0-31 produce control characters (NUL, SOH, etc.), resulting in keys like"key-\x00"instead of the likely intended"key-0".Suggested fix
for pb.Next() { // Simulate different clients - key := "key-" + string(rune(i%100)) + key := "key-" + strconv.Itoa(i%100) _ = limiter.GetLimiter(key) i++ }Add the import:
import "strconv"internal/handlers/auth_handler_test.go (1)
123-127: Inconsistent use oftestNameconstant.Line 126 uses the hard-coded string
"Test User"while the mock expectation at line 130 uses thetestNameconstant. This inconsistency could lead to test failures if the constant value changes.🐛 Proposed fix
body, err := json.Marshal(map[string]string{ "email": testEmail, "password": testPassword, - "name": "Test User", + "name": testName, })internal/handlers/subnet_handler_test.go (1)
59-93: DefinecontentTypeandapplicationJSONconstants in this test file or refactor to use table-driven tests.The test references undefined constants
contentTypeandapplicationJSONat lines 86 and 126 that cause compilation errors. These constants are defined ininstance_handler_test.gobut are not available in this file. Additionally, the test file should use table-driven tests instead of separate test functions to follow the coding guidelines for**/*_test.gofiles.internal/api/setup/router.go (1)
247-258: Security group routes lack RBAC permission checks and no permission constants are defined.All endpoints in the
sgGroup(lines 250-257) only have authentication middleware (httputil.Auth), while every other resource modification endpoint across the API useshttputil.Permissionmiddleware with domain permission constants. Additionally, noPermissionSecurityGroup*constants exist indomain/rbac.go, whereas similar resource types (VPC, Load Balancer, instances, etc.) all have defined permission sets. Since security groups control network access and topology, they require RBAC integration: definePermissionSecurityGroupCreate,PermissionSecurityGroupDelete,PermissionSecurityGroupUpdate(for rule modifications) constants and wrap each endpoint with the appropriate permission check.pkg/sdk/security_group_test.go (1)
168-171: Add tests forRemoveSecurityRuleandDetachSecurityGroup.The new SDK methods
RemoveSecurityRuleandDetachSecurityGroup(added inpkg/sdk/security_group.go) lack corresponding test coverage in this file. Consider adding tests following the existing patterns (e.g., similar toTestClientDeleteSecurityGroupandTestClientAttachSecurityGroup).Do you want me to generate the test implementations for these methods?
internal/handlers/security_group_handler.go (1)
110-113: Silent failure on invalidvpc_idquery parameter.When
vpc_idis provided but malformed, the parse error is silently ignored andvpcIDdefaults to zero UUID. This violates the "no silent failures" guideline and may cause unexpected behavior. Return a 400 error for invalidvpc_id.Suggested fix
var vpcID uuid.UUID if vpcIDStr != "" { - vpcID, _ = uuid.Parse(vpcIDStr) + var err error + vpcID, err = uuid.Parse(vpcIDStr) + if err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid vpc_id"}) + return + } }cmd/cloud/sg.go (1)
78-85: Potential panic if ID or VPCID is shorter than 8 characters.The slice operations
g.ID[:8]andg.VPCID[:8]will panic at runtime if the string length is less than 8. Consider using a helper function for safe truncation.Proposed fix with safe truncation helper
+func truncateID(id string, length int) string { + if len(id) < length { + return id + } + return id[:length] +} + for _, g := range groups { table.Append([]string{ - g.ID[:8], + truncateID(g.ID, 8), g.Name, - g.VPCID[:8], + truncateID(g.VPCID, 8), g.ARN, }) }
🤖 Fix all issues with AI agents
In `@cmd/cloud/sg.go`:
- Around line 192-205: The code slices r.ID[:8] inside the loop over sg.Rules
which can panic for short IDs; replace the direct slice with the safe truncation
helper used earlier (e.g., truncateID or safeTruncate) or add a small helper
func that returns s if len(s) <= n else s[:n], and call it where r.ID is
appended to table (replace r.ID[:8] with truncateID(r.ID, 8)) so table.Append
uses the safely truncated rule ID.
In `@cmd/cloud/storage.go`:
- Around line 74-76: Replace the two inline error format strings "Error: %v\n"
in cmd/cloud/storage.go with the shared errFmt constant from cmd/cloud/sg.go so
formatting is consistent; update the two locations that currently print with
fmt.Printf("Error: %v\n", err) to use fmt.Printf(errFmt, err) (do not change the
intentional, more specific error messages already present in other lines).
In `@docs/cli-reference.md`:
- Around line 397-407: Add a blank line immediately before the "**Flags**:"
heading and another blank line immediately after the table (before the "### `sg
remove-rule <rule-id>`" heading) so the Markdown table is surrounded by blank
lines and satisfies MD058; locate the table by the "**Flags**:" label and the
following pipe-delimited rows and insert one empty line above the "**Flags**:"
line and one empty line after the final "| `--priority` | `100` | Rule priority
|" row.
- Around line 370-432: Add documentation entries for the two missing subcommands
implemented in cmd/cloud/sg.go: `sg get [sg-id]` and `sg delete [sg-id]` under
the "Security Group Commands" section in docs/cli-reference.md, mirroring the
style of other commands (brief description, usage example like `cloud sg get
my-sg` and `cloud sg delete my-sg`, and note that `sg get` returns security
group details and rules while `sg delete` removes the security group). Ensure
they appear alongside `sg list`, `sg create`, etc., and follow the same
header/formatting conventions used for other sg subcommands.
In `@docs/swagger/docs.go`:
- Around line 2058-2457: The swagger spec for the DELETE operation on
"/security-groups/{id}" currently lists a 200 response but other delete
endpoints use 204; update the responses block under the "delete" operation for
"/security-groups/{id}" to use 204 (No Content) instead of 200 to match the API
convention, or if the handler actually returns 200 change the other delete
annotations to 200 for consistency — adjust the responses schema accordingly for
the "delete" method in the "/security-groups/{id}" section.
In `@docs/swagger/swagger.yaml`:
- Around line 2436-2688: The OpenAPI docs list DELETE operations for
/security-groups/{id} and /security-groups/rules/{rule_id} as returning HTTP 200
with a response body, but the handlers use 204 No Content; update the swagger
delete operations (the "delete" blocks for paths "/security-groups/{id}" and
"/security-groups/rules/{rule_id}") to return "204" instead of "200" and remove
or adjust the response schema to reflect no body (or reference an empty
response), ensuring the responses section and any generated examples match the
handlers' 204 semantics.
In `@internal/core/services/security_group.go`:
- Around line 231-250: DetachFromInstance currently only calls
repo.RemoveInstanceFromGroup and skips cleaning OVS flows; update
DetachFromInstance to mirror the AttachToInstance/RemoveRule pattern by calling
s.syncGroupFlows (or the appropriate flow cleanup helper) for the affected group
and instance before or after removing the DB association, ensuring any errors
from syncGroupFlows are handled/returned and that audit logging (s.auditSvc.Log)
still runs; locate the function DetachFromInstance and integrate a
syncGroupFlows call referencing the same groupID/instanceID used with
RemoveInstanceFromGroup to ensure OVS rules are cleaned up.
In `@internal/handlers/auth_handler_test.go`:
- Line 108: Add the missing constant definitions for contentType and
applicationJSON to internal/handlers/auth_handler_test.go so the uses at
req.Header.Set(contentType, applicationJSON) (and other occurrences) compile;
specifically define constants named contentType and applicationJSON with values
"Content-Type" and "application/json" respectively, or move these constants into
a shared test utilities package and import it from auth_handler_test.go to avoid
duplication across handler test files.
In `@internal/repositories/ovs/adapter_test.go`:
- Around line 83-120: Tests TestOvsAdapterCreateBridgeInvalidName,
TestOvsAdapterDeleteBridgeInvalidName and TestOvsAdapterAddPortInvalidName call
t.Fatalf with a dynamic message variable (errInvalidBridge) which triggers
SA1006; replace those t.Fatalf(errInvalidBridge) calls with
t.Fatal(errInvalidBridge) (or t.Fatalf with a format string and args) so the
test logs the message without treating it as a format string.
In `@internal/repositories/postgres/security_group_repo.go`:
- Around line 117-127: Update GetRuleByID and DeleteRule to enforce that the
caller owns the parent security group: modify GetRuleByID (function GetRuleByID)
to either join security_rules to security_groups and add a WHERE
security_groups.user_id = $2 (pass the caller user_id) or first query
security_groups for the rule’s group_id and verify its user_id before returning;
do the same for DeleteRule (function DeleteRule) so the DELETE only proceeds
when the associated security_groups.user_id matches the caller (use a DELETE ...
USING join or pre-check the group ownership), and return NotFound or a
permission error when ownership does not match.
In `@internal/repositories/postgres/shared_test.go`:
- Around line 3-6: Rename the constant testDbError to testDBError to follow Go
initialism conventions: change the declaration const testDbError -> testDBError
and update every reference to that symbol (e.g., in tests that assert DB error
behavior) to use testDBError; run `go test` or your test suite to find and fix
any remaining references to the old name.
In `@pkg/sdk/shared_test.go`:
- Around line 3-13: Rename the constant testApiKey to testAPIKey to follow Go
acronym casing conventions, update its declaration in the constants block
(replace testApiKey with testAPIKey) and then replace all references to the old
name throughout the package's tests so they compile; ensure you update any test
helpers or assertions that reference testApiKey to use testAPIKey instead.
In `@pkg/testutil/constants.go`:
- Around line 1-2: Add a package-level documentation comment at the top of the
constants.go file describing the purpose of the package; specifically add a doc
comment above "package testutil" that briefly explains that the testutil package
provides shared constants and helpers used across tests to improve readability
and reuse.
In `@tests/multitenancy_e2e_test.go`:
- Around line 109-116: The resp.Body.Close() return value in registerAndLogin is
ignored and triggers the errcheck linter; change the cleanup to explicitly
acknowledge the error when resp is non-nil by calling _ = resp.Body.Close() (or
otherwise handle the error) after ensuring resp != nil so the linter is
satisfied and you still preserve the intended behavior of ignoring registration
errors.
🧹 Nitpick comments (33)
cmd/cloud/instance_test.go (1)
184-186: Consider extracting the second port mapping for consistency.The refactor extracts
portMappingbut line 185 still concatenates a hardcoded string",8443:443". For full consistency, consider defining a second constant.💡 Suggested improvement
const portMapping = "8080:80" +const portMappingSecondary = "8443:443" // In test case: - ports: portMapping + ",8443:443", + ports: portMapping + "," + portMappingSecondary,internal/core/services/health_test.go (1)
48-76: Consider addingAssertExpectationsto remaining subtests.The
Healthysubtest verifies mock expectations, butDegraded_DBandDegraded_Dockerdo not. For consistency and to ensure all expected mock calls were made, consider adding assertions to these subtests as well.♻️ Suggested improvement
t.Run("Degraded_DB", func(t *testing.T) { db := new(mockCheckable) compute := new(fullMockComputeBackend) svc := NewHealthServiceImpl(db, compute) db.On("Ping", mock.Anything).Return(errors.New("db error")) compute.On("Ping", mock.Anything).Return(nil) res := svc.Check(context.Background()) assert.Equal(t, "DEGRADED", res.Status) assert.Contains(t, res.Checks["database"], "DISCONNECTED") assert.Equal(t, "CONNECTED", res.Checks["docker"]) + db.AssertExpectations(t) + compute.AssertExpectations(t) }) t.Run("Degraded_Docker", func(t *testing.T) { db := new(mockCheckable) compute := new(fullMockComputeBackend) svc := NewHealthServiceImpl(db, compute) db.On("Ping", mock.Anything).Return(nil) compute.On("Ping", mock.Anything).Return(errors.New("docker error")) res := svc.Check(context.Background()) assert.Equal(t, "DEGRADED", res.Status) assert.Equal(t, "CONNECTED", res.Checks["database"]) assert.Contains(t, res.Checks["docker"], "DISCONNECTED") + db.AssertExpectations(t) + compute.AssertExpectations(t) })internal/repositories/filesystem/adapter.go (1)
28-52: Constant usage looks correct; consider handling theCloseerror.The traversal check correctly uses the new constant. However, line 44 silently discards the error from
f.Close(). Per coding guidelines, avoid blank identifier assignments like_ = someFunc(). For write operations, failing to close properly could mean data wasn't flushed to disk.Consider logging or handling the close error
- defer func() { _ = f.Close() }() + defer func() { + if closeErr := f.Close(); closeErr != nil { + // Log or handle the close error appropriately + } + }()Alternatively, if you want to return the close error when no write error occurred:
func (s *LocalFileStore) Write(ctx context.Context, bucket, key string, r io.Reader) (written int64, err error) { // ... setup code ... defer func() { if closeErr := f.Close(); closeErr != nil && err == nil { err = errors.Wrap(errors.Internal, "failed to close file", closeErr) } }() // ... rest of function ... }Based on coding guidelines, avoid silent failures with blank identifier assignments.
internal/handlers/storage_handler_test.go (1)
127-127: Inconsistent route registration: hardcoded path instead of constant.Line 127 uses a hardcoded
"/storage/:bucket"while other tests usestorageObjectPath. Consider adding astorageBucketPathconstant for consistency.Suggested fix
Add a new constant:
const ( testFileName = "test.txt" storageBucketName = "b1" storageObjectPath = "/storage/:bucket/:key" storageBasePath = "/storage/" + storageBucketPath = "/storage/:bucket" )Then update line 127:
- r.GET("/storage/:bucket", handler.List) + r.GET(storageBucketPath, handler.List).agent/workflows/vision.md (4)
19-22: Clarify or remove the arbitrary file limit.The
head -100in the command limits the line count to only the first 100 Go files. If the project grows beyond 100 files, this will produce incomplete statistics without warning users.♻️ Proposed fix
Either remove the limit entirely:
# View project stats -wc -l $(find internal -name "*.go" | head -100) | tail -1 +wc -l $(find internal -name "*.go") | tail -1Or add a comment explaining why the limit exists:
-# View project stats +# View project stats (limited to first 100 files for performance) wc -l $(find internal -name "*.go" | head -100) | tail -1
46-46: Fix inconsistent project name capitalization."TheCloud" here is inconsistent with "The Cloud" used elsewhere in the document (lines 6, 9).
♻️ Proposed fix
-2. What would make a developer choose TheCloud over AWS for a side project? +2. What would make a developer choose The Cloud over AWS for a side project?
104-112: Clarify the weighted scoring calculation.The example shows individual scores but doesn't demonstrate the actual weighted calculation. The notation "20 * weights = HIGH PRIORITY" is unclear—it's not obvious what "20" represents or how to compute the final score.
♻️ Proposed fix to show complete calculation
**Example Scoring:**Feature: Kubernetes as a Service
Learning: 5 (massive)
User Impact: 5 (everyone wants K8s)
Differentiator: 3 (others have it)
Feasibility: 2 (complex)
Portfolio: 5 (impressive)
-Total: 20 * weights = HIGH PRIORITY
+
+Weighted Total:
- (5 × 5) + (5 × 4) + (3 × 4) + (2 × 3) + (5 × 3)
- = 25 + 20 + 12 + 6 + 15
- = 78 points → HIGH PRIORITY
</details> --- `104-104`: **Add language specifier to fenced code block.** The fenced code block is missing a language specification, which markdownlint flags. Adding `text` makes the intent clear and satisfies the linter. Based on static analysis hints. <details> <summary>♻️ Proposed fix</summary> ```diff **Example Scoring:** -``` +```text Feature: Kubernetes as a Serviceinternal/core/services/gateway_test.go (1)
114-153: Consider using table-driven tests for error scenarios.The error tests use subtests which is good, but the coding guidelines recommend using table-driven tests. This is a minor suggestion since subtests are still readable and maintainable.
♻️ Optional: Convert to table-driven test
func TestGatewayServiceErrors(t *testing.T) { ctx := appcontext.WithUserID(context.Background(), uuid.New()) userID := appcontext.UserIDFromContext(ctx) tests := []struct { name string setupMock func(*MockGatewayRepo) action func(ports.GatewayService) error }{ { name: "CreateRoute_RepoError", setupMock: func(repo *MockGatewayRepo) { repo.On("CreateRoute", ctx, mock.Anything).Return(assert.AnError) }, action: func(svc ports.GatewayService) error { _, err := svc.CreateRoute(ctx, "test", "/api", "http://target", true, 100) return err }, }, // ... additional cases } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { repo, _, svc := setupGatewayServiceTest(nil) tc.setupMock(repo) err := tc.action(svc) assert.Error(t, err) }) } }pkg/testutil/constants.go (1)
3-43: Well-structured test constants, minor observation on duplicate values.The centralized test constants improve consistency and eliminate magic numbers across tests. This aligns well with the coding guidelines.
One observation:
TestVXLANRemoteIP(line 26) andTestIPPrivate(line 7) both have the value"192.168.1.1". If these are intended to represent distinct concepts, consider using different values to avoid potential confusion in tests that use both.internal/core/services/benchmarks_test.go (1)
135-135: Silent failure onos.Setenv.Discarding the error from
os.Setenvviolates the coding guideline against blank identifier assignments. While unlikely to fail, consider logging or handling the error.Suggested fix
- _ = os.Setenv("TRACING_ENABLED", "false") + if err := os.Setenv("TRACING_ENABLED", "false"); err != nil { + b.Logf("failed to set TRACING_ENABLED: %v", err) + }internal/core/services/identity_test.go (1)
108-108: Consider extracting the revoke action to a constant for consistency.The
"api_key.revoke"literal could be extracted to a constant likeapiKeyRevokeActionfor consistency withapiKeyCreateAction.Suggested change
const ( apiKeyCreateAction = "api_key.create" apiKeyType = "api_key" + apiKeyRevokeAction = "api_key.revoke" + apiKeyRotateAction = "api_key.rotate" )Then update the usages at lines 108 and 138.
internal/handlers/subnet_handler_test.go (1)
95-275: Test coverage is comprehensive.The tests cover success cases, invalid UUID handling, invalid requests, and service errors for all CRUD operations. Test function names now follow Go conventions without underscores.
As per coding guidelines, consider consolidating similar test cases (e.g., invalid ID tests for Get, Delete) into table-driven tests for better maintainability. This is optional but would reduce duplication.
tests/multitenancy_e2e_test.go (1)
19-23: Remove commented-out code.Lines 21-22 contain commented-out code that should be removed. Dead code comments add noise and can become stale.
♻️ Suggested fix
const instancesPathFmt = "%s/instances/%s" - -// baseURL is now usually defined by constant, but let's assume we use the one from testutil -// const baseURL = "http://localhost:8080"internal/repositories/postgres/vpc_repo_unit_test.go (2)
102-104: Inconsistent use ofselectVpcconstant.This test case uses an inline query string while other cases (lines 81, 129) use the
selectVpcconstant. Consider using the constant for consistency.♻️ Suggested fix
- mock.ExpectQuery("SELECT id, user_id, name, COALESCE\\(cidr_block::text, ''\\), network_id, vxlan_id, status, arn, created_at FROM vpcs"). + mock.ExpectQuery(selectVpc).
244-280: Consider adding a db-error test case for Delete.Other repository methods (
Create,GetByID,List) include atestDbErrorcase to verify error handling when the database returns an error. This test function only covers success and not-found scenarios.internal/repositories/postgres/subnet_repo_test.go (1)
106-108: Inconsistent use ofselectSubnetconstant.This test case uses an inline query string while other cases (lines 84, 129) use the
selectSubnetconstant. Consider using the constant for consistency.♻️ Suggested fix
- mock.ExpectQuery("SELECT id, user_id, vpc_id, name, cidr_block::text, availability_zone, COALESCE\\(gateway_ip::text, ''\\), arn, status, created_at FROM subnets"). + mock.ExpectQuery(selectSubnet).internal/repositories/postgres/security_group_repo.go (1)
117-127: Consider aligning error handling withscanSecurityGrouppattern.The error handling here differs from
scanSecurityGroup(lines 80-90) which handlespgx.ErrNoRowsinternally.scanSecurityRule(lines 236-244) doesn't handleErrNoRows, so you handle it externally here. While functionally correct, this creates an inconsistent pattern within the same file.Consider updating
scanSecurityRuleto handleErrNoRowslikescanSecurityGroupdoes:Alternative: Update scanSecurityRule for consistency
func (r *SecurityGroupRepository) scanSecurityRule(row pgx.Row) (domain.SecurityRule, error) { var rule domain.SecurityRule var direction string if err := row.Scan(&rule.ID, &rule.GroupID, &direction, &rule.Protocol, &rule.PortMin, &rule.PortMax, &rule.CIDR, &rule.Priority, &rule.CreatedAt); err != nil { + if err == pgx.ErrNoRows { + return domain.SecurityRule{}, errors.New(errors.NotFound, "security rule not found") + } - return domain.SecurityRule{}, err + return domain.SecurityRule{}, errors.Wrap(errors.Internal, "failed to scan security rule", err) } rule.Direction = domain.RuleDirection(direction) return rule, nil }Then simplify
GetRuleByID:func (r *SecurityGroupRepository) GetRuleByID(ctx context.Context, ruleID uuid.UUID) (*domain.SecurityRule, error) { query := `SELECT id, group_id, direction, protocol, port_min, port_max, cidr, priority, created_at FROM security_rules WHERE id = $1` rule, err := r.scanSecurityRule(r.db.QueryRow(ctx, query, ruleID)) 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, nil }internal/repositories/ovs/adapter_unit_test.go (1)
138-155: Consider consolidating to table-driven tests.The subtest pattern here is good, but tests like
TestOvsAdapterDeleteFlowRule,TestOvsAdapterCreateVXLANTunnel, and similar could be consolidated into table-driven tests for better maintainability when testing multiple scenarios.Example table-driven pattern
func TestOvsAdapterDeleteFlowRule(t *testing.T) { tests := []struct { name string bridge string wantErr bool errType apperrors.Type }{ {"success", "br0", false, ""}, {"invalid bridge", badBridge, true, apperrors.InvalidInput}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { fx := &fakeExecer{cmd: &fakeCmd{}} a := &OvsAdapter{ofctlPath: ovsOfctlPath, logger: slog.Default(), exec: fx} err := a.DeleteFlowRule(context.Background(), tt.bridge, "match") if tt.wantErr { require.Error(t, err) require.True(t, apperrors.Is(err, tt.errType)) } else { require.NoError(t, err) } }) } }internal/core/services/security_group_test.go (2)
163-186: Good test coverage for the happy path.The new
RemoveRuletest properly verifies the service correctly orchestrates rule lookup, group retrieval, VPC lookup, flow rule deletion, and database cleanup.Consider adding error case tests in
TestSecurityGroupServiceErrorsforRemoveRule, such as:
- Rule not found (
GetRuleByIDreturns error)- Group not found (
GetByIDreturns error after rule fetch)- Flow deletion failure (
DeleteFlowRulereturns error)
188-212: Consider expanding error case coverage.The error tests cover
Create,Delete, andAddRulefailures, but the newRemoveRule,AttachToInstance, andDetachFromInstancemethods would benefit from similar error case testing for completeness.internal/handlers/security_group_handler.go (1)
21-25: Addomitemptyto optional field.The
Descriptionfield is optional but lacks theomitemptyJSON tag. Per coding guidelines, optional fields should includeomitempty.Suggested fix
type CreateSecurityGroupRequest struct { VPCID uuid.UUID `json:"vpc_id" binding:"required"` Name string `json:"name" binding:"required"` - Description string `json:"description"` + Description string `json:"description,omitempty"` }internal/handlers/security_group_handler_test.go (1)
72-75: Consider using descriptive parameter names.The parameter names were changed from
ctx, instanceID, groupIDtoc, i, g. While functional, the previous names were more self-documenting.Suggested improvement
-func (m *mockSecurityGroupService) DetachFromInstance(c context.Context, i uuid.UUID, g uuid.UUID) error { - args := m.Called(c, i, g) +func (m *mockSecurityGroupService) DetachFromInstance(ctx context.Context, instanceID, groupID uuid.UUID) error { + args := m.Called(ctx, instanceID, groupID) return args.Error(0) }pkg/sdk/subnet_test.go (1)
106-106: Inconsistent header key usage.Line 106 uses the string literal
"Content-Type"while lines 25 and 62 use thecontentTypeconstant. Use the constant consistently.Suggested fix
- w.Header().Set("Content-Type", testutil.TestContentTypeAppJSON) + w.Header().Set(contentType, testutil.TestContentTypeAppJSON)docs/swagger/swagger.json (1)
2415-2423: Consider using a dedicated request type for AddRule endpoint.The request body references
domain.SecurityRulewhich includes server-generated fields (id,group_id,created_at) that shouldn't be part of the request payload. This could confuse API consumers about which fields they should provide.A dedicated
httphandlers.AddSecurityRuleRequesttype (similar toCreateSecurityGroupRequest) would make the API contract clearer by only exposing the fields the client should supply:direction,protocol,port_min,port_max,cidr, andpriority.cmd/cloud/sg.go (3)
69-73: Avoid silent failure on JSON marshaling error.Per coding guidelines, blank identifier assignment (
_ = someFunc()) should be avoided. Whilejson.MarshalIndentrarely fails for in-memory structs, handling the error ensures robustness.Proposed fix
if outputJSON { - data, _ := json.MarshalIndent(groups, "", " ") + data, err := json.MarshalIndent(groups, "", " ") + if err != nil { + fmt.Printf(errFmt, err) + return + } fmt.Println(string(data)) return }Based on coding guidelines, do not use silent failures.
177-181: Same silent failure pattern with JSON marshaling.Proposed fix
if outputJSON { - data, _ := json.MarshalIndent(sg, "", " ") + data, err := json.MarshalIndent(sg, "", " ") + if err != nil { + fmt.Printf(errFmt, err) + return + } fmt.Println(string(data)) return }Based on coding guidelines, do not use silent failures.
90-120: Consider validating port range consistency.The command accepts
--port-minand--port-maxflags without validating thatport-min <= port-max. Invalid ranges could cause unexpected behavior server-side or confusing error messages.Optional validation
portMin, _ := cmd.Flags().GetInt("port-min") portMax, _ := cmd.Flags().GetInt("port-max") + if portMin > portMax { + fmt.Printf("Error: --port-min (%d) cannot be greater than --port-max (%d)\n", portMin, portMax) + return + } cidr, _ := cmd.Flags().GetString("cidr")docs/swagger/docs.go (1)
2400-2457: Avoid usingdomain.SecurityRuleas the “create rule” request model (server-owned fields leak into input).
POST /security-groups/{id}/rulestakesdomain.SecurityRule, which contains fields likeid,created_at, andgroup_id. Prefer a dedicated request schema (e.g.,httphandlers.CreateSecurityRuleRequest) that only allows client-settable fields, and setgroup_idfrom the path on the server.Also applies to: 4025-4059
docs/swagger/swagger.yaml (1)
578-623: Avoiddomain.SecurityRuleas the add-rule request schema.Using
domain.SecurityRuleforPOST /security-groups/{id}/rulesallows client-providedid/created_at/group_id. A dedicated request schema would prevent conflicting inputs and reduce validation surface.Also applies to: 2560-2596
internal/repositories/postgres/security_group_repo_test.go (1)
473-509:ListInstanceGroupstests look fine; consider extracting the repeated SELECT string into a constant for consistency.internal/repositories/ovs/adapter_test.go (2)
25-69: Make integration cleanup robust even when earlier subtests fail.If
CreateBridgesucceeds but a later subtestFatals, cleanup might not run. Consider registeringt.Cleanup(func(){ _ = adapter.DeleteBridge(ctx, bridgeName) })immediately after creating the bridge (and optionally best-effort delete flow rule too).
30-32: Consider usingexec.LookPath("ovs-vsctl")instead of hardcoding/usr/bin/ovs-vsctl.Hardcoding can break on systems where OVS is installed elsewhere.
|
|
||
| **Flags**: | ||
| | Flag | Default | Description | | ||
| |------|---------|-------------| | ||
| | `--direction` | `ingress` | Direction (ingress/egress) | | ||
| | `--protocol` | `tcp` | Protocol (tcp/udp/icmp/all) | | ||
| | `--port-min` | `0` | Minimum port | | ||
| | `--port-max` | `0` | Maximum port | | ||
| | `--cidr` | `0.0.0.0/0` | CIDR block | | ||
| | `--priority` | `100` | Rule priority | | ||
|
|
There was a problem hiding this comment.
Add blank lines around the flags table.
The static analysis tool flagged that tables should be surrounded by blank lines (MD058). Add a blank line before and after the flags table for proper Markdown formatting.
📝 Proposed fix
```bash
cloud sg add-rule my-sg --protocol tcp --port-min 80 --port-max 80 --cidr 0.0.0.0/0Flags:
+
| Flag | Default | Description |
|---|---|---|
--direction |
ingress |
Direction (ingress/egress) |
--protocol |
tcp |
Protocol (tcp/udp/icmp/all) |
--port-min |
0 |
Minimum port |
--port-max |
0 |
Maximum port |
--cidr |
0.0.0.0/0 |
CIDR block |
--priority |
100 |
Rule priority |
sg remove-rule <rule-id>
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
**Flags**:
| Flag | Default | Description |
|------|---------|-------------|
| `--direction` | `ingress` | Direction (ingress/egress) |
| `--protocol` | `tcp` | Protocol (tcp/udp/icmp/all) |
| `--port-min` | `0` | Minimum port |
| `--port-max` | `0` | Maximum port |
| `--cidr` | `0.0.0.0/0` | CIDR block |
| `--priority` | `100` | Rule priority |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
399-399: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
In `@docs/cli-reference.md` around lines 397 - 407, Add a blank line immediately
before the "**Flags**:" heading and another blank line immediately after the
table (before the "### `sg remove-rule <rule-id>`" heading) so the Markdown
table is surrounded by blank lines and satisfies MD058; locate the table by the
"**Flags**:" label and the following pipe-delimited rows and insert one empty
line above the "**Flags**:" line and one empty line after the final "|
`--priority` | `100` | Rule priority |" row.
| const ( | ||
| testNotFound = "not found" | ||
| testDbError = "db error" | ||
| ) |
There was a problem hiding this comment.
Rename testDbError to testDBError per Go conventions.
Go style guidelines require initialisms to be all uppercase. The static analysis correctly identifies that Db should be DB.
Proposed fix
const (
testNotFound = "not found"
- testDbError = "db error"
+ testDBError = "db error"
)Note: This will require updating all usages in volume_repo_test.go (and any other files) to use testDBError instead of testDbError.
📝 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.
| const ( | |
| testNotFound = "not found" | |
| testDbError = "db error" | |
| ) | |
| const ( | |
| testNotFound = "not found" | |
| testDBError = "db error" | |
| ) |
🧰 Tools
🪛 GitHub Check: test
[failure] 5-5:
ST1003: const testDbError should be testDBError (staticcheck)
🤖 Prompt for AI Agents
In `@internal/repositories/postgres/shared_test.go` around lines 3 - 6, Rename the
constant testDbError to testDBError to follow Go initialism conventions: change
the declaration const testDbError -> testDBError and update every reference to
that symbol (e.g., in tests that assert DB error behavior) to use testDBError;
run `go test` or your test suite to find and fix any remaining references to the
old name.
| const ( | ||
| testApiKey = "test-api-key" | ||
| testSgName = "test-sg" | ||
| sg123 = "sg-123" | ||
| sgBasePath = "/security-groups" | ||
| sgDetailPath = "/security-groups/" | ||
| testSubnetName = "test-subnet" | ||
| subnet1 = "subnet-1" | ||
| contentType = "Content-Type" | ||
| testVpcName = "new-vpc" | ||
| ) |
There was a problem hiding this comment.
Fix acronym casing per Go conventions.
Static analysis correctly flags that testApiKey should be testAPIKey. Go convention is to keep acronyms in uppercase.
Proposed fix
const (
- testApiKey = "test-api-key"
+ testAPIKey = "test-api-key"
testSgName = "test-sg"
sg123 = "sg-123"
sgBasePath = "/security-groups"
sgDetailPath = "/security-groups/"
testSubnetName = "test-subnet"
subnet1 = "subnet-1"
contentType = "Content-Type"
testVpcName = "new-vpc"
)After renaming, update all usages of testApiKey to testAPIKey in test files within this package.
#!/bin/bash
# Find all usages of testApiKey that need updating
rg -n 'testApiKey' pkg/sdk/🧰 Tools
🪛 GitHub Check: test
[failure] 4-4:
ST1003: const testApiKey should be testAPIKey (staticcheck)
🤖 Prompt for AI Agents
In `@pkg/sdk/shared_test.go` around lines 3 - 13, Rename the constant testApiKey
to testAPIKey to follow Go acronym casing conventions, update its declaration in
the constants block (replace testApiKey with testAPIKey) and then replace all
references to the old name throughout the package's tests so they compile;
ensure you update any test helpers or assertions that reference testApiKey to
use testAPIKey instead.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
cmd/cloud/sg.go (1)
78-85: Potential panic from direct ID slicing.
g.ID[:8]andg.VPCID[:8]will panic if either ID is shorter than 8 characters. ThetruncateIDhelper was added (line 241) to prevent exactly this issue—apply it here as well.Proposed fix
for _, g := range groups { table.Append([]string{ - g.ID[:8], + truncateID(g.ID, 8), g.Name, - g.VPCID[:8], + truncateID(g.VPCID, 8), g.ARN, }) }pkg/sdk/security_group_test.go (1)
115-171: Add tests for RemoveSecurityRule and DetachSecurityGroup methods.The RemoveSecurityRule and DetachSecurityGroup methods are implemented in security_group.go but lack corresponding test coverage in this file. Add TestClientRemoveSecurityRule and TestClientDetachSecurityGroup to match the existing test pattern.
internal/repositories/ovs/adapter_test.go (1)
129-133: Fix SA1006:t.Fatalfwith no format arguments.Line 131 uses
t.Fatalf("expected error for invalid port name")which, while currently working, should uset.Fatalfor consistency with other tests and to avoid staticcheck warnings.Proposed fix
err = adapter.DeletePort(context.Background(), "bridge", "invalid port") if err == nil { - t.Fatalf("expected error for invalid port name") + t.Fatal("expected error for invalid port name") }internal/handlers/security_group_handler.go (1)
106-122: Silent failure on UUID parse error.Line 112 discards the parse error with
vpcID, _ = uuid.Parse(vpcIDStr). Per coding guidelines, avoid blank identifier assignments. Ifvpc_idis provided but invalid, the user should receive feedback rather than having it silently ignored.Proposed fix
var vpcID uuid.UUID if vpcIDStr != "" { - vpcID, _ = uuid.Parse(vpcIDStr) + var err error + vpcID, err = uuid.Parse(vpcIDStr) + if err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid vpc_id format"}) + return + } }Based on coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()"internal/core/services/security_group.go (1)
143-147: OVS flow sync failure is logged but not rolled back.If
syncGroupFlowsfails after the rule is persisted to the database, the system is left in an inconsistent state. The comment acknowledges this, but there's no compensation logic or TODO to track it.Would you like me to help draft a compensation pattern or open an issue to track adding transactional semantics here?
internal/repositories/postgres/security_group_repo_test.go (1)
265-299: FixDeleteRuleDB-error test arg mismatch (likely failing after ownership checks).The success case expects
(ruleID, userID)but the DB-error case expects only(ruleID)and calls withcontext.Background(). IfDeleteRulealways enforces ownership, this test will fail.Proposed fix
t.Run(testDBError, func(t *testing.T) { mock, err := pgxmock.NewPool() assert.NoError(t, err) defer mock.Close() repo := NewSecurityGroupRepository(mock) ruleID := uuid.New() + userID := uuid.New() + ctx := appcontext.WithUserID(context.Background(), userID) mock.ExpectExec("DELETE FROM security_rules"). - WithArgs(ruleID). + WithArgs(ruleID, userID). WillReturnError(errors.New(testDBError)) - err = repo.DeleteRule(context.Background(), ruleID) + err = repo.DeleteRule(ctx, ruleID) assert.Error(t, err) })
🤖 Fix all issues with AI agents
In `@docs/swagger/swagger.json`:
- Around line 2290-2382: The GET /security-groups/{id} operation currently
documents the query parameter "vpc_id" as "optional if ID is UUID", which is
ambiguous; update the OpenAPI spec for the GET /security-groups/{id} path to
clearly state the lookup semantics: explain whether {id} can be either UUID or
name, the precedence when both {id} and vpc_id are provided, and whether vpc_id
is required for non-UUID names (or split into separate endpoints if different
behavior); modify the "description" for the "vpc_id" parameter (and optionally
the overall operation "description" or add examples) to explicitly document the
resolution rules so clients know when to include vpc_id and how conflicts are
handled.
In `@docs/swagger/swagger.yaml`:
- Around line 2525-2556: The current GET operation (summary: "Get security
group") ambiguously describes the path parameter "id" vs query parameter
"vpc_id"; update the documentation for this operation to explicitly state the
resolution rules (e.g., in the description: "If id parses as a UUID, it is
treated as the security group ID; otherwise id is treated as a name and vpc_id
is required to disambiguate") or alternatively add two distinct endpoints (one
/security-groups/{id} for UUIDs and one /security-groups/by-name with query
params name and vpc_id). Make the chosen precedence rule or endpoint split
explicit in the operation description and parameter docs for "id" and "vpc_id"
so clients know whether vpc_id is optional or mandatory when passing a name.
In `@internal/core/services/security_group_test.go`:
- Around line 148-167: TestSecurityGroupServiceDetachFromInstance is missing a
mock for the network flow deletion called by DetachFromInstance ->
removeGroupFlows; add a network mock expectation similar to the attach test such
as expecting network.DeleteFlowRule with the VPC network ID ("net1") and any
rule payload and returning nil so the test aligns with the actual call path
(refer to symbols: TestSecurityGroupServiceDetachFromInstance,
DetachFromInstance, removeGroupFlows, network.DeleteFlowRule).
In `@internal/repositories/postgres/security_group_repo.go`:
- Around line 135-150: In DeleteRule, don’t return the raw error from r.db.Exec;
wrap it consistently using errors.Wrap (like other repo methods) so upstream
gets the wrapped context. Replace the direct return of err after r.db.Exec(ctx,
query, ruleID, userID) with a wrapped error (e.g., return errors.Wrap(err,
"failed to delete security rule") ), keeping the existing NotFound handling for
res.RowsAffected().
🧹 Nitpick comments (13)
cmd/cloud/sg.go (1)
69-73: Silent error handling for JSON marshaling.The blank identifier on
json.MarshalIndentviolates the coding guideline against silent failures. While unlikely to fail for these types, consider logging or handling the error.Suggested improvement
if outputJSON { - data, _ := json.MarshalIndent(groups, "", " ") + data, err := json.MarshalIndent(groups, "", " ") + if err != nil { + fmt.Printf(errFmt, err) + return + } fmt.Println(string(data)) return }The same pattern applies to line 178 in
sgGetCmd.tests/multitenancy_e2e_test.go (1)
181-185: Consider handling errors in cleanup function.Line 184 ignores both the response and error from
client.Do(req). While this is cleanup code, silently ignoring errors can mask issues during test debugging.♻️ Suggested improvement
func deleteInstance(t *testing.T, client *http.Client, token, id string) { req, _ := http.NewRequest("DELETE", fmt.Sprintf(instancesPathFmt, testutil.TestBaseURL, id), nil) req.Header.Set(testutil.TestHeaderAPIKey, token) - _, _ = client.Do(req) + resp, err := client.Do(req) + if err != nil { + t.Logf("Warning: failed to delete instance %s: %v", id, err) + return + } + _ = resp.Body.Close() }internal/repositories/ovs/adapter_test.go (1)
25-69: Integration test structure looks solid.The test properly:
- Guards execution with environment variable check
- Validates OVS availability before proceeding
- Uses unique bridge names to avoid collisions
- Follows a logical create → use → cleanup flow
One minor observation: the hardcoded path
/usr/bin/ovs-vsctlat line 30 differs from howNewOvsAdapterdiscovers the binary viaLookPath. Consider usingexec.LookPath("ovs-vsctl")for consistency.internal/repositories/postgres/security_group_repo.go (1)
152-157:Deletedoesn't check affected rows.Unlike
DeleteRulewhich checksRowsAffected()to returnNotFoundwhen no rows are deleted, theDeletemethod for security groups silently succeeds even if the group doesn't exist or isn't owned by the user. Consider adding the same pattern for consistency.internal/core/services/security_group_test.go (1)
194-218: Consider expanding error test cases.
TestSecurityGroupServiceErrorscovers Create, Delete, and AddRule errors. Consider adding error cases for the newRemoveRulemethod (e.g., rule not found, delete failure) to maintain test coverage parity.internal/core/services/security_group.go (3)
29-43: Constructor with 5 dependencies should use a params struct.Per coding guidelines, services with 3 or more dependencies should use a params struct for constructor injection. This improves readability and makes it easier to add/modify dependencies.
♻️ Suggested refactor
+type SecurityGroupServiceParams struct { + Repo ports.SecurityGroupRepository + VpcRepo ports.VpcRepository + Network ports.NetworkBackend + AuditSvc ports.AuditService + Logger *slog.Logger +} + -func NewSecurityGroupService( - repo ports.SecurityGroupRepository, - vpcRepo ports.VpcRepository, - network ports.NetworkBackend, - auditSvc ports.AuditService, - logger *slog.Logger, -) *SecurityGroupService { +func NewSecurityGroupService(params SecurityGroupServiceParams) *SecurityGroupService { return &SecurityGroupService{ - repo: repo, - vpcRepo: vpcRepo, - network: network, - auditSvc: auditSvc, - logger: logger, + repo: params.Repo, + vpcRepo: params.VpcRepo, + network: params.Network, + auditSvc: params.AuditSvc, + logger: params.Logger, } }Based on coding guidelines: "Use a params struct for service constructors with 3 or more dependencies".
73-76: Silent failure on audit logging violates coding guidelines.Using blank identifier assignment
_ = s.auditSvc.Log(...)suppresses errors without handling them. This pattern appears multiple times in this file (lines 73, 114, 149, 194, 224, 253). Consider logging audit failures or returning them if audit integrity is required.♻️ Suggested approach
- _ = s.auditSvc.Log(ctx, userID, "security_group.create", "security_group", sgID.String(), map[string]interface{}{ + if err := s.auditSvc.Log(ctx, userID, "security_group.create", "security_group", sgID.String(), map[string]interface{}{ "name": name, "vpc_id": vpcID, - }) + }); err != nil { + s.logger.Warn("failed to log audit event", "action", "security_group.create", "error", err) + }Based on coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()".
277-291: removeGroupFlows continues on individual flow deletion errors.The helper logs errors but continues deleting remaining flows even if one fails. This is reasonable for cleanup, but the function always returns
nilregardless of whether any deletions failed. Consider returning an aggregated error or at least the last error encountered.♻️ Optional improvement
func (s *SecurityGroupService) removeGroupFlows(ctx context.Context, sg *domain.SecurityGroup) error { vpc, err := s.vpcRepo.GetByID(ctx, sg.VPCID) if err != nil { return err } + var lastErr error for _, rule := range sg.Rules { flow := s.translateToFlow(rule) if err := s.network.DeleteFlowRule(ctx, vpc.NetworkID, flow.Match); err != nil { s.logger.Error("failed to delete flow rule", "rule", rule.ID, "error", err) + lastErr = err } } - return nil + return lastErr }internal/repositories/postgres/subnet_repo_test.go (1)
96-117: Consider using assert.Require for critical type assertions.The error type assertion
theCloudErr, ok := err.(*theclouderrors.Error)silently skips the assertion if the type doesn't match. Usingassert.IsTypeorrequire.True(t, ok)would fail the test explicitly if the error type is unexpected.♻️ Suggested improvement
theCloudErr, ok := err.(*theclouderrors.Error) - if ok { - assert.Equal(t, theclouderrors.NotFound, theCloudErr.Type) - } + assert.True(t, ok, "expected *theclouderrors.Error type") + assert.Equal(t, theclouderrors.NotFound, theCloudErr.Type)docs/swagger/swagger.json (1)
2047-2440: Consider a dedicated request model forPOST /security-groups/{id}/rules(avoid server-owned fields).The request schema is
domain.SecurityRule, which includes fields likeid,group_id, andcreated_atthat are typically server-generated; a slimmerhttphandlers.AddSecurityRuleRequest(or similar) would reduce ambiguity for clients.docs/swagger/swagger.yaml (1)
2436-2684: ForPOST /security-groups/{id}/rules, prefer a request schema overdomain.SecurityRule.Using the domain model as the request body risks clients sending
id/group_id/created_at; a dedicated request DTO would make contract clearer.internal/repositories/postgres/security_group_repo_test.go (2)
302-324: Minor: align GetRuleByID test data with existing constants for CIDR/direction.Use
testutil.TestAnyCIDRandstring(domain.RuleIngress)to match the rest of this file and reduce accidental drift.
27-558: Overall test structure is consistent with the repo’s existing style.One follow-up: consider table-driven consolidation for the repetitive success/DB-error/not-found subtests. As per coding guidelines, table-driven tests are preferred.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
cmd/cloud/sg.gocmd/cloud/storage.godocs/cli-reference.mddocs/swagger/docs.godocs/swagger/swagger.jsondocs/swagger/swagger.yamlinternal/core/services/security_group.gointernal/core/services/security_group_test.gointernal/handlers/security_group_handler.gointernal/repositories/ovs/adapter_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/security_group_repo.gointernal/repositories/postgres/security_group_repo_test.gointernal/repositories/postgres/shared_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/volume_repo_test.gointernal/repositories/postgres/vpc_repo_unit_test.gopkg/sdk/security_group_test.gopkg/sdk/shared_test.gopkg/sdk/subnet_test.gopkg/sdk/vpc_test.gopkg/testutil/constants.gotests/multitenancy_e2e_test.go
💤 Files with no reviewable changes (1)
- internal/repositories/postgres/volume_repo_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/cli-reference.md
- cmd/cloud/storage.go
- pkg/sdk/subnet_test.go
🧰 Additional context used
📓 Path-based instructions (8)
**/*.go
📄 CodeRabbit inference engine (GEMINI.md)
**/*.go: Do not use global variables (e.g.,var DB *sql.DB)
Do not panic in production code - return errors instead
Do not use silent failures - avoid blank identifier assignment like_ = someFunc()
Do not use magic numbers - use named constants instead
Do not skipcontext.Contextas the first parameter in functions
Do not create circular dependencies between packages
Use constructor injection for dependencies instead of global initialization
Propagatecontext.Contextto all blocking calls
Use PascalCase for exported types and structs (e.g.,InstanceService,LaunchRequest)
Use PascalCase for exported interfaces (e.g.,InstanceRepository,ComputeBackend)
Use PascalCase for exported functions and camelCase for unexported functions
Use SCREAMING_SNAKE_CASE for status constants and PascalCase for limit constants (e.g.,StatusRunning,MaxPortsPerInstance)
Use camelCase for variable names (e.g.,instanceRepo,vpcID)
Includejsontags on struct fields withomitemptyfor optional fields
Files:
pkg/sdk/vpc_test.gointernal/handlers/security_group_handler.gocmd/cloud/sg.gopkg/sdk/shared_test.gointernal/repositories/postgres/security_group_repo.gointernal/repositories/postgres/shared_test.gointernal/core/services/security_group.gotests/multitenancy_e2e_test.gointernal/repositories/postgres/vpc_repo_unit_test.gopkg/sdk/security_group_test.godocs/swagger/docs.gointernal/core/services/security_group_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/subnet_repo_test.gopkg/testutil/constants.gointernal/repositories/postgres/security_group_repo_test.gointernal/repositories/ovs/adapter_test.go
**/*_test.go
📄 CodeRabbit inference engine (GEMINI.md)
**/*_test.go: Do not test with real external dependencies in unit tests - use mocks instead
Use table-driven tests in test files
Usetestify/mockfor creating mock objects in tests
Files:
pkg/sdk/vpc_test.gopkg/sdk/shared_test.gointernal/repositories/postgres/shared_test.gotests/multitenancy_e2e_test.gointernal/repositories/postgres/vpc_repo_unit_test.gopkg/sdk/security_group_test.gointernal/core/services/security_group_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/security_group_repo_test.gointernal/repositories/ovs/adapter_test.go
internal/handlers/*_handler.go
📄 CodeRabbit inference engine (GEMINI.md)
internal/handlers/*_handler.go: Handlers (internal/handlers/) must import Services only, not Repositories or other layers
Do not put business logic in handlers
Do not import repositories directly in handlers
Create handlers with the naming convention<resource>_handler.goatinternal/handlers/
Use handler-layer error handling withhttputil.Error(c, err)to automatically map errors to HTTP status codes
Usehttputil.Success(c, http.StatusOK, data)for successful handler responses
Validate request input in handlers usingc.ShouldBindJSON(&req)before processing
Include Swagger documentation comments on handler methods with@Summary,@Tags,@Security, and@Routerdirectives
Files:
internal/handlers/security_group_handler.go
internal/repositories/**/*.go
📄 CodeRabbit inference engine (GEMINI.md)
Repositories (
internal/repositories/) must implement Ports interfaces
Files:
internal/repositories/postgres/security_group_repo.gointernal/repositories/postgres/shared_test.gointernal/repositories/postgres/vpc_repo_unit_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/security_group_repo_test.gointernal/repositories/ovs/adapter_test.go
internal/repositories/postgres/*.go
📄 CodeRabbit inference engine (GEMINI.md)
Create repository implementations at
internal/repositories/postgres/<name>.go
Files:
internal/repositories/postgres/security_group_repo.gointernal/repositories/postgres/shared_test.gointernal/repositories/postgres/vpc_repo_unit_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/security_group_repo_test.go
internal/core/services/*.go
📄 CodeRabbit inference engine (GEMINI.md)
internal/core/services/*.go: Services (internal/core/services/) must import Ports and Domain only, not Repositories directly
Do not skip tests for new services
Create service implementations atinternal/core/services/<name>.go
Use a params struct for service constructors with 3 or more dependencies
Files:
internal/core/services/security_group.gointernal/core/services/security_group_test.go
internal/core/services/**/*.go
📄 CodeRabbit inference engine (GEMINI.md)
internal/core/services/**/*.go: Useinternal/errorspackage for domain errors
Use service-layer error handling withinternal/errorspackage (e.g.,errors.New(errors.NotFound, "message")orerrors.Wrap(errors.Internal, "message", err))
Files:
internal/core/services/security_group.gointernal/core/services/security_group_test.go
internal/core/services/**/*_test.go
📄 CodeRabbit inference engine (GEMINI.md)
Mock repositories in service tests
Files:
internal/core/services/security_group_test.go
🧠 Learnings (20)
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/handlers/*_handler.go : Include Swagger documentation comments on handler methods with Summary, Tags, Security, and Router directives
Applied to files:
internal/handlers/security_group_handler.godocs/swagger/docs.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/handlers/*_handler.go : Create handlers with the naming convention `<resource>_handler.go` at `internal/handlers/`
Applied to files:
internal/handlers/security_group_handler.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*.go : Use camelCase for variable names (e.g., `instanceRepo`, `vpcID`)
Applied to files:
cmd/cloud/sg.gopkg/sdk/shared_test.gointernal/repositories/postgres/shared_test.gointernal/repositories/postgres/vpc_repo_unit_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/security_group_repo_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*.go : Use SCREAMING_SNAKE_CASE for status constants and PascalCase for limit constants (e.g., `StatusRunning`, `MaxPortsPerInstance`)
Applied to files:
cmd/cloud/sg.gopkg/sdk/shared_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*_test.go : Do not test with real external dependencies in unit tests - use mocks instead
Applied to files:
pkg/sdk/shared_test.gointernal/repositories/postgres/shared_test.gotests/multitenancy_e2e_test.gointernal/repositories/postgres/vpc_repo_unit_test.gointernal/core/services/security_group_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/security_group_repo_test.gointernal/repositories/ovs/adapter_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*_test.go : Use table-driven tests in test files
Applied to files:
pkg/sdk/shared_test.gointernal/repositories/postgres/shared_test.gotests/multitenancy_e2e_test.gointernal/repositories/postgres/vpc_repo_unit_test.gointernal/core/services/security_group_test.gointernal/repositories/postgres/security_group_repo_test.gointernal/repositories/ovs/adapter_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/core/services/*.go : Do not skip tests for new services
Applied to files:
pkg/sdk/shared_test.gointernal/repositories/postgres/shared_test.gotests/multitenancy_e2e_test.gointernal/core/services/security_group_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*.go : Use PascalCase for exported types and structs (e.g., `InstanceService`, `LaunchRequest`)
Applied to files:
pkg/sdk/shared_test.gointernal/repositories/postgres/instance_repo_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*.go : Use PascalCase for exported interfaces (e.g., `InstanceRepository`, `ComputeBackend`)
Applied to files:
pkg/sdk/shared_test.gointernal/repositories/postgres/shared_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/security_group_repo_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*.go : Do not use magic numbers - use named constants instead
Applied to files:
pkg/sdk/shared_test.gopkg/testutil/constants.gointernal/repositories/ovs/adapter_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*.go : Use PascalCase for exported functions and camelCase for unexported functions
Applied to files:
pkg/sdk/shared_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/repositories/postgres/*.go : Create repository implementations at `internal/repositories/postgres/<name>.go`
Applied to files:
internal/repositories/postgres/shared_test.gointernal/repositories/postgres/vpc_repo_unit_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/security_group_repo_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/core/services/**/*_test.go : Mock repositories in service tests
Applied to files:
internal/repositories/postgres/shared_test.gotests/multitenancy_e2e_test.gointernal/repositories/postgres/vpc_repo_unit_test.gointernal/core/services/security_group_test.gointernal/repositories/postgres/instance_repo_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/security_group_repo_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/repositories/**/*.go : Repositories (`internal/repositories/`) must implement Ports interfaces
Applied to files:
internal/repositories/postgres/shared_test.gointernal/repositories/postgres/vpc_repo_unit_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*.go : Do not use global variables (e.g., `var DB *sql.DB`)
Applied to files:
internal/repositories/postgres/shared_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/core/services/*.go : Services (`internal/core/services/`) must import Ports and Domain only, not Repositories directly
Applied to files:
internal/core/services/security_group.gointernal/core/services/security_group_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*_test.go : Use `testify/mock` for creating mock objects in tests
Applied to files:
tests/multitenancy_e2e_test.gointernal/core/services/security_group_test.gointernal/repositories/postgres/subnet_repo_test.gointernal/repositories/postgres/security_group_repo_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/core/services/**/*.go : Use `internal/errors` package for domain errors
Applied to files:
internal/core/services/security_group_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/core/ports/*.go : Port repository interfaces must include CRUD methods (Create, GetByID, Update, Delete) with context as first parameter
Applied to files:
internal/repositories/postgres/subnet_repo_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*.go : Do not use silent failures - avoid blank identifier assignment like `_ = someFunc()`
Applied to files:
internal/repositories/ovs/adapter_test.go
🧬 Code graph analysis (9)
pkg/sdk/vpc_test.go (2)
pkg/testutil/constants.go (2)
TestContentTypeAppJSON(37-37)TestCIDR(9-9)pkg/sdk/vpc.go (1)
VPC(8-17)
internal/handlers/security_group_handler.go (1)
pkg/httputil/response.go (1)
Success(23-34)
internal/repositories/postgres/security_group_repo.go (3)
internal/core/ports/security_group.go (1)
SecurityGroupRepository(11-37)internal/core/context/context.go (1)
UserIDFromContext(22-28)internal/errors/errors.go (4)
New(58-60)NotFound(11-11)Wrap(62-64)Internal(13-13)
internal/core/services/security_group.go (3)
internal/core/ports/security_group.go (1)
SecurityGroupService(40-59)internal/errors/errors.go (1)
Wrap(62-64)internal/core/context/context.go (1)
UserIDFromContext(22-28)
internal/repositories/postgres/vpc_repo_unit_test.go (4)
pkg/testutil/constants.go (1)
TestCIDR(9-9)internal/errors/errors.go (1)
New(58-60)internal/repositories/postgres/vpc_repo.go (1)
NewVpcRepository(19-21)internal/core/context/context.go (1)
WithUserID(17-19)
pkg/sdk/security_group_test.go (1)
pkg/testutil/constants.go (2)
TestContentTypeAppJSON(37-37)TestAnyCIDR(11-11)
internal/core/services/security_group_test.go (3)
internal/core/context/context.go (1)
WithUserID(17-19)pkg/sdk/security_group.go (2)
SecurityGroup(8-16)SecurityRule(18-26)pkg/testutil/constants.go (3)
TestAnyCIDR(11-11)TestGoogleDNSCIDR(12-12)TestTenCIDR(13-13)
internal/repositories/postgres/instance_repo_test.go (4)
internal/core/domain/image.go (1)
Image(24-38)internal/core/domain/instance.go (1)
StatusRunning(25-25)pkg/testutil/constants.go (1)
TestIPHost(7-7)internal/errors/errors.go (2)
New(58-60)Error(39-44)
internal/repositories/ovs/adapter_test.go (4)
pkg/testutil/constants.go (1)
TestIPHost(7-7)internal/repositories/ovs/adapter_unit_test.go (3)
TestOvsAdapterType(125-128)TestOvsAdapterAddFlowRuleInvalidBridge(84-91)TestOvsAdapterListFlowRules(216-223)internal/repositories/ovs/adapter.go (1)
NewOvsAdapter(45-63)internal/core/ports/network.go (1)
FlowRule(8-13)
🪛 GitHub Actions: Coverage Gate
tests/multitenancy_e2e_test.go
[error] 51-51: TestMultiTenancyE2E failed: server not ready. Server must be running at http://localhost:8080
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: SonarCloud Scan
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: test-libvirt
- GitHub Check: OpenAPI Validation
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: SonarCloud Scan
- GitHub Check: test-libvirt
🔇 Additional comments (47)
cmd/cloud/sg.go (6)
18-24: LGTM!Good use of constants to centralize flag names, descriptions, and message formats. This improves maintainability and consistency across commands.
137-149: LGTM!Clean implementation following the established command patterns with consistent error formatting.
151-163: LGTM!Consistent implementation with proper argument handling and message formatting.
165-208: Good implementation with proper ID truncation.The
truncateIDhelper is correctly used for rule IDs at line 198, addressing the panic risk identified in previous reviews. The command provides a clear, readable output of security group details.
210-222: LGTM!Straightforward delete implementation with consistent error handling.
241-246: LGTM!Clean helper function that prevents slice-bounds panics. Ensure it's used consistently wherever IDs are truncated for display (see earlier comment about lines 80, 82).
pkg/sdk/shared_test.go (1)
3-13: LGTM! Test constants are well-organized.The acronym casing issue from the previous review has been addressed (
testAPIKey). These centralized constants improve test maintainability by eliminating magic strings.pkg/testutil/constants.go (3)
1-2: LGTM! Package documentation has been added.The package comment issue from the previous review has been addressed.
4-28: Network constants are well-organized.Good use of semantic naming for test constants. Minor note:
TestVXLANRemoteIP(line 27) andTestIPPrivate(line 8) share the same value"192.168.1.1", which is fine since they represent different semantic test contexts.
30-44: Auth-related constants look good.Constants are appropriately named and organized. The test database URLs with embedded credentials are acceptable for a test utility package.
pkg/sdk/vpc_test.go (3)
13-37: LGTM! Test refactored to use centralized constants.The test function naming now follows Go conventions (no underscores), and the use of
testutil.TestContentTypeAppJSONandtestAPIKeyimproves maintainability.
39-66: Test correctly validates VPC creation with centralized constants.Good use of
testVpcNameandtestutil.TestCIDRfor consistent test data.
68-95: LGTM! Get and Delete VPC tests are well-structured.Tests follow the established pattern with proper assertions.
tests/multitenancy_e2e_test.go (2)
49-96: LGTM! E2E test structure is well-designed.The test correctly validates multi-tenancy isolation with proper use of centralized constants. The
defer func() { _ = resp.Body.Close() }()pattern properly handles the linter requirement from the previous review.
109-133: Previous review feedback addressed.The
_ = resp.Body.Close()pattern on lines 115 and 124 correctly satisfies the errcheck linter while acknowledging the intentionally ignored return value.pkg/sdk/security_group_test.go (3)
14-46: LGTM! CreateSecurityGroup test is well-structured.Good use of centralized constants (
testSgName,sgBasePath,testAPIKey) and proper request/response validation.
48-71: LGTM! ListSecurityGroups test correctly validates query parameters.The test properly verifies the VPC ID query parameter filtering.
73-113: LGTM! Get and Delete tests follow consistent patterns.Good use of
sg123andsgDetailPathconstants for path construction.internal/repositories/ovs/adapter_test.go (2)
18-23: Good use of constants for test messaging.Extracting test-specific strings into constants improves maintainability and enables consistent error messages across tests.
162-176: Test assumes non-existent bridge returns empty rules.
TestOvsAdapterListFlowRulescallsListFlowRuleson a bridge named "bridge" which likely doesn't exist. The test expects empty rules rather than an error. Verify this is the intended behavior of the adapter when querying a non-existent bridge.internal/repositories/postgres/shared_test.go (1)
1-6: LGTM!Clean shared test constants file. The
testDBErrornaming now correctly follows Go conventions for initialisms (DB vs Db). These constants will help standardize error scenarios across Postgres repository tests.internal/core/services/security_group_test.go (3)
29-30: Good extraction of test constant.Using
testSGNameconstant improves test readability and maintainability.
169-192: Comprehensive test forRemoveRule.The test properly:
- Sets up user context with ownership
- Mocks the complete call chain (GetRuleByID → GetByID → GetByID for VPC → DeleteFlowRule → DeleteRule)
- Verifies audit logging
220-243: LGTM - Good coverage of flow translation scenarios.Tests UDP, ICMP, and TCP with port ranges using centralized CIDR constants from testutil. The audit mock using
mock.Anythingfor all parameters is appropriate here since the focus is on flow rule generation.internal/handlers/security_group_handler.go (5)
21-30: Well-structured request types.
CreateSecurityGroupRequestandAttachDetachSGRequestprovide clear, documented input shapes with appropriatebinding:"required"tags. ReusingAttachDetachSGRequestfor both attach and detach operations reduces duplication.
124-149: LGTM - Clean delete handler.Proper UUID validation, error handling via
httputil.Error, and correct 204 No Content response for successful deletion.
151-185: LGTM - AddRule handler well-implemented.Validates group ID, binds request body, and returns 201 Created with the created rule. Swagger documentation is complete.
242-268: LGTM - Detach handler mirrors Attach structure.Consistent implementation with the Attach handler, reusing
AttachDetachSGRequestand following the same validation/response patterns.
215-240: Swagger annotation matches the actual router configuration.The actual route is registered as
DELETE /security-groups/rules/:rule_id(line 255 ofinternal/api/setup/router.go), which corresponds to the Swagger annotation@Router /security-groups/rules/{rule_id} [delete]. The parameter syntax differs between frameworks (Gin uses:rule_id, Swagger uses{rule_id}), but they represent the same route.internal/repositories/postgres/security_group_repo.go (1)
117-133: Ownership check added toGetRuleByID- good security improvement.The JOIN with
security_groupsensures users can only retrieve rules they own. However, the error handling has an issue:scanSecurityRule(lines 253-261) returns errors without checking forpgx.ErrNoRows, so the check at line 127 will never be true.The
scanSecurityRulehelper doesn't handlepgx.ErrNoRowslikescanSecurityGroupdoes (lines 80-90). Either:
- Handle
ErrNoRowsinscanSecurityRule, or- Check the error before calling
scanSecurityRuleOption 1: Update scanSecurityRule
func (r *SecurityGroupRepository) scanSecurityRule(row pgx.Row) (domain.SecurityRule, error) { var rule domain.SecurityRule var direction string if err := row.Scan(&rule.ID, &rule.GroupID, &direction, &rule.Protocol, &rule.PortMin, &rule.PortMax, &rule.CIDR, &rule.Priority, &rule.CreatedAt); err != nil { + if err == pgx.ErrNoRows { + return domain.SecurityRule{}, err + } return domain.SecurityRule{}, err } rule.Direction = domain.RuleDirection(direction) return rule, nil }Then in GetRuleByID:
func (r *SecurityGroupRepository) GetRuleByID(ctx context.Context, ruleID uuid.UUID) (*domain.SecurityRule, error) { // ... rule, err := r.scanSecurityRule(r.db.QueryRow(ctx, query, ruleID, userID)) if err != nil { - if err == pgx.ErrNoRows { + if errors.Is(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 &rule, nil }⛔ Skipped due to learnings
Learnt from: CR Repo: PoyrazK/thecloud PR: 0 File: GEMINI.md:0-0 Timestamp: 2026-01-14T11:33:57.670Z Learning: Applies to internal/core/ports/*.go : Port repository interfaces must include CRUD methods (Create, GetByID, Update, Delete) with context as first parameterinternal/core/services/security_group.go (1)
244-250: Good: DetachFromInstance now includes OVS flow cleanup.The
removeGroupFlowscall addresses the concern from the previous review about asymmetric behavior between attach and detach operations.docs/swagger/docs.go (4)
2058-2155: LGTM: Security groups list and create endpoints are well-documented.The GET and POST endpoints for
/security-groupsinclude proper security annotations, parameter definitions, and response schemas. The requiredvpc_idquery parameter for listing and the request body schema for creation are appropriately defined.
2258-2299: LGTM: Remove rule endpoint uses 204 No Content.The DELETE
/security-groups/rules/{rule_id}endpoint correctly returns 204 for successful deletion, which is consistent with RESTful conventions and other delete endpoints in this API.
2352-2392: LGTM: Security group delete endpoint uses 204 No Content.This addresses the past review comment about aligning with other delete endpoints. The response is now consistent with the API convention.
3854-3864: LGTM: Domain types for security groups are properly defined.The
RuleDirectionenum withingress/egressvalues,SecurityGroup, andSecurityRuledefinitions include appropriate properties and documentation. The x-enum-varnames provide useful Go constant mappings.Also applies to: 3986-4053
internal/repositories/postgres/subnet_repo_test.go (2)
19-24: LGTM: Test constants improve maintainability.The introduction of
testSubnetName,testAZ,selectSubnet, anddeleteSubnetQueryconstants reduces duplication and makes tests more maintainable. The SQL patterns are properly escaped for regex matching with pgxmock.
26-70: LGTM: Create tests use centralized test utilities.The tests properly use
testutil.TestSubnetCIDRandtestutil.TestGatewayIPinstead of hardcoded values, and reference the sharedtestDBErrorconstant for error scenarios.internal/repositories/postgres/instance_repo_test.go (3)
19-23: LGTM: Test constants centralize test data.The
testInstanceName,testInstanceImg, andselectQueryconstants improve test maintainability and consistency across test cases.
25-71: LGTM: Instance create tests are well-structured.The tests use centralized constants, testutil values, and properly mock database interactions. The
UpdatedAtfield is now included in the test setup.
231-279: LGTM: Update tests verify optimistic locking.The tests properly verify both successful updates with version increment and concurrency conflict scenarios when no rows are affected.
internal/repositories/postgres/vpc_repo_unit_test.go (3)
19-22: LGTM: Test constants follow established pattern.The
testVpcNameandselectVpcconstants align with the test refactoring approach used in other repository tests.
92-117: Good documentation in not-found test case.The inline comments explaining the expected error wrapping behavior (
errors.New(errors.NotFound, ...)) help future maintainers understand the test intent.
222-241: LGTM: Scan error test validates row parsing.Testing with an invalid UUID value properly validates the repository's error handling when row scanning fails.
docs/swagger/swagger.json (1)
3843-4042: New SG schemas look consistent and reusable.
domain.RuleDirection,domain.SecurityGroup,domain.SecurityRule, and the handler request models are wired consistently across endpoints.Also applies to: 4376-4390, 4527-4544
docs/swagger/swagger.yaml (1)
483-623: Definitions are coherent and match the new endpoints.The new enum + models and the attach/detach/create request objects line up cleanly with the
/security-groupspaths.Also applies to: 858-867, 961-972
internal/repositories/postgres/security_group_repo_test.go (2)
19-25: Nice: centralized SQL snippets and test constants.This should reduce drift across tests and make query expectations more consistent.
301-346: The test assertion is correct and does not need modification.The
errors.New()anderrors.Wrap()functions ininternal/errors/errors.goboth returnErroras a value type, not a pointer (*Error). The Error struct has only value receivers (e.g.,func (e Error) Error() string). Therefore, the type assertionerr.(theclouderrors.Error)in the test will work correctly and will not panic.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "/security-groups/{id}": { | ||
| "get": { | ||
| "security": [ | ||
| { | ||
| "APIKeyAuth": [] | ||
| } | ||
| ], | ||
| "description": "Gets details of a specific security group", | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "security-groups" | ||
| ], | ||
| "summary": "Get security group", | ||
| "parameters": [ | ||
| { | ||
| "type": "string", | ||
| "description": "Security Group ID", | ||
| "name": "id", | ||
| "in": "path", | ||
| "required": true | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "VPC ID (optional if ID is UUID)", | ||
| "name": "vpc_id", | ||
| "in": "query" | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "schema": { | ||
| "$ref": "#/definitions/domain.SecurityGroup" | ||
| } | ||
| }, | ||
| "404": { | ||
| "description": "Not Found", | ||
| "schema": { | ||
| "$ref": "#/definitions/httputil.Response" | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal Server Error", | ||
| "schema": { | ||
| "$ref": "#/definitions/httputil.Response" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "delete": { | ||
| "security": [ | ||
| { | ||
| "APIKeyAuth": [] | ||
| } | ||
| ], | ||
| "description": "Deletes a security group", | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "security-groups" | ||
| ], | ||
| "summary": "Delete security group", | ||
| "parameters": [ | ||
| { | ||
| "type": "string", | ||
| "description": "Security Group ID", | ||
| "name": "id", | ||
| "in": "path", | ||
| "required": true | ||
| } | ||
| ], | ||
| "responses": { | ||
| "204": { | ||
| "description": "No Content" | ||
| }, | ||
| "404": { | ||
| "description": "Not Found", | ||
| "schema": { | ||
| "$ref": "#/definitions/httputil.Response" | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal Server Error", | ||
| "schema": { | ||
| "$ref": "#/definitions/httputil.Response" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Clarify GET /security-groups/{id} lookup semantics (optional vpc_id).
The description “VPC ID (optional if ID is UUID)” is hard to interpret in OpenAPI terms; if {id} supports both UUID and name, consider documenting the exact precedence/rules (or splitting endpoints).
🤖 Prompt for AI Agents
In `@docs/swagger/swagger.json` around lines 2290 - 2382, The GET
/security-groups/{id} operation currently documents the query parameter "vpc_id"
as "optional if ID is UUID", which is ambiguous; update the OpenAPI spec for the
GET /security-groups/{id} path to clearly state the lookup semantics: explain
whether {id} can be either UUID or name, the precedence when both {id} and
vpc_id are provided, and whether vpc_id is required for non-UUID names (or split
into separate endpoints if different behavior); modify the "description" for the
"vpc_id" parameter (and optionally the overall operation "description" or add
examples) to explicitly document the resolution rules so clients know when to
include vpc_id and how conflicts are handled.
| get: | ||
| description: Gets details of a specific security group | ||
| parameters: | ||
| - description: Security Group ID | ||
| in: path | ||
| name: id | ||
| required: true | ||
| type: string | ||
| - description: VPC ID (optional if ID is UUID) | ||
| in: query | ||
| name: vpc_id | ||
| type: string | ||
| produces: | ||
| - application/json | ||
| responses: | ||
| "200": | ||
| description: OK | ||
| schema: | ||
| $ref: '#/definitions/domain.SecurityGroup' | ||
| "404": | ||
| description: Not Found | ||
| schema: | ||
| $ref: '#/definitions/httputil.Response' | ||
| "500": | ||
| description: Internal Server Error | ||
| schema: | ||
| $ref: '#/definitions/httputil.Response' | ||
| security: | ||
| - APIKeyAuth: [] | ||
| summary: Get security group | ||
| tags: | ||
| - security-groups |
There was a problem hiding this comment.
Document the {id} vs vpc_id behavior more explicitly.
If {id} can be UUID or name, spell out precedence (e.g., “treat as UUID if parse succeeds; else treat as name and require vpc_id”) or split into separate endpoints.
🤖 Prompt for AI Agents
In `@docs/swagger/swagger.yaml` around lines 2525 - 2556, The current GET
operation (summary: "Get security group") ambiguously describes the path
parameter "id" vs query parameter "vpc_id"; update the documentation for this
operation to explicitly state the resolution rules (e.g., in the description:
"If id parses as a UUID, it is treated as the security group ID; otherwise id is
treated as a name and vpc_id is required to disambiguate") or alternatively add
two distinct endpoints (one /security-groups/{id} for UUIDs and one
/security-groups/by-name with query params name and vpc_id). Make the chosen
precedence rule or endpoint split explicit in the operation description and
parameter docs for "id" and "vpc_id" so clients know whether vpc_id is optional
or mandatory when passing a name.
| func TestSecurityGroupServiceDetachFromInstance(t *testing.T) { | ||
| repo, vpcRepo, _, auditSvc, svc := setupSecurityGroupServiceTest(t) | ||
| defer repo.AssertExpectations(t) | ||
|
|
||
| ctx := context.Background() | ||
| instID := uuid.New() | ||
| sgID := uuid.New() | ||
| vpcID := uuid.New() | ||
| sg := &domain.SecurityGroup{ID: sgID, VPCID: vpcID} | ||
| vpc := &domain.VPC{ID: vpcID, NetworkID: "net1"} | ||
|
|
||
| repo.On("RemoveInstanceFromGroup", mock.Anything, instID, sgID).Return(nil) | ||
| repo.On("GetByID", mock.Anything, sgID).Return(sg, nil) | ||
| vpcRepo.On("GetByID", mock.Anything, vpcID).Return(vpc, nil) | ||
|
|
||
| repo.On("RemoveInstanceFromGroup", ctx, instID, sgID).Return(nil) | ||
| auditSvc.On("Log", mock.Anything, mock.Anything, "security_group.detach", "instance", instID.String(), mock.Anything).Return(nil) | ||
|
|
||
| err := svc.DetachFromInstance(ctx, instID, sgID) | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the DetachFromInstance implementation
rg -nA 20 'func.*DetachFromInstance' internal/core/services/security_group.goRepository: PoyrazK/thecloud
Length of output: 808
🏁 Script executed:
# Check the removeGroupFlows implementation to see what network calls it makes
rg -nA 15 'func.*removeGroupFlows' internal/core/services/security_group.goRepository: PoyrazK/thecloud
Length of output: 558
🏁 Script executed:
# Also check the AttachToInstance implementation for comparison
rg -nA 20 'func.*AttachToInstance' internal/core/services/security_group.goRepository: PoyrazK/thecloud
Length of output: 733
🏁 Script executed:
# Check the AttachToInstance test to see what network calls it mocks
rg -nB 5 -A 20 'TestSecurityGroupServiceAttachToInstance' internal/core/services/security_group_test.goRepository: PoyrazK/thecloud
Length of output: 1092
Add mock expectation for network flow rule deletion in TestSecurityGroupServiceDetachFromInstance
The DetachFromInstance implementation calls s.removeGroupFlows() which invokes s.network.DeleteFlowRule() for each rule in the security group. The test should mock this expectation to align with how TestSecurityGroupServiceAttachToInstance mocks network.AddFlowRule:
network.On("DeleteFlowRule", mock.Anything, "net1", mock.Anything).Return(nil)
🤖 Prompt for AI Agents
In `@internal/core/services/security_group_test.go` around lines 148 - 167,
TestSecurityGroupServiceDetachFromInstance is missing a mock for the network
flow deletion called by DetachFromInstance -> removeGroupFlows; add a network
mock expectation similar to the attach test such as expecting
network.DeleteFlowRule with the VPC network ID ("net1") and any rule payload and
returning nil so the test aligns with the actual call path (refer to symbols:
TestSecurityGroupServiceDetachFromInstance, DetachFromInstance,
removeGroupFlows, network.DeleteFlowRule).
| func (r *SecurityGroupRepository) DeleteRule(ctx context.Context, ruleID uuid.UUID) error { | ||
| query := `DELETE FROM security_rules WHERE id = $1` | ||
| _, err := r.db.Exec(ctx, query, ruleID) | ||
| return err | ||
| userID := appcontext.UserIDFromContext(ctx) | ||
| query := ` | ||
| DELETE FROM security_rules sr | ||
| USING security_groups sg | ||
| WHERE sr.group_id = sg.id AND sr.id = $1 AND sg.user_id = $2 | ||
| ` | ||
| res, err := r.db.Exec(ctx, query, ruleID, userID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if res.RowsAffected() == 0 { | ||
| return errors.New(errors.NotFound, "security rule not found or you do not own it") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Wrap error in DeleteRule for consistency.
Line 144 returns the raw error without wrapping, unlike other repository methods that use errors.Wrap. This inconsistency could affect error handling upstream.
Proposed fix
res, err := r.db.Exec(ctx, query, ruleID, userID)
if err != nil {
- return err
+ return errors.Wrap(errors.Internal, "failed to delete security rule", 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.
| func (r *SecurityGroupRepository) DeleteRule(ctx context.Context, ruleID uuid.UUID) error { | |
| query := `DELETE FROM security_rules WHERE id = $1` | |
| _, err := r.db.Exec(ctx, query, ruleID) | |
| return err | |
| userID := appcontext.UserIDFromContext(ctx) | |
| query := ` | |
| DELETE FROM security_rules sr | |
| USING security_groups sg | |
| WHERE sr.group_id = sg.id AND sr.id = $1 AND sg.user_id = $2 | |
| ` | |
| res, err := r.db.Exec(ctx, query, ruleID, userID) | |
| if err != nil { | |
| return err | |
| } | |
| if res.RowsAffected() == 0 { | |
| return errors.New(errors.NotFound, "security rule not found or you do not own it") | |
| } | |
| return nil | |
| } | |
| func (r *SecurityGroupRepository) DeleteRule(ctx context.Context, ruleID uuid.UUID) error { | |
| userID := appcontext.UserIDFromContext(ctx) | |
| query := ` | |
| DELETE FROM security_rules sr | |
| USING security_groups sg | |
| WHERE sr.group_id = sg.id AND sr.id = $1 AND sg.user_id = $2 | |
| ` | |
| res, err := r.db.Exec(ctx, query, ruleID, userID) | |
| if err != nil { | |
| return errors.Wrap(errors.Internal, "failed to delete security rule", err) | |
| } | |
| if res.RowsAffected() == 0 { | |
| return errors.New(errors.NotFound, "security rule not found or you do not own it") | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/repositories/postgres/security_group_repo.go` around lines 135 -
150, In DeleteRule, don’t return the raw error from r.db.Exec; wrap it
consistently using errors.Wrap (like other repo methods) so upstream gets the
wrapped context. Replace the direct return of err after r.db.Exec(ctx, query,
ruleID, userID) with a wrapped error (e.g., return errors.Wrap(err, "failed to
delete security rule") ), keeping the existing NotFound handling for
res.RowsAffected().
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 66 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
internal/handlers/auth_handler_test.go:1
- Missing constant definition for 'contentType' and 'applicationJSON' which are used in the test file. These should be added to the const block or imported from a shared test constants file to avoid undefined variable errors.
package httphandlers
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const ( | ||
| testSubnetName = "test-subnet" | ||
| vpcsPathPrefix = "/vpcs/" | ||
| subnetsPathPrefix = "/subnets/" | ||
| subnetsPath = "/subnets" | ||
| ) |
There was a problem hiding this comment.
Missing constant definitions for 'contentType' and 'applicationJSON' which are used in lines 86 and 126. These should be added to the const block to ensure all referenced constants are defined.
| const ( | ||
| testFileName = "test.txt" | ||
| storageBucketName = "b1" | ||
| storageObjectPath = "/storage/:bucket/:key" | ||
| storageBasePath = "/storage/" | ||
| ) |
There was a problem hiding this comment.
The constant 'storageBasePath' ends with a trailing slash but is used in string concatenation (e.g., line 97). Consider documenting this pattern or using path joining functions to avoid potential double-slash issues.
| } | ||
|
|
||
| httputil.Success(c, http.StatusOK, gin.H{"message": "security group deleted"}) | ||
| c.Status(http.StatusNoContent) |
There was a problem hiding this comment.
The Delete handler returns 204 No Content, but the RemoveRule handler (line 239) also returns 204. For consistency with other handlers in the codebase, consider whether a success message response would be more appropriate for Delete, similar to how Attach and Detach return success messages.
| for _, rule := range sg.Rules { | ||
| flow := s.translateToFlow(rule) | ||
| if err := s.network.DeleteFlowRule(ctx, vpc.NetworkID, flow.Match); err != nil { | ||
| s.logger.Error("failed to delete flow rule", "rule", rule.ID, "error", err) | ||
| } | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The removeGroupFlows method logs errors but doesn't propagate them (always returns nil at line 290). Consider whether failures to delete individual flow rules should be accumulated and returned, or if the current behavior of best-effort cleanup with logging is intentional. Document this behavior if intentional.
| for _, rule := range sg.Rules { | |
| flow := s.translateToFlow(rule) | |
| if err := s.network.DeleteFlowRule(ctx, vpc.NetworkID, flow.Match); err != nil { | |
| s.logger.Error("failed to delete flow rule", "rule", rule.ID, "error", err) | |
| } | |
| } | |
| return nil | |
| var firstErr error | |
| for _, rule := range sg.Rules { | |
| flow := s.translateToFlow(rule) | |
| if err := s.network.DeleteFlowRule(ctx, vpc.NetworkID, flow.Match); err != nil { | |
| s.logger.Error("failed to delete flow rule", "rule", rule.ID, "error", err) | |
| if firstErr == nil { | |
| firstErr = err | |
| } | |
| } | |
| } | |
| return firstErr |
| func truncateID(id string, n int) string { | ||
| if len(id) <= n { | ||
| return id | ||
| } | ||
| return id[:n] | ||
| } |
There was a problem hiding this comment.
The truncateID helper function is defined in sg.go but could be useful in other CLI command files. Consider moving this to a shared utility file (e.g., cmd/cloud/util.go) for reuse across commands.
| func (r *SecurityGroupRepository) DeleteRule(ctx context.Context, ruleID uuid.UUID) error { | ||
| query := `DELETE FROM security_rules WHERE id = $1` | ||
| _, err := r.db.Exec(ctx, query, ruleID) | ||
| return err | ||
| userID := appcontext.UserIDFromContext(ctx) | ||
| query := ` | ||
| DELETE FROM security_rules sr | ||
| USING security_groups sg | ||
| WHERE sr.group_id = sg.id AND sr.id = $1 AND sg.user_id = $2 | ||
| ` | ||
| res, err := r.db.Exec(ctx, query, ruleID, userID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if res.RowsAffected() == 0 { | ||
| return errors.New(errors.NotFound, "security rule not found or you do not own it") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The error message 'security rule not found or you do not own it' combines two distinct error cases. Consider checking ownership separately to provide more specific error messages: 'security rule not found' vs 'access denied to security rule'.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/multitenancy_e2e_test.go`:
- Around line 182-186: The deleteInstance helper leaks the HTTP response body
because the result of client.Do(req) is ignored; change it to capture (resp,
err) := client.Do(req), handle the error appropriately (fail the test or log)
and always close the response body when resp != nil—e.g., drain to io.Discard
and call resp.Body.Close()—so update deleteInstance to use resp from client.Do
and ensure resp.Body is closed.
- Around line 101-103: The test currently calls resp.Body.Close() without
handling its error, causing an errcheck failure; update the success branch in
the test (the block that checks `err == nil && resp.StatusCode == 200`) to
explicitly ignore the close error like the other tests by replacing
`resp.Body.Close()` with `_ = resp.Body.Close()` (or alternatively check and
handle the returned error), ensuring the identifier `resp.Body.Close()` is the
one changed.
🧹 Nitpick comments (1)
tests/multitenancy_e2e_test.go (1)
21-22: Remove commented-out dead code.These commented lines serve no purpose since
testutil.TestBaseURLis now used throughout. Keeping commented code reduces clarity.🧹 Suggested fix
const instancesPathFmt = "%s/instances/%s" - -// baseURL is now usually defined by constant, but let's assume we use the one from testutil -// const baseURL = "http://localhost:8080"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/multitenancy_e2e_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (GEMINI.md)
**/*.go: Do not use global variables (e.g.,var DB *sql.DB)
Do not panic in production code - return errors instead
Do not use silent failures - avoid blank identifier assignment like_ = someFunc()
Do not use magic numbers - use named constants instead
Do not skipcontext.Contextas the first parameter in functions
Do not create circular dependencies between packages
Use constructor injection for dependencies instead of global initialization
Propagatecontext.Contextto all blocking calls
Use PascalCase for exported types and structs (e.g.,InstanceService,LaunchRequest)
Use PascalCase for exported interfaces (e.g.,InstanceRepository,ComputeBackend)
Use PascalCase for exported functions and camelCase for unexported functions
Use SCREAMING_SNAKE_CASE for status constants and PascalCase for limit constants (e.g.,StatusRunning,MaxPortsPerInstance)
Use camelCase for variable names (e.g.,instanceRepo,vpcID)
Includejsontags on struct fields withomitemptyfor optional fields
Files:
tests/multitenancy_e2e_test.go
**/*_test.go
📄 CodeRabbit inference engine (GEMINI.md)
**/*_test.go: Do not test with real external dependencies in unit tests - use mocks instead
Use table-driven tests in test files
Usetestify/mockfor creating mock objects in tests
Files:
tests/multitenancy_e2e_test.go
🧠 Learnings (5)
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*_test.go : Use table-driven tests in test files
Applied to files:
tests/multitenancy_e2e_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*_test.go : Do not test with real external dependencies in unit tests - use mocks instead
Applied to files:
tests/multitenancy_e2e_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/core/services/*.go : Do not skip tests for new services
Applied to files:
tests/multitenancy_e2e_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to **/*_test.go : Use `testify/mock` for creating mock objects in tests
Applied to files:
tests/multitenancy_e2e_test.go
📚 Learning: 2026-01-14T11:33:57.670Z
Learnt from: CR
Repo: PoyrazK/thecloud PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-01-14T11:33:57.670Z
Learning: Applies to internal/core/services/**/*_test.go : Mock repositories in service tests
Applied to files:
tests/multitenancy_e2e_test.go
🧬 Code graph analysis (1)
tests/multitenancy_e2e_test.go (2)
pkg/testutil/constants.go (4)
TestBaseURL(35-35)TestHeaderAPIKey(36-36)TestPasswordStrong(31-31)TestContentTypeAppJSON(37-37)internal/handlers/auth_handler.go (2)
RegisterRequest(25-29)LoginRequest(31-34)
🪛 GitHub Check: test
tests/multitenancy_e2e_test.go
[failure] 102-102:
Error return value of resp.Body.Close is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: SonarCloud Scan
- GitHub Check: Analyze (go)
- GitHub Check: test-libvirt
- GitHub Check: OpenAPI Validation
- GitHub Check: Analyze (go)
- GitHub Check: SonarCloud Scan
- GitHub Check: test-libvirt
🔇 Additional comments (4)
tests/multitenancy_e2e_test.go (4)
49-96: LGTM!The test function is well-structured with clear sub-tests for different multi-tenancy scenarios. The use of
testutilconstants improves maintainability, and the defer pattern for closing response bodies is correct.
110-134: LGTM!The function properly handles registration failures gracefully (allowing re-runs) and correctly uses the
_ = resp.Body.Close()pattern for error acknowledgment. The testutil constants are consistently applied.
136-162: LGTM!The function correctly handles both synchronous (201) and asynchronous (202) instance creation responses, aligning with the PR objectives. The testutil constants are consistently applied.
164-180: LGTM!Clean implementation with proper error handling and resource cleanup.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func deleteInstance(t *testing.T, client *http.Client, token, id string) { | ||
| req, _ := http.NewRequest("DELETE", fmt.Sprintf("%s/instances/%s", baseURL, id), nil) | ||
| req.Header.Set("X-API-Key", token) | ||
| client.Do(req) | ||
| req, _ := http.NewRequest("DELETE", fmt.Sprintf(instancesPathFmt, testutil.TestBaseURL, id), nil) | ||
| req.Header.Set(testutil.TestHeaderAPIKey, token) | ||
| _, _ = client.Do(req) | ||
| } |
There was a problem hiding this comment.
Potential resource leak: response body not closed.
When client.Do(req) succeeds, the response body is never closed. While this is cleanup code in a test, it's good practice to close response bodies to avoid resource leaks, especially if tests run repeatedly.
🔧 Suggested fix
func deleteInstance(t *testing.T, client *http.Client, token, id string) {
req, _ := http.NewRequest("DELETE", fmt.Sprintf(instancesPathFmt, testutil.TestBaseURL, id), nil)
req.Header.Set(testutil.TestHeaderAPIKey, token)
- _, _ = client.Do(req)
+ resp, err := client.Do(req)
+ if err == nil && resp != nil {
+ _ = resp.Body.Close()
+ }
}📝 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.
| func deleteInstance(t *testing.T, client *http.Client, token, id string) { | |
| req, _ := http.NewRequest("DELETE", fmt.Sprintf("%s/instances/%s", baseURL, id), nil) | |
| req.Header.Set("X-API-Key", token) | |
| client.Do(req) | |
| req, _ := http.NewRequest("DELETE", fmt.Sprintf(instancesPathFmt, testutil.TestBaseURL, id), nil) | |
| req.Header.Set(testutil.TestHeaderAPIKey, token) | |
| _, _ = client.Do(req) | |
| } | |
| func deleteInstance(t *testing.T, client *http.Client, token, id string) { | |
| req, _ := http.NewRequest("DELETE", fmt.Sprintf(instancesPathFmt, testutil.TestBaseURL, id), nil) | |
| req.Header.Set(testutil.TestHeaderAPIKey, token) | |
| resp, err := client.Do(req) | |
| if err == nil && resp != nil { | |
| _ = resp.Body.Close() | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@tests/multitenancy_e2e_test.go` around lines 182 - 186, The deleteInstance
helper leaks the HTTP response body because the result of client.Do(req) is
ignored; change it to capture (resp, err) := client.Do(req), handle the error
appropriately (fail the test or log) and always close the response body when
resp != nil—e.g., drain to io.Discard and call resp.Body.Close()—so update
deleteInstance to use resp from client.Do and ensure resp.Body is closed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 66 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const ( | ||
| testEmail = "test@example.com" | ||
| testPassword = "password123" | ||
| testName = "Test User" |
There was a problem hiding this comment.
The constant testName is ambiguous and could refer to various types of names. Consider renaming to testUserName to make it clear this is a user's name.
| const testFileName = "test.txt" | ||
| const ( | ||
| testFileName = "test.txt" | ||
| storageBucketName = "b1" |
There was a problem hiding this comment.
The constant storageBucketName has a very short value 'b1' which is not descriptive. Consider using a more descriptive value like 'test-bucket' to improve test readability.
| testContentType = "Content-Type" | ||
| testAppJSON = "application/json" |
There was a problem hiding this comment.
These constants duplicate the same values defined in pkg/testutil/constants.go as TestContentTypeAppJSON. Consider using the shared constants from testutil instead of defining local duplicates.
|
Overview
This PR completes the implementation of the Security Groups feature. Security groups act as virtual firewalls for compute instances, controlling inbound and outbound traffic at the network level using Open vSwitch flow rules.
Key Changes
RemoveRule,Attach, andDetachinSecurityGroupService.sgcommand suite with support forcreate,list,get,delete,add-rule,remove-rule,attach, anddetach./docs.Verification
make testpasses globally.make swaggeroutput verified and synced.contexthandling across all instrumented layers.Summary by CodeRabbit
New Features
CLI
Documentation
Other
✏️ Tip: You can customize this high-level summary in your review settings.