Improve: Event Gateway functionalities #676
Conversation
WalkthroughAdds WebSubApi as the async API kind, replaces servers with upstream/vhosts and per-operation policies, introduces event_gateway router configuration (host/port/listener/timeout), makes topic register/deregister and HTTP calls context-aware, and updates xDS routing, route metadata, and related constants. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Gateway Controller
participant Storage as StoredConfig/Memory
participant Router as Router (event_gateway)
participant WebSubHub as WebSub Hub
participant XDS as xDS Translator
Controller->>Storage: Persist WebSubApi config (displayName, upstream/vhosts)
Controller->>Controller: Build per-operation policies
Controller->>Router: Register topic (ctx with timeout, router host/port)
Router->>WebSubHub: HTTP POST subscribe (request with ctx)
WebSubHub-->>Router: 2xx / error
Router-->>Controller: registration result
Controller->>XDS: TranslateConfigs (use upstream main, vhost)
XDS->>Router: Push xDS snapshot (routes & clusters)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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 (9)
gateway/gateway-controller/pkg/utils/helpers.go (1)
11-29: Update comment/error text to matchWebSubApi(currently saysasync/websub)This will help avoid confusion when debugging config parsing failures.
Proposed change
-// Supports both HTTP REST APIs and async/websub kinds. +// Supports both HTTP REST APIs and WebSubApi kinds. func ExtractNameVersion(cfg api.APIConfiguration) (string, string, error) { @@ if cfg.Kind == api.WebSubApi { d, err := cfg.Spec.AsWebhookAPIData() if err != nil { - return "", "", fmt.Errorf("failed to parse async/websub api config data: %w", err) + return "", "", fmt.Errorf("failed to parse WebSubApi config data: %w", err) } return d.DisplayName, d.Version, nil }gateway/gateway-controller/pkg/storage/memory.go (1)
83-89: Updateexisting.Kindwheneverexisting.Configurationis updated to prevent divergenceWhen configurations are updated via handlers or deployment utilities,
existing.Configurationis replaced butexisting.Kindis not updated. Since topic management decisions depend oncfg.Configuration.Kind(memory.go lines 83, 130, 182) while query methods filter bycfg.Kind(GetAllByKind, GetByKindNameAndVersion, GetByKindAndHandle), a mismatch can occur if an API's type changes. This creates inconsistent state: topics may be managed based on the new type while queries use the old type.Update the following locations to sync
existing.Kind = string(apiConfig.Kind)or equivalent wheneverConfigurationis updated:
gateway/gateway-controller/pkg/api/handlers/handlers.go:603gateway/gateway-controller/pkg/utils/api_deployment.go:407gateway/gateway-controller/pkg/utils/mcp_deployment.go:194Also applies to: 130-136, 182-188
gateway/gateway-controller/pkg/api/handlers/handlers.go (4)
469-477: Misleading error message: still referencesasync/websubafter switching toWebSubApi.
This will confuse operators troubleshooting bad requests.Proposed fix
- log.Warn("Configuration kind mismatch", - zap.String("expected", "RestApi or async/websub"), + log.Warn("Configuration kind mismatch", + zap.String("expected", "RestApi or WebSubApi"), zap.String("actual", cfg.Kind), zap.String("handle", handle))
609-695: Topic lifecycle ops: avoid unbounded goroutines + fix error logging.
Two issues here:
- Spawning one goroutine per topic can overwhelm the process on large updates.
zap.Error(err)at Line 688 logs an unrelated outererr(often nil).Suggested direction (bounded concurrency + correct error details)
- // Check if topic operations failed and return error + // Check if topic operations failed and return error if regErrs > 0 || deregErrs > 0 { - log.Error("Failed to register & deregister topics", zap.Error(err)) + log.Error("Topic lifecycle operations failed", + zap.Int("register_errors", int(regErrs)), + zap.Int("deregister_errors", int(deregErrs))) c.JSON(http.StatusInternalServerError, api.ErrorResponse{ Status: "error", Message: "Topic lifecycle operations failed", }) return }For concurrency, consider
errgroup.Group+SetLimit(n)(or a semaphore) around per-topic calls.
877-928: DeleteAPI still uses hard-coded 10s timeout for WebSub topic deregistration.
This is inconsistent with UpdateAPI/Deploy and undermines the “admin-configurable timeouts” objective.Proposed fix
- ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout( + context.Background(), + time.Duration(s.routerConfig.EventGateway.TimeoutSeconds)*time.Second, + ) defer cancel()
1664-1701: Critical: WebSub policy builder will panic on publish-only channels and ignores publish policies.
The code accessesch.Subscribe.Policieswithout checking ifch.Subscribeis nil. SinceSubscribeis a pointer field in the generatedChannelstruct, channels with onlyPublishoperations (as inwebsubhub.yaml) will have nilSubscribe, causing a runtime panic. Additionally,ch.Publishoperations are never processed—onlySubscribeoperations have policy handling.Minimal safe fix pattern (guard + handle both operations)
for _, ch := range apiData.Channels { var finalPolicies []policyenginev1.PolicyInstance - - if ch.Subscribe.Policies != nil && len(*ch.Subscribe.Policies) > 0 { + // TODO: decide routeKey differentiation between subscribe vs publish if both exist on same path + if ch.Subscribe != nil && ch.Subscribe.Policies != nil && len(*ch.Subscribe.Policies) > 0 { ... } else { ... } routeKey := xds.GenerateRouteName("POST", apiData.Context, apiData.Version, ch.Path, s.routerConfig.GatewayHost) ... }Then add a parallel block for
ch.Publish(or refactor into a helper that accepts the operation object).gateway/gateway-controller/pkg/config/api_validator.go (2)
88-117: Validation messages still refer toasync/websubin the WebSubApi path.
Please update the error strings so users seeWebSubApiconsistently.Proposed fix
- Message: fmt.Sprintf("Invalid spec format for async/websub: %v", err), + Message: fmt.Sprintf("Invalid spec format for WebSubApi: %v", err),
261-281: Validation error fields/messages don’t match the model (DisplayNamevsname).
You validatespec.DisplayNamebut return errors onspec.name, which is confusing for API users and UI clients.Proposed fix
- if spec.DisplayName == "" { + if spec.DisplayName == "" { errors = append(errors, ValidationError{ - Field: "spec.name", - Message: "API name is required", + Field: "spec.displayName", + Message: "API display name is required", }) } else if len(spec.DisplayName) > 100 { errors = append(errors, ValidationError{ - Field: "spec.name", - Message: "API name must be 1-100 characters", + Field: "spec.displayName", + Message: "API display name must be 1-100 characters", }) } else if !v.urlFriendlyNameRegex.MatchString(spec.DisplayName) { errors = append(errors, ValidationError{ - Field: "spec.name", - Message: "API name must be URL-friendly (only letters, numbers, spaces, hyphens, underscores, and dots allowed)", + Field: "spec.displayName", + Message: "API display name must be URL-friendly (only letters, numbers, spaces, hyphens, underscores, and dots allowed)", }) }gateway/gateway-controller/pkg/utils/api_deployment.go (1)
441-521: Retry loop semantics are unclear (<= maxRetriesdoes 6 attempts when maxRetries=5).
Either rename tomaxAttemptsor switch to< maxRetriesand adjust the final error message accordingly.Proposed fix (treat maxRetries as “retries after first attempt”)
- for attempt := 0; attempt <= maxRetries; attempt++ { + for attempt := 0; attempt <= maxRetries; attempt++ { // attempt 0 is the initial try; maxRetries are additional retries ... } - return fmt.Errorf("WebSubHub request failed after %d retries; last status: %d", maxRetries, lastStatus) + return fmt.Errorf("WebSubHub request failed after %d retries; last status: %d", maxRetries, lastStatus)If you want exactly
maxRetriestotal attempts, change the loop toattempt < maxRetries.
🤖 Fix all issues with AI agents
In `@gateway/configs/config.yaml`:
- Around line 53-62: The YAML defaults for event_gateway must match the Go
defaultConfig(): change event_gateway.enabled from false to true and either
remove the timeout_seconds key or set timeout_seconds: 0 so it behaves like the
Go default (omitted → 0); update the event_gateway block in config.yaml to
reflect these values to avoid dev/production mismatch with defaultConfig().
In `@gateway/examples/websubhub.yaml`:
- Around line 2-4: The YAML example uses the incorrect key `metaData`; change
the field name `metaData` to `metadata` (all lowercase) in the WebSubApi object
(e.g., the top-level map containing `kind: WebSubApi` and `name: WebSubHub-API`)
so it matches the OpenAPI/schema expectation and avoids validation/parsing
failures.
In `@gateway/gateway-controller/pkg/utils/api_deployment.go`:
- Around line 199-204: Add validation to ensure EventGateway.TimeoutSeconds is
>= 1 during config load (same place other timeouts are validated) so
context.WithTimeout never receives zero/negative durations; update the config
validation routine that checks
RouteTimeoutInSeconds/MaxRouteTimeoutInSeconds/RouteIdleTimeoutInSeconds to also
validate s.routerConfig.EventGateway.TimeoutSeconds (enforce a minimum of 1 and
return a config error if violated) and ensure any code paths calling
context.WithTimeout (e.g., where RegisterTopicWithHub and UnregisterTopicWithHub
are invoked) can rely on this guarantee.
- Around line 105-125: The EventGateway.WebSubHubURL can be configured without a
scheme, causing fmt.Sprintf("%s:%d", ...) to produce an upstream URL without
http/https and later fail validateUpstreamUrl(); update Config.Validate() to
check EventGateway.WebSubHubURL contains a valid URL scheme (http or https) by
parsing the string (e.g., via url.Parse) and returning an error if Scheme is
empty or not http/https. Ensure the validation message references
EventGateway.WebSubHubURL so callers know which field to fix; this stops
webhookData.Upstream.Main.Url being populated with an invalid value used by
apiConfig.Spec.FromWebhookAPIData and downstream validateUpstreamUrl().
In `@gateway/gateway-controller/pkg/xds/translator.go`:
- Line 392: cfg.GetContext() is being called and its return value discarded
(same orphaned-call bug as in translateAsyncAPIConfig); capture the returned
value (e.g., ctx := cfg.GetContext() or whatever the function's return names
are) and use that context where needed or pass it into the subsequent
translation logic, or remove the call if unused; update the call site in
translator.go (the cfg.GetContext() invocation) to assign and propagate the
result rather than ignoring it.
- Around line 693-694: The Domains value for the
DYNAMIXC_FORWARD_PROXY_VHOST_WEBSUBHUB virtual host is using the full URL
t.routerConfig.EventGateway.WebSubHubURL but must supply only the hostname;
replace the direct use with the parsed hostname (e.g., via
url.Parse(...).Hostname()) so Domains: []string{hostname} is set, matching the
parsing approach used elsewhere (see usages around functions referencing
WebSubHubURL on lines ~287 and ~354) and ensuring you handle parse errors or
empty hostnames consistently.
🧹 Nitpick comments (9)
gateway/gateway-controller/pkg/constants/constants.go (1)
98-99: Rename exported constant to Go style (CamelCase) to avoid API churn later
WEBSUBHUB_INTERNAL_CLUSTER_NAMEis exported but uses all-caps + underscores, which is atypical for Go exported identifiers and makes future cleanup harder.Proposed change
- WEBSUBHUB_INTERNAL_CLUSTER_NAME = "WEBSUBHUB_INTERNAL_CLUSTER" + // WebSubHub internal cluster name used by the event gateway. + WebSubHubInternalClusterName = "WEBSUBHUB_INTERNAL_CLUSTER"gateway/gateway-controller/pkg/storage/memory.go (1)
141-167: Hoist invariant topic components out of the loop; consider sanitizingDisplayNamebefore building topic IDsRight now
name/context/versionare recomputed for every channel, andDisplayNamemay contain characters that make topic IDs unstable (spaces,/, etc.); currently only a leading/is trimmed.Proposed change (hoist invariants; optional sanitization left to you)
func (cs *ConfigStore) updateTopics(cfg *models.StoredConfig) error { asyncData, err := cfg.Configuration.Spec.AsWebhookAPIData() if err != nil { return fmt.Errorf("failed to parse async API data: %w", err) } @@ apiTopicsPerRevision := make(map[string]bool) + name := strings.TrimPrefix(asyncData.DisplayName, "/") + context := strings.TrimPrefix(asyncData.Context, "/") + version := strings.TrimPrefix(asyncData.Version, "/") for _, topic := range asyncData.Channels { - name := strings.TrimPrefix(asyncData.DisplayName, "/") - context := strings.TrimPrefix(asyncData.Context, "/") - version := strings.TrimPrefix(asyncData.Version, "/") path := strings.TrimPrefix(topic.Path, "/") modifiedTopic := fmt.Sprintf("%s_%s_%s_%s", name, context, version, path) cs.TopicManager.Add(cfg.ID, modifiedTopic) apiTopicsPerRevision[modifiedTopic] = true }gateway/gateway-controller/pkg/models/stored_config.go (2)
51-64: WebSubApi composite key now usesDisplayName:Version(consistent with new model).
Looks coherent with the migration (WebhookAPIData). Consider whether returning""on parse error should be surfaced/logged upstream to avoid silent index misses.
66-80:asyncDatavariable name is misleading post-migration.
Pure readability, but renaming towebhookData/webSubDatawould reduce confusion.gateway/gateway-controller/pkg/utils/api_deployment.go (1)
179-266: Good resiliency win: topic ops now honor configurable timeouts + cancellation.
This aligns well with the PR’s stated goal to prevent resource leaks during registration/deregistration.Also consider bounding per-topic goroutines (same concern as in handlers.go) to avoid spikes on large topic sets.
gateway/gateway-controller/api/openapi.yaml (1)
1961-1997: Consider addingrequired: [main]toupstreamin WebhookAPIData.The
upstreamproperty inWebhookAPIDatalacks arequiredconstraint formain, unlike theAPIConfigData.upstreamdefinition (lines 1806-1815) which explicitly requiresmain. This inconsistency could allow invalid configurations whereupstreamis provided without amainreference.Suggested fix
upstream: type: object description: API-level upstream configuration + required: + - main properties: main: $ref: "#/components/schemas/Upstream" sandbox: $ref: "#/components/schemas/Upstream"gateway/gateway-controller/pkg/xds/translator.go (3)
71-72: ConstantWebSubHubInternalClusterNameduplicatesconstants.WEBSUBHUB_INTERNAL_CLUSTER_NAME.This local constant duplicates
constants.WEBSUBHUB_INTERNAL_CLUSTER_NAME(defined ingateway/gateway-controller/pkg/constants/constants.go). The code inconsistently uses the centralized constant at line 298 and 353, but references the local constant at line 618. Consider removing this local constant and using the centralized one consistently.Suggested fix
const ( DynamicForwardProxyClusterName = "dynamic-forward-proxy-cluster" ExternalProcessorGRPCServiceClusterName = "ext-processor-grpc-service" OTELCollectorClusterName = "otel_collector" - WebSubHubInternalClusterName = "WEBSUBHUB_INTERNAL_CLUSTER" )Then update line 618 to use
constants.WEBSUBHUB_INTERNAL_CLUSTER_NAME:ClusterSpecifier: &route.RouteAction_Cluster{Cluster: constants.WEBSUBHUB_INTERNAL_CLUSTER_NAME},
613-618: Virtual host name should use a defined constant.The vhost name
"WEBSUBHUB_INTERNAL_VHOST"is a hardcoded string literal. Consider defining this as a constant for maintainability and consistency with other naming conventions in this file.
980-993: Missingvhostkey in metadata for async routes.The
createRoutePerTopicfunction's metadata map is missing thevhostkey that is present in the regularcreateRoutefunction (line 901). This inconsistency could affect downstream consumers expecting uniform metadata structure.Proposed fix
metaMap := map[string]interface{}{ "route_name": routeName, "api_name": apiName, "api_version": apiVersion, "api_context": context, "path": path, "method": method, + "vhost": vhost, }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
gateway/gateway-controller/pkg/api/generated/generated.gois excluded by!**/generated/**go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
gateway/configs/config.yamlgateway/examples/websubhub.yamlgateway/gateway-controller/api/openapi.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/constants/constants.gogateway/gateway-controller/pkg/models/stored_config.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/utils/api_deployment.gogateway/gateway-controller/pkg/utils/helpers.gogateway/gateway-controller/pkg/xds/translator.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
📚 Learning: 2025-12-19T07:14:39.045Z
Learnt from: nimsara66
Repo: wso2/api-platform PR: 521
File: gateway/gateway-controller/pkg/utils/llm_transformer.go:229-243
Timestamp: 2025-12-19T07:14:39.045Z
Learning: In the gateway LLM configuration flow, validation is performed in pkg/config/llm_validator.go before transformation in pkg/utils/llm_transformer.go, ensuring that required fields like upstream.Auth.Header and upstream.Auth.Value are non-nil before the transformer accesses them.
Applied to files:
gateway/gateway-controller/pkg/constants/constants.gogateway/gateway-controller/pkg/config/api_validator.go
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/gateway-controller/pkg/utils/helpers.gogateway/gateway-controller/pkg/storage/memory.gogateway/examples/websubhub.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/utils/api_deployment.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/models/stored_config.gogateway/gateway-controller/api/openapi.yamlgateway/gateway-controller/pkg/xds/translator.go
📚 Learning: 2026-01-09T05:57:57.216Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/storage/memory.go:484-503
Timestamp: 2026-01-09T05:57:57.216Z
Learning: In gateway/gateway-controller/pkg/storage/memory.go, ensure that when rotating API keys via StoreAPIKey, the API key ID on the incoming object must equal the existing key's ID if a key with the same name already exists (existingKeyID != ""). Do not modify apiKey.ID during update; add an explicit check or assertion to prevent ID mismatches. This guarantees updates don't change identity and helps maintain consistency.
Applied to files:
gateway/gateway-controller/pkg/storage/memory.go
📚 Learning: 2025-11-08T13:06:22.133Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 110
File: platform-api/src/internal/service/api.go:432-476
Timestamp: 2025-11-08T13:06:22.133Z
Learning: In the platform-api Go codebase (file: platform-api/src/internal/service/api.go), the DeployAPIRevision method is designed to automatically create API-gateway associations during deployment if they don't already exist. There is no requirement that associations must pre-exist before deployment; the system checks for existing associations and creates them on-the-fly as needed.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
📚 Learning: 2026-01-07T17:41:01.083Z
Learnt from: VirajSalaka
Repo: wso2/api-platform PR: 642
File: gateway/gateway-controller/pkg/utils/api_deployment.go:570-589
Timestamp: 2026-01-07T17:41:01.083Z
Learning: In gateway/gateway-controller/pkg/utils/api_deployment.go, when enableReplicaSync is true (multi-replica mode), the in-memory store (s.store) is intentionally not updated immediately during DeployAPIConfiguration. Instead, all replicas (including the originating replica) update their in-memory stores through the event-driven path via the EventListener to ensure consistent state synchronization across the cluster. This means immediate reads may not find the config until the EventListener processes the event, which is expected behavior.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
📚 Learning: 2026-01-09T05:58:05.318Z
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.
Applied to files:
gateway/gateway-controller/pkg/models/stored_config.go
📚 Learning: 2026-01-09T06:09:06.281Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 644
File: gateway/gateway-controller/pkg/utils/api_key.go:1380-1388
Timestamp: 2026-01-09T06:09:06.281Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the SetHashingConfig and GetHashingConfig methods are test-only utilities used in api_key_hash_test.go to dynamically switch hashing configurations during sequential tests. They are not called in production code paths where the hashing configuration is set once during APIKeyService initialization via NewAPIKeyService.
Applied to files:
gateway/gateway-controller/pkg/models/stored_config.go
📚 Learning: 2025-12-23T11:46:48.758Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/api/openapi.yaml:2259-2262
Timestamp: 2025-12-23T11:46:48.758Z
Learning: In gateway/gateway-controller/api/openapi.yaml, the APIKey schema should define operations as a string (not an array) containing a stringified representation of allowed operations. Ensure downstream tooling that parses this field is updated to handle a string, and update validators/consumers accordingly to avoid expecting an array.
Applied to files:
gateway/gateway-controller/api/openapi.yaml
🧬 Code graph analysis (6)
gateway/gateway-controller/pkg/utils/helpers.go (1)
gateway/gateway-controller/pkg/api/generated/generated.go (1)
WebSubApi(32-32)
gateway/gateway-controller/pkg/config/config.go (2)
gateway/gateway-controller/pkg/metrics/metric_wrappers.go (1)
Enabled(29-29)gateway/policy-engine/internal/metrics/metric_wrappers.go (1)
Enabled(29-29)
gateway/gateway-controller/pkg/storage/memory.go (1)
gateway/gateway-controller/pkg/api/generated/generated.go (1)
WebSubApi(32-32)
gateway/gateway-controller/pkg/utils/api_deployment.go (1)
gateway/gateway-controller/pkg/api/generated/generated.go (2)
WebSubApi(32-32)Upstream(1215-1222)
gateway/gateway-controller/pkg/config/api_validator.go (2)
gateway/gateway-controller/pkg/api/generated/generated.go (5)
RestApi(31-31)WebSubApi(32-32)ValidationError(1243-1249)WebhookAPIData(1252-1285)Upstream(1215-1222)gateway/gateway-controller/pkg/config/validator.go (1)
ValidationError(24-27)
gateway/gateway-controller/pkg/xds/translator.go (2)
gateway/gateway-controller/pkg/api/generated/generated.go (1)
WebSubApi(32-32)gateway/gateway-controller/pkg/constants/constants.go (1)
WEBSUBHUB_INTERNAL_CLUSTER_NAME(98-98)
🔇 Additional comments (7)
gateway/examples/websubhub.yaml (1)
9-10: Check whethervhosts.main: "*"is actually allowed by validation/translator.
If wildcard vhosts are not supported (or need a specific format), this example will be misleading.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
735-741: Good: xDS snapshot update now uses configured timeout.
Aligning snapshot update timeout with EventGateway config is a nice resiliency improvement.gateway/gateway-controller/pkg/utils/api_deployment.go (1)
97-125: No issues found. All kind checks across validators, handlers, helpers, and the API deployment logic consistently use the proper enum constants (api.RestApi,api.WebSubApi) rather than hardcoded strings. The "async/websub" references are limited to error messages and comments, which pose no divergence risk.gateway/gateway-controller/api/openapi.yaml (2)
1752-1753: LGTM - WebSubApi kind addition aligns with retrieved learnings.The addition of
WebSubApito thekindenum is consistent with the migration fromasync/websubnoted in the AI summary and aligns with the retrieved learning that only async/websub API kind is currently supported. This consolidates the async API types under a cleaner naming convention.
2112-2130: LGTM - Per-operation policy support in Channel.The addition of
policiesarrays withinsubscribeandpublishoperations aligns with the PR objective to add policy support at subscriber and publisher levels. The schema correctly references the existingPolicyschema.gateway/gateway-controller/pkg/xds/translator.go (2)
166-184: LGTM - WebSubApi kind check correctly updated.The condition now properly checks for
api.WebSubApiwhich aligns with the new kind enum defined in the OpenAPI spec and the generated code (gateway/gateway-controller/pkg/api/generated/generated.go:31).
955-1002: LGTM -createRoutePerTopicsignature and metadata enhancements.The updated function signature now includes
vhostfor proper route grouping. The addition of dynamic metadata (route_name,api_name,api_version, etc.) enables downstream correlation for policies, logging, and tracing. The implementation correctly usesGenerateRouteNamefor consistent route naming.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/examples/websubhub.yaml`:
- Around line 12-41: YAML indentation for the channels is inconsistent: align
the subscribe->policies block for both channel entries so `policies` is indented
the same number of spaces under `subscribe` for the path: /pull-requests and
path: /issues entries, and remove the extra space after the dash in `- path` so
it becomes `- path`; ensure `requestHeaders` and `responseHeaders` remain
aligned under `params` with the same indentation in both entries.
♻️ Duplicate comments (1)
gateway/examples/websubhub.yaml (1)
3-4: ChangemetaDatatometadata(lowercase).The field name should be
metadata(all lowercase) to match the OpenAPI schema and generated API types. UsingmetaDatawill cause validation/parsing failures.-metaData: +metadata: name: WebSubHub-API
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gateway/examples/websubhub.yamlgateway/gateway-controller/pkg/utils/websub_topic_registration_test.go
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/gateway-controller/pkg/utils/websub_topic_registration_test.gogateway/examples/websubhub.yaml
📚 Learning: 2026-01-07T17:41:01.083Z
Learnt from: VirajSalaka
Repo: wso2/api-platform PR: 642
File: gateway/gateway-controller/pkg/utils/api_deployment.go:570-589
Timestamp: 2026-01-07T17:41:01.083Z
Learning: In gateway/gateway-controller/pkg/utils/api_deployment.go, when enableReplicaSync is true (multi-replica mode), the in-memory store (s.store) is intentionally not updated immediately during DeployAPIConfiguration. Instead, all replicas (including the originating replica) update their in-memory stores through the event-driven path via the EventListener to ensure consistent state synchronization across the cluster. This means immediate reads may not find the config until the EventListener processes the event, which is expected behavior.
Applied to files:
gateway/gateway-controller/pkg/utils/websub_topic_registration_test.go
📚 Learning: 2025-11-08T13:06:22.133Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 110
File: platform-api/src/internal/service/api.go:432-476
Timestamp: 2025-11-08T13:06:22.133Z
Learning: In the platform-api Go codebase (file: platform-api/src/internal/service/api.go), the DeployAPIRevision method is designed to automatically create API-gateway associations during deployment if they don't already exist. There is no requirement that associations must pre-exist before deployment; the system checks for existing associations and creates them on-the-fly as needed.
Applied to files:
gateway/gateway-controller/pkg/utils/websub_topic_registration_test.go
📚 Learning: 2026-01-13T15:42:24.281Z
Learnt from: O-sura
Repo: wso2/api-platform PR: 665
File: gateway/policy-engine/internal/analytics/analytics.go:277-277
Timestamp: 2026-01-13T15:42:24.281Z
Learning: In gateway/policy-engine/internal/analytics/analytics.go, when processing AI-related metadata for LLM providers, the aiMetadata.VendorVersion field should be populated from APIVersionKey (the API's version) rather than AIProviderAPIVersionMetadataKey (the AI provider's API version). This is intentional based on how LLM providers are handled in the system.
Applied to files:
gateway/examples/websubhub.yaml
📚 Learning: 2026-01-09T11:48:26.307Z
Learnt from: piyumaldk
Repo: wso2/api-platform PR: 648
File: cli/src/internal/gateway/resources.go:28-31
Timestamp: 2026-01-09T11:48:26.307Z
Learning: The standard kind for MCP resources in gateway manifests is "Mcp" (PascalCase), not "mcp" (lowercase). This aligns with the ResourceKindMCP constant in cli/src/internal/gateway/resources.go and follows the same PascalCase pattern as ResourceKindRestAPI ("RestApi").
Applied to files:
gateway/examples/websubhub.yaml
🧬 Code graph analysis (1)
gateway/gateway-controller/pkg/utils/websub_topic_registration_test.go (1)
gateway/gateway-controller/pkg/storage/topics.go (1)
TopicManager(26-29)
🔇 Additional comments (5)
gateway/gateway-controller/pkg/utils/websub_topic_registration_test.go (4)
24-37: LGTM! WebSubApi schema migration looks correct.The inline YAML configuration has been properly updated to use the new
WebSubApikind withapiVersion: gateway.api-platform.wso2.com/v1alpha1, and thevhostsblock replaces the legacy servers definition. The topic assertions at lines 60-61 correctly use the new naming conventiontestapi_test_v1_topic1.
64-146: LGTM! Revision deployment test correctly validates topic deregistration.The test properly verifies that when a channel is removed in a subsequent revision, the corresponding topic is deregistered. The assertions at lines 144-145 correctly check that
topic1persists whiletopic2is removed.
148-248: LGTM! Concurrent topic registration test is well-structured.The test correctly validates thread-safe topic registration for multiple API configurations using goroutines and
sync.WaitGroup. The topic naming conventions in assertions (lines 242-247) align with the updated YAML configurations.
250-302: LGTM! Config deletion test properly validates topic cleanup.The test correctly verifies the lifecycle behavior where topics are deregistered when the associated configuration is deleted from the store.
gateway/examples/websubhub.yaml (1)
9-10: LGTM! vhosts configuration is correct.The
vhostsblock withmain: "*"correctly replaces the legacy servers configuration, aligning with the new WebSubApi schema.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
gateway/gateway-controller/pkg/config/api_validator.go (2)
266-281: Fix incorrect field name in error messages.The error messages reference
spec.namebut the actual field being validated isDisplayName. For consistency withvalidateRestData(which correctly usesspec.displayName), update the field names.Proposed fix
// Validate name if spec.DisplayName == "" { errors = append(errors, ValidationError{ - Field: "spec.name", + Field: "spec.displayName", Message: "API name is required", }) } else if len(spec.DisplayName) > 100 { errors = append(errors, ValidationError{ - Field: "spec.name", + Field: "spec.displayName", Message: "API name must be 1-100 characters", }) } else if !v.urlFriendlyNameRegex.MatchString(spec.DisplayName) { errors = append(errors, ValidationError{ - Field: "spec.name", + Field: "spec.displayName", Message: "API name must be URL-friendly (only letters, numbers, spaces, hyphens, underscores, and dots allowed)", }) }
336-350: Remove duplicate validation and fix misleading error message.The same validation (
validatePathParametersForAsyncAPIs) is called twice consecutively, which will produce two error messages for the same issue. Additionally, the second error message says "unbalanced braces" but the function actually rejects ANY braces (not just unbalanced ones).Proposed fix
// Validate path parameters have balanced braces if !v.validatePathParametersForAsyncAPIs(op.Path) { errors = append(errors, ValidationError{ Field: fmt.Sprintf("spec.channels[%d].path", i), - Message: "Operation path has braces which is not allowed", - }) - } - - if !v.validatePathParametersForAsyncAPIs(op.Path) { - errors = append(errors, ValidationError{ - Field: fmt.Sprintf("spec.channels[%d].path", i), - Message: "Channel path has unbalanced braces in parameters", + Message: "Channel path cannot contain path parameters (braces not allowed)", }) }gateway/gateway-controller/pkg/api/handlers/handlers.go (3)
949-956: Misleading error message for delete operation.The error message on line 950 says "Failed to register & deregister topics", but
DeleteAPIonly performs deregistration (not registration). This appears to be copy-pasted fromUpdateAPI. Consider updating the message for clarity.🔧 Suggested fix
- log.Error("Failed to register & deregister topics", zap.Error(err)) + log.Error("Failed to deregister topics", zap.Error(err))
716-723: Incorrect error variable in log statement.On line 717,
zap.Error(err)referenceserrfrom an earlier operation (the validator or parser), not from the topic registration/deregistration operations. The actual errors are tracked via atomic counters (regErrs,deregErrs), so the logged error is misleading.🔧 Suggested fix
- log.Error("Failed to register & deregister topics", zap.Error(err)) + log.Error("Failed to complete topic lifecycle operations", + zap.Int32("register_errors", regErrs), + zap.Int32("deregister_errors", deregErrs))
1693-1742: Fix route key generation for WebSubApi: use VHosts.Main.Default instead of GatewayHost.Line 1732 uses
s.routerConfig.GatewayHost, but this doesn't match the host configuration used by the xDS translator'stranslateAsyncAPIConfig(which usesVHosts.Main.Default). This causes route keys in the policy engine to not match xDS route names, preventing policies from being applied to WebSubApi routes. Additionally, the WebSubApi case should handle API-specific vhosts fromapiData.Vhostslike the RestApi case does (lines 1785-1792).
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 922-924: The delete-path uses a hardcoded 10s timeout for the
UnregisterTopicWithHub call, causing inconsistent behavior versus UpdateAPI
which uses s.routerConfig.EventGateway.TimeoutSeconds; change the context
creation in the delete handler to use
time.Duration(s.routerConfig.EventGateway.TimeoutSeconds)*time.Second (or the
existing helper used by UpdateAPI) when calling
s.deploymentService.UnregisterTopicWithHub so both UpdateAPI and the delete flow
share the same configurable timeout value.
In `@gateway/gateway-controller/pkg/config/api_validator.go`:
- Around line 261-262: Update the function docstring for validateAsyncData to
accurately describe its purpose (it validates async/WebSub API data, not
http/rest), and either remove the unused isWebSub parameter from the function
signature and all callers (if it's not needed) or use it with a short
explanatory comment inside validateAsyncData to document intended behavior; make
corresponding updates to any references to validateAsyncData and add a brief
comment on the parameter if you retain it.
In `@gateway/gateway-controller/pkg/config/validator_test.go`:
- Around line 256-274: The test TestValidateWebSubURLConfig_WithoutSchema has a
copy-paste comment mismatch and lacks an assertion verifying the specific
validation message; update the comment to accurately describe the test case and
add an assertion checking the returned error message from
validateEventGatewayConfig contains the expected "http or https scheme" text
(e.g., assert.Error(t, err) followed by assert.Contains(t, err.Error(), "http or
https scheme")) so the test verifies both failure and the specific error
content.
- Around line 236-254: The test comment in TestValidateEventGWConfig_Enabled is
incorrect (mentions auth methods); update the comment to accurately describe
this test as verifying that EventGateway configuration validation passes when
EventGateway is enabled and required fields (WebSubHubURL, WebSubHubPort,
WebSubHubListenerPort, TimeoutSeconds) are present, i.e., change the comment
above the TestValidateEventGWConfig_Enabled function to reflect EventGateway
validation rather than auth methods.
♻️ Duplicate comments (2)
gateway/examples/websubhub.yaml (1)
12-41: Fix inconsistent YAML indentation in channels block.The indentation issues flagged in the previous review are still present:
- Double space after hyphen on lines 12 and 27 (
- path→- path)policiesindent mismatch: line 14 uses 8-space indent, line 29 uses 9-space indentThis inconsistency may cause YAML parsing issues or confusion for users referencing this example.
Suggested fix for consistent indentation
channels: - - path: /pull-requests - subscribe: - policies: + - path: /pull-requests + subscribe: + policies: - name: ModifyHeaders version: v1.0.0 enabled: true params: requestHeaders: - action: SET name: operation-level-req-header value: hello responseHeaders: - action: SET name: operation-level-res-header value: world - - path: /issues - subscribe: - policies: + - path: /issues + subscribe: + policies: - name: ModifyHeaders version: v1.0.0 enabled: true params: requestHeaders: - action: SET name: operation-level-req-header value: hello responseHeaders: - action: SET name: operation-level-res-header value: worldgateway/gateway-controller/pkg/config/config.go (1)
396-403: MissingTimeoutSecondsdefault causes validation failure.The
defaultConfig()setsEnabled: truebut does not provide a default forTimeoutSeconds. SincevalidateEventGatewayConfig()requiresTimeoutSeconds > 0, the default configuration will fail validation when the event gateway is enabled.Proposed fix
Router: RouterConfig{ EventGateway: EventGatewayConfig{ Enabled: true, WebSubHubURL: "http://host.docker.internal", WebSubHubPort: 9098, RouterHost: "localhost", WebSubHubListenerPort: 8083, + TimeoutSeconds: 10, },
🧹 Nitpick comments (5)
gateway/gateway-controller/pkg/config/config.go (1)
686-705: Consider validatingRouterHostwhen event gateway is enabled.The
RouterHostfield is used for WebSub topic registration but there's no validation to ensure it's non-empty when the event gateway is enabled. An emptyRouterHostcould lead to malformed URLs at runtime.Proposed addition
func (c *Config) validateEventGatewayConfig() error { + if strings.TrimSpace(c.GatewayController.Router.EventGateway.RouterHost) == "" { + return fmt.Errorf("router.event_gateway.router_host is required when event gateway is enabled") + } if c.GatewayController.Router.EventGateway.WebSubHubPort < 1 || c.GatewayController.Router.EventGateway.WebSubHubPort > 65535 {gateway/gateway-controller/pkg/config/api_validator.go (1)
299-305: Fix misleading comment or implement conditional logic.The comment says "Validate upstream if not WebSub" but the validation runs unconditionally. Either:
- Update the comment to reflect actual behavior, or
- Implement the conditional logic if WebSub shouldn't require upstream validation
Option 1: Fix comment
- // Validate upstream if not WebSub + // Validate upstream (optional for WebSubApi) if spec.Upstream.Main != nil {gateway/gateway-controller/pkg/api/handlers/handlers.go (3)
973-980: Inconsistent timeout for xDS snapshot update.The xDS snapshot update timeout here is hardcoded to
10*time.Second, but inUpdateAPI(line 764), it uses the configurablerouterConfig.EventGateway.TimeoutSeconds. Consider using the same configurable timeout for consistency across operations.♻️ Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(s.routerConfig.EventGateway.TimeoutSeconds)*time.Second)
764-770: Consider using a dedicated xDS timeout configuration.The xDS snapshot update timeout uses
EventGateway.TimeoutSeconds, which semantically may not be the most appropriate since xDS updates apply to all API kinds, not just event gateway. Consider whether a dedicated timeout configuration for xDS operations would be clearer.
108-108: Note: httpClient timeout is separate from configurable EventGateway timeout.The
httpClientis initialized with a fixed 10-second timeout, which is independent ofrouterConfig.EventGateway.TimeoutSeconds. While both timeouts work together (context deadline + HTTP client timeout), consider whether the HTTP client timeout should also be configurable for consistency, especially if the EventGateway timeout is set to a value different from 10 seconds.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gateway/configs/config.yamlgateway/examples/websubhub.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/config/validator_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/configs/config.yaml
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
📚 Learning: 2025-12-19T07:14:39.045Z
Learnt from: nimsara66
Repo: wso2/api-platform PR: 521
File: gateway/gateway-controller/pkg/utils/llm_transformer.go:229-243
Timestamp: 2025-12-19T07:14:39.045Z
Learning: In the gateway LLM configuration flow, validation is performed in pkg/config/llm_validator.go before transformation in pkg/utils/llm_transformer.go, ensuring that required fields like upstream.Auth.Header and upstream.Auth.Value are non-nil before the transformer accesses them.
Applied to files:
gateway/gateway-controller/pkg/config/config.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/validator_test.go
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/gateway-controller/pkg/config/api_validator.gogateway/examples/websubhub.yamlgateway/gateway-controller/pkg/api/handlers/handlers.go
📚 Learning: 2026-01-13T15:42:24.281Z
Learnt from: O-sura
Repo: wso2/api-platform PR: 665
File: gateway/policy-engine/internal/analytics/analytics.go:277-277
Timestamp: 2026-01-13T15:42:24.281Z
Learning: In gateway/policy-engine/internal/analytics/analytics.go, when processing AI-related metadata for LLM providers, the aiMetadata.VendorVersion field should be populated from APIVersionKey (the API's version) rather than AIProviderAPIVersionMetadataKey (the AI provider's API version). This is intentional based on how LLM providers are handled in the system.
Applied to files:
gateway/examples/websubhub.yaml
📚 Learning: 2026-01-09T11:48:26.307Z
Learnt from: piyumaldk
Repo: wso2/api-platform PR: 648
File: cli/src/internal/gateway/resources.go:28-31
Timestamp: 2026-01-09T11:48:26.307Z
Learning: The standard kind for MCP resources in gateway manifests is "Mcp" (PascalCase), not "mcp" (lowercase). This aligns with the ResourceKindMCP constant in cli/src/internal/gateway/resources.go and follows the same PascalCase pattern as ResourceKindRestAPI ("RestApi").
Applied to files:
gateway/examples/websubhub.yaml
📚 Learning: 2025-11-08T13:06:22.133Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 110
File: platform-api/src/internal/service/api.go:432-476
Timestamp: 2025-11-08T13:06:22.133Z
Learning: In the platform-api Go codebase (file: platform-api/src/internal/service/api.go), the DeployAPIRevision method is designed to automatically create API-gateway associations during deployment if they don't already exist. There is no requirement that associations must pre-exist before deployment; the system checks for existing associations and creates them on-the-fly as needed.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
📚 Learning: 2026-01-07T17:41:01.083Z
Learnt from: VirajSalaka
Repo: wso2/api-platform PR: 642
File: gateway/gateway-controller/pkg/utils/api_deployment.go:570-589
Timestamp: 2026-01-07T17:41:01.083Z
Learning: In gateway/gateway-controller/pkg/utils/api_deployment.go, when enableReplicaSync is true (multi-replica mode), the in-memory store (s.store) is intentionally not updated immediately during DeployAPIConfiguration. Instead, all replicas (including the originating replica) update their in-memory stores through the event-driven path via the EventListener to ensure consistent state synchronization across the cluster. This means immediate reads may not find the config until the EventListener processes the event, which is expected behavior.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
🧬 Code graph analysis (2)
gateway/gateway-controller/pkg/config/api_validator.go (2)
gateway/gateway-controller/pkg/api/generated/generated.go (5)
RestApi(31-31)WebSubApi(32-32)ValidationError(1243-1249)WebhookAPIData(1252-1285)Upstream(1215-1222)gateway/gateway-controller/pkg/config/validator.go (1)
ValidationError(24-27)
gateway/gateway-controller/pkg/config/validator_test.go (1)
gateway/gateway-controller/pkg/config/config.go (4)
Config(35-41)GatewayController(52-63)RouterConfig(182-195)EventGatewayConfig(198-205)
🔇 Additional comments (9)
gateway/examples/websubhub.yaml (2)
2-4: LGTM on the kind and metadata changes.The migration from
async/websubtoWebSubApialigns with the PR objectives and the gateway-controller's current API schema. The metadata field naming is now correct.
9-10: LGTM on the vhosts structure.The new
vhostsblock replacing the legacyserversdefinition aligns with the broader PR changes introducing upstream/vhosts concepts in the gateway configuration.gateway/gateway-controller/pkg/config/config.go (1)
642-647: LGTM!The conditional validation is correctly wired to only run
validateEventGatewayConfig()when the event gateway is enabled, following the same pattern used for other optional components like the policy engine.gateway/gateway-controller/pkg/config/api_validator.go (3)
88-93: LGTM!The kind validation correctly updated to support the new
WebSubApikind, and the error message is clear and accurate.
129-164: LGTM!The
validateUpstreamhelper is well-structured with clear validation logic:
- Rejects invalid union case (both ref and url set)
- Requires at least one of ref or url
- Delegates to specific validators
166-209: LGTM!The
validateUpstreamUrlhelper provides thorough URL validation including scheme and host checks. ThevalidateUpstreamRefplaceholder with TODO is acceptable for the current scope.gateway/gateway-controller/pkg/api/handlers/handlers.go (3)
469-479: LGTM!The kind check correctly includes
WebSubApialongsideRestApifor API retrieval.
659-661: Good: Configurable timeout for topic registration.The timeout is now driven by
routerConfig.EventGateway.TimeoutSeconds, which aligns with the PR objective of improving resiliency through configurable timeouts.
568-572: No changes needed - the URL format is correct.The
WebSubHubURLconfiguration is validated inconfig.go(lines 694-700) to require a valid http or https scheme. When concatenated with the port on line 569 usingfmt.Sprintf("%s:%d", ...), the resulting URL will always include a scheme (e.g.,http://example.com:9098). Downstream validators that parse the URL withurl.Parse()(as seen inapi_validator.go,mcp_validator.go, andllm_validator.go) expect and can handle this format.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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 (3)
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
972-980: Inconsistent timeout: hardcoded vs. configurable.This xDS snapshot update in
DeleteAPIuses a hardcoded10*time.Second, but the equivalent code inUpdateAPI(line 764) uses the configurables.routerConfig.EventGateway.TimeoutSeconds. This inconsistency could lead to different behavior between update and delete operations.Suggested fix for consistency
// Update xDS snapshot asynchronously go func() { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(s.routerConfig.EventGateway.TimeoutSeconds)*time.Second) defer cancel() if err := s.snapshotManager.UpdateSnapshot(ctx, correlationID); err != nil {gateway/gateway-controller/pkg/config/api_validator.go (2)
266-281: Inconsistent error field names: code usesDisplayNamebut errors reportspec.name.The validation checks
spec.DisplayNamebut the error fields all reference"spec.name". This is inconsistent withvalidateRestData(lines 216-231) which correctly uses"spec.displayName". This will cause confusion when users try to fix validation errors.Proposed fix for consistency
// Validate name if spec.DisplayName == "" { errors = append(errors, ValidationError{ - Field: "spec.name", - Message: "API name is required", + Field: "spec.displayName", + Message: "API display name is required", }) } else if len(spec.DisplayName) > 100 { errors = append(errors, ValidationError{ - Field: "spec.name", - Message: "API name must be 1-100 characters", + Field: "spec.displayName", + Message: "API display name must be 1-100 characters", }) } else if !v.urlFriendlyNameRegex.MatchString(spec.DisplayName) { errors = append(errors, ValidationError{ - Field: "spec.name", - Message: "API name must be URL-friendly (only letters, numbers, spaces, hyphens, underscores, and dots allowed)", + Field: "spec.displayName", + Message: "API display name must be URL-friendly (only letters, numbers, spaces, hyphens, underscores, and dots allowed)", }) }
336-350: Duplicate validation check with conflicting error messages.The same condition
!v.validatePathParametersForAsyncAPIs(op.Path)is checked twice with different error messages. The first message ("braces which is not allowed") is correct, but then it's duplicated with a contradictory message about "unbalanced braces". SincevalidatePathParametersForAsyncAPIsrejects any braces entirely, only the first check is needed.Proposed fix to remove duplicate
// Validate path parameters have balanced braces if !v.validatePathParametersForAsyncAPIs(op.Path) { errors = append(errors, ValidationError{ Field: fmt.Sprintf("spec.channels[%d].path", i), - Message: "Operation path has braces which is not allowed", - }) - } - - if !v.validatePathParametersForAsyncAPIs(op.Path) { - errors = append(errors, ValidationError{ - Field: fmt.Sprintf("spec.channels[%d].path", i), - Message: "Channel path has unbalanced braces in parameters", + Message: "Channel path cannot contain path parameters (braces are not allowed)", }) }
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 1700-1712: The code assumes ch.Subscribe is non-nil when checking
ch.Subscribe.Policies; add a nil check for ch.Subscribe before accessing its
Policies (e.g., guard with "if ch.Subscribe != nil && ch.Subscribe.Policies !=
nil && len(*ch.Subscribe.Policies) > 0") so you only iterate and call
convertAPIPolicy when Subscribe exists; ensure finalPolicies and addedNames are
only created inside that guarded block to avoid panics.
♻️ Duplicate comments (1)
gateway/gateway-controller/pkg/config/api_validator.go (1)
261-262: Fix incorrect function comment.The comment says "validates the data section of the configuration for http/rest kind" but this function validates async/WebSub APIs, not REST APIs.
Proposed fix
-// validateAsyncData validates the data section of the configuration for http/rest kind +// validateAsyncData validates the data section of the configuration for async/WebSub kind func (v *APIValidator) validateAsyncData(spec *api.WebhookAPIData) []ValidationError {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
gateway/examples/websubhub.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/validator_test.go
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/examples/websubhub.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/config/api_validator.go
📚 Learning: 2026-01-13T15:42:24.281Z
Learnt from: O-sura
Repo: wso2/api-platform PR: 665
File: gateway/policy-engine/internal/analytics/analytics.go:277-277
Timestamp: 2026-01-13T15:42:24.281Z
Learning: In gateway/policy-engine/internal/analytics/analytics.go, when processing AI-related metadata for LLM providers, the aiMetadata.VendorVersion field should be populated from APIVersionKey (the API's version) rather than AIProviderAPIVersionMetadataKey (the AI provider's API version). This is intentional based on how LLM providers are handled in the system.
Applied to files:
gateway/examples/websubhub.yaml
📚 Learning: 2026-01-09T11:48:26.307Z
Learnt from: piyumaldk
Repo: wso2/api-platform PR: 648
File: cli/src/internal/gateway/resources.go:28-31
Timestamp: 2026-01-09T11:48:26.307Z
Learning: The standard kind for MCP resources in gateway manifests is "Mcp" (PascalCase), not "mcp" (lowercase). This aligns with the ResourceKindMCP constant in cli/src/internal/gateway/resources.go and follows the same PascalCase pattern as ResourceKindRestAPI ("RestApi").
Applied to files:
gateway/examples/websubhub.yaml
📚 Learning: 2025-11-08T13:06:22.133Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 110
File: platform-api/src/internal/service/api.go:432-476
Timestamp: 2025-11-08T13:06:22.133Z
Learning: In the platform-api Go codebase (file: platform-api/src/internal/service/api.go), the DeployAPIRevision method is designed to automatically create API-gateway associations during deployment if they don't already exist. There is no requirement that associations must pre-exist before deployment; the system checks for existing associations and creates them on-the-fly as needed.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
📚 Learning: 2026-01-07T17:41:01.083Z
Learnt from: VirajSalaka
Repo: wso2/api-platform PR: 642
File: gateway/gateway-controller/pkg/utils/api_deployment.go:570-589
Timestamp: 2026-01-07T17:41:01.083Z
Learning: In gateway/gateway-controller/pkg/utils/api_deployment.go, when enableReplicaSync is true (multi-replica mode), the in-memory store (s.store) is intentionally not updated immediately during DeployAPIConfiguration. Instead, all replicas (including the originating replica) update their in-memory stores through the event-driven path via the EventListener to ensure consistent state synchronization across the cluster. This means immediate reads may not find the config until the EventListener processes the event, which is expected behavior.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
📚 Learning: 2025-12-19T07:14:39.045Z
Learnt from: nimsara66
Repo: wso2/api-platform PR: 521
File: gateway/gateway-controller/pkg/utils/llm_transformer.go:229-243
Timestamp: 2025-12-19T07:14:39.045Z
Learning: In the gateway LLM configuration flow, validation is performed in pkg/config/llm_validator.go before transformation in pkg/utils/llm_transformer.go, ensuring that required fields like upstream.Auth.Header and upstream.Auth.Value are non-nil before the transformer accesses them.
Applied to files:
gateway/gateway-controller/pkg/config/validator_test.gogateway/gateway-controller/pkg/config/api_validator.go
🧬 Code graph analysis (3)
gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
gateway/gateway-controller/pkg/api/generated/generated.go (3)
RestApi(31-31)WebSubApi(32-32)Upstream(1215-1222)platform-api/src/internal/dto/gateway_internal.go (1)
Upstream(52-54)
gateway/gateway-controller/pkg/config/validator_test.go (1)
gateway/gateway-controller/pkg/config/config.go (3)
Config(35-41)RouterConfig(182-195)EventGatewayConfig(198-205)
gateway/gateway-controller/pkg/config/api_validator.go (1)
gateway/gateway-controller/pkg/api/generated/generated.go (5)
RestApi(31-31)WebSubApi(32-32)ValidationError(1243-1249)WebhookAPIData(1252-1285)Upstream(1215-1222)
🔇 Additional comments (6)
gateway/examples/websubhub.yaml (1)
1-41: LGTM!The example file correctly demonstrates the new WebSubApi structure with vhosts and per-channel subscribe policies. The indentation issues from previous reviews have been addressed.
gateway/gateway-controller/pkg/config/validator_test.go (1)
236-275: LGTM!The test comments have been corrected and the error message assertion has been added as requested in previous reviews. Both tests provide appropriate coverage for EventGateway validation.
gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
469-479: LGTM!The kind validation correctly allows both
RestApiandWebSubApitypes, aligning with the PR's goal of supporting WebSub API functionality.
555-582: LGTM on WebSubApi upstream defaulting.The logic to create a default upstream when
Upstream.Mainis nil for WebSubApi configurations is reasonable. It ensures downstream logic can safely rely on the field being present.gateway/gateway-controller/pkg/config/api_validator.go (2)
88-93: LGTM!The kind validation correctly supports both
RestApiandWebSubApi, with a clear error message.
299-305: LGTM on upstream validation.The conditional upstream validation correctly handles optional main and sandbox upstreams for WebSub APIs.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| for _, ch := range apiData.Channels { | ||
| var finalPolicies []policyenginev1.PolicyInstance | ||
|
|
||
| if ch.Policies != nil && len(*ch.Policies) > 0 { | ||
| if ch.Subscribe.Policies != nil && len(*ch.Subscribe.Policies) > 0 { | ||
| // Operation has policies: use operation policy order as authoritative | ||
| // This allows operations to reorder, override, or extend API-level policies | ||
| finalPolicies = make([]policyenginev1.PolicyInstance, 0, len(*ch.Policies)) | ||
| finalPolicies = make([]policyenginev1.PolicyInstance, 0, len(*ch.Subscribe.Policies)) | ||
| addedNames := make(map[string]struct{}) | ||
|
|
||
| for _, opPolicy := range *ch.Policies { | ||
| for _, opPolicy := range *ch.Subscribe.Policies { | ||
| finalPolicies = append(finalPolicies, convertAPIPolicy(opPolicy)) | ||
| addedNames[opPolicy.Name] = struct{}{} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Channel struct definition
fd -t f "\.go$" gateway/gateway-controller | head -20 | xargs grep -l "type Channel struct" | head -5Repository: wso2/api-platform
Length of output: 118
🏁 Script executed:
# Search for Channel struct more broadly
rg "type Channel struct" -A 20 gateway/gateway-controller/Repository: wso2/api-platform
Length of output: 2238
🏁 Script executed:
# Also check the handlers.go file context around the disputed lines
rg -B 10 -A 20 "for _, ch := range apiData.Channels" gateway/gateway-controller/pkg/api/handlers/handlers.goRepository: wso2/api-platform
Length of output: 1284
🏁 Script executed:
# Get the full Channel struct definition focusing on Subscribe field
rg "type Channel struct" -A 100 gateway/gateway-controller/pkg/api/generated/generated.go | grep -A 5 "Subscribe"Repository: wso2/api-platform
Length of output: 460
🏁 Script executed:
# More specific search for Subscribe field in Channel struct
rg "Subscribe\s+\*?struct" gateway/gateway-controller/pkg/api/generated/generated.goRepository: wso2/api-platform
Length of output: 81
🏁 Script executed:
# Get a larger context around Channel struct
sed -n '/^type Channel struct/,/^}/p' gateway/gateway-controller/pkg/api/generated/generated.go | head -80Repository: wso2/api-platform
Length of output: 1941
Add nil check for ch.Subscribe before accessing its fields.
Line 1703 accesses ch.Subscribe.Policies without first verifying ch.Subscribe is not nil. Since Subscribe is an optional pointer field (indicated by the omitempty JSON tag), this will cause a panic if a channel is configured without a subscribe operation.
Suggested fix
for _, ch := range apiData.Channels {
var finalPolicies []policyenginev1.PolicyInstance
- if ch.Subscribe.Policies != nil && len(*ch.Subscribe.Policies) > 0 {
+ if ch.Subscribe != nil && ch.Subscribe.Policies != nil && len(*ch.Subscribe.Policies) > 0 {
// Operation has policies: use operation policy order as authoritative
// This allows operations to reorder, override, or extend API-level policies
finalPolicies = make([]policyenginev1.PolicyInstance, 0, len(*ch.Subscribe.Policies))🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go` around lines 1700 -
1712, The code assumes ch.Subscribe is non-nil when checking
ch.Subscribe.Policies; add a nil check for ch.Subscribe before accessing its
Policies (e.g., guard with "if ch.Subscribe != nil && ch.Subscribe.Policies !=
nil && len(*ch.Subscribe.Policies) > 0") so you only iterate and call
convertAPIPolicy when Subscribe exists; ensure finalPolicies and addedNames are
only created inside that guarded block to avoid panics.
1 participant
Purpose
Admins should be able to configure event gateway, timeouts etc.
Goals
Related Issues
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.