Gateway Manifest Retrieval Support#1369
Merged
slahirucd7 merged 12 commits intowso2:mainfrom Mar 20, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds gateway manifest sync: the gateway controller classifies policies as Changes
Sequence DiagramsequenceDiagram
participant GC as Gateway Controller
participant CP as Control Plane API
participant DB as Platform Database
participant Client as External Client
GC->>CP: Establish connection
CP-->>GC: Connected
rect rgba(100, 150, 200, 0.5)
Note over GC,CP: Manifest push on connect
GC->>GC: Build manifest (policies with ManagedBy)
GC->>CP: POST /api/internal/v1/gateways/{gatewayId}/manifest
CP->>CP: ReceiveGatewayManifest (auth, validate)
CP->>DB: UpdateGatewayManifest (store JSON)
DB-->>CP: 200/OK
CP-->>GC: 204 No Content
end
rect rgba(150, 200, 100, 0.5)
Note over Client,CP: Manifest retrieval
Client->>CP: GET /api/v1/gateways/{gatewayId}/manifest
CP->>DB: GetGatewayManifest
DB-->>CP: manifest bytes
CP-->>Client: 200 OK + ManifestSyncResponse
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
platform-api/src/internal/handler/gateway_internal.go (1)
652-655: Consider movinggatewayGroupout of themcpProxyGroupblock for readability.This works, but the current nesting makes route ownership harder to scan.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/handler/gateway_internal.go` around lines 652 - 655, The gateway route registration is currently declared inside the mcpProxyGroup block which makes ownership harder to scan; move the declaration of gatewayGroup := r.Group("/api/internal/v1/gateways") and its POST registration gatewayGroup.POST("/:gatewayId/manifest", h.ReceiveGatewayManifest) out of the mcpProxyGroup closure so it is defined at the same top-level router scope as other API groups, keeping the same group path and handler (ReceiveGatewayManifest) but not nested inside mcpProxyGroup.gateway/gateway-controller/pkg/utils/api_utils.go (1)
602-609: Reuse the shared HTTP client for manifest POSTs.
NewAPIUtilsServicealready buildss.clientwith the service timeout, pool tuning, and TLS settings. Spinning up a fresh client/transport here drops those settings and defeats connection reuse on the retrying manifest-sync path.Proposed refactor
- client := &http.Client{ - Timeout: s.config.Timeout, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: s.config.InsecureSkipVerify, - }, - }, - } - req, err := http.NewRequest("POST", url, bytes.NewBuffer(jsonData)) if err != nil { return fmt.Errorf("failed to create manifest request: %w", err) } @@ - resp, err := client.Do(req) + resp, err := s.client.Do(req)Also applies to: 624-624
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/utils/api_utils.go` around lines 602 - 609, The code creates a new local http.Client (client := &http.Client{...}) for manifest POSTs which bypasses the shared s.client configured in NewAPIUtilsService and prevents connection reuse; replace the local client creation with using the existing s.client (and ensure any TLS/insecure and Timeout behavior uses s.config via the shared s.client), and remove duplicate client instantiation at the other occurrence (the second instance noted around the other diff) so manifest POSTs reuse s.client and its transport/pool tuning.platform-api/src/resources/openapi.yaml (1)
4665-4684: Modelreadystate explicitly sopoliciesis guaranteed.
ManifestSyncResponsecurrently permitsstatus: readywithoutpolicies. That weakens the contract for clients polling this endpoint.♻️ Proposed schema refinement
ManifestSyncResponse: - type: object - required: - - status - properties: - status: - type: string - enum: [pending, ready, failed] - description: | - Current state of the manifest sync job. - - `pending`: Request sent to gateway controller, waiting for response. - - `ready`: Manifest received and policies are available. - - `failed`: Request could not be delivered (gateway offline or error). - example: "ready" - policies: - type: array - description: Custom policies installed on the gateway. Only present when status is `ready`. - items: - $ref: '#/components/schemas/GatewayPolicyDefinition' + oneOf: + - type: object + required: [status] + properties: + status: + type: string + enum: [pending, failed] + - type: object + required: [status, policies] + properties: + status: + type: string + enum: [ready] + policies: + type: array + items: + $ref: '#/components/schemas/GatewayPolicyDefinition'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/resources/openapi.yaml` around lines 4665 - 4684, ManifestSyncResponse currently allows status: "ready" without guaranteeing policies; change the schema to use a oneOf with two explicit variants to enforce the contract: one variant where status is enum ["ready"] and policies is required (items $ref to GatewayPolicyDefinition), and a second variant where status is enum ["pending","failed"] and policies is omitted (or optional). Update the ManifestSyncResponse definition (referencing ManifestSyncResponse, status, policies, and GatewayPolicyDefinition) so clients seeing status: "ready" can rely on policies being present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 2717-2719: The retry backoff uses time.Sleep(attempt*2s) which
blocks cancellation; change it to a context-aware wait by replacing the direct
sleep with a select that waits on time.After(backoff) and on ctx.Done(), so the
loop (where attempt and maxRetries are used) returns promptly when the provided
context is cancelled. Ensure the function uses the surrounding context variable
(ctx) — or add one if missing — and on ctx.Done() break/return with the
appropriate error instead of sleeping.
- Around line 2674-2708: The manifest push does not propagate the incoming
correlation ID from the event to the downstream service; update the gateway
manifest handling so the correlation ID (event["correlationId"]) is captured and
passed into c.apiUtilsService.PushGatewayManifest (modify PushGatewayManifest
signature and all callers to accept a correlationId parameter), and update the
internal manifest POST contract/service handler to accept and validate the
correlation ID header/body so the request/response can be strictly correlated
(refer to PushGatewayManifest and the gateway manifest handling block that reads
event["correlationId"]).
In `@gateway/gateway-controller/pkg/utils/api_utils.go`:
- Around line 579-587: ManifestPolicyEntry.IsCustomPolicy is a nullable *bool
which is being marshalled as null when omitted; ensure callers normalize it to a
real boolean before sending by setting ManifestPolicyEntry.IsCustomPolicy to a
non-nil pointer to false when it's nil (or change the struct to use a
non-pointer bool and update usages accordingly), and apply the same
normalization where ManifestPolicyEntry instances are prepared for POST (also
cover the other occurrences referenced around the same type).
In `@platform-api/src/config/config.go`:
- Around line 132-134: The ManifestSyncTimeoutSecs and ManifestReadyTTLSecs
fields on the config struct must be validated to be positive to avoid immediate
expiry/retrigger loops; update the config initialization/validation (where you
parse/env-load the config) to check ManifestSyncTimeoutSecs > 0 and
ManifestReadyTTLSecs > 0 (referencing the ManifestSyncTimeoutSecs and
ManifestReadyTTLSecs struct fields) and either return a descriptive error or
replace non-positive values with safe defaults (e.g., 30 and 60) before using
them; ensure the validation happens immediately after env parsing so callers
never observe non-positive values.
In `@platform-api/src/internal/handler/gateway_internal.go`:
- Around line 586-591: When handling h.gatewayService.VerifyToken(apiKey),
distinguish auth failures from backend/internal errors: if errors.Is(err,
auth.ErrInvalidCredentials) or your existing helper (e.g.
auth.IsUnauthorized(err) or an auth helper used elsewhere) indicates an
authentication error, keep the 401 response using h.slogger.Warn and
utils.NewErrorResponse; otherwise treat it as a server error—log with
h.slogger.Error including the error details and return
http.StatusInternalServerError with an appropriate utils.NewErrorResponse
message; update the VerifyToken error branch to use errors.Is or the existing
auth helper to decide between 401 and 500.
In `@platform-api/src/internal/handler/gateway.go`:
- Around line 501-510: The error handling after calling
gatewayService.SyncManifest should distinguish client-side invalid gateway IDs
from server errors: update the error branch around SyncManifest(gatewayId,
orgId) to check for an "invalid gateway id" or UUID parse error (e.g.,
err.Error() contains "invalid" or "invalid UUID" or a specific validation error
returned by SyncManifest) and return HTTP 400 with a NewErrorResponse(400, "Bad
Request", "Invalid gateway ID") for that case; keep the existing check for
"gateway not found" returning 404 and fall back to 500 for all other errors.
Ensure you modify the error-handling block in gateway.go where
gatewayService.SyncManifest is called so the correct status codes are returned.
In `@platform-api/src/internal/repository/gateway.go`:
- Around line 427-447: UpdateManifestJob currently returns nil even when no rows
are affected; modify GatewayRepo.UpdateManifestJob to inspect the sql.Result
returned by r.db.Exec(r.db.Rebind(query), args...), check res.RowsAffected(),
and if it is 0 return a not-found error (e.g., a repository-level ErrNotFound or
sql.ErrNoRows) instead of nil; also preserve and return any Exec error
encountered before checking RowsAffected.
- Around line 459-461: The code in gateway.go incorrectly converts sql.ErrNoRows
into (nil, nil, nil), hiding a real not-found condition; change the branch that
tests errors.Is(err, sql.ErrNoRows) so it returns an explicit not-found error
instead of nils (for example return nil, nil, err or wrap err with context like
fmt.Errorf("gateway not found: %w", err) or use your repository sentinel
ErrNotFound), keeping the rest of the error handling unchanged; locate the
conditional that checks errors.Is(err, sql.ErrNoRows) in gateway.go and replace
the nil return with a proper error return so callers can distinguish “not found”
from success.
In `@platform-api/src/internal/service/gateway_events.go`:
- Around line 1195-1202: Validate that the correlationID is non-empty (and
ideally non-whitespace) at the start of BroadcastGatewayManifestRequestEvent; if
invalid, return an error immediately instead of broadcasting. In practice, add a
simple check in GatewayEventsService.BroadcastGatewayManifestRequestEvent that
trims/validates correlationID and returns a descriptive error (e.g.,
fmt.Errorf("missing correlationID")) before constructing dto.GatewayEventDTO and
publishing the event so the method never emits events without a valid
CorrelationID.
In `@platform-api/src/internal/service/gateway.go`:
- Around line 203-206: The loop over policies currently filters out non-custom
policies (for _, p := range policies { if !p.IsCustomPolicy { continue } }) and
does not preserve or return the IsCustomPolicy flag, preventing callers from
filtering; update the iteration and the policy-mapping logic (the code that
converts each p into the returned policy object/DTO) to include a boolean field
IsCustomPolicy (or propagate p.IsCustomPolicy) into the returned policy
representation instead of dropping non-custom entries or stripping the flag, and
ensure the alternative branch preserves and returns policies with
IsCustomPolicy=false so callers can perform caller-side filtering (apply same
change for the other similar loop handling policies at the other location).
- Around line 222-228: The code currently mutates s.manifestStore[gatewayID]
under s.manifestMu before calling s.gatewayRepo.UpdateManifestJob(gatewayID,
"ready"), which can leave the in-memory cache inconsistent if the DB update
fails; change the sequence so you either (a) perform UpdateManifestJob first and
only set s.manifestStore[gatewayID]=custom on success, or (b) if you must set
the cache first, capture the previous value (and whether it existed) before
assigning, then call UpdateManifestJob and if it returns an error revert
s.manifestStore to the previous state inside the same s.manifestMu lock to avoid
races; ensure you reference manifestStore, manifestMu,
gatewayRepo.UpdateManifestJob, gatewayID and custom when locating the change.
- Around line 170-175: The code currently returns a ManifestJob with Status
"ready" when DB status is "ready" even if s.manifestStore[gatewayID] is missing;
update the block that checks status == "ready" (around ManifestJob,
manifestStore, manifestMu) to verify the manifestStore contains a non-nil entry
for gatewayID before returning ready; if the entry is absent or nil, do not
return Status "ready"—either trigger/queue a manifest sync/load for that gateway
(call the existing sync/load routine) and return a non-ready job (or an error)
so the caller re-triggers sync, and ensure you still use s.manifestMu
RLock/RUnlock when reading s.manifestStore[gatewayID].
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/utils/api_utils.go`:
- Around line 602-609: The code creates a new local http.Client (client :=
&http.Client{...}) for manifest POSTs which bypasses the shared s.client
configured in NewAPIUtilsService and prevents connection reuse; replace the
local client creation with using the existing s.client (and ensure any
TLS/insecure and Timeout behavior uses s.config via the shared s.client), and
remove duplicate client instantiation at the other occurrence (the second
instance noted around the other diff) so manifest POSTs reuse s.client and its
transport/pool tuning.
In `@platform-api/src/internal/handler/gateway_internal.go`:
- Around line 652-655: The gateway route registration is currently declared
inside the mcpProxyGroup block which makes ownership harder to scan; move the
declaration of gatewayGroup := r.Group("/api/internal/v1/gateways") and its POST
registration gatewayGroup.POST("/:gatewayId/manifest", h.ReceiveGatewayManifest)
out of the mcpProxyGroup closure so it is defined at the same top-level router
scope as other API groups, keeping the same group path and handler
(ReceiveGatewayManifest) but not nested inside mcpProxyGroup.
In `@platform-api/src/resources/openapi.yaml`:
- Around line 4665-4684: ManifestSyncResponse currently allows status: "ready"
without guaranteeing policies; change the schema to use a oneOf with two
explicit variants to enforce the contract: one variant where status is enum
["ready"] and policies is required (items $ref to GatewayPolicyDefinition), and
a second variant where status is enum ["pending","failed"] and policies is
omitted (or optional). Update the ManifestSyncResponse definition (referencing
ManifestSyncResponse, status, policies, and GatewayPolicyDefinition) so clients
seeing status: "ready" can rely on policies being present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8afddb92-c798-44d2-b2d1-79a8f458ab82
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
gateway/gateway-controller/api/openapi-management.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/controlplane/events.gogateway/gateway-controller/pkg/utils/api_utils.goplatform-api/src/config/config.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/handler/gateway.goplatform-api/src/internal/handler/gateway_internal.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/server/server.goplatform-api/src/internal/service/gateway.goplatform-api/src/internal/service/gateway_events.goplatform-api/src/resources/openapi.yaml
slahirucd7
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
gateway/gateway-controller/pkg/controlplane/client.go (1)
2702-2715:⚠️ Potential issue | 🟠 MajorManifest push retry path is not shutdown-aware and can delay stop/reconnect cleanup.
The goroutine does not observe cancellation before retries, and backoff wait is blocking. Since
Stop()waits on the waitgroup, this path can keep shutdown waiting unnecessarily.💡 Suggested fix
func (c *Client) pushGatewayManifestOnConnect(gatewayID string) { @@ for attempt := 1; attempt <= maxRetries; attempt++ { + select { + case <-c.ctx.Done(): + c.logger.Info("Aborting gateway manifest push: client context cancelled", + slog.String("gateway_id", gatewayID), + ) + return + case <-c.stopChan: + c.logger.Info("Aborting gateway manifest push: stop signal received", + slog.String("gateway_id", gatewayID), + ) + return + default: + } + pushErr = c.apiUtilsService.PushGatewayManifest(gatewayID, policies) if pushErr == nil { break } @@ if attempt < maxRetries { - time.Sleep(time.Duration(attempt) * 2 * time.Second) + select { + case <-time.After(time.Duration(attempt) * 2 * time.Second): + case <-c.ctx.Done(): + return + case <-c.stopChan: + return + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/controlplane/client.go` around lines 2702 - 2715, The retry loop around c.apiUtilsService.PushGatewayManifest is not observing cancellation and uses blocking time.Sleep, which can delay shutdown via Stop()/waitgroup; update the retry logic in the function containing this loop to accept/propagate a context (or use the existing ctx), check ctx.Done() before each retry and when waiting, replace time.Sleep with a select that waits on time.After(backoff) and ctx.Done(), and if ctx is canceled break/return immediately; also prefer passing ctx into PushGatewayManifest if that API supports it (or wrap the call in a select to abort if ctx is canceled) so shutdown/reconnect can proceed promptly.platform-api/src/internal/repository/gateway.go (2)
425-429:⚠️ Potential issue | 🟠 MajorDetect “gateway not found” when manifest update affects zero rows.
UpdateGatewayManifestcan return success even when the gateway row does not exist. Please checkRowsAffected()and return a not-found error when it is zero.Suggested fix
func (r *GatewayRepo) UpdateGatewayManifest(gatewayID string, manifest []byte) error { query := `UPDATE gateways SET manifest = ? WHERE uuid = ?` - _, err := r.db.Exec(r.db.Rebind(query), string(manifest), gatewayID) - return err + result, err := r.db.Exec(r.db.Rebind(query), string(manifest), gatewayID) + if err != nil { + return err + } + rows, err := result.RowsAffected() + if err != nil { + return err + } + if rows == 0 { + return errors.New("gateway not found") + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/repository/gateway.go` around lines 425 - 429, The UpdateGatewayManifest function currently ignores whether the UPDATE actually modified a row; change it to capture the sql.Result from r.db.Exec in UpdateGatewayManifest, call RowsAffected() on that result, and if it returns 0 return a not-found error (use your repository's existing NotFound or gatewayNotFound error type/message) instead of nil; otherwise return the original err (or nil) as appropriate.
437-440:⚠️ Potential issue | 🟠 MajorDo not treat missing gateway rows as “no manifest yet.”
Mapping
sql.ErrNoRowsto(nil, nil)hides a real missing-gateway condition. Keepraw == nilas “no manifest yet,” but return a not-found error for missing rows.Suggested fix
if err != nil { if errors.Is(err, sql.ErrNoRows) { - return nil, nil + return nil, errors.New("gateway not found") } return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/repository/gateway.go` around lines 437 - 440, The code currently maps sql.ErrNoRows to (nil, nil), which hides a missing-gateway condition; change the error handling in the function that queries the gateway (the block checking "if err != nil" and "errors.Is(err, sql.ErrNoRows)") to return a not-found error instead of (nil, nil) when errors.Is(err, sql.ErrNoRows) is true (e.g., return repository.ErrNotFound or the package's standard NotFound error), while preserving the existing semantics where raw == nil still denotes “no manifest yet.” Ensure you update the error return path in that same function (the code handling "err" and "raw") so callers can distinguish missing DB rows from a nil manifest.
🧹 Nitpick comments (1)
platform-api/src/internal/service/gateway.go (1)
41-43: Update stale comment aboutIsCustomPolicybehavior.The comment says
IsCustomPolicyis not stored/returned, but the current implementation does both. Please align the comment with actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/gateway.go` around lines 41 - 43, The struct comment for GatewayPolicyInput is stale about IsCustomPolicy; update the comment above the GatewayPolicyInput type to reflect that IsCustomPolicy is currently persisted and returned (not just used for filtering). Edit the documentation text to remove the "is not stored or returned" clause and instead state that IsCustomPolicy is included in storage/response, keeping the rest of the comment about raw policy data and any filtering behavior accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 347-352: The current code only calls
c.pushGatewayManifestOnConnect(gatewayID) on connect, but the manifest-sync
protocol requires responding to "gateway.manifest.request" (including timeout
re-triggers); update the control-plane client to subscribe/handle the
"gateway.manifest.request" message and invoke the same push logic from that
handler instead of relying solely on connect-time goroutine. Specifically, wire
the handler for "gateway.manifest.request" to call
c.pushGatewayManifestOnConnect(gatewayID) (or extract the push logic into a
shared method used by both the connect path and the request handler) and
implement the timeout/re-trigger behavior in that handler so pending polls are
satisfied without waiting for a reconnect.
In `@platform-api/src/internal/service/gateway.go`:
- Around line 112-116: The mapping of the input policy to the stored type drops
the Description field; update the construction of GatewayPolicyDefinition inside
the manifest entry creation (the block that builds entry :=
GatewayPolicyDefinition{ ... }) to set Description: p.Description (or the
appropriate source field) so the stored GatewayPolicyDefinition retains the
policy description along with Name, Version, and IsCustomPolicy.
- Around line 91-97: The current handler in gateway.go collapses repository
errors from s.gatewayRepo.GetByUUID into a generic "gateway not found"; change
it to first handle and return the repository error (or wrap it) when err != nil
so real failures surface, and only treat gateway == nil or
gateway.OrganizationID != orgID as not-found cases; specifically update the
gatewayRepo.GetByUUID error branch to return the repo error (e.g., wrap with
context) and keep the subsequent nil/ownership checks returning the not-found
error.
In `@platform-api/src/resources/gateway-internal-api.yaml`:
- Around line 829-833: The OpenAPI schema's policies array in
gateway-internal-api.yaml is unbounded which risks oversized payloads; update
the `policies` array schema to include a `maxItems` (e.g., 50 or another
team-agreed limit) and add limits on nested objects (e.g., `maxProperties` or
`additionalProperties: false`) for the policy item object to constrain request
size and memory usage; apply the same changes to the other occurrence referenced
(the second `policies` schema around lines 853-860) so both array definitions
enforce the new limits.
In `@platform-api/src/resources/openapi.yaml`:
- Around line 1743-1767: Update the GET /gateways/{gatewayId}/manifest
(operationId GetGatewayManifest) OpenAPI contract so clients can implement
trigger-and-poll: add a required "status" property to the ManifestSyncResponse
schema with an enum of at least "pending" and "ready", and add a "manifestSync"
(or "manifest_sync") object to ManifestSyncResponse that contains sync metadata
(e.g., lastAttemptTime, lastSuccessTime, maybe a syncVersion or error message)
and, when status == "ready", the manifest payload (or a reference to it). Ensure
ManifestSyncResponse's required list includes "status" (and "manifestSync" if
you choose), and document the semantics of the enum values so consumers can
implement the pending→ready poll loop.
---
Duplicate comments:
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 2702-2715: The retry loop around
c.apiUtilsService.PushGatewayManifest is not observing cancellation and uses
blocking time.Sleep, which can delay shutdown via Stop()/waitgroup; update the
retry logic in the function containing this loop to accept/propagate a context
(or use the existing ctx), check ctx.Done() before each retry and when waiting,
replace time.Sleep with a select that waits on time.After(backoff) and
ctx.Done(), and if ctx is canceled break/return immediately; also prefer passing
ctx into PushGatewayManifest if that API supports it (or wrap the call in a
select to abort if ctx is canceled) so shutdown/reconnect can proceed promptly.
In `@platform-api/src/internal/repository/gateway.go`:
- Around line 425-429: The UpdateGatewayManifest function currently ignores
whether the UPDATE actually modified a row; change it to capture the sql.Result
from r.db.Exec in UpdateGatewayManifest, call RowsAffected() on that result, and
if it returns 0 return a not-found error (use your repository's existing
NotFound or gatewayNotFound error type/message) instead of nil; otherwise return
the original err (or nil) as appropriate.
- Around line 437-440: The code currently maps sql.ErrNoRows to (nil, nil),
which hides a missing-gateway condition; change the error handling in the
function that queries the gateway (the block checking "if err != nil" and
"errors.Is(err, sql.ErrNoRows)") to return a not-found error instead of (nil,
nil) when errors.Is(err, sql.ErrNoRows) is true (e.g., return
repository.ErrNotFound or the package's standard NotFound error), while
preserving the existing semantics where raw == nil still denotes “no manifest
yet.” Ensure you update the error return path in that same function (the code
handling "err" and "raw") so callers can distinguish missing DB rows from a nil
manifest.
---
Nitpick comments:
In `@platform-api/src/internal/service/gateway.go`:
- Around line 41-43: The struct comment for GatewayPolicyInput is stale about
IsCustomPolicy; update the comment above the GatewayPolicyInput type to reflect
that IsCustomPolicy is currently persisted and returned (not just used for
filtering). Edit the documentation text to remove the "is not stored or
returned" clause and instead state that IsCustomPolicy is included in
storage/response, keeping the rest of the comment about raw policy data and any
filtering behavior accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bbbac0f-dea5-4559-adfb-d1e0fd23fc0b
📒 Files selected for processing (11)
gateway/gateway-controller/pkg/controlplane/client.goplatform-api/src/config/config.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/handler/gateway.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/service/gateway.goplatform-api/src/resources/gateway-internal-api.yamlplatform-api/src/resources/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- platform-api/src/config/config.go
- platform-api/src/internal/database/schema.postgres.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
platform-api/src/resources/openapi.yaml (1)
1970-1994:⚠️ Potential issue | 🟠 MajorManifest sync contract is still underspecified for trigger-and-poll clients.
The path is exposed as
/manifest, andManifestSyncResponsedoes not require a syncstatus(pending/ready). That prevents clients from implementing the documented polling flow deterministically.Suggested contract patch
- /gateways/{gatewayId}/manifest: + /gateways/{gatewayId}/manifest-sync: get: - summary: Get gateway policy manifest + summary: Trigger/poll gateway policy manifest sync ... responses: '200': - description: Gateway policy manifest + description: Manifest sync state and payload (when ready) content: application/json: schema: $ref: '#/components/schemas/ManifestSyncResponse' ManifestSyncResponse: type: object + required: + - status properties: + status: + type: string + enum: [pending, ready] + description: Current manifest synchronization state. policies: type: array + description: Present when status is `ready`. items: $ref: '#/components/schemas/GatewayPolicyDefinition'Also applies to: 5109-5119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/resources/openapi.yaml` around lines 1970 - 1994, The GET operation GetGatewayManifest at path /gateways/{gatewayId}/manifest currently returns ManifestSyncResponse but lacks a deterministic sync status for trigger-and-poll clients; update the ManifestSyncResponse schema to include a required "status" property (enum: "pending" | "ready" | "error") and ensure any controller code that produces this response sets the status accordingly so clients can poll until status == "ready"; also update the OpenAPI responses/docs to indicate when "pending" is returned and document any retry/polling semantics for GetGatewayManifest.platform-api/src/internal/service/gateway.go (2)
112-116:⚠️ Potential issue | 🟡 MinorPreserve
Descriptionwhen mapping incoming manifest policies.
GatewayPolicyInput.Descriptionis dropped during conversion, so stored manifest entries lose metadata.Proposed fix
entry := GatewayPolicyDefinition{ Name: p.Name, Version: p.Version, + Description: p.Description, IsCustomPolicy: p.IsCustomPolicy, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/gateway.go` around lines 112 - 116, The mapping from incoming policies is dropping the Description field: when constructing a GatewayPolicyDefinition from the input (currently building entry with Name, Version, IsCustomPolicy), include the Description by adding Description: p.Description so GatewayPolicyDefinition preserves GatewayPolicyInput.Description during conversion and stored manifest entries retain their metadata.
91-94:⚠️ Potential issue | 🟠 MajorDo not collapse repository failures into “gateway not found.”
This branch hides real repository errors and makes diagnostics/HTTP mapping inaccurate.
Proposed fix
gateway, err := s.gatewayRepo.GetByUUID(gatewayID) - if err != nil || gateway == nil { + if err != nil { + return nil, fmt.Errorf("failed to get gateway: %w", err) + } + if gateway == nil { return nil, errors.New("gateway not found") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/gateway.go` around lines 91 - 94, The code currently collapses repository errors into a generic "gateway not found"; change the logic in the GetByUUID handling so that if s.gatewayRepo.GetByUUID returns a non-nil err you return or wrap that err (preserving the original repository error for accurate diagnostics/HTTP mapping), and only return the "gateway not found" error when err == nil && gateway == nil; update the return path around the call to s.gatewayRepo.GetByUUID to preserve repository errors (or annotate them with context) rather than replacing them with the not-found message.gateway/gateway-controller/pkg/utils/api_utils.go (1)
579-587:⚠️ Potential issue | 🟠 MajorAvoid emitting
"isCustomPolicy": nullin manifest payloads.
IsCustomPolicyis nullable but serialized as a required key, so nil values becomenullin JSON. That can break the boolean contract and downstream filtering semantics.Proposed fix
type ManifestPolicyEntry struct { Name string `json:"name"` Version string `json:"version"` Description *string `json:"description,omitempty"` Parameters map[string]interface{} `json:"parameters,omitempty"` SystemParameters map[string]interface{} `json:"systemParameters,omitempty"` - IsCustomPolicy *bool `json:"isCustomPolicy"` + IsCustomPolicy bool `json:"isCustomPolicy"` }Also applies to: 593-597
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/utils/api_utils.go` around lines 579 - 587, ManifestPolicyEntry currently serializes IsCustomPolicy as a required key, emitting "isCustomPolicy": null when the pointer is nil; update the struct tag on ManifestPolicyEntry.IsCustomPolicy to `json:"isCustomPolicy,omitempty"` so nil values are omitted, and apply the same fix to the other struct(s) that declare an IsCustomPolicy pointer (the similar definition referenced around lines 593-597) to prevent null booleans in manifest payloads.platform-api/src/resources/gateway-internal-api.yaml (1)
829-860:⚠️ Potential issue | 🟠 MajorConstrain manifest payload size in schema.
policiesand nested policy schema objects are currently unbounded, which leaves this endpoint open to oversized payload pressure.Suggested schema hardening
policies: type: array description: All policies installed on the gateway. + maxItems: 1000 items: type: object @@ parameters: type: object description: JSON Schema for the policy's user-configurable parameters. Present for custom policies. + maxProperties: 200 additionalProperties: true systemParameters: type: object description: JSON Schema for the policy's system-level parameters. Present for custom policies. + maxProperties: 200 additionalProperties: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/resources/gateway-internal-api.yaml` around lines 829 - 860, The policy schema is unbounded and needs size limits: add a sensible maxItems (and optionally minItems) to the policies array and add maxLength for string fields like name and version; for the nested objects parameters and systemParameters add maxProperties and set additionalProperties to true only if needed (or switch to additionalProperties: false and use patternProperties) to prevent arbitrarily large objects; ensure you update the schema entries named policies, parameters, and systemParameters (and the properties name/version) to include these constraints so the manifest payload is bounded.
🧹 Nitpick comments (2)
platform-api/src/internal/server/server.go (1)
170-170: Minor: Extra space in argument list.There's a double space between
gatewayEventsService,andslogger. This is a trivial formatting nit.- gatewayService := service.NewGatewayService(gatewayRepo, orgRepo, apiRepo, gatewayEventsService, slogger) + gatewayService := service.NewGatewayService(gatewayRepo, orgRepo, apiRepo, gatewayEventsService, slogger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/server/server.go` at line 170, Remove the extra space in the argument list when calling NewGatewayService: locate the call to service.NewGatewayService using the identifiers gatewayRepo, orgRepo, apiRepo, gatewayEventsService and slogger and ensure there is only a single space (or no extra spaces) between the comma after gatewayEventsService and slogger so the arguments are consistently formatted.gateway/gateway-controller/pkg/utils/api_utils.go (1)
602-609: Reuses.clientinstead of creating a per-call client; this ensures consistent transport configuration across all operations.The per-call
http.Clientcreated here omits theMinVersion: tls.VersionTLS12requirement and connection pool tuning (MaxIdleConns, MaxIdleConnsPerHost, MaxConnsPerHost, IdleConnTimeout) already configured inNewAPIUtilsService. This same pattern appears in other methods in this file—apply the same fix wherever per-call clients are created.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/utils/api_utils.go` around lines 602 - 609, The per-call http.Client created in this method overwrites shared transport settings and omits TLS/connection-pool options; replace the local client creation with reuse of the service-level s.client (constructed in NewAPIUtilsService) so the TLSConfig.MinVersion (tls.VersionTLS12) and connection pool tuning (MaxIdleConns, MaxIdleConnsPerHost, MaxConnsPerHost, IdleConnTimeout) are preserved; update this method (and other methods in the file using per-call clients) to use s.client and ensure any per-request timeouts are applied via context with deadline rather than a new http.Client.Timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 348-353: handleMessage currently lacks a case for the
"gateway.manifest.request" event so re-sync requests fall through to the default
unknown-event path; add a switch case for "gateway.manifest.request" in
handleMessage that calls c.pushGatewayManifest() (mirror the existing logic that
pushes the manifest on connect such as pushGatewayManifestOnConnect) so the
control plane-triggered re-transmit is handled properly.
In `@platform-api/src/internal/handler/gateway.go`:
- Around line 498-508: The current error handling around
h.gatewayService.GetStoredManifest(gatewayId, orgId) maps "gateway not found" to
404 but treats invalid UUID formats as 500; update the handler to detect an
invalid-UUID error (e.g., check err.Error() for "invalid UUID" or the same
pattern used in GetGateway) and return HTTP 400 with a descriptive message, keep
the 404 mapping for "gateway not found", and change the internal error response
message from "Failed to sync gateway manifest" to "Failed to retrieve gateway
manifest" so the error text matches the retrieval context; locate this logic in
the gateway manifest retrieval block that calls GetStoredManifest and apply the
checks and message changes there.
In `@platform-api/src/internal/service/gateway.go`:
- Around line 41-43: The comment for GatewayPolicyInput is outdated: it
currently states IsCustomPolicy "is used only for filtering and is not stored or
returned," but IsCustomPolicy is persisted in GatewayPolicyDefinition and
included in the stored manifest JSON; update the doc comment for
GatewayPolicyInput to reflect that IsCustomPolicy is persisted and included in
stored manifest data (reference GatewayPolicyInput, IsCustomPolicy, and
GatewayPolicyDefinition), ensuring the comment no longer contradicts the
implementation.
In `@platform-api/src/resources/gateway-internal-api.yaml`:
- Around line 834-852: The policy item schema currently requires only "name" and
"version" but allows "isCustomPolicy" to be omitted; update the OpenAPI schema
for the policy object by adding "isCustomPolicy" to the required list (alongside
"name" and "version") so every policy entry must include the boolean
isCustomPolicy property referenced in the properties block (name, version,
description, isCustomPolicy).
---
Duplicate comments:
In `@gateway/gateway-controller/pkg/utils/api_utils.go`:
- Around line 579-587: ManifestPolicyEntry currently serializes IsCustomPolicy
as a required key, emitting "isCustomPolicy": null when the pointer is nil;
update the struct tag on ManifestPolicyEntry.IsCustomPolicy to
`json:"isCustomPolicy,omitempty"` so nil values are omitted, and apply the same
fix to the other struct(s) that declare an IsCustomPolicy pointer (the similar
definition referenced around lines 593-597) to prevent null booleans in manifest
payloads.
In `@platform-api/src/internal/service/gateway.go`:
- Around line 112-116: The mapping from incoming policies is dropping the
Description field: when constructing a GatewayPolicyDefinition from the input
(currently building entry with Name, Version, IsCustomPolicy), include the
Description by adding Description: p.Description so GatewayPolicyDefinition
preserves GatewayPolicyInput.Description during conversion and stored manifest
entries retain their metadata.
- Around line 91-94: The code currently collapses repository errors into a
generic "gateway not found"; change the logic in the GetByUUID handling so that
if s.gatewayRepo.GetByUUID returns a non-nil err you return or wrap that err
(preserving the original repository error for accurate diagnostics/HTTP
mapping), and only return the "gateway not found" error when err == nil &&
gateway == nil; update the return path around the call to
s.gatewayRepo.GetByUUID to preserve repository errors (or annotate them with
context) rather than replacing them with the not-found message.
In `@platform-api/src/resources/gateway-internal-api.yaml`:
- Around line 829-860: The policy schema is unbounded and needs size limits: add
a sensible maxItems (and optionally minItems) to the policies array and add
maxLength for string fields like name and version; for the nested objects
parameters and systemParameters add maxProperties and set additionalProperties
to true only if needed (or switch to additionalProperties: false and use
patternProperties) to prevent arbitrarily large objects; ensure you update the
schema entries named policies, parameters, and systemParameters (and the
properties name/version) to include these constraints so the manifest payload is
bounded.
In `@platform-api/src/resources/openapi.yaml`:
- Around line 1970-1994: The GET operation GetGatewayManifest at path
/gateways/{gatewayId}/manifest currently returns ManifestSyncResponse but lacks
a deterministic sync status for trigger-and-poll clients; update the
ManifestSyncResponse schema to include a required "status" property (enum:
"pending" | "ready" | "error") and ensure any controller code that produces this
response sets the status accordingly so clients can poll until status ==
"ready"; also update the OpenAPI responses/docs to indicate when "pending" is
returned and document any retry/polling semantics for GetGatewayManifest.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/utils/api_utils.go`:
- Around line 602-609: The per-call http.Client created in this method
overwrites shared transport settings and omits TLS/connection-pool options;
replace the local client creation with reuse of the service-level s.client
(constructed in NewAPIUtilsService) so the TLSConfig.MinVersion
(tls.VersionTLS12) and connection pool tuning (MaxIdleConns,
MaxIdleConnsPerHost, MaxConnsPerHost, IdleConnTimeout) are preserved; update
this method (and other methods in the file using per-call clients) to use
s.client and ensure any per-request timeouts are applied via context with
deadline rather than a new http.Client.Timeout.
In `@platform-api/src/internal/server/server.go`:
- Line 170: Remove the extra space in the argument list when calling
NewGatewayService: locate the call to service.NewGatewayService using the
identifiers gatewayRepo, orgRepo, apiRepo, gatewayEventsService and slogger and
ensure there is only a single space (or no extra spaces) between the comma after
gatewayEventsService and slogger so the arguments are consistently formatted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2751d91-e0ab-4cdd-885f-594927d78481
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
gateway/gateway-controller/api/openapi-management.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/controlplane/events.gogateway/gateway-controller/pkg/utils/api_utils.goplatform-api/src/config/config.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/handler/gateway.goplatform-api/src/internal/handler/gateway_internal.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/server/server.goplatform-api/src/internal/service/gateway.goplatform-api/src/resources/gateway-internal-api.yamlplatform-api/src/resources/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- platform-api/src/internal/repository/gateway.go
- platform-api/src/internal/database/schema.postgres.sql
- platform-api/src/internal/database/schema.sqlite.sql
- gateway/gateway-controller/api/openapi-management.yaml
- platform-api/src/internal/repository/interfaces.go
- platform-api/src/internal/handler/gateway_internal.go
- platform-api/src/config/config.go
Comment on lines
+834
to
+852
| required: | ||
| - name | ||
| - version | ||
| properties: | ||
| name: | ||
| type: string | ||
| description: Unique policy name | ||
| example: "set-wso2-headers" | ||
| version: | ||
| type: string | ||
| description: Semantic version of the policy | ||
| example: "v0.8.0" | ||
| description: | ||
| type: string | ||
| description: Human-readable description of the policy | ||
| isCustomPolicy: | ||
| type: boolean | ||
| description: Whether this is a user-installed custom policy | ||
| example: true |
Contributor
There was a problem hiding this comment.
Require isCustomPolicy in each policy item.
The endpoint contract describes isCustomPolicy as part of every policy entry, but the schema currently allows it to be omitted.
Proposed fix
required:
- name
- version
+ - isCustomPolicy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-api/src/resources/gateway-internal-api.yaml` around lines 834 - 852,
The policy item schema currently requires only "name" and "version" but allows
"isCustomPolicy" to be omitted; update the OpenAPI schema for the policy object
by adding "isCustomPolicy" to the required list (alongside "name" and "version")
so every policy entry must include the boolean isCustomPolicy property
referenced in the properties block (name, version, description, isCustomPolicy).
| ) | ||
|
|
||
| policies := make([]utils.ManifestPolicyEntry, 0, len(c.policyDefinitions)) | ||
| for _, def := range c.policyDefinitions { |
Contributor
There was a problem hiding this comment.
Currently, we are using policyDefinitions DTO (generated.go). I guess we have to introduce a model for this.
Contributor
Author
There was a problem hiding this comment.
IMO opinion it can be handled as a separate task. This PR need to be merged soon. This is being opened for more ~1 week now.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
platform-api/src/internal/service/gateway.go (2)
41-43:⚠️ Potential issue | 🟡 MinorUpdate stale
GatewayPolicyInputcomment to match behavior.Line 42 says
IsCustomPolicyis not stored/returned, but it is persisted inGatewayPolicyDefinitionand used in returned manifest data.Suggested fix
-// IsCustomPolicy is used only for filtering and is not stored or returned. +// IsCustomPolicy indicates whether the policy is custom and is persisted in stored/returned manifest data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/gateway.go` around lines 41 - 43, The comment on GatewayPolicyInput is stale: it incorrectly states IsCustomPolicy "is not stored or returned" while the field is persisted in GatewayPolicyDefinition and included in returned manifest data; update the doc comment for GatewayPolicyInput to state that IsCustomPolicy is stored (persisted to GatewayPolicyDefinition) and propagated in the returned manifest/response so it accurately reflects current behavior.
91-97:⚠️ Potential issue | 🟠 MajorDo not collapse repository failures into “gateway not found.”
Line 92 currently maps
GetByUUIDerrors to not-found, which hides real backend failures and breaks error classification.Suggested fix
gateway, err := s.gatewayRepo.GetByUUID(gatewayID) - if err != nil || gateway == nil { + if err != nil { + return nil, fmt.Errorf("failed to get gateway: %w", err) + } + if gateway == nil { return nil, errors.New("gateway not found") } if gateway.OrganizationID != orgID { return nil, errors.New("gateway not found") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/gateway.go` around lines 91 - 97, The current code collapses repository errors from s.gatewayRepo.GetByUUID into a generic "gateway not found"; change the logic to propagate actual repository errors (return the err) when GetByUUID returns a non-nil error, and only return a "gateway not found" error when err is nil but gateway == nil or when gateway.OrganizationID != orgID; update the gateway retrieval block around s.gatewayRepo.GetByUUID and the subsequent OrganizationID check so backend failures are not masked and only true not-found/unauthorized cases produce the not-found error.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/utils/api_utils.go (1)
602-625: Reuses.clientinstead of spinning up a second HTTP transport here.This method creates a duplicate client, bypassing the shared configuration from
NewAPIUtilsService. The new client lacksMinVersion: tls.VersionTLS12and connection pool tuning (MaxIdleConns,MaxIdleConnsPerHost,MaxConnsPerHost), causing manifest POSTs to drift from the shared timeout/TLS/pooling settings. Using the existing client keeps all control-plane calls consistent and avoids transport duplication.Suggested cleanup
- client := &http.Client{ - Timeout: s.config.Timeout, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: s.config.InsecureSkipVerify, - }, - }, - } - req, err := http.NewRequest("POST", url, bytes.NewBuffer(jsonData)) if err != nil { return fmt.Errorf("failed to create manifest request: %w", err) } @@ - resp, err := client.Do(req) + resp, err := s.client.Do(req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/utils/api_utils.go` around lines 602 - 625, The code creates a new http.Client/Transport instead of reusing the shared client on the service; replace the local client creation with the service's existing HTTP client (s.client) so all calls inherit TLS settings (e.g. MinVersion: tls.VersionTLS12) and connection pool tuning (MaxIdleConns, MaxIdleConnsPerHost, MaxConnsPerHost) established in NewAPIUtilsService; update the function that builds and sends the manifest request (the block creating http.NewRequest and calling client.Do) to call s.client.Do(req) and remove the local Transport/HTTP client construction to avoid duplicating transports and diverging settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-controller/pkg/utils/policy_loader.go`:
- Around line 201-209: The custom policy map is keyed only by entry.Name causing
different versions to collide; change the keying to include version (e.g., key
:= entry.Name + "|" + entry.Version) wherever the map customPolicies is
populated in the lock.Policies loop and ensure LocalPolicyNames/any lookup logic
(which currently checks customPolicies[entry.Name]) uses the same "name|version"
composite key so ownership is resolved per name+version rather than just name.
---
Duplicate comments:
In `@platform-api/src/internal/service/gateway.go`:
- Around line 41-43: The comment on GatewayPolicyInput is stale: it incorrectly
states IsCustomPolicy "is not stored or returned" while the field is persisted
in GatewayPolicyDefinition and included in returned manifest data; update the
doc comment for GatewayPolicyInput to state that IsCustomPolicy is stored
(persisted to GatewayPolicyDefinition) and propagated in the returned
manifest/response so it accurately reflects current behavior.
- Around line 91-97: The current code collapses repository errors from
s.gatewayRepo.GetByUUID into a generic "gateway not found"; change the logic to
propagate actual repository errors (return the err) when GetByUUID returns a
non-nil error, and only return a "gateway not found" error when err is nil but
gateway == nil or when gateway.OrganizationID != orgID; update the gateway
retrieval block around s.gatewayRepo.GetByUUID and the subsequent OrganizationID
check so backend failures are not masked and only true not-found/unauthorized
cases produce the not-found error.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/utils/api_utils.go`:
- Around line 602-625: The code creates a new http.Client/Transport instead of
reusing the shared client on the service; replace the local client creation with
the service's existing HTTP client (s.client) so all calls inherit TLS settings
(e.g. MinVersion: tls.VersionTLS12) and connection pool tuning (MaxIdleConns,
MaxIdleConnsPerHost, MaxConnsPerHost) established in NewAPIUtilsService; update
the function that builds and sends the manifest request (the block creating
http.NewRequest and calling client.Do) to call s.client.Do(req) and remove the
local Transport/HTTP client construction to avoid duplicating transports and
diverging settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20f51980-a374-4fe0-81d0-356992eb7dda
📒 Files selected for processing (10)
gateway/gateway-builder/templates/Dockerfile.gateway-controller.tmplgateway/gateway-controller/api/openapi-management.yamlgateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/utils/api_utils.gogateway/gateway-controller/pkg/utils/policy_loader.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/service/gateway.go
✅ Files skipped from review due to trivial changes (2)
- gateway/gateway-builder/templates/Dockerfile.gateway-controller.tmpl
- gateway/gateway-controller/api/openapi-management.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- platform-api/src/internal/repository/gateway.go
- gateway/gateway-controller/pkg/controlplane/client.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
gateway/gateway-controller/pkg/controlplane/client.go (2)
371-376:⚠️ Potential issue | 🟠 MajorAdd a re-sync path, not just the connect-time POST.
Line 371 only schedules a one-shot upload during
Connect(). If that POST fails after retries or the control plane later asks for a resend, there is still no event path that invokes this helper again, so manifest sync cannot recover until the next reconnect. Wire the same helper into the manifest-request branch inhandleMessage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/controlplane/client.go` around lines 371 - 376, The current Connect() only schedules a one-shot manifest upload via pushGatewayManifestOnConnect(gatewayID), so failed POSTs or later manifest-request messages won't trigger a resend; update handleMessage to call the same helper in the manifest-request branch so requests from the control plane re-trigger pushGatewayManifestOnConnect, and ensure the call follows the same goroutine/waitgroup pattern used in Connect() (c.wg.Add(1); go func(gwID string){ defer c.wg.Done(); c.pushGatewayManifestOnConnect(gwID) }(gatewayID)) so retries and concurrency semantics are consistent.
3074-3076:⚠️ Potential issue | 🟡 MinorMake the retry backoff cancellation-aware.
Line 3075 blocks on
time.Sleep, soStop()can sit inwg.Wait()until the current backoff finishes even afterc.ctxorstopChanhas been closed.Suggested fix
if attempt < maxRetries { - time.Sleep(time.Duration(attempt) * 2 * time.Second) + select { + case <-time.After(time.Duration(attempt) * 2 * time.Second): + case <-c.ctx.Done(): + return + case <-c.stopChan: + return + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/controlplane/client.go` around lines 3074 - 3076, The retry backoff currently blocks on time.Sleep(attempt*2s) which prevents Stop() from returning while wg.Wait() waits; replace the blocking sleep in the retry loop (the block using attempt and maxRetries) with a cancellation-aware wait: create a timer via time.After or time.NewTimer for time.Duration(attempt)*2*time.Second and use a select that listens on the timer channel AND on c.ctx.Done() (and stopChan if present) so you break out immediately when cancellation occurs; ensure you stop the timer when canceled and return or break the retry loop promptly to allow Stop()/wg.Wait() to complete.gateway/gateway-controller/pkg/utils/policy_loader.go (1)
201-209:⚠️ Potential issue | 🟠 MajorKey the local-policy set by
name|version.Line 203 and Line 209 collapse every build-lock entry for a policy name into one boolean. If only one version of a policy is local, the downstream ownership lookup will still mark the other versions as
customer, so the manifest reports the wrongmanagedBy. The lookup that consumesLocalPolicyNamesneeds to use the same composite key.Suggested fix
for _, entry := range lock.Policies { + key := entry.Name + "|" + entry.Version if entry.FilePath != "" { - customPolicies[entry.Name] = true + customPolicies[key] = true pl.logger.Debug("Detected locally developed policy via filePath", slog.String("name", entry.Name), slog.String("version", entry.Version), slog.String("filePath", entry.FilePath)) } else if entry.Gomodule != "" && !strings.HasPrefix(entry.Gomodule, "github.com/wso2") { - customPolicies[entry.Name] = true + customPolicies[key] = true pl.logger.Debug("Detected third-party custom policy via gomodule", slog.String("name", entry.Name), slog.String("version", entry.Version), slog.String("gomodule", entry.Gomodule)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/utils/policy_loader.go` around lines 201 - 209, The map customPolicies currently keys only by entry.Name causing different versions to collapse; change the key to the composite "name|version" for all places that mark local policies (i.e., when iterating lock.Policies in the block using entry.FilePath and entry.Gomodule) so you set customPolicies[entry.Name + "|" + entry.Version] = true (and ensure any consumer expecting LocalPolicyNames uses the same composite key). Update any other assignments/readers of LocalPolicyNames/ customPolicies to use this composite key to preserve per-version ownership.platform-api/src/internal/handler/gateway_internal.go (1)
541-555: 🛠️ Refactor suggestion | 🟠 MajorUse the existing
authenticateRequesthelper to avoid duplicating auth logic.This inline authentication duplicates the logic in
authenticateRequest(lines 60-82). The existing helper already distinguishes between missing API key, invalid token, and internal errors, returning appropriate status codes (401 vs 500).♻️ Suggested refactor
func (h *GatewayInternalAPIHandler) ReceiveGatewayManifest(c *gin.Context) { - clientIP := c.ClientIP() - - apiKey := c.GetHeader("api-key") - if apiKey == "" { - h.slogger.Warn("Unauthorized access attempt - Missing API key", "clientIP", clientIP) - c.JSON(http.StatusUnauthorized, utils.NewErrorResponse(401, "Unauthorized", - "API key is required. Provide 'api-key' header.")) - return - } - - gateway, err := h.gatewayService.VerifyToken(apiKey) - if err != nil { - h.slogger.Warn("Authentication failed", "clientIP", clientIP, "error", err) - c.JSON(http.StatusUnauthorized, utils.NewErrorResponse(401, "Unauthorized", - "Invalid or expired API key")) - return - } + _, authenticatedGatewayID, ok := h.authenticateRequest(c) + if !ok { + return + } gatewayID := c.Param("gatewayId") - if gatewayID == "" || gatewayID != gateway.ID { + if gatewayID == "" || gatewayID != authenticatedGatewayID { c.JSON(http.StatusForbidden, utils.NewErrorResponse(403, "Forbidden", "Gateway ID mismatch")) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/handler/gateway_internal.go` around lines 541 - 555, The inline auth block duplicates authenticateRequest; replace the manual header read and VerifyToken call in gateway_internal.go with a call to authenticateRequest(c) and use its returned values (e.g., token/gateway and error/status) instead of reimplementing logic; ensure you preserve clientIP logging (h.slogger) and respond using the status/error returned by authenticateRequest (so missing key, invalid token, and internal errors map to the helper's 401/500 behavior) and remove the duplicated apiKey/gatewayService.VerifyToken code.platform-api/src/internal/service/gateway.go (2)
41-43:⚠️ Potential issue | 🟡 MinorComment contradicts implementation.
The comment states
IsCustomPolicy"is used only for filtering and is not stored or returned," butIsCustomPolicyis persisted inGatewayPolicyDefinition(line 58) and included in the stored manifest JSON.📝 Suggested fix
-// GatewayPolicyInput is the raw policy data received from the gateway controller. -// IsCustomPolicy is used only for filtering and is not stored or returned. +// GatewayPolicyInput is the raw policy data received from the gateway controller. +// IsCustomPolicy indicates whether this is a user-installed custom policy. type GatewayPolicyInput struct {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/gateway.go` around lines 41 - 43, The comment on GatewayPolicyInput is inaccurate: the IsCustomPolicy field is not only used for filtering but is persisted in GatewayPolicyDefinition and included in the stored manifest JSON; update the comment for GatewayPolicyInput to state that IsCustomPolicy is persisted and stored (referencing GatewayPolicyDefinition and the manifest storage) so the documentation matches the implementation.
91-97:⚠️ Potential issue | 🟠 MajorSeparate repository errors from "gateway not found" responses.
Collapsing
err != nilandgateway == nilinto the same "gateway not found" error hides actual repository failures (e.g., database connectivity issues) and returns misleading responses. Repository errors should surface as internal server errors.🐛 Suggested fix
func (s *GatewayService) GetStoredManifest(gatewayID, orgID string) (*Manifest, error) { gateway, err := s.gatewayRepo.GetByUUID(gatewayID) - if err != nil || gateway == nil { + if err != nil { + return nil, fmt.Errorf("failed to get gateway: %w", err) + } + if gateway == nil { return nil, errors.New("gateway not found") } if gateway.OrganizationID != orgID { return nil, errors.New("gateway not found") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/gateway.go` around lines 91 - 97, The code currently treats any repository error the same as a missing gateway; update the GetByUUID handling so that if s.gatewayRepo.GetByUUID(gatewayID) returns a non-nil err you return/surface that error (or wrap it as an internal repository error) while only returning a "gateway not found" response when gateway == nil or when gateway.OrganizationID != orgID; specifically change the block around s.gatewayRepo.GetByUUID to first check err and return it (or an internal error) then check gateway == nil and finally verify gateway.OrganizationID against orgID.platform-api/src/resources/gateway-internal-api.yaml (1)
750-757:⚠️ Potential issue | 🟠 MajorAdd
maxItemsconstraint and requireisCustomPolicy.Per static analysis (CKV_OPENAPI_21), the
policiesarray lacks bounds, which poses a DoS risk from oversized payloads. Additionally,isCustomPolicyshould be required per the endpoint contract described in the PR objectives.♻️ Suggested fix
policies: type: array description: All policies installed on the gateway. + maxItems: 1000 items: type: object required: - name - version + - isCustomPolicy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/resources/gateway-internal-api.yaml` around lines 750 - 757, The policies array definition is missing an upper bound and doesn't require isCustomPolicy; update the OpenAPI fragment for the "policies" schema so the array includes a sensible maxItems (e.g., maxItems: 100) and inside items ensure the object required list includes "isCustomPolicy" and that items.properties contains an "isCustomPolicy" boolean; modify the schema referenced by policies to add maxItems: 100 and add "isCustomPolicy" to the required array for the item object (and define its type as boolean) so the endpoint contract and CKV_OPENAPI_21 constraint are satisfied.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/utils/api_utils.go (1)
600-607: Reuse the shared API client for manifest POSTs.Line 600 spins up a second
http.Client, so this path bypasses the transport hardening and pool settings configured inNewAPIUtilsServiceand gives manifest sync a different TLS profile than the rest ofAPIUtilsService. Send this request throughs.clientinstead of rebuilding the transport here.Suggested fix
- client := &http.Client{ - Timeout: s.config.Timeout, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: s.config.InsecureSkipVerify, - }, - }, - } - req, err := http.NewRequest("POST", url, bytes.NewBuffer(jsonData)) if err != nil { return fmt.Errorf("failed to create manifest request: %w", err) } @@ - resp, err := client.Do(req) + resp, err := s.client.Do(req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/utils/api_utils.go` around lines 600 - 607, The manifest POST currently creates a new http.Client with its own TLS/transport settings (the block that constructs client := &http.Client{ Timeout: s.config.Timeout, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ InsecureSkipVerify: s.config.InsecureSkipVerify, }, }, }), which bypasses the shared, hardened transport created in NewAPIUtilsService; change the manifest sync code to use the existing s.client (the shared http.Client on APIUtilsService) for the request so it inherits the configured transport, pool, and TLS profile instead of constructing a new client locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform-api/src/internal/handler/gateway_internal.go`:
- Around line 614-617: The gatewayGroup route block is declared inside the
mcpProxyGroup block which is misleading; move the gatewayGroup creation and its
POST registration (gatewayGroup := r.Group("/api/internal/v1/gateways") and
gatewayGroup.POST("/:gatewayId/manifest", h.ReceiveGatewayManifest)) out of the
mcpProxyGroup { ... } scope so it sits alongside the other top-level route
groups (using r as the router) to match the file's routing structure and avoid
incorrect nesting.
In `@platform-api/src/resources/gateway-internal-api.yaml`:
- Line 719: The OpenAPI path key
"/api/internal/v1/gateways/{gatewayId}/manifest" redundantly repeats the base
path defined in servers[].url; change the path key to remove the duplicated
prefix so it matches other paths (e.g., use "/gateways/{gatewayId}/manifest"
instead of "/api/internal/v1/gateways/{gatewayId}/manifest") so the effective
route becomes servers[].url + "/gateways/{gatewayId}/manifest"; update the path
entry associated with that string in the document (the path key itself) and
verify no other endpoints repeat the base prefix.
---
Duplicate comments:
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 371-376: The current Connect() only schedules a one-shot manifest
upload via pushGatewayManifestOnConnect(gatewayID), so failed POSTs or later
manifest-request messages won't trigger a resend; update handleMessage to call
the same helper in the manifest-request branch so requests from the control
plane re-trigger pushGatewayManifestOnConnect, and ensure the call follows the
same goroutine/waitgroup pattern used in Connect() (c.wg.Add(1); go func(gwID
string){ defer c.wg.Done(); c.pushGatewayManifestOnConnect(gwID) }(gatewayID))
so retries and concurrency semantics are consistent.
- Around line 3074-3076: The retry backoff currently blocks on
time.Sleep(attempt*2s) which prevents Stop() from returning while wg.Wait()
waits; replace the blocking sleep in the retry loop (the block using attempt and
maxRetries) with a cancellation-aware wait: create a timer via time.After or
time.NewTimer for time.Duration(attempt)*2*time.Second and use a select that
listens on the timer channel AND on c.ctx.Done() (and stopChan if present) so
you break out immediately when cancellation occurs; ensure you stop the timer
when canceled and return or break the retry loop promptly to allow
Stop()/wg.Wait() to complete.
In `@gateway/gateway-controller/pkg/utils/policy_loader.go`:
- Around line 201-209: The map customPolicies currently keys only by entry.Name
causing different versions to collapse; change the key to the composite
"name|version" for all places that mark local policies (i.e., when iterating
lock.Policies in the block using entry.FilePath and entry.Gomodule) so you set
customPolicies[entry.Name + "|" + entry.Version] = true (and ensure any consumer
expecting LocalPolicyNames uses the same composite key). Update any other
assignments/readers of LocalPolicyNames/ customPolicies to use this composite
key to preserve per-version ownership.
In `@platform-api/src/internal/handler/gateway_internal.go`:
- Around line 541-555: The inline auth block duplicates authenticateRequest;
replace the manual header read and VerifyToken call in gateway_internal.go with
a call to authenticateRequest(c) and use its returned values (e.g.,
token/gateway and error/status) instead of reimplementing logic; ensure you
preserve clientIP logging (h.slogger) and respond using the status/error
returned by authenticateRequest (so missing key, invalid token, and internal
errors map to the helper's 401/500 behavior) and remove the duplicated
apiKey/gatewayService.VerifyToken code.
In `@platform-api/src/internal/service/gateway.go`:
- Around line 41-43: The comment on GatewayPolicyInput is inaccurate: the
IsCustomPolicy field is not only used for filtering but is persisted in
GatewayPolicyDefinition and included in the stored manifest JSON; update the
comment for GatewayPolicyInput to state that IsCustomPolicy is persisted and
stored (referencing GatewayPolicyDefinition and the manifest storage) so the
documentation matches the implementation.
- Around line 91-97: The code currently treats any repository error the same as
a missing gateway; update the GetByUUID handling so that if
s.gatewayRepo.GetByUUID(gatewayID) returns a non-nil err you return/surface that
error (or wrap it as an internal repository error) while only returning a
"gateway not found" response when gateway == nil or when gateway.OrganizationID
!= orgID; specifically change the block around s.gatewayRepo.GetByUUID to first
check err and return it (or an internal error) then check gateway == nil and
finally verify gateway.OrganizationID against orgID.
In `@platform-api/src/resources/gateway-internal-api.yaml`:
- Around line 750-757: The policies array definition is missing an upper bound
and doesn't require isCustomPolicy; update the OpenAPI fragment for the
"policies" schema so the array includes a sensible maxItems (e.g., maxItems:
100) and inside items ensure the object required list includes "isCustomPolicy"
and that items.properties contains an "isCustomPolicy" boolean; modify the
schema referenced by policies to add maxItems: 100 and add "isCustomPolicy" to
the required array for the item object (and define its type as boolean) so the
endpoint contract and CKV_OPENAPI_21 constraint are satisfied.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/utils/api_utils.go`:
- Around line 600-607: The manifest POST currently creates a new http.Client
with its own TLS/transport settings (the block that constructs client :=
&http.Client{ Timeout: s.config.Timeout, Transport: &http.Transport{
TLSClientConfig: &tls.Config{ InsecureSkipVerify: s.config.InsecureSkipVerify,
}, }, }), which bypasses the shared, hardened transport created in
NewAPIUtilsService; change the manifest sync code to use the existing s.client
(the shared http.Client on APIUtilsService) for the request so it inherits the
configured transport, pool, and TLS profile instead of constructing a new client
locally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1a3de76-cd6c-49e4-a6bf-937b014c9e8a
📒 Files selected for processing (20)
gateway/gateway-builder/templates/Dockerfile.gateway-controller.tmplgateway/gateway-controller/api/openapi-management.yamlgateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/utils/api_utils.gogateway/gateway-controller/pkg/utils/policy_loader.goplatform-api/src/config/config.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/handler/gateway.goplatform-api/src/internal/handler/gateway_internal.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/server/server.goplatform-api/src/internal/service/gateway.goplatform-api/src/resources/gateway-internal-api.yamlplatform-api/src/resources/openapi.yaml
✅ Files skipped from review due to trivial changes (5)
- gateway/gateway-builder/templates/Dockerfile.gateway-controller.tmpl
- platform-api/src/internal/database/schema.sql
- gateway/gateway-controller/pkg/config/config.go
- gateway/gateway-controller/api/openapi-management.yaml
- platform-api/src/resources/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- platform-api/src/internal/database/schema.sqlite.sql
- gateway/gateway-controller/cmd/controller/main.go
- platform-api/src/internal/repository/interfaces.go
- platform-api/src/internal/database/schema.postgres.sql
- platform-api/src/internal/repository/gateway.go
- platform-api/src/internal/server/server.go
- platform-api/src/config/config.go
- platform-api/src/internal/handler/gateway.go
changes to resolve conflicts generated go file update
malinthaprasan
approved these changes
Mar 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Overview
Implements an async mechanism for the Platform API to request and retrieve the list of installed policies (gateway manifest) from a connected gateway controller. This enables Choreo product APIM to discover which policies are available on a given gateway.
How It Works
gateways/{gwID}/manifestendpoint is called the stored JSON will be returned to the client.Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment
Summary by CodeRabbit
New Features
Chores