Improve: Event Gateway functionalities #676
Conversation
WalkthroughAdds WebSubApi and Event Gateway configuration; replaces channel path with name+method (SUB); makes topic register/unregister context-aware using configurable router host/ports and timeouts; introduces WebSub hub HTTPS-aware listeners, new constants, X‑Hub signature and channel persistence, validators, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Gateway Controller
participant Storage as StoredConfig/DB
participant Registrar as APIDeploymentService
participant WebSubHub as WebSub Hub
participant Translator as xDS Translator
participant Router as Runtime Router
Controller->>Storage: Persist WebSubApi (displayName, vhosts, channels)
Controller->>Controller: Build channel-level topics & policies
Controller->>Registrar: RegisterTopicWithHub(ctx, topic, routerHost, port)
Registrar->>WebSubHub: HTTP POST /hub (with ctx)
WebSubHub-->>Registrar: 2xx / error
Registrar-->>Controller: registration result
Controller->>Translator: TranslateConfigs(all stored configs)
Translator->>Router: Push xDS snapshot (listeners/routes/clusters)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
470-474: Update log message to reflect WebSubApi naming.
The warning still mentions “async/websub”, which is now renamed.🔧 Suggested fix
- zap.String("expected", "RestApi or async/websub"), + zap.String("expected", "RestApi or WebSubApi"),gateway/gateway-controller/pkg/config/api_validator.go (2)
266-280: Fix inconsistent error field names.The validation checks
spec.DisplayNamebut the error messages reportField: "spec.name". This should be"spec.displayName"to match the actual field being validated and align withvalidateRestData.Proposed fix
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-349: Remove duplicate path parameter validation.The same validation condition
!v.validatePathParametersForAsyncAPIs(op.Path)is checked twice with different error messages. This appears to be a copy-paste error and will produce two contradictory error messages for the same path.Proposed fix - remove the duplicate check
// 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", - }) - }gateway/gateway-controller/api/openapi.yaml (1)
1961-2023: Schema pattern mismatch: wildcard asterisk not allowed by vhosts regexThe
vhosts.mainpattern'^[a-zA-Z0-9\.\-]+$'explicitly rejects the wildcard"*"used in the provided examplegateway/examples/websubhub.yaml. The example file usesmain: "*"for wildcard matching, but this value would fail OpenAPI schema validation against the current pattern. Either update the pattern to allow wildcards (e.g.,'^(\*|[a-zA-Z0-9\.\-]+)$') or change the example to use a specific domain.
♻️ Duplicate comments (4)
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
1704-1712: Guard against nil Subscribe before dereferencing Policies.
Subscribeis optional; this will panic when a channel has no subscribe operation.🔧 Suggested fix
- if ch.Subscribe.Policies != nil && len(*ch.Subscribe.Policies) > 0 { + if ch.Subscribe != nil && ch.Subscribe.Policies != nil && len(*ch.Subscribe.Policies) > 0 {gateway/gateway-controller/pkg/xds/translator.go (2)
692-694: Use hostname (not full URL) for virtual host Domains.
Envoy vhost matching expects hostnames; using a full URL can prevent matches.🔧 Suggested fix
+ parsedURL, err := url.Parse(t.routerConfig.EventGateway.WebSubHubURL) + if err != nil { + return nil, fmt.Errorf("invalid WebSubHubURL: %w", err) + } + host := parsedURL.Hostname() + if host == "" { + return nil, fmt.Errorf("invalid WebSubHubURL: missing host") + } dynamicForwardProxyRouteConfig := &route.RouteConfiguration{ Name: "dynamic-forward-proxy-routing", VirtualHosts: []*route.VirtualHost{{ Name: "DYNAMIXC_FORWARD_PROXY_VHOST_WEBSUBHUB", - Domains: []string{t.routerConfig.EventGateway.WebSubHubURL}, // this should be websubhub domains + Domains: []string{host},
391-391: Remove unusedcfg.GetContext()call.
It has no effect and was previously flagged.🔧 Suggested fix
- cfg.GetContext()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 {
🧹 Nitpick comments (2)
gateway/gateway-controller/pkg/storage/memory.go (1)
150-158: Extract topic-key composition into a shared helper.
This logic is duplicated with the update path in utils/APIDeploymentService and risks drift.♻️ Suggested refactor
+func buildTopicKey(displayName, context, version, path string) string { + name := strings.TrimPrefix(displayName, "/") + ctx := strings.TrimPrefix(context, "/") + ver := strings.TrimPrefix(version, "/") + p := strings.TrimPrefix(path, "/") + return fmt.Sprintf("%s_%s_%s_%s", name, ctx, ver, p) +} + 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) 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) + modifiedTopic := buildTopicKey(asyncData.DisplayName, asyncData.Context, asyncData.Version, topic.Path) cs.TopicManager.Add(cfg.ID, modifiedTopic) apiTopicsPerRevision[modifiedTopic] = true }gateway/gateway-controller/pkg/xds/translator.go (1)
67-72: Prefer the shared WebSubHub cluster constant.
Avoid duplicating the constant to reduce divergence risk.♻️ Suggested refactor
- WebSubHubInternalClusterName = "WEBSUBHUB_INTERNAL_CLUSTER"- ClusterSpecifier: &route.RouteAction_Cluster{Cluster: WebSubHubInternalClusterName}, + ClusterSpecifier: &route.RouteAction_Cluster{Cluster: constants.WEBSUBHUB_INTERNAL_CLUSTER_NAME},Also applies to: 617-618
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
gateway/gateway-controller/pkg/api/handlers/handlers.go (4)
716-724: Remove misleadingzap.Error(err)—erris guaranteed to be nil here.At this point,
erris from thes.db.GetConfigByHandle(handle)call at line 620, which would have caused an early return if it were non-nil. Loggingzap.Error(nil)provides no useful debugging information. The actual errors are tracked via the atomic counters.🔧 Suggested fix
if regErrs > 0 || deregErrs > 0 { - 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)) c.JSON(http.StatusInternalServerError, api.ErrorResponse{ Status: "error", Message: "Topic lifecycle operations failed",
763-771: Inconsistent xDS snapshot timeout between UpdateAPI and DeleteAPI.This uses
s.routerConfig.EventGateway.TimeoutSecondsfor the xDS snapshot update, but the equivalent code inDeleteAPI(line 975) uses a hardcoded10*time.Second. Additionally, using the EventGateway timeout for xDS operations is semantically misaligned since xDS updates are not specific to the event gateway.Consider using a dedicated xDS/general timeout configuration for consistency across both operations.
🔧 Suggested fix for consistency
Either use a hardcoded timeout consistently:
- ctx, cancel := context.WithTimeout(context.Background(), time.Duration(s.routerConfig.EventGateway.TimeoutSeconds)*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)Or introduce a dedicated xDS timeout configuration and use it in both UpdateAPI and DeleteAPI.
1701-1731: Potential nil pointer dereference: missing check forch.Subscribe.Policies.Line 1704 checks
ch.Subscribe != nilbut then immediately dereferences*ch.Subscribe.Policieswithout verifying thatPoliciesis not nil. SinceSubscribe.Policiesis a pointer type (*[]Policywithomitempty), it can be nil when a channel has a subscribe operation but no policies defined, causing a panic.🐛 Suggested fix
for _, ch := range apiData.Channels { var finalPolicies []policyenginev1.PolicyInstance - if ch.Subscribe != 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))
949-957: Fix error logging:erris nil and message is incorrect for delete operation.Similar to the UpdateAPI issue,
errhere is from an earlier successful call (line 836) and is guaranteed to be nil. Additionally, the message "Failed to register & deregister topics" is misleading since the delete operation only performs deregistration.🔧 Suggested fix
if deregErrs > 0 { - log.Error("Failed to register & deregister topics", zap.Error(err)) + log.Error("Failed to deregister topics", + zap.Int32("deregister_errors", deregErrs)) c.JSON(http.StatusInternalServerError, api.ErrorResponse{ Status: "error", Message: "Topic lifecycle operations failed",
|
Re-triggering the operator test Forwarding from 127.0.0.1:9090 -> 8080 |
| - RestApi | ||
| - async/websub | ||
| - async/websocket | ||
| - async/sse | ||
| - WebSubApi |
There was a problem hiding this comment.
@BLasan
I had a chat with @malinthaprasan regarding introducing WebSocket, and then we thought it was good to separate out RestAPI, WebSocket, and WebSubAPI into separate Rest API resources.
Something like
- GET /apis -> return all types of APIs with common API details
- GET /rest-apis -> return only Rest APIs
- GET /web-socket -> return only WebSockeet APIs
- GET /web-sub-apis -> return only WebSub APIs
- POST /rest-apis -> create Rest API
- POST /web-socket -> create WebSocket API
- POST /web-sub-apis -> create WebSub API
- No POST /apis
There was a problem hiding this comment.
Maybe we can do that in a separate PR
There was a problem hiding this comment.
+1 doing in a separate PR once properly finalized
There was a problem hiding this comment.
We should make this a separate discussion thread.
BLasan
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@platform-api/src/internal/model/api.go`:
- Around line 80-90: The XHubSignature field is defined in the model but not
propagated through DTOs, mappers, persistence, or schema; add it end-to-end by:
1) adding a matching XHubSignature DTO (e.g., type XHubSignature in
platform-api/src/internal/dto/api.go) and include it on the SecurityConfig DTO;
2) update mapper functions SecurityDTOToModel and SecurityModelToDTO in
platform-api/src/internal/utils/api.go to translate XHubSignature between DTO
and model; 3) extend the DB schema
(platform-api/src/internal/database/schema.sql) with an xhub_signature table or
columns mirroring HeaderName/Secret/Enabled; and 4) update repository functions
insertSecurityConfig and loadSecurityConfig in
platform-api/src/internal/repository/api.go to persist and load the
XHubSignature data when saving and retrieving SecurityConfig so the field is not
dropped.
In `@platform-api/src/internal/repository/api.go`:
- Around line 887-929: The SQL in loadChannels currently selects every
api_operations row; update loadChannels to only select rows that are
channel-type (add a discriminator filter, e.g. WHERE api_uuid = ? AND kind =
'channel' or the appropriate discriminator column) and after scanning each row
hydrate the channel's backend routing by loading the backend service (call or
reuse an existing loader—e.g., loadBackendService or add a query to fetch
backend service fields and set channel.Request.BackendService / channel.Backend
accordingly) before appending to channels; also update loadOperations to add the
inverse filter (exclude rows where kind = 'channel') so operations and channels
are mutually exclusive.
- Around line 836-855: The INSERT in insertChannel is wrong: update the SQL to
insert the correct columns (api_uuid, name, description, method, path,
authentication_required, scopes) and pass exactly those 7 values (apiId,
channel.Name, channel.Description, channel.Request.Method, channel.Request.Path,
authRequired, scopesJSON); then add persistence for channel.BackendServices by
inserting into operation_backend_services just like insertOperation does (use
the channel's generated operation ID from the result) and ensure loadChannels
mirrors loadOperations by loading BackendServices via operation_backend_services
so channels reconstruct their BackendServices.
In `@platform-api/src/internal/service/api.go`:
- Around line 868-870: When updating APIs, ensure switching to async protocols
(e.g., WebSub or WS) doesn't leave existingAPI.Channels empty: in the update
path that reads req.Channels and assigns existingAPI.Channels, detect if the
target protocol/type is an async type and then either (a) reject the update with
a validation error if req.Channels is nil or empty, or (b) populate
existingAPI.Channels with sensible defaults before persisting; implement this
check around the req.Channels handling (use the same update function that
touches req.Channels and existingAPI.Channels) so async APIs always have at
least one channel.
- Around line 120-127: The conditional that auto-generates channels incorrectly
includes constants.APITypeWS causing WebSocket APIs to get default channels even
though the gateway-controller (api_deployment.go) doesn't support WS; update the
condition in the block that calls s.generateDefaultChannels(req.Type) to remove
constants.APITypeWS so it only runs for constants.APITypeWebSub, i.e., change
the branch that checks (constants.APITypeWebSub == req.Type ||
constants.APITypeWS == req.Type) to only check constants.APITypeWebSub (leave
s.generateDefaultChannels and req.Channels assignment as-is).
🧹 Nitpick comments (4)
platform-api/src/internal/dto/api.go (1)
140-145: Clarify Channel/ChannelRequest doc comments.
These comments still refer to “operations,” which can be misleading for channel semantics.💡 Suggested doc fix
-// Channel represents an API operation +// Channel represents an API channel ... -// OperationRequest represents operation request details +// ChannelRequest represents channel request detailsAlso applies to: 156-163
platform-api/src/internal/model/api.go (1)
225-230: Align channel doc comments with channel semantics.💡 Suggested doc fix
-// Channel represents an API operation +// Channel represents an API channel ... -// ChannelRequest represents channel request details +// ChannelRequest represents channel request detailsAlso applies to: 241-248
platform-api/src/resources/openapi.yaml (1)
2163-2240: Tighten ChannelRequest descriptions to match PUB/SUB semantics.
The current text still refers to “HTTP method/operation”; updating those descriptions improves clarity for async APIs.💡 Suggested doc tweaks
- method: - description: HTTP method for the operation + method: + description: Channel action (PUB for publish, SUB for subscribe) ... - backend-services: - description: List of backend services for this operation + backend-services: + description: List of backend services for this channel ... - policies: - description: List of policies to be applied on the operation + policies: + description: List of policies to be applied on the channelplatform-api/src/internal/service/api.go (1)
1084-1131: Reduce duplication and avoid string literal for API type.Small maintainability win: build the base channel slice once and use
constants.APITypeWebSubinstead of a string literal. This prevents drift if constants change.♻️ Suggested refactor
func (s *APIService) generateDefaultChannels(asyncAPIType string) []dto.Channel { - if asyncAPIType == "WEBSUB" { - return []dto.Channel{ - { - Name: "Default", - Description: "Default SUB Channel", - Request: &dto.ChannelRequest{ - Method: "SUB", - Path: "/_default", - Authentication: &dto.AuthenticationConfig{ - Required: false, - Scopes: []string{}, - }, - Policies: []dto.Policy{}, - }, - }, - } - } - return []dto.Channel{ - { - Name: "Default", - Description: "Default SUB Channel", - Request: &dto.ChannelRequest{ - Method: "SUB", - Path: "/_default", - Authentication: &dto.AuthenticationConfig{ - Required: false, - Scopes: []string{}, - }, - Policies: []dto.Policy{}, - }, - }, - { - Name: "Default PUB Channel", - Description: "Default PUB Channel", - Request: &dto.ChannelRequest{ - Method: "PUB", - Path: "/_default", - Authentication: &dto.AuthenticationConfig{ - Required: false, - Scopes: []string{}, - }, - Policies: []dto.Policy{}, - }, - }, - } + channels := []dto.Channel{ + { + Name: "Default", + Description: "Default SUB Channel", + Request: &dto.ChannelRequest{ + Method: "SUB", + Path: "/_default", + Authentication: &dto.AuthenticationConfig{ + Required: false, + Scopes: []string{}, + }, + Policies: []dto.Policy{}, + }, + }, + } + if asyncAPIType != constants.APITypeWebSub { + channels = append(channels, dto.Channel{ + Name: "Default PUB Channel", + Description: "Default PUB Channel", + Request: &dto.ChannelRequest{ + Method: "PUB", + Path: "/_default", + Authentication: &dto.AuthenticationConfig{ + Required: false, + Scopes: []string{}, + }, + Policies: []dto.Policy{}, + }, + }) + } + return channels }
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 (3)
gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
470-474: Update kind-mismatch log to mention WebSubApi.
The log still references “async/websub,” which no longer matches the kind naming.🔧 Suggested fix
- log.Warn("Configuration kind mismatch", - zap.String("expected", "RestApi or async/websub"), + log.Warn("Configuration kind mismatch", + zap.String("expected", "RestApi or WebSubApi"),
1694-1733: Guard against nilch.Policiesto avoid panics.The
Policiesfield is defined as*[]Policywithomitemptytags, making it optional and thus nullable. The code dereferences*ch.Policieswithout a nil check, which will panic if a channel omits policies.🐛 Suggested fix
- if len(*ch.Policies) > 0 { + if ch.Policies != nil && len(*ch.Policies) > 0 {platform-api/src/internal/repository/api.go (1)
599-649: Persist/load XHubSignature even when OAuth2 is absent.Right now XHubSignature is inserted and loaded only inside the OAuth2 branch. If an API uses XHubSignature without OAuth2 (likely for WebSub), it will never persist or load, silently disabling security.
🐛 Proposed fix
func (r *APIRepo) insertSecurityConfig(tx *sql.Tx, apiId string, security *model.SecurityConfig) error { // Insert API Key security if present if security.APIKey != nil { ... } // Insert OAuth2 security if present if security.OAuth2 != nil { ... - if security.XHubSignature != nil { - xHubQuery := ` - INSERT INTO xhub_signature_security (api_uuid, enabled, secret, algorithm, header) - VALUES (?, ?, ?, ?, ?) - ` - _, err := tx.Exec(xHubQuery, apiId, security.XHubSignature.Enabled, - security.XHubSignature.Secret, security.XHubSignature.Algorithm, security.XHubSignature.Header) - if err != nil { - return err - } - } } + + // Insert XHub Signature security if present (independent of OAuth2) + if security.XHubSignature != nil { + xHubQuery := ` + INSERT INTO xhub_signature_security (api_uuid, enabled, secret, algorithm, header) + VALUES (?, ?, ?, ?, ?) + ` + if _, err := tx.Exec(xHubQuery, apiId, security.XHubSignature.Enabled, + security.XHubSignature.Secret, security.XHubSignature.Algorithm, security.XHubSignature.Header); err != nil { + return err + } + } return nil } func (r *APIRepo) loadSecurityConfig(apiId string) (*model.SecurityConfig, error) { security := &model.SecurityConfig{Enabled: true} ... - // Load XHub Signature security if present - xHub := &model.XHubSignatureSecurity{} - xHubQuery := ` - SELECT enabled, secret, algorithm, header - FROM xhub_signature_security WHERE api_uuid = ? - ` - err = r.db.QueryRow(xHubQuery, apiId).Scan(&xHub.Enabled, - &xHub.Secret, &xHub.Algorithm, &xHub.Header) - if err == nil { - security.XHubSignature = xHub - } else if !errors.Is(err, sql.ErrNoRows) { - return nil, err - } + // Load XHub Signature security if present (independent of OAuth2) + xHub := &model.XHubSignatureSecurity{} + xHubQuery := ` + SELECT enabled, secret, algorithm, header + FROM xhub_signature_security WHERE api_uuid = ? + ` + err = r.db.QueryRow(xHubQuery, apiId).Scan(&xHub.Enabled, + &xHub.Secret, &xHub.Algorithm, &xHub.Header) + if err == nil { + security.XHubSignature = xHub + } else if !errors.Is(err, sql.ErrNoRows) { + return nil, err + }Also applies to: 714-727
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/config/api_validator.go`:
- Around line 325-349: The loop starting at "for i, ch := range channels" calls
v.validatePathParametersForAsyncAPIs(ch.Name) twice, causing duplicate
ValidationError entries; remove the redundant second validation block (the one
that appends "Channel name has {} in parameters") so that
validatePathParametersForAsyncAPIs is invoked once per channel and only a single
ValidationError (using the intended message "Operation path has braces which is
not allowed" or consolidate into one clear message) is appended via
ValidationError with Field formatted by fmt.Sprintf("spec.channels[%d].name",
i).
- Around line 261-279: The docstring and ValidationError Field values in
validateAsyncData are incorrect: update the function comment to indicate it
validates WebSub/async data, and change all ValidationError Field entries to use
the correct path "spec.displayName" (not "spec.name"); ensure the length and
regex checks still reference spec.DisplayName and urlFriendlyNameRegex, and keep
the ValidationError construction using the ValidationError type from
APIValidator for consistency.
In `@gateway/gateway-controller/pkg/storage/memory.go`:
- Around line 150-157: The current topic normalization in the loop over
asyncData.Channels (where contextWithVersion is built by replacing "/" with "_"
and then combined into modifiedTopic before calling cs.TopicManager.Add) can
produce collisions (e.g., "/a/b" vs "/a_b"); change the normalization to a
reversible or unambiguous encoding instead — for example apply a reversible
encoder like url.PathEscape (net/url.PathEscape) or use a reserved delimiter
that cannot appear in context segments (e.g., "::") when creating modifiedTopic,
and ensure decoding logic (if any) and TopicManager.Add consumers are updated to
expect the new format.
In `@platform-api/src/internal/dto/api.go`:
- Around line 65-77: The XHubSignatureSecurity.Secret field is being returned in
API read responses (via dto.API used by GetAPI, ListAPIs, CreateAPI, UpdateAPI)
and must be excluded; update the API surface by introducing a read-only response
DTO (e.g., APIResponse or XHubSignatureSecurityResponse) that omits Secret,
convert handlers GetAPI/ListAPIs/CreateAPI/UpdateAPI to return the new DTO (or
implement a custom JSON MarshalJSON on XHubSignatureSecurity that conditionally
hides Secret when serializing for responses), and ensure persistence still
encrypts Secret at rest (verify storage APIs/encryption logic used when saving
and reading secrets).
♻️ Duplicate comments (4)
gateway/gateway-controller/pkg/xds/translator.go (2)
689-748: Use hostname (not full URL) for dynamic forward proxyDomains.Envoy
VirtualHost.Domainsmatches Host header values; a full URL with scheme won’t match. ParseWebSubHubURLand useHostname(), with validation.🔧 Proposed fix
- dynamicForwardProxyRouteConfig := &route.RouteConfiguration{ + parsedURL, err := url.Parse(t.routerConfig.EventGateway.WebSubHubURL) + if err != nil { + return nil, fmt.Errorf("invalid WebSubHubURL: %w", err) + } + hostname := parsedURL.Hostname() + if hostname == "" { + return nil, fmt.Errorf("invalid WebSubHubURL: missing hostname") + } + dynamicForwardProxyRouteConfig := &route.RouteConfiguration{ Name: "dynamic-forward-proxy-routing", VirtualHosts: []*route.VirtualHost{{ Name: "DYNAMIXC_FORWARD_PROXY_VHOST_WEBSUBHUB", - Domains: []string{t.routerConfig.EventGateway.WebSubHubURL}, // this should be websubhub domains + Domains: []string{hostname}, // hostname only
388-393: Remove orphanedcfg.GetContext()call.Line 391 calls
cfg.GetContext()without using the result; it has no effect.platform-api/src/internal/service/api.go (1)
882-884: Prevent async updates from producing zero channels.If an HTTP API switches to WebSub and
req.Channelsis nil,existingAPI.Channelsmay stay empty. Add a default or validation guard.🛠️ Possible fix
if req.Channels != nil { existingAPI.Channels = *req.Channels } + if req.Channels == nil && + existingAPI.Type == constants.APITypeWebSub && + len(existingAPI.Channels) == 0 { + existingAPI.Channels = s.generateDefaultChannels(existingAPI.Type) + }platform-api/src/internal/repository/api.go (1)
862-895: Channels still indistinguishable from operations, and channel backend-services aren’t persisted.
loadChannelsandloadOperationsboth read all rows fromapi_operations, so channels/operations bleed into each other.insertChannelalso skips persisting channel backend-services (and loadChannels doesn’t hydrate them). This matches the previously reported issue and still applies.Also applies to: 913-1013
🧹 Nitpick comments (7)
gateway/gateway-controller/pkg/utils/api_deployment.go (1)
179-266: Configurable, context-bound topic ops look good.
Reminder: for gateway component changes, rebuild the local gateway image (cd gateway && make build-local) before testing/deploying. As per coding guidelines, ...gateway/gateway-controller/api/openapi.yaml (1)
1747-1753: WebSubApi kind exposure looks correct.This keeps the contract aligned with the only async kind currently supported and helps prevent unsupported submissions. Based on learnings, WebSubApi is the sole async kind handled by gateway-controller. As per coding guidelines, rebuild gateway images with
cd gateway && make build-local.gateway/gateway-controller/pkg/xds/translator.go (3)
67-72: Prefer a single WebSubHub cluster-name constant.This file now defines
WebSubHubInternalClusterNamewhileconstants.WEBSUBHUB_INTERNAL_CLUSTER_NAMEalready exists; consider consolidating to avoid drift.
343-383: WebSubHubURL path normalization isn’t applied.
parsedMainURL.Pathis set but never used (PrefixRewrite remains/hub), so custom hub paths would be ignored. Either wire the parsed path into route rewriting or drop the normalization to avoid misleading config.
953-996: Channel method parameter isn’t used for HTTP matching.Header matching is hard-coded to
POST, so any non-POST method is ignored. If you intend SUB/PUB semantics, normalize method → HTTP method explicitly and use it in the matcher to avoid drift.♻️ Suggested normalization
func (t *Translator) createRoutePerTopic(apiName, apiVersion, context, method, name, clusterName, vhost string) *route.Route { routeName := GenerateRouteName(method, context, apiVersion, name, vhost) + httpMethod := method + if method == "SUB" { + httpMethod = "POST" + } r := &route.Route{ Name: routeName, Match: &route.RouteMatch{ Headers: []*route.HeaderMatcher{{ Name: ":method", HeaderMatchSpecifier: &route.HeaderMatcher_StringMatch{ StringMatch: &matcher.StringMatcher{ MatchPattern: &matcher.StringMatcher_Exact{ - Exact: "POST", + Exact: httpMethod, }, }, }, }}, },platform-api/src/internal/service/api.go (2)
94-209: Remove CreateAPI debug prints.These unconditional stdout prints add noise and bypass structured logging; please remove or replace with the standard logger at an appropriate level.
🧹 Suggested cleanup
- fmt.Println("Project Created") @@ - fmt.Println("Error generating handle:", err) @@ - fmt.Println("Channels Created") @@ - fmt.Println("Backend Services Created") @@ - fmt.Println("Got API Model") @@ - fmt.Println("Created API in Repository: ", apiModel.ID) @@ - fmt.Println("Associated Backends") @@ - fmt.Println("Associated Devportal")
1099-1103: Preferconstants.APITypeWebSubover string literal.♻️ Suggested tweak
- if asyncAPIType == "WEBSUB" { + if asyncAPIType == constants.APITypeWebSub {
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway/gateway-controller/api/openapi.yaml (1)
1806-1816: Update the REST API example to match the newupstreamschema.The schema now expects
upstream.main/sandbox, but the POST/apisexample still showsupstreams:. That mismatch will confuse users.✏️ Suggested doc fix
- upstreams: - - url: http://petstore.swagger.io/v2 + upstream: + main: + url: http://petstore.swagger.io/v2gateway/gateway-controller/pkg/xds/translator.go (1)
633-636: Useevent_gateway.timeout_secondsfor WebSub routes.Both WebSub route actions hard-code
30s, which ignores the new timeout config.✅ Suggested fix
- Timeout: durationpb.New(30 * time.Second), + Timeout: durationpb.New(time.Duration(t.routerConfig.EventGateway.TimeoutSeconds) * time.Second),- Timeout: durationpb.New(30 * time.Second), + Timeout: durationpb.New(time.Duration(t.routerConfig.EventGateway.TimeoutSeconds) * time.Second),Also applies to: 849-855
🤖 Fix all issues with AI agents
In `@gateway/configs/config.yaml`:
- Around line 107-112: The TLS cert_path and key_path under downstream_tls
currently use absolute, developer-specific file paths; change them to either
repo-relative paths (e.g., listener-certs/default-listener.crt and
listener-certs/default-listener.key) or reference environment variables (e.g.,
${TLS_CERT_PATH} and ${TLS_KEY_PATH}) so the config works across environments;
update the downstream_tls cert_path and key_path entries to use those
placeholders and ensure any startup scripts or deployment manifests set the
corresponding env vars or mount the relative cert files.
In `@gateway/gateway-controller/pkg/config/config.go`:
- Around line 687-705: In validateEventGatewayConfig ensure that when
c.GatewayController.Router.EventGateway.Enabled is true you require a non-empty
websub_hub_url and that the parsed URL contains a non-empty host; update the
existing WebSubHubURL check in validateEventGatewayConfig (and use url.Parse
like already done) to return an error if
strings.TrimSpace(c.GatewayController.Router.EventGateway.WebSubHubURL) == "" or
if the parsed URL's u.Host == "" (and still enforce http/https scheme),
producing a clear error referencing router.event_gateway.websub_hub_url; leave
the other port and timeout validations unchanged.
In `@gateway/gateway-controller/pkg/xds/translator.go`:
- Around line 703-713: The listener port selection currently hardcodes ports in
the block that sets listenerName and port (using isHTTPS to pick 8088/8083);
replace those literals with values read from the WebSub listener config
(event_gateway.websub_hub_listener_port and any dedicated fields like
websub_hub_listener_https_port or websub_hub_listener_dynamic_forward_proxy_port
if present) and fall back to the current defaults only if the config is empty;
update the listenerName construction to use the resolved port variable and apply
the same change to the other similar block that sets listenerName/port (the
second occurrence handling the dynamic/HTTPS listener) so all WebSub listeners
honor configuration.
- Around line 906-919: The first HttpFilter append is using wellknown.Router but
should use the dynamic forward proxy filter name, so change the Name on the
HttpFilter that attaches dynamicFwdAny (the append call that references
dynamicFwdAny) from wellknown.Router to the string
"envoy.filters.http.dynamic_forward_proxy" to avoid duplicate router names and
ensure dynamicFwdAny is applied; leave the subsequent HttpFilter that uses
routerAny named as wellknown.Router.
In `@platform-api/src/internal/repository/api.go`:
- Around line 713-727: The XHubSignature loading (the xHub variable and the
r.db.QueryRow call that scans into xHub.Enabled/Secret/Algorithm/Header and sets
security.XHubSignature) is erroneously nested inside the OAuth2 success branch;
move that entire block so it runs independently after the OAuth2 handling and
its error checks (i.e., not inside the if err == nil for OAuth2 and before the
shared else if !errors.Is(err, sql.ErrNoRows) branch). Ensure the new placement
still checks for sql.ErrNoRows and only sets security.XHubSignature when
QueryRow returns nil, preserving the existing error return behavior for
non-ErrNoRows errors.
- Around line 862-881: The insertChannel function can panic because
channel.Request is accessed unconditionally for Method and Name; fix it by
introducing local variables (e.g., methodStr, requestName) initialized to empty
strings, then inside the existing if channel.Request != nil block assign
methodStr = channel.Request.Method and requestName = channel.Request.Name (keep
the existing authRequired/scopes handling), and finally use methodStr and
requestName in the tx.Exec call in place of channel.Request.Method and
channel.Request.Name so the function no longer dereferences a nil
channel.Request.
- Line 1090: The DeleteAPI implementation is missing a delete for
xhub_signature_security so orphaned rows remain; update the deleteQueries slice
in the DeleteAPI function to include the SQL "DELETE FROM
xhub_signature_security WHERE api_uuid = ?" (same placeholder style as the other
deletes) so the API's associated signature security rows are removed when
DeleteAPI runs.
- Around line 638-650: The XHubSignature INSERT (xHubQuery and its tx.Exec call
that uses security.XHubSignature.Enabled/Secret/Algorithm/Header) is currently
nested inside the OAuth2 conditional and thus only runs when security.OAuth2 !=
nil; move the entire XHubSignature check and insertion so it executes
independently (i.e., after the OAuth2 block and other security blocks) by
placing the if security.XHubSignature != nil { ... tx.Exec(...) } outside and
following the OAuth2-related code paths so XHubSignature is persisted regardless
of OAuth2 configuration.
♻️ Duplicate comments (4)
platform-api/src/internal/repository/api.go (2)
913-959: No discriminator filter to load only channels.The query selects all rows from
api_operationswithout filtering for channel-specific records. This will return operations as channels. Additionally,BackendServicesare not being loaded (unlikeloadOperationswhich callsloadOperationBackendServices).This issue was previously flagged in past reviews regarding filtering channel rows and hydrating backend services.
872-896: Missing discriminator column causes channels and operations to be indistinguishable.Both
insertChannelandinsertOperationinsert intoapi_operationswithout any discriminator column (e.g.,typeorkind). This meansloadOperationswill return channels as operations, andloadChannelswill return operations as channels, causing data corruption.Additionally,
BackendServicesare not being persisted for channels (unlike operations which persist them viaoperation_backend_services).Suggested approach
- Add a discriminator column to
api_operationstable (e.g.,operation_type VARCHAR(20)with values'OPERATION'or'CHANNEL')- Update
insertChannelto setoperation_type = 'CHANNEL'- Update
insertOperationto setoperation_type = 'OPERATION'- Update
loadChannelsto filterWHERE operation_type = 'CHANNEL'- Update
loadOperationsto filterWHERE operation_type = 'OPERATION'- Add BackendServices persistence similar to
insertOperation(lines 829-850)The BackendServices persistence issue was previously flagged.
gateway/gateway-controller/pkg/xds/translator.go (2)
405-409: Remove the unusedcfg.GetContext()call.🧹 Suggested cleanup
- cfg.GetContext()
842-846: Domains must be hostnames, not full URLs.Envoy virtual host
Domainsexpects hostnames (e.g.,host.docker.internal). Supplying a URL with scheme will never match the Host header.✅ Suggested fix
- VirtualHosts: []*route.VirtualHost{{ - Name: "DYNAMIXC_FORWARD_PROXY_VHOST_WEBSUBHUB", - Domains: []string{t.routerConfig.EventGateway.WebSubHubURL}, // this should be websubhub domains + parsedURL, err := url.Parse(t.routerConfig.EventGateway.WebSubHubURL) + if err != nil || parsedURL.Hostname() == "" { + return nil, fmt.Errorf("invalid websub_hub_url for vhost domains: %s", t.routerConfig.EventGateway.WebSubHubURL) + } + VirtualHosts: []*route.VirtualHost{{ + Name: "DYNAMIXC_FORWARD_PROXY_VHOST_WEBSUBHUB", + Domains: []string{parsedURL.Hostname()},
🧹 Nitpick comments (2)
gateway/gateway-controller/pkg/config/config.go (1)
371-404: Rebuild gateway images after these gateway-controller config changes.As per coding guidelines, please run:
cd gateway && make build-localgateway/gateway-controller/pkg/xds/translator.go (1)
67-72: Use the shared WEBSUBHUB_INTERNAL_CLUSTER_NAME constant.There’s a local duplicate that can drift from
constants.WEBSUBHUB_INTERNAL_CLUSTER_NAME. Prefer the shared constant and remove the local one.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway/gateway-controller/pkg/utils/api_deployment.go (1)
487-513: Fix success-path handling for WebSubHub requests.A 2xx response still sets
lastStatusand returns a non-success error because the success path never returns. This will make all topic operations fail even when the hub succeeds.🐛 Proposed fix
- // Ensure body is closed before next loop/return - func() { + // Ensure body is closed before next loop/return + success := func() bool { defer resp.Body.Close() if resp.StatusCode >= 200 && resp.StatusCode < 300 { logger.Debug("Topic request sent to WebSubHub", slog.String("topic", topic), slog.String("mode", mode), slog.Int("status", resp.StatusCode)) + return true } lastStatus = resp.StatusCode - }() - - // Success path returned above - if lastStatus == 0 { - return nil - } + return false + }() + + if success { + return nil + }gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
1704-1734: Guard optional channel policies before dereferencing.
ch.Policiesis optional; dereferencing it without a nil check can panic for channels without policies.🐛 Proposed fix
- if len(*ch.Policies) > 0 { + if ch.Policies != nil && len(*ch.Policies) > 0 {
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/xds/translator.go (1)
180-186: Reminder: rebuild gateway images after these gateway changes.Please run
cd gateway && make build-localbefore validation. As per coding guidelines, rebuilds are required for gateway component updates.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/examples/websubhub.yaml`:
- Around line 17-20: The example uses the old channel key "path" for the channel
entries (e.g., the channel entries for "issues" and "pull_requests" with method
SUB); update those entries to use the required "name" key instead of "path" so
they conform to the Channel schema (replace "path: issues" and "path:
pull_requests" with "name: issues" and "name: pull_requests" respectively while
keeping the method set to SUB).
🧹 Nitpick comments (5)
gateway/gateway-controller/pkg/config/config.go (1)
202-210: LGTM on the expanded EventGatewayConfig defaults.
The new RouterHost/WebSubHubListenerPort/TimeoutSeconds defaults make the feature usable out-of-the-box. Also, remember to rebuild gateway images (cd gateway && make build-local) after these gateway component changes. As per coding guidelines, please ensure this build step is included.Also applies to: 414-420
gateway/gateway-controller/pkg/xds/translator.go (4)
809-884: Remove large blocks of commented-out code.Lines 809-884 contain extensive commented-out code that duplicates the active implementation. This reduces readability and creates maintenance burden. If this code is no longer needed, it should be removed. If it's being preserved for reference, consider documenting the rationale in a separate location.
♻️ Proposed cleanup
Remove the entire commented block from lines 809-884 to improve code clarity.
948-996: Remove additional commented-out code blocks.Similar to the previous comment, lines 948-996 contain commented-out code that should be removed for clarity.
1016-1019: Remove small commented-out code block.Another small commented block that should be cleaned up.
1056-1061: Remove trailing commented-out code.Final commented block that should be removed.
|
@BLasan tests are faling, can you plz look into it. |
…gistration/deregistration
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
473-479: Update kind mismatch log label.
Line 475 still says “async/websub” though the kind is now WebSubApi.🔧 Proposed fix
- slog.String("expected", "RestApi or async/websub"), + slog.String("expected", "RestApi or WebSubApi"),
1668-1707: Guard nil channel policies to avoid panic.
ch.Policiesis optional;len(*ch.Policies)will panic when it’s nil. Add a nil check before dereferencing.🔧 Proposed fix
- if len(*ch.Policies) > 0 { + if ch.Policies != nil && len(*ch.Policies) > 0 {gateway/gateway-controller/pkg/utils/api_deployment.go (1)
474-500: Fix success handling insendTopicRequestToHub(2xx currently treated as error).On a successful 2xx response,
lastStatusis set to 200 and the function returns a non‑success error. This makes every registration/deregistration fail even when the hub accepts the request.🐛 Proposed fix
- func() { - defer resp.Body.Close() - - if resp.StatusCode >= 200 && resp.StatusCode < 300 { - logger.Debug("Topic request sent to WebSubHub", - slog.String("topic", topic), - slog.String("mode", mode), - slog.Int("status", resp.StatusCode)) - } - - lastStatus = resp.StatusCode - }() + func() { + defer resp.Body.Close() + + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + logger.Debug("Topic request sent to WebSubHub", + slog.String("topic", topic), + slog.String("mode", mode), + slog.Int("status", resp.StatusCode)) + lastStatus = 0 + return + } + + lastStatus = resp.StatusCode + }() // Success path returned above if lastStatus == 0 { return nil }
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 739-740: The async UpdateSnapshot calls are using mixed timeout
sources: one uses context.WithTimeout(context.Background(),
time.Duration(s.routerConfig.EventGateway.TimeoutSeconds)*time.Second) while
others pass a hardcoded 10*time.Second; standardize them by replacing the
hardcoded 10*time.Second usages with
time.Duration(s.routerConfig.EventGateway.TimeoutSeconds)*time.Second so all
UpdateSnapshot invocations use the configured EventGateway.TimeoutSeconds
(locate the UpdateSnapshot call sites and any context.WithTimeout calls in this
file and make the substitution).
In `@gateway/gateway-controller/pkg/utils/helpers.go`:
- Around line 21-26: The error message for the WebSubApi parsing branch is
outdated ("failed to parse async/websub api config data"); update the fmt.Errorf
string inside the cfg.Kind == api.WebSubApi branch where AsWebhookAPIData() is
called so it accurately references WebSub/Webhook (e.g., "failed to parse websub
webhook api config data" or similar) — modify the error returned in that block
(the fmt.Errorf call) to the clearer text.
In `@gateway/gateway-controller/pkg/xds/translator.go`:
- Around line 1037-1057: createDynamicFwdListenerForWebSubHub builds a listener
for HTTPS ports but never attaches TLS (TransportSocket) to the
listener/filterChain; update createDynamicFwdListenerForWebSubHub to mirror the
TLS handling from createInternalListenerForWebSubHub: when isHTTPS is true,
construct the same TLS TransportSocket (with downstream_tls_context or the same
tls config object used by createInternalListenerForWebSubHub) and set it on the
listener (listener.TransportSocket) or filterChain as appropriate so the
listener created by createDynamicFwdListenerForWebSubHub has a proper TLS
context for HTTPS traffic.
In `@platform-api/src/internal/dto/api.go`:
- Around line 146-151: Update the doc comment for the Channel type to accurately
describe its semantics: replace the current line "Channel represents an API
operation" with a concise comment that states Channel models an API channel
(e.g., a communication channel or message/operation channel) and briefly
mentions its fields (Name, Description, Request) so readers understand it
represents a channel definition rather than a generic operation; locate the
comment directly above the Channel struct declaration and update it accordingly.
- Around line 162-169: The top-of-type doc comment is a copy-paste error:
replace the incorrect "OperationRequest" comment with a correct comment for
ChannelRequest. Locate the type declaration for ChannelRequest and update its
leading comment to accurately describe ChannelRequest (e.g., "ChannelRequest
represents channel request details") so the doc comment matches the
ChannelRequest type.
In `@platform-api/src/internal/repository/api.go`:
- Around line 116-121: Add validation in validateCreateAPIRequest and
validateUpdateAPIRequest to enforce exclusivity between api.Operations and
api.Channels based on the API type: if api.Type indicates HTTP, reject requests
that provide non-empty api.Channels (and/or both fields); if api.Type indicates
WebSub, reject requests that provide non-empty api.Operations (and/or both
fields); return a descriptive validation error. Also update UpdateAPI to rely on
these validators (or add the same check there) so it does not unconditionally
apply both operations and channels—use the existing symbols
validateCreateAPIRequest, validateUpdateAPIRequest, UpdateAPI, api.Operations,
api.Channels, and r.insertChannel to locate where to add the checks and error
returns.
🧹 Nitpick comments (12)
gateway/gateway-controller/pkg/storage/memory.go (1)
81-86: WebSubApi gating aligns with new kind.
Looks good.Reminder: gateway component changes should rebuild images with
cd gateway && make build-local.Also applies to: 128-133, 180-182
platform-api/src/resources/openapi.yaml (1)
1935-1940: Clarify operations vs channels exclusivity.
Consider noting that channels are for WebSub/async APIs and are mutually exclusive with operations to avoid ambiguous client payloads.💡 Suggested doc tweak
- description: List of channels exposed by this API + description: List of channels exposed by this API (WebSub/async only; mutually exclusive with operations)platform-api/src/internal/service/api.go (2)
94-107: Replacefmt.Printlndebug output with structured logging (or remove).These prints will leak to stdout in production paths. Prefer the existing logger or remove them.
♻️ Suggested cleanup
- fmt.Println("Project Created") ... - fmt.Println("Error generating handle:", err) ... - fmt.Println("Channels Created") ... - fmt.Println("Backend Services Created") ... - fmt.Println("Got API Model") ... - fmt.Println("Created API in Repository: ", apiModel.ID) ... - fmt.Println("Associated Backends") ... - fmt.Println("Associated Devportal")Also applies to: 132-133, 168-172, 177-178, 198-206
924-971: Use API type constants instead of string literals.Avoid
"WEBSUB"literals here; useconstants.APITypeWebSubfor consistency and to prevent typos.♻️ Proposed tweak
- if asyncAPIType == "WEBSUB" { + if asyncAPIType == constants.APITypeWebSub {gateway/gateway-controller/pkg/utils/api_deployment.go (1)
66-83: Reminder: rebuild gateway images after gateway changes.cd gateway && make build-localgateway/gateway-controller/api/openapi.yaml (1)
2114-2157: Consider removing commented-out schema blocks.Large commented-out schema sections (subscribe, publish, parameters, bindings) add noise to the OpenAPI spec. If these are intended for future use, consider documenting them in a separate design document or adding a brief inline comment explaining the rationale instead of leaving full schema blocks commented out.
gateway/gateway-controller/pkg/xds/translator.go (6)
74-74: Redundant constant definition.
WebSubHubInternalClusterNameis defined locally butconstants.WEBSUBHUB_INTERNAL_CLUSTER_NAMEis used elsewhere in this file (lines 331, 402). Consider removing this local constant and using the centralized constant consistently.♻️ Suggested fix
const ( DynamicForwardProxyClusterName = "dynamic-forward-proxy-cluster" ExternalProcessorGRPCServiceClusterName = "ext-processor-grpc-service" OTELCollectorClusterName = "otel_collector" - WebSubHubInternalClusterName = "WEBSUBHUB_INTERNAL_CLUSTER" )Then update line 694 to use
constants.WEBSUBHUB_INTERNAL_CLUSTER_NAME.
821-894: Remove large commented-out code block.This ~70-line commented-out code block adds significant noise and makes the file harder to maintain. If this code is no longer needed, remove it. If it's preserved for reference, consider moving it to a design document or version control history.
959-1007: Remove commented-out code block.Similar to above, this commented-out code block should be removed to improve code clarity. The active implementation above serves the same purpose.
1027-1035: Remove commented-out code block.Small commented-out marshalling code that duplicates the active
pbstmarshalling above. Remove for clarity.
1067-1072: Remove commented-out filter chain code.This commented-out alternative filter chain definition duplicates the active code and should be removed.
763-773: Wire listener ports to configuration values.The TODO comment indicates ports should come from config. Currently,
constants.WEBSUB_HUB_INTERNAL_HTTP_PORTandconstants.WEBSUB_HUB_INTERNAL_HTTPS_PORTare used, butevent_gateway.websub_hub_listener_portexists in config and isn't being used. This contradicts the PR objective to "allow administrators to configure event gateway behavior."♻️ Suggested approach
// Use config value for HTTP port with constant as fallback httpPort := uint32(constants.WEBSUB_HUB_INTERNAL_HTTP_PORT) if t.routerConfig.EventGateway.WebSubHubListenerPort > 0 { httpPort = uint32(t.routerConfig.EventGateway.WebSubHubListenerPort) } var listenerName string var port uint32 if isHTTPS { listenerName = fmt.Sprintf("listener_https_%d", constants.WEBSUB_HUB_INTERNAL_HTTPS_PORT) port = uint32(constants.WEBSUB_HUB_INTERNAL_HTTPS_PORT) } else { listenerName = fmt.Sprintf("listener_http_%d", httpPort) port = httpPort }Consider adding a separate config field for HTTPS port if needed.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway/gateway-controller/pkg/xds/translator.go (2)
901-918:Domainsfield requires hostname, not full URL.The
Domainsfield at line 905 receives a full URL (e.g.,"http://host.docker.internal") but Envoy expects hostnames only (e.g.,"host.docker.internal"). This will cause route matching to fail.Proposed fix
func (t *Translator) createDynamicFwdListenerForWebSubHub(isHTTPS bool) (*listener.Listener, error) { + parsedHubURL, err := url.Parse(t.routerConfig.EventGateway.WebSubHubURL) + if err != nil { + return nil, fmt.Errorf("failed to parse WebSubHubURL: %w", err) + } + hubHostname := parsedHubURL.Hostname() + dynamicForwardProxyRouteConfig := &route.RouteConfiguration{ Name: "dynamic-forward-proxy-routing", VirtualHosts: []*route.VirtualHost{{ Name: "DYNAMIXC_FORWARD_PROXY_VHOST_WEBSUBHUB", - Domains: []string{t.routerConfig.EventGateway.WebSubHubURL}, + Domains: []string{hubHostname}, Routes: []*route.Route{{
1420-1443: Method parameter ignored in header matcher.The
methodparameter is accepted (line 1420) and used inGenerateRouteName(line 1421), but the header matcher at line 1430 hardcodes"POST"instead of using themethodvariable. This means all routes will only match POST requests regardless of the intended method.Proposed fix
Match: &route.RouteMatch{ Headers: []*route.HeaderMatcher{{ Name: ":method", HeaderMatchSpecifier: &route.HeaderMatcher_StringMatch{ StringMatch: &matcher.StringMatcher{ MatchPattern: &matcher.StringMatcher_Exact{ - Exact: "POST", + Exact: method, }, }, }, }}, },
🤖 Fix all issues with AI agents
In `@platform-api/src/internal/service/api.go`:
- Around line 650-663: Type-specific validation is happening against req.Type
before defaults are applied, so a request missing the type but including
channels can bypass validation and later default to HTTP in CreateAPI; fix by
resolving/applying defaults for the request type prior to the switch (e.g., call
the same defaulting logic used in CreateAPI or a helper like
applyDefaults/getDefaultType) and then run the switch that checks req.Operations
and req.Channels (the current switch over req.Type in api.go) against the
resolved type to ensure channels/operations are rejected correctly.
- Around line 729-731: validateUpdateAPIRequest must enforce that operations and
channels are mutually exclusive according to the API type so applyAPIUpdates
cannot mix them; update validateUpdateAPIRequest (the function that validates
update requests) to check the target API type (use existingAPI.Type or the type
implied by the request) and reject updates that include Channels when the target
type is an HTTP/operations API or that include Operations when the target type
is a WebSub/channels API; specifically, when req.Channels != nil or
req.Operations != nil, ensure the opposite field set on existingAPI
(existingAPI.Channels or existingAPI.Operations) is absent/empty for the
intended target type and return a validation error if not, and ensure this check
runs before applyAPIUpdates mutates existingAPI.
🧹 Nitpick comments (4)
gateway/gateway-controller/pkg/xds/translator.go (2)
820-894: Remove commented-out code block.Lines 820-894 contain ~75 lines of commented-out code. This dead code clutters the file and reduces readability. Version control preserves history if this code is ever needed again.
763-773: Listener ports still not configurable via config.The TODO at line 764 notes that config values should be used. While using constants (lines 768-772) is better than magic numbers, the PR objective states administrators should be able to configure event gateway behavior. The
WebSubHubListenerPortconfig field exists but isn't wired here.Consider reading from config with constants as fallback:
port := uint32(constants.WEBSUB_HUB_INTERNAL_HTTP_PORT) if t.routerConfig.EventGateway.WebSubHubListenerPort > 0 { port = uint32(t.routerConfig.EventGateway.WebSubHubListenerPort) }platform-api/src/internal/service/api.go (2)
94-106: Replace stdout prints with structured logging.
fmt.Println(Line 94, 105, 132, 168, 171, 177, 198, 206) bypasses log levels and context; prefer the existing logger so logs are consistent and filterable.Also applies to: 132-132, 168-171, 177-177, 198-198, 206-206
939-985: Avoid hard-coded API type string ingenerateDefaultChannels.Using
"WEBSUB"risks drift from the canonical constant and can create incorrect defaults if the constant changes.♻️ Suggested fix
- if asyncAPIType == "WEBSUB" { + if asyncAPIType == constants.APITypeWebSub { return []dto.Channel{ {
| // Type-specific validations | ||
| // Ensure that WebSub APIs do not have operations and HTTP APIs do not have channels | ||
| switch req.Type { | ||
| case constants.APITypeWebSub: | ||
| // For WebSub APIs, ensure that at least one channel is defined | ||
| if req.Operations != nil || len(req.Operations) > 0 { | ||
| return errors.New("WebSub APIs cannot have operations defined") | ||
| } | ||
| case constants.APITypeHTTP: | ||
| // For HTTP APIs, ensure that at least one operation is defined | ||
| if req.Channels != nil || len(req.Channels) > 0 { | ||
| return errors.New("HTTP APIs cannot have channels defined") | ||
| } | ||
| } |
There was a problem hiding this comment.
Type validation runs before defaults; channels can slip into HTTP APIs.
At Line 650, validation uses req.Type before defaults are applied. If a client omits type but includes channels, validation doesn’t reject it, and later CreateAPI defaults to HTTP—persisting an invalid state.
🐛 Suggested fix
- // Validate API type if provided
- if req.Type != "" && !constants.ValidAPITypes[req.Type] {
+ apiType := req.Type
+ if apiType == "" {
+ apiType = constants.APITypeHTTP
+ }
+ // Validate API type (after defaulting)
+ if !constants.ValidAPITypes[apiType] {
return constants.ErrInvalidAPIType
}
// Type-specific validations
- switch req.Type {
+ switch apiType {
case constants.APITypeWebSub:
if req.Operations != nil || len(req.Operations) > 0 {
return errors.New("WebSub APIs cannot have operations defined")
}
case constants.APITypeHTTP:
if req.Channels != nil || len(req.Channels) > 0 {
return errors.New("HTTP APIs cannot have channels defined")
}
}🤖 Prompt for AI Agents
In `@platform-api/src/internal/service/api.go` around lines 650 - 663,
Type-specific validation is happening against req.Type before defaults are
applied, so a request missing the type but including channels can bypass
validation and later default to HTTP in CreateAPI; fix by resolving/applying
defaults for the request type prior to the switch (e.g., call the same
defaulting logic used in CreateAPI or a helper like
applyDefaults/getDefaultType) and then run the switch that checks req.Operations
and req.Channels (the current switch over req.Type in api.go) against the
resolved type to ensure channels/operations are rejected correctly.
| if req.Channels != nil { | ||
| existingAPI.Channels = *req.Channels | ||
| } |
There was a problem hiding this comment.
Updates can mix operations & channels; enforce exclusivity on update.
applyAPIUpdates now applies channels (Line 729), but validateUpdateAPIRequest doesn’t ensure the target API type only carries operations or channels. That lets HTTP APIs retain channels (or WebSub APIs retain operations) when type or fields are updated.
🧩 Suggested validation (target-type aware)
func (s *APIService) validateUpdateAPIRequest(existingAPIModel *model.API, req *UpdateAPIRequest, orgUUID string) error {
...
+ targetType := existingAPIModel.Type
+ if req.Type != nil {
+ targetType = *req.Type
+ }
+
+ ops := existingAPIModel.Operations
+ if req.Operations != nil {
+ ops = *req.Operations
+ }
+ chans := existingAPIModel.Channels
+ if req.Channels != nil {
+ chans = *req.Channels
+ }
+
+ switch targetType {
+ case constants.APITypeWebSub:
+ if len(ops) > 0 {
+ return errors.New("WebSub APIs cannot have operations defined")
+ }
+ case constants.APITypeHTTP:
+ if len(chans) > 0 {
+ return errors.New("HTTP APIs cannot have channels defined")
+ }
+ }
...
}🤖 Prompt for AI Agents
In `@platform-api/src/internal/service/api.go` around lines 729 - 731,
validateUpdateAPIRequest must enforce that operations and channels are mutually
exclusive according to the API type so applyAPIUpdates cannot mix them; update
validateUpdateAPIRequest (the function that validates update requests) to check
the target API type (use existingAPI.Type or the type implied by the request)
and reject updates that include Channels when the target type is an
HTTP/operations API or that include Operations when the target type is a
WebSub/channels API; specifically, when req.Channels != nil or req.Operations !=
nil, ensure the opposite field set on existingAPI (existingAPI.Channels or
existingAPI.Operations) is absent/empty for the intended target type and return
a validation error if not, and ensure this check runs before applyAPIUpdates
mutates existingAPI.
3 participants
Purpose
Admins should be able to configure event gateway, timeouts etc.
Goals
Related Issues
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.