revamp gateway-controller#2277
Closed
tharindu1st wants to merge 2 commits into
Closed
Conversation
tharindu1st
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika and
tharikaGitHub
as code owners
Contributor
Dependency Validation ResultsDependency name: github.com/envoyproxy/go-control-plane Dependency name: github.com/wso2/api-platform/common Dependency name: github.com/wso2/api-platform/gateway/gateway-controller Dependency name: google.golang.org/protobuf Next Steps
|
Contributor
📝 WalkthroughRevamped the gateway controller into a more modular, event-gateway-focused setup. What changed
Impact
WalkthroughGateway controller startup now delegates to Sequence Diagram(s)sequenceDiagram
participant Client
participant EventAPIServer
participant WebhookSecretService
participant EventSQLStore
participant WebhookSecretStore
participant SnapshotManager
Client->>EventAPIServer: Create WebSub API secret
EventAPIServer->>WebhookSecretService: Generate(...)
WebhookSecretService->>EventSQLStore: SaveWebhookSecret(...)
WebhookSecretService->>WebhookSecretStore: Store plaintext secret
WebhookSecretService->>SnapshotManager: Refresh snapshot
EventAPIServer->>Client: 201 Created
sequenceDiagram
participant main
participant serverRun as server.Run
participant EventGatewayExtension
participant EventListener
main->>serverRun: Run(configPath, [NewEventGatewayExtension()])
serverRun->>EventGatewayExtension: Initialize(svc)
serverRun->>EventGatewayExtension: RegisterEventProcessors(listener, svc)
EventGatewayExtension->>EventListener: RegisterProcessor(WebhookSecretProcessor)
serverRun->>EventGatewayExtension: RegisterRoutes(router, svc)
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
gateway/gateway-controller/pkg/storage/sql_store.go (1)
53-58: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFail fast on invalid or duplicate kind registrations.
RegisterConfigKindsilently overwrites existing mappings and accepts unchecked table names, while later reads depend on this registry. Return an error or otherwise reject empty values, invalid identifiers, and remapping an existing kind to a different table during extension schema setup.Also applies to: 306-314, 990-1014
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/storage/sql_store.go` around lines 53 - 58, RegisterConfigKind currently overwrites existing kind-to-table mappings and accepts empty or invalid inputs, so update it to fail fast instead of silently registering bad state. Add validation for kind and tableName identifiers, and reject attempts to remap an existing kind to a different table while allowing the same mapping to be re-registered safely if needed. Propagate the failure through the extension schema setup path that calls RegisterConfigKind, and ensure the registry lookup code used by LoadFromDatabase only sees validated entries.gateway/gateway-controller/pkg/bootstrap/runner_test.go (1)
552-709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-nil extension case to this suite.
These updates only cover
generateAuthConfig(cfg, nil), so the new extension merge path is still untested. Add a small fake extension that asserts prefixed and legacy entries are generated and that duplicate route keys are handled explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/bootstrap/runner_test.go` around lines 552 - 709, The TestGenerateAuthConfig suite only covers generateAuthConfig(cfg, nil), so the extension merge path is still untested. Add a non-nil fake extension case in TestGenerateAuthConfig that exercises generateAuthConfig with an extension and verifies both prefixed and legacy resource-role entries are produced. Use the existing generateAuthConfig helper and ResourceRoles assertions, and explicitly cover how duplicate route keys are handled when the extension contributes overlapping routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@event-gateway/event-gateway-controller/Dockerfile`:
- Around line 27-29: The Dockerfile is copying the gateway-controller module
into a path that does not match the module’s local replace target, so fix the
COPY destination used in the `gateway-controller` build stage to match the path
expected by `go.mod` and `go mod download` (the one referenced by the
`gateway-controller` module’s replace). Update both the initial dependency-copy
step and the later copy step that reuse the `gateway-controller` module so the
files land under the same `/gateway/gateway-controller` location instead of
`/gateway-controller`.
In
`@event-gateway/event-gateway-controller/pkg/eventprocessors/webhook_secret.go`:
- Line 112: The upsert path in webhook_secret processing dereferences
providerManager without checking whether encryption is configured, so
CREATE/UPDATE events can panic when extension initialization leaves it nil.
Update the webhook secret upsert handling in the relevant method around the
Decrypt call to return early for CREATE/UPDATE when providerManager is nil, and
ensure the guard is applied before any providerManager.Decrypt usage.
In `@event-gateway/event-gateway-controller/pkg/extension/extension.go`:
- Around line 276-289: The artifact-clear flow in extension.go is using
e.db.GetWebhookSecretsByArtifact, so it only iterates DB rows and can miss stale
in-memory secrets after the rows are already gone. Update the clear logic to
read the current secrets from webhookSecretStore instead, then remove each
matching entry from the in-memory store using the existing Remove path and keep
the warning/error logging in the same clear loop.
In `@event-gateway/event-gateway-controller/pkg/services/webhook_secret.go`:
- Line 129: The mutating webhook secret flows currently ignore failures from
publishWebhookSecretEvent and Regenerate is still emitting a CREATE event for an
existing secret. Update the relevant methods that call
s.publishWebhookSecretEvent to return any publish error back to the caller
instead of treating the operation as successful, and change the regeneration
path to publish an UPDATE event. Use the existing symbols
publishWebhookSecretEvent, Regenerate, and the create/update call sites in
webhook_secret.go to make the fix consistently across the affected methods.
In `@gateway/gateway-controller/pkg/bootstrap/runner.go`:
- Around line 849-853: Reject duplicate auth entries when merging extension
authz maps in runner.go. In the extension merge loop that iterates over
extensions and calls AuthzEntries, detect when a methodAndPath already exists in
relativeRoles; if the existing roles differ, fail startup instead of
overwriting. If the duplicate roles are identical, allow it or treat it as a
no-op, and make the check explicit around the relativeRoles assignment.
- Around line 429-434: The extension option collection in runner.go is silently
discarding any PolicyXDSOptions entry that does not implement
policyxds.ServerOption, which hides wiring mistakes. Update the loop over
extensions and ext.PolicyXDSOptions() so that non-server options are treated as
an error: log the extension and option type, then fail fast instead of appending
only valid options and continuing. Use the existing extension iteration in
runner.go as the place to surface the invalid xDS option immediately.
- Around line 911-918: The raw adapter methods in eventListenerAdapter and the
related adapter at the other referenced location silently ignore failed type
assertions, which hides wiring mistakes; update RegisterEventProcessorRaw (and
the matching method) to surface the mismatch by returning an error or failing
startup instead of no-oping, and adjust the caller contract so bad eventType
values are detected early rather than silently dropping event processing.
In `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 148-150: The HMAC syncer registration order is wrong: the Client
contract in SetHMACSecretSyncer requires wiring before Start(), but the runner
currently starts cpClient before SetControlPlaneDeps, leaving early WebSub/HMAC
callbacks without a registered syncer. Reorder the bootstrap flow in runner.go
so extension/control-plane dependencies are set on cpClient before calling
cpClient.Start(), and keep the SetHMACSecretSyncer/SetControlPlaneDeps path
consistent with the pre-Start contract.
In `@gateway/gateway-controller/pkg/storage/sqlite.go`:
- Line 76: The SQLite schema version expectation has changed, so the
unsupported-schema test in sqlite_test.go is now out of sync. Update the test
assertion that currently expects schema version 3 to match the new
currentSchemaVersion value used in sqlite.go, and adjust the test case around
the unsupported-version check so it validates against version 4.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/bootstrap/runner_test.go`:
- Around line 552-709: The TestGenerateAuthConfig suite only covers
generateAuthConfig(cfg, nil), so the extension merge path is still untested. Add
a non-nil fake extension case in TestGenerateAuthConfig that exercises
generateAuthConfig with an extension and verifies both prefixed and legacy
resource-role entries are produced. Use the existing generateAuthConfig helper
and ResourceRoles assertions, and explicitly cover how duplicate route keys are
handled when the extension contributes overlapping routes.
In `@gateway/gateway-controller/pkg/storage/sql_store.go`:
- Around line 53-58: RegisterConfigKind currently overwrites existing
kind-to-table mappings and accepts empty or invalid inputs, so update it to fail
fast instead of silently registering bad state. Add validation for kind and
tableName identifiers, and reject attempts to remap an existing kind to a
different table while allowing the same mapping to be re-registered safely if
needed. Propagate the failure through the extension schema setup path that calls
RegisterConfigKind, and ensure the registry lookup code used by LoadFromDatabase
only sees validated entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3166066-14b7-4627-b852-5b58d11208e8
⛔ Files ignored due to path filters (2)
event-gateway/event-gateway-controller/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (37)
event-gateway/event-gateway-controller/Dockerfileevent-gateway/event-gateway-controller/Makefileevent-gateway/event-gateway-controller/cmd/main.goevent-gateway/event-gateway-controller/go.modevent-gateway/event-gateway-controller/pkg/eventprocessors/webhook_secret.goevent-gateway/event-gateway-controller/pkg/extension/extension.goevent-gateway/event-gateway-controller/pkg/services/webhook_secret.goevent-gateway/event-gateway-controller/pkg/storage/event-gateway-db.postgres.sqlevent-gateway/event-gateway-controller/pkg/storage/event-gateway-db.sqlevent-gateway/event-gateway-controller/pkg/storage/event-gateway-db.sqlserver.sqlevent-gateway/event-gateway-controller/pkg/storage/schema.goevent-gateway/event-gateway-controller/pkg/webhooksecretxds/snapshot.gogateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/handlers_test.gogateway/gateway-controller/pkg/bootstrap/extension.gogateway/gateway-controller/pkg/bootstrap/runner.gogateway/gateway-controller/pkg/bootstrap/runner_test.gogateway/gateway-controller/pkg/bootstrap/runtime_bootstrap.gogateway/gateway-controller/pkg/bootstrap/runtime_bootstrap_test.gogateway/gateway-controller/pkg/controlplane/api_deleted_test.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/controlplane/client_integration_test.gogateway/gateway-controller/pkg/controlplane/controlplane_test.gogateway/gateway-controller/pkg/eventlistener/listener.gogateway/gateway-controller/pkg/eventlistener/listener_test.gogateway/gateway-controller/pkg/policyxds/combined_cache.gogateway/gateway-controller/pkg/policyxds/server.gogateway/gateway-controller/pkg/secrets/service_test.gogateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sqlgateway/gateway-controller/pkg/storage/interface.gogateway/gateway-controller/pkg/storage/sql_store.gogateway/gateway-controller/pkg/storage/sqlite.gogateway/gateway-controller/pkg/subscriptionxds/subscription_snapshot_test.gogateway/gateway-controller/pkg/utils/mock_db_test.go
💤 Files with no reviewable changes (3)
- gateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sql
- gateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sql
- gateway/gateway-controller/pkg/eventlistener/listener_test.go
Comment on lines
+276
to
+289
| secrets, err := e.db.GetWebhookSecretsByArtifact(artifactID) | ||
| if err != nil { | ||
| e.log.Error("Failed to fetch webhook secrets for artifact clear", | ||
| slog.String("artifact_id", artifactID), | ||
| slog.Any("error", err)) | ||
| return | ||
| } | ||
| for _, ws := range secrets { | ||
| if err := e.webhookSecretStore.Remove(ws.ArtifactUUID, ws.Name); err != nil && err != webhooksecret.ErrNotFound { | ||
| e.log.Warn("Failed to remove webhook secret from memory during artifact clear", | ||
| slog.String("secret_uuid", ws.UUID), | ||
| slog.Any("error", err)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear artifact secrets from the in-memory store, not from the DB result set.
If the DB rows have already been removed, this fetch returns no rows and leaves stale in-memory secrets. Use the store contents for the artifact when clearing.
Proposed fix
- secrets, err := e.db.GetWebhookSecretsByArtifact(artifactID)
- if err != nil {
- e.log.Error("Failed to fetch webhook secrets for artifact clear",
- slog.String("artifact_id", artifactID),
- slog.Any("error", err))
- return
- }
- for _, ws := range secrets {
- if err := e.webhookSecretStore.Remove(ws.ArtifactUUID, ws.Name); err != nil && err != webhooksecret.ErrNotFound {
+ for secretName := range e.webhookSecretStore.GetAll()[artifactID] {
+ if err := e.webhookSecretStore.Remove(artifactID, secretName); err != nil && err != webhooksecret.ErrNotFound {
e.log.Warn("Failed to remove webhook secret from memory during artifact clear",
- slog.String("secret_uuid", ws.UUID),
+ slog.String("artifact_id", artifactID),
+ slog.String("secret_name", secretName),
slog.Any("error", err))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| secrets, err := e.db.GetWebhookSecretsByArtifact(artifactID) | |
| if err != nil { | |
| e.log.Error("Failed to fetch webhook secrets for artifact clear", | |
| slog.String("artifact_id", artifactID), | |
| slog.Any("error", err)) | |
| return | |
| } | |
| for _, ws := range secrets { | |
| if err := e.webhookSecretStore.Remove(ws.ArtifactUUID, ws.Name); err != nil && err != webhooksecret.ErrNotFound { | |
| e.log.Warn("Failed to remove webhook secret from memory during artifact clear", | |
| slog.String("secret_uuid", ws.UUID), | |
| slog.Any("error", err)) | |
| } | |
| } | |
| for secretName := range e.webhookSecretStore.GetAll()[artifactID] { | |
| if err := e.webhookSecretStore.Remove(artifactID, secretName); err != nil && err != webhooksecret.ErrNotFound { | |
| e.log.Warn("Failed to remove webhook secret from memory during artifact clear", | |
| slog.String("artifact_id", artifactID), | |
| slog.String("secret_name", secretName), | |
| slog.Any("error", err)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@event-gateway/event-gateway-controller/pkg/extension/extension.go` around
lines 276 - 289, The artifact-clear flow in extension.go is using
e.db.GetWebhookSecretsByArtifact, so it only iterates DB rows and can miss stale
in-memory secrets after the rows are already gone. Update the clear logic to
read the current secrets from webhookSecretStore instead, then remove each
matching entry from the in-memory store using the existing Remove path and keep
the warning/error logging in the same clear loop.
Comment on lines
+429
to
+434
| for _, ext := range extensions { | ||
| for _, opt := range ext.PolicyXDSOptions() { | ||
| if serverOpt, ok := opt.(policyxds.ServerOption); ok { | ||
| serverOpts = append(serverOpts, serverOpt) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not ignore invalid extension xDS options.
This loop silently drops any value that is not a policyxds.ServerOption. That makes extension wiring mistakes look like a successful startup while the extra xDS behavior never turns on. Log the extension/type and fail fast here instead of continuing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/gateway-controller/pkg/bootstrap/runner.go` around lines 429 - 434,
The extension option collection in runner.go is silently discarding any
PolicyXDSOptions entry that does not implement policyxds.ServerOption, which
hides wiring mistakes. Update the loop over extensions and
ext.PolicyXDSOptions() so that non-server options are treated as an error: log
the extension and option type, then fail fast instead of appending only valid
options and continuing. Use the existing extension iteration in runner.go as the
place to surface the invalid xDS option immediately.
Comment on lines
+849
to
+853
| // Merge in authorization entries from each extension. | ||
| for _, ext := range extensions { | ||
| for methodAndPath, roles := range ext.AuthzEntries() { | ||
| relativeRoles[methodAndPath] = roles | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject duplicate auth entries from extensions.
These assignments overwrite existing role mappings on key collision. If an extension reuses a built-in route key by mistake, the controller starts with a different authorization map for that route. Detect duplicates and stop startup unless the role sets are intentionally identical.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/gateway-controller/pkg/bootstrap/runner.go` around lines 849 - 853,
Reject duplicate auth entries when merging extension authz maps in runner.go. In
the extension merge loop that iterates over extensions and calls AuthzEntries,
detect when a methodAndPath already exists in relativeRoles; if the existing
roles differ, fail startup instead of overwriting. If the duplicate roles are
identical, allow it or treat it as a no-op, and make the check explicit around
the relativeRoles assignment.
Comment on lines
+911
to
+918
| func (a *eventListenerAdapter) RegisterEventProcessorRaw(eventType interface{}, fn func(interface{})) { | ||
| et, ok := eventType.(eventhub.EventType) | ||
| if !ok { | ||
| return | ||
| } | ||
| a.l.RegisterEventProcessor(et, func(event eventhub.Event) { | ||
| fn(event) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Surface raw adapter type mismatches.
Both adapters return quietly on failed type assertions. That turns extension contract mistakes into silent loss of event processing or control-plane wiring. Return an error or fail startup instead of no-oping here.
Also applies to: 926-929
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/gateway-controller/pkg/bootstrap/runner.go` around lines 911 - 918,
The raw adapter methods in eventListenerAdapter and the related adapter at the
other referenced location silently ignore failed type assertions, which hides
wiring mistakes; update RegisterEventProcessorRaw (and the matching method) to
surface the mismatch by returning an error or failing startup instead of
no-oping, and adjust the caller contract so bad eventType values are detected
early rather than silently dropping event processing.
Comment on lines
+148
to
+150
| // SetHMACSecretSyncer registers an HMAC secret syncer. Must be called before Start(). | ||
| func (c *Client) SetHMACSecretSyncer(s HMACSecretSyncer) { | ||
| c.hmacSecretSyncer = s |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Register the HMAC syncer before starting the client.
Line 148 documents a pre-Start() contract, but the provided runner wiring starts cpClient before calling SetControlPlaneDeps, so early WebSub/HMAC events can hit these delegate methods with no syncer registered. Move extension dependency wiring before cpClient.Start().
Proposed ordering fix in gateway/gateway-controller/pkg/bootstrap/runner.go
cpClient := controlplane.NewClient(
...
)
-if err := cpClient.Start(); err != nil {
- log.Error("Failed to start control plane client", slog.Any("error", err))
- // Don't fail startup — gateway can run in degraded mode without control plane.
-}
// Wire extension deps into the control plane client.
for _, ext := range extensions {
ext.SetControlPlaneDeps(&controlPlaneAdapter{cpClient})
}
+
+if err := cpClient.Start(); err != nil {
+ log.Error("Failed to start control plane client", slog.Any("error", err))
+ // Don't fail startup — gateway can run in degraded mode without control plane.
+}Also applies to: 2489-2499
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/gateway-controller/pkg/controlplane/client.go` around lines 148 -
150, The HMAC syncer registration order is wrong: the Client contract in
SetHMACSecretSyncer requires wiring before Start(), but the runner currently
starts cpClient before SetControlPlaneDeps, leaving early WebSub/HMAC callbacks
without a registered syncer. Reorder the bootstrap flow in runner.go so
extension/control-plane dependencies are set on cpClient before calling
cpClient.Start(), and keep the SetHMACSecretSyncer/SetControlPlaneDeps path
consistent with the pre-Start contract.
| } | ||
|
|
||
| const currentSchemaVersion = 3 | ||
| const currentSchemaVersion = 4 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the SQLite schema-version test expectation.
Line 76 now reports expected 4, but the provided sqlite_test.go snippet still checks for expected 3, so that unsupported-schema test will fail.
Suggested test update
- assert.ErrorContains(t, err, "failed to initialize schema: unsupported schema version 5, expected 3; delete the database to recreate")
+ assert.ErrorContains(t, err, "failed to initialize schema: unsupported schema version 5, expected 4; delete the database to recreate")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/gateway-controller/pkg/storage/sqlite.go` at line 76, The SQLite
schema version expectation has changed, so the unsupported-schema test in
sqlite_test.go is now out of sync. Update the test assertion that currently
expects schema version 3 to match the new currentSchemaVersion value used in
sqlite.go, and adjust the test case around the unsupported-version check so it
validates against version 4.
5832da8 to
01da668
Compare
Contributor
Dependency Validation ResultsDependency name: github.com/gin-gonic/gin Dependency name: github.com/google/uuid Dependency name: github.com/wso2/api-platform/common Dependency name: github.com/wso2/api-platform/gateway/gateway-controller Next Steps
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@event-gateway/event-gateway-controller/Makefile`:
- Around line 21-23: The default version is inconsistent across build and
packaging entrypoints, so align the VERSION fallback in Makefile with the
Dockerfile and the Helm-side default used for deployment. Update the version
default in the Makefile so the image tag and embedded metadata resolve to the
same baseline version regardless of whether the build is triggered through make,
Dockerfile, or Helm, and keep the related version constants in sync.
In `@event-gateway/event-gateway-controller/pkg/extension/extension.go`:
- Around line 73-75: The initialization path in extension.go should not continue
when loadWebhookSecretsFromDatabase fails, because the in-memory secret store
would be incomplete. Update the webhook secret loading block in the extension
setup (around svc.EncryptionProviderManager and loadWebhookSecretsFromDatabase)
to return the error after logging it instead of only calling svc.Logger.Warn, so
startup fails fast when stored secrets cannot be restored.
In `@event-gateway/event-gateway-controller/pkg/handlers/event_api_server.go`:
- Around line 143-155: The publishEvent helper in EventAPIServer only logs
EventHub publish errors, so delete flows can still succeed after storage
mutation even when the sync event fails. Update the delete path to surface the
publish failure back to the caller, either by changing publishEvent to return an
error and handling it in the delete handlers or by restructuring the
delete-and-publish sequence into one durable operation so the request cannot
return success unless the event is emitted.
- Around line 80-88: The WebBroker router in eventApiServer is missing the
update endpoint registration, so add the missing PUT route for a WebBroker API
alongside the existing CreateWebBrokerAPI, ListWebBrokerAPIs,
GetWebBrokerAPIById, and DeleteWebBrokerAPI handlers. Register the
/webbroker-apis/:id route in the same router setup where the other WebBroker API
routes are defined, and point it to the appropriate update handler so CRUD is
complete.
In
`@event-gateway/event-gateway-controller/pkg/handlers/webbroker_api_handler.go`:
- Around line 160-170: DeleteWebBroker currently removes only the API config in
webbroker_api_handler, leaving API keys behind. Update the delete flow to use
the same key-cleanup path as DeleteWebSubAPI so WebBroker key records/state are
removed before or as part of the Storage.DeleteConfig call. Locate the logic in
the WebBroker delete handler and reuse the shared cleanup/deletion helper or
equivalent key-deletion sequence used by DeleteWebSubAPI.
- Around line 114-129: The error handling in webbroker_api_handler.go is
collapsing all non-database lookup failures from GetConfigByKindAndHandle into a
404, which hides real storage errors. Update the handler logic in the
WebBrokerApi lookup path (and the matching WebSub secret handler path) so that
only a true “not found” condition returns 404, while unexpected errors fall
through to a generic 500/internal error response like the other handlers in this
package. Keep the existing IsDatabaseUnavailableError branch, and use the same
handler methods and storage call sites to preserve the intended behavior.
In `@event-gateway/event-gateway-controller/pkg/handlers/websub_api_handler.go`:
- Around line 181-196: The WebSub API config lookup error handling in the
handler path currently turns every non-database-unavailable failure from
GetConfigByKindAndHandle into a 404, which hides unexpected storage errors.
Update the affected handler branches to distinguish a true not-found case from
other errors, like the secret handlers already do, and return a 500/internal
error for unexpected lookup failures while keeping 404 only for missing configs.
Apply the same fix in the other WebSub API handler blocks that use
GetConfigByKindAndHandle.
- Around line 281-294: The WebSub delete flow in websub_api_handler’s delete
handler currently treats RemoveAPIKeysAPI as best-effort, so a failed API key
cleanup still returns success after DeleteConfig. Update the handler to use a
single failure path after DeleteConfig succeeds: if RemoveAPIKeysAPI returns an
error, log it and return an error response instead of continuing to the 200
success path. Keep the behavior localized to the delete handler and reference
the existing DeleteConfig and RemoveAPIKeysAPI calls so both resources are
handled consistently.
- Around line 557-563: The webhook secret creation flow currently calls Generate
even when WebhookSecretCreationRequest.DisplayName is missing or blank. In
websub_api_handler.go, add a TrimSpace validation for request.DisplayName right
after bindRequestBody and before webhookSecretService.Generate, and return an
HTTP 400 ErrorResponse with a clear message if it is empty.
In `@event-gateway/event-gateway-controller/pkg/storage/sql_store.go`:
- Around line 153-167: The webhook-secret storage methods are not consistently
scoped to the active gateway, so queries can affect rows from other gateways
when the database is shared. Update the SQL in sql_store.go for the secret CRUD
and bulk-load paths (the create/update/delete/read helpers around the shown Exec
call) to always filter by s.gatewayID, using the existing s.gatewayID field in
WHERE clauses and lookup conditions. Make sure every operation that reads,
updates, deletes, or loads secrets applies the gateway scope consistently so
secret rows are isolated per gateway.
- Around line 112-138: The webhook_secrets schema in sql_store.go does not
guarantee a single row per uuid, but GetWebhookSecretByUUID relies on uuid being
uniquely identifiable. Update the DDL in the webhook_secrets table definition
for both dialect branches so uuid is declared as a primary key or at least has a
unique constraint, while preserving the existing gateway_id/artifact_uuid/name
uniqueness. Use the webhook_secrets table creation block and the
GetWebhookSecretByUUID-related storage paths to ensure the schema matches the
lookup expectation.
In `@gateway/gateway-controller/pkg/server/runtime_bootstrap.go`:
- Around line 127-149: The startup rendering path is mutating the shared
apiConfig returned from configStore.GetAll(), which can leak resolved values
into the stored in-memory config. Update the runtime_bootstrap flow around
templateengine.RenderSpec and transformer.Transform to operate on a deep copy of
apiConfig for rendering/transformation, then use only the copy for runtime
processing while leaving the original stored config unchanged and unresolved.
In `@gateway/gateway-controller/pkg/server/server.go`:
- Around line 648-653: The REST server setup in server.go only sets
ReadHeaderTimeout, leaving request body reads, response writes, and idle
keep-alives unbounded. Update the http.Server initialization in the server
startup block to also set explicit ReadTimeout, WriteTimeout, and IdleTimeout
values alongside the existing APIPort/Handler configuration. Use the server
creation code near the log.Info("Starting REST API server") call as the place to
apply these lifecycle limits.
- Around line 449-506: `extension.Services` is missing the `APIKeyService`
field, so extensions may see a nil dependency at initialization. Update the `svc
:= &extension.Services{...}` literal in `server.go` to assign `APIKeyService`
using the existing API key service instance used in this setup, alongside the
other service fields like `APIDeploymentService` and `ControlPlaneClient`,
before calling extension initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ade8010-ef2b-48a5-b31a-0ef58573b4a7
⛔ Files ignored due to path filters (1)
event-gateway/event-gateway-controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
event-gateway/Makefileevent-gateway/event-gateway-controller/Dockerfileevent-gateway/event-gateway-controller/Makefileevent-gateway/event-gateway-controller/cmd/controller/main.goevent-gateway/event-gateway-controller/go.modevent-gateway/event-gateway-controller/pkg/eventprocessors/webhook_secret_processor.goevent-gateway/event-gateway-controller/pkg/extension/extension.goevent-gateway/event-gateway-controller/pkg/handlers/event_api_server.goevent-gateway/event-gateway-controller/pkg/handlers/webbroker_api_handler.goevent-gateway/event-gateway-controller/pkg/handlers/websub_api_handler.goevent-gateway/event-gateway-controller/pkg/service/webhook_secret.goevent-gateway/event-gateway-controller/pkg/storage/interface.goevent-gateway/event-gateway-controller/pkg/storage/sql_store.gogateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/websub_api_handler.gogateway/gateway-controller/pkg/api/management/event_types.gogateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/eventlistener/listener.gogateway/gateway-controller/pkg/eventlistener/listener_test.gogateway/gateway-controller/pkg/eventlistener/webhook_secret_processor.gogateway/gateway-controller/pkg/extension/extension.gogateway/gateway-controller/pkg/server/runtime_bootstrap.gogateway/gateway-controller/pkg/server/server.gogateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sqlgateway/gateway-controller/pkg/storage/interface.gogateway/gateway-controller/pkg/storage/loaders.gogateway/gateway-controller/pkg/storage/sql_store.go
💤 Files with no reviewable changes (10)
- gateway/gateway-controller/pkg/api/handlers/websub_api_handler.go
- gateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sql
- gateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sql
- gateway/gateway-controller/pkg/eventlistener/webhook_secret_processor.go
- gateway/gateway-controller/pkg/eventlistener/listener_test.go
- gateway/gateway-controller/pkg/storage/loaders.go
- gateway/gateway-controller/pkg/storage/sql_store.go
- gateway/gateway-controller/pkg/storage/interface.go
- gateway/gateway-controller/pkg/storage/gateway-controller-db.sql
- gateway/gateway-controller/api/management-openapi.yaml
✅ Files skipped from review due to trivial changes (1)
- gateway/gateway-controller/pkg/api/management/event_types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- event-gateway/event-gateway-controller/go.mod
Comment on lines
+449
to
+506
| apiSvc := utils.NewAPIDeploymentService(configStore, db, snapshotManager, validator, &cfg.Router, eventHubInstance, gatewayID, secretsService) | ||
| mcpSvc := utils.NewMCPDeploymentService(configStore, db, snapshotManager, policyManager, policyValidator, eventHubInstance, gatewayID, secretsService) | ||
| llmSvc := utils.NewLLMDeploymentService(configStore, db, snapshotManager, lazyResourceXDSManager, templateDefinitions, | ||
| apiSvc, &cfg.Router, policyVersionResolver, policyValidator) | ||
|
|
||
| cpClient := controlplane.NewClient( | ||
| cfg.Controller.ControlPlane, | ||
| log, configStore, | ||
| db, snapshotManager, | ||
| validator, | ||
| &cfg.Router, | ||
| apiKeyXDSManager, apiKeyStore, | ||
| &cfg.APIKey, | ||
| policyManager, | ||
| cfg, policyDefinitions, | ||
| lazyResourceXDSManager, | ||
| templateDefinitions, | ||
| subscriptionSnapshotManager, | ||
| eventHubInstance, | ||
| secretsService, | ||
| webhookSecretStore, | ||
| webhookSecretSnapshotManager, | ||
| ) | ||
| if err := cpClient.Start(); err != nil { | ||
| log.Error("Failed to start control plane client", slog.Any("error", err)) | ||
| // Don't fail startup — gateway can run in degraded mode without control plane | ||
| } | ||
|
|
||
| restAPIService := restapi.NewRestAPIService( | ||
| configStore, db, snapshotManager, policyManager, | ||
| apiSvc, apiKeyXDSManager, | ||
| cpClient, &cfg.Router, cfg, | ||
| &http.Client{Timeout: 10 * time.Second}, config.NewParser(), validator, log, | ||
| eventHubInstance, secretsService, | ||
| ) | ||
| igw := immutable.NewImmutableGW(cfg.ImmutableGateway, restAPIService, llmSvc, mcpSvc) | ||
|
|
||
| // Build the Services struct for extensions and call Initialize on each. | ||
| svc := &extension.Services{ | ||
| Storage: db, | ||
| ConfigStore: configStore, | ||
| GatewayID: gatewayID, | ||
| Logger: log, | ||
| Config: cfg, | ||
| SnapshotManager: snapshotManager, | ||
| PolicyManager: policyManager, | ||
| EventHub: eventHubInstance, | ||
| SecretService: secretsService, | ||
| EncryptionProviderManager: encryptionProviderManager, | ||
| WebhookSecretStore: webhookSecretStore, | ||
| WebhookSecretSnapshotManager: webhookSecretSnapshotManager, | ||
| APIDeploymentService: apiSvc, | ||
| APIKeyXDSManager: apiKeyXDSManager, | ||
| ControlPlaneClient: cpClient, | ||
| RouterConfig: &cfg.Router, | ||
| Validator: validator, | ||
| PolicyDefinitions: policyDefinitions, | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Populate Services.APIKeyService before extension initialization.
extension.Services exposes APIKeyService, but the struct literal leaves it nil. Extensions using the documented service field can fail at runtime.
Proposed fix
apiSvc := utils.NewAPIDeploymentService(configStore, db, snapshotManager, validator, &cfg.Router, eventHubInstance, gatewayID, secretsService)
+ apiKeyService := utils.NewAPIKeyService(configStore, db, apiKeyXDSManager, &cfg.APIKey, eventHubInstance, gatewayID)
mcpSvc := utils.NewMCPDeploymentService(configStore, db, snapshotManager, policyManager, policyValidator, eventHubInstance, gatewayID, secretsService)
llmSvc := utils.NewLLMDeploymentService(configStore, db, snapshotManager, lazyResourceXDSManager, templateDefinitions,
apiSvc, &cfg.Router, policyVersionResolver, policyValidator)
@@
WebhookSecretStore: webhookSecretStore,
WebhookSecretSnapshotManager: webhookSecretSnapshotManager,
APIDeploymentService: apiSvc,
+ APIKeyService: apiKeyService,
APIKeyXDSManager: apiKeyXDSManager,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| apiSvc := utils.NewAPIDeploymentService(configStore, db, snapshotManager, validator, &cfg.Router, eventHubInstance, gatewayID, secretsService) | |
| mcpSvc := utils.NewMCPDeploymentService(configStore, db, snapshotManager, policyManager, policyValidator, eventHubInstance, gatewayID, secretsService) | |
| llmSvc := utils.NewLLMDeploymentService(configStore, db, snapshotManager, lazyResourceXDSManager, templateDefinitions, | |
| apiSvc, &cfg.Router, policyVersionResolver, policyValidator) | |
| cpClient := controlplane.NewClient( | |
| cfg.Controller.ControlPlane, | |
| log, configStore, | |
| db, snapshotManager, | |
| validator, | |
| &cfg.Router, | |
| apiKeyXDSManager, apiKeyStore, | |
| &cfg.APIKey, | |
| policyManager, | |
| cfg, policyDefinitions, | |
| lazyResourceXDSManager, | |
| templateDefinitions, | |
| subscriptionSnapshotManager, | |
| eventHubInstance, | |
| secretsService, | |
| webhookSecretStore, | |
| webhookSecretSnapshotManager, | |
| ) | |
| if err := cpClient.Start(); err != nil { | |
| log.Error("Failed to start control plane client", slog.Any("error", err)) | |
| // Don't fail startup — gateway can run in degraded mode without control plane | |
| } | |
| restAPIService := restapi.NewRestAPIService( | |
| configStore, db, snapshotManager, policyManager, | |
| apiSvc, apiKeyXDSManager, | |
| cpClient, &cfg.Router, cfg, | |
| &http.Client{Timeout: 10 * time.Second}, config.NewParser(), validator, log, | |
| eventHubInstance, secretsService, | |
| ) | |
| igw := immutable.NewImmutableGW(cfg.ImmutableGateway, restAPIService, llmSvc, mcpSvc) | |
| // Build the Services struct for extensions and call Initialize on each. | |
| svc := &extension.Services{ | |
| Storage: db, | |
| ConfigStore: configStore, | |
| GatewayID: gatewayID, | |
| Logger: log, | |
| Config: cfg, | |
| SnapshotManager: snapshotManager, | |
| PolicyManager: policyManager, | |
| EventHub: eventHubInstance, | |
| SecretService: secretsService, | |
| EncryptionProviderManager: encryptionProviderManager, | |
| WebhookSecretStore: webhookSecretStore, | |
| WebhookSecretSnapshotManager: webhookSecretSnapshotManager, | |
| APIDeploymentService: apiSvc, | |
| APIKeyXDSManager: apiKeyXDSManager, | |
| ControlPlaneClient: cpClient, | |
| RouterConfig: &cfg.Router, | |
| Validator: validator, | |
| PolicyDefinitions: policyDefinitions, | |
| } | |
| apiSvc := utils.NewAPIDeploymentService(configStore, db, snapshotManager, validator, &cfg.Router, eventHubInstance, gatewayID, secretsService) | |
| apiKeyService := utils.NewAPIKeyService(configStore, db, apiKeyXDSManager, &cfg.APIKey, eventHubInstance, gatewayID) | |
| mcpSvc := utils.NewMCPDeploymentService(configStore, db, snapshotManager, policyManager, policyValidator, eventHubInstance, gatewayID, secretsService) | |
| llmSvc := utils.NewLLMDeploymentService(configStore, db, snapshotManager, lazyResourceXDSManager, templateDefinitions, | |
| apiSvc, &cfg.Router, policyVersionResolver, policyValidator) | |
| cpClient := controlplane.NewClient( | |
| cfg.Controller.ControlPlane, | |
| log, configStore, | |
| db, snapshotManager, | |
| validator, | |
| &cfg.Router, | |
| apiKeyXDSManager, apiKeyStore, | |
| &cfg.APIKey, | |
| policyManager, | |
| cfg, policyDefinitions, | |
| lazyResourceXDSManager, | |
| templateDefinitions, | |
| subscriptionSnapshotManager, | |
| eventHubInstance, | |
| secretsService, | |
| webhookSecretStore, | |
| webhookSecretSnapshotManager, | |
| ) | |
| if err := cpClient.Start(); err != nil { | |
| log.Error("Failed to start control plane client", slog.Any("error", err)) | |
| // Don't fail startup — gateway can run in degraded mode without control plane | |
| } | |
| restAPIService := restapi.NewRestAPIService( | |
| configStore, db, snapshotManager, policyManager, | |
| apiSvc, apiKeyXDSManager, | |
| cpClient, &cfg.Router, cfg, | |
| &http.Client{Timeout: 10 * time.Second}, config.NewParser(), validator, log, | |
| eventHubInstance, secretsService, | |
| ) | |
| igw := immutable.NewImmutableGW(cfg.ImmutableGateway, restAPIService, llmSvc, mcpSvc) | |
| // Build the Services struct for extensions and call Initialize on each. | |
| svc := &extension.Services{ | |
| Storage: db, | |
| ConfigStore: configStore, | |
| GatewayID: gatewayID, | |
| Logger: log, | |
| Config: cfg, | |
| SnapshotManager: snapshotManager, | |
| PolicyManager: policyManager, | |
| EventHub: eventHubInstance, | |
| SecretService: secretsService, | |
| EncryptionProviderManager: encryptionProviderManager, | |
| WebhookSecretStore: webhookSecretStore, | |
| WebhookSecretSnapshotManager: webhookSecretSnapshotManager, | |
| APIDeploymentService: apiSvc, | |
| APIKeyService: apiKeyService, | |
| APIKeyXDSManager: apiKeyXDSManager, | |
| ControlPlaneClient: cpClient, | |
| RouterConfig: &cfg.Router, | |
| Validator: validator, | |
| PolicyDefinitions: policyDefinitions, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/gateway-controller/pkg/server/server.go` around lines 449 - 506,
`extension.Services` is missing the `APIKeyService` field, so extensions may see
a nil dependency at initialization. Update the `svc := &extension.Services{...}`
literal in `server.go` to assign `APIKeyService` using the existing API key
service instance used in this setup, alongside the other service fields like
`APIDeploymentService` and `ControlPlaneClient`, before calling extension
initialization.
01da668 to
4ab7267
Compare
Contributor
Dependency Validation ResultsDependency name: github.com/gin-gonic/gin Dependency name: github.com/google/uuid Dependency name: github.com/wso2/api-platform/common Dependency name: github.com/wso2/api-platform/gateway/gateway-controller Next Steps
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/api/management/generated.go (1)
41-43: 🎯 Functional Correctness | 🔵 TrivialVerify the intended source of
APIKeyStatusconstants.The generated file
gateway/gateway-controller/pkg/api/management/generated.godefines unprefixed constants (Active,Expired,Revoked), while the rest of the codebase uses prefixed constants (models.APIKeyStatusActive, etc.) defined ingateway/gateway-controller/pkg/models/api_key.go.This inconsistency suggests that either:
- The generated constants are unused and can be removed or aligned with the
modelspackage definition.- The OpenAPI specification used for generation should be updated to include the
APIKeyStatusprefix in the constant names to match the existing codebase style.Ensure the
generated.gofile does not introduce breaking changes or shadow themodelsconstants. If the generated constants are not intended for external use, disable their export or align the naming via OpenAPI conventions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/api/management/generated.go` around lines 41 - 43, The `APIKeyStatus` constants in `generated.go` are exported without the `APIKeyStatus` prefix, which conflicts with the existing `models.APIKeyStatus*` naming used elsewhere. Update the generated enum source so the constants are either aligned to the `models` package naming convention or not exported at all, and verify the OpenAPI spec/generator settings are the intended source for these names. Make sure `APIKeyStatus` generation does not shadow or introduce alternate symbols compared to `models/api_key.go`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@event-gateway/event-gateway-controller/pkg/service/webhook_secret.go`:
- Around line 147-149: The webhook secret flow in Generate and Regenerate should
not return an error after the secret has already been persisted; if
publishWebhookSecretEvent fails after the DB upsert/commit, the request should
still return the committed result and the failure should be logged for recovery.
Update the post-persist handling around publishWebhookSecretEvent in
webhook_secret.go so it matches the existing DB-reload recovery pattern instead
of propagating an error, and apply the same fix in the Generate/Regenerate paths
referenced by the webhook secret service methods.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/api/management/generated.go`:
- Around line 41-43: The `APIKeyStatus` constants in `generated.go` are exported
without the `APIKeyStatus` prefix, which conflicts with the existing
`models.APIKeyStatus*` naming used elsewhere. Update the generated enum source
so the constants are either aligned to the `models` package naming convention or
not exported at all, and verify the OpenAPI spec/generator settings are the
intended source for these names. Make sure `APIKeyStatus` generation does not
shadow or introduce alternate symbols compared to `models/api_key.go`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e3bdcd6-b6e1-476f-bf0c-26005f2abacd
⛔ Files ignored due to path filters (1)
event-gateway/event-gateway-controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
event-gateway/Makefileevent-gateway/event-gateway-controller/Dockerfileevent-gateway/event-gateway-controller/Makefileevent-gateway/event-gateway-controller/cmd/controller/main.goevent-gateway/event-gateway-controller/go.modevent-gateway/event-gateway-controller/pkg/eventprocessors/webhook_secret_processor.goevent-gateway/event-gateway-controller/pkg/extension/extension.goevent-gateway/event-gateway-controller/pkg/handlers/event_api_server.goevent-gateway/event-gateway-controller/pkg/handlers/webbroker_api_handler.goevent-gateway/event-gateway-controller/pkg/handlers/websub_api_handler.goevent-gateway/event-gateway-controller/pkg/service/webhook_secret.goevent-gateway/event-gateway-controller/pkg/storage/interface.goevent-gateway/event-gateway-controller/pkg/storage/sql_store.gogateway/build-manifest.yamlgateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/webbroker_api_handler.gogateway/gateway-controller/pkg/api/handlers/websub_api_handler.gogateway/gateway-controller/pkg/api/management/event_types.gogateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/eventlistener/listener.gogateway/gateway-controller/pkg/eventlistener/listener_test.gogateway/gateway-controller/pkg/eventlistener/webhook_secret_processor.gogateway/gateway-controller/pkg/extension/extension.gogateway/gateway-controller/pkg/server/runtime_bootstrap.gogateway/gateway-controller/pkg/server/server.gogateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sqlgateway/gateway-controller/pkg/storage/interface.gogateway/gateway-controller/pkg/storage/loaders.gogateway/gateway-controller/pkg/storage/sql_store.go
💤 Files with no reviewable changes (11)
- gateway/gateway-controller/pkg/api/handlers/websub_api_handler.go
- gateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sql
- gateway/gateway-controller/pkg/storage/interface.go
- gateway/gateway-controller/pkg/eventlistener/listener_test.go
- gateway/gateway-controller/pkg/storage/sql_store.go
- gateway/gateway-controller/api/management-openapi.yaml
- gateway/gateway-controller/pkg/storage/gateway-controller-db.sql
- gateway/gateway-controller/pkg/api/handlers/webbroker_api_handler.go
- gateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sql
- gateway/gateway-controller/pkg/storage/loaders.go
- gateway/gateway-controller/pkg/eventlistener/webhook_secret_processor.go
✅ Files skipped from review due to trivial changes (1)
- gateway/gateway-controller/pkg/api/management/event_types.go
🚧 Files skipped from review as they are similar to previous changes (15)
- gateway/gateway-controller/pkg/extension/extension.go
- event-gateway/Makefile
- event-gateway/event-gateway-controller/pkg/storage/interface.go
- event-gateway/event-gateway-controller/cmd/controller/main.go
- event-gateway/event-gateway-controller/go.mod
- gateway/gateway-controller/pkg/server/runtime_bootstrap.go
- event-gateway/event-gateway-controller/pkg/extension/extension.go
- gateway/gateway-controller/pkg/eventlistener/listener.go
- gateway/gateway-controller/pkg/api/handlers/handlers.go
- event-gateway/event-gateway-controller/pkg/eventprocessors/webhook_secret_processor.go
- event-gateway/event-gateway-controller/Dockerfile
- event-gateway/event-gateway-controller/pkg/storage/sql_store.go
- gateway/gateway-controller/pkg/server/server.go
- event-gateway/event-gateway-controller/pkg/handlers/websub_api_handler.go
- event-gateway/event-gateway-controller/pkg/handlers/webbroker_api_handler.go
Comment on lines
+147
to
+149
| if err := s.publishWebhookSecretEvent("CREATE", artifactUUID, ws.UUID, ws.Name, correlationID); err != nil { | ||
| return nil, "", fmt.Errorf("failed to publish webhook secret create event: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid failing the request after the secret has already been committed.
Generate and Regenerate persist the new ciphertext before publishing the event; if publish then fails, callers get an error and do not receive the one-time plaintext even though the stored secret changed. Either make persistence and event publication atomic, or log the publish failure and return the committed result.
Based on learnings, failures after a successful DB upsert are intentionally logged and recovered from DB reload rather than forcing rollback/error propagation.
Also applies to: 203-205, 241-242
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@event-gateway/event-gateway-controller/pkg/service/webhook_secret.go` around
lines 147 - 149, The webhook secret flow in Generate and Regenerate should not
return an error after the secret has already been persisted; if
publishWebhookSecretEvent fails after the DB upsert/commit, the request should
still return the committed result and the failure should be logged for recovery.
Update the post-persist handling around publishWebhookSecretEvent in
webhook_secret.go so it matches the existing DB-reload recovery pattern instead
of propagating an error, and apply the same fix in the Generate/Regenerate paths
referenced by the webhook secret service methods.
Source: Learnings
Contributor
Dependency Validation ResultsDependency name: github.com/gin-gonic/gin Dependency name: github.com/google/uuid Dependency name: github.com/wso2/api-platform/common Dependency name: github.com/wso2/api-platform/gateway/gateway-controller Next Steps
|
Contributor
Dependency Validation ResultsDependency name: github.com/gin-gonic/gin Dependency name: github.com/google/uuid Next Steps
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
revamp gateway-controller