Add per-operation upstream override for REST API operations#2012
Add per-operation upstream override for REST API operations#2012mehara-rothila wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-operation main and sandbox upstream references with schema support, validation, shared upstream lookup, deterministic cluster naming, REST/XDS routing integration, timeout propagation, persistence coverage, runtime regressions, and end-to-end scenarios. ChangesPer-Operation Upstream Routing
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant APIValidator
participant RESTTransformer
participant XDSTranslator
participant UpstreamDefinition
Client->>APIValidator: Submit operation upstream refs
APIValidator->>UpstreamDefinition: Resolve referenced definitions
UpstreamDefinition-->>APIValidator: Definition and timeout
APIValidator-->>RESTTransformer: Validated operation configuration
RESTTransformer->>XDSTranslator: Route override, cluster, and basePath
XDSTranslator->>UpstreamDefinition: Resolve cluster and timeout
XDSTranslator-->>Client: Main or sandbox route configuration
🚥 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 |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@gateway/gateway-controller/api/management-openapi.yaml`:
- Around line 4139-4148: OperationUpstream currently allows an empty object
which is invalid; update the OperationUpstream schema (the OperationUpstream
object that defines properties main and sandbox) to require at least one of
those properties by adding an anyOf clause such as anyOf: - required: [main] -
required: [sandbox] (or an equivalent oneOf/anyOf expression) so the schema
enforces presence of main or sandbox and prevents {} being valid.
In `@gateway/gateway-controller/pkg/policy/builder.go`:
- Around line 185-186: The current condition treats any non-nil
op.Upstream.Sandbox as active; change it to perform a content-based check like
apiSandboxHasContent so empty sandbox objects are ignored. Introduce/compute a
per-op predicate (e.g., perOpSandboxHasContent) that mirrors the api-level
sandbox content test and use if apiSandboxHasContent || (op.Upstream != nil &&
perOpSandboxHasContent) to decide whether to append effectiveSandboxVHost.
In `@gateway/gateway-controller/pkg/xds/translator_test.go`:
- Around line 2061-2074: Update TestResolvePerOpUpstream_DedupSameURL to ensure
URL-stability by creating two distinct api.Upstream instances with different Url
values (e.g., "http://shared-svc:8080" and "http://shared-svc:8081") but the
same apiID, method, path and env, then call translator.resolvePerOpUpstream for
both and assert the returned cluster names are equal; keep the test name and use
the Translator.resolvePerOpUpstream and translator variable references so the
test still targets the same logic and verifies that cluster key derivation does
not depend on the exact URL string.
🪄 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: 4012d4ee-58df-4773-b82e-fa11e65290d8
📒 Files selected for processing (16)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/validator_test.gogateway/gateway-controller/pkg/models/runtime_deploy_config.gogateway/gateway-controller/pkg/policy/builder.gogateway/gateway-controller/pkg/policy/builder_test.gogateway/gateway-controller/pkg/transform/restapi.gogateway/gateway-controller/pkg/transform/restapi_test.gogateway/gateway-controller/pkg/utils/commonutils.gogateway/gateway-controller/pkg/utils/commonutils_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.gogateway/it/features/per-op-upstream-basic.featuregateway/it/features/per-op-upstream-ref.featuregateway/it/features/per-op-upstream-validation.feature
- OperationUpstream schema now requires at least one of main or sandbox
via anyOf, rejecting empty {} at the schema layer as defense in depth
alongside the validator.
- Policy builder per-op sandbox check is now content-based (trims and
checks Url/Ref non-empty) matching the apiSandboxHasContent pattern
used elsewhere. Fixes asymmetric pointer-only check that would emit
sandbox policy chains for empty sandbox wrappers.
- TestResolvePerOpUpstream_DedupSameURL now uses two distinct URLs with
identical apiID|METHOD|path|env, asserting cluster name independence
from URL. Pins the EDS-stable design contract.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/policyxds/policyxds_test.go (1)
120-120: 💤 Low valueConsider using a hash-like cluster key suffix for test realism.
The cluster key
"main_fixture"has the correctmain_prefix for an API-level main environment cluster, but the suffixfixturedoesn't reflect the production naming pattern introduced in this PR. According to the PR objectives, API-level clusters follow the pattern<env>_<sha256(apiID|env)[:8]>(e.g.,main_a1b2c3d4). While the current fixture name is functionally correct and clear, using a hash-like suffix such as"main_12ab34cd"would make the test more representative of production behavior.📝 Proposed update for more realistic fixture naming
Upstream: models.RouteUpstream{ - ClusterKey: "main_fixture", + ClusterKey: "main_12ab34cd", },UpstreamClusters: map[string]*models.UpstreamCluster{ - "main_fixture": { + "main_12ab34cd": { BasePath: "/",Also applies to: 132-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/policyxds/policyxds_test.go` at line 120, Update the test fixture cluster key strings that currently use "main_fixture" to a realistic hash-like suffix such as "main_12ab34cd" to match the production pattern; locate the occurrences in the policyxds_test setup where ClusterKey: "main_fixture" is declared (the test fixture variables/constants used in Policy XDS tests) and replace them with "main_12ab34cd" (or another 8-char hex-like hash) to make the test names reflect the <env>_<sha256(... )[:8]> convention.
🤖 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.
Nitpick comments:
In `@gateway/gateway-controller/pkg/policyxds/policyxds_test.go`:
- Line 120: Update the test fixture cluster key strings that currently use
"main_fixture" to a realistic hash-like suffix such as "main_12ab34cd" to match
the production pattern; locate the occurrences in the policyxds_test setup where
ClusterKey: "main_fixture" is declared (the test fixture variables/constants
used in Policy XDS tests) and replace them with "main_12ab34cd" (or another
8-char hex-like hash) to make the test names reflect the <env>_<sha256(...
)[:8]> convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8729c50c-d6aa-40f4-b0b0-efec0f9c903c
📒 Files selected for processing (13)
gateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/policyxds/policyxds_test.gogateway/gateway-controller/pkg/transform/restapi.gogateway/gateway-controller/pkg/transform/restapi_test.gogateway/gateway-controller/pkg/utils/clusterkey/clusterkey.gogateway/gateway-controller/pkg/utils/clusterkey/clusterkey_test.gogateway/gateway-controller/pkg/utils/commonutils_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.gogateway/it/features/api-level-eds-stable.featuregateway/it/features/per-op-upstream-basic.featuregateway/it/suite_test.go
💤 Files with no reviewable changes (1)
- gateway/gateway-controller/pkg/utils/commonutils_test.go
✅ Files skipped from review due to trivial changes (2)
- gateway/gateway-controller/cmd/controller/main.go
- gateway/it/suite_test.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/config/validator_test.go (1)
773-796: ⚡ Quick winIsolate invalid-timeout cases with subtests.
Using
requireinside the loop means the first failing case can stop execution before the next case is checked. Wrapping eachbadTimeoutint.Run(...)keeps both cases independently validated and easier to diagnose.Proposed test refactor
- for _, badTimeout := range []string{"0s", "-5s"} { + for _, badTimeout := range []string{"0s", "-5s"} { + t.Run(badTimeout, func(t *testing.T) { connect := badTimeout definitions := &[]api.UpstreamDefinition{ { Name: "my-upstream", Timeout: &api.UpstreamTimeout{ Connect: &connect, }, Upstreams: []struct { Url string `json:"url" yaml:"url"` Weight *int `json:"weight,omitempty" yaml:"weight,omitempty"` }{ { Url: "http://backend:8080", }, }, }, } errors := validator.validateUpstreamDefinitions(definitions) require.Len(t, errors, 1, "timeout %q must be rejected", badTimeout) assert.Equal(t, "spec.upstreamDefinitions[0].timeout.connect", errors[0].Field) assert.Contains(t, errors[0].Message, "must be a positive duration") + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/config/validator_test.go` around lines 773 - 796, The test loop in validator_test.go uses require inside a for-range which can abort the whole loop on first failure; refactor by wrapping each badTimeout iteration in a subtest using t.Run(fmt.Sprintf("timeout=%s", badTimeout), func(t *testing.T) { ... }) and move the assertions (require.Len, assert.Equal, assert.Contains) into that subtest, capturing the loop variable (e.g., tt := badTimeout) before using it to build the definitions passed to validator.validateUpstreamDefinitions so each case runs and reports independently.
🤖 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 `@gateway/gateway-controller/api/management-openapi.yaml`:
- Around line 4136-4137: The schemas that currently use a $ref with a sibling
description (referencing "`#/components/schemas/RestAPIOperationUpstream`") must
be changed so the description is not a sibling of $ref; instead wrap the
reference in an allOf and move the description onto the enclosing schema object.
Concretely, replace occurrences where a property has "$ref:
'`#/components/schemas/RestAPIOperationUpstream`'" alongside "description" by
creating an object with "description: <same text>" and "allOf: [{ $ref:
'`#/components/schemas/RestAPIOperationUpstream`' }]" (apply this change for every
place referencing RestAPIOperationUpstream in the file).
In `@gateway/gateway-controller/pkg/config/api_validator.go`:
- Around line 656-667: The per-operation upstream ref checks (using
upstreamRefRegex and length >100 in the validate path that returns
ValidationError for field/refName) are stricter than names allowed by
validateUpstreamDefinitions, causing valid definition names to become
unreferencable; to fix, apply the same validation rules to
spec.upstreamDefinitions[*].name inside validateUpstreamDefinitions (reuse
upstreamRefRegex and the 100-character limit and return a ValidationError with
the same message format when a definition name violates them) OR remove the
extra constraints from the per-operation check so both places use the same
contract; update validateUpstreamDefinitions to reference the same
upstreamRefRegex and error messages (Field and Message) so names and refs are
aligned.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/config/validator_test.go`:
- Around line 773-796: The test loop in validator_test.go uses require inside a
for-range which can abort the whole loop on first failure; refactor by wrapping
each badTimeout iteration in a subtest using t.Run(fmt.Sprintf("timeout=%s",
badTimeout), func(t *testing.T) { ... }) and move the assertions (require.Len,
assert.Equal, assert.Contains) into that subtest, capturing the loop variable
(e.g., tt := badTimeout) before using it to build the definitions passed to
validator.validateUpstreamDefinitions so each case runs and reports
independently.
🪄 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: a0b21e0b-ca37-459a-b596-04dce4de1349
📒 Files selected for processing (18)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/validator_test.gogateway/gateway-controller/pkg/policy/builder.gogateway/gateway-controller/pkg/policy/builder_test.gogateway/gateway-controller/pkg/transform/restapi.gogateway/gateway-controller/pkg/transform/restapi_test.gogateway/gateway-controller/pkg/utils/upstreamref/upstreamref.gogateway/gateway-controller/pkg/utils/upstreamref/upstreamref_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.gogateway/gateway-controller/tests/integration/storage_test.gogateway/gateway-runtime/policy-engine/internal/kernel/translator.gogateway/gateway-runtime/policy-engine/internal/kernel/translator_test.gogateway/it/features/per-op-upstream-basic.featuregateway/it/features/per-op-upstream-ref.featuregateway/it/features/per-op-upstream-validation.feature
🚧 Files skipped from review as they are similar to previous changes (4)
- gateway/gateway-controller/pkg/policy/builder.go
- gateway/gateway-controller/pkg/policy/builder_test.go
- gateway/gateway-controller/pkg/transform/restapi.go
- gateway/gateway-controller/pkg/xds/translator.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
4cfd850 to
4caca38
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
80924d0 to
66dd517
Compare
|
@coderabbitai review Please do a focused pass on code duplication only (not correctness - the code is
For each finding: file:line, why it duplicates, and a concrete shape to fix it. Known and intentionally left as-is, please skip these:
Cluster-name outputs must stay byte-identical - don't propose anything that changes them. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===1. VALIDATOR: repeated literals/logic in api_validator.go==="
grep -n "100\|maxLength\|MaxLength\|not found in upstreamDefinitions\|must match pattern\|must not exceed\|regexp.MustCompile\|upstreamRefRegex\|connectTimeoutRegex" gateway/gateway-controller/pkg/config/api_validator.go
echo ""
echo "===2. FULL VALIDATOR FILE (for structural dup check)==="
cat -n gateway/gateway-controller/pkg/config/api_validator.go
echo ""
echo "===3. RESTAPI.GO FULL (for per-op loop dup check)==="
cat -n gateway/gateway-controller/pkg/transform/restapi.go
echo ""
echo "===4. TRANSLATOR.GO FULL (for per-op loop dup check)==="
cat -n gateway/gateway-controller/pkg/xds/translator.goLength of output: 185847 I have enough evidence now. Here are the concrete duplication findings, scoped strictly to duplication (not correctness), with the three known items excluded as requested.
1.
|
|
@coderabbitai Thanks, but please re-run scoped to THIS PR's code only. Only flag duplication where BOTH occurrences are in code this PR adds or modifies (check against the diff base d891a12). Do NOT flag pre-existing base functions or duplication between our code and base code - specifically skip validateUpstream, validateUpstreamUrl, validateUpstreamRef, validateUpstreamDefinitions (api_validator.go), resolveUpstreamURL (restapi.go), and resolveUpstreamCluster (translator.go) - those predate this PR and are out of scope. In-scope surfaces are only: the upstreamref package, validateOperationUpstreamRef and validateUpstreamRefName, perOpDefinitionClusterKey, resolvePerOpDefinitionCluster, the sandbox helpers HasContent/SandboxActive, the clusterkey Hash/HashedName helpers, and the per-op route loops we added in restapi.go and translator.go. Still exclude (by design): clusterkey.DefinitionName, the perOpDefinitionClusterKey vs resolvePerOpDefinitionCluster cross-module pair, and the policy-engine kernel mirror. Cluster-name outputs must stay byte-identical. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===CHECK BASE COMMIT AVAILABLE==="
git cat-file -e d891a12e4 2>&1 && echo "base exists" || echo "base NOT found, trying fetch"
git log --oneline -1 d891a12e4 2>/dev/null || true
echo ""
echo "===DIFF FOR upstreamref.go==="
git diff d891a12e4 -- gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go || true
echo ""
echo "===DIFF FOR clusterkey.go==="
git diff d891a12e4 -- gateway/gateway-controller/pkg/utils/clusterkey/clusterkey.go || true
echo ""
echo "===DIFF FOR api_validator.go (validateOperationUpstreamRef / validateUpstreamRefName)==="
git diff d891a12e4 -- gateway/gateway-controller/pkg/config/api_validator.go | grep -n "validateOperationUpstreamRef\|validateUpstreamRefName\|^@@\|^+" | head -300
echo ""
echo "===DIFF FOR restapi.go (perOpDefinitionClusterKey, sandbox helpers, per-op loop)==="
git diff d891a12e4 -- gateway/gateway-controller/pkg/transform/restapi.go > /tmp/restapi.diff
wc -l /tmp/restapi.diff
cat /tmp/restapi.diff
echo ""
echo "===DIFF FOR translator.go (resolvePerOpDefinitionCluster, per-op loop)==="
git diff d891a12e4 -- gateway/gateway-controller/pkg/xds/translator.go > /tmp/translator.diff
wc -l /tmp/translator.diff
cat /tmp/translator.diffLength of output: 41480 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===restapi.go line numbers for per-op override block==="
grep -n "op.Upstream.Main != nil\|op.Upstream.Sandbox != nil\|perOpDefinitionClusterKey\|per-op main upstream\|per-op sandbox upstream" gateway/gateway-controller/pkg/transform/restapi.go
echo ""
echo "===translator.go line numbers for per-op override block==="
grep -n "op.Upstream != nil && op.Upstream.Main != nil\|op.Upstream != nil && op.Upstream.Sandbox != nil\|resolvePerOpDefinitionCluster\|per-op main upstream\|per-op sandbox upstream" gateway/gateway-controller/pkg/xds/translator.go
echo ""
echo "===api_validator.go and upstreamref.go trim-empty check line numbers==="
grep -n "refName == \"\"\|TrimSpace(\*ref)\|TrimSpace(ref)" gateway/gateway-controller/pkg/config/api_validator.go gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.goLength of output: 3593
1.
|
|
@coderabbitai we're renaming the Our candidates:
Please (a) add any stronger options we've missed, then (b) rank ALL of them (ours + yours) best-to-worst with a one-line rationale each. Judge by: clarity of intent (it overrides the API-level upstream for a single operation), consistency with the sibling schemas ( |
|
Tip For best results, initiate chat on the files or code changes.
(a) Additional candidates worth considering
(b) Full ranking, best to worst
Recommendation: go with |
|
@coderabbitai please do an equivalence audit of this PR's recent rework. Compare commit dac1f56 (the earlier verified state of this branch) against the current head 38958c3 and confirm the observable behavior is unchanged: for the same deployed API, identical routing results (which backend each operation reaches on main and sandbox vhosts, per-op ref reuse of definition clusters, fallbacks), identical path rewriting (definition basePath handling), identical timeout propagation, identical hostRewrite behavior, identical precedence (op-policy > api-policy > per-op ref > api-level upstream), and an identical validation accept/reject set. The following changes in that range are INTENTIONAL, please verify they are implemented consistently but do not report them as regressions:
Please verify with file:line evidence and flag ONLY things outside this list: any route/cluster/policy-chain output that changed, any validation outcome that flipped, any error path lost, or any type/nil-safety issue introduced by the string-parameter change. If nothing is out of place, say explicitly that the range is behavior-preserving apart from the listed intentional changes. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===VERIFY COMMITS EXIST==="
git cat-file -e dac1f564a 2>&1 && echo "base ok" || echo "base missing"
git cat-file -e 38958c316 2>&1 && echo "head ok" || echo "head missing"
echo ""
echo "===DIFF clusterkey.go==="
git diff dac1f564a 38958c316 -- gateway/gateway-controller/pkg/utils/clusterkey/clusterkey.go
echo ""
echo "===DIFF upstreamref.go==="
git diff dac1f564a 38958c316 -- gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go
echo ""
echo "===DIFF api_validator.go==="
git diff dac1f564a 38958c316 -- gateway/gateway-controller/pkg/config/api_validator.go > /tmp/validator.diff
wc -l /tmp/validator.diff
cat /tmp/validator.diff
echo ""
echo "===DIFF restapi.go==="
git diff dac1f564a 38958c316 -- gateway/gateway-controller/pkg/transform/restapi.go > /tmp/restapi.diff
wc -l /tmp/restapi.diff
cat /tmp/restapi.diff
echo ""
echo "===DIFF translator.go==="
git diff dac1f564a 38958c316 -- gateway/gateway-controller/pkg/xds/translator.go > /tmp/translator.diff
wc -l /tmp/translator.diff
cat /tmp/translator.diff
echo ""
echo "===DIFF management-openapi.yaml (upstream sections)==="
git diff dac1f564a 38958c316 -- gateway/gateway-controller/api/management-openapi.yaml
echo ""
echo "===DIFF generated.go (upstream types)==="
git diff dac1f564a 38958c316 -- gateway/gateway-controller/pkg/api/management/generated.go | head -300Length of output: 50193 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===resolveTimeoutFromDefinition definition (dac1f564a and 38958c316)==="
git show dac1f564a:gateway/gateway-controller/pkg/xds/translator.go | grep -n "func resolveTimeoutFromDefinition" -A 20
echo "---HEAD---"
git show 38958c316:gateway/gateway-controller/pkg/xds/translator.go | grep -n "func resolveTimeoutFromDefinition" -A 20
echo ""
echo "===check for any other diff hunks touching hostRewrite/timeout/precedence not yet reviewed==="
git diff dac1f564a 38958c316 --stat -- gateway/gateway-controller/pkg gateway/gateway-controller/api
echo ""
echo "===full diff of any remaining files under pkg (excluding tests) to be thorough==="
git diff dac1f564a 38958c316 --name-only -- gateway/gateway-controller/pkg gateway/gateway-controller/api | grep -v _test.goLength of output: 9315
Verification of each intentional change1.
2. OpenAPI consolidation — 3. Validator centralization — 4. 5.
Nothing flagged outside the listNo route/cluster/policy-chain output changed, no validation outcome flipped, no error path lost, and no type/nil-safety issue from the string-parameter change (the field was never a pointer to begin with). One scope note (not a regression, just flagging for accuracy)The full If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
38958c3 to
f3672a6
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
3a96a42 to
dc1a079
Compare
dc1a079 to
ca2f800
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
gateway/it/features/api-level-url-stable.feature (1)
220-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScenario title doesn't match its content.
Title says "API-level upstream with cluster_header routing", but the scenario only exercises an API-level
upstream.main.refpointing atupstreamDefinitions— there's nocluster_headerconfig or assertion anywhere in the scenario. This looks like leftover/copy-paste wording that will confuse future maintainers about what's actually covered.✏️ Suggested rename
- Scenario: API-level upstream with cluster_header routing (default upstream cluster resolves correctly) + Scenario: API-level upstream ref resolves to the referenced upstreamDefinitions entry🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/it/features/api-level-url-stable.feature` around lines 220 - 259, Rename the scenario title to describe the API-level upstream using the default upstream cluster resolution via upstreamDefinitions and upstream.main.ref. Remove the inaccurate “cluster_header routing” wording while leaving the scenario steps unchanged.gateway/gateway-controller/pkg/config/api_validator.go (1)
41-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow this comment or reuse the shared helper
upstreamRefRegexonly backs the API/per-op ref checks. Definition names still use the separateupstreamDefinitionNameRegexand its own messages, so this doc comment overstates the field’s scope and leaves two copies of the same rule to drift. Either route definition-name validation throughvalidateUpstreamRefNametoo, or trim the comment to refs only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/config/api_validator.go` around lines 41 - 43, Trim the comment for upstreamRefRegex to describe only API-level and per-operation upstream reference validation. Keep upstreamDefinitionNameRegex and its distinct validation messages unchanged.
🤖 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 `@gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go`:
- Around line 53-72: Update ParseConnectTimeout to accept a zero duration such
as 0s, while continuing to reject negative durations and invalid formats. Change
the non-positive duration validation in ParseConnectTimeout to enforce only the
shared contract’s disallowed values.
In `@gateway/it/features/api-level-url-stable.feature`:
- Around line 450-480: The API update scenario should wait for configuration
propagation before inspecting Envoy clusters. After the successful update in the
scheme/port scenario, add the same endpoint-readiness wait used by the other
update scenarios; if that endpoint cannot become ready, replace it with a short
retry around the `/clusters` check and document the exception.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/config/api_validator.go`:
- Around line 41-43: Trim the comment for upstreamRefRegex to describe only
API-level and per-operation upstream reference validation. Keep
upstreamDefinitionNameRegex and its distinct validation messages unchanged.
In `@gateway/it/features/api-level-url-stable.feature`:
- Around line 220-259: Rename the scenario title to describe the API-level
upstream using the default upstream cluster resolution via upstreamDefinitions
and upstream.main.ref. Remove the inaccurate “cluster_header routing” wording
while leaving the scenario steps 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7434504b-6d83-434d-bd29-35fb352d1c74
📒 Files selected for processing (22)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/validator_test.gogateway/gateway-controller/pkg/transform/llm_test.gogateway/gateway-controller/pkg/transform/restapi.gogateway/gateway-controller/pkg/transform/restapi_test.gogateway/gateway-controller/pkg/utils/clusterkey/clusterkey.gogateway/gateway-controller/pkg/utils/clusterkey/clusterkey_test.gogateway/gateway-controller/pkg/utils/upstreamref/upstreamref.gogateway/gateway-controller/pkg/utils/upstreamref/upstreamref_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.gogateway/gateway-controller/tests/integration/storage_test.gogateway/gateway-runtime/policy-engine/internal/kernel/translator_test.gogateway/it/features/api-level-url-stable.featuregateway/it/features/per-op-upstream-basic.featuregateway/it/features/per-op-upstream-ref.featuregateway/it/features/per-op-upstream-validation.featuregateway/it/steps_envoy_admin.gogateway/it/suite_test.gogateway/spec/impls/1-basic-gateway-with-controller/data-model.md
f707aad to
afa9253
Compare
…gateway # Conflicts: # gateway/gateway-controller/pkg/api/management/generated.go
…ted contract The management validator deliberately accepts a zero connect timeout and enforces only the duration format, so deploying a definition with connect 0s returns 201. The scenario previously expected a 400 from the stricter pre-rebase contract and failed once CI ran the suite against the merged main.
…gateway # Conflicts: # gateway/gateway-controller/pkg/api/management/generated.go
Summary
Adds a per-operation
upstreamoverride on REST API operations: an operation can route itsmainand/orsandboxtraffic to a different backend than the API-level upstream. Per-op targets are ref-only: each references a named entry inspec.upstreamDefinitionsrather than carrying an inline URL. Operations without a per-op upstream fall back to the API-level upstream, exactly as before.A per-op route reuses the referenced
upstreamDefinition's cluster (one cluster per definition, so operations sharing arefshare it) rather than minting a cluster per operation. API-levelmain/sandboxcluster names are URL-stable (derived from a hash of the API's identity, not the URL). Both are URL-independent, so URL edits update Envoy endpoints in place instead of recreating clusters.Why
Real-world APIs often need individual operations to reach different backend services. Today that needs path-based workarounds or splitting into multiple APIs. This lets the API definition declare per-operation routing directly, while backend URLs stay defined once in
upstreamDefinitionsand are referenced by name.Design decisions
Ref-only at the operation level.
operations[].upstream.main/.sandboxcarry only arefto a namedupstreamDefinition, with no inlineurland no per-target timeout/hostRewrite. Backends are declared once inspec.upstreamDefinitions; connect timeout lives on the definition. (API-levelupstreamstill accepts eitherurlorref, unchanged.)Per-op routes reuse the definition cluster. A per-op
refdoes not create a new cluster. It points the operation's route at the cluster the referencedupstreamDefinitionalready owns (upstream_<kind>_<apiID>_<defName>, built once per definition). N operations sharing arefcost one cluster, not N, and the route inherits the definition's authoritative base path.cluster_headerstays on with that cluster as the default, so a dynamic-endpoint policy can still override the operation (precedence: op-policy > api-policy > per-op ref > api-level upstream).URL-stable API-level cluster naming. API-level
main/sandboxcluster names derive from the fullsha256(apiID)(64 hex), viaclusterkey.HashedName(env, apiID), producingmain_<hash>/sandbox_<hash>. Theenvis a plaintext prefix, not part of the hash, somainandsandboxshare the same hash. The URL is intentionally excluded, so a URL edit keeps the same cluster name and Envoy updates its endpoints in place (no connection drain) rather than recreating the cluster. The clusters areSTRICT_DNSwith an inline load assignment. Rollout: a one-time cluster rebuild on the first config push after upgrade (URL-derived to identity-based names), handled gracefully by Envoy's CDS warming; stable thereafter.Single source of truth. Ref resolution and connect-timeout parsing go through
pkg/utils/upstreamref(FindByName,ParseConnectTimeout); API-level cluster-name hashing goes throughpkg/utils/clusterkey(Hashreturns the full SHA-256 hash,HashedNamejoins a prefix to it). Both are consumed by the validator, the RDC transformer, and the xDS translator so they cannot drift. The policy-engine kernel (a separate Go module that cannot importclusterkey) reconstructs the definition cluster name for dynamic-endpoint targets; a kernel unit test pins its output byte-for-byte toclusterkey.DefinitionNameso the two modules cannot drift either.What changed (gateway-controller layer)
management-openapi.yaml): optionalupstreamonOperationresolving toOperationUpstream(main/sandbox), each an inline ref-only object ({ ref }, no named target schema). Wrapper and each sub-field locked withadditionalProperties: false, plusminProperties: 1on the wrapper (an emptyupstream: {}is rejected).api_validator.go): rejects an emptyupstream: {}wrapper; requires at least one ofmain/sandbox; rejects arefthat does not resolve; and enforces one name contract (^[a-zA-Z0-9\-_]+$, max 100 chars) on per-op refs, API-level refs, andupstreamDefinitionnames so every valid definition name is referenceable.transform/restapi.go) and xDS translator (xds/translator.go): per-op routes reuse the referenced definition's cluster (upstream_<kind>_<apiID>_<defName>, built once per definition); API-level clusters use URL-stable namesmain_<hash>/sandbox_<hash>; operations without a per-op upstream fall back to the API-level cluster.pkg/utils/upstreamref,pkg/utils/clusterkey): new leaf packages.upstreamrefhandles ref resolution and connect-timeout parsing; the stdlib-onlyclusterkeyhandles URL-stable API-level cluster-key hashing (Hash/HashedName) and definition cluster naming (DefinitionName).Cluster names: before / after
For an API with UUID
0190b3e2-7b1c-7c2a-9b3d-1a2b3c4d5e6f(sha256(apiID)=54a9b3e5ce2b6ccb97168e5948a66f48e084213b38eb8c7dc01c6f624a63c2f2):cluster_http_default-backend_8080(URL-derived)main_54a9b3e5ce2b6ccb97168e5948a66f48e084213b38eb8c7dc01c6f624a63c2f2(identity-based)cluster_http_sandbox-backend_8080(URL-derived)sandbox_54a9b3e5ce2b6ccb97168e5948a66f48e084213b38eb8c7dc01c6f624a63c2f2(identity-based)user-serviceupstream_RestApi_0190b3e2-7b1c-7c2a-9b3d-1a2b3c4d5e6f_user-serviceclusterkey.DefinitionNamehelper)Only the API-level names change. Editing a backend URL previously renamed the API-level cluster, so Envoy dropped and recreated it; with identity-based names the cluster name never changes and Envoy updates the endpoints in place. Definition cluster names were already URL-independent.
Test coverage
pkg/config,pkg/transform,pkg/xds,pkg/utils/upstreamref, andpkg/utils/clusterkey: deterministic hash tests (Hash), validator rejection tests (empty/unknown/pattern/length refs, with exact error reasons), URL-stable cluster-name contract tests (same name across a URL edit), definition-cluster reuse and cross-API isolation tests, per-op route tests asserting the exact referenced definition cluster, a policy chain-order test (API-level policies precede operation-level, so the operation-level policy wins as the last write), and a policy-engine kernel test pinning thex-target-upstreamvalue for a dotted/coloned definition name byte-for-byte to the controller'sclusterkey.DefinitionName.Example
One API exercising every capability: API-level fallback, per-op main override, per-op main+sandbox overrides, definition base path and connect timeout, host rewrite, and a dynamic-endpoint policy overriding a per-op ref.
Routing result:
GET /productsgoes todefault-backend(no per-op upstream, API-level fallback)GET /users/{id}goes touser-serviceunder/api/v2(per-op main ref, inherits the definition's basePath)POST /ordersgoes toorder-serviceon main anduser-serviceon sandbox (per-op overrides per environment)GET /recommendationsgoes topromo-service(the operation-level dynamic-endpoint policy overrides the per-op ref)