feat(policy-engine/controller): support host (:authority) rewrite via dynamic metadata#1609
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:
WalkthroughAdds a new host-rewrite policy and wires host mutations through the request translation pipeline (path, method, host), updates SDK policy action schema to include Host, adjusts ext_proc header mutation rules, moves policy registrations, updates several go.mod versions, and adds integration tests for host rewrite. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant PolicyEngine as Policy Engine/Translator
participant LuaFilter as Lua Request Transformation
participant Upstream
Client->>Gateway: HTTP request (original Host)
Gateway->>PolicyEngine: Translate request actions (policies)
PolicyEngine->>PolicyEngine: Build RequestMutations{Path, Method, Host}
PolicyEngine->>Gateway: Attach dynamic metadata (includes Host)
Gateway->>LuaFilter: ext_proc reads dynamic metadata
LuaFilter->>LuaFilter: resolve_target_host() -> target host
LuaFilter->>LuaFilter: rewrite :authority and Host headers
LuaFilter->>Upstream: Forward request (rewritten Host)
Upstream->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/it/features/host-rewrite.feature (1)
105-133: Good test for hostRewrite: manual dependency, but scenario name could be clearer.This scenario validates that host rewrite only works when
hostRewrite: manualis configured on the upstream. The assertionshould contain "echo-backend"correctly verifies the backend-derived host is preserved when manual mode is not enabled.However, the scenario name "Host rewrite without hostRewrite manual should not work" is slightly ambiguous - it could imply the policy fails rather than that it's intentionally bypassed. Consider a clearer name like "Host rewrite policy is ignored when upstream hostRewrite is not set to manual".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/it/features/host-rewrite.feature` around lines 105 - 133, Rename the ambiguous scenario title to clearly state expected behavior: update the Scenario line currently "Host rewrite without hostRewrite manual should not work" to something like "Host rewrite policy is ignored when upstream hostRewrite is not set to manual" in the host-rewrite.feature so the test intent is explicit; keep the Given/When/Then steps and assertions (including the JSON assertion on "headers.Host" containing "echo-backend") unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gateway/it/features/host-rewrite.feature`:
- Around line 105-133: Rename the ambiguous scenario title to clearly state
expected behavior: update the Scenario line currently "Host rewrite without
hostRewrite manual should not work" to something like "Host rewrite policy is
ignored when upstream hostRewrite is not set to manual" in the
host-rewrite.feature so the test intent is explicit; keep the Given/When/Then
steps and assertions (including the JSON assertion on "headers.Host" containing
"echo-backend") unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a33a5d51-6e03-44ab-8ea4-35f8f2b8c647
⛔ Files ignored due to path filters (5)
gateway/gateway-builder/go.sumis excluded by!**/*.sumgateway/gateway-controller/go.sumis excluded by!**/*.sumgateway/gateway-runtime/policy-engine/go.sumis excluded by!**/*.sumgateway/sample-policies/transform-payload-case/go.sumis excluded by!**/*.sumgateway/system-policies/analytics/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
gateway/build-manifest.yamlgateway/build.yamlgateway/gateway-builder/go.modgateway/gateway-controller/go.modgateway/gateway-controller/lua/request_transformation.luagateway/gateway-runtime/policy-engine/go.modgateway/gateway-runtime/policy-engine/internal/kernel/extproc.gogateway/gateway-runtime/policy-engine/internal/kernel/translator.gogateway/gateway-runtime/policy-engine/internal/kernel/translator_test.gogateway/it/features/host-rewrite.featuregateway/it/suite_test.gogateway/sample-policies/transform-payload-case/go.modgateway/system-policies/analytics/go.modsdk/core/policy/v1alpha2/action.go
4f0d023 to
9718ae0
Compare
… dynamic metadata - Add Host field to SDK request modification types so policies can request rewriting the request host/authority. - Translator now includes host in the ext_proc dynamic metadata namespace (api_platform.policy_engine.envoy.filters.http.ext_proc) alongside path and method when policies request routing mutations. - Refactor translateRequestActionsCore to return a result struct instead of a long tuple, and refactor buildDynamicMetadata to accept a RequestMutations struct (path/method/host) to avoid growing parameter lists. - Update Lua request_transformation filter to read the host value from dynamic metadata and replace :authority and Host headers.
# Conflicts: # gateway/build-manifest.yaml
…er vhost - Change PROXY_HOST__HEADER_POLICY_NAME to 'host-rewrite' and update params format - Adjust GetHostAdditionPolicyParams to render host-rewrite params (host: value) - Update unit test expectations to check for 'host' field instead of 'request' When an LLM proxy is created with a provider that has a vhost configured, the transformer now generates a host-rewrite policy to set the Host header for upstream requests, replacing the previous set-headers policy.
9718ae0 to
2fd4cb4
Compare
467acd0 to
493466b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/gateway-controller/lua/request_transformation.lua (1)
206-214: Harden host rewrite by validating authority format before replacement.Line 211 and Line 212 currently accept any non-empty string. Rejecting obvious invalid forms (scheme/path/whitespace) will prevent malformed upstream authority values.
Proposed hardening diff
local function resolve_target_host(metadata) @@ return nil end +local function is_valid_authority(value) + if type(value) ~= "string" or value == "" then + return false + end + if string.find(value, "%s") ~= nil then + return false + end + if string.find(value, "://", 1, true) ~= nil then + return false + end + if string.find(value, "/", 1, true) ~= nil then + return false + end + return true +end + @@ - if target_host ~= nil and type(target_host) == "string" and target_host ~= "" then + if target_host ~= nil and is_valid_authority(target_host) then handle:logInfo("host_rewrite: target_host=" .. tostring(target_host)) -- Replace both :authority (HTTP/2) and Host (HTTP/1) for compatibility handle:headers():replace(":authority", target_host) handle:headers():replace("host", target_host) + elseif target_host ~= nil then + handle:logWarn("host_rewrite: invalid target_host ignored") end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/lua/request_transformation.lua` around lines 206 - 214, The host-rewrite currently replaces :authority/host with any non-empty string from resolve_target_host; tighten validation by verifying target_host is a valid authority (e.g. non-empty string, does not contain "://", does not start with "/", contains no whitespace) before calling handle:headers():replace(":authority", target_host) and replace("host", target_host); if validation fails, log a warning via handle:logInfo/handle:logWarn and skip the replacement. Reference target_host, resolve_target_host, and the handle:headers():replace calls when adding the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gateway/gateway-controller/lua/request_transformation.lua`:
- Around line 206-214: The host-rewrite currently replaces :authority/host with
any non-empty string from resolve_target_host; tighten validation by verifying
target_host is a valid authority (e.g. non-empty string, does not contain "://",
does not start with "/", contains no whitespace) before calling
handle:headers():replace(":authority", target_host) and replace("host",
target_host); if validation fails, log a warning via
handle:logInfo/handle:logWarn and skip the replacement. Reference target_host,
resolve_target_host, and the handle:headers():replace calls when adding the
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f13d9e8-4455-437f-abe3-dcd056c44ee2
⛔ Files ignored due to path filters (5)
gateway/gateway-builder/go.sumis excluded by!**/*.sumgateway/gateway-controller/go.sumis excluded by!**/*.sumgateway/gateway-runtime/policy-engine/go.sumis excluded by!**/*.sumgateway/sample-policies/transform-payload-case/go.sumis excluded by!**/*.sumgateway/system-policies/analytics/go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
gateway/build-manifest.yamlgateway/build.yamlgateway/gateway-builder/go.modgateway/gateway-controller/default-policies/host-rewrite.yamlgateway/gateway-controller/default-policies/model-round-robin.yamlgateway/gateway-controller/default-policies/model-weighted-round-robin.yamlgateway/gateway-controller/go.modgateway/gateway-controller/lua/request_transformation.luagateway/gateway-controller/pkg/constants/constants.gogateway/gateway-controller/pkg/utils/llm_transformer.gogateway/gateway-controller/pkg/utils/llm_transformer_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-runtime/policy-engine/go.modgateway/gateway-runtime/policy-engine/internal/kernel/extproc.gogateway/gateway-runtime/policy-engine/internal/kernel/translator.gogateway/gateway-runtime/policy-engine/internal/kernel/translator_test.gogateway/it/features/host-rewrite.featuregateway/it/suite_test.gogateway/sample-policies/transform-payload-case/go.modgateway/system-policies/analytics/go.modsdk/core/policy/v1alpha2/action.go
✅ Files skipped from review due to trivial changes (11)
- gateway/gateway-controller/default-policies/model-round-robin.yaml
- gateway/system-policies/analytics/go.mod
- gateway/sample-policies/transform-payload-case/go.mod
- gateway/gateway-builder/go.mod
- gateway/gateway-runtime/policy-engine/go.mod
- gateway/it/suite_test.go
- gateway/gateway-controller/go.mod
- gateway/gateway-controller/default-policies/model-weighted-round-robin.yaml
- gateway/gateway-runtime/policy-engine/internal/kernel/translator_test.go
- gateway/it/features/host-rewrite.feature
- gateway/build-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
- gateway/gateway-controller/pkg/utils/llm_transformer.go
- gateway/gateway-controller/pkg/constants/constants.go
- gateway/gateway-controller/default-policies/host-rewrite.yaml
- gateway/gateway-runtime/policy-engine/internal/kernel/translator.go
- gateway/build.yaml
Purpose
Fix #1598
Summary by CodeRabbit
New Features
Tests