feat(platform-api): support upstreamDefinitions pool and per-operation upstream overrides#2155
Conversation
mehara-rothila
changed the title
📝 WalkthroughWalkthroughThis change adds reusable upstream definitions and per-operation upstream overrides across the OpenAPI contract, generated API models, internal models, YAML DTOs, service validation, API handlers, conversion utilities, repository persistence, and deployment YAML generation. Upstream definitions support named backend pools, optional base paths, weighted targets, and connect timeouts. Operations can reference named definitions for main and sandbox upstreams. Create and update flows validate definitions and references, while tests cover conversion, merging, persistence, and deployment output. Sequence Diagram(s)sequenceDiagram
participant Client
participant APIHandler
participant APIService
participant Repository
participant DeploymentYAML
Client->>APIHandler: submit API with upstream definitions
APIHandler->>APIService: create or update request
APIService->>APIService: validate definitions and operation references
APIService->>Repository: persist validated configuration
Repository-->>APIService: stored API configuration
APIService->>DeploymentYAML: build deployment specification
DeploymentYAML-->>Client: upstream pools and operation references
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/src/resources/openapi.yaml`:
- Around line 8455-8464: UpstreamTimeout.connect's OpenAPI regex only allows a
single unit but the runtime uses time.ParseDuration (see
spec.upstreamDefinitions[%d].timeout.connect), so update the OpenAPI schema for
UpstreamTimeout.connect to either (A) broaden the pattern to match Go
time.ParseDuration format (allow multiple sequential value+unit segments,
decimals, and units like ns|us|µs|ms|s|m|h, optional leading minus) or (B)
explicitly document/validate that only single-unit durations are accepted;
locate the UpstreamTimeout.connect property in the OpenAPI file and change the
pattern or description accordingly.
- Around line 8427-8447: Update the ReusableUpstream schema so its basePath
property is constrained to a path-only form and each upstream.url is constrained
to host[:port]-only (no path/query/fragment) to match gateway validation:
tighten basePath (property name basePath) to require a leading "/" and a path
pattern (e.g., path segment characters only) and change
upstreams.items.properties.url to validate host[:port] (e.g., restrict
format/pattern to host with optional port) instead of generic uri; keep timeout
($ref: '`#/components/schemas/UpstreamTimeout`') unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c978ac83-b305-4917-85d3-8826d792c30e
📒 Files selected for processing (1)
platform-api/src/resources/openapi.yaml
mehara-rothila
changed the title
e27ea92 to
3cb7216
Compare
|
@coderabbitai a naming sanity-check on the upstream schemas in this PR, please. Context: this PR (#2155) is the platform-api / control-plane side of the per-operation upstream feature. Its gateway / data-plane companion is #2012 (#2012), which adds the matching schemas to Current state in
How the gateway side names the same things:
The clash: our field is Proposed rename, keeping
Please advise:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Find the OpenAPI files =="
fd -p 'openapi.yaml|management-openapi.yaml'
echo
echo "== Outline current platform-api upstream-related schema names =="
rg -n -C2 '^\s{4}(Upstream|UpstreamDefinition|ReusableUpstream|UpstreamTimeout|RESTAPI):|upstreamDefinitions:|`#/components/schemas/`(Upstream|UpstreamDefinition|ReusableUpstream|UpstreamTimeout)\b' \
platform-api/src/resources/openapi.yaml || true
echo
echo "== Read the relevant platform-api slice =="
start=$(rg -n '^\s{4}RESTAPI:' platform-api/src/resources/openapi.yaml | cut -d: -f1 | head -n1)
end=$(rg -n '^\s{4}UpstreamTimeout:' platform-api/src/resources/openapi.yaml | cut -d: -f1 | head -n1)
if [[ -n "${start:-}" && -n "${end:-}" ]]; then
sed -n "$((start-10)),$((end+80))p" platform-api/src/resources/openapi.yaml | nl -ba
fi
echo
echo "== Current branch: all refs to UpstreamDefinition / ReusableUpstream in platform-api =="
rg -n -C2 '`#/components/schemas/`(UpstreamDefinition|ReusableUpstream)\b|^\s{4}(UpstreamDefinition|ReusableUpstream):' platform-api/src/resources/openapi.yaml || true
echo
echo "== Companion PR `#2012`: inspect gateway management-openapi upstream naming =="
gh pr diff 2012 --repo wso2/api-platform -- gateway/gateway-controller/api/management-openapi.yaml | \
rg -n -C3 'UpstreamDefinition|UpstreamReference|OperationUpstream|upstreamDefinitions|timeout|basePath|main:|sandbox:' || true
echo
echo "== Companion PR `#2012` metadata =="
gh pr view 2012 --repo wso2/api-platform --json number,title,headRefName,baseRefName,state,urlLength of output: 4416 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== platform-api: RESTAPI and upstream schema slice =="
sed -n '6318,8498p' platform-api/src/resources/openapi.yaml
echo
echo "== platform-api: precise refs/usages of UpstreamDefinition and ReusableUpstream =="
rg -n -C2 '`#/components/schemas/`(UpstreamDefinition|ReusableUpstream)\b|^\s{4}(UpstreamDefinition|ReusableUpstream):' \
platform-api/src/resources/openapi.yaml
echo
echo "== gateway companion PR `#2012`: files changed =="
gh pr view 2012 --repo wso2/api-platform --json files \
--jq '.files[].path'
echo
echo "== gateway companion PR `#2012`: upstream-related diff excerpt from management-openapi.yaml =="
gh pr diff 2012 --repo wso2/api-platform | \
sed -n '/gateway\/gateway-controller\/api\/management-openapi.yaml/,$p' | \
rg -n -C4 'UpstreamDefinition|UpstreamReference|OperationUpstream|upstreamDefinitions|`#/components/schemas/`' || true
echo
echo "== gateway main branch: current upstream naming in management-openapi.yaml =="
rg -n -C3 '^\s{4}(UpstreamDefinition|UpstreamReference|OperationUpstream|API|RESTAPI):|upstreamDefinitions:|`#/components/schemas/`(UpstreamDefinition|UpstreamReference|OperationUpstream)\b' \
gateway/gateway-controller/api/management-openapi.yaml || trueLength of output: 131926
Bottom line: I recommend making the rename now. It reduces local ambiguity and brings the control-plane terminology into line with the gateway model. One small follow-up suggestion: if you do this rename, also update the ✏️ Learnings added
|
|
@coderabbitai we're renaming the Our candidates:
Please (a) add any stronger options, then (b) rank ALL of them best-to-worst with a one-line rationale each. Judge by: cross-plane consistency with the gateway (which calls this |
|
Tip For best results, initiate chat on the files or code changes.
Additional candidates to consider: Ranking (best to worst):
Recommendation unchanged: ✏️ Learnings added
|
|
@coderabbitai for the agreed rename, please show a focused before/after of ONLY the affected schema blocks, not the full file: the |
3cb7216 to
f2e8961
Compare
|
@coderabbitai full review This is the control-plane (platform-api) side of per-operation upstream routing. The data-plane contract it must match is in PR #2012 (gateway-controller/pkg/config/api_validator.go and management-openapi.yaml). Please verify each of these and flag any divergence:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR 2155 files changed =="
gh pr view 2155 --repo wso2/api-platform --json files --jq '.files[].path'
echo
echo "== PR 2155 diff stat =="
gh pr diff 2155 --repo wso2/api-platform --stat
echo
echo "== Confirm no generated.go / validator / test files touched =="
gh pr diff 2155 --repo wso2/api-platform | rg -n '^diff --git' || trueLength of output: 1675 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Full diff PR 2155 (patch) =="
gh pr diff 2155 --repo wso2/api-platform --patch > /tmp/pr2155.diff
wc -l /tmp/pr2155.diff
echo
echo "== Show diff per file with line counts =="
rg -n '^diff --git' /tmp/pr2155.diff
echo
echo "== upstream.go full diff =="
awk '/^diff --git a\/platform-api\/src\/internal\/model\/upstream.go/,/^diff --git a\/platform-api\/src\/internal\/(model\/api|repository)/' /tmp/pr2155.diff | head -300
echo
echo "== utils/api.go diff =="
awk '/^diff --git a\/platform-api\/src\/internal\/utils\/api\.go/,/^diff --git a\/platform-api\/src\/internal\/utils\/api_test\.go/' /tmp/pr2155.diff | head -400Length of output: 18754 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== constants/error.go diff =="
sed -n '353,365p' /tmp/pr2155.diff
awk '/^diff --git a\/platform-api\/src\/internal\/constants\/error\.go/,/^diff --git a\/platform-api\/src\/internal\/dto\/api\.go/' /tmp/pr2155.diff
echo
echo "== dto/api.go diff =="
awk '/^diff --git a\/platform-api\/src\/internal\/dto\/api\.go/,/^diff --git a\/platform-api\/src\/internal\/handler\/api\.go/' /tmp/pr2155.diff
echo
echo "== handler/api.go diff =="
awk '/^diff --git a\/platform-api\/src\/internal\/handler\/api\.go/,/^diff --git a\/platform-api\/src\/internal\/model\/api\.go/' /tmp/pr2155.diff
echo
echo "== model/api.go diff =="
awk '/^diff --git a\/platform-api\/src\/internal\/model\/api\.go/,/^diff --git a\/platform-api\/src\/internal\/model\/upstream\.go/' /tmp/pr2155.diff
echo
echo "== service/api.go diff (validation logic) =="
awk '/^diff --git a\/platform-api\/src\/internal\/service\/api\.go/,/^diff --git a\/platform-api\/src\/internal\/service\/api_test\.go/' /tmp/pr2155.diffLength of output: 43390 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR 2012 files changed =="
gh pr view 2012 --repo wso2/api-platform --json files --jq '.files[].path' || true
echo
echo "== PR 2012 diff for api_validator.go =="
gh pr diff 2012 --repo wso2/api-platform --patch -- gateway/gateway-controller/pkg/config/api_validator.go > /tmp/pr2012_validator.diff || true
wc -l /tmp/pr2012_validator.diff
cat /tmp/pr2012_validator.diff | head -400
echo
echo "== generated.go diff (nil safety on OperationUpstream) =="
awk '/^diff --git a\/platform-api\/src\/api\/generated\.go/,/^diff --git a\/platform-api\/src\/internal\/constants\/error\.go/' /tmp/pr2155.diff | head -200
echo
echo "== repository/api_test.go and service/api_test.go and utils/api_test.go: search upstream-related test names =="
awk '/^diff --git a\/platform-api\/src\/internal\/repository\/api_test\.go/,/^diff --git a\/platform-api\/src\/internal\/service\/api\.go/' /tmp/pr2155.diff | rg -n '^\+func Test|^\+\s*t\.Run'
awk '/^diff --git a\/platform-api\/src\/internal\/service\/api_test\.go/,/^diff --git a\/platform-api\/src\/internal\/utils\/api\.go/' /tmp/pr2155.diff | rg -n '^\+func Test|^\+\s*\{name:|^\+\s*name:'
awk '/^diff --git a\/platform-api\/src\/internal\/utils\/api_test\.go/,0' /tmp/pr2155.diff | rg -n '^\+func Test|^\+\s*\{name:|^\+\s*name:'Length of output: 12110 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR 2012 head branch =="
head_ref=$(gh pr view 2012 --repo wso2/api-platform --json headRefName --jq .headRefName)
echo "$head_ref"
echo
echo "== Fetch api_validator.go content from PR2012 head =="
gh api repos/wso2/api-platform/contents/gateway/gateway-controller/pkg/config/api_validator.go?ref="$head_ref" --jq .content | base64 -d > /tmp/api_validator.go
wc -l /tmp/api_validator.go
rg -n 'func Validate|upstream|Upstream|weight|Weight|timeout|Timeout|regexp|MustCompile' /tmp/api_validator.go | head -100
echo
echo "== Fetch upstreamref.go content =="
gh api repos/wso2/api-platform/contents/gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go?ref="$head_ref" --jq .content | base64 -d > /tmp/upstreamref.go
cat /tmp/upstreamref.goLength of output: 1301 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Try git fetch branch directly =="
git fetch origin feat/per-op-upstream-gateway:refs/remotes/origin/feat/per-op-upstream-gateway 2>&1 | tail -20 || true
git show origin/feat/per-op-upstream-gateway:gateway/gateway-controller/pkg/config/api_validator.go > /tmp/api_validator.go 2>&1 || echo "FAILED git show validator"
git show origin/feat/per-op-upstream-gateway:gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go > /tmp/upstreamref.go 2>&1 || echo "FAILED git show upstreamref"
wc -l /tmp/api_validator.go /tmp/upstreamref.go 2>&1 || true
echo
echo "== grep upstream-related validation in api_validator.go =="
rg -n 'upstream|Upstream|weight|Weight|timeout|Timeout|regexp|MustCompile|func ' /tmp/api_validator.go 2>/dev/null | head -150
echo
echo "== upstreamref.go content =="
cat /tmp/upstreamref.go 2>/dev/nullLength of output: 1443 🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/2012/head:refs/remotes/origin/pr-2012 2>&1 | tail -20
git show origin/pr-2012:gateway/gateway-controller/pkg/config/api_validator.go > /tmp/api_validator.go 2>&1 || echo "FAILED validator"
git show origin/pr-2012:gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go > /tmp/upstreamref.go 2>&1 || echo "FAILED upstreamref"
wc -l /tmp/api_validator.go /tmp/upstreamref.go
echo
echo "== grep validation logic =="
rg -n 'upstream|Upstream|weight|Weight|timeout|Timeout|regexp|MustCompile|func ' /tmp/api_validator.go 2>/dev/null | head -200
echo
echo "== upstreamref.go =="
cat /tmp/upstreamref.go 2>/dev/nullLength of output: 13864 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '158,260p' /tmp/api_validator.goLength of output: 3206
1. Validation parity with the gateway validator — Matches closely.
2. Validation on create and merged update, surfaced as 400 — Confirmed.
3. Persistence round-trip — 4. Deployment YAML emission — Matches the gateway shape. 5. Nil-safety and generated.go — 6. Tests — Yes, they exercise the intended paths: repository round-trip, One open item from earlier in this thread: the agreed rename ( ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
platform-api/src/internal/utils/api_test.go (1)
672-679: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftApply the agreed upstream type rename.
These tests continue to encode
ReusableUpstream; rename the reusable pool entry toUpstreamDefinitionand reserveUpstreamEndpointfor direct endpoint shapes across the schema, generated code, models, and tests. As noted in the PR objectives, this is the agreed naming model.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/utils/api_test.go` around lines 672 - 679, Rename the reusable pool type represented by UpstreamDefinitions and its entries from ReusableUpstream to UpstreamDefinition across the schema, generated code, models, and tests. Reserve UpstreamEndpoint for direct endpoint shapes, updating all references and generated artifacts consistently while preserving the existing behavior and structure.platform-api/src/internal/handler/api.go (1)
162-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Warnfor expected 400-level validation failures.Invalid upstream definitions are client-input errors, not unexpected server failures. Reserve
Errorfor 5xx conditions and useWarnhere.Based on learnings, expected 4xx handler errors should use
slog.Warn, whileslog.Erroris reserved for unexpected server-side failures.Also applies to: 319-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/handler/api.go` around lines 162 - 163, Change the logging call for constants.ErrInvalidUpstreamDefinition in the handler to h.slogger.Warn, preserving the existing message and structured fields. Apply the same severity change to the corresponding handler branch around lines 319-320, while leaving unexpected server-side failures logged at Error.Source: Learnings
platform-api/src/internal/utils/api.go (1)
1539-1545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCanonicalize methods before constructing lookup keys.
Normalize the method once at the conversion boundary and use that canonical value throughout merging. The current inline normalization only affects the temporary key; the underlying operation can retain user-supplied casing.
As per coding guidelines, user-supplied HTTP methods must be uppercased before map-key creation and downstream route processing.
Also applies to: 1552-1554
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/utils/api.go` around lines 1539 - 1545, In the override-operation merge logic around byKey, uppercase each user-supplied op.Request.Method at the conversion boundary before constructing lookup keys, and use that canonical method for downstream route processing as well. Update the corresponding logic at the additional referenced range so the underlying operation no longer retains mixed-case methods while preserving the existing upstream lookup behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/src/internal/handler/api.go`:
- Around line 162-166: Sanitize client responses in the
invalid-upstream-definition branches, including the corresponding occurrences
near lines 319, 702, and 925: keep logging the detailed err server-side, but
replace err.Error() passed to utils.NewErrorResponse with the fixed message
“Invalid upstream definition”.
In `@platform-api/src/internal/service/api_test.go`:
- Around line 844-846: Update the schema fixture setup around runtime.Caller and
schemaPath to resolve the path to an absolute, cleaned value, then verify it
remains within the intended test root before calling os.ReadFile. Reject or fail
the test for paths outside that root while preserving the existing
schema-loading behavior for valid paths.
In `@platform-api/src/internal/service/api.go`:
- Around line 694-701: Update resolveUpstreamRef to reject whitespace-padded
references or consistently normalize the reference before it is persisted and
emitted; ensure validation, resolution, and deployment use the same canonical
value rather than only validating strings.TrimSpace(ref).
- Around line 723-731: Update both URL validation paths around url.Parse to
validate parsed.Hostname() rather than only parsed.Host, rejecting empty
hostnames such as http://:8080. Preserve the existing http/https scheme
requirement, and only accept a port when a non-empty hostname is present.
---
Nitpick comments:
In `@platform-api/src/internal/handler/api.go`:
- Around line 162-163: Change the logging call for
constants.ErrInvalidUpstreamDefinition in the handler to h.slogger.Warn,
preserving the existing message and structured fields. Apply the same severity
change to the corresponding handler branch around lines 319-320, while leaving
unexpected server-side failures logged at Error.
In `@platform-api/src/internal/utils/api_test.go`:
- Around line 672-679: Rename the reusable pool type represented by
UpstreamDefinitions and its entries from ReusableUpstream to UpstreamDefinition
across the schema, generated code, models, and tests. Reserve UpstreamEndpoint
for direct endpoint shapes, updating all references and generated artifacts
consistently while preserving the existing behavior and structure.
In `@platform-api/src/internal/utils/api.go`:
- Around line 1539-1545: In the override-operation merge logic around byKey,
uppercase each user-supplied op.Request.Method at the conversion boundary before
constructing lookup keys, and use that canonical method for downstream route
processing as well. Update the corresponding logic at the additional referenced
range so the underlying operation no longer retains mixed-case methods while
preserving the existing upstream lookup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9aa52fdf-c292-4869-ad6a-295d40587ab3
📒 Files selected for processing (12)
platform-api/src/api/generated.goplatform-api/src/internal/constants/error.goplatform-api/src/internal/dto/api.goplatform-api/src/internal/handler/api.goplatform-api/src/internal/model/api.goplatform-api/src/internal/model/upstream.goplatform-api/src/internal/repository/api_test.goplatform-api/src/internal/service/api.goplatform-api/src/internal/service/api_test.goplatform-api/src/internal/utils/api.goplatform-api/src/internal/utils/api_test.goplatform-api/src/resources/openapi.yaml
✅ Files skipped from review due to trivial changes (1)
- platform-api/src/api/generated.go
|
@coderabbitai we've decided to keep the pool entry schema named ReusableUpstream and are not renaming it to UpstreamDefinition. My earlier comment that called the rename "agreed" was premature; on reflection we are not doing it in this PR. Reasoning: adopting UpstreamDefinition for the pool would also force renaming the pre-existing UpstreamDefinition endpoint schema (the url/ref endpoint under Upstream.main/sandbox) to UpstreamEndpoint, which churns a large amount of pre-existing code (dto, model, service, utils, and their tests) that is unrelated to this feature. This PR is scoped to adding the upstreamDefinitions pool and per-operation upstream overrides, and we want to keep that scope tight. The schema shapes already match the gateway contract in #2012; only the pool entry name differs, and ReusableUpstream reads clearly for a reusable named entry in the pool. Please note this decision and do not re-flag the ReusableUpstream -> UpstreamDefinition rename in future reviews of this PR. If the maintainers prefer the rename, we will do it as a separate follow-up. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
13d47a5 to
47ed679
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="wso2/api-platform"
git fetch origin \
pull/2012/head:refs/remotes/origin/pr-2012 \
main:refs/remotes/origin/main >/dev/null 2>&1 || true
for ref in origin/pr-2012 origin/main; do
echo "===== $ref: candidate files ====="
git ls-tree -r --name-only "$ref" -- gateway/gateway-controller |
rg '(api_validator\.go|restapi\.go|translator\.go|resilience|cluster|deploy|config)' || true
echo
echo "===== $ref: all connect-timeout / timeout parsing references (with paths/lines) ====="
git grep -n -i -E \
'connect.?timeout|timeout\.connect|ConnectTimeout|ParseDuration|Parse.*Duration|ResilienceDuration|createWeightedCluster|UpstreamCluster|Route.*Timeout|timeout:' \
"$ref" -- gateway/gateway-controller || true
echo
done
echo "===== PR `#2012`: map likely files before targeted reads ====="
for f in \
gateway/gateway-controller/pkg/config/api_validator.go \
gateway/gateway-controller/pkg/transform/restapi.go \
gateway/gateway-controller/pkg/xds/translator.go
do
echo "--- $f"
git show "origin/pr-2012:$f" > "/tmp/$(basename "$f")"
ast-grep outline "/tmp/$(basename "$f")" --items all 2>/dev/null | head -300 || true
done
echo "===== PR `#2012`: numbered context for all relevant symbols ====="
for f in /tmp/api_validator.go /tmp/restapi.go /tmp/translator.go; do
echo "--- $f"
rg -n -i -C5 \
'connect.?timeout|timeout\.connect|ConnectTimeout|ParseDuration|ResilienceDuration|createWeightedCluster|UpstreamCluster|validate.*[Uu]pstream|validate.*[Tt]imeout|resilience' \
"$f" || true
done
echo "===== Find checked-in Envoy proto/validation evidence, if any ====="
git ls-tree -r --name-only origin/pr-2012 |
rg -i '(cluster.*\.proto$|cds.*\.proto$|envoy.*proto|validate.*proto|go-control-plane)' | head -100 || true
git grep -n -i -E 'connect_timeout.*(greater|gt|required|min|zero)|ConnectTimeout.*(greater|gt|required|min|zero)' \
origin/pr-2012 -- ':!vendor' ':!node_modules' || trueLength of output: 212788 🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/2012/head:refs/remotes/origin/pr-2012 main:refs/remotes/origin/main >/dev/null 2>&1 || true
echo "===== Exact PR `#2012` numbered source slices ====="
for spec in \
"gateway/gateway-controller/pkg/config/api_validator.go:400:430" \
"gateway/gateway-controller/pkg/transform/restapi.go:84:115" \
"gateway/gateway-controller/pkg/transform/restapi.go:285:345" \
"gateway/gateway-controller/pkg/transform/restapi.go:529:585" \
"gateway/gateway-controller/pkg/transform/restapi.go:649:670" \
"gateway/gateway-controller/pkg/xds/translator.go:521:615" \
"gateway/gateway-controller/pkg/xds/translator.go:625:700" \
"gateway/gateway-controller/pkg/xds/translator.go:2969:3065" \
"gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go:1:100" \
"gateway/gateway-controller/pkg/models/runtime_deploy_config.go:75:135" \
"gateway/gateway-controller/pkg/config/validator_test.go:805:880" \
"gateway/gateway-controller/pkg/utils/upstreamref/upstreamref_test.go:145:175" \
"gateway/gateway-controller/pkg/transform/restapi_test.go:1270:1340" \
"gateway/gateway-controller/pkg/xds/translator_test.go:2490:2545"
do
path="${spec%%:*}"
rest="${spec#*:}"
start="${rest%%:*}"
end="${rest##*:}"
echo "--- $path (Lines $start-$end)"
git show "origin/pr-2012:$path" | nl -ba | sed -n "${start},${end}p"
done
echo
echo "===== Dispatch order: transformer path versus legacy fallback ====="
git show origin/pr-2012:gateway/gateway-controller/pkg/xds/translator.go |
nl -ba | sed -n '625,940p'
echo
echo "===== Same relevant functions on main (for divergence check) ====="
for spec in \
"gateway/gateway-controller/pkg/transform/restapi.go:550:590" \
"gateway/gateway-controller/pkg/xds/translator.go:1788:1825" \
"gateway/gateway-controller/pkg/xds/translator.go:2890:3000" \
"gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go:45:85"
do
path="${spec%%:*}"
rest="${spec#*:}"
start="${rest%%:*}"
end="${rest##*:}"
echo "--- $path (Lines $start-$end)"
git show "origin/main:$path" | nl -ba | sed -n "${start},${end}p"
done
echo
echo "===== Envoy Go dependency version ====="
for f in go.mod gateway/gateway-controller/go.mod; do
if git cat-file -e "origin/pr-2012:$f" 2>/dev/null; then
echo "--- $f"
git show "origin/pr-2012:$f" | rg -n 'go-control-plane|envoy' || true
fi
doneLength of output: 2873 🤖 web_search executed:
Length of output: 10441 🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/2012/head:refs/remotes/origin/pr-2012 main:refs/remotes/origin/main >/dev/null 2>&1 || true
show_slice() {
local ref="$1" path="$2" start="$3" end="$4"
echo "--- $ref:$path (Lines $start-$end)"
git show "$ref:$path" | cat -n | sed -n "${start},${end}p"
}
echo "===== PR `#2012`: validation and primary RestAPITransformer flow ====="
show_slice origin/pr-2012 gateway/gateway-controller/pkg/config/api_validator.go 409 429
show_slice origin/pr-2012 gateway/gateway-controller/pkg/transform/restapi.go 84 110
show_slice origin/pr-2012 gateway/gateway-controller/pkg/transform/restapi.go 285 345
show_slice origin/pr-2012 gateway/gateway-controller/pkg/transform/restapi.go 529 585
show_slice origin/pr-2012 gateway/gateway-controller/pkg/transform/restapi.go 649 669
echo "===== PR `#2012`: RuntimeDeployConfig -> Envoy cluster emission ====="
show_slice origin/pr-2012 gateway/gateway-controller/pkg/models/runtime_deploy_config.go 82 130
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator.go 236 265
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator.go 521 615
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator.go 625 745
echo "===== PR `#2012`: legacy xDS path, shared connect parser, and resilience parser ====="
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator.go 940 970
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator.go 1112 1161
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator.go 1896 1925
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator.go 2969 3065
show_slice origin/pr-2012 gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go 45 85
echo "===== PR `#2012`: tests that pin zero behavior ====="
show_slice origin/pr-2012 gateway/gateway-controller/pkg/config/validator_test.go 819 875
show_slice origin/pr-2012 gateway/gateway-controller/pkg/utils/upstreamref/upstreamref_test.go 145 175
show_slice origin/pr-2012 gateway/gateway-controller/pkg/transform/restapi_test.go 1277 1335
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator_test.go 2423 2458
show_slice origin/pr-2012 gateway/gateway-controller/pkg/xds/translator_test.go 2503 2542
echo "===== Confirm `main` keeps the primary rejection ====="
show_slice origin/main gateway/gateway-controller/pkg/transform/restapi.go 559 580
show_slice origin/main gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go 45 85Length of output: 48117
ConclusionA definition with Therefore, platform-api rejecting 1. End-to-end trace for
|
| Setting | Parser | "0s" behavior |
|---|---|---|
upstreamDefinitions[].timeout.connect |
definitionConnectTimeout in the primary path; upstreamref.ParseConnectTimeout in legacy fallback |
Rejected as non-positive |
resilience.timeout / resilience.idleTimeout |
parseDurationAllowZero, called by ResolveResilience |
Accepted as explicit disablement |
The resilience path is explicit:
// gateway/gateway-controller/pkg/xds/translator.go
// Lines 2979–3005
func parseDurationAllowZero(timeoutStr *string) (*time.Duration, error) {
...
duration, err := time.ParseDuration(s)
...
return &duration, nil
}
// "0s" yields a non-nil zero duration (meaning "explicitly disabled").
func ResolveResilience(...) ...TestParseDurationAllowZero_MatchesCRDPattern includes "0s" and "0ms" among accepted inputs (Lines 2425–2441).
That zero then reaches Envoy RouteAction.Timeout through routeTimeoutOrDefault; the route test explicitly verifies that a zero route timeout disables it. This is appropriate for route timeouts, but it is not the behavior for cluster connection timeouts.
4. Envoy constraint
Envoy’s v3 Cluster.connect_timeout is a google.protobuf.Duration with validation:
google.protobuf.Duration connect_timeout = 4 [
(validate.rules).duration = {gt {}}
];So an explicitly configured value must be strictly greater than zero. Envoy uses a default of 5 seconds only when the field is not set; a present zero duration is not equivalent to omission and violates the constraint.
Source: Envoy cluster.proto.
This matches the gateway’s transformer/shared-parser requirement for positive definition connect timeouts.
5. Practical result and platform-api parity
A gateway deployment containing a definition with:
timeout:
connect: 0sdoes not produce an Envoy cluster:
- Gateway management validation admits it.
- The primary
RestAPITransformerrejects it before cluster construction. TranslateConfigstries the legacy fallback.- The legacy parser rejects it as non-positive too.
TranslateConfigslogs the translation error and excludes that API’s routes/clusters from the generated snapshot.
So platform-api rejecting "0s" at create and update is consistent with the gateway’s effective deployment behavior and with Envoy’s strict-positive Cluster.connect_timeout requirement.
The actual gateway inconsistency is narrower: its management validator and TestValidateUpstreamDefinitions_ZeroConnectTimeout currently advertise "0s" as valid even though both deployment paths reject it. The gateway validator/test should be changed to reject zero for upstreamDefinitions[].timeout.connect; route resilience should continue accepting zero because it has different semantics.
I’m withdrawing my earlier recommendation that platform-api should accept "0s".
✏️ Learnings added
Learnt from: mehara-rothila
URL: https://github.com/wso2/api-platform/pull/2155
Timestamp: 2026-07-22T17:35:16.166Z
Learning: For upstream definition connection timeouts in wso2/api-platform gateway-controller PR `#2012` and main, `timeout.connect: "0s"` is accepted by `gateway/gateway-controller/pkg/config/api_validator.go::validateUpstreamDefinitionsList`, but deployment rejects it in both paths: the primary `gateway/gateway-controller/pkg/transform/restapi.go::definitionConnectTimeout` rejects `d <= 0`, and legacy xDS translation reaches `gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go::ParseConnectTimeout`, which also rejects `d <= 0`. Envoy v3 `Cluster.connect_timeout` has a strict-positive duration constraint. Platform-api should reject non-positive upstream definition connect timeouts to match actual gateway deployment behavior; gateway admission validation/tests accepting zero are inconsistent and should be fixed separately. Route resilience timeouts are different: `gateway/gateway-controller/pkg/xds/translator.go::parseDurationAllowZero` intentionally accepts `"0s"` as an explicit disabled `RouteAction` timeout.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Pull request overview
Adds control-plane support in platform-api for reusable upstream definition pools and per-operation upstream overrides for REST APIs, including OpenAPI/schema updates, model/DTO mapping, deployment YAML emission, and create/update-time validation to prevent dangling refs.
Changes:
- Extend the Platform API OpenAPI contract to support
upstreamDefinitionsplus per-operationupstreamref overrides and a sharedUpstreamReferenceschema. - Add service-layer validation (
validateUpstreamRefs) on create and merged-update to enforce name/url/basePath/weight/timeout constraints and ref resolution. - Update model/util/DTO conversions and add unit + integration tests covering persistence and YAML/HTTP round-trips.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| platform-api/resources/openapi.yaml | Adds schemas and fields for reusable upstream definitions and per-operation upstream refs. |
| platform-api/internal/utils/api.go | Maps upstreamDefinitions + per-op upstream between API/model and emits them into deployment YAML. |
| platform-api/internal/utils/api_upstream_test.go | Unit tests for deployment YAML emission and model/API round-trips for new upstream fields. |
| platform-api/internal/service/api.go | Adds create + update merged-config validation for upstreamDefinitions and ref resolution. |
| platform-api/internal/service/api_test.go | Adjusts existing service tests to use a now-valid upstream on create. |
| platform-api/internal/service/upstream_validation_test.go | Adds unit + SQLite-backed integration tests for upstream validation and update semantics. |
| platform-api/internal/repository/api_upstream_test.go | Verifies upstreamDefinitions and per-op upstream survive DB JSON-blob round trip. |
| platform-api/internal/model/upstream.go | Introduces internal model types for reusable upstreams, targets, timeouts, and per-op upstream refs. |
| platform-api/internal/model/api.go | Extends RestAPIConfig and OperationRequest to carry upstreamDefinitions and per-op upstream. |
| platform-api/internal/handler/api_upstream_integration_test.go | Handler-level tests asserting HTTP 400 mappings and GET/POST/PUT JSON round-trip for new fields. |
| platform-api/internal/dto/api.go | Extends deployment YAML DTO to include upstreamDefinitions pool + timeout/backends. |
| platform-api/api/generated.go | Regenerates API types to include upstreamDefinitions, per-op upstream, and UpstreamReference. |
Files not reviewed (1)
- platform-api/api/generated.go: Generated file
| if definition.Timeout != nil && definition.Timeout.Connect != nil { | ||
| m.Timeout = &model.UpstreamTimeout{Connect: *definition.Timeout.Connect} | ||
| } |
| properties: | ||
| connect: | ||
| type: string | ||
| description: Connection timeout duration (e.g., "5s", "500ms") |
Purpose
Add control-plane support for reusable upstream definitions and per-operation upstream routing on REST APIs, so individual operations of an API can route to different backends. This is the platform-api counterpart of the gateway-side change in PR #2012 (per-operation upstream routing in the gateway controller), which this PR pairs with: the platform stores, validates, and emits the configuration; the gateway enforces and routes it.
Approach
RESTAPI(and the create/update requests) gain an optionalupstreamDefinitionspool of named, reusable upstreams (name,basePath,timeout.connect, weightedupstreams[]), and each operation gains an optionalupstreamoverride whosemain/sandboxreference a pool entry by name (refonly, no inline URLs). The API-levelupstream.main/upstream.sandboxbecome a url-or-ref union. Field names and constraints mirror the gateway's management OpenAPI contract exactly.rest_apis.configurationJSON blob.validateUpstreamRefs, run on create and on the merged config on update): definition names must be unique and match the sharedUpstreamReferencecontract (1-100 chars,^[a-zA-Z0-9\-_]+$); definition URLs must be http/https host[:port] only (no path, query, or fragment);basePathmust start with/and must not end with/(same regex as the gateway validator; empty is treated as unset sinceomitemptydrops it from the deployment YAML); weights must be 0-100;timeout.connectmust be a single unsigned ms/s/m/h unit and positive; every API-level and per-operation ref must resolve to a declared definition; the API-level union must set exactly one of url or ref. Violations map to HTTP 400.Example
A REST API with a reusable pool, an API-level default upstream (url-or-ref), and one operation overridden to a pool entry:
Validation parity with the gateway
Independent cross-checks of this branch against the gateway branch confirmed the accept/reject sets match for names, refs, URLs, weights, basePath, and timeout format, with two deliberate, safe differences: the platform rejects non-positive connect timeouts such as
0s(the gateway's management validation accepts them but its translation layer then fails, so the platform enforces the effective contract), and the platform rejects an API-level upstream with neither url nor ref (previously accepted and then failed at deploy). A known gateway-internal item (weighted targets applying only on one translation path) is pre-existing on main and tracked separately.Testing
validateUpstreamRefscovers names, duplicates, URLs (path/query/fragment/scheme/host), weights (0, 100, out-of-range), basePath (leading/trailing slash, bare/, space, query, empty-as-unset), timeout formats, and ref resolution for API-level and per-operation targets.Note on strictness
The service now rejects an API-level upstream with neither
urlnorrefon create and on the merged configuration of updates, closing a previously accepted configuration that could only fail at deploy time. Practical HTTP behavior is unchanged, since the handlers already required at least one upstream endpoint on both create and update.