Add per-operation upstream override for REST API operations#2012
Open
mehara-rothila wants to merge 10 commits into
Open
Add per-operation upstream override for REST API operations#2012mehara-rothila wants to merge 10 commits into
mehara-rothila wants to merge 10 commits into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 upstream overrides for REST operations: schema and generated models (ref-only per-op main/sandbox), validator checks (pattern, max length, existence, and timeout parsing), shared helpers (upstreamref lookup and timeout parsing, clusterkey API-level naming), transformer and XDS wiring to reuse upstream-definition clusters with basePath/timeout inheritance, RDC/xDS cluster naming stability, and extensive unit, integration, and BDD tests. Sequence Diagram(s)sequenceDiagram
participant Client
participant ManagementAPI
participant Transformer
participant XdsTranslator
participant UpstreamDefinitions
participant Envoy
Client->>ManagementAPI: deploy RestApi (may include per-op upstream.ref)
ManagementAPI->>Transformer: StoredConfig -> RuntimeDeployConfig (include per-op upstream refs)
Transformer->>UpstreamDefinitions: resolve per-op ref via upstreamref.FindByName
Transformer->>XdsTranslator: emit routes referencing definition cluster keys
XdsTranslator->>Envoy: create/register clusters (API-scoped clusterkey.APILevel / reused definition clusters)
Envoy-->>Client: traffic routed to correct backend (per-op or API-level)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
mehara-rothila
pushed a commit
to mehara-rothila/api-platform
that referenced
this pull request
May 22, 2026
- 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.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
mehara-rothila
force-pushed
the
feat/per-op-upstream-gateway
branch
2 times, most recently
from
4cfd850 to
4caca38
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
renuka-fernando
requested review from
RakhithaRR,
Tharsanan1,
VirajSalaka and
renuka-fernando
as code owners
mehara-rothila
force-pushed
the
feat/per-op-upstream-gateway
branch
from
f95b728 to
e6b7b7a
Compare
mehara-rothila
force-pushed
the
feat/per-op-upstream-gateway
branch
from
e6b7b7a to
5b22c76
Compare
|
@coderabbitai please review |
Contributor
|
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
gateway/gateway-controller/api/management-openapi.yaml (1)
4135-4137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap the
$refto preserve thedescriptionreliably.
descriptionbeside$refcan be dropped by OpenAPI 3.0.3 tooling. UseallOfaround the reference at this property, as done in the nested fields.Suggested patch
upstream: - $ref: "`#/components/schemas/RestAPIOperationUpstream`" description: Per-operation upstream override with main and sandbox sub-fields. + allOf: + - $ref: "`#/components/schemas/RestAPIOperationUpstream`"For OpenAPI 3.0.3 Schema Objects, are sibling fields next to `$ref` ignored by tooling, and is `allOf: [{ $ref: ... }]` the recommended way to retain metadata like `description`?🤖 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/api/management-openapi.yaml` around lines 4135 - 4137, The OpenAPI schema property "upstream" currently uses a bare $ref to "`#/components/schemas/RestAPIOperationUpstream`" with a sibling description which can be dropped by some OpenAPI 3.0.3 tooling; replace the direct $ref with an allOf wrapper (e.g., allOf: [{ $ref: "`#/components/schemas/RestAPIOperationUpstream`" }]) and keep the description alongside that allOf to ensure the description is preserved for the upstream property and its referenced schema.
🤖 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.
Duplicate comments:
In `@gateway/gateway-controller/api/management-openapi.yaml`:
- Around line 4135-4137: The OpenAPI schema property "upstream" currently uses a
bare $ref to "`#/components/schemas/RestAPIOperationUpstream`" with a sibling
description which can be dropped by some OpenAPI 3.0.3 tooling; replace the
direct $ref with an allOf wrapper (e.g., allOf: [{ $ref:
"`#/components/schemas/RestAPIOperationUpstream`" }]) and keep the description
alongside that allOf to ensure the description is preserved for the upstream
property and its referenced schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33dde7dc-c358-4de9-84a1-97139169e714
📒 Files selected for processing (24)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/cmd/controller/main.gogateway/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/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/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-eds-stable.featuregateway/it/features/per-op-upstream-basic.featuregateway/it/features/per-op-upstream-ref.featuregateway/it/features/per-op-upstream-validation.featuregateway/it/suite_test.go
✅ Files skipped from review due to trivial changes (1)
- gateway/gateway-controller/cmd/controller/main.go
🚧 Files skipped from review as they are similar to previous changes (19)
- gateway/it/suite_test.go
- gateway/gateway-controller/pkg/models/runtime_deploy_config.go
- gateway/it/features/per-op-upstream-validation.feature
- gateway/gateway-controller/tests/integration/storage_test.go
- gateway/gateway-controller/pkg/utils/clusterkey/clusterkey_test.go
- gateway/gateway-controller/pkg/utils/clusterkey/clusterkey.go
- gateway/it/features/per-op-upstream-ref.feature
- gateway/gateway-controller/pkg/policyxds/policyxds_test.go
- gateway/gateway-controller/pkg/utils/upstreamref/upstreamref.go
- gateway/gateway-controller/pkg/policy/builder_test.go
- gateway/gateway-controller/pkg/policy/builder.go
- gateway/it/features/api-level-eds-stable.feature
- gateway/it/features/per-op-upstream-basic.feature
- gateway/gateway-controller/pkg/xds/translator_test.go
- gateway/gateway-controller/pkg/utils/upstreamref/upstreamref_test.go
- gateway/gateway-controller/pkg/config/validator_test.go
- gateway/gateway-controller/pkg/transform/restapi.go
- gateway/gateway-controller/pkg/config/api_validator.go
- gateway/gateway-controller/pkg/transform/restapi_test.go
mehara-rothila
force-pushed
the
feat/per-op-upstream-gateway
branch
from
5b22c76 to
75c5c74
Compare
Contributor
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/config/api_validator.go`:
- Around line 374-391: The connect-timeout parsing must enforce allowed units
before calling time.ParseDuration: check def.Timeout.Connect (trimmed into
timeoutStr) against a regex that only permits numeric values with units ms|s|m|h
(e.g. `^\d+(\.\d+)?(ms|s|m|h)$`), and if it fails append a ValidationError for
the same field ("spec.upstreamDefinitions[%d].timeout.connect") with a clear
message about allowed units; only when the regex passes call
time.ParseDuration(timeoutStr) and keep the existing error handling for parse
errors and non-positive durations (referencing def.Timeout.Connect, timeoutStr,
time.ParseDuration and the ValidationError append sites).
🪄 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: 62d2e209-ad88-477d-a649-1d2728183da7
📒 Files selected for processing (24)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/cmd/controller/main.gogateway/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/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/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-eds-stable.featuregateway/it/features/per-op-upstream-basic.featuregateway/it/features/per-op-upstream-ref.featuregateway/it/features/per-op-upstream-validation.featuregateway/it/suite_test.go
…eout time.ParseDuration accepts units beyond the declared ms|s|m|h contract (ns, us, and compound durations like 1h30m), so an out-of-contract connect timeout could pass validation. Reject any value that is not a single ms|s|m|h duration as an invalid format, keeping the existing distinct invalid-format and non-positive messages.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Use "URL-stable" instead of "EDS-stable" for the cluster-naming comments (the clusters are STRICT_DNS with inline endpoints, not EDS), and drop the PR-number references from the basePath comments so they read on their own.
The RuntimeDeployConfig transform already rejects a config whose main and sandbox resolve to the same vhost, where sandbox routes would collide with main routes on the same route key. The router xDS path built sandbox routes without that guard, so the two paths disagreed. Add the same check to the xDS path so both reject the configuration consistently.
Describe the current wiring (only policy xDS receives the transformer registry; main Envoy xDS translates RestAPI configs directly) instead of implying the xDS translator cannot use transformers.
mehara-rothila
force-pushed
the
feat/per-op-upstream-gateway
branch
from
79cff8b to
bc4fb65
Compare
…op sandbox rejection
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 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 fromsha256(apiID|env)(main_<hash>/sandbox_<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 hash-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(APILevel). Both are consumed by the validator, the RDC transformer, and the xDS translator so they can't drift.What changed (gateway-controller layer)
management-openapi.yaml): optionalupstreamonOperationresolving toRestAPIOperationUpstream(main/sandbox), each a ref-onlyRestAPIOperationUpstreamTarget. Wrapper and leaf both locked withadditionalProperties: false.api_validator.go): rejects an emptyupstream: {}wrapper; requires at least one ofmain/sandbox; rejects arefthat doesn't resolve; and enforces one name contract (^[a-zA-Z0-9\-_]+$, max 100 chars) on both per-op 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 (APILevel).Test coverage
pkg/config,pkg/transform,pkg/xds,pkg/policy,pkg/utils/upstreamref, andpkg/utils/clusterkey: deterministic hash tests (APILevel), validator rejection tests, URL-stable cluster-name contract tests (same name across a URL edit), and definition-cluster reuse tests (operations sharing arefshare one cluster).per-op-upstream-basic(9),per-op-upstream-ref(6),per-op-upstream-validation(5),api-level-eds-stable(3)) covering API-level fallback, per-op main/sandbox ref overrides, validation rejections, dynamic-endpoint precedence over per-op refs, and URL-stable in-place endpoint updates. All passing against a local docker-compose stack.Example