Skip to content

make upstream url available to policies#2585

Merged
tharindu1st merged 1 commit into
wso2:mainfrom
tharindu1st:aws-authentication
Jul 12, 2026
Merged

make upstream url available to policies#2585
tharindu1st merged 1 commit into
wso2:mainfrom
tharindu1st:aws-authentication

Conversation

@tharindu1st

Copy link
Copy Markdown
Contributor

make upstream url available to policies

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@tharindu1st, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 34e598ce-e647-4f67-b64e-8c96c937ff0e

📥 Commits

Reviewing files that changed from the base of the PR and between 4ebe762 and 4764ff0.

⛔ Files ignored due to path filters (2)
  • gateway/gateway-controller/go.sum is excluded by !**/*.sum
  • gateway/gateway-runtime/policy-engine/go.sum is excluded by !**/*.sum
📒 Files selected for processing (14)
  • gateway/gateway-controller/go.mod
  • gateway/gateway-controller/pkg/models/runtime_deploy_config.go
  • gateway/gateway-controller/pkg/policyxds/snapshot.go
  • gateway/gateway-controller/pkg/transform/restapi.go
  • gateway/gateway-controller/pkg/transform/restapi_test.go
  • gateway/gateway-controller/pkg/xds/translator.go
  • gateway/gateway-runtime/policy-engine/go.mod
  • gateway/gateway-runtime/policy-engine/internal/admin/dumper.go
  • gateway/gateway-runtime/policy-engine/internal/admin/types.go
  • gateway/gateway-runtime/policy-engine/internal/constants/constants.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/translator.go
  • gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go
📝 Walkthrough

Walkthrough

Changes

Default upstream routing

Layer / File(s) Summary
Controller upstream contract and route generation
gateway/gateway-controller/pkg/models/..., gateway/gateway-controller/pkg/transform/..., gateway/gateway-controller/pkg/policyxds/..., gateway/gateway-controller/pkg/xds/...
Routes now carry serialized compiled-in upstream information, including resolved URL data, and sandbox configurations enable shared cluster-header routing.
Runtime route metadata ingestion and administration
gateway/gateway-runtime/policy-engine/internal/admin/..., gateway/gateway-runtime/policy-engine/internal/xdsclient/..., gateway/gateway-runtime/policy-engine/go.mod
Runtime metadata types, dumps, and xDS handling preserve default_upstream as typed upstream information; the SDK dependency is updated.
Default upstream execution-context wiring
gateway/gateway-runtime/policy-engine/internal/constants/..., gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go, gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
Route default upstream data is copied into execution and request contexts, with a key for current-upstream metadata.
Directive resolution and request routing
gateway/gateway-runtime/policy-engine/internal/kernel/translator.go
Request translation uses last-write-wins upstream directives, applies resolved overrides, and falls back to the compiled-in default while updating routing metadata and path operations.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ControllerTransform
  participant RouteConfigResource
  participant XDSHandler
  participant PolicyExecutionContext
  participant RequestTranslator
  ControllerTransform->>RouteConfigResource: attach default_upstream
  RouteConfigResource->>XDSHandler: deliver route config
  XDSHandler->>PolicyExecutionContext: populate DefaultUpstream
  PolicyExecutionContext->>RequestTranslator: provide default upstream
  RequestTranslator->>RequestTranslator: apply directive override or default
Loading

Possibly related PRs

  • wso2/api-platform#2577: Introduces UpstreamInfo and UpstreamSlot, which are used by the upstream data flow in this change.

Suggested reviewers: RakhithaRR, Krishanx92, Tharsanan1, VirajSalaka, malinthaprasan, AnuGayan, chamilaadhi, renuka-fernando, ashera96, pubudu538, dushaniw, tgtshanika

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is only a one-line summary and omits all required template sections. Fill in the template sections: Purpose, Goals, Approach, User stories, Documentation, Automation tests, Security checks, Samples, Related PRs, and Test environment.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the main change: exposing upstream URL to policies.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
gateway/gateway-runtime/policy-engine/internal/kernel/translator.go (1)

369-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the triplicated resolve/apply routing block into a shared helper.

The following 7-line pattern is copy-pasted across translateRequestActionsCore (lines 373-382), TranslateRequestHeaderActions (lines 524-533), and TranslateRequestHeaderActionsWithBodyMerge (lines 718-727):

applied := false
if directive.isSet() {
    if info, label, basePathKnown, ok := resolveUpstreamRedirect(execCtx, directive); ok {
        applyUpstreamRedirect(execCtx, headerOps, dynamicMetadata, &mutations.Path, info, label, basePathKnown)
        applied = true
    }
}
if !applied && execCtx.defaultUpstreamCluster != "" {
    applyDefaultUpstream(execCtx, headerOps, dynamicMetadata)
}

All three sites use identical parameter types. Extracting this into a single helper would eliminate the duplication and ensure consistent behavior if the routing logic changes in the future.

♻️ Proposed helper extraction
+// applyUpstreamRouting resolves and applies the last-write-wins upstream directive,
+// falling back to the route's compiled-in default when no directive is set or
+// the directive cannot be resolved.
+func applyUpstreamRouting(
+	execCtx *PolicyExecutionContext,
+	directive upstreamRedirectDirective,
+	headerOps map[string][]*headerOp,
+	localDynamicMetadata map[string]map[string]interface{},
+	pathMutation **string,
+) {
+	applied := false
+	if directive.isSet() {
+		if info, label, basePathKnown, ok := resolveUpstreamRedirect(execCtx, directive); ok {
+			applyUpstreamRedirect(execCtx, headerOps, localDynamicMetadata, pathMutation, info, label, basePathKnown)
+			applied = true
+		}
+	}
+	if !applied && execCtx.defaultUpstreamCluster != "" {
+		applyDefaultUpstream(execCtx, headerOps, localDynamicMetadata)
+	}
+}

Then each call site becomes a single line:

-	applied := false
-	if directive.isSet() {
-		if info, label, basePathKnown, ok := resolveUpstreamRedirect(execCtx, directive); ok {
-			applyUpstreamRedirect(execCtx, headerOps, out.DynamicMetadata, &out.Mutations.Path, info, label, basePathKnown)
-			applied = true
-		}
-	}
-	if !applied && execCtx.defaultUpstreamCluster != "" {
-		applyDefaultUpstream(execCtx, headerOps, out.DynamicMetadata)
-	}
+	applyUpstreamRouting(execCtx, directive, headerOps, out.DynamicMetadata, &out.Mutations.Path)

Also applies to: 523-533, 717-727

🤖 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-runtime/policy-engine/internal/kernel/translator.go` around
lines 369 - 382, Extract the duplicated upstream routing logic into a shared
helper near the existing routing helpers, accepting the execution context,
directive, header operations, dynamic metadata, and path mutations. Move the
resolveUpstreamRedirect/applyUpstreamRedirect and default-cluster fallback
behavior into that helper, then replace the blocks in
translateRequestActionsCore, TranslateRequestHeaderActions, and
TranslateRequestHeaderActionsWithBodyMerge with calls to it.
🤖 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-runtime/policy-engine/internal/kernel/translator.go`:
- Around line 158-187: Ensure applyDefaultUpstream always populates
CurrentUpstreamKey in both execCtx.dynamicMetadata and localDynamicMetadata
whenever a default upstream cluster is configured, including when
execCtx.defaultUpstream is nil; construct a partial UpstreamInfo using
execCtx.defaultUpstreamCluster (and any available fields) before calling ToMap.
Also reconcile the constants.go documentation if the contract remains
conditional.

---

