feat: Elastic IP support and documentation improvements#55
Conversation
fix(ci): fix E2E tests and docker network isolation
📝 WalkthroughWalkthroughAdds Elastic IP management (domain, repo, service, handlers, migrations, tests, docs, and Swagger), introduces Docker network configurability for runtime/tests/CI via DOCKER_DEFAULT_NETWORK / TEST_DOCKER_NETWORK, and updates CI/E2E workflows and docker-compose to use those variables. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as ElasticIP Handler
participant Service as ElasticIP Service
participant EIPRepo as ElasticIP Repo
participant InstRepo as Instance Repo
participant Audit as Audit Service
participant DB as Database
rect rgba(200, 150, 255, 0.5)
Note over Client,DB: Allocate Elastic IP
Client->>Handler: POST /elastic-ips
Handler->>Service: AllocateIP(ctx)
Service->>Service: generateDeterministicIP(uuid)
Service->>EIPRepo: Create(elasticIP)
EIPRepo->>DB: INSERT INTO elastic_ips
DB-->>EIPRepo: OK
Service->>Audit: LogAction("allocate_ip")
Audit-->>Service: OK
Service-->>Handler: ElasticIP
Handler-->>Client: 201 Created
end
rect rgba(150, 200, 255, 0.5)
Note over Client,DB: Associate Elastic IP
Client->>Handler: POST /elastic-ips/:id/associate {instance_id}
Handler->>Service: AssociateIP(eipID, instanceID)
Service->>EIPRepo: GetByID(eipID)
EIPRepo->>DB: SELECT ...
DB-->>EIPRepo: ElasticIP
Service->>InstRepo: GetByID(instanceID)
InstRepo->>DB: SELECT ...
DB-->>InstRepo: Instance
Service->>EIPRepo: Update(elasticIP -> associated)
EIPRepo->>DB: UPDATE elastic_ips
DB-->>EIPRepo: OK
Service->>Audit: LogAction("associate_ip")
Audit-->>Service: OK
Service-->>Handler: ElasticIP
Handler-->>Client: 200 OK
end
rect rgba(255, 200, 150, 0.5)
Note over Client,DB: Release Elastic IP
Client->>Handler: DELETE /elastic-ips/:id
Handler->>Service: ReleaseIP(eipID)
Service->>EIPRepo: GetByID(eipID)
EIPRepo->>DB: SELECT ...
DB-->>EIPRepo: ElasticIP
Service->>EIPRepo: Delete(eipID)
EIPRepo->>DB: DELETE FROM elastic_ips
DB-->>EIPRepo: OK
Service->>Audit: LogAction("release_ip")
Audit-->>Service: OK
Service-->>Handler: success
Handler-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds Elastic IP (EIP) support across domain/service/repository/HTTP layers and updates test infrastructure + documentation to cover the new API and Docker network configurability.
Changes:
- Introduces EIP domain model, service logic (allocate/associate/disassociate/release), Postgres repository, migrations, and HTTP handlers/routes with RBAC permissions.
- Improves test configurability (env-driven URLs/ports/network), adds EIP E2E + unit/handler tests, and adjusts chaos/E2E workflows for docker compose projects.
- Updates docs (DB schema, architecture, features, API reference) and regenerates Swagger artifacts.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/helpers/helpers_test.go | Uses shared testutil.TestBaseURL instead of hardcoded localhost URL. |
| tests/elastic_ip_e2e_test.go | New end-to-end test for EIP allocate/list/get/associate/disassociate/release flows. |
| tests/chaos_test.go | Switches container operations to docker compose service-based commands. |
| pkg/testutil/container.go | Increases Postgres testcontainer startup timeout to reduce flakiness. |
| pkg/testutil/constants.go | Makes key test constants env-configurable (base URL, DB URL, ports, docker network). |
| internal/repositories/postgres/migrations/060_create_elastic_ips.up.sql | Adds elastic_ips table + indexes (EIP persistence layer). |
| internal/repositories/postgres/migrations/060_create_elastic_ips.down.sql | Adds down migration for EIP table/indexes. |
| internal/repositories/postgres/elastic_ip_repo.go | New Postgres repository implementing EIP CRUD + tenant-scoped queries. |
| internal/platform/config.go | Adds DockerDefaultNetwork config for docker backend network selection. |
| internal/handlers/elastic_ip_handler_test.go | Adds handler tests for allocate/list/associate endpoints. |
| internal/handlers/elastic_ip_handler.go | New HTTP handler implementing EIP endpoints. |
| internal/core/services/setup_test.go | Ensures elastic_ips is included in DB cleanup for service tests. |
| internal/core/services/instance_test.go | Uses env-configurable docker network + instance port selection for tests. |
| internal/core/services/instance.go | Supports configurable docker network fallback when resolving network config. |
| internal/core/services/elastic_ip_test.go | Adds service-level tests for EIP lifecycle behavior. |
| internal/core/services/elastic_ip.go | Implements core EIP business logic + audit logging. |
| internal/core/ports/elastic_ip.go | Defines ports/interfaces for EIP service and repository. |
| internal/core/domain/rbac.go | Adds RBAC permission constants for EIP operations. |
| internal/core/domain/elastic_ip.go | Adds EIP domain entity + status enum + validation. |
| internal/api/setup/router.go | Registers /elastic-ips routes with auth/tenant/RBAC middleware. |
| internal/api/setup/dependencies.go | Wires EIP repository/service and passes docker network config into InstanceService. |
| docs/vision.md | Mentions Elastic IP management in the product roadmap. |
| docs/swagger/swagger.yaml | Swagger spec update for EIP models/endpoints/permissions. |
| docs/swagger/swagger.json | Swagger JSON update for EIP models/endpoints/permissions. |
| docs/swagger/docs.go | Regenerated embedded swagger docs including EIP endpoints. |
| docs/database.md | Documents elastic_ips table schema. |
| docs/backend.md | Documents ElasticIPService and /elastic-ips endpoints at a high level. |
| docs/architecture.md | Adds ElasticIPService to the architecture overview. |
| docs/api-reference.md | Adds API reference section for Elastic IP endpoints. |
| docs/FEATURES.md | Expands networking section to include Elastic IPs. |
| docker-compose.yml | Removes fixed container names, makes ports configurable, propagates docker network name to API. |
| .github/workflows/e2e.yml | Uses unique compose project names + passes docker network into API/tests; adds failure log dump. |
| .github/workflows/ci.yml | Uses env-driven docker network creation and adjusts image tag publishing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // EIPStatusAssociated indicates the IP is currently mapped to a compute instance. | ||
| EIPStatusAssociated ElasticIPStatus = "associated" | ||
|
|
||
| // EIPStatusReleased indicates the IP has been returned to the pool (soft delete state). |
There was a problem hiding this comment.
EIPStatusReleased is documented as a “soft delete state”, but the current ReleaseIP logic hard-deletes the row. This makes the enum value/comment misleading (and exposes a status that can never occur). Either implement a released/soft-delete flow or remove/adjust this status.
| // EIPStatusReleased indicates the IP has been returned to the pool (soft delete state). | |
| // EIPStatusReleased indicates the IP has been released and is no longer allocated to a user. |
| // Allocate reserves a new Elastic IP. | ||
| // @Summary Allocate an Elastic IP | ||
| // @Tags elastic-ips | ||
| // @Security APIKeyAuth | ||
| // @Produce json | ||
| // @Success 201 {object} domain.ElasticIP | ||
| // @Failure 400 {object} httputil.Response | ||
| // @Failure 404 {object} httputil.Response | ||
| // @Failure 500 {object} httputil.Response | ||
| // @Router /elastic-ips [post] |
There was a problem hiding this comment.
These @Success annotations declare responses as domain.ElasticIP, but handlers call httputil.Success, which wraps payloads in httputil.Response{data=...}. Consider updating the annotations to httputil.Response{data=domain.ElasticIP} / ...{data=[]domain.ElasticIP} to keep the generated Swagger accurate (see AuthHandler for the pattern).
| user_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| tenant_id UUID NOT NULL REFERENCES tenants(id) ON DELETE CASCADE, | ||
| instance_id UUID REFERENCES instances(id) ON DELETE SET NULL, | ||
| vpc_id UUID REFERENCES vpcs(id) ON DELETE SET NULL, |
There was a problem hiding this comment.
This documented elastic_ips schema (notably the tenant_id / user_id FKs and delete behavior) doesn’t match the 060_create_elastic_ips migration added in this PR. Please align the docs and migration so operators don’t provision the wrong schema.
| user_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, | |
| tenant_id UUID NOT NULL REFERENCES tenants(id) ON DELETE CASCADE, | |
| instance_id UUID REFERENCES instances(id) ON DELETE SET NULL, | |
| vpc_id UUID REFERENCES vpcs(id) ON DELETE SET NULL, | |
| user_id UUID NOT NULL, | |
| tenant_id UUID NOT NULL, | |
| instance_id UUID, | |
| vpc_id UUID, |
| "id": "uuid", | ||
| "public_ip": "100.64.x.y", | ||
| "status": "allocated", | ||
| "arn": "arn:thecloud:vpc:local:tenant:eip/uuid" |
There was a problem hiding this comment.
The service generates EIP ARNs as arn:thecloud:vpc:local:<user-id>:eip/<eip-id>, but this example uses tenant in that position. Consider updating the example to match the actual ARN format.
| "arn": "arn:thecloud:vpc:local:tenant:eip/uuid" | |
| "arn": "arn:thecloud:vpc:local:<user-id>:eip/<eip-id>" |
| checkResp := getRequest(t, client, testutil.TestBaseURL+testutil.TestRouteInstances+"/"+instanceID, token) | ||
| var statusRes struct { | ||
| Data struct { | ||
| Status domain.InstanceStatus `json:"status"` | ||
| } `json:"data"` |
There was a problem hiding this comment.
The instance readiness polling loop ignores the GET response status code and the JSON decode error. If the instance endpoint returns an error/HTML or a non-200, the test may silently wait until timeout. Consider asserting checkResp.StatusCode and handling decode errors during polling for clearer failures.
| -- Optimization indexes | ||
| CREATE INDEX idx_elastic_ips_tenant_id ON elastic_ips(tenant_id); | ||
| CREATE INDEX idx_elastic_ips_instance_id ON elastic_ips(instance_id); |
There was a problem hiding this comment.
The migrator reruns all *.up.sql on every startup and only logs errors, so non-idempotent CREATE INDEX statements will repeatedly error after the first run. Consider using CREATE INDEX IF NOT EXISTS ... for the new elastic_ips indexes (including the partial unique index below).
| CREATE TABLE IF NOT EXISTS elastic_ips ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| user_id UUID NOT NULL REFERENCES users(id), | ||
| tenant_id UUID NOT NULL, |
There was a problem hiding this comment.
tenant_id is created without a FK, while other tenant-scoped tables typically use tenant_id UUID ... REFERENCES tenants(id) (e.g. later tenant migrations). If elastic_ips is tenant-scoped, consider adding a FK (likely via a follow-up migration after the tenants table exists, or by moving this migration later).
| tenant_id UUID NOT NULL, | |
| tenant_id UUID NOT NULL REFERENCES tenants(id), |
|
|
||
| // generateDeterministicIP creates a consistent "public" IP for a given UUID within the 100.64.0.0/10 range. | ||
| func (s *elasticIPService) generateDeterministicIP(u uuid.UUID) string { | ||
| // 100.64.0.0 + bytes 12-15 of UUID |
There was a problem hiding this comment.
The comment says “100.64.0.0 + bytes 12-15 of UUID”, but the implementation only uses bytes 12-14 and ignores byte 15, reducing the address space and making the comment inaccurate. Consider incorporating u[15] (and/or updating the comment) so the algorithm matches the stated intent.
| // 100.64.0.0 + bytes 12-15 of UUID | |
| // 100.64.0.0 + bytes 12-14 of UUID |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
322-326:⚠️ Potential issue | 🟡 MinorDeploy tags no longer include the Git ref name (e.g.,
v1.2.3).Both staging and production image pushes now tag with
:staging/:latestand:${{ github.sha }}only. For production releases (triggered byrefs/tags/v*), the semver tag (e.g.,ghcr.io/.../thecloud:v1.2.3) is no longer pushed. This makes it harder to pull a specific release version by its semantic version. Consider adding${{ github.ref_name }}back for tag-triggered builds.Proposed fix for production deploy
tags: | ghcr.io/${{ steps.repo_name.outputs.REPO_LC }}:latest ghcr.io/${{ steps.repo_name.outputs.REPO_LC }}:${{ github.sha }} + ghcr.io/${{ steps.repo_name.outputs.REPO_LC }}:${{ github.ref_name }}Also applies to: 358-362
🤖 Fix all issues with AI agents
In `@docker-compose.yml`:
- Line 109: The DOCKER_DEFAULT_NETWORK default value may not match the actual
Compose network name because the explicit network name ("name: cloud-network")
is commented out, so Compose prefixes the network with the project name; update
either the docker-compose network block to uncomment and set name: cloud-network
so the network is created with that exact name, or change the default for
DOCKER_DEFAULT_NETWORK to the prefixed form your project uses (e.g.,
<project>_cloud-network) so the runtime lookup that uses DOCKER_DEFAULT_NETWORK
succeeds; look for the DOCKER_DEFAULT_NETWORK env var reference and the network
block containing the commented "name: cloud-network" to make the corresponding
change.
In `@internal/core/services/elastic_ip.go`:
- Around line 18-32: Replace the current NewElasticIPService signature with a
constructor that accepts a single params struct (e.g., ElasticIPServiceParams)
containing the existing dependencies (repo ports.ElasticIPRepository,
instanceRepo ports.InstanceRepository, auditSvc ports.AuditService, logger
*slog.Logger), update the constructor body to initialize and return
&elasticIPService using those params, and update any callers to pass the params
struct instead of separate arguments; keep the returned type as
ports.ElasticIPService and retain the elasticIPService struct fields unchanged.
- Around line 183-187: The function generateDeterministicIP in elasticIPService
uses magic-number octets for the CGNAT range (100, 64, and the 64 mask offset
and byte indices 12–15); replace these literals with clearly named constants
(e.g., CGNAT_FIRST_OCTET, CGNAT_SECOND_OCTET_BASE, CGNAT_SECOND_OCTET_MASK,
UUID_IP_BYTE_1, UUID_IP_BYTE_2, UUID_IP_BYTE_3) and use those constants in the
IPv4 construction so the intent (100.64.0.0/10 and which UUID bytes are used) is
explicit and maintainable.
In `@internal/handlers/elastic_ip_handler_test.go`:
- Line 136: The test silently ignores errors from http.NewRequest (req, _ :=
http.NewRequest(...)); change this to capture the error (req, err :=
http.NewRequest(...)) and immediately assert success with require.NoError(t,
err) (or require.NoError(tt.t, err) if the table test stores a testing.T
reference) so the test fails fast on unexpected request construction failures.
In `@tests/chaos_test.go`:
- Around line 35-45: The docker compose calls in tests/chaos_test.go use
exec.Command("docker", "compose", ...) without specifying the compose file or
project context, which can mis-target containers when tests run from another
cwd; update the exec.Command invocations that stop/start Redis (the lines
creating cmd := exec.Command("docker", "compose", "stop", "redis") and the
deferred exec.Command("docker", "compose", "start", "redis").Run()) to pass an
explicit "-f", "<path-to-docker-compose.yml>" (or compute the repo/test
directory at runtime via os.Getwd()/runtime.Caller and build the path) and
include "-p" if you want a deterministic project name, ensuring both stop and
start use the same flags and paths and surface errors (use require.NoError or
handle err) rather than swallowing them.
In `@tests/elastic_ip_e2e_test.go`:
- Line 135: Replace the silent decode with proper error handling: capture the
error from json.NewDecoder(checkResp.Body).Decode(&statusRes) into a variable
(e.g., err), and when non-nil log it with context (including
checkResp.StatusCode and the response body if feasible) using the test logger
(t.Logf or t.Errorf) inside the polling loop so decoding failures are visible;
keep using checkResp and statusRes but do not ignore the error or assign it to
_.
🧹 Nitpick comments (12)
internal/core/services/setup_test.go (1)
117-119: Pre-existing: silent failures in cleanup loop.Line 118 discards both return values with
_, _, which the coding guidelines flag. If aDELETEfails (e.g., due to a missing table after a failed migration), tests will silently proceed with stale data, making failures hard to diagnose. Consider logging or failing on error.♻️ Suggested improvement
for _, table := range tables { - _, _ = db.Exec(ctx, "DELETE FROM "+table+" CASCADE") + _, err := db.Exec(ctx, "DELETE FROM "+table+" CASCADE") + if err != nil { + t.Logf("Warning: failed to clean table %s: %v", table, err) + } }As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()".internal/repositories/postgres/migrations/060_create_elastic_ips.up.sql (1)
14-21: The regular index oninstance_id(line 16) partially overlaps with the partial unique index (lines 19-21).The partial unique index already covers lookups for non-NULL
instance_idvalues. The regular index on line 16 adds value only for queries scanning NULLinstance_idrows (e.g., "find unassociated EIPs"). If that query pattern is expected, both indexes are justified; otherwise the regular index is redundant overhead on writes.docker-compose.yml (1)
10-10:DB_PORT_MAPPINGtakes the fullhost:containerpair as a single variable.This is unusual — most compose files use separate variables for the host port only (e.g.,
"${DB_HOST_PORT:-5433}:5432"). The current approach means a misconfigured value (e.g.,DB_PORT_MAPPING=5433) without the container port would silently break the mapping. Consider splitting this for clarity and safety.pkg/testutil/constants.go (2)
8-13:getEnvVarduplicatesgetEnvininternal/platform/config.go.Both helpers do the same thing (read env with fallback). Consider extracting a shared utility to avoid duplication, or at minimum note that
getEnvinconfig.gousesos.LookupEnv(distinguishes empty from unset) while this one usesos.Getenv(treats empty as unset). The behavioral difference means an explicitly set empty env var will be treated differently by each helper.
15-112: Switching fromconsttovarmakes all test constants mutable.This is necessary for the env-var–driven values, but the remaining ~40 plain string literals (e.g.,
TestIPLocalhost,TestCIDR) are now mutable too. Not a practical risk in tests, but if you want to preserve immutability for the static values, you could keep them in a separateconstblock.internal/core/services/instance_test.go (2)
81-91: Silent failures in test setup may hide configuration issues.Lines 84, 86-90 silently discard errors. While understandable for idempotent setup (network may already exist, pre-pull is best-effort), consider at minimum logging errors at
t.Logflevel so CI failures are diagnosable. As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like_ = someFunc()".
133-137: Consider exportinggetEnvVarfromtestutilor adding aTestInstancePortconstant.This manual env-var-with-fallback pattern mirrors
getEnvVarinpkg/testutil/constants.go, but that helper is unexported. Either export it or addTestInstancePortalongsideTestPortin the constants file for consistency.internal/handlers/elastic_ip_handler_test.go (1)
75-142: Table-driven test structure is undermined by name-based conditionals in the loop.Lines 126-128 and 131-133 use string comparisons against
tt.nameandtt.urlto conditionally build the URL and body, bypassing the table-driven data. Thebodyfield on line 109 is declared but never actually used. This couples the loop logic to specific test case names, making it fragile and hard to extend.Consider moving URL and body construction into the table (e.g., via a
buildRequestfunc field) so each case is self-contained.Also,
Get,Release, andDisassociatehandler methods are not covered.♻️ Suggested approach
tests := []struct { name string method string - url string - setupMock func(svc *mockElasticIPService, eipID, instID uuid.UUID) - body interface{} + buildURL func(eipID uuid.UUID) string + setupMock func(svc *mockElasticIPService, eipID, instID uuid.UUID) + buildBody func(instID uuid.UUID) []byte expectedStatus int }{ { name: "Allocate", method: "POST", - url: "/elastic-ips", + buildURL: func(_ uuid.UUID) string { return "/elastic-ips" }, setupMock: func(svc *mockElasticIPService, eipID, instID uuid.UUID) { svc.On("AllocateIP", mock.Anything).Return(&domain.ElasticIP{ID: eipID, PublicIP: "1.2.3.4"}, nil).Once() }, + buildBody: func(_ uuid.UUID) []byte { return nil }, expectedStatus: http.StatusCreated, },Then in the loop, simply call
url := tt.buildURL(eipID)andbody := tt.buildBody(instID).internal/core/services/elastic_ip_test.go (1)
20-33: Service tests use real Postgres repositories instead of mocks.This file uses
postgres.NewElasticIPRepository(db)and other real repositories. While these integration tests are valuable, the coding guidelines forinternal/core/services/**/*_test.gorequire mocked repositories in service tests. Consider either:
- Adding a separate unit test file with mocked repositories (e.g.,
elastic_ip_unit_test.go) for fast, isolated testing.- Moving these integration tests to a dedicated integration test directory.
As per coding guidelines, "Mock repositories in service tests".
tests/elastic_ip_e2e_test.go (1)
35-63: Sequential subtests sharing state — be aware of skip propagation.The subtests share
eipIDandpublicIPvia closure and depend on sequential execution order. IfAllocateIPis skipped or fails, subsequent subtests (ListIPs,GetIP, etc.) will operate on emptyeipID, leading to confusing 404 failures rather than clear skip signals. Consider adding a guard at the top of dependent subtests:if eipID == "" { t.Skip("skipping: no EIP allocated") }internal/core/domain/elastic_ip.go (1)
39-53: HardenValidateagainst unknown status values.Right now any non-empty status passes. Enforcing the known lifecycle states avoids persisting invalid values.
♻️ Proposed refactor
func (e *ElasticIP) Validate() error { if e.UserID == uuid.Nil { return errors.New("user ID is required") } if e.TenantID == uuid.Nil { return errors.New("tenant ID is required") } if e.PublicIP == "" { return errors.New("public IP address is required") } if e.Status == "" { return errors.New("status is required") } + switch e.Status { + case EIPStatusAllocated, EIPStatusAssociated, EIPStatusReleased: + // valid + default: + return errors.New("status is invalid") + } return nil }docs/swagger/swagger.yaml (1)
2887-2939: Questionable 404 responses on list and allocate endpoints.
GET /elastic-ips(list): A 404 response is unusual for a collection endpoint — an empty list with 200 is the standard REST convention.POST /elastic-ips(allocate): A 404 response doesn't make sense for resource creation/allocation.These likely come from the Swagger annotations on the Go handlers. Consider removing the 404 response declarations from the list and allocate handler annotations to align with REST conventions and with how other list/create endpoints in this spec are defined (e.g.,
GET /instances,POST /instances).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/chaos_test.go (1)
64-64:⚠️ Potential issue | 🟡 MinorSilent failure on
io.ReadAll.
body, _ := io.ReadAll(respH.Body)discards the error. Consider logging on failure for diagnostic visibility. As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like_ = someFunc()".Proposed fix
- body, _ := io.ReadAll(respH.Body) + body, readErr := io.ReadAll(respH.Body) + if readErr != nil { + t.Logf("failed to read health check body: %v", readErr) + }
🤖 Fix all issues with AI agents
In `@internal/core/services/elastic_ip.go`:
- Around line 207-211: Update the function comment in generateDeterministicIP to
correctly state which UUID bytes are used (bytes 12-14) to match the
implementation that builds the IPv4 address using UUID_IP_BYTE_1,
UUID_IP_BYTE_2, and UUID_IP_BYTE_3; do not change the logic—just correct the
docstring so it no longer says "bytes 12-15".
In `@tests/chaos_test.go`:
- Around line 45-51: The deferred cleanup uses require.NoError which calls
FailNow inside a defer — replace that non-fatal assurance with a non-fatal
check: in the deferred func that restarts Redis (the anonymous defer func
containing t.Log, exec.Command("docker", "compose", ... "start", "redis").Run(),
and time.Sleep), remove require.NoError and instead use assert.NoError(t, err)
or if you prefer the testing.T API, check err and call t.Errorf("Failed to start
Redis container: %v", err); keep the restart command and the sleep as-is so
cleanup proceeds without calling FailNow from within defer.
🧹 Nitpick comments (3)
internal/handlers/elastic_ip_handler_test.go (2)
128-136: Table-driven test pattern is undermined by name-based conditionals.The
if url == "/elastic-ips/{id}/associate"andif tt.name == "Associate"checks couple the test loop to specific test-case names/URLs rather than letting the table data drive behavior. Consider adding abuildURLandbuildBodyfunc field (or similar) to each table entry so the loop stays generic.Also,
body, _ = json.Marshal(...)on line 135 silently discards the error. As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like_ = someFunc()".
78-115: Test coverage is limited to happy paths only.The table only covers successful Allocate, List, and Associate. Consider adding error cases (e.g., service returns an error, invalid UUID in path) and covering the remaining handler methods (Get, Release, Disassociate) to improve handler test coverage.
internal/core/services/elastic_ip.go (1)
191-205: Constants use non-idiomatic Go naming convention.Go convention (and the coding guidelines' examples like
StatusRunning,MaxPortsPerInstance) favors PascalCase for exported constants.SCREAMING_SNAKE_CASEis atypical in Go. Since these are only used within this file, they could also be unexported.Suggested naming
const ( - CGNAT_FIRST_OCTET = 100 - CGNAT_SECOND_OCTET_BASE = 64 - CGNAT_SECOND_OCTET_MASK = 64 - UUID_IP_BYTE_1 = 12 - UUID_IP_BYTE_2 = 13 - UUID_IP_BYTE_3 = 14 + cgnatFirstOctet = 100 + cgnatSecondOctetBase = 64 + cgnatSecondOctetMask = 64 + uuidIPByte1 = 12 + uuidIPByte2 = 13 + uuidIPByte3 = 14 )
| // generateDeterministicIP creates a consistent "public" IP for a given UUID within the 100.64.0.0/10 range. | ||
| func (s *elasticIPService) generateDeterministicIP(u uuid.UUID) string { | ||
| // 100.64.0.0 + bytes 12-15 of UUID | ||
| ip := net.IPv4(CGNAT_FIRST_OCTET, CGNAT_SECOND_OCTET_BASE+u[UUID_IP_BYTE_1]%CGNAT_SECOND_OCTET_MASK, u[UUID_IP_BYTE_2], u[UUID_IP_BYTE_3]) | ||
| return ip.String() |
There was a problem hiding this comment.
Comment says "bytes 12-15" but only bytes 12-14 are used.
Line 209 comment references bytes 12-15, but the implementation uses indices 12, 13, and 14 (three bytes, not four). Minor documentation inaccuracy.
Fix
- // 100.64.0.0 + bytes 12-15 of UUID
+ // 100.64.0.0 + bytes 12-14 of UUID📝 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.
| // generateDeterministicIP creates a consistent "public" IP for a given UUID within the 100.64.0.0/10 range. | |
| func (s *elasticIPService) generateDeterministicIP(u uuid.UUID) string { | |
| // 100.64.0.0 + bytes 12-15 of UUID | |
| ip := net.IPv4(CGNAT_FIRST_OCTET, CGNAT_SECOND_OCTET_BASE+u[UUID_IP_BYTE_1]%CGNAT_SECOND_OCTET_MASK, u[UUID_IP_BYTE_2], u[UUID_IP_BYTE_3]) | |
| return ip.String() | |
| // generateDeterministicIP creates a consistent "public" IP for a given UUID within the 100.64.0.0/10 range. | |
| func (s *elasticIPService) generateDeterministicIP(u uuid.UUID) string { | |
| // 100.64.0.0 + bytes 12-14 of UUID | |
| ip := net.IPv4(CGNAT_FIRST_OCTET, CGNAT_SECOND_OCTET_BASE+u[UUID_IP_BYTE_1]%CGNAT_SECOND_OCTET_MASK, u[UUID_IP_BYTE_2], u[UUID_IP_BYTE_3]) | |
| return ip.String() | |
| } |
🤖 Prompt for AI Agents
In `@internal/core/services/elastic_ip.go` around lines 207 - 211, Update the
function comment in generateDeterministicIP to correctly state which UUID bytes
are used (bytes 12-14) to match the implementation that builds the IPv4 address
using UUID_IP_BYTE_1, UUID_IP_BYTE_2, and UUID_IP_BYTE_3; do not change the
logic—just correct the docstring so it no longer says "bytes 12-15".
| defer func() { | ||
| t.Log("Restarting Redis container...") | ||
| _ = exec.Command("docker", "start", "cloud-redis").Run() | ||
| err := exec.Command("docker", "compose", "-f", composeFile, "-p", projectName, "start", "redis").Run() | ||
| require.NoError(t, err, "Failed to start Redis container") | ||
| // Wait for Redis to be ready again | ||
| time.Sleep(2 * time.Second) | ||
| }() |
There was a problem hiding this comment.
Avoid require.NoError inside a defer — use assert or t.Errorf instead.
require.NoError calls t.FailNow(), which triggers runtime.Goexit(). When called inside a deferred function, this can cause confusing behavior and may mask the original test failure. Use a non-fatal assertion for cleanup.
Proposed fix
defer func() {
t.Log("Restarting Redis container...")
err := exec.Command("docker", "compose", "-f", composeFile, "-p", projectName, "start", "redis").Run()
- require.NoError(t, err, "Failed to start Redis container")
+ assert.NoError(t, err, "Failed to start Redis container")
// Wait for Redis to be ready again
time.Sleep(2 * time.Second)
}()📝 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.
| defer func() { | |
| t.Log("Restarting Redis container...") | |
| _ = exec.Command("docker", "start", "cloud-redis").Run() | |
| err := exec.Command("docker", "compose", "-f", composeFile, "-p", projectName, "start", "redis").Run() | |
| require.NoError(t, err, "Failed to start Redis container") | |
| // Wait for Redis to be ready again | |
| time.Sleep(2 * time.Second) | |
| }() | |
| defer func() { | |
| t.Log("Restarting Redis container...") | |
| err := exec.Command("docker", "compose", "-f", composeFile, "-p", projectName, "start", "redis").Run() | |
| assert.NoError(t, err, "Failed to start Redis container") | |
| // Wait for Redis to be ready again | |
| time.Sleep(2 * time.Second) | |
| }() |
🤖 Prompt for AI Agents
In `@tests/chaos_test.go` around lines 45 - 51, The deferred cleanup uses
require.NoError which calls FailNow inside a defer — replace that non-fatal
assurance with a non-fatal check: in the deferred func that restarts Redis (the
anonymous defer func containing t.Log, exec.Command("docker", "compose", ...
"start", "redis").Run(), and time.Sleep), remove require.NoError and instead use
assert.NoError(t, err) or if you prefer the testing.T API, check err and call
t.Errorf("Failed to start Redis container: %v", err); keep the restart command
and the sleep as-is so cleanup proceeds without calling FailNow from within
defer.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e.yml (1)
112-126:⚠️ Potential issue | 🟡 MinorThe existing "Debug Logs on Failure" step (Line 122) is missing
COMPOSE_PROJECT_NAME.The new step correctly sets
COMPOSE_PROJECT_NAME, but the pre-existing step at Line 122 doesn't. With a dynamic project name (thecloud-${{ github.run_id }}),docker compose logsat Line 126 will target the wrong (default) project and produce no useful output.Either add the env var to the old step or consolidate the two failure steps into one.
Proposed fix — add env to existing step
- name: Debug Logs on Failure if: failure() + env: + COMPOSE_PROJECT_NAME: thecloud-${{ github.run_id }} run: | echo "Dumping container logs for debugging..." docker compose logs --tail=200
🤖 Fix all issues with AI agents
In @.github/workflows/e2e.yml:
- Around line 39-43: The Postgres readiness loop "until docker compose exec -T
postgres pg_isready -U cloud; do ... done" can hang indefinitely; wrap the loop
in a timeout (matching the API readiness check, e.g., use "timeout 90") or
prefix the until command with a timeout so the check fails after a bounded
period; ensure the job exits non-zero if timeout triggers so the workflow stops
(mirror the approach used for the API readiness check that uses "timeout 90").
| # Wait for Postgres to be healthy | ||
| until docker compose exec -T postgres pg_isready -U cloud; do | ||
| echo "Waiting for postgres..." | ||
| sleep 2 | ||
| done |
There was a problem hiding this comment.
Add a timeout to the Postgres readiness loop.
Unlike the API readiness check (Line 85) which uses timeout 90, this loop will run indefinitely if Postgres fails to start, relying only on the job-level timeout (default 6h).
Proposed fix
# Wait for Postgres to be healthy
- until docker compose exec -T postgres pg_isready -U cloud; do
+ timeout 60 bash -c 'until docker compose exec -T postgres pg_isready -U cloud; do
echo "Waiting for postgres..."
sleep 2
- done
+ done' || { echo "Postgres failed to become ready"; docker compose logs postgres; exit 1; }📝 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.
| # Wait for Postgres to be healthy | |
| until docker compose exec -T postgres pg_isready -U cloud; do | |
| echo "Waiting for postgres..." | |
| sleep 2 | |
| done | |
| # Wait for Postgres to be healthy | |
| timeout 60 bash -c 'until docker compose exec -T postgres pg_isready -U cloud; do | |
| echo "Waiting for postgres..." | |
| sleep 2 | |
| done' || { echo "Postgres failed to become ready"; docker compose logs postgres; exit 1; } |
🤖 Prompt for AI Agents
In @.github/workflows/e2e.yml around lines 39 - 43, The Postgres readiness loop
"until docker compose exec -T postgres pg_isready -U cloud; do ... done" can
hang indefinitely; wrap the loop in a timeout (matching the API readiness check,
e.g., use "timeout 90") or prefix the until command with a timeout so the check
fails after a bounded period; ensure the job exits non-zero if timeout triggers
so the workflow stops (mirror the approach used for the API readiness check that
uses "timeout 90").
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
internal/core/services/instance_test.go:124
setupInstanceServiceTestnow creates a Docker network usingtestutil.TestDockerNetwork, but theInstanceServiceunder test is constructed without settingDockerNetwork, so it will still fall back to the hard-coded "cloud-network" inresolveNetworkConfig. IfTEST_DOCKER_NETWORKis set to a non-default value, the test will create one network but the service will use another. PassDockerNetwork: testutil.TestDockerNetworkintoInstanceServiceParamshere to keep the test consistent with the new configurability.
// In integration tests, we frequently rely on a shared Docker network.
// We ensure it exists here so that Provisioning (which uses it as a fallback) succeeds.
if compute.Type() == "docker" {
_, _ = compute.CreateNetwork(ctx, testutil.TestDockerNetwork)
// Pre-pull test image to prevent flakes in CI environments with slow registries or restrictive daemons
_, _ = compute.LaunchInstanceWithOptions(ctx, coreports.CreateInstanceOptions{
Name: "pre-pull-image",
ImageName: testImage,
})
_ = compute.DeleteInstance(ctx, "pre-pull-image")
}
// Ensure default instance type exists
defaultType := &domain.InstanceType{
ID: testInstanceType,
Name: "Basic 2",
VCPUs: 1,
MemoryMB: 128,
DiskGB: 1,
}
_, _ = itRepo.Create(ctx, defaultType)
eventRepo := postgres.NewEventRepository(db)
eventSvc := services.NewEventService(eventRepo, nil, slog.Default())
auditRepo := postgres.NewAuditRepository(db)
auditSvc := services.NewAuditService(auditRepo)
taskQueue := &InMemoryTaskQueue{}
network := noop.NewNoopNetworkAdapter(slog.Default())
svc := services.NewInstanceService(services.InstanceServiceParams{
Repo: repo,
VpcRepo: vpcRepo,
SubnetRepo: subnetRepo,
VolumeRepo: volumeRepo,
InstanceTypeRepo: itRepo,
Compute: compute,
Network: network,
EventSvc: eventSvc,
AuditSvc: auditSvc,
TaskQueue: taskQueue,
Logger: slog.Default(),
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #### `elastic_ips` - Static/Elastic IPs | ||
| ```sql | ||
| CREATE TABLE elastic_ips ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| user_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| tenant_id UUID NOT NULL REFERENCES tenants(id) ON DELETE CASCADE, | ||
| instance_id UUID REFERENCES instances(id) ON DELETE SET NULL, | ||
| vpc_id UUID REFERENCES vpcs(id) ON DELETE SET NULL, | ||
| public_ip VARCHAR(45) NOT NULL UNIQUE, | ||
| status VARCHAR(50) NOT NULL DEFAULT 'allocated', | ||
| arn VARCHAR(255) NOT NULL UNIQUE, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| ); | ||
|
|
||
| CREATE UNIQUE INDEX idx_elastic_ips_instance_id_unique | ||
| ON elastic_ips(instance_id) | ||
| WHERE instance_id IS NOT NULL; | ||
| CREATE INDEX idx_elastic_ips_tenant_id ON elastic_ips(tenant_id); | ||
| ``` |
There was a problem hiding this comment.
The documented elastic_ips schema here doesn’t match the actual migration added in this PR (060): the migration does not add FKs to tenants, does not specify ON DELETE CASCADE, and does not declare arn as UNIQUE. Please update the documentation to reflect the real schema (or update the migration/schema to match this doc).
| CREATE TABLE IF NOT EXISTS elastic_ips ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| user_id UUID NOT NULL REFERENCES users(id), |
There was a problem hiding this comment.
This new migration file is missing the -- +goose Up directive that appears at the top of other migration files in this repo. Even if the in-app migrator doesn’t parse it, keeping the directive consistent helps tooling/readability for humans and any external migration runners.
| const ( | ||
| composeFile = "../docker-compose.yml" | ||
| projectName = "cloud" | ||
| ) |
There was a problem hiding this comment.
projectName is hard-coded to "cloud" for docker compose commands, but docker-compose.yml no longer sets container_name, and CI sets COMPOSE_PROJECT_NAME dynamically. With -p cloud, these commands will target the wrong project and likely fail (or stop the wrong containers). Read the project name from COMPOSE_PROJECT_NAME (or omit -p and rely on env) to match how the stack is started.
| // Simulate public IP allocation from CGNAT range 100.64.0.0/10 for demo/simulation | ||
| // In a real system, this would come from an IP pool manager or provider SDK | ||
| publicIP := s.generateDeterministicIP(id) | ||
|
|
||
| eip := &domain.ElasticIP{ | ||
| ID: id, | ||
| UserID: userID, | ||
| TenantID: tenantID, | ||
| PublicIP: publicIP, |
There was a problem hiding this comment.
Deterministic IP allocation based on only 3 UUID bytes gives a pool of ~4.2M addresses (100.64.0.0/10 subset), so collisions are guaranteed at scale and will manifest as occasional unique-constraint failures on elastic_ips.public_ip. AllocateIP currently doesn’t retry on conflict, so allocation can fail nondeterministically. Consider allocating from a larger space (e.g., use 4 UUID bytes) and/or retrying with a new UUID/IP when repo.Create fails due to a uniqueness violation.
|
|
||
| // generateDeterministicIP creates a consistent "public" IP for a given UUID within the 100.64.0.0/10 range. | ||
| func (s *elasticIPService) generateDeterministicIP(u uuid.UUID) string { | ||
| // 100.64.0.0 + bytes 12-15 of UUID |
There was a problem hiding this comment.
The comment says the generated IP is based on UUID bytes 12–15, but the implementation uses bytes 12–14 (plus the fixed first octet and derived second octet). Update the comment (or the implementation) so they match.
| // 100.64.0.0 + bytes 12-15 of UUID | |
| // Use bytes 12-14 of the UUID to derive an IP within 100.64.0.0/10 (12th byte affects the second octet, 13th and 14th bytes are the third and fourth octets). |
| "id": "uuid", | ||
| "public_ip": "100.64.x.y", | ||
| "status": "allocated", | ||
| "arn": "arn:thecloud:vpc:local:tenant:eip/uuid" |
There was a problem hiding this comment.
The example ARN format here uses tenant in the ARN (arn:thecloud:vpc:local:tenant:eip/uuid), but ElasticIPService currently builds the ARN with the user ID (...:local:<userID>:eip/<id>). Update the docs example (or the ARN generation) so API consumers get the correct ARN format.
| "arn": "arn:thecloud:vpc:local:tenant:eip/uuid" | |
| "arn": "arn:thecloud:vpc:local:user-id:eip/uuid" |
| func TestElasticIPHandler(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| method string | ||
| url string | ||
| setupMock func(svc *mockElasticIPService, eipID, instID uuid.UUID) | ||
| body interface{} | ||
| expectedStatus int | ||
| }{ | ||
| { | ||
| name: "Allocate", | ||
| method: "POST", | ||
| url: eipRoute, | ||
| setupMock: func(svc *mockElasticIPService, eipID, instID uuid.UUID) { | ||
| svc.On("AllocateIP", mock.Anything).Return(&domain.ElasticIP{ID: eipID, PublicIP: "1.2.3.4"}, nil).Once() | ||
| }, | ||
| expectedStatus: http.StatusCreated, | ||
| }, | ||
| { | ||
| name: "List", | ||
| method: "GET", | ||
| url: eipRoute, | ||
| setupMock: func(svc *mockElasticIPService, eipID, instID uuid.UUID) { | ||
| svc.On("ListElasticIPs", mock.Anything).Return([]*domain.ElasticIP{}, nil).Once() | ||
| }, | ||
| expectedStatus: http.StatusOK, | ||
| }, | ||
| { | ||
| name: "Associate", | ||
| method: "POST", | ||
| url: "/elastic-ips/{id}/associate", | ||
| setupMock: func(svc *mockElasticIPService, eipID, instID uuid.UUID) { | ||
| svc.On("AssociateIP", mock.Anything, eipID, instID).Return(&domain.ElasticIP{}, nil).Once() | ||
| }, | ||
| body: map[string]string{"instance_id": ""}, // Filled in loop | ||
| expectedStatus: http.StatusOK, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Handler tests currently cover Allocate, List, and Associate success paths only. For consistency with other handler test files in this repo, add coverage for Get/Release/Disassociate and key error cases (invalid UUID path param, invalid/missing JSON body, service errors) to ensure standardized error responses stay correct.
| -- Optimization indexes | ||
| CREATE INDEX idx_elastic_ips_tenant_id ON elastic_ips(tenant_id); | ||
| CREATE INDEX idx_elastic_ips_instance_id ON elastic_ips(instance_id); | ||
|
|
||
| -- Business rule: An instance can have at most one Elastic IP | ||
| CREATE UNIQUE INDEX idx_elastic_ips_instance_unique | ||
| ON elastic_ips(instance_id) | ||
| WHERE instance_id IS NOT NULL; |
There was a problem hiding this comment.
Migration creates indexes without IF NOT EXISTS. Since RunMigrations executes all *.up.sql files on every startup and only logs errors, these CREATE INDEX statements will repeatedly error after the first run and spam logs. Use CREATE INDEX IF NOT EXISTS (including the partial unique index) or otherwise make index creation idempotent.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/gateway_e2e_test.go (2)
210-223:⚠️ Potential issue | 🟡 MinorMissing status code assertions on route creation responses.
In
VerifyPriorityMatching, thepostRequestresponses for the general and specific routes (resGen,resSpec) are closed without verifyingStatusCreated. Other subtests (e.g.,VerifyMethodMatchingat lines 284, 295) already do this. A silent creation failure here would make the priority-matching assertions misleading.Proposed fix
resGen := postRequest(t, client, testutil.TestBaseURL+gatewayRoutesPath, token, payloadGen) + require.Equal(t, http.StatusCreated, resGen.StatusCode, "general pattern route creation failed") _ = resGen.Body.Close() // 2. Specific exact route (High priority implied by length or explicit) ... resSpec := postRequest(t, client, testutil.TestBaseURL+gatewayRoutesPath, token, payloadSpec) + require.Equal(t, http.StatusCreated, resSpec.StatusCode, "specific exact route creation failed") _ = resSpec.Body.Close()
253-254:⚠️ Potential issue | 🟡 MinorSame missing status check on route creation in
VerifyExtensionMatching.
resExtis closed without assertingStatusCreated, same pattern as theVerifyPriorityMatchingissue above.Proposed fix
resExt := postRequest(t, client, testutil.TestBaseURL+gatewayRoutesPath, token, payload) + require.Equal(t, http.StatusCreated, resExt.StatusCode, "extension route creation failed") _ = resExt.Body.Close()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CREATE TABLE elastic_ips ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| user_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| tenant_id UUID NOT NULL REFERENCES tenants(id) ON DELETE CASCADE, | ||
| instance_id UUID REFERENCES instances(id) ON DELETE SET NULL, | ||
| vpc_id UUID REFERENCES vpcs(id) ON DELETE SET NULL, | ||
| public_ip VARCHAR(45) NOT NULL UNIQUE, | ||
| status VARCHAR(50) NOT NULL DEFAULT 'allocated', | ||
| arn VARCHAR(255) NOT NULL UNIQUE, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| ); | ||
|
|
||
| CREATE UNIQUE INDEX idx_elastic_ips_instance_id_unique | ||
| ON elastic_ips(instance_id) | ||
| WHERE instance_id IS NOT NULL; | ||
| CREATE INDEX idx_elastic_ips_tenant_id ON elastic_ips(tenant_id); |
There was a problem hiding this comment.
This documented elastic_ips schema does not match the actual migration in 060_create_elastic_ips.up.sql (e.g., tenant_id/user_id foreign keys + ON DELETE CASCADE, status/arn uniqueness, and index naming). Please update the docs to reflect the real DB schema (or update the migration to match what’s documented) so operators don’t provision the wrong structure.
| CREATE TABLE elastic_ips ( | |
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | |
| user_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, | |
| tenant_id UUID NOT NULL REFERENCES tenants(id) ON DELETE CASCADE, | |
| instance_id UUID REFERENCES instances(id) ON DELETE SET NULL, | |
| vpc_id UUID REFERENCES vpcs(id) ON DELETE SET NULL, | |
| public_ip VARCHAR(45) NOT NULL UNIQUE, | |
| status VARCHAR(50) NOT NULL DEFAULT 'allocated', | |
| arn VARCHAR(255) NOT NULL UNIQUE, | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | |
| ); | |
| CREATE UNIQUE INDEX idx_elastic_ips_instance_id_unique | |
| ON elastic_ips(instance_id) | |
| WHERE instance_id IS NOT NULL; | |
| CREATE INDEX idx_elastic_ips_tenant_id ON elastic_ips(tenant_id); | |
| -- NOTE: The authoritative schema for this table is defined in: | |
| -- migrations/060_create_elastic_ips.up.sql | |
| -- | |
| -- High-level behavior: | |
| -- * Tracks static/elastic IP allocations per tenant/user. | |
| -- * Enforces foreign keys to users, tenants, instances, and VPCs with | |
| -- appropriate ON DELETE behavior. | |
| -- * Ensures uniqueness of allocation state/ARN and instance bindings via | |
| -- unique indexes and supporting indexes for common lookups. | |
| -- | |
| -- Refer to the migration file above for the exact column definitions, | |
| -- constraints, and index names. |
| // In integration tests, we frequently rely on a shared Docker network. | ||
| // We ensure it exists here so that Provisioning (which uses it as a fallback) succeeds. | ||
| if compute.Type() == "docker" { | ||
| _, _ = compute.CreateNetwork(ctx, "cloud-network") | ||
| _, _ = compute.CreateNetwork(ctx, testutil.TestDockerNetwork) | ||
| // Pre-pull test image to prevent flakes in CI environments with slow registries or restrictive daemons |
There was a problem hiding this comment.
The test creates a Docker network using testutil.TestDockerNetwork, but the InstanceService constructed below is not passed DockerNetwork, so it will still default to cloud-network. If TEST_DOCKER_NETWORK is set to something else, the service may try to attach containers to a network that wasn’t created. Pass DockerNetwork: testutil.TestDockerNetwork when constructing the service (or keep the created network name aligned with the service default).
| // Simulate public IP allocation from CGNAT range 100.64.0.0/10 for demo/simulation | ||
| // In a real system, this would come from an IP pool manager or provider SDK | ||
| publicIP := s.generateDeterministicIP(id) | ||
|
|
||
| eip := &domain.ElasticIP{ | ||
| ID: id, | ||
| UserID: userID, | ||
| TenantID: tenantID, | ||
| PublicIP: publicIP, | ||
| Status: domain.EIPStatusAllocated, | ||
| ARN: fmt.Sprintf("arn:thecloud:vpc:local:%s:eip/%s", userID, id), | ||
| CreatedAt: time.Now(), | ||
| UpdatedAt: time.Now(), | ||
| } | ||
|
|
||
| if err := s.repo.Create(ctx, eip); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
generateDeterministicIP maps a UUID down to a 100.64.0.0/10 address space (~4M IPs), so collisions are mathematically possible and will surface as DB unique-constraint failures on public_ip during allocation. Consider adding a retry loop that regenerates the UUID/IP when the insert fails due to a unique violation, or switch to an allocation strategy that guarantees uniqueness (e.g., an IP pool table/sequence).
|
|
||
| var body []byte | ||
| if tt.name == "Associate" { | ||
| body, _ = json.Marshal(map[string]string{"instance_id": instID.String()}) |
There was a problem hiding this comment.
json.Marshal errors are ignored here. Even though it’s unlikely to fail for this payload, it can hide real issues and makes debugging harder. Please assert/require the marshal error is nil.
| body, _ = json.Marshal(map[string]string{"instance_id": instID.String()}) | |
| var err error | |
| body, err = json.Marshal(map[string]string{"instance_id": instID.String()}) | |
| require.NoError(t, err) |
| instance_id UUID REFERENCES instances(id) ON DELETE SET NULL, | ||
| vpc_id UUID REFERENCES vpcs(id) ON DELETE SET NULL, | ||
| status VARCHAR(20) NOT NULL DEFAULT 'allocated', | ||
| arn VARCHAR(255) NOT NULL, |
There was a problem hiding this comment.
arn is declared NOT NULL but not UNIQUE here, while other resources often enforce ARN uniqueness at the DB layer. If the API expects ARN to uniquely identify an EIP (and to avoid accidental duplicates), add a UNIQUE constraint/index on arn.
| arn VARCHAR(255) NOT NULL, | |
| arn VARCHAR(255) NOT NULL UNIQUE, |
Overview
This PR implements comprehensive support for Elastic IPs (EIPs) within 'The Cloud' platform, following the hexagonal architecture patterns. It includes full CRUD operations, association logic with instances, and enhanced documentation.
Changes
ElasticIPServicewith robust error handling and audit logging.swagger.yaml.docs/api-reference.mdand Swagger spec to ensure a consistent API contract.Verification Results
make swagger.Summary by CodeRabbit
New Features
Documentation
CI & Infrastructure
Tests