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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
|
… gateway contract Mirrors the gateway validator's basePath rule (must start with '/', must not end with '/'; empty treated as unset) in validateUpstreamRefs and the published OpenAPI pattern, so a stored API can no longer fail at deploy time. Adds handler integration tests pinning the HTTP 400 mapping for invalid upstream configuration on create and update.
The previous wording claimed an omitted weight defaults to 100. An omitted weight is not serialized, and the gateway's load balancer applies its own default endpoint weight, so the description now states that instead of a fixed value.
47ed679 to
a932f98
Compare
|
@coderabbitai Please review the rebased changes and additionally verify cross-PR consistency with PR #2012, which implements the gateway-side counterpart of this feature (per-operation upstream routing in the gateway controller). Specifically, check whether the validation rules and wire format in this PR match the gateway contract on that PR's head:
Please report any mismatch you find between the two sides, and any issues in this PR's own changes (including the error handling migration to apperror.ValidationFailed and the new handler integration tests). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Refresh PR 2155 diff (rebased) =="
gh pr diff 2155 --repo wso2/api-platform --patch > /tmp/pr2155_new.diff
wc -l /tmp/pr2155_new.diff
rg -n '^diff --git' /tmp/pr2155_new.diff
echo
echo "== Search for apperror.ValidationFailed migration =="
rg -n 'apperror\.ValidationFailed|ErrInvalidUpstreamDefinition' /tmp/pr2155_new.diff
echo
echo "== handler/api.go diff (post-rebase) =="
awk '/^diff --git a\/platform-api\/src\/internal\/handler\/api\.go/,/^diff --git a\/platform-api\/src\/internal\/handler\//{if(!/^diff --git a\/platform-api\/src\/internal\/handler\/api\.go/ && /^diff --git/) exit; print}' /tmp/pr2155_new.diff | head -300
echo
echo "== new handler integration test files =="
rg -n '^diff --git.*handler.*_test' /tmp/pr2155_new.diffLength of output: 3644 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== service/api.go full diff (validation logic incl xor, ref resolution) =="
sed -n '379,644p' /tmp/pr2155_new.diff
echo
echo "== upstream_validation_test.go diff (name pattern/length, basePath, timeout, weight) =="
sed -n '811,1292p' /tmp/pr2155_new.diff | head -300
echo
echo "== openapi.yaml diff =="
sed -n '1765,1905p' /tmp/pr2155_new.diff
sed -n '2179,2229p' /tmp/pr2155_new.diff
echo
echo "== generated.go diff excerpt =="
sed -n '24,134p' /tmp/pr2155_new.diff
sed -n '2204,2217p' /tmp/pr2155_new.diff
echo
echo "== handler integration test =="
sed -n '1905,2096p' /tmp/pr2155_new.diffLength of output: 24642 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check basePath validation in service/api.go (this PR) =="
rg -n 'basePath|BasePath' /tmp/pr2155_new.diff
echo
echo "== openapi.yaml diff full =="
sed -n '1765,1905p' /tmp/pr2155_new.diff
echo
echo "== check for additional openapi.yaml hunks =="
sed -n '2179,2229p' /tmp/pr2155_new.diff
echo
echo "== gateway PR 2012 head: basePath validation in api_validator.go =="
rg -n -i 'basepath' /tmp/api_validator.go /tmp/upstreamref.go 2>/dev/null || true
echo
echo "== refresh PR2012 head files (already fetched earlier) - re-check management-openapi.yaml for basePath pattern =="
git show origin/pr-2012:gateway/gateway-controller/api/management-openapi.yaml > /tmp/mgmt2012.yaml 2>&1 || echo "fetch failed"
rg -n -i -B3 -A10 'basePath' /tmp/mgmt2012.yaml || trueLength of output: 14501 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Refetch PR 2012 head branch =="
git fetch origin pull/2012/head:refs/remotes/origin/pr-2012 2>&1 | tail -5 || true
git show origin/pr-2012:gateway/gateway-controller/api/management-openapi.yaml > /tmp/mgmt2012.yaml 2>&1 && echo OK || echo FAILED
echo
echo "== basePath pattern & rule on gateway side =="
rg -n -i -B3 -A15 'basepath' /tmp/mgmt2012.yaml || true
echo
echo "== gateway api_validator.go basePath rule (refetch) =="
git show origin/pr-2012:gateway/gateway-controller/pkg/config/api_validator.go > /tmp/api_validator2.go 2>&1 && echo OK || echo FAILED
rg -n -i -B3 -A15 'basepath' /tmp/api_validator2.go || true
echo
echo "== gateway deployment YAML field names (upstreamDefinitions, upstream.main/sandbox.ref casing) =="
rg -n -i 'upstreamDefinitions|yaml:"' /tmp/api_validator2.go | head -50
echo
echo "== find gateway deployment spec parsing struct (dto/model) =="
git ls-tree -r origin/pr-2012 --name-only | rg -i 'deploy|upstream' | grep -v _testLength of output: 17158 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== platform-api dto/upstream.go (deployment YAML DTO) =="
git show HEAD:platform-api/internal/dto/upstream.go 2>/dev/null || find_path=$(fd -HI 'upstream.go' platform-api/internal/dto) && cat "$find_path" 2>/dev/null
fd -HI 'upstream.go' platform-api/internal/dto
echo
echo "== show dto/upstream.go content =="
cat platform-api/internal/dto/upstream.go 2>/dev/null || echo "not found at that path"
echo
echo "== gateway models/deployment.go upstream-related yaml tags =="
git show origin/pr-2012:gateway/gateway-controller/pkg/models/deployment.go > /tmp/gw_deployment.go 2>&1 && echo OK
rg -n -i -B2 -A2 'upstream|yaml:"' /tmp/gw_deployment.go | rg -i -A2 -B2 'upstream'
echo
echo "== full deployment.go upstream struct =="
awk '/UpstreamDefinition|type Upstream|OperationUpstream/{f=1} f{print; if(/^}/ && f) c++; if(c==1){print "---"; f=0; c=0}}' /tmp/gw_deployment.go | head -150Length of output: 2866 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== platform-api dto/api.go relevant structs =="
rg -n -A15 'type UpstreamDefinitionYAML|type UpstreamBackendYAML|type UpstreamTimeoutYAML|UpstreamDefinitions \[\]UpstreamDefinitionYAML|Upstream ' platform-api/internal/dto/api.go
echo
echo "== gateway deployment.go (retry fetch) =="
git show origin/pr-2012:gateway/gateway-controller/pkg/models/deployment.go > /tmp/gw_deployment.go 2>&1 && echo OK || echo FAILED
wc -l /tmp/gw_deployment.go
rg -n -i 'upstream' /tmp/gw_deployment.goLength of output: 3643 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== find gateway CRD/api types file with UpstreamDefinition struct (yaml tags) =="
git ls-tree -r origin/pr-2012 --name-only -- gateway | rg -i 'api\.go|types' | head -30
echo
echo "== search for 'type UpstreamDefinition struct' across pr-2012 tree =="
git grep -n 'type UpstreamDefinition struct' origin/pr-2012 -- gateway | head -5
git grep -n 'type OperationUpstream struct' origin/pr-2012 -- gateway | head -5
git grep -n 'type Upstream struct' origin/pr-2012 -- gateway | head -5Length of output: 1965 🏁 Script executed: #!/bin/bash
set -euo pipefail
git show origin/pr-2012:gateway/gateway-controller/pkg/api/management/generated.go > /tmp/gw_generated.go
sed -n '1355,1420p' /tmp/gw_generated.go
echo "---"
sed -n '1695,1780p' /tmp/gw_generated.goLength of output: 7700 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== find apperror.ValidationFailed -> HTTP mapping =="
rg -n 'ValidationFailed' platform-api/internal/apperror/*.go platform-api/internal/handler/*.go 2>/dev/null | head -40
echo
echo "== handler integration test content =="
sed -n '1905,2100p' /tmp/pr2155_new.diff
echo
echo "== check old ErrInvalidUpstreamDefinition still referenced anywhere (leftover) =="
rg -n 'ErrInvalidUpstreamDefinition' platform-api/internal --type goLength of output: 14254 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== constants/error.go current state =="
rg -n 'ErrInvalidUpstreamDefinition' platform-api/internal/constants/error.go || echo "removed (good)"
echo
echo "== handler/api.go: check no leftover manual error mapping for old constant =="
rg -n 'ErrInvalidUpstreamDefinition|invalid upstream definition' platform-api/internal/handler/api.go || echo "none found"
echo
echo "== confirm central apperror.ValidationFailed -> 400 mapping exists =="
rg -n 'ValidationFailed' platform-api/internal/apperror/*.go | head -20
fd -HI 'apperror' platform-api/internal -t d
rg -n 'StatusBadRequest|400' platform-api/internal/apperror/*.go | head -20Length of output: 6426
1.
2. API-level 3. 4. Deployment YAML field names/casing — match. I compared One real mismatch found — 5. 6. New handler integration tests — good coverage.
These exercise the full stack rather than just the service-layer unit tests, which is a solid addition. Bottom line: validation rules, URL/ref unions, ref resolution, and deployment YAML wire format are all consistent with ✏️ Learnings added
|
|
@coderabbitai Thank you for the thorough cross-check. On the weight description, I believe the alignment should go in the other direction: the gateway-side text is the stale one. On the current gateway head, the primary translation path applies weights. RestAPITransformer builds an endpoint for every target in a definition, carrying each weight, into the RuntimeDeployConfig (gateway-controller/pkg/transform/restapi.go, the endpoints loop), and the translator emits weighted clusters via createWeightedCluster (gateway-controller/pkg/xds/translator.go), which sets LoadBalancingWeight for weights greater than zero. TranslateConfigs uses this transformer path first for the RestApi kind; the legacy path that reads only the first target runs only as a fallback. We also verified this empirically on a gateway built from PR #2012's head: a per-operation ref to a pool weighted 80/20 distributed 30 requests exactly 24/6 across the two targets. The "Reserved for future multi-target load balancing; not applied yet" description in the gateway schema predates createWeightedCluster and is not part of PR #2012's diff, so this PR's wording reflects current behavior, and the gateway description should be refreshed in a separate follow-up instead. "Only the first target is used" remains true only for the legacy fallback path and the API-level slot cluster, which is tracked as a known follow-up item. I will fix the leftover comment reference to the removed sentinel in api_upstream_integration_test.go, thank you for catching it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/2012/head:refs/remotes/origin/pr-2012 2>&1 | tail -5 || true
echo "== transform/restapi.go =="
git show origin/pr-2012:gateway/gateway-controller/pkg/transform/restapi.go > /tmp/restapi_transform.go 2>&1 && echo OK || echo FAILED
rg -n -i 'weight|endpoint|upstream' /tmp/restapi_transform.go | head -80
echo
echo "== xds/translator.go: createWeightedCluster =="
git show origin/pr-2012:gateway/gateway-controller/pkg/xds/translator.go > /tmp/xds_translator.go 2>&1 && echo OK || echo FAILED
rg -n -i 'createWeightedCluster|LoadBalancingWeight' /tmp/xds_translator.go
echo
echo "== createWeightedCluster function body =="
rg -n -A30 'func.*createWeightedCluster' /tmp/xds_translator.go
echo
echo "== TranslateConfigs: confirm order of transformer vs legacy path for RestApi kind =="
rg -n -i 'func TranslateConfigs' -A60 /tmp/xds_translator.go | head -100
rg -rn 'TranslateConfigs' gateway/gateway-controller/pkg 2>/dev/null | head -5 || true
git ls-tree -r origin/pr-2012 --name-only -- gateway | rg -i 'xds'Length of output: 8873
In for _, up := range def.Upstreams {
...
ep := models.Endpoint{Host: parsedURL.Hostname(), Port: port}
if up.Weight != nil {
ep.Weight = up.Weight
}
endpoints = append(endpoints, ep)
}
...
rdc.UpstreamClusters[defClusterKey] = &models.UpstreamCluster{
...
Endpoints: endpoints,
...
}And in for i, ep := range endpoints {
lb := &endpoint.LbEndpoint{ ... }
if ep.Weight != nil && *ep.Weight > 0 {
lb.LoadBalancingWeight = &wrapperspb.UInt32Value{Value: uint32(*ep.Weight)}
}
...
}That's consistent with your empirical 24/6 result for an 80/20 split, and confirms weights across Noted on the (๑˘◡˘๑) ✏️ Learnings added
|
Checks the PRAGMA foreign_keys errors in both new test setups, corrects the service test env comment that overstated production wiring, condenses the UpstreamDefinitionYAML doc comment to sibling density, and widens the validateUpstreamRefs call-site comment to cover the pool and union checks it performs.
2 participants
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.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). Known gateway-internal items (weighted targets applying only on one translation path, definition connect timeout not applied on the runtime-deployment path) are 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.