Repository navigation
api: add packAsBytes to SecurityPolicy extAuth bodyToExtAuth - #10213
jamesglennan wants to merge 3 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10213 +/- ##
=======================================
Coverage 81.54% 81.54%
=======================================
Files 266 266
Lines 41568 41570 +2
=======================================
+ Hits 33896 33900 +4
+ Misses 7671 7669 -2
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
978ee81 to
71b2288
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71b22881b0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: James Glennan <james.glennan@cyberark.com>
Signed-off-by: James Glennan <james.glennan@cyberark.com>
Default maxRequestBytes to 32768 bytes (32KiB), matching the default per-connection buffer limit, so options such as packAsBytes can be set without also specifying a buffer size. This is a standalone commit so it can be reverted if maintainers prefer to keep maxRequestBytes required. Signed-off-by: James Glennan <james.glennan@cyberark.com>
71b2288 to
5c91d9d
Compare
|
@zirain Not sure what the protocol is to get this merged, do I need to get API change approval? |
This branch has not been deployed
3 participants
What this PR does / why we need it:
Adds
packAsBytestoSecurityPolicyextAuth.bodyToExtAuth. It maps to Envoy's ext_authzBufferSettings.pack_as_bytes.By default, Envoy sends the buffered request body to a gRPC external authorization service as a UTF-8 string in
AttributeContext.HttpRequest.body. Binary or non-UTF-8 bodies (protobuf, file uploads, compressed payloads) can't be inspected reliably that way. WithpackAsBytes: true, the body is sent as raw bytes inraw_bodyinstead.Envoy says
pack_as_bytesonly affectsgrpc_serviceand has no effect withhttp_service. A CEL rule onExtAuththerefore rejectspackAsBytes: trueunlessgrpcis set. An explicitfalseis still allowed withhttp.Making
maxRequestBytesoptional (separate commit, open for discussion)maxRequestBytesis currently required wheneverbodyToExtAuthis set. That made sense when it was the only field, but it means someone who only wants raw bytes still has to choose a buffer size. The last commit makes it optional and defaults it to 32768 bytes (32KiB), which matches Envoy Gateway's default per-connection listener buffer limit. The translator always fills in the default, so Envoy never receives a 0, which it would reject.It's a standalone commit with its own tests. If maintainers prefer to keep
maxRequestBytesrequired, reverting that commit leaves the rest of the PR working. I raised the question on #10193.Changes
BodyToExtAuth.PackAsBytes *bool, plus a CEL rule onExtAuththat only allowspackAsBytes: truewith gRPC.BufferSettings.PackAsBytes.packAsBytesset to true, false or unset, and withmaxRequestBytesset, unset, orbodyToExtAuth: {}.packAsBytestrue, unset, and with themaxRequestBytesdefault.packAsBytestrue and unset.Which issue(s) this PR fixes:
Fixes #10193
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes. (Proposed in SupportpackAsBytesin SecurityPolicyextAuth.bodyToExtAuth#10193 and awaiting triage. This PR stays a draft until the API is agreed.)make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.