Nitpick comments:
In `@gateway/gateway-runtime/policy-engine/internal/kernel/translator.go`:
- Around line 369-382: Extract the duplicated upstream routing logic into a
shared helper near the existing routing helpers, accepting the execution
context, directive, header operations, dynamic metadata, and path mutations.
Move the resolveUpstreamRedirect/applyUpstreamRedirect and default-cluster
fallback behavior into that helper, then replace the blocks in
translateRequestActionsCore, TranslateRequestHeaderActions, and
TranslateRequestHeaderActionsWithBodyMerge with calls to it.
🪄 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: 53fc4b27-f981-4147-8875-0ba05c632423

📥 Commits

Reviewing files that changed from the base of the PR and between 7eb1e89 and 420eb5c.

⛔ Files ignored due to path filters (1)
  • gateway/gateway-runtime/policy-engine/go.sum is excluded by !**/*.sum
📒 Files selected for processing (12)
  • gateway/gateway-controller/pkg/models/runtime_deploy_config.go
  • gateway/gateway-controller/pkg/policyxds/snapshot.go
  • gateway/gateway-controller/pkg/transform/restapi.go
  • gateway/gateway-controller/pkg/xds/translator.go
  • gateway/gateway-runtime/policy-engine/go.mod
  • gateway/gateway-runtime/policy-engine/internal/admin/dumper.go
  • gateway/gateway-runtime/policy-engine/internal/admin/types.go
  • gateway/gateway-runtime/policy-engine/internal/constants/constants.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/translator.go
  • gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go

Comment thread gateway/gateway-runtime/policy-engine/internal/kernel/translator.go
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Validation Results

⚠️ Please verify the scope of the dependencies usage is necessary

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Validation Results

⚠️ Please verify the scope of the dependencies usage is necessary

@github-actions

Copy link
Copy Markdown
Contributor

Dependency Validation Results

⚠️ Please verify the scope of the dependencies usage is necessary

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
gateway/gateway-controller/pkg/xds/translator.go (1)

1070-1083: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

upstreamDefPaths still isn't consumed in createRoute
If this legacy path still handles UpstreamName-based routing, the selected upstream's base path never reaches the policy engine, so requests can be rewritten against the wrong backend path.

🤖 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/xds/translator.go` around lines 1070 - 1083,
Pass the populated upstreamDefPaths map into createRoute and consume it when
resolving UpstreamName-based routes, applying the selected upstream definition’s
base path before policy-engine path transformation. Preserve the "/" fallback
for definitions without an explicit BasePath and ensure all relevant createRoute
call sites receive the map.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/transform/restapi_test.go (1)

439-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider also asserting the sandbox cluster exists in UpstreamClusters.

The assertion change from UseClusterHeader=false/empty DefaultCluster to UseClusterHeader=true/DefaultCluster=expectedSandboxCluster is correct — it matches the implementation logic useClusterHeader := hasUpstreamDefinitions || hasSandbox and the sandbox patch in restapi.go:302-324. The expectedSandboxCluster value (upstream_sandbox_sandbox-backend_9080) correctly uses the ClusterKey format, not EnvoyClusterName.

For additional robustness, consider also verifying the cluster actually exists in the map, as TestRestAPITransformer_DefaultClusterReferencesRealCluster does at lines 503-509. Based on learnings, tests should assert that rdc.UpstreamClusters[r.Upstream.DefaultCluster] exists rather than only comparing DefaultCluster to a string literal.

♻️ Optional: add cluster-existence assertion
 		assert.True(t, r.Upstream.UseClusterHeader)
 		assert.Equal(t, expectedSandboxCluster, r.Upstream.DefaultCluster,
 			"sandbox route must default to the sandbox cluster, not main")
+		_, ok := rdc.UpstreamClusters[r.Upstream.DefaultCluster]
+		assert.True(t, ok, "default cluster %q must exist in UpstreamClusters", r.Upstream.DefaultCluster)
 	})
🤖 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/transform/restapi_test.go` around lines 439 -
452, Extend the sandbox route test around NewRestAPITransformer and the
rdc.Routes[sandboxRouteKey] assertions to verify that r.Upstream.DefaultCluster
exists in rdc.UpstreamClusters, matching the pattern used by
TestRestAPITransformer_DefaultClusterReferencesRealCluster. Keep the existing
UseClusterHeader and expectedSandboxCluster assertions unchanged.

Source: Learnings

🤖 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/xds/translator.go`:
- Around line 431-466: Update buildMatchHeaders to normalize the method
parameter with strings.ToUpper before constructing the mandatory :method
matcher, and use the normalized value in the exact StringMatcher. Leave the
configured header matching logic unchanged.

---

Outside diff comments:
In `@gateway/gateway-controller/pkg/xds/translator.go`:
- Around line 1070-1083: Pass the populated upstreamDefPaths map into
createRoute and consume it when resolving UpstreamName-based routes, applying
the selected upstream definition’s base path before policy-engine path
transformation. Preserve the "/" fallback for definitions without an explicit
BasePath and ensure all relevant createRoute call sites receive the map.

---

Nitpick comments:
In `@gateway/gateway-controller/pkg/transform/restapi_test.go`:
- Around line 439-452: Extend the sandbox route test around
NewRestAPITransformer and the rdc.Routes[sandboxRouteKey] assertions to verify
that r.Upstream.DefaultCluster exists in rdc.UpstreamClusters, matching the
pattern used by TestRestAPITransformer_DefaultClusterReferencesRealCluster. Keep
the existing UseClusterHeader and expectedSandboxCluster assertions 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: 27e8bc83-275e-4898-9189-04bde2f53416

📥 Commits

Reviewing files that changed from the base of the PR and between 420eb5c and 4ebe762.

⛔ Files ignored due to path filters (2)
  • gateway/gateway-controller/go.sum is excluded by !**/*.sum
  • gateway/gateway-runtime/policy-engine/go.sum is excluded by !**/*.sum
📒 Files selected for processing (14)
  • gateway/gateway-controller/go.mod
  • gateway/gateway-controller/pkg/models/runtime_deploy_config.go
  • gateway/gateway-controller/pkg/policyxds/snapshot.go
  • gateway/gateway-controller/pkg/transform/restapi.go
  • gateway/gateway-controller/pkg/transform/restapi_test.go
  • gateway/gateway-controller/pkg/xds/translator.go
  • gateway/gateway-runtime/policy-engine/go.mod
  • gateway/gateway-runtime/policy-engine/internal/admin/dumper.go
  • gateway/gateway-runtime/policy-engine/internal/admin/types.go
  • gateway/gateway-runtime/policy-engine/internal/constants/constants.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/translator.go
  • gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go
🚧 Files skipped from review as they are similar to previous changes (10)
  • gateway/gateway-runtime/policy-engine/internal/admin/dumper.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
  • gateway/gateway-runtime/policy-engine/internal/constants/constants.go
  • gateway/gateway-controller/pkg/models/runtime_deploy_config.go
  • gateway/gateway-controller/pkg/policyxds/snapshot.go
  • gateway/gateway-runtime/policy-engine/internal/admin/types.go
  • gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go
  • gateway/gateway-controller/pkg/transform/restapi.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/translator.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
gateway/gateway-controller/pkg/xds/translator.go (1)

1070-1083: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

upstreamDefPaths still isn't consumed in createRoute
If this legacy path still handles UpstreamName-based routing, the selected upstream's base path never reaches the policy engine, so requests can be rewritten against the wrong backend path.

🤖 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/xds/translator.go` around lines 1070 - 1083,
Pass the populated upstreamDefPaths map into createRoute and consume it when
resolving UpstreamName-based routes, applying the selected upstream definition’s
base path before policy-engine path transformation. Preserve the "/" fallback
for definitions without an explicit BasePath and ensure all relevant createRoute
call sites receive the map.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/transform/restapi_test.go (1)

439-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider also asserting the sandbox cluster exists in UpstreamClusters.

The assertion change from UseClusterHeader=false/empty DefaultCluster to UseClusterHeader=true/DefaultCluster=expectedSandboxCluster is correct — it matches the implementation logic useClusterHeader := hasUpstreamDefinitions || hasSandbox and the sandbox patch in restapi.go:302-324. The expectedSandboxCluster value (upstream_sandbox_sandbox-backend_9080) correctly uses the ClusterKey format, not EnvoyClusterName.

For additional robustness, consider also verifying the cluster actually exists in the map, as TestRestAPITransformer_DefaultClusterReferencesRealCluster does at lines 503-509. Based on learnings, tests should assert that rdc.UpstreamClusters[r.Upstream.DefaultCluster] exists rather than only comparing DefaultCluster to a string literal.

♻️ Optional: add cluster-existence assertion
 		assert.True(t, r.Upstream.UseClusterHeader)
 		assert.Equal(t, expectedSandboxCluster, r.Upstream.DefaultCluster,
 			"sandbox route must default to the sandbox cluster, not main")
+		_, ok := rdc.UpstreamClusters[r.Upstream.DefaultCluster]
+		assert.True(t, ok, "default cluster %q must exist in UpstreamClusters", r.Upstream.DefaultCluster)
 	})
🤖 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/transform/restapi_test.go` around lines 439 -
452, Extend the sandbox route test around NewRestAPITransformer and the
rdc.Routes[sandboxRouteKey] assertions to verify that r.Upstream.DefaultCluster
exists in rdc.UpstreamClusters, matching the pattern used by
TestRestAPITransformer_DefaultClusterReferencesRealCluster. Keep the existing
UseClusterHeader and expectedSandboxCluster assertions unchanged.

Source: Learnings

🤖 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/xds/translator.go`:
- Around line 431-466: Update buildMatchHeaders to normalize the method
parameter with strings.ToUpper before constructing the mandatory :method
matcher, and use the normalized value in the exact StringMatcher. Leave the
configured header matching logic unchanged.

---

Outside diff comments:
In `@gateway/gateway-controller/pkg/xds/translator.go`:
- Around line 1070-1083: Pass the populated upstreamDefPaths map into
createRoute and consume it when resolving UpstreamName-based routes, applying
the selected upstream definition’s base path before policy-engine path
transformation. Preserve the "/" fallback for definitions without an explicit
BasePath and ensure all relevant createRoute call sites receive the map.

---

Nitpick comments:
In `@gateway/gateway-controller/pkg/transform/restapi_test.go`:
- Around line 439-452: Extend the sandbox route test around
NewRestAPITransformer and the rdc.Routes[sandboxRouteKey] assertions to verify
that r.Upstream.DefaultCluster exists in rdc.UpstreamClusters, matching the
pattern used by TestRestAPITransformer_DefaultClusterReferencesRealCluster. Keep
the existing UseClusterHeader and expectedSandboxCluster assertions 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: 27e8bc83-275e-4898-9189-04bde2f53416

📥 Commits

Reviewing files that changed from the base of the PR and between 420eb5c and 4ebe762.

⛔ Files ignored due to path filters (2)
  • gateway/gateway-controller/go.sum is excluded by !**/*.sum
  • gateway/gateway-runtime/policy-engine/go.sum is excluded by !**/*.sum
📒 Files selected for processing (14)
  • gateway/gateway-controller/go.mod
  • gateway/gateway-controller/pkg/models/runtime_deploy_config.go
  • gateway/gateway-controller/pkg/policyxds/snapshot.go
  • gateway/gateway-controller/pkg/transform/restapi.go
  • gateway/gateway-controller/pkg/transform/restapi_test.go
  • gateway/gateway-controller/pkg/xds/translator.go
  • gateway/gateway-runtime/policy-engine/go.mod
  • gateway/gateway-runtime/policy-engine/internal/admin/dumper.go
  • gateway/gateway-runtime/policy-engine/internal/admin/types.go
  • gateway/gateway-runtime/policy-engine/internal/constants/constants.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/translator.go
  • gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go
🚧 Files skipped from review as they are similar to previous changes (10)
  • gateway/gateway-runtime/policy-engine/internal/admin/dumper.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
  • gateway/gateway-runtime/policy-engine/internal/constants/constants.go
  • gateway/gateway-controller/pkg/models/runtime_deploy_config.go
  • gateway/gateway-controller/pkg/policyxds/snapshot.go
  • gateway/gateway-runtime/policy-engine/internal/admin/types.go
  • gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go
  • gateway/gateway-controller/pkg/transform/restapi.go
  • gateway/gateway-runtime/policy-engine/internal/kernel/translator.go
🛑 Comments failed to post (1)
gateway/gateway-controller/pkg/xds/translator.go (1)

431-466: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Normalize HTTP method before building the :method matcher.

buildMatchHeaders builds the Envoy :method matcher directly from the method parameter without uppercasing it. As per coding guidelines: "Normalize any user-supplied HTTP method string to uppercase with strings.ToUpper() at the earliest extraction point before comparisons, map key creation, route generation, or Envoy :method matcher construction." This is a newly-introduced shared helper and a natural place to add the guard, independent of whether upstream CRD/API validation already constrains the value.

🛡️ Proposed fix
 func buildMatchHeaders(method string, rdcRoute *models.Route) []*route.HeaderMatcher {
 	headerMatchers := []*route.HeaderMatcher{{
 		Name: ":method",
 		HeaderMatchSpecifier: &route.HeaderMatcher_StringMatch{
 			StringMatch: &matcher.StringMatcher{
-				MatchPattern: &matcher.StringMatcher_Exact{Exact: method},
+				MatchPattern: &matcher.StringMatcher_Exact{Exact: strings.ToUpper(strings.TrimSpace(method))},
 			},
 		},
 	}}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

func buildMatchHeaders(method string, rdcRoute *models.Route) []*route.HeaderMatcher {
	headerMatchers := []*route.HeaderMatcher{{
		Name: ":method",
		HeaderMatchSpecifier: &route.HeaderMatcher_StringMatch{
			StringMatch: &matcher.StringMatcher{
				MatchPattern: &matcher.StringMatcher_Exact{Exact: strings.ToUpper(strings.TrimSpace(method))},
			},
		},
	}}
	for _, hm := range rdcRoute.MatchHeaders {
		name := strings.ToLower(strings.TrimSpace(hm.Name))
		matchType := strings.TrimSpace(hm.Type)
		if matchType == "RegularExpression" {
			headerMatchers = append(headerMatchers, &route.HeaderMatcher{
				Name: name,
				HeaderMatchSpecifier: &route.HeaderMatcher_SafeRegexMatch{
					SafeRegexMatch: &matcher.RegexMatcher{Regex: hm.Value},
				},
			})
		} else {
			headerMatchers = append(headerMatchers, &route.HeaderMatcher{
				Name: name,
				HeaderMatchSpecifier: &route.HeaderMatcher_StringMatch{
					StringMatch: &matcher.StringMatcher{
						MatchPattern: &matcher.StringMatcher_Exact{Exact: hm.Value},
					},
				},
			})
		}
	}
	return headerMatchers
}
🤖 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/xds/translator.go` around lines 431 - 466,
Update buildMatchHeaders to normalize the method parameter with strings.ToUpper
before constructing the mandatory :method matcher, and use the normalized value
in the exact StringMatcher. Leave the configured header matching logic
unchanged.

Source: Coding guidelines

@github-actions

Copy link
Copy Markdown
Contributor

Dependency Validation Results

⚠️ Please verify the scope of the dependencies usage is necessary

@tharindu1st
tharindu1st merged commit b2e50b6 into wso2:main Jul 12, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants