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

router: fix query parameter present_match support #38000

Merged
merged 5 commits into from
Jan 21, 2025

Conversation

agrawroh
Copy link
Contributor

Description

The query parameter matcher implementation did not properly implement present_match functionality. While the matcher code exists, it isn't properly initializing the matcher, causing present_match checks to always fail.

This PR adds the missing functionality and also adds the test coverage to verify both present_match: true and
present_match: false behaviors work as expected.

Fixes: #37951


Commit Message: router: fix query parameter present_match support
Additional Description: Adds the missing support for present_match while matching query parameters.
Risk Level: Low
Testing: Added Unit Tests
Docs Changes: N/A
Release Notes: Added

@agrawroh agrawroh force-pushed the qery_params_presence branch from ede56ce to 6014488 Compare January 14, 2025 14:42
@agrawroh
Copy link
Contributor Author

@alyssawilk Could you please help review this small PR to fix query params present_match behavior?

Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #38000 was synchronize by agrawroh.

see: more, trace.

@agrawroh agrawroh requested a review from alyssawilk January 15, 2025 23:01
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

LGTM. @ggreenway given I'm still under the weather can you sanity check this LGTY?

@agrawroh
Copy link
Contributor Author

@alyssawilk Is it okay to merge this?

@alyssawilk
Copy link
Contributor

yeah sorry wanted a second maintainer look and looks like @wbpcode looked

@alyssawilk alyssawilk merged commit 2b41333 into envoyproxy:main Jan 21, 2025
25 checks passed
bazmurphy pushed a commit to bazmurphy/envoy that referenced this pull request Jan 29, 2025
## Description

The query parameter matcher implementation did not properly implement
`present_match` functionality. While the matcher code exists, it isn't
properly initializing the matcher, causing `present_match` checks to
always fail.

This PR adds the missing functionality and also adds the test coverage
to verify both `present_match: true` and
`present_match: false` behaviors work as expected.

**Fixes:** envoyproxy#37951

---

**Commit Message:** router: fix query parameter present_match support
**Additional Description:** Adds the missing support for `present_match`
while matching query parameters.
**Risk Level:** Low
**Testing:** Added Unit Tests
**Docs Changes:** N/A
**Release Notes:** Added

---------

Signed-off-by: Rohit Agrawal <[email protected]>
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