Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ext_authz: fix body buffering feature on ExtAuthZ per-route filter #31437

Merged
merged 5 commits into from
Jan 3, 2024

Conversation

agrawroh
Copy link
Contributor

@agrawroh agrawroh commented Dec 19, 2023

Background

The with_request_body feature on per-route ExtAuthZ filter is not working as expected.

See: #31436

Changes

This PR fixes the body propagation when with_request_body is enabled on a per-route ExtAuthZ filter. Current implementation reads the flags for partial body and max bytes size only from the main HTTP filter and discards these configs on the per-route ExtAuthZ filter.

Commit Message: fix body buffering feature on ExtAuthZ per-route filter
Additional Description:add new private fields to hold the final state for the max bytes and whether partial body is allowed in the ExtAuthZ filter
Risk Level: Low
Testing: Added Tests
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #31437 was opened by agrawroh.

see: more, trace.

@agrawroh agrawroh force-pushed the extauthz-fix branch 3 times, most recently from e52b678 to d19f38d Compare December 19, 2023 06:00
@agrawroh agrawroh marked this pull request as ready for review December 19, 2023 17:05
@agrawroh agrawroh requested a review from ggreenway as a code owner December 19, 2023 17:05
Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for fixing this

Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

Defer to @ggreenway for review and merge

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Drive-by comment, please add a release note.

Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM. Please add a release note.

/wait

@agrawroh
Copy link
Contributor Author

agrawroh commented Jan 2, 2024

LGTM. Please add a release note.

/wait

Added, Thanks!

@ggreenway ggreenway merged commit 99ebb7c into envoyproxy:main Jan 3, 2024
53 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.

4 participants