Improvements to decompression logic#2878
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds configurable per-direction decompressed-body limits for the policy engine, bounded buffered and streaming decompression, overflow handling, stream cleanup, and configuration and kernel tests. ChangesDecompression limits
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
gateway/gateway-runtime/policy-engine/internal/kernel/extproc_test.go (1)
101-106: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest the request and response limits independently.
Both constructor arguments currently use
testMaxDecompressedBytes, so this test cannot catch request/response assignment swaps or dropped values.Proposed test adjustment
- server := NewExternalProcessorServer(kernel, chainExecutor, tracingConfig, "test-service", testMaxDecompressedBytes, testMaxDecompressedBytes) + const requestLimit int64 = 1024 + const responseLimit int64 = 2048 + server := NewExternalProcessorServer(kernel, chainExecutor, tracingConfig, "test-service", requestLimit, responseLimit) require.NotNil(t, server) assert.Equal(t, kernel, server.kernel) assert.Equal(t, chainExecutor, server.executor) assert.NotNil(t, server.tracer) + assert.Equal(t, requestLimit, server.maxRequestDecompressedBytes) + assert.Equal(t, responseLimit, server.maxResponseDecompressedBytes)🤖 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/extproc_test.go` around lines 101 - 106, Update the NewExternalProcessorServer test setup to pass distinct request and response decompression-limit values, then assert the corresponding server fields retain each value independently. Keep the existing kernel, executor, and tracer assertions unchanged, and use the server’s request/response limit symbols to verify constructor assignment order.gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go (1)
103-107: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a regression test for deferred decompressor cleanup.
Exercise
Processwith active request/response stream decompressors and verify they are closed and cleared on EOF, receive errors, send errors, and processing errors.🤖 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/extproc.go` around lines 103 - 107, Add a regression test covering Process with active request and response stream decompressors, verifying deferred cleanup closes and clears both decompressors on EOF, receive errors, send errors, and processing errors. Exercise each outcome through the existing Process test setup and assert cleanup occurs after the call returns.gateway/gateway-runtime/policy-engine/internal/config/config.go (1)
171-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTracked TODO in a new public config surface.
Request/Responseare exported koanf-bound fields; renaming later is a breaking config change for operators. Worth resolving the naming before this ships rather than after[policy_engine.request]is documented in the template.Want me to open an issue to track the rename?
🤖 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/config/config.go` around lines 171 - 176, Resolve the naming TODO in the configuration struct before exposing the public koanf keys: replace the generic Request and Response BodyConfig fields with names that clearly represent their body-processing scope, and update all references and configuration documentation/templates consistently. Preserve separate request- and response-direction limits while avoiding a later breaking rename of the policy_engine configuration surface.gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go (1)
519-534: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBuffered paths still fall back to raw compressed bytes on non-limit decompression errors.
Streaming request/response now fail closed for malformed encodings (Lines 593-598, 919-929), but the buffered request/response paths still clear the encoding and hand raw compressed bytes to policies. That means policies inspect ciphertext-like bytes and body-driven decisions (auth, guardrails, content checks) silently degrade for a malformed
content-encoding— the same failure mode the streaming change deliberately closed. Consider aligning buffered handling with the streaming behavior in a follow-up.🤖 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/execution_context.go` around lines 519 - 534, Update the buffered request decompression handling around decompressBody so every non-limit decompression error fails closed like the streaming paths, rather than clearing ec.requestContentEncoding and forwarding raw compressed bytes to policies. Preserve the existing ErrDecompressedTooLarge response handling, and apply the same behavior to the corresponding buffered response path if it has the same fallback.
🤖 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-runtime/policy-engine/internal/config/config.go`:
- Around line 171-176: Resolve the naming TODO in the configuration struct
before exposing the public koanf keys: replace the generic Request and Response
BodyConfig fields with names that clearly represent their body-processing scope,
and update all references and configuration documentation/templates
consistently. Preserve separate request- and response-direction limits while
avoiding a later breaking rename of the policy_engine configuration surface.
In `@gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go`:
- Around line 519-534: Update the buffered request decompression handling around
decompressBody so every non-limit decompression error fails closed like the
streaming paths, rather than clearing ec.requestContentEncoding and forwarding
raw compressed bytes to policies. Preserve the existing ErrDecompressedTooLarge
response handling, and apply the same behavior to the corresponding buffered
response path if it has the same fallback.
In `@gateway/gateway-runtime/policy-engine/internal/kernel/extproc_test.go`:
- Around line 101-106: Update the NewExternalProcessorServer test setup to pass
distinct request and response decompression-limit values, then assert the
corresponding server fields retain each value independently. Keep the existing
kernel, executor, and tracer assertions unchanged, and use the server’s
request/response limit symbols to verify constructor assignment order.
In `@gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go`:
- Around line 103-107: Add a regression test covering Process with active
request and response stream decompressors, verifying deferred cleanup closes and
clears both decompressors on EOF, receive errors, send errors, and processing
errors. Exercise each outcome through the existing Process test setup and assert
cleanup occurs after the call returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7ef5f0a-1127-4feb-8c65-668b800db23e
📒 Files selected for processing (13)
gateway/configs/config-template.tomlgateway/gateway-runtime/policy-engine/cmd/policy-engine/main.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.gogateway/gateway-runtime/policy-engine/internal/config/config.gogateway/gateway-runtime/policy-engine/internal/config/config_test.gogateway/gateway-runtime/policy-engine/internal/kernel/decompression.gogateway/gateway-runtime/policy-engine/internal/kernel/decompression_test.gogateway/gateway-runtime/policy-engine/internal/kernel/execution_context.gogateway/gateway-runtime/policy-engine/internal/kernel/execution_context_test.gogateway/gateway-runtime/policy-engine/internal/kernel/extproc.gogateway/gateway-runtime/policy-engine/internal/kernel/extproc_bench_test.gogateway/gateway-runtime/policy-engine/internal/kernel/extproc_test.gogateway/gateway-runtime/policy-engine/internal/kernel/translator_test.go
bec0062 to
8b5496d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
gateway/gateway-runtime/policy-engine/internal/kernel/execution_context_test.go (1)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: extract a test-server helper.
The constructor call is repeated ~25 times with identical trailing limit args; a small helper (
newTestServer(kernel, exec)plus a limits variant) would keep the next signature change to one line.Also applies to: 101-101, 131-131, 642-642, 693-693, 808-808, 891-891
🤖 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/execution_context_test.go` at line 53, Extract a test helper for constructing the external processor server with the repeated tracing and decompression-limit arguments, such as a default helper accepting kernel and executor plus a limits variant for differing limits. Replace the repeated NewExternalProcessorServer calls in execution_context_test.go, including the referenced locations, while preserving existing argument values and behavior.gateway/gateway-runtime/policy-engine/internal/config/config.go (1)
171-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTracked TODO on
BodyConfignaming.The struct is already exported and keyed as
[policy_engine.request]/[policy_engine.response], so a later rename is a breaking config/API change. Worth resolving now or tracking explicitly.Want me to open an issue for the rename?
🤖 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/config/config.go` around lines 171 - 191, The TODO indicates BodyConfig has a misleading generic name despite containing direction-specific request/response settings. Rename BodyConfig and update the Request and Response field types, comments, and all references to use a clearer direction-agnostic configuration name; ensure the change is consistently applied across the config API and parsing paths.
🤖 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/execution_context_test.go`:
- Around line 82-91: Replace the non-blocking decoderDone checks in the
decompressor cleanup test with bounded waits, allowing each decoder goroutine
time to finish and signal completion while still failing on timeout. Preserve
the separate validation and failure messages for requestDecomp and
responseDecomp.
---
Nitpick comments:
In `@gateway/gateway-runtime/policy-engine/internal/config/config.go`:
- Around line 171-191: The TODO indicates BodyConfig has a misleading generic
name despite containing direction-specific request/response settings. Rename
BodyConfig and update the Request and Response field types, comments, and all
references to use a clearer direction-agnostic configuration name; ensure the
change is consistently applied across the config API and parsing paths.
In
`@gateway/gateway-runtime/policy-engine/internal/kernel/execution_context_test.go`:
- Line 53: Extract a test helper for constructing the external processor server
with the repeated tracing and decompression-limit arguments, such as a default
helper accepting kernel and executor plus a limits variant for differing limits.
Replace the repeated NewExternalProcessorServer calls in
execution_context_test.go, including the referenced locations, while preserving
existing argument values and behavior.
🪄 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 Plus
Run ID: 9b40221f-a01b-4160-b0bd-248fdd93c370
📒 Files selected for processing (14)
gateway/configs/config-template.tomlgateway/gateway-runtime/policy-engine/cmd/policy-engine/main.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.gogateway/gateway-runtime/policy-engine/internal/config/config.gogateway/gateway-runtime/policy-engine/internal/config/config_test.gogateway/gateway-runtime/policy-engine/internal/kernel/decompression.gogateway/gateway-runtime/policy-engine/internal/kernel/decompression_test.gogateway/gateway-runtime/policy-engine/internal/kernel/downstream_upstream_test.gogateway/gateway-runtime/policy-engine/internal/kernel/execution_context.gogateway/gateway-runtime/policy-engine/internal/kernel/execution_context_test.gogateway/gateway-runtime/policy-engine/internal/kernel/extproc.gogateway/gateway-runtime/policy-engine/internal/kernel/extproc_bench_test.gogateway/gateway-runtime/policy-engine/internal/kernel/extproc_test.gogateway/gateway-runtime/policy-engine/internal/kernel/translator_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- gateway/gateway-runtime/policy-engine/cmd/policy-engine/main.go
- gateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.go
- gateway/gateway-runtime/policy-engine/internal/kernel/extproc_bench_test.go
- gateway/gateway-runtime/policy-engine/internal/kernel/extproc_test.go
- gateway/gateway-runtime/policy-engine/internal/kernel/translator_test.go
- gateway/gateway-runtime/policy-engine/internal/kernel/execution_context.go
- gateway/gateway-runtime/policy-engine/internal/config/config_test.go
- gateway/gateway-runtime/policy-engine/internal/kernel/decompression.go
- gateway/gateway-runtime/policy-engine/internal/kernel/extproc.go
8b5496d to
94ff883
Compare
1 participant
Purpose
Improvements request response decompression
Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment