Add support for registering/ revoking API Keys generated externally in gateways#845
Conversation
Thushani-Jayasekera
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
WalkthroughAdds end-to-end API key management: in-memory and SQLite stores with external-key indexing and hashing, new create/update/revoke REST endpoints and events, schema migration, validation utilities, xDS/SDK contract changes, build-context updates, and platform/event broadcasting. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PlatformAPI as Platform API<br/>Handler
participant APIService as APIKeyService
participant GatewayEvents as Gateway Events<br/>Service
participant GatewayControl as Gateway Control Plane
participant GatewayStore as Gateway APIKey Store
participant PolicyEngine as Policy Engine / xDS
Client->>PlatformAPI: POST /apis/{id}/api-keys (create/inject)
PlatformAPI->>APIService: CreateAPIKey(apiId, dto)
APIService->>APIService: validate API & deployments
APIService->>GatewayEvents: BroadcastAPIKeyCreatedEvent(gatewayID,event)
loop per gateway
GatewayEvents->>GatewayControl: deliver "apikey.created"
GatewayControl->>GatewayStore: StoreAPIKey(apiId, apiKey)
GatewayStore->>GatewayStore: compute indexKey / hash / update externalKeyIndex
GatewayControl->>PolicyEngine: push xDS update (APIKeyData with Source/IndexKey)
PolicyEngine->>PolicyEngine: apply key data to runtime
end
APIService->>PlatformAPI: result / summary
PlatformAPI->>Client: 201 Created (or partial-failure)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway/gateway-controller/pkg/controlplane/client.go (1)
509-513: Don’t log raw event payloads that include API keys.
apikey.createdcarries the plain API key; logging full payloads at Info level leaks secrets. Remove or redact the key before logging.🛡️ Suggested redaction
- c.logger.Info("Received WebSocket event", - slog.String("type", eventType), - slog.String("payload", string(message)), - ) + c.logger.Info("Received WebSocket event", + slog.String("type", eventType), + )- c.logger.Info("API Key Created Event", - slog.Any("payload", event["payload"]), - slog.Any("timestamp", event["timestamp"]), - slog.Any("correlationId", event["correlationId"]), - ) + c.logger.Info("API Key Created Event", + slog.Any("timestamp", event["timestamp"]), + slog.Any("correlationId", event["correlationId"]), + )Also applies to: 628-632
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2406-2419: Map duplicate key creation to HTTP 409CreateAPIKey currently returns 500 for any non‑“not found” error. External key injection collisions should surface as 409 Conflict. Consider detecting
storage.ErrConflict(and ensure the service wraps it) before the generic 500 path.🛠️ Suggested handler mapping
- result, err := s.apiKeyService.CreateAPIKey(params) + result, err := s.apiKeyService.CreateAPIKey(params) if err != nil { + if errors.Is(err, storage.ErrConflict) { + c.JSON(http.StatusConflict, api.ErrorResponse{ + Status: "error", + Message: err.Error(), + }) + return + } // Check error type to determine appropriate status code if strings.Contains(err.Error(), "not found") { c.JSON(http.StatusNotFound, api.ErrorResponse{🔧 Import addition
import ( + "errors" "context" "encoding/json"
🤖 Fix all issues with AI agents
In `@common/apikey/store.go`:
- Around line 173-200: Normalize missing api key Source to "local" before
matching: when parsing or iterating keys in the lookup logic that uses
parseAPIKey, aks.apiKeysByAPI and compareAPIKeys, treat any apiKey with an empty
Source as Source == "local" so legacy keys match ValidateAPIKey's allowed
values; apply the same fallback in the RevokeAPIKey local-key path (i.e., when
loading apiKey.Source == "" set it to "local" before comparing or checking
Source == "local").
In `@gateway/gateway-controller/api/openapi.yaml`:
- Around line 2232-2240: The OpenAPI schema for api_key is missing the enforced
minimum length; update the api_key schema in gateway-controller/api/openapi.yaml
(the api_key property block) to include minLength: 16 so the contract matches
the service validation for externally provided API keys.
In `@gateway/gateway-controller/pkg/storage/sqlite.go`:
- Around line 220-262: The v6 migration unconditionally runs ALTER TABLE ADD
COLUMN for source and external_ref_id which will fail on fresh DBs because those
columns may already exist; update the version==5 migration block to mirror the
masked_api_key pattern by querying pragma_table_info('api_keys') with
s.db.QueryRow to check for the existence of each column (e.g., check for
name='source' and name='external_ref_id') and only run the corresponding
s.db.Exec ALTER TABLE statements (the ones that add "source TEXT NOT NULL
DEFAULT 'local'" and "external_ref_id TEXT NULL") if the columnExists count is
0, leaving the index creation and PRAGMA user_version logic unchanged.
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 206-216: When SaveAPIKey returns storage.ErrConflict for an
external key (inside the isExternalKeyInjection branch where you currently
return fmt.Errorf("the provided API key already exists in the system")),
preserve the sentinel by returning a wrapped error that retains
storage.ErrConflict (e.g., use fmt.Errorf("…: %w", storage.ErrConflict) or
errors.Join/Wrap) so the HTTP layer can detect ErrConflict; update the return in
the isExternalKeyInjection branch where s.db.SaveAPIKey(apiKey) is checked and
keep the existing logger.Error call (referencing s.db.SaveAPIKey,
storage.ErrConflict, isExternalKeyInjection, params.Handle, operationType,
params.CorrelationID).
- Around line 1732-1815: RevokeExternalAPIKeyFromEvent currently assumes s.db
and s.xdsManager are non-nil and only updates the DB/xDS, leaving the in-memory
ConfigStore stale; update the function to first nil-guard optional dependencies
(return or skip DB/xDS steps if s.db or s.xdsManager are nil while still
performing idempotent checks) and after setting targetKey.Status =
models.APIKeyStatusRevoked and persisting via s.db.UpdateAPIKey, also remove the
revoked key from the in-memory config (via the retrieved config object from
s.store: remove the matching key by Name or ID from the API keys list and
persist the config back to s.store with the existing store method), and only
then attempt s.xdsManager.RevokeAPIKey inside a nil-check so failures do not
crash; reference symbols: RevokeExternalAPIKeyFromEvent, s.store, config, s.db,
UpdateAPIKey, s.xdsManager, RevokeAPIKey, and models.APIKeyStatusRevoked.
- Around line 1577-1730: The CreateExternalAPIKeyFromEvent function updates the
DB and xDS but never updates the in‑memory ConfigStore and assumes optional
components (s.db, s.xdsManager) are non‑nil; fix by (1) guarding calls to s.db
and s.xdsManager with nil checks before using them, (2) after successfully
updating an existing key (inside the loop where key.Name == keyName) and after
saving a newly created apiKey, also upsert the same APIKey into the in‑memory
ConfigStore via s.store (use the store's API key upsert/save method), and (3)
keep the existing behavior of not failing the operation if
xdsManager.StoreAPIKey returns an error but still log it; apply these changes in
the CreateExternalAPIKeyFromEvent function to both the update branch and the
create branch.
In `@platform-api/src/internal/service/apikey.go`:
- Around line 71-82: The current code always sets operations := "[\"*\"]" and
ignores req.Operations, which over-grants permissions; change the logic in the
API key creation path (the variable operations used when constructing
model.APIKeyCreatedEvent) to use req.Operations when it is non-empty/non-nil and
only set the default "[\"*\"]" fallback if req.Operations is empty; update the
assignment before building the event so APIKeyCreatedEvent.Operations receives
either req.Operations or the wildcard default.
In `@platform-api/src/internal/service/gateway_events.go`:
- Around line 219-249: The retry loop in BroadcastAPIKeyCreatedEvent uses
maxRetries = 1 so it never actually retries; update the retry policy to match
the intended behavior by increasing maxRetries to a value >1 (e.g., 3) or make
it configurable, and ensure retryDelay/backoff remains correct; change the
constant in BroadcastAPIKeyCreatedEvent (and mirror the same fix in the similar
function around lines 260-291) so the loop will perform multiple attempts and
the logging/returned error reflect the configured retry count.
🧹 Nitpick comments (12)
platform-api/src/internal/service/gateway_events.go (1)
293-366: Consider extracting the shared broadcast/send loop.
The created/revoked broadcasters duplicate serialization, payload checks, send loops, and delivery stats updates. A small helper that accepts event type + payload would reduce drift risk.Also applies to: 368-441
gateway/gateway-builder/Dockerfile (1)
40-41: Rebuild gateway images after this gateway-component change.
Runcd gateway && make build-localto ensure the new build context is picked up.As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/policy-engine/Dockerfile (1)
41-42: Remember to rebuild gateway images after this Dockerfile change.Please run
cd gateway && make build-localso the policy-engine image picks up the new common-module build context.As per coding guidelines, When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local.Also applies to: 76-76
gateway/policy-engine/internal/xdsclient/api_key_handler.go (1)
21-33: Run the gateway build after these changes.This touches gateway components, so please rebuild the image using
cd gateway && make build-local.
As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.common/apikey/store.go (1)
95-110:externalKeyIndexis unused — wire it in or drop it.The index is initialized but never populated/cleared or queried in validation/revocation. That makes it dead code now and a future foot‑gun if relied on later. Either implement updates in
StoreAPIKey/removeFromAPIMapping/ClearAlland use it for external lookups, or remove it until it’s needed.gateway/gateway-controller/pkg/controlplane/client.go (1)
106-134: Run the gateway build after these changes.Gateway-controller changes require rebuilding the image:
cd gateway && make build-local.
As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/storage/sqlite.go (1)
1181-1241: Run the gateway build after these changes.Gateway-controller changes require rebuilding the image:
cd gateway && make build-local.
As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/controlplane/events.go (2)
46-46: Rebuild gateway images after control-plane event schema changesPlease run
cd gateway && make build-localso the updated event types are included in the gateway-controller image. As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.
46-77: Align API key event field names with Go initialisms and track the expiry TODO
ApiId/ApiKey/ExternalRefIddiverge from Go initialism style and the existingAPIDeployedEventPayload.APIID. Renaming improves consistency and avoids lint noise. Also, please track theexpires_inTODO with a follow‑up issue so it doesn’t get lost.♻️ Suggested initialism alignment
type APIKeyCreatedEventPayload struct { - ApiId string `json:"apiId"` + APIID string `json:"apiId"` KeyName string `json:"keyName"` - ApiKey string `json:"apiKey"` - ExternalRefId *string `json:"externalRefId,omitempty"` + APIKey string `json:"apiKey"` + ExternalRefID *string `json:"externalRefId,omitempty"` Operations string `json:"operations"` ExpiresAt *string `json:"expiresAt,omitempty"` // ISO 8601 format // TODO: Support expires in field } ... type APIKeyRevokedEventPayload struct { - ApiId string `json:"apiId"` + APIID string `json:"apiId"` KeyName string `json:"keyName"` }gateway/gateway-controller/api/openapi.yaml (1)
352-352: Rebuild gateway image after OpenAPI contract updatesPlease run
cd gateway && make build-localso the updated OpenAPI contract is included in the gateway build. As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2363-2363: Rebuild gateway image after handler changesPlease run
cd gateway && make build-localso the updated handlers are included in the gateway build. As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/utils/api_key.go (1)
46-46: Rebuild gateway image after API key service updatesPlease run
cd gateway && make build-localso the updated API key utilities are included in the gateway build. As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/storage/sqlite.go`:
- Around line 258-268: The UPDATE in the s.db.Exec call that backfills legacy
API keys incorrectly includes "source != 'local'", which prevents matching
NULLs; remove that "source != 'local'" condition so the WHERE clause is just the
OR-combined checks (source IS NULL OR trim(source) = '' OR lower(trim(source)) =
'null') in the UPDATE statement invoked via s.db.Exec to ensure NULL and
empty/'null' values are updated to 'local'.
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 1577-1768: The update branch in CreateExternalAPIKeyFromEvent
overwrites key.ExternalRefId even when the incoming externalRefId is nil,
potentially erasing an existing reference; change the update logic in the
existing-key path (where key is updated and fields like APIKey, MaskedAPIKey,
Operations, Status, UpdatedAt, ExpiresAt, Source are set) to only assign
key.ExternalRefId = externalRefId when externalRefId != nil (leave the existing
value otherwise) so that absent event fields do not clear stored ExternalRefId.
In `@platform-api/src/internal/handler/api_key.go`:
- Around line 46-118: The CreateAPIKey handler currently returns
dto.CreateAPIKeyResponse.KeyId set to req.Name which is wrong; update the flow
so h.apiKeyService.CreateAPIKey (or a new variant) returns the gateway-generated
key ID and use that value in the CreateAPIKey response, or remove KeyId from the
response until the service provides the real ID—modify the call to
h.apiKeyService.CreateAPIKey to return (string, error) or fetch the created ID,
replace usage of req.Name when building dto.CreateAPIKeyResponse, and if you
choose to omit it, remove KeyId from the returned struct instance (and adjust
dto if necessary).
🧹 Nitpick comments (4)
common/apikey/store.go (2)
80-88: Align the effectiveSource comment with behavior.The comment mentions
"null"but the code only treats empty as local. Either handle"null"or drop the mention to avoid confusion.✏️ Suggested comment tweak
-// effectiveSource returns the effective source for matching: empty or "null" is treated as "local" for legacy keys. +// effectiveSource returns the effective source for matching: empty is treated as "local" for legacy keys.
105-120: externalKeyIndex isn’t wired up yet.It’s initialized but never populated/cleared, so it can’t deliver the intended O(1) external lookups. Consider maintaining it in Store/Revoke/Clear or remove it until it’s used.
gateway/gateway-controller/pkg/utils/api_key.go (1)
46-61: New creation params/result structs are clear.
Reminder: rebuild gateway images after these gateway changes (cd gateway && make build-local).As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}— When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/storage/sqlite.go (1)
220-283: Reminder: rebuild gateway images after gateway changes.Please make sure the gateway Docker images are rebuilt with
cd gateway && make build-local.As per coding guidelines,
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.
Thushani-Jayasekera
changed the title
Thushani-Jayasekera
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-controller/pkg/controlplane/client.go (1)
509-513: Stop logging plaintext API keys.
The payload logs include the rawapiKeyfrom apikey.created/updated events, which leaks secrets into logs. Please redact or omit those fields.🔒 Redact API keys from logs
@@ import ( "context" "crypto/tls" "encoding/json" "fmt" "log/slog" "net/http" "sync" "sync/atomic" "time" + "strings" @@ - c.logger.Info("Received WebSocket event", - slog.String("type", eventType), - slog.String("payload", string(message)), - ) + if strings.HasPrefix(eventType, "apikey.") { + c.logger.Info("Received WebSocket event", + slog.String("type", eventType), + slog.Any("correlation_id", event["correlationId"]), + ) + } else { + c.logger.Info("Received WebSocket event", + slog.String("type", eventType), + slog.String("payload", string(message)), + ) + } @@ - c.logger.Info("API Key Created Event", - slog.Any("payload", event["payload"]), - slog.Any("timestamp", event["timestamp"]), - slog.Any("correlationId", event["correlationId"]), - ) + c.logger.Info("API Key Created Event", + slog.Any("timestamp", event["timestamp"]), + slog.Any("correlationId", event["correlationId"]), + ) @@ - c.logger.Info("API Key Updated Event", - slog.Any("payload", event["payload"]), - slog.Any("timestamp", event["timestamp"]), - slog.Any("correlationId", event["correlationId"]), - ) + c.logger.Info("API Key Updated Event", + slog.Any("timestamp", event["timestamp"]), + slog.Any("correlationId", event["correlationId"]), + )Also applies to: 630-634, 788-792
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/api/openapi.yaml`:
- Around line 505-541: The API contract allows omitting the new key but the
update handler requires it; update the OpenAPI spec for operationId updateAPIKey
to require api_key by creating a dedicated schema (e.g.,
components.schemas.APIKeyUpdateRequest) that sets "required: [\"api_key\"]" and
references that schema in both application/json and application/yaml requestBody
entries (replace APIKeyRegenerationRequest with APIKeyUpdateRequest), or
alternatively add an inline schema under the requestBody that includes
"required: [\"api_key\"]" and the api_key property; apply the same change for
the other update endpoint that currently references APIKeyRegenerationRequest so
invalid update requests are rejected by the contract.
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 499-545: The code dereferences params.Request.ApiKey inside
updateAPIKey without validating it, which can panic or allow short keys; before
calling updateAPIKey from UpdateAPIKey (and in the other call site referenced),
explicitly validate params.Request is non-nil and params.Request.ApiKey is
non-nil and meets the configured minimum length (e.g., compare
len(*params.Request.ApiKey) >= MinAPIKeyLength), log a clear warning via logger
and return a descriptive error if validation fails, and update updateAPIKey to
assume a validated non-nil ApiKey (or defensively check again) so you never
directly dereference a nil pointer or accept keys below the minimum length.
- Around line 2235-2244: When updating targetKey in the API key update code (the
block that sets targetKey.APIKey, targetKey.MaskedAPIKey via s.MaskAPIKey,
targetKey.Status = models.APIKeyStatusActive, targetKey.UpdatedAt, and
targetKey.ExpiresAt = expiresAt), avoid overwriting an existing expiry with a
nil expiresAt: only assign targetKey.ExpiresAt when expiresAt is non-nil (e.g.,
check expiresAt != nil before setting), otherwise leave targetKey.ExpiresAt
unchanged; keep the existing logic that preserves targetKey.Source when empty.
🧹 Nitpick comments (4)
platform-api/src/internal/service/apikey.go (1)
71-82: Document the intentional operations override (and test it).
Line 71 hard‑defaults to["*"]; please add a short note thatreq.Operationsis intentionally ignored in this release and add tests that assert this behavior.Based on learnings: In platform-api/src/internal/service/apikey.go, ensure the CreateAPIKey path intentionally ignores any requested operation-scoping (req.Operations) and always uses ["*"] (all operations); add a code comment and tests.📝 Suggested comment
- operations := "[\"*\"]" // Default to all operations + // NOTE: Operation-level scoping is not supported yet; ignore req.Operations + // and always grant full access. Tests should assert this behavior. + operations := "[\"*\"]" // Default to all operationsplatform-api/src/internal/dto/apikey.go (1)
20-37: Clarify operations format/default.
TheOperationscomment implies a raw"*", but the system uses a stringified JSON array (e.g.,["*"]). A small doc tweak would prevent confusion.📝 Suggested doc tweak
- // Operations specifies which API operations this key can access (default: "*" for all) + // Operations specifies which API operations this key can access as a stringified + // JSON array (default: ["*"] for all) Operations string `json:"operations,omitempty"`gateway/gateway-controller/pkg/controlplane/client.go (1)
19-39: Rebuild gateway images after these changes.
Runcd gateway && make build-localso the gateway-controller image includes the new API key event handling.As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2488-2557: Consider deduplicating UpdateAPIKey and RegenerateAPIKey handler logic.These handlers are nearly identical (binding, logging, error mapping). A shared helper would reduce drift and make future changes safer.
As per coding guidelines, when modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/policy-engine/Dockerfile`:
- Around line 101-102: Update the misleading Dockerfile comment to accurately
reflect the COPY command: the COPY instruction "COPY --from=common .
/api-platform/common" copies the entire source tree, not just Go module files,
so change the comment above that line to say it copies the full common source
(or limit the COPY to only go.mod and go.sum if you intended to cache
dependencies); refer to the COPY --from=common . /api-platform/common line in
the Dockerfile and either adjust the comment text or modify the COPY to target
go.mod and go.sum explicitly.
🧹 Nitpick comments (3)
gateway/gateway-controller/api/openapi.yaml (1)
2422-2456: Consider addingexternal_ref_idtoAPIKeyRegenerationRequestfor consistency.The
APIKeyCreationRequestincludesexternal_ref_idfor tracing externally generated keys, butAPIKeyRegenerationRequestlacks this field. If external keys can be updated/rotated with new values from Cloud APIM, the same tracing capability would be valuable.📝 Suggested schema addition
APIKeyRegenerationRequest: type: object properties: api_key: type: string description: The new API key value to set (minimum 16 characters) example: "apip_1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef" minLength: 16 + external_ref_id: + type: string + description: | + External reference ID for the API key (e.g., Cloud APIM key ID). + This field is optional and used for tracing purposes only. + example: "cloud-apim-key-98765" expires_in: type: objectgateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2506-2510: Minor: Log message says "rotation" but operation is "update".The log message at line 2506 says "Starting API key rotation" and line 2553 says "API key rotation completed", but this is the
UpdateAPIKeyhandler (for external key injection). Consider using "update" terminology for clarity, reserving "rotation" forRegenerateAPIKey.📝 Suggested logging update
- log.Debug("Starting API key rotation", + log.Debug("Starting API key update", slog.String("handle", handle), slog.String("key name", apiKeyName), slog.String("user", user.UserID), slog.String("correlation_id", correlationID))- log.Info("API key rotation completed", + log.Info("API key update completed", slog.String("handle", handle), slog.String("key_name", apiKeyName), slog.String("user", user.UserID), slog.String("correlation_id", correlationID))gateway/policy-engine/Dockerfile (1)
80-83: Potentially redundant copy, but follows existing pattern.Since
policy-compilerinheritsFROM builder-base, the common module is already downloaded and available. This mirrors the existing pattern for SDK (lines 86-89), so it may be intentional for explicit dependency declaration. Consider consolidating if optimizing build times becomes a priority.
| put: | ||
| summary: Update an API key with a new regenerated value |
There was a problem hiding this comment.
Should this be a PUT or POST /apis/{id}/api-keys/{apiKeyName}/regenerate ?
There was a problem hiding this comment.
Since this endpoint is used for external API Keys, and the API Key has already been regenerated in the platform where users manage their keys, they will use this endpoint to update the existing key value in the gateway.
In that case, I assume PUT /apis/{id}/api-keys/{apiKeyName} would be more appropriate?
There was a problem hiding this comment.
+1
So then,
PUT /apis/{id}/api-keys/{apiKeyName}for injecting an API Key with full payload (including expiry times...)
What about regenerating a key for an existing api key, without injecting a key? Is it a POST?
POST /apis/{id}/api-keys/{apiKeyName}/regenerate
cc: @malinthaprasan
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 (2)
gateway/gateway-controller/pkg/controlplane/client.go (1)
509-529:⚠️ Potential issue | 🟠 MajorRedact API key payloads from WebSocket logs.
With apikey.created/updated events, loggingpayloadnow risks leaking plaintext keys into logs. Consider skipping payload logging or redactingapi_keyfor these event types.🔒 Suggested redaction guard
- // Log the event to console - c.logger.Info("Received WebSocket event", - slog.String("type", eventType), - slog.String("payload", string(message)), - ) + // Log the event to console (avoid leaking API keys) + logArgs := []any{slog.String("type", eventType)} + if eventType != "apikey.created" && eventType != "apikey.updated" { + logArgs = append(logArgs, slog.String("payload", string(message))) + } + c.logger.Info("Received WebSocket event", logArgs...)gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2387-2397:⚠️ Potential issue | 🟠 MajorSupport YAML request bodies per OpenAPI specification.
The API specification advertises both
application/yamlandapplication/jsonfor CreateAPIKey and UpdateAPIKey endpoints. UsingShouldBindJSONbreaks the contract for YAML clients. Other handlers in this codebase (e.g., CreateAPI) follow a pattern of reading the raw body and passing the Content-Type header to the service layer for format-aware parsing. Apply the same approach here to accept both JSON and YAML as specified.
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 2512-2523: The UpdateAPIKey handler currently uses
c.ShouldBindJSON(&request) which only supports JSON; change to a content-type
aware bind (e.g., use c.ShouldBindWith(&request, binding.JSON) or binding.YAML
based on c.GetHeader("Content-Type") or use gin's ShouldBindWith with logic that
picks binding.YAML when "application/yaml" or "text/yaml" is present) so both
JSON and YAML are accepted; mirror the same change for CreateAPIKey and
RegenerateAPIKey handlers (replace their ShouldBindJSON calls with the
content-type-aware binding) to keep behavior consistent with the OpenAPI spec,
then rebuild images with: cd gateway && make build-local.
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 770-847: In handleAPIKeyUpdatedEvent update the typo in the
initial log message from "API Key Updated Event recieved" to "API Key Updated
Event received" so the logger.Info call (the one using c.logger.Info with
slog.Any for timestamp and correlationId) uses the correct spelling; locate and
edit that string in the function to improve log searchability.
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 2260-2274: The external-update path currently only validates
apiId, keyName, and plainAPIKey length but doesn't validate the expiresAt
timestamp; add the same expiration validation used by local update/regenerate
flows by checking the expiresAt value (variable expiresAt) against
time.Now().UTC() and returning a clear error when expiresAt is in the past (or
otherwise invalid) so external update events cannot set a past expiration unless
the local/regenerate logic explicitly permits it; place this check near the
existing input checks (after plainAPIKey length) and reuse any existing
expiration-parsing/validation helper if available.
- Around line 523-571: Trim params.Request.ApiKey once into a local variable
(e.g., trimmedKey) before any checks, then use trimmedKey for the emptiness
check, length comparison against minAPIKeyLength, for hashing via s.hashAPIKey
and for masking via s.MaskAPIKey; replace all uses of *params.Request.ApiKey in
this section (validation, hashedAPIKey, maskedAPIKey) with the trimmed value so
whitespace-only keys are rejected and stored values are consistent.
- Around line 1953-1968: The external-create path currently skips validating
expiresAt and can accept past expiration timestamps while still marking keys
active; update the validation logic (around the block validating apiId, keyName,
plainAPIKey) to check the expiresAt value: if expiresAt is non-nil and is before
time.Now(), either return an error (reject external create with past expiration)
or explicitly set the key's active flag to false and adjust any state/returned
fields accordingly (e.g., where the code later marks the key active). Reference
the expiresAt variable and the code that sets the key active to locate where to
add this check, and update unit tests that exercise external create flows to
cover both past and future expiresAt cases.
- Around line 646-669: The DB is updated via s.db.UpdateAPIKey(existingKey)
before writing to s.store.StoreAPIKey, so if StoreAPIKey fails the DB and
ConfigStore become inconsistent; change the order or add a rollback: first
attempt s.store.StoreAPIKey(existingKey) and only then persist to DB, or if you
must update DB first capture the previous DB state (the original API key object
or its fields) before calling s.db.UpdateAPIKey(existingKey) and, if
s.store.StoreAPIKey(existingKey) returns an error, call
s.db.UpdateAPIKey(previousKey) to restore the prior state and return the error;
use the existing symbols existingKey, s.db.UpdateAPIKey, s.store.StoreAPIKey and
propagate/log errors via logger/slog consistently, and handle the s.db == nil
case by skipping DB rollback when there is no persistent DB.
🧹 Nitpick comments (3)
gateway/gateway-controller/tests/integration/schema_test.go (1)
101-107: Update looks correct; remember to rebuild gateway images.The schema version assertion aligns with the v6 migration. Per gateway guidelines, please run
cd gateway && make build-localafter this change.gateway/gateway-controller/api/openapi.yaml (1)
352-385: Remember to rebuild gateway images after spec changes.
Runcd gateway && make build-localso images include the updated OpenAPI contract.As per coding guidelines
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/utils/api_key.go (1)
168-171: Reminder: rebuild gateway images after these changes.Please run
cd gateway && make build-localafter updating gateway components.As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.
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)
gateway/gateway-controller/pkg/controlplane/client.go (1)
509-529:⚠️ Potential issue | 🔴 CriticalAvoid logging API key payloads. The current log prints the full event payload, which will include plaintext API keys for
apikey.*events. Redact or omit the payload for those types to prevent secret leakage.🔒 Suggested redaction
- c.logger.Info("Received WebSocket event", - slog.String("type", eventType), - slog.String("payload", string(message)), - ) + payloadForLog := string(message) + if eventType == "apikey.created" || eventType == "apikey.updated" || eventType == "apikey.revoked" { + payloadForLog = "[redacted]" + } + c.logger.Info("Received WebSocket event", + slog.String("type", eventType), + slog.String("payload", payloadForLog), + )
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 681-683: The ExpiresAt parsing currently uses time.Parse with
time.RFC3339 which fails on timestamps with fractional seconds; update both
parsing sites (the time.Parse calls that consume payload.ExpiresAt—e.g., the
block referencing payload.ExpiresAt and the second occurrence at the other
parse) to use time.RFC3339Nano instead of time.RFC3339 so fractional-second
ISO‑8601 timestamps are accepted; keep the existing error handling around
parsedTime unchanged.
| if payload.ExpiresAt != nil && *payload.ExpiresAt != "" { | ||
| parsedTime, err := time.Parse(time.RFC3339, *payload.ExpiresAt) | ||
| if err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file and read the relevant lines
wc -l gateway/gateway-controller/pkg/controlplane/client.goRepository: wso2/api-platform
Length of output: 118
🏁 Script executed:
# Read the relevant sections
sed -n '675,690p' gateway/gateway-controller/pkg/controlplane/client.go
echo "---"
sed -n '815,830p' gateway/gateway-controller/pkg/controlplane/client.goRepository: wso2/api-platform
Length of output: 1021
🏁 Script executed:
# Also check the entire context around these lines for better understanding
sed -n '670,695p' gateway/gateway-controller/pkg/controlplane/client.goRepository: wso2/api-platform
Length of output: 725
🏁 Script executed:
# Find where payload is defined and created
rg "type.*Payload\|ExpiresAt" gateway/gateway-controller/pkg/controlplane/client.go -B 3 -A 3 | head -50Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Search for where these event payloads come from
rg "ExpiresAt" gateway/gateway-controller/pkg/controlplane/ -t go | grep -E "\.(ExpiresAt|payload\.ExpiresAt)" | head -20Repository: wso2/api-platform
Length of output: 736
🏁 Script executed:
# Check the events package to understand the payload structure
fd "event" gateway/gateway-controller/pkg/ -type f -name "*.go" | head -10Repository: wso2/api-platform
Length of output: 231
🏁 Script executed:
# Let's also verify Go time parsing behavior with RFC3339 vs RFC3339Nano
python3 << 'EOF'
import subprocess
import json
# Create a test Go program to verify time parsing behavior
test_code = '''package main
import (
"fmt"
"time"
)
func main() {
// Test cases
timestamps := []string{
"2024-01-15T10:30:45Z", // RFC3339 without fractional seconds
"2024-01-15T10:30:45.123Z", // RFC3339 with fractional seconds
"2024-01-15T10:30:45.123456789Z", // RFC3339Nano
}
fmt.Println("Testing time.RFC3339:")
for _, ts := range timestamps {
_, err := time.Parse(time.RFC3339, ts)
fmt.Printf(" %s: %v\n", ts, err)
}
fmt.Println("\nTesting time.RFC3339Nano:")
for _, ts := range timestamps {
_, err := time.Parse(time.RFC3339Nano, ts)
fmt.Printf(" %s: %v\n", ts, err)
}
}
'''
print(test_code)
EOFRepository: wso2/api-platform
Length of output: 668
🏁 Script executed:
# Search for the payload type definition
rg "type.*EventPayload|type.*Payload" gateway/gateway-controller/pkg/controlplane/ -A 10 | head -60Repository: wso2/api-platform
Length of output: 4249
🏁 Script executed:
# Look for event definitions in the events package
find gateway/gateway-controller -name "*.go" -exec grep -l "ExpiresAt" {} \;Repository: wso2/api-platform
Length of output: 432
🏁 Script executed:
# Check if there's a proto or schema file defining the event structure
find gateway/gateway-controller -type f \( -name "*.proto" -o -name "*.yaml" -o -name "*.yml" -o -name "*.json" \) | head -20Repository: wso2/api-platform
Length of output: 1479
🏁 Script executed:
# Search broader for payload definitions
rg "ExpiresAt.*string\|ExpiresAt.*Time" gateway/gateway-controller/ -B 2 -A 2Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check the API definition to see what format ExpiresAt is expected to be
grep -A 30 -B 5 "expiresAt\|ExpiresAt" gateway/gateway-controller/api/openapi.yaml | head -80Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Look for where ExpiresAt is generated or set
rg "ExpiresAt.*=" gateway/gateway-controller/ -B 2 -A 2 | head -40Repository: wso2/api-platform
Length of output: 2835
🏁 Script executed:
# Check if there are any tests that show what format ExpiresAt can have
find gateway/gateway-controller -name "*test*" -type f -name "*.go" -exec grep -l "ExpiresAt" {} \;Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Read the openapi.yaml file to see ExpiresAt definition
cat gateway/gateway-controller/api/openapi.yaml | grep -A 5 -B 5 "expiresAt" | head -50Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check if there are examples or test data showing ExpiresAt format
find gateway -type f \( -name "*.go" -o -name "*.json" \) -exec grep -l "expiresAt.*2024\|ExpiresAt.*2024" {} \; | head -10Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check the generated API code for ExpiresAt
rg "ExpiresAt" gateway/gateway-controller/pkg/api/generated/ -A 2 -B 2Repository: wso2/api-platform
Length of output: 1864
🏁 Script executed:
# Verify Go's time parsing behavior: RFC3339 vs RFC3339Nano
cat > /tmp/test_time.go << 'EOF'
package main
import (
"fmt"
"time"
)
func main() {
// Test RFC3339 vs RFC3339Nano
timestamps := []string{
"2024-01-15T10:30:45Z", // RFC3339 format
"2024-01-15T10:30:45.123Z", // With milliseconds
"2024-01-15T10:30:45.123456789Z", // With nanoseconds
}
fmt.Println("RFC3339 parsing:")
for _, ts := range timestamps {
_, err := time.Parse(time.RFC3339, ts)
if err != nil {
fmt.Printf(" ❌ %s: %v\n", ts, err)
} else {
fmt.Printf(" ✅ %s: OK\n", ts)
}
}
fmt.Println("\nRFC3339Nano parsing:")
for _, ts := range timestamps {
_, err := time.Parse(time.RFC3339Nano, ts)
if err != nil {
fmt.Printf(" ❌ %s: %v\n", ts, err)
} else {
fmt.Printf(" ✅ %s: OK\n", ts)
}
}
}
EOF
# Check if Go is available
which goRepository: wso2/api-platform
Length of output: 107
🏁 Script executed:
# Let's check the RFC3339 vs RFC3339Nano constants in Go's source or documentation
# by examining what formats they accept
python3 << 'EOF'
import re
# Go's time format constants
rfc3339 = "2006-01-02T15:04:05Z07:00"
rfc3339_nano = "2006-01-02T15:04:05.999999999Z07:00"
print("RFC3339 format:", rfc3339)
print("RFC3339Nano format:", rfc3339_nano)
print()
print("Difference: RFC3339Nano includes fractional seconds (.999999999)")
print()
# Test cases that would be parsed
test_cases = [
("2024-01-15T10:30:45Z", "Basic RFC3339"),
("2024-01-15T10:30:45.123Z", "With milliseconds"),
("2024-01-15T10:30:45.123456789Z", "With nanoseconds"),
("2024-01-15T10:30:45+05:30", "With timezone offset"),
("2024-01-15T10:30:45.123+05:30", "With fractional seconds and offset"),
]
print("Parsing behavior:")
for ts, desc in test_cases:
# RFC3339 requires no fractional seconds
if '.' in ts:
rfc3339_result = "FAIL (has fractional seconds)"
else:
rfc3339_result = "OK"
# RFC3339Nano accepts everything
rfc3339_nano_result = "OK"
print(f" {desc}: '{ts}'")
print(f" RFC3339: {rfc3339_result}")
print(f" RFC3339Nano: {rfc3339_nano_result}")
EOFRepository: wso2/api-platform
Length of output: 767
🏁 Script executed:
# Check if there's any documentation or comments about the ExpiresAt format in the codebase
rg "ISO.8601\|ISO-8601\|fractional\|millisecond\|nanosecond" gateway/gateway-controller/ -B 2 -A 2Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Look for actual usage patterns - check if events are created with fractional seconds
rg "payload.ExpiresAt\|Event.*Payload" gateway/ -B 5 -A 5 | grep -E "ExpiresAt|time\." | head -30Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Verify both locations in the file need the fix
sed -n '679,685p' gateway/gateway-controller/pkg/controlplane/client.go
echo "---"
sed -n '819,825p' gateway/gateway-controller/pkg/controlplane/client.goRepository: wso2/api-platform
Length of output: 732
🏁 Script executed:
# Check if there are any other time.Parse calls with RFC3339 in the gateway directory
rg "time.RFC3339" gateway/gateway-controller/ -B 2 -A 2Repository: wso2/api-platform
Length of output: 7870
Use RFC3339Nano for ExpiresAt parsing. ISO‑8601 timestamps often include fractional seconds; time.RFC3339 rejects those timestamps and silently drops the API key events, while time.RFC3339Nano handles both formats.
✅ Suggested fix
- parsedTime, err := time.Parse(time.RFC3339, *payload.ExpiresAt)
+ parsedTime, err := time.Parse(time.RFC3339Nano, *payload.ExpiresAt)Also applies to: 821-823
🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/controlplane/client.go` around lines 681 -
683, The ExpiresAt parsing currently uses time.Parse with time.RFC3339 which
fails on timestamps with fractional seconds; update both parsing sites (the
time.Parse calls that consume payload.ExpiresAt—e.g., the block referencing
payload.ExpiresAt and the second occurrence at the other parse) to use
time.RFC3339Nano instead of time.RFC3339 so fractional-second ISO‑8601
timestamps are accepted; keep the existing error handling around parsedTime
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 2757-2769: bindRequestBody currently checks Content-Type with
case-sensitive strings.Contains which can miss mixed-case headers; normalize the
header before matching by reading c.GetHeader("Content-Type"), trimming
whitespace, splitting off any parameters (e.g. after ';') and converting to
lower-case (e.g. use strings.ToLower on the media type) and then check for
"application/yaml" or "text/yaml"; keep the current fallback to ShouldBindJSON
for application/json or empty/unsupported types so behavior is unchanged aside
from robust matching.
🧹 Nitpick comments (4)
gateway/gateway-controller/pkg/storage/sqlite.go (1)
22-30: Run gateway build-local after gateway changes.
Please rebuild the gateway image withcd gateway && make build-localbefore final validation.
As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.common/apikey/store.go (1)
155-199: Consider indexing hashed external keys when IndexKey is provided.
The!isHashedKey(apiKey.APIKey)guard preventsexternalKeyIndexfrom being populated for hashed external keys, so the O(1) path is skipped under the default hashing config. IfIndexKeyis precomputed from the plain key, you can index regardless of hash state; mirror the same logic in removal paths to keep the index clean.♻️ Suggested adjustment (index when IndexKey exists, even if hashed)
- if effectiveSource(apiKey.Source) == "external" && !isHashedKey(apiKey.APIKey) { + if effectiveSource(apiKey.Source) == "external" { var indexKey string if apiKey.IndexKey != "" { // Use pre-computed index key from database (avoids re-hashing on restart) indexKey = apiKey.IndexKey - } else { + } else if !isHashedKey(apiKey.APIKey) { // Compute index key (backward compatibility for keys without IndexKey) indexKey = computeExternalKeyIndexKey(apiId, apiKey.APIKey) } - aks.externalKeyIndex[indexKey] = apiKey.ID + if indexKey != "" { + aks.externalKeyIndex[indexKey] = apiKey.ID + } }gateway/gateway-controller/pkg/utils/api_key.go (1)
1018-1157: Compute and persist IndexKey for external keys before hashing.
Right now external-key creation/update paths don’t setIndexKey, so the policy-engine store can’t populate the O(1) lookup index when keys are hashed. Consider computing a deterministic index hash from the plain key (before hashing) and assigningapiKey.IndexKeyin the request-based flow and both external event flows. Please confirm whether O(1) lookup is expected for hashed external keys in your deployment.♻️ Example (request flow; mirror in external create/update)
+ var indexKey *string + if isExternalKey { + sum := sha256.Sum256([]byte(plainAPIKeyValue)) + key := fmt.Sprintf("%s:%s", config.ID, hex.EncodeToString(sum[:])) + indexKey = &key + } ... apiKey := &models.APIKey{ ... Source: source, // "local" or "external" + IndexKey: indexKey, }Also applies to: 1971-2132, 2285-2390
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2367-2375: Rebuild gateway image after handler changes.
Please runcd gateway && make build-localto refresh the gateway image after these controller updates.As per coding guidelines: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway/gateway-controller/pkg/storage/apikey_store.go (1)
50-81:⚠️ Potential issue | 🔴 CriticalGuard external key indexing against nil
IndexKeyand ensure map initialization.The Store method and related functions dereference
*apiKey.IndexKeywithout checking for nil when Source is "external". If an external key is loaded from the database with a NULLindex_keycolumn, or if an event lacks this field, the code will panic. Additionally,externalKeyIndex[apiKey.APIId]is not initialized before use in the Store method's update path (lines 73–80).Other code paths (memory.go, sqlite.go) correctly guard against nil IndexKey, so this is an inconsistency.
🛡️ Suggested fix
func (s *APIKeyStore) Store(apiKey *models.APIKey) error { s.mu.Lock() defer s.mu.Unlock() + if apiKey.Source == "external" { + if apiKey.IndexKey == nil { + return fmt.Errorf("index_key is required for external API keys") + } + if s.externalKeyIndex[apiKey.APIId] == nil { + s.externalKeyIndex[apiKey.APIId] = make(map[string]*string) + } + } + // Check if an API key with the same APIId and name already exists existingKeys, apiIdExists := s.apiKeysByAPI[apiKey.APIId] var existingKeyID = ""Also applies to: addToAPIMapping (lines 197–200), removeFromAPIMapping (lines 212–215).
gateway/gateway-controller/pkg/utils/api_key.go (1)
1386-1408:⚠️ Potential issue | 🟡 Minor
DisplayNamenot preserved during regeneration.The
regeneratedKeystruct inregenerateAPIKeydoesn't includeDisplayName. SinceexistingKeyhas aDisplayNamefield (per the model), it should be preserved during regeneration to maintain consistency.🔧 Proposed fix
// Create the regenerated API key regeneratedKey := &models.APIKey{ ID: existingKey.ID, Name: existingKey.Name, + DisplayName: existingKey.DisplayName, APIKey: hashedAPIKeyValue, // Store hashed key MaskedAPIKey: maskedAPIKeyValue, // Store masked key for display
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/api/openapi.yaml`:
- Around line 2305-2314: The OpenAPI spec is inconsistent with the
implementation constant MIN_API_KEY_LENGTH = 20; update the APIKey-related
schemas so they all enforce a 20-character minimum: change the UpdateAPIKey
endpoint description that currently says "minimum 16 characters" to "minimum 20
characters" and change the APIKeyRegenerationRequest schema's minLength from 16
to 20 to match APIKeyCreationRequest and the implementation; ensure references
to APIKeyCreationRequest, UpdateAPIKey, and APIKeyRegenerationRequest
consistently state minLength: 20 and any description text mentions 20
characters.
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 672-679: Guard all dereferences of payload.DisplayName: before
using *payload.DisplayName when building APIKeyCreationRequest and anywhere
else, first check payload.DisplayName != nil; change the update handler
condition from if *payload.DisplayName == "" to if payload.DisplayName == nil ||
*payload.DisplayName == "" so nil is checked before dereference; in the update
event path replace unmarshalling into APIKeyCreatedEvent with a dedicated
APIKeyUpdatedEvent struct (create APIKeyUpdatedEvent if fields differ) and use
that type in the update handler to avoid field mismatch or data loss (refer to
APIKeyCreationRequest, payload.DisplayName, APIKeyCreatedEvent, and the update
handler function to locate the changes).
- Around line 629-653: The handler currently does direct map assertions (e.g.,
event["userId"].(string), event["apiId"].(string)) which can panic and also
reference nested fields incorrectly; instead, in handleAPIKeyCreatedEvent (and
mirror in handleAPIKeyRevokedEvent) first unmarshal the incoming event map/bytes
into the structured APIKeyCreatedEvent/APIKeyRevokedEvent types, check for
unmarshal errors, then construct the logger from the safely-typed fields (e.g.,
evt.UserId, evt.Payload.ApiId, evt.Payload.KeyName) and use those for
correlation_id/user_id/api_id/operation logging; remove direct map assertions
and add defensive nil/empty checks on required fields before logging or
proceeding.
In `@gateway/gateway-controller/pkg/controlplane/events.go`:
- Around line 46-58: The APIKeyCreatedEventPayload struct is missing the keyName
field needed to correlate later update/revoke events; add a KeyName field to
APIKeyCreatedEventPayload (e.g., KeyName *string `json:"keyName,omitempty"`) so
the gateway preserves the platform key identity, update any marshaling/usage
sites that construct or read APIKeyCreatedEventPayload to include/expect this
field and run tests to ensure compatibility.
In `@gateway/gateway-controller/pkg/storage/memory.go`:
- Around line 533-539: Guard against nil apiKey.IndexKey before dereferencing in
the external-key handling around externalKeyIndex: when apiKey.Source ==
"external" first check that apiKey.IndexKey != nil before using *apiKey.IndexKey
for deletion or insertion; if IndexKey is nil, skip updating cs.externalKeyIndex
(or handle as a separate case), but still ensure
cs.apiKeysByAPI[apiKey.APIId][apiKey.ID] = apiKey runs as before. Update the
code paths that reference apiKey.IndexKey (the two locations around
externalKeyIndex) to perform this nil check to avoid a panic.
- Around line 559-561: The insert into cs.externalKeyIndex risks nil pointer
dereference because it dereferences *apiKey.IndexKey; before using
*apiKey.IndexKey in the assignment inside the block that checks apiKey.Source ==
"external", first check that apiKey.IndexKey != nil and return or handle the
error if nil, and also ensure cs.externalKeyIndex[apiKey.APIId] map is
initialized (create map if nil) before assigning
cs.externalKeyIndex[apiKey.APIId][*apiKey.IndexKey] = &apiKey.ID; update the
code around the apiKey.Source == "external" branch to perform these guards.
In `@gateway/gateway-controller/pkg/utils/api_key_validation.go`:
- Around line 60-69: The ValidateDisplayName function uses len(trimmed) which
counts bytes, not UTF-8 runes; change the length check to use
utf8.RuneCountInString(trimmed) (import unicode/utf8) and compare that rune
count against the 100 limit so multi-byte characters (emoji/CJK/etc.) are
counted correctly while preserving the empty-string check on trimmed; update the
error message logic to use the rune count result when enforcing the maximum.
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 592-599: The call to s.xdsManager.StoreAPIKey is passing the stale
existingKey; update it to pass the new hashed key variable (updatedKey) so the
policy engine receives the updated API key. Locate the StoreAPIKey invocation in
api_key.go (the block that checks s.xdsManager != nil) and replace existingKey
with updatedKey in the argument list, preserving the other parameters (apiId,
apiName, apiVersion, params.CorrelationID) and existing error handling.
- Around line 1159-1167: The code in updateAPIKeyFromRequest dereferences
request.ApiKey without checking for nil; add a defensive nil check at the start
of the function (before using *request.ApiKey or calling ValidateAPIKeyValue)
and return a descriptive error if it is nil so callers know the API key value is
missing; update any related log messages in this function (using logger) and
keep ValidateAPIKeyValue usage unchanged once you have safely dereferenced the
pointer.
- Around line 1942-1975: UpdateExternalAPIKeyFromEvent builds APIKeyUpdateParams
but omits APIKeyName, so UpdateAPIKey cannot find the existing key; set
APIKeyName = request.DisplayName on the APIKeyUpdateParams before calling
s.UpdateAPIKey (use the same field handling as in
RevokeExternalAPIKeyFromEvent), ensuring request.DisplayName is passed through
to identify the key in UpdateAPIKey/GetAPIKeyByName.
In `@gateway/policies/policy-manifest.yaml`:
- Around line 7-9: The manifest entry for the policy named "api-key-auth"
currently points to a personal fork in the gomodule value; update the gomodule
string for the api-key-auth policy to use the official repository
"github.com/wso2/gateway-controllers/policies/api-key-auth@v0" (replacing
"github.com/Thushani-Jayasekera/..." and the version) and ensure the policy code
is published to that official repo/tag before merging to main so the manifest
references the released official package.
In `@platform-api/src/internal/service/gateway_events.go`:
- Around line 211-248: The comment for BroadcastAPIKeyCreatedEvent says "Single
attempt delivery" but the code sets const maxAttempts = 2 and retries; fix by
making behavior and docs match: either change maxAttempts to 1 in
BroadcastAPIKeyCreatedEvent (to enforce single attempt) or update the method
comment lines (and any similar comments for BroadcastAPIKeyRevokedEvent) to
state that the method will make up to two attempts; reference the maxAttempts
constant and the BroadcastAPIKeyCreatedEvent function when applying the change
so only the comment or the constant is modified to keep them consistent.
🧹 Nitpick comments (7)
gateway/gateway-controller/pkg/constants/constants.go (1)
141-143: Consider using PascalCase for consistency with existing constants.The new constants use
SCREAMING_SNAKE_CASE(MIN_API_KEY_LENGTH,MAX_API_KEY_LENGTH) while existing API key constants at lines 137-139 usePascalCase(APIKeyPrefix,APIKeyLen,APIKeySeparator). For consistency within this file, consider renaming toMinAPIKeyLengthandMaxAPIKeyLength.♻️ Suggested naming change
// API Key length constants - MIN_API_KEY_LENGTH = 20 - MAX_API_KEY_LENGTH = 128 + MinAPIKeyLength = 20 + MaxAPIKeyLength = 128common/apikey/store.go (2)
256-260: Side-effect:ValidateAPIKeymutates the key's status.Line 258 sets
targetAPIKey.Status = Expiredduring validation. While this lazy expiration marking may be intentional, it's a surprising side-effect for a validation method. Consider either:
- Documenting this behavior in the method's doc comment, or
- Moving expiration status updates to a separate method
The exclusive lock at line 207 is correct given this mutation.
187-199: Duplicate code blocks can be consolidated.Lines 188-192 and 194-198 have identical logic. Since both branches (update and insert) perform the same external index insertion, this can be simplified.
♻️ Suggested simplification
// For external keys with plain text (not hashed), add to fast lookup index // This enables O(1) lookup during validation instead of O(n) iteration if apiKey.Source == "external" { - if existingKeyID != "" { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID - } else { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID + if aks.externalKeyIndex[apiId] == nil { + aks.externalKeyIndex[apiId] = make(map[string]*string) } + aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID }gateway/gateway-controller/pkg/controlplane/client.go (1)
111-136: Rebuild gateway images after these gateway-controller changes.Please run
cd gateway && make build-localto ensure the updated gateway artifacts are rebuilt. As per coding guidelines,gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/storage/sqlite.go (1)
1557-1574: Consider wrapping duplicate check and update in a transaction for consistency.The duplicate check in
UpdateAPIKeyruns outside a transaction, unlikeSaveAPIKeywhich uses a transaction. While SQLite's single-writer model (SetMaxOpenConns(1)at line 55) serializes writes and mitigates TOCTOU concerns, wrapping both operations in a transaction would be more consistent withSaveAPIKeyand safer if the database backend changes in the future.sdk/gateway/policy/v1alpha/api_key.go (2)
185-199: Simplify duplicate code in external key index update.Both branches of the
if existingKeyID != ""/elseblock perform identical operations for external keys. This can be consolidated.♻️ Proposed simplification
// For external keys with plain text (not hashed), add to fast lookup index // This enables O(1) lookup during validation instead of O(n) iteration if apiKey.Source == "external" { - if existingKeyID != "" { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID - } else { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID + if aks.externalKeyIndex[apiId] == nil { + aks.externalKeyIndex[apiId] = make(map[string]*string) } + aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID }
341-359: Consider logging instead of returning error during bulk removal.If
RemoveAPIKeysByAPIencounters an empty API key value (lines 349-351, 353-355), it returns an error, potentially leaving the store in a partially cleaned state. Since this condition indicates data corruption rather than a recoverable error, consider logging a warning and continuing with cleanup of remaining keys.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/gateway/policy/v1alpha/api_key.go (1)
125-195:⚠️ Potential issue | 🟠 MajorRequire
IndexKeyfor external keys to keep the index consistent.If an external key arrives without
IndexKey, the store currently inserts an empty key (or deletes using a derived value from the hashed API key), which makes external lookups fail and leaves stale entries. Reject missingIndexKeyexplicitly.🛠️ Suggested fix
if existingKeyID != "" { // Remove old external key index entry if it exists oldKey := aks.apiKeysByAPI[apiId][existingKeyID] if oldKey != nil && oldKey.Source == "external" { - var oldIndexKey string - if oldKey.IndexKey != "" { - oldIndexKey = oldKey.IndexKey - } else { - oldIndexKey = computeExternalKeyIndexKey(oldKey.APIKey) - if oldIndexKey == "" { - return fmt.Errorf("failed to compute index key") - } - } - delete(aks.externalKeyIndex[apiId], oldIndexKey) + if oldKey.IndexKey == "" { + return fmt.Errorf("external API key must have IndexKey set") + } + delete(aks.externalKeyIndex[apiId], oldKey.IndexKey) } @@ if apiKey.Source == "external" { + if apiKey.IndexKey == "" { + return fmt.Errorf("external API key must have IndexKey set") + } if existingKeyID != "" { if aks.externalKeyIndex[apiId] == nil { aks.externalKeyIndex[apiId] = make(map[string]*string) } aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID
🤖 Fix all issues with AI agents
In `@common/apikey/api_key_hash_test.go`:
- Around line 50-62: The test creates an APIKey with hashed APIKey but does not
set Source or IndexKey, so StoreAPIKey won't populate the external index and
ValidateAPIKey will return ErrNotFound for the plain-key path; update the test
APIKey creation (the APIKey struct instance used in the test) to set Source =
"external" and set IndexKey to the corresponding plain API key value used in the
test so StoreAPIKey will index it and ValidateAPIKey can find it via the plain
key path.
In `@common/apikey/store.go`:
- Around line 125-195: The StoreAPIKey method inserts empty externalKeyIndex
entries when an external APIKey lacks IndexKey, breaking lookup/revocation;
update APIkeyStore.StoreAPIKey to require a non-empty apiKey.IndexKey for
apiKey.Source == "external" (return a clear error like ErrInvalidInput or
fmt.Errorf) before any writes, and avoid computing a replacement IndexKey from
hashed APIKey during insert/update; also ensure when replacing an existing
external key you remove the old index using either oldKey.IndexKey or
computeExternalKeyIndexKey(oldKey.APIKey) only for cleanup but never insert an
empty index entry for the new key into externalKeyIndex[apiId].
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 847-892: The code currently rejects update events when
payload.DisplayName is empty and always sets ExternalRefId/DisplayName on
apiKeyCreationRequest which can overwrite existing values; remove the hard
failure for payload.DisplayName (delete the DisplayName nil/empty check) and
change construction of apiKeyCreationRequest so you only set
apiKeyCreationRequest.DisplayName and apiKeyCreationRequest.ExternalRefId when
payload.DisplayName != "" and payload.ExternalRefId != "" respectively (leave
them nil otherwise), keeping ApiKey and other required fields as before;
reference payload.DisplayName, payload.ExternalRefId, api.APIKeyCreationRequest
and apiKeyCreationRequest to locate and apply the conditional assignments.
- Around line 701-726: The logger.Error calls in the API key expiration handling
(e.g., the call that currently uses "API key expiration time must be in the
future, got: %s (current time: %s)" and the "Unsupported expiration unit: %s"
call) are using fmt-style placeholders which slog does not interpret; update
these to use structured attributes instead (e.g., logger.Error("API key
expiration time must be in the future", slog.String("expires_at",
parsedExpiresAt.Format(time.RFC3339)), slog.String("now",
now.Format(time.RFC3339))) and logger.Error("Unsupported expiration unit",
slog.Any("expires_in.unit", payload.ExpiresIn.Unit))). Apply the same change to
the duplicated occurrences later in the file so all logger.Error usages in the
API key creation logic use structured slog attributes rather than format
strings.
In `@gateway/gateway-controller/pkg/storage/memory.go`:
- Around line 530-545: The update path for apiKeysByAPI/externalKeyIndex is
deleting the external index using the new apiKey.IndexKey instead of removing
the old index entry, which leaks stale entries after rotation; to fix, when
existingKeyID != "" first fetch the oldEntry :=
cs.apiKeysByAPI[apiKey.APIId][existingKeyID] and if oldEntry != nil &&
oldEntry.Source == "external" && oldEntry.IndexKey != nil delete
cs.externalKeyIndex[apiKey.APIId][*oldEntry.IndexKey] (with nil-map checks),
then proceed to set cs.apiKeysByAPI[apiKey.APIId][apiKey.ID]=apiKey and insert
the new externalKeyIndex entry using apiKey.IndexKey as currently done.
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 542-575: The updatedKey returned by updateAPIKeyFromRequest
currently retains PlainAPIKey and is passed to s.db.UpdateAPIKey and
s.store.StoreAPIKey; before persisting or storing, clear the secret field (set
updatedKey.PlainAPIKey = "" or equivalent) so plaintext isn't kept in DB or
ConfigStore/xDS, and propagate that cleared updatedKey to s.db.UpdateAPIKey and
s.store.StoreAPIKey while keeping rollback logic using existingKey where needed.
- Around line 1173-1178: The code dereferences request.DisplayName without a nil
guard; update the logic around ValidateDisplayName and displayName so you first
check whether request.DisplayName is nil before dereferencing it—only call
ValidateDisplayName(*request.DisplayName) and set displayName :=
strings.TrimSpace(*request.DisplayName) when request.DisplayName != nil,
otherwise either skip validation/assignment or return a clear error if a
DisplayName is required; refer to the ValidateDisplayName function and the
request.DisplayName / displayName variables to locate and adjust the checks.
🧹 Nitpick comments (3)
gateway/policies/policy-manifest.yaml (1)
25-26: Remember to rebuild gateway images after this policy manifest change.Per gateway build instructions, run
cd gateway && make build-localso the updated policy module is baked into the image.gateway/gateway-controller/pkg/utils/api_key_validation.go (1)
19-28: Rebuild gateway images after these changes.Please run
cd gateway && make build-localso the updated validation utilities are packaged in the gateway images. As per coding guidelinesgateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
2367-2440: LGTM — create flow and structured logging look solid.
Reminder to rebuild gateway images after these gateway-controller changes:cd gateway && make build-local.As per coding guidelines, When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local.
Thushani-Jayasekera
force-pushed
the
api-key
branch
from
098bedf to
96ec5ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-controller/api/openapi.yaml (1)
2389-2393:⚠️ Potential issue | 🟡 Minor
api_keydescription should allow external formats.
The schema still implies anapip_prefix, which isn’t true for external keys.✏️ Suggested wording
- description: Generated API key with apip_ prefix + description: API key value (platform-generated keys use apip_ prefix; external keys may use any format)
🤖 Fix all issues with AI agents
In `@docs/gateway/policies/apikey-authentication.md`:
- Around line 445-449: Update the injected response schema entry for
api_key.api_key to remove the claim that the key "starts with `apip_`" and
instead use neutral wording (e.g., "The actual API key value" or "The actual API
key value (format may vary)") so the documentation no longer implies a fixed
`apip_` prefix; locate and modify the line defining `api_key.api_key` in the
schema table.
In `@platform-api/src/internal/handler/api_key.go`:
- Around line 74-78: The handler currently enforces api_key as required which
contradicts the OpenAPI spec; remove the validation so the service can generate
a key when none is provided: delete the explicit check that returns BadRequest
for req.ApiKey == "" in the API key handler (the if block returning 400) and
also remove the `binding:"required"` tag on the ApiKey field in the request
struct (APIKeyCreationRequest / whatever struct defines ApiKey) so the field is
optional and the existing key-generation code path executes when ApiKey is
absent.
In `@sdk/gateway/policy/v1alpha/api_key.go`:
- Around line 152-165: The removal logic for legacy external keys incorrectly
computes a fallback index by calling computeExternalKeyIndexKey(oldKey.APIKey)
when oldKey.IndexKey is empty, but oldKey.APIKey for external keys contains a
hashed value so this produces a wrong index and fails to delete; change the
branch in the block that references oldKey, oldKey.IndexKey, oldKey.APIKey,
computeExternalKeyIndexKey, externalKeyIndex and apiKeysByAPI (around the
removal code) to NOT compute a fallback from APIKey for external-source
keys—instead log a warning noting a legacy hashed APIKey was encountered
(include existingKeyID and apiId) and skip the delete (or mark the key for
migration/cleanup), so you avoid deleting the wrong index entry; ensure the log
is actionable for later migration.
🧹 Nitpick comments (6)
sdk/gateway/policy/v1alpha/api_key.go (2)
185-199: Simplify duplicate branches.Both the
if existingKeyID != ""andelsebranches execute identical code. This can be consolidated.♻️ Suggested simplification
if apiKey.Source == "external" { - if existingKeyID != "" { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID - } else { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID + if aks.externalKeyIndex[apiId] == nil { + aks.externalKeyIndex[apiId] = make(map[string]*string) } + aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID }
340-354: Same fallback issue as inStoreAPIKey.The fallback at lines 346-351 has the same problem: computing index key from
apiKey.APIKey(hashed) instead of plain text. This is the same edge case affecting legacy external keys.Additionally, line 352 uses
apiKey.APIIdinstead of theapiIdparameter. While functionally equivalent (since keys are retrieved fromaks.apiKeysByAPI[apiId]), using the parameter would be more consistent.♻️ Minor consistency fix
- delete(aks.externalKeyIndex[apiKey.APIId], indexKey) + delete(aks.externalKeyIndex[apiId], indexKey)platform-api/src/internal/service/gateway_events.go (1)
410-438: Consider extracting a shared retry helper for API key broadcasts.
Create/Update/Revoke wrappers are nearly identical; a small helper would reduce duplication and keep retry behavior aligned.gateway/gateway-controller/pkg/controlplane/client.go (1)
92-136: Reminder: rebuild gateway images after these gateway changes.
Please runcd gateway && make build-localto refresh gateway images.As per coding guidelines,
gateway/**/*.{go,yaml,yml,Dockerfile}changes require rebuilding Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/utils/api_key.go (2)
168-321: Creation flow handles external vs local keys cleanly.Collision handling, rollback on ConfigStore failure, and clearing plaintext key material are all solid. Also, remember to rebuild the gateway images after these changes.
As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}requires rebuilding Docker images usingcd gateway && make build-local.
324-326: Track or resolve the TODO before release.The TODO about policy‑engine index cleanup should be implemented or tracked in an issue to avoid drifting behavior. I can help draft the change or open an issue if you'd like.
Thushani-Jayasekera
force-pushed
the
api-key
branch
from
6581784 to
4b1920f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 686-695: The code dereferences keyCreatedEvent.Payload.DisplayName
directly in the if condition which can panic if DisplayName is nil; update the
logic in the block handling keyCreatedEvent to first check that
keyCreatedEvent.Payload != nil and keyCreatedEvent.Payload.DisplayName != nil
before calling strings.TrimSpace or passing the value to
utils.ValidateDisplayName, and only run strings.TrimSpace and
utils.ValidateDisplayName when the pointer is non-nil (otherwise skip
validation); keep the existing baseLogger.Error call in the validation failure
path (use *keyCreatedEvent.Payload.DisplayName when safe).
- Around line 930-939: The update handler currently parses payload.ExpiresAt
with time.Parse using time.RFC3339 which rejects timestamps with fractional
seconds; in the block handling payload.ExpiresAt (where parsedExpiresAt is
assigned and error-checked) replace time.RFC3339 with time.RFC3339Nano so
fractional-second RFC3339 timestamps are accepted and the logger/error path
remains the same.
In `@sdk/gateway/policy/v1alpha/api_key.go`:
- Around line 340-354: In the external-key cleanup loop over apiKeys, avoid
computing computeExternalKeyIndexKey(apiKey.APIKey) when apiKey.APIKey already
contains a hashed value (which causes a hash-of-hash and fails to delete the
right entry); instead, mirror the StoreAPIKey fix: if apiKey.IndexKey is empty
and apiKey.Source == "external", emit a warning (use the same logger used in
StoreAPIKey) and skip deletion for that entry rather than calling
computeExternalKeyIndexKey; update the block around computeExternalKeyIndexKey,
delete(aks.externalKeyIndex[apiKey.APIId], indexKey) and the surrounding logic
to perform this check and skip.
- Around line 305-315: The external-index lookup uses
computeExternalKeyIndexKey(providedAPIKey) which trims internally, but
compareAPIKeys is still given the untrimmed providedAPIKey causing mismatches;
update the block that sets matchedKey (the section using
computeExternalKeyIndexKey, aks.externalKeyIndex, and compareAPIKeys) to trim
the provided API key first (use the same trimmedAPIKey pattern as
ValidateAPIKey) and pass that trimmed value into compareAPIKeys so the hash
comparison and revocation logic are consistent.
- Around line 557-567: In removeFromAPIMapping there’s the same
legacy-external-key bug: when apiKey.IndexKey is empty the code computes
computeExternalKeyIndexKey(apiKey.APIKey) (causing a hash-of-hash) and deletes
the wrong map entry; change the fallback to
computeExternalKeyIndexKey(apiKey.ExternalKey) instead, keep the guard that
returns if the computed indexKey is empty, and then call
delete(aks.externalKeyIndex[apiKey.APIId], indexKey) as before so the correct
external-key index is removed.
🧹 Nitpick comments (5)
gateway/gateway-controller/pkg/api/handlers/handlers_test.go (1)
1089-1130: Consider renaming test functions to match the renamedCreateAPIKeymethod.The test function names still use the "Generate" prefix (
TestGenerateAPIKeyNoAuth,TestGenerateAPIKeyInvalidAuthContext,TestGenerateAPIKeyInvalidBody) while the comments and method calls have been updated to referenceCreateAPIKey. This inconsistency could cause confusion when searching for tests or understanding their purpose.🔧 Suggested rename for consistency
-// TestGenerateAPIKeyNoAuth tests CreateAPIKey without authentication -func TestGenerateAPIKeyNoAuth(t *testing.T) { +// TestCreateAPIKeyNoAuth tests CreateAPIKey without authentication +func TestCreateAPIKeyNoAuth(t *testing.T) {-// TestGenerateAPIKeyInvalidAuthContext tests CreateAPIKey with invalid auth context -func TestGenerateAPIKeyInvalidAuthContext(t *testing.T) { +// TestCreateAPIKeyInvalidAuthContext tests CreateAPIKey with invalid auth context +func TestCreateAPIKeyInvalidAuthContext(t *testing.T) {-// TestGenerateAPIKeyInvalidBody tests CreateAPIKey with invalid body -func TestGenerateAPIKeyInvalidBody(t *testing.T) { +// TestCreateAPIKeyInvalidBody tests CreateAPIKey with invalid body +func TestCreateAPIKeyInvalidBody(t *testing.T) {gateway/gateway-controller/pkg/storage/sqlite_test.go (1)
834-848: Consider adding new schema v6 fields to the test helper.The
createTestAPIKey()helper doesn't include the new fields added in schema v6 (Source,ExternalRefId,IndexKey,DisplayName). While the tests likely pass due to nullable/default values, explicitly setting these fields would improve test coverage for the new external API key functionality.♻️ Suggested enhancement
func createTestAPIKey() *models.APIKey { apiKeyCounter++ return &models.APIKey{ ID: fmt.Sprintf("test-apikey-%d", apiKeyCounter), Name: fmt.Sprintf("Test API Key %d", apiKeyCounter), + DisplayName: fmt.Sprintf("Test API Key %d", apiKeyCounter), APIKey: fmt.Sprintf("apk_%d_%d", apiKeyCounter, time.Now().UnixNano()), MaskedAPIKey: "apk_***", APIId: "test-api-id", Operations: "*", Status: models.APIKeyStatusActive, + Source: "local", CreatedAt: time.Now(), CreatedBy: "test-user", UpdatedAt: time.Now(), } }Consider also adding dedicated tests for external API keys with
Source: "external"and populatedExternalRefId/IndexKeyfields to ensure schema v6 functionality is covered.gateway/policy-engine/Dockerfile (1)
22-22: Align Go version in gateway/policy-engine/Dockerfile with gateway/gateway-builder/Dockerfile.The policy-engine Dockerfile uses
golang:1.25.6-alpine3.23while gateway-builder usesgolang:1.25.5-alpine3.23. Consider using the same Go version across gateway Dockerfiles to maintain consistency and avoid subtle build differences.gateway/gateway-controller/pkg/controlplane/client.go (1)
734-757: Consider extracting expiration calculation to a shared helper.The expiration calculation logic (lines 734-757 in created handler and 949-972 in updated handler) is duplicated. Consider extracting to a shared helper function to reduce code duplication and ensure consistent behavior.
♻️ Example helper signature
// calculateExpiresAt computes the expiration time from ExpiresIn specification func calculateExpiresAt(expiresIn *ExpiresInSpec, now time.Time) (*time.Time, error) { // ... shared logic }sdk/gateway/policy/v1alpha/api_key.go (1)
187-199: Simplify duplicate external key index update logic.Both branches (
existingKeyID != ""andelse) contain identical code. This can be consolidated.♻️ Suggested simplification
if apiKey.Source == "external" { - if existingKeyID != "" { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID - } else { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID + if aks.externalKeyIndex[apiId] == nil { + aks.externalKeyIndex[apiId] = make(map[string]*string) } + aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID }
| if payload.ExpiresAt != nil { | ||
| // payload.ExpiresAt is likely a *string (RFC3339). Attempt to parse it to time.Time | ||
| parsedExpiresAt, err := time.Parse(time.RFC3339, *payload.ExpiresAt) | ||
| if err != nil { | ||
| logger.Error("Invalid expires_at format for API key, expected RFC3339", | ||
| slog.Any("expires_at", *payload.ExpiresAt), | ||
| slog.Any("error", err), | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
Use time.RFC3339Nano for parsing ExpiresAt in update handler.
Same issue as the created handler - timestamps with fractional seconds will be rejected.
🔧 Suggested fix
- parsedExpiresAt, err := time.Parse(time.RFC3339, *payload.ExpiresAt)
+ parsedExpiresAt, err := time.Parse(time.RFC3339Nano, *payload.ExpiresAt)🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/controlplane/client.go` around lines 930 -
939, The update handler currently parses payload.ExpiresAt with time.Parse using
time.RFC3339 which rejects timestamps with fractional seconds; in the block
handling payload.ExpiresAt (where parsedExpiresAt is assigned and error-checked)
replace time.RFC3339 with time.RFC3339Nano so fractional-second RFC3339
timestamps are accepted and the logger/error path remains the same.
There was a problem hiding this comment.
@Thushani-Jayasekera, acknowledged. I've noted your decision.
🧠 Learnings used
Learnt from: Thushani-Jayasekera
Repo: wso2/api-platform PR: 845
File: gateway/gateway-controller/pkg/utils/api_key.go:1229-1233
Timestamp: 2026-02-02T11:51:36.196Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the UpdateAPIKey flow intentionally clears an API key's expiration when both request.ExpiresAt and request.ExpiresIn are nil. Omitting expiry fields in an update request is treated as an explicit intent to remove expiration, not as a signal to preserve the existing expiry.
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/pkg/utils/api_key.go:257-380
Timestamp: 2025-12-23T10:41:47.584Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the RevokeAPIKey method intentionally proceeds with database deletion and xDS policy engine revocation even when the API key is not found in the memory store (apiKey is nil). This is by design to handle partial failure recovery: if a previous revocation attempt removed the key from memory but failed to clean up the database or policy engine, subsequent retries will still perform the cleanup operations to ensure eventual consistency.
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/memory.go:484-503
Timestamp: 2026-01-09T05:58:05.318Z
Learning: In gateway/gateway-controller/pkg/storage/memory.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: sdk/gateway/policy/v1alpha/api_key.go:119-123
Timestamp: 2026-01-09T06:08:39.423Z
Learning: In sdk/gateway/policy/v1alpha/api_key.go, during API key rotation via the StoreAPIKey method, the API key ID is never modified. When an existing API key is found by name (existingKeyID != ""), the incoming apiKey.ID is guaranteed to be the same as existingKeyID, so no ID mismatch can occur during updates.
Learnt from: Thushani-Jayasekera
Repo: wso2/api-platform PR: 845
File: platform-api/src/internal/handler/api_key.go:74-78
Timestamp: 2026-02-02T18:19:59.926Z
Learning: In platform-api (Cloud APIM control plane), API keys are always generated externally and injected into hybrid gateways. The platform-api CreateAPIKey endpoint (POST /api/v1/apis/{apiId}/api-keys) requires the api_key field to be provided, unlike the gateway-controller API which supports both local generation and external injection. The required validation for api_key in platform-api/src/internal/handler/api_key.go is intentional.
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/apikey_store.go:219-222
Timestamp: 2026-01-09T05:57:36.635Z
Learning: In gateway/gateway-controller/pkg/storage/apikey_store.go, during API key rotation, the API key ID and name remain constant—only the API key value changes. Therefore, when addToAPIMapping finds an existing key by name, the existingKeyID will always equal the incoming apiKey.ID.
Learnt from: Thushani-Jayasekera
Repo: wso2/api-platform PR: 845
File: platform-api/src/internal/service/apikey.go:71-82
Timestamp: 2026-01-29T06:05:09.158Z
Learning: In platform-api/src/internal/service/apikey.go, the CreateAPIKey method intentionally ignores req.Operations and always defaults to "[\"*\"]" (all operations) for API key creation. Operation-level scoping is not supported in the current release, and all API keys are granted full access regardless of what the request specifies.
Learnt from: VirajSalaka
Repo: wso2/api-platform PR: 686
File: platform-api/src/internal/service/api.go:920-922
Timestamp: 2026-01-15T11:14:32.797Z
Learning: In the platform-api service (file: platform-api/src/internal/service/api.go), API handles (the ID field) are immutable identifiers used as references and should not be updated after API creation. They serve as stable, user-facing identifiers for the API lifecycle.
Learnt from: dushaniw
Repo: wso2/api-platform PR: 671
File: platform-api/src/internal/service/deployment.go:0-0
Timestamp: 2026-01-25T17:49:39.364Z
Learning: In the platform-api deployment service (file: platform-api/src/internal/service/deployment.go), the DeployAPI method does not broadcast undeployment events when deploying a new API version to a gateway. The gateway is designed to intelligently handle the replacement of existing deployments when it receives a new deployment event for the same API, eliminating the need for explicit undeployment notifications from the platform-api service during the deployment flow.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/gateway/policy/v1alpha/api_key.go (1)
250-259:⚠️ Potential issue | 🟡 MinorDead code:
Status == Expiredcheck is unreachable.After passing the check at line 251,
targetAPIKey.Statusis guaranteed to beActive. ThetargetAPIKey.Status == Expiredcondition at line 256 can never evaluate totrue.🐛 Proposed fix
// Check if the API key is active if targetAPIKey.Status != Active { return false, nil } - // Check if the API key has expired - if targetAPIKey.Status == Expired || (targetAPIKey.ExpiresAt != nil && time.Now().After(*targetAPIKey.ExpiresAt)) { + // Check if the API key has expired based on expiration timestamp + if targetAPIKey.ExpiresAt != nil && time.Now().After(*targetAPIKey.ExpiresAt) { targetAPIKey.Status = Expired return false, nil }
🧹 Nitpick comments (2)
sdk/gateway/policy/v1alpha/api_key.go (1)
185-199: Simplify duplicate branches.Both
if existingKeyID != ""andelsebranches contain identical code. The external key index insertion logic doesn't need to differentiate between update and insert cases.♻️ Proposed simplification
// For external keys with plain text (not hashed), add to fast lookup index // This enables O(1) lookup during validation instead of O(n) iteration if apiKey.Source == "external" { - if existingKeyID != "" { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID - } else { - if aks.externalKeyIndex[apiId] == nil { - aks.externalKeyIndex[apiId] = make(map[string]*string) - } - aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID + if aks.externalKeyIndex[apiId] == nil { + aks.externalKeyIndex[apiId] = make(map[string]*string) } + aks.externalKeyIndex[apiId][apiKey.IndexKey] = &apiKey.ID }gateway/gateway-controller/pkg/controlplane/client.go (1)
705-757: Consider simplifying expiration handling.The intermediate pointer variable
duration(line 706) adds unnecessary indirection. It's assigned and immediately dereferenced without any other use.Also, this expiration-parsing block is duplicated in
handleAPIKeyUpdatedEvent(lines 921-972). Consider extracting a shared helper function to reduce duplication.♻️ Suggested simplification for duration handling
- var expiresAt *time.Time - var duration *int - now := time.Now() + var expiresAt *time.Time + now := time.Now() ... } else if payload.ExpiresIn != nil { - duration = &payload.ExpiresIn.Duration - timeDuration := time.Duration(*duration) + timeDuration := time.Duration(payload.ExpiresIn.Duration) switch payload.ExpiresIn.Unit {
|
/azp run Operator Integration Test / integration-test (pull_request) |
|
I am +1 to merge after OpenAPI related issues are fixed. We can add other changes iteratively |
New changes
Converted all snake_case serialization tags to camelCase across the API key feature. Database (db:) tags Fields renamed:
Files: gateway/gateway-controller/pkg/models/api_key.go, platform-api/src/internal/dto/apikey.go,
Changed MIN_API_KEY_LENGTH from 20 to 36 (UUID length) as the default. Updated OpenAPI spec minLength Files: gateway/gateway-controller/pkg/constants/constants.go, gateway/gateway-controller/api/openapi.yaml,
Added MinKeyLength and MaxKeyLength fields to APIKeyConfig. Defaults to 36 and 128. Validation ensures min Files: gateway/gateway-controller/pkg/config/config.go,
Moved apiKeyNameMinLength (3), apiKeyNameMaxLength (63), and displayNameMaxLength (100) from local Files: gateway/gateway-controller/pkg/constants/constants.go,
GenerateAPIKeyName now pads with random hex characters when the sanitized name is shorter than the Files: gateway/gateway-controller/pkg/utils/api_key_validation.go
|
|
@renuka-fernando , I have both files /Users/thushanij/Repositories/api-platform/common/apikey/store.go (new) and /Users/thushanij/Repositories/api-platform/sdk/gateway/policy/v1alpha/api_key.go maintained until we do a release in gateway-controller repo with the update to use common pkg. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-controller/api/openapi.yaml (1)
2294-2342:⚠️ Potential issue | 🟠 MajorMake
displayNamerequired in APIKeyCreationRequest.
Server validation requires it, but the schema currently allows omission.Suggested fix
APIKeyCreationRequest: type: object + required: + - displayName properties: displayName:
🤖 Fix all issues with AI agents
In `@common/apikey/store.go`:
- Around line 290-305: The revocation path uses providedAPIKey without trimming,
causing mismatches vs ValidateAPIKey which trims; before calling parseAPIKey,
computeExternalKeyIndexKey, or compareAPIKeys in the RevokeAPIKey logic,
normalize providedAPIKey by trimming whitespace (use the same helper or
strings.TrimSpace as used in ValidateAPIKey) so parseAPIKey(providedAPIKey),
computeExternalKeyIndexKey(providedAPIKey) and compareAPIKeys(...) operate on
the trimmed value and revocation correctly finds local (parseAPIKey ->
parsedAPIkey.ID) or external (externalKeyIndex lookup -> compareAPIKeys) keys.
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 663-679: Replace the fragile string match on err.Error() that
detects API key collisions with a sentinel error check using errors.Is(err,
storage.ErrConflict); locate the block in the function where it checks for "API
key value already exists" (near the call to s.regenerateAPIKey and the variables
existingKey, params.Request, user.UserID) and change the condition to use
errors.Is(err, storage.ErrConflict) so the regeneration retry path sets
result.IsRetry = true and logs using params.Handle and params.CorrelationID as
before.
In `@sdk/gateway/policy/v1alpha/api_key.go`:
- Around line 296-301: The comment above the parseAPIKey block is inaccurate: it
claims an empty Source is treated as "local" while the code explicitly checks
apiKey.Source == "local". Update the comment (near parseAPIKey(providedAPIKey),
apiKeysByAPI, and the matching logic that sets matchedKey) to remove or correct
the "empty Source treated as 'local'" statement so it accurately reflects that
only explicit "local" Source values are considered local; do not change the
matching logic itself unless you intend to treat empty as local, in which case
modify the apiKey.Source check to accept empty string or "local" consistently.
🧹 Nitpick comments (5)
gateway/gateway-controller/pkg/config/config.go (1)
566-571: Reminder: rebuild gateway Docker image after gateway config changes.
Runcd gateway && make build-localonce these updates are merged.As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components, rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/utils/api_key_validation.go (1)
41-65: Normalize whitespace insideValidateAPIKeyValueto avoid caller footguns.
Keeps validation consistent even if a caller forgets to trim.Suggested update
func (s *APIKeyService) ValidateAPIKeyValue(plainKey string) error { minLength := constants.DefaultMinAPIKeyLength maxLength := constants.DefaultMaxAPIKeyLength @@ - if plainKey == "" { + trimmed := strings.TrimSpace(plainKey) + if trimmed == "" { return fmt.Errorf("API key cannot be empty") } - if len(plainKey) < minLength { + if len(trimmed) < minLength { return fmt.Errorf("API key is too short (minimum %d characters required)", minLength) } - if len(plainKey) > maxLength { + if len(trimmed) > maxLength { return fmt.Errorf("API key is too long (maximum %d characters allowed)", maxLength) } return nil }gateway/gateway-controller/pkg/utils/api_key.go (3)
168-321: CreateAPIKey flow looks solid; remember to rebuild gateway images.
Please rebuild images before validating changes:cd gateway && make build-localAs per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.
324-326: Track or resolve the TODO in the revocation path.
Leaving this in a hot path can drift; consider filing an issue or implementing the cleanup. Want me to draft a fix or open a ticket?
1170-1182: Comment contradicts enforced display-name requirement.
The code requiresdisplay_name, but the comment says it’s optional. Update the comment (or relax the logic) to avoid confusion.✍️ Comment fix
- // Handle displayName - optional during update - // If not provided or empty, keep the existing displayName + // Handle displayName - required during update
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/gateway/policies/apikey-authentication.md (1)
638-714:⚠️ Potential issue | 🟡 MinorReplace literal API key in Regenerate section example.
Line 702 contains an example API key value that triggers secret scanners. Use a placeholder for consistency with other sections.
🛠️ Proposed fix
- "apiKey": "apip_18dfd4da48f276043b32d37_bhuced7y3gfd8r4w8bcf4wg", + "apiKey": "apip_<REGENERATED_KEY_VALUE>",
🤖 Fix all issues with AI agents
In `@common/apikey/store.go`:
- Around line 297-302: The comment above the local-key parsing block is
incorrect: it says "empty Source treated as 'local'" but the code only checks
for apiKey.Source == "local"; update the comment to accurately describe behavior
(e.g., "Try to parse as local key (format: key_id); only keys with Source ==
\"local\" are accepted") so it matches the logic in parseAPIKey,
aks.apiKeysByAPI[apiId][parsedAPIkey.ID], the APIKey.Source check, and
compareAPIKeys usage that sets matchedKey.
In `@docs/gateway/policies/apikey-authentication.md`:
- Around line 455-568: In the "Update API Key" section (endpoint `PUT
/apis/{id}/api-keys/{apiKeyName}`) replace all literal API key strings used in
JSON examples and response payloads with a clear placeholder (e.g., "<API_KEY>"
or "{apiKey}")—specifically update the `apiKey` field in the request bodies
shown in the Example Request blocks and the `apiKey` value in the Successful
Response example so no real-looking keys appear; ensure the placeholder format
matches the Generate/Inject sections for consistency.
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 1977-2005: The CreateExternalAPIKeyFromEvent function (and the
other external-event helper around lines 2041-2068) dereference request without
a nil check which can panic on malformed events; add a defensive nil-guard at
the start of each helper that checks if request == nil, log a clear error via
the provided logger (e.g., logger.Error("nil APIKeyCreationRequest",
slog.String("api_id", handle), slog.String("correlation_id", correlationID)))
and return a descriptive error (or nil result) before building
APIKeyCreationParams or calling CreateAPIKey; ensure you reference
APIKeyCreationRequest, APIKeyCreationParams, CreateExternalAPIKeyFromEvent and
CreateAPIKey so the fix is applied to both helpers.
🧹 Nitpick comments (3)
docs/gateway/policies/apikey-authentication.md (1)
492-492: Remove implementation details from user-facing documentation.The parenthetical explanation references Go implementation details (
request.ExpiresAt,request.ExpiresIn,nil) which are too technical for user-facing API documentation. Users don't need to know about internal request structures.📝 Suggested user-friendly wording
-| `expiresIn` | object | No | Relative expiration time from now. Omitting both `expiresAt` and `expiresIn` removes the API key's expiration (UpdateAPIKey clears expiry when `request.ExpiresAt` and `request.ExpiresIn` are both nil). | +| `expiresIn` | object | No | Relative expiration time from now. Omitting both `expiresAt` and `expiresIn` removes the API key's expiration. |The behavior is already clear from the first part of the sentence; the technical details add no value for API consumers.
gateway/gateway-controller/pkg/utils/api_key.go (2)
46-107: New API‑key param/result structs look good.Clearer request/response plumbing for create/update flows. Reminder: rebuild gateway Docker images after these changes (
cd gateway && make build-local).As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: rebuild Docker images usingcd gateway && make build-local.
324-326: Resolve or track the TODO before release.The revocation TODO should be converted into a tracked issue or addressed now to avoid regressions.
If you want, I can draft the index‑cleanup logic or open a tracking issue.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/utils/api_key.go`:
- Around line 1251-1278: The updatedKey construction in updateAPIKeyFromRequest
(building models.APIKey with values from existingKey) currently omits
ExternalRefId and thus clears it; preserve existingKey.ExternalRefId unless the
incoming request provides a non-empty ExternalRefId by setting
updatedKey.ExternalRefId = request.ExternalRefId if non-empty, otherwise
updatedKey.ExternalRefId = existingKey.ExternalRefId (use the existingKey and
updatedKey symbols to locate the change and ensure ExternalRefId is included in
the struct literal).
🧹 Nitpick comments (3)
common/apikey/store.go (2)
254-257: RedundantStatus == Expiredcheck.After passing the check at line 249 (
if targetAPIKey.Status != Active), the status is guaranteed to beActive. The conditiontargetAPIKey.Status == Expiredat line 254 will always evaluate tofalse.♻️ Suggested simplification
- // Check if the API key has expired - if targetAPIKey.Status == Expired || (targetAPIKey.ExpiresAt != nil && time.Now().After(*targetAPIKey.ExpiresAt)) { + // Check if the API key has expired (time-based expiration even if status not yet updated) + if targetAPIKey.ExpiresAt != nil && time.Now().After(*targetAPIKey.ExpiresAt) { targetAPIKey.Status = Expired return false, nil }
353-353: Minor inconsistency: use parameterapiIdinstead ofapiKey.APIId.For consistency and clarity, prefer using the function parameter
apiIdover the fieldapiKey.APIIdwhen deleting from the external key index.♻️ Suggested fix
- delete(aks.externalKeyIndex[apiKey.APIId], indexKey) + delete(aks.externalKeyIndex[apiId], indexKey)gateway/gateway-controller/pkg/utils/api_key.go (1)
168-171: Reminder: rebuild gateway Docker images after gateway changes.Please run
cd gateway && make build-localfor gateway-controller updates.
3 participants
Purpose
Fix #947
This PR implements end-to-end external API key injection from Cloud APIM to hybrid gateways, enabling centralized key management with real-time distribution via WebSocket events. It allows hybrid gateways to receive, store, validate, and revoke API keys created externally while maintaining compatibility with locally-generated keys.
Goals
Hybrid gateways need the ability to:
Approach
New REST API Endpoints
/apis/{id}/api-keys/apis/{id}/api-keys/apis/{id}/api-keys/{apiKeyName}/apis/{id}/api-keys/{apiKeyName}/regenerate/apis/{id}/api-keys/{apiKeyName}Request/Response Highlights:
Database Schema Changes (Schema v6)
New columns added to api_keys table:
sourceTEXT DEFAULT 'local'localorexternalexternal_ref_idTEXT NULLindex_keyTEXT NULLdisplay_nameTEXTNew indexes for performance:
Constraints:
External Key Index for O(1) Validation
Index Structure:
externalKeyIndex: map[string]map[string]*string
└── API ID → SHA-256 Hash → Pointer to API Key ID
How it works:
Validation Flow:
Duplicate API Key Prevention
Mechanisms:
New Platform API REST API Endpoints
/apis/{id}/api-keys/apis/{id}/api-keys/{apiKeyName}/apis/{id}/api-keys/{apiKeyName}WebSocket Event Handling
Events supported:
apikey.createdhandleAPIKeyCreatedEventapikey.updatedhandleAPIKeyUpdatedEventapikey.revokedhandleAPIKeyRevokedEventWebSocket Message for creation (in the wss://localhost:9243/api/internal/v1/ws/gateways/connect connection)
Event Flow:
Cloud APIM → Platform-API → Gateway Events Service → Gateway-Controller → Policy-Engine
All events are broadcast to connected gateways where the API is deployed.
Event Broadcasting Retry Logic
Retry Configuration by Event Type:
Event Retry Matrix
Delivery Semantics:
On Broadcast Failure:
fmt.Errorf("failed to deliver API key created event: %w", lastError)
WebSocket Connection Retry Logic
Reconnection Strategy:
xDS Snapshot Updates
Trigger Points:
Update Process:
Event Processing Failure Behavior
When event handlers fail:
No Circuit Breaker implemented, but:
Hash Algorithm Support
Multiple hashing algorithms supported for flexibility:
SHA-256$sha256$<salt_hex>$<hash_hex>Bcrypt$2a$,$2b$,$2y$prefixesArgon2id$argon2id$prefixSecurity Features:
Other Improvements
- Minimum external key length increased: 20 characters
- Whitespace trimming on input: strings.TrimSpace(providedAPIKey)
- Empty key rejection with clear error messages
- Expiration timestamp validation (must be in the future)
- Correlation IDs for request tracing and audit trails
- Common module added to gateway-builder Dockerfile
- Updated Makefile targets for common module dependency
- Schema renamed: APIKeyGenerationResponse → APIKeyCreationResponse
- Enhanced field descriptions and pattern validations
- Added external_ref_id to response schema
Files Changed
Key modules affected:
Testing
New test file common/apikey/api_key_hash_test.go with:
Response:
Summary by CodeRabbit
New Features
Documentation