Conversation
…c metadata in the policy engine, and add integration tests.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR introduces a Request Transformation Policy feature enabling rewriting of request paths, query parameters, and HTTP methods through dynamic metadata communication between the policy engine and a Lua-based transformation filter integrated into the Envoy HTTP filter chain. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Envoy as Envoy (Gateway)
participant ExtProc as Policy Engine<br/>(ext_proc)
participant LuaFilter as Lua Filter
participant Upstream
Client->>Envoy: HTTP Request (GET /api/v1/users)
Envoy->>ExtProc: RequestHeaders Phase
ExtProc->>ExtProc: Evaluate Policies<br/>(Request Transformation)
ExtProc->>ExtProc: Set Dynamic Metadata<br/>(request_transformation.target_method: POST)
ExtProc->>Envoy: ResponseHeaders with<br/>DynamicMetadata
Envoy->>LuaFilter: RequestHeaders Phase
LuaFilter->>LuaFilter: Read Dynamic Metadata<br/>from ext_proc namespace
LuaFilter->>LuaFilter: Extract target_method<br/>Normalize & Validate
LuaFilter->>LuaFilter: Apply Path Rewrite<br/>(regex/prefix/full)
LuaFilter->>LuaFilter: Apply Query Rewrite<br/>(Add/Replace/Remove)
LuaFilter->>LuaFilter: Update :method Header<br/>(e.g., GET → POST)
LuaFilter->>Upstream: Transformed Request
Upstream->>LuaFilter: Response
LuaFilter->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@policies/request-transformation/design.md`:
- Around line 514-516: The design doc's namespace entry currently lists
"envoy.filters.http.lua" but the implementation uses
"api_platform.policy_engine.envoy.filters.http.ext_proc"; update the table row
for the request_transformation.target_method policy to use the implementation
namespace "api_platform.policy_engine.envoy.filters.http.ext_proc" (or, if you
prefer to change code, make the Lua script use "envoy.filters.http.lua") so the
design entry for request_transformation.target_method matches the actual
namespace string used by the Lua implementation.
🧹 Nitpick comments (9)
gateway/gateway-controller/default-policies/request-transformation.yaml (1)
35-42: Consider adding conditional validation forvaluefield.The
valuefield hasminLength: 1but is only required forExactandRegexmatch types (notPresent). The current schema allows omittingvalue(it's not inrequired), but if provided, it must be non-empty. This is technically correct behavior.However, the schema doesn't enforce that
valuemust be present whentypeisExactorRegex. Users could providetype: Exactwithout avalue, which would likely fail at runtime.kubernetes/helm/gateway-helm-chart/values.yaml (1)
174-176: Minor: Comment is too narrow for the feature scope.The comment says "request method transformation" but the Lua script handles path rewriting, query parameter rewriting, AND method transformation. Consider updating for accuracy:
📝 Suggested comment update
- # Lua script path for request method transformation + # Lua script path for request transformation (path, query, method rewrites) lua_script_path: ./lua/request_transformation.luakubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml (1)
82-84: Duplicate[gateway_controller.router]section header.A
[gateway_controller.router]section already exists at line 44. While TOML allows redeclaring sections to continue adding keys, this creates fragmented configuration that's harder to maintain. Consider movinglua_script_pathalongside the other router settings:♻️ Suggested consolidation
[gateway_controller.router] gateway_host = {{ $gc.router.gateway_host | quote }} listener_port = {{ $gc.router.listener_port }} https_enabled = {{ $gc.router.https_enabled }} https_port = {{ $gc.router.https_port }} tracing_service_name = {{ $gc.router.tracing_service_name | default "" | quote }} + lua_script_path = {{ $gc.router.lua_script_path | quote }} [gateway_controller.router.access_logs] ... - - [gateway_controller.router] - lua_script_path = {{ $gc.router.lua_script_path | quote }} - [gateway_controller.router.policy_engine]gateway/configs/config-template.toml (1)
105-107: Duplicate[gateway_controller.router]section header.Same issue as the Helm template -
[gateway_controller.router]is already declared at line 40. Consolidatelua_script_pathwith the existing router settings for cleaner configuration:♻️ Suggested consolidation
[gateway_controller.router] # Gateway host for incoming requests gateway_host = "*" # Listener port for incoming HTTP traffic (Envoy proxy port) listener_port = 8080 # HTTPS listener configuration https_enabled = true https_port = 8443 # Tracing service name tracing_service_name = "router" +# Lua script path for request transformation +lua_script_path = "./lua/request_transformation.lua" [gateway_controller.router.access_logs] ... - -[gateway_controller.router] -lua_script_path = "./lua/request_transformation.lua" - [gateway_controller.router.policy_engine]gateway/gateway-controller/pkg/config/config.go (1)
212-212: Consider adding validation for LuaScriptPath.The
LuaScriptPathfield is added to the config, but there's no corresponding validation in theValidate()method. While the translator will fail if the file doesn't exist, adding validation here would provide earlier feedback during config loading.This is optional since the error will be caught at runtime during filter creation.
gateway/gateway-controller/lua/request_transformation.lua (1)
55-55: Hardcoded namespace string duplicates constant value.The namespace
"api_platform.policy_engine.envoy.filters.http.ext_proc"is hardcoded here but defined asExtProcMetadataNamespacein constants.go. While Lua scripts can't directly reference Go constants, consider adding a comment documenting the dependency to help maintainers keep them in sync.- local extproc_metadata = dynamic_metadata:get("api_platform.policy_engine.envoy.filters.http.ext_proc") + -- NOTE: This namespace must match constants.ExtProcMetadataNamespace in Go code + local extproc_metadata = dynamic_metadata:get("api_platform.policy_engine.envoy.filters.http.ext_proc")policies/request-transformation/design.md (1)
31-33: Add language specifier to fenced code block.The code block lacks a language specifier per markdownlint MD040.
-``` +```text Request → ext_proc (Policy Engine) → Lua Filter → Upstream -``` +```gateway/it/features/request-transformation.feature (1)
213-253: Consider verifying parameter removal in query rewrite test.The scenario tests Add and Replace actions but doesn't explicitly verify that the
debugparameter was removed. While line 253 checks the expected values, adding verification thatdebugis absent would strengthen the test.This is optional since the test validates the core functionality.
gateway/gateway-controller/pkg/xds/translator.go (1)
2370-2402: Well-implemented Lua filter creation with proper error handling.The implementation:
- Falls back to default path if not configured
- Validates file readability and non-empty content
- Uses inline code delivery as documented in design.md
Minor observation:
script(trimmed) is used for validation butscriptBytes(untrimmed) is used for InlineCode. This is fine since Lua ignores surrounding whitespace, but for consistency you could usescript:luaConfig := &luav3.Lua{ - InlineCode: string(scriptBytes), + InlineCode: script, }
| server_header_value = "WSO2 API Platform" | ||
|
|
||
| [gateway_controller.router.lua.request_transformation] | ||
| script_path = "./lua/request_transformation.lua" |
There was a problem hiding this comment.
Should this be a config? This is an internal file we maintain, right? Users are not required to change this.
There was a problem hiding this comment.
Yes — this is an internal script we ship. I ll remove it from the default config templates, but keep the configurability in code. That way anyone who custom‑builds the gateway can still ship a differently named/path Lua file and point the gateway to it via config(no need to touch the code for changing a file path in n the image) if needed. WDYT?
2 participants
Purpose
Summary by CodeRabbit
New Features
Documentation
Tests